fix(admin): let the editor recover from a save conflict instead of resending a refused token - #2902
fix(admin): let the editor recover from a save conflict instead of resending a refused token#2902danielmlr wants to merge 2 commits into
Conversation
…sending a refused token The first refused save left the rejected revision token from emdash-cms#2860 in place, so every later save and autosave resent it and the writer could not get their work in at all. The editor now takes the server's current token when a save comes back as a conflict, and suspends autosave for that entry until the writer decides. Without the suspension the queued autosave writes over the other version as soon as the token is refreshed, which is the silent overwrite the token was added to prevent. When the recovery read fails there is no newer version to offer, so the notice goes and the save error is reported rather than swallowed. Discarding the draft takes the notice down as well.
🦋 Changeset detectedLatest commit: de610f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 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 |
github-actions
Bot
added
review/needs-review
@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-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@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: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approach judgment: This is the right fix for the right problem. Letting the editor detect a 409 CONFLICT, fetch the server’s current _rev, pause autosave, and offer an explicit “save over” action matches EmDash’s opt-in concurrency model and prevents the rejected token from being retried blindly. The changeset is user-facing and specific, the UI strings are Lingui-wrapped, and the new RTL-safe Kumo Banner avoids directional markup.
What I checked: I read the diff and the full changed files, traced the _rev flow from fetchContent/updateContent through revisionTokensRef, verified the server returns CONFLICT for stale revision checks, reviewed the test file against the existing browser-test utilities, and checked the changeset against .changeset/README.md.
Headline conclusion: the implementation is sound and the tests cover the main regression paths (autosave pauses, token updates after a successful recovery read, failure when the read fails, and discarding the draft clears the notice). There is one real bug in the conflict banner action: it clears the notice before it knows the save can actually start, which can leave the editor without the conflict affordance when submitSave is blocked.
| onClick={() => { | ||
| onDismissSaveConflict?.(); | ||
| submitSave(); | ||
| }} |
There was a problem hiding this comment.
[needs fixing] The Save anyway action dismisses the conflict banner before submitSave() has actually attempted the save. submitSave() can return early when another save is in flight, a URL field is invalid, or an unsupported Portable Text mark is present, so the banner can disappear while the stale token and conflict remain.
Move the save attempt before dismissing; the parent mutations already clear conflictedEntryId on success, so the banner will still go away once the save is confirmed.
| onClick={() => { | |
| onDismissSaveConflict?.(); | |
| submitSave(); | |
| }} | |
| onClick={() => { | |
| submitSave(); | |
| onDismissSaveConflict?.(); | |
| }} |
There was a problem hiding this comment.
Taken, and the diagnosis is right, but the suggested patch does not change the
behaviour it describes. Reordering the two calls still runs
onDismissSaveConflict?.() unconditionally, so an early return from
submitSave() still leaves the writer without the banner.
There is also a worse consequence than a lost affordance. A banner still on
screen means recoverFromSaveConflict has already swapped the entry's token for
the server's current _rev; a failed read clears the notice instead. Clearing
hasSaveConflict lifts the autosave guard, and nothing raises it back. Not at
once, since the conditions that made submitSave return early guard autosave
too, but the first autosave after one of them clears writes against a token the
server now accepts, and the other version is gone without anyone choosing it.
That is what the suspension was added for.
The comment's prose has the fix its patch does not: both content mutations
already clear conflictedEntryId in onSuccess. So the action only saves now.
The notice goes when a save is confirmed and stays whenever one is not, whether
submitSave returned early, the request failed, or the retry hit a second
conflict. That left onDismissSaveConflict without a caller, so it is gone too.
This is a separate commit, with a test that fails on the old handler: it puts
an invalid URL in a field, clicks Save anyway, and asserts the banner is still
there and no second PUT went out.
The Save anyway action cleared the notice and then called submitSave, which returns early when a save is in flight, a URL field is invalid, or an unsupported Portable Text mark is present. On that path the writer lost the banner without a save being attempted. Worse than losing the affordance: the recovery read has already refreshed the revision token, so clearing hasSaveConflict also lifts the autosave block. The next keystroke autosaves against the fresh token and succeeds, overwriting the other writer silently, which is what the suspension exists to prevent. The action now only saves. Both content mutations already clear conflictedEntryId in onSuccess, so the notice goes when the save is confirmed and stays whenever it is not. That leaves onDismissSaveConflict without a caller, so it is removed.
github-actions
Bot
added
review/needs-rereview
1 participant
What does this PR do?
Restores saving in the content editor after someone else changes the same entry.
The first refused save used to leave the rejected revision token from #2860 in
place, so every later save and autosave resent it and the writer could not get
their work in at all.
The editor now takes the server's current token when a save comes back as a
conflict, leaves the form untouched, and shows a banner offering to save over
the newer version; the banner stays until a save succeeds. When the recovery
read fails the refused token stays, since dropping it would make the next save
a blind write, so the banner goes and the error is reported. Discarding clears
it too. Saves that do not conflict are unchanged.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
The notice in the content editor after another writer saved the same entry. What
the writer typed is still in the field, and the action offers to put it over the
newer version.
The same notice in the Arabic locale. The banner and its action follow the
right-to-left direction without any change to the markup.