diff --git a/.changeset/nav-declared-empty-group-dropped.md b/.changeset/nav-declared-empty-group-dropped.md new file mode 100644 index 0000000000..89a5b532dd --- /dev/null +++ b/.changeset/nav-declared-empty-group-dropped.md @@ -0,0 +1,56 @@ +--- +"@objectstack/rest": patch +--- + +fix(rest): drop a navigation group that was DECLARED empty, not just one the gate emptied (#7380) + +`filterAppForUser`'s docblock has promised "Empty groups collapse so the sidebar +doesn't render a label with no children" since #4651, and its `filterNav` branch +carried the matching comment. The guard in front of that branch was +`Array.isArray(e.children) && e.children.length > 0`, so a group authored +`children: []` never reached the rule that owns the promise — it fell through the +`else` and shipped in `GET /meta/app` as a bare label. The one shape the sentence +most obviously covers was the one shape it could not reach. + +The judgement is now on what SURVIVES rather than on how the entry got there. A +`type: 'group'` with no surviving children is dropped whether it **became** empty +(children filtered away by `requiredPermissions` / the ADR-0057 D10 +`requiresService` gate) or **started** empty (`children: []`). Nesting composes: +an outer group left holding only a dropped inner group collapses in the same +pass. A group carrying no `children` key at all — unreachable through the spec, +where `children` is required on both the input and output `group` branches, but +reachable at runtime because this filter reads untyped documents off the metadata +store — is the same dead label and drops too. + +**Contribution slots are the shape this actually shipped.** `setup.app.ts` is +authored entirely out of it: nine `type: 'group'` anchors with `children: []`, +filled on read by `Registry.applyNavContributions` (ADR-0029 D7) from whichever +capability packages are installed. That merge runs in the protocol layer *before* +this filter, so a slot a plugin filled arrives here with children and survives, +while a slot left empty because its capability is disabled arrives `[]` and is +now dropped — exactly the "a disabled capability contributes nothing and its slot +stays empty" case `setup.app.ts` documents. Deployments that ran without the +optional plugins were serving those anchors as empty, unopenable sidebar +headings; they now disappear, and the ones with contributions are untouched. + +**The rule is `type: 'group'` and nothing else.** The navigation union nests on +two branches (`object` and `group`). An `object` entry navigates on its own +`objectName`, so `{ type: 'object', objectName: 'lead', children: [] }` is a live +link that nests nothing, and emptiness says nothing about whether to serve it — +non-group entries keep their existing behaviour exactly, including when the gate +empties their children. A group cannot be a target: `GroupNavItemSchema` is a +`strictObject` declaring no `objectName` / `pageName` / `componentRef` / `url` +(it rejects them), and its docblock reads "Does not perform navigation itself." +Measured before the change: 41 `type: 'group'` entries across the shipped apps +(`account`, `setup`, `studio`), the examples (`app-crm`, `app-showcase`, +`app-todo`) and the spec's nav type-assertion fixtures. 16 are childless — the 9 +`setup` slots plus 7 spec fixtures, none in the example apps — and zero of the 41 +carry `objectName` / `pageName` / `componentRef` / `url` or any other target. The +drop is therefore unconditional; no standalone-group shape needed sparing. + +One consequence worth naming: because `areas[].navigation` is filtered through +this same `filterNav`, an area whose entries are all childless groups now empties +and is dropped by the existing area-collapse rule. An area authored +`navigation: []` is still passed through untouched, as before — a group is a +sidebar label and nothing else, while an area is a workspace the shell can select +on its own, and that divergence is documented at `filterAreas`. diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index e66b01ae04..973b446155 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -2772,7 +2772,11 @@ export class RestServer { * a subset of the user's system permissions. * - Recursively strips child navigation entries (groups, items) whose * `requiredPermissions` are not satisfied. Empty groups collapse so - * the sidebar doesn't render a label with no children. + * the sidebar doesn't render a label with no children — [#7380] a + * `type: 'group'` with no SURVIVING children is dropped whether it was + * emptied by the gate or authored `children: []`. Only `group` collapses; + * an `object` entry is its own target and is served however many children + * it has. See the rule at the `filterNav` branch for the measurement. * - [#4722] Applies the SAME item gate to every `areas[].navigation` tree. * Both trees are the same shape and the keys mean the same thing in both, * so `filterNav` is reused — there is deliberately no second @@ -2839,13 +2843,57 @@ export class RestServer { const req = Array.isArray(e.requiredPermissions) ? e.requiredPermissions : []; if (req.length > 0 && !req.every((p: string) => sysPerms.has(p))) continue; if (typeof e.requiresService === 'string' && serviceGate && serviceGate(e.requiresService) === false) continue; - if (Array.isArray(e.children) && e.children.length > 0) { + // [#7380] A `group` is judged on what SURVIVES, never on how it + // got there. Both childless shapes render the same dead sidebar + // label, so both are dropped: + // - BECAME empty — authored with children, all gated away; + // - STARTED empty — authored `children: []`. + // The old guard (`children.length > 0`) sent the second shape + // down the else branch, which never reaches the drop rule, so a + // declared-empty group shipped as a bare label the docblock + // above already promised it would not. That shape is not a + // corner case: `setup.app.ts` is authored entirely out of it — + // nine `children: []` contribution slots (ADR-0029 D7) that + // `Registry.applyNavContributions` fills on read, BEFORE this + // filter runs. So a slot a capability plugin filled arrives here + // with children and survives; a slot left empty because its + // capability is disabled arrives `[]` and is now dropped, which + // is exactly the "a disabled capability contributes nothing and + // its slot stays empty" case `setup.app.ts` documents. + // + // The rule is `type === 'group'` ONLY, and stays that way. The + // union nests on two branches (`NAV_VARIANTS_ACCEPTING_CHILDREN` + // = `object` | `group`), and an `object` entry is its own + // navigation target — `{ type: 'object', objectName: 'lead', + // children: [] }` is a live link to the lead list, not a label, + // so emptiness says nothing about whether to serve it. A group + // cannot be a target: `GroupNavItemSchema` is a `strictObject` + // over the base keys plus `expanded`/`children` and declares no + // `objectName` / `pageName` / `componentRef` / `url` — it + // REJECTS them — and its docblock reads "Does not perform + // navigation itself." Measured against that before the change + // (#7380): 41 `type: 'group'` entries across the shipped apps + // (`account`, `setup`, `studio`), the examples (`app-crm`, + // `app-showcase`, `app-todo`) and the spec's nav type-assertion + // fixtures. 16 are childless — the 9 `setup` slots and 7 spec + // fixtures; the three example apps have none — and ZERO of the + // 41 carry `objectName` / `pageName` / `componentRef` / `url` or + // any other target. So the drop is unconditional: there is no + // standalone childless-group shape in the tree to spare. + // + // A group with NO `children` key is covered by the same rule for + // the same reason — same dead label. It is unreachable through + // the spec (`children` is required on both the input and output + // group branches; `app.nav-type-assertions.ts` pins that with a + // `@ts-expect-error`), but this filter reads untyped documents + // off the metadata store, so leaving it out would just reopen + // the bypass one keyword over. + if (Array.isArray(e.children)) { const kids = filterNav(e.children); - // Drop empty groups so the sidebar doesn't render a label - // with nothing under it (matches AppSidebar UX). if (e.type === 'group' && kids.length === 0) continue; out.push({ ...e, children: kids }); } else { + if (e.type === 'group') continue; out.push(e); } } @@ -2858,11 +2906,25 @@ export class RestServer { // INSIDE an area, through the very same `filterNav` the top-level tree // uses, so the two trees can never disagree about what a key means. // - // Collapse rule, taken from what `filterNav` already does to a `group`: - // an area whose authored tree is emptied BY the gate is dropped (a bare - // area label with nothing reachable under it is not a useful response), - // while an area authored empty is passed through untouched — filtering - // reports what the caller may not see, it does not tidy the metadata. + // Collapse rule: an area whose authored tree is emptied BY the gate is + // dropped (a bare area label with nothing reachable under it is not a + // useful response), while an area authored `navigation: []` is passed + // through untouched — filtering reports what the caller may not see, it + // does not tidy the metadata. + // + // [#7380] That second half is where an area and a `group` now DIVERGE, + // deliberately: `filterNav` drops a childless group however it got that + // way, an area authored empty still ships. The reason is what the two + // shapes are. A group is a sidebar label and nothing else, so childless + // it renders dead — and the shipped `setup` app authors nine of them as + // contribution SLOTS, which makes "declared empty" the normal steady + // state of an unfilled one rather than an authoring slip. An area is a + // top-level workspace the shell can select and route to on its own; an + // author who ships `navigation: []` has declared an area that is not + // populated yet, and this filter is not the layer that judges that. + // What is NOT divergent is the walk: an area whose entries are all + // childless groups empties through the very same `filterNav` and is + // dropped by the rule above — one implementation, as everywhere else. const filterAreas = (list: any[]): any[] => { const out: any[] = []; for (const a of list) { diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index b69632e03b..93ad6cbe6f 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -3309,6 +3309,290 @@ describe('filterAppForUser — the enforced permission layers (#4651, #4722)', ( }); }); +// --------------------------------------------------------------------------- +// #7380 — the empty-group drop rule is about the RESULT, not the provenance +// +// `filterNav`'s docblock has promised "Empty groups collapse so the sidebar +// doesn't render a label with no children" since #4651, but the guard was +// `Array.isArray(e.children) && e.children.length > 0`: a group authored +// `children: []` never entered the branch that OWNS the drop rule, so it fell +// through the else and shipped — the one shape the sentence most obviously +// covers was the one shape it could not reach. +// +// This is not hypothetical metadata. `setup.app.ts` is authored ENTIRELY out of +// that shape: nine `type: 'group'` anchors with `children: []`, filled on read +// by `Registry.applyNavContributions` (ADR-0029 D7) from whichever capability +// packages are installed. The merge runs in the protocol layer BEFORE this +// filter, so at this seam a filled slot has children and an unfilled one does +// not — which is precisely the signal the rule needs, and precisely what the +// old guard threw away. +// +// The rule is `type === 'group'` and nothing else. The union nests on two +// branches (`object` | `group`); an `object` entry navigates on its own +// `objectName`, so `children: []` on one says nothing about serving it. A group +// cannot navigate — `GroupNavItemSchema` is a `strictObject` that declares no +// target key and rejects any, and its docblock says "Does not perform +// navigation itself". Surveyed against that before writing this: 41 group +// entries across the shipped apps (`account`, `setup`, `studio`), the examples +// (`app-crm`, `app-showcase`, `app-todo`) and the spec's nav type-assertion +// fixtures. 16 are childless — the 9 `setup` slots plus 7 spec fixtures, with +// none in the three example apps — and zero of the 41 carry `objectName` / +// `pageName` / `componentRef` / `url` or any other target. The drop is +// therefore unconditional; there is no standalone group shape to spare. +// --------------------------------------------------------------------------- + +describe('filterAppForUser — childless groups drop however they got that way (#7380)', () => { + const make = () => new RestServer(createMockServer() as any, createMockProtocol() as any, ANON_API as any); + const ids = (a: any): string[] => (a?.navigation ?? []).map((e: any) => e.id); + const areaIds = (a: any, i: number): string[] => (a?.areas?.[i]?.navigation ?? []).map((e: any) => e.id); + + it('a group DECLARED `children: []` is dropped — the regression #7380 reports', () => { + const rest: any = make(); + const app = { + name: 'setup', + navigation: [ + { id: 'nav_home', type: 'object', objectName: 'home' }, + { id: 'group_integrations', type: 'group', label: 'Integrations', children: [] }, + ], + }; + // No permission is involved: the caller here holds everything there is to + // hold, so a survival would be the authored shape leaking, not a gate. + expect(ids(rest.filterAppForUser(app, new Set(['setup.access'])))).toEqual(['nav_home']); + expect(ids(rest.filterAppForUser(app, new Set()))).toEqual(['nav_home']); + }); + + it('a group emptied BY the gate is still dropped — the half that already worked', () => { + const rest: any = make(); + const app = { + name: 'setup', + navigation: [ + { id: 'nav_home', type: 'object', objectName: 'home' }, + { + id: 'group_admin', type: 'group', label: 'Admin', + children: [{ id: 'nav_users', type: 'object', requiredPermissions: ['admin.access'] }], + }, + ], + }; + expect(ids(rest.filterAppForUser(app, new Set()))).toEqual(['nav_home']); + expect(ids(rest.filterAppForUser(app, new Set(['admin.access'])))) + .toEqual(['nav_home', 'group_admin']); + }); + + it('a group with surviving children is kept, with only the gated ones stripped', () => { + const rest: any = make(); + const app = { + name: 'setup', + navigation: [{ + id: 'group_mixed', type: 'group', label: 'Mixed', + children: [ + { id: 'nav_public', type: 'object', objectName: 'public_thing' }, + { id: 'nav_secret', type: 'object', objectName: 'secret_thing', requiredPermissions: ['admin.access'] }, + ], + }], + }; + const out = rest.filterAppForUser(app, new Set()); + expect(ids(out)).toEqual(['group_mixed']); + expect(out.navigation[0].children.map((c: any) => c.id)).toEqual(['nav_public']); + // The label survives intact — collapsing is about emptiness, not tidying. + expect(out.navigation[0].label).toBe('Mixed'); + }); + + it('nesting: an outer group left holding only a dead inner group collapses too', () => { + // The recursion has to see the inner drop before it judges the outer one, + // which is only true because the rule lives inside `filterNav` itself. + const rest: any = make(); + const app = { + name: 'setup', + navigation: [{ + id: 'group_outer', type: 'group', label: 'Outer', + children: [{ id: 'group_inner', type: 'group', label: 'Inner', children: [] }], + }], + }; + expect(ids(rest.filterAppForUser(app, new Set()))).toEqual([]); + }); + + it('a group with no `children` key at all is the same dead label, and drops', () => { + // Unreachable through the spec — `children` is required on both the input + // and output `group` branches, pinned by `@ts-expect-error` in + // `app.nav-type-assertions.ts` — but this filter reads untyped documents + // off the metadata store, so the shape is reachable at runtime and would + // otherwise be the same bypass one keyword over. + const rest: any = make(); + const app = { + name: 'setup', + navigation: [ + { id: 'nav_home', type: 'object', objectName: 'home' }, + { id: 'group_bare', type: 'group', label: 'Bare' }, + ], + }; + expect(ids(rest.filterAppForUser(app, new Set()))).toEqual(['nav_home']); + }); + + // -- the boundary: NON-group entries are untouched by this rule ------------- + + it('a NON-group entry with `children: []` keeps today\'s behaviour — it is a target', () => { + // `{ type: 'object', objectName: 'lead', children: [] }` is a live link to + // the lead list that happens to nest nothing. Collapsing it would delete a + // reachable destination, which is the opposite of what the docblock rule + // says, and the rule says `group` for exactly this reason. + const rest: any = make(); + const app = { + name: 'crm', + navigation: [ + { id: 'nav_leads', type: 'object', objectName: 'lead', children: [] }, + { id: 'nav_page', type: 'page', pageName: 'ops', children: [] }, + { id: 'nav_link', type: 'url', url: 'https://example.com', children: [] }, + { id: 'nav_sep', type: 'separator' }, + ], + }; + const out = rest.filterAppForUser(app, new Set()); + expect(ids(out)).toEqual(['nav_leads', 'nav_page', 'nav_link', 'nav_sep']); + // Served as authored — the empty array is not rewritten or removed either. + expect(out.navigation[0]).toMatchObject({ objectName: 'lead', children: [] }); + }); + + it('a non-group entry still keeps its gated children stripped', () => { + const rest: any = make(); + const app = { + name: 'crm', + navigation: [{ + id: 'nav_leads', type: 'object', objectName: 'lead', + children: [ + { id: 'nav_hot', type: 'object', objectName: 'hot_lead' }, + { id: 'nav_all', type: 'object', objectName: 'all_leads', requiredPermissions: ['admin.access'] }, + ], + }], + }; + const out = rest.filterAppForUser(app, new Set()); + // Emptied to zero children it would STILL be served — it is a target. + expect(ids(out)).toEqual(['nav_leads']); + expect(out.navigation[0].children.map((c: any) => c.id)).toEqual(['nav_hot']); + }); + + it('an object entry emptied to zero children by the gate is still served', () => { + const rest: any = make(); + const app = { + name: 'crm', + navigation: [{ + id: 'nav_leads', type: 'object', objectName: 'lead', + children: [{ id: 'nav_all', type: 'object', requiredPermissions: ['admin.access'] }], + }], + }; + const out = rest.filterAppForUser(app, new Set()); + expect(ids(out)).toEqual(['nav_leads']); + expect(out.navigation[0].children).toEqual([]); + }); + + // -- contribution slots: the shape this bug actually shipped ---------------- + + it('a contribution slot FILLED before the filter survives; an unfilled one drops', () => { + // The `setup` app's real lifecycle, in the order the runtime runs it: + // `Registry.applyNavContributions` merges each plugin's items into the slot + // it names (protocol layer), THEN the REST server gates the merged document. + // So the filter sees a filled slot as a normal group with children, and an + // unfilled slot as `children: []` — the app-showcase case and the app-crm + // case from #7380, one filter apart. + const rest: any = make(); + const setupShell = () => ({ + name: 'setup', + navigation: [ + // Filled by an installed capability (e.g. plugin-webhooks, ADR-0029 K2.a). + { + id: 'group_integrations', type: 'group', label: 'Integrations', + children: [{ id: 'nav_webhooks', type: 'object', objectName: 'sys_webhook' }], + }, + // Slot whose capability is not installed — "a disabled capability + // contributes nothing and its slot stays empty" (setup.app.ts). + { id: 'group_diagnostics', type: 'group', label: 'Diagnostics', children: [] }, + ], + }); + const out = rest.filterAppForUser(setupShell(), new Set(['setup.access'])); + expect(ids(out)).toEqual(['group_integrations']); + expect(out.navigation[0].children.map((c: any) => c.id)).toEqual(['nav_webhooks']); + // The dead slot's label is gone from the body, not merely unrendered. + expect(JSON.stringify(out)).not.toContain('Diagnostics'); + }); + + it('a filled slot whose contributed items are all gated away collapses like any other', () => { + // Being filled is not a licence to survive: the slot is judged on what the + // CALLER may see, so a contribution the caller cannot satisfy leaves the + // same dead label as no contribution at all. + const rest: any = make(); + const app = () => ({ + name: 'setup', + navigation: [{ + id: 'group_diagnostics', type: 'group', label: 'Diagnostics', + children: [{ id: 'nav_logs', type: 'object', requiredPermissions: ['manage_platform_settings'] }], + }], + }); + expect(ids(rest.filterAppForUser(app(), new Set(['setup.access'])))).toEqual([]); + expect(ids(rest.filterAppForUser(app(), new Set(['setup.access', 'manage_platform_settings'])))) + .toEqual(['group_diagnostics']); + }); + + it('the ADR-0057 D10 service gate empties a slot the same way, and it drops', () => { + const rest: any = make(); + const app = () => ({ + name: 'setup', + navigation: [{ + id: 'group_org', type: 'group', label: 'Organization', + children: [{ id: 'nav_orgs', type: 'object', objectName: 'sys_organization', requiresService: 'org-scoping' }], + }], + }); + expect(ids(rest.filterAppForUser(app(), new Set(), (n: string) => n !== 'org-scoping'))).toEqual([]); + expect(ids(rest.filterAppForUser(app(), new Set(), () => true))).toEqual(['group_org']); + }); + + // -- one implementation: the same rule inside `areas[]` --------------------- + + it('AREA level: the same rule runs inside `areas[]` — one `filterNav`, not two', () => { + const rest: any = make(); + const app = { + name: 'crm', + areas: [{ + id: 'area_ops', label: 'Ops', + navigation: [ + { id: 'nav_tasks', type: 'object', objectName: 'task' }, + { id: 'group_soon', type: 'group', label: 'Soon', children: [] }, + ], + }], + }; + expect(areaIds(rest.filterAppForUser(app, new Set()), 0)).toEqual(['nav_tasks']); + }); + + it('AREA level: an area holding ONLY childless groups empties and is dropped', () => { + // Consequence of the above, pinned so it is deliberate rather than noticed + // later: the area-collapse rule reads `filterNav`'s output, and that output + // is now empty for this shape. An area authored `navigation: []` is still + // passed through (pinned in the #4651/#4722 block) — that divergence is + // explained at `filterAreas`. + const rest: any = make(); + const app = { + name: 'crm', + areas: [ + { id: 'area_dead', label: 'Dead', navigation: [{ id: 'group_soon', type: 'group', children: [] }] }, + { id: 'area_authored_empty', label: 'Soon', navigation: [] }, + ], + }; + const out = rest.filterAppForUser(app, new Set()); + expect(out.areas.map((a: any) => a.id)).toEqual(['area_authored_empty']); + }); + + it('does not mutate the app it filters', () => { + const rest: any = make(); + const app = { + name: 'setup', + navigation: [ + { id: 'group_empty', type: 'group', label: 'Empty', children: [] }, + { id: 'nav_home', type: 'object', objectName: 'home' }, + ], + }; + const before = JSON.stringify(app); + rest.filterAppForUser(app, new Set()); + expect(JSON.stringify(app)).toBe(before); + }); +}); + // --------------------------------------------------------------------------- // ADR-0057 D10 — requiresService capability gate (filterAppForUser) // ---------------------------------------------------------------------------