Use the full Page editor in collection previews - #4424
Conversation
This comment has been minimized.
This comment has been minimized.
Visual recap — generation failedThe visual recap could not be generated for this pull request. This is informational only and does not block the PR. Diagnostic: No plan URL: create-visual-recap failed 403 Forbidden: {"error":"Creating an org-visible visual recap requires an active organization. Connect Plan from an organization or publish with private visibility."} Agent output: The recap source has been authored with four UI states (preview before/after, row actions, recovery, unavailable), a persistence-flow diagram, all 29 changed files, and six annotated key-code tabs. I’m doing a final local integrity pass on the JSON and MDX structure; CI will perform the actual publish. stderr: Reading additional input from stdin... |
There was a problem hiding this comment.
Builder reviewed your changes — looks good ✅
Review Details
Incremental Code Review Summary
The latest PR head addresses both previously reported recovery defects. Conflict-draft cleanup now requires the persisted title/body to exactly match the retained draft before deletion, preventing a later no-op save of the winning server version from deleting a user’s conflicted local edit. Draft restoration now requires and uses draft.baseDocumentUpdatedAt, preserves loadedContentWasEmpty, and keeps the draft available when the compare-and-swap restore or versioned deletion fails.
The additional update-document CAS tests and focused recovery coverage are good safeguards, and the preview save barrier still coordinates primary Page content, mounted Blocks fields, and scalar properties before transitions. Four independent balanced reviews found no new confirmed actionable issues; one repeated delete-error concern is handled by the shared DocumentEditor delete callback’s existing error toast and was not re-reported.
Risk remains standard because this is shared editor persistence and collection-preview state. The two prior review threads were verified fixed and resolved before this review.
🧪 Browser testing: Dev server is healthy, but full visual verification remains blocked because Chrome automation tools were unavailable to all executors. An 18-case plan covering preview parity, persistence, recovery, responsive layouts, traversal, deletion, and reload/back behavior was produced; all cases were marked environment-blocked.
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. Cleanup compares the retained payload with acknowledged saved content, so a no-op save cannot erase a different conflict draft. Restoration uses the draft’s original document version and guards stale titles even when the body is unchanged.
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
Merge resolution and recovery fixes: 268 focused editor/hook tests, 18 PGlite recovery/update database tests, TypeScript, all 71 current guards, and changed-file lint/format passed. Independent recovery review passed. After the latest main integration, 99 layout/toolbar tests and TypeScript passed. Independent headed-browser acceptance passed on
df9987f2c6: mobile editing/traversal/focus/Open page, desktop preview/selection/sidebar controls, stale-draft protection and exact discard, and successful current-draft restoration, with saved-data readbacks.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 deployment has not been verified. Current main has been integrated, including editor selection context, access-aware loading, database refreshes, and the PGlite local backend.