feat: commentIdBase prop for collision-free collab comment IDs (#257)#956
Draft
jedrazb wants to merge 9 commits into
Draft
feat: commentIdBase prop for collision-free collab comment IDs (#257)#956jedrazb wants to merge 9 commits into
jedrazb wants to merge 9 commits into
Conversation
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.
- Scope the collaboration-example dev server to Playwright runs that include the chromium project, so project-scoped runs (notably the release publish-path parity smoke gate) don't boot an unrelated server. - Time-box the example's startup GitHub stars fetch (2s) so a restricted network can't hang the webServer startup window. - Document the two preconditions for cross-peer collision-freedom (distinct clientIDs; fewer than `stride` allocations per session). - Use Vue binding syntax in the Vue prop's JSDoc example. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
All contributors have signed the CLA ✍️ ✅ Posted by the CLA bot. |
6 tasks
Contributor
Greptile SummaryAdds optional
Confidence Score: 4/5Safe to merge; the core logic is correct for all documented usage patterns and the backward-compatible defaults preserve existing single-editor behaviour. The allocator correctly filters out-of-partition IDs in seedAbove, both React and Vue wire up the props symmetrically, unit tests cover the key collab edge cases, and the parity contract is updated. Two observations hold the score: next() is unbounded and will silently overflow ceiling if a session mints more than stride IDs (the ceiling variable exists but is unused in next()), and the E2E test's .last() card picker could non-deterministically grab a peer-synced card depending on sidebar sort order under slow Yjs sync. packages/core/src/prosemirror/commentIdAllocator.ts — the next() overflow path; e2e/tests/collab-comment-id.spec.ts — the .last() card-ID picker.
|
| Filename | Overview |
|---|---|
| packages/core/src/prosemirror/commentIdAllocator.ts | Core change: adds base/stride params for ID partitioning; seedAbove correctly clamps to partition, but next() is unbounded and silently overflows ceiling |
| packages/core/src/prosemirror/tests/commentIdAllocator.test.ts | Good unit coverage: default sequence, partition isolation, in-partition seed, out-of-partition seed, and cross-peer contamination all tested |
| packages/react/src/components/DocxEditor.tsx | Correctly threads commentIdBase/commentIdStride into useRef(createCommentIdAllocator(...)); read-once-at-mount behavior is documented and intentional |
| packages/vue/src/components/DocxEditor.vue | Vue component initializes allocator with props at setup time; commentIdStride defaults to undefined (which correctly maps to Infinity via function default) |
| e2e/tests/collab-comment-id.spec.ts | Hermetic two-peer BroadcastChannel test; the .last() card picker could race against Yjs sync delivering the peer's card out of order |
| playwright.config.ts | Conditionally boots the collab dev server only when chromium project is in scope; requestedProjects parsing handles both --project=x and --project x forms correctly |
| examples/collaboration/src/useCollaboration.ts | Derives commentIdBase = ydoc.clientID * 1_000_000 inside the roomName-scoped useMemo and exports both props; clean integration |
| examples/collaboration/vite.config.ts | AbortSignal.timeout(2000) added to the GitHub stars fetch to prevent hanging the Playwright webServer startup window on restricted networks |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["DocxEditor mount\n(commentIdBase, commentIdStride)"] --> B["createCommentIdAllocator(base, stride)\nnextId = base + 1\nceiling = base + stride"]
B --> C["seedCommentAllocator called\non document load"]
C --> D{"For each comment/\nrevision mark ID"}
D --> E["allocator.seedAbove(id)"]
E --> F{"id >= nextId\nAND id <= ceiling?"}
F -- Yes --> G["nextId = id + 1\n(in-partition seed)"]
F -- No --> H["Ignored\n(out-of-partition\nor already passed)"]
G --> I["allocator.next()\nreturns nextId++"]
H --> I
I --> J{"nextId > ceiling?\n(overflow)"}
J -- "No (common case)" --> K["ID minted in own partition\n✓ No collision"]
J -- "Yes (> stride IDs minted)" --> L["ID crosses into\nadjacent partition\n⚠ Silent overflow"]
%%{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"}}}%%
flowchart TD
A["DocxEditor mount\n(commentIdBase, commentIdStride)"] --> B["createCommentIdAllocator(base, stride)\nnextId = base + 1\nceiling = base + stride"]
B --> C["seedCommentAllocator called\non document load"]
C --> D{"For each comment/\nrevision mark ID"}
D --> E["allocator.seedAbove(id)"]
E --> F{"id >= nextId\nAND id <= ceiling?"}
F -- Yes --> G["nextId = id + 1\n(in-partition seed)"]
F -- No --> H["Ignored\n(out-of-partition\nor already passed)"]
G --> I["allocator.next()\nreturns nextId++"]
H --> I
I --> J{"nextId > ceiling?\n(overflow)"}
J -- "No (common case)" --> K["ID minted in own partition\n✓ No collision"]
J -- "Yes (> stride IDs minted)" --> L["ID crosses into\nadjacent partition\n⚠ Silent overflow"]
Reviews (1): Last reviewed commit: "fix(comment-id): address review on colla..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+55
to
63
| export function createCommentIdAllocator(base = 0, stride = Infinity): CommentIdAllocator { | ||
| let nextId = base + 1; | ||
| const ceiling = base + stride; | ||
| return { | ||
| next: () => nextId++, | ||
| seedAbove(maxId: number) { | ||
| if (maxId >= nextId) nextId = maxId + 1; | ||
| if (maxId >= nextId && maxId <= ceiling) nextId = maxId + 1; | ||
| }, | ||
| }; |
Contributor
There was a problem hiding this comment.
next() is unbounded — silent partition overflow
ceiling is computed but only consulted in seedAbove; next() never checks it. Once a peer mints its 1,000,001st ID it silently emits values in the adjacent partition. From that point, the other peer's seedAbove will accept those values (they fall within (adjacentBase, adjacentBase + stride]) and advance the victim's counter, causing the exact collision this feature is meant to prevent — with no error or warning.
Adding a dev-mode guard directly in next() (e.g., if (process.env.NODE_ENV !== 'production' && nextId > ceiling) console.warn(...)) would make the overflow immediately visible during development without any production overhead.
Comment on lines
+24
to
+28
| const card = page.locator(`${SIDEBAR} .docx-comment-card[data-comment-id]`).last(); | ||
| await card.waitFor({ state: 'visible' }); | ||
| const id = await card.getAttribute('data-comment-id'); | ||
| if (id == null) throw new Error('comment card rendered without data-comment-id'); | ||
| return Number(id); |
Contributor
There was a problem hiding this comment.
.last() card picker is racing against Yjs sync
By the time peer B presses Enter and the assertion resolves, peer A's comment card may have already synced into peer B's sidebar. If the sidebar renders synced cards after the locally-minted card (e.g., by comment ID ascending, where peer B's base is higher than peer A's), .last() returns the correct card. But if the sidebar sorts by document position or Y.Array insertion order, A's card might appear after B's, causing .last() to return A's ID — making idB === idA and tripping the distinctness assertion non-deterministically.
A more resilient approach is to snapshot visible IDs before adding the comment and then wait for a new ID to appear, rather than relying on positional order.
Contributor
Author
|
Heads up before this lands: the per-peer ID scheme produces comment/revision IDs that Word won't accept, so collaborative saves will come back corrupt. The relevant attributes ( <xsd:simpleType name="ST_DecimalNumber">
<xsd:restriction base="xsd:integer"/>
</xsd:simpleType>The schema doesn't match what Word actually does, though. Word reads these IDs as signed 32-bit ints, so anything above 2,147,483,647 (0x7FFFFFFF) gets rejected: the file opens into "unreadable content" / Document Recovery and the comment or revision is dropped or renumbered. We already depend on this exact limit elsewhere. The scheme here is The partitioning idea itself is fine for keeping IDs unique; the problem is just that There are really two separate requirements:
I'd keep the per-peer partition for the live side, and renumber the comment + revision IDs down to a dense 1..N at serialize time, only when something is actually out of range so normal single-editor saves stay untouched, with a hard 0x7FFFFFFF clamp as a backstop. One thing to watch: the editor uses a single ID space shared across comments and revisions, so the renumber has to draw from one sequence rather than two. This also covers oversized IDs coming from a malformed input file, not just collab. Suggest holding the merge until the save side is handled. The partition on its own isn't safe for Word. |
Contributor
|
Follow-up patch addressing the int32 What it does:
Verified locally: Patch (apply with
|
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
2 participants
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.
Supersedes #942
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 optionalcommentIdBase/commentIdStrideprops (React + Vue) that partition the ID space: passydoc.clientID * 1_000_000withcommentIdStride={1_000_000}and each peer mints from a disjoint range. Default0/Infinitypreserves the single-editor1, 2, 3, …sequence. Fixes #257.Review fixes applied on top of the original work:
chromiumproject, so project-scoped runs (notably the release publish-path parity smoke gate) don't boot an unrelated server.clientIDs; fewer thanstrideallocations per session).Known follow-up (not addressed here):
DocxEditor.tsxcrossed 2000 lines so themax-linescap was nudged to 2020. The real fix is the planned file split, tracked separately.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.