Add runpane panes focus command (#401) - #461
Conversation
parsakhaz
left a comment
There was a problem hiding this comment.
Verdict: Request changes - the branch fails typecheck and optional panel focus can be overwritten by the persisted active panel.
Counts: Must Fix: 2 (security: 0) · Should Fix: 0 · pass 1/3
Must Fix
-
MF-1 - The PR fails typecheck in every touched TypeScript package ·
frontend/src/hooks/useIPCEvents.ts:220,main/src/ipc/runpane.ts:370,main/src/ipc/runpane.ts:2148,packages/runpane/src/localControl.ts:889· restore the existing deletion-event contract, keep daemon request values asPaneCommandValue, and decode the focus response with a boundary schema · violates the PR checklist and repository typecheck requirement- Evidence:
pnpm --filter frontend typecheckreports TS2552 for undefinedSessionDeletedEventData;pnpm --filter main typecheckreports TS2345 twice becauseunknownis passed toPaneCommandValuehelpers;pnpm --filter runpane typecheckreports TS2353 because the options object is passed whereBoundarySchema<PaneFocusResult>is required. - Failure scenario: CI cannot build or typecheck the branch, so the command cannot ship.
- Evidence:
-
MF-2 - Selecting
--panelupdates only renderer memory and can be immediately reverted ·main/src/ipc/runpane.ts:338,frontend/src/hooks/useIPCEvents.ts:212· persist the validated selection throughpanelManager.setActivePanelbefore emitting the renderer focus request, and cover that call in the focus tests · violates the stated intent to select the requested panel exactly like clicking it in the UI- Evidence: the normal click path calls
panelApi.setActivePanelinfrontend/src/components/SessionView.tsx:575, which reachespanelManager.setActivePaneland updates the database. The new handler only calls the Zustand setter. On a pane switch,SessionViewthen readspanelApi.getActivePanelatfrontend/src/components/SessionView.tsx:247and writes that persisted value back into the store. - Failure scenario: focusing a different pane with
--panelbriefly sets the requested tab, then the pane-load effect restores the previously persisted tab, so the command reports success without leaving the requested panel selected.
- Evidence: the normal click path calls
Praise
- The main handler rejects archived or missing panes and cross-pane panel IDs before raising the window, and the new unit tests prove those negative paths.
Cannot verify
- The configured code-reviewer skill could not run because its required
.claude/agents/code-reviewer.mdinstruction file is absent locally, absent from this PR, and absent from Parsa's upstream skill folder. This review follows the skill's required report format and evidence standard, but does not claim the missing role instructions were applied.
914006e to
3aa3ff6
Compare
parsakhaz
left a comment
There was a problem hiding this comment.
Verdict: Approve - both Must-Fix findings are resolved and the final branch passes the required checks.
Counts: Must Fix: 0 (security: 0) · Should Fix: 0 · pass 2/3
Resolved
- MF-1 fixed - root typecheck now passes across frontend, main, runpane, and shared. The focus response has boundary decoding, request parsers retain
PaneCommandValue, and the unrelated broken session-deletion typing was restored. - MF-2 fixed -
--panelnow persists throughpanelManager.setActivePanelbefore the renderer event. Split-layout activation is also aligned through one tested pure layout operation.
Checks
pnpm typecheckpnpm lintpnpm --filter main exec vitest run src/ipc/runpane.test.ts(67 passed)pnpm --filter frontend exec vitest run src/utils/panelLayout.test.ts(55 passed)pnpm run check:runpane-contract- Isolated Electron and local-wrapper QA for pane focus, panel focus with persisted readback, and archived-pane rejection
|
Completed all three passes and pushed head Pass results
Verification
The disposable QA pane was archived, its managed worktree was removed, and its QA-only local branches/worktrees were deleted. The long temporary path caused an unrelated optional permission-socket warning, but the Pane daemon and focus flow remained healthy. Follow-ups
Left for parsa
|
parsakhaz
left a comment
There was a problem hiding this comment.
Verdict: Approve - both Must-Fix findings are resolved and the final branch passes the required checks.
Counts: Must Fix: 0 (security: 0) · Should Fix: 0 · pass 2/3
Resolved
- MF-1 fixed - root typecheck now passes across frontend, main, runpane, and shared. The focus response has boundary decoding, request parsers retain
PaneCommandValue, and the unrelated broken session-deletion typing was restored. - MF-2 fixed -
--panelnow persists throughpanelManager.setActivePanelbefore the renderer event. Split-layout activation is also aligned through one tested pure layout operation. - CI parity fixed - the Python wrapper now parses, dispatches, confirms, and invokes
panes focuswith the same local-control behavior as the npm wrapper.
Checks
pnpm typecheckpnpm lintpnpm --filter main exec vitest run src/ipc/runpane.test.ts(67 passed)pnpm --filter frontend exec vitest run src/utils/panelLayout.test.ts(55 passed)pnpm run check:runpane-contractnode scripts/test-runpane-contract.jsnode scripts/test-runpane-package-smoke.js- Isolated Electron and local-wrapper QA for pane focus, panel focus with persisted readback, and archived-pane rejection
Description
Adds
runpane panes focuscommand. Raises the Pane window, selects a pane, and optionally selects a specific panel within it.Closes #401.
Follows the same pattern as
panes rename/panes archive: contract.json -> generated artifacts -> CLI dispatch -> daemon handler -> renderer event.New pieces: window-raising plumbing (
show/restore/focuson BrowserWindow) and apane:focus-requestedIPC event that tells the renderer to callsetActiveSessionandsetActivePanel.Usage:
runpane panes focus --pane [--panel ] --source user|agent --yes [--json]
Requires
--yes. Accepts--source agent|user. Archived/missing panes and mismatched panels return clear errors, never falls back to focusing something else. Doctrine text added to contract: agents invoke this only on explicit user request, never proactively.Type of Change
Checklist
pnpm typecheckandpnpm lintlocallypnpm electron-devCritical Areas Modified
Screenshots (if applicable)
N/A. CLI command with no UI changes. The renderer-side change is a new IPC listener that calls existing store actions (
setActiveSession,setActivePanel).Additional Notes
5 new tests in
runpane.test.ts: happy path, minimized window restore + panel selection, archived pane rejection, unknown pane rejection, panel-not-belonging rejection. All 64 existing tests pass (1 pre-existing skip).Contract artifacts regenerated via
pnpm run generate-runpane-contract. The 6 generated files were not hand-edited.The handler sends
pane:focus-requestedviawindow.webContents.send()directly rather than throughsendRendererEventinevents.ts, since the handler already holds the raised window reference. This matches howwindow:focus-changedis dispatched inindex.ts.Automated QA
Status: passed on head
cbfcab8dusing an isolated Pane data directory.runpane:panes:focus.--panel, then focused its second terminal panel explicitly.panels listthat the requested second panel became the sole persisted active panel.panes focus.No screenshot is included because this command adds no new visual surface. Final human review can confirm native macOS window raising if desired.