Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): an unloaded attachment list is not an empty one - #4691
Merged
yinlianghui merged 1 commit intoAug 15, 2026
Merged
Conversation
RecordAttachmentsPanel's refresh() catch split authorization out in #4685 and folded every other failure back into setRows([]), so a network failure, a 5xx and a 401/AUTH_REQUIRED all rendered "No attachments yet. Upload a file to get started." — an affirmative claim about the record's contents from a panel that never got an answer. The panel now carries the sibling four-way status vocabulary (loading / loaded / denied / unavailable): every state meaning "the panel does not know" is answered before rows.length === 0 is allowed to mean "the record holds nothing". A non-authz failure renders a distinct unavailable state with a retry (unlike denied, an outage and a lapsed session are fixable by a second attempt) and withdraws the Upload affordance. It leaks nothing from the error. The empty state is now reserved for a genuine 200-with-zero-rows; the denied state is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnQd8iMMUwXQEV1crFmQiQ
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 15, 2026 09:44
Uh oh!
There was an error while loading. Please reload this page.
yinlianghui
deleted the
claude/issue-4684-attachments-unavailable-state
branch
August 15, 2026 09:44
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#4684
The defect
RecordAttachmentsPanel.refresh()split authorization out of itscatchin PR #4685 and folded everything else back intosetRows([]). So three different failures — a network failure (server unreachable, DNS, aborted request), a 5xx, and a 401 /AUTH_REQUIRED— all rendered:That is an affirmative claim about the record's contents, made by a panel that never got an answer, over a record that may hold thousands of files. Same defect class as #4269, one status over. The 401 is the sharpest case: an expired session is authentication, not authorization, so
isPermissionErrordeliberately does not claim it (verified at source — it matches 403 /PERMISSION_DENIED/FORBIDDEN/ an RLS denial only), which left it landing in the empty state.The fix
The panel now carries the sibling four-way status vocabulary —
loading/loaded/denied/unavailable— replacing theloadingboolean and PR #4685'slistDeniedboolean with one enum. The render chain is the assertion discipline: every state meaning "the panel does not know" is answered beforerows.length === 0is allowed to mean "the record holds nothing".unavailable(new) — any non-authz failure. Renders "We couldn't load the attachments for this record." with a Retry, and withdraws the Upload affordance.denied— unchanged from PR fix(app-shell): a denied attachment list is not an empty one #4685. Its JSX block is untouched; only the condition it hangs on changed fromlistDeniedtostatus === 'denied', so the rendered output is byte-identical.loaded+ zero rows — now the only branch entitled to the empty state, reached only after a read that came back.Why
unavailablekeeps a retry anddenieddoes not: a denial is permanent for this caller and retrying just re-earns the same 403, while an outage or a lapsed session are exactly what a second attempt fixes. Why Upload is withdrawn underunavailabletoo: offering an upload against a list the panel could not reach is the same over-assertion as the empty state it replaces — and the upload's own three-step presigned flow would fail on the same outage.No leaking, same bar as PR #4685: the unavailable state renders the i18n sentence and nothing sourced from the error — no status code, no server message, no host — and
setErroris not called, because a failed list is a state of the panel rather than an error banner about something the user did.This restores a house rule that had already landed twice as a bug fix:
HomeActionCenter(#4235) may only say "You're all caught up" once the inbox has answered, and an unloadable app list (#4300) is UNKNOWN rather than "no default app".The deliberate pin from PR #4685, rewritten not deleted
PR #4685 planted
pins today's behaviour for a NON-authz failure: still the empty stateprecisely so this change would be conscious. It is rewritten in place asa NON-authz failure no longer reaches the DENIED state (that split is authz-only)— the sameTypeError('Failed to fetch')input, now asserting the unavailable state and that the denied state has not crept outward to claim outages. What the pin protected (nobody widens this branch silently) survives as a stricter assertion.Premises verified before building
main— confirmed at source.isPermissionError— confirmed by reading the predicate.refresh()setsstatustoloadingsynchronously androwsis already empty in this state, so the loading branch wins on the very next render and the Retry button — the only handler that callsrefresh()— unmounts for the duration of the read. Covered by a test that countsfindcalls.find()degrades a bare 404 to{ data: [], total: 0 }(is404Error, A list request that 404s withOBJECT_API_DISABLEDrenders as the generic empty state — "this page cannot work" reads as "you have no records" #4408), so it resolves through the success path and still renders the empty state.Two notes on the edges, both deliberate:
dataSource/recordIdissues no read, sostatusstaysloading(a spinner) instead of showing the empty state. Not forced toloaded(that would assert an empty record from zero evidence) nor tounavailable(nothing failed). Every guard dep is in the callback's dependency list, so a late-arriving prop re-runs the read on its own.OBJECT_API_DISABLED/OBJECT_API_METHOD_NOT_ALLOWEDrefusal folds intounavailablerather than getting its own fifth state. Honest (the panel still asserts nothing) but less precise thanListView.classifyLoadError'sapi-disabled, since the retry can never succeed. Filed separately as an observation rather than widened here.Classifier reuse
No third classifier was written.
isPermissionErrorfrom@object-ui/reactis reused exactly as PR #4685 imported it.ListView.classifyLoadErrorwas checked and is not importable — it is a module-localfunctioninpackages/plugin-list/src/ListView.tsx, not exported from the file or the package — and app-shell does not depend on plugin-list. Its five-way split is also finer than this panel's render needs: the panel only needs assert-vs-don't-assert, plus the denied/retryable distinction that decides the affordance.Verification
Local gate union run after the final commit, at
37d57c64c(working tree clean, so the union ran on the tree that is HEAD):vitest run packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.test.tsxvitest run packages/app-shell/(full package)vitest run packages/i18n/type-checkfor app-shell + i18nScope: 2 of 47 workspace projects, both Done — filters matched, not a silent zero-matcheslint --quieton all changed filespnpm run check:i18n-keysenvaluepnpm run check:i18n-driftenvalues changedpnpm run check:control-bytesnode scripts/check-changeset-presence.mjsReverse verification, direction predicted before running: reverting only the component (tests kept) should turn red exactly the 8 tests that require
record-attachments-unavailableand leave the other 16 green. Observed exactly that —8 failed | 16 passed (24), with PR #4685's five denied tests, the empty-state test, the loaded test and the #2755 / #2970 suites all still green, so the new assertions are load-bearing and nothing else was disturbed. The fix was committed first; restoring it left the tree byte-identical to HEAD (git diff HEADempty), and the suite was re-run green from that restored state.Scope
RecordAttachmentsPanel.tsx+ its test file + two new i18n keys (detail.attachmentsLoadFailed,detail.retryLoadAttachments) in all ten locale packs + a changeset. Nothing else touched.Generated by Claude Code