fix(app-shell): a failed package lookup is not an empty result (objectui#7881) - #7906
Merged
Merged
Conversation
…tui#7881)
`fetchFullPackage` — the `PackageSwitcher` helper behind "Package info &
settings" — fetched `/api/v1/packages` and went straight to `res.json()`,
never reading `res.ok`. The platform answers a failed read in the ADR-0112
envelope, `{ success: false, error: { code, message } }`, and that envelope
parses cleanly through the reader below it: `root` becomes the error object,
which is neither an array nor carries `packages`, so the list fell to `[]` and
`.find()` to `null`. Nothing threw, so `openManage`'s `catch` — the one that
toasts `formatMetadataError` — never ran, and the two lines after it still
fired: `setManage(null)` then `setManageOpen(true)`.
`PackageDetailSheet` renders `null` for a null package, so during an outage the
author clicked the menu item and got silence: no sheet, no toast, no
explanation — and `manageOpen` stuck true with no rendered sheet to close it.
Third variant of the objectui#7368 family after objectui#7821 (PR #7879): not a
lost toast and not an inverted decision, but a failure laundered into a
successful-looking empty result. An empty list is a completely legitimate
success answer, which is exactly why it must never be the value a failure
produces.
Measured before deciding what to read. `GET /api/v1/packages` is served by the
direct-mount registrar, which mounts first in the production stack and is
pinned at zero hand-written bodies, so every failure leaves through the shared
`sendError` / `sendThrownError`: 401 UNAUTHENTICATED, 403 FORBIDDEN, 503
SERVICE_UNAVAILABLE and 500 INTERNAL_ERROR, all one shape. The envelope's own
`success` is therefore not a second bit here — `sendOk` writes `true` on every
2xx and the error writers `false` on every non-2xx, which is `!res.ok`
restated. So `res.ok` is the decision and the envelope is read for the words;
in the 5xx band the platform withholds the producer's prose for the generic
`Internal server error`, leaving `error.code` as the only discriminating word,
so the code travels with the message. A non-JSON error body — the one other
reachable shape, a proxy's HTML 502/504 — names the status instead of the JSON
syntax error the author used to be shown.
Reported through this surface's existing posture, not a second one:
`formatMetadataError` on the shared `studio-package-list` sonner id, so one
outage across this surface's four callers of that endpoint is one toast rather
than four. Deliberately not also recorded in `pkgsErr`: that slot is the
switcher list's own state, written exactly where `pkgs` is, and this callback
never writes `pkgs`.
And the sheet no longer opens on a `null` package at all — this card's
user-visible deliverable. A successful list that does not contain the package
(deleted or uninstalled elsewhere) now says so.
Nine behavioural pins in StudioDesignSurface.packageLookupFailure.test.tsx.
Three of them are negative controls that stay green with the fix reverted, so
the other six are provably not restating an existing assertion — and a "fix"
that merely stopped opening the sheet cannot pass the file.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sam
marked this pull request as ready for review
September 6, 2026 02:47
os-sam
pushed a commit
that referenced
this pull request
Sep 6, 2026
…cord (objectui#7907)
The tail of `onManageChanged` — the managed-snapshot refresh that runs after every
lifecycle action fired from the `PackageDetailSheet` — swallowed a failed
`fetchFullPackage` under a bare `catch {}` commented "keep the current snapshot".
That snapshot is one the action itself had just made stale, so the author disabled a
package, was told nothing, and went on reading `Status: Enabled`.
`PackageDetailSheet` derives its lifecycle verb from the record it holds (`enabled`
picks both the button label and the endpoint it POSTs), so leaving it open over a
snapshot known to be pre-action re-armed the author with the verb they had just
fired. The failure is now reported through this surface's existing objectui#7368
posture — `formatMetadataError` on the shared `studio-package-list` sonner id, so one
outage that rejects both halves of this callback is still one toast — and the sheet
closes rather than present the pre-action record as current. Still a degradation and
never a throw: the editor, the top bar and the package list stay, and no navigation
is inferred from a refresh that could not happen (objectui#7821).
The same tail dropped `fresh === null` — a successful read whose list no longer
contains the package — just as quietly, and now reports it with the sentence
`openManage` already uses.
Not recorded in `pkgsErr`, measured rather than inherited: that slot is written
exactly where `pkgs` is — the mount effect and this callback's HEAD. The tail writes
neither; it writes `manage`. The head has just recorded the list's own verdict, so
writing the slot from here would mark the trigger `failed` over names the head
refreshed successfully a moment ago.
Pre-existing, and objectui#7881 (PR #7906) made it much easier to hit rather than
causing it: before that fix this `catch` could only ever see a `res.json()`
rejection; now that `fetchFullPackage` refuses a non-2xx it also swallowed every
401 / 403 / 503 / 500.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3
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 free
to 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 #7881
The defect
fetchFullPackage— thePackageSwitcherhelper behind "Package info & settings" —fetched
/api/v1/packagesand went straight tores.json(), never readingres.ok.The platform answers a failed read in the ADR-0112 envelope,
{ success: false, error: { code, message } }, and that envelope parses cleanlythrough the reader below it:
rootbecomes the error object, which is neither an arraynor carries
packages, so the list fell to[]and.find()tonull. Nothing threw,so
openManage'scatch— the one that toastsformatMetadataError— never ran, andthe two lines after it still fired:
setManage(null)thensetManageOpen(true).PackageDetailSheetstarts withif (!pkg) return null, so during an outage the authorclicked the menu item and got silence: no sheet, no toast, no explanation — and
manageOpenstucktruewith no rendered sheet to close it.Third variant of the objectui#7368 family after objectui#7821 (landed as #7879): not a
lost toast and not an inverted decision, but a failure laundered into a
successful-looking empty result. An empty list is a completely legitimate success
answer, which is exactly why it must never be the value a failure produces.
What was measured before deciding what to read
GET /api/v1/packagesis served by the direct-mount registrar(
@objectstack/restpackage-routes.ts), which mounts first in the production stackand is pinned by
check:route-envelopeat zero hand-written bodies — every failureleaves through the shared
sendError/sendThrownError. Reachable on this path:UNAUTHENTICATEDFORBIDDENstudio.access/setup.accesscapability gateSERVICE_UNAVAILABLEINTERNAL_ERRORAll four carry one shape, pinned wire-side by
package-envelope.conformance.test.ts(
body.success === false,erroran object,error.codea registered code,error.messagea non-empty string).So the envelope's own
successis not a second bit here.sendOkwritestrueonevery 2xx and the error writers write
falseon every non-2xx, which makes it!res.okrestated — reading it as a second decision input would be a tolerant path for a shape
this seam cannot produce.
res.okis the decision; the envelope is read for thewords. In the 5xx band the platform withholds the producer's prose and substitutes
the generic
Internal server error(INTERNAL_ERROR_MESSAGE), leavingerror.codeasthe only discriminating word — which is why the code travels with the message.
A second response shape exists and is reported here as asked: a non-JSON error body,
i.e. a proxy's HTML 502/504. That is the one arm the pre-fix code already reached the
catch on — by way of
res.json()rejecting — and it showed the authorUnexpected token '<'. Covered: the tolerant read names the status instead. No thirdshape was found on this path.
The fix
res.okis read, and a non-2xx throws carrying the server's ownerror.messageplus
error.code, orHTTP nnnwhen the body is not the envelope at all.formatMetadataErroron the sharedstudio-package-listsonner id, so one outageacross this surface's four callers of that endpoint is one toast, not four.
No new state machine, no new slot, no new channel.
nullpackage — this card's user-visibledeliverable. A successful list that simply does not contain the package (deleted or
uninstalled elsewhere) now says so through the same channel rather than opening over
nothing.
One deliberate deviation, flagged for review
The failure is not also recorded in
pkgsErr. That slot is the switcher list'sstate and is written exactly where
pkgsis — the mount effect andonManageChanged.openManagenever writespkgs, so the names in the trigger are precisely as current asthey were a moment ago, and marking the trigger
failedwould say otherwise. The twosibling
fetchPackagescall sites that likewise do not write the list (the writabilitycourtesy gate, the namespace lookup) report the same way — shared toast id, no slot
write. Say the word if the dispatch intended the slot write too.
Evidence — the ablation
Mutation leg: the source file put back to its pre-fix state (
origin/maind9580f464), the new pins kept.deba94efvs on-disk781e0c89, and the anchors flipped:if (!res.ok)2 to 1,setManage(await fetchFullPackage(id));0 to 1,manageMissing1 to 0../StudioDesignSurfaceby relativespecifier, so vitest transforms the
.tsxsource directly and nodist/standsbetween the mutation and the run.
⭐ 6 red, and the 3 negative controls green — plus all 5 of #7879's pins in the same
run. That last clause is what proves the new pins are not restating an existing
assertion, and that a "fix" which merely stopped opening the sheet could not pass this
file.
The reds are the defect itself, verbatim from the log:
data-managed-id=""on four of them — the sheet opened on a null package;AssertionError: expected "vi.fn()" to be called at least once—openManage'scatch never ran, nothing was reported at all;
to be 'HTTP 502'and got the pre-fix answer instead: aSyntaxErrorreading "Unexpected token", naming the opening angle bracket of theproxy's HTML error page. That is what the author used to be shown.
Restore leg proved by state, never by an exit code: on-disk blob back to
deba94ef= the HEAD blob,git diff HEAD --name-onlyempty, anchors back to2 / 0 / 1. The script carried atrap ... EXIT INT TERMrestoring an absolutepath, and the restore named
HEADexplicitly (a baregit checkout -- pathrestoresfrom the index, which
git checkout ref -- pathhad just written with the mutation).Verification — all on the pushed commit
c2c0f04f8,git diff HEADemptyTest Files 3 passed (3)·Tests 21 passed (21)studio-design/directoryTest Files 50 passed (50)·Tests 271 passed (271)metadata-admin/suites that read the edited i18n tableTest Files 16 passed (16)·Tests 136 passed (136)pnpm --filter @object-ui/app-shell run type-checkcheck:governed-queue-guard(--testover the 4 changed paths)✅ NOT GOVERNED — 4 path(s) checked against 5 governed surface(s); none matched.check:governed-queue-guard(--self-test)OK ... 132 cases passnode scripts/check-changeset-presence.mjs✅ 2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)check:i18n-keys·check:i18n-drift·check:i18n-dead-keyscheck:control-bytes✅ check-control-bytes: OK (scanned 6393 tracked text file(s); skipped 85 binary).check:vi-mock-specifiers·check:vi-mock-inheritThe type-check green is not vacuous:
tsc -p tsconfig.test.json --listFileslists allthree edited/added files (1 hit each), so the program really contains them.
Lint — a measured narrowing, not a skip
The repo-wide scan is CI's run. What was measured here instead:
eslint .over the whole affectedpackage linted 1079 files (count from
--format json), 0 errors.StudioDesignSurface.tsxare all pre-existing — they sit at lines 691-4211, and every line this PR adds is in
363-460.
eslint.config.jsextendstseslint.configs.recommendedwithlanguageOptions: { ecmaVersion, globals }anddeclares no
parserOptions.projectand noprojectService— type-aware linting isnot enabled, so every rule's verdict is a function of that file's own text plus the
shared config. A diff confined to three files cannot move any untouched file's verdict.
Boundaries held
/homerecovery destination is not touched — that belongs toobjectui#7373, which stays open.
onManageChangedis byte-for-byte unchanged — fix(app-shell): a failed package-list refresh is not a deletion (objectui#7821) #7879 just repaired it.fetchis deliberately kept: retiring it in favour of thepackages-ioreader is the card's option 3, and it cannot reusefetchPackagesas-is because
parsePackagestrims the record this sheet needs. Out of scope here.Generated by Claude Code