-
Notifications
You must be signed in to change notification settings - Fork 53
feat: all the scheduled objects of that day #1002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 6250638 in 3 minutes and 1 seconds. Click for details.
- Reviewed
392lines of code in6files - Skipped
0files when reviewing. - Skipped posting
18draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/src/components/blocks/list/agenda-list-items.tsx:63
- Draft comment:
This is duplicating the existing list item render logic. Consider extending the existing implementation to support the overdue indicator feature instead. - SortableItem render function (list-items.tsx) - Reason this comment was not posted:
Comment looked like it was already resolved.
2. apps/web/eslint.config.mjs:15
- Draft comment:
Consider a comment about the rationale for disabling '@next/next/no-duplicate-head' in the repo context. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. apps/web/next.config.ts:9
- Draft comment:
Disabling type checking and ESLint errors during production build risks code quality. Ensure this is a short-term measure. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code already has clear warning comments. The suggestion to add "short-term measure only" is speculative - we don't know if this is meant to be temporary or permanent. The PR author has explicitly added warning comments showing they understand the tradeoffs. This seems like a configuration choice that was made deliberately. The comment raises valid concerns about code quality. Disabling these checks could lead to issues making it to production. While the concerns are valid, the code already acknowledges these risks with explicit warnings. The comment doesn't add new actionable information. Delete the comment as it's merely restating warnings that are already present in the code and making assumptions about the temporary nature of the change.
4. apps/web/src/actions/calendar.ts:17
- Draft comment:
Good fallback with 'response.events || []' to avoid undefined returns. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. apps/web/src/actions/objects.ts:11
- Draft comment:
Return object with todayObjects and overdueObjects with fallbacks is clear; ensure backend always provides expected keys. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. apps/web/src/components/blocks/list/agenda-list-items.tsx:62
- Draft comment:
Extraction of renderItem improves code reuse. Ensure that any callbacks (e.g., updateObject) are not redundantly defined on each render if performance becomes an issue. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. apps/web/src/components/blocks/list/agenda-list-items.tsx:148
- Draft comment:
Conditional rendering for overdue and today sections is clear. Consider memoizing sorted arrays if performance becomes a concern with larger datasets. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
8. apps/web/src/contexts/block-context.tsx:37
- Draft comment:
Extraction of items and overdueItems based on arrayType is well handled. Verify that default values for queries remain consistent with hook return types. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
9. apps/web/src/contexts/block-context.tsx:115
- Draft comment:
The use of 'isHandlingCalendarDrop' with setTimeout is acceptable, but be cautious of potential race conditions if drop events occur rapidly. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
10. apps/web/eslint.config.mjs:15
- Draft comment:
Ensure that disabling '@next/next/no-duplicate-head' is justified; consider linking to a bug report or detailing the underlying issue. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to justify a change by linking to a bug report or detailing the issue. This is similar to asking for confirmation or explanation, which is against the rules. The comment does not provide a specific code suggestion or point out a specific issue with the code itself.
11. apps/web/next.config.ts:8
- Draft comment:
Disabling type-checking and ESLint during production builds can hide critical errors. Consider using environment-specific configurations. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The author has clearly made an intentional choice to disable these checks, with explicit warning comments. The suggestion to use environment-specific configs is vague and doesn't provide concrete value. The PR rules say not to make speculative comments or ask for confirmations. This feels like a "are you sure?" type comment. The comment does point out a legitimate concern about production build safety. Maybe the author hasn't fully considered the implications. The author's warning comments show they have considered the implications. The comment doesn't provide specific, actionable changes beyond what the author has already acknowledged. Delete the comment. It's essentially asking "are you sure?" when the code already shows the author is aware of the tradeoffs.
12. apps/web/src/actions/objects.ts:11
- Draft comment:
Good defensive coding: returning default empty arrays for 'todayObjects' and 'overdueObjects' prevents undefined errors. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
13. apps/web/src/components/blocks/list/agenda-list-items.tsx:129
- Draft comment:
Security risk: add rel='noopener noreferrer' to the anchor tag when using target='_blank' to prevent reverse tabnabbing. - Reason this comment was not posted:
Comment was on unchanged code.
14. apps/web/src/components/blocks/list/agenda-list-items.tsx:33
- Draft comment:
Consider memoizing sorted arrays (sortedTodayItems and sortedOverdueItems) with useMemo to improve performance for large lists. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
15. apps/web/src/components/blocks/list/agenda-list-items.tsx:45
- Draft comment:
Consider memoizing the renderIcon function (or moving it outside the component) to prevent re-creation on each render if performance becomes a concern. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
16. apps/web/src/contexts/block-context.tsx:66
- Draft comment:
Consider memoizing today's date (moment().format(...)) to avoid recalculating it on every render. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
17. apps/web/src/contexts/block-context.tsx:141
- Draft comment:
Instead of invalidating both 'inbox-objects' and 'today-objects', consider invalidating only the relevant query key based on arrayType to reduce unnecessary network requests. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
18. apps/web/src/contexts/block-context.tsx:196
- Draft comment:
Typographical error: The error message in the useBlock hook currently reads "useBlock must be used within an BlockProvider". Consider changing "an BlockProvider" to "a BlockProvider" for correct grammar. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_Gya6kXoVzspoPq2M
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What did you ship?
Fixes:
Checklist:
OR:
Important
This PR separates overdue and today's scheduled objects, updates ESLint and Next.js configurations, and improves error handling for calendar events.
getTodayObjectsinobjects.tsnow returns bothtodayObjectsandoverdueObjects.AgendaListItemsinagenda-list-items.tsxdisplays overdue and today's items separately, with overdue items marked with a red star.BlockProviderinblock-context.tsxhandlesoverdueItemsandtodayObjectsseparately based onarrayType.@next/next/no-duplicate-headrule ineslint.config.mjs.next.config.ts.reactStrictModeinnext.config.tsfor development.getEventsByDateincalendar.tsreturns an empty array ifresponse.eventsis undefined to prevent blocking the app.This description was created by
for 6250638. You can customize this summary. It will automatically update as commits are pushed.