Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): tell the operator a 503 means the commit store is unreachable, not commits HTTP 503 - #4146
Merged
Conversation
…eachable (#3529) `commitHistory.ts` flattened every non-OK response to a bare status code (`commits HTTP {status}` / `HTTP {status}`). Nothing was swallowed and no fictional "no history" was ever rendered — those fail-loud properties held and still hold — but the meaning the backend already sends was lost on the one screen where it matters most: the rollback surface, read mid-incident. Failures now throw a `CommitStoreError` carrying `status`, the ADR-0112 `code` and a `retryable` flag, and `CommitTimeline` renders a sentence instead of a number. 404, 500 and 503 stay tellable apart. The revert half gets a different sentence on purpose: a write that could not reach the store may still have landed, and re-issuing appends a second revert commit to an append-only log. Two details of the report were checked against the producer and came back different, which is why the copy is authored client-side: the semantic code arrives at `error.code`, not `details.code` (objectstack#3842 lifts it and drops `details`), and the envelope `message` for this class is withheld as the generic `Internal server error` (`declaresServerFault`, objectstack#5811). Classification keys on status first, so a proxy-shed 503 with an HTML body still reads retryable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
yinlianghui
marked this pull request as ready for review
August 10, 2026 14:04
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#3529
The build-history panel is the rollback surface — an operator reads it mid-incident. It answered every failed commit-store call with a bare status code (
commits HTTP 503), so an outage and a missing package looked identical while their dispositions differ.What changed
packages/app-shell/src/preview/commitHistory.ts—fetchCommitsand the same-shapedrevertCommitnow throw aCommitStoreErrorcarryingstatus, the ADR-0112code, and aretryableflag. Classification keys on the HTTP status first and treats the envelope code as a second signal.packages/app-shell/src/preview/CommitTimeline.tsx— the panel renders a sentence rather than a number, and the revert toast gets its own sentence.preview.history.loadFailedUnavailable,preview.history.revertUnavailable. Neither takes interpolation arguments.retryableis deliberately not "any 5xx". Only 503 /SERVICE_UNAVAILABLEmeans "this did not happen"; a 500 is the server saying it broke while doing the work, which a retry does not answer. 404, 500 and 503 stay tellable apart.The revert half says something different on purpose. A write that could not reach the store may still have landed — a proxy can shed a 503 after forwarding — and re-issuing appends a second revert commit to an append-only log. So its copy asks the operator to re-read the timeline before retrying, instead of "try again".
Two premise details did not survive checking, and they are why the copy is authored client-side
The card's core premise holds: the retryable meaning was being flattened. Two supporting details in the body are wrong at the browser boundary, and both strengthen the fix rather than weaken it.
1. The code arrives at
error.code, notdetails.code.HttpDispatcher.errorFromThrownparks the thrown error's.codeindetails(objectstackpackages/runtime/src/http-dispatcher.ts:718), andbuildApiError/splitSemanticCodethen lift it intoerror.codeand dropdetailsentirely when it held nothing else (packages/runtime/src/error-envelope.ts:100-125, objectstack#3842). The source says so itself athttp-dispatcher.ts:704. A consumer readingdetails.codewould be running a check that can only ever pass vacuously.2. The envelope
messagefor this class is withheld. The card expects the 503 prose to say "unknown".declaresServerFault(objectstackpackages/types/src/error-leak.ts:113) is true for exactly this error — status at or above 500 with a string code — sodispatcher-plugin.ts:521replaces the sentence with the genericINTERNAL_ERROR_MESSAGE("Internal server error") whileerror.codesurvives. Rendering the envelope message would have shown the operator a generic string and lost the status: strictly worse than the bare code it replaced. Hence the retryable copy is authored on the client side, and the envelope message is preferred only for the statuses where it is not withheld (4xx) — which is what keeps a 404 reading differently from a 500.No alias chain was added beside
error.code. A tolerant read of a shape this endpoint does not send is the consumer-side leniency Prime Directive #12 removes.Reverse verification — predicted, and the prediction has two halves
Prediction recorded before running: the classification and presentation cases go red; the fail-loud cases stay green, because
origin/mainalready failed loud (the issue body says so, and the triage comment confirmed it) — those pins are regression guards, not evidence for this change.Both halves confirmed. Restoring both source files to
origin/mainand keeping the new tests: 27 of 37 red, 10 green. The 10 that held green are exactly the five fail-loud cases, the two success paths, the two pre-existingrevertCommitbehaviours, and the "three different messages" case — that last one passes on both sides, sincecommits HTTP 404/500/503are also three distinct strings.One honest caveat: the three fail-loud cases in
CommitTimeline.test.tsxdid go red on the revert, but as an artifact — they anchor on the newdata-testid="commit-history-error", which does not exist onorigin/main. They are genuine guards going forward; the load-bearing fail-loud evidence is thecommitHistory.test.tsblock that stayed green across the revert.Verification
pnpm vitest run packages/app-shell/src/preview/— 6 files, 58 tests passed (37 new).pnpm --filter @object-ui/app-shell --filter @object-ui/i18n type-check— Done, after building the closure withpnpm --filter '@object-ui/app-shell^...' build(the first run was a stale-distfalse red).pnpm --filter @object-ui/app-shell --filter @object-ui/i18n lint— 0 errors. The one warning onCommitTimeline.tsxis the pre-existinguseEffectat the old line 73, untouched here.node scripts/check-i18n-call-site-keys.mjs— green: every inlinedefaultValuematches itsenvalue and every call site passes exactly the arguments that value has holes for.node scripts/check-i18n-en-drift.mjs— green: "2 key(s) added, 0 en value(s) changed".packages/i18n/src/__tests__/all-locales-key-parity.test.ts— 20 passed.Note for the PM
The claim flagged #4118 as in flight over app-shell i18n. This PR's new keys land in
packages/i18n/src/locales/*.ts(the i18next packs), not app-shell's module-localmetadata-admin/i18n.tstable, so the surfaces differ; whichever lands second rebases only if #4118 also edits the ten packs.Generated by Claude Code