Use on demand sync strategy for the todo items #38
Conversation
…r the user switched to another project
|
Warning Rate limit exceeded@fulopkovacs has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds a required project_id foreign key to todo_items (DB + migrations) and propagates that change through API handlers, collection query behavior, request-tracking (href instead of pathname), and UI components to support project-scoped todo items and cache-driven on-demand syncing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as TodoBoards (client)
participant Collection as todoItemsCollection
participant Cache as Query Cache
participant API as local /api/todo-items
participant DB as Postgres (todo_items table)
User->>UI: open board / interact (projectId)
UI->>Collection: queryFn({ meta.loadSubsetOptions (projectId, orderBy, limit, offset) })
Collection->>API: GET /api/todo-items?projectId=X&... (construct from meta)
API->>API: parse request.href, extract query params
API->>DB: query todo_items WHERE project_id = X (and other filters)
DB-->>API: return filtered TodoItemRecord[]
API-->>Collection: respond with TodoItemRecord[]
Collection->>Cache: setQueriesData(...) update todo-items cache
Cache-->>UI: cache provides todoItems for render
Note over UI,Collection: On insert/update, Collection writes directly to Cache (setQueriesData) instead of local writeUpdate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Preview DeploymentStatus: ✅ Ready! Preview URL: Open Preview Commit: Built and deployed successfully |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/TodoBoards.tsx (1)
483-489: Sort comparator doesn't return 0 for equal elements, causing potentially unstable sort.The comparator returns
-1for both "Todo" and "In Progress", which doesn't properly establish ordering between them. This could lead to inconsistent board ordering across renders.🔎 Proposed fix
const sortedBoards = useMemo( () => - boards.sort((a) => - a.name === "Todo" ? -1 : a.name === "In Progress" ? -1 : 1, - ), + [...boards].sort((a, b) => { + const order = { "Todo": 0, "In Progress": 1, "Done": 2 }; + return (order[a.name as keyof typeof order] ?? 3) - (order[b.name as keyof typeof order] ?? 3); + }), [boards], );Note: Also using
[...boards]to avoid mutating the original array sincesort()mutates in place.
🧹 Nitpick comments (2)
src/local-api/helpers.ts (1)
3-3: Consider removing unused type.The
Pathnametype alias appears to be unused after the schema change frompathnametohref. Consider removing it if it's no longer needed elsewhere.src/local-api/api.todo-items.ts (1)
68-68: Update variable type annotation.The type annotation references the schema but could be simplified for clarity.
🔎 Suggested simplification
- let newTodoItemData: z.infer<typeof todoItemCreateData>; + let newTodoItemData: TodoItemCreateDataType;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
drizzle/migrations/0001_previous_chronomancer.sqldrizzle/migrations/meta/0001_snapshot.jsondrizzle/migrations/meta/_journal.jsonpackage.jsonpublic/sw.jssrc/client/pgliteHelpers.tssrc/collections/apiRequests.tssrc/collections/todoItems.tssrc/components/ApiRequestsPanel.tsxsrc/components/CreateOrEditTodoItems.tsxsrc/components/TodoBoards.tsxsrc/components/TodoBoardsLoading.tsxsrc/db/migrations.jsonsrc/db/schema.tssrc/db/seed.tssrc/integrations/tanstack-query/root-provider.tsxsrc/local-api/api.todo-items.tssrc/local-api/helpers.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
**/package.json: Always usepnpm installinstead ofnpm installfor installing dependencies
Always usepnpm add <package>instead ofnpm install <package>for adding packages
Always usepnpm add -D <package>instead ofnpm install --save-dev <package>for adding dev dependencies
Always usepnpm remove <package>instead ofnpm uninstall <package>for removing packages
Always usepnpm run <script>instead ofnpm run <script>for running scripts
Always usepnpm dlx <command>instead ofnpx <command>
Always usepnpm create <template>instead ofnpm create <template>
Files:
package.json
package.json
📄 CodeRabbit inference engine (AGENTS.md)
package.json: Usepnpm devfor starting the development server
Usepnpm buildfor production build
Usepnpm run testfor running tests
Usepnpm lintfor linting
Usepnpm typecheckfor type checking
Usepnpm formatfor code formatting
Usepnpm db.generateto generate database migrations
Usepnpm db.local.migrateto apply local migrations
Usepnpm db.prod.migrateto apply production migrations
Files:
package.json
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Always ensure no formatting/linting issues exist in code - use
pnpm check --fixto automatically fix issues
Files:
src/db/schema.tssrc/client/pgliteHelpers.tssrc/integrations/tanstack-query/root-provider.tsxsrc/collections/apiRequests.tssrc/local-api/helpers.tssrc/components/CreateOrEditTodoItems.tsxsrc/components/TodoBoards.tsxsrc/components/ApiRequestsPanel.tsxsrc/db/seed.tssrc/collections/todoItems.tssrc/components/TodoBoardsLoading.tsxsrc/local-api/api.todo-items.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer functions over arrow functions for React components
Files:
src/db/schema.tssrc/client/pgliteHelpers.tssrc/integrations/tanstack-query/root-provider.tsxsrc/collections/apiRequests.tssrc/local-api/helpers.tssrc/components/CreateOrEditTodoItems.tsxsrc/components/TodoBoards.tsxsrc/components/ApiRequestsPanel.tsxsrc/db/seed.tssrc/collections/todoItems.tssrc/components/TodoBoardsLoading.tsxsrc/local-api/api.todo-items.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
typeoverinterfacefor type definitions
Files:
src/db/schema.tssrc/client/pgliteHelpers.tssrc/integrations/tanstack-query/root-provider.tsxsrc/collections/apiRequests.tssrc/local-api/helpers.tssrc/components/CreateOrEditTodoItems.tsxsrc/components/TodoBoards.tsxsrc/components/ApiRequestsPanel.tsxsrc/db/seed.tssrc/collections/todoItems.tssrc/components/TodoBoardsLoading.tsxsrc/local-api/api.todo-items.ts
🧠 Learnings (5)
📚 Learning: 2025-12-29T13:20:08.388Z
Learnt from: CR
Repo: fulopkovacs/trytanstackdb.com PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-29T13:20:08.388Z
Learning: Applies to package.json : Use `pnpm db.generate` to generate database migrations
Applied to files:
package.json
📚 Learning: 2025-12-29T13:20:08.388Z
Learnt from: CR
Repo: fulopkovacs/trytanstackdb.com PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-29T13:20:08.388Z
Learning: Applies to package.json : Use `pnpm db.local.migrate` to apply local migrations
Applied to files:
package.json
📚 Learning: 2025-12-29T13:20:08.388Z
Learnt from: CR
Repo: fulopkovacs/trytanstackdb.com PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-29T13:20:08.388Z
Learning: Applies to package.json : Use `pnpm db.prod.migrate` to apply production migrations
Applied to files:
package.json
📚 Learning: 2025-12-24T15:25:07.106Z
Learnt from: fulopkovacs
Repo: fulopkovacs/trytanstackdb.com PR: 22
File: src/components/TodoBoards.tsx:63-81
Timestamp: 2025-12-24T15:25:07.106Z
Learning: In Tailwind CSS v4, prefer canonical classes using bg-linear-to-* (e.g., bg-linear-to-b, bg-linear-to-t, bg-linear-to-r) over the older bg-gradient-to-* syntax. This aligns with Tailwind LSP's suggestCanonicalClasses rule. Apply across TSX files (e.g., src/components/*.tsx); replace occurrences of bg-gradient-to-* with the corresponding bg-linear-to-* equivalents and verify gradients visually.
Applied to files:
src/integrations/tanstack-query/root-provider.tsxsrc/components/CreateOrEditTodoItems.tsxsrc/components/TodoBoards.tsxsrc/components/ApiRequestsPanel.tsxsrc/components/TodoBoardsLoading.tsx
📚 Learning: 2025-12-28T23:34:13.416Z
Learnt from: fulopkovacs
Repo: fulopkovacs/trytanstackdb.com PR: 32
File: src/components/HomeIntro.tsx:16-25
Timestamp: 2025-12-28T23:34:13.416Z
Learning: In Motion (motion/react), when configuring variant transitions, use delayChildren: stagger(x) to create per-child delays (e.g., delayChildren: stagger(0.2)). This is different from the older API that used a separate staggerChildren property. Apply this guidance to TSX files that declare motion variants (e.g., in src/components/**/*.tsx) and verify that stagger is imported from framer-motion. Ensure transitions reflect the intended stagger timing and that no conflicting delay values override the staggered effect.
Applied to files:
src/integrations/tanstack-query/root-provider.tsxsrc/components/CreateOrEditTodoItems.tsxsrc/components/TodoBoards.tsxsrc/components/ApiRequestsPanel.tsxsrc/components/TodoBoardsLoading.tsx
🧬 Code graph analysis (7)
src/client/pgliteHelpers.ts (3)
public/sw.js (2)
url(16-16)url(48-48)src/local-api/index.ts (1)
API(6-10)src/local-api/helpers.ts (1)
APIType(19-19)
src/integrations/tanstack-query/root-provider.tsx (1)
src/routes/__root.tsx (1)
MyRouterContext(22-24)
src/components/CreateOrEditTodoItems.tsx (2)
src/db/schema.ts (1)
TodoItemRecord(76-76)src/collections/todoItems.ts (1)
todoItemsCollection(55-207)
drizzle/migrations/meta/_journal.json (1)
src/db/migrate.ts (1)
recordMigration(29-37)
src/components/TodoBoards.tsx (2)
src/components/TodoBoardsLoading.tsx (1)
LoadingTasksOnBoardSkeleton(7-19)src/collections/todoItems.ts (1)
todoItemsCollection(55-207)
src/components/ApiRequestsPanel.tsx (1)
src/lib/utils.ts (1)
cn(4-6)
src/collections/todoItems.ts (1)
src/db/schema.ts (1)
TodoItemRecord(76-76)
🔇 Additional comments (28)
src/collections/apiRequests.ts (1)
13-13: LGTM!The addition of the optional
searchfield properly captures query parameters for API request tracking, aligning with the URL-based approach implemented inpgliteHelpers.ts.src/client/pgliteHelpers.ts (1)
74-89: LGTM!The shift from
pathnametohrefwith URL parsing correctly enables capturing both the path and query parameters. The handler lookup byurl.pathnamealigns with the API route structure, and storing bothpathnameandsearchprovides complete request tracking.src/components/CreateOrEditTodoItems.tsx (3)
23-24: LGTM!The addition of
projectIdto the required props correctly aligns with the database schema changes that add project-based organization to todo items.
55-55: LGTM!Including
projectIdin the insert operation is consistent with the updated schema and prop requirements.
45-47: No changes needed—synchronoustoArrayaccess is safe in this context.The collection data is guaranteed to be loaded because
CreateOrEditTodoItemsis rendered withinTodoBoards, which usesuseLiveQueryto load all todo items for the project (line 333). Users can only trigger the dialog afterTodoBoardshas already rendered with loaded data. The same synchronoustoArraypattern is already used elsewhere inTodoBoards(lines 379, 386, 426) with explicit comments confirming the data is "already-loaded collection data."package.json (1)
44-46: All TanStack dependency versions are valid and free from known vulnerabilities.Verification confirms that @tanstack/db@0.5.16, @tanstack/query-db-collection@1.0.12, and @tanstack/react-db@0.1.60 all exist on npm and are maintained by official TanStack developers with no reported security advisories.
src/components/TodoBoardsLoading.tsx (2)
7-19: LGTM!The new
LoadingTasksOnBoardSkeletoncomponent is well-structured and correctly exported for reuse inTodoBoards.tsx. The variable skeleton count based onboardIndexprovides visual variety during loading states.
21-44: LGTM!The refactor from
taskCounttoboardIndexprop cleanly delegates skeleton count determination toLoadingTasksOnBoardSkeleton, and the usage inTodoBoardsLoadingcorrectly passes board indices 0, 1, 2.src/local-api/helpers.ts (2)
21-25: LGTM!The schema update from
pathnametohrefwithz.url()validation is a good improvement. In Zod 4, standalone string format schemas likez.url()provide stronger validation. This aligns with the PR's shift to tracking full URLs for API requests.
50-68: LGTM!The
constructRequestForHandlerfunction correctly usesrequestData.hrefto construct theRequestobject, which accepts full URLs as the first argument.src/components/ApiRequestsPanel.tsx (2)
45-77: LGTM!The
JsonViewerenhancements are well-implemented:
- The
defaultOpenprop provides flexibility for different use cases- The conditional rendering (
typeof data === "string" ? data : JSON.stringify(...)) correctly handles raw strings vs objects
84-157: LGTM!Good UI improvements:
- The
overflow-hiddenandshrink-0classes prevent layout shifts during expansion/collapse- Displaying
request.searchalongsidepathnameprovides better visibility into API request details- The new Search
JsonViewerwithdefaultOpen={!!request.search}is a nice UX touchsrc/components/TodoBoards.tsx (3)
33-36: LGTM!Good refactoring to pass
todoItems,projectId,showTodoItemsLoading, andboardIndexas props. This enables better control over loading states and reduces the need for per-board queries.Also applies to: 173-185
430-433: Good defensive check.The null check for
overTodoItemwith early return prevents potential runtime errors when the drop target isn't found.
252-299: LGTM!The conditional rendering between
LoadingTasksOnBoardSkeletonand the sortable list is clean, and the integration withScrollShadow,DropIndicator, and virtualized rendering is well-structured.src/collections/todoItems.ts (1)
58-117: LGTM on the query function implementation.The dynamic query parameter building from
loadSubsetOptionsis well-structured, handling filters, sorting, limit, and offset appropriately.drizzle/migrations/meta/_journal.json (1)
11-18: LGTM!The new migration journal entry correctly follows the established format with proper index sequencing and version consistency.
drizzle/migrations/0001_previous_chronomancer.sql (1)
1-3: No action needed. This migration is safe.The
NOT NULLcolumn addition without aDEFAULTvalue works here becausetodo_itemsis empty when this migration runs. Migration 0000 creates the table and migration 0001 adds the column before any data is inserted. The subsequent seed data includesproject_idvalues for all todo items, so no constraint violation occurs.Likely an incorrect or invalid review comment.
public/sw.js (1)
82-82: LGTM! Improved URL handling.Sending the full
hrefinstead of justpathnamecorrectly preserves query parameters and other URL components, which aligns with the updated request handling in the main thread.src/db/schema.ts (2)
64-66: LGTM! Well-structured foreign key.The
projectIdfield is correctly defined with:
- Appropriate foreign key reference to
projectsTable.idCASCADEdelete behavior to maintain referential integritynotNullconstraint matching the migration
72-72: LGTM! Good indexing strategy.Adding an index on
projectIdwill optimize queries filtering by project, which aligns with the new GET handler's query parameter support.src/db/seed.ts (2)
19-22: LGTM! Correct type refinement.The
TodoItemBasetype correctly omitsprojectIdsince it's added during record construction at line 218, following the same pattern asboardId.
213-221: LGTM! Consistent record construction.The
projectIdis correctly added to each todo item record during the mapping phase, maintaining consistency with the updated schema.src/local-api/api.todo-items.ts (2)
1-1: LGTM! Import added for array filtering.The
inArrayimport is correctly added to support the newboardId_inquery parameter functionality.
10-10: All internal client code already includesprojectId— validation enforced at API level.While making
projectIdrequired intodoItemCreateDatais a potential breaking change for external API consumers, all internal code paths in this codebase already properly passprojectId. TheCreateOrEditTodoItemscomponent requires bothboardIdandprojectId, and the API schema validation will catch any missing values at runtime. No action needed within this codebase.src/db/migrations.json (1)
17-21: No changes needed—migration strategy is appropriate for this development project.Adding a
NOT NULLcolumn without a default value is acceptable here because the seed script populatestodo_itemswith theprojectIdalready set (it's included in the mock data). The seed function is idempotent and checks for prior execution, so a fresh database setup handles this correctly. The development workflow (as documented in DEVELOPMENT.md) expects schema resets when schemas change, which aligns with this migration pattern for a pglite-based development database.drizzle/migrations/meta/0001_snapshot.json (2)
246-258: Confirm cascade delete behavior is intentional.The
todo_items_project_id_projects_id_fkforeign key usesON DELETE CASCADE, which means deleting a project will automatically delete all associated todo items. Combined with the cascade onboards.project_id, this creates a deletion chain: deleting a project removes all its boards and todo items.Verify this cascade behavior aligns with your data retention and user experience requirements.
194-231: LGTM: Index configuration supports project-based queries.The indexes are well-designed for the new project-based organization:
todo_items_project_id_idxenables efficient filtering of items by projecttodo_items_board_id_position_idxsupports ordered retrieval within boardsThis aligns with the PR objective of using an on-demand sync strategy for todo items.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/collections/todoItems.ts (2)
71-73: Remove debug logging before merging.This debug
console.infowas flagged in a previous review and should be removed for production.🔎 Proposed fix
// Parse the expressions into simple format const parsed = parseLoadSubsetOptions({ where, orderBy, limit }); - console.info({ - parsed, - }); - // Build query parameters from parsed filtersBased on past review comments, this should be removed.
161-186: DuplicatesetQueriesDatacalls with debug logging.The two
setQueriesDatacalls (lines 163-171 and 173-186) perform identical operations on the same query key. The second call includes aconsole.logthat appears to be debug code. This was flagged in a previous review and should be removed.🔎 Proposed fix
// Update the TanStack Query cache so switching projects shows correct data const queryClient = TanstackQuery.getContext().queryClient; queryClient.setQueriesData<TodoItemRecord[]>( { queryKey: todoItemsQueryKey }, (oldData) => { if (!oldData) return oldData; return oldData.map((item) => item.id === original.id ? { ...item, ...changes } : item, ); }, ); - - queryClient.setQueriesData<TodoItemRecord[]>( - { queryKey: todoItemsQueryKey }, - (oldData) => { - console.log( - "Updating query cache, oldData:", - oldData?.length, - "items", - ); - if (!oldData) return oldData; - return oldData.map((item) => - item.id === original.id ? { ...item, ...changes } : item, - ); - }, - );Based on past review comments, remove the duplicate call and its debug logging.
🧹 Nitpick comments (2)
src/collections/todoItems.ts (2)
111-115: Add error handling for fetch call.The fetch request lacks error handling. Network failures or non-OK responses will silently propagate, potentially causing the query to fail without clear error messages.
🔎 Proposed fix with error handling
const res = await fetch(`/api/todo-items?${params}`, { method: "GET" }); + if (!res.ok) { + throw new Error(`Failed to fetch todo items: ${res.status} ${res.statusText}`); + } + const todoItems: TodoItemRecord[] = await res.json(); return todoItems;
187-197: Incomplete error handling for update failures.When
updateTodoItemfails, the client cache may become out of sync with server state since no refetch or invalidation occurs. The TODO comment indicates this is known but unresolved.Consider implementing the commented-out invalidation logic to ensure cache consistency after failures:
🔎 Potential implementation
} catch (_) { toast.error(`Failed to update todo item "${original.title}"`); - // TODO: handle this one later properly - // with queryClient.invalidateQueries(todoItemsQueryKey); - // // Do not sync if the collection is already refetching - // if (todoItemsCollection.utils.isRefetching === false) { - // // Sync back the server's data - // todoItemsCollection.utils.refetch(); - // } + const queryClient = TanstackQuery.getContext().queryClient; + // Invalidate to refetch server state on next access + await queryClient.invalidateQueries({ queryKey: todoItemsQueryKey }); + return { refetch: true }; } - // Do not sync back the server's data by default - return { - refetch: false, - }; + return { refetch: false };Would you like me to open an issue to track implementing proper sync-back logic for update failures?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/collections/todoItems.ts
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Always ensure no formatting/linting issues exist in code - use
pnpm check --fixto automatically fix issues
Files:
src/collections/todoItems.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer functions over arrow functions for React components
Files:
src/collections/todoItems.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
typeoverinterfacefor type definitions
Files:
src/collections/todoItems.ts
🧬 Code graph analysis (1)
src/collections/todoItems.ts (1)
src/db/schema.ts (1)
TodoItemRecord(76-76)
🔇 Additional comments (2)
src/collections/todoItems.ts (2)
1-5: Past import issue has been resolved.The types are now correctly accessed via the
IRnamespace imported from the main@tanstack/dbentry point, rather than from internal paths. This properly uses the public API.
118-118: LGTM! Sync mode aligns with PR objectives.The addition of
syncMode: "on-demand"correctly implements the PR's goal of using on-demand sync strategy for todo items.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.