From b4dbcf7f2178db67813753769d7e1f21b520e647 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 16:40:16 +0000 Subject: [PATCH] fix(app-shell): point the remaining four Settings senders at /apps/setup/system (#3611) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit objectui#3590 / PR #3608 fixed the three call sites inside its declared file surface. These four were outside it and kept the bare `/apps/setup` — which, because `AppContent` mounts the system hub only under `isSystemRoute`, is the "No Apps Configured" empty state's own URL on a zero-app deployment. Every one of them looped in place there. Three are live defects: - `AppSidebar`'s no-active-app sidebar header (`system-sidebar-header`), which renders ONLY when there is no active app — unreachable except in exactly the state where its target was broken. - `AppSidebar`'s user-menu "Settings" entry. - `SystemRedirect`'s bare `/system` legacy bookmark. The forwarder was already half right (every suffixed bookmark was rewritten to `/apps/setup/system…` correctly); the bare branch now agrees with the suffixed branch beside it. No new logic. The fourth, `QuickActions`' "System Settings" card, is dormant — the component has zero JSX call sites repo-wide, so no user reaches it today. Corrected in the same pass so the dead link cannot return with the component if it is ever remounted. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GTRjn8xBqp75dk7kFupVRt --- .changeset/remaining-setup-links-3611.md | 15 ++ .../app-shell/src/console/ConsoleShell.tsx | 10 +- .../__tests__/systemRedirectTarget.test.tsx | 74 ++++++ .../src/console/home/QuickActions.tsx | 4 +- .../QuickActions.settingsTarget.test.tsx | 110 +++++++++ packages/app-shell/src/layout/AppSidebar.tsx | 15 +- .../appSidebarSettingsTargets.test.tsx | 222 ++++++++++++++++++ 7 files changed, 445 insertions(+), 5 deletions(-) create mode 100644 .changeset/remaining-setup-links-3611.md create mode 100644 packages/app-shell/src/console/__tests__/systemRedirectTarget.test.tsx create mode 100644 packages/app-shell/src/console/home/__tests__/QuickActions.settingsTarget.test.tsx create mode 100644 packages/app-shell/src/layout/__tests__/appSidebarSettingsTargets.test.tsx diff --git a/.changeset/remaining-setup-links-3611.md b/.changeset/remaining-setup-links-3611.md new file mode 100644 index 0000000000..0ad340074c --- /dev/null +++ b/.changeset/remaining-setup-links-3611.md @@ -0,0 +1,15 @@ +--- +'@object-ui/app-shell': patch +--- + +Point the four remaining "Settings" senders at the system hub `/apps/setup/system` instead of the bare `/apps/setup` (objectui#3611). + +Same root cause as objectui#3590, which fixed the three call sites inside its declared file surface: `AppContent` mounts the system hub only under `isSystemRoute`, which keys on a `/system` path segment, so on a zero-app deployment the bare `/apps/setup` *is* the "No Apps Configured" empty state's own URL and every entry spelling it looped in place. + +Three of the four are live defects, all reachable on a zero-app deployment today: + +- `AppSidebar`'s no-active-app sidebar header (`system-sidebar-header`) — the sharpest of them, since it renders *only* when there is no active app, i.e. it was unreachable except in exactly the state where its target was broken. +- `AppSidebar`'s user-menu "Settings" entry. +- `SystemRedirect`'s bare `/system` legacy bookmark. This forwarder was already half right — every *suffixed* bookmark (`/system/users`) was correctly rewritten to `/apps/setup/system/users`, and only the bare one dropped the `system` segment. The bare branch now agrees with the suffixed branch beside it; no new logic. + +The fourth, `QuickActions`' "System Settings" card, is dormant — the component has zero JSX call sites repo-wide, so no user can reach it today. It is corrected in the same pass so the dead link cannot return with the component if it is ever remounted. diff --git a/packages/app-shell/src/console/ConsoleShell.tsx b/packages/app-shell/src/console/ConsoleShell.tsx index 2806131247..3f5361b961 100644 --- a/packages/app-shell/src/console/ConsoleShell.tsx +++ b/packages/app-shell/src/console/ConsoleShell.tsx @@ -370,11 +370,17 @@ export function RootRedirect() { /** * SystemRedirect — forwards legacy /system/* URLs to the canonical - * /apps/setup/* location so bookmarks keep working. Suffix is preserved. + * /apps/setup/system/* location so bookmarks keep working. Suffix is preserved. + * + * #3611 — the bare `/system` bookmark used to land on the bare `/apps/setup`, + * which on a zero-app deployment is the "No Apps Configured" empty state's own + * URL. Every suffixed bookmark was already forwarded to `/apps/setup/system…`; + * the bare one now agrees with them instead of dropping the `system` segment + * that makes the hub mount at all. */ export function SystemRedirect() { const location = useLocation(); const suffix = location.pathname.replace(/^\/system/, ''); - const target = suffix ? `/apps/setup/system${suffix}` : '/apps/setup'; + const target = suffix ? `/apps/setup/system${suffix}` : '/apps/setup/system'; return ; } diff --git a/packages/app-shell/src/console/__tests__/systemRedirectTarget.test.tsx b/packages/app-shell/src/console/__tests__/systemRedirectTarget.test.tsx new file mode 100644 index 0000000000..eff8996e95 --- /dev/null +++ b/packages/app-shell/src/console/__tests__/systemRedirectTarget.test.tsx @@ -0,0 +1,74 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `SystemRedirect` — the legacy `/system*` bookmark forwarder (objectui#3611). + * + * ## The defect: the component disagreed with itself + * + * The forwarder was already HALF right. It built its target as + * + * suffix ? `/apps/setup/system${suffix}` : '/apps/setup' + * + * so every SUFFIXED legacy bookmark (`/system/users`) was correctly forwarded + * to `/apps/setup/system/users`, while the BARE `/system` bookmark dropped the + * `system` segment entirely and landed on `/apps/setup`. + * + * That segment is not decoration: `AppContent` mounts the system hub only when + * `isSystemRoute` (`pathname.includes('/system')`) holds, so on a zero-app + * deployment the bare `/apps/setup` falls through to the "No Apps Configured" + * empty state — it is that empty state's own URL. The fix makes the bare branch + * agree with the suffixed branch beside it; it adds no new logic. + * + * ## Route shape + * + * The route below is spelled exactly as the real consumers spell it + * (`apps/console/src/App.tsx`, `examples/console-starter/src/App.tsx`): + * ``. The splat also matches the bare `/system`, with + * an empty splat — which is precisely how the defective branch was reachable. + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import { MemoryRouter, Routes, Route, useLocation } from 'react-router-dom'; + +import { SystemRedirect } from '../ConsoleShell'; + +/** Reports where the redirect actually landed, including search and hash. */ +function Landing() { + const { pathname, search, hash } = useLocation(); + return
{`${pathname}${search}${hash}`}
; +} + +function landedFrom(entry: string): string { + render( + + + {/* The one route declaration every console consumer ships. */} + } /> + } /> + + , + ); + return screen.getByTestId('landing').textContent ?? ''; +} + +describe('SystemRedirect legacy bookmark forwarding (objectui#3611)', () => { + it('THE FIX: the bare /system bookmark lands on the system hub, not the empty state URL', () => { + expect(landedFrom('/system')).toBe('/apps/setup/system'); + }); + + it('REGRESSION: suffixed bookmarks — the half that was already correct — are unchanged', () => { + expect(landedFrom('/system/users')).toBe('/apps/setup/system/users'); + }); + + it('REGRESSION: a deep suffix keeps every segment', () => { + expect(landedFrom('/system/metadata/object')).toBe('/apps/setup/system/metadata/object'); + }); + + it('preserves search and hash on the bare bookmark too', () => { + // The bare branch is the one that changed, so its query/hash carry-over is + // worth pinning explicitly rather than inferring it from the suffixed case. + expect(landedFrom('/system?tab=general#audit')).toBe('/apps/setup/system?tab=general#audit'); + }); +}); diff --git a/packages/app-shell/src/console/home/QuickActions.tsx b/packages/app-shell/src/console/home/QuickActions.tsx index d14d442ec7..67bbb49aca 100644 --- a/packages/app-shell/src/console/home/QuickActions.tsx +++ b/packages/app-shell/src/console/home/QuickActions.tsx @@ -46,7 +46,9 @@ export function QuickActions() { label: t('home.quickActions.systemSettings', { defaultValue: 'System Settings' }), description: t('home.quickActions.systemSettingsDesc', { defaultValue: 'Configure your workspace' }), icon: Settings, - href: '/apps/setup', + // #3611 — the system hub, not the bare `/apps/setup` (which is the + // "No Apps Configured" empty state's own URL on a zero-app deployment). + href: '/apps/setup/system', iconBg: 'bg-gradient-to-br from-emerald-500/15 to-teal-500/10 ring-emerald-500/20', iconText: 'text-emerald-600 dark:text-emerald-400', hoverBorder: 'hover:border-emerald-500/40', diff --git a/packages/app-shell/src/console/home/__tests__/QuickActions.settingsTarget.test.tsx b/packages/app-shell/src/console/home/__tests__/QuickActions.settingsTarget.test.tsx new file mode 100644 index 0000000000..7f1799f513 --- /dev/null +++ b/packages/app-shell/src/console/home/__tests__/QuickActions.settingsTarget.test.tsx @@ -0,0 +1,110 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `QuickActions` — "System Settings" card target (objectui#3611). + * + * ## This one is DORMANT, and the test says so on purpose + * + * Unlike the other three sites #3611 fixes, no user can reach this card today: + * `QuickActions` has zero JSX call sites repo-wide. It is exported from + * `console/home/index.ts` and rendered by nobody (`HomePage` builds its own + * tiles). So there is no user-visible behavior change here and nothing to + * verify through a mounted page. + * + * It was fixed anyway, in the same pass, for one reason: the day someone + * remounts this component on `/home`, the dead link comes back with it. This + * file is the guard that makes that reappearance impossible — it renders the + * component DIRECTLY (the honest scope for dormant code) rather than pretending + * a route reaches it. + * + * ## The target + * + * Same root cause as its three live siblings: `AppContent` mounts the system + * hub only on `isSystemRoute`, so a bare `/apps/setup` is the "No Apps + * Configured" empty state's own URL on a zero-app deployment. The card's + * sibling ("Manage Objects") already spelled `/apps/setup/system/...`, which is + * what made this one the odd entry out. + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { MemoryRouter, Routes, Route, useLocation } from 'react-router-dom'; + +vi.mock('@object-ui/i18n', async (importOriginal) => ({ + ...(await importOriginal>()), + useObjectTranslation: () => ({ + t: (key: string, options?: Record) => String(options?.defaultValue ?? key), + }), +})); + +import { QuickActions } from '../QuickActions'; + +/** Reports where a card's navigate() actually put the router. */ +function Landing() { + const { pathname } = useLocation(); + return
{pathname}
; +} + +function renderQuickActions() { + render( + + + + } /> + + , + ); +} + +const SYSTEM_HUB = '/apps/setup/system'; + +describe('QuickActions system-settings card (objectui#3611, dormant)', () => { + it('DORMANCY PRECONDITION: nothing renders this component, so the fix is a guard, not a user-visible change', async () => { + // Recorded as an assertion rather than prose so it goes red the day the + // component is remounted — at which point the pin below stops being a + // guard and becomes a live-path test, and this file should be re-read. + const { readFileSync, readdirSync, statSync } = await import('node:fs'); + const path = await import('node:path'); + const { fileURLToPath } = await import('node:url'); + + const here = path.dirname(fileURLToPath(import.meta.url)); + // .../src/console/home/__tests__ -> .../src + const srcRoot = path.resolve(here, '../../..'); + + const callSites: string[] = []; + const walk = (dir: string) => { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + if (entry.name === 'node_modules' || entry.name === 'dist') continue; + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + walk(full); + } else if (/\.tsx$/.test(entry.name) && !/\.(test|spec)\.tsx$/.test(entry.name)) { + if (/ { + const user = userEvent.setup(); + renderQuickActions(); + + await user.click(screen.getByTestId('quick-action-system-settings')); + + expect(screen.getByTestId('landing')).toHaveTextContent(SYSTEM_HUB); + }); + + it('REGRESSION: the sibling card that was already hub-scoped is unchanged', async () => { + const user = userEvent.setup(); + renderQuickActions(); + + await user.click(screen.getByTestId('quick-action-manage-objects')); + + expect(screen.getByTestId('landing')).toHaveTextContent(`${SYSTEM_HUB}/metadata/object`); + }); +}); diff --git a/packages/app-shell/src/layout/AppSidebar.tsx b/packages/app-shell/src/layout/AppSidebar.tsx index 9e0ecf9e10..926dffe9bc 100644 --- a/packages/app-shell/src/layout/AppSidebar.tsx +++ b/packages/app-shell/src/layout/AppSidebar.tsx @@ -451,7 +451,14 @@ export function AppSidebar({ activeAppName, onAppChange }: { activeAppName: stri /* No-app fallback header */ navigate('/apps/setup')} + /* #3611 — the system hub is `/apps/setup/system`, not the bare + `/apps/setup`. This header renders ONLY when `activeApp` is + falsy, i.e. exactly on the zero-app deployment where + `/apps/setup` is the "No Apps Configured" empty state's own + URL — so the bare target sent the user back to the screen + they were already looking at. Same fix as the `sys-settings` + entry above (#3590). */ + onClick={() => navigate('/apps/setup/system')} data-testid="system-sidebar-header" >
@@ -680,8 +687,12 @@ export function AppSidebar({ activeAppName, onAppChange }: { activeAppName: stri + {/* #3611 — "Settings" means the system hub. The bare + `/apps/setup` resolves to the "No Apps Configured" empty + state on a zero-app deployment, so this entry looped in + place there. */} navigate('/apps/setup')} + onClick={() => navigate('/apps/setup/system')} > {t('user.settings', { defaultValue: 'Settings' })} diff --git a/packages/app-shell/src/layout/__tests__/appSidebarSettingsTargets.test.tsx b/packages/app-shell/src/layout/__tests__/appSidebarSettingsTargets.test.tsx new file mode 100644 index 0000000000..4584431fd5 --- /dev/null +++ b/packages/app-shell/src/layout/__tests__/appSidebarSettingsTargets.test.tsx @@ -0,0 +1,222 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * AppSidebar's two IMPERATIVE "Settings" senders — must target the system HUB + * (objectui#3611). + * + * ## Why these two are separate from the ones #3590 fixed + * + * #3590 / PR #3608 retargeted the `sys-settings` NAV ENTRY (a declarative + * `url:` on a navigation item, asserted as an `href` in + * `systemNavSettingsTarget.test.tsx`) and the empty state's CTA. The two sites + * pinned here are neither: they are `onClick={() => navigate(...)}` handlers, + * so they carry no `href` and no navigation item to inspect. They were outside + * #3590's declared file surface and stayed on the bare URL. + * + * ## The defect + * + * `AppContent` mounts the system hub only when `isSystemRoute` + * (`pathname.includes('/system')`) holds. A bare `/apps/setup` therefore falls + * into the `!activeApp && !isCreateAppRoute && !isSystemRoute && !isMetadataRoute` + * guard and renders the "No Apps Configured" empty state — on a zero-app + * deployment `/apps/setup` IS that empty state's own URL. Both senders below + * spelled it, so both looped in place. + * + * `system-sidebar-header` is the sharpest of the two: it renders ONLY in the + * `activeApp` falsy branch, i.e. it is unreachable EXCEPT in exactly the state + * where its old target was broken. The user-menu entry renders in every + * deployment but is only *broken* in the zero-app one. + * + * ## What is asserted, and what is not + * + * These assert the URL each handler SENDS. What that 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 dropdown primitives are replaced with passthroughs (same technique, and + * same reason, as `WorkspaceSwitcher.test.tsx`): the subject here is the + * constant the handler closes over, not Radix's open/close choreography, and + * jsdom + Radix's modal `pointer-events: none` makes driving the real menu a + * source of flake unrelated to anything this file is measuring. + */ + +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 userEvent from '@testing-library/user-event'; +import { MemoryRouter } from 'react-router-dom'; + +// --------------------------------------------------------------------------- +// Mocks — providers and console-only chrome, matching the sibling sidebar +// suites. `@object-ui/layout` stays REAL so the fallback nav cluster below is +// the one AppSidebar's own render path emits. +// --------------------------------------------------------------------------- + +/** The imperative target under test. `MemoryRouter`/`Link`/`useLocation` stay real. */ +const navigate = vi.fn(); +vi.mock('react-router-dom', async (importOriginal) => ({ + ...(await importOriginal>()), + useNavigate: () => navigate, +})); + +// Passthrough dropdown primitives so the footer menu's items render without +// interaction. Everything else in @object-ui/components (Sidebar*, Avatar) is +// the real implementation. +vi.mock('@object-ui/components', async (importOriginal) => ({ + ...(await importOriginal>()), + DropdownMenu: ({ children }: { children?: React.ReactNode }) =>
{children}
, + DropdownMenuTrigger: ({ children }: { children?: React.ReactNode }) => <>{children}, + DropdownMenuContent: ({ children }: { children?: React.ReactNode }) =>
{children}
, + DropdownMenuGroup: ({ children }: { children?: React.ReactNode }) =>
{children}
, + DropdownMenuLabel: ({ children }: { children?: React.ReactNode }) =>
{children}
, + DropdownMenuSeparator: () =>
, + DropdownMenuItem: ({ + children, + onClick, + }: { + children?: React.ReactNode; + onClick?: () => void; + }) => ( +
+ {children} +
+ ), +})); + +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, + }), +})); + +vi.mock('@object-ui/auth', () => ({ + useAuth: () => ({ + user: { id: 'u1', name: 'Ada', email: 'ada@example.com' }, + 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 both senders exist 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(), +})); +vi.mock('../../context/NavigationContext', () => ({ + useNavigationContext: () => ({ context: 'app', currentAppName: 'setup' }), +})); +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'; + +/** The system hub — the reachable target, and what every sibling entry prefixes. */ +const SYSTEM_HUB = '/apps/setup/system'; +/** The empty state's OWN url — what both senders used to spell. */ +const BARE_SETUP = '/apps/setup'; + +function renderSidebar() { + return render( + + + {}} /> + + , + ); +} + +beforeEach(() => { + localStorage.clear(); + navigate.mockClear(); +}); + +describe('AppSidebar imperative Settings senders (objectui#3611)', () => { + it('the no-app sidebar header sends to the system hub, not back to the empty state', async () => { + const user = userEvent.setup(); + renderSidebar(); + + // Precondition: with zero apps this really is the no-active-app branch — + // otherwise the header under test would not be on screen at all and the + // assertion below would be vacuous. + expect(screen.getByTestId('system-fallback-nav')).toBeInTheDocument(); + + await user.click(screen.getByTestId('system-sidebar-header')); + + expect(navigate).toHaveBeenCalledWith(SYSTEM_HUB); + // The regression: bare `/apps/setup` re-renders the very empty state this + // header is drawn on top of. + expect(navigate).not.toHaveBeenCalledWith(BARE_SETUP); + }); + + it('the user-menu Settings entry sends to the system hub', async () => { + const user = userEvent.setup(); + renderSidebar(); + + // `Settings` is an exact match — it does not collide with the fallback + // cluster's `System Settings` link. + await user.click(screen.getByRole('menuitem', { name: 'Settings' })); + + expect(navigate).toHaveBeenCalledWith(SYSTEM_HUB); + expect(navigate).not.toHaveBeenCalledWith(BARE_SETUP); + }); + + it('REGRESSION: the sibling app-switcher entry that was already hub-scoped is unchanged', () => { + renderSidebar(); + + // `sys-settings`, corrected by #3590, is the anchor that made these two the + // odd ones out. It is a declarative `url:` (an href), not a navigate() — + // which is exactly why #3590's sweep did not reach the two above. + expect(screen.getByRole('link', { name: 'System Settings' })).toHaveAttribute( + 'href', + SYSTEM_HUB, + ); + }); +});