-
Notifications
You must be signed in to change notification settings - Fork 11
fix: Escape key reliably interrupts agent chat sessions #130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
danshapiro
wants to merge
6
commits into
main
Choose a base branch
from
fix/escape-interrupt
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
7177902
docs: update freshell-orchestration skill with auth token location an…
061229c
docs: add implementation plan for escape interrupt fix
05061fe
fix: add container-level Escape handler so interrupt works regardless…
c0df0ad
refactor: remove redundant Escape handler from ChatComposer textarea
065e3bd
chore: sync package-lock.json version to 0.6.0
4d4170f
fix: override NODE_ENV=production in vitest configs to fix all test f…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,299 @@ | ||
| # Fix Escape Key Not Interrupting Freshclaude Agent Chat | ||
|
|
||
| > **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. | ||
|
|
||
| **Goal:** Make the Escape key reliably interrupt a running Freshclaude agent chat session, regardless of which element inside the agent-chat pane has focus. | ||
|
|
||
| **Architecture:** Move the Escape-to-interrupt handler from the `<textarea>` `onKeyDown` in `ChatComposer` up to a container-level `onKeyDown` on the `AgentChatView` wrapper `<div>`. Add `tabIndex={-1}` so the container can receive keyboard events. This scopes the interrupt to the agent-chat pane without affecting other panes (terminals, editors, browsers). The existing `handleContainerPointerUp` already restores focus inside the container on click. | ||
|
|
||
| **Tech Stack:** React 18, Vitest, Testing Library, userEvent | ||
|
|
||
| **Known issue:** All client-side tests currently fail with `act(...) is not supported in production builds of React` — this is a pre-existing environment issue on main, not caused by our changes. Write tests correctly; verify they are structurally sound even if the runner rejects them. | ||
|
|
||
| --- | ||
|
|
||
| ### Task 1: Add container-level Escape handler to AgentChatView | ||
|
|
||
| **Files:** | ||
| - Modify: `src/components/agent-chat/AgentChatView.tsx:377-378` | ||
|
|
||
| **Step 1: Write the failing test** | ||
|
|
||
| Create a new test file that verifies the container-level Escape behavior. This tests AgentChatView in isolation with a mocked Redux store and WebSocket client. | ||
|
|
||
| Create: `test/unit/client/components/agent-chat/AgentChatView-interrupt.test.tsx` | ||
|
|
||
| ```tsx | ||
| import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest' | ||
| import { render, screen, cleanup, fireEvent } from '@testing-library/react' | ||
| import { Provider } from 'react-redux' | ||
| import { configureStore } from '@reduxjs/toolkit' | ||
| import AgentChatView from '../../../../../src/components/agent-chat/AgentChatView' | ||
| import agentChatReducer from '../../../../../src/store/agentChatSlice' | ||
| import panesReducer from '../../../../../src/store/panesSlice' | ||
| import settingsReducer from '../../../../../src/store/settingsSlice' | ||
| import type { AgentChatPaneContent } from '../../../../../src/store/paneTypes' | ||
|
|
||
| // Mock ws-client to capture sent messages | ||
| const mockSend = vi.fn() | ||
| vi.mock('../../../../../src/lib/ws-client', () => ({ | ||
| getWsClient: () => ({ | ||
| send: mockSend, | ||
| onReconnect: () => () => {}, | ||
| }), | ||
| })) | ||
|
|
||
| // Mock api | ||
| vi.mock('../../../../../src/lib/api', () => ({ | ||
| api: { get: vi.fn(), post: vi.fn() }, | ||
| })) | ||
|
|
||
| function makeStore(sessionState?: Record<string, unknown>) { | ||
| return configureStore({ | ||
| reducer: { | ||
| agentChat: agentChatReducer, | ||
| panes: panesReducer, | ||
| settings: settingsReducer, | ||
| }, | ||
| preloadedState: sessionState, | ||
| }) | ||
| } | ||
|
|
||
| const basePaneContent: AgentChatPaneContent = { | ||
| kind: 'agent-chat', | ||
| provider: 'freshclaude', | ||
| createRequestId: 'req-1', | ||
| sessionId: 'sess-1', | ||
| status: 'running', | ||
| } | ||
|
|
||
| describe('AgentChatView Escape interrupt', () => { | ||
| beforeEach(() => { | ||
| mockSend.mockClear() | ||
| }) | ||
| afterEach(() => { | ||
| cleanup() | ||
| }) | ||
|
|
||
| it('sends sdk.interrupt when Escape is pressed on the container while running', () => { | ||
| const store = makeStore() | ||
| render( | ||
| <Provider store={store}> | ||
| <AgentChatView tabId="tab-1" paneId="pane-1" paneContent={basePaneContent} /> | ||
| </Provider> | ||
| ) | ||
| const container = screen.getByRole('region', { name: /chat/i }) | ||
| fireEvent.keyDown(container, { key: 'Escape' }) | ||
| expect(mockSend).toHaveBeenCalledWith({ | ||
| type: 'sdk.interrupt', | ||
| sessionId: 'sess-1', | ||
| }) | ||
| }) | ||
|
|
||
| it('does not send sdk.interrupt when Escape is pressed while idle', () => { | ||
| const store = makeStore() | ||
| const idleContent = { ...basePaneContent, status: 'idle' as const } | ||
| render( | ||
| <Provider store={store}> | ||
| <AgentChatView tabId="tab-1" paneId="pane-1" paneContent={idleContent} /> | ||
| </Provider> | ||
| ) | ||
| const container = screen.getByRole('region', { name: /chat/i }) | ||
| fireEvent.keyDown(container, { key: 'Escape' }) | ||
| expect(mockSend).not.toHaveBeenCalledWith( | ||
| expect.objectContaining({ type: 'sdk.interrupt' }) | ||
| ) | ||
| }) | ||
|
|
||
| it('does not send sdk.interrupt for non-Escape keys while running', () => { | ||
| const store = makeStore() | ||
| render( | ||
| <Provider store={store}> | ||
| <AgentChatView tabId="tab-1" paneId="pane-1" paneContent={basePaneContent} /> | ||
| </Provider> | ||
| ) | ||
| const container = screen.getByRole('region', { name: /chat/i }) | ||
| fireEvent.keyDown(container, { key: 'a' }) | ||
| expect(mockSend).not.toHaveBeenCalledWith( | ||
| expect.objectContaining({ type: 'sdk.interrupt' }) | ||
| ) | ||
| }) | ||
| }) | ||
| ``` | ||
|
|
||
| **Step 2: Run test to verify it fails** | ||
|
|
||
| Run: `npx vitest run test/unit/client/components/agent-chat/AgentChatView-interrupt.test.tsx` | ||
|
|
||
| Expected: FAIL — the container doesn't have `onKeyDown` or `tabIndex` yet, so `fireEvent.keyDown` on the container won't trigger any interrupt. (Note: may also fail with pre-existing React `act()` error.) | ||
|
|
||
| **Step 3: Add container-level onKeyDown and tabIndex to AgentChatView** | ||
|
|
||
| In `src/components/agent-chat/AgentChatView.tsx`: | ||
|
|
||
| 1. Add a `handleContainerKeyDown` callback (near line 237, after `handleContainerPointerUp`): | ||
|
|
||
| ```tsx | ||
| const handleContainerKeyDown = useCallback((e: React.KeyboardEvent) => { | ||
| if (e.key === 'Escape' && isRunning) { | ||
| e.preventDefault() | ||
| handleInterrupt() | ||
| } | ||
| }, [isRunning, handleInterrupt]) | ||
| ``` | ||
|
|
||
| Note: `isRunning` is derived at line 305 (`const isRunning = paneContent.status === 'running'`). The callback references `isRunning` and `handleInterrupt`, both of which are defined before the return statement. Place the callback definition after line 305 (after `isRunning` is derived) so the dependency is available. | ||
|
|
||
| 2. Update the outer `<div>` on line 378 to add `tabIndex={-1}` and `onKeyDown`: | ||
|
|
||
| Change: | ||
| ```tsx | ||
| <div className={cn('h-full w-full flex flex-col', hidden ? 'tab-hidden' : 'tab-visible')} role="region" aria-label={`${providerLabel} Chat`} onPointerUp={handleContainerPointerUp}> | ||
| ``` | ||
|
|
||
| To: | ||
| ```tsx | ||
| <div className={cn('h-full w-full flex flex-col', hidden ? 'tab-hidden' : 'tab-visible')} role="region" aria-label={`${providerLabel} Chat`} tabIndex={-1} onKeyDown={handleContainerKeyDown} onPointerUp={handleContainerPointerUp}> | ||
| ``` | ||
|
|
||
| **Step 4: Run test to verify it passes** | ||
|
|
||
| Run: `npx vitest run test/unit/client/components/agent-chat/AgentChatView-interrupt.test.tsx` | ||
|
|
||
| Expected: PASS (or pre-existing React act() failure — structurally verify the test is correct) | ||
|
|
||
| **Step 5: Commit** | ||
|
|
||
| ```bash | ||
| git add src/components/agent-chat/AgentChatView.tsx test/unit/client/components/agent-chat/AgentChatView-interrupt.test.tsx | ||
| git commit -m "fix: add container-level Escape handler so interrupt works regardless of focus" | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### Task 2: Remove redundant Escape handler from ChatComposer | ||
|
|
||
| Now that the container handles Escape, the textarea-level handler is redundant. Remove it to avoid double-firing. | ||
|
|
||
| **Files:** | ||
| - Modify: `src/components/agent-chat/ChatComposer.tsx:41-44` | ||
| - Modify: `src/components/agent-chat/ChatComposer.tsx:9` (props interface) | ||
| - Modify: `src/components/agent-chat/ChatComposer.tsx:17` (destructured props) | ||
| - Modify: `src/components/agent-chat/ChatComposer.tsx:45` (useCallback deps) | ||
| - Modify: `test/unit/client/components/agent-chat/ChatComposer.test.tsx:58-76` | ||
|
|
||
| **Step 1: Update the ChatComposer tests** | ||
|
|
||
| The two Escape-specific tests in `ChatComposer.test.tsx` (lines 58-76) should be removed since Escape handling is now the container's responsibility (tested in Task 1). The `onInterrupt` prop remains because the Stop button still uses it. | ||
|
|
||
| Remove the two tests: | ||
| - `'calls onInterrupt when Escape is pressed while running'` (lines 58-66) | ||
| - `'does not call onInterrupt when Escape is pressed while not running'` (lines 68-76) | ||
|
|
||
| **Step 2: Run tests to verify the removed tests no longer exist** | ||
|
|
||
| Run: `npx vitest run test/unit/client/components/agent-chat/ChatComposer.test.tsx` | ||
|
|
||
| Expected: 7 tests (down from 9). The stop-button click test still exercises `onInterrupt`. | ||
|
|
||
| **Step 3: Remove Escape handling from ChatComposer.tsx** | ||
|
|
||
| In `src/components/agent-chat/ChatComposer.tsx`, remove the `isRunning` dependency from `handleKeyDown`: | ||
|
|
||
| Change the `handleKeyDown` callback (lines 36-45) from: | ||
| ```tsx | ||
| const handleKeyDown = useCallback((e: KeyboardEvent<HTMLTextAreaElement>) => { | ||
| if (e.key === 'Enter' && !e.shiftKey) { | ||
| e.preventDefault() | ||
| handleSend() | ||
| } | ||
| if (e.key === 'Escape' && isRunning) { | ||
| e.preventDefault() | ||
| onInterrupt() | ||
| } | ||
| }, [handleSend, isRunning, onInterrupt]) | ||
| ``` | ||
|
|
||
| To: | ||
| ```tsx | ||
| const handleKeyDown = useCallback((e: KeyboardEvent<HTMLTextAreaElement>) => { | ||
| if (e.key === 'Enter' && !e.shiftKey) { | ||
| e.preventDefault() | ||
| handleSend() | ||
| } | ||
| }, [handleSend]) | ||
| ``` | ||
|
|
||
| Also remove `isRunning` from the `ChatComposerProps` interface (line 13) and the destructured props (line 17), since it's no longer needed by ChatComposer. **Keep `onInterrupt`** — it's still used by the Stop button (line 77). | ||
|
|
||
| Remove line 13: `isRunning?: boolean` | ||
|
|
||
| Update line 17 from: | ||
| ```tsx | ||
| function ChatComposer({ onSend, onInterrupt, disabled, isRunning, placeholder }, ref) { | ||
| ``` | ||
| To: | ||
| ```tsx | ||
| function ChatComposer({ onSend, onInterrupt, disabled, placeholder }, ref) { | ||
| ``` | ||
|
|
||
| Update the JSX that conditionally renders the Stop button (line 74). It currently checks `isRunning` which we're removing from props. This needs to come from a new prop or we need to keep `isRunning`. | ||
|
|
||
| **Wait — reconsider.** `isRunning` is also used on line 74 to toggle between the Stop button and Send button. We need to keep `isRunning` as a prop for that UI toggle. Only remove the Escape handler from `handleKeyDown`, not the `isRunning` prop. | ||
|
|
||
| Revised change — just remove the Escape block from `handleKeyDown`: | ||
|
|
||
| ```tsx | ||
| const handleKeyDown = useCallback((e: KeyboardEvent<HTMLTextAreaElement>) => { | ||
| if (e.key === 'Enter' && !e.shiftKey) { | ||
| e.preventDefault() | ||
| handleSend() | ||
| } | ||
| }, [handleSend]) | ||
| ``` | ||
|
|
||
| And remove `isRunning` and `onInterrupt` from the `useCallback` deps since they're no longer referenced in the callback. | ||
|
|
||
| **Step 4: Update AgentChatView to stop passing isRunning to ChatComposer for Escape** | ||
|
|
||
| No change needed — `AgentChatView` still passes `isRunning` and `onInterrupt` to `ChatComposer` for the Stop button toggle. This is correct. | ||
|
|
||
| **Step 5: Run all agent-chat tests** | ||
|
|
||
| Run: `npx vitest run test/unit/client/components/agent-chat/` | ||
|
|
||
| Expected: All tests pass (structurally correct; may have pre-existing act() failures). | ||
|
|
||
| **Step 6: Commit** | ||
|
|
||
| ```bash | ||
| git add src/components/agent-chat/ChatComposer.tsx test/unit/client/components/agent-chat/ChatComposer.test.tsx | ||
| git commit -m "refactor: remove redundant Escape handler from ChatComposer textarea" | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### Task 3: Verify no regressions and clean up | ||
|
|
||
| **Step 1: Run the full test suite** | ||
|
|
||
| Run: `npm test` | ||
|
|
||
| Expected: Same pass/fail counts as baseline (no new failures introduced). | ||
|
|
||
| **Step 2: Run lint** | ||
|
|
||
| Run: `npm run lint` | ||
|
|
||
| Expected: No new lint errors. The `tabIndex={-1}` on a `<div>` with `role="region"` and `onKeyDown` is valid per jsx-a11y rules (interactive handlers on focusable elements). | ||
|
|
||
| **Step 3: Manual smoke test (if possible)** | ||
|
|
||
| If the dev server can be started in the worktree, open Freshclaude, send a prompt that generates a long response, click somewhere in the message area (not the textarea), then press Escape. The generation should stop. | ||
|
|
||
| **Step 4: Final commit (if any lint/cleanup needed)** | ||
|
|
||
| ```bash | ||
| git add -A | ||
| git commit -m "chore: lint and cleanup" | ||
| ``` |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This handler interrupts any running session for every bubbled
Escapekeydown inside the pane, including when the user is trying to dismiss the settings popover or close a select/menu.AgentChatSettingsalready binds Escape-to-close behavior, but the containeronKeyDownruns earlier in the bubble path, so pressing Escape while settings are open will now also sendsdk.interruptand stop generation unexpectedly. Please scope this shortcut to contexts where Escape is intended to mean “interrupt” (e.g., composer/message area) or explicitly skip dialog/form controls.Useful? React with 👍 / 👎.