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 3 commits
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 |
|---|---|---|
|
|
@@ -1254,8 +1254,8 @@ describe('<ModelDialog />', () => { | |
| expect(typeof childOnHighlight).toBe('function'); | ||
| }); | ||
|
|
||
| it('calls onClose prop when "escape" key is pressed', () => { | ||
| const { props } = renderComponent(); | ||
| it('reports the unchanged model when "escape" closes the primary picker', () => { | ||
| const { props, mockHistoryManager } = renderComponent(); | ||
|
|
||
| expect(mockedUseKeypress).toHaveBeenCalled(); | ||
|
|
||
|
|
@@ -1272,6 +1272,13 @@ describe('<ModelDialog />', () => { | |
| paste: false, | ||
| sequence: '', | ||
| }); | ||
| expect(mockHistoryManager.addItem).toHaveBeenCalledWith( | ||
| { | ||
| type: 'info', | ||
| text: `Kept model as ${DEFAULT_QWEN_MODEL}`, | ||
| }, | ||
| expect.any(Number), | ||
| ); | ||
| expect(props.onClose).toHaveBeenCalledTimes(1); | ||
|
|
||
| keyPressHandler({ | ||
|
|
@@ -1282,9 +1289,58 @@ describe('<ModelDialog />', () => { | |
| paste: false, | ||
| sequence: '', | ||
| }); | ||
| expect(mockHistoryManager.addItem).toHaveBeenCalledTimes(1); | ||
| expect(props.onClose).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('does not close the primary picker on "left"', () => { | ||
| const { props, mockHistoryManager } = renderComponent(); | ||
|
|
||
| const keyPressHandler = mockedUseKeypress.mock.calls[0][0]; | ||
| keyPressHandler({ | ||
| name: 'left', | ||
| ctrl: false, | ||
| meta: false, | ||
| shift: false, | ||
| paste: false, | ||
| sequence: '', | ||
| }); | ||
|
|
||
| expect(mockHistoryManager.addItem).not.toHaveBeenCalled(); | ||
| expect(props.onClose).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it.each([ | ||
| [{ isFastModelMode: true }, 'escape'], | ||
| [{ isVoiceModelMode: true }, 'escape'], | ||
| [{ isVisionModelMode: true }, 'escape'], | ||
| [{ isCompactionModelMode: true }, 'escape'], | ||
| [{ isImageModelMode: true }, 'escape'], | ||
| [{ isFastModelMode: true }, 'left'], | ||
| [{ isVoiceModelMode: true }, 'left'], | ||
| [{ isVisionModelMode: true }, 'left'], | ||
| [{ isCompactionModelMode: true }, 'left'], | ||
| [{ isImageModelMode: true }, 'left'], | ||
| ])( | ||
| 'does not report the primary model when closing an auxiliary picker (%j, %s)', | ||
| (modeProps, keyName) => { | ||
| const { props, mockHistoryManager } = renderComponent(modeProps); | ||
|
|
||
| const keyPressHandler = mockedUseKeypress.mock.calls[0][0]; | ||
| keyPressHandler({ | ||
| name: keyName, | ||
| ctrl: false, | ||
| meta: false, | ||
| shift: false, | ||
| paste: false, | ||
| sequence: '', | ||
| }); | ||
|
|
||
| expect(mockHistoryManager.addItem).not.toHaveBeenCalled(); | ||
| expect(props.onClose).toHaveBeenCalledTimes(1); | ||
|
Comment on lines
+1486
to
+1487
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 auxiliary-dismissal tests could also assert zero recorder calls. The production guard remains unchanged and the current Critical fix covers successful auxiliary selections only. Because this is review round 5 and the repository guidance is Critical-only at this stage, I am deferring the extra negative assertion and leaving the thread unresolved. |
||
| }, | ||
| ); | ||
|
|
||
| it('updates initialIndex when config context changes', () => { | ||
| const mockGetModel = vi.fn(() => DEFAULT_QWEN_MODEL); | ||
| const mockGetAuthType = vi.fn(() => 'qwen-oauth'); | ||
|
|
||
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.