Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions apps/server/src/keybindings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,29 @@ it.layer(NodeServices.layer)("keybindings", (it) => {
}).pipe(Effect.provide(makeKeybindingsLayer())),
);

it.effect("replaces a rule whose stored key uses an alias spelling", () =>
Effect.gen(function* () {
const { keybindingsConfigPath } = yield* ServerConfig.ServerConfig;
yield* writeKeybindingsConfig(keybindingsConfigPath, [
{ key: "escape", command: "script.run-tests.run" },
]);
yield* Effect.gen(function* () {
const keybindings = yield* Keybindings.Keybindings;
// The settings UI renders a stored "escape" rule as "esc", so the
// replace target arrives spelled differently than it was persisted.
return yield* keybindings.upsertKeybindingRule({
key: "mod+m",
command: "script.run-tests.run",
replace: { key: "esc", command: "script.run-tests.run" },
});
});

const persisted = yield* readKeybindingsConfig(keybindingsConfigPath);
const persistedView = persisted.map(({ key, command }) => ({ key, command }));
assert.deepEqual(persistedView, [{ key: "mod+m", command: "script.run-tests.run" }]);
}).pipe(Effect.provide(makeKeybindingsLayer())),
);

it.effect("removes only the targeted custom keybinding", () =>
Effect.gen(function* () {
const { keybindingsConfigPath } = yield* ServerConfig.ServerConfig;
Expand Down
20 changes: 15 additions & 5 deletions apps/server/src/keybindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,21 @@ export const ResolvedKeybindingsFromConfig = Schema.Array(ResolvedKeybindingFrom
);

function isSameKeybindingRule(left: KeybindingRule, right: KeybindingRule): boolean {
return (
left.command === right.command &&
left.key === right.key &&
(left.when ?? undefined) === (right.when ?? undefined)
);
if (left.command !== right.command) return false;
if ((left.when ?? undefined) !== (right.when ?? undefined)) return false;
if (left.key === right.key) return true;
// A key can be spelled more than one way ("esc"/"escape", "space"/" "), and
// the settings UI renders a stored rule back as the alias. Comparing raw
// strings would then fail to match a rule against itself, so replacing that
// rule would leave the original behind instead of updating it.
const leftKey = canonicalKeybindingKey(left);
return leftKey !== null && leftKey === canonicalKeybindingKey(right);
}

function canonicalKeybindingKey(rule: KeybindingRule): string | null {
const parsed = parseKeybindingShortcut(rule.key);
if (!parsed) return null;
return encodeShortcut(parsed);
}

function keybindingShortcutContext(rule: KeybindingRule): string | null {
Expand Down
16 changes: 16 additions & 0 deletions apps/web/src/components/ChatView.logic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,22 @@ import {
} from "../lib/terminalContext";
import type { DraftThreadEnvMode } from "../composerDraftStore";

/**
* Matches any open floating layer: dialogs, menus, selects, popovers, and
* ad-hoc dialogs that only carry `role="dialog"` (like the expanded image
* viewer). Used to suppress global key behavior — type-to-focus and the
* thread.interrupt shortcut — while an overlay owns the keyboard.
*/
export const OPEN_FLOATING_LAYER_SELECTOR = [
'[data-slot="dialog"]',
'[data-slot="menu-popup"]',
'[data-slot="select-popup"]',
'[data-slot="popover-popup"]',
'[data-slot="combobox-popup"]',
'[data-slot="autocomplete-popup"]',
'[role="dialog"]',
].join(",");

export const LAST_INVOKED_SCRIPT_BY_PROJECT_KEY = "t3code:last-invoked-script-by-project";
export const MAX_HIDDEN_MOUNTED_TERMINAL_THREADS = 10;
export const MAX_HIDDEN_MOUNTED_PREVIEW_THREADS = 3;
Expand Down
23 changes: 12 additions & 11 deletions apps/web/src/components/ChatView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ import {
PullRequestDialogState,
cloneComposerImageForRetry,
deriveLockedProvider,
OPEN_FLOATING_LAYER_SELECTOR,
readFileAsDataUrl,
reconcileMountedTerminalThreadIds,
resolveThreadMetadataUpdateForNextTurn,
Expand Down Expand Up @@ -436,14 +437,7 @@ const TYPE_TO_FOCUS_INTERACTIVE_SELECTOR = [
'[role="switch"]',
'[role="tab"]',
].join(",");
const TYPE_TO_FOCUS_FLOATING_LAYER_SELECTOR = [
'[data-slot="dialog"]',
'[data-slot="menu-popup"]',
'[data-slot="select-popup"]',
'[data-slot="popover-popup"]',
'[data-slot="combobox-popup"]',
'[data-slot="autocomplete-popup"]',
].join(",");
const TYPE_TO_FOCUS_FLOATING_LAYER_SELECTOR = OPEN_FLOATING_LAYER_SELECTOR;

type EnvironmentUnavailableState = {
readonly environmentId: EnvironmentId;
Expand Down Expand Up @@ -4570,7 +4564,17 @@ function ChatViewContent(props: ChatViewProps) {
modelPickerOpen: composerRef.current?.isModelPickerOpen() ?? false,
};

const command = resolveShortcutCommand(event, keybindings, {
context: shortcutContext,
});

// Type-to-focus only claims keys that resolve to no binding: a user who
// explicitly bound a bare printable key (for example to
// thread.interrupt) means the command, and this handler runs in the
// capture phase, so consuming the key here would shadow bindings
// dispatched by bubble-phase listeners.
if (
!command &&
!shortcutContext.terminalFocus &&
!shortcutContext.modelPickerOpen &&
shouldTypeToFocusComposer(event)
Expand All @@ -4582,9 +4586,6 @@ function ChatViewContent(props: ChatViewProps) {
}
}

const command = resolveShortcutCommand(event, keybindings, {
context: shortcutContext,
});
if (!command) return;

if (command === "terminal.toggle") {
Expand Down
53 changes: 52 additions & 1 deletion apps/web/src/components/chat/ChatComposer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,14 @@ import {
detectComposerTrigger,
expandCollapsedComposerCursor,
replaceTextRange,
shouldInterruptRunningThreadFromKeybinding,
shouldSubmitComposerOnEnter,
} from "../../composer-logic";
import { deriveComposerSendState, readFileAsDataUrl } from "../ChatView.logic";
import {
deriveComposerSendState,
OPEN_FLOATING_LAYER_SELECTOR,
readFileAsDataUrl,
} from "../ChatView.logic";
import {
dataTransferHasComposerMention,
makeComposerMentionDragHandlers,
Expand Down Expand Up @@ -2275,6 +2280,52 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps)
terminalOpen,
]);

// Interrupting the running turn is a keybinding rather than a hard-coded key
// so it shows up in Settings and can be rebound. It only claims the shortcut
// while a turn is running; otherwise the key falls through untouched.
//
// Bubble phase, unlike the capture-phase composer.stash listener above:
// stash must beat the browser's save dialog, while interrupt must lose to
// any open overlay that dismisses on the same key. By the time the event
// bubbles to window every overlay handler has run, and a dismiss marks the
// event consumed via preventDefault.
useEffect(() => {
const handler = (event: globalThis.KeyboardEvent) => {
Comment thread
macroscopeapp[bot] marked this conversation as resolved.
// Interrupt is one-shot: key auto-repeat would spray concurrent
// interrupt requests that race each other for the same turn.
if (event.repeat) return;
// An overlay (dialog, menu, select, command palette) already consumed
// this press to dismiss itself.
if (event.defaultPrevented) return;
const command = resolveShortcutCommand(event, keybindings, {
context: {
terminalFocus: getTerminalFocusOwner() !== null,
terminalOpen,
modelPickerOpen: isComposerModelPickerOpen,
},
});
if (
!shouldInterruptRunningThreadFromKeybinding({ command, isRunning: phase === "running" })
) {
return;
}
// An open floating layer owns the keyboard even when its dismiss
// handler has not run yet — window bubble listeners fire in
// registration order, so a later-mounted overlay (the expanded image
// viewer) marks the event only after this handler has already seen it.
// Checking the DOM instead of the event answers "is an overlay open"
// regardless of listener ordering; the model picker is excluded via
// the default binding's when clause.
if (isCommandPaletteOpen() || document.querySelector(OPEN_FLOATING_LAYER_SELECTOR) !== null) {
return;
}
event.preventDefault();
Comment thread
macroscopeapp[bot] marked this conversation as resolved.
onInterrupt();
};
window.addEventListener("keydown", handler);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High chat/ChatComposer.tsx:2317

The thread.interrupt listener on window runs in bubble phase, but ChatView's type-to-focus handler runs in capture phase on the same window. When thread.interrupt is rebound to a bare printable key (e.g. x) and focus is on non-interactive chat content, the capture-phase handler calls shouldTypeToFocusComposer, inserts the key into the composer, and calls stopPropagation — so the bubble-phase interrupt handler never fires and the running turn is not interrupted. The interrupt binding needs to be resolved before type-to-focus can consume printable shortcuts, or otherwise excluded from that path.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/chat/ChatComposer.tsx around line 2317:

The `thread.interrupt` listener on `window` runs in bubble phase, but `ChatView`'s type-to-focus handler runs in capture phase on the same `window`. When `thread.interrupt` is rebound to a bare printable key (e.g. `x`) and focus is on non-interactive chat content, the capture-phase handler calls `shouldTypeToFocusComposer`, inserts the key into the composer, and calls `stopPropagation` — so the bubble-phase interrupt handler never fires and the running turn is not interrupted. The interrupt binding needs to be resolved before type-to-focus can consume printable shortcuts, or otherwise excluded from that path.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid. Fixed in b00ab8c: ChatView's handler now resolves the shortcut before the type-to-focus branch, and type-to-focus only claims keys that resolve to no binding. An explicit binding on a bare printable key wins consistently — for every command, not just thread.interrupt — and the event then falls through the dispatch chain untouched to the bubble-phase interrupt listener. One deliberate trade-off: such a binding now always claims its key, so it no longer types into the composer even while the thread is idle. That matches how every modifier-chord binding already behaves, and the alternative — key types when idle, interrupts when running — seemed worse. Shipped defaults are all modifier chords, so nothing changes without a custom binding.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

return () => window.removeEventListener("keydown", handler);
Comment thread
cursor[bot] marked this conversation as resolved.
}, [isComposerModelPickerOpen, keybindings, onInterrupt, phase, terminalOpen]);

// ------------------------------------------------------------------
// Callbacks: images
// ------------------------------------------------------------------
Expand Down
22 changes: 22 additions & 0 deletions apps/web/src/composer-logic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
isCollapsedCursorAdjacentToInlineToken,
parseStandaloneComposerSlashCommand,
replaceTextRange,
shouldInterruptRunningThreadFromKeybinding,
shouldSubmitComposerOnEnter,
} from "./composer-logic";
import { INLINE_TERMINAL_CONTEXT_PLACEHOLDER } from "./lib/terminalContext";
Expand All @@ -26,6 +27,27 @@ describe("shouldSubmitComposerOnEnter", () => {
});
});

describe("shouldInterruptRunningThreadFromKeybinding", () => {
it("interrupts a running thread", () => {
expect(
shouldInterruptRunningThreadFromKeybinding({ command: "thread.interrupt", isRunning: true }),
).toBe(true);
});

it("leaves the shortcut alone when the thread is not running", () => {
expect(
shouldInterruptRunningThreadFromKeybinding({ command: "thread.interrupt", isRunning: false }),
).toBe(false);
});

it.each(["composer.stash", "thread.next", null] as const)(
"ignores the unrelated command %s",
(command) => {
expect(shouldInterruptRunningThreadFromKeybinding({ command, isRunning: true })).toBe(false);
},
);
});

describe("detectComposerTrigger", () => {
it("detects @path trigger at cursor", () => {
const text = "Please check @src/com";
Expand Down
10 changes: 10 additions & 0 deletions apps/web/src/composer-logic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,16 @@ export function shouldSubmitComposerOnEnter(input: {
return !input.isMobileViewport && !input.shiftKey;
}

// The interrupt shortcut is only ours while a turn is actually running. When
// the thread is idle the key stays free for whatever else claims it, so the
// default Escape binding does not swallow dismiss behavior.
export function shouldInterruptRunningThreadFromKeybinding(input: {
command: string | null;
isRunning: boolean;
}): boolean {
return input.command === "thread.interrupt" && input.isRunning;
}

const isInlineTokenSegment = (
segment:
| { type: "text"; text: string }
Expand Down
1 change: 1 addition & 0 deletions packages/contracts/src/keybindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export type ModelPickerJumpKeybindingCommand =
export const THREAD_KEYBINDING_COMMANDS = [
"thread.previous",
"thread.next",
"thread.interrupt",
...THREAD_JUMP_KEYBINDING_COMMANDS,
] as const;
export type ThreadKeybindingCommand = (typeof THREAD_KEYBINDING_COMMANDS)[number];
Expand Down
1 change: 1 addition & 0 deletions packages/shared/src/keybindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export const DEFAULT_KEYBINDINGS: ReadonlyArray<KeybindingRule> = [
{ key: "mod+o", command: "editor.openFavorite" },
{ key: "mod+shift+[", command: "thread.previous" },
{ key: "mod+shift+]", command: "thread.next" },
{ key: "escape", command: "thread.interrupt", when: "!terminalFocus && !modelPickerOpen" },
...THREAD_JUMP_KEYBINDING_COMMANDS.map((command, index) => ({
key: `mod+${index + 1}`,
command,
Expand Down
Loading