Skip to content

Fix shared document reliability#2357

Open
3mdistal wants to merge 13 commits into
mainfrom
codex/content-shared-document-reliability
Open

Fix shared document reliability#2357
3mdistal wants to merge 13 commits into
mainfrom
codex/content-shared-document-reliability

Conversation

@3mdistal

@3mdistal 3mdistal commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • render shared viewer bodies from durable SQL while retaining collaboration presence
  • bind editable SQL documents to one stable Y.Doc from first mount and prevent duplicate seeding during persisted-state projection
  • preserve active-peer or just-emitted content across an ambiguous empty snapshot while letting canonical empty SQL clear stale CRDT state
  • wait one hosted awareness poll plus margin before empty-SQL clearing, so peer presence arriving after document sync cannot lose active content
  • make comment activation atomic and keep anchored and unanchored comment cards stable, separated, connected, and reachable on narrow screens
  • prevent document refreshes from restarting the one-shot flush reader, and preserve comment drafts until their mutation succeeds
  • add privacy-safe document/favorite audit scopes, guaranteed pre-clear recovery versions, and resilient partial Files-sidebar loading

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

  • current head: 4ff223df8
  • exact-head Content deploy 6a62e9ae62396b00087fad1b is green at https://deploy-preview-2357--agent-native-content.netlify.app
  • Toolkit full suite: 625 tests passed
  • final focused Toolkit reconcile suites: 17 tests passed
  • final focused Content editor/comment suites: 43 tests passed after the request-storm and pending-draft repairs
  • Toolkit and Content typechecks passed
  • i18n catalog guard passed across 17 catalog directories
  • full Content run reached 1,841 passing tests; the sole unrelated runner failure was the known missing Node localStorage shim
  • exact-head CI passes Fast tests, build, lint/format, typecheck, Content DB, Content parity, Core integration, security, static template checks, changeset, deploy, visual recap, and the scaffold E2E rerun
  • real Yjs regressions cover persisted-state projection without duplicate seed, delayed peer awareness after document sync, no-peer canonical empty clearing after the bounded settle, and first local edits surviving that window
  • 30-thread deterministic layout execution passed with upper, middle, and lower anchors non-overlapping; the real browser also proved highlight activation, exact thread opening, and scroll stability

Exact-head acceptance

  • H1 owner created a private document and the first edit persisted
  • H2 reload showed the body exactly once with no duplication
  • H3 a separately authenticated Viewer saw the exact body and read-only title/editor controls
  • H4 owner and Viewer converged on the four-line revision; attempted Viewer typing did not mutate it
  • H5 intentional clear propagated, pre-clear Version history was visible, and restoring it recovered the exact body for both sessions
  • H6 creating a highlighted comment opened the correct thread; closing, reactivating the highlight, and scrolling retained the thread
  • H7 30-thread upper/middle/lower layout execution was non-overlapping, combined with real-browser anchor activation and scroll stability
  • H8 Comments rendered as a usable sheet with a reachable reply composer at 320px and 390x844
  • H9 anonymous /page/qH8bP384dPR4 access redirected to sign-in without exposing the private document
  • H10 favorite/unfavorite remained personal UI state and left the exact body unchanged; Version history was visible, while raw audit payload input was not directly observable in this UI

After the final pending-draft patch, the exact-head browser held POST /_agent-native/actions/add-comment at 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

  • no schema migration or new credential surface
  • includes Toolkit and Core changesets plus Content changelog entries
  • repository review is approved with no known unresolved threads
  • authoritative Content deploy is green for the exact head
  • this PR is ready for review and intentionally remains unmerged until explicit merge authorization

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Here's a visual recap of what changed:

Visual recap

Open the full interactive recap

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

builder-io-integration[bot]

This comment was marked as outdated.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

builder-io-integration[bot]

This comment was marked as outdated.

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 peerCountRef is 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: null receive the normal gap because the layout code checks positions.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.

Comment thread packages/toolkit/src/editor/useCollabReconcile.ts Outdated
Comment thread templates/content/app/components/editor/CommentsSidebar.tsx Outdated
@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants