Skip to content

Keep Content's Builder saves from snapping back - #8083

Open
3mdistal wants to merge 19 commits into
mainfrom
alice/content-builder-real-drafts
Open

3mdistal wants to merge 19 commits into
mainfrom
alice/content-builder-real-drafts

Conversation

@3mdistal

@3mdistal 3mdistal commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Saving a Builder entry from Content now sticks. Before this change, a save to a published entry went out as a Builder autosave. Builder keeps autosaves out of its read API, so the next refresh read the old live values back and the edit snapped back in Content. Draft entries got the same autosave and snapped back the same way.

What changes

  • The save follows the entry's live state. Right before the write, Content reads the entry. If Builder reports it as a draft, Content sends a plain save, which Builder's reads return. If it's published, Content sends an autosave, which leaves the live page alone. Neither request sends a published field.
  • Saved changes survive refreshes. For an autosave, Content records the live values its pre-write check read for each field it wrote, and for the body, the live blocks hash and lastUpdated (__agent_native_builder_unpublished_changes on the row's source values). Each save records its own live read, so a second save after Builder publishes the first still sticks. Refreshes and every body read keep Content's saved value while Builder still reports that live value. A read older than the latest save's live check (a queued body read, say) leaves the saved changes alone. Builder's value wins once Builder publishes, discards, or edits that field, or unpublishes the entry. It also wins when a read made after the save reports meta.hasAutosaves: false. Reference fields compare by id when both sides carry one. An empty body is a body too: saving one keeps it empty, and an empty body Builder publishes replaces Content's saved one and empties the page if nobody has edited it. The record notes when the live body was empty, so an empty read is never mistaken for the body the live check didn't read.
  • The pre-write check is field-level for saves. If Builder's lastUpdated moved since review, a save blocks only when a field it writes no longer holds the reviewed value (or the live value Content saved over), or when the body changed or can't be compared. The entry name, which every write to the safe model also sends, is checked the same way, against the name Builder had at review rather than the title. A save that leaves the title alone sends that name back unchanged, so it can't rename the entry. The block names the field. The check reads every field the save writes, so a custom field the public-key read would otherwise leave out is compared too. A live update in place also blocks when Builder's publication state changed since review, so a publish or unpublish there can't change what was approved. Publish and unpublish stay strict.
  • Each row shows Builder's status. A read-only "Builder status" select property (Published, Draft, Unpublished changes, in Builder's own words) sits first in the table, the board, and the page's info sidebar. Unpublished changes means Content saved changes Builder hasn't published, or Builder reports its own autosaves. Content keeps the value: attach adds the property, even to an empty model, and refresh, body reads, and every save update it in the same transaction that stores the row. A row keeps a status only while some Builder source in the collection still has it, so rows that leave the source lose it, as do rows of a Builder source replaced by a local one or turned into details, and a copy of a row or page doesn't carry it. It's a system property, so it can't be edited, renamed, deleted, or mapped to a Builder field, and setup discovery tells agents so. After a save, the row's stored publication state follows the write: a publish makes it published, an unpublish or a new entry makes it a draft, and a draft save takes the state Builder's reply reports. If the reply leaves it out, the row takes the state the live check found just before the write, unless a refresh read Builder after the check.
  • The save dialog says what each row does. Rows read "Saves the draft" for a draft entry and "Saved, not live until you publish in Builder" for a published one, tagged Save. The footer counts them as "2 saves" rather than "2 draft saves", the button reads "Save 2 to Builder" when every selected row is a save, and Stage only reads "Saves changes to Builder — never publishes." The row, footer, and button labels are now translated. The agent-facing "Stage only" description no longer says it never writes to Builder.
  • A stored gate still matches after the switch. Gate validation treats the two saves as one intent, so a retry after the live check still matches its gate. Stored-response recovery reconciles the request Builder actually received, with the live values recorded before the write, for every write effect. It also keeps the "this change is live" summary.

Limits

  • The live check narrows the race but doesn't close it. A save from someone else that lands between the check and the write is still overwritten. Builder has no conditional write.
  • A draft that someone publishes inside that window takes the change live, because the plain save sends no published field. The execution summary says so.
  • Unpublished changes someone else makes in Builder are invisible to the read API, so Content can't detect a conflict with them. The status shows them as Unpublished changes only if Builder's meta.hasAutosaves flag says so.
  • Every Builder read asks for meta.hasAutosaves, but whether Builder returns it in a read limited to named fields hasn't been checked against a live space. An autosave leaves lastUpdated alone, so hasAutosaves: false only counts from a read with a newer lastUpdated. A discard in Builder that leaves the live value and lastUpdated alone therefore gives Content no signal: the record clears only when a field's live value changes or the entry stops being published.
  • A Builder read that leaves out data.blocks entirely is treated as a read without the body, so it never replaces a saved body.
  • If the record of unpublished changes can't be read, Builder's values win for that row and each refresh logs an error. The saved changes are still in Builder as its autosave, and the next save from Content writes a fresh record.
  • If Content stops after Builder accepts a draft save but before it records the response, the retry sees the change in Builder and blocks instead of writing again. A refresh then shows the saved values.
  • Disconnecting the Builder source clears every status but leaves the empty Builder status property, which can't be deleted because it's a system property.

Follow-ups

  • A stacked PR makes the status editable: choosing Published or Draft stages a publish or unpublish for review. That only applies in "Publish updates" mode with per-item publish and unpublish allowed.
  • A separate PR keeps unsaved Content edits through a refresh, using a per-field three-way merge.
  • After a successful push, the dialog ends on "Checked" and "Ready to send to Builder", because the push response leaves out the change sets it just applied. That's on main already and needs its own fix.

Testing

  • New unit tests cover:
    • the unpublished-changes record, including a second save after Builder publishes the first, reads from before a save, an empty live body, a live body the check didn't read, or an empty published body, a saved body kept as one unit, and reference ids;
    • the draft-save and autosave choice, the live re-pick, and gate equivalence;
    • the field-level check, including an unverifiable body, a reference with the same label but a different id, an entry name Builder changed, an entry name that differs from the title and is kept, the fields the live read asks for, and a live update whose publication state changed;
    • the draft that went live;
    • reconcile recording, and stored-response recovery for a live write and for a draft save that went live;
    • the refresh and body-read keep rule, including a metadata refresh that puts the stored body back, a queued body read from before a save, a reference Builder changed, and an empty body Builder published;
    • body hydration of an empty body saved over a live one, and of an empty body Builder published over a saved one (database tests: without the fix, the old body comes back into the row and the page, and the next refresh queues the read again);
    • hasAutosaves capture, the entry name, and telling an empty body apart from a read without one;
    • the Builder status: written on attach and refresh, added to an empty model, stored in the same transaction as a refresh's rows (a failed status write rolls the rows back), cleared for rows that leave the source, a source a local one replaced, or a source turned into details, while kept for rows another Builder source has, left off copies, followed through a body read (which leaves the row and its status alone when a save changed it during the read), never mapped to a Builder field, reported to agents as a system definition, and read as Unpublished changes when the record can't be read (database tests); the stored publication state after a publish, an unpublish, a draft save that went live, an autosave, and a reply that leaves the state out; and the save dialog's row, footer, and button labels.
  • In templates/content, the Builder source, status, write-settings, resync, lifecycle and row-batch tests and the review dialog tests pass (pnpm exec vitest --run actions/_builder-status-property.db actions/_builder-cms-unpublished-changes actions/execute-builder-source-execution actions/_builder-cms-write-settings actions/resync-content-database-source actions/content-database-lifecycle actions/database-row-batch-actions app/components/editor/database-sources/BuilderSourceReviewDialog).
  • Running actions, app/components/editor, app/lib and shared in templates/content: 6905 pass. The 9 failures are in the comment-AI, block-action and GitHub-source suites, which also failed without this change in the earlier run.
  • pnpm guards: everything passes except dead-settings-keys, which fails on keys in other apps. pnpm guard:i18n-catalogs and pnpm guard:i18n-changed-copy pass.
  • Against the Builder test model, through the guard that allows writes only to that model, on 4f0a354: saving a renamed published entry and a renamed draft sent exactly two PATCHes. Builder's public API then showed the published entry's live title unchanged and the draft's new title, and the rows read Unpublished changes and Draft. The footer and Stage only wording were checked in the running app on da94c0e. The button verb is covered by tests: showing it needs the prepare step, which uploads payload files to the Builder space.
content_product_impact:
  lane: contract_repair
  features:
    - content.feature.trust-your-connected-sources
  capabilities:
    - content.source.sync-policy
    - content.source.builder-codec
  record_change: none
  proof:
    - pnpm exec vitest --run actions/_builder-status-property.db actions/_builder-cms-unpublished-changes actions/execute-builder-source-execution actions/resync-content-database-source (templates/content)
  rationale: Builder saves from Content stop being overwritten by the next refresh, and each row shows Builder's own status, without changing the sync policy or codec contract.

🤖 Generated with Claude Code

A save that should leave the live page alone now picks its request from the
publication state Builder reports right before the write: a draft entry gets a
plain save that Builder's reads return, and a published entry gets an autosave.
Builder keeps autosaves out of its reads, so Content records the live value
each saved field replaced and keeps its saved value through refreshes and body
reads while Builder still reports that value.

The pre-write check is now field-level for saves: when Builder's lastUpdated
moved, a save blocks only if a field it writes changed, or the body changed or
cannot be compared. Publish and unpublish stay strict.
@github-actions

Copy link
Copy Markdown
Contributor

Here's a visual recap of what changed:

Visual recap

Open the full interactive recap

@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 3 potential issues 🟡

Review Details

Review summary

PR #8083 changes Content’s Builder write path to choose between a draft save and autosave using Builder’s live publication state, then tracks live values replaced by autosaves so refreshes can retain Content’s pending edits. It also narrows stale-write conflicts to changed fields and adapts execution gate/recovery comparisons. This is high risk because it changes external data mutation and reconciliation. The approach is thoughtfully covered by focused tests and documents the remaining race without conditional Builder writes, but several paths still lose or incorrectly preserve pending values.

Findings

  • 🟡 MEDIUM — Body autosaves over an empty live body record no baseline, so a later refresh can snap the edit back.
  • 🟡 MEDIUM — Empty-body and pristine body-hydration merges bypass the new preservation helper and can overwrite saved fields.
  • 🟡 MEDIUM — When Builder omits meta.hasAutosaves, discarding an autosave without changing its live field can leave Content’s saved value indefinitely.
  • ⚪ LOW — An unreadable change record is silently discarded in favor of Builder’s value; a corrupt record can therefore revert a saved edit without surfacing a failure.
  • ⚪ LOW — A fresh live-baseline body read uses keepFields: true, which can temporarily retain a saved field after Builder has changed it.

The write-effect selection and gate normalization are coherent, and the tests cover the principal draft/published transitions. 🧪 Browser testing: Will run after this review (PR touches UI code).

Comment thread templates/content/actions/_builder-cms-unpublished-changes.ts Outdated
Comment thread templates/content/actions/_database-source-utils.ts Outdated
}
if (
args.incoming.published !== "published" ||
args.incoming[BUILDER_CMS_HAS_AUTOSAVES_KEY] === false

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.

🟡 Discarded autosaves can remain visible when status is omitted

When Builder omits meta.hasAutosaves, this path keeps Content’s saved value while the live field remains unchanged. If the autosave is discarded in Builder without changing that live field, the record is never cleared and Content continues presenting the discarded value as current.

Fix in Builder

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and I'm leaving this one open as a known limit rather than a fix. When Builder omits meta.hasAutosaves, a discard in Builder that leaves the live value alone gives Content no signal, so Content keeps showing the discarded save. It clears once Builder reports hasAutosaves: false, changes that field, or the entry stops being published. It's listed under Limits in the PR description. Whether Builder's read API returns meta.hasAutosaves still needs a check against a live space.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked against the live space, on the test model only. Builder's read does return meta.hasAutosaves: it reports true on two published QA entries with pending changes, and leaves it out on entries with none. After Content's own autosave on the test entry, reads 12 seconds and 2 minutes later still left it out. So the flag doesn't cover Content's autosaves, and a missing flag can't be read as "nothing pending". The limit stands as described.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

…y an unedited page when Builder's body is empty
builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

…tes, and block live updates whose publication state changed
builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor

Thanks for the update. The preflight remains vulnerable to a time-of-check/time-of-use race: a Builder edit can land after the live read and before Content's unconditional write, then be overwritten. Since Builder offers no conditional write, please keep this blocked until the write boundary prevents the lost update or the product explicitly accepts that risk.

A read-only "Builder status" select property (Published, Draft,
Unpublished changes) now sits first in Builder collections, in the table,
the board, and the page info sidebar. Attach, refresh, body reads, and
saves keep it current, and rows leaving the source lose it. It's never
mapped to a Builder field. After a save, the row's stored publication
state follows the write.

The save dialog's row labels use the mockup's wording and are translated,
and the agent-facing Stage only description no longer says it never
writes to Builder.

@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 1 potential issue 🟡

Review Details

Code Review Summary

This high-risk update adds a read-only Builder status property across collection views and the page info panel, plus localized save-effect labels; it also extends the earlier draft/autosave reconciliation work. The status is derived from persisted source values and updated on refresh, hydration, and successful writes. The approach is broadly consistent, and focused status, execution, and dialog tests pass (78 tests).

Finding

  • 🟡 MEDIUM — A successful publish can leave the new status property showing “Unpublished changes.” Reconciliation updates published and clears Content’s own pending-change record, but retains the pre-publish meta.hasAutosaves: true; the status helper treats that stale flag as authoritative until a later refresh.

The existing unresolved comment about discarded autosaves when hasAutosaves is omitted remains open and was not reposted. No previously open issue was verified fixed in this update.

🧪 Browser testing: Skipped — the browser-test-planner agent is unavailable in this environment.

livePublished: args.plan.payload.livePreflight?.published,
responseBody: args.responseBody,
});
if (published) next.published = published;

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.

🟡 Publish reconciliation retains the stale autosave status

Issue: Reconciliation updates published here, but leaves the existing __builder.meta.hasAutosaves value untouched. If it was true before a successful publish, builderCmsEntryStatus() continues to report unpublished_changes even though the publish cleared Content’s pending-change record and made the entry live. Clear or refresh this flag as part of the publish reconciliation so the new status property reflects the completed action without waiting for another source refresh.

Fix in Builder

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaving this as is. __builder.meta.hasAutosaves is Builder's own report from its last read. Builder's write API doesn't say whether a publish sent through it discards an autosave made in Builder's editor. Clearing the flag on publish would show Published while Builder may still hold those edits. Keeping it can show Unpublished changes until the next refresh, which reads Builder's flag again and settles it. The publish clears Content's own record of unpublished changes either way. The follow-up PR that lets you publish from the status will check on the test model whether a publish clears Builder's autosave, and set the flag from that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The live run on the test model (#8168) couldn't settle this. The entry never carried Builder's flag, before or after Content's autosave, so there was no editor autosave for the publish to clear. The status read correctly after each push and after a refresh. Two other QA entries in the model do carry the flag, but they hold pending Builder changes Content can't see, so I didn't publish over them. Keeping Builder's flag until the next read stays.

…waps

A refresh now stores its rows and their statuses in one transaction, so a
save can no longer land between them and leave the status stale. The status
property is added when a Builder source is attached, even to an empty
model. A save whose reply leaves out the publication state keeps the stored
one; only an autosave falls back to the live check.

Copies of a row or page don't carry its Builder status, and a row keeps a
status only while some Builder source in the collection still has it, so
replacing the Builder source with a local one clears it. Setup discovery
reports the status as a system definition agents can't edit, and the
French save labels say "entrée publiée".
The save dialog's footer counted autosaves and draft saves as "draft
saves" and its button said "Push N updates", while the rows say "Save".
The footer now says "2 saves", the button says "Save 2 to Builder" when
every selected row is a save, and the Stage only description says
"Saves changes to Builder — never publishes." A save to a published entry
isn't a draft. The footer and button labels move into the catalog,
translated in all 10 locales.
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 1 potential issue 🟡

Review Details

Incremental review — PR #8083

Reviewed the latest head da94c0eb2a59209ac7226a67a07168e66c54a6a2 (43 changed files; 5,901 lines) for this high-risk Builder write/reconciliation change. The core architecture continues to separate live preflight from the write, preserve per-field pending values, and expose Builder status through a system property. Focused validation passed: 5 Vitest files, 200 tests.

Finding

  • 🟡 MEDIUM: Empty-body reconciliation can preserve a stale Content body after Builder advances to a newer empty body. The latest code still treats liveBodyEmpty as sufficient to keep the saved body, without first checking whether lastUpdated advanced.

The three existing unresolved comments were checked and remain unchanged: discarded autosaves when the status is omitted; stale autosave status after publish; and preflight using a refreshed source-row baseline. I did not repost or resolve them. This review remains blocking while the prior baseline issue is open and the new body reconciliation defect is present.

🧪 Browser testing: Skipped — the browser-test-planner agent is unavailable in this environment (although the dev server is running).

if (!readHash && builderValues[BUILDER_CMS_BODY_BLOCK_COUNT_KEY] !== 0) {
return true;
}
if (readHash ? readHash === changes.liveBlocksHash : changes.liveBodyEmpty) {

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.

🟡 Empty-body reconciliation ignores a newer Builder timestamp

When Content records an empty live body, this branch returns true for every later empty-body read before consulting liveBodyLastUpdated. If Builder later advances lastUpdated while the body remains empty (for example, publishing or discarding the pending change), Content keeps its saved body and unpublished-change record instead of accepting Builder’s newer state. Only treat the empty body as unchanged when its recorded timestamp still matches the read.

Fix in Builder

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is on purpose, and it matches how a recorded hash and every other field work: Content keeps its saved value while Builder still reports the live value it was saved over. lastUpdated is per entry, not per field. A newer one with the body still empty can just mean Builder saved some other field, and Content's body edit is still unpublished. Dropping the body on any newer timestamp would throw it away in that case. The test "keeps a body saved over an empty live body while Builder's body stays empty" in _builder-cms-unpublished-changes.test.ts covers exactly this, with lastUpdated: 2000 against a record from 1000.

The two cases you mention are handled elsewhere:

  • Publishing Content's change puts Content's (non-empty) body live. The read then has a hash, it doesn't match the empty-body record, lastUpdated has moved, so Builder's body wins. The same test checks this ("Published body").
  • Discarding the change in Builder: a read made after the save with hasAutosaves: false clears the whole record, body included, before this check runs ("drops the record when a read made after the save says nothing is pending"). A discard that leaves both the live value and lastUpdated alone gives Content no signal at all. That's listed under Limits in the PR body.

The timestamp is only a fallback here: it lets a read that hashes the body differently still count as unchanged when Builder saved nothing since.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

@3mdistal

Copy link
Copy Markdown
Contributor Author

On the two findings that are only in the review of 3ab7760:

  • A title-only save blocked by a Builder body edit: this can't happen. The live check only compares bodies when the change itself rewrites the body: its baseline hash comes from the change set's bodyChange, which a title-only change doesn't have. When Builder's lastUpdated has moved, a field-level save checks only the fields it writes, and treats the body as unverifiable only if it writes the body.
  • Saves skip the status property in a collection that doesn't have it yet: intended. A Builder refresh replaces the rows through replaceMockSourceRows, which adds the property and writes every row's status in one pass, so a collection attached before this change gets it on its first refresh. Adding it on a save would show one row's status and leave the rest blank until that refresh.

The draft-save publication state is fixed in 1cdf026; details are on its thread.

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 has a few items to flag 🟡

Review Details

Incremental review — PR #8083

Reviewed latest head defc0fc1270b49c00226741064d9aa02fd4112c0 (44 files; 6,345 changed lines). The empty-body hydration race is fixed: the branch now filters the source-row update by the snapshot's sourceValuesJson, and if that compare-and-set misses, it leaves the hydration queued instead of marking it hydrated. I verified this and resolved the corresponding thread before submitting. Focused regression passed: the targeted hydration concurrency test (1 passed; 39 skipped).

The other three open comments remain unchanged: discarded autosaves may persist when Builder omits status; publish reconciliation can retain a stale autosave flag; and empty-body reconciliation can return before checking a newer timestamp. I did not repost them. No new confirmed issues were found.

🧪 Browser testing: Skipped — browser-test-planner is unavailable; the dev server is running.

This branch has not been deployed

No deployments
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