From cf36b2cc40effd66206688bca0ef7979c5480eab Mon Sep 17 00:00:00 2001 From: Christoph Leiter Date: Fri, 24 Jul 2026 15:16:37 +0200 Subject: [PATCH] fix(app): pin the active workspace from a collapsed sidebar section (#2299) Cmd+Shift+P did nothing whenever the active workspace's sidebar row was not rendered. The `workspace.pin` handler was registered by the row itself, gated on `selected && canPin`, so collapsing the row's section unmounted the only handler for the action and the dispatcher found zero candidates. The keypress was swallowed with no toast and no error. Move the action to a single always-mounted handler keyed on the active route selection, following the existing `useGlobalNewWorkspaceAction` / `useActiveWorktreeNewAction` pattern, and delete the two per-row registrations. Besides the reported case this also fixes a collapsed status group, a collapsed Pinned section (so unpinning works too), and focus mode. The handler lives in a headless component rather than being called from the root layout, so subscribing to the active workspace's pin state does not re-render the whole app shell. Two supporting changes: - The controller now takes a narrow `PinnableWorkspace` instead of a full `SidebarWorkspaceEntry`, so a caller without a sidebar row can build one. Its in-flight guard moves to module scope, because the row menus and the shortcut hold separate controller instances and a per-instance guard would let a keypress and a menu click fire two concurrent, opposite RPCs. - The handler resolves the descriptor id via `useWorkspaceFields` rather than reusing the route id. The route carries an opaque workspace id that is not guaranteed to equal the descriptor id, which is why `selectWorkspace` resolves it through `resolveWorkspaceMapKeyByIdentity`. Both the RPC and the in-flight key need the descriptor id so that rows and this handler agree on one identity. `workspace.archive` has the same row-scoped defect and is deliberately left alone: it carries per-row state (`isArchiving`, optimistic hiding, the risky-worktree confirm) and needs its own change. Covered by a Playwright spec: the three collapse cases fail without this fix, plus one-RPC-per-press and a rejected-pin case that asserts the error toast and that the next press still succeeds. Co-authored-by: Claude Opus 4.8 --- .../sidebar-workspace-pin-shortcut.spec.ts | 266 ++++++++++++++++++ packages/app/src/app/_layout.tsx | 2 + .../src/components/sidebar-workspace-list.tsx | 11 - .../sidebar/sidebar-status-list.tsx | 11 - .../workspace-pin-shortcut-handler.tsx | 9 + .../hooks/use-global-workspace-pin-action.ts | 64 +++++ .../src/hooks/use-sidebar-workspace-pin.ts | 27 +- 7 files changed, 360 insertions(+), 30 deletions(-) create mode 100644 packages/app/e2e/sidebar-workspace-pin-shortcut.spec.ts create mode 100644 packages/app/src/components/workspace-pin-shortcut-handler.tsx create mode 100644 packages/app/src/hooks/use-global-workspace-pin-action.ts diff --git a/packages/app/e2e/sidebar-workspace-pin-shortcut.spec.ts b/packages/app/e2e/sidebar-workspace-pin-shortcut.spec.ts new file mode 100644 index 000000000..b9a0bb89e --- /dev/null +++ b/packages/app/e2e/sidebar-workspace-pin-shortcut.spec.ts @@ -0,0 +1,266 @@ +import { test, expect, type Page } from "./fixtures"; +import { gotoAppShell } from "./helpers/app"; +import { daemonWsRoutePattern } from "./helpers/daemon-port"; +import { seedWorkspace, type SeededWorkspace } from "./helpers/seed-client"; +import { getServerId } from "./helpers/server-id"; + +// The pin shortcut used to be registered by the sidebar row itself, so it silently did nothing +// whenever the row was unmounted — a collapsed project section being the common case. It now +// lives in a single always-mounted handler keyed on the active route selection. +const PIN_SHORTCUT = "ControlOrMeta+Shift+P"; + +function workspaceRow(page: Page, workspaceId: string) { + return page.getByTestId(`sidebar-workspace-row-${getServerId()}:${workspaceId}`); +} + +function pinnedSection(page: Page) { + return page.getByTestId("sidebar-pinned-section"); +} + +// Opens the workspace so it becomes the active route selection, which is what the shortcut acts on. +async function openWorkspace(page: Page, workspaceId: string) { + const row = workspaceRow(page, workspaceId); + await expect(row).toBeVisible({ timeout: 30_000 }); + await row.click(); + await expect(page).toHaveURL(/\/workspace\//, { timeout: 30_000 }); +} + +// The project key is host-scoped and not exposed by the seed helper, so the header is addressed by +// its display name, scoped to project rows so a workspace row can never match. Pressing the header +// toggles the section, which unmounts every workspace row under it. +async function collapseProjectSection(page: Page, project: SeededWorkspace): Promise { + const header = page + .locator('[data-testid^="sidebar-project-row-"]') + .filter({ hasText: project.projectDisplayName }); + await expect(header).toHaveCount(1, { timeout: 30_000 }); + + await header.click(); + await expect(workspaceRow(page, project.workspaceId)).toHaveCount(0, { timeout: 10_000 }); +} + +async function switchToStatusGrouping(page: Page): Promise { + await page.getByTestId("sidebar-display-preferences-menu").click(); + await page.getByTestId("sidebar-grouping-status").click(); + await expect(page.getByTestId("sidebar-status-list-scroll")).toBeVisible({ timeout: 10_000 }); +} + +// Status mode buckets workspaces by state rather than project, so the group holding this workspace +// is discovered from the rows container it sits in rather than assumed. +async function collapseStatusGroupContaining(page: Page, workspaceId: string): Promise { + const rows = page + .locator('[data-testid^="sidebar-status-group-rows-"]') + .filter({ has: workspaceRow(page, workspaceId) }); + await expect(rows).toHaveCount(1, { timeout: 30_000 }); + + const rowsTestId = await rows.getAttribute("data-testid"); + const bucket = rowsTestId?.replace("sidebar-status-group-rows-", ""); + expect(bucket).toBeTruthy(); + + await page.getByTestId(`sidebar-status-group-${bucket}`).click(); + await expect(workspaceRow(page, workspaceId)).toHaveCount(0, { timeout: 10_000 }); +} + +function readSessionMessage( + message: string | Buffer, +): { type?: unknown; requestId?: unknown } | null { + const raw = typeof message === "string" ? message : message.toString("utf8"); + try { + const envelope = JSON.parse(raw) as { type?: unknown; message?: unknown }; + if (envelope.type !== "session" || typeof envelope.message !== "object") { + return null; + } + return envelope.message as { type?: unknown; requestId?: unknown }; + } catch { + return null; + } +} + +const PIN_REJECTION_MESSAGE = "Pin rejected by test."; + +interface PinRpcGate { + /** Pin requests the client has sent so far. */ + sentCount(): number; +} + +// Proxies everything so the app boots against the real daemon, counting pin RPCs and optionally +// rejecting the first `rejectFirst` of them. The count asserts how many pins one keypress actually +// dispatched, which the rendered pin state cannot show — a toggle that fired twice lands back +// where it started. +async function installPinRpcGate( + page: Page, + options: { rejectFirst?: number } = {}, +): Promise { + const rejectFirst = options.rejectFirst ?? 0; + let sent = 0; + + await page.routeWebSocket(daemonWsRoutePattern(), (ws) => { + const server = ws.connectToServer(); + + ws.onMessage((message) => { + const sessionMessage = readSessionMessage(message); + if ( + sessionMessage?.type === "workspace.pin.set.request" && + typeof sessionMessage.requestId === "string" + ) { + sent += 1; + if (sent <= rejectFirst) { + ws.send( + JSON.stringify({ + type: "session", + message: { + type: "rpc_error", + payload: { + requestId: sessionMessage.requestId, + requestType: "workspace.pin.set.request", + error: PIN_REJECTION_MESSAGE, + code: "transport", + }, + }, + }), + ); + return; + } + } + + try { + server.send(message); + } catch { + // server socket already closed + } + }); + + server.onMessage((message) => { + try { + ws.send(message); + } catch { + // client socket already closed + } + }); + }); + + return { sentCount: () => sent }; +} + +test.describe("Pin workspace shortcut", () => { + test("pins the active workspace while its project section is collapsed", async ({ page }) => { + const workspace = await seedWorkspace({ repoPrefix: "pin-shortcut-collapsed-" }); + + try { + await gotoAppShell(page); + await openWorkspace(page, workspace.workspaceId); + await collapseProjectSection(page, workspace); + + await page.keyboard.press(PIN_SHORTCUT); + + await expect(pinnedSection(page)).toBeVisible({ timeout: 10_000 }); + await expect( + pinnedSection(page).getByTestId( + `sidebar-workspace-row-${getServerId()}:${workspace.workspaceId}`, + ), + ).toBeVisible(); + } finally { + await workspace.cleanup(); + } + }); + + test("unpins the active workspace while the Pinned section is collapsed", async ({ page }) => { + const workspace = await seedWorkspace({ repoPrefix: "pin-shortcut-unpin-" }); + + try { + await gotoAppShell(page); + await openWorkspace(page, workspace.workspaceId); + + await page.keyboard.press(PIN_SHORTCUT); + await expect(pinnedSection(page)).toBeVisible({ timeout: 10_000 }); + + await page.getByTestId("sidebar-pinned-section-header").click(); + await expect(workspaceRow(page, workspace.workspaceId)).toHaveCount(0, { timeout: 10_000 }); + + await page.keyboard.press(PIN_SHORTCUT); + + await expect(pinnedSection(page)).toHaveCount(0, { timeout: 10_000 }); + } finally { + await workspace.cleanup(); + } + }); + + test("sends exactly one pin RPC per press when the row is rendered and selected", async ({ + page, + }) => { + const workspace = await seedWorkspace({ repoPrefix: "pin-shortcut-expanded-" }); + + try { + const gate = await installPinRpcGate(page); + + await gotoAppShell(page); + await openWorkspace(page, workspace.workspaceId); + + await page.keyboard.press(PIN_SHORTCUT); + await expect(pinnedSection(page)).toBeVisible({ timeout: 10_000 }); + // Counting frames catches a press that produces zero or two RPCs — a misfiring in-flight + // guard, or a second dispatch path. It cannot detect a duplicate handler registration: + // `keyboardActionDispatcher.dispatch` returns at the first handler that returns true, so a + // shadowed second handler is unobservable from outside by design. + expect(gate.sentCount()).toBe(1); + + await page.keyboard.press(PIN_SHORTCUT); + await expect(pinnedSection(page)).toHaveCount(0, { timeout: 10_000 }); + expect(gate.sentCount()).toBe(2); + await expect(workspaceRow(page, workspace.workspaceId)).toHaveCount(1); + } finally { + await workspace.cleanup(); + } + }); + + test("pins the active workspace while its status group is collapsed", async ({ page }) => { + const workspace = await seedWorkspace({ repoPrefix: "pin-shortcut-status-" }); + + try { + await gotoAppShell(page); + await openWorkspace(page, workspace.workspaceId); + await switchToStatusGrouping(page); + await collapseStatusGroupContaining(page, workspace.workspaceId); + + await page.keyboard.press(PIN_SHORTCUT); + + await expect(pinnedSection(page)).toBeVisible({ timeout: 10_000 }); + await expect( + pinnedSection(page).getByTestId( + `sidebar-workspace-row-${getServerId()}:${workspace.workspaceId}`, + ), + ).toBeVisible(); + } finally { + await workspace.cleanup(); + } + }); + + test("shows an error toast when the host rejects the pin, and the next press succeeds", async ({ + page, + }) => { + const workspace = await seedWorkspace({ repoPrefix: "pin-shortcut-failure-" }); + + try { + const gate = await installPinRpcGate(page, { rejectFirst: 1 }); + + await gotoAppShell(page); + await openWorkspace(page, workspace.workspaceId); + + await page.keyboard.press(PIN_SHORTCUT); + + await expect(page.getByTestId("app-toast-message")).toContainText(PIN_REJECTION_MESSAGE, { + timeout: 10_000, + }); + await expect(pinnedSection(page)).toHaveCount(0); + await expect(workspaceRow(page, workspace.workspaceId)).toHaveCount(1); + + // The failure must leave the action usable: the in-flight guard has to release the key so a + // retry is not swallowed. Without that release the workspace is unpinnable for the session. + await page.keyboard.press(PIN_SHORTCUT); + + await expect(pinnedSection(page)).toBeVisible({ timeout: 10_000 }); + expect(gate.sentCount()).toBe(2); + } finally { + await workspace.cleanup(); + } + }); +}); diff --git a/packages/app/src/app/_layout.tsx b/packages/app/src/app/_layout.tsx index 5591949f0..febae56d9 100644 --- a/packages/app/src/app/_layout.tsx +++ b/packages/app/src/app/_layout.tsx @@ -32,6 +32,7 @@ import { AppDiagnosticHost } from "@/components/app-diagnostic-host"; import { LeftSidebar } from "@/components/left-sidebar"; import { WindowSidebarMenuToggle } from "@/components/headers/menu-header"; import { SidebarModelProvider } from "@/components/sidebar/sidebar-model"; +import { WorkspacePinShortcutHandler } from "@/components/workspace-pin-shortcut-handler"; import { CompactExplorerSidebarHost } from "@/components/compact-explorer-sidebar-host"; import { ProviderSettingsHost } from "@/components/provider-settings-host"; import { RootErrorBoundary } from "@/components/root-error-boundary"; @@ -551,6 +552,7 @@ function AppContainer({ children, chromeEnabled: chromeEnabledOverride }: AppCon + diff --git a/packages/app/src/components/sidebar-workspace-list.tsx b/packages/app/src/components/sidebar-workspace-list.tsx index 9acba693e..36ffc8951 100644 --- a/packages/app/src/components/sidebar-workspace-list.tsx +++ b/packages/app/src/components/sidebar-workspace-list.tsx @@ -1368,17 +1368,6 @@ function WorkspaceRowWithMenu({ }, }); - useKeyboardActionHandler({ - handlerId: `workspace-pin-${workspace.workspaceKey}`, - actions: ["workspace.pin"], - enabled: selected && canPin, - priority: 0, - handle: () => { - onTogglePin?.(); - return true; - }, - }); - return ( <> { - onTogglePin?.(); - return true; - }, - }); - return ( <> ({ + id: workspace.id, + pinnedAt: workspace.pinnedAt ?? null, + })); + const canPin = useHostFeature(serverId, "workspacePinning"); + const togglePin = useSidebarWorkspacePinController(); + + const handle = useCallback(() => { + if (!serverId || !fields || !canPin) { + return false; + } + const workspaceKey = buildWorkspaceTabPersistenceKey({ + serverId, + workspaceId: fields.id, + }); + if (!workspaceKey) { + return false; + } + togglePin({ + serverId, + workspaceId: fields.id, + workspaceKey, + pinnedAt: fields.pinnedAt, + }); + return true; + }, [canPin, fields, serverId, togglePin]); + + useKeyboardActionHandler({ + handlerId: "workspace-pin-global", + actions: WORKSPACE_PIN_ACTIONS, + enabled: serverId !== null && fields !== null && canPin, + priority: 0, + handle, + }); +} diff --git a/packages/app/src/hooks/use-sidebar-workspace-pin.ts b/packages/app/src/hooks/use-sidebar-workspace-pin.ts index 2019442fb..9edb09f8b 100644 --- a/packages/app/src/hooks/use-sidebar-workspace-pin.ts +++ b/packages/app/src/hooks/use-sidebar-workspace-pin.ts @@ -1,22 +1,33 @@ -import { useCallback, useRef } from "react"; +import { useCallback } from "react"; import { useTranslation } from "react-i18next"; import { useMutation } from "@tanstack/react-query"; import { useToast } from "@/contexts/toast-context"; import type { SidebarWorkspaceEntry } from "@/hooks/use-sidebar-workspaces-list"; import { getHostRuntimeStore } from "@/runtime/host-runtime"; -export type ToggleSidebarWorkspacePin = (workspace: SidebarWorkspaceEntry) => void; +// Everything the pin toggle actually needs. Kept narrower than SidebarWorkspaceEntry so the +// global keyboard handler can build one from the active route selection without a sidebar row. +export type PinnableWorkspace = Pick< + SidebarWorkspaceEntry, + "serverId" | "workspaceId" | "workspaceKey" | "pinnedAt" +>; + +export type ToggleSidebarWorkspacePin = (workspace: PinnableWorkspace) => void; + +// Module scope, not a per-hook ref: the sidebar row menus and the global keyboard shortcut each +// hold their own controller instance, and a per-instance guard would let a keypress and a menu +// click fire two concurrent, opposite setWorkspacePinned calls for the same workspace. +const pendingWorkspaceKeys = new Set(); export function useSidebarWorkspacePinController(): ToggleSidebarWorkspacePin { const { t } = useTranslation(); const toast = useToast(); - const pendingWorkspaceKeysRef = useRef(new Set()); const mutation = useMutation({ mutationFn: async ({ workspace, pinned, }: { - workspace: SidebarWorkspaceEntry; + workspace: PinnableWorkspace; pinned: boolean; }) => { const client = getHostRuntimeStore().getClient(workspace.serverId); @@ -31,17 +42,17 @@ export function useSidebarWorkspacePinController(): ToggleSidebarWorkspacePin { ); }, onSettled: (_data, _error, { workspace }) => { - pendingWorkspaceKeysRef.current.delete(workspace.workspaceKey); + pendingWorkspaceKeys.delete(workspace.workspaceKey); }, }); const mutate = mutation.mutate; return useCallback( - (workspace: SidebarWorkspaceEntry) => { - if (pendingWorkspaceKeysRef.current.has(workspace.workspaceKey)) { + (workspace: PinnableWorkspace) => { + if (pendingWorkspaceKeys.has(workspace.workspaceKey)) { return; } - pendingWorkspaceKeysRef.current.add(workspace.workspaceKey); + pendingWorkspaceKeys.add(workspace.workspaceKey); mutate({ workspace, pinned: workspace.pinnedAt == null }); }, [mutate],