Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/home-administration-group-3609.md
Original file line number Diff line number Diff line change
@@ -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 => <Link to={item.url || '/home'}>)` 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.
84 changes: 64 additions & 20 deletions packages/app-shell/src/layout/UnifiedSidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}
Expand Down Expand Up @@ -594,26 +603,61 @@ export function UnifiedSidebar({ activeAppName }: UnifiedSidebarProps) {
) : (
/* Home Navigation */
<>
<SidebarGroup>
<SidebarGroupContent>
<SidebarMenu>
{homeNavigation.map((item) => {
const NavIcon = getIcon(item.icon);
const isActive = location.pathname === item.url;
return (
<SidebarMenuItem key={item.id}>
<SidebarMenuButton asChild tooltip={item.label as string} isActive={isActive}>
<Link to={item.url || '/home'}>
<NavIcon className="h-4 w-4" />
<span>{item.label as string}</span>
</Link>
</SidebarMenuButton>
</SidebarMenuItem>
);
})}
</SidebarMenu>
</SidebarGroupContent>
</SidebarGroup>
{/* ONE navigation renderer, both arms (objectui#3609).

This arm used to hand-roll
`homeNavigation.map(item => <Link to={item.url || '/home'}>)`.
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. */}
<NavigationRenderer
items={homeNavigation}
basePath=""
evaluateVisibility={evalVis}
checkPermission={checkPerm}
checkCapability={checkCap}
resolveObjectLabel={(objectName, fallback) => 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 && (
Expand Down
148 changes: 115 additions & 33 deletions packages/app-shell/src/layout/__tests__/systemNavSettingsTarget.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 => <Link to={item.url ||
* '/home'}>)` 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';
Expand Down Expand Up @@ -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',
}));

Expand All @@ -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<Record<string, unknown>>()),
getLazyIcon: () => () => null,
}));

vi.mock('../../hooks/useRecentItems', () => ({ useRecentItems: () => ({ recentItems: [] }) }));
vi.mock('../../hooks/useFavorites', () => ({
Expand Down Expand Up @@ -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<readonly [string, string]> = [
['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(
<MemoryRouter initialEntries={['/home']}>
<SidebarProvider>
<UnifiedSidebar activeAppName="" />
</SidebarProvider>
</MemoryRouter>,
);
}

beforeEach(() => {
localStorage.clear();
isWorkspaceAdmin = true;
});

describe('sidebar system-settings target (objectui#3590)', () => {
Expand Down Expand Up @@ -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 `<NavigationRenderer>`, which is what
// descends into `type: 'group'` children. The HOME arm hand-rolls
// `homeNavigation.map(item => <Link to={item.url || '/home'}>)` — 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(
<MemoryRouter initialEntries={['/home']}>
<SidebarProvider>
<UnifiedSidebar activeAppName="" />
</SidebarProvider>
</MemoryRouter>,
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');
});
});
Loading