mirror of
https://github.com/stablyai/orca.git
synced 2026-09-27 00:02:37 +00:00
* test(repro): demonstrate #10142 tab X close bypasses running-process confirmation
Unit repro: closeTerminalTab (the X-button/middle-click entry) never consults inspectRuntimeTerminalProcess and drops a tab with a live child.
E2E repro: Cmd+W shows 'Stop running command?' for a tab running sleep 300; cancelling then clicking the tab X closes it silently.
Co-authored-by: Orca <help@stably.ai>
* fix(terminal): confirm running-process close on every tab close path (#10142)
The tab-strip X button, middle-click and the tab context menu closed a
terminal with a live child process without asking, while Cmd+W raised
"Stop running command?" for the same tab. The probe lived only in
TerminalPane's pane-level close handler; every mouse entry point reaches
closeTerminalTab(), which guarded pinned tabs and nothing else.
Move the decision into closeTerminalTab, above the web-runtime branch so
paired/remote host-backed tabs are covered too, and give the last-pane
keyboard close back to it instead of probing twice:
- running-terminal-close-guard.ts probes every live PTY of the tab and
fails open on a rejected probe or a stale remote handle, matching what
Cmd+W already did. No live PTY ids => fully synchronous close, so idle,
parked and hibernated tabs keep today's behavior.
- shouldConfirmRunningTerminalClose keeps lifecycle echoes, bulk closes,
CLI/RPC closes and the post-confirmation re-entry off the modal path.
- A standalone confirm store drives RunningTerminalCloseDialog, which
reuses the existing CloseTerminalDialog (no new user-visible strings).
The request carries the tab label because a tab-strip close can target
a tab the user is not looking at, and dedupes by tab id.
- TerminalPane.handleRequestClosePane now delegates the last pane to
closeTerminalTab. Its transport ptyId is nullable by design, so the old
path silently skipped the prompt mid-reattach; the pane keeps its own
probe only for closing one pane of a split.
- Agent panes win the dialog copy when a split has both an agent and a
plain command busy, instead of depending on PTY spawn order.
- Tab-group closeItem ran leaveWorktreeIfEmpty synchronously after a close
that can now defer; it moves to onClosed and still honors skipEmptyCheck.
Co-authored-by: Orca <help@stably.ai>
* fix(terminal): close the running-process confirmation gaps on every path (#10142)
Follow-up hardening on the tab-close confirmation, from review of the first
pass:
- A pinned tab with `confirmClosePinnedTab` off never got the running-process
prompt on any path, including Cmd+W, which is a regression against the old
pane-level behavior: the pinned branch short-circuited on pinned-ness alone
and re-entered with `force`, which the running guard excludes. The pin prompt
now supersedes only when it will actually appear; with the setting off the
close falls through to the running guard.
- The probe chain had no `.catch`, so a throw in the decision (a copy-kind
lookup on a tab id makePaneKey rejects, a store subscriber) left the tab
silently unclosed with no user feedback. It now fails open, as the pane path
it replaced did.
- A wedged remote inspect RPC could leave the X button looking dead for its
full 15s timeout. The probe is now bounded; every close path shares the
bound, so keyboard and mouse still behave identically.
- The agent-vs-command copy had two resolvers on exactly the keyboard/mouse
seam this issue is about. terminal-close-copy-kind.ts is now the single
policy; TerminalPane and the tab-strip guard both call it.
- The running queue is async while the pinned queue is synchronous, so both
could be pending at once and stack two modal overlays. The running dialog now
waits for a visible pinned confirmation.
- Deduping a repeat close request dropped the second caller's callbacks; it now
folds them in, so both closes resolve from one prompt. Ticking "don't ask
again" also drains queued prompts instead of showing one the user just opted
out of, and a queued prompt no longer inherits the previous tab's tick.
closeTerminalTab drops its private pinned predicate for the shared
isUnifiedTabPinned, whose only consumer the previous commit had removed.
* test(e2e): wait for `sleep` to own the terminal before closing it (#10142)
The running-process close specs polled `hasChildProcesses` to decide the tab
was busy, but macOS starts the shell under `login`, so an initialising terminal
already reports a child before `sleep 300` runs. Both specs could therefore
press close against a shell that never started the command: the probe correctly
saw an idle terminal and closed without asking, and the adjudicated repro failed
against a correct fix.
Wait for `foregroundProcess === 'sleep'` instead. Assertions are unchanged, and
the repro still fails at the pre-fix baseline (a92d8e0b0d).
* fix(terminal): ask instead of closing when the close probe times out (#10142)
Round-1 review follow-ups.
- The 4s probe bound closed the tab outright, but `inspectRuntimeTerminalProcess`
gives remote runtimes a 15s RPC timeout: any probe taking 4-15s silently killed
a running remote command that Cmd+W used to prompt about, and the pane path now
delegates its last-pane close to this guard. An unanswered probe is unknown, not
idle, so the timeout raises the confirmation with every pty treated as a
candidate. Failing open still applies to an *answered* probe (rejection, stale
remote handle), which is the pre-existing pane behavior.
- The split-pane Cmd+W probe had no bound at all; it now shares the same one, so
the two paths give the same answer to the same question.
- The renamed regression spec dropped its repro scaffolding: the hardcoded
/tmp screenshot directory (also a cross-platform path violation) and the
title/docblock that still described the bug as open.
* fix(terminal): adopt the double-activation guard and layout pty lookup from #10167 (#10142)
Cross-referenced against @innocarpe's #10167, which solved the same issue.
Two things it got right that this branch did not:
- Queue actions now hold off for 350ms after a queued request replaces the
visible one, matching the sibling pinned-tab confirmation. Without it the
second click of a double-click aimed at one tab lands on the next tab's
prompt and kills a running process the user never saw asked about — the
exact bug class this PR exists to close.
- The pty lookup unions the layout bindings with ptyIdsByTabId. A mounting
pane is bound into the layout before the liveness map catches up, and the
store's own teardown collector unions both for that reason, so reading only
the map let a close slip through that window with no prompt.
Also replaces the render-time ref write that failed React Doctor's
"Ref mutated during render" rule: the queued-request checkbox reset now goes
through CloseTerminalDialog's existing subject-change reset instead of
remounting via key, which keeps the exit animation on one element.
Co-authored-by: Orca <help@stably.ai>
---------
Co-authored-by: Orca <help@stably.ai>
91 lines
3.4 KiB
TypeScript
91 lines
3.4 KiB
TypeScript
/**
|
|
* Regression for #10142: keyboard and mouse enforce the same running-process close
|
|
* confirmation. Both halves run against one tab with a live `sleep 300` child:
|
|
* 1. Cmd/Ctrl+W -> "Stop running command?" dialog (cancelled, tab survives).
|
|
* 2. X click -> the same dialog, and the tab is still there behind it.
|
|
*/
|
|
import { test, expect } from './helpers/orca-app'
|
|
import type { Page } from '@stablyai/playwright-test'
|
|
import {
|
|
waitForSessionReady,
|
|
waitForActiveWorktree,
|
|
getActiveTabId,
|
|
ensureTerminalVisible
|
|
} from './helpers/store'
|
|
import {
|
|
execInTerminal,
|
|
focusActiveTerminalInput,
|
|
waitForActivePanePtyId,
|
|
waitForActiveTerminalManager,
|
|
waitForPaneCount,
|
|
waitForTerminalOutput
|
|
} from './helpers/terminal'
|
|
|
|
const SORTABLE_TAB = '[data-testid="sortable-tab"]'
|
|
|
|
function countRenderedTabs(page: Page): Promise<number> {
|
|
return page.locator(SORTABLE_TAB).count()
|
|
}
|
|
|
|
function closeDialogTitle(page: Page) {
|
|
return page.getByText(/Stop running command\?|Stop this agent\?/)
|
|
}
|
|
|
|
test.describe.configure({ mode: 'serial' })
|
|
|
|
test('the tab X button applies the same running-process confirmation as Cmd+W', async ({
|
|
orcaPage
|
|
}) => {
|
|
test.setTimeout(120_000)
|
|
await waitForSessionReady(orcaPage)
|
|
await waitForActiveWorktree(orcaPage)
|
|
await ensureTerminalVisible(orcaPage)
|
|
const hasPaneManager = await waitForActiveTerminalManager(orcaPage, 30_000)
|
|
.then(() => true)
|
|
.catch(() => false)
|
|
test.skip(!hasPaneManager, 'Electron automation never mounted the live TerminalPane manager.')
|
|
await waitForPaneCount(orcaPage, 1, 30_000)
|
|
|
|
const ptyId = await waitForActivePanePtyId(orcaPage)
|
|
await execInTerminal(orcaPage, ptyId, 'echo repro-10142-ready')
|
|
await waitForTerminalOutput(orcaPage, 'repro-10142-ready', 20_000)
|
|
await execInTerminal(orcaPage, ptyId, 'sleep 300')
|
|
// Only press close once `sleep` is the foreground process; otherwise the probe
|
|
// legitimately sees an idle shell and closing is correct. `hasChildProcesses` alone is
|
|
// not enough: macOS spawns the shell under `login`, so a still-initialising terminal
|
|
// reports a child before `sleep 300` has run.
|
|
await expect
|
|
.poll(
|
|
async () =>
|
|
(await orcaPage.evaluate((id) => window.api.pty.inspectProcess(id), ptyId))
|
|
.foregroundProcess,
|
|
{ timeout: 20_000, message: 'sleep 300 never became the foreground process' }
|
|
)
|
|
.toBe('sleep')
|
|
|
|
const busyTabId = (await getActiveTabId(orcaPage))!
|
|
const busyTab = orcaPage.locator(`${SORTABLE_TAB}[data-tab-id="${busyTabId}"]`).first()
|
|
|
|
// 1. Keyboard close prompts.
|
|
await focusActiveTerminalInput(orcaPage)
|
|
await orcaPage.keyboard.press(process.platform === 'darwin' ? 'Meta+w' : 'Control+w')
|
|
await expect(closeDialogTitle(orcaPage)).toBeVisible({ timeout: 15_000 })
|
|
await orcaPage.getByRole('button', { name: /^Cancel$/ }).click()
|
|
await expect(closeDialogTitle(orcaPage)).toBeHidden()
|
|
await expect(busyTab).toBeVisible()
|
|
const tabsBefore = await countRenderedTabs(orcaPage)
|
|
|
|
// 2. Same tab, same running child, mouse close.
|
|
await busyTab.hover()
|
|
await busyTab.getByRole('button', { name: /^Close tab /i }).click()
|
|
await orcaPage.waitForTimeout(1_500)
|
|
|
|
expect(
|
|
{
|
|
confirmDialogVisible: await closeDialogTitle(orcaPage).isVisible(),
|
|
tabStillPresent: (await countRenderedTabs(orcaPage)) === tabsBefore
|
|
},
|
|
'X-button close must apply the same running-process confirmation as Cmd+W'
|
|
).toEqual({ confirmDialogVisible: true, tabStillPresent: true })
|
|
})
|