…rop a group with nothing left in it
An app nav item's `visible` CEL predicate IS evaluated per item by the shell
today — measured with a lit control, two sessions against one navigation, and
the absence asserted as a difference rather than on its own. Two paths into the
same subtree did not apply it.
The Favorites section collects pinned items by walking `children` and asked
nothing about the node it descended through, so an author could gate a group
with `visible`, watch the group leave the tree, and still find a pinned entry
from inside it rendered under Favorites. `requiredPermissions` and the
`requiresObject` / `requiresService` gates leaked the same way.
A group whose children were all gated away still rendered its own label as a
disclosure that opens onto nothing — and contradicted `hasVisibleNavigationItems`,
which already scores such a group as contributing nothing and is what the area
switcher elects areas by.
The per-item guard sequence was spelled out twice and omitted once. It is now a
single `passesNavItemGuards` that `NavigationItemRenderer`, the area derivation
and the Favorites collection share.
Presentation only: no server-side check is touched, and an unevaluable predicate
still fails open, now pinned.
Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018HrVaotisyhgmot9o2MLRq
Fixes #10119
Clause-②: no. Derived from the diff: it NARROWS what renders and adds a module-private helper. No exported signature moves (
NavigationRenderer's props,hasVisibleNavigationItems' signature andNavigationVisibilityOptionsare unchanged), no accept set is relaxed, no published surface widens.Clause-②-correction — the claim comment on this card (comment id
5751359491) deliberately carried noClause-②:line, because the value is declared from the diff and there was no diff. The line above is that declaration; this is not a secondClaim:.1. The card's headline does not reproduce on today's
main— measured, with a lit controlThe card reports that an app nav item's
visibleCEL predicate is "authored, validated, persisted, and served to the browser intact — and then ignored". Re-derived here rather than relayed, and it is no longer true.The reading that counts is a comparison, not an absence: the card's own predicate, in the shape the server serves it, run against two sessions in one run, with the
trueleg lit so thefalseleg's absence means something.positionsSecretAdmin Onlydemo_memberfalseother_teamtrueevaluateVisibilityroutes the served envelope to the CEL engine andNavigationRendererapplies it per item. The behaviour the card asks for landed in@object-ui/layout/@object-ui/app-shell17.1.0 — the release note for it is the one that begins "Nav gating fields finally gate" — while the card was filed against a Console build served alongside@objectstack/*17.2.0, which is versioned on its own cadence.⇒ this PR does not re-fix that. The ablation below shows it: with the fix reverted, the five cells describing the headline stay green.
2. What IS still broken — two other paths into the same subtree
The card's repro carries a predicate on both a group and its leaf, which is exactly where the remaining defect lives. Both of these reproduce on
mainand are fixed here.(a) A pinned descendant escaped its hidden ancestor. The Favorites section collects pinned entries by walking
children, and that walk asked nothing about the node it descended through. So an author could gate a group withvisible, watch the group leave the tree, and still find a pinned entry from inside it rendered under Favorites — the predicate evaluated, answeredfalse, and changed nothing the excluded user could see. That is the authoring-trap shape the card is about, one level down.requiredPermissionsand therequiresObject/requiresServicegates leaked by the same route, because the collection applied none of them; fixing onlyvisiblewould have left the identical trap on the gate the card itself calls the better one. Reachable in the shipped Console:UnifiedSidebar— the sidebarConsoleLayoutmounts — passesenablePinningand restores pins from persisted state.(b) A group whose children were all hidden rendered as an empty husk. Its own label still rendered as a disclosure that opens onto nothing. That also made a documented invariant false:
hasVisibleNavigationItems— the predicate the area switcher elects areas by — already scores such a group as contributing nothing, and its docblock claims it "can never disagree with the rendered navigation". It did disagree.3. The existing evaluator was found and reused — no second one was written
Named by symbol, as asked:
evaluateVisibility(@object-ui/app-shell'sExpressionProvider) — the shipped predicate evaluator. It handles literals and delegates everything else toExpressionEvaluator.evaluateCondition, which routes CEL envelopes to the canonical@objectstack/formulaengine. It is the same function the per-action capability gates andRecordFormPage's field gates run through. Called, not copied.buildExpressionUser— the session-to-current_usernormaliser that putspositionsin scope, which is what the card's predicate names.hasVisibleNavigationItems(@object-ui/layout) — the existing derived-visibility predicate. Fix (b) calls this, rather than asking a new question about children.The one thing added is
passesNavItemGuards, and it is a de-duplication, not a new evaluator: the per-item guard sequence was written out twice (inNavigationItemRendererand inhasVisibleNavigationItems) and omitted once (in the Favorites collection) — which is precisely how the two defects above arose. The three callers now share one statement instead of agreeing about it.4. The nesting rule, settled
false⇒ the group and its entire subtree are absent, on every path the shell surfaces them, the Favorites section included. An author who gates a group has gated its contents; a descendant escaping through a pin re-creates the trap.Net effect: the sidebar and the area switcher can no longer disagree about what a user can reach, because one statement decides both.
5. Failure direction: FAIL-OPEN, chosen deliberately and pinned
A predicate that cannot be evaluated — malformed, missing session data, evaluator unavailable — renders the entry.
evaluateVisibility's own comment states it as "FAIL-OPEN IS UNCHANGED, deliberately", and reversing it is a behaviour change this card does not carry a mandate for.⛔ Nothing server-side is removed, weakened or "optimised away" on the strength of this gate, and this is not a security fix. A user who types the URL directly gets exactly the answer they get today.
6. Pins
All in one new file —
navItemVisibleEvaluated.test.tsxin@object-ui/app-shell's layout tests — which mounts the shipped chain: the realExpressionProvider, the real evaluator built from a real session bag, and the realNavigationRenderer, wired with the same one-linerUnifiedSidebarandAppSidebaruse.falseis absent and thetrueone is present, both legs in one run against the same shell.falsepredicate takes its subtree with it, and a group whose children are all hidden leaves no husk.visiblekey renders identically for both sessions.7. Reverse verification
The fix was committed first, then reverted on disk from the pinned base commit (proof that the mutation landed: the
passesNavItemGuardsmarker count went 4 to 0), the pin was run, and the file was restored fromHEADand confirmed byte-identical by blob hash with an emptygit diff HEAD.Ablated result — the predicted direction, and it separates the two claims cleanly:
The five that stay green are the headline the card reported. The two that go red are what this PR actually fixes.
8. Checks on this head
Head is a merge of
origin/main(merged, never rebased) taken after the last code change.vitest run packages/layout/vitest run packages/app-shell/type-check—@object-ui/layouttype-check—@object-ui/app-shellpnpm lint(repo-wide, 47 tasks)check:control-bytescheck:new-line-citationsThe one failure, named rather than swept up.
AppContent.inaccessibleAppStrand.test.tsxtrips the repo's network-escape guard on/api/v1/meta,/api/v1/meta/packageand/api/v1/meta/diagnostics. It is not attributable to this change and it is not claimed as pre-existing either — here is what was and was not measured:@object-ui/layoutwhile the failing fixture is inapp-shell's console tests and exercises neither nav groups nor pinned favorites.Acceptance notes
Noted while working here, not filed and not fixed — out of this card's scope:
NavigationItem.visibleis declaredboolean | string, while the transport actually hands the gate a{ dialect, source }envelope, so every fixture must cast to describe what production sends. This is a known and deliberate divergence — the declaration says so in its own docblock and a spec-derived-unions pin holds it in place — so it is recorded here rather than raised as a finding. Carrier for it: any future change to that declaration.Generated by Claude Code