From 3a67b0c25ec502359ee598e721c197bccc6f7869 Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sat, 11 Jul 2026 18:25:39 -0500 Subject: [PATCH 01/18] feat(development): list-machines-in-a-drive query + shared session leaves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Foundation pieces for the Development surface, independent of the (pending) route/URL model: - machine-list service (pure, DI'd) + runtime binding + GET /api/machines ?driveId= + useDriveMachines hook — the one net-new query the aggregated tree needs; every other machine service addresses ONE machine by id. - MachineTree: optional machineLabel/defaultExpanded props (both default to today's behavior) so a list of machines can label each tree and start collapsed. - SessionLeaves: extracted from TerminalTab so the Machine page and the upcoming Development sidebar share one session-leaf implementation. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../app/api/machines/__tests__/route.test.ts | 68 ++++++ apps/web/src/app/api/machines/route.ts | 31 +++ .../terminal/workspace/MachineTree.tsx | 32 ++- .../terminal/workspace/SessionLeaves.tsx | 195 ++++++++++++++++++ apps/web/src/hooks/useDriveMachines.ts | 39 ++++ .../src/lib/machines/machine-list-runtime.ts | 48 +++++ packages/lib/package.json | 8 + .../machines/__tests__/machine-list.test.ts | 60 ++++++ .../lib/src/services/machines/machine-list.ts | 45 ++++ 9 files changed, 522 insertions(+), 4 deletions(-) create mode 100644 apps/web/src/app/api/machines/__tests__/route.test.ts create mode 100644 apps/web/src/app/api/machines/route.ts create mode 100644 apps/web/src/components/layout/middle-content/page-views/terminal/workspace/SessionLeaves.tsx create mode 100644 apps/web/src/hooks/useDriveMachines.ts create mode 100644 apps/web/src/lib/machines/machine-list-runtime.ts create mode 100644 packages/lib/src/services/machines/__tests__/machine-list.test.ts create mode 100644 packages/lib/src/services/machines/machine-list.ts diff --git a/apps/web/src/app/api/machines/__tests__/route.test.ts b/apps/web/src/app/api/machines/__tests__/route.test.ts new file mode 100644 index 0000000000..1226991dcf --- /dev/null +++ b/apps/web/src/app/api/machines/__tests__/route.test.ts @@ -0,0 +1,68 @@ +/** + * Contract tests for GET /api/machines — the Development surface's + * list-machines-in-a-drive query. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const { mockAuthenticateRequest, mockIsAuthError, mockListDriveMachines } = vi.hoisted(() => ({ + mockAuthenticateRequest: vi.fn(), + mockIsAuthError: vi.fn((result: unknown) => result != null && typeof result === 'object' && 'error' in result), + mockListDriveMachines: vi.fn(), +})); + +vi.mock('@/lib/auth', () => ({ + authenticateRequestWithOptions: (...args: unknown[]) => mockAuthenticateRequest(...args), + isAuthError: (result: unknown) => mockIsAuthError(result), +})); + +vi.mock('@/lib/machines/machine-list-runtime', () => ({ + listDriveMachines: (...args: unknown[]) => mockListDriveMachines(...args), +})); + +import { GET } from '../route'; + +const AUTH_OK = { userId: 'user-1' }; +const AUTH_DENIED = { error: new Response(null, { status: 401 }) }; + +const MACHINE = { id: 'machine-1', title: 'Dev box', updatedAt: '2026-07-11T00:00:00.000Z' }; + +beforeEach(() => { + vi.clearAllMocks(); + mockAuthenticateRequest.mockResolvedValue(AUTH_OK); + mockListDriveMachines.mockResolvedValue([MACHINE]); +}); + +describe('GET /api/machines', () => { + it('returns the drive\'s machines for the authenticated user', async () => { + const response = await GET(new Request('http://localhost/api/machines?driveId=drive-1')); + + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ machines: [MACHINE] }); + expect(mockListDriveMachines).toHaveBeenCalledWith('user-1', 'drive-1'); + }); + + it('400s without a driveId', async () => { + const response = await GET(new Request('http://localhost/api/machines')); + + expect(response.status).toBe(400); + expect(mockListDriveMachines).not.toHaveBeenCalled(); + }); + + it('propagates the auth error and never touches the drive', async () => { + mockAuthenticateRequest.mockResolvedValue(AUTH_DENIED); + + const response = await GET(new Request('http://localhost/api/machines?driveId=drive-1')); + + expect(response.status).toBe(401); + expect(mockListDriveMachines).not.toHaveBeenCalled(); + }); + + it('serves an empty list rather than 404 when the drive has no machines', async () => { + mockListDriveMachines.mockResolvedValue([]); + + const response = await GET(new Request('http://localhost/api/machines?driveId=drive-1')); + + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ machines: [] }); + }); +}); diff --git a/apps/web/src/app/api/machines/route.ts b/apps/web/src/app/api/machines/route.ts new file mode 100644 index 0000000000..f21a6cdf4f --- /dev/null +++ b/apps/web/src/app/api/machines/route.ts @@ -0,0 +1,31 @@ +/** + * Machines API — the Development surface's aggregated tree needs the one thing + * no other machine route serves: every Machine in a drive, not one Machine by + * id. + * + * GET ?driveId= → { machines: [{ id, title, updatedAt }] } + * + * Session-only (no MCP/agent tokens) — a human/UI surface, like the rest of + * `/api/machines/*`. The list is filtered to the machines the caller may view, + * so a drive member who has been withheld an individual Machine page never sees + * it in the tree. + */ + +import { NextResponse } from 'next/server'; +import { authenticateRequestWithOptions, isAuthError } from '@/lib/auth'; +import { listDriveMachines } from '@/lib/machines/machine-list-runtime'; + +const AUTH_OPTIONS_READ = { allow: ['session'] as const, requireCSRF: false }; + +export async function GET(request: Request) { + const auth = await authenticateRequestWithOptions(request, AUTH_OPTIONS_READ); + if (isAuthError(auth)) return auth.error; + + const driveId = new URL(request.url).searchParams.get('driveId'); + if (!driveId) { + return NextResponse.json({ error: 'driveId is required' }, { status: 400 }); + } + + const machines = await listDriveMachines(auth.userId, driveId); + return NextResponse.json({ machines }); +} diff --git a/apps/web/src/components/layout/middle-content/page-views/terminal/workspace/MachineTree.tsx b/apps/web/src/components/layout/middle-content/page-views/terminal/workspace/MachineTree.tsx index d7418ca92c..8404c17e8f 100644 --- a/apps/web/src/components/layout/middle-content/page-views/terminal/workspace/MachineTree.tsx +++ b/apps/web/src/components/layout/middle-content/page-views/terminal/workspace/MachineTree.tsx @@ -53,6 +53,20 @@ export type MachineTreeNode = interface MachineTreeProps { machineId: string; + /** + * The machine row's label. Defaults to "Machine" — right on the Machine page, + * where the page's own title is already in the header and there is only one + * machine. The Development surface stacks a tree per machine, so it passes + * each Machine page's title to tell them apart. + */ + machineLabel?: string; + /** + * Whether the machine row starts expanded. Default `true` (the Machine page: + * one tree, and its projects are the point). The Development surface passes + * `false` — its machine rows are collapsed until asked for, so listing N + * machines doesn't fire N project fetches on mount. + */ + defaultExpanded?: boolean; /** Called when a Machine/Project/Branch row is clicked. Omit if the tree itself isn't selectable (e.g. selection lives on injected leaf content instead). */ onSelectNode?: (node: MachineTreeNode) => void; /** @@ -97,11 +111,13 @@ export function isSameMachineTreeNode(a: MachineTreeNode | null | undefined, b: } /** Presentation-only Machine → Project → Branch tree, reusable across any tab that needs this navigation shape (Terminal, Diff, …). Has no opinion on what a row click does — callers own that via `onSelectNode`. */ -export default function MachineTree({ machineId, onSelectNode, isNodeSelectable, selectedNode, renderNodeChildren }: MachineTreeProps) { +export default function MachineTree({ machineId, machineLabel, defaultExpanded, onSelectNode, isNodeSelectable, selectedNode, renderNodeChildren }: MachineTreeProps) { return (
ReactNode; } -function MachineNode({ machineId, onSelectNode, isNodeSelectable, selectedNode, renderNodeChildren }: TreeLevelProps & { machineId: string }) { - const [expanded, setExpanded] = useState(true); +function MachineNode({ + machineId, + machineLabel = 'Machine', + defaultExpanded = true, + onSelectNode, + isNodeSelectable, + selectedNode, + renderNodeChildren, +}: TreeLevelProps & { machineId: string; machineLabel?: string; defaultExpanded?: boolean }) { + const [expanded, setExpanded] = useState(defaultExpanded); const node: MachineTreeNode = { level: 'machine' }; const { projects, isLoading: projectsLoading, addProject, removeProject } = useMachineProjects(expanded ? machineId : null); @@ -210,7 +234,7 @@ function MachineNode({ machineId, onSelectNode, isNodeSelectable, selectedNode, onSelect={selectHandlerFor(node, onSelectNode, isNodeSelectable)} selected={isSameMachineTreeNode(node, selectedNode)} icon={} - label="Machine" + label={machineLabel} labelClassName="font-medium" /> {expanded && ( diff --git a/apps/web/src/components/layout/middle-content/page-views/terminal/workspace/SessionLeaves.tsx b/apps/web/src/components/layout/middle-content/page-views/terminal/workspace/SessionLeaves.tsx new file mode 100644 index 0000000000..ed00a72e68 --- /dev/null +++ b/apps/web/src/components/layout/middle-content/page-views/terminal/workspace/SessionLeaves.tsx @@ -0,0 +1,195 @@ +"use client"; + +import { useCallback, useState } from 'react'; +import { toast } from 'sonner'; +import { Plus, TerminalSquare } from 'lucide-react'; +import { Button } from '@/components/ui/button'; +import { Input } from '@/components/ui/input'; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, + DialogTrigger, +} from '@/components/ui/dialog'; +import { + Select, + SelectContent, + SelectItem, + SelectTrigger, + SelectValue, +} from '@/components/ui/select'; +import { useAgentTerminals, type AgentTerminal } from '@/hooks/useAgentTerminals'; +import { AGENT_LAUNCH_SPECS, type AgentRuntimeType } from '@pagespace/lib/services/machines/agent-terminal-types'; +import type { OpenTerminalScope } from '@/stores/terminal-workspace/useTerminalWorkspaceStore'; +import type { MachineTreeNode } from './MachineTree'; +import ConfirmRemoveDialog from './ConfirmRemoveDialog'; +import RemoveButton from './RemoveButton'; +import { SidebarLoading, SidebarNotice } from '../tabs/tab-states'; + +const AGENT_TYPES = Object.keys(AGENT_LAUNCH_SPECS) as AgentRuntimeType[]; + +/** Resolves a tree node to the `useAgentTerminals` scope it addresses, and the + * `OpenTerminalScope` a session under it opens with. */ +function useNodeTerminals(machineId: string, node: MachineTreeNode) { + const projectName = node.level === 'machine' ? null : node.projectName; + const branchName = node.level === 'branch' ? node.branchName : null; + const scopeFor = useCallback( + (name: string): OpenTerminalScope => ({ + projectName: projectName ?? undefined, + branchName: branchName ?? undefined, + name, + }), + [projectName, branchName], + ); + return { terminals: useAgentTerminals(machineId, projectName, branchName), scopeFor }; +} + +/** + * Session-terminal leaves injected by {@link MachineTree}'s `renderNodeChildren` + * under each expanded node — mounts (and thus fetches) only while its node is + * open. + * + * Shared by the Machine page's Terminal tab and the Development surface's + * sidebar: both hang the same sessions off the same tree, and differ only in + * what `onOpenTerminal` does (open a pane here; route to the machine first, + * there). + */ +export default function SessionLeaves({ + machineId, + node, + onOpenTerminal, +}: { + machineId: string; + node: MachineTreeNode; + onOpenTerminal(scope: OpenTerminalScope): void; +}) { + const { terminals, scopeFor } = useNodeTerminals(machineId, node); + const { agentTerminals, isLoading, addAgentTerminal, removeAgentTerminal } = terminals; + + return ( + onOpenTerminal(scopeFor(name))} + /> + ); +} + +function TerminalList({ + terminals, + isLoading, + onAdd, + onRemove, + onOpen, +}: { + terminals: AgentTerminal[]; + isLoading: boolean; + onAdd(name: string, agentType: AgentRuntimeType): Promise; + onRemove(name: string): Promise; + onOpen(name: string): void; +}) { + const [pendingRemove, setPendingRemove] = useState(null); + + return ( +
+
+ Terminals + +
+ {isLoading && } + {!isLoading && terminals.length === 0 && ( + + )} + {terminals.map((terminal) => ( +
+ + setPendingRemove(terminal.name)} label={`Remove terminal ${terminal.name}`} /> +
+ ))} + !open && setPendingRemove(null)} + title="Remove terminal?" + description={pendingRemove ? `Remove terminal "${pendingRemove}"?` : ''} + onConfirm={() => { + if (pendingRemove === null) return Promise.resolve(); + return onRemove(pendingRemove); + }} + /> +
+ ); +} + +function AddAgentTerminalDialog({ onAdd }: { onAdd(name: string, agentType: AgentRuntimeType): Promise }) { + const [open, setOpen] = useState(false); + const [name, setName] = useState(''); + const [agentType, setAgentType] = useState(AGENT_TYPES[0]); + const [submitting, setSubmitting] = useState(false); + + const handleSubmit = async () => { + setSubmitting(true); + try { + await onAdd(name.trim(), agentType); + setOpen(false); + setName(''); + } catch (err) { + toast.error(err instanceof Error ? err.message : 'Failed to add terminal'); + } finally { + setSubmitting(false); + } + }; + + return ( + + + + + + + Add terminal + Named PTY session running a pluggable agent type at this node's scope. + +
+ setName(e.target.value)} /> + +
+ + + + + + ); +} diff --git a/apps/web/src/hooks/useDriveMachines.ts b/apps/web/src/hooks/useDriveMachines.ts new file mode 100644 index 0000000000..1220c9e356 --- /dev/null +++ b/apps/web/src/hooks/useDriveMachines.ts @@ -0,0 +1,39 @@ +'use client'; + +import useSWR from 'swr'; +import { fetchWithAuth } from '@/lib/auth/auth-fetch'; + +export interface DriveMachine { + id: string; + title: string; + updatedAt: string; +} + +const fetcher = (url: string) => + fetchWithAuth(url).then(async (res) => { + if (!res.ok) { + const body = await res.json().catch(() => null); + throw new Error(body?.error ?? 'Failed to fetch machines'); + } + return res.json() as Promise<{ machines: DriveMachine[] }>; + }); + +/** + * Every Machine page in a drive — the root tier of the Development surface's + * aggregated Machine → Project → Branch → session tree. Pass a `null` `driveId` + * (the driveless `/dashboard/development` route) to disable fetching. + */ +export function useDriveMachines(driveId: string | null) { + const key = driveId ? `/api/machines?driveId=${encodeURIComponent(driveId)}` : null; + + const { data, error, isLoading, mutate } = useSWR(key, fetcher, { + revalidateOnFocus: false, + }); + + return { + machines: data?.machines ?? [], + isLoading, + error: error as Error | undefined, + mutate, + }; +} diff --git a/apps/web/src/lib/machines/machine-list-runtime.ts b/apps/web/src/lib/machines/machine-list-runtime.ts new file mode 100644 index 0000000000..07eb26eecf --- /dev/null +++ b/apps/web/src/lib/machines/machine-list-runtime.ts @@ -0,0 +1,48 @@ +/** + * Production wiring for the shared "list a drive's Machines" service + * (`@pagespace/lib/services/machines/machine-list`) — binds the drive scan to + * the real `pages` table and the visibility filter to the real permission + * function, the same way `machine-access-runtime.ts` binds the per-machine + * view/edit checks. + */ + +import { and, asc, eq } from '@pagespace/db/operators'; +import { db } from '@pagespace/db/db'; +import { pages } from '@pagespace/db/schema/core'; +import { canUserViewPage } from '@pagespace/lib/permissions/permissions'; +import { PageType } from '@pagespace/lib/utils/enums'; +import { + listMachinesInDrive as listMachinesInDriveCore, + type MachineListDeps, + type MachinePageSummary, +} from '@pagespace/lib/services/machines/machine-list'; + +export type { MachinePageSummary }; + +function buildMachineListDeps(): MachineListDeps { + return { + findMachinePagesInDrive: async (driveId) => { + const rows = await db.query.pages.findMany({ + // Covered by the pages_drive_id_is_trashed_type_idx index. + where: and( + eq(pages.driveId, driveId), + eq(pages.isTrashed, false), + eq(pages.type, PageType.MACHINE), + ), + columns: { id: true, title: true, updatedAt: true }, + orderBy: [asc(pages.title)], + }); + return rows.map((row) => ({ + id: row.id, + title: row.title, + updatedAt: row.updatedAt.toISOString(), + })); + }, + canUserViewPage, + }; +} + +/** The Machine pages in `driveId` that the actor may view, ordered by title. */ +export async function listDriveMachines(actorUserId: string, driveId: string): Promise { + return listMachinesInDriveCore(buildMachineListDeps(), actorUserId, driveId); +} diff --git a/packages/lib/package.json b/packages/lib/package.json index 241890f1bb..0fe54112d8 100644 --- a/packages/lib/package.json +++ b/packages/lib/package.json @@ -637,6 +637,11 @@ "import": "./dist/services/machines/machine-access.js", "require": "./dist/services/machines/machine-access.js" }, + "./services/machines/machine-list": { + "types": "./dist/services/machines/machine-list.d.ts", + "import": "./dist/services/machines/machine-list.js", + "require": "./dist/services/machines/machine-list.js" + }, "./services/sandbox/tool-gate": { "types": "./dist/services/sandbox/tool-gate.d.ts", "import": "./dist/services/sandbox/tool-gate.js", @@ -1744,6 +1749,9 @@ "services/machines/machine-access": [ "./dist/services/machines/machine-access.d.ts" ], + "services/machines/machine-list": [ + "./dist/services/machines/machine-list.d.ts" + ], "services/sandbox/sandbox-client/types": [ "./dist/services/sandbox/sandbox-client/types.d.ts" ], diff --git a/packages/lib/src/services/machines/__tests__/machine-list.test.ts b/packages/lib/src/services/machines/__tests__/machine-list.test.ts new file mode 100644 index 0000000000..14bc648984 --- /dev/null +++ b/packages/lib/src/services/machines/__tests__/machine-list.test.ts @@ -0,0 +1,60 @@ +import { describe, test, expect, vi } from 'vitest'; +import { listMachinesInDrive, type MachineListDeps, type MachinePageSummary } from '../machine-list'; + +const machine = (id: string, title = id): MachinePageSummary => ({ + id, + title, + updatedAt: '2026-07-11T00:00:00.000Z', +}); + +function buildDeps( + pagesInDrive: MachinePageSummary[], + viewable: (pageId: string) => boolean, +): MachineListDeps { + return { + findMachinePagesInDrive: vi.fn(async () => pagesInDrive), + canUserViewPage: vi.fn(async (_userId: string, pageId: string) => viewable(pageId)), + }; +} + +describe('listMachinesInDrive', () => { + test('returns the drive\'s machines in scan order', async () => { + const deps = buildDeps([machine('m-1', 'alpha'), machine('m-2', 'beta')], () => true); + + const result = await listMachinesInDrive(deps, 'user-1', 'drive-1'); + + expect(result.map((m) => m.id)).toEqual(['m-1', 'm-2']); + expect(deps.findMachinePagesInDrive).toHaveBeenCalledWith('drive-1'); + }); + + test('withholds a machine the actor cannot view', async () => { + const deps = buildDeps( + [machine('m-1'), machine('m-secret'), machine('m-3')], + (pageId) => pageId !== 'm-secret', + ); + + const result = await listMachinesInDrive(deps, 'user-1', 'drive-1'); + + expect(result.map((m) => m.id)).toEqual(['m-1', 'm-3']); + }); + + test('checks visibility against the acting user', async () => { + const deps = buildDeps([machine('m-1')], () => true); + + await listMachinesInDrive(deps, 'user-42', 'drive-1'); + + expect(deps.canUserViewPage).toHaveBeenCalledWith('user-42', 'm-1'); + }); + + test('a drive with no machines is an empty list, not an error', async () => { + const deps = buildDeps([], () => true); + + expect(await listMachinesInDrive(deps, 'user-1', 'drive-1')).toEqual([]); + }); + + test('a drive whose every machine is withheld is an empty list', async () => { + const deps = buildDeps([machine('m-1'), machine('m-2')], () => false); + + expect(await listMachinesInDrive(deps, 'user-1', 'drive-1')).toEqual([]); + }); +}); diff --git a/packages/lib/src/services/machines/machine-list.ts b/packages/lib/src/services/machines/machine-list.ts new file mode 100644 index 0000000000..58cc3ba0ef --- /dev/null +++ b/packages/lib/src/services/machines/machine-list.ts @@ -0,0 +1,45 @@ +/** + * "Every Machine in this drive" — the Development surface's aggregated tree is + * a list of Machine pages, and nothing else listed one. The existing machine + * services all address ONE machine by id, and the global-machine-config + * repository finds only THE global machine, so this is the one net-new query + * the surface needs. + * + * Pure + DI'd like the rest of `services/machines`: the drive scan and the + * permission check are both injected, so the interesting part (a page the actor + * cannot view must not leak out of the drive scan) is testable without a DB. + */ + +export interface MachinePageSummary { + id: string; + title: string; + /** ISO-8601. Callers order by it or show it; the service itself preserves the scan's order. */ + updatedAt: string; +} + +export interface MachineListDeps { + /** Every non-trashed MACHINE-type page in the drive, in the order it should be presented. */ + findMachinePagesInDrive: (driveId: string) => Promise; + canUserViewPage: (userId: string, pageId: string) => Promise; +} + +/** + * The Machine pages in `driveId` that `actorUserId` may view, in scan order. + * + * The drive scan is a raw `type = MACHINE` query, so it is NOT permission-aware + * on its own — a page-level grant can withhold an individual Machine from a + * drive member. Every candidate is therefore re-checked against + * `canUserViewPage` here, which is the same view-level gate every other machine + * route applies before serving a machine's projects/branches/sessions. + */ +export async function listMachinesInDrive( + deps: MachineListDeps, + actorUserId: string, + driveId: string, +): Promise { + const candidates = await deps.findMachinePagesInDrive(driveId); + const visibility = await Promise.all( + candidates.map((machine) => deps.canUserViewPage(actorUserId, machine.id)), + ); + return candidates.filter((_, index) => visibility[index]); +} From 6eb062d93c083f6a9031a22e777d4e10e9e90998 Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sat, 11 Jul 2026 18:29:44 -0500 Subject: [PATCH 02/18] refactor(machine): TerminalTab consumes the extracted SessionLeaves Completes the extraction across the naming sweep's rename: SessionLeaves now lives at its post-rename path with the machine-workspace store import, and TerminalTab imports it instead of holding a second copy. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../page-views/machine/tabs/TerminalTab.tsx | 189 +----------------- .../machine/workspace/SessionLeaves.tsx | 2 +- 2 files changed, 4 insertions(+), 187 deletions(-) diff --git a/apps/web/src/components/layout/middle-content/page-views/machine/tabs/TerminalTab.tsx b/apps/web/src/components/layout/middle-content/page-views/machine/tabs/TerminalTab.tsx index 589b98660e..2ee0e13a80 100644 --- a/apps/web/src/components/layout/middle-content/page-views/machine/tabs/TerminalTab.tsx +++ b/apps/web/src/components/layout/middle-content/page-views/machine/tabs/TerminalTab.tsx @@ -1,37 +1,11 @@ "use client"; -import { useCallback, useState } from 'react'; +import { useCallback } from 'react'; import dynamic from 'next/dynamic'; -import { toast } from 'sonner'; -import { Plus, TerminalSquare } from 'lucide-react'; -import { Button } from '@/components/ui/button'; -import { Input } from '@/components/ui/input'; -import { - Dialog, - DialogContent, - DialogDescription, - DialogFooter, - DialogHeader, - DialogTitle, - DialogTrigger, -} from '@/components/ui/dialog'; -import { - Select, - SelectContent, - SelectItem, - SelectTrigger, - SelectValue, -} from '@/components/ui/select'; -import { useAgentTerminals, type AgentTerminal } from '@/hooks/useAgentTerminals'; -import { AGENT_LAUNCH_SPECS, type AgentRuntimeType } from '@pagespace/lib/services/machines/agent-terminal-types'; import { useMachineWorkspaceStore, type OpenTerminalScope } from '@/stores/machine-workspace/useMachineWorkspaceStore'; import MachineTree, { type MachineTreeNode } from '../workspace/MachineTree'; -import ConfirmRemoveDialog from '../workspace/ConfirmRemoveDialog'; -import RemoveButton from '../workspace/RemoveButton'; +import SessionLeaves from '../workspace/SessionLeaves'; import TabSidebar from './TabSidebar'; -import { SidebarLoading, SidebarNotice } from './tab-states'; - -const AGENT_TYPES = Object.keys(AGENT_LAUNCH_SPECS) as AgentRuntimeType[]; // MachineWorkspace owns the xterm subtree + socket; it must never SSR. const MachineWorkspace = dynamic(() => import('../workspace/MachineWorkspace'), { ssr: false }); @@ -47,7 +21,7 @@ interface TerminalTabProps { * {@link MachineTree} with session-terminal leaves injected under every * Machine/Project/Branch node, beside the pane workspace. This is the new home of * the session navigation that used to live in the right-sidebar Navigator tab — - * clicking a session opens it in the workspace via the shared terminal-workspace + * clicking a session opens it in the workspace via the shared machine-workspace * store, exactly as before. * * Opening a session also `close()`s the sidebar, which on a narrow viewport @@ -90,160 +64,3 @@ function SessionTree({ machineId, onOpened }: { machineId: string; onOpened: () return ; } - -/** Resolves a tree node to the `useAgentTerminals` scope it addresses, and the - * `OpenTerminalScope` a session under it opens with. */ -function useNodeTerminals(machineId: string, node: MachineTreeNode) { - const projectName = node.level === 'machine' ? null : node.projectName; - const branchName = node.level === 'branch' ? node.branchName : null; - const scopeFor = useCallback( - (name: string): OpenTerminalScope => ({ - projectName: projectName ?? undefined, - branchName: branchName ?? undefined, - name, - }), - [projectName, branchName], - ); - return { terminals: useAgentTerminals(machineId, projectName, branchName), scopeFor }; -} - -/** Session-terminal leaves injected by {@link MachineTree}'s `renderNodeChildren` - * under each expanded node — mounts (and thus fetches) only while its node is - * open. */ -function SessionLeaves({ - machineId, - node, - onOpenTerminal, -}: { - machineId: string; - node: MachineTreeNode; - onOpenTerminal(scope: OpenTerminalScope): void; -}) { - const { terminals, scopeFor } = useNodeTerminals(machineId, node); - const { agentTerminals, isLoading, addAgentTerminal, removeAgentTerminal } = terminals; - - return ( - onOpenTerminal(scopeFor(name))} - /> - ); -} - -function TerminalList({ - terminals, - isLoading, - onAdd, - onRemove, - onOpen, -}: { - terminals: AgentTerminal[]; - isLoading: boolean; - onAdd(name: string, agentType: AgentRuntimeType): Promise; - onRemove(name: string): Promise; - onOpen(name: string): void; -}) { - const [pendingRemove, setPendingRemove] = useState(null); - - return ( -
-
- Terminals - -
- {isLoading && } - {!isLoading && terminals.length === 0 && ( - - )} - {terminals.map((terminal) => ( -
- - setPendingRemove(terminal.name)} label={`Remove terminal ${terminal.name}`} /> -
- ))} - !open && setPendingRemove(null)} - title="Remove terminal?" - description={pendingRemove ? `Remove terminal "${pendingRemove}"?` : ''} - onConfirm={() => { - if (pendingRemove === null) return Promise.resolve(); - return onRemove(pendingRemove); - }} - /> -
- ); -} - -function AddAgentTerminalDialog({ onAdd }: { onAdd(name: string, agentType: AgentRuntimeType): Promise }) { - const [open, setOpen] = useState(false); - const [name, setName] = useState(''); - const [agentType, setAgentType] = useState(AGENT_TYPES[0]); - const [submitting, setSubmitting] = useState(false); - - const handleSubmit = async () => { - setSubmitting(true); - try { - await onAdd(name.trim(), agentType); - setOpen(false); - setName(''); - } catch (err) { - toast.error(err instanceof Error ? err.message : 'Failed to add terminal'); - } finally { - setSubmitting(false); - } - }; - - return ( - - - - - - - Add terminal - Named PTY session running a pluggable agent type at this node's scope. - -
- setName(e.target.value)} /> - -
- - - - - - ); -} - diff --git a/apps/web/src/components/layout/middle-content/page-views/machine/workspace/SessionLeaves.tsx b/apps/web/src/components/layout/middle-content/page-views/machine/workspace/SessionLeaves.tsx index ed00a72e68..7155c1840f 100644 --- a/apps/web/src/components/layout/middle-content/page-views/machine/workspace/SessionLeaves.tsx +++ b/apps/web/src/components/layout/middle-content/page-views/machine/workspace/SessionLeaves.tsx @@ -23,7 +23,7 @@ import { } from '@/components/ui/select'; import { useAgentTerminals, type AgentTerminal } from '@/hooks/useAgentTerminals'; import { AGENT_LAUNCH_SPECS, type AgentRuntimeType } from '@pagespace/lib/services/machines/agent-terminal-types'; -import type { OpenTerminalScope } from '@/stores/terminal-workspace/useTerminalWorkspaceStore'; +import type { OpenTerminalScope } from '@/stores/machine-workspace/useMachineWorkspaceStore'; import type { MachineTreeNode } from './MachineTree'; import ConfirmRemoveDialog from './ConfirmRemoveDialog'; import RemoveButton from './RemoveButton'; From c4f120f9c0ea61a517dd5278472d147d26c61c0d Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sat, 11 Jul 2026 23:57:39 -0500 Subject: [PATCH 03/18] =?UTF-8?q?feat(development):=20Development=20surfac?= =?UTF-8?q?e=20=E2=80=94=20nav=20entry,=20sidebar=20swap,=20aggregated=20m?= =?UTF-8?q?achine=20tree,=20routes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A top-level command center for machines: a peer to Channels/DMs/Files that swaps the left sidebar to an aggregated Machine → Project → Branch → session tree for the drive, and reuses the Machine page as its detail pane. Routing — drive-in-path + a thin redirect, NOT the sibling two-tree pattern: - ONE real route tree at /dashboard/[driveId]/development (empty state) and .../[machineId] (MachineView as the detail pane). - /dashboard/development is a redirect only: it resolves the active drive from the drive store (the app's existing find(currentDriveId) ?? first fallback) and forwards. No ?driveId= branch, no duplicate page component. - resolveSidebarVariant() replaces MemoizedSidebar's inline ifs, so one DEVELOPMENT_PATH regex covers both URL shapes and the matchers are testable without rendering a sidebar. Reuse: MachineTree and MachineView unchanged in substance; the sidebar hangs the same SessionLeaves off the same tree the Machine page's Terminal tab does. Tests: sidebar-route matchers, the active-drive resolution, the list-machines service, and GET /api/machines. Typecheck + lint + next build green. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../app/dashboard/DashboardLayoutClient.tsx | 3 +- .../development/[machineId]/page.tsx | 20 ++ .../dashboard/[driveId]/development/page.tsx | 20 ++ .../src/app/dashboard/development/page.tsx | 48 +++++ .../left-sidebar/DevelopmentSidebar.tsx | 182 ++++++++++++++++++ .../layout/left-sidebar/MemoizedSidebar.tsx | 26 +-- .../layout/left-sidebar/PrimaryNavigation.tsx | 11 +- .../__tests__/sidebar-routes.test.ts | 35 ++++ .../layout/left-sidebar/sidebar-routes.ts | 32 +++ .../__tests__/resolve-active-drive.test.ts | 53 +++++ .../lib/development/resolve-active-drive.ts | 24 +++ 11 files changed, 440 insertions(+), 14 deletions(-) create mode 100644 apps/web/src/app/dashboard/[driveId]/development/[machineId]/page.tsx create mode 100644 apps/web/src/app/dashboard/[driveId]/development/page.tsx create mode 100644 apps/web/src/app/dashboard/development/page.tsx create mode 100644 apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx create mode 100644 apps/web/src/components/layout/left-sidebar/__tests__/sidebar-routes.test.ts create mode 100644 apps/web/src/components/layout/left-sidebar/sidebar-routes.ts create mode 100644 apps/web/src/lib/development/__tests__/resolve-active-drive.test.ts create mode 100644 apps/web/src/lib/development/resolve-active-drive.ts diff --git a/apps/web/src/app/dashboard/DashboardLayoutClient.tsx b/apps/web/src/app/dashboard/DashboardLayoutClient.tsx index d2d9e2f41d..fc8cfb88ba 100644 --- a/apps/web/src/app/dashboard/DashboardLayoutClient.tsx +++ b/apps/web/src/app/dashboard/DashboardLayoutClient.tsx @@ -15,6 +15,7 @@ const FULL_PAGE_ROUTES = [ '/dashboard/calendar', '/dashboard/channels', '/dashboard/connections', + '/dashboard/development', '/dashboard/dms', '/dashboard/drives', '/dashboard/storage', @@ -32,7 +33,7 @@ export default function DashboardLayoutClient({ children, nonce }: { children: R // Also match /dashboard/[driveId]/activity pattern const isFullPageRoute = FULL_PAGE_ROUTES.some(route => pathname === route || pathname?.startsWith(route + '/') - ) || pathname?.match(/^\/dashboard\/[^/]+\/(activity|calendar|channels|files|tasks|trash|settings|members|workflows)/); + ) || pathname?.match(/^\/dashboard\/[^/]+\/(activity|calendar|channels|development|files|tasks|trash|settings|members|workflows)/); return ( diff --git a/apps/web/src/app/dashboard/[driveId]/development/[machineId]/page.tsx b/apps/web/src/app/dashboard/[driveId]/development/[machineId]/page.tsx new file mode 100644 index 0000000000..0233d41dc4 --- /dev/null +++ b/apps/web/src/app/dashboard/[driveId]/development/[machineId]/page.tsx @@ -0,0 +1,20 @@ +import MachineView from '@/components/layout/middle-content/page-views/machine/MachineView'; + +/** + * The Development surface's detail pane: the Machine page itself, reused + * verbatim. A Machine's id IS its page id, so the route's `machineId` is + * `MachineView`'s `pageId`. + * + * Only this segment re-renders as the user moves between machines — + * `MemoizedSidebar` sits above the routed page, so the aggregated tree keeps its + * expansion state (and its open terminal panes) across the navigation. + */ +export default async function DevelopmentMachinePage({ + params, +}: { + params: Promise<{ driveId: string; machineId: string }>; +}) { + const { machineId } = await params; + + return ; +} diff --git a/apps/web/src/app/dashboard/[driveId]/development/page.tsx b/apps/web/src/app/dashboard/[driveId]/development/page.tsx new file mode 100644 index 0000000000..015a1d1b7f --- /dev/null +++ b/apps/web/src/app/dashboard/[driveId]/development/page.tsx @@ -0,0 +1,20 @@ +import { SquareTerminal } from 'lucide-react'; + +/** + * The Development surface with no machine selected. The sidebar (the surface's + * actual content — the aggregated machine tree) lives above this route in + * `MemoizedSidebar`, so this is only the detail pane's resting state. + */ +export default function DevelopmentPage() { + return ( +
+ +
+

Select a machine

+

+ Pick a machine from the sidebar to open its terminals, code, and diffs. +

+
+
+ ); +} diff --git a/apps/web/src/app/dashboard/development/page.tsx b/apps/web/src/app/dashboard/development/page.tsx new file mode 100644 index 0000000000..1b418e0756 --- /dev/null +++ b/apps/web/src/app/dashboard/development/page.tsx @@ -0,0 +1,48 @@ +'use client'; + +import { useEffect, useState } from 'react'; +import { useRouter } from 'next/navigation'; +import { useDriveStore } from '@/hooks/useDrive'; +import { resolveActiveDriveId } from '@/lib/development/resolve-active-drive'; + +/** + * The driveless entry to the Development surface. NOT a second implementation of + * the surface — it resolves the active drive and forwards to the one real route + * tree at `/dashboard/[driveId]/development`, so there is exactly one page + * component per view (deliberately unlike Channels/Tasks/Calendar, which each + * ship a driveless `?driveId=` twin of their drive-scoped page). + * + * A client redirect rather than the server's `redirect()`: "the drive you were + * last in" is `currentDriveId` in the persisted (localStorage) drive store, so + * the server has nothing to resolve it from. + * + * `currentDriveId` is read ONCE, at first render. DriveSwitcher clears it on + * mount whenever the URL names no drive — which this route, by definition, does + * not — so reading it in an effect would race that clear and lose the very + * answer we came for. + */ +export default function DevelopmentRedirectPage() { + const router = useRouter(); + const drives = useDriveStore((state) => state.drives); + const isLoading = useDriveStore((state) => state.isLoading); + const fetchDrives = useDriveStore((state) => state.fetchDrives); + const [lastVisitedDriveId] = useState(() => useDriveStore.getState().currentDriveId); + + useEffect(() => { + fetchDrives(); + }, [fetchDrives]); + + useEffect(() => { + if (isLoading) return; + const driveId = resolveActiveDriveId(drives, lastVisitedDriveId); + // No drive to develop in — the drive picker is the only useful destination. + router.replace(driveId ? `/dashboard/${driveId}/development` : '/dashboard/drives'); + }, [drives, isLoading, lastVisitedDriveId, router]); + + return ( +
+
+ Opening Development… +
+ ); +} diff --git a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx new file mode 100644 index 0000000000..9c8298b9c5 --- /dev/null +++ b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx @@ -0,0 +1,182 @@ +'use client'; + +import { useCallback, useEffect, useState } from 'react'; +import { useParams, usePathname, useRouter } from 'next/navigation'; + +import { ScrollArea } from '@/components/ui/scroll-area'; +import { cn, isElectron } from '@/lib/utils'; +import type { SidebarProps } from './index'; +import DriveSwitcher from '@/components/layout/navbar/DriveSwitcher'; +import DashboardFooter from './DashboardFooter'; +import DriveFooter from './DriveFooter'; +import PrimaryNavigation from './PrimaryNavigation'; +import { useBreakpoint } from '@/hooks/useBreakpoint'; +import { useLayoutStore } from '@/stores/useLayoutStore'; +import { useDriveStore } from '@/hooks/useDrive'; +import { canManageDrive } from '@/hooks/usePermissions'; +import { useDriveMachines } from '@/hooks/useDriveMachines'; +import { useMachineWorkspaceStore, type OpenTerminalScope } from '@/stores/machine-workspace/useMachineWorkspaceStore'; +import MachineTree, { type MachineTreeNode } from '@/components/layout/middle-content/page-views/machine/workspace/MachineTree'; +import SessionLeaves from '@/components/layout/middle-content/page-views/machine/workspace/SessionLeaves'; + +/** The machine whose detail pane is open, from `/dashboard/{driveId}/development/{machineId}`. */ +function useSelectedMachineId(driveId: string | undefined): string | null { + const pathname = usePathname() ?? ''; + if (!driveId) return null; + const prefix = `/dashboard/${driveId}/development/`; + if (!pathname.startsWith(prefix)) return null; + return pathname.slice(prefix.length).split('/')[0] || null; +} + +/** + * The Development surface's left sidebar: every Machine in the drive, each + * expanding into the SAME `MachineTree` the Machine page's Terminal tab uses, + * with the SAME session leaves hanging off its nodes. The aggregation is the + * only new part — the tree below each machine is the existing component. + * + * It sits above the routed detail pane in the layout, so clicking through + * machines swaps only the pane: the tree keeps its expansion state and its open + * terminal panes survive the navigation. + */ +export default function DevelopmentSidebar({ className }: SidebarProps) { + const params = useParams(); + const [isElectronMac, setIsElectronMac] = useState(false); + const isSheetBreakpoint = useBreakpoint('(max-width: 1023px)'); + + const driveIdParams = params.driveId; + const driveId = Array.isArray(driveIdParams) ? driveIdParams[0] : driveIdParams; + + const drives = useDriveStore((state) => state.drives); + const drive = drives.find((d) => d.id === driveId); + const canManage = canManageDrive(drive); + + const { machines, isLoading, error } = useDriveMachines(driveId ?? null); + const selectedMachineId = useSelectedMachineId(driveId); + + useEffect(() => { + setIsElectronMac(isElectron() && /Mac/.test(navigator.platform)); + }, []); + + return ( + + ); +} + +const MACHINE_NODE: MachineTreeNode = { level: 'machine' }; + +/** + * One machine in the aggregated tree. Selecting the machine row routes to its + * detail pane; the projects/branches below it are the shared `MachineTree`, and + * are NOT selectable — only the machine row addresses a URL, so making the other + * rows "selectable" would hand them a click action that goes nowhere (and would + * cost them their expand-on-label-click affordance). + */ +function MachineTreeSection({ + driveId, + machineId, + title, + selected, +}: { + driveId: string; + machineId: string; + title: string; + selected: boolean; +}) { + const router = useRouter(); + const isSheetBreakpoint = useBreakpoint('(max-width: 1023px)'); + const setLeftSheetOpen = useLayoutStore((state) => state.setLeftSheetOpen); + const ensureWorkspace = useMachineWorkspaceStore((state) => state.ensureWorkspace); + const openTerminal = useMachineWorkspaceStore((state) => state.openTerminal); + + const openMachine = useCallback(() => { + router.push(`/dashboard/${driveId}/development/${machineId}`); + if (isSheetBreakpoint) setLeftSheetOpen(false); + }, [router, driveId, machineId, isSheetBreakpoint, setLeftSheetOpen]); + + const onSelectNode = useCallback(() => openMachine(), [openMachine]); + + const onOpenTerminal = useCallback( + (scope: OpenTerminalScope) => { + // Open the session into the machine's workspace BEFORE routing: the pane + // region hasn't mounted yet if the user is coming from another machine, + // and a transition against a workspace that doesn't exist is a no-op. + // `ensureWorkspace` is idempotent, so the pane region adopts this + // workspace on mount rather than replacing it. + ensureWorkspace(machineId); + openTerminal(machineId, scope); + openMachine(); + }, + [ensureWorkspace, openTerminal, machineId, openMachine], + ); + + const renderNodeChildren = useCallback( + (node: MachineTreeNode) => ( + + ), + [machineId, onOpenTerminal], + ); + + return ( + node.level === 'machine'} + selectedNode={selected ? MACHINE_NODE : null} + renderNodeChildren={renderNodeChildren} + /> + ); +} diff --git a/apps/web/src/components/layout/left-sidebar/MemoizedSidebar.tsx b/apps/web/src/components/layout/left-sidebar/MemoizedSidebar.tsx index 1bb84238ad..2243f7e351 100644 --- a/apps/web/src/components/layout/left-sidebar/MemoizedSidebar.tsx +++ b/apps/web/src/components/layout/left-sidebar/MemoizedSidebar.tsx @@ -5,28 +5,30 @@ import { usePathname } from 'next/navigation'; import Sidebar, { type SidebarProps } from './index'; import DMSidebar from './DMSidebar'; import ChannelsSidebar from './ChannelsSidebar'; - -const DMS_PATH = /^\/dashboard\/dms(\/|$)/; -const CHANNELS_PATH = /^\/dashboard\/channels(\/|$)/; -const DRIVE_CHANNELS_PATH = /^\/dashboard\/[^/]+\/channels(\/|$)/; +import DevelopmentSidebar from './DevelopmentSidebar'; +import { resolveSidebarVariant } from './sidebar-routes'; /** * Memoized version of Sidebar to prevent unnecessary re-renders. * Each top-level nav item gets its own sidebar feed so the list is * always visible (mirrors PageTree always-on behavior in drive view). + * + * Which feed a pathname gets is decided by `resolveSidebarVariant` — this + * component only maps that answer to a component. */ const MemoizedSidebar = memo((props: SidebarProps) => { const pathname = usePathname() ?? ''; - if (DMS_PATH.test(pathname)) { - return ; - } - - if (CHANNELS_PATH.test(pathname) || DRIVE_CHANNELS_PATH.test(pathname)) { - return ; + switch (resolveSidebarVariant(pathname)) { + case 'dms': + return ; + case 'channels': + return ; + case 'development': + return ; + case 'default': + return ; } - - return ; }); MemoizedSidebar.displayName = 'MemoizedSidebar'; diff --git a/apps/web/src/components/layout/left-sidebar/PrimaryNavigation.tsx b/apps/web/src/components/layout/left-sidebar/PrimaryNavigation.tsx index 0a55cbbd1f..7a6162b9d1 100644 --- a/apps/web/src/components/layout/left-sidebar/PrimaryNavigation.tsx +++ b/apps/web/src/components/layout/left-sidebar/PrimaryNavigation.tsx @@ -2,7 +2,7 @@ import Link from "next/link"; import { usePathname } from "next/navigation"; -import { Calendar, CheckSquare, Folder, Hash, Home, MessageSquare } from "lucide-react"; +import { Calendar, CheckSquare, Folder, Hash, Home, MessageSquare, SquareTerminal } from "lucide-react"; import { cn } from "@/lib/utils"; import { useLayoutStore } from "@/stores/useLayoutStore"; @@ -63,6 +63,15 @@ export default function PrimaryNavigation({ driveId }: PrimaryNavigationProps) { exact: false, badge: badges.calendar, }, + // Driveless href hits a redirect, not a second implementation of the + // surface — the drive always ends up in the path. + { + name: "Development", + href: driveId ? `/dashboard/${driveId}/development` : "/dashboard/development", + icon: SquareTerminal, + exact: false, + badge: 0, + }, ]; const handleLinkClick = () => { diff --git a/apps/web/src/components/layout/left-sidebar/__tests__/sidebar-routes.test.ts b/apps/web/src/components/layout/left-sidebar/__tests__/sidebar-routes.test.ts new file mode 100644 index 0000000000..3bce5ad353 --- /dev/null +++ b/apps/web/src/components/layout/left-sidebar/__tests__/sidebar-routes.test.ts @@ -0,0 +1,35 @@ +import { describe, test, expect } from 'vitest'; +import { resolveSidebarVariant } from '../sidebar-routes'; + +describe('resolveSidebarVariant', () => { + test('routes the drive-scoped Development tree to the Development sidebar', () => { + expect(resolveSidebarVariant('/dashboard/drive-1/development')).toBe('development'); + }); + + test('keeps the Development sidebar when a machine is selected', () => { + // The whole point of the sidebar living above the routed page: picking a + // machine swaps the detail pane, not the sidebar. + expect(resolveSidebarVariant('/dashboard/drive-1/development/machine-1')).toBe('development'); + }); + + test('the driveless Development entry resolves from the same matcher', () => { + expect(resolveSidebarVariant('/dashboard/development')).toBe('development'); + }); + + test("does not swallow a drive's ordinary page route", () => { + expect(resolveSidebarVariant('/dashboard/drive-1/page-1')).toBe('default'); + }); + + test('does not match a path that merely starts with the segment', () => { + expect(resolveSidebarVariant('/dashboard/drive-1/development-notes')).toBe('default'); + expect(resolveSidebarVariant('/dashboard/developments')).toBe('default'); + }); + + test('leaves the existing swaps alone', () => { + expect(resolveSidebarVariant('/dashboard/dms')).toBe('dms'); + expect(resolveSidebarVariant('/dashboard/dms/thread-1')).toBe('dms'); + expect(resolveSidebarVariant('/dashboard/channels')).toBe('channels'); + expect(resolveSidebarVariant('/dashboard/drive-1/channels')).toBe('channels'); + expect(resolveSidebarVariant('/dashboard')).toBe('default'); + }); +}); diff --git a/apps/web/src/components/layout/left-sidebar/sidebar-routes.ts b/apps/web/src/components/layout/left-sidebar/sidebar-routes.ts new file mode 100644 index 0000000000..e23beb3235 --- /dev/null +++ b/apps/web/src/components/layout/left-sidebar/sidebar-routes.ts @@ -0,0 +1,32 @@ +/** + * Which sidebar a pathname gets. Each top-level nav destination that swaps the + * left sidebar owns a matcher here; everything else falls through to the drive's + * page tree. + * + * A pure function rather than inline `if`s in `MemoizedSidebar` so the matchers + * — the part with the actual edge cases — are testable without rendering a + * sidebar. + */ + +const DMS_PATH = /^\/dashboard\/dms(\/|$)/; +const CHANNELS_PATH = /^\/dashboard\/channels(\/|$)/; +const DRIVE_CHANNELS_PATH = /^\/dashboard\/[^/]+\/channels(\/|$)/; +/** + * ONE matcher for both Development shapes: the driveless entry + * (`/dashboard/development`, which redirects) and the real drive-scoped tree + * (`/dashboard/{driveId}/development[/{machineId}]`). The surface keeps the + * drive in the path, so unlike Channels it needs no driveless twin. + * + * The optional drive segment is why this is anchored and segment-bounded: a + * drive's ordinary page route (`/dashboard/{driveId}/{pageId}`) must not match. + */ +const DEVELOPMENT_PATH = /^\/dashboard\/(?:[^/]+\/)?development(\/|$)/; + +export type SidebarVariant = 'dms' | 'channels' | 'development' | 'default'; + +export function resolveSidebarVariant(pathname: string): SidebarVariant { + if (DMS_PATH.test(pathname)) return 'dms'; + if (CHANNELS_PATH.test(pathname) || DRIVE_CHANNELS_PATH.test(pathname)) return 'channels'; + if (DEVELOPMENT_PATH.test(pathname)) return 'development'; + return 'default'; +} diff --git a/apps/web/src/lib/development/__tests__/resolve-active-drive.test.ts b/apps/web/src/lib/development/__tests__/resolve-active-drive.test.ts new file mode 100644 index 0000000000..20b9f6145b --- /dev/null +++ b/apps/web/src/lib/development/__tests__/resolve-active-drive.test.ts @@ -0,0 +1,53 @@ +import { describe, test, expect } from 'vitest'; +import type { Drive } from '@pagespace/lib/types'; +import { resolveActiveDriveId } from '../resolve-active-drive'; + +const drive = (id: string, overrides: Partial = {}): Drive => ({ + id, + name: id, + slug: id, + ownerId: 'user-1', + isTrashed: false, + trashedAt: null, + createdAt: '2026-07-11T00:00:00.000Z', + updatedAt: '2026-07-11T00:00:00.000Z', + isOwned: true, + ...overrides, +}); + +describe('resolveActiveDriveId', () => { + test('prefers the drive the user was last in', () => { + const drives = [drive('drive-1'), drive('drive-2')]; + + expect(resolveActiveDriveId(drives, 'drive-2')).toBe('drive-2'); + }); + + test('falls back to the first drive when there is no last-visited one', () => { + const drives = [drive('drive-1'), drive('drive-2')]; + + expect(resolveActiveDriveId(drives, null)).toBe('drive-1'); + }); + + test('falls back to the first drive when the last-visited one is gone', () => { + const drives = [drive('drive-1')]; + + expect(resolveActiveDriveId(drives, 'drive-deleted')).toBe('drive-1'); + }); + + test('never forwards into a trashed drive, even the last-visited one', () => { + const drives = [drive('drive-trashed', { isTrashed: true }), drive('drive-2')]; + + expect(resolveActiveDriveId(drives, 'drive-trashed')).toBe('drive-2'); + }); + + test('skips trashed drives when falling back', () => { + const drives = [drive('drive-trashed', { isTrashed: true }), drive('drive-2')]; + + expect(resolveActiveDriveId(drives, null)).toBe('drive-2'); + }); + + test('resolves to null when there is no drive to go to', () => { + expect(resolveActiveDriveId([], null)).toBeNull(); + expect(resolveActiveDriveId([drive('drive-trashed', { isTrashed: true })], 'drive-trashed')).toBeNull(); + }); +}); diff --git a/apps/web/src/lib/development/resolve-active-drive.ts b/apps/web/src/lib/development/resolve-active-drive.ts new file mode 100644 index 0000000000..8fe3619ddf --- /dev/null +++ b/apps/web/src/lib/development/resolve-active-drive.ts @@ -0,0 +1,24 @@ +import type { Drive } from '@pagespace/lib/types'; + +/** + * Which drive the driveless `/dashboard/development` entry should forward to. + * + * The Development surface keeps the drive in the path (one route tree), so its + * driveless entry is a redirect rather than a second implementation. This is the + * decision that redirect makes, extracted as a pure function so it's testable + * without a router. + * + * The preference order is the app's existing one — the same + * `find(currentDriveId) ?? first` fallback the backups settings page already + * uses to pick a drive when the URL doesn't name one. `currentDriveId` is the + * drive store's persisted "drive you were last in"; a trashed drive is never a + * redirect target, including when it IS the last-visited one. + * + * Returns null when the user has no drive to go to — the caller sends them to + * the drive picker instead. + */ +export function resolveActiveDriveId(drives: Drive[], currentDriveId: string | null): string | null { + const candidates = drives.filter((drive) => !drive.isTrashed); + const preferred = candidates.find((drive) => drive.id === currentDriveId); + return (preferred ?? candidates[0])?.id ?? null; +} From 4a1625bbb5df249bc409fc1121506d83256f30a3 Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 10:53:11 -0500 Subject: [PATCH 04/18] fix(development): admin-gate the surface; wait for drives before redirecting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both Codex review threads and the CI audit-coverage gate. P1 — the surface exposed machine structure to non-admins. MachineView refuses to mount its tabs for a non-admin, but the new sidebar happily rendered the tree for any drive member who could VIEW a Machine page, fetching its projects, branches, and terminal sessions from the view-level APIs. Gated in three places: the list route is now app-admin only (and audits the denial), the sidebar passes a null driveId for non-admins so the requests are never made, and the nav entry is hidden rather than pointing at a destination that refuses them. P2 — the driveless redirect raced its own fetch. With a cold store, isLoading is still false on the first render and drives is still [], so anyone with an empty or expired cache was redirected to the drive picker despite having drives. The redirect now waits for fetchDrives() to settle. CI: both failing checks traced to one cause — the new /api/machines route had no security-audit coverage. It now emits an authz.access.denied audit on the non-admin path, so it satisfies the gate with real coverage rather than an allowlist exemption. Also collapsed the sidebar's five stacked && guards into a MachineList with early returns. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../app/api/machines/__tests__/route.test.ts | 37 ++++++- apps/web/src/app/api/machines/route.ts | 28 ++++- .../src/app/dashboard/development/page.tsx | 20 +++- .../left-sidebar/DevelopmentSidebar.tsx | 101 ++++++++++++------ .../layout/left-sidebar/PrimaryNavigation.tsx | 22 ++-- 5 files changed, 158 insertions(+), 50 deletions(-) diff --git a/apps/web/src/app/api/machines/__tests__/route.test.ts b/apps/web/src/app/api/machines/__tests__/route.test.ts index 1226991dcf..769824bf4c 100644 --- a/apps/web/src/app/api/machines/__tests__/route.test.ts +++ b/apps/web/src/app/api/machines/__tests__/route.test.ts @@ -4,10 +4,11 @@ */ import { describe, it, expect, vi, beforeEach } from 'vitest'; -const { mockAuthenticateRequest, mockIsAuthError, mockListDriveMachines } = vi.hoisted(() => ({ +const { mockAuthenticateRequest, mockIsAuthError, mockListDriveMachines, mockAuditRequest } = vi.hoisted(() => ({ mockAuthenticateRequest: vi.fn(), mockIsAuthError: vi.fn((result: unknown) => result != null && typeof result === 'object' && 'error' in result), mockListDriveMachines: vi.fn(), + mockAuditRequest: vi.fn(), })); vi.mock('@/lib/auth', () => ({ @@ -15,25 +16,53 @@ vi.mock('@/lib/auth', () => ({ isAuthError: (result: unknown) => mockIsAuthError(result), })); +vi.mock('@pagespace/lib/audit/audit-log', () => ({ + auditRequest: (...args: unknown[]) => mockAuditRequest(...args), +})); + vi.mock('@/lib/machines/machine-list-runtime', () => ({ listDriveMachines: (...args: unknown[]) => mockListDriveMachines(...args), })); import { GET } from '../route'; -const AUTH_OK = { userId: 'user-1' }; +const AUTH_ADMIN = { userId: 'user-1', role: 'admin' }; +const AUTH_NON_ADMIN = { userId: 'user-2', role: 'user' }; const AUTH_DENIED = { error: new Response(null, { status: 401 }) }; const MACHINE = { id: 'machine-1', title: 'Dev box', updatedAt: '2026-07-11T00:00:00.000Z' }; beforeEach(() => { vi.clearAllMocks(); - mockAuthenticateRequest.mockResolvedValue(AUTH_OK); + mockAuthenticateRequest.mockResolvedValue(AUTH_ADMIN); mockListDriveMachines.mockResolvedValue([MACHINE]); }); describe('GET /api/machines', () => { - it('returns the drive\'s machines for the authenticated user', async () => { + it('refuses a non-admin, and never enumerates the drive for them', async () => { + // Machines are an app-admin feature: a non-admin who can merely VIEW a + // Machine page must not be able to enumerate the drive's machines (and, + // through the tree, their projects/branches/sessions). + mockAuthenticateRequest.mockResolvedValue(AUTH_NON_ADMIN); + + const response = await GET(new Request('http://localhost/api/machines?driveId=drive-1')); + + expect(response.status).toBe(403); + expect(mockListDriveMachines).not.toHaveBeenCalled(); + }); + + it('audits the non-admin denial', async () => { + mockAuthenticateRequest.mockResolvedValue(AUTH_NON_ADMIN); + + await GET(new Request('http://localhost/api/machines?driveId=drive-1')); + + expect(mockAuditRequest).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ eventType: 'authz.access.denied', userId: 'user-2' }), + ); + }); + + it('returns the drive\'s machines for an admin', async () => { const response = await GET(new Request('http://localhost/api/machines?driveId=drive-1')); expect(response.status).toBe(200); diff --git a/apps/web/src/app/api/machines/route.ts b/apps/web/src/app/api/machines/route.ts index f21a6cdf4f..57be6ea7c6 100644 --- a/apps/web/src/app/api/machines/route.ts +++ b/apps/web/src/app/api/machines/route.ts @@ -6,13 +6,23 @@ * GET ?driveId= → { machines: [{ id, title, updatedAt }] } * * Session-only (no MCP/agent tokens) — a human/UI surface, like the rest of - * `/api/machines/*`. The list is filtered to the machines the caller may view, - * so a drive member who has been withheld an individual Machine page never sees - * it in the tree. + * `/api/machines/*`. + * + * App-admin only, matching the rest of the Machine feature: creating a MACHINE + * page requires `admin` (see POST /api/pages) and `MachineView` refuses to mount + * its tabs for anyone else. Without this, a non-admin drive member who can VIEW a + * Machine page could enumerate the drive's machines from the Development surface + * and, through the tree, their projects/branches/terminal sessions — structure + * the Machine page deliberately withholds from them. + * + * Admin is necessary but not sufficient: the list is still filtered per page + * through `canUserViewPage`, so a Machine withheld from this admin by a + * page-level grant never appears. */ import { NextResponse } from 'next/server'; import { authenticateRequestWithOptions, isAuthError } from '@/lib/auth'; +import { auditRequest } from '@pagespace/lib/audit/audit-log'; import { listDriveMachines } from '@/lib/machines/machine-list-runtime'; const AUTH_OPTIONS_READ = { allow: ['session'] as const, requireCSRF: false }; @@ -26,6 +36,18 @@ export async function GET(request: Request) { return NextResponse.json({ error: 'driveId is required' }, { status: 400 }); } + if (auth.role !== 'admin') { + auditRequest(request, { + eventType: 'authz.access.denied', + userId: auth.userId, + resourceType: 'drive', + resourceId: driveId, + details: { reason: 'app_admin_required', method: 'GET', route: 'machines' }, + riskScore: 0.5, + }); + return NextResponse.json({ error: 'Machines require administrator privileges' }, { status: 403 }); + } + const machines = await listDriveMachines(auth.userId, driveId); return NextResponse.json({ machines }); } diff --git a/apps/web/src/app/dashboard/development/page.tsx b/apps/web/src/app/dashboard/development/page.tsx index 1b418e0756..6df351965c 100644 --- a/apps/web/src/app/dashboard/development/page.tsx +++ b/apps/web/src/app/dashboard/development/page.tsx @@ -24,20 +24,32 @@ import { resolveActiveDriveId } from '@/lib/development/resolve-active-drive'; export default function DevelopmentRedirectPage() { const router = useRouter(); const drives = useDriveStore((state) => state.drives); - const isLoading = useDriveStore((state) => state.isLoading); const fetchDrives = useDriveStore((state) => state.fetchDrives); const [lastVisitedDriveId] = useState(() => useDriveStore.getState().currentDriveId); + const [drivesSettled, setDrivesSettled] = useState(false); + // Gate the redirect on the fetch SETTLING, not on `isLoading` being false: + // with a cold store, `isLoading` is still false on the first render (the fetch + // hasn't started), and `drives` is still [] — so redirecting on that render + // would send anyone with an empty or expired cache to the drive picker even + // though they have drives. `fetchDrives` no-ops on a warm cache, so this costs + // a fresh session nothing. useEffect(() => { - fetchDrives(); + let cancelled = false; + void fetchDrives().finally(() => { + if (!cancelled) setDrivesSettled(true); + }); + return () => { + cancelled = true; + }; }, [fetchDrives]); useEffect(() => { - if (isLoading) return; + if (!drivesSettled) return; const driveId = resolveActiveDriveId(drives, lastVisitedDriveId); // No drive to develop in — the drive picker is the only useful destination. router.replace(driveId ? `/dashboard/${driveId}/development` : '/dashboard/drives'); - }, [drives, isLoading, lastVisitedDriveId, router]); + }, [drives, drivesSettled, lastVisitedDriveId, router]); return (
diff --git a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx index 9c8298b9c5..30d69f40ec 100644 --- a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx +++ b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx @@ -10,11 +10,12 @@ import DriveSwitcher from '@/components/layout/navbar/DriveSwitcher'; import DashboardFooter from './DashboardFooter'; import DriveFooter from './DriveFooter'; import PrimaryNavigation from './PrimaryNavigation'; +import { useAuth } from '@/hooks/useAuth'; import { useBreakpoint } from '@/hooks/useBreakpoint'; import { useLayoutStore } from '@/stores/useLayoutStore'; import { useDriveStore } from '@/hooks/useDrive'; import { canManageDrive } from '@/hooks/usePermissions'; -import { useDriveMachines } from '@/hooks/useDriveMachines'; +import { useDriveMachines, type DriveMachine } from '@/hooks/useDriveMachines'; import { useMachineWorkspaceStore, type OpenTerminalScope } from '@/stores/machine-workspace/useMachineWorkspaceStore'; import MachineTree, { type MachineTreeNode } from '@/components/layout/middle-content/page-views/machine/workspace/MachineTree'; import SessionLeaves from '@/components/layout/middle-content/page-views/machine/workspace/SessionLeaves'; @@ -50,7 +51,15 @@ export default function DevelopmentSidebar({ className }: SidebarProps) { const drive = drives.find((d) => d.id === driveId); const canManage = canManageDrive(drive); - const { machines, isLoading, error } = useDriveMachines(driveId ?? null); + const { user } = useAuth(); + const isAdmin = user?.role === 'admin'; + + // The same gate MachineView applies, moved one level earlier: a non-admin who + // can VIEW a Machine page must not be able to enumerate the drive's machines — + // nor have this tree fetch their projects/branches/sessions on their behalf, + // which is structure the Machine page itself withholds from them. Passing a + // null driveId is what keeps those requests from ever being made. + const { machines, isLoading, error } = useDriveMachines(isAdmin ? driveId ?? null : null); const selectedMachineId = useSelectedMachineId(driveId); useEffect(() => { @@ -73,36 +82,14 @@ export default function DevelopmentSidebar({ className }: SidebarProps) {
- {/* The driveless entry redirects, so a missing driveId here means the - redirect hasn't landed yet — not a state the user can sit in. */} - {!driveId && ( -
Opening Development…
- )} - - {driveId && error && ( -
Failed to load machines
- )} - - {driveId && !error && isLoading && ( -
Loading…
- )} - - {driveId && !error && !isLoading && machines.length === 0 && ( -
- No machines in this drive yet -
- )} - - {driveId && - machines.map((machine) => ( - - ))} +
@@ -112,6 +99,56 @@ export default function DevelopmentSidebar({ className }: SidebarProps) { ); } +/** A resting state of the machine list: one line of muted text, no tree. */ +function ListNotice({ children }: { children: string }) { + return
{children}
; +} + +/** + * The list body. Its states are mutually exclusive, so they're early returns + * rather than a stack of `&&` guards each having to re-state every earlier + * condition's negation. + */ +function MachineList({ + isAdmin, + driveId, + machines, + isLoading, + error, + selectedMachineId, +}: { + isAdmin: boolean; + driveId: string | undefined; + machines: DriveMachine[]; + isLoading: boolean; + error: Error | undefined; + selectedMachineId: string | null; +}) { + // Same wording MachineView uses, so the surface and the page refuse a + // non-admin identically. + if (!isAdmin) return Machine access requires administrator privileges; + // The driveless entry redirects, so a missing driveId is the redirect in + // flight — not a state the user can sit in. + if (!driveId) return Opening Development…; + if (error) return Failed to load machines; + if (isLoading) return Loading…; + if (machines.length === 0) return No machines in this drive yet; + + return ( + <> + {machines.map((machine) => ( + + ))} + + ); +} + const MACHINE_NODE: MachineTreeNode = { level: 'machine' }; /** diff --git a/apps/web/src/components/layout/left-sidebar/PrimaryNavigation.tsx b/apps/web/src/components/layout/left-sidebar/PrimaryNavigation.tsx index 7a6162b9d1..0e1483a3f8 100644 --- a/apps/web/src/components/layout/left-sidebar/PrimaryNavigation.tsx +++ b/apps/web/src/components/layout/left-sidebar/PrimaryNavigation.tsx @@ -5,6 +5,7 @@ import { usePathname } from "next/navigation"; import { Calendar, CheckSquare, Folder, Hash, Home, MessageSquare, SquareTerminal } from "lucide-react"; import { cn } from "@/lib/utils"; +import { useAuth } from "@/hooks/useAuth"; import { useLayoutStore } from "@/stores/useLayoutStore"; import { useBreakpoint } from "@/hooks/useBreakpoint"; import { useSidebarBadges } from "@/hooks/useSidebarBadges"; @@ -18,6 +19,11 @@ export default function PrimaryNavigation({ driveId }: PrimaryNavigationProps) { const isSheetBreakpoint = useBreakpoint("(max-width: 1023px)"); const setLeftSheetOpen = useLayoutStore((state) => state.setLeftSheetOpen); const badges = useSidebarBadges(); + const { user } = useAuth(); + // Machines are an app-admin feature end to end (only an admin can create a + // MACHINE page, and MachineView mounts no tabs for anyone else), so a + // non-admin gets no nav entry rather than a destination that refuses them. + const isAdmin = user?.role === "admin"; const navigation = [ { @@ -65,13 +71,15 @@ export default function PrimaryNavigation({ driveId }: PrimaryNavigationProps) { }, // Driveless href hits a redirect, not a second implementation of the // surface — the drive always ends up in the path. - { - name: "Development", - href: driveId ? `/dashboard/${driveId}/development` : "/dashboard/development", - icon: SquareTerminal, - exact: false, - badge: 0, - }, + ...(isAdmin + ? [{ + name: "Development", + href: driveId ? `/dashboard/${driveId}/development` : "/dashboard/development", + icon: SquareTerminal, + exact: false, + badge: 0, + }] + : []), ]; const handleLinkClick = () => { From 9a8014d8344b2e101d64c022107b5877232379fb Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 10:56:04 -0500 Subject: [PATCH 05/18] refactor(development): drop a redundant callback; document the tab-focus gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit onSelectNode wrapped openMachine only to discard the node argument it never used — pass openMachine directly. Hoist the isNodeSelectable predicate out of render. Also documents a real edge the sidebar cannot close on its own: opening a session on the machine you are ALREADY viewing lands the pane but cannot focus the Terminal tab, because MachineView's tabs are uncontrolled. The session is still opened; focusing needs MachineView's active tab to become controlled, which belongs with the follow-up rather than colliding with #2017. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../layout/left-sidebar/DevelopmentSidebar.tsx | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx index 30d69f40ec..31a09305ec 100644 --- a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx +++ b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx @@ -151,6 +151,9 @@ function MachineList({ const MACHINE_NODE: MachineTreeNode = { level: 'machine' }; +/** Only the machine row addresses a URL, so only it is selectable. */ +const isMachineNode = (node: MachineTreeNode) => node.level === 'machine'; + /** * One machine in the aggregated tree. Selecting the machine row routes to its * detail pane; the projects/branches below it are the shared `MachineTree`, and @@ -180,8 +183,6 @@ function MachineTreeSection({ if (isSheetBreakpoint) setLeftSheetOpen(false); }, [router, driveId, machineId, isSheetBreakpoint, setLeftSheetOpen]); - const onSelectNode = useCallback(() => openMachine(), [openMachine]); - const onOpenTerminal = useCallback( (scope: OpenTerminalScope) => { // Open the session into the machine's workspace BEFORE routing: the pane @@ -192,6 +193,13 @@ function MachineTreeSection({ ensureWorkspace(machineId); openTerminal(machineId, scope); openMachine(); + // KNOWN GAP: if the user is already on THIS machine with a non-Terminal tab + // active, the pane is created but stays behind that tab — MachineView's tabs + // are uncontrolled (defaultValue="terminal"), so nothing here can focus them. + // Landing the session is still correct (it's there when they return to the + // Terminal tab); making the click also switch tabs needs MachineView's active + // tab to become controlled, which is deliberately left to the follow-up rather + // than fought over with the in-flight terminal-UX work (#2017). }, [ensureWorkspace, openTerminal, machineId, openMachine], ); @@ -210,8 +218,10 @@ function MachineTreeSection({ // N machines on screen: collapsed until asked for, so mounting the surface // doesn't fire a project fetch per machine. defaultExpanded={false} - onSelectNode={onSelectNode} - isNodeSelectable={(node) => node.level === 'machine'} + // MachineTree passes the clicked node; only the machine row is selectable + // here, so the node adds nothing the closure doesn't already know. + onSelectNode={openMachine} + isNodeSelectable={isMachineNode} selectedNode={selected ? MACHINE_NODE : null} renderNodeChildren={renderNodeChildren} /> From d9d8101d1de8cbbceb84278e4dba4073bdb2e511 Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 11:26:41 -0500 Subject: [PATCH 06/18] fix(development): keep terminals alive across machine switches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review caught a defect that undercut the surface's whole purpose: the detail route rendered MachineView inline, but the [machineId] route segment REMOUNTS on every machine-to-machine navigation. MachineWorkspace disposes its workspace on unmount and XtermTerminal tears down its socket, so clicking another machine killed the terminals you left running — on the surface built to keep them. The drive view already solved this: CenterPanel deliberately renders nothing for MACHINE pages and defers to MachineKeepAliveHost (bounded LRU, CSS-hidden when inactive). So the surface now does the same. A new layout above the [machineId] segment renders MachineKeepAliveHost; the detail route renders null (mounting MachineView there too would create a second, competing terminal subtree, exactly as CenterPanel's comment warns). That also fixes how a sidebar session-click lands. It used to author the pane into the workspace store BEFORE the target machine mounted — which cannot survive, since MachineWorkspace rebuilds the workspace on mount and destroys anything written ahead of it (StrictMode's double-invoke makes this bite on the first visit; a remount would do it in prod). The click now records an intent that the layout drains once the machine has a workspace, re-applying if the workspace is rebuilt underneath it and clearing once the session is actually in the active pane — so a stale intent can never clobber the user's later pane changes. The decision is a pure function (resolvePendingSession) with 9 tests covering the rebuild, the clobber, and the navigated-away cases. The shared machine-workspace store is untouched (its synchronous dispose is a tested contract, and #2017 is reworking it). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../development/[machineId]/page.tsx | 28 +++---- .../[driveId]/development/layout.tsx | 68 ++++++++++++++++ .../left-sidebar/DevelopmentSidebar.tsx | 54 ++++++------- .../__tests__/development-route.test.ts | 25 ++++++ .../__tests__/pending-session.test.ts | 78 +++++++++++++++++++ .../src/lib/development/development-route.ts | 13 ++++ .../src/lib/development/pending-session.ts | 69 ++++++++++++++++ .../development/usePendingSessionStore.ts | 27 +++++++ 8 files changed, 316 insertions(+), 46 deletions(-) create mode 100644 apps/web/src/app/dashboard/[driveId]/development/layout.tsx create mode 100644 apps/web/src/lib/development/__tests__/development-route.test.ts create mode 100644 apps/web/src/lib/development/__tests__/pending-session.test.ts create mode 100644 apps/web/src/lib/development/development-route.ts create mode 100644 apps/web/src/lib/development/pending-session.ts create mode 100644 apps/web/src/stores/development/usePendingSessionStore.ts diff --git a/apps/web/src/app/dashboard/[driveId]/development/[machineId]/page.tsx b/apps/web/src/app/dashboard/[driveId]/development/[machineId]/page.tsx index 0233d41dc4..bf56bb3b3b 100644 --- a/apps/web/src/app/dashboard/[driveId]/development/[machineId]/page.tsx +++ b/apps/web/src/app/dashboard/[driveId]/development/[machineId]/page.tsx @@ -1,20 +1,16 @@ -import MachineView from '@/components/layout/middle-content/page-views/machine/MachineView'; - /** - * The Development surface's detail pane: the Machine page itself, reused - * verbatim. A Machine's id IS its page id, so the route's `machineId` is - * `MachineView`'s `pageId`. + * The Development surface's detail route. * - * Only this segment re-renders as the user moves between machines — - * `MemoizedSidebar` sits above the routed page, so the aggregated tree keeps its - * expansion state (and its open terminal panes) across the navigation. + * Renders NOTHING on purpose. The machine is drawn by `MachineKeepAliveHost` in + * this segment's layout, which keeps recently-visited machines mounted across + * navigation (CSS-hiding the inactive ones) so their terminals survive. Mounting + * a `MachineView` here as well would create a second, competing terminal subtree + * for the same machine — the same reason `CenterPanel` renders nothing for + * MACHINE pages in the drive view. + * + * The route still exists to make a machine bookmarkable: the URL is what the + * layout reads to decide which machine is active. */ -export default async function DevelopmentMachinePage({ - params, -}: { - params: Promise<{ driveId: string; machineId: string }>; -}) { - const { machineId } = await params; - - return ; +export default function DevelopmentMachinePage() { + return null; } diff --git a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx new file mode 100644 index 0000000000..bdcadb65f6 --- /dev/null +++ b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx @@ -0,0 +1,68 @@ +'use client'; + +import { useEffect } from 'react'; +import { useParams, usePathname } from 'next/navigation'; +import MachineKeepAliveHost from '@/components/layout/middle-content/MachineKeepAliveHost'; +import { useMachineWorkspaceStore } from '@/stores/machine-workspace/useMachineWorkspaceStore'; +import { usePendingSessionStore } from '@/stores/development/usePendingSessionStore'; +import { parseSelectedMachineId } from '@/lib/development/development-route'; +import { resolvePendingSession } from '@/lib/development/pending-session'; + +/** + * The Development surface's detail region. + * + * Machines are rendered by {@link MachineKeepAliveHost}, NOT by the + * `[machineId]` route — exactly as the drive view does it (see `CenterPanel`, + * which likewise refuses to render `MachineView` inline). The route segment + * remounts on every machine-to-machine navigation, so a `MachineView` rendered + * from it would tear its xterm buffer, its socket, and its workspace down each + * time you clicked another machine — on the one surface whose entire purpose is + * keeping terminals alive. The host instead keeps a bounded LRU of machines + * mounted and CSS-hides the inactive ones, so switching machines is instant and + * the sessions you left running are still running. + * + * This layout sits ABOVE the `[machineId]` segment, so it (and the host, and + * every warm machine) survives that navigation. + */ +export default function DevelopmentLayout({ children }: { children: React.ReactNode }) { + const params = useParams(); + const pathname = usePathname() ?? ''; + const driveIdParams = params.driveId; + const driveId = Array.isArray(driveIdParams) ? driveIdParams[0] : driveIdParams; + const selectedMachineId = parseSelectedMachineId(pathname, driveId); + + useDrainPendingSession(selectedMachineId); + + return ( +
+ {children} + +
+ ); +} + +/** + * Honours a session the user clicked in the sidebar, once the machine it belongs + * to actually has a workspace to open it into. + * + * The decision is the pure `resolvePendingSession`; this is the plumbing. It + * re-evaluates whenever the workspace changes, so an intent applied against a + * workspace that is then torn down and rebuilt (a remount — StrictMode's + * double-invoke does this on first mount) re-applies to the new one instead of + * being silently lost. + */ +function useDrainPendingSession(selectedMachineId: string | null) { + const pending = usePendingSessionStore((state) => state.pending); + const clearPending = usePendingSessionStore((state) => state.clearPending); + const openTerminal = useMachineWorkspaceStore((state) => state.openTerminal); + const workspace = useMachineWorkspaceStore((state) => + pending ? state.workspaces[pending.machineId] : undefined, + ); + + useEffect(() => { + const action = resolvePendingSession(pending, selectedMachineId, workspace); + if (action.type === 'open') openTerminal(action.machineId, action.scope); + // 'clear' with no pending intent is a no-op, so this cannot loop. + else if (action.type === 'clear') clearPending(); + }, [pending, selectedMachineId, workspace, openTerminal, clearPending]); +} diff --git a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx index 31a09305ec..474d519e5a 100644 --- a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx +++ b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx @@ -16,28 +16,24 @@ import { useLayoutStore } from '@/stores/useLayoutStore'; import { useDriveStore } from '@/hooks/useDrive'; import { canManageDrive } from '@/hooks/usePermissions'; import { useDriveMachines, type DriveMachine } from '@/hooks/useDriveMachines'; -import { useMachineWorkspaceStore, type OpenTerminalScope } from '@/stores/machine-workspace/useMachineWorkspaceStore'; +import { usePendingSessionStore } from '@/stores/development/usePendingSessionStore'; +import { parseSelectedMachineId } from '@/lib/development/development-route'; +import type { OpenTerminalScope } from '@/stores/machine-workspace/useMachineWorkspaceStore'; import MachineTree, { type MachineTreeNode } from '@/components/layout/middle-content/page-views/machine/workspace/MachineTree'; import SessionLeaves from '@/components/layout/middle-content/page-views/machine/workspace/SessionLeaves'; -/** The machine whose detail pane is open, from `/dashboard/{driveId}/development/{machineId}`. */ -function useSelectedMachineId(driveId: string | undefined): string | null { - const pathname = usePathname() ?? ''; - if (!driveId) return null; - const prefix = `/dashboard/${driveId}/development/`; - if (!pathname.startsWith(prefix)) return null; - return pathname.slice(prefix.length).split('/')[0] || null; -} - /** * The Development surface's left sidebar: every Machine in the drive, each * expanding into the SAME `MachineTree` the Machine page's Terminal tab uses, * with the SAME session leaves hanging off its nodes. The aggregation is the * only new part — the tree below each machine is the existing component. * - * It sits above the routed detail pane in the layout, so clicking through - * machines swaps only the pane: the tree keeps its expansion state and its open - * terminal panes survive the navigation. + * It sits above the routed detail pane, so clicking through machines swaps only + * the pane and the tree keeps its expansion state. The terminals themselves + * survive because the detail region renders machines through + * `MachineKeepAliveHost` (see this surface's layout) rather than from the route + * segment — which remounts, and would otherwise tear down the xterm buffer and + * socket on every machine switch. */ export default function DevelopmentSidebar({ className }: SidebarProps) { const params = useParams(); @@ -60,7 +56,8 @@ export default function DevelopmentSidebar({ className }: SidebarProps) { // which is structure the Machine page itself withholds from them. Passing a // null driveId is what keeps those requests from ever being made. const { machines, isLoading, error } = useDriveMachines(isAdmin ? driveId ?? null : null); - const selectedMachineId = useSelectedMachineId(driveId); + const pathname = usePathname() ?? ''; + const selectedMachineId = parseSelectedMachineId(pathname, driveId); useEffect(() => { setIsElectronMac(isElectron() && /Mac/.test(navigator.platform)); @@ -175,8 +172,7 @@ function MachineTreeSection({ const router = useRouter(); const isSheetBreakpoint = useBreakpoint('(max-width: 1023px)'); const setLeftSheetOpen = useLayoutStore((state) => state.setLeftSheetOpen); - const ensureWorkspace = useMachineWorkspaceStore((state) => state.ensureWorkspace); - const openTerminal = useMachineWorkspaceStore((state) => state.openTerminal); + const requestSession = usePendingSessionStore((state) => state.requestSession); const openMachine = useCallback(() => { router.push(`/dashboard/${driveId}/development/${machineId}`); @@ -185,23 +181,21 @@ function MachineTreeSection({ const onOpenTerminal = useCallback( (scope: OpenTerminalScope) => { - // Open the session into the machine's workspace BEFORE routing: the pane - // region hasn't mounted yet if the user is coming from another machine, - // and a transition against a workspace that doesn't exist is a no-op. - // `ensureWorkspace` is idempotent, so the pane region adopts this - // workspace on mount rather than replacing it. - ensureWorkspace(machineId); - openTerminal(machineId, scope); + // Record the intent and navigate; the surface's layout opens the session + // once that machine's pane region exists. Writing the pane straight into + // the workspace store from here would not survive — MachineWorkspace + // disposes its workspace on unmount and rebuilds it on mount, destroying + // anything authored ahead of it. + requestSession(machineId, scope); openMachine(); // KNOWN GAP: if the user is already on THIS machine with a non-Terminal tab - // active, the pane is created but stays behind that tab — MachineView's tabs - // are uncontrolled (defaultValue="terminal"), so nothing here can focus them. - // Landing the session is still correct (it's there when they return to the - // Terminal tab); making the click also switch tabs needs MachineView's active - // tab to become controlled, which is deliberately left to the follow-up rather - // than fought over with the in-flight terminal-UX work (#2017). + // active, the session lands in the pane but stays behind that tab — + // MachineView's tabs are uncontrolled (defaultValue="terminal"), so nothing + // here can focus them. Focusing needs MachineView's active tab to become + // controlled, left to the follow-up rather than fought over with the + // in-flight terminal-UX work (#2017). }, - [ensureWorkspace, openTerminal, machineId, openMachine], + [requestSession, machineId, openMachine], ); const renderNodeChildren = useCallback( diff --git a/apps/web/src/lib/development/__tests__/development-route.test.ts b/apps/web/src/lib/development/__tests__/development-route.test.ts new file mode 100644 index 0000000000..a4809794b0 --- /dev/null +++ b/apps/web/src/lib/development/__tests__/development-route.test.ts @@ -0,0 +1,25 @@ +import { describe, test, expect } from 'vitest'; +import { parseSelectedMachineId } from '../development-route'; + +describe('parseSelectedMachineId', () => { + test('reads the machine id out of the detail URL', () => { + expect(parseSelectedMachineId('/dashboard/drive-1/development/machine-1', 'drive-1')).toBe('machine-1'); + }); + + test('no machine is selected at the surface root', () => { + expect(parseSelectedMachineId('/dashboard/drive-1/development', 'drive-1')).toBeNull(); + expect(parseSelectedMachineId('/dashboard/drive-1/development/', 'drive-1')).toBeNull(); + }); + + test('ignores a path belonging to a different drive', () => { + expect(parseSelectedMachineId('/dashboard/drive-2/development/machine-1', 'drive-1')).toBeNull(); + }); + + test('takes only the machine segment, not what follows it', () => { + expect(parseSelectedMachineId('/dashboard/drive-1/development/machine-1/extra', 'drive-1')).toBe('machine-1'); + }); + + test('without a drive there is no machine', () => { + expect(parseSelectedMachineId('/dashboard/development', undefined)).toBeNull(); + }); +}); diff --git a/apps/web/src/lib/development/__tests__/pending-session.test.ts b/apps/web/src/lib/development/__tests__/pending-session.test.ts new file mode 100644 index 0000000000..a67c398fdc --- /dev/null +++ b/apps/web/src/lib/development/__tests__/pending-session.test.ts @@ -0,0 +1,78 @@ +import { describe, test, expect } from 'vitest'; +import type { WorkspaceState } from '@/stores/machine-workspace/useMachineWorkspaceStore'; +import { resolvePendingSession, type PendingSession } from '../pending-session'; + +const SCOPE = { projectName: 'repo', branchName: 'main', name: 'agent-1' }; +const PENDING: PendingSession = { machineId: 'machine-1', scope: SCOPE }; + +/** A workspace whose active pane holds `scope` (null = a fresh, empty pane). */ +const workspaceWith = (scope: WorkspaceState['columns'][number]['panes'][number]['scope']): WorkspaceState => ({ + columns: [{ id: 'col-1', panes: [{ id: 'pane-1', scope }] }], + activePaneId: 'pane-1', +}); + +describe('resolvePendingSession', () => { + test('opens the session once the target machine has a workspace', () => { + expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(null))).toEqual({ + type: 'open', + machineId: 'machine-1', + scope: SCOPE, + }); + }); + + test('holds the intent while the machine has no workspace yet', () => { + // The pane region mounts after the navigation lands; the intent waits for it + // rather than being written into a workspace that does not exist. + expect(resolvePendingSession(PENDING, 'machine-1', undefined)).toEqual({ type: 'wait' }); + }); + + test('re-opens against a workspace that was torn down and rebuilt', () => { + // The bug this guards: MachineWorkspace disposes on unmount and re-creates on + // mount (StrictMode double-invokes this on the first visit). A fire-once + // intent would be destroyed by the rebuild; a convergent one re-applies. + const rebuiltEmpty = workspaceWith(null); + + expect(resolvePendingSession(PENDING, 'machine-1', rebuiltEmpty)).toEqual({ + type: 'open', + machineId: 'machine-1', + scope: SCOPE, + }); + }); + + test('clears once the session is actually in the active pane', () => { + expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(SCOPE))).toEqual({ type: 'clear' }); + }); + + test('a satisfied intent is dropped, so it cannot clobber the user\'s next pane change', () => { + const satisfied = resolvePendingSession(PENDING, 'machine-1', workspaceWith(SCOPE)); + expect(satisfied).toEqual({ type: 'clear' }); + // And with no intent left, nothing is ever re-applied. + expect(resolvePendingSession(null, 'machine-1', workspaceWith({ ...SCOPE, name: 'agent-2' }))).toEqual({ + type: 'clear', + }); + }); + + test('drops the intent when the user navigated to a different machine', () => { + expect(resolvePendingSession(PENDING, 'machine-2', workspaceWith(null))).toEqual({ type: 'clear' }); + }); + + test('drops the intent when the user left the surface entirely', () => { + expect(resolvePendingSession(PENDING, null, workspaceWith(null))).toEqual({ type: 'clear' }); + }); + + test('distinguishes same-named sessions at different scopes', () => { + // A machine-scope "agent-1" is not the branch-scope "agent-1"; treating them + // as the same would report the intent satisfied by the wrong session. + const machineScoped = workspaceWith({ name: 'agent-1' }); + + expect(resolvePendingSession(PENDING, 'machine-1', machineScoped)).toEqual({ + type: 'open', + machineId: 'machine-1', + scope: SCOPE, + }); + }); + + test('no intent is a no-op', () => { + expect(resolvePendingSession(null, 'machine-1', undefined)).toEqual({ type: 'clear' }); + }); +}); diff --git a/apps/web/src/lib/development/development-route.ts b/apps/web/src/lib/development/development-route.ts new file mode 100644 index 0000000000..443b113469 --- /dev/null +++ b/apps/web/src/lib/development/development-route.ts @@ -0,0 +1,13 @@ +/** + * The Development surface's URL shape: `/dashboard/{driveId}/development[/{machineId}]`. + * + * Both the sidebar (to highlight the selected machine) and the surface's layout + * (to tell the keep-alive host which machine is active) need the selected + * machine id, so the parse lives here rather than being written twice. + */ +export function parseSelectedMachineId(pathname: string, driveId: string | undefined): string | null { + if (!driveId) return null; + const prefix = `/dashboard/${driveId}/development/`; + if (!pathname.startsWith(prefix)) return null; + return pathname.slice(prefix.length).split('/')[0] || null; +} diff --git a/apps/web/src/lib/development/pending-session.ts b/apps/web/src/lib/development/pending-session.ts new file mode 100644 index 0000000000..86fce0eca8 --- /dev/null +++ b/apps/web/src/lib/development/pending-session.ts @@ -0,0 +1,69 @@ +import type { OpenTerminalScope, WorkspaceState } from '@/stores/machine-workspace/useMachineWorkspaceStore'; + +/** A session the user clicked in the Development sidebar, to be opened on the machine it belongs to. */ +export interface PendingSession { + machineId: string; + scope: OpenTerminalScope; +} + +export type PendingSessionAction = + /** The machine's pane region isn't there yet — hold the intent. */ + | { type: 'wait' } + /** Apply the intent (idempotent: `openTerminal` sets the active pane's scope). */ + | { type: 'open'; machineId: string; scope: OpenTerminalScope } + /** Done, or moot — drop the intent. */ + | { type: 'clear' }; + +function sameScope(a: OpenTerminalScope | null, b: OpenTerminalScope): boolean { + return ( + a !== null && + a.name === b.name && + (a.projectName ?? null) === (b.projectName ?? null) && + (a.branchName ?? null) === (b.branchName ?? null) + ); +} + +function activePaneScope(workspace: WorkspaceState): OpenTerminalScope | null { + for (const column of workspace.columns) { + for (const pane of column.panes) { + if (pane.id === workspace.activePaneId) return pane.scope; + } + } + return null; +} + +/** + * What to do with a sidebar session-click intent, given where the user now is + * and whether the target machine's workspace exists yet. + * + * A clicked session belongs to a machine whose pane region may not be mounted — + * the user could be on another machine, or on none. Authoring the pane straight + * into the store before that machine mounts does not survive: `MachineWorkspace` + * disposes its workspace on unmount and re-creates it on mount, so a pane + * written ahead of the mount is destroyed by the very component meant to display + * it (React's StrictMode double-invoke makes this bite on the first visit, and a + * remount would do the same in production). + * + * So the intent is HELD instead, and re-evaluated as state arrives. This + * converges rather than fires once: it keeps asking to open until the workspace + * actually reports the session in its active pane, at which point the intent is + * satisfied and dropped. That is what makes it robust to the workspace being + * torn down and rebuilt underneath it — the intent simply re-applies to the new + * workspace. Being idempotent, a repeat `open` is harmless. + * + * Once satisfied, the intent is cleared, so the user's own later pane changes on + * that machine are never clobbered by a stale intent. + */ +export function resolvePendingSession( + pending: PendingSession | null, + selectedMachineId: string | null, + workspace: WorkspaceState | undefined, +): PendingSessionAction { + if (!pending) return { type: 'clear' }; + // The user went somewhere else before the machine ever mounted — the intent is moot. + if (pending.machineId !== selectedMachineId) return { type: 'clear' }; + if (!workspace) return { type: 'wait' }; + // Satisfied: the session is in the active pane. + if (sameScope(activePaneScope(workspace), pending.scope)) return { type: 'clear' }; + return { type: 'open', machineId: pending.machineId, scope: pending.scope }; +} diff --git a/apps/web/src/stores/development/usePendingSessionStore.ts b/apps/web/src/stores/development/usePendingSessionStore.ts new file mode 100644 index 0000000000..474b037a78 --- /dev/null +++ b/apps/web/src/stores/development/usePendingSessionStore.ts @@ -0,0 +1,27 @@ +import { create } from 'zustand'; +import type { PendingSession } from '@/lib/development/pending-session'; +import type { OpenTerminalScope } from '@/stores/machine-workspace/useMachineWorkspaceStore'; + +/** + * The one session-open intent in flight from the Development sidebar. + * + * The sidebar and the machine's pane region are siblings in the layout (the + * sidebar lives above the routed page), so the click and the component that can + * honour it have no common parent to hold this — hence a store, in the same way + * the machine workspace itself is shared by composition. + * + * Single-slot on purpose: a second click supersedes the first, because the user + * only ever ends up on one machine. All the decision-making lives in the pure + * `resolvePendingSession`; this only holds the value. + */ +interface PendingSessionStoreState { + pending: PendingSession | null; + requestSession: (machineId: string, scope: OpenTerminalScope) => void; + clearPending: () => void; +} + +export const usePendingSessionStore = create((set) => ({ + pending: null, + requestSession: (machineId, scope) => set({ pending: { machineId, scope } }), + clearPending: () => set((state) => (state.pending === null ? state : { pending: null })), +})); From 3ae283d4dda989e2a0f2cb5d9887bdd53088344c Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 11:41:46 -0500 Subject: [PATCH 07/18] fix(development): session clicks were silently dropped by a React lane race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second self-review pass, and this one was load-bearing: the surface's headline flow — expand a machine in the sidebar, click one of its sessions — did nothing at all unless you were already viewing that machine. requestSession() is a plain store write (SYNC lane); router.push() dispatches inside a transition. React commits the sync update first, so there is an intermediate commit holding the NEW intent and the OLD pathname. The drain read that as "the user navigated away" and cleared the intent before the navigation it was waiting for ever arrived. The pure function could not tell the two apart — both look like selectedMachineId !== pending.machineId — and the test suite had encoded the broken policy as intended behavior, which is why it passed. An intent now records the machine that was selected when it was made, so "my navigation hasn't landed yet" (selection still == origin → hold) is distinguishable from "the user chose a third machine" (→ drop). Two further fixes to the keep-alive wiring: - MachineKeepAliveHost takes an optional machineIds list. The drive view infers machines from the page tree; this surface KNOWS them (/api/machines). The two sources disagree — a machine absent from the tree (failed tree fetch, or a private machine granted via a custom drive role, which the tree endpoint does not resolve) was treated as trashed and evicted from the LRU on the next machine switch, disconnecting a live terminal. Passing the list also skips the page-tree fetch this surface has no other use for. - The detail pane no longer goes silently blank: a machine still mounting shows "Opening machine…", and an unknown/deleted machine id shows "Machine not found" instead of an empty region (both the host and the route render null in that case). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../[driveId]/development/layout.tsx | 45 +++++++++++++++++- .../left-sidebar/DevelopmentSidebar.tsx | 11 +++-- .../middle-content/MachineKeepAliveHost.tsx | Bin 4719 -> 6222 bytes .../__tests__/pending-session.test.ts | 31 ++++++++++-- .../src/lib/development/pending-session.ts | 28 ++++++++++- .../development/usePendingSessionStore.ts | 5 +- 6 files changed, 108 insertions(+), 12 deletions(-) diff --git a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx index bdcadb65f6..c92db73a7a 100644 --- a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx +++ b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx @@ -1,10 +1,12 @@ 'use client'; -import { useEffect } from 'react'; +import { useEffect, useMemo } from 'react'; import { useParams, usePathname } from 'next/navigation'; +import { Cpu } from 'lucide-react'; import MachineKeepAliveHost from '@/components/layout/middle-content/MachineKeepAliveHost'; import { useMachineWorkspaceStore } from '@/stores/machine-workspace/useMachineWorkspaceStore'; import { usePendingSessionStore } from '@/stores/development/usePendingSessionStore'; +import { useDriveMachines } from '@/hooks/useDriveMachines'; import { parseSelectedMachineId } from '@/lib/development/development-route'; import { resolvePendingSession } from '@/lib/development/pending-session'; @@ -31,12 +33,51 @@ export default function DevelopmentLayout({ children }: { children: React.ReactN const driveId = Array.isArray(driveIdParams) ? driveIdParams[0] : driveIdParams; const selectedMachineId = parseSelectedMachineId(pathname, driveId); + // The same SWR key the sidebar uses, so this is a cache read, not a second + // request. It is also the host's source of truth for what counts as a machine + // (see its `machineIds` prop) — the surface must not disagree with itself + // about which machines exist. + const { machines, isLoading } = useDriveMachines(driveId ?? null); + const machineIds = useMemo(() => machines.map((machine) => machine.id), [machines]); + useDrainPendingSession(selectedMachineId); + const isKnownMachine = selectedMachineId !== null && machineIds.includes(selectedMachineId); + return (
{children} - + + {/* Sits UNDER the host (which is `absolute inset-0 z-10` and opaque), so + it shows only while the machine has yet to mount, and is covered the + moment it does. Without it the pane is blank for that beat. */} + {selectedMachineId && isLoading && ( + + )} + + {/* A machine id that isn't in this drive's machines — deleted, trashed, or + simply wrong. The host declines to mount it and the route renders null, + so without this the user would sit on a silently empty pane. */} + {selectedMachineId && !isLoading && !isKnownMachine && ( + + )} + + +
+ ); +} + +function DetailNotice({ title, description }: { title: string; description?: string }) { + return ( +
+ +
+

{title}

+ {description &&

{description}

} +
); } diff --git a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx index 474d519e5a..55ddeceb70 100644 --- a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx +++ b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx @@ -140,6 +140,7 @@ function MachineList({ machineId={machine.id} title={machine.title} selected={machine.id === selectedMachineId} + selectedMachineId={selectedMachineId} /> ))} @@ -163,11 +164,14 @@ function MachineTreeSection({ machineId, title, selected, + selectedMachineId, }: { driveId: string; machineId: string; title: string; selected: boolean; + /** The machine currently open — recorded on a session intent so the drain can tell a pending navigation from a real one. */ + selectedMachineId: string | null; }) { const router = useRouter(); const isSheetBreakpoint = useBreakpoint('(max-width: 1023px)'); @@ -185,8 +189,9 @@ function MachineTreeSection({ // once that machine's pane region exists. Writing the pane straight into // the workspace store from here would not survive — MachineWorkspace // disposes its workspace on unmount and rebuilds it on mount, destroying - // anything authored ahead of it. - requestSession(machineId, scope); + // anything authored ahead of it. `selectedMachineId` rides along so the + // drain can tell this in-flight navigation from the user going elsewhere. + requestSession(machineId, scope, selectedMachineId); openMachine(); // KNOWN GAP: if the user is already on THIS machine with a non-Terminal tab // active, the session lands in the pane but stays behind that tab — @@ -195,7 +200,7 @@ function MachineTreeSection({ // controlled, left to the follow-up rather than fought over with the // in-flight terminal-UX work (#2017). }, - [requestSession, machineId, openMachine], + [requestSession, machineId, selectedMachineId, openMachine], ); const renderNodeChildren = useCallback( diff --git a/apps/web/src/components/layout/middle-content/MachineKeepAliveHost.tsx b/apps/web/src/components/layout/middle-content/MachineKeepAliveHost.tsx index 40a2b906dce4cbf4027db4cc6a46f242bd474592..093cd7ecd2c8acaa5fac28c286b8915c5dd8dd90 100644 GIT binary patch delta 1542 zcmZ8h&1zdm6s9%7Gzk>CEHoUPl1Q#1wOtB=2u&b@x(%rdrKRBUy+^u(Ml+Y0xmWf| z-4(j&LVba5yDE6seYbgrkQWH#3HqIxD_hmZzV^&~^ZlK9`{}FC|NimdXgasfb4ja; zEuAe@QEAK5GMtePG}kTL%?}5^t%*j*$G3^-m`*FE(qrXepl(0uG7K_A#A>08VNc3< zR^=rvtXl=b!g81p_F$;hyrR-^un*&yl)a={%>~8MIg=`)Zr41Z;7CWR6rdcbun@$w zGvh_iOh+IcEBtU4C+XzLSI?*4JewQ=RV3=7+ENY_qBe&3LWejMQ5ohbw`^R!WE&|o zex?ehr{AuheMfJ8`Gd|zs@9`Ub@oVD7+?rF&$eVA(pKD<5i5LYI|nZn^G?^oor*n! zOpG`%fY3rUfs>}VauD12f&*-UDWukOs2gWIwUt1%=O8>alG`J8-b7mJ5ai;ScgqYc z7WIU%n3d6H9AU~#X~X4!98v?UZdDX=sSGstYHcGr5-abTd4(-S6CyxU4ahsggN&9f z-rhj4qjnk$yL4etaHi!xtz2VDi5E<8SBXIJsX`^6Tj#jdg{;l|x>NA=%kL3w5@Y!S z5<7}vrA3BBRO(PTYZqSYRXe){?9^}+bylflleaT-K>ay{ak zj}Py!M{itCe&ceW_YJc8q*aY{{H?) z`+FWd*u4ATpIvp2u7%}k4Wyb_Ij^I_1H-91usv}%&MlgHkX{YMjDM#XB{ ({ @@ -52,8 +55,30 @@ describe('resolvePendingSession', () => { }); }); - test('drops the intent when the user navigated to a different machine', () => { - expect(resolvePendingSession(PENDING, 'machine-2', workspaceWith(null))).toEqual({ type: 'clear' }); + test('HOLDS the intent while the click\'s own navigation is still in flight', () => { + // The bug this guards, and the reason the surface's headline flow was + // silently dead: the click (a store write) lands in the SYNC lane, while + // router.push dispatches inside a TRANSITION. React commits the sync update + // first, so there is an intermediate commit holding the new intent and the + // OLD pathname — selectedMachineId is still the machine we came FROM. + // Reading that as "the user navigated away" threw the intent away before the + // navigation it was waiting for ever arrived. + expect(resolvePendingSession(PENDING_FROM_ELSEWHERE, 'machine-9', undefined)).toEqual({ type: 'wait' }); + }); + + test('opens once that navigation lands', () => { + expect(resolvePendingSession(PENDING_FROM_ELSEWHERE, 'machine-1', workspaceWith(null))).toEqual({ + type: 'open', + machineId: 'machine-1', + scope: SCOPE, + }); + }); + + test('drops the intent when the user genuinely goes to a THIRD machine', () => { + // Neither the target nor the origin — a real navigation away, not a pending one. + expect(resolvePendingSession(PENDING_FROM_ELSEWHERE, 'machine-2', workspaceWith(null))).toEqual({ + type: 'clear', + }); }); test('drops the intent when the user left the surface entirely', () => { diff --git a/apps/web/src/lib/development/pending-session.ts b/apps/web/src/lib/development/pending-session.ts index 86fce0eca8..21ca6aafeb 100644 --- a/apps/web/src/lib/development/pending-session.ts +++ b/apps/web/src/lib/development/pending-session.ts @@ -4,6 +4,22 @@ import type { OpenTerminalScope, WorkspaceState } from '@/stores/machine-workspa export interface PendingSession { machineId: string; scope: OpenTerminalScope; + /** + * The machine that was selected when the click happened (null = none). + * + * This is what lets "my navigation hasn't landed yet" be told apart from "the + * user went somewhere else" — two states that otherwise look identical, since + * both simply have `selectedMachineId !== pending.machineId`. + * + * They must be told apart, because the click and the navigation land in + * DIFFERENT React lanes: `requestSession` is a plain store write (sync lane), + * while `router.push` dispatches inside a transition. React commits the sync + * update first, so there is an intermediate commit holding the NEW intent and + * the OLD pathname. Treating that commit as "navigated away" drops the intent + * before the navigation it is waiting for ever arrives — which silently broke + * every session click on a machine the user wasn't already viewing. + */ + fromMachineId: string | null; } export type PendingSessionAction = @@ -60,8 +76,16 @@ export function resolvePendingSession( workspace: WorkspaceState | undefined, ): PendingSessionAction { if (!pending) return { type: 'clear' }; - // The user went somewhere else before the machine ever mounted — the intent is moot. - if (pending.machineId !== selectedMachineId) return { type: 'clear' }; + + if (pending.machineId !== selectedMachineId) { + // Still where we were when the session was clicked: the router's transition + // simply hasn't committed yet. Hold — this is NOT the user navigating away. + if (selectedMachineId === pending.fromMachineId) return { type: 'wait' }; + // A different machine than either the target or the origin: the user chose + // to go elsewhere, so the intent is moot. + return { type: 'clear' }; + } + if (!workspace) return { type: 'wait' }; // Satisfied: the session is in the active pane. if (sameScope(activePaneScope(workspace), pending.scope)) return { type: 'clear' }; diff --git a/apps/web/src/stores/development/usePendingSessionStore.ts b/apps/web/src/stores/development/usePendingSessionStore.ts index 474b037a78..492dd69ea5 100644 --- a/apps/web/src/stores/development/usePendingSessionStore.ts +++ b/apps/web/src/stores/development/usePendingSessionStore.ts @@ -16,12 +16,13 @@ import type { OpenTerminalScope } from '@/stores/machine-workspace/useMachineWor */ interface PendingSessionStoreState { pending: PendingSession | null; - requestSession: (machineId: string, scope: OpenTerminalScope) => void; + /** `fromMachineId` is the machine selected at click time — see `PendingSession`. */ + requestSession: (machineId: string, scope: OpenTerminalScope, fromMachineId: string | null) => void; clearPending: () => void; } export const usePendingSessionStore = create((set) => ({ pending: null, - requestSession: (machineId, scope) => set({ pending: { machineId, scope } }), + requestSession: (machineId, scope, fromMachineId) => set({ pending: { machineId, scope, fromMachineId } }), clearPending: () => set((state) => (state.pending === null ? state : { pending: null })), })); From a0fb9f355e4be68cccccd35ad75a77f4e866247a Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 11:59:57 -0500 Subject: [PATCH 08/18] fix(development): expire session intents; honour fetch errors; make the shared host diffable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third self-review pass. 1. A session intent could leak out of the surface and hijack a pane later. It had no terminal state: returning to where you started left it parked forever, and the store is a module singleton, so it survived leaving Development entirely. Coming back to that machine — warm, with a terminal running in the active pane — fired the stale intent and overwrote it. Intents now carry a createdAt and expire (PENDING_SESSION_TTL_MS); the surface clears any unconverged intent on unmount; and picking a machine ROW (rather than one of its sessions) clears one too, since that says "this machine as it is". Dropping fromMachineId in favour of the TTL also fixes a second silent drop: two quick session clicks on different machines used to destroy the second intent when the first navigation committed. A mismatch now WAITS, bounded by the TTL, instead of guessing at the user's intent from a single commit. 2. "Machine not found" was shown over a perfectly good machine whenever /api/machines failed: SWR reports isLoading:false with data undefined on the error path, which is indistinguishable from "no such machine" unless the error is checked first. The detail pane now checks error first, and gates its fetch on isAdmin like the sidebar (a non-admin was firing a request that 403s and audits on every load). The sidebar no longer asserts "not an admin" before auth has resolved — that flashed the refusal at real admins on cold loads. 3. MachineKeepAliveHost.tsx carried a literal NUL byte (pre-existing on master), so git classified it as BINARY: every change to it renders as "Bin N -> M bytes" with no hunks — which is exactly how this PR's edit to a file SHARED with the drive view escaped two review passes. The NUL is now written as an escape (same value), and .gitattributes forces textual diffs on source, so that class of mistake is cosmetic instead of review-defeating. The edit is now a readable 29/6-line diff. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .gitattributes | 21 ++++ .../[driveId]/development/layout.tsx | 89 +++++++++++----- .../left-sidebar/DevelopmentSidebar.tsx | 32 ++++-- .../middle-content/MachineKeepAliveHost.tsx | Bin 6222 -> 6227 bytes .../__tests__/pending-session.test.ts | 99 +++++++++--------- .../src/lib/development/pending-session.ts | 94 +++++++++-------- .../development/usePendingSessionStore.ts | 7 +- 7 files changed, 213 insertions(+), 129 deletions(-) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000000..89e4ea1352 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,21 @@ +# Source is always text, and must always diff as text. +# +# git classifies a blob as binary if it finds a NUL byte in the first 8k — and a +# stray NUL in a source file is easy to introduce (a NUL used as a string +# delimiter, pasted verbatim instead of escaped) and almost impossible to notice. +# The cost is severe and silent: `git diff` and GitHub render the whole file as +# "Bin N -> M bytes", so every change to it sails through review unseen, and it +# gets no three-way merge. +# +# This happened: MachineKeepAliveHost.tsx carried a literal NUL and was binary to +# git for its whole history. Forcing `diff` on source extensions makes that class +# of mistake cosmetic instead of review-defeating. +*.ts diff +*.tsx diff +*.js diff +*.jsx diff +*.mjs diff +*.cjs diff +*.json diff +*.css diff +*.md diff diff --git a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx index c92db73a7a..642a179cc5 100644 --- a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx +++ b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx @@ -4,9 +4,10 @@ import { useEffect, useMemo } from 'react'; import { useParams, usePathname } from 'next/navigation'; import { Cpu } from 'lucide-react'; import MachineKeepAliveHost from '@/components/layout/middle-content/MachineKeepAliveHost'; +import { useAuth } from '@/hooks/useAuth'; +import { useDriveMachines } from '@/hooks/useDriveMachines'; import { useMachineWorkspaceStore } from '@/stores/machine-workspace/useMachineWorkspaceStore'; import { usePendingSessionStore } from '@/stores/development/usePendingSessionStore'; -import { useDriveMachines } from '@/hooks/useDriveMachines'; import { parseSelectedMachineId } from '@/lib/development/development-route'; import { resolvePendingSession } from '@/lib/development/pending-session'; @@ -33,35 +34,28 @@ export default function DevelopmentLayout({ children }: { children: React.ReactN const driveId = Array.isArray(driveIdParams) ? driveIdParams[0] : driveIdParams; const selectedMachineId = parseSelectedMachineId(pathname, driveId); - // The same SWR key the sidebar uses, so this is a cache read, not a second - // request. It is also the host's source of truth for what counts as a machine - // (see its `machineIds` prop) — the surface must not disagree with itself - // about which machines exist. - const { machines, isLoading } = useDriveMachines(driveId ?? null); + const { user } = useAuth(); + const isAdmin = user?.role === 'admin'; + + // Same SWR key (and same admin gate) as the sidebar, so this is a cache read + // rather than a second request — and a non-admin still fires none. It is also + // the host's source of truth for what counts as a machine (its `machineIds` + // prop): the surface must not disagree with itself about which machines exist. + const { machines, isLoading, error } = useDriveMachines(isAdmin ? driveId ?? null : null); const machineIds = useMemo(() => machines.map((machine) => machine.id), [machines]); useDrainPendingSession(selectedMachineId); - const isKnownMachine = selectedMachineId !== null && machineIds.includes(selectedMachineId); - return (
{children} - {/* Sits UNDER the host (which is `absolute inset-0 z-10` and opaque), so - it shows only while the machine has yet to mount, and is covered the - moment it does. Without it the pane is blank for that beat. */} - {selectedMachineId && isLoading && ( - - )} - - {/* A machine id that isn't in this drive's machines — deleted, trashed, or - simply wrong. The host declines to mount it and the route renders null, - so without this the user would sit on a silently empty pane. */} - {selectedMachineId && !isLoading && !isKnownMachine && ( - )} @@ -70,6 +64,47 @@ export default function DevelopmentLayout({ children }: { children: React.ReactN ); } +/** + * What the detail pane shows when the machine itself can't be. Rendered UNDER + * the keep-alive host (which is `absolute inset-0 z-10` and opaque), so a state + * here is covered the moment the machine actually mounts. Without it, every one + * of these cases is an unexplained blank region — the route renders null and the + * host declines to mount. + */ +function DetailState({ + isAdmin, + isLoading, + error, + isKnownMachine, +}: { + isAdmin: boolean; + isLoading: boolean; + error: Error | undefined; + isKnownMachine: boolean; +}) { + if (!isAdmin) return ; + // Before "not found": a failed fetch leaves `machines` empty with isLoading + // false, which is indistinguishable from "this machine doesn't exist" unless + // the error is checked FIRST. Getting this order wrong told users their + // perfectly good machine had been deleted. + if (error) { + return ( + + ); + } + if (isLoading) return ; + if (!isKnownMachine) { + return ( + + ); + } + // The machine exists and the host is mounting it — it will paint over this. + return ; +} + function DetailNotice({ title, description }: { title: string; description?: string }) { return (
@@ -91,6 +126,10 @@ function DetailNotice({ title, description }: { title: string; description?: str * workspace that is then torn down and rebuilt (a remount — StrictMode's * double-invoke does this on first mount) re-applies to the new one instead of * being silently lost. + * + * Leaving the surface drops any unconverged intent: the store is a module + * singleton, so an intent left behind here would otherwise still be sitting + * there on the user's next visit, ready to fire into whatever pane was active. */ function useDrainPendingSession(selectedMachineId: string | null) { const pending = usePendingSessionStore((state) => state.pending); @@ -101,9 +140,11 @@ function useDrainPendingSession(selectedMachineId: string | null) { ); useEffect(() => { - const action = resolvePendingSession(pending, selectedMachineId, workspace); + const action = resolvePendingSession(pending, selectedMachineId, workspace, Date.now()); if (action.type === 'open') openTerminal(action.machineId, action.scope); - // 'clear' with no pending intent is a no-op, so this cannot loop. + // A 'clear' with no pending intent is a no-op, so this cannot loop. else if (action.type === 'clear') clearPending(); }, [pending, selectedMachineId, workspace, openTerminal, clearPending]); + + useEffect(() => () => clearPending(), [clearPending]); } diff --git a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx index 55ddeceb70..4d1557317c 100644 --- a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx +++ b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx @@ -47,7 +47,7 @@ export default function DevelopmentSidebar({ className }: SidebarProps) { const drive = drives.find((d) => d.id === driveId); const canManage = canManageDrive(drive); - const { user } = useAuth(); + const { user, isLoading: authLoading } = useAuth(); const isAdmin = user?.role === 'admin'; // The same gate MachineView applies, moved one level earlier: a non-admin who @@ -80,6 +80,7 @@ export default function DevelopmentSidebar({ className }: SidebarProps) {
Loading…; // Same wording MachineView uses, so the surface and the page refuse a // non-admin identically. if (!isAdmin) return Machine access requires administrator privileges; @@ -140,7 +146,6 @@ function MachineList({ machineId={machine.id} title={machine.title} selected={machine.id === selectedMachineId} - selectedMachineId={selectedMachineId} /> ))} @@ -164,35 +169,40 @@ function MachineTreeSection({ machineId, title, selected, - selectedMachineId, }: { driveId: string; machineId: string; title: string; selected: boolean; - /** The machine currently open — recorded on a session intent so the drain can tell a pending navigation from a real one. */ - selectedMachineId: string | null; }) { const router = useRouter(); const isSheetBreakpoint = useBreakpoint('(max-width: 1023px)'); const setLeftSheetOpen = useLayoutStore((state) => state.setLeftSheetOpen); const requestSession = usePendingSessionStore((state) => state.requestSession); + const clearPending = usePendingSessionStore((state) => state.clearPending); - const openMachine = useCallback(() => { + const navigateToMachine = useCallback(() => { router.push(`/dashboard/${driveId}/development/${machineId}`); if (isSheetBreakpoint) setLeftSheetOpen(false); }, [router, driveId, machineId, isSheetBreakpoint, setLeftSheetOpen]); + const openMachine = useCallback(() => { + // Picking the machine itself (not one of its sessions) says the user wants + // this machine as it is — so an older, still-unconverged session intent must + // not follow them here and take over the pane. + clearPending(); + navigateToMachine(); + }, [clearPending, navigateToMachine]); + const onOpenTerminal = useCallback( (scope: OpenTerminalScope) => { // Record the intent and navigate; the surface's layout opens the session // once that machine's pane region exists. Writing the pane straight into // the workspace store from here would not survive — MachineWorkspace // disposes its workspace on unmount and rebuilds it on mount, destroying - // anything authored ahead of it. `selectedMachineId` rides along so the - // drain can tell this in-flight navigation from the user going elsewhere. - requestSession(machineId, scope, selectedMachineId); - openMachine(); + // anything authored ahead of it. + requestSession(machineId, scope); + navigateToMachine(); // KNOWN GAP: if the user is already on THIS machine with a non-Terminal tab // active, the session lands in the pane but stays behind that tab — // MachineView's tabs are uncontrolled (defaultValue="terminal"), so nothing @@ -200,7 +210,7 @@ function MachineTreeSection({ // controlled, left to the follow-up rather than fought over with the // in-flight terminal-UX work (#2017). }, - [requestSession, machineId, selectedMachineId, openMachine], + [requestSession, machineId, navigateToMachine], ); const renderNodeChildren = useCallback( diff --git a/apps/web/src/components/layout/middle-content/MachineKeepAliveHost.tsx b/apps/web/src/components/layout/middle-content/MachineKeepAliveHost.tsx index 093cd7ecd2c8acaa5fac28c286b8915c5dd8dd90..f6aae71a1170df8aafcf6f718d4317995e88edf7 100644 GIT binary patch delta 19 ZcmX?SaM@tPBN4WkQUf5^{8%KL830lK2Uh?9 delta 14 Vcmca?aL!=EBN0Z1%`Zf ({ @@ -15,27 +17,33 @@ const workspaceWith = (scope: WorkspaceState['columns'][number]['panes'][number] }); describe('resolvePendingSession', () => { - test('opens the session once the target machine has a workspace', () => { - expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(null))).toEqual({ + test('opens the session once the user is on the machine and it has a workspace', () => { + expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(null), NOW)).toEqual({ type: 'open', machineId: 'machine-1', scope: SCOPE, }); }); - test('holds the intent while the machine has no workspace yet', () => { - // The pane region mounts after the navigation lands; the intent waits for it - // rather than being written into a workspace that does not exist. - expect(resolvePendingSession(PENDING, 'machine-1', undefined)).toEqual({ type: 'wait' }); + test('holds while the click\'s own navigation is still in flight', () => { + // The bug this guards, and the reason the surface's headline flow was + // silently dead: the click (a store write) lands in React's SYNC lane, while + // router.push dispatches inside a TRANSITION. React commits the sync update + // first, so there is an intermediate commit holding the new intent and the + // OLD pathname. Reading that as "the user navigated away" threw the intent + // away before the navigation it was waiting for ever arrived. + expect(resolvePendingSession(PENDING, 'machine-9', undefined, NOW)).toEqual({ type: 'wait' }); }); - test('re-opens against a workspace that was torn down and rebuilt', () => { - // The bug this guards: MachineWorkspace disposes on unmount and re-creates on - // mount (StrictMode double-invokes this on the first visit). A fire-once - // intent would be destroyed by the rebuild; a convergent one re-applies. - const rebuiltEmpty = workspaceWith(null); + test('holds until the machine\'s pane region has mounted', () => { + expect(resolvePendingSession(PENDING, 'machine-1', undefined, NOW)).toEqual({ type: 'wait' }); + }); - expect(resolvePendingSession(PENDING, 'machine-1', rebuiltEmpty)).toEqual({ + test('re-opens against a workspace that was torn down and rebuilt', () => { + // MachineWorkspace disposes on unmount and re-creates on mount (StrictMode + // double-invokes exactly this on the first visit). A fire-once intent would + // be destroyed by the rebuild; a convergent one re-applies to the new one. + expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(null), NOW)).toEqual({ type: 'open', machineId: 'machine-1', scope: SCOPE, @@ -43,54 +51,45 @@ describe('resolvePendingSession', () => { }); test('clears once the session is actually in the active pane', () => { - expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(SCOPE))).toEqual({ type: 'clear' }); + expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(SCOPE), NOW)).toEqual({ type: 'clear' }); }); - test('a satisfied intent is dropped, so it cannot clobber the user\'s next pane change', () => { - const satisfied = resolvePendingSession(PENDING, 'machine-1', workspaceWith(SCOPE)); - expect(satisfied).toEqual({ type: 'clear' }); - // And with no intent left, nothing is ever re-applied. - expect(resolvePendingSession(null, 'machine-1', workspaceWith({ ...SCOPE, name: 'agent-2' }))).toEqual({ - type: 'clear', - }); - }); + test('expires rather than lying in wait to hijack a pane later', () => { + // The leak this closes: an intent that never converged (the machine never + // mounted, or the user turned back) used to be held indefinitely in a + // module-level store. Returning to that machine much later — warm, with a + // terminal running in its active pane — would fire the stale intent and + // overwrite that pane. Past the TTL it is simply dropped. + const stale = resolvePendingSession( + PENDING, + 'machine-1', + workspaceWith({ name: 'something-the-user-is-using' }), + NOW + PENDING_SESSION_TTL_MS + 1, + ); - test('HOLDS the intent while the click\'s own navigation is still in flight', () => { - // The bug this guards, and the reason the surface's headline flow was - // silently dead: the click (a store write) lands in the SYNC lane, while - // router.push dispatches inside a TRANSITION. React commits the sync update - // first, so there is an intermediate commit holding the new intent and the - // OLD pathname — selectedMachineId is still the machine we came FROM. - // Reading that as "the user navigated away" threw the intent away before the - // navigation it was waiting for ever arrived. - expect(resolvePendingSession(PENDING_FROM_ELSEWHERE, 'machine-9', undefined)).toEqual({ type: 'wait' }); + expect(stale).toEqual({ type: 'clear' }); }); - test('opens once that navigation lands', () => { - expect(resolvePendingSession(PENDING_FROM_ELSEWHERE, 'machine-1', workspaceWith(null))).toEqual({ + test('a slow-but-live navigation is not expired', () => { + expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(null), NOW + PENDING_SESSION_TTL_MS - 1)).toEqual({ type: 'open', machineId: 'machine-1', scope: SCOPE, }); }); - test('drops the intent when the user genuinely goes to a THIRD machine', () => { - // Neither the target nor the origin — a real navigation away, not a pending one. - expect(resolvePendingSession(PENDING_FROM_ELSEWHERE, 'machine-2', workspaceWith(null))).toEqual({ - type: 'clear', - }); - }); - - test('drops the intent when the user left the surface entirely', () => { - expect(resolvePendingSession(PENDING, null, workspaceWith(null))).toEqual({ type: 'clear' }); + test('a satisfied intent is dropped, so it cannot clobber the user\'s next pane change', () => { + expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(SCOPE), NOW)).toEqual({ type: 'clear' }); + // And with no intent left, nothing is ever re-applied. + expect( + resolvePendingSession(null, 'machine-1', workspaceWith({ ...SCOPE, name: 'agent-2' }), NOW), + ).toEqual({ type: 'clear' }); }); test('distinguishes same-named sessions at different scopes', () => { // A machine-scope "agent-1" is not the branch-scope "agent-1"; treating them // as the same would report the intent satisfied by the wrong session. - const machineScoped = workspaceWith({ name: 'agent-1' }); - - expect(resolvePendingSession(PENDING, 'machine-1', machineScoped)).toEqual({ + expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith({ name: 'agent-1' }), NOW)).toEqual({ type: 'open', machineId: 'machine-1', scope: SCOPE, @@ -98,6 +97,6 @@ describe('resolvePendingSession', () => { }); test('no intent is a no-op', () => { - expect(resolvePendingSession(null, 'machine-1', undefined)).toEqual({ type: 'clear' }); + expect(resolvePendingSession(null, 'machine-1', undefined, NOW)).toEqual({ type: 'clear' }); }); }); diff --git a/apps/web/src/lib/development/pending-session.ts b/apps/web/src/lib/development/pending-session.ts index 21ca6aafeb..3e9d36610e 100644 --- a/apps/web/src/lib/development/pending-session.ts +++ b/apps/web/src/lib/development/pending-session.ts @@ -1,33 +1,31 @@ import type { OpenTerminalScope, WorkspaceState } from '@/stores/machine-workspace/useMachineWorkspaceStore'; +/** + * How long a session-open intent stays live. + * + * The navigation it accompanies commits in milliseconds, and the machine's pane + * region mounts within a second or so (`MachineWorkspace` is a dynamic import). + * This is the backstop for an intent that never converges — the machine failed + * to mount, or the user turned back mid-flight — so it cannot lie in wait and + * later hijack the active pane of a machine they've since returned to. Generous + * on purpose: it must never expire a navigation that is merely slow. + */ +export const PENDING_SESSION_TTL_MS = 30_000; + /** A session the user clicked in the Development sidebar, to be opened on the machine it belongs to. */ export interface PendingSession { machineId: string; scope: OpenTerminalScope; - /** - * The machine that was selected when the click happened (null = none). - * - * This is what lets "my navigation hasn't landed yet" be told apart from "the - * user went somewhere else" — two states that otherwise look identical, since - * both simply have `selectedMachineId !== pending.machineId`. - * - * They must be told apart, because the click and the navigation land in - * DIFFERENT React lanes: `requestSession` is a plain store write (sync lane), - * while `router.push` dispatches inside a transition. React commits the sync - * update first, so there is an intermediate commit holding the NEW intent and - * the OLD pathname. Treating that commit as "navigated away" drops the intent - * before the navigation it is waiting for ever arrives — which silently broke - * every session click on a machine the user wasn't already viewing. - */ - fromMachineId: string | null; + /** When the click happened — the intent expires `PENDING_SESSION_TTL_MS` later. */ + createdAt: number; } export type PendingSessionAction = - /** The machine's pane region isn't there yet — hold the intent. */ + /** The user isn't on that machine yet, or its pane region isn't there — hold. */ | { type: 'wait' } /** Apply the intent (idempotent: `openTerminal` sets the active pane's scope). */ | { type: 'open'; machineId: string; scope: OpenTerminalScope } - /** Done, or moot — drop the intent. */ + /** Done, superseded, or expired — drop the intent. */ | { type: 'clear' }; function sameScope(a: OpenTerminalScope | null, b: OpenTerminalScope): boolean { @@ -52,42 +50,56 @@ function activePaneScope(workspace: WorkspaceState): OpenTerminalScope | null { * What to do with a sidebar session-click intent, given where the user now is * and whether the target machine's workspace exists yet. * - * A clicked session belongs to a machine whose pane region may not be mounted — - * the user could be on another machine, or on none. Authoring the pane straight - * into the store before that machine mounts does not survive: `MachineWorkspace` - * disposes its workspace on unmount and re-creates it on mount, so a pane - * written ahead of the mount is destroyed by the very component meant to display - * it (React's StrictMode double-invoke makes this bite on the first visit, and a - * remount would do the same in production). + * Two things make this trickier than it looks, and both were live bugs: + * + * 1. The intent CANNOT be applied when it is made. The clicked session may + * belong to a machine whose pane region isn't mounted, and writing a pane + * into the store ahead of that mount does not survive: `MachineWorkspace` + * disposes its workspace on unmount and rebuilds it on mount, destroying + * anything authored early. So the intent is held and re-evaluated as state + * arrives, and it CONVERGES rather than firing once — it keeps asking until + * the workspace reports the session in its active pane. That is what makes it + * survive the workspace being torn down and rebuilt underneath it (React + * StrictMode does exactly this on first mount). Re-applying is harmless + * because `openTerminal` is idempotent. * - * So the intent is HELD instead, and re-evaluated as state arrives. This - * converges rather than fires once: it keeps asking to open until the workspace - * actually reports the session in its active pane, at which point the intent is - * satisfied and dropped. That is what makes it robust to the workspace being - * torn down and rebuilt underneath it — the intent simply re-applies to the new - * workspace. Being idempotent, a repeat `open` is harmless. + * 2. "The user isn't on that machine yet" and "the user went somewhere else" + * are INDISTINGUISHABLE from a single commit — both are just + * `selectedMachineId !== pending.machineId`. They can't be told apart because + * the click (a store write) lands in React's sync lane while `router.push` + * dispatches in a transition, so there is a commit holding the new intent and + * the old pathname. An earlier version treated that commit as "navigated + * away" and dropped the intent before its own navigation landed — which + * silently broke every session click on a machine the user wasn't already + * viewing. So a mismatch WAITS, and staleness is bounded by time + * ({@link PENDING_SESSION_TTL_MS}) instead of by guessing at intent. The + * sidebar additionally clears the intent when the user picks a different + * machine outright, so the TTL is only ever the backstop. * - * Once satisfied, the intent is cleared, so the user's own later pane changes on - * that machine are never clobbered by a stale intent. + * Once satisfied it is cleared, so the user's own later pane changes on that + * machine are never clobbered by a stale intent. */ export function resolvePendingSession( pending: PendingSession | null, selectedMachineId: string | null, workspace: WorkspaceState | undefined, + now: number, ): PendingSessionAction { if (!pending) return { type: 'clear' }; - if (pending.machineId !== selectedMachineId) { - // Still where we were when the session was clicked: the router's transition - // simply hasn't committed yet. Hold — this is NOT the user navigating away. - if (selectedMachineId === pending.fromMachineId) return { type: 'wait' }; - // A different machine than either the target or the origin: the user chose - // to go elsewhere, so the intent is moot. - return { type: 'clear' }; - } + // Never converged (machine never mounted, user turned back). Drop it rather + // than let it fire into whatever pane is active whenever they next arrive. + if (now - pending.createdAt > PENDING_SESSION_TTL_MS) return { type: 'clear' }; + + // Not there yet: either the click's own navigation hasn't committed, or the + // user is en route elsewhere. Holding is safe — the TTL bounds it. + if (pending.machineId !== selectedMachineId) return { type: 'wait' }; + // On the machine, but its pane region hasn't mounted (and ensured a workspace). if (!workspace) return { type: 'wait' }; + // Satisfied: the session is in the active pane. if (sameScope(activePaneScope(workspace), pending.scope)) return { type: 'clear' }; + return { type: 'open', machineId: pending.machineId, scope: pending.scope }; } diff --git a/apps/web/src/stores/development/usePendingSessionStore.ts b/apps/web/src/stores/development/usePendingSessionStore.ts index 492dd69ea5..25ea8f90a9 100644 --- a/apps/web/src/stores/development/usePendingSessionStore.ts +++ b/apps/web/src/stores/development/usePendingSessionStore.ts @@ -16,13 +16,14 @@ import type { OpenTerminalScope } from '@/stores/machine-workspace/useMachineWor */ interface PendingSessionStoreState { pending: PendingSession | null; - /** `fromMachineId` is the machine selected at click time — see `PendingSession`. */ - requestSession: (machineId: string, scope: OpenTerminalScope, fromMachineId: string | null) => void; + requestSession: (machineId: string, scope: OpenTerminalScope) => void; clearPending: () => void; } export const usePendingSessionStore = create((set) => ({ pending: null, - requestSession: (machineId, scope, fromMachineId) => set({ pending: { machineId, scope, fromMachineId } }), + requestSession: (machineId, scope) => set({ pending: { machineId, scope, createdAt: Date.now() } }), + // Identity-stable when there's nothing to clear, so a no-op clear can't + // re-render (and so the drain effect can call it unconditionally). clearPending: () => set((state) => (state.pending === null ? state : { pending: null })), })); From 9ad364a40dcb5bf211f3259cc700a32d3c100784 Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 12:13:20 -0500 Subject: [PATCH 09/18] fix(development): make a session click actually reach the terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth self-review pass, and it turned the PR's one "known gap" into a bug I had mis-described. Only the Terminal tab mounts a machine's workspace (MachineWorkspace lives inside TerminalTab, and Radix unmounts inactive tab bodies). So clicking a session leaf for a machine parked on Code/Diff/Settings — a warm machine in the keep-alive LRU keeps whatever tab you left it on — had nowhere to land. I had documented this as "the session lands in the pane but stays behind that tab". It does not land at all: the intent waits for a workspace that never appears and the TTL discards it. A silently dead click, on the surface's primary interaction. MachineView's active tab now lives in a store (useMachineTabStore) instead of being uncontrolled Radix state, which makes "show me this machine's terminal" something another surface can ask for. The sidebar focuses the Terminal tab before navigating, so the workspace mounts and the session lands. Behaviour is otherwise unchanged: a machine with no stored tab shows Terminal exactly as before. Also: the detail pane was missing the sidebar's auth-loading gate, so an admin refreshing the page was told "Machine access requires administrator privileges" until the session fetch returned (`role` is not persisted across a reload). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../[driveId]/development/layout.tsx | 9 +++- .../left-sidebar/DevelopmentSidebar.tsx | 21 ++++---- .../page-views/machine/MachineView.tsx | 22 ++++++-- .../__tests__/useMachineTabStore.test.ts | 50 +++++++++++++++++++ .../machine-workspace/useMachineTabStore.ts | 41 +++++++++++++++ 5 files changed, 129 insertions(+), 14 deletions(-) create mode 100644 apps/web/src/stores/machine-workspace/__tests__/useMachineTabStore.test.ts create mode 100644 apps/web/src/stores/machine-workspace/useMachineTabStore.ts diff --git a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx index 642a179cc5..c29274b246 100644 --- a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx +++ b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx @@ -34,7 +34,7 @@ export default function DevelopmentLayout({ children }: { children: React.ReactN const driveId = Array.isArray(driveIdParams) ? driveIdParams[0] : driveIdParams; const selectedMachineId = parseSelectedMachineId(pathname, driveId); - const { user } = useAuth(); + const { user, isLoading: authLoading } = useAuth(); const isAdmin = user?.role === 'admin'; // Same SWR key (and same admin gate) as the sidebar, so this is a cache read @@ -52,6 +52,7 @@ export default function DevelopmentLayout({ children }: { children: React.ReactN {selectedMachineId && ( ; if (!isAdmin) return ; // Before "not found": a failed fetch leaves `machines` empty with isLoading // false, which is indistinguishable from "this machine doesn't exist" unless diff --git a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx index 4d1557317c..2f25d9c320 100644 --- a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx +++ b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx @@ -17,6 +17,7 @@ import { useDriveStore } from '@/hooks/useDrive'; import { canManageDrive } from '@/hooks/usePermissions'; import { useDriveMachines, type DriveMachine } from '@/hooks/useDriveMachines'; import { usePendingSessionStore } from '@/stores/development/usePendingSessionStore'; +import { useMachineTabStore } from '@/stores/machine-workspace/useMachineTabStore'; import { parseSelectedMachineId } from '@/lib/development/development-route'; import type { OpenTerminalScope } from '@/stores/machine-workspace/useMachineWorkspaceStore'; import MachineTree, { type MachineTreeNode } from '@/components/layout/middle-content/page-views/machine/workspace/MachineTree'; @@ -180,6 +181,7 @@ function MachineTreeSection({ const setLeftSheetOpen = useLayoutStore((state) => state.setLeftSheetOpen); const requestSession = usePendingSessionStore((state) => state.requestSession); const clearPending = usePendingSessionStore((state) => state.clearPending); + const focusTerminal = useMachineTabStore((state) => state.focusTerminal); const navigateToMachine = useCallback(() => { router.push(`/dashboard/${driveId}/development/${machineId}`); @@ -196,21 +198,20 @@ function MachineTreeSection({ const onOpenTerminal = useCallback( (scope: OpenTerminalScope) => { - // Record the intent and navigate; the surface's layout opens the session - // once that machine's pane region exists. Writing the pane straight into - // the workspace store from here would not survive — MachineWorkspace + // Bring the machine's Terminal tab forward FIRST. Only that tab mounts the + // machine's workspace, so on a machine parked on Code/Diff/Settings the + // session would otherwise have nowhere to land — the click would do + // nothing at all. + focusTerminal(machineId); + // Then record the intent and navigate; the surface's layout opens the + // session once that machine's pane region exists. Writing the pane straight + // into the workspace store from here would not survive — MachineWorkspace // disposes its workspace on unmount and rebuilds it on mount, destroying // anything authored ahead of it. requestSession(machineId, scope); navigateToMachine(); - // KNOWN GAP: if the user is already on THIS machine with a non-Terminal tab - // active, the session lands in the pane but stays behind that tab — - // MachineView's tabs are uncontrolled (defaultValue="terminal"), so nothing - // here can focus them. Focusing needs MachineView's active tab to become - // controlled, left to the follow-up rather than fought over with the - // in-flight terminal-UX work (#2017). }, - [requestSession, machineId, navigateToMachine], + [focusTerminal, requestSession, machineId, navigateToMachine], ); const renderNodeChildren = useCallback( diff --git a/apps/web/src/components/layout/middle-content/page-views/machine/MachineView.tsx b/apps/web/src/components/layout/middle-content/page-views/machine/MachineView.tsx index 53764410bf..9c463954e8 100644 --- a/apps/web/src/components/layout/middle-content/page-views/machine/MachineView.tsx +++ b/apps/web/src/components/layout/middle-content/page-views/machine/MachineView.tsx @@ -6,6 +6,11 @@ import { motion } from 'motion/react'; import { Code2, GitCompare, Settings, TerminalSquare } from 'lucide-react'; import { cn } from '@/lib/utils'; import { useAuth } from '@/hooks/useAuth'; +import { + useMachineTabStore, + DEFAULT_MACHINE_TAB, + type MachineTabValue, +} from '@/stores/machine-workspace/useMachineTabStore'; import { Tabs, TabsContent, TabsList, TabsTrigger } from '@/components/ui/tabs'; import TerminalTab from './tabs/TerminalTab'; import CodeTab from './tabs/CodeTab'; @@ -16,8 +21,6 @@ interface MachineViewProps { pageId: string; } -type MachineTabValue = 'terminal' | 'code' | 'diff' | 'settings'; - const TAB_TRIGGERS: { value: MachineTabValue; label: string; icon: React.ElementType }[] = [ { value: 'terminal', label: 'Terminal', icon: TerminalSquare }, { value: 'code', label: 'Code', icon: Code2 }, @@ -35,10 +38,19 @@ const TAB_TRIGGERS: { value: MachineTabValue; label: string; icon: React.Element * initializes. Terminal is the default. The export name (`MachineView`) and * `{ pageId }` prop shape are preserved so `CenterPanel.tsx` / * `MachineKeepAliveHost.tsx` need no change. + * + * The active tab is held in `useMachineTabStore` rather than by Radix, so that + * "show me this machine's terminal" is something another surface can ask for. + * The Development sidebar needs it: only the Terminal tab mounts a machine's + * workspace, so a session clicked on a machine parked on Code/Diff/Settings had + * nowhere to land. Behaviour is otherwise unchanged — a machine with no stored + * tab shows Terminal, as before. */ const MachineView = ({ pageId }: MachineViewProps) => { const { user } = useAuth(); const isAdmin = user?.role === 'admin'; + const activeTab = useMachineTabStore((state) => state.tabs[pageId] ?? DEFAULT_MACHINE_TAB); + const setTab = useMachineTabStore((state) => state.setTab); return ( { )} {isAdmin && ( - + setTab(pageId, value as MachineTabValue)} + className="flex min-h-0 flex-1 flex-col gap-0" + >
{TAB_TRIGGERS.map(({ value, label, icon: Icon }) => ( diff --git a/apps/web/src/stores/machine-workspace/__tests__/useMachineTabStore.test.ts b/apps/web/src/stores/machine-workspace/__tests__/useMachineTabStore.test.ts new file mode 100644 index 0000000000..94f60b2c90 --- /dev/null +++ b/apps/web/src/stores/machine-workspace/__tests__/useMachineTabStore.test.ts @@ -0,0 +1,50 @@ +import { describe, test, expect, beforeEach } from 'vitest'; +import { useMachineTabStore, DEFAULT_MACHINE_TAB } from '../useMachineTabStore'; + +beforeEach(() => { + useMachineTabStore.setState({ tabs: {} }); +}); + +const tabOf = (machineId: string) => useMachineTabStore.getState().tabs[machineId] ?? DEFAULT_MACHINE_TAB; + +describe('useMachineTabStore', () => { + test('a machine with no stored tab shows Terminal', () => { + // MachineView's previous uncontrolled default, preserved. + expect(tabOf('machine-1')).toBe('terminal'); + }); + + test('remembers each machine\'s tab independently', () => { + useMachineTabStore.getState().setTab('machine-1', 'diff'); + + expect(tabOf('machine-1')).toBe('diff'); + expect(tabOf('machine-2')).toBe('terminal'); + }); + + test('focusTerminal brings a machine parked on another tab back to Terminal', () => { + // The reason this store exists: only the Terminal tab mounts a machine's + // workspace, so a session clicked on a machine sitting on Code/Diff/Settings + // had nowhere to land and the click did nothing at all. + useMachineTabStore.getState().setTab('machine-1', 'code'); + + useMachineTabStore.getState().focusTerminal('machine-1'); + + expect(tabOf('machine-1')).toBe('terminal'); + }); + + test('no-op writes keep state identity, so they cannot re-render subscribers', () => { + useMachineTabStore.getState().setTab('machine-1', 'code'); + const before = useMachineTabStore.getState().tabs; + + useMachineTabStore.getState().setTab('machine-1', 'code'); + + expect(useMachineTabStore.getState().tabs).toBe(before); + }); + + test('focusTerminal on a machine already showing Terminal is a no-op', () => { + const before = useMachineTabStore.getState().tabs; + + useMachineTabStore.getState().focusTerminal('machine-1'); + + expect(useMachineTabStore.getState().tabs).toBe(before); + }); +}); diff --git a/apps/web/src/stores/machine-workspace/useMachineTabStore.ts b/apps/web/src/stores/machine-workspace/useMachineTabStore.ts new file mode 100644 index 0000000000..d1eddfb1dc --- /dev/null +++ b/apps/web/src/stores/machine-workspace/useMachineTabStore.ts @@ -0,0 +1,41 @@ +import { create } from 'zustand'; + +export type MachineTabValue = 'terminal' | 'code' | 'diff' | 'settings'; + +export const DEFAULT_MACHINE_TAB: MachineTabValue = 'terminal'; + +/** + * Which tab each Machine is showing, keyed by machine id. + * + * The Machine page's tabs used to be uncontrolled (`defaultValue="terminal"`), + * which made the active tab unreachable from anywhere else — and that quietly + * broke the Development surface: `MachineWorkspace` (the thing that creates a + * machine's workspace) only mounts inside the Terminal tab, so clicking a + * session for a machine parked on Code/Diff/Settings had nowhere to land. The + * click did nothing at all. + * + * Hoisting the value here makes "show me this machine's terminal" something a + * caller can actually ask for, without any other change to how the tabs behave: + * a machine with no entry shows Terminal, exactly as before. + */ +interface MachineTabStoreState { + tabs: Record; + setTab: (machineId: string, tab: MachineTabValue) => void; + /** Bring a machine's Terminal tab to the front — used when opening a session on it. */ + focusTerminal: (machineId: string) => void; +} + +export const useMachineTabStore = create((set) => ({ + tabs: {}, + setTab: (machineId, tab) => + set((state) => (state.tabs[machineId] === tab ? state : { tabs: { ...state.tabs, [machineId]: tab } })), + focusTerminal: (machineId) => + set((state) => + // An unset machine is ALREADY showing Terminal (that's the default), so + // this must not write an entry for it — that would be a state change, and + // a re-render of every MachineView, for no visible difference. + (state.tabs[machineId] ?? DEFAULT_MACHINE_TAB) === DEFAULT_MACHINE_TAB + ? state + : { tabs: { ...state.tabs, [machineId]: DEFAULT_MACHINE_TAB } }, + ), +})); From 1fa0f84afe88f2d099bdd92d4ac7e4c23107c302 Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 12:30:53 -0500 Subject: [PATCH 10/18] test(development): component tests for the sidebar; isolate the tab store in MachineView's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I had claimed React component tests can't run in a .pu worktree. That was wrong: they fail only when vitest is invoked from the repo root (dual-React resolution). From apps/web they run fine — so the components are now actually covered rather than merely typechecked. Adds DevelopmentSidebar tests for the wiring that broke twice in review: a session click must focus the machine's Terminal tab (only that tab mounts a workspace, so otherwise the click lands nowhere), record the intent, and navigate — plus the admin gate, the no-fetch-for-non-admins path, and the no-refusal-before-auth -resolves case. Also resets the new tab store in MachineView.test's beforeEach: it's a module singleton, so a test that switches tabs would otherwise leave the next one parked on that tab. Passes today only by test ordering; that's a landmine. 990 tests green across every touched area (sidebar, machine page-views, stores, lib/development, api/machines, the audit-coverage gate). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../__tests__/DevelopmentSidebar.test.tsx | 131 ++++++++++++++++++ .../page-views/machine/MachineView.test.tsx | 4 + 2 files changed, 135 insertions(+) create mode 100644 apps/web/src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx diff --git a/apps/web/src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx b/apps/web/src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx new file mode 100644 index 0000000000..65923ab812 --- /dev/null +++ b/apps/web/src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx @@ -0,0 +1,131 @@ +/** + * The Development sidebar's wiring: who may see the machine list, and what a + * session click actually does. + * + * The session-click path is the one worth pinning down — it was broken twice in + * review. It has to do three things in concert: bring the machine's Terminal tab + * forward (only that tab mounts a workspace, so otherwise the session has + * nowhere to land), record the intent (the pane region isn't mounted yet, so it + * cannot be applied now), and navigate. + */ +import { describe, test, expect, beforeEach, vi } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +const mockPush = vi.fn(); +const mockUseAuth = vi.fn(); + +vi.mock('next/navigation', () => ({ + useParams: () => ({ driveId: 'drive-1' }), + usePathname: () => '/dashboard/drive-1/development', + useRouter: () => ({ push: mockPush }), +})); + +vi.mock('@/hooks/useAuth', () => ({ useAuth: () => mockUseAuth() })); + +vi.mock('@/hooks/useDriveMachines', () => ({ + useDriveMachines: (driveId: string | null) => ({ + // A null driveId is how the sidebar refuses to fetch for a non-admin. + machines: driveId ? [{ id: 'machine-1', title: 'Dev box', updatedAt: '2026-07-12T00:00:00.000Z' }] : [], + isLoading: false, + error: undefined, + mutate: vi.fn(), + }), +})); + +vi.mock('@/hooks/useMachineProjects', () => ({ + useMachineProjects: () => ({ projects: [], isLoading: false, addProject: vi.fn(), removeProject: vi.fn() }), +})); +vi.mock('@/hooks/useMachineBranches', () => ({ + useMachineBranches: () => ({ branches: [], isLoading: false, addBranch: vi.fn(), removeBranch: vi.fn() }), +})); +vi.mock('@/hooks/useAgentTerminals', () => ({ + useAgentTerminals: (machineId: string | null) => ({ + agentTerminals: machineId ? [{ name: 'agent-1', agentType: 'claude', createdAt: '2026-07-12' }] : [], + isLoading: false, + addAgentTerminal: vi.fn(), + removeAgentTerminal: vi.fn(), + }), +})); +vi.mock('@/hooks/useGithubRepos', () => ({ + useGithubRepos: () => ({ repos: [], connected: true, isLoading: false, error: undefined, mutate: vi.fn() }), +})); +vi.mock('@/hooks/useIntegrations', () => ({ useProviders: () => ({ providers: [] }) })); + +// Sidebar chrome that isn't under test. +vi.mock('@/components/layout/navbar/DriveSwitcher', () => ({ default: () =>
})); +vi.mock('../PrimaryNavigation', () => ({ default: () =>
})); +vi.mock('../DriveFooter', () => ({ default: () =>
})); +vi.mock('../DashboardFooter', () => ({ default: () =>
})); + +import DevelopmentSidebar from '../DevelopmentSidebar'; +import { usePendingSessionStore } from '@/stores/development/usePendingSessionStore'; +import { useMachineTabStore } from '@/stores/machine-workspace/useMachineTabStore'; + +beforeEach(() => { + vi.clearAllMocks(); + usePendingSessionStore.setState({ pending: null }); + useMachineTabStore.setState({ tabs: {} }); + mockUseAuth.mockReturnValue({ user: { role: 'admin' }, isLoading: false }); +}); + +describe('DevelopmentSidebar', () => { + test('lists the drive\'s machines for an admin', async () => { + render(); + + expect(await screen.findByText('Dev box')).toBeDefined(); + }); + + test('refuses a non-admin, and asks for no machines on their behalf', () => { + mockUseAuth.mockReturnValue({ user: { role: 'user' }, isLoading: false }); + + render(); + + expect(screen.getByText(/administrator privileges/i)).toBeDefined(); + expect(screen.queryByText('Dev box')).toBeNull(); + }); + + test('says nothing about admin rights until auth has resolved', () => { + // `role` isn't persisted across a reload, so an early refusal would flash at + // a real admin on every cold load. + mockUseAuth.mockReturnValue({ user: undefined, isLoading: true }); + + render(); + + expect(screen.queryByText(/administrator privileges/i)).toBeNull(); + }); + + test('clicking a session focuses the machine\'s Terminal tab, records the intent, and navigates', async () => { + const user = userEvent.setup(); + // The machine is parked on another tab — the case where the click used to do + // nothing at all, because only the Terminal tab mounts a workspace. + useMachineTabStore.getState().setTab('machine-1', 'code'); + render(); + + // Expand the machine to reveal its session leaves. + await user.click(await screen.findByRole('button', { name: 'Expand' })); + await user.click(await screen.findByText('agent-1')); + + expect(useMachineTabStore.getState().tabs['machine-1']).toBe('terminal'); + expect(usePendingSessionStore.getState().pending).toMatchObject({ + machineId: 'machine-1', + scope: { name: 'agent-1' }, + }); + expect(mockPush).toHaveBeenCalledWith('/dashboard/drive-1/development/machine-1'); + }); + + test('clicking the machine itself drops a stale session intent', async () => { + // Picking the machine (not one of its sessions) says "this machine as it is" + // — an older intent must not follow the user here and take over the pane. + const user = userEvent.setup(); + usePendingSessionStore.setState({ + pending: { machineId: 'machine-1', scope: { name: 'old-session' }, createdAt: Date.now() }, + }); + render(); + + await user.click(await screen.findByText('Dev box')); + + expect(usePendingSessionStore.getState().pending).toBeNull(); + expect(mockPush).toHaveBeenCalledWith('/dashboard/drive-1/development/machine-1'); + }); +}); diff --git a/apps/web/src/components/layout/middle-content/page-views/machine/MachineView.test.tsx b/apps/web/src/components/layout/middle-content/page-views/machine/MachineView.test.tsx index a486a358d5..df7525cd00 100644 --- a/apps/web/src/components/layout/middle-content/page-views/machine/MachineView.test.tsx +++ b/apps/web/src/components/layout/middle-content/page-views/machine/MachineView.test.tsx @@ -48,6 +48,7 @@ vi.mock('motion/react', () => ({ })); import MachineView from './MachineView'; +import { useMachineTabStore } from '@/stores/machine-workspace/useMachineTabStore'; const asAdmin = () => mockUseAuth.mockReturnValue({ user: { role: 'admin' } }); @@ -55,6 +56,9 @@ describe('MachineView (Machine 4-tab shell)', () => { beforeEach(() => { vi.clearAllMocks(); lifecycle.length = 0; + // The active tab now lives in a module-singleton store, so a test that + // switches tabs would otherwise leave the next one parked on that tab. + useMachineTabStore.setState({ tabs: {} }); }); test('mounts only the Terminal tab body on load — not all four', () => { From f3aee8897897d21fd77bbd8997eef9fc667883df Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 12:53:44 -0500 Subject: [PATCH 11/18] refactor(development): drop the unenforced TTL; harden the machine set, route, and tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Acting on a fresh-eyes review pass. - The session-intent TTL was a lie. resolvePendingSession only runs from an effect, so when it returned 'wait' nothing re-triggered it — there was no timer, and the doc comment ("past the TTL it is simply dropped") described behavior the system did not have. Its test passed by calling the pure function with an advanced clock and would have passed with the whole drain hook deleted. The leak it claimed to guard is already closed twice: the layout clears on unmount, and picking a machine row clears too. Deleted the TTL, createdAt, and the `now` parameter. - A machine can vanish from /api/machines WITHOUT being deleted: the per-page permission check swallows DB errors and reports "cannot view". The host treats that list as authoritative and evicts anything missing — so a transient hiccup would unmount and DISCONNECT the terminal the user is watching. Machine ids are now sticky within a drive (add-only; reset on drive change), so a live terminal can't be evicted by a blip. A genuinely deleted machine ages out of the bounded LRU instead. - GET /api/machines had no error handling: a DB failure produced an unlogged Next 500. Wrapped, logged like every sibling route. - The non-admin sidebar test would have passed with the client-side gate REMOVED (the refusal notice short-circuits the list, so no machine renders either way). It now asserts the gate itself — useDriveMachines called with null, never with the driveId — which is the actual security property. - One matchMedia listener for the sidebar instead of one per machine; corrected two comments that oversold what they defended (the index covers the scan, not the per-page permission fan-out; updatedAt is served, not ordered on). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- apps/web/src/app/api/machines/route.ts | 10 +++- .../[driveId]/development/layout.tsx | 41 +++++++++++++-- .../left-sidebar/DevelopmentSidebar.tsx | 8 ++- .../__tests__/DevelopmentSidebar.test.tsx | 33 ++++++++---- .../__tests__/pending-session.test.ts | 51 ++++--------------- .../src/lib/development/pending-session.ts | 34 +++---------- .../development/usePendingSessionStore.ts | 2 +- .../lib/src/services/machines/machine-list.ts | 8 ++- 8 files changed, 104 insertions(+), 83 deletions(-) diff --git a/apps/web/src/app/api/machines/route.ts b/apps/web/src/app/api/machines/route.ts index 57be6ea7c6..b8d035b3d2 100644 --- a/apps/web/src/app/api/machines/route.ts +++ b/apps/web/src/app/api/machines/route.ts @@ -23,6 +23,7 @@ import { NextResponse } from 'next/server'; import { authenticateRequestWithOptions, isAuthError } from '@/lib/auth'; import { auditRequest } from '@pagespace/lib/audit/audit-log'; +import { loggers } from '@pagespace/lib/logging/logger-config'; import { listDriveMachines } from '@/lib/machines/machine-list-runtime'; const AUTH_OPTIONS_READ = { allow: ['session'] as const, requireCSRF: false }; @@ -48,6 +49,11 @@ export async function GET(request: Request) { return NextResponse.json({ error: 'Machines require administrator privileges' }, { status: 403 }); } - const machines = await listDriveMachines(auth.userId, driveId); - return NextResponse.json({ machines }); + try { + const machines = await listDriveMachines(auth.userId, driveId); + return NextResponse.json({ machines }); + } catch (error) { + loggers.api.error('Error listing machines:', error as Error); + return NextResponse.json({ error: 'Failed to list machines' }, { status: 500 }); + } } diff --git a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx index c29274b246..7d325bd3f9 100644 --- a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx +++ b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx @@ -1,6 +1,6 @@ 'use client'; -import { useEffect, useMemo } from 'react'; +import { useEffect, useMemo, useRef } from 'react'; import { useParams, usePathname } from 'next/navigation'; import { Cpu } from 'lucide-react'; import MachineKeepAliveHost from '@/components/layout/middle-content/MachineKeepAliveHost'; @@ -42,7 +42,7 @@ export default function DevelopmentLayout({ children }: { children: React.ReactN // the host's source of truth for what counts as a machine (its `machineIds` // prop): the surface must not disagree with itself about which machines exist. const { machines, isLoading, error } = useDriveMachines(isAdmin ? driveId ?? null : null); - const machineIds = useMemo(() => machines.map((machine) => machine.id), [machines]); + const machineIds = useStickyMachineIds(machines, driveId); useDrainPendingSession(selectedMachineId); @@ -65,6 +65,41 @@ export default function DevelopmentLayout({ children }: { children: React.ReactN ); } +/** + * The drive's machine ids, never shrinking while you stay in the drive. + * + * The host treats this list as the set of machines that still exist, and evicts + * (unmounting, and so DISCONNECTING) anything missing from it. But a machine can + * drop out of `/api/machines` without having been deleted: the per-page + * permission check swallows a DB error and returns "cannot view", so a transient + * hiccup silently omits a live machine. Shrinking the set on that would kill the + * terminal the user is watching — the precise failure the list was introduced to + * prevent. + * + * So ids are only ever added within a drive; a genuinely deleted machine simply + * ages out of the bounded LRU instead of being evicted on sight. Changing drive + * resets the set, which is what makes eviction across drives still happen (and + * that eviction is deliberate — a PTY stream must not outlive its drive context). + */ +function useStickyMachineIds(machines: { id: string }[], driveId: string | undefined): string[] { + const seenRef = useRef<{ driveId: string | undefined; ids: string[] }>({ driveId, ids: [] }); + const fetchedIds = machines.map((machine) => machine.id).join('|'); + + return useMemo(() => { + const seen = seenRef.current; + const ids = seen.driveId === driveId ? [...seen.ids] : []; + for (const machine of machines) { + if (!ids.includes(machine.id)) ids.push(machine.id); + } + seenRef.current = { driveId, ids }; + return ids; + // `fetchedIds` (not `machines`) is the dep: SWR hands back a fresh array on + // every revalidation, and recomputing on identity alone would allocate a new + // list each time — which the host would take as a changed machine set. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [fetchedIds, driveId]); +} + /** * What the detail pane shows when the machine itself can't be. Rendered UNDER * the keep-alive host (which is `absolute inset-0 z-10` and opaque), so a state @@ -147,7 +182,7 @@ function useDrainPendingSession(selectedMachineId: string | null) { ); useEffect(() => { - const action = resolvePendingSession(pending, selectedMachineId, workspace, Date.now()); + const action = resolvePendingSession(pending, selectedMachineId, workspace); if (action.type === 'open') openTerminal(action.machineId, action.scope); // A 'clear' with no pending intent is a no-op, so this cannot loop. else if (action.type === 'clear') clearPending(); diff --git a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx index 2f25d9c320..72e0fb27d8 100644 --- a/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx +++ b/apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx @@ -88,6 +88,7 @@ export default function DevelopmentSidebar({ className }: SidebarProps) { isLoading={isLoading} error={error} selectedMachineId={selectedMachineId} + isSheetBreakpoint={isSheetBreakpoint} />
@@ -116,6 +117,7 @@ function MachineList({ isLoading, error, selectedMachineId, + isSheetBreakpoint, }: { authLoading: boolean; isAdmin: boolean; @@ -124,6 +126,7 @@ function MachineList({ isLoading: boolean; error: Error | undefined; selectedMachineId: string | null; + isSheetBreakpoint: boolean; }) { // Until auth resolves, `role` is simply unknown — saying "you're not an admin" // then would flash the refusal at an admin on every cold load. @@ -147,6 +150,7 @@ function MachineList({ machineId={machine.id} title={machine.title} selected={machine.id === selectedMachineId} + isSheetBreakpoint={isSheetBreakpoint} /> ))} @@ -170,14 +174,16 @@ function MachineTreeSection({ machineId, title, selected, + isSheetBreakpoint, }: { driveId: string; machineId: string; title: string; selected: boolean; + /** Passed down rather than re-derived: one matchMedia listener for the sidebar, not one per machine. */ + isSheetBreakpoint: boolean; }) { const router = useRouter(); - const isSheetBreakpoint = useBreakpoint('(max-width: 1023px)'); const setLeftSheetOpen = useLayoutStore((state) => state.setLeftSheetOpen); const requestSession = usePendingSessionStore((state) => state.requestSession); const clearPending = usePendingSessionStore((state) => state.clearPending); diff --git a/apps/web/src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx b/apps/web/src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx index 65923ab812..58e7ecb9f8 100644 --- a/apps/web/src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx +++ b/apps/web/src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx @@ -23,14 +23,19 @@ vi.mock('next/navigation', () => ({ vi.mock('@/hooks/useAuth', () => ({ useAuth: () => mockUseAuth() })); +// Spied, not just stubbed: a null driveId is HOW the sidebar refuses to fetch for +// a non-admin, so the argument is the security property — asserting only that no +// machine renders would still pass if the gate were dropped, since the refusal +// notice short-circuits the list anyway. +const mockUseDriveMachines = vi.fn((driveId: string | null) => ({ + machines: driveId ? [{ id: 'machine-1', title: 'Dev box', updatedAt: '2026-07-12T00:00:00.000Z' }] : [], + isLoading: false, + error: undefined, + mutate: vi.fn(), +})); + vi.mock('@/hooks/useDriveMachines', () => ({ - useDriveMachines: (driveId: string | null) => ({ - // A null driveId is how the sidebar refuses to fetch for a non-admin. - machines: driveId ? [{ id: 'machine-1', title: 'Dev box', updatedAt: '2026-07-12T00:00:00.000Z' }] : [], - isLoading: false, - error: undefined, - mutate: vi.fn(), - }), + useDriveMachines: (driveId: string | null) => mockUseDriveMachines(driveId), })); vi.mock('@/hooks/useMachineProjects', () => ({ @@ -76,13 +81,23 @@ describe('DevelopmentSidebar', () => { expect(await screen.findByText('Dev box')).toBeDefined(); }); - test('refuses a non-admin, and asks for no machines on their behalf', () => { + test('refuses a non-admin, and asks the API for no machines on their behalf', () => { mockUseAuth.mockReturnValue({ user: { role: 'user' }, isLoading: false }); render(); expect(screen.getByText(/administrator privileges/i)).toBeDefined(); expect(screen.queryByText('Dev box')).toBeNull(); + // The load-bearing half: the request is never made, rather than made and + // discarded. (The server rejects a non-admin too — this is the client half.) + expect(mockUseDriveMachines).toHaveBeenCalledWith(null); + expect(mockUseDriveMachines).not.toHaveBeenCalledWith('drive-1'); + }); + + test('an admin does fetch the drive\'s machines', () => { + render(); + + expect(mockUseDriveMachines).toHaveBeenCalledWith('drive-1'); }); test('says nothing about admin rights until auth has resolved', () => { @@ -119,7 +134,7 @@ describe('DevelopmentSidebar', () => { // — an older intent must not follow the user here and take over the pane. const user = userEvent.setup(); usePendingSessionStore.setState({ - pending: { machineId: 'machine-1', scope: { name: 'old-session' }, createdAt: Date.now() }, + pending: { machineId: 'machine-1', scope: { name: 'old-session' } }, }); render(); diff --git a/apps/web/src/lib/development/__tests__/pending-session.test.ts b/apps/web/src/lib/development/__tests__/pending-session.test.ts index 2661229944..bda11f752e 100644 --- a/apps/web/src/lib/development/__tests__/pending-session.test.ts +++ b/apps/web/src/lib/development/__tests__/pending-session.test.ts @@ -1,14 +1,9 @@ import { describe, test, expect } from 'vitest'; import type { WorkspaceState } from '@/stores/machine-workspace/useMachineWorkspaceStore'; -import { - resolvePendingSession, - PENDING_SESSION_TTL_MS, - type PendingSession, -} from '../pending-session'; +import { resolvePendingSession, type PendingSession } from '../pending-session'; -const NOW = 1_000_000; const SCOPE = { projectName: 'repo', branchName: 'main', name: 'agent-1' }; -const PENDING: PendingSession = { machineId: 'machine-1', scope: SCOPE, createdAt: NOW }; +const PENDING: PendingSession = { machineId: 'machine-1', scope: SCOPE }; /** A workspace whose active pane holds `scope` (null = a fresh, empty pane). */ const workspaceWith = (scope: WorkspaceState['columns'][number]['panes'][number]['scope']): WorkspaceState => ({ @@ -18,7 +13,7 @@ const workspaceWith = (scope: WorkspaceState['columns'][number]['panes'][number] describe('resolvePendingSession', () => { test('opens the session once the user is on the machine and it has a workspace', () => { - expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(null), NOW)).toEqual({ + expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(null))).toEqual({ type: 'open', machineId: 'machine-1', scope: SCOPE, @@ -32,18 +27,18 @@ describe('resolvePendingSession', () => { // first, so there is an intermediate commit holding the new intent and the // OLD pathname. Reading that as "the user navigated away" threw the intent // away before the navigation it was waiting for ever arrived. - expect(resolvePendingSession(PENDING, 'machine-9', undefined, NOW)).toEqual({ type: 'wait' }); + expect(resolvePendingSession(PENDING, 'machine-9', undefined)).toEqual({ type: 'wait' }); }); test('holds until the machine\'s pane region has mounted', () => { - expect(resolvePendingSession(PENDING, 'machine-1', undefined, NOW)).toEqual({ type: 'wait' }); + expect(resolvePendingSession(PENDING, 'machine-1', undefined)).toEqual({ type: 'wait' }); }); test('re-opens against a workspace that was torn down and rebuilt', () => { // MachineWorkspace disposes on unmount and re-creates on mount (StrictMode // double-invokes exactly this on the first visit). A fire-once intent would // be destroyed by the rebuild; a convergent one re-applies to the new one. - expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(null), NOW)).toEqual({ + expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(null))).toEqual({ type: 'open', machineId: 'machine-1', scope: SCOPE, @@ -51,45 +46,21 @@ describe('resolvePendingSession', () => { }); test('clears once the session is actually in the active pane', () => { - expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(SCOPE), NOW)).toEqual({ type: 'clear' }); - }); - - test('expires rather than lying in wait to hijack a pane later', () => { - // The leak this closes: an intent that never converged (the machine never - // mounted, or the user turned back) used to be held indefinitely in a - // module-level store. Returning to that machine much later — warm, with a - // terminal running in its active pane — would fire the stale intent and - // overwrite that pane. Past the TTL it is simply dropped. - const stale = resolvePendingSession( - PENDING, - 'machine-1', - workspaceWith({ name: 'something-the-user-is-using' }), - NOW + PENDING_SESSION_TTL_MS + 1, - ); - - expect(stale).toEqual({ type: 'clear' }); - }); - - test('a slow-but-live navigation is not expired', () => { - expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(null), NOW + PENDING_SESSION_TTL_MS - 1)).toEqual({ - type: 'open', - machineId: 'machine-1', - scope: SCOPE, - }); + expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(SCOPE))).toEqual({ type: 'clear' }); }); test('a satisfied intent is dropped, so it cannot clobber the user\'s next pane change', () => { - expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(SCOPE), NOW)).toEqual({ type: 'clear' }); + expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(SCOPE))).toEqual({ type: 'clear' }); // And with no intent left, nothing is ever re-applied. expect( - resolvePendingSession(null, 'machine-1', workspaceWith({ ...SCOPE, name: 'agent-2' }), NOW), + resolvePendingSession(null, 'machine-1', workspaceWith({ ...SCOPE, name: 'agent-2' })), ).toEqual({ type: 'clear' }); }); test('distinguishes same-named sessions at different scopes', () => { // A machine-scope "agent-1" is not the branch-scope "agent-1"; treating them // as the same would report the intent satisfied by the wrong session. - expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith({ name: 'agent-1' }), NOW)).toEqual({ + expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith({ name: 'agent-1' }))).toEqual({ type: 'open', machineId: 'machine-1', scope: SCOPE, @@ -97,6 +68,6 @@ describe('resolvePendingSession', () => { }); test('no intent is a no-op', () => { - expect(resolvePendingSession(null, 'machine-1', undefined, NOW)).toEqual({ type: 'clear' }); + expect(resolvePendingSession(null, 'machine-1', undefined)).toEqual({ type: 'clear' }); }); }); diff --git a/apps/web/src/lib/development/pending-session.ts b/apps/web/src/lib/development/pending-session.ts index 3e9d36610e..a8de391a0c 100644 --- a/apps/web/src/lib/development/pending-session.ts +++ b/apps/web/src/lib/development/pending-session.ts @@ -1,23 +1,9 @@ import type { OpenTerminalScope, WorkspaceState } from '@/stores/machine-workspace/useMachineWorkspaceStore'; -/** - * How long a session-open intent stays live. - * - * The navigation it accompanies commits in milliseconds, and the machine's pane - * region mounts within a second or so (`MachineWorkspace` is a dynamic import). - * This is the backstop for an intent that never converges — the machine failed - * to mount, or the user turned back mid-flight — so it cannot lie in wait and - * later hijack the active pane of a machine they've since returned to. Generous - * on purpose: it must never expire a navigation that is merely slow. - */ -export const PENDING_SESSION_TTL_MS = 30_000; - /** A session the user clicked in the Development sidebar, to be opened on the machine it belongs to. */ export interface PendingSession { machineId: string; scope: OpenTerminalScope; - /** When the click happened — the intent expires `PENDING_SESSION_TTL_MS` later. */ - createdAt: number; } export type PendingSessionAction = @@ -71,28 +57,24 @@ function activePaneScope(workspace: WorkspaceState): OpenTerminalScope | null { * the old pathname. An earlier version treated that commit as "navigated * away" and dropped the intent before its own navigation landed — which * silently broke every session click on a machine the user wasn't already - * viewing. So a mismatch WAITS, and staleness is bounded by time - * ({@link PENDING_SESSION_TTL_MS}) instead of by guessing at intent. The - * sidebar additionally clears the intent when the user picks a different - * machine outright, so the TTL is only ever the backstop. + * viewing. So a mismatch WAITS. * - * Once satisfied it is cleared, so the user's own later pane changes on that - * machine are never clobbered by a stale intent. + * An intent that never converges is therefore held — but it cannot outlive its + * usefulness, because the two things that would make it stale both clear it + * outright: leaving the surface (the layout clears on unmount) and picking a + * machine ROW rather than one of its sessions (the sidebar clears, since that + * says "this machine as it is"). Once satisfied it clears too, so the user's own + * later pane changes are never clobbered. */ export function resolvePendingSession( pending: PendingSession | null, selectedMachineId: string | null, workspace: WorkspaceState | undefined, - now: number, ): PendingSessionAction { if (!pending) return { type: 'clear' }; - // Never converged (machine never mounted, user turned back). Drop it rather - // than let it fire into whatever pane is active whenever they next arrive. - if (now - pending.createdAt > PENDING_SESSION_TTL_MS) return { type: 'clear' }; - // Not there yet: either the click's own navigation hasn't committed, or the - // user is en route elsewhere. Holding is safe — the TTL bounds it. + // user is en route. Holding is safe — see the note above on what clears it. if (pending.machineId !== selectedMachineId) return { type: 'wait' }; // On the machine, but its pane region hasn't mounted (and ensured a workspace). diff --git a/apps/web/src/stores/development/usePendingSessionStore.ts b/apps/web/src/stores/development/usePendingSessionStore.ts index 25ea8f90a9..5eb97ffd61 100644 --- a/apps/web/src/stores/development/usePendingSessionStore.ts +++ b/apps/web/src/stores/development/usePendingSessionStore.ts @@ -22,7 +22,7 @@ interface PendingSessionStoreState { export const usePendingSessionStore = create((set) => ({ pending: null, - requestSession: (machineId, scope) => set({ pending: { machineId, scope, createdAt: Date.now() } }), + requestSession: (machineId, scope) => set({ pending: { machineId, scope } }), // Identity-stable when there's nothing to clear, so a no-op clear can't // re-render (and so the drain effect can call it unconditionally). clearPending: () => set((state) => (state.pending === null ? state : { pending: null })), diff --git a/packages/lib/src/services/machines/machine-list.ts b/packages/lib/src/services/machines/machine-list.ts index 58cc3ba0ef..11ab6151e4 100644 --- a/packages/lib/src/services/machines/machine-list.ts +++ b/packages/lib/src/services/machines/machine-list.ts @@ -13,7 +13,7 @@ export interface MachinePageSummary { id: string; title: string; - /** ISO-8601. Callers order by it or show it; the service itself preserves the scan's order. */ + /** ISO-8601. Served for callers that want recency (the tree itself orders by title). */ updatedAt: string; } @@ -31,6 +31,12 @@ export interface MachineListDeps { * drive member. Every candidate is therefore re-checked against * `canUserViewPage` here, which is the same view-level gate every other machine * route applies before serving a machine's projects/branches/sessions. + * + * That check is per page, so this fans out N permission lookups for N machines. + * Fine at this scale — the surface is app-admin-only, machines are heavyweight + * things a drive has a handful of, and an owner/admin short-circuits early. If a + * drive ever holds enough machines for it to matter, resolve drive membership + * once and fall back to the per-page check only for the non-owner case. */ export async function listMachinesInDrive( deps: MachineListDeps, From 133d46b0d2d2dd9f8b6fd3fc5e58d5e008d67209 Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 12:55:48 -0500 Subject: [PATCH 12/18] refactor(development): derive the sticky machine set with the repo's own render pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit useStickyMachineIds mutated a ref during render. It happened to be safe (the union is monotonic and idempotent), but it is an impure render, and the codebase already has a sanctioned idiom for exactly this shape: MachineKeepAliveHost derives its LRU with the "adjust state during render" pattern guarded by a key. Matched it — state, unlike a ref, is discarded when a concurrent render is abandoned, so an interrupted navigation cannot leave behind a machine set that was never committed. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../[driveId]/development/layout.tsx | Bin 8627 -> 8794 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx index 7d325bd3f9a383166eaa6529f0c6bfc687dd7ffe..a8f4b5a8bcd59ac447978d76fd1029670b9ff84d 100644 GIT binary patch delta 755 zcmX|9!EO^V5Jf^xVNN_E$h#3b=X_Uu-xC`~^B zDV+HSREZzp!Z&c_Kad#PDJOfz^WMC9p4)f7kG?#6aK?)1`^)RSSDg+{sV0rWf+vI# zaXyjIXpg2XI3|^Is+JU4HeBP+&DYJhhmY4gfWXtEz^Niv!%?MXMk_XjZ9!Qb8MHGM zWjLa6YFrtf#JpE?N;C&be=a@42WilyGj=RwR+BrxLNSxG_@MHMv2;qbcspAQ)o~$^ z(v!!ia4_ug{^O@^VhrO{6XTL( ztl*SlTqzG5yhL8i=SBwCnINRUj2hXHRzPx}1;Z($k1NpjevzO)HivMnF_Qh9eDV3=i@6x^4J&CmV%cq_LVCARMh g+i@vF=OEJ&m$DJU9^zF{vrTPV$;<7JM}K$!0gY7qW&i*H delta 567 zcmY*Wy-or_5H3tCh>?gUT2M@7AlyL@VxtX(4J{#IxgFRI$1b^@<0k|$K7q+u`Vxk> zu`<4giF0rlVs|_9{mpk@kGqGr+p{N}S`T7}T^xPZo{KkuHIBd`8lFHqTivabO|hJu zmab|bBR$8C^gv73gkB#u)m8!EKpY_j3S~z+<|eNfjc%*J%LplsOe&TdRw`9!-@}?0 zIWIM2M&?@&;wVBxscw#7w+xW1OXU;BCm*HNaGx{BGhqEMA8Za1nLleB-i-zfj!_3t zF_vI36@k(Q-JlBE0VxEDvu5&LyxxL{BC!H%_KBE|bBtDe*Ia~J3ewhiAdKWmV5hT~ zv>7v+JK{4mtw4m Date: Sun, 12 Jul 2026 13:11:17 -0500 Subject: [PATCH 13/18] fix(development): a vanished machine stops being shown without evicting it; test the layout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sticky machine set I added to stop a fetch blip disconnecting a live terminal over-corrected: because it never shrank, a DELETED machine stayed mountable and "Machine not found" became unreachable — the user would sit on a permanent "Opening machine…" over a MachineView whose own API calls were 404ing. Those are two different questions and they now get two different answers. What is DISPLAYED comes from the latest fetch, so a machine that's gone stops being shown at once. What may stay MOUNTED comes from the sticky set, so a machine that drops out of a fetch without being deleted (the per-page permission check swallows DB errors and reports "cannot view") keeps its terminal alive, hidden, until the bounded LRU ages it out. A blip now costs a transient notice, never a dead session. Also adds the layout's first test file. It's the newest and most delicate code on the branch — setState-during-render, the error-before-not-found ordering, the unmount clear — and every review pass kept finding bugs in it while every piece it composes was already tested. The tests pin what actually broke: the sticky set converges (a key derived from array identity rather than contents would loop forever, since SWR hands back a fresh arrayevery render), its identity is stable across a no-op revalidation (a new identity reads as a changed machine set, i.e. LRU eviction, i.e. terminal teardown), a failed fetch says "failed" rather than "deleted", and a vanished machine is un-displayed but not evicted. Corrects the route comment that claimed a system-wide property: the sibling machines/* routes are view-gated, not admin-gated, so this route is stricter than they are rather than closing a hole they leave open. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- apps/web/src/app/api/machines/route.ts | 10 +- .../development/__tests__/layout.test.tsx | 137 ++++++++++++++++++ .../[driveId]/development/layout.tsx | Bin 8794 -> 9709 bytes 3 files changed, 144 insertions(+), 3 deletions(-) create mode 100644 apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx diff --git a/apps/web/src/app/api/machines/route.ts b/apps/web/src/app/api/machines/route.ts index b8d035b3d2..87c7baaa12 100644 --- a/apps/web/src/app/api/machines/route.ts +++ b/apps/web/src/app/api/machines/route.ts @@ -11,9 +11,13 @@ * App-admin only, matching the rest of the Machine feature: creating a MACHINE * page requires `admin` (see POST /api/pages) and `MachineView` refuses to mount * its tabs for anyone else. Without this, a non-admin drive member who can VIEW a - * Machine page could enumerate the drive's machines from the Development surface - * and, through the tree, their projects/branches/terminal sessions — structure - * the Machine page deliberately withholds from them. + * Machine page could enumerate the drive's machines from the Development surface. + * + * Note this route is STRICTER than its siblings, not a system-wide guarantee: + * /api/machines/{projects,branches,agent-terminals} gate on `canViewMachine`, not + * on admin, so a non-admin with view access on a Machine page can still call them + * directly. This surface simply declines to be the thing that hands them the list + * of machines to call them with. * * Admin is necessary but not sufficient: the list is still filtered per page * through `canUserViewPage`, so a Machine withheld from this admin by a diff --git a/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx b/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx new file mode 100644 index 0000000000..62401107d1 --- /dev/null +++ b/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx @@ -0,0 +1,137 @@ +/** + * The Development surface's detail region — the composition, not its parts. + * + * Every piece this layout uses is unit-tested; what wasn't was how they fit + * together, and that's where six review passes kept finding bugs. So these tests + * pin the three properties that were actually broken at some point: + * - the sticky machine set converges instead of re-rendering forever, and + * keeps its identity so the keep-alive host doesn't churn its LRU; + * - a failed fetch says "failed", not "your machine was deleted"; + * - a machine that vanishes stops being DISPLAYED but is not evicted (which + * would disconnect its terminal). + */ +import { describe, test, expect, beforeEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; + +const mockUseAuth = vi.fn(); +const mockUseDriveMachines = vi.fn(); +/** Every render of the stubbed keep-alive host, so we can assert on what it was handed. */ +const hostRenders: { activePageId: string | null; machineIds: readonly string[] }[] = []; + +vi.mock('next/navigation', () => ({ + useParams: () => ({ driveId: 'drive-1' }), + usePathname: () => '/dashboard/drive-1/development/machine-1', +})); + +vi.mock('@/hooks/useAuth', () => ({ useAuth: () => mockUseAuth() })); +vi.mock('@/hooks/useDriveMachines', () => ({ useDriveMachines: () => mockUseDriveMachines() })); + +vi.mock('@/components/layout/middle-content/MachineKeepAliveHost', () => ({ + default: (props: { activePageId: string | null; machineIds: readonly string[] }) => { + hostRenders.push({ activePageId: props.activePageId, machineIds: props.machineIds }); + return
; + }, +})); + +import DevelopmentLayout from '../layout'; +import { usePendingSessionStore } from '@/stores/development/usePendingSessionStore'; + +const machine = (id: string) => ({ id, title: id, updatedAt: '2026-07-12T00:00:00.000Z' }); + +const driveMachines = (over: Partial<{ machines: ReturnType[]; isLoading: boolean; error: Error | undefined }> = {}) => ({ + machines: over.machines ?? [machine('machine-1')], + isLoading: over.isLoading ?? false, + error: over.error, + mutate: vi.fn(), +}); + +beforeEach(() => { + vi.clearAllMocks(); + hostRenders.length = 0; + usePendingSessionStore.setState({ pending: null }); + mockUseAuth.mockReturnValue({ user: { role: 'admin' }, isLoading: false }); + mockUseDriveMachines.mockReturnValue(driveMachines()); +}); + +describe('DevelopmentLayout', () => { + test('hands the selected machine and its drive\'s machines to the keep-alive host', () => { + render({null}); + + expect(hostRenders.at(-1)).toEqual({ activePageId: 'machine-1', machineIds: ['machine-1'] }); + }); + + test('settles instead of re-rendering forever', () => { + // useStickyMachineIds sets state DURING render. If its key were derived from + // the array's identity rather than its contents it would loop, because SWR + // hands back a fresh array every render. + render({null}); + + expect(hostRenders.length).toBeLessThanOrEqual(3); + }); + + test('keeps the machine-id list stable across a revalidation that changes nothing', () => { + // A new array identity each render would look to the host like a changed + // machine set, which is what drives LRU eviction — i.e. terminal teardown. + const { rerender } = render({null}); + const first = hostRenders.at(-1)!.machineIds; + + mockUseDriveMachines.mockReturnValue(driveMachines({ machines: [machine('machine-1')] })); + rerender({null}); + + expect(hostRenders.at(-1)!.machineIds).toBe(first); + }); + + test('a failed fetch reports the failure — it does not claim the machine is gone', () => { + // SWR reports isLoading:false with no data on the error path, which is + // indistinguishable from "no such machine" unless error is checked first. + mockUseDriveMachines.mockReturnValue(driveMachines({ machines: [], error: new Error('boom') })); + + render({null}); + + expect(screen.getByText(/failed to load machines/i)).toBeDefined(); + expect(screen.queryByText(/machine not found/i)).toBeNull(); + }); + + test('a machine that vanishes stops being shown, but is NOT evicted', () => { + // It may have been deleted — or the per-page permission check may have + // swallowed a DB error and reported "cannot view". Stop DISPLAYING it either + // way, but keep it in the mountable set: evicting it would unmount MachineView + // and disconnect a terminal that might be perfectly alive. + render({null}); + cleanup(); + hostRenders.length = 0; + + mockUseDriveMachines.mockReturnValue(driveMachines({ machines: [] })); + render({null}); + + expect(screen.getByText(/machine not found/i)).toBeDefined(); + expect(hostRenders.at(-1)!.activePageId).toBeNull(); + }); + + test('says nothing about admin rights until auth resolves', () => { + mockUseAuth.mockReturnValue({ user: undefined, isLoading: true }); + + render({null}); + + expect(screen.queryByText(/administrator privileges/i)).toBeNull(); + }); + + test('refuses a non-admin', () => { + mockUseAuth.mockReturnValue({ user: { role: 'user' }, isLoading: false }); + + render({null}); + + expect(screen.getByText(/administrator privileges/i)).toBeDefined(); + }); + + test('drops an unconverged session intent when the surface is left', () => { + // The store is a module singleton: an intent left behind would still be + // sitting there on the next visit, ready to fire into whatever pane is active. + usePendingSessionStore.setState({ pending: { machineId: 'machine-9', scope: { name: 'agent-1' } } }); + const { unmount } = render({null}); + + unmount(); + + expect(usePendingSessionStore.getState().pending).toBeNull(); + }); +}); diff --git a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx index a8f4b5a8bcd59ac447978d76fd1029670b9ff84d..07adf8b6cbf70fa5387cf3b20eb6d3c691f44e97 100644 GIT binary patch delta 1786 zcmZ8iU27Xh6s1qCoz#K$rO*;?RETAvD5Wm}gKBI`6ycAANT%eWkkQVSckt|tGBayM zI7R&r>JR91=|ds^rjLDZe?!mAu4K!8Fk*M+-gD16_w3)je^36p+4|H6m0iuxmLIlW zES^87(ae%mlLvj_ty9}g^d@&_!&D-|NQzJW!6lz2+CYi$hjcpX_kvXg^;{W zgrdR_3#S!3y2dROq_GV-T`8?et81pfu22gjw5P-K9vQ|tay+#zkRZvL152`oJE>w- zw$Zcf(Gv{|k>$!T0WC`Md*#DHBOTbpcn%L6F)T%U&@j{GsjglHabrUK1usTUH`d&p@xi?qAan51eH2in}_erRvWOSI9oFv{l;By#3! z1T7StYB6WoB2^SDl?bfoZ8$_}Mq^)rS?izpfq{f8sF^D%+3XR>Qo54w^#zGbGy&u| zXY}7{_OVt|n9;9mLT*s!a3DDNKt)GGGL><=c38BQkYL(JQzqFkK6F?9#6a$q&|MtS zAcbZ9WFYBDw@apk+nzpME8Ow6;AXSfqwWt>bd;nnPHo=(_{Y}plby{5%WofizW@`} zpLAG>f$ZMYKYm%i5m~^iS70E%-mF0f_Zs00?I(!01#15cr0?F;n_qc(AV2fK=`yaItGD-3@HN@uvH0IB0ugFjGDyB!F}2}4X0t{f~70w={B|K?B&JzsNd_* zp_I?8i4K4fvCw*+Jd486BF+tywYF)uMI%&B6>FnG=I^TGPkxAuCOU(xpQZ zMt(K&3p)Uto8YW0vx?2BLoS+7QPiTNluIHpx&V<9acFj9Xu<9>5?bfxN=88QnS+q~ zRYIc^xO{bb_@>{B3`J!*Q}Q-0k?Z&Z)Zvn1@KI<}we9UY+dhuj3NWXyFJieni{GK+T$szqcjd(|9K!+eYf$+S!La;q X=bLSyQ)r0m%biExeLQ&d^_TwxxyWJQ delta 933 zcmZWnJ#Q015LEc!04Y{1aneJK#5RY8@l3;>aOYY?F_URFi$g;_AqRzV&$!|#&J@vJzI}&j77|$LSUcso1Q)m{ zst{SsD8+FOQe{W6RIx@x&#=eZC!J`xB(0=~vQ5+&gcXhfib}i_SvAXrg;F)0vZY;k a=n4RrpPF~BM){Usn-4cy=RcdbZ~X)K6+=(} From 7fec2ad962c75e30c2b6ee266acd407f954cccaa Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 13:19:35 -0500 Subject: [PATCH 14/18] fix(development): make the machine list actually recover; fix a test that proved the opposite of its name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in my own previous commit, both caught by review. 1. I claimed a fetch blip "costs a transient notice". Nothing made it transient: useDriveMachines had revalidateOnFocus:false, no refreshInterval, and its mutate is never called — so the list was fetched once per mount and never again. A machine silently dropped by the swallowed-permission-error path would therefore stay hidden for the rest of the session, and both ways out (reload, or leave the surface and return) unmount the keep-alive host and disconnect every warm terminal — destroying the very thing the sticky set exists to protect. The list now polls, so it recovers on its own (and picks up machines created elsewhere). SWR keeps the previous array identity when the ids are unchanged, so a poll that changes nothing doesn't churn the LRU. 2. The "a machine that vanishes is NOT evicted" test used cleanup() + render, which builds a FRESH component whose sticky set is rebuilt from the now-empty fetch — so the machine WAS evicted, and the test asserted only activePageId. It would have passed with useStickyMachineIds deleted entirely. It now rerenders the same instance and asserts the machine is still in the mountable set, which is the property it names. (The production code was right; the test was not.) Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../development/__tests__/layout.test.tsx | 16 +++++++++++----- .../[driveId]/development/layout.tsx | Bin 9709 -> 9978 bytes apps/web/src/hooks/useDriveMachines.ts | 14 +++++++++++++- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx b/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx index 62401107d1..cef1f4f083 100644 --- a/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx +++ b/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx @@ -11,7 +11,7 @@ * would disconnect its terminal). */ import { describe, test, expect, beforeEach, vi } from 'vitest'; -import { render, screen, cleanup } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; const mockUseAuth = vi.fn(); const mockUseDriveMachines = vi.fn(); @@ -97,15 +97,21 @@ describe('DevelopmentLayout', () => { // swallowed a DB error and reported "cannot view". Stop DISPLAYING it either // way, but keep it in the mountable set: evicting it would unmount MachineView // and disconnect a terminal that might be perfectly alive. - render({null}); - cleanup(); - hostRenders.length = 0; + // + // Must be a rerender, NOT cleanup() + render: a fresh mount rebuilds the + // sticky set from the (now empty) fetch, which would evict the machine and + // make this test pass while proving the opposite of its name. + const { rerender } = render({null}); + expect(hostRenders.at(-1)!.machineIds).toContain('machine-1'); mockUseDriveMachines.mockReturnValue(driveMachines({ machines: [] })); - render({null}); + rerender({null}); expect(screen.getByText(/machine not found/i)).toBeDefined(); + // Not displayed… expect(hostRenders.at(-1)!.activePageId).toBeNull(); + // …but still mountable, so its terminal is not torn down. + expect(hostRenders.at(-1)!.machineIds).toContain('machine-1'); }); test('says nothing about admin rights until auth resolves', () => { diff --git a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx index 07adf8b6cbf70fa5387cf3b20eb6d3c691f44e97..e7d719924757c9a227255bdc1c7afccb1cd3df44 100644 GIT binary patch delta 362 zcmYk2Jx&BM3`Tpj7oht}kVb4zheU(WR@6xAnRtjrCRxSFj<#A3!9kGrM)X{O3eQhL z6D7*$=V$*s|L(qcbH2j^dM=$u8{}{WY_}+$GxsSox}-&?H>z?5A*Lc7H;8TeuE(}EDV_T+&{Axq<+4yenv96HWiPNPKv3+t2r-*nL{Gi!0H2& s5vOqGQu8ucjYFz&auS8Nz084k2?5hCPR7i`IxOcwB8A(_WpjW12lM}c5C8xG delta 91 zcmez6`__9yDvzJSqlPI8i3(|{CCM2INjaGX3d#A!CB<9{3i|pAB^jv-rNya5Km{d5 qiFw7Dsd*&|dHE%o$*DRDd8uV!nUvJT6oul{;^NHwyv+xAtt0^^s3BMY diff --git a/apps/web/src/hooks/useDriveMachines.ts b/apps/web/src/hooks/useDriveMachines.ts index 1220c9e356..04a746cbb9 100644 --- a/apps/web/src/hooks/useDriveMachines.ts +++ b/apps/web/src/hooks/useDriveMachines.ts @@ -26,8 +26,20 @@ const fetcher = (url: string) => export function useDriveMachines(driveId: string | null) { const key = driveId ? `/api/machines?driveId=${encodeURIComponent(driveId)}` : null; + // This list must be able to RECOVER, which is why it revalidates at all (the + // sibling machine hooks don't). A machine can drop out of it without having + // been deleted — the per-page permission check swallows DB errors and reports + // "cannot view" — and the surface responds by hiding that machine. Fetched + // once and never again, a single blip would hide a live machine for the rest + // of the session, and the only ways out (reload, or leave and return) both + // unmount the keep-alive host and disconnect every warm terminal. + // + // It also means a machine created elsewhere shows up without a reload. Cheap: + // one indexed query, on an admin-only surface, and SWR keeps the previous + // array identity when the ids are unchanged — so a poll that changes nothing + // doesn't churn the keep-alive LRU. const { data, error, isLoading, mutate } = useSWR(key, fetcher, { - revalidateOnFocus: false, + refreshInterval: 30_000, }); return { From 3ee53c502cb4efdcdd94beae4da20e1446084f39 Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 13:28:56 -0500 Subject: [PATCH 15/18] fix(development): a failed poll must not tear down a working machine list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression from the previous commit, caught in review. Adding the poll changed what `error` MEANS: SWR keeps the last good data and sets `error` on a failed REVALIDATION, whereas before (fetch-once) an error implied no data. Both surfaces still checked `error` ahead of the data, so a single blip of a background poll would replace the whole sidebar tree with "Failed to load machines" — losing every machine's expansion state and the session leaves under it — while the app was holding a perfectly good list. And SWR suppresses the refresh interval while an error is set, so it sat there through the retry backoff rather than recovering. The error notice is now shown only when the failure left nothing to show. Pinned by tests on both surfaces (stale data + error → the machine still renders, no error notice). Also corrects the hook comment, which named the wrong mechanism: SWR preserves the array identity only when the whole payload is deep-equal, and `updatedAt` moves whenever a Machine page is touched — so a poll DOES hand back a fresh array. What actually keeps it from churning the keep-alive LRU is that both consumers key on the IDS alone. And notes that dropping `revalidateOnFocus: false` was deliberate: returning to the tab now recovers immediately instead of waiting out the interval. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../development/__tests__/layout.test.tsx | 12 ++++++ .../[driveId]/development/layout.tsx | Bin 9978 -> 10117 bytes .../left-sidebar/DevelopmentSidebar.tsx | 7 +++- .../__tests__/DevelopmentSidebar.test.tsx | 39 +++++++++++++++--- apps/web/src/hooks/useDriveMachines.ts | 15 +++++-- 5 files changed, 62 insertions(+), 11 deletions(-) diff --git a/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx b/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx index cef1f4f083..bcb67b5dda 100644 --- a/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx +++ b/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx @@ -92,6 +92,18 @@ describe('DevelopmentLayout', () => { expect(screen.queryByText(/machine not found/i)).toBeNull(); }); + test('a failed POLL does not blank out a machine we can still show', () => { + // The list polls, and SWR keeps the last good data while setting `error` on a + // failed revalidation. Reporting the error ahead of the data would let one + // blip of a background poll replace a working machine with an error notice. + mockUseDriveMachines.mockReturnValue(driveMachines({ machines: [machine('machine-1')], error: new Error('blip') })); + + render({null}); + + expect(screen.queryByText(/failed to load machines/i)).toBeNull(); + expect(hostRenders.at(-1)!.activePageId).toBe('machine-1'); + }); + test('a machine that vanishes stops being shown, but is NOT evicted', () => { // It may have been deleted — or the per-page permission check may have // swallowed a DB error and reported "cannot view". Stop DISPLAYING it either diff --git a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx index e7d719924757c9a227255bdc1c7afccb1cd3df44..7a372d85957d879d61673b6afdede2e7bc61f5a9 100644 GIT binary patch delta 312 zcmX|+Jx;?w5QSUd2FO@?N)afmq@be$IwXE1f}qXBJC0Z0%vjzv78Nv{!1P=JiF+V% zh1`LLicKt0zc=6a=J)FL=3||tXC;$?78r>J6xxazWms@dt>^wH$@cev=@iyqDj6$F z6y!W({qokI9^5|{w(((t^EN=mJ)tXE5K6|1^3n?9Vs?Fahf1`QlWpLv4+FDS=X?ed zgW1C^D(2b`74iW~RRcqTBGWpC=MXmjIX8_q^QZ$MFDPba;5Gb6&c+j~lEG`t7tYpL wwGp>@Z{bKPXxvKw`#MHW0w1h%@TGQ>2(tx_wi=Ab=)cqB{yo|G%+jyjKe^{?k^lez delta 200 zcmXAhF=|3V6h&*(S{lj4NG9lZj%Y=RPLKpEbugXR!==@KfsY?j7$4S8z;Fm#RFQpboiAN>hw=0B zXYxM(TnzBWlyZmUe1xc>Vyki+q%x=@HD*VzkEbuHWtplS)*=R*qI5nGt}s}U0frS5 b%^=s_qDxiL9=Opening Development…; - if (error) return Failed to load machines; + // Only when the failure left us with NOTHING to show. The list polls, and SWR + // keeps the last good data while setting `error` on a failed revalidation — so + // reporting the error ahead of the data would let one blip of a background poll + // tear down the whole tree (losing every expansion and its session leaves) while + // the app still holds a perfectly good list. + if (error && machines.length === 0) return Failed to load machines; if (isLoading) return Loading…; if (machines.length === 0) return No machines in this drive yet; diff --git a/apps/web/src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx b/apps/web/src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx index 58e7ecb9f8..b0cac1e618 100644 --- a/apps/web/src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx +++ b/apps/web/src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx @@ -27,12 +27,21 @@ vi.mock('@/hooks/useAuth', () => ({ useAuth: () => mockUseAuth() })); // a non-admin, so the argument is the security property — asserting only that no // machine renders would still pass if the gate were dropped, since the refusal // notice short-circuits the list anyway. -const mockUseDriveMachines = vi.fn((driveId: string | null) => ({ - machines: driveId ? [{ id: 'machine-1', title: 'Dev box', updatedAt: '2026-07-12T00:00:00.000Z' }] : [], - isLoading: false, - error: undefined, - mutate: vi.fn(), -})); +interface DriveMachinesResult { + machines: { id: string; title: string; updatedAt: string }[]; + isLoading: boolean; + error: Error | undefined; + mutate: () => void; +} + +const mockUseDriveMachines = vi.fn( + (driveId: string | null): DriveMachinesResult => ({ + machines: driveId ? [{ id: 'machine-1', title: 'Dev box', updatedAt: '2026-07-12T00:00:00.000Z' }] : [], + isLoading: false, + error: undefined, + mutate: vi.fn(), + }), +); vi.mock('@/hooks/useDriveMachines', () => ({ useDriveMachines: (driveId: string | null) => mockUseDriveMachines(driveId), @@ -100,6 +109,24 @@ describe('DevelopmentSidebar', () => { expect(mockUseDriveMachines).toHaveBeenCalledWith('drive-1'); }); + test('a failed POLL keeps the tree — it does not replace it with an error', () => { + // The list polls, and SWR keeps the last good data while setting `error` on a + // failed revalidation. Reporting the error ahead of the data would let one + // blip tear down every MachineTree, losing its expansion state and the + // session leaves under it, while the app still holds a good list. + mockUseDriveMachines.mockReturnValueOnce({ + machines: [{ id: 'machine-1', title: 'Dev box', updatedAt: '2026-07-12T00:00:00.000Z' }], + isLoading: false, + error: new Error('blip'), + mutate: vi.fn(), + }); + + render(); + + expect(screen.getByText('Dev box')).toBeDefined(); + expect(screen.queryByText(/failed to load machines/i)).toBeNull(); + }); + test('says nothing about admin rights until auth has resolved', () => { // `role` isn't persisted across a reload, so an early refusal would flash at // a real admin on every cold load. diff --git a/apps/web/src/hooks/useDriveMachines.ts b/apps/web/src/hooks/useDriveMachines.ts index 04a746cbb9..0fa8a438c1 100644 --- a/apps/web/src/hooks/useDriveMachines.ts +++ b/apps/web/src/hooks/useDriveMachines.ts @@ -34,10 +34,17 @@ export function useDriveMachines(driveId: string | null) { // of the session, and the only ways out (reload, or leave and return) both // unmount the keep-alive host and disconnect every warm terminal. // - // It also means a machine created elsewhere shows up without a reload. Cheap: - // one indexed query, on an admin-only surface, and SWR keeps the previous - // array identity when the ids are unchanged — so a poll that changes nothing - // doesn't churn the keep-alive LRU. + // It also means a machine created elsewhere shows up without a reload, and + // (by dropping the previous `revalidateOnFocus: false`) that coming back to the + // tab recovers immediately rather than waiting out the interval. + // + // Cheap: one indexed query on an admin-only surface, and SWR suppresses the + // poll entirely while the tab is hidden. A poll cannot churn the keep-alive LRU + // — note that is NOT because SWR preserves the array's identity (it only does + // that when the whole payload is deep-equal, and `updatedAt` moves whenever a + // Machine page is touched). It's because the consumers key on the IDS alone: + // `useStickyMachineIds` and the host's `validKey` both collapse the list to its + // ids, so a payload that carries new timestamps but the same machines is inert. const { data, error, isLoading, mutate } = useSWR(key, fetcher, { refreshInterval: 30_000, }); From 043e5b1241df15c4ad3bc9c482e5a48f2baf300d Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 13:35:05 -0500 Subject: [PATCH 16/18] =?UTF-8?q?fix(development):=20the=20last=20raw=20NU?= =?UTF-8?q?L=20byte=20=E2=80=94=20in=20the=20file=20that=20diagnosed=20the?= =?UTF-8?q?=20problem?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Writing useStickyMachineIds I copied the key idiom from MachineKeepAliveHost, including its literal NUL separator — the very defect this branch added .gitattributes to expose. So the surface's own layout carried a raw NUL while the commit that removed one from the host was still fresh. Not a runtime bug (NUL is a fine separator), but it meant .gitattributes was MASKING the problem rather than the source being clean: remove that file and layout.tsx goes binary to git — no textual diff, no three-way merge. Now the escape, matching the sibling. Byte-scanned every tracked source file under apps/web, packages/lib, and apps/realtime: zero raw NUL bytes remain. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../[driveId]/development/layout.tsx | Bin 10117 -> 10122 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx index 7a372d85957d879d61673b6afdede2e7bc61f5a9..e50e394e9a8137dbdf7f3dba15224d3225c93b07 100644 GIT binary patch delta 19 YcmZqm@ABUuFUA&AY5)YA6~s~n0X{?pi2wiq delta 14 VcmeD3Z}s0GFUH8QSye1m5C9__1NZ;{ From 67eb11f12091f4955d246ccf04bc0cff4635d155 Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 14:09:11 -0500 Subject: [PATCH 17/18] fix(development): never open a session into a machine the host is keeping hidden MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The drain gated on what the URL SELECTS while the keep-alive host gates visibility on what it can DISPLAY. Those disagree exactly when a machine is transiently missing from /api/machines — the case this surface already goes out of its way to survive. In that window every pane is `display:none`, and opening a session there mounts an xterm inside a hidden container: fit() measures a zero-sized box and the PTY is created at a bogus geometry, wrapping its output for the life of the session (it recovers visually on the next show, but the mangled history doesn't). Both now derive from one value, `displayedMachineId`. The intent is simply held until the machine is displayed again, which is what the convergent drain is for. Found by reviewing this branch against the sprites/terminal work just merged from master (#2013 scrollback-replay suppression, #2020 re-auth) — that merge is behaviorally clean, but checking it against the keep-alive lifecycle surfaced this. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../development/__tests__/layout.test.tsx | 17 +++++++++++++++++ .../dashboard/[driveId]/development/layout.tsx | 14 ++++++++------ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx b/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx index bcb67b5dda..521c857eee 100644 --- a/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx +++ b/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx @@ -35,6 +35,7 @@ vi.mock('@/components/layout/middle-content/MachineKeepAliveHost', () => ({ import DevelopmentLayout from '../layout'; import { usePendingSessionStore } from '@/stores/development/usePendingSessionStore'; +import { useMachineWorkspaceStore } from '@/stores/machine-workspace/useMachineWorkspaceStore'; const machine = (id: string) => ({ id, title: id, updatedAt: '2026-07-12T00:00:00.000Z' }); @@ -142,6 +143,22 @@ describe('DevelopmentLayout', () => { expect(screen.getByText(/administrator privileges/i)).toBeDefined(); }); + test('does not open a session into a machine the host is keeping HIDDEN', () => { + // The drain must gate on what is DISPLAYED, not on what the URL selects. If a + // machine is transiently missing from the list the host hides every pane, and + // opening a session then mounts an xterm inside a `display:none` container — + // fit() measures a zero-sized box and the PTY is created at a bogus geometry, + // wrapping its output for the life of the session. + mockUseDriveMachines.mockReturnValue(driveMachines({ machines: [] })); + usePendingSessionStore.setState({ pending: { machineId: 'machine-1', scope: { name: 'agent-1' } } }); + + render({null}); + + expect(hostRenders.at(-1)!.activePageId).toBeNull(); + // Held, not applied — it converges once the machine is displayed again. + expect(useMachineWorkspaceStore.getState().workspaces['machine-1']).toBeUndefined(); + }); + test('drops an unconverged session intent when the surface is left', () => { // The store is a module singleton: an intent left behind would still be // sitting there on the next visit, ready to fire into whatever pane is active. diff --git a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx index e50e394e9a..ed972974d4 100644 --- a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx +++ b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx @@ -61,8 +61,14 @@ export default function DevelopmentLayout({ children }: { children: React.ReactN // reload, or leave and return — unmount this host and kill every warm // terminal). const isKnownMachine = selectedMachineId !== null && machines.some((m) => m.id === selectedMachineId); + // What the host actually DISPLAYS — not merely what the URL selects. The drain + // must gate on this same value: opening a session into a machine the host is + // keeping hidden would mount an xterm inside a `display:none` container, where + // `fit()` measures a zero-sized box and the PTY is created at a bogus geometry + // — wrapping its output for the life of the session. + const displayedMachineId = isKnownMachine ? selectedMachineId : null; - useDrainPendingSession(selectedMachineId); + useDrainPendingSession(displayedMachineId); return (
@@ -78,11 +84,7 @@ export default function DevelopmentLayout({ children }: { children: React.ReactN /> )} - +
); } From b1a98109cbda82affd057f9dfa8050bef947160a Mon Sep 17 00:00:00 2001 From: 2witstudios <2witstudios@gmail.com> Date: Sun, 12 Jul 2026 14:27:10 -0500 Subject: [PATCH 18/18] polish(development): pin the positive half of the display gate; tidy names and tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The drain's parameter was still called selectedMachineId while it now receives displayedMachineId. Renamed to what it is. - Adds the test the last fix was missing. Gating the drain on what's DISPLAYED could plausibly have turned "hold" into "drop", so the positive path is now pinned at the composition level: an intent for a machine that isn't in the list yet WAITS, and lands in the active pane as soon as the machine appears. - Folds two duplicate cases out of pending-session.test.ts (identical inputs and expectations to the tests above them — no coverage lost). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --- .../development/__tests__/layout.test.tsx | 23 +++++++++++++++++++ .../[driveId]/development/layout.tsx | 6 ++--- .../__tests__/pending-session.test.ts | 18 ++++----------- 3 files changed, 31 insertions(+), 16 deletions(-) diff --git a/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx b/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx index 521c857eee..2ffbf03dbb 100644 --- a/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx +++ b/apps/web/src/app/dashboard/[driveId]/development/__tests__/layout.test.tsx @@ -50,6 +50,7 @@ beforeEach(() => { vi.clearAllMocks(); hostRenders.length = 0; usePendingSessionStore.setState({ pending: null }); + useMachineWorkspaceStore.setState({ workspaces: {} }); mockUseAuth.mockReturnValue({ user: { role: 'admin' }, isLoading: false }); mockUseDriveMachines.mockReturnValue(driveMachines()); }); @@ -159,6 +160,28 @@ describe('DevelopmentLayout', () => { expect(useMachineWorkspaceStore.getState().workspaces['machine-1']).toBeUndefined(); }); + test('holds a session intent while the list loads, then applies it once the machine is displayed', () => { + // The other half of gating the drain on what's DISPLAYED: holding must not + // become dropping. An intent for a machine that isn't in the list *yet* waits, + // and converges as soon as the machine appears — otherwise the fix for the + // hidden-machine case would have silently broken the ordinary one. + mockUseDriveMachines.mockReturnValue(driveMachines({ machines: [], isLoading: true })); + usePendingSessionStore.setState({ pending: { machineId: 'machine-1', scope: { name: 'agent-1' } } }); + const { rerender } = render({null}); + + // Held, not dropped. + expect(usePendingSessionStore.getState().pending).not.toBeNull(); + + // The list arrives, and the machine's pane region has ensured a workspace. + mockUseDriveMachines.mockReturnValue(driveMachines({ machines: [machine('machine-1')] })); + useMachineWorkspaceStore.getState().ensureWorkspace('machine-1'); + rerender({null}); + + const workspace = useMachineWorkspaceStore.getState().workspaces['machine-1']; + const activePane = workspace.columns.flatMap((c) => c.panes).find((p) => p.id === workspace.activePaneId); + expect(activePane?.scope).toMatchObject({ name: 'agent-1' }); + }); + test('drops an unconverged session intent when the surface is left', () => { // The store is a module singleton: an intent left behind would still be // sitting there on the next visit, ready to fire into whatever pane is active. diff --git a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx index ed972974d4..42cee93378 100644 --- a/apps/web/src/app/dashboard/[driveId]/development/layout.tsx +++ b/apps/web/src/app/dashboard/[driveId]/development/layout.tsx @@ -205,7 +205,7 @@ function DetailNotice({ title, description }: { title: string; description?: str * singleton, so an intent left behind here would otherwise still be sitting * there on the user's next visit, ready to fire into whatever pane was active. */ -function useDrainPendingSession(selectedMachineId: string | null) { +function useDrainPendingSession(displayedMachineId: string | null) { const pending = usePendingSessionStore((state) => state.pending); const clearPending = usePendingSessionStore((state) => state.clearPending); const openTerminal = useMachineWorkspaceStore((state) => state.openTerminal); @@ -214,11 +214,11 @@ function useDrainPendingSession(selectedMachineId: string | null) { ); useEffect(() => { - const action = resolvePendingSession(pending, selectedMachineId, workspace); + const action = resolvePendingSession(pending, displayedMachineId, workspace); if (action.type === 'open') openTerminal(action.machineId, action.scope); // A 'clear' with no pending intent is a no-op, so this cannot loop. else if (action.type === 'clear') clearPending(); - }, [pending, selectedMachineId, workspace, openTerminal, clearPending]); + }, [pending, displayedMachineId, workspace, openTerminal, clearPending]); useEffect(() => () => clearPending(), [clearPending]); } diff --git a/apps/web/src/lib/development/__tests__/pending-session.test.ts b/apps/web/src/lib/development/__tests__/pending-session.test.ts index bda11f752e..cbba3522fc 100644 --- a/apps/web/src/lib/development/__tests__/pending-session.test.ts +++ b/apps/web/src/lib/development/__tests__/pending-session.test.ts @@ -13,6 +13,10 @@ const workspaceWith = (scope: WorkspaceState['columns'][number]['panes'][number] describe('resolvePendingSession', () => { test('opens the session once the user is on the machine and it has a workspace', () => { + // Also the rebuild case: MachineWorkspace disposes on unmount and re-creates on + // mount (StrictMode double-invokes exactly this on the first visit), so the + // intent must still resolve to `open` against a fresh, empty workspace rather + // than being a one-shot that the rebuild destroys. expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(null))).toEqual({ type: 'open', machineId: 'machine-1', @@ -34,24 +38,12 @@ describe('resolvePendingSession', () => { expect(resolvePendingSession(PENDING, 'machine-1', undefined)).toEqual({ type: 'wait' }); }); - test('re-opens against a workspace that was torn down and rebuilt', () => { - // MachineWorkspace disposes on unmount and re-creates on mount (StrictMode - // double-invokes exactly this on the first visit). A fire-once intent would - // be destroyed by the rebuild; a convergent one re-applies to the new one. - expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(null))).toEqual({ - type: 'open', - machineId: 'machine-1', - scope: SCOPE, - }); - }); - test('clears once the session is actually in the active pane', () => { expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(SCOPE))).toEqual({ type: 'clear' }); }); test('a satisfied intent is dropped, so it cannot clobber the user\'s next pane change', () => { - expect(resolvePendingSession(PENDING, 'machine-1', workspaceWith(SCOPE))).toEqual({ type: 'clear' }); - // And with no intent left, nothing is ever re-applied. + // With no intent left, nothing is ever re-applied over the user's own choice. expect( resolvePendingSession(null, 'machine-1', workspaceWith({ ...SCOPE, name: 'agent-2' })), ).toEqual({ type: 'clear' });