Fix duplicate row ID on concurrent row insertion#127
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 31 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe changes address a production issue where concurrent worksheet axis insertions could generate duplicate row IDs by replacing sequential ID generation with random 4-character base36 identifiers. The function signature and all call sites are updated to remove counter dependencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Verification: verify:selfResult: ✅ PASS in 119.6s
Verification: verify:integrationResult: ✅ PASS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sheets/src/model/workbook/worksheet-grid.ts (1)
32-33: 🛠️ Refactor suggestion | 🟠 Major
nextRowId/nextColIdare unused—remove them along with migration code that sets them.These fields are initialized here but never read or used for ID generation;
createWorksheetAxisIdgenerates IDs randomly using crypto instead. They're also never updated whenensureAxisLengthgrows the axes, confirming they serve no functional purpose.Removing them requires:
- Deleting initialization from
ensureWorksheetGrid(worksheet-grid.ts:32-33)- Removing from the
WorksheetGridShapeinterface (worksheet-record.ts:7-8)- Removing from
WorksheetDocumentinterface (worksheet-document.ts:49-50)- Updating migration code that sets these fields (worksheet-shape-migration.ts:435-436)
- Adjusting migration validation logic (worksheet-shape-migration.ts:316-317)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheets/src/model/workbook/worksheet-grid.ts` around lines 32 - 33, Remove the unused nextRowId and nextColId fields and related migration handling: delete the initialization lines in ensureWorksheetGrid that set ws.nextRowId and ws.nextColId, remove those properties from the WorksheetGridShape and WorksheetDocument interfaces, and update the migration code and validation in the worksheet-shape-migration logic to stop reading/writing these fields (they are unused because createWorksheetAxisId generates IDs and ensureAxisLength updates axes). Ensure no remaining references to nextRowId/nextColId exist after the changes.
🧹 Nitpick comments (3)
docs/tasks/active/20260413-random-axis-id-todo.md (2)
10-14: Update task checkboxes to reflect completed work.Based on the code changes in this PR:
- Tasks 1-3 appear to be implemented
- Task 4 (concurrency test) is partially addressed—there's a uniqueness test but not a true concurrent insertion test
- Task 5 status is mentioned as passing in the PR description
Consider marking completed items or removing this file if merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/tasks/active/20260413-random-axis-id-todo.md` around lines 10 - 14, Update the checklist in this task file to reflect implemented changes: mark items 1, 2, and 3 as completed (since createWorksheetAxisId, ensureAxisLength, and insertYorkieWorksheetAxis were changed), adjust item 4 to note that the uniqueness test exists but a true concurrency test for simultaneous inserts is still needed (or remove/replace it if you add the concurrent test separately), and mark item 5 as passed if pnpm verify:fast succeeded; alternatively remove the file if it's no longer needed.
1-14: Consider adding a paired lessons file.As per coding guidelines, non-trivial tasks in
docs/tasks/active/should use paired files:YYYYMMDD-<slug>-todo.mdandYYYYMMDD-<slug>-lessons.md. A lessons file documenting root cause analysis and the reasoning behind choosing random IDs over other solutions would be valuable for future reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/tasks/active/20260413-random-axis-id-todo.md` around lines 1 - 14, The review requests adding a paired lessons file alongside the todo: create a new file named with the same date/slug but ending in -lessons.md that documents the root cause (shared nextRowId counter causing concurrent duplicate axis IDs), why we chose 4-char base36 random IDs in createWorksheetAxisId(prefix, number) instead of other approaches, how ensureAxisLength() and insertYorkieWorksheetAxis() were updated to remove counter dependency, and include notes on the concurrency test added and verification steps (pnpm verify:fast) so future devs can understand the reasoning and reproduce the fix; reference the functions createWorksheetAxisId, ensureAxisLength, and insertYorkieWorksheetAxis in the lessons file.packages/sheets/test/model/worksheet-record.test.ts (1)
13-20: Probabilistic test may flake over time.With 36^4 ≈ 1.68M possibilities, generating 1000 IDs has a non-trivial collision probability (~0.03% per run via birthday paradox). While unlikely in a single run, this could cause occasional CI failures across many runs.
Consider either:
- Lowering the count to reduce collision probability further (e.g., 500 IDs)
- Accepting that the test verifies "high uniqueness" rather than absolute guarantees
Also, task 4 in the todo file mentions adding a concurrency test for simultaneous row inserts—this test only validates single-threaded uniqueness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheets/test/model/worksheet-record.test.ts` around lines 13 - 20, The probabilistic uniqueness test "should generate unique IDs across many calls" using createWorksheetAxisId('r') can flake; reduce collision risk by lowering iterations (e.g., from 1000 to 500) or change the assertion to verify "high uniqueness" (e.g., expect(ids.size).toBeGreaterThanOrEqual(490)) instead of exact equality; update the test body that constructs the Set and the final expect to use the new count or threshold and leave a brief comment referencing createWorksheetAxisId and the reason for the threshold.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sheets/src/model/workbook/worksheet-record.ts`:
- Around line 80-81: Add an explicit engines constraint to the package.json for
the packages/sheets package so consumers and CI know the minimum Node version;
update the packages/sheets package.json to include an "engines" field (e.g.,
"node": ">=18" to satisfy Vitest and modern crypto.getRandomValues availability)
and commit the change so tests and downstream consumers are aware of the runtime
requirement.
---
Outside diff comments:
In `@packages/sheets/src/model/workbook/worksheet-grid.ts`:
- Around line 32-33: Remove the unused nextRowId and nextColId fields and
related migration handling: delete the initialization lines in
ensureWorksheetGrid that set ws.nextRowId and ws.nextColId, remove those
properties from the WorksheetGridShape and WorksheetDocument interfaces, and
update the migration code and validation in the worksheet-shape-migration logic
to stop reading/writing these fields (they are unused because
createWorksheetAxisId generates IDs and ensureAxisLength updates axes). Ensure
no remaining references to nextRowId/nextColId exist after the changes.
---
Nitpick comments:
In `@docs/tasks/active/20260413-random-axis-id-todo.md`:
- Around line 10-14: Update the checklist in this task file to reflect
implemented changes: mark items 1, 2, and 3 as completed (since
createWorksheetAxisId, ensureAxisLength, and insertYorkieWorksheetAxis were
changed), adjust item 4 to note that the uniqueness test exists but a true
concurrency test for simultaneous inserts is still needed (or remove/replace it
if you add the concurrent test separately), and mark item 5 as passed if pnpm
verify:fast succeeded; alternatively remove the file if it's no longer needed.
- Around line 1-14: The review requests adding a paired lessons file alongside
the todo: create a new file named with the same date/slug but ending in
-lessons.md that documents the root cause (shared nextRowId counter causing
concurrent duplicate axis IDs), why we chose 4-char base36 random IDs in
createWorksheetAxisId(prefix, number) instead of other approaches, how
ensureAxisLength() and insertYorkieWorksheetAxis() were updated to remove
counter dependency, and include notes on the concurrency test added and
verification steps (pnpm verify:fast) so future devs can understand the
reasoning and reproduce the fix; reference the functions createWorksheetAxisId,
ensureAxisLength, and insertYorkieWorksheetAxis in the lessons file.
In `@packages/sheets/test/model/worksheet-record.test.ts`:
- Around line 13-20: The probabilistic uniqueness test "should generate unique
IDs across many calls" using createWorksheetAxisId('r') can flake; reduce
collision risk by lowering iterations (e.g., from 1000 to 500) or change the
assertion to verify "high uniqueness" (e.g.,
expect(ids.size).toBeGreaterThanOrEqual(490)) instead of exact equality; update
the test body that constructs the Set and the final expect to use the new count
or threshold and leave a brief comment referencing createWorksheetAxisId and the
reason for the threshold.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee1c6f30-33a0-4c3c-bbc4-21dfecb86581
📒 Files selected for processing (5)
docs/tasks/active/20260413-random-axis-id-todo.mdpackages/frontend/src/app/spreadsheet/yorkie-worksheet-axis.tspackages/sheets/src/model/workbook/worksheet-grid.tspackages/sheets/src/model/workbook/worksheet-record.tspackages/sheets/test/model/worksheet-record.test.ts
| export function createWorksheetAxisId(prefix: 'r' | 'c'): string { | ||
| const bytes = crypto.getRandomValues(new Uint8Array(AXIS_ID_LENGTH)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a Node.js version constraint and where this module is imported
echo "=== Checking Node.js version in package.json files ==="
fd -t f 'package.json' --exec grep -l 'engines' {} \; | xargs -I {} sh -c 'echo "--- {} ---" && cat {} | jq ".engines"'
echo ""
echo "=== Checking if worksheet-record.ts is used in backend/server code ==="
rg -l 'worksheet-record' --type ts -g '**/backend/**' -g '**/server/**'Repository: wafflebase/wafflebase
Length of output: 248
🏁 Script executed:
#!/bin/bash
# Check all package.json files for engine constraints
echo "=== All package.json engine constraints ==="
fd -t f 'package.json' --exec sh -c 'cat {} | jq -r "select(.engines != null) | \"\({}\): \(.engines)\"" 2>/dev/null' \;
echo ""
echo "=== Root package.json ==="
cat package.json | jq '.engines // "No engine constraint"'
echo ""
echo "=== Check worksheet-record imports in test files ==="
rg -l 'worksheet-record|createWorksheetAxisId' --type ts -g '**/*.test.ts' -g '**/*.spec.ts' | head -20
echo ""
echo "=== Check where createWorksheetAxisId is used ==="
rg 'createWorksheetAxisId' --type ts -B 2 -A 2 | head -50Repository: wafflebase/wafflebase
Length of output: 3585
🏁 Script executed:
#!/bin/bash
# Check how crypto is imported in worksheet-record.ts
echo "=== Check crypto import in worksheet-record.ts ==="
head -20 packages/sheets/src/model/workbook/worksheet-record.ts
echo ""
echo "=== Check if crypto is imported at all ==="
rg 'import.*crypto' packages/sheets/src/model/workbook/worksheet-record.ts
echo ""
echo "=== Check vitest/test configuration ==="
fd -t f 'vitest.config.*' -o 'vite.config.*' --exec cat {} \; | head -100
echo ""
echo "=== Check packages.json in packages/sheets ==="
cat packages/sheets/package.json | jq '{engines, devDependencies: .devDependencies | keys | map(select(contains("vitest") or contains("node")))}'Repository: wafflebase/wafflebase
Length of output: 940
🏁 Script executed:
#!/bin/bash
# Read the full worksheet-record.ts file to find crypto reference
echo "=== Full worksheet-record.ts file ==="
cat packages/sheets/src/model/workbook/worksheet-record.ts
echo ""
echo "=== Check for any build/polyfill config ==="
fd -type f 'tsconfig.json' 'vite.config.ts' 'webpack.config.ts' --exec sh -c 'echo "--- {} ---" && cat {}' \; | head -150Repository: wafflebase/wafflebase
Length of output: 2834
🏁 Script executed:
#!/bin/bash
# Check packages/sheets context and purpose
echo "=== packages/sheets README ==="
cat packages/sheets/README.md 2>/dev/null || echo "No README found"
echo ""
echo "=== packages/sheets package.json (name, description, main) ==="
cat packages/sheets/package.json | jq '{name, description, main, browser, exports}'
echo ""
echo "=== Check if sheets is exported as a browser library ==="
cat packages/sheets/tsconfig.json 2>/dev/null | jq '.compilerOptions.lib, .compilerOptions.target' || echo "No tsconfig or lib config found"Repository: wafflebase/wafflebase
Length of output: 4016
Consider adding an engines constraint to packages/sheets for clarity.
crypto.getRandomValues() relies on the global crypto object, which is standard in browsers and available in Node.js 15+. Since the library lacks an explicit engines field in package.json, add "engines": { "node": ">=15" } to document the minimum Node.js version requirement. This also affects the test environment (Vitest requires Node.js 18+) and any downstream consumers who may bundle this code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sheets/src/model/workbook/worksheet-record.ts` around lines 80 - 81,
Add an explicit engines constraint to the package.json for the packages/sheets
package so consumers and CI know the minimum Node version; update the
packages/sheets package.json to include an "engines" field (e.g., "node": ">=18"
to satisfy Vitest and modern crypto.getRandomValues availability) and commit the
change so tests and downstream consumers are aware of the runtime requirement.
Concurrent row/column insertion by two clients could read the same nextRowId counter value and generate identical axis IDs (e.g. both create "r93"), causing a single row ID to appear at two positions in rowOrder. This was observed in production where editing row 15 also affected row 30 because both mapped to "r93". Replace the shared sequential counter with 4-char base36 random IDs generated via crypto.getRandomValues (1.6M+ combinations), making collisions structurally impossible regardless of concurrency. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
78adea1 to
cd18ed9
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Summary
createWorksheetAxisId()used a sharednextRowIdcounter. When two clients inserted rows simultaneously, both read the same counter value and generated identical IDs (e.g., both creater93), leaving a duplicate inrowOrder.sheet-a39621d7,r93appeared at both visual row 15 and row 30. Editing row 15 caused changes to appear at row 30.crypto.getRandomValues(~1.6M combinations), making collisions structurally impossible regardless of concurrency.Changed files
packages/sheets/src/model/workbook/worksheet-record.tscreateWorksheetAxisId()→ random ID generationpackages/sheets/src/model/workbook/worksheet-grid.tsensureAxisLength()packages/frontend/src/app/spreadsheet/yorkie-worksheet-axis.tsensureAxisLength()+insertYorkieWorksheetAxis()packages/sheets/test/model/worksheet-record.test.tsTest plan
pnpm verify:fastpasses (1115 sheets tests + all other packages)worksheet-record.test.tsvalidates ID format (/^[rc][a-z0-9]{4}$/) and uniqueness across 1000 calls🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation