fix(skills): keep revalidated terminal safe and focused

This commit is contained in:
Brennan Benson
2026-07-16 16:35:20 -07:00
parent 820e1787a0
commit 9865fce3f9
2 changed files with 79 additions and 3 deletions
@@ -1,6 +1,6 @@
// @vitest-environment happy-dom
import { act, useState, type ReactNode } from 'react'
import { act, useLayoutEffect, useRef, useState, type ReactNode } from 'react'
import { createRoot, type Root } from 'react-dom/client'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import type {
@@ -24,6 +24,7 @@ const mocks = vi.hoisted(() => ({
onInteracted?: (method: 'keyboard' | 'pointer', event?: { key?: string }) => void
onTerminalExit?: () => void
}[],
terminalCommits: [] as [command: string, inert: boolean][],
notifyChanged: vi.fn()
}))
@@ -42,8 +43,20 @@ vi.mock('@/hooks/useInstalledAgentSkills', () => ({
vi.mock('@/components/onboarding/OnboardingInlineCommandTerminal', () => ({
OnboardingInlineCommandTerminal: (props: (typeof mocks.terminalProps)[number]) => {
const terminalRef = useRef<HTMLDivElement>(null)
mocks.terminalProps.push(props)
return <div data-testid="update-terminal">{props.command}</div>
useLayoutEffect(() => {
mocks.terminalCommits.push([
props.command,
terminalRef.current?.parentElement?.inert ?? false
])
})
return (
<div ref={terminalRef} data-testid="update-terminal">
{props.command}
<textarea data-testid="update-terminal-input" />
</div>
)
}
}))
@@ -155,6 +168,7 @@ describe('SkillFreshnessUpdateDialog', () => {
mocks.refresh.mockReset()
mocks.notifyChanged.mockReset()
mocks.terminalProps.length = 0
mocks.terminalCommits.length = 0
})
afterEach(async () => {
@@ -200,6 +214,7 @@ describe('SkillFreshnessUpdateDialog', () => {
it('shows the up-to-date state once every installation is current', async () => {
await renderDialog()
await openViaRequest()
mocks.terminalCommits.length = 0
mocks.inventory = {
schemaVersion: 1,
@@ -211,6 +226,7 @@ describe('SkillFreshnessUpdateDialog', () => {
}
await rerender()
expect(mocks.terminalCommits).toContainEqual(['npx skills update orca-cli --global', true])
expect(container?.textContent).toContain('All installed Orca skills are up to date.')
expect(container?.querySelector('[data-testid="update-terminal"]')).toBeNull()
})
@@ -267,7 +283,11 @@ describe('SkillFreshnessUpdateDialog', () => {
await renderDialog()
await openViaRequest()
const firstTerminal = container?.querySelector('[data-testid="update-terminal"]')
const terminalInput = container?.querySelector<HTMLTextAreaElement>(
'[data-testid="update-terminal-input"]'
)
const terminalWrapper = container?.querySelector<HTMLElement>('[data-skill-update-terminal]')
terminalInput?.focus()
expect(firstTerminal).not.toBeNull()
expect(terminalWrapper?.hasAttribute('inert')).toBe(false)
@@ -278,6 +298,7 @@ describe('SkillFreshnessUpdateDialog', () => {
expect(container?.textContent).toContain('Checking installed Orca skills')
expect(container?.querySelector('[data-testid="update-terminal"]')).toBe(firstTerminal)
expect(terminalWrapper?.hasAttribute('inert')).toBe(true)
terminalInput?.blur()
mocks.inventory = { ...eligibleInventory(), scannedAt: 2 }
mocks.loading = false
@@ -285,6 +306,20 @@ describe('SkillFreshnessUpdateDialog', () => {
expect(container?.querySelector('[data-testid="update-terminal"]')).toBe(firstTerminal)
expect(terminalWrapper?.hasAttribute('inert')).toBe(false)
expect(document.activeElement).toBe(terminalInput)
mocks.inventory = null
mocks.loading = true
await rerender()
const closeButton = Array.from(container?.querySelectorAll('button') ?? []).find(
(candidate) => candidate.textContent?.trim() === 'Close'
)
closeButton?.focus()
mocks.inventory = { ...eligibleInventory(), scannedAt: 3 }
mocks.loading = false
await rerender()
expect(document.activeElement).toBe(closeButton)
})
it('shows a failed scan as an error instead of indefinite progress', async () => {
@@ -138,6 +138,9 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element {
const [terminalCommand, setTerminalCommand] = useState<string | null>(null)
const [awaitingExitRefresh, setAwaitingExitRefresh] = useState(false)
const terminalSubmittedRef = useRef(false)
const terminalWrapperRef = useRef<HTMLDivElement>(null)
const terminalFocusTargetRef = useRef<HTMLElement | null>(null)
const terminalAuthorizationWasPendingRef = useRef(false)
const inventoryAtTerminalExitRef = useRef<SkillFreshnessInventory | null>(null)
const inventory = state.inventory
const eligibleNames = useMemo(() => inventory?.eligibleUpdateNames ?? [], [inventory])
@@ -152,7 +155,10 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element {
const terminalAuthorizationPending =
terminalCommand !== null &&
!terminalSubmittedRef.current &&
(state.loading || state.error !== null || inventory === null)
(state.loading ||
state.error !== null ||
inventory === null ||
terminalCommand !== updateCommand)
useEffect(() => {
if (!open) {
@@ -195,6 +201,27 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element {
updateCommand
])
useEffect(() => {
if (terminalCommand === null) {
// Why: the dialog stays mounted while closed, so do not retain a detached
// xterm subtree after the draft exits or loses authorization.
terminalFocusTargetRef.current = null
}
const wasPending = terminalAuthorizationWasPendingRef.current
terminalAuthorizationWasPendingRef.current = terminalAuthorizationPending
if (!wasPending || terminalAuthorizationPending) {
return
}
const focusTarget = terminalFocusTargetRef.current
if (!focusTarget?.isConnected || !terminalWrapperRef.current?.contains(focusTarget)) {
terminalFocusTargetRef.current = null
return
}
// Why: Chromium drops focus when an ancestor becomes inert; unchanged
// authorization should return focus to the preserved xterm instance.
focusTarget.focus()
}, [terminalAuthorizationPending, terminalCommand])
const handleOpenChange = (next: boolean): void => {
// Why: closing is the natural point to re-observe bytes so a completed update
// clears the state and the lingering nudge does not fire again.
@@ -247,10 +274,24 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element {
{terminalCommand ? (
<div
ref={terminalWrapperRef}
data-skill-update-terminal=""
aria-busy={terminalAuthorizationPending}
inert={terminalAuthorizationPending}
className={terminalAuthorizationPending ? 'opacity-50' : undefined}
onFocusCapture={(event) => {
terminalFocusTargetRef.current = event.target as HTMLElement
}}
onBlurCapture={(event) => {
// Why: a concrete next target means the user moved focus; inert
// uses a null target, which should be restored after revalidation.
if (
event.relatedTarget instanceof Node &&
!event.currentTarget.contains(event.relatedTarget)
) {
terminalFocusTargetRef.current = null
}
}}
>
<OnboardingInlineCommandTerminal
key={terminalCommand}