Repository navigation
Sheets themselves cross to the other people on the board - #34
Merged
Merged
Conversation
PR 1 addressed ops to a sheet so two editors on different sheets would
stop writing into each other's canvas. It left the sheets themselves
local: adding, renaming, duplicating or removing one happened on your
screen and nowhere else, and the other people found out when they next
reloaded.
So sheets grow the seam the store has had all along. `on` says "draw
again", which is what a tab strip wants; `onChanges` says *what happened*,
in three words a wire can carry:
{ t:'sheet-add', id, name, index, order, objects }
{ t:'sheet-rename', id, name }
{ t:'sheet-del', id }
`sheet-add` carries the contents, which is what makes duplicating a sheet
one message rather than a message plus every object on it — and carries
the ids, so the copy is the same copy on every screen. There is no
`sheet-order`: nothing reorders sheets yet, and a vocabulary with a word
nothing says is a word that will be wrong when something does.
Everything is written to survive arriving twice or arriving for something
that is not here, because a channel resends and a client can be told
about a sheet it already has. The last sheet cannot be deleted by anyone,
including a remote — that invariant belongs to the board rather than to
whoever pressed Delete.
A sheet somebody else makes does *not* become the sheet on your screen.
Being moved off your work because a colleague pressed `+` is how a
collaborative tool becomes unusable. The one change that does move you is
somebody deleting the sheet you are looking at, and only because there is
then nothing else to show.
That last case is why `app.js` stopped wrapping the sheet commands and
subscribed instead: a switch is no longer always this person's doing, and
a subscription cannot be forgotten by a command added later.
Also here, and not about sheets: `the first look at a workspace marks
nothing` has been failing about one run in five, on CI and now locally.
`settle()` answers once the shell is up with nothing loading, and the
account gate renders inside the shell — so the previous test's list can
still be in flight, and the record it seeds lands after this test clears
storage. Then the workspace is not a first look, both planted boards are
news, and the run that catches it is whichever one was slow. It reloads
before clearing now, which unloads the document that would have written.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GJ9tdqJPUktg2XR3M7Ncqr
📝 WalkthroughWalkthroughSheet changes now use structured local and remote events. The sheet model validates and applies these events after hydration. Synchronization transports sheet events during and after hydration. The application updates viewport state through active-sheet subscriptions. ChangesSheet synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LocalSheets
participant BoardSync
participant SHEET_EVENT
participant RemoteSheets
participant AppViewport
LocalSheets->>BoardSync: emit LOCAL sheet change
BoardSync->>SHEET_EVENT: broadcast sheet payload
SHEET_EVENT->>RemoteSheets: deliver sheet change
RemoteSheets->>RemoteSheets: applyChanges
RemoteSheets->>AppViewport: notify active-sheet change
AppViewport->>AppViewport: restore camera or fit viewport
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (3)
test/node/sync.test.js (2)
391-395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the renamed sheet by ID.
The predicate succeeds if a different sheet receives the name
Discovery. Matchmadebefore accepting the remote rename. The remote change contract identifies the target sheet by ID.Proposed test change
await waitFor( - () => his.sheets.list().some((s) => s.name === 'Discovery'), + () => his.sheets.list().find((s) => s.id === made)?.name === 'Discovery', 'the rename to reach Bob', );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/node/sync.test.js` around lines 391 - 395, Update the waitFor predicate in the rename synchronization test to require both the expected sheet ID from made and the name Discovery, rather than matching any sheet by name alone. Preserve the existing remote rename assertion and wait message.
397-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest deletion of the active sheet.
Bob remains on the initial sheet here. This only tests removal of a non-active sheet. Select
madebefore the remote deletion, then assert thatactiveIdchanges to the remaining sheet. The PR objective requires this fallback behavior.Proposed test change
+his.sheets.select(made); hers.sheets.remove(made); await waitFor(() => !his.sheets.has(made), 'the removal to reach Bob'); assert.equal(his.sheets.size, 1); +assert.equal(his.sheets.activeId, his.sheets.list()[0].id);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/node/sync.test.js` around lines 397 - 399, Update the deletion scenario around hers.sheets.remove(made) to have Bob select made before Alice removes it, then wait for the remote removal and assert Bob’s activeId falls back to the remaining sheet. Retain the existing sheet-count assertion while verifying the active-sheet fallback behavior.src/core/sheets.js (1)
424-432: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local
atbinding to avoid shadowing theat(id)helper.Line 426 declares
const atas a number inside thesheet-addblock. The closure also definesat = (id) => entries.findIndex(...)at line 242. The current code is correct, becausedropresolves the helper from the outer scope. A future edit that callsat(change.id)inside this block would throw. Use a distinct name.♻️ Proposed rename
if (change.t === 'sheet-add') { if (find(change.id)) continue; - const at = Math.min(Math.max(change.index, 0), entries.length); + const index = Math.min(Math.max(change.index, 0), entries.length); entries.splice(at, 0, { id: change.id, - name: asName(change.name, `Sheet ${at + 1}`), + name: asName(change.name, `Sheet ${index + 1}`), state: asState({ order: change.order, objects: change.objects }), });Also update the
entries.splice(at, ...)call toentries.splice(index, ...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/sheets.js` around lines 424 - 432, Rename the local numeric binding `at` in the `sheet-add` branch to `index`, and update the corresponding `entries.splice(at, ...)` call to use `index`; leave the outer `at(id)` helper unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/platform/sync.js`:
- Around line 190-205: Update the sheets.onChanges handler in
src/platform/sync.js (lines 190-205) to queue local changes while holding
instead of discarding them, and ensure release() replays the queued SHEET_EVENT
through the existing outgoing path. Strengthen test/node/sync-hydration.test.js
(lines 381-397) after landed() and settle() to assert that the SHEET_EVENT for
the Early sheet was sent.
---
Nitpick comments:
In `@src/core/sheets.js`:
- Around line 424-432: Rename the local numeric binding `at` in the `sheet-add`
branch to `index`, and update the corresponding `entries.splice(at, ...)` call
to use `index`; leave the outer `at(id)` helper unchanged.
In `@test/node/sync.test.js`:
- Around line 391-395: Update the waitFor predicate in the rename
synchronization test to require both the expected sheet ID from made and the
name Discovery, rather than matching any sheet by name alone. Preserve the
existing remote rename assertion and wait message.
- Around line 397-399: Update the deletion scenario around
hers.sheets.remove(made) to have Bob select made before Alice removes it, then
wait for the remote removal and assert Bob’s activeId falls back to the
remaining sheet. Retain the existing sheet-count assertion while verifying the
active-sheet fallback behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1549d7f6-ad68-47d9-b486-729c6d23965b
📒 Files selected for processing (9)
src/app.jssrc/core/sheets.jssrc/platform/sync.jstest/browser/shell.test.jstest/node/sheets.test.jstest/node/sync-cursors.test.jstest/node/sync-hydration.test.jstest/node/sync-writer.test.jstest/node/sync.test.js
Review found that a sheet made during hydration was never sent: the `holding` guard dropped the change instead of queueing it, and the test asserted only that nothing went out before the board landed — which a permanent discard satisfies just as well. Another test that could not fail. The half it did not find is worse. `sheets.load` replaces every entry, so that sheet was wiped locally when the board arrived. Sending it would have announced a sheet this client no longer had, which is why queueing the change on its own would have made things worse rather than better. Both come from the same place: the sheets on screen before the board lands are a placeholder, and a sheet added to a placeholder is a sheet added to nothing. So the set of sheets is fixed until `hydrate` says the board is here — `sheets.ready()`, called on both branches, the loaded one and the seeded one. The strip offers no `+` and no menu until then, which on any normal load nobody ever sees. What can still be done in that window is what could always be done in it: draw on the canvas. Those ops are replayed onto the board when it arrives, which works precisely because there is exactly one canvas to replay them onto — the case this now guarantees. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GJ9tdqJPUktg2XR3M7Ncqr
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/SheetTabs.jsx (1)
129-151: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate the double-click rename path as well.
The new condition hides the menu until
sheets.settled, but the tab’sonDoubleClickhandler at Line 124 still opens the rename dialog before readiness.commands.renameSheetthen returnsfalsebecausesrc/core/sheets.jsrejects unsettled renames. The user can open a dialog that cannot save.Apply the same readiness gate to the double-click handler or disable tab actions until the board is ready. The PR objective requires sheet controls to remain unavailable during hydration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/SheetTabs.jsx` around lines 129 - 151, Gate the tab’s onDoubleClick rename handler with the same sheets.settled readiness condition used by the “more” menu, preventing the rename dialog from opening during hydration. Keep rename behavior unchanged once the board is settled, and ensure sheet controls remain unavailable until readiness.src/core/sheets.js (1)
229-240: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake concurrent sheet changes converge.
applyChangesdoes not commute concurrent changes. Concurrent deletes can leave each client with a different surviving sheet. Concurrent adds at index1can leave clients with different sheet orders. Becausebroadcast.selfisfalse, transport ordering cannot repair these optimistic states. Add deterministic conflict resolution or authoritative reconciliation, with model tests for concurrentsheet-addandsheet-delevents.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/sheets.js` around lines 229 - 240, Update the sheet-change application flow, especially applyChanges and the sheet-add/sheet-del handling around drop, so concurrent operations converge deterministically on every client rather than relying on transport order. Use a stable conflict-resolution or authoritative-reconciliation strategy for concurrent deletes and same-index adds, preserve active-sheet behavior, and add model tests covering concurrent sheet-add and sheet-del events.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/SheetTabs.jsx`:
- Around line 129-151: Gate the tab’s onDoubleClick rename handler with the same
sheets.settled readiness condition used by the “more” menu, preventing the
rename dialog from opening during hydration. Keep rename behavior unchanged once
the board is settled, and ensure sheet controls remain unavailable until
readiness.
In `@src/core/sheets.js`:
- Around line 229-240: Update the sheet-change application flow, especially
applyChanges and the sheet-add/sheet-del handling around drop, so concurrent
operations converge deterministically on every client rather than relying on
transport order. Use a stable conflict-resolution or
authoritative-reconciliation strategy for concurrent deletes and same-index
adds, preserve active-sheet behavior, and add model tests covering concurrent
sheet-add and sheet-del events.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9f2cbc7-11d8-4e68-8fbe-2c9b66730b3a
📒 Files selected for processing (9)
src/app.jssrc/components/SheetTabs.jsxsrc/core/sheets.jssrc/platform/sync.jstest/node/sheets.test.jstest/node/sync-cursors.test.jstest/node/sync-hydration.test.jstest/node/sync-writer.test.jstest/node/sync.test.js
🚧 Files skipped from review as they are similar to previous changes (7)
- test/node/sync-cursors.test.js
- src/app.js
- test/node/sync-writer.test.js
- test/node/sheets.test.js
- src/platform/sync.js
- test/node/sync.test.js
- test/node/sync-hydration.test.js
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
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.
Second of three. #33 addressed ops to a sheet, so two editors on different sheets stopped writing into each other's canvas. It left the sheets themselves local: adding, renaming, duplicating or removing one happened on your screen and nowhere else, and everyone else found out when they next reloaded. This is that half.
The vocabulary
Sheets grow the seam the store has had all along.
onsays "draw again", which is what a tab strip wants;onChangessays what happened, in three words a wire can carry:sheet-addcarries the contents, which makes duplicating a sheet one message rather than a message plus every object on it — and carries the ids, so the copy is the same copy on every screen.There is deliberately no
sheet-order: nothing reorders sheets yet, and a vocabulary with a word nothing says is a word that will be wrong when something finally does.Its own broadcast event (
sheet) rather than moreops, because it is a change to the board rather than to a canvas — stretching the op vocabulary to cover the set of sheets would leaveisOpanswering for two different kinds of thing. Held in the same queue as the ops before hydration, though, because the order between them is the point: a sheet that is added and then drawn on is two messages, and the second means nothing until the first has landed.What it refuses to do
+is how a collaborative tool becomes unusable. The one change that does move you is somebody deleting the sheet you are looking at, and only because there is then nothing else to show.app.jsstopped wrapping the sheet commands and subscribed instead: a switch is no longer always this person's doing, and a subscription cannot be forgotten by a command added later.The test I should have written last time
#33 shipped with a bug that only CI could catch — two clients deriving different ids for the same sheet, so every message crossed addressed to nothing. The two-client suite runs against real Realtime and needs a local Supabase, which this machine cannot run.
So the same claim is pinned where it can be checked anywhere:
what one client announces is what another appliesbuilds two sheet models from the same board, feeds one's announcements to the other, and asserts they end up identical — sheets, names, and the ids of a duplicated sheet's objects. I broke id agreement on purpose and watched it fail.The real-Realtime tests are here too, in
sync.test.js: a sheet crossing, a rename and a removal crossing, an edit on one sheet staying off another, and a duplicate arriving with the same object ids. Those first execute in CI.One flake, fixed
the first look at a workspace marks nothinghas been failing about one run in five — twice on #33's CI, and finally on this machine, which is what made it findable.settle()answers once the shell is up with nothing loading, and the account gate renders inside the shell. So the previous test's list can still be in flight when it returns, and the seen-record that list seeds lands after this test clears storage. The workspace is then not a first look, both planted boards are news, and the run that catches it is whichever one was slow enough.It reloads before clearing now, which unloads the document that would have written. Three clean full runs since.
Not fixed here, and worth its own change:
session.sendin the CDP helper has no timeout, so an evaluate that never returns hangs the run for ever. That has happened twice in one sitting, in two different files, and on CI it would burn the job until the workflow timeout.Tests
658 passing, line coverage 96.0%.
core/sheets.jsat 100%.🤖 Generated with Claude Code
https://claude.ai/code/session_01GJ9tdqJPUktg2XR3M7Ncqr
Summary by CodeRabbit
New Features
Bug Fixes
Tests