feat: commentIdBase prop for collision-free collab comment IDs (#257)#942
feat: commentIdBase prop for collision-free collab comment IDs (#257)#942samcorcos wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ Posted by the CLA bot. |
|
I have read the CLA Document and I hereby sign the CLA |
Greptile SummaryAdds an optional
Confidence Score: 3/5The base-partitioning approach works well for the primary scenario (concurrent first comments on an empty document), but the The core allocator change is sound and the React/Vue wiring is correct. The gap is in The most important file to revisit is
|
| Filename | Overview |
|---|---|
| packages/core/src/prosemirror/commentIdAllocator.ts | Adds base parameter to createCommentIdAllocator; seedAbove can push a peer's counter into another peer's range when cross-peer revision IDs are present in PM state |
| e2e/tests/collab-comment-id.spec.ts | New e2e regression test for #257; button selector is fragile (CSS property match) and getAttribute null case is unguarded |
| packages/react/src/components/DocxEditor.tsx | Adds commentIdBase prop (default 0), passes to createCommentIdAllocator via useRef — correctly read once at mount as documented |
| packages/vue/src/components/DocxEditor.vue | Adds commentIdBase prop with default 0, passes to createCommentIdAllocator at setup time; consistent with React behaviour |
| examples/collaboration/src/useCollaboration.ts | Computes commentIdBase = ydoc.clientID * 1_000_000 from useMemo and exposes it on CollaborationState; clean and correct |
| packages/core/src/prosemirror/tests/commentIdAllocator.test.ts | Good unit coverage for base parameter; test for "seedAbove above base" explicitly documents the cross-partition raise behaviour but no test exercises the mixed revision-ID scenario |
| scripts/parity/parity.contract.json | Adds commentIdBase to the parity contract; React + Vue both implement it so the contract check should pass |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant App
participant useCollaboration
participant DocxEditor
participant createCommentIdAllocator
participant seedCommentAllocator
App->>useCollaboration: mount(roomName)
useCollaboration->>useCollaboration: new Y.Doc() → clientID
useCollaboration-->>App: "commentIdBase = clientID * 1_000_000"
App->>DocxEditor: render(commentIdBase)
DocxEditor->>createCommentIdAllocator: createCommentIdAllocator(commentIdBase)
createCommentIdAllocator-->>DocxEditor: "allocator (nextId = base+1)"
Note over DocxEditor: Document load
DocxEditor->>seedCommentAllocator: seedAbove(maxId from doc)
seedCommentAllocator-->>DocxEditor: "nextId raised if maxId >= nextId"
Note over DocxEditor: Peer A adds comment
DocxEditor->>DocxEditor: allocator.next() → base_A + 1
Note over DocxEditor: Peer B (different base) adds comment
DocxEditor->>DocxEditor: allocator.next() → base_B + 1
Note over DocxEditor: IDs are disjoint ✓
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant App
participant useCollaboration
participant DocxEditor
participant createCommentIdAllocator
participant seedCommentAllocator
App->>useCollaboration: mount(roomName)
useCollaboration->>useCollaboration: new Y.Doc() → clientID
useCollaboration-->>App: "commentIdBase = clientID * 1_000_000"
App->>DocxEditor: render(commentIdBase)
DocxEditor->>createCommentIdAllocator: createCommentIdAllocator(commentIdBase)
createCommentIdAllocator-->>DocxEditor: "allocator (nextId = base+1)"
Note over DocxEditor: Document load
DocxEditor->>seedCommentAllocator: seedAbove(maxId from doc)
seedCommentAllocator-->>DocxEditor: "nextId raised if maxId >= nextId"
Note over DocxEditor: Peer A adds comment
DocxEditor->>DocxEditor: allocator.next() → base_A + 1
Note over DocxEditor: Peer B (different base) adds comment
DocxEditor->>DocxEditor: allocator.next() → base_B + 1
Note over DocxEditor: IDs are disjoint ✓
Comments Outside Diff (1)
-
packages/core/src/prosemirror/commentIdAllocator.ts, line 48-50 (link)seedAbovecan silently break the base partition in mixed comment+tracked-change collab sessionsseedCommentAllocatorcomputes a single global maximum across all comment IDs and tracked-change revision marks (revisionId) in the PM state, then passes it toseedAbove. In Vue, this runs before every comment creation (useCommentLifecycle.ts:244); in React it runs at every document load.Concretely: peer B has
base = 1_000_000(nextId = 1_000_001). Peer A hasbase = 5_000_000and makes a tracked change (revisionId = 5_000_001), which Yjs syncs into peer B's PM state. Peer B next creates a comment:seedCommentAllocatorfindsmax = 5_000_001, firesseedAbove(5_000_001)→ peer B'snextIdjumps to5_000_002. Peer A simultaneously creates another tracked change →revisionId = 5_000_002. Both peers mint the same ID.The JSDoc's "never collide" guarantee only holds when tracked-change IDs from other peers are absent or below the local base. A targeted fix: in
seedAbove(or inseedCommentAllocator), skip IDs that fall strictly abovenextId's base epoch — i.e., only seed ifmaxId < base + stridefor some defined stride — or document thatcommentIdBasedoes not provide collision safety when tracked changes are also used concurrently.
Reviews (1): Last reviewed commit: "chore: changeset for commentIdBase (#257..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await page.waitForFunction(() => { | ||
| for (const b of document.querySelectorAll('[data-testid="docx-editor"] button')) { | ||
| const s = getComputedStyle(b); | ||
| if (s.position === 'absolute' && s.zIndex === '50') return true; | ||
| } | ||
| return false; | ||
| }); | ||
| const handle = await page.evaluateHandle(() => { | ||
| for (const b of document.querySelectorAll('[data-testid="docx-editor"] button')) { | ||
| const s = getComputedStyle(b); | ||
| if (s.position === 'absolute' && s.zIndex === '50') return b; | ||
| } | ||
| return null; | ||
| }); | ||
| await handle.asElement()!.click(); | ||
| } |
There was a problem hiding this comment.
clickFloatingAddComment identifies the add-comment button by matching position === 'absolute' and zIndex === '50' at runtime. Any unrelated CSS change — a Tailwind utility swap, a z-index token change, or a browser default difference — silently breaks the selector and the test fails with a confusing timeout rather than a useful assertion error. A data-testid="add-comment-btn" (or equivalent stable attribute) on the floating button would make intent explicit and survive styling refactors.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const id = await page | ||
| .locator(`${SIDEBAR} .docx-comment-card[data-comment-id]`) | ||
| .last() | ||
| .getAttribute('data-comment-id'); | ||
| return Number(id); |
There was a problem hiding this comment.
null from getAttribute silently coerces to 0
getAttribute('data-comment-id') returns string | null. If the attribute is absent or the locator races against a re-render, Number(null) === 0. The test then hits expect(0).toBeGreaterThan(1_000_000) with a misleading failure instead of a clear "attribute was missing" error. Adding a non-null assertion (e.g., if (id === null) throw new Error('data-comment-id missing')) before Number(id) would produce a diagnostic-friendly failure.
|
Interesting! I underestimated Greptile. Will update. |
seedAbove now ignores IDs outside (base, base+stride] so a peer's synced tracked-change revisionId can't pull this peer's allocator into another partition; commentIdStride prop threads the stride through React/Vue and the collab example. Floating add-comment button gets a data-testid so the e2e test no longer matches on computed CSS, and getAttribute null is guarded for a clearer failure.
|
Whoops, didn't mean to close this @jedrazb -- still a valid PR. Apologies. |
|
@samcorcos do you have plans to reopen? or should i open a fresh PR from your branch to get this fixed? EDIT: cherrypicked your work and continuing in #956 |
3 participants
In a collaborative session every peer's
createCommentIdAllocator()started at1, so the first comment from each peer got the sameidand replies threaded onto the wrong card. This adds an optionalcommentIdBaseprop (React + Vue) that offsets the allocator — passydoc.clientID * 1_000_000and each peer mints from a disjoint range. Default0preserves the single-editor1, 2, 3, …sequence. Fixes #257.Before — every peer's first comment is
id: 1:After — per-peer base (
clientID × 1e6):Repro that now passes (
e2e/tests/collab-comment-id.spec.ts): two pages join the same room, each adds a comment, assert the twodata-comment-idvalues are distinct and both peers see both comments. Hermetic via y-webrtc's BroadcastChannel path — no external signaling.check:parity-contract)api:extractsnapshots updatedcreateCommentIdAllocator(base)(default unchanged, disjoint bases,seedAbovemonotone)examples/collaborationwired up; README caveat removedNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.