Skip to content

Fix shared document reliability#2357

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

Fix shared document reliability#2357
3mdistal wants to merge 9 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
  • 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 clients first emitted content is preserved; stale persisted Y.Doc content with no active writer is cleared by SQL after a bounded awareness settle.

Verification

  • current head: 1f16a3a73
  • Toolkit full suite: 625 tests passed
  • final focused Toolkit reconcile suites: 17 tests passed
  • final focused Content editor/comment suites: 57 tests passed
  • 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 is the known missing Node localStorage shim
  • 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
  • independent final review found no code blockers at 1f16a3a73
  • exact-head Content deploy 6a6271d4cb0f4200083de7fa is green at https://deploy-preview-2357--agent-native-content.netlify.app
  • frozen H1-H10 acceptance remains blocked before H1 because this runtime cannot provide isolated owner, Viewer, and anonymous browser sessions; no disposable data was created

Risk and landing gate

  • no schema migration or new credential surface
  • includes a Toolkit changeset and Content changelog entry
  • current-head checks pass except Fast tests: two CI attempts each timed out in unrelated packages/core/src/a2a/client.spec.ts after 8,794 Core tests passed; that exact file passes locally 24/24 and this branch does not touch A2A
  • authoritative netlify/agent-native-content/deploy-preview is green for the exact head
  • repository review is approved with no unresolved threads
  • authenticated H1-H10 Content acceptance and a green required Fast-tests check remain mandatory before merge
  • 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.

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