diff --git a/apps/desktop/src/app/command-palette/index.tsx b/apps/desktop/src/app/command-palette/index.tsx index 42bcef4e820a..e86bd2415e39 100644 --- a/apps/desktop/src/app/command-palette/index.tsx +++ b/apps/desktop/src/app/command-palette/index.tsx @@ -71,6 +71,7 @@ import { COMMAND_CENTER_ROUTE, CRON_ROUTE, MESSAGING_ROUTE, + navigateToWorkspacePage, NEW_CHAT_ROUTE, PROFILES_ROUTE, sessionRoute, @@ -351,7 +352,7 @@ export function CommandPalette() { } }, [open, pendingPage]) - const go = useCallback((path: string) => () => navigate(path), [navigate]) + const go = useCallback((path: string) => () => navigateToWorkspacePage(navigate, path), [navigate]) // Step up one nested page (or back to the root list), clearing the filter so // the parent page doesn't reopen mid-search. diff --git a/apps/desktop/src/app/contrib/wiring.tsx b/apps/desktop/src/app/contrib/wiring.tsx index de2e0becffdb..0cbda7af36f1 100644 --- a/apps/desktop/src/app/contrib/wiring.tsx +++ b/apps/desktop/src/app/contrib/wiring.tsx @@ -78,10 +78,11 @@ import { closeAllTerminals } from '../right-sidebar/terminal/terminals' import { $workspaceIsPage, CRON_ROUTE, + navigateToWorkspacePage, routeSessionId, sessionRoute, SETTINGS_ROUTE, - syncWorkspaceIsPage + syncWorkspaceRoute } from '../routes' import { SessionPickerOverlay } from '../session-picker-overlay' import { SessionSwitcher } from '../session-switcher' @@ -186,11 +187,12 @@ export function ContribWiring({ children }: { children: ReactNode }) { routedSessionIdRef.current = null }, []) - // Mirror "the workspace is showing a full page" into its atom — the - // workspace pane contribution re-registers headerVeto from it, so the main - // zone's tab bar stands down on pages (and returns with the chat). + // Point the workspace at the route: the pane contribution re-registers + // headerVeto from $workspaceIsPage (so the main zone's tab bar stands down + // on pages), and a page route fronts the pane so it can't stay stuck behind + // a focused session tile. useEffect(() => { - syncWorkspaceIsPage(location.pathname) + syncWorkspaceRoute(location.pathname) }, [location.pathname]) const { @@ -1015,7 +1017,7 @@ export function ContribWiring({ children }: { children: ReactNode }) { initialSection={commandCenterInitialSection} onClose={closeOverlayToPreviousRoute} onDeleteSession={removeSession} - onNavigateRoute={path => navigate(path)} + onNavigateRoute={path => navigateToWorkspacePage(navigate, path)} onOpenSession={sessionId => navigate(sessionRoute(sessionId))} /> diff --git a/apps/desktop/src/app/hooks/use-keybinds.ts b/apps/desktop/src/app/hooks/use-keybinds.ts index 8a1fe275478e..7e49029d1cea 100644 --- a/apps/desktop/src/app/hooks/use-keybinds.ts +++ b/apps/desktop/src/app/hooks/use-keybinds.ts @@ -57,6 +57,7 @@ import { ARTIFACTS_ROUTE, CRON_ROUTE, MESSAGING_ROUTE, + navigateToWorkspacePage, PROFILES_ROUTE, sessionRoute, SETTINGS_ROUTE, @@ -138,9 +139,9 @@ export function useKeybinds(deps: KeybindRuntimeDeps): void { 'nav.commandCenter': deps.toggleCommandCenter, 'nav.settings': () => navigate(SETTINGS_ROUTE), 'nav.profiles': () => navigate(PROFILES_ROUTE), - 'nav.skills': () => navigate(SKILLS_ROUTE), - 'nav.messaging': () => navigate(MESSAGING_ROUTE), - 'nav.artifacts': () => navigate(ARTIFACTS_ROUTE), + 'nav.skills': () => navigateToWorkspacePage(navigate, SKILLS_ROUTE), + 'nav.messaging': () => navigateToWorkspacePage(navigate, MESSAGING_ROUTE), + 'nav.artifacts': () => navigateToWorkspacePage(navigate, ARTIFACTS_ROUTE), 'nav.cron': () => navigate(CRON_ROUTE), 'nav.agents': () => navigate(AGENTS_ROUTE), diff --git a/apps/desktop/src/app/routes.ts b/apps/desktop/src/app/routes.ts index b3c28a0ae017..9ee9365c3287 100644 --- a/apps/desktop/src/app/routes.ts +++ b/apps/desktop/src/app/routes.ts @@ -1,8 +1,11 @@ import { atom } from 'nanostores' import type { ReactNode } from 'react' +import { noteActiveTreeGroup, revealTreePane } from '@/components/pane-shell/tree/store' import { registry } from '@/contrib/registry' +type NavigateLike = (to: string, options?: { replace?: boolean }) => void + export const SESSION_ROUTE_PREFIX = '/' export const NEW_CHAT_ROUTE = '/' export const SETTINGS_ROUTE = '/settings' @@ -133,16 +136,30 @@ export function isOverlayView(view: AppView): boolean { return OVERLAY_VIEWS.has(view) } +/** The pathname of a router target. Every classifier below reasons about a + * PATH, but callers navigate to full targets (`/skills?tab=mcp`), and an + * unstripped query reaches the session-id parser — `/skills?tab=mcp` reads as + * the session `skills?tab=mcp`, so Capabilities classifies as a chat. + * `sessionRoute` percent-encodes ids, so `?`/`#` can only start a query or a + * hash. */ +export function routePathname(to: string): string { + const cut = to.search(/[?#]/) + + return cut === -1 ? to : to.slice(0, cut) +} + export function isNewChatRoute(pathname: string): boolean { - return pathname === NEW_CHAT_ROUTE + return routePathname(pathname) === NEW_CHAT_ROUTE } export function routeSessionId(pathname: string): string | null { - if (!pathname.startsWith(SESSION_ROUTE_PREFIX) || RESERVED_PATHS.has(pathname) || isContributedPath(pathname)) { + const path = routePathname(pathname) + + if (!path.startsWith(SESSION_ROUTE_PREFIX) || RESERVED_PATHS.has(path) || isContributedPath(path)) { return null } - const id = pathname.slice(SESSION_ROUTE_PREFIX.length) + const id = path.slice(SESSION_ROUTE_PREFIX.length) return id && !id.includes('/') ? decodeURIComponent(id) : null } @@ -169,15 +186,26 @@ export function sessionRoute(sessionId: string): string { } export function appViewForPath(pathname: string): AppView { - if (isNewChatRoute(pathname) || routeSessionId(pathname)) { + const path = routePathname(pathname) + + if (isNewChatRoute(path) || routeSessionId(path)) { return 'chat' } - if (isContributedPath(pathname)) { + if (isContributedPath(path)) { return 'extension' } - return APP_VIEW_BY_PATH.get(pathname) ?? 'chat' + return APP_VIEW_BY_PATH.get(path) ?? 'chat' +} + +/** Does `to` land on a full page rendered INSIDE the workspace pane + * (skills/messaging/artifacts/contributed routes)? Overlays don't count — + * they float over whatever the workspace is already showing. */ +function isWorkspacePageRoute(to: string): boolean { + const view = appViewForPath(to) + + return view !== 'chat' && !isOverlayView(view) } /** True while the workspace pane shows a FULL PAGE (skills/messaging/ @@ -187,11 +215,49 @@ export function appViewForPath(pathname: string): AppView { * (settings/…) don't count — the chat stays beneath them. */ export const $workspaceIsPage = atom(false) -export function syncWorkspaceIsPage(pathname: string): void { - const view = appViewForPath(pathname) - const isPage = view !== 'chat' && !isOverlayView(view) +function revealWorkspacePane(): void { + noteActiveTreeGroup(null) + revealTreePane('workspace') +} + +/** + * Point the workspace at `pathname`: mirror "showing a full page" into + * `$workspaceIsPage`, and FRONT the pane when it is one. + * + * A page renders inside `workspace`, so a main zone parked on a session tile + * keeps the tile on screen while the route and the page content change behind + * it — the navigation looks dead (#72602). Session switches already front the + * pane in `store/session-states.ts`; pages had no equivalent. + * + * The router location drives this, so every entry point gets it without opting + * in: sidebar, keybinds, command palette, Command Center, contributed + * statusbar/titlebar `to` targets, back/forward, and cold-start restore. + */ +export function syncWorkspaceRoute(pathname: string): void { + const isPage = isWorkspacePageRoute(pathname) if (isPage !== $workspaceIsPage.get()) { $workspaceIsPage.set(isPage) } + + if (isPage) { + revealWorkspacePane() + } +} + +/** + * Navigate to `to`, fronting the workspace pane when it is a page route. + * + * `syncWorkspaceRoute` covers route CHANGES; this covers the RE-CLICK, the one + * case it can't see — hitting Capabilities while already on `/skills` with a + * tile focused leaves the location untouched, so no effect fires and only an + * imperative reveal brings the page back. Use it wherever a nav affordance can + * be triggered from the page it targets. + */ +export function navigateToWorkspacePage(navigate: NavigateLike, to: string, options?: { replace?: boolean }): void { + navigate(to, options) + + if (isWorkspacePageRoute(to)) { + revealWorkspacePane() + } } diff --git a/apps/desktop/src/app/routes.workspace-reveal.test.ts b/apps/desktop/src/app/routes.workspace-reveal.test.ts new file mode 100644 index 000000000000..7db75e610aed --- /dev/null +++ b/apps/desktop/src/app/routes.workspace-reveal.test.ts @@ -0,0 +1,189 @@ +/** + * A full page (Capabilities/Messaging/Artifacts/a contributed route) renders + * INSIDE the `workspace` pane, so navigating to one has to front that pane — + * otherwise a main zone parked on a session tile keeps the tile on screen and + * the click looks dead until the app restarts (#72602). + * + * Two layers, both covered here: `syncWorkspaceRoute` (the router location, + * every entry point) and `navigateToWorkspacePage` (the re-click, where the + * location doesn't change). + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { registry } from '@/contrib/registry' + +import { + $workspaceIsPage, + AGENTS_ROUTE, + appViewForPath, + ARTIFACTS_ROUTE, + CRON_ROUTE, + MESSAGING_ROUTE, + navigateToWorkspacePage, + NEW_CHAT_ROUTE, + routePathname, + ROUTES_AREA, + routeSessionId, + sessionRoute, + SETTINGS_ROUTE, + SKILLS_ROUTE, + syncWorkspaceRoute +} from './routes' + +vi.mock('@/components/pane-shell/tree/store', async importOriginal => ({ + ...(await importOriginal>()), + noteActiveTreeGroup: vi.fn(), + revealTreePane: vi.fn() +})) + +const { noteActiveTreeGroup, revealTreePane } = await import('@/components/pane-shell/tree/store') + +const CONTRIBUTED_ROUTE = '/kanban' + +function contributeRoute(): () => void { + return registry.register({ + area: ROUTES_AREA, + data: { path: CONTRIBUTED_ROUTE }, + id: 'test-route', + render: () => null + }) +} + +/** Did the workspace pane get fronted? Both calls, or the tab stays put. */ +const fronted = () => + vi.mocked(revealTreePane).mock.calls.some(([pane]) => pane === 'workspace') && + vi.mocked(noteActiveTreeGroup).mock.calls.some(([group]) => group === null) + +beforeEach(() => { + vi.mocked(revealTreePane).mockClear() + vi.mocked(noteActiveTreeGroup).mockClear() + $workspaceIsPage.set(false) +}) + +afterEach(() => { + $workspaceIsPage.set(false) +}) + +describe('routePathname', () => { + it('keeps a bare path and drops a query or hash', () => { + expect(routePathname(SKILLS_ROUTE)).toBe('/skills') + expect(routePathname('/skills?tab=mcp')).toBe('/skills') + expect(routePathname('/skills?tab=mcp&server=ctx7')).toBe('/skills') + expect(routePathname('/settings#keys')).toBe('/settings') + }) + + it('leaves an encoded session id alone', () => { + const route = sessionRoute('a?b#c') + + expect(routePathname(route)).toBe(route) + expect(routeSessionId(route)).toBe('a?b#c') + }) +}) + +describe('classification of targets carrying a query', () => { + // The palette navigates to every one of these (Capabilities tabs, MCP + // servers), and Settings redirects old /settings?tab=mcp deep links to the + // last one. Unstripped, they parsed as SESSION ids and read as 'chat'. + it.each([ + [`${SKILLS_ROUTE}?tab=skills`, 'skills'], + [`${SKILLS_ROUTE}?tab=toolsets`, 'skills'], + [`${SKILLS_ROUTE}?tab=mcp&server=ctx7`, 'skills'], + [`${SETTINGS_ROUTE}?tab=keys`, 'settings'] + ])('%s is not a session route', (to, view) => { + expect(routeSessionId(to)).toBeNull() + expect(appViewForPath(to)).toBe(view) + }) +}) + +describe('syncWorkspaceRoute', () => { + it('publishes and fronts on a page route', () => { + syncWorkspaceRoute(SKILLS_ROUTE) + + expect($workspaceIsPage.get()).toBe(true) + expect(fronted()).toBe(true) + }) + + it('fronts on a page route reached with a query', () => { + syncWorkspaceRoute(`${SKILLS_ROUTE}?tab=mcp`) + + expect($workspaceIsPage.get()).toBe(true) + expect(fronted()).toBe(true) + }) + + it('fronts when moving between two pages — the atom never changes, the tab must', () => { + syncWorkspaceRoute(ARTIFACTS_ROUTE) + vi.mocked(revealTreePane).mockClear() + vi.mocked(noteActiveTreeGroup).mockClear() + + syncWorkspaceRoute(MESSAGING_ROUTE) + + expect($workspaceIsPage.get()).toBe(true) + expect(fronted()).toBe(true) + }) + + it('fronts on a contributed page route', () => { + const dispose = contributeRoute() + + try { + syncWorkspaceRoute(CONTRIBUTED_ROUTE) + + expect(appViewForPath(CONTRIBUTED_ROUTE)).toBe('extension') + expect(fronted()).toBe(true) + } finally { + dispose() + } + }) + + it.each([ + ['a session route', sessionRoute('sess-a')], + ['the new-chat route', NEW_CHAT_ROUTE], + ['an overlay', SETTINGS_ROUTE], + ['an overlay with a query', `${SETTINGS_ROUTE}?tab=keys`], + ['another overlay', CRON_ROUTE], + ['yet another overlay', AGENTS_ROUTE] + ])('leaves the tab alone on %s', (_label, to) => { + syncWorkspaceRoute(to) + + expect($workspaceIsPage.get()).toBe(false) + expect(revealTreePane).not.toHaveBeenCalled() + }) +}) + +describe('navigateToWorkspacePage', () => { + it('navigates and fronts, so a re-click on the page you are already on still shows it', () => { + const navigate = vi.fn() + + navigateToWorkspacePage(navigate, SKILLS_ROUTE) + + expect(navigate).toHaveBeenCalledWith(SKILLS_ROUTE, undefined) + expect(fronted()).toBe(true) + }) + + it.each([`${SKILLS_ROUTE}?tab=skills`, `${SKILLS_ROUTE}?tab=toolsets`, `${SKILLS_ROUTE}?tab=mcp&server=ctx7`])( + 'fronts for the palette target %s', + to => { + navigateToWorkspacePage(vi.fn(), to) + + expect(fronted()).toBe(true) + } + ) + + it('passes navigation options through', () => { + const navigate = vi.fn() + + navigateToWorkspacePage(navigate, ARTIFACTS_ROUTE, { replace: true }) + + expect(navigate).toHaveBeenCalledWith(ARTIFACTS_ROUTE, { replace: true }) + }) + + it('navigates without fronting for chat and overlay targets', () => { + const navigate = vi.fn() + + navigateToWorkspacePage(navigate, sessionRoute('sess-a')) + navigateToWorkspacePage(navigate, SETTINGS_ROUTE) + + expect(navigate).toHaveBeenCalledTimes(2) + expect(revealTreePane).not.toHaveBeenCalled() + }) +}) diff --git a/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx b/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx index 531937b64b83..31882d91b66d 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx +++ b/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx @@ -3,6 +3,7 @@ import type { MutableRefObject } from 'react' import { useEffect } from 'react' import { afterEach, describe, expect, it, vi } from 'vitest' +import { noteActiveTreeGroup, revealTreePane } from '@/components/pane-shell/tree/store' import { getSession, getSessionMessages, type SessionInfo } from '@/hermes' import { createClientSessionState } from '@/lib/chat-runtime' import { clearSessionDraft, stashSessionDraft, takeSessionDraft } from '@/store/composer' @@ -55,6 +56,12 @@ vi.mock('@/store/profile', async importOriginal => ({ ensureGatewayProfile: vi.fn().mockResolvedValue(undefined) })) +vi.mock('@/components/pane-shell/tree/store', async importOriginal => ({ + ...(await importOriginal>()), + noteActiveTreeGroup: vi.fn(), + revealTreePane: vi.fn() +})) + const RUNTIME_SESSION_ID = 'rt-new-001' function deferred() { @@ -69,7 +76,7 @@ function deferred() { type HarnessHandle = Pick< ReturnType, - 'createBackendSessionForSend' | 'startFreshSessionDraft' + 'createBackendSessionForSend' | 'selectSidebarItem' | 'startFreshSessionDraft' > function storedSession(overrides: Partial = {}): SessionInfo { @@ -1590,3 +1597,21 @@ describe('createBackendSessionForSend workspace target', () => { expect(params).toMatchObject({ cwd: '/clicked-workspace' }) }) }) +describe('selectSidebarItem', () => { + it('fronts the workspace pane when navigating to a sidebar route (issue #72602)', async () => { + const navigate = vi.fn() + const requestGateway = vi.fn(async () => ({}) as never) + let handle: HarnessHandle | null = null + + render( (handle = value)} requestGateway={requestGateway} />) + await waitFor(() => expect(handle).not.toBeNull()) + + act(() => { + handle!.selectSidebarItem({ icon: (() => null) as never, id: 'skills', label: 'Capabilities', route: '/skills' }) + }) + + expect(navigate).toHaveBeenCalledWith('/skills', undefined) + expect(noteActiveTreeGroup).toHaveBeenCalledWith(null) + expect(revealTreePane).toHaveBeenCalledWith('workspace') + }) +}) diff --git a/apps/desktop/src/app/session/hooks/use-session-actions/index.ts b/apps/desktop/src/app/session/hooks/use-session-actions/index.ts index fd490666d126..5d293c012b21 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions/index.ts +++ b/apps/desktop/src/app/session/hooks/use-session-actions/index.ts @@ -68,7 +68,7 @@ import { broadcastSessionsChanged } from '@/store/session-sync' import { isWatchWindow } from '@/store/windows' import type { SessionCreateResponse, SessionMessage, SessionResumeResponse, UsageStats } from '@/types/hermes' -import { NEW_CHAT_ROUTE, sessionRoute, SETTINGS_ROUTE } from '../../../routes' +import { navigateToWorkspacePage, NEW_CHAT_ROUTE, sessionRoute, SETTINGS_ROUTE } from '../../../routes' import type { ClientSessionState, SidebarNavItem } from '../../../types' import { sessionContextDrift } from '../session-context-drift' @@ -458,7 +458,7 @@ export function useSessionActions({ } if (item.route) { - navigate(item.route) + navigateToWorkspacePage(navigate, item.route) } }, [navigate, startFreshSessionDraft]