diff --git a/.changeset/no-apps-go-to-settings-target-3590.md b/.changeset/no-apps-go-to-settings-target-3590.md new file mode 100644 index 000000000..4d4d4cafa --- /dev/null +++ b/.changeset/no-apps-go-to-settings-target-3590.md @@ -0,0 +1,7 @@ +--- +'@object-ui/app-shell': patch +--- + +Point the "System Settings" entries at the system hub `/apps/setup/system` instead of the bare `/apps/setup` (objectui#3590). + +`AppContent` mounts the system hub only under `isSystemRoute`, which keys on a `/system` path segment. A bare `/apps/setup` therefore matched no pseudo-route except `isSetupRoute` and fell back into the "No Apps Configured" guard — i.e. on a zero-app deployment it *is* that empty state's own URL, so the empty state's `go-to-settings-btn` re-rendered the very screen it sits on. Retargeted three call sites: the empty state's CTA, `AppSidebar`'s no-active-app `sys-settings` fallback entry, and `UnifiedSidebar`'s `/home` Administration `sys-settings` entry. Every sibling entry in both clusters already spelled `/apps/setup/system/...`. diff --git a/packages/app-shell/src/console/AppContent.tsx b/packages/app-shell/src/console/AppContent.tsx index 92333202e..e898dd85b 100644 --- a/packages/app-shell/src/console/AppContent.tsx +++ b/packages/app-shell/src/console/AppContent.tsx @@ -593,7 +593,19 @@ export function AppContent({ extraRoutes, extraRoutesNoApp }: AppContentProps = - diff --git a/packages/app-shell/src/console/__tests__/AppContent.noAppsCta.test.tsx b/packages/app-shell/src/console/__tests__/AppContent.noAppsCta.test.tsx index a6cf72210..012da8164 100644 --- a/packages/app-shell/src/console/__tests__/AppContent.noAppsCta.test.tsx +++ b/packages/app-shell/src/console/__tests__/AppContent.noAppsCta.test.tsx @@ -18,9 +18,13 @@ * /apps/setup and /apps/setup/ * - * which is precisely where the sidebar's no-active-app system navigation and - * the empty state's own `go-to-settings-btn` send a zero-app user - * (`layout/AppSidebar.tsx` systemFallbackNavigation → `/apps/setup`). + * which is where a zero-app user still arrives from the remaining bare + * `/apps/setup` senders (`layout/AppSidebar.tsx`'s no-app sidebar header and + * user-menu "Settings", `console/ConsoleShell.tsx`'s legacy `/system` redirect) + * and from bookmarks. Note both CTA-shaped senders that used to point here have + * since been retargeted at the system hub: the empty state's own + * `go-to-settings-btn` and both sidebars' `sys-settings` entry (objectui#3590) — + * the ENTRY family below is unchanged, only who points at it. * * `/` is NOT such a URL: with zero apps `RootLandingRedirect` resolves to * `/home` and `AppContent` never mounts. So the empty state always renders @@ -138,6 +142,19 @@ function LocationProbe() { return
{location.pathname}
; } +/** + * The host's system routes, reduced to the one entry this file asserts on. + * `apps/console/src/AppContent.tsx` builds a `systemRoutes` fragment whose FIRST + * entry is `} />`, and passes the + * SAME fragment to both `extraRoutes` and `extraRoutesNoApp`. With zero apps only + * the `extraRoutesNoApp` branch is reachable, so that is the one wired below — + * and it is what makes `/apps/setup/system` a real destination rather than + * another URL that renders nothing (objectui#3590). + */ +const systemRoutesStub = ( + system hub} /> +); + /** * The reference host's route tree, reduced to the parts that decide this * question: the `/apps/:appName/*` subtree, the landing route, and the @@ -149,7 +166,7 @@ function renderConsoleAt(initialUrl: string) { - } /> + } /> landing} /> home} /> } /> @@ -199,18 +216,38 @@ describe('AppContent — no-apps empty state CTA (objectui#3573)', () => { expect(screen.queryByTestId('root-landing')).not.toBeInTheDocument(); }); - it('leaves the sibling go-to-settings CTA on its absolute /apps/setup target', async () => { - renderConsoleAt('/apps/setup/sys_inbox_message'); + it('sibling go-to-settings CTA opens the system hub instead of looping onto this same empty state', async () => { + // objectui#3590 — this REPLACES the pin that used to sit here (*"leaves the + // sibling go-to-settings CTA on its absolute /apps/setup target"*: pathname + // `/apps/setup`, `create-first-app-btn` still present). That pin recorded + // current behaviour to prove #3573 had not touched this button; it explicitly + // did not bless the target. The target was wrong: bare `/apps/setup` is this + // empty state's OWN url, so the click re-rendered the same screen. + renderConsoleAt('/apps/setup'); fireEvent.click(await screen.findByTestId('go-to-settings-btn')); - expect(pathname()).toBe('/apps/setup'); + expect(await screen.findByTestId('system-hub-page')).toBeInTheDocument(); + expect(pathname()).toBe('/apps/setup/system'); + // The loop this pins: the empty state must be GONE. Asserting only the URL + // would stay green for a target that merely renders nothing, and asserting + // only the hub would miss a screen that rendered both. + expect(screen.queryByTestId('create-first-app-btn')).not.toBeInTheDocument(); expect(screen.queryByTestId('root-landing')).not.toBeInTheDocument(); - // NB: this asserts CURRENT behaviour, it does not bless it. `/apps/setup` - // bare IS this empty state's own URL (`isSystemRoute` needs a `/system` - // segment), so on a zero-app deployment this sibling CTA is a no-op loop — - // filed separately as #3590. Kept here only to prove the #3573 fix did not - // touch it; update this expectation together with #3590. - expect(await screen.findByTestId('create-first-app-btn')).toBeInTheDocument(); + }); + + it('reaches the SAME system hub from a deeper splat URL (absolute target, depth-independent)', async () => { + // The depth the replaced pin used. The `/system` segment is what flips + // `isSystemRoute`, i.e. the switch that mounts `extraRoutesNoApp` — so the + // splat segment must not leak into the target either (a relative `system` + // would build `/apps/setup/sys_inbox_message/system` here: still + // `isSystemRoute`, but matching no route inside that branch — which has no + // catch-all — and therefore rendering blank). + renderConsoleAt('/apps/setup/sys_inbox_message'); + fireEvent.click(await screen.findByTestId('go-to-settings-btn')); + + expect(await screen.findByTestId('system-hub-page')).toBeInTheDocument(); + expect(pathname()).toBe('/apps/setup/system'); + expect(screen.queryByTestId('create-first-app-btn')).not.toBeInTheDocument(); }); it('MEASUREMENT: a non-pseudo /apps/:appName URL never reaches this empty state', async () => { diff --git a/packages/app-shell/src/layout/AppSidebar.tsx b/packages/app-shell/src/layout/AppSidebar.tsx index 23c137fe9..9e0ecf9e1 100644 --- a/packages/app-shell/src/layout/AppSidebar.tsx +++ b/packages/app-shell/src/layout/AppSidebar.tsx @@ -308,9 +308,18 @@ export function AppSidebar({ activeAppName, onAppChange }: { activeAppName: stri // Fallback system navigation when no active app exists — routes into the Setup app. // The marketplace entry is hidden from non-admin members (install is gated to // owner/admin on the server, so non-admins have no reason to see it). + // + // #3590 — `sys-settings` targets `/apps/setup/system` (the system hub), not the + // bare `/apps/setup`. This whole cluster renders ONLY when `activeApp` is falsy, + // and `activeApp` (above) is `matched || activeApps[0]` — falsy only when the + // deployment has zero active+visible apps. In exactly that case bare + // `/apps/setup` renders `AppContent`'s "No Apps Configured" empty state (its + // `isSystemRoute` guard needs a `/system` segment), so the cluster's head entry + // was a dead link in the one situation the cluster exists for. Every sibling + // below already spells `/apps/setup/system/...`. const systemFallbackNavigation: NavigationItem[] = React.useMemo(() => { const items: NavigationItem[] = [ - { id: 'sys-settings', label: t('layout.systemNav.systemSettings', { defaultValue: 'System Settings' }), type: 'url' as const, url: '/apps/setup', icon: 'settings' }, + { id: 'sys-settings', label: t('layout.systemNav.systemSettings', { defaultValue: 'System Settings' }), type: 'url' as const, url: '/apps/setup/system', icon: 'settings' }, { id: 'sys-apps', label: t('layout.systemNav.applications', { defaultValue: 'Applications' }), type: 'url' as const, url: '/apps/setup/system/apps', icon: 'layout-grid' }, ]; if (isWorkspaceAdmin) { diff --git a/packages/app-shell/src/layout/UnifiedSidebar.tsx b/packages/app-shell/src/layout/UnifiedSidebar.tsx index 6ed93fe95..bc4e94058 100644 --- a/packages/app-shell/src/layout/UnifiedSidebar.tsx +++ b/packages/app-shell/src/layout/UnifiedSidebar.tsx @@ -318,8 +318,18 @@ export function UnifiedSidebar({ activeAppName }: UnifiedSidebarProps) { { id: 'docs', label: t('layout.systemNav.documentation', { defaultValue: 'Documentation' }), type: 'url' as const, url: '/docs', icon: 'book-open' }, ]; if (isWorkspaceAdmin) { + // #3590 — `sys-settings` targets the system hub `/apps/setup/system`, not + // the bare `/apps/setup`. This cluster exists FOR the fresh env described + // above (no apps yet), and that is precisely where bare `/apps/setup` is a + // dead link: with zero apps it renders `AppContent`'s "No Apps Configured" + // empty state (`isSystemRoute` keys on a `/system` segment), so an admin + // landing on `/home` — `resolveLandingPath([])` — had no way through. The + // `/system` form resolves in BOTH branches (`extraRoutesNoApp` with no + // active app, `extraRoutes` once one exists), so an app-bearing deployment + // now reaches the hub here instead of whatever app `/apps/setup` fell back + // to. Every sibling below already spells `/apps/setup/system/...`. const adminItems: NavigationItem[] = [ - { id: 'sys-settings', label: t('layout.systemNav.systemSettings', { defaultValue: 'System Settings' }), type: 'url' as const, url: '/apps/setup', icon: 'settings' }, + { id: 'sys-settings', label: t('layout.systemNav.systemSettings', { defaultValue: 'System Settings' }), type: 'url' as const, url: '/apps/setup/system', icon: 'settings' }, { id: 'sys-apps', label: t('layout.systemNav.applications', { defaultValue: 'Applications' }), type: 'url' as const, url: '/apps/setup/system/apps', icon: 'layout-grid' }, { id: 'sys-marketplace', label: t('layout.systemNav.appMarketplace', { defaultValue: 'App Marketplace' }), type: 'url' as const, url: '/apps/setup/system/marketplace', icon: 'store' }, { id: 'sys-objects', label: t('layout.systemNav.objectManager', { defaultValue: 'Object Manager' }), type: 'url' as const, url: '/apps/setup/system/metadata/object', icon: 'database' }, diff --git a/packages/app-shell/src/layout/__tests__/systemNavSettingsTarget.test.tsx b/packages/app-shell/src/layout/__tests__/systemNavSettingsTarget.test.tsx new file mode 100644 index 000000000..810ed6fc5 --- /dev/null +++ b/packages/app-shell/src/layout/__tests__/systemNavSettingsTarget.test.tsx @@ -0,0 +1,190 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Sidebar "System Settings" entry — must target the system HUB (objectui#3590). + * + * ## The defect, and why both sidebars carry it + * + * `AppContent` decides which branch renders by string-matching the pathname: + * `isSystemRoute = location.pathname.includes('/system')`. Only that flag mounts + * the host's `extraRoutesNoApp` fragment, where `apps/console/src/AppContent.tsx` + * declares `` → `SystemHubPage`. A **bare** `/apps/setup` + * therefore matches no pseudo-route except `isSetupRoute`, falls into the + * `!activeApp && !isCreateAppRoute && !isSystemRoute && !isMetadataRoute` guard + * and re-renders the "No Apps Configured" empty state. On a zero-app deployment + * `/apps/setup` IS that empty state's own URL. + * + * Both sidebars pointed their `sys-settings` entry at that bare URL, in clusters + * whose every OTHER entry already spells `/apps/setup/system/...`: + * + * - `AppSidebar.systemFallbackNavigation` renders ONLY when `activeApp` is falsy, + * and `activeApp` there is `matched || activeApps[0]` — falsy exactly when the + * deployment has zero active+visible apps. So its head entry was dead in the + * one situation the cluster exists for. Pinned by the first test below, which + * renders with `apps: []`. + * - `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. + * + * 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`. + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import React from 'react'; +import { render, screen } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +// --------------------------------------------------------------------------- +// Mocks — providers and console-only chrome, matching the sibling sidebar +// suites. @object-ui/components and @object-ui/layout stay REAL so the hrefs +// asserted below are the ones each sidebar's own render path actually emits. +// --------------------------------------------------------------------------- + +vi.mock('@object-ui/i18n', async (importOriginal) => ({ + ...(await importOriginal>()), + useObjectTranslation: () => ({ + t: (key: string, options?: Record) => String(options?.defaultValue ?? key), + }), + useObjectLabel: () => ({ + objectLabel: ({ label }: { label?: string }) => label, + viewLabel: (_o: string, _v: string, fallback?: string) => fallback, + dashboardLabel: ({ label }: { label?: string }) => label, + navGroupLabel: (_a: string, _g: string, fallback?: string) => fallback, + }), +})); + +// Both clusters below are admin surfaces — UnifiedSidebar's Administration group +// is gated on `useIsWorkspaceAdmin`. +vi.mock('@object-ui/auth', () => ({ + useAuth: () => ({ user: null, signOut: vi.fn(), isAuthEnabled: false, activeOrganization: null }), + useIsWorkspaceAdmin: () => true, + getUserInitials: () => 'U', +})); + +vi.mock('@object-ui/permissions', () => ({ + usePermissions: () => ({ can: () => true, hasCapabilities: () => true }), +})); + +/** The zero-app deployment this whole screen exists for. */ +vi.mock('../../providers/MetadataProvider', () => ({ + useMetadata: () => ({ apps: [], objects: [] }), +})); + +vi.mock('../../providers/ExpressionProvider', () => ({ + useExpressionContext: () => ({ evaluator: null }), + evaluateVisibility: (expr: unknown) => expr !== false && expr !== 'false', +})); + +vi.mock('../../utils', () => ({ + resolveI18nLabel: (label: unknown) => (typeof label === 'string' ? label : ''), + matchAppBySegment: (apps: Array<{ name?: string }>, segment?: string) => + apps.find((a) => a?.name === segment), + 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. +vi.mock('../../utils/getIcon', () => ({ getIcon: () => () => null })); + +vi.mock('../../hooks/useRecentItems', () => ({ useRecentItems: () => ({ recentItems: [] }) })); +vi.mock('../../hooks/useFavorites', () => ({ + useFavorites: () => ({ favorites: [], removeFavorite: vi.fn() }), +})); +vi.mock('../../hooks/useNavPins', () => ({ + useNavPins: () => ({ togglePin: vi.fn(), applyPins: (items: unknown) => items }), +})); +vi.mock('../../hooks/useNavActionDispatch', () => ({ + useNavActionDispatch: () => vi.fn(), +})); +// The `/home` shell — the context whose navigation carries the admin cluster. +vi.mock('../../context/NavigationContext', () => ({ + useNavigationContext: () => ({ context: 'home', currentAppName: undefined }), +})); +vi.mock('../ContextSelectors', () => ({ + useAppContextSelectors: () => ({ contextValues: {}, element: null }), + contextSelectorQueryKey: (id: string) => (id === 'active_package' ? 'package' : id), + STUDIO_PACKAGE_SELECTOR_ID: 'active_package', +})); +vi.mock('../LocalizedSidebarTrigger', () => ({ + LocalizedSidebarTrigger: () => null, +})); + +import { SidebarProvider } from '@object-ui/components'; +import { AppSidebar } from '../AppSidebar'; +import { UnifiedSidebar } from '../UnifiedSidebar'; + +/** The system hub — the reachable target, and what every sibling entry prefixes. */ +const SYSTEM_HUB = '/apps/setup/system'; + +beforeEach(() => { + localStorage.clear(); +}); + +describe('sidebar system-settings target (objectui#3590)', () => { + it('AppSidebar: the no-active-app fallback cluster heads at the system hub', () => { + render( + + + {}} /> + + , + ); + + // Precondition: with zero apps this really is the fallback cluster, not an + // app's own navigation — otherwise the assertion below would be vacuous. + expect(screen.getByTestId('system-fallback-nav')).toBeInTheDocument(); + + expect(screen.getByRole('link', { name: 'System Settings' })).toHaveAttribute( + 'href', + SYSTEM_HUB, + ); + // The regression: bare `/apps/setup` re-renders the empty state this cluster + // is displayed on top of. + expect(screen.getByRole('link', { name: 'System Settings' })).not.toHaveAttribute( + 'href', + '/apps/setup', + ); + // The rest of the cluster was already hub-scoped; kept as the consistency + // anchor that made the head entry the odd one out. + expect(screen.getByRole('link', { name: 'Applications' })).toHaveAttribute( + 'href', + `${SYSTEM_HUB}/apps`, + ); + }); + + 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( + + + + + , + ); + + 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(); + }); +});