Fix shared document reliability#2357
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed: Open the full interactive recap |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🔴
Review Details
Code Review Summary
This incremental review covers the latest 1,483-line update across Toolkit collaboration reconciliation, Content document persistence/audit behavior, comment layout and activation, and partial Files-sidebar loading. The overall direction is sound: SQL remains the durable source for viewers, editors avoid duplicate Yjs seeding, recovery snapshots are tested, and the comment layout has dedicated geometry tests.
Key Findings
- 🔴 HIGH — The empty-SQL/Yjs adoption decision can run before remote awareness has synchronized, so a joining editor may see active collaborator content in Yjs while
peerCountRefis still zero and clear that content as stale. This is a potential active-edit data-loss race; the added test pre-populates awareness and does not cover this ordering. - 🟡 MEDIUM — Unanchored threads that have a position record with
layoutTop: nullreceive the normal gap because the layout code checkspositions.has()rather than whether the position is anchored. This makes the orphan separator spacing inconsistent.
The database action and audit changes otherwise appear appropriately scoped, and the new regression coverage is useful. Risk level: Standard, because this combines shared state/business mutations with UI changes.
🧪 Browser testing: Planned after this review (PR touches UI code), but all 16 planned cases were blocked because browser-testing executors lacked Chrome automation tools; the dev server was reachable.

Summary
Why
A view-only recipient could see a document title with an empty body even though SQL still held content. Real two-session preview QA then exposed the matching owner-side failure: after a durable edit and reload, the snapshot-to-Yjs remount could seed the same body beside freshly projected persisted CRDT content and duplicate the document.
SQL remains canonical. Editors bind one stable Y.Doc before initial sync completes but stay non-editable until readiness. Viewers render a read-only SQL snapshot and retain presence. During an empty initial snapshot, active-peer or this client's first emitted content is preserved; stale persisted Y.Doc content with no active writer is cleared by SQL after a bounded awareness settle.
Final browser acceptance also found a long-session request storm: routine document object refreshes restarted the live-ingest flush effect, issuing 317 duplicate reads in three seconds and starving comment mutations. The save callback now stays behind a ref, reducing the same exact-head measurement to zero duplicate flush reads. Comment drafts clear only after mutation success and show a visible error on failure.
Verification
4ff223df86a62e9ae62396b00087fad1bis green at https://deploy-preview-2357--agent-native-content.netlify.appExact-head acceptance
/page/qH8bP384dPR4access redirected to sign-in without exposing the private documentAfter the final pending-draft patch, the exact-head browser held
POST /_agent-native/actions/add-commentat the network boundary and verified the reply textarea plus submit control were disabled with the draft intact; releasing the request saved the reply and closed the composer.The frozen interface matrix was executed in this root thread because the independent tester runtime could not access the separate owner IAB profile. Screenshots are retained in the local QA evidence bundle.
Risk and landing gate