fix(admin): echo _rev on editor save to prevent silent draft overwrite (#2121) - #2126
Conversation
emdash-cms#2121) The content API returns `_rev` on reads and honours it on writes (409 on a stale token), but the admin never echoed it back — every editor save was a blind write that could silently replace a newer draft revision. - Lift the envelope-level `_rev` onto the item in fetch/create/updateContent - Add `_rev` to ContentItem/UpdateContentInput so it round-trips - Echo rawItem._rev in handleSave/handleAutosave so the server rejects a stale save with 409 instead of overwriting a newer draft
🦋 Changeset detectedLatest commit: 9e741af 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 |
@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: |
There was a problem hiding this comment.
This is the right change for the right problem. The server already supports optimistic-concurrency _rev tokens, and the admin was simply dropping them for content reads/writes. The fix is additive and backwards-compatible: fetchContent/createContent/updateContent now lift the envelope _rev onto the returned item, and the editor's main handleSave/handleAutosave echo it back. A regression test verifies the API-level round-trip, and the changeset is correctly scoped.
However, the fix is incomplete within ContentEditPage. Two other editor-side write paths — author changes and SEO changes — still call updateMutation without _rev, so they remain blind writes that can silently overwrite a newer draft revision. They should echo the same token. I also noted that the existing router tests exercise save/autosave/SEO/author actions but never assert the request body, so an accidental drop of _rev in the router handlers would not be caught.
Changeset, type changes, and test coverage for the API layer look good.
Findings
-
[needs fixing]
packages/admin/src/router.tsx:1185-1207handleAuthorChangeandhandleSeoChangeboth sendsource: "auxiliary"writes without echoingrawItem?._rev. These are still blind writes, so changing an author or SEO field from the editor can silently overwrite a newer draft revision made by another session — the same data-loss bug in #2121. Include the token in bothchangesobjects and add it to the dependency arrays so the callbacks are recreated when the token refreshes.const handleAuthorChange = React.useCallback( (authorId: string | null) => { updateMutation.mutate({ targetId: id, targetLocale: rawItem?.locale ?? activeLocale, source: "auxiliary", changes: { authorId, _rev: rawItem?._rev }, }); }, [activeLocale, id, rawItem?.locale, rawItem?._rev, updateMutation.mutate], ); const handleSeoChange = React.useCallback( (seo: ContentSeoInput) => { updateMutation.mutate({ targetId: id, targetLocale: rawItem?.locale ?? activeLocale, source: "auxiliary", changes: { seo, _rev: rawItem?._rev }, }); }, [activeLocale, id, rawItem?.locale, rawItem?._rev, updateMutation.mutate], ); -
[suggestion]
packages/admin/tests/router.test.tsx:517The existing router tests mock
ContentEditorand exerciseonSave,onAutosave, andonSeoChange, but they only assert UI feedback state, not the outgoing request body. Consider capturing the serialized PUT bodies and asserting that_revis present for editor saves, autosaves, and auxiliary writes. TheglobalThis.fetchwrapper already used inContentNewPagetests can collect these bodies, which would give the router-level regression coverage needed to prevent_revfrom being dropped again.
There was a problem hiding this comment.
This PR makes the right fix to the right problem: the admin content editor now echoes the server's _rev optimistic-concurrency token back on save/autosave, engaging the existing validateRev check and turning silent draft overwrites into a 409 conflict. The approach is idiomatic and backwards-compatible (no _rev = blind write).
I read the diff, the full changed client code, the server content handler/runtime in packages/core, and the existing router.test.tsx coverage. The implementation is clean overall: the envelope-level _rev is lifted onto ContentItem, the mutation types carry it, handleSave/handleAutosave pass rawItem._rev, and the React dependency arrays include it. The changeset is user-facing and correctly scoped to @emdash-cms/admin.
Two gaps keep this from a clean approval:
-
Auxiliary editor writes don't echo
_rev.handleAuthorChangeandhandleSeoChangestill send blind writes throughupdateMutation. They read from the same potentially-stalerawItemand update the same content row, so they should echo_revtoo for consistent stale-write protection. -
The save/autosave
_revwiring lacks an integration test. The newcontent-rev.test.tscovers the API client functions, but if the{ ...payload, _rev: rawItem?._rev }lines inrouter.tsxwere removed the API-level tests would still pass.router.test.tsxalready exercises save/autosave inContentEditPageand should assert that the outgoing PUT body carries_rev.
I did not run the test suite, linter, or typechecker; those claims are unverified.
Findings
-
[needs fixing]
packages/admin/src/router.tsx:1191-1203handleAuthorChangeandhandleSeoChangeboth callupdateMutation.mutatewith changes read from the same potentially-stalerawItem, but they don't echorawItem._rev. That leaves two editor write paths as blind writes; a stale editor can still silently overwrite a newer author or SEO change. Since the update endpoint validates_revfor any provided field, these auxiliary handlers should echo it too.const handleAuthorChange = React.useCallback( (authorId: string | null) => { updateMutation.mutate({ targetId: id, targetLocale: rawItem?.locale ?? activeLocale, source: "auxiliary", changes: { authorId, _rev: rawItem?._rev }, }); }, [activeLocale, id, rawItem?.locale, rawItem?._rev, updateMutation.mutate], ); const handleSeoChange = React.useCallback( (seo: ContentSeoInput) => { updateMutation.mutate({ targetId: id, targetLocale: rawItem?.locale ?? activeLocale, source: "auxiliary", changes: { seo, _rev: rawItem?._rev }, }); }, [activeLocale, id, rawItem?.locale, rawItem?._rev, updateMutation.mutate], ); -
[needs fixing]
packages/admin/src/router.tsx:1162-1182The new
content-rev.test.tsverifies thatupdateContentsends_revwhen given one, but it does not prove thathandleSave/handleAutosaveactually passrawItem._revthrough. If a future edit removes{ ...payload, _rev: rawItem?._rev }, the API tests would still pass and the draft-overwrite bug would regress. The existingrouter.test.tsxalready rendersContentEditPageand triggers save/autosave; add an assertion that the outgoingPUT /_emdash/api/content/posts/post_1body includes the_revtoken from the mocked content GET response.// In packages/admin/tests/router.test.tsx, inside the ContentEditPage describe block: it("echoes _rev from the content GET on editor save and autosave", async () => { const { router, TestApp } = buildRouter(); mockFetch.on("GET", "/_emdash/api/content/posts/post_1", { data: { item: { /* existing post_1 fixture */ }, _rev: "djE6dDE=", }, }); await router.navigate({ to: "/content/$collection/$id", params: { collection: "posts", id: "post_1" }, }); const screen = await render(<TestApp />); await waitFor(() => { expect(screen.getByTestId("mock-title").element().textContent).toBe("Draft Title"); }); const puts: { body: unknown }[] = []; const origFetch = globalThis.fetch; globalThis.fetch = ((input, init) => { const url = typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; if (url.includes("/content/posts/post_1") && init?.method === "PUT") { puts.push({ body: JSON.parse(init.body as string) }); } return origFetch(input, init); }) as typeof fetch; await screen.getByRole("button", { name: "Save", exact: true }).click(); await screen.getByRole("button", { name: "Trigger Draft Sync" }).click(); globalThis.fetch = origFetch; expect(puts[0]?.body).toMatchObject({ _rev: "djE6dDE=" }); expect(puts[1]?.body).toMatchObject({ _rev: "djE6dDE=" }); });
|
This PR has been inactive for 14 days. It will be closed automatically in 7 days if there is no further activity. If you're still working on this, please push an update or leave a comment. |
|
@swissky This is close. Could you give it one final pass so we can merge it:
Small clarification on the existing review: metadata-only writes cannot overwrite draft content, but they should still use consistent concurrency protection. Once those are addressed, this should be ready to land. Thanks! |
|
I've implemented those changes so we can get this in. Thanks! |
|
@ascorbic thank you🙏🏻 |
What does this PR do?
The content API returns an
_revtoken on reads and honours it on writes (409 Conflict on a stale token), but the admin never echoed it back — every editor save and autosave was a blind write. When the editor loaded an entry inside the stale-read window and the user saved, the save silently repointeddraft_revision_idat a new revision built from stale state, orphaning the newer draft with no warning.This makes the editor round-trip
_revso the server-sidevalidateRevcheck actually engages:fetchContent/createContent/updateContentlift the envelope-level_revonto the returned item (the server returns it next toitem, not inside it).ContentItem/UpdateContentInputcarry the optional_revfield.handleSave/handleAutosaveechorawItem._rev, so a save based on a stale read is rejected with 409 and the editor shows a clear error instead of losing work.Autosave stays consistent: it updates the existing draft revision in place (no
updated_atbump on the content table), and the post-save cache invalidation refetches a fresh_rev.Closes #2121
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
packages/admin/tests/lib/content-rev.test.ts— 3 passing: fetchContent surfaces_rev, updateContent sends it, blind writes omit it.