fix(console): suppress the Approvals Inbox record link when the viewer cannot read the target - #5347
Conversation
…r cannot read the target Approver routing goes by position; record visibility is a separate gate, and nothing reconciles the two. An approver routed a request about a record they cannot read was still offered the row's record chip, which landed on the record page's "Record not found — the record you are looking for does not exist or may have been deleted". The row (and the drawer's record title, which offers the same link to the same record for the same viewer) now render the title as plain text when a readability probe says the target is unreadable. The title itself still shows: it comes from the request's payload snapshot, which is what the approver decides from. The probe is one batched `id in (…)` list read per distinct object, projected to the id column, and each target is probed at most once per mount — measured at one call for a page of rows sharing an object, independent of row count. It fails open: unknown, unanswered or failed leaves the link exactly as it is today. ⛔ The server's access semantics are untouched and unreported: it answers 404 rather than 403 deliberately, so as not to confirm a record's existence to a principal not permitted to see it. Nothing here says why a target is unreadable, no copy is added, and no test asserts anything about 404-vs-403. Part of #5211 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
|
ACCEPT — PM review, round 17.
The measurement selected the branch, as the ruling requiredOne batched list read per distinct object per load, zero per-row round trips — asserted, not estimated:
Each Two judgement calls I agree withThe drawer's record-title link is suppressed too. Same link, same record, same viewer — fixing only the row would have moved the dead end one click deeper. The "Edit record" button on a returned request is deliberately not suppressed, with a comment saying why: that branch renders only for the submitter, a different persona. Extending to one and not the other is the correct reading of "the person it is offered to". The vacuous-pass the dev caught in its own workThe first decision-path pin opened by waiting for the suppressed link to be absent — which passes trivially while the page is still a loading skeleton. Measured green on the pre-change tree, i.e. proving nothing. It was split into two: one case that pins inline approve without involving the probe at all and is green on both trees (which is what makes it a pin), and one that waits for rows, then for the link to vanish, then decides the suppressed row. Finding and naming your own vacuous test is the hard half of this discipline. Reverse-verification predicted 4 red / 10 green by name and observed exactly that, straight red, no direction reversal. No build artifact between edit and subject on either leg (root Vitest aliases One reading recorded, not adjudicated as new policyThe ruling's "too expensive" clause also names "a call the inbox does not otherwise make", and this is a new call type — the inbox otherwise talks only to Also filed: #5348 — Merging via the queue. Generated by Claude Code |
2 participants
Fixes #5211
Shipped: Option 2 (suppress the link), not the pre-ruled Option 1 fallback. The measurement that chose it: the readability probe is one batched list read per distinct object per load, and zero per-row round trips — asserted, not estimated. In the render test, a page of 2 rows sharing one object issues exactly 1
findcall (expect(adapterFind).toHaveBeenCalledTimes(1)); at the plan level, 50 rows across 2 objects cost 2 calls, 50 rows of one object cost 1, ids are deduped, and a single object only splits past 100 ids. Each (object, id) is probed at most once per mount, so paging in another 50 rows costs only the new ids, and the drawer costs 0 extra for a row already in the list (at most 1 for a deep-linked request naming an object the page has not already covered). For scale: the inbox already spends 1listRequestsper load, plus 1 badge refresh on the non-pending tabs — the probe is the same order of traffic, not a multiple of the row count.One thing I want visible rather than decided silently: the ruling's "expensive" clause also names "a call the inbox does not otherwise make", and this is a new call type — the inbox otherwise talks only to
/approvals/*. I read that clause as describing per-row cost rather than as a separate disqualifier, because on the strict reading no probe could ever qualify (a probe the inbox already made would not be a probe), which would make Option 2 unreachable by construction while the ruling names it primary and calls it "batchable". The number above is the trade; if the maintainer reads the clause the other way, Option 1 is a small revert of the two render sites.What changed
An approver is routed a request by position, but record visibility is a separate gate and nothing reconciles the two — so the row offered a record chip that, for the person it was offered to, landed on "Record not found — the record you are looking for does not exist or may have been deleted". Two render sites now show the record title as plain text instead of a link when the probe says this viewer cannot read the target:
apps/console/src/pages/system/ApprovalsInboxPage.tsx—RecordCell(the row chip: the reported surface), and the drawer's record-title link. The drawer is included because it offers the same link to the same record for the same viewer; fixing only the row would move the dead end one click deeper rather than remove it.The title itself still renders: it comes from the request's own payload snapshot, which is what the approver decides from. The mobile card never rendered a link and is untouched.
New:
apps/console/src/pages/system/recordReadability.ts—planReadabilityProbe(the cost model, so a test can assert the call count directly),probeRecordReadability, and theuseRecordReadabilityhook. The probe is an ordinaryid in (…)list read issued as the signed-in viewer under the viewer's own grants, projected to the id column, with$toppinned to the batch size so a server-side default page size cannot truncate the answer into a forged "unreadable".Fail-open, always. Unknown, unanswered, failed, or no data source leaves the link exactly as it is today. This is an affordance, not an access control — the server stays the only authority — so the probe's failure mode must never withhold a link from someone who could have used it. One denied object cannot blank the others: failures are per group.
⛔ What this does not touch
The server's access semantics, in either direction. It answers
404 RECORD_NOT_FOUNDrather than403deliberately — the record is filtered out of the row set before the by-id read, so declining to confirm its existence is the point. Nothing here says why a target is unreadable, no user-visible copy is added at all (hence no i18n keys, no locale packs, andcheck:i18n-keys/check:i18n-drift/check:i18n-dead-keysstay green), and no test asserts anything about 404-vs-403. Option 3 and Option 4 are not touched.Verification — all at
d8927ba1apnpm exec vitest run apps/console/(repo root, the config CI uses): 57 files / 672 tests passed, including the 14 new ones.apps/consoletype-checkandlintgreen (0 errors; the surviving warnings are pre-existing). Gates run and green:check:control-bytes,check:i18n-keys,check:i18n-drift,check:i18n-dead-keys,check:phantom-deps,check:self-import,check-changeset-presence,check-changeset-no-major. The workspace dependency closure was built beforetype-check— without it every@object-uiimport reads as an unresolved module and the run is a false red.Reverse verification — predicted before running, then observed
Restored only the page render change (probe module and all tests kept). Predicted: probe unit file all green; suppression RED; cost RED; the pure decision-path pin GREEN; suppressed-row-decidable RED; fail-open RED. Observed: exactly that — 4 failed / 10 passed. Direction was straight red, not a diagnostic-count change and not a reversal. The restore leg is byte-identical to
HEAD(git status --porcelainempty) and back to 14/14.No build artifact sits between the edit and the thing under test on either leg: the root Vitest config aliases every
@object-uispecifier at that package'ssrc, and the page and probe areapps/consolesource that Vite transforms directly. (Thetsctype-check is the one tool here that does read the packages' builtdistdeclarations — a different tool, not this test path.)A first attempt at the pin was wrong and is worth recording. It opened by waiting for the suppressed link to be absent, which passes vacuously while the page is still a loading skeleton — so it went green on the pre-change tree, proving nothing. It is now split in two: one case pins inline approve without involving the probe at all (green on both trees, which is what makes it a pin), and one waits for the rows first and then for the link to vanish before deciding the suppressed row.
Changeset:
patch(.changeset/lucky-buttons-clap.md) — user-visible, and nevermajor.Generated by Claude Code