Display remote peer cursors with name labels in docs editor#76
Conversation
Spec for displaying peer cursors with name labels in the collaborative docs editor. Presence stays in the frontend layer (not DocStore), peer cursor data passed as render parameters. Phase 1 covers caret + label; Phase 2 (planned) covers selection highlight. Co-Authored-By: Claude Opus 4.6 <[email protected]>
6 tasks: DocsPresence type, shared position utility + drawing, Canvas/editor integration, YorkieDocStore presence, DocsView wiring with label visibility + hover detection, and manual testing. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Extract coordinate calculation from Cursor.getPixelPosition() into resolvePositionPixel() in peer-cursor.ts so both the local cursor and future remote peer cursors share the same logic. Add drawPeerCaret and drawPeerLabel for rendering remote cursors on the docs canvas. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Wire PeerCursor types and resolvePositionPixel/drawPeerCaret/drawPeerLabel into DocCanvas.render() and the editor's paint loop, adding setPeerCursors, onCursorMove, and getPeerCursorPixels to EditorAPI. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Address code quality review findings: - Use editorRef.current in setTimeout to avoid stale editor reference - Add trailing-edge timer to cursor throttle so final position is broadcast - Read theme from ref instead of closure to prevent stale theme in callbacks - Wrap decodeURIComponent in try-catch for malformed percent-encoded usernames
Pass stackIndex through to drawPeerLabel so multiple peer labels at the same cursor position stack vertically instead of overlapping. Use the exported DocsPresence type in YorkieDocStore.getPresences() instead of an inline type definition.
When a remote peer deletes a block that the local selection still references, findIndex returns -1 and layout.blocks[-1] is undefined. Add early-return guards in getSelectionRects() and getSelectedText() so the selection gracefully returns empty results instead of crashing.
Remove unused isFirstLine variable in layout.ts, add missing textIndent/marginLeft to test BlockStyle fixtures, update design doc target version and add entry to README index.
|
Warning Rate limit exceeded
⌛ 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 (4)
📝 WalkthroughWalkthroughThis PR implements collaborative remote peer cursors in the docs editor, enabling users to see other peers' cursor positions as animated carets with username labels. It introduces shared peer-cursor rendering utilities, integrates presence updates with throttled cursor position synchronization, and wires the frontend presence layer to display computed peer cursor pixels on canvas. Changes
Sequence Diagram(s)sequenceDiagram
participant User1 as User 1 (Local)
participant DocsView as DocsView
participant Editor as EditorAPI
participant Store as YorkieDocStore
participant Yorkie as Yorkie Sync
participant User2Sync as User 2 Presence
participant User2View as User 2 DocsView
rect rgba(100, 200, 150, 0.5)
note over User1,Store: Local Cursor Movement & Sync
User1->>DocsView: moves cursor (onCursorMove event)
DocsView->>DocsView: throttle (~100ms)
DocsView->>Store: updateCursorPos(blockId, offset)
Store->>Yorkie: doc.update() set presence.activeCursorPos
Yorkie->>Yorkie: broadcast to peers
end
rect rgba(150, 180, 220, 0.5)
note over User2Sync,User2View: Remote Presence & Rendering
Yorkie->>User2Sync: others-changed event
User2Sync->>User2View: presence update received
User2View->>Store: getPresences()
Store-->>User2View: array of peer presences
User2View->>User2View: buildPeerCursors() + updateLabel timers
User2View->>Editor: setPeerCursors([peer1, peer2, ...])
Editor->>Editor: resolvePositionPixel() for each peer
Editor->>Editor: compute stacking & label visibility
Editor->>Editor: paint() docCanvas.render(peerCursors)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 103.1s
Verification: verify:integrationResult: ✅ PASS |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
packages/docs/test/view/peer-cursor.test.ts (1)
80-142: Add a wrap-boundary affinity case to this resolver suite.
resolvePositionPixel()takeslineAffinity, but the current coverage only exercises unambiguous offsets. Please add a case where the same offset can resolve to either end-of-line or start-of-next-line so regressions in soft-wrap cursor placement are caught.Based on learnings, include and consistently thread
lineAffinityalongsideblockIdandoffsetso wrap-boundary offsets resolve to the correct visual line context. As per coding guidelines,**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests for critical business logic and edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/test/view/peer-cursor.test.ts` around lines 80 - 142, Add a test that exercises wrap-boundary affinity by creating a block with two visual lines (e.g., first line text length N, second line starts immediately) and calling resolvePositionPixel with the same position offset at the boundary but with different lineAffinity values; specifically, pass a position object that includes blockId, offset (boundary offset), and lineAffinity ('backward' and 'forward'), call resolvePositionPixel twice (once per affinity) and assert the returned pixel coords differ in y (or line context) to prove the affinity chooses the preceding vs following visual line; locate resolvePositionPixel usages in this test suite (peer-cursor.test.ts) and follow the existing mockRun/mockLine/mockBlock/makePaginatedLayout/makeCtx pattern to build the layout and expectations.packages/docs/src/view/editor.ts (1)
230-242: Make overlapping label order deterministic.
stackIndexdepends on the incomingpeerCursorsorder. If that order changes between presence updates, peers at the same position will swap vertically. Sorting byclientKeyandclientIDbefore counting makes stacking stable.Suggested change
// Compute stacking indices for peers at the same position const stackCounts = new Map<string, number>(); - const resolvedPeers = peerPixels.map((p) => { - const count = stackCounts.get(p.clientKey) ?? 0; - stackCounts.set(p.clientKey, count + 1); - return { - pixel: p.pixel, - color: p.color, - username: p.username, - labelVisible: p.labelVisible, - stackIndex: count, - }; - }); + const resolvedPeers = [...peerPixels] + .sort( + (a, b) => + a.clientKey.localeCompare(b.clientKey) + || a.clientID.localeCompare(b.clientID), + ) + .map((p) => { + const count = stackCounts.get(p.clientKey) ?? 0; + stackCounts.set(p.clientKey, count + 1); + return { + pixel: p.pixel, + color: p.color, + username: p.username, + labelVisible: p.labelVisible, + stackIndex: count, + }; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/editor.ts` around lines 230 - 242, The stacking is non-deterministic because stackIndex is assigned in the incoming peerPixels order; before computing stackCounts and building resolvedPeers, sort peerPixels by their pixel position and then by clientKey and clientID (as tie-breakers) so peers at the same position always get the same stackIndex; keep the existing stackCounts/resolvedPeers mapping logic and only change the input order used to compute stackIndex.
🤖 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/docs-remote-cursor.md`:
- Around line 165-173: Update the spec to reflect that label text color is
dynamic rather than fixed white: replace the hardcoded "White (`#FFFFFF`)" entry
with a note that text color is determined at runtime using the same logic as the
implementation (see getLabelTextColor in packages/docs/src/view/peer-cursor.ts),
e.g., "Text color: dynamic — computed via getLabelTextColor (black or white)";
keep other layout entries unchanged.
- Around line 102-108: The fenced code block that begins with "Yorkie
others-changed event" is missing a language tag causing markdownlint MD040; add
a language identifier (use "text") to the opening fence so it becomes ```text
and keep the closing fence unchanged to satisfy the linter and preserve the
event-flow snippet formatting.
In `@packages/docs/src/view/editor.ts`:
- Around line 40-43: The peer-cursor API is missing lineAffinity, causing
wrap-boundary offsets to resolve incorrectly; update the onCursorMove callback
signature and the getPeerCursorPixels presence payload to include lineAffinity
(e.g., { blockId, offset, lineAffinity }) and thread that field through the
selection/text-editor helpers, ensuring callers pass lineAffinity into
resolvePositionPixel when computing pixel positions; update all related helpers
and any call sites (including functions that build peer cursor presences and the
resolvePositionPixel invocations) to accept and propagate lineAffinity so
visual-line hit-testing uses the correct affinity.
In `@packages/frontend/src/app/docs/docs-view.tsx`:
- Around line 214-243: The hover state isn't cleared when the pointer exits the
editor; update the logic around handleMouseMove/hoveredPeerClientID so pointer
exit clears the hover: add a pointerleave/pointerout (or mouseleave) event
handler on the same container that sets hoveredPeerClientID.current = null and
calls ed.setPeerCursors(buildPeerCursors()) using editorRef.current (same
approach as in handleMouseMove), and apply the same fix to the other hover block
referenced (lines ~249-263) so leaving the editor always hides the peer label.
- Around line 199-210: The trailing-send timer currently always re-arms for the
full CURSOR_UPDATE_THROTTLE which can delay the final broadcast; inside the
editor.onCursorMove handler (use symbols editor.onCursorMove,
cursorTrailingTimer, lastCursorUpdate, CURSOR_UPDATE_THROTTLE,
store.updateCursorPos) compute the remaining window = CURSOR_UPDATE_THROTTLE -
(Date.now() - lastCursorUpdate.current), clamp to >=0, and use that remaining
time for window.setTimeout so the suppressed move flushes exactly at
lastCursorUpdate + CURSOR_UPDATE_THROTTLE instead of adding a full extra
throttle interval.
In `@packages/frontend/src/app/docs/yorkie-doc-store.ts`:
- Around line 444-447: The current updateCursorPos method (updateCursorPos) and
the presence serializer drop Cursor.lineAffinity by only storing {blockId,
offset}; update the code to preserve and broadcast lineAffinity: change the
presence/activeCursorPos shape to include lineAffinity (e.g., {blockId, offset,
lineAffinity}), set p.set({ activeCursorPos: pos ?? undefined }) with that full
object, and update the DocsPresence type/serializer and any code that
reads/writes cursor presence to accept and forward lineAffinity so wrap-boundary
offsets resolve to the correct visual line for collaborators.
In `@packages/frontend/src/types/users.ts`:
- Around line 16-21: DocsPresence currently extends User but the actual presence
seeded in docs-detail.tsx only includes username, email, photo, and
activeCursorPos, making YorkieDocStore.getPresences() unsafe; either narrow the
DocsPresence type to exactly the runtime shape (e.g., remove/optionalize id and
authProvider) or ensure the presence initialization in docs-detail.tsx seeds the
missing User fields (id, authProvider, etc.) so the runtime payload matches the
DocsPresence type; update the type declaration in
packages/frontend/src/types/users.ts (DocsPresence) or the presence creation
logic in docs-detail.tsx and adjust any usages in YorkieDocStore.getPresences()
accordingly.
---
Nitpick comments:
In `@packages/docs/src/view/editor.ts`:
- Around line 230-242: The stacking is non-deterministic because stackIndex is
assigned in the incoming peerPixels order; before computing stackCounts and
building resolvedPeers, sort peerPixels by their pixel position and then by
clientKey and clientID (as tie-breakers) so peers at the same position always
get the same stackIndex; keep the existing stackCounts/resolvedPeers mapping
logic and only change the input order used to compute stackIndex.
In `@packages/docs/test/view/peer-cursor.test.ts`:
- Around line 80-142: Add a test that exercises wrap-boundary affinity by
creating a block with two visual lines (e.g., first line text length N, second
line starts immediately) and calling resolvePositionPixel with the same position
offset at the boundary but with different lineAffinity values; specifically,
pass a position object that includes blockId, offset (boundary offset), and
lineAffinity ('backward' and 'forward'), call resolvePositionPixel twice (once
per affinity) and assert the returned pixel coords differ in y (or line context)
to prove the affinity chooses the preceding vs following visual line; locate
resolvePositionPixel usages in this test suite (peer-cursor.test.ts) and follow
the existing mockRun/mockLine/mockBlock/makePaginatedLayout/makeCtx pattern to
build the layout and expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76069c57-b881-4e05-911d-8f3f83e58b3b
📒 Files selected for processing (16)
docs/design/README.mddocs/design/docs-remote-cursor.mddocs/tasks/active/20260324-docs-remote-cursor-todo.mdpackages/docs/src/index.tspackages/docs/src/view/cursor.tspackages/docs/src/view/doc-canvas.tspackages/docs/src/view/editor.tspackages/docs/src/view/layout.tspackages/docs/src/view/peer-cursor.tspackages/docs/src/view/selection.tspackages/docs/test/store/memory.test.tspackages/docs/test/view/peer-cursor.test.tspackages/frontend/src/app/docs/docs-detail.tsxpackages/frontend/src/app/docs/docs-view.tsxpackages/frontend/src/app/docs/yorkie-doc-store.tspackages/frontend/src/types/users.ts
💤 Files with no reviewable changes (1)
- packages/docs/src/view/layout.ts
- Add mouseleave handler to clear hover label on pointer exit - Use remaining throttle window for trailing cursor send - Sort peers by clientID for deterministic stack ordering - Add language tag to fenced code block in design doc - Update spec: label text color is dynamic, not fixed white - Narrow DocsPresence type to match runtime presence shape
Summary
Changes
Docs Package (
@wafflebase/docs)peer-cursor.ts(new): SharedresolvePositionPixel()utility,PeerCursortype,drawPeerCaret(),drawPeerLabel()with text truncation, label stacking, and edge clampingcursor.ts: RefactoredgetPixelPosition()to delegate to sharedresolvePositionPixel()doc-canvas.ts: Accept and renderPeerCursor[]— carets clipped to content area, labels unclippededitor.ts: AddedsetPeerCursors(),onCursorMove(),getPeerCursorPixels()toEditorAPIselection.ts: Guard against stale block references from remote deletions (fixes crash)layout.ts: Remove unusedisFirstLinevariableFrontend Package (
@wafflebase/frontend)users.ts: AddedDocsPresencetype (separate fromSheetPresence)yorkie-doc-store.ts: AddedupdateCursorPos()andgetPresences()(not onDocStoreinterface)docs-detail.tsx: AddedactiveCursorPostoinitialPresencedocs-view.tsx: Full integration — presence subscription, label visibility timers, hover detection, throttled cursor updates with trailing edgeDesign
See
docs/design/docs-remote-cursor.mdfor the full design document.Architecture: Presence stays in the frontend layer.
YorkieDocStoreexposes presence methods directly (not throughDocStore).DocsViewsubscribes to Yorkieothers-changedevents, buildsPeerCursor[], and passes it to the editor's render pipeline.Test plan
resolvePositionPixel,drawPeerCaret,drawPeerLabel(9 tests added)pnpm verify:selfpasses🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation