feat(ui): validate inline code spans against filesystem to enable cli… - #10340
feat(ui): validate inline code spans against filesystem to enable cli…#10340sylwester-liljegren wants to merge 1 commit into
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }, 3000) | ||
| }) | ||
| } | ||
| window.addEventListener("message", (event) => { |
There was a problem hiding this comment.
WARNING: Missing onCleanup for window.addEventListener — potential memory leak
The event listener registered here is never removed. Although DataBridge is a top-level component that currently never unmounts during the session, the lack of cleanup is a latent leak: if the component ever remounts (e.g. hot module reload, future refactors, tests) a new listener accumulates while the old one is never removed, causing duplicate validateFilesResult handling.
Consistent with the pattern on lines 322–323 (another window.addEventListener in the same file), wrap this in an onMount/onCleanup pair:
onMount(() => {
const handler = (event: MessageEvent) => {
const msg = event.data
if (msg?.type === "validateFilesResult" && msg.id) {
const cb = pending.get(msg.id)
if (cb) {
pending.delete(msg.id)
cb(msg.existing ?? [])
}
}
}
window.addEventListener("message", handler)
onCleanup(() => window.removeEventListener("message", handler))
})|
|
||
| // File existence validation for code span candidates | ||
| const pending = new Map<string, (existing: string[]) => void>() | ||
| let counter = 0 |
There was a problem hiding this comment.
SUGGESTION: let counter = 0 — prefer const with a ref object per the project style guide
The style guide says to avoid let statements. Since counter is mutated in place, use a single-element object instead:
| let counter = 0 | |
| const counter = { n: 0 } |
And update the increment to const id = \vf-${++counter.n}``.
| // Candidates that exist as files get promoted to .file-link; others stay plain code. | ||
| // Results are cached so morphdom re-renders can be resolved immediately. | ||
| let bodyRef: HTMLDivElement | undefined | ||
| const cache = new Map<string, boolean>() |
There was a problem hiding this comment.
WARNING: cache is never cleared — unbounded memory growth during long sessions
The Map<string, boolean> accumulates one entry per unique candidate path encountered across the lifetime of this component instance. In a long session with many assistant messages referencing many distinct paths, this can grow large. Consider bounding the cache size (e.g. LRU with a cap of ~500 entries), or clearing it when the session changes.
| elements.get(p)!.push(el) | ||
| } | ||
| if (!pending.length || !data.validateFiles) return | ||
| data.validateFiles(pending).then((existing) => { |
There was a problem hiding this comment.
SUGGESTION: Unhandled rejection from validateFiles promise
If data.validateFiles rejects (e.g. the extension returns an error or the promise is rejected for any reason), the rejection is silently swallowed here. Add a .catch to avoid unhandled promise rejections:
| data.validateFiles(pending).then((existing) => { | |
| data.validateFiles(pending).then((existing) => { |
Change to:
data.validateFiles(pending)
.then((existing) => {
const set = new Set(existing)
for (const p of pending) cache.set(p, set.has(p))
for (const [p, els] of elements) {
for (const el of els) promote(el, p, set.has(p))
}
})
.catch(() => { /* validateFiles timed out or failed — candidates stay as plain code */ })
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge OverviewThe previous 4 issues are all resolved. Two new issues found in the incremental diff (
Issue Details (click to expand)WARNING
Previously Resolved Issues (carried forward)
Files Reviewed (2 files changed in incremental diff)
Fix these issues in Kilo Cloud Reviewed by claude-sonnet-4.6 · 126,811 tokens |
02bb498 to
7c08bb7
Compare
| const root = this.getWorkspaceDirectory(this.currentSession?.id) | ||
| fileLinks | ||
| .validateFiles(root, paths) | ||
| .then((existing) => this.postMessage({ type: "validateFilesResult", id, existing })) |
There was a problem hiding this comment.
WARNING: Missing .catch() on handleValidateFiles — unhandled promise rejection risk
If fileLinks.validateFiles() rejects unexpectedly (e.g. an unhandled vscode API error), or if this.postMessage() throws, the rejection is silently lost. The webview's validateFiles call will then time out after 3 seconds and resolve to [], which is recoverable — but the silent failure makes debugging harder and causes unnecessary 3-second delays on every affected render during that error condition.
Add a .catch() to log the failure:
fileLinks
.validateFiles(root, paths)
.then((existing) => this.postMessage({ type: "validateFilesResult", id, existing }))
.catch((err) => console.error("[Kilo New] handleValidateFiles failed:", err))| () => show(uri), | ||
| () => { | ||
| const name = filePath.split(/[\\/]/).pop() || filePath | ||
| vscode.workspace.findFiles(`**/${name}`, "**/node_modules/**", 5).then((matches) => { |
There was a problem hiding this comment.
WARNING: findFiles().then() has no .catch() — errors silently swallowed
If vscode.workspace.findFiles() rejects (e.g. workspace not ready, cancellation), the error is silently lost. Same issue exists on the show() calls at lines 35 and 41 — openTextDocument().then(showTextDocument) rejections are also unhandled.
Add .catch() handlers:
vscode.workspace.findFiles(`**/${name}`, "**/node_modules/**", 5).then((matches) => {
// ... existing logic ...
}).catch((err) => console.error("[Kilo New] findFiles failed:", err))And on the show helper:
const show = (target: vscode.Uri) =>
vscode.workspace.openTextDocument(target)
.then((doc) => vscode.window.showTextDocument(doc, opts))
.catch((err) => console.error("[Kilo New] openFile show failed:", err))
7c08bb7 to
75e6767
Compare
…ckable file links Make file references in agent responses clickable by validating inline code spans against the filesystem. Code spans that match real files in the workspace become clickable links that open the file at the referenced line. Non-existent paths remain as plain code. The implementation introduces a two-phase approach: markdown rendering marks all code spans as file-link candidates, then post-render validation checks each candidate against the workspace filesystem. Confirmed files are promoted to clickable links with cached results to avoid redundant checks during re-renders. Changes include: - New `extractSuffix()` and `normalizeCandidatePath()` utilities for parsing and normalizing candidate paths - Updated `extractFilePathFromHref()` to return structured path/line/column data - Post-render validation in message-part component with caching - VS Code extension filesystem stat checks and fallback workspace search for dead links - Message protocol extensions for validateFiles/validateFilesResult round-trips - CSS styling for file-path links with hover effects - Comprehensive test updates for new path parsing logic
75e6767 to
a2d3cb2
Compare
|
Hey @sylwester-liljegren, thanks for the detailed write-up. The context and testing steps are helpful. One thing: this does affect visible UI behavior, so I don’t think screenshots are N/A here. Could you please add before/after screenshots or a short screen recording showing:
This would make the UI impact much easier to review. Also, since this is a fairly large change with explicit trade-offs, touching rendering, protocol messages, filesystem validation, and click behavior, could you please consider splitting this PR into smaller reviewable pieces if possible? For example, the path parsing/tests, the protocol/filesystem validation, and the UI rendering/click behavior could potentially be reviewed separately. That would make it much easier for us to review carefully and give concrete feedback. |
Good points, @johnnyeric . I thought first that the modifications would only change what code spans were rendered as clickable links or not, hence this decision of not showing any screenshots. But I can fix this (in one of the smaller PRs to be created as per your recommendation). I'll fix this during this week whenever I get the chance. After I have created the smaller PRs, then this initial PR may be closed in favor of those smaller PRs. |
|
@johnnyeric Alright, I have created two separate PR:s for this: #11218 and #11219. #11218 focuses on the protocol for file link validation using the local filesystem, whereas #11219 focuses on path parsing and UI rendering in VS Code. This way, these two parts can be reviewed isolated from each other in their own PR. Keep in mind that in the current state, #11219 contains not only path parsing and UI rendering changes, but also changes seen in #11218, which might make that PR look big initially. However, after #11218 has been merged, the number of files in #11219 will collapse to 7 files, making it easier to review #11219 later on. So you may begin reviewing #11218, whereas I will gather some visuals for how the changes in #11219 will impact the UI as per your recommendation. As soon as #11218 has been merged and #11219 has been supplied with visuals, I'll open it for external review. My recommendation is to keep this PR opened in the mean time until the new PRs have been merged/closed in case another organisation of PRs is desired for better review conditions. |
|
To stay organized pull requests are automatically closed after 30 days of inactivity. If the pull request is still relevant please reopen it or create a fresh new one. |
2 participants
…ckable file links
Make file references in agent responses clickable by validating inline code spans against the filesystem. Code spans that match real files in the workspace become clickable links that open the file at the referenced line. Non-existent paths remain as plain code.
The implementation introduces a two-phase approach: markdown rendering marks all code spans as file-link candidates, then post-render validation checks each candidate against the workspace filesystem. Confirmed files are promoted to clickable links with cached results to avoid redundant checks during re-renders.
Changes include:
extractSuffix()andnormalizeCandidatePath()utilities for parsing and normalizing candidate pathsextractFilePathFromHref()to return structured path/line/column dataContext
File references in agent responses (e.g.
src/foo.ts:42,LICENSE,.gitignore) were not reliably clickable. The original approach used regex-based auto-detection on inline code spans, which produced false positives (Effect.gen,@tsconfig/bun,.envwhen it doesn't exist) and false negatives (extensionless files likeLICENSE,Makefile). This PR replaces regex heuristics with filesystem validation — the extension stat-checks every code span candidate against the workspace, so only real files become clickable links.Implementation
Every backtick code span is a candidate. The
marked.tsxcodespan renderer strips an optional:line[-range][:col]suffix, normalizes the remaining text into a workspace-relative path (prepending./for bare names likeLICENSE), and renders the element with afile-link-candidateCSS class anddata-file-candidateattribute.Post-render validation via extension round-trip. After each markdown render,
message-part.tsxcollects all candidate paths from the DOM, sends them in a singlevalidateFilesmessage to the extension.KiloProvider.handleValidateFilesstat-checks each path against the workspace root, rejects directories (FileType.Directory), and responds with the list of paths that are actual files.Promote or strip. On response, candidates that exist as files get promoted to
.file-link(clickable, dotted underline). Candidates that don't exist get their marker attributes removed and stay as plain<code>. Results are cached in aMap<string, boolean>so that whenmorphdomre-renders the DOM during streaming (resetting elements back tofile-link-candidate), the effect can immediately re-promote from cache without another round-trip.Fallback for
handleOpenFile. If the exact path doesn't exist when clicked (edge case), the extension searches the workspace by filename, offers a QuickPick for multiple matches, or shows a "File not found" warning.Markdown links also supported. The
linkrenderer inmarked.tsxdetects file-path hrefs viaextractFilePathFromHrefand applies afile-path-linkclass with matching visual styling. The click handler destructures{ path, line, column }from the href.Tradeoffs:
postMessageper render cycle.Screenshots
N/A
How to Test
packages/opencode/src/index.ts,LICENSE,.gitignore) appear with dotted underlines and are clickable — clicking opens the file in the editorsrc/foo.ts:42) open at the correct line.envif not present) and non-file code spans (Effect.gen,@tsconfig/bun,useState) render as plain monospace code with no underline