-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(cli): do not append trailing space for directory completions (#4092) #4288
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 all commits
cd443a8
8ed8860
13b3b35
ecd8047
f0be4c2
9b6e7d8
eb1a7e5
978d874
79c6ce0
ab145c6
7492c81
c1c50d0
62ca7cd
f8cc432
0ae90dd
1fb292c
eb30a8a
05bb656
862cefb
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 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |||||||||||
| * SPDX-License-Identifier: Apache-2.0 | ||||||||||||
| */ | ||||||||||||
|
|
||||||||||||
| import type { SlashCommand, CommandContext } from './types.js'; | ||||||||||||
| import type { SlashCommand, CommandContext, CommandCompletionItem } from './types.js'; | ||||||||||||
| import { CommandKind } from './types.js'; | ||||||||||||
| import { MessageType } from '../types.js'; | ||||||||||||
| import * as fs from 'node:fs'; | ||||||||||||
|
|
@@ -58,7 +58,7 @@ function findExistingWorkspaceDirectory( | |||||||||||
| * Returns directory path completions for the given partial argument. | ||||||||||||
| * Supports comma-separated paths by completing only the last segment. | ||||||||||||
| */ | ||||||||||||
| export function getDirPathCompletions(partialArg: string): string[] { | ||||||||||||
| export function getDirPathCompletions(partialArg: string): CommandCompletionItem[] { | ||||||||||||
| const lastComma = partialArg.lastIndexOf(','); | ||||||||||||
| const prefix = lastComma >= 0 ? partialArg.substring(0, lastComma + 1) : ''; | ||||||||||||
| const partial = | ||||||||||||
|
|
@@ -85,7 +85,10 @@ export function getDirPathCompletions(partialArg: string): string[] { | |||||||||||
| e.name.startsWith(namePrefix) && | ||||||||||||
| !e.name.startsWith('.'), | ||||||||||||
| ) | ||||||||||||
| .map((e) => prefix + path.join(searchDir, e.name)) | ||||||||||||
| .map((e) => ({ | ||||||||||||
| value: prefix + path.join(searchDir, e.name) + path.sep, | ||||||||||||
| isDirectory: true, | ||||||||||||
|
Collaborator
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. [Critical] After tab-completion, the buffer becomes e.g. The function already handles trailing
Suggested change
β qwen-latest-series-invite-beta-v28 via Qwen Code /review
Contributor
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. done, thx for your [Critical] |
||||||||||||
| })) | ||||||||||||
| .slice(0, 8); | ||||||||||||
| } catch { | ||||||||||||
|
Collaborator
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. [Critical] Build is broken β duplicate Line 94 is a duplicate of line 93 ( tsc reports 4 errors: This appears to be a merge/rebase artifact from the previous fix attempt.
Suggested change
Also delete lines 99-103 (the orphaned test code and extra β qwen-latest-series-invite-beta-v34 via Qwen Code /review |
||||||||||||
| return []; | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -143,6 +143,15 @@ describe('useAtCompletion', () => { | |
| 'dir/', | ||
| 'file.txt', | ||
| ]); | ||
| // Verify isDirectory flag | ||
|
Collaborator
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. [Suggestion] These new The host test fails at line 142 ( Impact: Suggested fix: Add a self-contained test that mocks the file search results directly (similar to how it('should set isDirectory based on trailing slash in file search results', async () => {
// Mock fileSearch to return ['dir/', 'file.txt']
// Assert isDirectory is true for 'dir/' and false for 'file.txt'
});β qwen-latest-series-invite-beta-v34 via Qwen Code /review |
||
| const dirSuggestion = result.current.suggestions.find( | ||
| (s) => s.value === 'dir/', | ||
| ); | ||
| const fileSuggestion = result.current.suggestions.find( | ||
| (s) => s.value === 'file.txt', | ||
| ); | ||
| expect(dirSuggestion?.isDirectory).toBe(true); | ||
| expect(fileSuggestion?.isDirectory).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -211,9 +211,13 @@ export function useAtCompletion(props: UseAtCompletionProps): void { | |
| return; | ||
| } | ||
|
|
||
| // isDirectory relies on crawler.ts in @qwen-code/qwen-code-core | ||
| // always normalizing paths with posix '/' via fdir.withPathSeparator('/'). | ||
| // If the crawler ever switches to path.sep, this check must be updated. | ||
| const suggestions = results.map((p) => ({ | ||
| label: p, | ||
| value: escapePath(p), | ||
| isDirectory: p.endsWith('/'), | ||
|
Collaborator
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. [Suggestion] The sole consumer ( Consider normalizing to an explicit boolean in one place β e.g., β qwen-latest-series-invite-beta-v34 via Qwen Code /review
Collaborator
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. [Suggestion] If someone changes the crawler to use Suggested fix: add a comment documenting this dependency, or extract a shared constant (e.g., β qwen-latest-series-invite-beta-v34 via Qwen Code /review |
||
| })); | ||
| dispatch({ type: 'SEARCH_SUCCESS', payload: suggestions }); | ||
| } catch (error) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -552,6 +552,37 @@ describe('useCommandCompletion', () => { | |
| expect(result.current.textBuffer.text).toBe('@src/file1.txt '); | ||
| }); | ||
|
|
||
| it('should not append trailing space for directory completions', async () => { | ||
|
Collaborator
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. [Suggestion] The new test only covers directory completion at end-of-line ( Suggested fix: add a test mirroring the existing "should complete a file path when cursor is not at the end of the line" test (line 586), but with a directory suggestion: it('should not append trailing space for directory completions when cursor is mid-line', async () => {
const text = '@src/com is a dir';
// ... setup with isDirectory: true suggestion ...
expect(result.current.textBuffer.text).toBe('@src/components/ is a dir');
});β qwen-latest-series-invite-beta-v34 via Qwen Code /review |
||
| setupMocks({ | ||
| atSuggestions: [ | ||
| { label: 'src/components/', value: 'src/components/', isDirectory: true }, | ||
| ], | ||
| }); | ||
|
|
||
| const { result } = renderHook(() => { | ||
| const textBuffer = useTextBufferForTest('@src/com'); | ||
| const completion = useCommandCompletion( | ||
| textBuffer, | ||
| testRootDir, | ||
| [], | ||
| mockCommandContext, | ||
| false, | ||
| mockConfig, | ||
| ); | ||
| return { ...completion, textBuffer }; | ||
| }); | ||
|
|
||
| await waitFor(() => { | ||
| expect(result.current.suggestions.length).toBe(1); | ||
| }); | ||
|
|
||
| act(() => { | ||
| result.current.handleAutocomplete(0); | ||
| }); | ||
|
|
||
| expect(result.current.textBuffer.text).toBe('@src/components/'); | ||
| }); | ||
|
|
||
| it('should complete a file path when cursor is not at the end of the line', async () => { | ||
| const text = '@src/fi is a good file'; | ||
| const cursorOffset = 7; // after "i" | ||
|
|
@@ -585,6 +616,46 @@ describe('useCommandCompletion', () => { | |
| '@src/file1.txt is a good file', | ||
| ); | ||
| }); | ||
|
|
||
| it('should preserve existing space after directory completions at mid-line cursor', async () => { | ||
| const text = '@src/com is a dir'; | ||
| const cursorOffset = 8; // after "m" | ||
|
|
||
| setupMocks({ | ||
| atSuggestions: [ | ||
| { | ||
| label: 'src/components/', | ||
| value: 'src/components/', | ||
| isDirectory: true, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| const { result } = renderHook(() => { | ||
| const textBuffer = useTextBufferForTest(text, cursorOffset); | ||
| const completion = useCommandCompletion( | ||
| textBuffer, | ||
| testRootDir, | ||
| [], | ||
| mockCommandContext, | ||
| false, | ||
| mockConfig, | ||
| ); | ||
| return { ...completion, textBuffer }; | ||
| }); | ||
|
|
||
| await waitFor(() => { | ||
| expect(result.current.suggestions.length).toBe(1); | ||
| }); | ||
|
|
||
| act(() => { | ||
| result.current.handleAutocomplete(0); | ||
| }); | ||
|
|
||
| expect(result.current.textBuffer.text).toBe( | ||
| '@src/components/ is a dir', | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('argument hint ghost text', () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -228,7 +228,8 @@ export function useCommandCompletion( | |
|
|
||
| const lineCodePoints = toCodePoints(buffer.lines[cursorRow] || ''); | ||
| const charAfterCompletion = lineCodePoints[end]; | ||
| if (charAfterCompletion !== ' ') { | ||
| const isDirectory = suggestions[indexToUse].isDirectory; | ||
|
Collaborator
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. [Suggestion] The condition Consider only suppressing the space when the cursor is at end-of-line: if (charAfterCompletion !== ' ' && !(isDirectory && !charAfterCompletion)) {β qwen-latest-series-invite-beta-v34 via Qwen Code /review |
||
| if (charAfterCompletion !== ' ' && !(isDirectory && !charAfterCompletion)) { | ||
| suggestionText += ' '; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1122,4 +1122,39 @@ describe('useSlashCompletion', () => { | |
| expect(mockSetIsLoadingSuggestions).not.toHaveBeenCalled(); | ||
| expect(mockSetIsPerfectMatch).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| describe('isDirectory propagation', () => { | ||
| it('should propagate isDirectory from CommandCompletionItem to Suggestion', async () => { | ||
| const mockCompletionFn = vi.fn().mockResolvedValue([ | ||
| { value: '/tmp/workspace/', isDirectory: true }, | ||
| { value: '/tmp/file.txt' }, | ||
| ]); | ||
|
|
||
| const slashCommands = [ | ||
| createTestCommand({ | ||
| name: 'dir', | ||
| description: 'test', | ||
| completion: mockCompletionFn, | ||
| }), | ||
| ]; | ||
|
|
||
| const { result } = renderHook(() => | ||
| useTestHarnessForSlashCompletion( | ||
| true, | ||
| '/dir ', | ||
| slashCommands, | ||
| mockCommandContext, | ||
| ), | ||
|
Collaborator
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. [Critical] The change expect(result.current.suggestions).toEqual([
{ label: 'pdf', value: 'pdf', description: 'Create PDF documents' },
{ label: 'xlsx', value: 'xlsx', description: 'Work with spreadsheets' },
]);vitest Fix: either update the test at line 828 to include ...(item.isDirectory ? { isDirectory: true } : {}),β qwen-latest-series-invite-beta-v34 via Qwen Code /review |
||
| ); | ||
|
|
||
| await waitFor(() => { | ||
| expect(result.current.suggestions.length).toBe(2); | ||
| }); | ||
|
|
||
| // First suggestion (directory) should have isDirectory: true | ||
| expect(result.current.suggestions[0].isDirectory).toBe(true); | ||
| // Second suggestion (file) should NOT have isDirectory flag | ||
| expect(result.current.suggestions[1].isDirectory).toBeFalsy(); | ||
| }); | ||
| }); | ||
| }); | ||
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.
[Suggestion] The
!e.name.startsWith('.')filter (hiding hidden directories) has no test. If someone removes or inverts this filter, no test fails and directories like.gitwould start appearing in tab completions.Suggested fix: In
beforeEach, create a.hiddendirectory intempTestDir, then in existing tests verify it's excluded:β qwen-latest-series-invite-beta-v34 via Qwen Code /review