feat(sheet): live presence & live calculation for the collaborative spreadsheet#312
Closed
SamTV12345 wants to merge 10 commits into
Closed
feat(sheet): live presence & live calculation for the collaborative spreadsheet#312SamTV12345 wants to merge 10 commits into
SamTV12345 wants to merge 10 commits into
Conversation
Spec for Google-Docs-style collaboration on the spreadsheet: - active-cell cursor presence with author color + name label - live broadcast of in-progress cell input + live recompute of dependent cells before commit ponytail-trimmed: one new SHEET_PRESENCE frame each direction; cleanup reuses existing USER_LEAVE and NEW_SHEET_OP.author; no server-side presence cache / snapshot / heartbeat in v1 (upgrade paths documented). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5 TDD tasks: server relay (Go) + presence reducer/overlay (vitest) + grid decorations + editor wiring + two-session Playwright E2E. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n deadlock) vite 8 is rolldown-based and handles CommonJS natively. The legacy @rollup/plugin-commonjs plugin deadlocks the rolldown transform when bundling HyperFormula, so `vite build --mode sheet` hung indefinitely and the sheet bundle never built. Skip the plugin for the sheet entry only; other entries are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a Playwright spec covering: remote cursor tag, live formula text on the viewer, dependent-cell live recompute (C2=31) before commit, result after commit (B2=30), and cursor cleanup on disconnect. Fix found by the e2e: render the presence name tag via a CSS ::after pseudo-element (data-label) instead of a text node, so a peer's name never leaks into the cell's textContent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Final-review findings: - Honor data.readonly on the client: read-only viewers get non-editable cells, so no live-edit/commit frames originate locally (server already strips/rejects them). Closes the plan's "enforced on both client and server" gap and the type-then-revert UX bug. - Add a direct unit test for the load-bearing effectiveCells property: a remote live raw replaces (not duplicates) a committed cell at the same coordinate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
PR Summary by QodoAdd sheet live cursor presence and pre-commit formula recalculation Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
1. Presence hits rate limiter
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+49
to
+58
| // Live-edit throttle (trailing, ~60ms) so typing does not flood the socket. | ||
| let liveTimer: ReturnType<typeof setTimeout> | null = null; | ||
| let lastLive: { row: number; col: number; raw: string } | null = null; | ||
| const sendLiveEdit = (row: number, col: number, raw: string): void => { | ||
| lastLive = { row, col, raw }; | ||
| if (liveTimer) return; | ||
| liveTimer = setTimeout(() => { | ||
| liveTimer = null; | ||
| if (lastLive) sendPresence(lastLive.row, lastLive.col, true, lastLive.raw); | ||
| }, 60); |
There was a problem hiding this comment.
1. Presence hits rate limiter 🐞 Bug ☼ Reliability
Client.readPump applies CommitRateLimiting to all websocket messages, but the new live-edit presence loop can emit ~16 msgs/sec (60ms throttle), exceeding the default 10/sec and causing SHEET_PRESENCE and potentially nearby SHEET_OP messages to be dropped. This can intermittently break live presence fidelity and (in timing windows) reject a commit sent shortly after a burst of presence frames.
Agent Prompt
### Issue description
The websocket `CommitRateLimiting` check is applied before message decoding, so `SHEET_PRESENCE` frames count toward the same limiter as commits. With the new 60ms trailing live-edit throttle, a normal typing burst can exceed the default 10 events/sec, causing presence and potentially subsequent sheet ops to be silently dropped.
### Issue Context
`SHEET_PRESENCE` is intentionally high-frequency/ephemeral traffic; it should not compete with commit reliability.
### Fix Focus Areas
- lib/ws/client.go[132-191]
- lib/settings/configRegistry.go[241-250]
- ui/src/js/sheet/sheetEditor.ts[42-73]
### Suggested fix
1. Parse enough of the incoming JSON envelope to identify `SHEET_PRESENCE` early.
2. Apply a separate limiter for presence (higher points/sec) or skip `CommitRateLimiting` for `SHEET_PRESENCE` and instead apply:
- a presence-specific msg/sec limit, and
- a payload-size cap (see other finding) to prevent abuse.
3. Keep `CommitRateLimiting` for write-bearing messages (`USER_CHANGES`, `SHEET_OP`, etc.).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+88
to
92
| // Read-only viewers get non-editable cells: with no typing, no live-edit | ||
| // or commit frames originate from the client (the server strips them too). | ||
| td.contentEditable = opts.readOnly ? 'false' : 'true'; | ||
| this.attach(td, r, c); | ||
| tr.appendChild(td); |
There was a problem hiding this comment.
2. Read-only cursor broken 🐞 Bug ≡ Correctness
Read-only mode sets td.contentEditable = 'false', but cursor presence is driven by the cell focus event; non-focusable <td> elements will not reliably receive focus on click, so onSelect (and thus cursor presence) won’t fire for read-only users. This contradicts the server behavior that explicitly allows read-only sessions to relay cursor moves while stripping live edits.
Agent Prompt
### Issue description
In read-only mode, cells are made non-editable via `contentEditable='false'`, but selection/cursor presence is wired to `focus` on the `<td>`. If the cell cannot be focused, read-only users cannot move their cursor (no `onSelect`), even though the server supports read-only cursor movement.
### Issue Context
The server-side presence relay strips `editing/raw` for read-only but keeps row/col, meaning read-only cursor movement is a supported behavior.
### Fix Focus Areas
- ui/src/js/sheet/sheetView.ts[86-148]
- ui/src/js/sheet/sheetEditor.ts[114-138]
- lib/ws/SheetHandler.go[159-165]
### Suggested fix
Implement read-only as “selectable but not editable”:
- Keep the cell focusable (e.g., `td.tabIndex = 0` and/or handle `click` to select) and continue emitting `onSelect`.
- Prevent edits without relying solely on `contentEditable=false`:
- Use `beforeinput` handler to `preventDefault()` when `opts.readOnly`,
- Ignore/suppress `onLiveEdit`/`onEdit` when read-only.
This preserves cursor presence while preventing typing/commits from the client.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+184
to
+190
| } else if strings.Contains(decodedMessage, "SHEET_PRESENCE") { | ||
| var presence ws.SheetPresenceIncoming | ||
| if err := json.Unmarshal(message, &presence); err != nil { | ||
| logger.Error("Error unmarshalling SHEET_PRESENCE: ", err) | ||
| continue | ||
| } | ||
| c.Handler.HandlePresence(c, presence) |
There was a problem hiding this comment.
3. Substring routing misroutes frames 🐞 Bug ≡ Correctness
Websocket message routing uses strings.Contains(decodedMessage, ...) on the full JSON, but SHEET_PRESENCE includes user-controlled raw; if raw contains tokens like SHEET_OP or CLIENT_READY, the message can be routed to the wrong branch and dropped or handled incorrectly. This makes presence unreliable for certain inputs and is abusable by crafted clients.
Agent Prompt
### Issue description
`Client.readPump` routes by substring search over the entire JSON payload. Because presence frames embed arbitrary user text (`raw`), normal cell contents can contain routing substrings and trigger the wrong handler.
### Issue Context
Presence frames are high-frequency and carry arbitrary `raw` strings. Substring routing is therefore not safe.
### Fix Focus Areas
- lib/ws/client.go[132-210]
- lib/models/ws/clientReady.go[1-27]
- lib/models/ws/sheetMessages.go[1-99]
### Suggested fix
Replace `strings.Contains` routing with JSON field-based routing:
1. Unmarshal into a small envelope:
- `event`, and
- `data.type` + `data.component` + `data.data` as `json.RawMessage`.
2. If `data.type == 'CLIENT_READY'` handle client-ready.
3. Else if `data.type == 'COLLABROOM'`, unmarshal inner `data.data` just enough to read `type` (`SHEET_OP`, `SHEET_PRESENCE`, etc.) and switch on that.
This avoids misrouting due to user-controlled content inside other fields.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
|
Superseded: the feature already landed in main via #311 (auto-merged), so this branch conflicts. The remaining fixes (rolldown build fix, tag-pollution ::after fix, client-side read-only, E2E + collision test) are delivered cleanly on top of main in the follow-up PR. |
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.
What
Makes the collaborative spreadsheet feel like Google Docs:
Finishes the presence that the original spreadsheet design (
docs/superpowers/specs/2026-06-21-collaborative-spreadsheet-design.md§5) envisioned but deferred, and goes one step further with live pre‑commit calculation.How (ephemeral, minimal)
One new
SHEET_PRESENCEwebsocket frame each direction — never persisted, never routed through the OT serialization goroutine.lib/ws/SheetHandler.go,lib/models/ws/sheetMessages.go,lib/ws/client.go): a direct relay that stamps identity (userId/name/color) server‑side fromsession.Author— the incoming frame carries no identity fields, so spoofing is structurally impossible. Read‑only sessions haveediting/rawstripped.USER_LEAVE(disconnect) drops a peer's cursor + live edit;NEW_SHEET_OP.author(commit) clears the author's overlay flicker‑free. No new server state, no snapshot/heartbeat in v1.ui/src/js/sheet/): a smallSheetPresencereducer + a pureeffectiveCellsoverlay that layers remote in‑progress raws onto the formula engine's grid so dependents recompute deterministically on each viewer.rawstays the single source of truth;valueis derived locally. Live‑edit is throttled (~60 ms), selection debounced (~50 ms).Build fix included
vite build --mode sheethung indefinitely — vite 8 is rolldown‑based and handles CommonJS natively, but the legacy@rollup/plugin-commonjsplugin deadlocked the rolldown transform while bundling HyperFormula, so the sheet bundle had never built. Skipped that plugin for the sheet entry only; other bundles are unchanged.Testing
lib/ws): relay with server‑stamped identity, sender exclusion, read‑only stripping, unknown‑author best‑effort relay.effectiveCellsoverlay drives a real HyperFormula recompute (C2 = 31) and wins over a committed cell on collision.playwright/specs/sheet_presence.spec.ts): two sessions — remote cursor tag, live formula text, dependentC2 = 31before Enter,B2 = 30after commit, cursor cleanup on disconnect. ✅ passing.Deliberately out of scope (v1)
Join‑snapshot of idle cursors, heartbeat/TTL, multi‑sheet‑tab rendering, range selections — all documented with upgrade paths in
docs/superpowers/specs/2026-06-25-sheet-live-presence-design.md§8. The text‑pad path is untouched.🤖 Generated with Claude Code