Add concurrency move tests and fix setFormulaResolver delegation#41
Merged
Conversation
Move operations (row/column drag-reorder) had zero concurrent editing test coverage, which was the likely cause of divergence bugs seen in live collaboration when users move rows/columns simultaneously. Adds 11 new concurrency matrix cases: - 3 move vs value-edit (row forward, row backward, column forward) - 4 move vs structural-change (move+insert, move+delete, move+move, column move+insert) - 1 move vs formula (formula reference remapping after row move) - 3 gap-coverage (col insert adjacent, row delete different indexes, col delete+insert adjacent) Total concurrency cases: 24 → 35. All new cases registered in the Yorkie convergence test as characterization cases. Co-Authored-By: Claude Opus 4.6 <[email protected]>
📝 WalkthroughWalkthroughThis PR extends the concurrency testing infrastructure to comprehensively cover move operations for rows and columns. It adds new test cases, helper implementations to handle move operations, and documentation capturing lessons learned and test planning for move-related concurrency scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
Verification: verify:selfResult: ✅ PASS in 97.1s
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: 2
🧹 Nitpick comments (1)
packages/sheet/test/helpers/concurrency-case-table.ts (1)
678-678: Remove the leftover placeholder note.This table is the canonical expectation source now, so
placeholder — will be corrected after first test runmakes the snapshot look intentionally unverified. Replace it with the reasoning behind the expected state or drop the comment.As per coding guidelines, "Write clear comments explaining the 'why' behind complex logic, not just the 'what'."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheet/test/helpers/concurrency-case-table.ts` at line 678, Remove the leftover placeholder comment "placeholder — will be corrected after first test run" in concurrency-case-table.ts and either replace it with a brief explanatory comment describing why this table is the canonical expected state for the tests (the rationale for the values and any non-obvious decisions) or delete the comment entirely so the snapshot isn't misleading; locate the exact placeholder string in the test helper and update it accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/tasks/active/20260316-concurrency-move-tests-todo.md`:
- Around line 34-35: The summary claims 35 concurrency cases but the actual
concurrencyCases array (in
packages/sheet/test/helpers/concurrency-case-table.ts) contains only 34 entries;
inspect the concurrencyCases export and add the missing scenario (ensure it is
one of the characterization cases where aThenB ≠ bThenA) to make the array
length 35, or if the extra case was never intended, update the Markdown summary
(the "Total: 24 → 35 concurrency cases." line and related sentence) to reflect
the true total; verify counts by checking the exported concurrencyCases
identifier and the list of added scenarios to ensure the doc and code stay in
sync.
In `@packages/frontend/tests/app/spreadsheet/yorkie-concurrency.test.ts`:
- Around line 37-47: The test suite is hard-coding matchesSerialOrder === false
for several move-related cases while one case ("value edit vs row move forward
(cell in moved range)") is still marked as a provisional placeholder in
concurrency-case-table.ts; update the tests so that provisional cases are not
asserted as final: locate the characterizationCases array and the test loop that
asserts matchesSerialOrder (the symbols characterizationCases and
matchesSerialOrder in yorkie-concurrency.test.ts), then either move the
placeholder case into a pending/skip bucket or add a skip/todo marker (so it is
not asserted) until Yorkie confirms the correct behavior; keep the Yorkie-only
gating (YORKIE_RPC_ADDR) but ensure skipped/pending cases do not fail the run.
---
Nitpick comments:
In `@packages/sheet/test/helpers/concurrency-case-table.ts`:
- Line 678: Remove the leftover placeholder comment "placeholder — will be
corrected after first test run" in concurrency-case-table.ts and either replace
it with a brief explanatory comment describing why this table is the canonical
expected state for the tests (the rationale for the values and any non-obvious
decisions) or delete the comment entirely so the snapshot isn't misleading;
locate the exact placeholder string in the test helper and update it
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16697b58-fa7c-4838-954e-357940d783e0
📒 Files selected for processing (6)
docs/tasks/active/20260316-concurrency-move-tests-lessons.mddocs/tasks/active/20260316-concurrency-move-tests-todo.mdpackages/frontend/tests/app/spreadsheet/yorkie-concurrency.test.tspackages/frontend/tests/helpers/two-user-yorkie.tspackages/sheet/test/helpers/concurrency-case-table.tspackages/sheet/test/helpers/concurrency-driver.ts
Comment on lines
+37
to
+47
| "value edit vs row move forward (cell in moved range)", | ||
| "value edit vs row move backward (cell in moved range)", | ||
| "value edit vs column move forward", | ||
| "row move vs row insert at same index", | ||
| "row move vs row delete at source", | ||
| "row move vs row move (different rows)", | ||
| "column move vs column insert at same index", | ||
| "formula reference vs row move", | ||
| "column insert vs column insert at adjacent indexes", | ||
| "row delete vs row delete at different indexes", | ||
| "column delete vs column insert at adjacent indexes", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file structure
head -100 packages/frontend/tests/app/spreadsheet/yorkie-concurrency.test.ts | cat -nRepository: wafflebase/wafflebase
Length of output: 3838
🏁 Script executed:
# Let's specifically look at lines 35-90 to understand the context
sed -n '35,90p' packages/frontend/tests/app/spreadsheet/yorkie-concurrency.test.ts | cat -nRepository: wafflebase/wafflebase
Length of output: 2148
🏁 Script executed:
# Search for "characterizationCases" to understand how it's used
rg "characterizationCases" packages/frontend/tests/app/spreadsheet/yorkie-concurrency.test.ts -A 5 -B 5Repository: wafflebase/wafflebase
Length of output: 747
🏁 Script executed:
# Check for any Yorkie-related TODOs or pending verification notes in the file
rg -i "yorkie|pending|verify|convergence" packages/frontend/tests/app/spreadsheet/yorkie-concurrency.test.tsRepository: wafflebase/wafflebase
Length of output: 601
🏁 Script executed:
# Find the concurrency case table to understand these cases
find . -name "concurrency-case-table.ts" -type fRepository: wafflebase/wafflebase
Length of output: 120
🏁 Script executed:
# Check the concurrency case table to see how these cases are defined
cat packages/sheet/test/helpers/concurrency-case-table.ts | head -200Repository: wafflebase/wafflebase
Length of output: 4894
🏁 Script executed:
# Search for any comments in the test file about the move cases or Yorkie status
rg -i "move|convergence" packages/frontend/tests/app/spreadsheet/yorkie-concurrency.test.ts -B 2 -A 2Repository: wafflebase/wafflebase
Length of output: 639
🏁 Script executed:
# Check git history or comments about Yorkie verification status
rg -i "yorkie.*pending|pending.*yorkie|RPC" packages/frontend/tests/app/spreadsheet/ -B 2 -A 2Repository: wafflebase/wafflebase
Length of output: 1701
🏁 Script executed:
# Search for the move case definitions in concurrency-case-table.ts
rg "move forward|move backward|row move|column move" packages/sheet/test/helpers/concurrency-case-table.ts -B 2 -A 15Repository: wafflebase/wafflebase
Length of output: 5094
🏁 Script executed:
# Check file size to see if we can view the full file
wc -l packages/sheet/test/helpers/concurrency-case-table.tsRepository: wafflebase/wafflebase
Length of output: 122
🏁 Script executed:
# Get the tail portion to see move case definitions
tail -600 packages/sheet/test/helpers/concurrency-case-table.ts | head -300Repository: wafflebase/wafflebase
Length of output: 8437
🏁 Script executed:
# Check if there are more placeholder comments in the move cases
rg "placeholder" packages/sheet/test/helpers/concurrency-case-table.ts -B 3 -A 3Repository: wafflebase/wafflebase
Length of output: 319
🏁 Script executed:
# Let's get the full move cases section to see all expect definitions
sed -n '/--- Move concurrency cases ---/,/^ \];/p' packages/sheet/test/helpers/concurrency-case-table.ts | head -400Repository: wafflebase/wafflebase
Length of output: 9764
Move cases in characterizationCases need verification before hard-coding matchesSerialOrder === false.
The "value edit vs row move forward (cell in moved range)" case has a // placeholder — will be corrected after first test run marker in the concurrency-case-table.ts, indicating the expected behavior is provisional. All 11 move-related cases (lines 37–47) assert matchesSerialOrder === false in the test loop (lines 65–77), but at least one expectation is explicitly unverified. The tests run only when YORKIE_RPC_ADDR is set; move the placeholder case to a pending bucket or add a skip/todo marker until Yorkie confirms its outcome.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/tests/app/spreadsheet/yorkie-concurrency.test.ts` around
lines 37 - 47, The test suite is hard-coding matchesSerialOrder === false for
several move-related cases while one case ("value edit vs row move forward (cell
in moved range)") is still marked as a provisional placeholder in
concurrency-case-table.ts; update the tests so that provisional cases are not
asserted as final: locate the characterizationCases array and the test loop that
asserts matchesSerialOrder (the symbols characterizationCases and
matchesSerialOrder in yorkie-concurrency.test.ts), then either move the
placeholder case into a pending/skip bucket or add a skip/todo marker (so it is
not asserted) until Yorkie confirms the correct behavior; keep the Yorkie-only
gating (YORKIE_RPC_ADDR) but ensure skipped/pending cases do not fail the run.
Spreadsheet was missing a delegation method for Sheet.setFormulaResolver(), causing a TypeError in production builds where the minified Spreadsheet instance had no such method. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Remove leftover "placeholder" comment from concurrency-case-table.ts and fix the case count in the todo doc (23 → 34, not 24 → 35). Co-Authored-By: Claude Opus 4.6 <[email protected]>
hackerwins
changed the title
5 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
ConcurrencyOptype withmove-rows/move-columnsand updated both MemStore and YorkieStore concurrency driversNew test cases
Test plan
pnpm verify:fastpasses (lint + all unit tests)YORKIE_RPC_ADDRto verify CRDT convergence for new cases🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Documentation
Tests