fix(console): honour each row's own hidden: true in the approvals queue and its amount sort - #6364
Merged
os-support-ai merged 2 commits intoAug 25, 2026
Conversation
…ueue The approvals queue rendered, and sorted on, an amount field the object declares `hidden: true`. objectui#5565 put the filter inside `decisionAmountEntry` behind an OPTIONAL `hiddenKeys` parameter and passed it at one of five call sites — the drawer. The desktop row, the mobile card and both halves of the amount comparator called it bare. Passing the page's existing `hiddenPayloadKeys` at those sites would have been wrong: that set is keyed to the OPEN request, while the queue is N rows spanning K objects, so one object's declarations would have been applied to every row. - `hiddenFields.ts`: `useHiddenFields` (single object) becomes `useHiddenFieldsByObject`, a per-object lookup on the `useRecordReadability` batching pattern, plus `planHiddenFieldReads` as the cost model — one metadata read per distinct object per mount, not one per row. - `decisionAmountEntry`'s `hiddenKeys` is now REQUIRED, so the compiler asks "whose hidden keys?" at every present and future call site. That is what closes the defect class rather than these four call sites. - The amount comparator asks each row about its own object: the queue orders on exactly the figure it renders, and a row left with no renderable amount sinks with the other amount-less rows. Fail-open is unchanged and deliberate: an unanswered or failed metadata read is "nothing known to be hidden" and renders today's figure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
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-support-ai
marked this pull request as ready for review
August 25, 2026 16:16
os-support-ai
deleted the
claude/issue-6020-approvals-queue-hidden-amount
branch
August 25, 2026 16:28
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 #6020
The approvals inbox queue rendered, and sorted on, an amount field the object declares
hidden: true.The call-site census, re-derived on this branch's base (
194fae184)decisionAmountEntryalready contained the filter — objectui#5565 added it — but thehiddenKeysparameter was optional, and one of five call sites passed it:hiddenKeys?:1845:444RecordCell, desktop queue row:1649:1061sortKey === 'amount'comparator, left:1062sortKey === 'amount'comparator, rightMeasured, not assumed: every line above was re-derived from
origin/mainat194fae184, and all five matched the numbers on the card. A filter that lives in a function body but is unpassed at four of five call sites reads exactly like a repaired defect — the function body is not the measurement, the call sites are.Why the obvious repair is wrong, and what this does instead
Threading the page's existing
hiddenPayloadKeysinto the four sites does not work. That set wasuseHiddenFields(selected?.object_name)— keyed to the drawer's currently-selected request. The queue is N rows spanning K objects, so one object's declarations would have been applied to every row: fields hidden on rows whose object never declared them, fields missed on rows whose object did. That is worse than the current miss, because it would look repaired.So the lookup became per-object, on the batching pattern
recordReadability.tsalready established for this page:useHiddenFields(one object) is replaced byuseHiddenFieldsByObject(objectNames), returning aforObject(name)lookup. Every consumer asks with its ownobject_name.planHiddenFieldReads(names)is the cost model, asplanReadabilityProbeis for the readability probe: its returned length is exactly the number of metadata reads a render adds. Distinct objects only, first-seen order, non-names dropped. A page of N rows over K objects costs K reads, not N, and every object is read at most once per mount.rowsplusselected), so a deep-linked drawer whose request is not in the current row set is still covered.decisionAmountEntry'shiddenKeysparameter is now required. That, not the four edits, is what closes the defect class: the compiler now asks whose hidden keys at every present and future call site. (payloadSummarykeeps its optional parameter — a required parameter cannot follow the optionalexcludeKeywithout reordering the signature, and its one call site already passes the set.)Ordering posture, stated with its acceptance criteria
The comparator half is not a visibility change, so it is declared rather than assumed: the queue orders on exactly the figure it renders. A row whose amount field is hidden loses its sort key and sinks with the other amount-less rows, keeping their relative newest-first order — which is the behaviour that surface already has for a request carrying no amount at all. The row itself never leaves the inbox; an approver still sees and can act on every request routed to them.
The reason this half is in scope: ordering is disclosure. Sorting on a hidden figure tells a viewer who never sees it how it compares with every other row, which leaks its relative magnitude.
When the hidden-key set is empty — fail-open, deliberately preserved
forObjectreturns the empty set for four distinct situations, deliberately collapsed into one answer: nothing declared hidden · this object was never asked about · the read has not answered yet · the read failed. All four render today's figure and keep today's ordering.Nothing here converts that to fail-closed. This is a presentation filter, not an access control — the server stays the only authority, FLS redaction happens at serve time, and
hiddenis ruled UI-only (objectstack#10749;internal: trueis the serialization primitive). Degrading an approver's decision surface on a transient metadata error would break the primary workflow to enforce a declaration that was never the security boundary.When the empty set changes is unchanged too. Reads still resolve per object, so the first paint renders exactly what it renders today and the trim applies as the declarations arrive — the same shown-then-hidden transition the drawer has had since #5565, now also possible on a queue row, and on the amount ordering if a reviewer has already chosen that sort. It is bounded: the read is
GET /api/v1/meta/object/:name, already on the adapter'sMetadataCache(LRU, 5-minute TTL, in-flight de-duplication), so on any page whose objects the session has touched it is resolved before the first paint. Trading it for a fail-closed hold would mean withholding an amount from every approver whenever metadata is slow or unreadable, which is the failure directionhiddenFields.tsexists to refuse. Called out explicitly so it is reviewed rather than inherited.Coverage — the fixture spans two objects on purpose
apps/console/src/pages/system/ApprovalsInboxPage.queueHiddenAmount.test.tsx, six tests through the page's own DOM. A single-object fixture would pass under the broken repair above, so the fixture is two objects with different declarations:showcase_purchasedeclarestotal_amounthidden.showcase_invoicedeclaresservice_feehidden, andtotal_amountnot.total_amounttotal_amountservice_feeThree further properties keep it from passing by accident:
freight_cost) after the hidden one, and trimmed the row renders that. So the drop happens inside the scan, before the field is chosen, and "the amount is gone" cannot be satisfied by a row that simply stopped rendering amounts. Same idea as the 6-field-cut promotion inApprovalsInboxPage.hiddenFieldTrim.test.tsx, transposed onto the pick.md:hiddenmobile card are both in the DOM here, so a rendered figure is exactly 2 leaf nodes and a trimmed one exactly 0. A repair that fixes one surface and forgets the other fails.getObjectSchemacalls).The sort fixture's three orders are three different permutations, so "sorted", "not sorted" and "sorted without the hidden figure" can never be confused: default newest-first
INV-8802 · INV-8801 · PO-4417; amount undeclaredPO-4417 · INV-8801 · INV-8802; amount with the declarationPO-4417 · INV-8802 · INV-8801.ApprovalsInboxPage.hiddenFieldTrim.test.tsxis untouched and still green — it covers the drawer, which was already correct, and it is now also the regression check on re-pointing the drawer at the shared lookup.planHiddenFieldReadsis pinned by three unit tests inhiddenFields.test.ts.Ablations — direction and counts predicted before running, both matched
No build artifact sits between the edit and the run: the page and the hook are this app's own source, imported directly by the test, and the root Vitest config aliases every
@object-uispecifier at package source — so there is nodistto rebuild between mutation and measurement. Each mutation was proven on disk before its run (git blob hash changed, injected marker counted, removed anchor counted at zero) and each restore proven afterwards (git hash-objectback to the HEAD blob,git diff HEADempty).A — the two render call sites neutered (
hiddenFields.forObject(r.object_name)replaced by an empty set atRecordCelland the mobile card).Predicted: RED, 2 failures — the render test, plus the render assertion inside the sort test, with that test's row-order assertion still passing because the comparator was untouched.
Actual:
Tests 2 failed | 4 passed (6). Test 1 failed at the hidden purchase figure (expected length 0, got 2); the sort test passedexpect(desktopRowTitles()).toEqual(['PO-4417', 'INV-8802', 'INV-8801'])and failed on the next line atamountNodes('USD 5,000.00')(0vs2). Prediction correct in direction, count and failing line.B — the comparator neutered (both
decisionAmountEntryarguments in the amount sort replaced by an empty set, render sites untouched).Predicted: RED, 1 failure — the sort test, on row order, actual
['PO-4417', 'INV-8801', 'INV-8802']against expected['PO-4417', 'INV-8802', 'INV-8801'].Actual:
Tests 1 failed | 5 passed (6),AssertionError: expected [ 'PO-4417', 'INV-8801', 'INV-8802' ] to deeply equal [ 'PO-4417', 'INV-8802', 'INV-8801' ]. Prediction correct in direction, count and values.Nothing was wrong in either prediction. The pair also settles T1 for review: the two halves fail independently, so neither is riding on the other's coverage.
Gates — run locally at
ff0457c5, each quoted from its own verdict lineGate set derived by reading the job step lists under
.github/workflows/(ci.yml,lint.yml,changeset-presence.yml,changeset-guard.yml,control-bytes.yml,vi-mock-specifiers.yml) and keeping what this diff can reach: four TypeScript files underapps/consoleplus one changeset.vitest run apps/console/Test Files 79 passed (79)/Tests 896 passed (896)pnpm --filter @object-ui/console type-checktsc --noEmit && tsc -b tsconfig.node.json --force, no diagnosticseslint .inapps/consoleerrors=0 warnings=211(JSON reporter counts)check-changeset-presence.mjs4 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)check-changeset-no-major.mjsNo changeset declares a major bump.check-changeset-fixed.mjsAll workspace packages are in the changeset fixed group.check-control-bytes.mjsOK (scanned 5237 tracked text file(s); skipped 85 binary)check-vi-mock-specifiers.mjsOK (… 686 relative specifier(s) resolved …)check-lint-coverage.mjs46/46 packages linted, 0 with outstanding errorscheck-type-check-coverage.mjs45/46 via type-check … 41/41 packages compile their testscheck-i18n-call-site-keys.mjsEvery in-scope call-site key resolves against the en packNotes on how those were read, not just that they were run:
cmd redirected to a file; EXIT=$?), never from ataildownstream of the command.apps/consolepackage, which is the only package this diff touches.tsc --noEmit --listFileslists all four changed files, including both.test.tsx/.test.tsfiles, so the console project does not exclude tests from its check.pnpm --workspace-concurrency=2 --filter '@object-ui/console^...' build, exit 0) —apps/consolehas nopathsmapping, so@object-ui/*resolves through each package'sdist. Judging before that build would have read stale or missing.d.tsin either direction.react-hooks/refswarnings on the new hook are the patternrecordReadability.tsalready carries onmainfor the identicallatest.current = …idiom (verified by linting that untouched file: same single warning). No new warning class.Out of scope
No unrelated defect was found on this path.
hiddenstays UI-only here: nothing in this change touches serialization, permissions, orinternal.Generated by Claude Code