fix(admin): ratchet content write revision forward on clean refetches - #3831
emdashbot[bot] wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: fe55832 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 |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR fixes the reported stale-revision downgrade by ratcheting _rev tokens by decoded version and suppressing adoption while the editor is dirty. The core logic is sound and well tested for the happy-path and explicit dirty/clean cases. I found two issues worth addressing before merge, neither of which is a hard blocker:
-
Race in dirty-state propagation. The editor reports dirty changes through
useEffect, so the parent can still seeisEditorDirty === falsefor a brief window after the editor has actually become dirty. Because theContentEditPagegates token adoption on!isEditorDirty, a poorly timed refetch (window focus, background invalidation, or an out-of-order response) can adopt an older server_revbefore the dirty signal arrives. This leaves the same class of bug the PR is fixing, just narrower. -
Fragile coupling to the server’s opaque
_revformat. The admin client now decodes_revwithatoband assumes the exact server encoding. If the server ever changes the format (base64url, extra fields, non-base64),decodeContentRevreturnsnullfor all newer tokens and the admin will refuse to ratchet forward, sticking to a stale token until the user hits a conflict and uses the recovery path. Centralizing the decoder (or having the server expose a monotonic version field) would make the admin less tightly coupled to the token serialization.
Both are relatively contained; the second is a maintainability/forward-compat risk and the first is a real but edge-case timing window.
| const onDirtyChangeRef = React.useRef(onDirtyChange); | ||
| onDirtyChangeRef.current = onDirtyChange; | ||
| React.useEffect(() => { | ||
| onDirtyChangeRef.current?.(isDirty); |
There was a problem hiding this comment.
[needs fixing] onDirtyChange is fired from a useEffect. That means the parent only learns the editor is dirty one React commit after the editor computed isDirty === true.
In ContentEditPage, the token-adoption check in router.tsx depends on !isEditorDirty:
if (
rawItem?._rev &&
!isEditorDirty &&
adoptNewerRevision(...)
) { ... }So if a refetch resolves in the brief window after the user makes a change but before this effect notifies the parent, the page can still adopt an older _rev while the editor has unsaved local changes — undermining the fix.
Options:
- Flush the dirty signal earlier/asynchronously in a way the parent can observe before the adoption logic runs. A synchronous ref or a callback invoked from the same event path that dirties the editor would close the window more reliably than
useEffect/useLayoutEffect. - At minimum, document the remaining race and add a test that schedules a refetch between the editor-dirtying event and the parent re-render.
| } | ||
| if (rawItem && !revisionTokensRef.current.has(rawItem.id)) { | ||
| if ( | ||
| rawItem?._rev && | ||
| !isEditorDirty && |
There was a problem hiding this comment.
[suggestion] The adoption guard mutates revisionTokensRef.current directly during render:
if (
rawItem?._rev &&
!isEditorDirty &&
adoptNewerRevision(...)
) {
revisionTokensRef.current.set(rawItem.id, rawItem._rev);
}Mutating refs during render is a side effect and can be unsafe with React 18/19 concurrent features (tearing, double invocation in Strict Mode). If the timing requirement is only that the next user-triggered save sees the new token, moving this into a useLayoutEffect would be safer and still run before paint/event handlers. If an even earlier guarantee is needed, consider deriving the candidate token in event handlers instead.
|
|
||
| /** | ||
| * Decode an EmDash content `_rev` token into its version and updatedAt. | ||
| * Tokens are opaque to callers but have a stable base64("version:updatedAt") | ||
| * format generated by the server. | ||
| */ | ||
| function decodeContentRev(rev: string): DecodedContentRev | null { |
There was a problem hiding this comment.
[suggestion] The admin client now hardcodes the server’s opaque _rev format (base64("version:updatedAt")) and decodes it with atob. This couples the admin to the server token serialization.
If the server format changes (e.g., base64url, additional fields, or non-base64 encoding), decodeContentRev will return null for every newer token, adoptNewerRevision will refuse to adopt, and the editor will stay on a stale token until a conflict forces recovery.
Prefer reusing the server’s own decoding helper if @emdash-cms/core is available to the admin bundle, or ask the API to expose a stable monotonic version field (e.g., _revVersion) so the client can ratchet without knowing the token encoding.
@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
@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: |
danielmlr
left a comment
danielmlr
left a comment
There was a problem hiding this comment.
The reporter has confirmed the original OG-image fix. Before merging, please add a regression using the real ContentEditor: make unsaved edits, refetch a newer server revision, and verify that the local edits remain, the original write revision is retained, and saving detects the conflict. Include the case where a local edit and a refetch result arrive together.
The current dirty-editor test mocks ContentEditor, so it does not exercise real form synchronization or dirty-state propagation. From reading the real editor’s synchronization effect, a refetch can replace form data and reset its saved baseline. Please fix any failures exposed by this regression and keep the existing clean-refetch and stale-read tests passing.
CompletedRevised and published fix for #3829. Branch bot/fix-3829; all targeted checks pass.
Outcome: Revised and published the fix for #3829 in response to maintainer review feedback on PR #3831. What changed:
Verification:
The candidate is published to `bot/fix-3829` (commit `423890faffa14636d982ea9e7327f2df7f921b43`). Mode: revise View live dashboard · Run: |
2e1f231 to
423890f
Compare
CompletedInvestigate issue #3829 in revise mode: address maintainer review comments by documenting the synchronous dirty signal and adding a regression test for the refetch-between-dirty-event race.
Outcome: Revised the candidate for #3829 to address the maintainer review feedback on PR #3831. What changed in this revision:
The underlying fix was already present in the applied candidate and remains unchanged:
Verification:
No unrelated files were modified. Mode: revise View live dashboard · Run: |
423890f to
3040550
Compare
CompletedRevise the cached-revision-token fix for #3829 to ratchet by versions decoded from _rev instead of relying on a server version field, keeping clean-refetch adoption, dirty retention, and non-downgrade protection.
Outcome: Revised the candidate fix for #3829. The previous implementation ratcheted the write token using a newly exposed `version` field on the admin `ContentItem` type. This revision removes that API-surface change and instead decodes the version from the opaque `_rev` token (`base64("version:updatedAt")`) directly in the admin router. What changed:
Verification:
Published to `bot/fix-3829` (commit `fe558320371f86d99b7228a10d51896a78b62c37`). Mode: revise View live dashboard · Run: |
…#3829) EmDash-Run: 08979f5b-fb02-42bf-bf29-55ceeecdc5c1
3040550 to
fe55832
Compare
Timed outImplementing: use item.version, guard ContentEditor refetch while dirty, add real-ContentEditor regression tests. Now verifying.
Outcome: The run stopped at its execution deadline before it could provide a checkpoint summary. The conversation and workspace are saved. A maintainer can continue with `@emdashbot retry`. Mode: revise View live dashboard · Run: |
1 participant
What does this PR do?
Fixes #3829.
ContentEditPagestored_revonly on the initial load and kept the cached older token after TanStack Query refetched a newer clean entry. SEO and other auxiliary saves then sent the stale token, producing 409 CONFLICT.Fix
ContentEditornow exposesonDirtyChangeand reports its dirty state to the page.ContentEditPagedecodes_rev(formatbase64("version:updatedAt")) and ratchets the stored write token forward only while the editor is clean and the fetched revision is newer. This preserves genuine conflict protection for unsaved edits and prevents a delayed old GET from overwriting a token from a newer successful write.Tests
Added
ContentEditPage – revision token synchronizationcoverage inpackages/admin/tests/router.test.tsx:Verification
tests/router.test.tsx+tests/editor-save-conflict.test.tsx: 44 passed.pnpm run typecheck(admin): passed.pnpm build(root): passed.pnpm format: applied.pnpm lint:quick: zero diagnostics.@emdash-cms/adminchangeset added.Closes #3829.
Try the candidate against your own site with the preview build:
Opened automatically by emdashbot. A maintainer must review before merge.
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