Uh oh!
There was an error while loading. Please reload this page.
fix(approvals): derive approver identities from positions, not retired user.roles - #5555
Merged
Merged
Conversation
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
|
This was referenced Aug 21, 2026
os-sales
marked this pull request as ready for review
August 21, 2026 11: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.
Refs #5424 — sites 2, 3 and 4 only. Site 1 (
AuthGuard'srequiredRoles) isdeliberately NOT touched and is deferred pending a maintainer ruling, and so is
the
roles?: string[]declaration inpackages/auth/src/types.ts(site 1 isstill its only live reader, so it cannot retire until site 1 is decided).
What landed
Framework ADR-0090 D3 renamed the session's
roleskey topositionswith nodeprecation window, and the protocol-17 session face emits no
roleskey atall. Three client read sites still followed the old spelling:
Site 2 —
packages/app-shell/src/hooks/sharedUserFeeds.ts(the real injury).approverIdentities()readu?.roles ?? []and nothing else, so it emittedno
role:identity at all. Approvals addressed to a position rather than toa person matched nothing and vanished from the bell badge, the bell's Approvals
tab and Home's To-do card — silently, with no error and no empty-state copy.
Now reads
positions.Site 3 —
apps/console/src/services/approvalsApi.ts.buildApproverIdentities()also splits the scalaruser.role, so it degradedrather than dying: on the measured payload it still yielded
role:userwhiledropping every business position name. Now reads
positions; the scalarrolesplit stays (protocol 17 still emits
role, and it is a separate identitysource that this card is not about).
Site 4 —
packages/app-shell/src/console/AppContent.tsx.Dropped
roles: (user as any).rolesfrom the expression user. It was alwaysundefined, so'manager' in current_user.rolesgot a context that answeredwrongly rather than one plainly missing the key.
positionsandisPlatformAdminwere already forwarded correctly beside it. The signed-outbranch never carried
roles, so removing it also makes the two branches agreeon one shape.
The retired spelling is not kept as a fallback anywhere —
packages/auth/src/types.tsforbids pairing the two in so many words, and eachsite now carries a pin that fails if the old key is ever readable again.
Verification
Every pin is shaped to fail because of the empty collection, not merely to
assert "it reads
positions" — that kind of assertion is green against thebroken code too, which also read a key and also produced a request. Site 2's
pins assert on the identities that actually reach the wire (the
approverIdquery the server matches
pending_approversagainst).Verified tree:
64330ca49— the final commit; the union below ran on itwith a clean working tree.
vitest run— app-shell hooks +AppContent.*+ console services + ApprovalsInboxPagepnpm --filter @object-ui/app-shell --filter @object-ui/console type-checktype-check$ tsc --noEmit …)pnpm --filter @object-ui/app-shell --filter @object-ui/console lintpnpm check:control-bytes✅ OK (scanned 4604 tracked text file(s))node scripts/check-changeset-presence.mjs✅ 6 source file(s) of 2 released package(s) changed, 1 changesetnode scripts/check-changeset-no-major.mjs✅ No changeset declares a major bumpDependency closure (
--filter '@object-ui/app-shell^...' --filter '@object-ui/console^...' build) was built first —type-checkistsc --noEmitand reads dependency.d.ts.Reverse verification (predictions written and committed BEFORE running)
All three sites were reverted to the
rolesspelling in one mutation, themutation was proved on disk with anchored
grep -con both the injected andthe deleted text (not on a bare diffstat, and not on the editor's exit code),
and the script restored via a
trap … EXIT INT TERM. These suites resolve thesubject through vitest's workspace
srcaliases, not throughdist, so norebuild is involved on either leg; the restore leg was confirmed by a clean
git status.Result: 8 red, 11 green. Site 2's pin went red in exactly the shape the card
describes:
and site 3's reproduced its own description verbatim —
expected [ 'role:user' ] to deeply equal [ 'role:manager', 'role:finance_approver', 'role:user' ], i.e."yields
role:userand loses every business position name".Two predictions were wrong, and are recorded as wrong in the test docblocks
rather than quietly re-written:
RED. Its fixture supplies
manageronly throughpositions, so the retiredread drops it and the overlap it means to exercise never forms. It is a real
assertion about the fixed behaviour, but it is not the independent control
the prediction claimed.
roleskey as a fallback" caseswere not named in the predictions; both go RED. That is correct and welcome
— the ablation is precisely what makes the ghost key legible again, so those
anti-alias pins are load-bearing rather than decorative.
Negative controls held: a user with neither
positionsnorroleproduces norole identity, no
role:undefined, and no throw; a session with no id issues norequest at all.
One disclosure — a new lint warning I did not suppress
Site 4's pin needs the expression-user construction to be reachable, so it was
extracted from
AppContent's body into an exportedbuildExpressionUser().That adds one
react-refresh/only-export-componentswarning toAppContent.tsx(a warning, not an error;lintexits 0). The rule's ownremedy is "use a new file", which is outside this card's file fence, so I left
the warning visible rather than silencing it with a disable comment — it is an
accurate reminder that this function's long-term home is its own module. Happy
to move it if you'd prefer that in this PR.
Out of scope, filed separately
packages/auth/src/AuthProvider.tsx:244produces the retired key: thepreview-mode synthetic user is built as
{ role, roles: [role] }with nopositions. It is a producer rather than a reader, it is outside this fence,and it is entangled with the site-1 ruling (preview mode is what still feeds
AuthGuard). Noted for triage rather than fixed here.Generated by Claude Code