From 5f6166e1e51af4df5215baf8af41685d435fbdc1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 16:25:29 +0000 Subject: [PATCH] fix(app-shell): render the /home Administration group through NavigationRenderer (#3609) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `UnifiedSidebar` chose its renderer with one ternary on `context === 'app' && activeApp`. Only the app arm rendered `NavigationRenderer` — the component that descends into `type: 'group'` children. The home arm hand-rolled a non-recursive `homeNavigation.map(item => )`, and home navigation is the only navigation that groups: the nine-entry Administration cluster collapsed into a single row, and because a group carries no `url` of its own that row fell back to `/home` — the page the admin was already standing on. None of the nine children reached the DOM. `resolveLandingPath([])` lands a fresh-deployment admin on exactly this screen, and `HomePage` had dropped its own System card on the grounds that the sidebar carried those entries, so the net effect was an admin with no route into system administration at all. Route the home arm through the same `NavigationRenderer` rather than teach a second renderer to recurse. `NavigationRenderer` takes `basePath` as a prop and reads only `useLocation()` — no `activeApp` coupling — so the reuse needed no loosening. `basePath=""` is what the home arm already computed, and every home entry is `type: 'url'` (verbatim href resolution), so surviving links keep byte-identical hrefs. The group states `expanded: true`: the renderer auto-collapses groups of >= 8 children when unauthored, a heuristic for one long section among many, whereas on `/home` this group IS the navigation — nine entries behind a closed disclosure would be the same defect respelled. Pinning/reorder are deliberately not forwarded (their persistence key resolves to the first app, not to home) nor are the `activeApp`-keyed label resolvers; both omissions are documented at the call site. Tests: the #3590 MEASUREMENT pin asserted the broken shape (`Administration` as a link to `/home`, children absent) and was designed to go red here. It is REPLACED, not duplicated — the same test now asserts the group is a disclosure rather than a link, is open by default, and that all nine entries render at their declared URLs. A second test pins the gate in the negative direction (a non-admin sees none of them while the ungated entries still render). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GTRjn8xBqp75dk7kFupVRt --- .changeset/home-administration-group-3609.md | 9 ++ .../app-shell/src/layout/UnifiedSidebar.tsx | 84 +++++++--- .../systemNavSettingsTarget.test.tsx | 148 ++++++++++++++---- 3 files changed, 188 insertions(+), 53 deletions(-) create mode 100644 .changeset/home-administration-group-3609.md diff --git a/.changeset/home-administration-group-3609.md b/.changeset/home-administration-group-3609.md new file mode 100644 index 0000000000..13d892160c --- /dev/null +++ b/.changeset/home-administration-group-3609.md @@ -0,0 +1,9 @@ +--- +'@object-ui/app-shell': patch +--- + +Render the `/home` Administration group as a real group, so its nine system-administration entries are reachable (objectui#3609). + +`UnifiedSidebar` picks its renderer with one ternary on `context === 'app' && activeApp`. Only the app arm rendered `NavigationRenderer`, the component that descends into `type: 'group'` children; the home arm hand-rolled `homeNavigation.map(item => )` with no recursion. Since home navigation is the only navigation that groups, the whole nine-entry Administration cluster collapsed into one row — and a group carries no `url` of its own, so `|| '/home'` pointed that row back at the page the user was already on. System Settings, Applications, App Marketplace, Object Manager, Datasources, Users, Organizations, Roles and Configuration never reached the DOM. `resolveLandingPath([])` sends a fresh-deployment admin to `/home`, and `HomePage` had deliberately dropped its own System card on the grounds that the sidebar already carried those entries, so the net effect was an admin with no route into system administration at all. + +The home arm now renders through the same `NavigationRenderer` as the app arm rather than growing a second renderer that recurses: the group becomes a Collapsible and every entry passes the same item-level `visible` / `requiredPermissions` / runtime-capability guards. Hrefs are unchanged — home entries are all `type: 'url'`, whose resolution is verbatim. The group states `expanded: true` so it opens by default: the renderer's unauthored default collapses groups of eight or more children, a heuristic for one long section among many, whereas on `/home` this group *is* the navigation. Pinning and drag-reorder stay off in the home context, where their persistence key resolves to the first app rather than to home. Non-admins are unaffected — the cluster is still built behind the `isWorkspaceAdmin` gate and is absent from their item tree. diff --git a/packages/app-shell/src/layout/UnifiedSidebar.tsx b/packages/app-shell/src/layout/UnifiedSidebar.tsx index bc4e94058e..e676593ee7 100644 --- a/packages/app-shell/src/layout/UnifiedSidebar.tsx +++ b/packages/app-shell/src/layout/UnifiedSidebar.tsx @@ -344,6 +344,15 @@ export function UnifiedSidebar({ activeAppName }: UnifiedSidebarProps) { label: t('layout.systemNav.administration', { defaultValue: 'Administration' }), type: 'group' as const, icon: 'shield', + // Opened by default (objectui#3609). `NavigationRenderer`'s unauthored + // default auto-collapses groups with >= 8 children, a heuristic meant + // for one long section among many inside an app's navigation. Here the + // group is not one section among many — on `/home` it IS the admin's + // navigation, and this cluster exists for the zero-app deployment whose + // whole complaint was "no way through". Nine entries behind a closed + // disclosure would re-create that, so the spec's `expanded` is stated + // explicitly rather than left to the count heuristic. + expanded: true, children: adminItems, }); } @@ -594,26 +603,61 @@ export function UnifiedSidebar({ activeAppName }: UnifiedSidebarProps) { ) : ( /* Home Navigation */ <> - - - - {homeNavigation.map((item) => { - const NavIcon = getIcon(item.icon); - const isActive = location.pathname === item.url; - return ( - - - - - {item.label as string} - - - - ); - })} - - - + {/* ONE navigation renderer, both arms (objectui#3609). + + This arm used to hand-roll + `homeNavigation.map(item => )`. + That map does not recurse, so `type: 'group'` was simply + unsupported here — and home navigation is the only navigation + that groups. The nine-entry Administration cluster collapsed into + a single row; a group carries no `url` of its own, so + `|| '/home'` pointed that row back at the page the user was + already standing on, and not one child ever reached the DOM. + `resolveLandingPath([])` sends a fresh-deployment admin to + exactly this screen, and `HomePage` deliberately dropped its own + System card because "the system entries are already in the nav" — + so the net effect was an admin with no route into system + administration at all. + + Routing this arm through the SAME `NavigationRenderer` the app + arm uses (above) removes the divergence instead of teaching a + second renderer to recurse: groups render as Collapsibles, and + every entry passes the same item-level `visible` / + `requiredPermissions` / runtime-capability guards. The component + takes `basePath` as a prop and reads only `useLocation()` — it + has no `activeApp` coupling — so nothing had to be loosened to + reuse it here. + + `basePath=""` is what this arm's `basePath` already computes to + (`context === 'app' && activeApp ? … : ''`), and every home entry + is `type: 'url'`, whose href resolution is verbatim — so the two + surviving top-level links keep byte-identical hrefs. + + Deliberately NOT forwarded from the app arm: + - `enablePinning` / `enableReorder` — both persist under + `useNavOrder(activeApp?.name || 'home')`, and in the home + context `activeApp` resolves to the FIRST app rather than to + home, so home navigation would adopt that app's saved root + order. A pinned section would also land directly above this + arm's own "Starred" group. Separate product decisions, not part + of unflattening a group. + - `resolveGroupLabel` / `resolveItemLabel` — keyed on + `activeApp.name`, which does not denote this context. Home + labels are already resolved through `t()` where the items are + constructed. */} + resolveNavObjectLabel({ name: objectName, label: fallback })} + resolveDashboardLabel={(dashboardName, fallback) => resolveNavDashboardLabel({ name: dashboardName, label: fallback })} + resolveViewLabel={(objectName, viewName, fallback) => resolveNavViewLabel(objectName, viewName, fallback)} + onAction={dispatchNavAction} + t={t} + templateContext={{ currentUserId: user?.id ?? null, currentOrgId: activeOrganization?.id ?? null, contextValues }} + /> {/* Starred Apps */} {favorites.filter(f => f.type === 'object' || f.type === 'dashboard' || f.type === 'page').length > 0 && ( diff --git a/packages/app-shell/src/layout/__tests__/systemNavSettingsTarget.test.tsx b/packages/app-shell/src/layout/__tests__/systemNavSettingsTarget.test.tsx index 810ed6fc59..b719725dac 100644 --- a/packages/app-shell/src/layout/__tests__/systemNavSettingsTarget.test.tsx +++ b/packages/app-shell/src/layout/__tests__/systemNavSettingsTarget.test.tsx @@ -25,13 +25,26 @@ * - `UnifiedSidebar.homeNavigation`'s Administration cluster is the `/home` * admin nav added so a fresh env (no apps yet) still has a real menu — * `resolveLandingPath([])` sends exactly that user to `/home`. Same head entry, - * same bare URL. Its entry is corrected too, but is DORMANT: the second test - * measures why (the home arm renders groups flat, so no child of the cluster - * reaches the DOM at all) rather than asserting an href that never renders. + * same bare URL. * * These assert the URL the entry CARRIES, not a navigation: what the URL then * resolves to is `AppContent`'s question, and is pinned end-to-end (click → * mounted hub) in `console/__tests__/AppContent.noAppsCta.test.tsx`. + * + * ## The measurement this file used to carry, and what replaced it (objectui#3609) + * + * When #3590 corrected `UnifiedSidebar`'s entry, that entry was not reachable: + * the home arm hand-rolled `homeNavigation.map(item => )` with no recursion, so the whole `type: 'group'` cluster collapsed + * into one row pointing at `/home` and no child reached the DOM. Rather than + * assert an href that never rendered, #3590 left a MEASUREMENT pin asserting the + * broken shape — `Administration` carrying `href="/home"`, `System Settings` + * absent — designed to go red the moment the group rendered its children. + * + * #3609 made it red by routing the home arm through the same + * `NavigationRenderer` the app arm uses. The pin is therefore GONE, not + * duplicated: it is replaced below by the real assertions it was standing in + * for, so the repo pins the fix instead of pinning both the bug and the fix. */ import '@testing-library/jest-dom/vitest'; @@ -60,10 +73,14 @@ vi.mock('@object-ui/i18n', async (importOriginal) => ({ })); // Both clusters below are admin surfaces — UnifiedSidebar's Administration group -// is gated on `useIsWorkspaceAdmin`. +// is gated on `useIsWorkspaceAdmin`. Mutable so the gate can be exercised in +// BOTH directions (see the non-admin test): the cluster is built behind an +// `if (isWorkspaceAdmin)` at construction, and reusing NavigationRenderer must +// not have introduced a path that renders it for anyone else. +let isWorkspaceAdmin = true; vi.mock('@object-ui/auth', () => ({ useAuth: () => ({ user: null, signOut: vi.fn(), isAuthEnabled: false, activeOrganization: null }), - useIsWorkspaceAdmin: () => true, + useIsWorkspaceAdmin: () => isWorkspaceAdmin, getUserInitials: () => 'U', })); @@ -88,9 +105,18 @@ vi.mock('../../utils', () => ({ appRouteSegment: (app: { name?: string }) => app?.name, })); -// Lazy lucide DynamicIcon would suspend mid-test; a null icon keeps each link's -// accessible name equal to its label text. +// Lazy lucide DynamicIcon fires an async `import()` from a `useEffect` and then +// setStates; a null icon keeps each link's accessible name equal to its label +// text and keeps the render synchronous. `AppSidebar`'s hand-written fallback +// cluster resolves icons through app-shell's own `getIcon`, while +// `NavigationRenderer` (now the home arm's renderer too) uses `getLazyIcon` from +// `@object-ui/components` — both entry points need stubbing, and the components +// package must otherwise stay REAL because the Sidebar primitives come from it. vi.mock('../../utils/getIcon', () => ({ getIcon: () => () => null })); +vi.mock('@object-ui/components', async (importOriginal) => ({ + ...(await importOriginal>()), + getLazyIcon: () => () => null, +})); vi.mock('../../hooks/useRecentItems', () => ({ useRecentItems: () => ({ recentItems: [] }) })); vi.mock('../../hooks/useFavorites', () => ({ @@ -122,8 +148,37 @@ import { UnifiedSidebar } from '../UnifiedSidebar'; /** The system hub — the reachable target, and what every sibling entry prefixes. */ const SYSTEM_HUB = '/apps/setup/system'; +/** + * Every entry of the `/home` Administration cluster, in declaration order. + * Asserted whole rather than by sample: the defect was that the group's + * children were never visited at all, so "some of them render" is not the + * property worth pinning — "all nine, at the URLs they declare" is. + */ +const ADMINISTRATION_ENTRIES: ReadonlyArray = [ + ['System Settings', SYSTEM_HUB], + ['Applications', `${SYSTEM_HUB}/apps`], + ['App Marketplace', `${SYSTEM_HUB}/marketplace`], + ['Object Manager', `${SYSTEM_HUB}/metadata/object`], + ['Datasources', '/apps/setup/component/metadata/resource?type=datasource'], + ['Users', `${SYSTEM_HUB}/users`], + ['Organizations', `${SYSTEM_HUB}/organizations`], + ['Roles', `${SYSTEM_HUB}/roles`], + ['Configuration', `${SYSTEM_HUB}/settings`], +]; + +function renderHomeSidebar() { + return render( + + + + + , + ); +} + beforeEach(() => { localStorage.clear(); + isWorkspaceAdmin = true; }); describe('sidebar system-settings target (objectui#3590)', () => { @@ -158,33 +213,60 @@ describe('sidebar system-settings target (objectui#3590)', () => { ); }); - it('MEASUREMENT: UnifiedSidebar renders the /home Administration cluster FLAT, so its retargeted entry is dormant', () => { - // Measured while retargeting `UnifiedSidebar`'s `sys-settings` entry: that - // entry is not reachable today, so the corrected URL is dormant rather than - // user-visible, and this file cannot honestly assert a navigation for it. - // - // Why: `UnifiedSidebar` runs ONE ternary on `context === 'app' && activeApp` - // (line ~437). Only the APP arm renders ``, which is what - // descends into `type: 'group'` children. The HOME arm hand-rolls - // `homeNavigation.map(item => )` — no - // recursion — so the whole 9-item Administration group collapses into a - // single link, and a group carries no `url`, so it falls back to `/home`: - // the page the admin is already on. - // - // The URL constant was corrected anyway (objectui#3590), so whoever fixes - // the flattening does not ship a dead `/apps/setup` link behind it. This pin - // records the measurement, and goes red the moment the group renders its - // children — which is the signal to replace it with the real href assertion. - render( - - - - - , + it('UnifiedSidebar: /home renders the Administration cluster as a GROUP, with all nine entries reachable (objectui#3609)', () => { + // Replaces the #3590 MEASUREMENT pin. Its two halves invert exactly: + // before → `Administration` IS a link, href `/home`; children absent. + // after → `Administration` is a disclosure, not a link; children present. + renderHomeSidebar(); + + // The group is a Collapsible trigger now, not a leaf link. The old shape is + // asserted gone by name, not merely "different href": a group carries no + // `url` of its own, so any future arm that renders it as a link can only + // reach the `|| '/home'` fallback and re-create the dead link. + expect(screen.queryByRole('link', { name: 'Administration' })).not.toBeInTheDocument(); + const trigger = screen.getByRole('button', { name: 'Administration' }); + // Opened by default. `NavigationRenderer` auto-collapses groups with >= 8 + // children unless the item states `expanded`; this cluster has nine and IS + // the admin's `/home` navigation, so `expanded: true` is stated on it. Left + // to the heuristic, Radix would unmount `CollapsibleContent` and the nine + // entries would be back out of the DOM — a different spelling of the bug. + expect(trigger).toHaveAttribute('aria-expanded', 'true'); + + for (const [label, href] of ADMINISTRATION_ENTRIES) { + expect(screen.getByRole('link', { name: label })).toHaveAttribute('href', href); + } + + // The head entry, called out because #3590 is what corrected it: it must + // not have regressed to the bare setup URL that re-renders the empty state. + expect(screen.getByRole('link', { name: 'System Settings' })).not.toHaveAttribute( + 'href', + '/apps/setup', ); - expect(screen.getByRole('link', { name: 'Administration' })).toHaveAttribute('href', '/home'); - expect(screen.queryByRole('link', { name: 'System Settings' })).not.toBeInTheDocument(); - expect(screen.queryByRole('link', { name: 'Applications' })).not.toBeInTheDocument(); + // The flat part of the home nav is unchanged — the two ungrouped entries + // still render, with the same hrefs, above the group. + expect(screen.getByRole('link', { name: 'Home' })).toHaveAttribute('href', '/home'); + expect(screen.getByRole('link', { name: 'Documentation' })).toHaveAttribute('href', '/docs'); + }); + + it('UnifiedSidebar: a non-admin on /home gets none of the Administration cluster', () => { + // The gate is `if (isWorkspaceAdmin)` where `homeNavigation` is built, so + // the cluster is absent from the item tree rather than hidden by the + // renderer. Pinned in the negative direction because the fix above changed + // WHO renders these items: had the reuse accidentally sourced them from + // somewhere ungated, only this assertion would notice. + isWorkspaceAdmin = false; + renderHomeSidebar(); + + expect(screen.queryByRole('button', { name: 'Administration' })).not.toBeInTheDocument(); + expect(screen.queryByRole('link', { name: 'Administration' })).not.toBeInTheDocument(); + for (const [label] of ADMINISTRATION_ENTRIES) { + expect(screen.queryByRole('link', { name: label })).not.toBeInTheDocument(); + } + + // …while the entries every user gets are still there — otherwise this test + // would pass on a sidebar that rendered nothing at all. + expect(screen.getByRole('link', { name: 'Home' })).toHaveAttribute('href', '/home'); + expect(screen.getByRole('link', { name: 'Documentation' })).toHaveAttribute('href', '/docs'); }); });