Repository navigation
fix(admin): stop saving an outdated rich text body over newer content - #4418
Conversation
The rich text editor reads its value only when it mounts. When the edit page picked up a newer version of an entry while the form had no unsaved edits, the form took the new data and the newer _rev, but the body kept the old document, so the next keystroke autosaved it over the newer version without a conflict. Discard changes had the same shape. Remount the field editors when the form takes a changed Portable Text value on a background read or a discard, and keep them on resets that follow the writer's own save. Mark discard resets in the router, close an open block panel on remount, and cancel and re-read a content read that is in flight when an autosave lands, so a late older read can't bring earlier text back.
🦋 Changeset detectedLatest commit: e39a0e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
upgrade-emdash
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-ai-search
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
There was a problem hiding this comment.
The approach is sound: reusing the existing fieldsGeneration remount key so the mount-initialized rich text editor picks up replacement content, and cancelling stale reads, fits the existing restore design. The two reviewers agree on this. They differ on whether it is complete. One found nothing to change, having checked the effect branches, the discard flow, the tests and the changeset. The other found one gap: a read after an explicit save is assumed to hold the writer's own body. If another writer's newer body arrives in that read, the editor can keep the older document while the route takes the newer _rev, and the next keystroke would overwrite it without a conflict. That finding is kept as needs_fixing. The first reviewer did not address this case and treated the own-save exemption as correct, because it keeps text typed while a save is in flight.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol.
🚀 Deploying Preview to Cloudflare 🚀Preview URL: https://noah-silly-euclid-c11b7a.try.emdashcms.com, https://noah-silly-euclid-c11b7a-emdash-playground.emdash-cms.workers.dev (commit e39a0e6)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
A save's reload can carry another writer's save that landed first, or a content:beforeSave change. The editor kept its older body while the form and the route took the newer version, so the next keystroke saved over it. Keep the editor only while the form has edits newer than the last save, which is what text typed during a save leaves behind.
There was a problem hiding this comment.
The approach is sound. Remounting the field editors through the existing fieldsGeneration key fits a rich text editor that only reads its value on mount. Cancelling and re-reading stale content reads also fits the existing design. Both reviewers approve and raise no findings. The earlier finding is fixed: a save's reload now remounts the editor when the body changed and nothing was typed after the save. The editor is kept only when edits are newer than the save. A route-level test covers another writer's body arriving in a save's reload, and a second test covers typing during a save. I did not resurrect the earlier own-save exemption finding, because both reviewers confirm it is resolved at this head.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol.
danielmlr
left a comment
There was a problem hiding this comment.
Thanks for this! A few cases still get through:
- I think the keep-or-replace call needs to happen per rich text field rather than for the whole form. At the moment the
!isDirtyRef.currentcheck keeps the body editor whenever any field is dirty. So if the title is edited while a save is in progress, and another tab changes the body in that window, the editor holds on to the old body while the form takes the newer one. The next keystroke in the body then saves the old text over it. Keeping the editor only when that field has edits the save didn't send, and replacing it otherwise, would close that gap. - When an editor is kept, the form probably wants to keep its own value for that field rather than
item.data. Right now the form gets the reloaded body and counts as saved, so the next keystroke goes out with the newer_rev. That can overwrite another tab's body without a conflict notice. With the form left dirty, the edit would go out with the old_revinstead. A save from another tab in between would then show the conflict notice, and otherwise the edit just goes through. It would also stop text typed during a save from showing as Saved. - Rich text inside a
blocksfield has the same problem, though it isn't new with this PR.changesPortableTextonly looks at top-levelportableTextfields, so a block's rich text keeps its old text after a background read or a discard, and saves it back on the next keystroke. Comparingblocksvalues as well should be enough. I'm happy for this one to be a follow-up if you'd rather keep the PR focused. - It'd be great to have route tests for these in
editor-refetch.test.tsx, like the ones already there: a title edit during a save, typing in the body during a save, and a block's rich text after a background read.
A save's reload kept every rich text editor whenever any field was dirty, so a title edit during the save let another tab's newer body slip past a stale editor. Rich text typed during the save showed as saved, and the next keystroke went out under the newer _rev. Decide per field: one with edits the save didn't send keeps its editor and its form value, so the form stays dirty and a conflicting save gets the conflict notice; any other takes the reloaded value and remounts on its own. Treat blocks fields like rich text, since rich text inside a block also reads its value once.
There was a problem hiding this comment.
The approach remains sound: per-field remounting refreshes read-once editors without replacing an editor that holds newer local edits.
Reviewed the follow-up diff, save/reset and dirty-baseline handling, field keys, added route-level regressions, and updated changeset. The earlier save-reload finding remains addressed. The follow-up covers independent rich text fields, rich text inside blocks, edits made during a save, and the subsequent conflict path. I found no new issues to fix in the changed code.
Static review only; I did not run tests, builds, or lint.
Follow-up review by openai/gpt-6.1-sol, scoped to the changes since the last review.
danielmlr
left a comment
There was a problem hiding this comment.
Thanks! One thing I think is worth fixing before you merge:
Saving after picking an image in a block remounts the whole blocks field, so collapsed blocks open up again. Main doesn't do that, and it's a pretty common flow. The server sends image values back with their keys in a different order, and changedReadOnceFields compares with JSON.stringify, so it sees a change that isn't there. I think comparing with equalJsonValues from PortableTextEditor.tsx, which ignores key order, would fix it. A route test for this would be great too.
What does this PR do?
Fixes the entry editor silently saving an outdated rich text body over newer content.
The rich text editor reads its value only when it mounts. When the edit page picked up a newer version of an entry while the form had no unsaved edits, the form took the new data and the newer
_rev, but the body kept the old document:ctx.content.update, or an API write._rev, but the body still shows "Hello"._rev, the server accepts it, and "world" is gone without a conflict notice.Discard changes had the same shape: the body kept the discarded text, and the next keystroke brought it back.
Changes:
ContentEditorremounts the field editors (thefieldsGenerationkey that restore already uses) when it takes the item's data and a rich text value changed, including rich text insideblocksfields: on background reads, discards, and a save's reload that brings a different value (another writer's save landing between the save and its reload, or acontent:beforeSavechange).blocksfield with edits the save didn't send keeps its editor and its form value, so the form stays dirty and the next save sends them under the save's_rev. If the entry changed elsewhere in the meantime, that save gets the conflict notice instead of overwriting it.resetEditorItem(id, { discarded: true })), so a failed discard can't change how a later save's reset is handled.patchAutosaveQueriescancels and re-reads a content read that is in flight when an autosave lands, as it already does for the revision lists. Otherwise a read sent before the autosave could land after it and reload the body with the older text.Found while reading the editor code. The new tests reproduce it on
main.Type of change
Your connection to this change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI assistance disclosure
Screenshots / test output
Not applicable: no visual change.
New tests in
editor-refetch.test.tsx(route level, real editor) andContentEditor.test.tsx. Onmain:"keeps the body's own autosave when an older read lands after it" passes on
mainand fails without the router change. With this change the admin suite passes (258 files, 3,751 tests).