fix(admin): stop autosave from retrying a payload the server rejected - #2614
fix(admin): stop autosave from retrying a payload the server rejected#2614danielmlr wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: c5d3ac1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 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
area/admin
size/L
review/needs-review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a focused, well-reasoned bug fix for #2417. The approach is sound: it mirrors the existing autosaveCompletionToken pattern with a new entry-scoped autosaveRejectionToken, pushes HTTP-status awareness into ApiError/isTerminalRequestError, and lets ContentEditor remember the exact serialized state that the server rejected so it doesn't schedule another autosave while the form is unchanged. The split (terminal 4xx, retryable 429/5xx/network) matches the issue's proposal, and manual Save continues to work.
I read the diff and the full changed files, traced the new token through router.tsx, verified the serialization comparison in ContentEditor.tsx, and checked the test coverage at the API-client, router, component, and e2e levels. All changed hunks have a meaningful test, and the test in packages/admin/tests/components/ContentEditor.test.tsx fails without the source change (it asserts one autosave call instead of two). No AGENTS.md conventions are violated: no new user-facing strings, no new logged-out queries, no SQL, no authorization changes, no new indexes, no stale or reviewer-facing comments, and the changeset is a concise, user-facing patch entry that names the affected behavior.
I found no logic bugs, regressions, or gaps. The code is clean and ready to merge.
@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: |
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
github-actions
Bot
added
review/approved
| /** Whether retrying the same request unchanged can never succeed. */ | ||
| export function isTerminalRequestError(error: unknown): boolean { | ||
| return ( | ||
| error instanceof ApiError && error.status >= 400 && error.status < 500 && error.status !== 429 |
There was a problem hiding this comment.
429 isn't the only retryable response. e.g. 408 and 425 should be retried. Can you create a list of retryable responses and exclude them.
There was a problem hiding this comment.
Done. RETRYABLE_CLIENT_ERROR_STATUSES in packages/admin/src/lib/api/client.ts now holds 408, 421, 425 and 429, and isTerminalRequestError excludes that list instead of special-casing 429.
I put 421 in it for the same reason as your two: it judges the connection rather than the body, and RFC 9110 says the client may retry over a different one.
409 and 401 I left terminal. A conflict repeats with the same body, and the admin client has no session refresh, so a resend cannot establish one. If you would rather have either of them retried, say which and I will move it.
github-actions
Bot
added
review/needs-rereview
|
Thanks. Could you resolve the conflicts, then we can get this merged |
0c444ad to
ac24d3f
Compare
A failed autosave left the form dirty and cleared the in-flight flag, so the editor scheduled the next attempt two seconds later and resent the same payload until the tab was closed. For a validation error that can never succeed, each attempt added another error toast. The edit page now signals a 4xx other than 429 to the editor through an entry-scoped rejection token, the counterpart of the completion token. The editor remembers the rejected state and schedules the next autosave only once the content differs from it. 5xx responses and network failures keep retrying as before.
Treating every 4xx except 429 as terminal was too broad. A 408 and a 425 say the request never reached the handler, and a 421 says it reached the wrong one; all three can succeed unchanged on the next attempt. The classification now works off a named list of retryable client error statuses instead of a single exception, so adding one is a list entry.
ac24d3f to
c5d3ac1
Compare
2 participants
What does this PR do?
When the server rejects an autosave with a 400, the editor resends the same payload every two seconds until the tab is closed, each attempt adding another failed-save toast. #2417 records 443 identical PUTs for one entry over 23 minutes; the trigger there was a value past a
maxLengththat nothing in the editor marks while typing.The editor schedules an autosave whenever the form is dirty and nothing is in flight; a failed one leaves it dirty and clears the flag, so the next timer starts at once.
isTerminalRequestErrorreads the status off theApiResponseErrorthatthrowResponseErroralready throws. A client error is terminal unless it is inRETRYABLE_CLIENT_ERROR_STATUSES— 408, 421, 425, 429 — which judge the connection or the rate limit, not the payload. A terminal error advances an entry-scopedautosaveRejectionToken, and the editor schedules no autosave while the form still matches the rejected state. Manual Save keeps working; the next content change autosaves again. 5xx and network failures keep retrying.Part of #2417
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
Revisions
ac24d3f— the retryable statuses are a named list instead of a single 429 exception, with 421 alongside 408 and 425. 409 and 401 stay terminal: a conflict repeats with the same body, and the admin client has no session refresh.ApiResponseErrorcarrying the status, so theApiErrorclass this PR added is dropped andisTerminalRequestErrornarrows on that type. Its status test goes too —preserves status, code, and details on API response errorscovers it.