Files
kandev/apps/web/components/task/dockview-add-panel-items.tsx
T
José Almeida 3bc092bf20 feat(terminal): first-class user terminals — stable seq, rename, park/resume (#1009)
* feat(terminal): persist user terminals with stable seq + park state

Ordinary user shells now have a DB-backed `user_terminals` row keyed by
the agentctl PTY id, carrying a per-task monotonic seq (gaps preserved on
delete), custom_name override, and open/parked state. The terminal
service owns the create/list/rename/park/resume/destroy surface and
guards reject `bottom-panel` (cmd+J) + `script-*` ids so passthrough
flows are untouched. Closing a tab now parks (PTY keeps running);
destroy kills the PTY and removes the row.

* feat(web): expose terminal seq badge, rename, and parked-terminals menu

Wires the right-panel strip and SSR page state to the new task-keyed
terminals endpoint and discriminated UserShell union, adds the `#N`
badge for ordinary terminals, plumbs context-menu / double-click rename,
and renders a "Parked" submenu so closed-but-running terminals can be
resumed. Closing a managed tab now parks instead of destroying so the
PTY (and its buffer) survives accidental closes.

* test(terminal): e2e coverage for first-class persistent entities

Covers the persistence guarantees the user explicitly called out:
monotonic seqs, refresh-survives-rename-and-park, gap-preserving destroy,
park+resume cycle, and bottom-panel/script guards. Exercises the new
user_shell.{create,list,rename,park,resume,destroy} WS actions through
the shared ApiClient.wsRequest helper.

* fix(terminal): atomic seq allocation + propagate destroy errors

- Repository.Create folds MAX(seq) read + INSERT into a single
  `INSERT … SELECT` statement so concurrent creates can no longer pick
  the same seq. Locked in with TestCreate_ConcurrentSeqAllocation (25
  parallel creates, all unique).
- wsUserShellStop now propagates the service-Destroy error so the
  frontend stops optimistically removing a row the backend failed to
  delete.
- Service.Destroy / CleanupTask log PTY-stop failures at Warn (instead
  of silently swallowing) while keeping the proceed-on-failure semantics
  intentional — the DB row is the source of truth for "user expects
  this terminal gone".
- IsUserShellAlive copies ProcessID under the read lock before calling
  r.Get(), removing the post-unlock pointer-deref race.
- Get-not-found test asserts errors.Is(err, ErrNotFound) so it locks to
  the repository contract.

* fix(web): close-tab waits for backend ack + className undefined guard

- handleCloseTab defers `removeTerminal` into the `.then()` of the
  park/destroy promise. A failed backend call leaves the tab visible
  rather than vanishing it optimistically; the next `user_shell.list`
  poll reconciles state.
- session-tabs guards `tab.className` with `?? ""` so an undefined value
  no longer produces "undefined group relative..." in the rendered
  class string.
- user-shells.test simplification: `createSessionRuntimeSlice` only
  consumes `set`, so the test passes the slice creator straight to
  `immer()` instead of wrapping it in an adapter with unused args.

* fix(terminal): task-scoped mutations + legacy-shell list + UI hardening

Round-2 review feedback (cubic P1/P2, claude, coderabbit):

Backend:
- Rename/Park/Resume/Destroy now require a task_id and verify the
  terminal belongs to that task via a shared requireOwnership helper.
  Cross-task mutations return ErrTaskMismatch instead of silently
  succeeding by raw terminal id.
- wsUserShellList no longer drops managed-shaped agentctl shells when
  the terminal service is disabled or task_id is missing — legacy
  passthrough callers see their existing shells again. `managedCovered`
  flag scopes the orphan filter to the DB-backed path.
- service_test.go uses the shared-cache in-memory SQLite + MaxOpenConns(1)
  setup, matching repository_test.go and removing the flake-one-race-away
  warning.
- New cross-task guard tests: TestRename_RejectsCrossTask,
  TestPark_RejectsCrossTask, TestDestroy_RejectsCrossTask, plus a
  TestRename_EmptyTaskIDSkipsOwnership for internal callers.

Frontend:
- updateUserShell patch type narrowed to Partial<Omit<UserShellInfo,
  "terminalId">> so the row key can't be silently mutated.
- destroyUserShell now throws on a missing WS client instead of silently
  resolving — handleCloseTab's .then() no longer fires prematurely.
- rename/park/resume/destroy API client + useTerminals hook plumb the
  active task id through every mutating call.

E2E:
- New "cross-task mutating ops are rejected" spec creates a terminal
  under task A and verifies task B can't rename/park/destroy it by raw
  id.
- Existing specs updated to send task_id with their mutating calls.

* fix(terminal): require task_id on all mutators + wire taskID into hooks

Round-3 review feedback (cubic P1, claude blocker, coderabbit):

Backend:
- requireOwnership no longer skips the ownership check when taskID is
  empty. The previous carve-out was reachable from the WS layer (where
  task_id is user-controlled), so an attacker who guessed a terminal id
  could omit task_id and mutate any row. CleanupTask walks the repo
  directly and never touched the bypass.
- TestRename_EmptyTaskIDRejected pins the new contract.

Frontend (the real bug claude caught):
- useUserShells now accepts a taskId parameter and forwards it on the
  user_shell.list WS call. Without it the backend's DB-backed ordinary-
  shell path was skipped and the Zustand store wiped the SSR-hydrated
  panel strip on the first WS fetch.
- Response mapping switched to a discriminated union: id (new field)
  falls back to terminal_id (legacy), and kind/seq/state/pty_status/
  display_name/custom_name flow into the store so the parked-terminals
  submenu actually populates.
- useTerminals + useMobileTerminals read the active task id from the
  store and pass it through to useUserShells.
- destroyUserShell now retries with user_shell.stop when the server
  returns an unknown-action error — keeps the rollout-window
  compatibility intact for older backends that only know the legacy
  action name.

* fix(terminal): list parked, rollout-safe kind default, helper extraction

Round-4 review feedback (claude + greptile P1):

- useUserShells now sends include_parked: true so the "Parked terminals"
  submenu populates after a page reload. Previously the WS list returned
  only state=open rows; parked terminals were invisible until the user
  parked something in the same session. The main strip still hides
  parked entries via buildTerminalsFromShells.
- useAddTerminal stops blindly defaulting kind to "ordinary" — that
  misclassified legacy passthrough payloads (terminal_id without seq)
  during a rollout where the frontend is newer than the backend. The
  managed-tab path then called parkUserShell on a server that didn't
  know the action. Now only tags as ordinary when kind is explicit or
  the row carries a seq.
- handleCloseTab extracted into a standalone useCloseTab + helpers
  hoisted into use-terminals-build.ts / use-terminals-types.ts. Brings
  use-terminals.ts back under the 600-line file limit and the
  useTerminalActions wrapper inside the 100-line function limit.

* fix(terminal): scope-keyed shell cache + null-task guard + tighter rollout default

Round-5 review feedback (cubic P1, coderabbit x4):

- useUserShells now keys its fetch cache on env+task (`${env}:${task}`),
  not just env. An earlier load without a task id (e.g. before the
  active task hydrated) no longer pins the store on a legacy-only shell
  list and blocks the task-scoped refetch.
- useMobileTerminals delays auto-create until a real taskID is
  available. Previously a null taskID could bootstrap a legacy
  non-persistent shell, mark the env as auto-created, and starve the
  real task of its first-class terminal. Guard key is now env:task, and
  releaseAutoCreatedEnvironment clears every scope on that env.
- useAddTerminal tightens the rollout check: a row is "ordinary" only
  when `seq !== undefined`. A bare `kind: "ordinary"` without seq from
  an older backend stays legacy so close → destroy on the first click.
- useRemoveTerminal reads the current activeTab from a ref instead of
  closing over the value captured when handleCloseTab first ran. Async
  park/destroy resolves after a user tab-switch no longer clobber the
  newer selection with the stale fallback shift.

Tests:
- New "waits for taskID before auto-creating — null taskID is a no-op"
  case in use-mobile-terminals.test.ts confirms the null-task guard.
- All existing mobile-terminal + slice + E2E tests pass.

* fix(terminal): live activeTab lookup + selectable-tab guard

Round-6 review feedback (cubic P2 x2):

- getActiveTab now reads the active tab from the Zustand store via
  storeApi.getState() instead of a useRef/useEffect pair. Closes the
  post-render race where an async close handler could resolve between
  the new render's commit and the effect-driven ref sync.
- computeTerminalTabValue requires activeTab to be selectable
  (present in `terminals` or === "commands") before honoring it. A
  stale id left over from a parked/destroyed terminal no longer pins
  the panel on a nonexistent tab and blocks the fallback shift to a
  real shell.

* chore(terminal): inline savedTabFromStorage to drop file under line cap

Round-7 review feedback (claude suggestion): the useMemo around the
synchronous getSessionStorage call was zero-value memoisation that
pushed use-terminals.ts three lines over the 600-line max-lines limit.
Inlined; file now at 600 lines.

* chore(terminal): drop savedTabExists local + comment to land under 600 lines

Round-8 follow-up (claude): the previous commit's two-line comment block
held the file at 603 lines (still over the 600-line cap). Inline
savedTabExists at the only call site and remove the redundant comment —
the round-7 PR thread already explains why the useMemo was removed. File
now at 599 lines.

* fix(terminal): plumb task_id through dockview + tolerate legacy destroys

Manual-test feedback: clicking X on a dockview terminal panel surfaced
"destroy terminal: terminal does not belong to the supplied task" because
the dockview path was creating + destroying terminals without sending
task_id at all. The new requireOwnership guard then correctly rejected
the empty task_id against a non-empty term.TaskID.

Frontend:
- addTerminalPanel grows task_id + title parameters; stamps both into
  panel params at creation time so the destroy cleanup can pass them
  back to user_shell.destroy / .stop.
- Every dockview-side createUserShell call site now passes the active
  task_id, so the backend creates a DB-backed ordinary terminal (with
  seq + displayName) instead of a legacy passthrough shell.
- destroyUserShell + stopUserShell already accepted an optional task_id;
  dockview-layout-setup.onDidRemovePanel now reads the stamped task id
  from panel params and forwards it.

Backend:
- wsUserShellStop now requires both terminalSvc and a non-empty
  req.TaskID before routing through the managed service. An empty
  task_id falls through to the legacy passthrough StopUserShell so older
  clients (and dockview panels persisted before task_id was stamped)
  keep working.

Known follow-ups (separate task): the #N badge, rename context menu,
and parked-terminals dropdown still only show on the mobile/tablet
right-panel strip. Desktop dockview shows the seq inside the panel
title text but doesn't expose the badge/rename/parked-list UI yet.

* feat(terminal): dockview tab badges + rename context menu + existing-terminals list

Manual-test feedback round 2:

- Custom dockview tab component (TerminalTab) renders the #N pill
  only when the active env has more than one ordinary terminal —
  mirrors the session-tab badge semantics. Right-click on any ordinary
  terminal tab now exposes Rename / Park / Destroy.
- Dockview + menu picks up a new "Terminals" section listing every
  ordinary terminal for the active task (open and parked). Clicking a
  row focuses the existing dockview panel if one is mounted, otherwise
  re-opens it; parked rows are auto-resumed via user_shell.resume
  before the panel is added. The base "Terminal" action was renamed
  to "New Terminal" so the existing-terminals list is distinguishable
  in the menu.
- Reopen menu uses a stable EMPTY_SHELLS sentinel for its store
  selector to avoid Zustand infinite re-renders when the env has no
  shells.

E2E:
- terminal-new-tab.spec uses getByRole("menuitem", { name: "New
  Terminal", exact: true }) to disambiguate from the new "Terminals"
  submenu label.
- Full terminal suite still passes (26/26).

* fix(terminal): tab title shows "Terminal" with seq as badge, populate store on session page

Re-reading the user feedback: the title text was rendering as
"Terminal 1" (backend's displayName), and the dockview "+" menu's
Terminals section was empty because nothing on desktop was calling
useUserShells to populate the store.

- TerminalTab.pickDisplayName now returns the literal "Terminal" for
  ordinary terminals (custom names still override). The seq lives only
  in the sibling badge — same split as session-tab where the agent
  name is the title and the seq is a pill before it. Reading "Terminal
  1" out of "[1] Terminal" instead of "[1] Terminal 1".
- LeftHeaderActions (dockview "+") now calls useUserShells on mount.
  Without this the desktop layout never triggered the WS list fetch,
  so the Terminals submenu and the badge "count > 1" check both saw
  an empty store and silently disappeared.
- TerminalReopenMenuItems also calls useUserShells defensively (cheap
  thanks to scope-keyed cache) so it works when mounted in isolation.

* feat(terminal): "New X" rows live inside their section labels

User feedback: move "New Agent" under the Agents section header and
"New Terminal" under the Terminals section header so the dockview "+"
menu reads top-to-bottom as Agents → New Agent → existing sessions →
Terminals → New Terminal → existing terminals → other panels.

- SessionReopenMenuItems accepts an optional onNewSession prop and
  renders the "New Agent" row as the first item under the "Agents"
  label. Render gate now keeps the section visible whenever the create
  handler is supplied, even with zero existing sessions.
- TerminalReopenMenuItems gets a matching onNewTerminal prop with
  the same behaviour ("New Terminal" row under the "Terminals" label).
- dockview-add-panel-items drops its standalone "New Agent" and "New
  Terminal" rows since both now live inside their sections.

* feat(terminal): auto-migrate default panel to DB-backed ordinary terminal

The legacy `terminal-default` dockview panel was hard-wired to a
non-managed `shell-default` PTY so it never picked up an ordinary
seq/customName/parked state — the new "Terminal N" badge UX could
only show a badge once the user had explicitly created a second
terminal. With useEnsureDefaultTerminalOrdinary the default panel is
upgraded into a real `shell-<uuid>` row on session-page mount, so
every visible tab is now a first-class ordinary terminal.

Plus a UI-level e2e spec asserting the multi-terminal title + badge
contract, and an expectation bump on the existing rename+park spec
since visiting the session page now mints 1 extra ordinary terminal.

* fix(terminal): park ordinary terminals on tab close so they survive refresh

Dockview's tab X close button routed straight to stopUserShell (an
alias for destroyUserShell), wiping the DB row on close — after a
refresh the user had no way to resume a still-running PTY they had
just tucked away.

The X is now a "hide for now" affordance: ordinary terminals get
parked (PTY survives, row stays in DB, reappears in the "+" →
Terminals menu with a `parked` pill), while scripts / bottom-panel /
legacy passthrough shells stay on the destroy path because they have
no resume story.

Permanent delete still lives on the right-click Destroy menu item
and (in the next commit) on each reopen-menu row.

* fix(terminal): preserve DB-backed terminal panel ids across layout restore

The serializer's normalizePanel rewrote every user-created terminal
panel id to `terminal-saved-N` and dropped environmentId/taskID from
params, so after a reload that panel couldn't be matched back to its
live PTY or its store record — the tab rendered with no badge and a
stale fallback title.

Treat `shell-<uuid>` panel ids the same way we already treat
`terminal-default`: preserve id and params on restore. Legacy
ephemeral `terminal-saved-*` ids still mint a fresh PTY on restore.
Also force tabComponent=terminalTab and title="Terminal" for any
restored terminal panel so layouts persisted before the badge UX
land on the new presentation.

* feat(terminal): destroy button on reopen-menu rows for permanent delete

Destroy used to live only on the tab's right-click menu — useless once
the tab was already closed. Each ordinary terminal row in the dockview
"+" → Terminals submenu now ends with an × button that confirms-then
calls destroyUserShell, killing the PTY and dropping the DB row so the
terminal does not reappear after reload.

Pairs with the previous close-to-park change: closing the tab parks
(soft, resumable), the × button on the menu row destroys (hard,
permanent).

* fix(terminal): reopen-menu focuses existing tab, drops parked pill, says Terminate

Three small fixes to the dockview "+" → Terminals submenu:

1. Clicking a row whose terminal is already mounted now FOCUSES the
   existing panel instead of duplicating it. The default-migrated
   panel keeps its registry id `terminal-default` even after the
   migration to a `shell-<uuid>` PTY, so a direct getPanel(uuid) miss
   would fall through to addTerminalPanel and create a second tab for
   the same PTY. Fall back to scanning `params.terminalId` so the
   lookup catches that case.

2. Drop the "parked" pill from the row — parking is now an internal
   detail (close = park) that the user doesn't need to read off the
   menu. Clicking the row brings the terminal back into view either
   way, so distinguishing it visually is noise.

3. Rename the row × tooltip + confirm copy from "Destroy" / "Permanently
   delete" to "Terminate" to match the new tab-context-menu label.

Spec: add a test that asserts no duplicate tab when reopening an
already-open terminal, and drop the parked-pill assertion from the
existing close+reload test.

* feat(terminal): empty-group watermark uses the rich + menu

When a user splits a group (split-right / split-down) the new half is
empty and dockview renders the watermark. Before this, the watermark
showed a hard-coded Agent/Plan/Terminal/Browser row and clicking
"Terminal" called createUserShell without a taskId — so it spawned a
legacy passthrough shell that never appeared in the user-shell list,
and clicking "Agent" only ever focused the singleton chat panel
(invisible if the task had multiple sessions).

The watermark now renders the same AddPanelMenuItems content the
header "+" uses, so the user gets:
- The Agents list (existing sessions, greyed when already open)
- The Terminals list (existing terminals, greyed when already open)
- The Browser/Plan/VS Code/Changes/Files/PR options

…served behind a single "Add panel" dropdown trigger centered on the
empty group.

* feat(terminal): inline rename on the tab; drop Park, rename Destroy → Terminate

The tab title is now editable in place — picking Rename from the
right-click context menu swaps the title for an input. Enter commits,
Escape / blur-with-no-change cancels, and the new name is pushed
through renameUserShell + the user-shell store so the API and badge
list stay in sync. Replaces the previous window.prompt which felt
out of place in the dockview chrome.

While here:
- Drop the "Park (hide, keep PTY)" context-menu item. Tab close
  already parks ordinary terminals, so the explicit Park action was
  redundant and broke the "one obvious affordance per outcome" rule.
- Rename "Destroy" → "Terminate" in the context menu. Matches the
  reopen-menu × tooltip and reads more like a system action than a
  destructive UI primitive.

Spec adds an inline-rename test asserting the input appears, accepts
typed text, and commits the new title to the tab.

* fix(terminal): right-click Terminate closes the tab; drop displayName DOM warning

Two unrelated fixes both landing in terminal-tab.tsx:

- Terminate used to fire the WS destroy and update the store, but left
  the dockview panel mounted with a dead PTY underneath. Now it
  optimistically drops the shell row from the store, closes the panel
  via api.close(), then sends the destroy — same effect as the X close
  but routed to the hard-destroy path (the onDidRemovePanel cleanup
  sees no shell entry and skips its park branch).

- TerminalTabBody typed `displayName` as a prop but never destructured
  it, so {...props} spread it into DockviewDefaultTab and on into a DOM
  div, triggering "React does not recognize the `displayName` prop on
  a DOM element". Drop it from the spread.

* feat(terminal): polish + menu — badge-style indices, flush-left New rows, terminate closes panel

Polish pass on the dockview "+" → Agents / Terminals submenus:

- Replace the plain `#N` text with the same pill-style badge the tabs
  use (small bg, rounded, 11px). Agents and terminals both pick this
  up — so the menu row reads "[N] label" matching the tab style.
- Drop the empty index-column placeholder span on the "New X" rows so
  the icon + label sit flush-left under the section header instead of
  awkwardly indented.
- Strip the backend "Terminal {seq}" suffix from row labels — the
  badge already carries the number, so showing "Terminal 1" next to
  the "1" pill duplicated it. Custom-renamed terminals still show
  their name.
- The row × button now also closes the dockview panel if the terminal
  is currently open, mirroring the right-click → Terminate flow.

Spec: switch the row-by-seq lookups from `hasText: "#N"` (now stale
since the `#` prefix is gone) to a `has:` filter against the new
`reopen-terminal-seq-N` badge testid, and add a regression test for
right-click → Terminate closing the tab and removing the row in one
gesture.

* fix: address terminal review feedback

* fix: resolve terminal review suggestions

* fix: catch terminal close stop failures

* fix: harden terminal terminate failures

* fix: use pinned pnpm docker arg

* fix: satisfy backend lint after main merge

* fix: stabilize e2e after layout merge
2026-05-24 12:59:30 +01:00

113 lines
3.7 KiB
TypeScript

"use client";
import {
IconBrandVscode,
IconDeviceDesktop,
IconFileText,
IconFolder,
IconGitBranch,
IconGitPullRequest,
} from "@tabler/icons-react";
import { DropdownMenuItem } from "@kandev/ui/dropdown-menu";
import { prPanelLabel } from "@/components/github/pr-utils";
import { prTaskKey } from "@/components/github/pr-detail-panel";
import { useDockviewStore } from "@/lib/state/dockview-store";
import { useAppStore } from "@/components/state-provider";
import type { TaskPR } from "@/lib/types/github";
import { RepositoryScriptsMenuItems } from "./repository-scripts-menu";
import { SessionReopenMenuItems } from "./session-reopen-menu";
import { TerminalReopenMenuItems } from "./terminal-reopen-menu";
type AddPanelMenuState = {
taskId: string | null;
isPassthrough: boolean;
hasChanges: boolean;
hasFiles: boolean;
/** All PRs linked to the task; multi-repo tasks render one menu item per PR. */
prs: TaskPR[];
};
type AddPanelMenuItemsProps = {
groupId: string;
state: AddPanelMenuState;
onNewSession: () => void;
onAddTerminal: () => void;
onRunScript: (scriptId: string) => void;
onRunDevScript: () => void;
};
export const MENU_ICON_CLASS = "h-3.5 w-3.5 mr-1.5 shrink-0";
export const MENU_ITEM_CLASS = "cursor-pointer text-xs";
export function AddPanelMenuItems({
groupId,
state,
onNewSession,
onAddTerminal,
onRunScript,
onRunDevScript,
}: AddPanelMenuItemsProps) {
const addBrowserPanel = useDockviewStore((s) => s.addBrowserPanel);
const addVscodePanel = useDockviewStore((s) => s.addVscodePanel);
const addPlanPanel = useDockviewStore((s) => s.addPlanPanel);
const addFilesPanel = useDockviewStore((s) => s.addFilesPanel);
const addChangesPanel = useDockviewStore((s) => s.addChangesPanel);
const addPRPanel = useDockviewStore((s) => s.addPRPanel);
const activeSessionId = useAppStore((s) => s.tasks.activeSessionId);
return (
<>
{state.taskId && (
<SessionReopenMenuItems
taskId={state.taskId}
groupId={groupId}
onNewSession={onNewSession}
/>
)}
<TerminalReopenMenuItems groupId={groupId} onNewTerminal={onAddTerminal} />
<DropdownMenuItem
onClick={() => addBrowserPanel(undefined, groupId)}
className={MENU_ITEM_CLASS}
>
<IconDeviceDesktop className={MENU_ICON_CLASS} />
Browser
</DropdownMenuItem>
<DropdownMenuItem onClick={() => addVscodePanel()} className={MENU_ITEM_CLASS}>
<IconBrandVscode className={MENU_ICON_CLASS} />
VS Code
</DropdownMenuItem>
{!state.isPassthrough && (
<DropdownMenuItem onClick={() => addPlanPanel({ groupId })} className={MENU_ITEM_CLASS}>
<IconFileText className={MENU_ICON_CLASS} />
Plan
</DropdownMenuItem>
)}
{!state.hasChanges && (
<DropdownMenuItem onClick={() => addChangesPanel(groupId)} className={MENU_ITEM_CLASS}>
<IconGitBranch className={MENU_ICON_CLASS} />
Changes
</DropdownMenuItem>
)}
{!state.hasFiles && (
<DropdownMenuItem onClick={() => addFilesPanel(groupId)} className={MENU_ITEM_CLASS}>
<IconFolder className={MENU_ICON_CLASS} />
Files
</DropdownMenuItem>
)}
{state.prs.map((pr) => (
<DropdownMenuItem
key={pr.id}
onClick={() => addPRPanel(prTaskKey(pr), activeSessionId)}
className={MENU_ITEM_CLASS}
>
<IconGitPullRequest className={MENU_ICON_CLASS} />
{state.prs.length > 1
? `${prPanelLabel(pr.pr_number)} — ${pr.repo}`
: prPanelLabel(pr.pr_number)}
</DropdownMenuItem>
))}
<RepositoryScriptsMenuItems onRunScript={onRunScript} onRunDevScript={onRunDevScript} />
</>
);
}