Skip to content
Merged
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
4 changes: 2 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

207 changes: 167 additions & 40 deletions src/__tests__/web/hooks/useMobileKeyboardHandler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
useMobileKeyboardHandler,
type MobileKeyboardSession,
} from '../../../web/hooks/useMobileKeyboardHandler';
import { WEB_DEFAULT_SHORTCUTS } from '../../../web/constants/webShortcuts';
import type { AITabData } from '../../../web/hooks/useWebSocket';

function createTabs(): AITabData[] {
Expand Down Expand Up @@ -51,17 +52,15 @@ describe('useMobileKeyboardHandler', () => {
vi.restoreAllMocks();
});

it('toggles input mode with Cmd+J', () => {
const handleModeToggle = vi.fn();
const handleSelectTab = vi.fn();
it('dispatches toggleMode on the configured shortcut (Cmd+J)', () => {
const toggleMode = vi.fn();
const activeSession: MobileKeyboardSession = { inputMode: 'ai' };

renderHook(() =>
useMobileKeyboardHandler({
activeSessionId: 'session-1',
shortcuts: WEB_DEFAULT_SHORTCUTS,
activeSession,
handleModeToggle,
handleSelectTab,
actions: { toggleMode },
})
);

Expand All @@ -71,78 +70,106 @@ describe('useMobileKeyboardHandler', () => {
document.dispatchEvent(event);
});

expect(handleModeToggle).toHaveBeenCalledTimes(1);
expect(handleModeToggle).toHaveBeenCalledWith('terminal');
expect(toggleMode).toHaveBeenCalledTimes(1);
});

it('cycles to previous and next tabs with Cmd+[ and Cmd+]', () => {
const handleModeToggle = vi.fn();
const handleSelectTab = vi.fn();
const tabs = createTabs();
it('dispatches prevTab/nextTab on Cmd+Shift+[ and Cmd+Shift+]', () => {
const prevTab = vi.fn();
const nextTab = vi.fn();
const activeSession: MobileKeyboardSession = {
inputMode: 'ai',
aiTabs: tabs,
aiTabs: createTabs(),
activeTabId: 'tab-2',
};

renderHook(() =>
useMobileKeyboardHandler({
activeSessionId: 'session-1',
shortcuts: WEB_DEFAULT_SHORTCUTS,
activeSession,
handleModeToggle,
handleSelectTab,
actions: { prevTab, nextTab },
})
);

const prevEvent = new KeyboardEvent('keydown', { key: '[', metaKey: true, cancelable: true });
const nextEvent = new KeyboardEvent('keydown', { key: ']', metaKey: true, cancelable: true });
act(() => {
document.dispatchEvent(
new KeyboardEvent('keydown', {
key: '[',
metaKey: true,
shiftKey: true,
cancelable: true,
})
);
});
expect(prevTab).toHaveBeenCalledTimes(1);

act(() => {
document.dispatchEvent(prevEvent);
document.dispatchEvent(
new KeyboardEvent('keydown', {
key: ']',
metaKey: true,
shiftKey: true,
cancelable: true,
})
);
});
expect(nextTab).toHaveBeenCalledTimes(1);
});

expect(handleSelectTab).toHaveBeenCalledWith('tab-1');
it('dispatches cyclePrev/cycleNext on Cmd+[ and Cmd+]', () => {
const cyclePrev = vi.fn();
const cycleNext = vi.fn();

renderHook(() =>
useMobileKeyboardHandler({
shortcuts: WEB_DEFAULT_SHORTCUTS,
activeSession: { inputMode: 'ai' },
actions: { cyclePrev, cycleNext },
})
);

act(() => {
document.dispatchEvent(nextEvent);
document.dispatchEvent(
new KeyboardEvent('keydown', { key: '[', metaKey: true, cancelable: true })
);
});
expect(cyclePrev).toHaveBeenCalledTimes(1);

expect(handleSelectTab).toHaveBeenCalledWith('tab-3');
act(() => {
document.dispatchEvent(
new KeyboardEvent('keydown', { key: ']', metaKey: true, cancelable: true })
);
});
expect(cycleNext).toHaveBeenCalledTimes(1);
});

it('does not handle shortcuts when there is no active session', () => {
const handleModeToggle = vi.fn();
const handleSelectTab = vi.fn();
it('closes the command palette on Escape when open', () => {
const onCloseCommandPalette = vi.fn();

renderHook(() =>
useMobileKeyboardHandler({
activeSessionId: null,
shortcuts: WEB_DEFAULT_SHORTCUTS,
activeSession: null,
handleModeToggle,
handleSelectTab,
isCommandPaletteOpen: true,
onCloseCommandPalette,
actions: {},
})
);

const event = new KeyboardEvent('keydown', { key: 'j', metaKey: true, cancelable: true });

act(() => {
document.dispatchEvent(event);
document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', cancelable: true }));
});

expect(handleModeToggle).not.toHaveBeenCalled();
expect(onCloseCommandPalette).toHaveBeenCalledTimes(1);
});

it('does not steal shortcuts from xterm when terminal is focused', () => {
const handleModeToggle = vi.fn();
const handleSelectTab = vi.fn();
const activeSession: MobileKeyboardSession = { inputMode: 'terminal' };
const toggleMode = vi.fn();

renderHook(() =>
useMobileKeyboardHandler({
activeSessionId: 'session-1',
activeSession,
handleModeToggle,
handleSelectTab,
shortcuts: WEB_DEFAULT_SHORTCUTS,
activeSession: { inputMode: 'terminal' },
actions: { toggleMode },
})
);

Expand All @@ -161,7 +188,107 @@ describe('useMobileKeyboardHandler', () => {
xtermInput.dispatchEvent(event);
});

expect(handleModeToggle).not.toHaveBeenCalled();
expect(toggleMode).not.toHaveBeenCalled();
xtermInput.remove();
});

it('ignores events when no handler is registered for the matched shortcut', () => {
const quickAction = vi.fn();

renderHook(() =>
useMobileKeyboardHandler({
shortcuts: WEB_DEFAULT_SHORTCUTS,
activeSession: null,
actions: { quickAction },
})
);

// Cmd+J (toggleMode) should be a no-op since only quickAction is registered.
act(() => {
document.dispatchEvent(
new KeyboardEvent('keydown', { key: 'j', metaKey: true, cancelable: true })
);
});

expect(quickAction).not.toHaveBeenCalled();
});

it('skips plain typing inside an input field', () => {
const newInstance = vi.fn();
// Simulate a user-customized shortcut bound to a single bare key.
const shortcuts = {
...WEB_DEFAULT_SHORTCUTS,
newInstance: { id: 'newInstance', label: 'New Agent', keys: ['n'] },
};

const input = document.createElement('input');
document.body.appendChild(input);
input.focus();

renderHook(() =>
useMobileKeyboardHandler({
shortcuts,
activeSession: null,
actions: { newInstance },
})
);

const event = new KeyboardEvent('keydown', { key: 'n', cancelable: true, bubbles: true });
act(() => {
input.dispatchEvent(event);
});

expect(newInstance).not.toHaveBeenCalled();
input.remove();
});

it('still fires modifier shortcuts while an input field is focused', () => {
const quickAction = vi.fn();

const input = document.createElement('input');
document.body.appendChild(input);
input.focus();

renderHook(() =>
useMobileKeyboardHandler({
shortcuts: WEB_DEFAULT_SHORTCUTS,
activeSession: null,
actions: { quickAction },
})
);

const event = new KeyboardEvent('keydown', {
key: 'k',
metaKey: true,
cancelable: true,
bubbles: true,
});
act(() => {
input.dispatchEvent(event);
});

expect(quickAction).toHaveBeenCalledTimes(1);
input.remove();
});

it('does not match an empty or modifier-only shortcut definition', () => {
const newInstance = vi.fn();
const shortcuts = {
newInstance: { id: 'newInstance', label: 'New Agent', keys: [] },
};

renderHook(() =>
useMobileKeyboardHandler({
shortcuts,
activeSession: null,
actions: { newInstance },
})
);

act(() => {
document.dispatchEvent(new KeyboardEvent('keydown', { key: 'n', cancelable: true }));
});

expect(newInstance).not.toHaveBeenCalled();
});
});
10 changes: 5 additions & 5 deletions src/__tests__/web/mobile/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2112,7 +2112,7 @@ describe('MobileApp', () => {
});
});

it('navigates to previous tab with Cmd+[', async () => {
it('navigates to previous tab with Cmd+Shift+[', async () => {
render(<MobileApp />);

await act(async () => {
Expand All @@ -2129,7 +2129,7 @@ describe('MobileApp', () => {
]);
});

fireEvent.keyDown(document, { key: '[', metaKey: true });
fireEvent.keyDown(document, { key: '[', metaKey: true, shiftKey: true });

expect(mockSend).toHaveBeenCalledWith({
type: 'select_tab',
Expand All @@ -2138,7 +2138,7 @@ describe('MobileApp', () => {
});
});

it('navigates to next tab with Cmd+]', async () => {
it('navigates to next tab with Cmd+Shift+]', async () => {
render(<MobileApp />);

await act(async () => {
Expand All @@ -2155,7 +2155,7 @@ describe('MobileApp', () => {
]);
});

fireEvent.keyDown(document, { key: ']', metaKey: true });
fireEvent.keyDown(document, { key: ']', metaKey: true, shiftKey: true });

expect(mockSend).toHaveBeenCalledWith({
type: 'select_tab',
Expand All @@ -2181,7 +2181,7 @@ describe('MobileApp', () => {
]);
});

fireEvent.keyDown(document, { key: ']', metaKey: true });
fireEvent.keyDown(document, { key: ']', metaKey: true, shiftKey: true });

expect(mockSend).toHaveBeenCalledWith({
type: 'select_tab',
Expand Down
1 change: 1 addition & 0 deletions src/main/web-server/managers/CallbackRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ export class CallbackRegistry {
audioFeedbackEnabled: false,
colorBlindMode: 'false',
conductorProfile: '',
shortcuts: {},
};
}

Expand Down
3 changes: 3 additions & 0 deletions src/main/web-server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import type { WebSocket } from 'ws';
import type { Theme } from '../../shared/theme-types';
import type { Shortcut } from '../../shared/shortcut-types';

// Re-export Theme for convenience
export type { Theme } from '../../shared/theme-types';
Expand Down Expand Up @@ -377,6 +378,8 @@ export interface WebSettings {
audioFeedbackEnabled: boolean;
colorBlindMode: string;
conductorProfile: string;
/** User-customized keyboard shortcuts (partial overrides of DEFAULT_SHORTCUTS). */
shortcuts: Record<string, Shortcut>;
}

/**
Expand Down
6 changes: 6 additions & 0 deletions src/main/web-server/web-server-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { isWebContentsAvailable } from '../utils/safe-send';
import type { ProcessManager } from '../process-manager';
import type { StoredSession, SettingsStoreInterface as SettingsStore } from '../stores/types';
import type { Group } from '../../shared/types';
import type { Shortcut } from '../../shared/shortcut-types';
import { getDefaultShell } from '../stores/defaults';

/** UUID v4 format regex for validating stored security tokens.
Expand Down Expand Up @@ -960,6 +961,7 @@ export function createWebServerFactory(deps: WebServerFactoryDependencies) {
audioFeedbackEnabled: settingsStore.get('audioFeedbackEnabled', false) as boolean,
colorBlindMode: settingsStore.get('colorBlindMode', 'false') as string,
conductorProfile: settingsStore.get('conductorProfile', '') as string,
shortcuts: settingsStore.get('shortcuts', {}) as Record<string, Shortcut>,
};
});
Comment on lines 961 to 966
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Inline import() type instead of a top-level import

Using an inline import('../../shared/shortcut-types').Shortcut type reference inside an as cast is non-idiomatic TypeScript and harder to read/maintain. The same pattern appears again at line 873–881. Since Shortcut is re-exported from ../../shared/shortcut-types (and already pulled into types.ts), a single top-level import would be cleaner:

import type { Shortcut } from '../../shared/shortcut-types';

Then both as Record<string, Shortcut> casts can drop the inline import path entirely.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


Expand Down Expand Up @@ -996,6 +998,10 @@ export function createWebServerFactory(deps: WebServerFactoryDependencies) {
audioFeedbackEnabled: settingsStore.get('audioFeedbackEnabled', false) as boolean,
colorBlindMode: settingsStore.get('colorBlindMode', 'false') as string,
conductorProfile: settingsStore.get('conductorProfile', '') as string,
shortcuts: settingsStore.get('shortcuts', {}) as Record<
string,
import('../../shared/shortcut-types').Shortcut
>,
};
server.broadcastSettingsChanged(settings);
}
Expand Down
Loading