Repository navigation
Conversation
…ones Builder changed too
|
Here's a visual recap of what changed: Open the full interactive recap |
…t-builder-refresh-keeps-edits
…ting' into alice/content-builder-refresh-keeps-edits
…clears the field's conflict
…ting' into alice/content-builder-refresh-keeps-edits
…and lock the row before the hydration queue
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🟡
Review Details
Code Review Summary
This PR preserves Content-side Builder CMS edits across refreshes, reconciles page titles and mapped properties against their prior Builder baselines, tracks per-field/body conflicts for review, and applies “set back to Builder” edits transactionally. It also hydrates mapped values and titles from body reads. The approach is sound in principle, and the added PGlite coverage exercises many edit, conflict, and staging races. I classified this as high risk because it changes data mutation and synchronization behavior. The previously open Title-mapped-property stage-back issue is fixed: filtering now uses the local field identity, and that thread was resolved.
Findings
- 🟡 MEDIUM: Empty-body hydration preserves and flags a previously synced body even when Content did not edit it.
- 🟡 MEDIUM: Body conflict decisions use a pre-lock document read and can miss a concurrent Content save.
- 🟡 MEDIUM: Stage-back can reject a now-empty change set, then a stale review decision can approve it again.
- 🟡 MEDIUM: A concurrent rejection can leave the staged property value without an open review item.
- 🟡 MEDIUM: Refresh can materialize values using a field-mapping snapshot that became stale before the transaction.
- Lower-priority observations on lock duration, malformed source-value JSON handling, and page-lock order are included for completeness.
No tests were independently run; the PR description reports 892 Content tests passing. 🧪 Browser testing: Will run after this review (the reconciliation changes affect user-visible Content pages and review flows).
| args.bodyKept && | ||
| args.localContent !== args.nextContent && | ||
| (previousBodyUnknown || | ||
| previousContent !== args.nextContent || |
There was a problem hiding this comment.
🟡 Unedited body Builder empties is kept and flagged as a conflict
When Builder reports an empty body, this path keeps any non-empty local body and marks it conflicted whenever the previous Builder body differs. A page whose body was synced and never edited therefore retains text Builder deleted and offers to push that stale text back. Compare the local body with the previous Builder baseline here; only preserve and flag it when it was edited or the baseline was unknown, and add a regression test for a known, unedited body Builder empties.
| nextValues = withBodyReadEditConflict({ | ||
| previousValues: sourceValues, | ||
| nextValues, | ||
| bodyKept: !shouldWriteBody, | ||
| localContent: currentContent, | ||
| nextContent, | ||
| }); |
There was a problem hiding this comment.
🟡 Body conflict detection uses content read before the membership lock
currentContent is read before this transaction takes the membership lock, and the keep-body path has no content CAS. A Content save between that read and lock acquisition can make the conflict decision stale, potentially persisting no conflict for a local edit that now differs from Builder. Re-read the document body under the lock and base the keep/write decision and conflict marker on that locked value, or abort/retry if it changed.
| state: | ||
| fieldChanges.length === 0 && !changeSet.bodyChangeJson | ||
| ? "rejected" |
There was a problem hiding this comment.
🟡 Stale review decisions can re-approve a now-empty change set
When setting back the final field/body change rejects this change set, a review request that read the earlier open state can still update the row by ID after waiting on this transaction, overwriting rejected with approved. That leaves an approved change set with no changes to execute. Make review decisions conditional on the state/payload they read, or otherwise serialize them with this transition and handle a failed compare-and-set.
| inArray(schema.contentDatabaseSourceChangeSets.state, [ | ||
| "pending_push", |
There was a problem hiding this comment.
🟡 Concurrent rejection can leave a staged value without a review item
This locked query excludes a change set rejected after the staging action's earlier snapshot, but the transaction still writes the staged property and only updates change sets returned here; it does not create a replacement review item. Because review decisions do not take the collection lock, this interleaving can report a staged value with nothing pending to push. Serialize rejection with staging or atomically revalidate/create the review item before committing the property write.
| await materializeSourceFieldPropertyValues({ | ||
| database: { ownerEmail: args.ownerEmail }, | ||
| sourceId: args.sourceId, | ||
| fields: args.fields, |
There was a problem hiding this comment.
🟡 Refresh can rematerialize properties using a stale field mapping
args.fields is read before replaceMockSourceRows enters the transaction, then used here to materialize Builder values. If a concurrent unbind/remap commits between that read and this write, refresh can repopulate a property that is no longer mapped, undoing the unbind and restoring removed values. Read the current mappings under the collection mutation lock inside this transaction, or validate a mapping version and retry.

Stacked on #8168. A Builder refresh treats Builder as the only source, so it drops Content's edits that haven't been pushed yet:
What changes
Each writer replaces only the conflict keys it owns: a refresh its mapped fields and the title, a body read the body, a push what it sent. The title has an entry of its own, so a property named "Title", which maps to Builder's title field too, doesn't clear the title's flag or the other way round. A body read takes the row's lock before its queue row, in the order queuing a read takes them.
Not covered
Live check
Run against the
agent-native-blog-article-testmodel, with a local Content server behind a guard that allows only that model. Content made no Builder writes. Builder-side edits went through Builder's MCP.Checks
templates/content: 892 tests in 35 files pass. They cover every Builder adapter, source, review, push, refresh, stage, and body hydration test file, plusBuilderSourceReviewDialog.ui,body-hydration,builder-body-hydration-pump,use-content-database,db.spec, and the newbuilder-refresh-local-edits.db. Each part fails its own test when reverted on its own: the review flag, the title rule, the body flag, the record carried through a rebuild, the body read's property and title writes, the bulk read's property write, the empty-body flag (its rule, its known-empty marker, and the kept-body check), and the stage that sets an edit back (its value, its conflict clear, its reread of the review item, its recheck of the row, its push check for setting back and for staging, its recheck of staged rows, and its single commit with the staged rows), the title's own conflict entry, the set-back keeping a page-title change, and the body read's lock order.tscis clean apart from the existingvirtual:react-router/server-builderror.pnpm guardspasses apart from the existingdead-settings-keysbaseline. No copy changed.🤖 Generated with Claude Code