From 4dc98484fd3f033d9d3029ab3749315e50ca9fd1 Mon Sep 17 00:00:00 2001 From: dykebo <92703265+dykebo@users.noreply.github.com> Date: Sat, 23 May 2026 23:37:23 +0800 Subject: [PATCH] feat(cli): do not append trailing space for directory completions (#4092) (#4288) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(cli): do not append trailing space for directory completions (#4092) ## What 在 @路径补全和 /dir add 命令的目录补全中不再追加尾部空格。这样可以允许用户在补全目录后直接按 Tab 继续深入下一级子目录，无需先删除空格。 ## Examples - Input: `@src/com` + Tab → Output: `@src/components/` (no trailing space) - Input: `/dir add ./pac` + Tab → Output: `/dir add ./packages/` (no trailing space) - File completions still append a space (e.g., `@src/file.txt `) ## Changes - Added `isDirectory` flag to `Suggestion` and `CommandCompletionItem` interfaces - Updated `handleAutocomplete` to skip trailing space when `isDirectory === true` - Modified `getDirPathCompletions` to return `CommandCompletionItem[]` with `isDirectory: true` - Added test case for directory completion behavior * fix(cli): append trailing / to directory completions for deeper navigation * fix(cli): propagate isDirectory and fix JSDoc comment ## Comment 2: Fix JSDoc in SuggestionsDisplay Removed "(ends with /)" from isDirectory description since it was factually incorrect. ## Comment 3: Add test for isDirectory propagation - Added test suite in useSlashCompletion.test.ts to verify directory command structure - Real filesystem testing is done in directoryCommand.test.tsx * fix(cli): add comprehensive isDirectory propagation tests Added getDirPathCompletions unit tests that verify: - Directory suggestions include isDirectory: true - Directory values end with / for continued navigation - Prefix filtering preserves isDirectory flag - Comma-separated path completion works correctly - Deeply nested directories maintain isDirectory flag This closes the testing gap identified in review comment 3. * fix(cli): address wenshao feedback - lint rules, real test, cross-platform Fixes 4 new review comments from wenshao: - [Critical] Empty catch {} blocks: guarded with if (tempTestDir) + void err - [Critical] useSlashCompletion.no-op test: replaced with real integration test that verifies isDirectory propagation through toSuggestion pass-through - [Suggestion] Windows path separator: using path.sep instead of hardcoded / in both directoryCommand.tsx and related test assertions * fix(cli): remove unused import and fix Windows path separator in tests - Remove unused directoryCommand import in useSlashCompletion.test.ts (TS6133) - Replace hardcoded / regex with path.sep-aware assertions in directoryCommand.test.tsx to fix Windows CI failures Co-authored-by: Qwen-Coder * Apply suggestion from @wenshao Co-authored-by: Shaojin Wen * Update packages/cli/src/ui/commands/directoryCommand.test.tsx Co-authored-by: Shaojin Wen * Update packages/cli/src/ui/commands/directoryCommand.tsx Co-authored-by: Shaojin Wen * Update packages/cli/src/ui/commands/directoryCommand.tsx Co-authored-by: Shaojin Wen * fix(cli): normalize isDirectory to explicit boolean in toSuggestion Normalize isDirectory from three-state (true/false/undefined) to explicit boolean (true/false) to prevent latent bugs in future code that might distinguish between false and undefined. Fixes review comment: isDirectory normalization is inconsistent across completion paths. Co-authored-by: Qwen-Coder * Update packages/cli/src/ui/hooks/useSlashCompletion.ts Co-authored-by: Shaojin Wen * chore: remove accidentally committed pr_body.md Co-authored-by: Qwen-Coder * chore: add pr_body.md to .gitignore Co-authored-by: Qwen-Coder * fix(cli): remove duplicate .slice and orphaned test code from directoryCommand.tsx Co-authored-by: Qwen-Coder * fix(cli): only suppress trailing space for dir completions at end-of-line When isDirectory is true, the trailing space was suppressed unconditionally, even when the cursor is mid-line. This caused directory completions to merge directly with following text (e.g. '@src/components/something'). Now only suppress the space when the cursor is at end-of-line, allowing continued Tab navigation into subdirectories. Co-authored-by: Qwen-Coder * docs(cli): document crawler path separator dependency for isDirectory check The isDirectory detection uses p.endsWith('/') which depends on the crawler in @qwen-code/qwen-code-core normalizing paths with posix '/' (fdir.withPathSeparator('/') in crawler.ts). Add a comment to make this implicit coupling explicit. Co-authored-by: Qwen-Coder * test(cli): add mid-line directory completion test Verify that directory completions append a trailing space when the cursor is mid-line, preventing the completed path from merging with following text. Co-authored-by: Qwen-Coder * Update packages/cli/src/ui/hooks/useCommandCompletion.test.ts Co-authored-by: Shaojin Wen --------- Co-authored-by: 方磊 Co-authored-by: Qwen-Coder Co-authored-by: Shaojin Wen --- .gitignore | 3 + .../src/ui/commands/directoryCommand.test.tsx | 120 +++++++++++++++++- .../cli/src/ui/commands/directoryCommand.tsx | 9 +- packages/cli/src/ui/commands/types.ts | 2 + .../src/ui/components/SuggestionsDisplay.tsx | 2 + .../cli/src/ui/hooks/useAtCompletion.test.ts | 9 ++ packages/cli/src/ui/hooks/useAtCompletion.ts | 4 + .../src/ui/hooks/useCommandCompletion.test.ts | 71 +++++++++++ .../cli/src/ui/hooks/useCommandCompletion.tsx | 3 +- .../src/ui/hooks/useSlashCompletion.test.ts | 35 +++++ .../cli/src/ui/hooks/useSlashCompletion.ts | 1 + 11 files changed, 254 insertions(+), 5 deletions(-) diff --git a/.gitignore b/.gitignore index 6ff1d950be..9734c67057 100644 --- a/.gitignore +++ b/.gitignore @@ -56,6 +56,9 @@ bundle junit.xml packages/*/coverage/ +# PR body draft +pr_body.md + # Generated files packages/cli/src/generated/ packages/core/src/generated/ diff --git a/packages/cli/src/ui/commands/directoryCommand.test.tsx b/packages/cli/src/ui/commands/directoryCommand.test.tsx index 5ad0bb1b13..23421ad2b1 100644 --- a/packages/cli/src/ui/commands/directoryCommand.test.tsx +++ b/packages/cli/src/ui/commands/directoryCommand.test.tsx @@ -5,13 +5,18 @@ */ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { directoryCommand, expandHomeDir } from './directoryCommand.js'; +import { + directoryCommand, + expandHomeDir, + getDirPathCompletions, +} from './directoryCommand.js'; import type { Config, WorkspaceContext } from '@qwen-code/qwen-code-core'; import type { CommandContext } from './types.js'; import { MessageType } from '../types.js'; import { SettingScope } from '../../config/settings.js'; import * as os from 'node:os'; import * as path from 'node:path'; +import * as fs from 'node:fs'; describe('directoryCommand', () => { let mockContext: CommandContext; @@ -323,3 +328,116 @@ describe('directoryCommand', () => { ); }); }); + +describe('getDirPathCompletions', () => { + let tempTestDir = ''; + + beforeEach(() => { + // Clean up any previous test runs + if (tempTestDir) { + try { + fs.rmSync(tempTestDir, { recursive: true, force: true }); + } catch (err) { + // ignore cleanup errors + void err; + } + } + + tempTestDir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-dir-test-')); + // Create a nested directory structure: root/sub1, root/sub2, root/sub1/deep + fs.mkdirSync(tempTestDir, { recursive: true }); + fs.mkdirSync(path.join(tempTestDir, 'sub1'), { recursive: true }); + fs.mkdirSync(path.join(tempTestDir, 'sub2'), { recursive: true }); + fs.mkdirSync(path.join(tempTestDir, 'sub1', 'deep'), { recursive: true }); + // Add some non-directory files (should be filtered out) + fs.writeFileSync(path.join(tempTestDir, 'file.txt'), ''); + fs.writeFileSync( + path.join(tempTestDir, 'sub1', 'nested.txt'), + '', + ); + }); + + afterAll(() => { + // Cleanup after all tests + if (tempTestDir) { + try { + fs.rmSync(tempTestDir, { recursive: true, force: true }); + } catch (err) { + // ignore cleanup errors + void err; + } + } + }); + + describe('directory completions should include isDirectory flag', () => { + it('should return suggestions with isDirectory: true and trailing /', () => { + // Use "/" suffix so getDirPathCompletions searches INSIDE the directory + const results = getDirPathCompletions(`${tempTestDir}/`); + + expect(results.length).toBeGreaterThan(0); + + // Each suggestion should be a CommandCompletionItem with isDirectory: true + results.forEach((suggestion) => { + expect(suggestion.value).toBeDefined(); + expect(suggestion.isDirectory).toBe(true); + + // Directory values should end with path separator for continued navigation + expect(suggestion.value.endsWith(path.sep)).toBe(true); + + // Should match one of our created directories + const dirNameWithoutSlash = suggestion.value.slice(0, -1); + const basename = path.basename(dirNameWithoutSlash); + expect(['sub1', 'sub2'].includes(basename)).toBe(true); + }); + }); + + it('should filter by prefix while preserving isDirectory flag', () => { + const results = getDirPathCompletions(`${tempTestDir}/su`); + + expect(results.length).toBeGreaterThan(0); + + // Only directories starting with "su" should be returned + results.forEach((suggestion) => { + expect(suggestion.isDirectory).toBe(true); + const sepRe = path.sep === '\\' ? '\\\\' : path.sep; + expect(suggestion.value).toMatch(new RegExp(`${sepRe}su.+$`)); + // Only top-level directories matching the prefix are returned + const basename = path.basename(suggestion.value.slice(0, -1)); + expect(basename).toMatch(/^su/); + const dirname = path.dirname(suggestion.value); + expect(dirname).toContain(tempTestDir); + }); + }); + + it('should support comma-separated paths with isDirectory flag on last segment', () => { + const multiPath = `${tempTestDir}, ${tempTestDir}/`; + const results = getDirPathCompletions(multiPath); + + expect(results.length).toBeGreaterThan(0); + + // Results should start with the prefix from first part + results.forEach((suggestion) => { + expect(suggestion.isDirectory).toBe(true); + expect(suggestion.value.startsWith(`${tempTestDir}`)).toBe(true); + expect(suggestion.value.endsWith(path.sep)).toBe(true); + }); + }); + + it('should handle deeply nested directories with isDirectory flag', () => { + // Navigate into sub1 + const deepResults = getDirPathCompletions(`${tempTestDir}/sub1/`); + + expect(deepResults.length).toBeGreaterThan(0); + + // Only directories inside sub1 should be returned + deepResults.forEach((suggestion) => { + expect(suggestion.isDirectory).toBe(true); + expect(suggestion.value).toContain('sub1'); + expect(suggestion.value.endsWith(path.sep)).toBe(true); + // The nested 'deep' directory should be in the results + const basename = path.basename(suggestion.value.slice(0, -1)); + expect(basename).toBe('deep'); + }); + }); + }); +}); diff --git a/packages/cli/src/ui/commands/directoryCommand.tsx b/packages/cli/src/ui/commands/directoryCommand.tsx index d12a183ff2..1919e8c113 100644 --- a/packages/cli/src/ui/commands/directoryCommand.tsx +++ b/packages/cli/src/ui/commands/directoryCommand.tsx @@ -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, + })) .slice(0, 8); } catch { return []; diff --git a/packages/cli/src/ui/commands/types.ts b/packages/cli/src/ui/commands/types.ts index b127326275..24aee4a1df 100644 --- a/packages/cli/src/ui/commands/types.ts +++ b/packages/cli/src/ui/commands/types.ts @@ -292,6 +292,8 @@ export interface CommandCompletionItem { value: string; label?: string; description?: string; + /** Whether the completion represents a directory path. When true, handleAutocomplete should NOT append a trailing space so the user can continue tab-completing deeper into the directory tree. */ + isDirectory?: boolean; } // The standardized contract for any command in the system. diff --git a/packages/cli/src/ui/components/SuggestionsDisplay.tsx b/packages/cli/src/ui/components/SuggestionsDisplay.tsx index d2514ba13c..8eaee4df89 100644 --- a/packages/cli/src/ui/components/SuggestionsDisplay.tsx +++ b/packages/cli/src/ui/components/SuggestionsDisplay.tsx @@ -28,6 +28,8 @@ export interface Suggestion { matchedAlias?: string; supportedModes?: ExecutionMode[]; modelInvocable?: boolean; + /** Whether the suggestion represents a directory path. When true, handleAutocomplete should NOT append a trailing space so the user can continue tab-completing deeper into the directory tree. */ + isDirectory?: boolean; } interface SuggestionsDisplayProps { suggestions: Suggestion[]; diff --git a/packages/cli/src/ui/hooks/useAtCompletion.test.ts b/packages/cli/src/ui/hooks/useAtCompletion.test.ts index e2162924bb..fc88425c2d 100644 --- a/packages/cli/src/ui/hooks/useAtCompletion.test.ts +++ b/packages/cli/src/ui/hooks/useAtCompletion.test.ts @@ -143,6 +143,15 @@ describe('useAtCompletion', () => { 'dir/', 'file.txt', ]); + // Verify isDirectory flag + 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); }); }); diff --git a/packages/cli/src/ui/hooks/useAtCompletion.ts b/packages/cli/src/ui/hooks/useAtCompletion.ts index 8f3c870ba6..d1793139b7 100644 --- a/packages/cli/src/ui/hooks/useAtCompletion.ts +++ b/packages/cli/src/ui/hooks/useAtCompletion.ts @@ -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('/'), })); dispatch({ type: 'SEARCH_SUCCESS', payload: suggestions }); } catch (error) { diff --git a/packages/cli/src/ui/hooks/useCommandCompletion.test.ts b/packages/cli/src/ui/hooks/useCommandCompletion.test.ts index a918348ea5..15fc438b3d 100644 --- a/packages/cli/src/ui/hooks/useCommandCompletion.test.ts +++ b/packages/cli/src/ui/hooks/useCommandCompletion.test.ts @@ -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 () => { + 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', () => { diff --git a/packages/cli/src/ui/hooks/useCommandCompletion.tsx b/packages/cli/src/ui/hooks/useCommandCompletion.tsx index eb19978541..d4a45dfa87 100644 --- a/packages/cli/src/ui/hooks/useCommandCompletion.tsx +++ b/packages/cli/src/ui/hooks/useCommandCompletion.tsx @@ -228,7 +228,8 @@ export function useCommandCompletion( const lineCodePoints = toCodePoints(buffer.lines[cursorRow] || ''); const charAfterCompletion = lineCodePoints[end]; - if (charAfterCompletion !== ' ') { + const isDirectory = suggestions[indexToUse].isDirectory; + if (charAfterCompletion !== ' ' && !(isDirectory && !charAfterCompletion)) { suggestionText += ' '; } diff --git a/packages/cli/src/ui/hooks/useSlashCompletion.test.ts b/packages/cli/src/ui/hooks/useSlashCompletion.test.ts index bb8e666582..733efbc5ae 100644 --- a/packages/cli/src/ui/hooks/useSlashCompletion.test.ts +++ b/packages/cli/src/ui/hooks/useSlashCompletion.test.ts @@ -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, + ), + ); + + 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(); + }); + }); }); diff --git a/packages/cli/src/ui/hooks/useSlashCompletion.ts b/packages/cli/src/ui/hooks/useSlashCompletion.ts index 034291ae56..9f6b0ead4f 100644 --- a/packages/cli/src/ui/hooks/useSlashCompletion.ts +++ b/packages/cli/src/ui/hooks/useSlashCompletion.ts @@ -521,6 +521,7 @@ function toSuggestion(item: string | CommandCompletionItem): Suggestion | null { label: item.label ?? item.value, value: item.value, description: item.description, + ...(item.isDirectory !== undefined && { isDirectory: item.isDirectory }), }; }