Use the full Page editor in collection previews - #4424
Conversation
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed: Open the full interactive recap |
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🔴
Review Details
Code Review Summary
PR #4424 replaces the collection preview editor with the canonical Page editor surface while preserving database-row traversal, preview-specific properties, and document/membership context. The broader persistence barrier is a sound direction: it attempts to flush the live body, title/metadata, additional Blocks fields, and scalar properties before navigation, and it adds explicit recovery handling for rejected or conflicted writes. The action-driven cache refresh and localized recovery copy are also good patterns.
This is a standard-risk change because it modifies shared editor state, document persistence, and recovery behavior across both full-page and collection-preview flows.
Key Findings
- 🔴 HIGH — Conflict recovery can delete the user’s retained local draft after the editor adopts the winning server version and a later no-op save reports
contentPersisted: true. This can permanently lose the conflicted edit. - 🟡 MEDIUM — Draft restoration uses the current document timestamp rather than the draft’s original CAS baseline, allowing a stale retained draft to overwrite an intervening server edit instead of surfacing a conflict.
🧪 Browser testing: Dev server is healthy, but browser testing was blocked because no Chrome automation tools were reachable; the planner produced a 24-flow full-scope plan with all cases marked unable to verify.
| return result; | ||
| } | ||
|
|
||
| await clear(); |
There was a problem hiding this comment.
🔴 Do not clear a conflict recovery draft after a no-op save
When a CAS conflict returns contentPersisted: false, the local edit is retained. After the query cache adopts the winning server document, a later close/navigation flush can perform no updates and return contentPersisted: true, causing this clear() call to delete the retained local conflict draft. Keep the retained draft until the retained payload is explicitly persisted or discarded; a no-op save of server content must not clear it.
Additional Info
Found by 1 of 4 code-review agents; independently confirmed against DocumentEditor.tsx no-op save path and query reconciliation behavior.
| id: document.id, | ||
| title: draft.title, | ||
| content: draft.content, | ||
| baseUpdatedAt: document.updatedAt, |
There was a problem hiding this comment.
🟡 Restore drafts against their original CAS baseline
Draft restoration sends baseUpdatedAt: document.updatedAt, which is the current server version rather than the retained draft’s baseDocumentUpdatedAt. If the Page changed after the draft was created, this can make the stale draft appear based on the newest content and overwrite the intervening edit instead of returning a conflict. Use the draft’s stored baseline (and preserve its related empty-content metadata) for recovery.
Additional Info
Found by 2 of 4 code-review agents and confirmed in source.

Problem
Collection row previews used a separate editor and save lifecycle. They lacked the full Page toolbar and collaboration behavior, and leaving a preview could miss edits that were still present in the rich-text editor.
Changes
Both the Page route and collection preview now mount the same Page surface, with document and membership context preserved. Previews retain previous/next traversal and inline properties while gaining the shared Share, Comments, Info, history, export, and Page actions. Collection-specific duplication and Favorites membership removal remain separate actions. On mobile, previews fill the viewport width, omit the duplicate header title, reduce the space above the editor title, and wrap property values. A labeled Row menu with a table icon distinguishes collection actions from the Page menu.
Leaving a preview waits for the live rich-text body, title/metadata, additional Blocks fields, and scalar property writes. A failed or conflicted save keeps the editor mounted. Existing recovery drafts remain available through an explicit restore/discard flow with version-checked deletion.
The change also makes provisional property permissions stale until the authoritative read returns, refreshes mounted preview Pages after direct Action updates, scopes nested Escape handling to the focused Page, resizes long titles when the preview width changes, and releases the transition lock before applying a completed close/navigation.
Validation
Mobile refinement: 121 focused tests, Content TypeScript, all 69 guards, and changed-file lint/format passed. Independent headed-browser QA passed the full mobile and desktop flow, tablet checkpoints, and the 639/640px breakpoint: width, title visibility, property wrapping, menu distinction, dirty navigation, close/reopen focus, Open page, and nested Escape behavior.
425 focused editor, cache, property, recovery, comment, and hydration tests passed, including disposable-database recovery tests.
Content TypeScript check and all 69 repository guards passed.
Independent technical review found no actionable issues in the persistence, identity, access, and refresh paths. Subsequent title-height, close-transition, and focus-restoration adjustments were checked in the browser and final Playwright run.
Manual browser evidence covers distinct row bodies, scalar property save and traversal, dirty Open page, independent database read-back, reload, and collection → preview → Info → full Page at 390, 768, and 1280px.
Two authorized browser clients showed title/body edits arriving in the untouched preview, with an independent saved-row check.
Final Playwright spec passed all 5 tests with retries disabled (45.4s). The spec runs serially against the isolated SQLite runtime to avoid concurrent fixture write contention.
This PR does not claim production acceptance. Role projections in Playwright mock the browser capability responses; they are not a live multi-user authorization matrix. Provider-backed writeback, full multi-user revocation coverage, and the broader media/Blocks/comment-anchor matrix remain pre-promotion QA. No schema migration or feature flag is introduced, and nothing has been deployed. Main has since added selection-context synchronization to the shared editor; combined behavior needs checking before merge.