Repository navigation
fix(cli): improve slash command history feedback #8365
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
Changes from 1 commit
90a67a7
e7183bb
facb38f
296961e
d7a14bf
ce6fbb9
4b2b65f
8d19d46
81618a6
15227e1
d1140e6
90f0145
c1dfe04
b44d4d3
deed974
24b12bf
c2ab3fc
cf3cc8c
64fc673
751deb0
a3df7ee
0502eed
cd49a42
92d1dc3
b6b0cc9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| # Slash command history feedback | ||
|
|
||
| ## Problem | ||
|
|
||
| Interactive slash commands are added to the TUI history before their action is | ||
| known. Commands that only open a dialog can therefore leave a bare invocation | ||
| behind after the dialog closes. The model picker has the same problem when it | ||
| is dismissed without a selection. | ||
|
|
||
| ## Design | ||
|
|
||
| - Do not add the built-in `/auth`, `/settings`, `/status`, `/help`, `/theme`, | ||
| `/editor`, or `/diff` invocations to visible TUI history. Bare `/effort`, | ||
| `/statusline`, and `/stats` pickers are hidden too. Their existing UI remains | ||
| unchanged, as do chat recording and slash-command telemetry. User and project | ||
| commands that override those names keep their invocation history. | ||
| - Resolve the command before adding its invocation so aliases use the canonical | ||
| command name for this decision. | ||
| - Preserve invocations for commands that directly perform work, change session | ||
| state, write data, or enter a management/security workflow. Argument-sensitive | ||
| commands only hide their bare picker form; for example, `/effort` is hidden | ||
| while `/effort high` remains visible. | ||
| - When the primary model picker is dismissed without a selection, add an info | ||
| message identifying the unchanged model. Successful selections keep their | ||
| existing feedback. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,15 +135,34 @@ function createTestCommand( | |
| }; | ||
| } | ||
|
|
||
| function createTestCommandPath(path: string): SlashCommand { | ||
| const names = path.split(' '); | ||
| let command = createTestCommand({ | ||
| name: names.at(-1) ?? path, | ||
| action: vi.fn(), | ||
| }); | ||
|
|
||
| for (let index = names.length - 2; index >= 0; index--) { | ||
| command = createTestCommand({ | ||
| name: names[index], | ||
| subCommands: [command], | ||
| }); | ||
| } | ||
|
|
||
| return command; | ||
| } | ||
|
|
||
| describe('useSlashCommandProcessor', () => { | ||
| const mockAddItem = vi.fn(); | ||
| const mockUpdateItem = vi.fn(); | ||
| const mockClearItems = vi.fn(); | ||
| const mockLoadHistory = vi.fn(); | ||
| const mockOpenThemeDialog = vi.fn(); | ||
| const mockOpenAuthDialog = vi.fn(); | ||
| const mockOpenSettingsDialog = vi.fn(); | ||
| const mockOpenMemoryDialog = vi.fn(); | ||
| const mockOpenModelDialog = vi.fn(); | ||
| const mockOpenHelpDialog = vi.fn(); | ||
| const mockSetQuittingMessages = vi.fn(); | ||
|
|
||
| const mockConfig = makeFakeConfig({}); | ||
|
|
@@ -159,13 +178,13 @@ describe('useSlashCommandProcessor', () => { | |
| openThemeDialog: mockOpenThemeDialog, | ||
| openEditorDialog: vi.fn(), | ||
| openMemoryDialog: mockOpenMemoryDialog, | ||
| openSettingsDialog: vi.fn(), | ||
| openSettingsDialog: mockOpenSettingsDialog, | ||
| openStatusLineDialog: vi.fn(), | ||
| openModelDialog: mockOpenModelDialog, | ||
| openTrustDialog: vi.fn(), | ||
| openPermissionsDialog: vi.fn(), | ||
| openApprovalModeDialog: vi.fn(), | ||
| openHelpDialog: vi.fn(), | ||
| openHelpDialog: mockOpenHelpDialog, | ||
| openResumeDialog: vi.fn(), | ||
| handleResume: vi.fn(), | ||
| handleBranch: vi.fn().mockResolvedValue(undefined), | ||
|
|
@@ -718,6 +737,197 @@ describe('useSlashCommandProcessor', () => { | |
| }); | ||
|
|
||
| describe('Action Result Handling', () => { | ||
| it.each([ | ||
| ['/auth status', 'auth', ['connect', 'login'], 'auth'], | ||
| ['/connect', 'auth', ['connect', 'login'], 'auth'], | ||
|
DragonnZhang marked this conversation as resolved.
Resolved
|
||
| ['/settings', 'settings', undefined, 'settings'], | ||
| ] as const)( | ||
| 'handles %s without adding the invocation to TUI history', | ||
| async (input, name, altNames, dialog) => { | ||
| const command = createTestCommand({ | ||
| name, | ||
| altNames: altNames ? [...altNames] : undefined, | ||
| action: vi.fn().mockResolvedValue({ type: 'dialog', dialog }), | ||
| }); | ||
| const result = setupProcessorHook([command]); | ||
| await waitFor(() => | ||
| expect(result.current.slashCommands).toHaveLength(1), | ||
| ); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleSlashCommand(input); | ||
| }); | ||
|
|
||
| expect(mockAddItem).not.toHaveBeenCalled(); | ||
| expect( | ||
| name === 'auth' ? mockOpenAuthDialog : mockOpenSettingsDialog, | ||
| ).toHaveBeenCalledTimes(1); | ||
| expect( | ||
| mockConfig.getChatRecordingService()?.recordSlashCommand, | ||
| ).toHaveBeenCalledWith({ | ||
| phase: 'invocation', | ||
| rawCommand: input, | ||
| sentToModel: false, | ||
| }); | ||
| }, | ||
| ); | ||
|
|
||
| it.each(['/help', '/?'])( | ||
| 'opens %s without adding the invocation to TUI history', | ||
| async (input) => { | ||
| const command = createTestCommand({ | ||
| name: 'help', | ||
| altNames: ['?'], | ||
| action: vi.fn().mockResolvedValue({ | ||
| type: 'dialog', | ||
| dialog: 'help', | ||
| }), | ||
| }); | ||
| const result = setupProcessorHook([command]); | ||
| await waitFor(() => | ||
| expect(result.current.slashCommands).toHaveLength(1), | ||
| ); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleSlashCommand(input); | ||
| }); | ||
|
|
||
| expect(mockAddItem).not.toHaveBeenCalled(); | ||
| expect(mockOpenHelpDialog).toHaveBeenCalledTimes(1); | ||
| }, | ||
| ); | ||
|
|
||
| it.each(['/diff', '/editor', '/theme'])( | ||
| 'hides the invocation for the %s panel', | ||
| async (input) => { | ||
| const command = createTestCommandPath(input.slice(1)); | ||
| const result = setupProcessorHook([command]); | ||
| await waitFor(() => | ||
| expect(result.current.slashCommands).toHaveLength(1), | ||
| ); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleSlashCommand(input); | ||
| }); | ||
|
|
||
| expect(mockAddItem).not.toHaveBeenCalled(); | ||
| }, | ||
| ); | ||
|
|
||
| it.each(['/effort', '/stats', '/statusline'])( | ||
| 'hides the invocation for the bare %s picker', | ||
| async (input) => { | ||
| const [name] = input.slice(1).split(' '); | ||
| const command = createTestCommand({ name, action: vi.fn() }); | ||
| const result = setupProcessorHook([command]); | ||
| await waitFor(() => | ||
| expect(result.current.slashCommands).toHaveLength(1), | ||
| ); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleSlashCommand(input); | ||
| }); | ||
|
|
||
| expect(mockAddItem).not.toHaveBeenCalled(); | ||
| }, | ||
| ); | ||
|
|
||
| it('hides the invocation for the /usage alias', async () => { | ||
| const command = createTestCommand({ | ||
| name: 'stats', | ||
| altNames: ['usage'], | ||
| action: vi.fn(), | ||
| }); | ||
| const result = setupProcessorHook([command]); | ||
| await waitFor(() => expect(result.current.slashCommands).toHaveLength(1)); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleSlashCommand('/usage'); | ||
| }); | ||
|
|
||
| expect(mockAddItem).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it.each([ | ||
| ['/effort high', 'effort'], | ||
| ['/statusline make it compact', 'statusline'], | ||
| ['/stats export', 'stats export'], | ||
| ])( | ||
| 'keeps the invocation for the direct action %s', | ||
| async (input, canonicalPath) => { | ||
| const command = createTestCommandPath(canonicalPath); | ||
| const result = setupProcessorHook([command]); | ||
| await waitFor(() => | ||
| expect(result.current.slashCommands).toHaveLength(1), | ||
| ); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleSlashCommand(input); | ||
| }); | ||
|
|
||
| expect(mockAddItem).toHaveBeenCalledTimes(1); | ||
| expect(mockAddItem).toHaveBeenCalledWith( | ||
| { type: MessageType.USER, text: input, sentToModel: false }, | ||
| expect.any(Number), | ||
| ); | ||
| }, | ||
| ); | ||
|
|
||
| it('shows status output without adding the invocation to TUI history', async () => { | ||
| const command = createTestCommand({ | ||
| name: 'status', | ||
| altNames: ['about'], | ||
| action: vi.fn().mockResolvedValue({ | ||
| type: 'message', | ||
| messageType: 'info', | ||
| content: 'status output', | ||
| }), | ||
| }); | ||
| const result = setupProcessorHook([command]); | ||
| await waitFor(() => expect(result.current.slashCommands).toHaveLength(1)); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleSlashCommand('/about'); | ||
| }); | ||
|
|
||
| expect(mockAddItem).toHaveBeenCalledTimes(1); | ||
| expect(mockAddItem).toHaveBeenCalledWith( | ||
| { type: MessageType.INFO, text: 'status output' }, | ||
|
Comment on lines
+1056
to
+1058
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed that the processor seam would benefit from a direct phase-result assertion for hidden invocations. This is a test-coverage gap rather than a confirmed current behavior defect, and the PR is already at review round 5, so I am deferring it under the Critical-only guidance. Leaving the thread unresolved. |
||
| expect.any(Number), | ||
| ); | ||
| }); | ||
|
|
||
| it('keeps the invocation for a file command that overrides status', async () => { | ||
| const command = createTestCommand( | ||
| { | ||
| name: 'status', | ||
| action: vi.fn().mockResolvedValue({ | ||
| type: 'message', | ||
| messageType: 'info', | ||
| content: 'custom status output', | ||
| }), | ||
| }, | ||
| CommandKind.FILE, | ||
| ); | ||
| const result = setupProcessorHook([], [command]); | ||
| await waitFor(() => expect(result.current.slashCommands).toHaveLength(1)); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleSlashCommand('/status'); | ||
| }); | ||
|
|
||
| expect(mockAddItem).toHaveBeenNthCalledWith( | ||
| 1, | ||
| { type: MessageType.USER, text: '/status', sentToModel: false }, | ||
| expect.any(Number), | ||
| ); | ||
| expect(mockAddItem).toHaveBeenNthCalledWith( | ||
| 2, | ||
| { type: MessageType.INFO, text: 'custom status output' }, | ||
| expect.any(Number), | ||
| ); | ||
| }); | ||
|
|
||
| it('should handle "dialog: theme" action', async () => { | ||
| const command = createTestCommand({ | ||
| name: 'themecmd', | ||
|
|
||
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.
Agreed that the double-Escape latch test could pin the recorder call count as well as live history. The production latch behavior is unchanged and no current defect was reproduced here. Under the round-5 Critical-only guidance, I am deferring this test follow-up and leaving the thread unresolved.