Use axis ID based selection for collaborative cursor tracking#130
Merged
Conversation
Selection uses stable CRDT axis IDs instead of absolute coordinates, so remote row/column inserts automatically preserve cursor position without explicit shift logic. Includes peer range overlay rendering. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
8-task plan covering: anchor conversion layer, Store interface, Sheet engine, YorkieStore, remote sync, overlay rendering, initialization, and end-to-end verification. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Introduce CellAnchor, RangeAnchor, and SelectionPresence types along with bidirectional conversion functions between stable axis IDs and visual coordinates (Ref/Range). This is the foundation for replacing coordinate-based selection with axis ID references that survive concurrent row/column insertions and deletions. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Extend the Store interface with axis-ID-based selection methods alongside the existing updateActiveCell (now deprecated). Widen getPresences return type to include optional SelectionPresence. Update MemStore, ReadOnlyStore, Sheet, and Overlay to match. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Sheet maintains CellAnchor/RangeAnchor alongside Ref-based fields. resolveAnchorsToRefs() re-resolves on remote sync. Remove manual activeCell shift logic from shiftCells(). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
YorkieStore writes SelectionPresence to Yorkie presence. UserPresence type includes both new selection and legacy activeCell fields for backward compatibility. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
reloadDimensions() calls resolveAnchorsToRefs() so activeCell automatically tracks its axis ID after remote row/col inserts. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Overlay handles both axis-ID-based and legacy Sref-based peer presences. Peer ranges are drawn with 10% opacity fill. Active cell keeps colored border + name label. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Ensures activeCellAnchor is set from the start so resolveAnchorsToRefs works on the first remote change. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
All call sites migrated to updateSelection(CellAnchor, RangeAnchor[]). The legacy method is no longer needed. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
colOrder only contains axis IDs for columns with data (e.g. 3), not the visual grid width (e.g. 100). Entire-row selection was rendered as only 3 columns wide instead of the full sheet width. Pass sheet dimension to rangeAnchorToRange so null axis fields expand to the visual grid size. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
selectEnd and addSelectionEnd now call syncSelectionToPresence() so range changes are immediately visible to peers. Also fix selectStart to clear ranges before setActiveCell to avoid sending stale ranges. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Selecting column E when only A-B have data caused colOrder[4] to be undefined, producing null colIds that were interpreted as select-all. Now ensureAxisOrder extends only the relevant axis (cols for column selection, rows for row selection) to avoid inflating document size. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
updatePeerLabelVisibility checked presence.activeCell (string) which is no longer set in the new axis-ID format. Now derives a stable key from presence.selection when available. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
When a remote peer inserts/deletes rows or columns while the local user is editing, the input cell now follows the active cell to its new visual position instead of staying at the old location. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Hover detection only checked presence.activeCell (legacy string). Now resolves axis-ID-based selection via anchorToRef so hovering a remote peer's active cell shows the name label. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
📝 WalkthroughWalkthroughImplements axis-ID anchored selection/presence: new anchor types and conversion utilities, store and Yorkie API updates to publish SelectionPresence, sheet engine anchor state and re-resolution logic, overlay/worksheet rendering of peer ranges, tests, and docs describing migration and rollout. Changes
Sequence DiagramsequenceDiagram
actor User
participant Sheet
participant Store
participant Yorkie
participant RemoteClient
participant Overlay
User->>Sheet: select cells (Ref/Range)
Sheet->>Sheet: convert Ref/Range → CellAnchor/RangeAnchor
Sheet->>Sheet: ensureAxisOrder if needed
Sheet->>Store: updateSelection(activeCellAnchor, rangeAnchors)
Store->>Yorkie: write SelectionPresence
Yorkie->>RemoteClient: broadcast presence
RemoteClient->>RemoteClient: keep anchors stable across structure edits
RemoteClient->>RemoteClient: on reload → resolveAnchorsToRefs()
Yorkie->>Overlay: push peer presences
Overlay->>Overlay: convert anchors → Ref/Range
Overlay->>Overlay: render active-cell border + translucent range fills
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly Related PRs
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 docstrings
🧪 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 |
Contributor
Verification: verify:selfResult: ✅ PASS in 112.9s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/frontend/src/app/spreadsheet/yorkie-store.ts (1)
21-39:⚠️ Potential issue | 🔴 CriticalRe-add the
MergeSpantype import.Line 839, Line 888, and the undo/redo merge snapshots still reference
MergeSpan, so this file no longer type-checks as written.🐛 Proposed fix
import type { Store, Grid, Cell, CellStyle, CellAnchor, RangeAnchor, + MergeSpan, FilterCondition, FilterState, HiddenState,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/app/spreadsheet/yorkie-store.ts` around lines 21 - 39, The import list in yorkie-store.ts is missing the MergeSpan type which is still referenced by the merge snapshot handling (undo/redo merge snapshots and usages of MergeSpan); restore the missing type by adding MergeSpan to the grouped type import from "@wafflebase/sheets" alongside Store, Grid, Cell, etc., so all references to MergeSpan type-check again.
🤖 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/design/sheets/axis-id-selection.md`:
- Around line 88-90: The markdown code fences around the flow diagram blocks
(for example the block containing "Presence (axis ID) ←→ Conversion Layer ←→
Sheet Engine (Ref) → Canvas") are unlabeled and trigger MD040; update those
fences to include a language tag such as "text" (e.g., change ``` to ```text)
for the blocks at the shown location and the other flow blocks referenced (lines
around 106-114, 118-127, 131-139) so each fenced block has a language
identifier.
In `@packages/frontend/src/app/spreadsheet/yorkie-store.ts`:
- Around line 624-635: In ensureAxisOrder, when updating the document via
this.doc.update and accessing ws = root.sheets[this.tabId], initialize
ws.rowOrder and ws.colOrder to empty arrays if they are undefined before using
.length or .push; specifically, inside the update callback ensure ws.rowOrder
||= [] and ws.colOrder ||= [] (or equivalent) prior to the while loops that call
createWorksheetAxisId so legacy sheets without axis-order fields do not throw.
- Around line 605-612: updateSelection currently writes only the new selection
shape which breaks legacy clients that read presence.activeCell; update the
yorkie-store.updateSelection implementation to also emit the legacy activeCell
by converting the activeCell anchor to a presence ref (use anchorToRef) and
setting it on the document presence alongside selection and activeTabId (i.e.,
call p.set('activeCell', anchorToRef(activeCell))). Add anchorToRef to the
imports from `@wafflebase/sheets` and update the updateSelection method to set
both selection and the legacy activeCell so older clients in user-presence.tsx
(lines referenced) continue to work.
In `@packages/sheets/src/model/workbook/anchor-conversion.ts`:
- Around line 50-61: The current logic treats indexOf==-1 as 0 and then clamps
to 1 via Math.max, which produces inverted or leaked selections; instead detect
deleted endpoints (startR/startC/endR/endC === 0) and repair by replacing a
single deleted endpoint with the surviving endpoint (e.g., if startR===0 set
startR = endR; if endR===0 set endR = startR; same for startC/endC), then clamp
both to [1, maxRows/maxCols] and finally normalize the range so the start <= end
(swap if needed) before returning the two points; reference the variables
startR/startC/endR/endC,
anchor.startRowId/anchor.endRowId/anchor.startColId/anchor.endColId, Math.max
and maxRows/maxCols in your change.
- Around line 20-29: anchorToRef and rangeAnchorToRange currently do linear
lookups with rowOrder.indexOf / colOrder.indexOf which causes O(n) work per
call; change these helpers to accept cached Maps (e.g., rowIndexMap:
Map<string,number>, colIndexMap: Map<string,number>) and use
map.get(anchor.rowId)/map.get(anchor.colId) to compute r and c (add +1 as
before), returning null if get() is undefined; update function signatures and
all call sites to pass the precomputed maps (and consider adding small wrapper
overloads that build maps once for legacy callers) so rendering and hit-testing
become O(1) per anchor.
In `@packages/sheets/src/model/worksheet/sheet.ts`:
- Around line 4022-4043: When resolving deleted anchors, recompute repaired
ranges before publishing presence: after computing newRef/refToAnchor and
setting this.activeCell/this.activeCellAnchor, build newRanges using
rangeAnchorToRange with rowOrder/colOrder/this.dimension, update this.ranges and
refresh this.rangeAnchors from those repaired ranges (convert ranges back to
anchors), then call syncSelectionToPresence() (or updateSelection()) exactly
once to publish the repaired activeCell and ranges; also ensure the same
republish happens when newRef survives but ranges changed (i.e., always
recompute ranges, update this.rangeAnchors, and call syncSelectionToPresence()
once at the end).
- Around line 3159-3167: The new collapsed range is appended after calling
syncSelectionToPresence(), so peers don't see the Ctrl+click selection; move the
ranges update earlier: in the method (where selectionType, normalizeRefToAnchor,
activeCell, activeCellAnchor are set) call this.ranges.push([anchor, anchor])
immediately after computing anchor (and after setting
activeCell/activeCellAnchor if needed) and then call
this.syncSelectionToPresence(); ensure you keep the existing activeCellAnchor
logic (refToAnchor(...)) intact so presence sync includes the newly pushed
range.
In `@packages/sheets/src/store/memory.ts`:
- Around line 322-335: MemStore must implement the axis-order API instead of
stubbing it: replace the no-op/empty-array implementations of updateSelection,
getRowOrder, getColOrder, and ensureAxisOrder in memory.ts with real logic that
maintains and returns internal row/col id arrays (use the same field names used
in this file), import createWorksheetAxisId, and ensure these arrays are updated
in shiftCells() and moveCells() whenever rows/cols are inserted/moved so
Sheet.syncSelectionToPresence() and resolveAnchorsToRefs() see the authoritative
axis ordering; ensure updateSelection updates selection anchors against those
arrays and ensureAxisOrder grows the arrays to cover _minRows/_minCols.
In `@packages/sheets/test/workbook/anchor-conversion.test.ts`:
- Around line 171-215: Add a test case in
packages/sheets/test/workbook/anchor-conversion.test.ts that exercises the
"beyond-axis-order" branch of rangeToRangeAnchor by passing a Range whose
start/end row or column indices are outside the provided rowOrder/colOrder
(e.g., r or c values greater than the orders' lengths) and asserting that the
function returns null for the corresponding
startRowId/startColId/endRowId/endColId (verifying the ?? null fallback); locate
the tests in the existing describe('rangeToRangeAnchor') block and add scenarios
for rows beyond rowOrder, columns beyond colOrder, and mixed beyond-axis cases
to ensure the null-serialization path is covered.
---
Outside diff comments:
In `@packages/frontend/src/app/spreadsheet/yorkie-store.ts`:
- Around line 21-39: The import list in yorkie-store.ts is missing the MergeSpan
type which is still referenced by the merge snapshot handling (undo/redo merge
snapshots and usages of MergeSpan); restore the missing type by adding MergeSpan
to the grouped type import from "@wafflebase/sheets" alongside Store, Grid,
Cell, etc., so all references to MergeSpan type-check again.
🪄 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: a6dbd946-7103-489c-807d-0d1fbd78877a
📒 Files selected for processing (15)
docs/design/README.mddocs/design/sheets/axis-id-selection.mddocs/tasks/README.mddocs/tasks/active/20260414-axis-id-selection-todo.mdpackages/frontend/src/app/spreadsheet/yorkie-store.tspackages/frontend/src/types/users.tspackages/sheets/src/index.tspackages/sheets/src/model/workbook/anchor-conversion.tspackages/sheets/src/model/worksheet/sheet.tspackages/sheets/src/store/memory.tspackages/sheets/src/store/readonly.tspackages/sheets/src/store/store.tspackages/sheets/src/view/overlay.tspackages/sheets/src/view/worksheet.tspackages/sheets/test/workbook/anchor-conversion.test.ts
Comment on lines
+20
to
+29
| export function anchorToRef( | ||
| anchor: CellAnchor, | ||
| rowOrder: string[], | ||
| colOrder: string[], | ||
| ): Ref | null { | ||
| const r = rowOrder.indexOf(anchor.rowId); | ||
| const c = colOrder.indexOf(anchor.colId); | ||
| if (r === -1 || c === -1) return null; | ||
| return { r: r + 1, c: c + 1 }; | ||
| } |
There was a problem hiding this comment.
Avoid linear ID lookups in the render path.
anchorToRef() and rangeAnchorToRange() resolve IDs with indexOf() against rowOrder/colOrder. These helpers are used by peer rendering and hover hit-testing, so large sheets turn every frame into O(axis length) work per peer. Please resolve anchors against cached Map<string, number> indexes instead of raw arrays.
Also applies to: 42-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sheets/src/model/workbook/anchor-conversion.ts` around lines 20 -
29, anchorToRef and rangeAnchorToRange currently do linear lookups with
rowOrder.indexOf / colOrder.indexOf which causes O(n) work per call; change
these helpers to accept cached Maps (e.g., rowIndexMap: Map<string,number>,
colIndexMap: Map<string,number>) and use
map.get(anchor.rowId)/map.get(anchor.colId) to compute r and c (add +1 as
before), returning null if get() is undefined; update function signatures and
all call sites to pass the precomputed maps (and consider adding small wrapper
overloads that build maps once for legacy callers) so rendering and hit-testing
become O(1) per anchor.
Comment on lines
+50
to
+61
| const startR = anchor.startRowId ? rowOrder.indexOf(anchor.startRowId) + 1 : 1; | ||
| const startC = anchor.startColId ? colOrder.indexOf(anchor.startColId) + 1 : 1; | ||
| const endR = anchor.endRowId ? rowOrder.indexOf(anchor.endRowId) + 1 : maxRows; | ||
| const endC = anchor.endColId ? colOrder.indexOf(anchor.endColId) + 1 : maxCols; | ||
|
|
||
| // indexOf returns -1 → +1 = 0 means deleted | ||
| if (anchor.startRowId && startR === 0 && anchor.endRowId && endR === 0) return null; | ||
| if (anchor.startColId && startC === 0 && anchor.endColId && endC === 0) return null; | ||
|
|
||
| return [ | ||
| { r: Math.max(1, startR), c: Math.max(1, startC) }, | ||
| { r: Math.max(1, endR), c: Math.max(1, endC) }, |
There was a problem hiding this comment.
Normalize repaired ranges instead of clamping missing endpoints to 1.
If only one endpoint ID was deleted, indexOf() returns -1, the +1 becomes 0, and the current Math.max(1, …) fallback can produce inverted ranges like { r: 5 } -> { r: 1 }. That leaks an invalid selection into overlay/sheet code after a remote delete. This path needs a real deleted-endpoint repair strategy and a final range normalization step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sheets/src/model/workbook/anchor-conversion.ts` around lines 50 -
61, The current logic treats indexOf==-1 as 0 and then clamps to 1 via Math.max,
which produces inverted or leaked selections; instead detect deleted endpoints
(startR/startC/endR/endC === 0) and repair by replacing a single deleted
endpoint with the surviving endpoint (e.g., if startR===0 set startR = endR; if
endR===0 set endR = startR; same for startC/endC), then clamp both to [1,
maxRows/maxCols] and finally normalize the range so the start <= end (swap if
needed) before returning the two points; reference the variables
startR/startC/endR/endC,
anchor.startRowId/anchor.endRowId/anchor.startColId/anchor.endColId, Math.max
and maxRows/maxCols in your change.
Comment on lines
+4022
to
+4043
| const newRef = anchorToRef(this.activeCellAnchor, rowOrder, colOrder); | ||
| if (newRef) { | ||
| this.activeCell = this.normalizeRefToAnchor(newRef); | ||
| } else { | ||
| // Axis ID was deleted — snap to {r:1, c:1} | ||
| this.activeCell = { r: 1, c: 1 }; | ||
| const newAnchor = refToAnchor(this.activeCell, rowOrder, colOrder); | ||
| if (newAnchor) { | ||
| this.activeCellAnchor = newAnchor; | ||
| this.syncSelectionToPresence(); | ||
| } | ||
| } | ||
|
|
||
| // Re-resolve ranges | ||
| const newRanges: Range[] = []; | ||
| for (const anchor of this.rangeAnchors) { | ||
| const range = rangeAnchorToRange(anchor, rowOrder, colOrder, this.dimension); | ||
| if (range) { | ||
| newRanges.push(range); | ||
| } | ||
| } | ||
| this.ranges = newRanges; |
There was a problem hiding this comment.
Rebuild and republish the repaired selection once, after delete resolution.
In the deleted-anchor branch, syncSelectionToPresence() fires before newRanges is derived below, so peers receive the fallback active cell with stale range anchors. When the active cell survives but a range endpoint is repaired or dropped, this method never republishes the repaired ranges at all. Recompute the repaired ranges first, refresh rangeAnchors from them, then send one updateSelection().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sheets/src/model/worksheet/sheet.ts` around lines 4022 - 4043, When
resolving deleted anchors, recompute repaired ranges before publishing presence:
after computing newRef/refToAnchor and setting
this.activeCell/this.activeCellAnchor, build newRanges using rangeAnchorToRange
with rowOrder/colOrder/this.dimension, update this.ranges and refresh
this.rangeAnchors from those repaired ranges (convert ranges back to anchors),
then call syncSelectionToPresence() (or updateSelection()) exactly once to
publish the repaired activeCell and ranges; also ensure the same republish
happens when newRef survives but ranges changed (i.e., always recompute ranges,
update this.rangeAnchors, and call syncSelectionToPresence() once at the end).
Comment on lines
+322
to
335
| updateSelection(_activeCell: CellAnchor, _ranges: RangeAnchor[]): void { | ||
| // No-op for memory store | ||
| } | ||
|
|
||
| getRowOrder(): string[] { | ||
| return []; | ||
| } | ||
|
|
||
| getColOrder(): string[] { | ||
| return []; | ||
| } | ||
|
|
||
| ensureAxisOrder(_minRows: number, _minCols: number): void { | ||
| // No-op for memory store |
There was a problem hiding this comment.
MemStore can't leave the axis-order API as stubs anymore.
Sheet.syncSelectionToPresence() and resolveAnchorsToRefs() now depend on these methods to create and remap authoritative anchors. Returning [] and a no-op here means MemStore-backed sheets lose the new stable-selection behavior in tests/dev as soon as rows or columns are inserted or moved.
♻️ Suggested direction
+ private rowOrder: string[] = [];
+ private colOrder: string[] = [];
+
updateSelection(_activeCell: CellAnchor, _ranges: RangeAnchor[]): void {
// No-op for memory store
}
getRowOrder(): string[] {
- return [];
+ return [...this.rowOrder];
}
getColOrder(): string[] {
- return [];
+ return [...this.colOrder];
}
- ensureAxisOrder(_minRows: number, _minCols: number): void {
- // No-op for memory store
+ ensureAxisOrder(minRows: number, minCols: number): void {
+ while (this.rowOrder.length < minRows) {
+ this.rowOrder.push(createWorksheetAxisId('r'));
+ }
+ while (this.colOrder.length < minCols) {
+ this.colOrder.push(createWorksheetAxisId('c'));
+ }
}// Also add `createWorksheetAxisId` to the imports and keep these arrays in sync
// inside `shiftCells()` / `moveCells()`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sheets/src/store/memory.ts` around lines 322 - 335, MemStore must
implement the axis-order API instead of stubbing it: replace the
no-op/empty-array implementations of updateSelection, getRowOrder, getColOrder,
and ensureAxisOrder in memory.ts with real logic that maintains and returns
internal row/col id arrays (use the same field names used in this file), import
createWorksheetAxisId, and ensure these arrays are updated in shiftCells() and
moveCells() whenever rows/cols are inserted/moved so
Sheet.syncSelectionToPresence() and resolveAnchorsToRefs() see the authoritative
axis ordering; ensure updateSelection updates selection anchors against those
arrays and ensureAxisOrder grows the arrays to cover _minRows/_minCols.
- Emit legacy activeCell string alongside selection in presence so user-presence.tsx jump feature continues working - Initialize rowOrder/colOrder in ensureAxisOrder for legacy sheets - Fix addSelection sync ordering (push range before syncing) - Fix resolveAnchorsToRefs to recompute ranges before publishing - Snap deleted single endpoint to surviving endpoint to prevent inverted ranges - Add fence language tags to design doc - Add beyond-axis-order and single-endpoint-deleted test cases Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/sheets/src/model/worksheet/sheet.ts (1)
4034-4047:⚠️ Potential issue | 🟠 MajorRepublish repaired ranges even when the active cell survives.
rangeAnchorToRange()can snap or drop ranges after a delete, butLine 4045only syncs when the active-cell anchor is deleted. If only a range endpoint is repaired,this.rangeschanges locally whilethis.rangeAnchorsand remote presence stay stale.🐛 Suggested fix
// Re-resolve ranges before syncing so peers get the repaired state const newRanges: Range[] = []; for (const anchor of this.rangeAnchors) { const range = rangeAnchorToRange(anchor, rowOrder, colOrder, this.dimension); if (range) { newRanges.push(range); } } - this.ranges = newRanges; - - // Republish repaired selection if activeCell was deleted - if (!newRef) { + const repairedRangeAnchors = newRanges.map((range) => + rangeToRangeAnchor(range, rowOrder, colOrder, this.selectionType), + ); + const rangeAnchorsChanged = + repairedRangeAnchors.length !== this.rangeAnchors.length || + repairedRangeAnchors.some((anchor, i) => { + const prev = this.rangeAnchors[i]; + return ( + !prev || + prev.startRowId !== anchor.startRowId || + prev.startColId !== anchor.startColId || + prev.endRowId !== anchor.endRowId || + prev.endColId !== anchor.endColId + ); + }); + this.ranges = newRanges; + this.rangeAnchors = repairedRangeAnchors; + + if (!newRef || rangeAnchorsChanged) { this.syncSelectionToPresence(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheets/src/model/worksheet/sheet.ts` around lines 4034 - 4047, The code only calls syncSelectionToPresence() when newRef is falsy, but rangeAnchorToRange() can modify ranges even if activeCell survives; after rebuilding this.ranges from this.rangeAnchors you should detect when the repaired ranges differ (or just always republish) and call syncSelectionToPresence() so remote presence reflects the repaired ranges; update the block after assigning this.ranges (in the code that uses rangeAnchorToRange, this.rangeAnchors, this.ranges, newRef and syncSelectionToPresence) to trigger sync when ranges changed or unconditionally.
🤖 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/design/sheets/axis-id-selection.md`:
- Around line 141-156: The document describes a future fallback algorithm for
deleted-axis handling but the shipped behavior is different; update the text to
describe the actual implementation: when anchorToRef(anchor, rowOrder, colOrder)
returns null for an activeCell, the code falls back to { r: 1, c: 1 } and then
pushes presence, and for ranges rely on rangeAnchorToRange() to snap or drop
deleted endpoints (if an endpoint's axis ID is missing it will be resolved by
rangeAnchorToRange or the range will be dropped), so replace the
cached-previous-order/deletion-index algorithm with this shipped behavior and
cite the functions anchorToRef, activeCell, and rangeAnchorToRange in the
explanation.
- Around line 176-195: The Store contract docs are missing
ensureAxisOrder(maxRow: number, maxCol: number): void which
Sheet.syncSelectionToPresence() now requires; add this method to the documented
Store interface alongside updateSelection, getPresences, getRowOrder and
getColOrder, and note that callers (e.g., Sheet.syncSelectionToPresence) must
call store.ensureAxisOrder(maxRow, maxCol) before using refToAnchor() or
rangeToRangeAnchor() so implementations can guarantee axis mapping is available.
In `@packages/sheets/src/model/worksheet/sheet.ts`:
- Around line 2583-2619: The method syncSelectionToPresence currently returns
early if this.activeCellAnchor is null which prevents ensureAxisOrder() from
running and stops bootstrapping row/col IDs; change the initial guard to check
for this.activeCell instead (e.g., if (!this.activeCell) return;) so that
ensureAxisOrder(maxRow, maxCol) runs even when activeCellAnchor is not yet set,
then continue to compute rowOrder/colOrder, re-resolve this.activeCellAnchor
with refToAnchor(this.activeCell, rowOrder, colOrder), build rangeAnchors via
rangeToRangeAnchor, and call this.store.updateSelection as before; keep the rest
of syncSelectionToPresence (ensureAxisOrder, refToAnchor, rangeToRangeAnchor,
this.store.updateSelection) intact.
---
Duplicate comments:
In `@packages/sheets/src/model/worksheet/sheet.ts`:
- Around line 4034-4047: The code only calls syncSelectionToPresence() when
newRef is falsy, but rangeAnchorToRange() can modify ranges even if activeCell
survives; after rebuilding this.ranges from this.rangeAnchors you should detect
when the repaired ranges differ (or just always republish) and call
syncSelectionToPresence() so remote presence reflects the repaired ranges;
update the block after assigning this.ranges (in the code that uses
rangeAnchorToRange, this.rangeAnchors, this.ranges, newRef and
syncSelectionToPresence) to trigger sync when ranges changed or unconditionally.
🪄 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: fc3c8278-e453-4db9-9b13-ba4a66dd4ecc
📒 Files selected for processing (5)
docs/design/sheets/axis-id-selection.mdpackages/frontend/src/app/spreadsheet/yorkie-store.tspackages/sheets/src/model/workbook/anchor-conversion.tspackages/sheets/src/model/worksheet/sheet.tspackages/sheets/test/workbook/anchor-conversion.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/sheets/test/workbook/anchor-conversion.test.ts
- packages/sheets/src/model/workbook/anchor-conversion.ts
- Move early return in syncSelectionToPresence after ensureAxisOrder so first selection sync can bootstrap axis IDs - Update design doc: deleted-axis handling matches implementation, add ensureAxisOrder to Store interface section Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/design/sheets/axis-id-selection.md (1)
223-225:⚠️ Potential issue | 🟡 MinorUpdate this risk row to the shipped deleted-axis repair path.
This row still describes previous-order snapshot tracking, but the implementation now falls back to
{ r: 1, c: 1 }for a deleted active cell and letsrangeAnchorToRange()repair or drop ranges. Keeping the old mitigation here points readers to logic that no longer exists.📝 Suggested doc update
-| Deleted axis ID detection requires previous state | Complexity in tracking deletions | Cache previous `rowOrder` snapshot on each remote sync. Diff is cheap (array comparison). | +| Deleted axis IDs can invalidate active/range anchors | Selection may need fallback or repair after remote deletes | Fall back to `{ r: 1, c: 1 }` when `anchorToRef()` returns `null`, then republish selection; let `rangeAnchorToRange()` snap surviving endpoints or drop fully deleted ranges. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/sheets/axis-id-selection.md` around lines 223 - 225, The risk row still advises caching a previous rowOrder snapshot for deleted-axis detection but the shipped implementation instead falls back to an active cell of { r: 1, c: 1 } and relies on rangeAnchorToRange() to repair or drop ranges; update the row to describe the shipped "deleted-axis repair path" by replacing the mitigation text with a note that deleted active cells now default to { r: 1, c: 1 } and that rangeAnchorToRange() performs repair/drop logic (so previous-order snapshot tracking is no longer used).packages/sheets/src/model/worksheet/sheet.ts (1)
4033-4046:⚠️ Potential issue | 🟠 MajorRepublish repaired ranges even when the active cell survives.
newRangesis rebuilt from anchor state, but this branch only syncs when the active-cell anchor was deleted. If a row/column delete repairs or drops a range whilenewRefstill resolves,this.rangeschanges locally butthis.rangeAnchorsand Yorkie presence stay stale.🐛 Suggested fix
this.ranges = newRanges; - // Republish repaired selection if activeCell was deleted - if (!newRef) { - this.syncSelectionToPresence(); - } + // Republish repaired selection once after both activeCell and ranges + // are resolved. If you want to avoid redundant writes, guard this with + // an explicit "selection changed" check instead. + this.syncSelectionToPresence();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheets/src/model/worksheet/sheet.ts` around lines 4033 - 4046, After rebuilding this.ranges from rangeAnchors (using rangeAnchorToRange) you must republish repaired ranges to presence even when newRef still resolves; change the logic so that after assigning this.ranges = newRanges you always call the presence-syncing routine (e.g., this.syncSelectionToPresence()) instead of only when !newRef so that Yorkie presence and this.rangeAnchors stay in sync when ranges are repaired by row/column edits.
🤖 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/worksheet/sheet.ts`:
- Around line 2583-2618: The selection state (activeCell, ranges, selectionType)
must be updated atomically and then published via
syncSelectionToPresence/updateSelection; ensure all mutating flows
(syncSelectionToPresence, setActiveCell, selectRowRange, selectColumnRange,
selectAll, resizeRange, autofill, mergeSelection, unmergeSelection) route their
final assignment of this.activeCell, this.ranges and this.selectionType through
a single commit point that calls syncSelectionToPresence (or a new private
commitSelectionState method) after all local mutations and axis-extension logic
complete; locate and replace any code that updates
this.ranges/this.activeCell/this.selectionType and then later calls
syncSelectionToPresence (or bypasses it) so that the authoritative values are
set first and only one syncSelectionToPresence/updateSelection invocation
happens at the end.
---
Duplicate comments:
In `@docs/design/sheets/axis-id-selection.md`:
- Around line 223-225: The risk row still advises caching a previous rowOrder
snapshot for deleted-axis detection but the shipped implementation instead falls
back to an active cell of { r: 1, c: 1 } and relies on rangeAnchorToRange() to
repair or drop ranges; update the row to describe the shipped "deleted-axis
repair path" by replacing the mitigation text with a note that deleted active
cells now default to { r: 1, c: 1 } and that rangeAnchorToRange() performs
repair/drop logic (so previous-order snapshot tracking is no longer used).
In `@packages/sheets/src/model/worksheet/sheet.ts`:
- Around line 4033-4046: After rebuilding this.ranges from rangeAnchors (using
rangeAnchorToRange) you must republish repaired ranges to presence even when
newRef still resolves; change the logic so that after assigning this.ranges =
newRanges you always call the presence-syncing routine (e.g.,
this.syncSelectionToPresence()) instead of only when !newRef so that Yorkie
presence and this.rangeAnchors stay in sync when ranges are repaired by
row/column edits.
🪄 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: 33489db0-f639-47b7-b5fd-fed311a4119a
📒 Files selected for processing (2)
docs/design/sheets/axis-id-selection.mdpackages/sheets/src/model/worksheet/sheet.ts
Comment on lines
+2583
to
+2618
| private syncSelectionToPresence(): void { | ||
| // Ensure axis orders cover the activeCell and the non-null axes of | ||
| // the selection. For column selection only cols need extending (not | ||
| // all 100 rows); for row selection only rows. | ||
| const minRow = this.activeCell.r; | ||
| const minCol = this.activeCell.c; | ||
| let maxRow = minRow; | ||
| let maxCol = minCol; | ||
| const isRow = this.selectionType === 'row'; | ||
| const isCol = this.selectionType === 'column'; | ||
| const isAll = this.selectionType === 'all'; | ||
| for (const [start, end] of this.ranges) { | ||
| if (!isCol && !isAll) { | ||
| maxRow = Math.max(maxRow, start.r, end.r); | ||
| } | ||
| if (!isRow && !isAll) { | ||
| maxCol = Math.max(maxCol, start.c, end.c); | ||
| } | ||
| } | ||
| this.store.ensureAxisOrder(maxRow, maxCol); | ||
|
|
||
| const rowOrder = this.store.getRowOrder(); | ||
| const colOrder = this.store.getColOrder(); | ||
|
|
||
| // Resolve activeCell anchor (bootstrap on first call or after axis extension) | ||
| const freshAnchor = refToAnchor(this.activeCell, rowOrder, colOrder); | ||
| if (freshAnchor) { | ||
| this.activeCellAnchor = freshAnchor; | ||
| } | ||
| if (!this.activeCellAnchor) return; | ||
|
|
||
| const rangeAnchors = this.ranges.map((range) => | ||
| rangeToRangeAnchor(range, rowOrder, colOrder, this.selectionType), | ||
| ); | ||
| this.rangeAnchors = rangeAnchors; | ||
| this.store.updateSelection(this.activeCellAnchor, rangeAnchors); |
There was a problem hiding this comment.
Make selection updates atomic before publishing anchors.
syncSelectionToPresence() / setActiveCell() now define the authoritative anchor state, but several same-file flows still change this.ranges after that point or never come back through it (selectRowRange(), selectColumnRange(), selectAll(), resizeRange(), autofill(), mergeSelection(), unmergeSelection()). After one of those paths, the next structural re-resolve will rebuild from stale rangeAnchors, and peers will keep the previous selection. Please route the final activeCell / ranges / selectionType update through one commit path that syncs once at the end.
Also applies to: 2631-2640
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sheets/src/model/worksheet/sheet.ts` around lines 2583 - 2618, The
selection state (activeCell, ranges, selectionType) must be updated atomically
and then published via syncSelectionToPresence/updateSelection; ensure all
mutating flows (syncSelectionToPresence, setActiveCell, selectRowRange,
selectColumnRange, selectAll, resizeRange, autofill, mergeSelection,
unmergeSelection) route their final assignment of this.activeCell, this.ranges
and this.selectionType through a single commit point that calls
syncSelectionToPresence (or a new private commitSelectionState method) after all
local mutations and axis-extension logic complete; locate and replace any code
that updates this.ranges/this.activeCell/this.selectionType and then later calls
syncSelectionToPresence (or bypasses it) so that the authoritative values are
set first and only one syncSelectionToPresence/updateSelection invocation
happens at the end.
selectRowRange, selectColumnRange, selectAll, resizeRange, autofill, mergeSelection, and unmergeSelection all modify this.ranges without syncing to presence. Peers would see stale selection until the next setActiveCell call. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Merged
hackerwins
added a commit
that referenced
this pull request
Apr 19, 2026
This was referenced May 7, 2026
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
Ref,Sref) in Yorkie presence with stable axis ID references (CellAnchor,RangeAnchor) so selections automatically track cells across remote row/column inserts and deletesKey changes
New types & conversion layer (
anchor-conversion.ts):CellAnchor { rowId, colId }— stable cell reference using axis IDsRangeAnchor— stable range reference, null fields = entire row/column/allanchorToRef/refToAnchor— convert between axis IDs and visualRefStore interface:
updateActiveCell(Ref)→updateSelection(CellAnchor, RangeAnchor[])getRowOrder(),getColOrder(),ensureAxisOrder()Sheet engine:
activeCellAnchor/rangeAnchorsas authoritative selection stateresolveAnchorsToRefs()re-maps anchors after remote structural changesshiftCells()Overlay:
Test plan
pnpm verify:fastpasses🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests