Add reviewable Suggested Edits to Content - #4274
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 422 Unprocessable Entity: {"error":"plan.mdx:52:297: Could not parse expression with acorn"} Agent output: Repaired |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 8 potential issues 🔴
Review Details
Code Review Summary
PR #4274 adds a feature-flagged, durable Suggested Edits layer for Content, with proposal operations and threads kept separate from canonical document state until an authorized accept decision. The implementation also extends shared review actions, persistence, polling/Yjs synchronization, editor isolation, Comments history, permissions, localization, and documentation. The overall architecture is sensible: decision writes use compare-and-set semantics, accepted content is routed through canonical mutation state, and the feature is default-off. Because this PR mutates canonical user content and adds durable decision/idempotency behavior, I assessed it as High risk.
Key Findings
- 🔴 HIGH — Accepted suggestions do not reconcile the primary Blocks field for allowed system-database documents, leaving database-backed content stale.
- 🟡 MEDIUM — Suggestion create retries and concurrent duplicate creates are not reliably idempotent; decision retries can also fail after the first decision commits.
- 🟡 MEDIUM — Suggesting mode can still persist a pasted title through the paste handler.
- 🟡 MEDIUM — Unsupported structures and inline databases can be introduced through an ostensibly text-only accepted Markdown replacement.
- 🟡 MEDIUM — Suggestion history has an unbounded N+1 query pattern, and the merged history is not chronologically interleaved.
- 🟡 MEDIUM — The non-Postgres transactional sync path can reuse cursors and permanently skip events.
The ACL rechecks, CAS decision claim, canonical/Yjs publication ordering, and default-off flag wiring are good patterns. 🧪 Browser testing: Unable to execute — dev server is healthy, but Chrome MCP/browser automation tools are unavailable in this environment; 17 grounded cases were planned and all were marked environment-unverifiable.
| }); | ||
| await coordination.ydoc.persist(tx, nextContent); | ||
| await coordination.sync.persist(tx); | ||
| }, |
There was a problem hiding this comment.
🔴 Accepted suggestions skip Blocks-field reconciliation
apply updates canonical documents.content and synchronization state, but does not perform the primary Blocks lock/identity reconciliation used by the normal canonical update path. The UI/action combination allows suggestions for system-database documents, so accepting one can leave the database-backed Blocks field on the old Markdown and cause stale or invalid content in database views. Run the same reconciliation inside this acceptance transaction and cover a system-database acceptance case.
Additional Info
Reported by 1 of 4 review agents; confirmed against the changed canonical acceptance path and system-database eligibility.
| if (!adapter) throw new Error("Suggestion adapter not registered"); | ||
| const adapterContext = { ...(ctx as any), suggestionAccess: access }; | ||
| const operations = | ||
| (await adapter.validateProposal({ ...args, ctx: adapterContext })) ?? |
There was a problem hiding this comment.
🟡 Create retries stop being idempotent after the document changes
The action performs current-resource/base-revision and feature-flag validation before looking up the existing creation idempotency record. If the first create committed but its response was lost, a later document edit or flag change makes the same-key retry fail instead of returning the already-created suggestion. Resolve the creation mapping (with the required access check) before state-dependent validation for duplicate requests.
Additional Info
Reported by 2 of 4 review agents.
| }, | ||
| tx, | ||
| ); | ||
| await recordSuggestionCreation(tx, args.idempotencyKey, created.id); |
There was a problem hiding this comment.
🟡 Concurrent duplicate creates can fail on the idempotency key
Two identical creates can both observe no existing creation record before either transaction inserts it, then race on the unique idempotency key. The loser receives a constraint error rather than the winner's suggestion, so a normal concurrent retry is reported as a failed creation. Claim the key atomically or recover the committed winner on conflict and return it after verifying request identity.
Additional Info
Reported by 2 of 4 review agents.
| current.id, | ||
| args.decision, | ||
| ); | ||
| if (!claimed) |
There was a problem hiding this comment.
🟡 Concurrent duplicate decisions are rejected after the first commit
Two requests with the same suggestion, decision, and idempotency key can both read the pending row; after the first CAS claim commits, the second gets zero affected rows and unconditionally throws. This breaks retry-safe decision semantics for concurrent network retries. When the claim fails, reload the suggestion and idempotency record in the transaction and return the recorded same decision, while still rejecting a conflicting decision.
Additional Info
Reported by 1 of 4 review agents; the race follows directly from the CAS failure branch.
| "Pages containing inline databases cannot accept suggestions yet", | ||
| ); | ||
| } | ||
| replacePreparedCollabContent(coordination.ydoc, nextContent); |
There was a problem hiding this comment.
🟡 Accepted Markdown can introduce unsupported content
The exclusion checks inspect the current content and the proposal's before.markdown, but the resulting after.markdown is not structurally validated before it replaces canonical content. A nominal text replacement can therefore add an inline database, media, embed, or other unsupported structure and bypass the v1 suggestion boundary. Validate the parsed resulting document and reject changed unsupported nodes/marks before mutation.
Additional Info
Reported by 2 of 4 review agents.
| }) | ||
| ).rows; | ||
| return ( | ||
| await Promise.all(rows.map((row) => getSuggestion(String(row.id), client))) |
There was a problem hiding this comment.
🟡 Suggestion history performs unbounded N+1 queries
listSuggestions loads all matching IDs and then calls getSuggestion for each, which performs additional suggestion and operations queries. The editor requests unfiltered history without pagination, so long-lived documents cause 1+2N database round trips and unbounded concurrent work while transferring full before/after snapshots. Batch the rows/operations and bound or paginate the history payload.
Additional Info
Reported by 3 of 4 review agents.
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| </div> | ||
| {renderSuggestionCards(historySuggestions)} |
There was a problem hiding this comment.
🟡 All discussions does not merge history chronologically
The combined history renders all matching suggestion cards first and regular comment threads afterward rather than interleaving the two collections by creation time. This makes the “All discussions” view misleading for mixed activity even though its filters apply to both types. Build one filtered, timestamp-ordered history sequence before rendering.
Additional Info
Reported by 1 of 4 review agents; aligns with the PR's merged Comments history behavior.
# Conflicts: # templates/content/app/components/editor/DocumentEditor.tsx # templates/content/app/i18n-data.ts
…ed-edits # Conflicts: # packages/core/src/collab/storage.spec.ts # packages/core/src/collab/storage.ts # packages/core/src/review/store.ts # templates/content/app/components/editor/DocumentEditor.layout.test.ts # templates/content/app/components/editor/DocumentEditor.tsx # templates/content/app/components/editor/VisualEditor.tsx
There was a problem hiding this comment.
Builder reviewed your changes and has a few items to flag 🟡
Review Details
Incremental Code Review Summary
The latest PR head substantially expands the Suggested Edits editor experience with contextual anchor resolution, fail-closed ambiguous matching, inline decorations, proposal-creation locking, narrow-drawer event isolation, exact-thread selection, and improved discussion filtering. The overall direction addresses the previous UX concerns, but this incremental review session was interrupted before the independent code-review workers could return findings, so I am not asserting that any previously open comments are fixed or reposting them. Existing unresolved review threads remain the source of truth until they are verified against the new head.
No new findings are submitted from this interrupted pass. 🧪 Browser testing: Unable to execute — browser automation tooling is unavailable in this environment; the feature remains UI-impacting and should be rerun with Chrome MCP available.
1 participant
Problem
Content editors need to propose changes without rewriting the shared page before review. Reviewers need to see exactly what changed, discuss it, and accept or reject it without losing concurrent edits. This PR adds that document-markdown workflow and repairs the editor, discussion, and persistence failures found while exercising it.
Incomplete checkpoint, pushed at Alice's request. Do not merge. Further investigation is deferred; this is not full product acceptance or a deployment.
Approach
Keep proposed edits separate from the canonical collaborative page until an authorized reviewer accepts them. Render pending changes beside their targets, retain their author and discussion identity, and reject ambiguous or stale application instead of silently overwriting newer content. This remains a document-markdown slice, not generic typed Revision support across every Content object.
What changed
Verification
The final bounded repair passed 37 slash-command tests, 201 editor/media tests, Content typecheck and targeted eligibility regressions. Both localization guards and diff whitespace checks passed. Earlier broader regression results and their scopes are retained in the governing QA note; they are not a claim that a new whole-suite run passed.
Owning-task native browser replay passed the database/page/image command exclusions, direct Generate guard, existing-image read-only controls in Suggesting, ordinary controls after exit, and text proposals beside images. Earlier unaffected desktop/mobile review, recovery and concurrent-edit evidence is retained. Bounded independent technical review closed its final generation-bypass finding; technical review does not override the browser failure below. New-head CI has not been accepted or monitored to completion.
Deferred failures and acceptance gaps
/code, type text, then Stop. The saved Add displays “Preview unavailable” and “Highlight unavailable.” Reject preserved the canonical baseline. Root cause is not established. Resume with this reproduction and verify preview, anchoring, accept/reject and reload after repair. This remains an acceptance failure, not optional post-merge polish.Review focus
Before a later integration, resolve the code-block failure and merge conflicts, refresh affected verification, and examine canonical isolation, permission/basis races, unsupported side-effect boundaries and transaction rollback. No general Suggested Edits completion claim is made.