Use fractional indexing for the positions of the todo items - #20
Conversation
|
Caution Review failedThe pull request is closed. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
drizzle/migrations/0000_absurd_polaris.sql (1)
14-14: Critical: Remove obsoleteitem_positions_in_the_projectfield from projects table.The migration SQL retains the
item_positions_in_the_projectfield in the projects table, butsrc/db/schema.tsremoves it. This creates a critical schema/migration mismatch that will cause runtime errors when the ORM tries to query or insert projects.Since the PR transitions to per-item position tracking using the new
positionfield intodo_items, this JSON field is no longer needed.🔎 Proposed fix
CREATE TABLE "projects" ( "id" text PRIMARY KEY NOT NULL, "name" text NOT NULL, "description" text NOT NULL, "created_at" date NOT NULL, - "item_positions_in_the_project" json DEFAULT '{}'::json NOT NULL, CONSTRAINT "projects_name_unique" UNIQUE("name") );src/db/schema.ts (1)
1-1: Fix formatting issues.The pipeline indicates formatting issues in this file.
#!/bin/bash # Run formatter on schema.ts pnpm format --write src/db/schema.tssrc/db/seed.ts (1)
297-314: Critical: Remove obsoleteitemPositionsInTheProjectconstruction.The code constructs
itemPositionsInTheProjectand includes it in the returned projects, but this field was removed fromProjectRecordinsrc/db/schema.ts. This will cause a TypeScript compilation error and runtime issues when inserting projects.Since position tracking now occurs via the per-item
positionfield, this entire code block should be removed.🔎 Proposed fix
- const projectsWithPositions: ProjectRecord[] = mockProjects.map((project) => { - const projectBoards = mockBoards.filter( - (board) => board.projectId === project.id, - ); - const itemPositionsInTheProject: Record<string, string[]> = {}; - - for (const board of projectBoards) { - const itemsInBoard = mockTodoItems - .filter((item) => item.boardId === board.id) - .map((item) => item.id); - itemPositionsInTheProject[board.id] = itemsInBoard; - } - - return { - ...project, - itemPositionsInTheProject, - }; - }); + const projectsWithPositions: ProjectRecord[] = mockProjects.map( + ({ todoItemsBaseArr, ...project }) => project, + ); return { mockUsers, mockBoards, mockTodoItems, mockProjects: projectsWithPositions, };src/local-api/api.todo-items.ts (1)
7-35: Remove redundant Omit type on line 35.Line 35 defines
newTodoItemDataasOmit<z.infer<typeof todoItemCreateData>, "projectId">, butprojectIdis not present in thetodoItemCreateDataschema (lines 7-14). The Omit is redundant and misleading.🔎 Proposed fix
- let newTodoItemData: Omit<z.infer<typeof todoItemCreateData>, "projectId">; + let newTodoItemData: z.infer<typeof todoItemCreateData>;
🧹 Nitpick comments (2)
src/db/seed.ts (1)
28-28: Address TODO: Differentiate between projects and boards.The TODO indicates a need to clarify the distinction between projects and boards in the seeding logic. This may involve refactoring the data model or generator structure.
Would you like me to help implement this clarification, or should I open a tracking issue for future work?
src/components/TodoBoards.tsx (1)
302-335: Consider renaming maxTodoItemsBoard for clarity.The query name
maxTodoItemsBoardis misleading—it fetches all todo items for the project, not just the maximum or a subset. Consider renaming to something likeprojectTodoItemsWithBoardorallTodoItemsInProjectfor better clarity.The query implementation itself is correct, with proper joins, filtering, and lexical sorting for fractional indexing.
🔎 Suggested naming improvement
- const { data: maxTodoItemsBoard } = useLiveQuery( + const { data: projectTodoItems } = useLiveQuery( (q) => q .from({Then update all references to
maxTodoItemsBoardthroughout the file to use the new name.
📜 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 (13)
TODO.md(1 hunks)drizzle/migrations/0000_absurd_polaris.sql(1 hunks)drizzle/migrations/meta/0000_snapshot.json(2 hunks)drizzle/migrations/meta/_journal.json(1 hunks)package.json(1 hunks)src/collections/todoItems.ts(1 hunks)src/components/CreateOrEditTodoItems.tsx(2 hunks)src/components/TodoBoards.tsx(11 hunks)src/db/migrations.json(1 hunks)src/db/schema.ts(1 hunks)src/db/seed.ts(4 hunks)src/local-api/api.todo-items.ts(5 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T21:20:23.301Z
Learnt from: CR
Repo: fulopkovacs/trytanstackdb.com PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T21:20:23.301Z
Learning: Use `pnpm db.generate` to generate database migrations
Applied to files:
src/db/seed.ts
🧬 Code graph analysis (2)
src/components/CreateOrEditTodoItems.tsx (1)
src/collections/todoItems.ts (1)
todoItemsCollection(60-95)
src/components/TodoBoards.tsx (4)
src/components/CreateOrEditTodoItems.tsx (1)
CreateOrEditTodoItems(19-143)src/collections/todoItems.ts (1)
todoItemsCollection(60-95)src/collections/boards.ts (1)
boardCollection(17-24)src/collections/projects.ts (1)
projectsCollection(16-41)
🪛 GitHub Actions: CI/CD
src/db/migrations.json
[error] 1-1: Formatter would have printed content; formatting issues detected in migrations.json.
src/db/schema.ts
[error] 1-1: Formatter would have printed content; formatting issues detected in schema.ts.
🪛 LanguageTool
TODO.md
[style] ~11-~11: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...? - [ ] (feature) query-driven sync - maybe the todos -> per board - [ ] `robots.tx...
(REP_MAYBE)
[style] ~15-~15: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ly) - [ ] (feature) paced mutations - maybe editing the todos - [ ] Make the root e...
(REP_MAYBE)
🔇 Additional comments (16)
drizzle/migrations/0000_absurd_polaris.sql (1)
30-31: LGTM: Position field correctly added.The new
positionfield is properly defined as NOT NULL and placed logically after the priority field, supporting the fractional indexing implementation.drizzle/migrations/meta/_journal.json (1)
8-9: LGTM: Migration journal correctly updated.The timestamp and tag reflect the new migration version for the position field addition.
src/collections/todoItems.ts (1)
69-71: LGTM: Insert payload correctly updated.The change to pass
newTodoItemdirectly aligns with the removal ofprojectIdfrom the creation flow and the new position-based ordering system.src/db/schema.ts (2)
62-62: LGTM: Position field correctly added to schema.The new
positionfield withnotNull()constraint properly supports the fractional indexing implementation. The schema change aligns with the migration's intent to add per-item position tracking.
32-37: Verify removal ofitemPositionsInTheProjectfield is reflected in migrations.The schema correctly removes
itemPositionsInTheProjectfrom the projects table as part of the position-based ordering refactor. However, the migration SQL files still contain this field, creating a critical mismatch. Ensure the migration is updated to match this schema.src/db/seed.ts (2)
29-45: LGTM: Fractional index generator correctly implemented.The per-board position generator using
generateKeyBetweenis well-designed. The lazy initialization pattern via theboardGeneratorsmap ensures each board maintains its own independent position sequence, which is essential for correct fractional indexing behavior.
54-168: LGTM: Seed data correctly uses position-based ordering.All todo items now receive deterministic positions via
getNextPosition, properly segregated by board. The use of board-specific generators (e.g., "board-1-todo", "board-2-in-progress", "large-project-board-Done") ensures correct ordering within each board's context.package.json (1)
54-54: No action required.The
fractional-indexingpackage version ^3.2.0 exists and is the latest stable release with no known security vulnerabilities.src/components/CreateOrEditTodoItems.tsx (3)
1-25: LGTM on component refactoring.The removal of
projectIdfrom props and the addition of fractional-indexing imports align well with the position-based ordering approach. The component signature is cleaner and more focused.
44-49: Correction: JavaScript < operator IS lexicographical - no issue here.After verification, JavaScript's comparison operators (<, >) perform lexicographical comparison based on Unicode code points, not locale-based comparison. The < and > operators compare strings using Unicode encodings, which is exactly what fractional indexing requires.
The sorting on line 46 using
a.position < b.positionis correct and consistent with thestringSort: "lexical"approach used inTodoBoards.tsx. Both methods produce the same lexicographical (byte-order) sort needed for fractional indexing.Please disregard my earlier concern about sorting inconsistency.
44-49: No changes needed. The code correctly uses native JavaScript string comparison (<operator) for fractional indexing, which performs lexicographical (Unicode code point) ordering. This is the recommended approach per the fractional-indexing library documentation and is appropriate for position sorting. UsinglocaleCompare()instead would actually break fractional indexing due to case-insensitive collation.Likely an incorrect or invalid review comment.
src/components/TodoBoards.tsx (4)
42-62: LGTM on findPrevItem helper.The helper function correctly identifies the previous item in the same board. The generic type constraints ensure type safety, and the logic properly handles edge cases (first item, different boards).
158-173: LGTM on Board component query with lexical sorting.The query correctly uses
stringSort: "lexical"for fractional indexing. The comment explaining the difference between lexical and locale-based sorting is excellent and helps prevent future bugs.
177-207: LGTM on drop indicator calculation.The drop indicator logic correctly handles all drag-and-drop scenarios and properly avoids showing indicators when the item wouldn't actually move. The useMemo dependencies are complete.
341-415: LGTM on drag-and-drop implementation.The
handleDragEndlogic correctly handles all three drag-and-drop scenarios:
- Dropping on an empty board or at the end
- Reordering within the same board
- Moving to a different board
The fractional indexing calculations are correct:
generateKeyBetween(lastPosition, null)for appendinggenerateKeyBetween(null, null)for empty boardsgenerateKeyBetween(prev?.position ?? null, overTodoItem.position)for inserting between itemsThe use of
findPrevItemwith the sortedmaxTodoItemsBoarddata ensures correct predecessor identification.drizzle/migrations/meta/0000_snapshot.json (1)
188-193: No changes needed. The migration correctly defines thepositioncolumn as part of the initial table creation withnotNull: trueand no default value, which is appropriate for CREATE TABLE statements since the table contains no existing data.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/db/schema.ts (1)
56-56: Add a compound index on (boardId, position) for query performance.Position-based ordering is used in queries that first filter by
boardIdthen sort byposition(e.g., TodoBoards.tsx lines 162, 323). A compound index on these columns in this order will allow the database to skip explicit sort operations for these common queries.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
drizzle/migrations/0000_volatile_doctor_strange.sql(1 hunks)drizzle/migrations/meta/0000_snapshot.json(2 hunks)drizzle/migrations/meta/_journal.json(1 hunks)src/db/migrations.json(1 hunks)src/db/schema.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/db/migrations.json
- drizzle/migrations/meta/_journal.json
🔇 Additional comments (4)
src/db/schema.ts (1)
1-1: LGTM: Import cleanup aligns with schema changes.The removal of
jsonfrom imports is appropriate sinceitemPositionsInTheProjectwas removed from the projects table.drizzle/migrations/meta/0000_snapshot.json (2)
181-186: Verify consistency with NOT NULL position column.The position column is defined as
notNull: truewithout a default value, consistent with the migration file. This requires that all position values are provided at insert time.Ensure that:
- All code paths creating todo items generate position values using fractional-indexing
- No legacy code attempts to create todo items without position
#!/bin/bash # Find all locations where todo items are inserted ast-grep --pattern 'db.insert($$$).values({ $$$ })' # Search for TodoItem creation or insertion patterns rg -n "insert.*todoItem|create.*TodoItem|new.*TodoItem" -g '*.ts' -g '*.tsx' -C5
2-2: No critical issue: This is a greenfield project without existing deployments.The migration ID change is not problematic here. Your project is in ALPHA stage with a local PGlite database (development-only), no deployment configurations, and this is the first (0000) migration ever created. No database instance has previously run the old migration ID
7f0dad39-3f27-4237-a783-43c6b501d2fc, and no migration tracking table contains records from it.As noted in the original review: "If no production/staging databases exist yet, this is acceptable" — which is exactly the case here.
Likely an incorrect or invalid review comment.
drizzle/migrations/0000_volatile_doctor_strange.sql (1)
29-30: The migration concern is not applicable—this is an initial table creation.The
positioncolumn is part of aCREATE TABLEstatement for a new schema, not anALTER TABLEoperation on existing data. Since migration0000creates thetodo_itemstable from scratch, no existing rows need a default value. TheNOT NULLconstraint is appropriate for initial schema creation.Likely an incorrect or invalid review comment.
…me up when the network latency is high
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/local-api/api.batch.todo-items.ts (1)
39-46: Consider excludingidfrom the update operation.Including
idin the.set()call is unnecessary since it's already used in the.where()clause. While Drizzle likely ignores it, explicitly excluding it improves clarity.🔎 Proposed refactor
await db.transaction(async (tx) => { for (const todoItemData of updatedData) { + const { id, ...updateFields } = todoItemData; await tx .update(todoItemsTable) - .set(todoItemData) + .set(updateFields) - .where(eq(todoItemsTable.id, todoItemData.id)); + .where(eq(todoItemsTable.id, id)); } });src/components/TodoBoards.tsx (1)
292-334: Consider reducing the debounce wait time.A 3-second debounce might feel sluggish for drag-and-drop interactions. While optimistic updates provide immediate visual feedback, users may expect changes to persist more quickly. Consider reducing to 1000-1500ms for better UX, unless the 3-second delay is intentional for rate-limiting or batching purposes.
🔎 Suggested adjustment
- strategy: debounceStrategy({ wait: 3000 }), + strategy: debounceStrategy({ wait: 1500 }),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/collections/todoItems.ts(2 hunks)src/components/TodoBoards.tsx(12 hunks)src/local-api/api.batch.todo-items.ts(1 hunks)src/local-api/api.todo-items.ts(4 hunks)src/local-api/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/local-api/api.batch.todo-items.ts (4)
src/local-api/api.todo-items.ts (1)
todoItemUpdateData(18-25)src/db/index.ts (1)
db(10-12)src/db/schema.ts (1)
todoItemsTable(47-57)src/local-api/helpers.ts (2)
json(29-43)APIRouteHandler(15-17)
src/components/TodoBoards.tsx (4)
src/components/CreateOrEditTodoItems.tsx (1)
CreateOrEditTodoItems(19-143)src/db/schema.ts (1)
TodoItemRecord(59-59)src/collections/todoItems.ts (2)
todoItemsCollection(80-115)batchUpdateTodoItem(38-56)src/collections/boards.ts (1)
boardCollection(17-24)
src/collections/todoItems.ts (1)
src/db/schema.ts (1)
TodoItemRecord(59-59)
🔇 Additional comments (9)
src/local-api/index.ts (1)
1-1: LGTM! Clean integration of the batch API route.The new batch endpoint is properly imported and exposed, enabling bulk updates for todo items.
Also applies to: 11-11
src/collections/todoItems.ts (2)
38-56: LGTM! Batch update function is well-structured.The new
batchUpdateTodoItemfunction properly handles batch updates with appropriate error handling.
90-90: LGTM! Simplified payload aligns with schema changes.Passing
newTodoItemdirectly is correct now that the position field is part of the todo item record.src/local-api/api.todo-items.ts (2)
13-13: LGTM! Position fields correctly integrated.The schema now properly includes
positionfor both creation and updates, aligning with the fractional indexing approach.Also applies to: 24-24
47-48: LGTM! Simplified creation logic.Removing the
projectIdextraction and directly usingtodoItemDatais correct given the schema changes.src/components/TodoBoards.tsx (4)
50-70: LGTM! Well-designed helper with clear constraints.The
findPrevItemfunction correctly finds the previous item within the same board, with good generic type constraints and clear documentation.
166-181: LGTM! Correct lexical sorting for fractional indexing.The query properly uses
stringSort: "lexical"for fractional index ordering, with an excellent comment explaining why this is necessary.
354-387: LGTM! Comprehensive query with correct ordering.The joined query properly fetches all necessary fields and applies lexical sorting for fractional indexes. The comment clearly explains the sorting requirements.
393-470: LGTM! Robust drag-and-drop handling.The
handleDragEndimplementation correctly handles three scenarios:
- Dropping on an empty column or at the end
- Reordering within the same column
- Moving to a different column
Position generation using
generateKeyBetweenwith appropriate boundaries is properly implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
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)
418-420: Critical: Fix method invocation syntax.Line 418 attempts to access
.findontodoItemsCollection.toArray, buttoArrayappears to be a method based on the related code snippet showingtodoItemsCollection.toArray()at line 331. This will cause a runtime TypeError.🔎 Proposed fix
- const overTodoItem = todoItemsCollection.toArray.find( + const overTodoItem = todoItemsCollection.toArray().find( (item) => item.id === over.id, );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/TodoBoards.tsx(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/TodoBoards.tsx (2)
src/components/CreateOrEditTodoItems.tsx (1)
CreateOrEditTodoItems(19-143)src/collections/todoItems.ts (2)
todoItemsCollection(80-115)batchUpdateTodoItem(38-56)
🔇 Additional comments (11)
src/components/TodoBoards.tsx (11)
15-30: LGTM! Imports correctly support fractional indexing.The new imports for
usePacedMutations,debounceStrategy, andgenerateKeyBetweenare appropriately added to support the fractional indexing feature with debounced batch updates.
50-70: LGTM! Helper function correctly identifies the previous item.The
findPrevItemfunction properly handles edge cases (first item, different board) and uses appropriate type constraints to ensure type safety.
166-181: LGTM! Correct use of lexical sorting for fractional indexes.The query properly uses
stringSort: "lexical"which is essential for maintaining correct ordering with fractional indexing. The explanatory comment is helpful.
185-215: LGTM! Drop indicator logic is correct.The
dropIndexcalculation properly handles both dropping over specific items and dropping on the board itself (empty column or end position).
238-269: LGTM! Rendering logic correctly uses live todoItems.The Board component now consistently renders based on the live-queried
todoItemsarray, properly integrating with the fractional indexing system.
292-334: Verify the 3-second debounce timing.The implementation correctly applies optimistic updates immediately and batches backend persistence. However, a 3-second debounce is relatively long—users performing rapid successive drags might experience a delay before changes are persisted. Additionally, there's no explicit error handling in
mutationFn; ifbatchUpdateTodoItemfails, the optimistic updates may remain inconsistent until the next refetch.Consider:
- Testing the debounce timing under typical usage patterns
- Adding error handling that reverts optimistic updates on failure:
mutationFn: async ({ transaction }) => { const mutations = transaction.mutations; const updates = mutations.reduce(/* ... */); try { await batchUpdateTodoItem({ data: updates }); await todoItemsCollection.utils.refetch(); } catch (error) { console.error("Failed to persist position updates:", error); // Refetch will restore backend state await todoItemsCollection.utils.refetch(); throw error; } },
401-416: LGTM! Correct positioning logic for drops on empty/end.The logic properly handles dropping on a board by generating a position after the last item (or the first position if empty) using
generateKeyBetween.
422-442: LGTM! Same-column reordering logic is correct.The code properly computes the new position between the previous item and the drop target using
findPrevItemandgenerateKeyBetween.
443-465: LGTM! Cross-board move logic is correct.The code properly handles moving items to a different board by computing the correct position and updating both
boardIdandposition.
480-495: LGTM! TodoBoards rendering structure is correct.The component properly sets up the DnD context and renders boards with the correct props.
354-387: Consider usingleftJoininstead ofinnerJoinif orphaned todo items should be displayed.The query uses
innerJoinfor board and project associations, meaning todo items with invalid or missingboardIdvalues—or boards with invalidprojectIdvalues—will be silently excluded from results. If the data schema enforces foreign key constraints with cascade delete or no-action rules, this is safe; orphaned records shouldn't exist. However, if constraints are not enforced, items could be hidden from the UI unintentionally. Verify whether the schema requires valid board/project references and update the join type accordingly.
…king the positions of todo items
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
drizzle/migrations/0000_outstanding_silvermane.sql(2 hunks)drizzle/migrations/meta/0000_snapshot.json(2 hunks)drizzle/migrations/meta/_journal.json(1 hunks)src/components/TodoBoards.tsx(12 hunks)src/db/migrations.json(1 hunks)src/db/schema.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T21:20:23.301Z
Learnt from: CR
Repo: fulopkovacs/trytanstackdb.com PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T21:20:23.301Z
Learning: Use `pnpm db.generate` to generate database migrations
Applied to files:
src/db/migrations.json
🧬 Code graph analysis (2)
src/db/migrations.json (2)
src/components/CreateOrEditTodoItems.tsx (2)
project(39-43)newItemData(32-44)src/db/seed.ts (2)
project(261-278)item(179-184)
src/components/TodoBoards.tsx (5)
src/components/CreateOrEditTodoItems.tsx (1)
CreateOrEditTodoItems(19-143)src/db/schema.ts (1)
TodoItemRecord(72-72)src/collections/todoItems.ts (2)
todoItemsCollection(80-115)batchUpdateTodoItem(38-56)src/collections/boards.ts (1)
boardCollection(17-24)src/collections/projects.ts (1)
projectsCollection(16-41)
🔇 Additional comments (9)
drizzle/migrations/meta/_journal.json (1)
8-9: LGTM!Migration metadata correctly updated with new timestamp and tag name for the fractional indexing migration.
drizzle/migrations/0000_outstanding_silvermane.sql (1)
43-43: LGTM!The composite index on
(board_id, position)is essential for efficiently querying and sorting todo items by their fractional position within each board. This aligns perfectly with the PR objective of using fractional indexing for position management.src/db/migrations.json (1)
1-17: LGTM!The compiled migration correctly reflects the schema changes:
item_positions_in_the_projectremoved from projects table,positionfield added to todo_items, and the composite index on(board_id, position)created. Previous formatting and schema mismatch issues have been addressed.src/db/schema.ts (1)
54-70: LGTM!The schema refactoring correctly implements fractional indexing by:
- Adding the required
positiontext field- Using the three-argument
pgTablesignature to define a composite index on(boardId, position)- Ensuring efficient queries for position-based sorting within boards
The schema changes align perfectly with the migration SQL.
src/components/TodoBoards.tsx (4)
50-70: LGTM!The
findPrevItemhelper correctly identifies the previous item within the same board from a position-sorted array. The boardId check ensures cross-board boundaries are respected when calculating new fractional positions.
166-181: LGTM!The query correctly implements lexical string sorting for fractional index positions. This is crucial because fractional indexing relies on lexical comparison (e.g., "Zz" < "a0") rather than locale-based sorting. The inline comment helpfully explains this distinction.
354-387: LGTM!The cross-table query correctly fetches all todo items for a project with their positions, ordered by board and position using lexical sorting. This structure supports the
findPrevItemhelper by maintaining items grouped by board and sorted by fractional position within each group.
393-470: LGTM!The drag-and-drop logic correctly implements fractional indexing for all scenarios:
- Drop on empty/end of column: Uses
generateKeyBetween(lastPosition, null)to append after last item- Reorder within column: Uses
generateKeyBetween(prev, over)to insert between adjacent items- Move to another column: Uses
generateKeyBetween(prev, over)with new boardId to position correctly in target boardThe logic properly handles edge cases like dropping on empty columns and maintains correct ordering through lexical string comparison.
drizzle/migrations/meta/0000_snapshot.json (1)
181-209: LGTM!The schema snapshot correctly documents the addition of the
positionfield and the composite index on(board_id, position). The index configuration (btree, ascending, nulls last) is appropriate for efficient range queries on fractional positions within boards.
| // Create paced mutation with 3 second debounce for updating todo positions | ||
| const updateTodoPosition = usePacedMutations< | ||
| { | ||
| itemId: string; | ||
| boardId?: string; | ||
| newPosition: string; | ||
| }, | ||
| TodoItemRecord | ||
| >({ | ||
| onMutate: ({ itemId, boardId, newPosition }) => { | ||
| // Apply optimistic update immediately | ||
| todoItemsCollection.update(itemId, (item) => { | ||
| if (boardId) { | ||
| item.boardId = boardId; | ||
| } | ||
| item.position = newPosition; | ||
| }); | ||
| }, | ||
| mutationFn: async ({ transaction }) => { | ||
| // Persist all position updates to the backend after debounce | ||
| const mutations = transaction.mutations; | ||
|
|
||
| const updates = mutations.reduce( | ||
| (acc, mutation) => { | ||
| const { modified, changes } = mutation; | ||
| acc.push({ | ||
| id: modified.id, | ||
| ...changes, | ||
| }); | ||
| return acc; | ||
| }, | ||
| [] as (Partial<TodoItemRecord> & { id: string })[], | ||
| ); | ||
|
|
||
| await batchUpdateTodoItem({ | ||
| data: updates, | ||
| }); | ||
|
|
||
| // Refetch to ensure consistency with backend | ||
| await todoItemsCollection.utils.refetch(); | ||
| }, | ||
| strategy: debounceStrategy({ wait: 1_500 }), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usePacedMutations usage and error handling patterns
echo "=== Searching for usePacedMutations implementation ==="
rg -n "usePacedMutations" --type=ts --type=tsx -B 2 -A 20
echo -e "\n=== Searching for error handling in mutations (onError, catch, try) ==="
rg -n "onError|catch|try" --type=ts --type=tsx | grep -i "mutation\|todo" | head -30
echo -e "\n=== Checking for framework error handling patterns ==="
rg -n "usePacedMutations\|paced.*mutation" --type=ts --type=tsx -B 5 -A 30 | grep -E "(onError|catch|error|rollback)" -iRepository: fulopkovacs/trytanstackdb.com
Length of output: 357
🌐 Web query:
TanStack React DB usePacedMutations error handling rollback
💡 Result:
Short answer — usePacedMutations supports optimistic updates and automatic rollback. Key points and how to handle errors:
- onMutate is where you apply the optimistic update immediately. (usePacedMutations docs). [1]
- The persistence is performed by your mutationFn (run by the chosen strategy). If mutationFn throws/rejects, the transaction is automatically rolled back and the optimistic changes are reverted. You can observe that by awaiting the transaction's isPersisted.promise and catching errors. (Error-handling + Transaction docs). [2][3]
- Transactions expose rollback() (you can call it manually) and rollbacks cascade for conflicting transactions. Use try/catch around await tx.isPersisted.promise to handle failures and show UI/errors. [3]
Minimal example pattern:
- onMutate: apply optimistic update.
- mutationFn: perform network call; throw on failure.
- call mutate(...) to get tx, then await tx.isPersisted.promise in a try/catch to detect rollback and report error to the user.
References:
- usePacedMutations docs. [1]
- Error handling guide (automatic rollback when mutation handler throws). [2]
- Transaction API (isPersisted.promise, rollback, cascade). [3]
Add error handling for mutation failures and persistence.
The framework automatically rolls back optimistic updates when mutationFn throws, but the code needs explicit error handling. The proper pattern is to catch transaction failures at the call site:
try {
const tx = updateTodoPosition.mutate({ itemId, boardId, newPosition });
await tx.isPersisted.promise;
} catch (error) {
// Handle rollback and show error to user
}Additionally, the refetch() call in mutationFn could fail silently. Wrap it in a try-catch or ensure the error propagates to trigger rollback if persistence should fail.
1 participant
Previously we tracked the positions of the todo items in a single json file per project. This led to errors, especially when the network latency was high and the items were being reordered fast.
String-based fractional indexing should solve this issue.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.