feat(vscode): add file picker to @ mention dropdown#12028
feat(vscode): add file picker to @ mention dropdown#12028sylwester-liljegren wants to merge 4 commits into
Conversation
…cker-mention # Conflicts: # packages/kilo-vscode/webview-ui/src/hooks/useFileMention.ts
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| suppress = true | ||
| try { | ||
| textarea.setSelectionRange(state.atStart, state.atEnd) | ||
| document.execCommand("insertText", false, `@${norm}${suffix}`) |
There was a problem hiding this comment.
WARNING: Missing focus() before execCommand("insertText", ...)
insertFilePickerResult calls textarea.setSelectionRange(...) and then document.execCommand("insertText", ...), but never calls textarea.focus() first. Compare with the synchronous selectMention path (line 188), which calls textarea.focus() before inserting.
This path runs asynchronously, after the native OS file-picker dialog (opened via showOpenDialog in KiloProvider.ts) closes and the filePickerResult message arrives. execCommand("insertText") operates on the currently focused editable element, not merely on whatever element had setSelectionRange called on it. If VS Code doesn't restore focus to this exact <textarea> after the native dialog closes, the insert can silently no-op (or land somewhere else), while knownPaths/mentionedPaths/syncMentionedPaths/state.setText all proceed as if the insertion succeeded, since they read textarea.value after the execCommand call regardless of whether it actually changed anything.
Consider calling textarea.focus() before setSelectionRange/execCommand, similar to the existing selectMention flow.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const match = before.match(AT_PATTERN)! | ||
| const prefix = /^\s/.test(match[0]) ? 1 : 0 | ||
| const atPos = match.index! + prefix | ||
| pickerState = { textarea, atStart: atPos, atEnd: cursor, setText: _setText, onSelect } |
There was a problem hiding this comment.
SUGGESTION: No request correlation for the file-picker round trip
pickerState is a single shared closure variable that stores the pending picker request's textarea/selection, with no request id. This is inconsistent with requestFileSearch in this same file, which correlates responses via an incrementing fileSearchCounter embedded in requestId (see line 111, 127-132), and RequestFilePickerMessage/FilePickerResultMessage (in webview-messages.ts/extension-messages.ts) don't carry a requestId either, unlike the sibling RequestFileSearchMessage/FileSearchResultMessage.
Today this is likely benign since showOpenDialog is a modal dialog and Agent Manager only mounts one PromptInput/useFileMention instance at a time, so a second requestFilePicker can't realistically be triggered while one is in flight. But it's a latent footgun if that assumption ever changes (e.g. a future split-view or re-entrant trigger) — a stale/duplicate filePickerResult would silently apply to whatever pickerState happens to be set at the time. Worth considering a requestId for consistency with the established pattern, even if not strictly required today.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| const seedFromText = (text: string) => { | ||
| const re = /@([\w./-]+\.[\w]+|[\w.-]+\/[\w./-]+)/g | ||
| const re = /@([\w.:/-]+\.[\w]+|[\w.:-]+\/[\w.:/-]+)/g |
There was a problem hiding this comment.
WARNING: Regex change lets URL-like text after @ be treated as a known path
Adding : to both alternatives of this regex (to support Windows drive letters like @C:/Users/file.ts) also makes it match URL-like text such as @https://example.com, which the old regex (/@([\w./-]+\.[\w]+|[\w.-]+\/[\w./-]+)/g, no colon in the character classes) correctly rejected — the colon after https broke the match entirely before.
With the new regex, seedFromText would call knownPaths.add("https://example.com") for such text, causing it to be treated as a "known mentioned path" for highlighting. Downstream, buildFileAttachments would then try to build a file attachment for it: since "https://example.com" isn't detected as absolute by isAbsolutePath, it gets prefixed with the workspace dir (${dir}/https://example.com), producing a nonsensical file path/attachment.
Consider scoping the added : support to only the drive-letter case (e.g. requiring it immediately follow a single letter at the start) rather than allowing it anywhere in the path segment.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| </> | ||
| )} | ||
| </div> | ||
| <Show when={item.type === "file-picker" && mention.mentionResults().length > 1}> |
There was a problem hiding this comment.
WARNING: Separator can render as a trailing divider with nothing below it
This condition checks the total mentionResults().length, not whether any items actually follow the file-picker row. buildMentionResults always orders results as [...terminal, ...gitChanges, FILE_PICKER_RESULT, ...fileResults]. If terminal and/or git-changes match but there are zero file results for the current query (e.g. typing @t matches "terminal" but no files), mentionResults() is [terminal, FILE_PICKER_RESULT] — length 2, so this renders true, but FILE_PICKER_RESULT is the last item, producing a divider below it with nothing underneath.
Consider checking whether this item is followed by another item instead, e.g. index() < mention.mentionResults().length - 1 combined with the file-picker type check.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| it("always includes file picker result", () => { | ||
| const result = buildMentionResults("", []) | ||
| expect(result[result.length - 1]).toEqual(FILE_PICKER_RESULT) |
There was a problem hiding this comment.
SUGGESTION: Test doesn't actually validate picker placement
This test calls buildMentionResults("", []) (empty items) and asserts result[result.length - 1] equals FILE_PICKER_RESULT. Since items is empty, FILE_PICKER_RESULT is trivially both the first and last element of the result — this would pass identically whether the implementation splices it before or after ...results, so it doesn't verify the "picker appears before regular file results" placement the test name implies. The sibling tests above (e.g. lines 75-83) already correctly verify ordering using non-empty items; this one could be dropped as redundant, or rewritten to assert an index/position using a non-empty items array.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Resolved since last review
Files Reviewed (1 file in incremental diff)
Previous Review Summaries (2 snapshots, latest commit 4b7fcee)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 4b7fcee)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Resolved since last review
Files Reviewed (9 files in incremental diff)
Fix these issues in Kilo Cloud Previous review (commit 1dfed48)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (10 files)
Reviewed by claude-sonnet-5 · Input: 28 · Output: 7.2K · Cached: 624.7K Review guidance: REVIEW.md from base branch |
|
@sylwester-liljegren One potential blocker: What if the user declined access to a file. This solution essentially bypasses that restriction. One could argue that's ok, the user selected it. But what if the file would need to be edited? Then subsequent reads would fail. |
@marius-kilocode Good point, I should have thought of it earlier. Just a question before making any changes: If the user mentions a file outside the workspace for the first time and then it references it for additional reads/writes all while the user previously blocked access to either that file or the directory that the file is within, shouldn't AI inform the user about its lack of access to the file for further reference and either a) prompt the user to allow AI to access that specific file at that path or b) just tell user to adjust the deny rules instead. Or the simpler way is to just insert a styled path into chat textarea and then let AI go through permissions system to check if it access the file or not and depending on how things are set up, then deny if there is an explicit deny rule and ask if no explicit allow rule exists (otherwise, just allow). What do you think is most sound? EDIT: I'm leaning toward the second option as it is more cleaner and more architecturally consistent. I'll implement this unless you have any other opinion on this matter. |
2 participants
Issue
Exception: this is a small, self-contained feature request that came directly from manual testing feedback; no tracking issue was filed.
Context
Users frequently need to reference files that live outside the current workspace (e.g. configuration files, shared libraries in a parent directory, or files on a different drive on Windows). The existing @ mention system only lists files within the workspace, making it impossible to attach out-of-workspace files without manually typing the full path.
Implementation
Added a Browse files... option to the @ mention dropdown that opens VS Code's native file picker dialog. When a file is selected, its full absolute path is inserted as a styled
@/full/pathmention with the same atomic selection/backspace behavior as workspace file mentions.Key pieces:
requestFilePickerand extension→webview messagefilePickerResultKiloProvider.tshandles the request viavscode.window.showOpenDialoguseFileMention.tsstores the textarea/selection state before opening the dialog and restores it viaexecCommandafter the result arrivesbuildFileAttachmentswas updated to handle Windows absolute paths (C:/...) and backslash normalization so out-of-workspace files are correctly resolved tofile://URLsScreenshots / Video
file_picker_demo.1.mp4
How to Test
Manual/local verification
bun run typecheck,bun run lint, andbun run test:unitfrompackages/kilo-vscode/— all pass@in the prompt input, and confirmed Browse files... appears near the top of the dropdown, below Terminal and Git changes, with a divider before workspace file resultsC:\Users\...\file.sh) — previously failed with a "file not found" error because the path was resolved relative to the workspace directory; now normalized to forward slashes and resolved as an absolute pathReviewer test steps
@Blocked checks and substitute verification
Checklist