feat(ui): add read_file file preview core with UI resource wiring and tracking#337
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a File Preview UI and server integration: new build script and deps, deterministic UI runtime artifacts, inlined UI resources, client-side preview app/components and styles, shared UI infra (RPC, lifecycle, theme, widget state), server resource endpoints, structured preview metadata on read results, and UI event tracking. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Browser (File Preview UI)
participant Host as Parent Window (MCP Host)
participant Server as Server
participant FS as Filesystem
Browser->>Host: rpc: ui/initialize
Host->>Server: CallTool(read_file, path)
Server->>FS: read file (path)
FS-->>Server: file content + metadata
Server->>Server: attach structuredContent + _meta (FILE_PREVIEW_RESOURCE_URI)
Server-->>Host: tool result (structured content)
Host->>Browser: postMessage(tool result)
Browser->>Browser: resolvePreviewFileType & render
alt markdown
Browser->>Browser: renderMarkdown(...)
else html
Browser->>Browser: renderHtmlPreview(...)
else code/text
Browser->>Browser: renderCodeViewer(...)
else unsupported
Browser->>Browser: render fallback
end
Browser->>Host: rpc: call_tool(track_ui_event, payload)
Host->>Server: CallTool(track_ui_event, payload)
Server->>Server: handleTrackUiEvent -> capture_ui_event
Server-->>Host: ack
Host->>Browser: postMessage(tracked)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:XXL
Nitpicks 🔍
|
|
CodeAnt AI finished reviewing your PR. |
edgarsskore
force-pushed
the
pr/file-preview-core-tracking
branch
from
69d0dbc to
9003ce4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/ui/file-preview/src/components/highlighting.ts`:
- Around line 36-37: Remove the incorrect aliasing of TOML and INI to the YAML
grammar: stop calling hljs.registerLanguage('toml', yaml) and
hljs.registerLanguage('ini', yaml) so we don't mis-highlight those formats;
instead either register proper community grammars for TOML/INI (e.g., use
highlightjs-lang/toml or an appropriate ini grammar) by registering them with
hljs.registerLanguage('toml', tomlGrammar) / hljs.registerLanguage('ini',
iniGrammar), or remove the aliases so highlightSource falls back to
hljs.highlightAuto(code) or plain text for those filetypes; update the
registration logic around the hljs.registerLanguage calls to reflect this
change.
In `@src/ui/file-preview/src/components/markdown-renderer.ts`:
- Around line 26-30: The class attribute uses raw normalizedLanguage from user
markdown; sanitize it in highlight(code, language) by stripping any characters
except [a-z0-9_-] (lowercase) and falling back to "text" if the result is empty
so attribute injection cannot occur; update the normalizedLanguage computation
in the highlight method (and any use of normalizedLanguage in the returned
template) to use the sanitized value.
In `@src/ui/shared/host-lifecycle.ts`:
- Around line 40-48: The initialize method currently dispatches
rpcClient.request('ui/initialize', ...) and immediately calls
rpcClient.notify('ui/notifications/initialized', {}), causing the initialized
notification to fire before the handshake resolves; change initialize (the
initialize function in host-lifecycle.ts) to await the request call inside a
try/catch and only call rpcClient.notify('ui/notifications/initialized', {})
after the await completes (in the success path if the host requires ordering, or
in both success and the catch if you still want best-effort notification),
ensuring any errors are caught and logged but do not crash rendering.
In `@src/ui/shared/theme-adaptation.ts`:
- Around line 95-112: The current loop in extractCssVariableMap returns `next`
as soon as it processes the first object-typed `candidate`, which can yield an
empty `{}` and prevent checking later candidates; change the logic so you only
return `next` when it contains at least one valid variable (e.g., check
`Object.keys(next).length > 0`) and otherwise continue to the next `candidate`,
and after the loop return an empty map if no candidates produced variables;
reference symbols: `extractCssVariableMap`, `rawMapCandidates`, `isObject`,
`isSafeCssVariableName`.
In `@src/ui/styles/components/tool-header.css`:
- Around line 162-165: The focus ring CSS for .icon-button:focus-visible is
using --border; update the rule to use the dedicated focus token --focus-ring
(with a safe fallback to --border) so focus indicators use the intended color
and better meet contrast requirements; adjust the declaration for
.icon-button:focus-visible to reference var(--focus-ring, var(--border)) while
keeping outline and outline-offset as-is.
In `@test/test-ui-event-tracking.js`:
- Around line 17-41: The test currently invokes handleTrackUiEvent via
testTrackUiEventCall which calls capture(...) and sends real GA4 requests;
update the test to avoid firing real network calls by mocking or stubbing the
capture function used by handleTrackUiEvent (or enable a dry-run/test mode) so
capture does not perform HTTP requests. Locate handleTrackUiEvent and the test
helper getRequestHandler / testTrackUiEventCall, replace or inject a mock
capture implementation that records calls and returns a dummy resolved value, or
toggle a test flag so handleTrackUiEvent skips real network calls, then assert
against the mocked call results instead of relying on real analytics.
🧹 Nitpick comments (19)
tsconfig.json (1)
17-20: Thesrc/**/*.d.tspattern is redundant.
src/**/*.tsalready matches.d.tsfiles. The extra entry is harmless but unnecessary.package.json (1)
93-95:highlight.jsandmarkdown-itshould bedevDependencies.These libraries are bundled by esbuild into the UI runtime at build time and aren't needed at server runtime. Listing them as
dependenciesbloats the install for consumers of the npm package.Suggested move
"dependencies": { - "highlight.js": "^11.11.1", ... - "markdown-it": "^14.1.0", ... }, "devDependencies": { + "highlight.js": "^11.11.1", + "markdown-it": "^14.1.0", ... }src/ui/styles/components/tool-header.css (1)
51-73: Pill variants are visually identical — consider consolidating or differentiating.
.file-pill--md,.file-pill--html, and.file-pill--jsoncurrently share identical declarations. If intentional placeholders for future theming, a brief comment would help. Otherwise, they can be collapsed into the base.file-pillrule.src/ui/shared/tool-header.ts (1)
30-32:actionsHtmlis injected without escaping — document the trust assumption.Unlike other config fields,
actionsHtmlis inserted as raw HTML. This is fine for internal use, but the interface should document that callers are responsible for sanitization to prevent XSS if this ever accepts external input.Suggested documentation
export interface ToolHeaderConfig { pillLabel: string; pillClassName?: string; title: string; subtitle: string; badges: string[]; + /** Raw HTML for the actions area. Caller must ensure this is safe/sanitized. */ actionsHtml: string; }src/ui/shared/rpc-client.ts (2)
36-42: Permissive defaults:targetOrigin: '*'andisTrustedSource: () => true.Both defaults together mean zero origin validation on sent and received messages. Any co-hosted frame could spoof responses and resolve/reject pending promises. While acceptable for a trusted MCP host environment, consider either:
- Requiring
targetOrigin(no default), or- At minimum, documenting the security trade-off in the JSDoc.
59-77: Unhandled promise rejection on timeout when no.catch()is attached.If a caller fires
request()without attaching a.catch()orawaitin a try/catch, the timeout rejection becomes an unhandled promise rejection. This is a general caller-responsibility concern, but since the timeout is built into the client, consider whether a default error handler or logging would be appropriate for debugging.src/ui/shared/host-lifecycle.ts (1)
33-38:ResizeObserveris never disconnected — no dispose path.Unlike
tool-shell.tswhich has adispose()method that cleans up listeners, theResizeObservercreated here has no teardown mechanism. If the UI is mounted/unmounted multiple times, observers will accumulate.Consider adding a
disposemethod toUiHostLifecyclethat callsobserver.disconnect().src/ui/shared/theme-adaptation.ts (1)
48-64: RecursiveresolveThemeModecould stack-overflow on adversarial input.
resolveThemeModerecurses into candidates returned bypickThemeCandidate, which extracts nested properties from the same object tree. If the input has circular references or deeply nested structures, this will blow the stack.In practice,
postMessagedata is structured-cloned (no circulars), so risk is low. A simple depth guard would make it robust:Optional depth-limited approach
-export function resolveThemeMode(value: unknown): ThemeMode | undefined { +export function resolveThemeMode(value: unknown, depth = 0): ThemeMode | undefined { + if (depth > 5) return undefined; const direct = normalizeThemeMode(value); if (direct) { return direct; } ... for (const candidate of pickThemeCandidate(value)) { - const resolved = resolveThemeMode(candidate); + const resolved = resolveThemeMode(candidate, depth + 1);src/ui/file-preview/index.html (1)
1-16: Consider adding aContent-Security-Policymeta tag for defense-in-depth.This page renders user-supplied file content (including HTML previews). A restrictive CSP meta tag would mitigate XSS risk from malicious file content, e.g.:
<meta http-equiv="Content-Security-Policy" content="default-src 'none'; style-src 'self' 'unsafe-inline'; script-src 'self'; img-src data:;" />Tune the directives to match what the preview actually needs (e.g., if markdown rendering uses inline styles).
test/test-ui-event-tracking.js (1)
9-15: Accessingserver._requestHandlersis fragile — relies on MCP SDK internals.This accessor isn't part of the public API and may break on SDK upgrades. Acceptable for integration tests, but worth a brief comment noting the coupling.
src/handlers/filesystem-handlers.ts (1)
148-163: Content is duplicated incontent[0].textandstructuredContent.contentfor text files.Both carry the full
textContentstring, doubling memory for the response payload. Thelengthread option bounds it, but for the default 1000-line limit this could still be meaningful. Consider whetherstructuredContentcould reference or defer to the primary content instead.src/ui/file-preview/src/components/highlighting.ts (1)
39-41: TheescapeHtmlre-export wrapper adds no value over importingsharedEscapeHtmldirectly.This is a single-line delegation. Consumers in this package could import from
'../../../shared/escape-html.js'directly, removing a layer of indirection.src/ui/file-preview/src/components/html-renderer.ts (1)
12-29: Regex-based HTML sanitization is fragile — consider using a proper sanitizer like DOMPurify.The regex approach has known bypass vectors (e.g., malformed tags, encoding tricks). While the sandboxed iframe + CSP provide a strong secondary boundary,
sanitizeHtmlis still the first line of defense whenallowUnsafeScriptsis false.Additionally,
<style>is not in the blocked tag list, which allows CSS injection within the sandboxed frame (the CSP permitsstyle-src 'unsafe-inline').Suggested improvement
If adding DOMPurify is not desired, at minimum add
styleto the blocked-tag list (or allowlist only safe tags instead of blocklisting):- const blockedTagPattern = /<\/?(script|iframe|object|embed|link|meta|base|form)[^>]*>/gi; + const blockedTagPattern = /<\/?(script|iframe|object|embed|link|meta|base|form|style)[^>]*>/gi;src/ui/file-preview/src/components/code-viewer.ts (1)
64-67:normalizedLanguageis injected unescaped into an HTML class attribute.While current callers pass safe language identifiers from the extension map, the
languageparameter is astring— if an unexpected value flows in, it could break the HTML structure.Defensive fix
export function renderCodeViewer(code: string, language = 'text'): string { - const normalizedLanguage = language || 'text'; + const normalizedLanguage = (language || 'text').replace(/[^a-z0-9_-]/gi, ''); const highlighted = highlightSource(code, normalizedLanguage); return `<pre class="code-viewer"><code class="hljs language-${normalizedLanguage}">${highlighted}</code></pre>`; }src/ui/file-preview/src/app.ts (4)
335-335:body.noticeis injected into HTML without escaping.All current notice values are hardcoded strings, so this is safe today. However, if a future change introduces dynamic content in a notice, this becomes an XSS vector. Consider escaping defensively.
Defensive fix
- const notice = body.notice ? `<div class="notice">${body.notice}</div>` : ''; + const notice = body.notice ? `<div class="notice">${escapeHtml(body.notice)}</div>` : '';
146-162: OS detection viauserAgent.includes('win')is brittle.While this works today since "darwin" and common Linux user agents don't contain "win", some niche Linux distros or browser variants could include "win" in their UA string. Consider checking for
'win'at the start of the platform substring or usingnavigator.platform/navigator.userAgentData?.platformfor more reliable detection.More robust alternative
- const userAgent = navigator.userAgent.toLowerCase(); - if (userAgent.includes('win')) { + const platform = (navigator.userAgentData?.platform ?? navigator.platform ?? '').toLowerCase(); + if (platform.startsWith('win')) { const escaped = trimmedPath.replace(/"/g, '""'); return `explorer /select,"${escaped}"`; } - if (userAgent.includes('mac')) { + if (platform.startsWith('mac') || platform === 'darwin') { return `open -R ${shellQuote(trimmedPath)}`; }
220-243: Copy button tooltip never resets to original state after copying.After a successful or failed copy,
setButtonStatechanges the title/aria-label to 'Copied' or 'Copy failed', but there's no logic to reset it back to 'Copy source'. This means subsequent hover/screen-reader interactions will show stale state.Suggested fix: reset after a delay
copyButton.addEventListener('click', async () => { trackUiEvent?.('copy_clicked', { file_type: payload.fileType, file_extension: getFileExtensionForAnalytics(payload.filePath) }); + const originalLabel = copyButton.getAttribute('title') ?? 'Copy source'; + try { if (navigator.clipboard?.writeText) { await navigator.clipboard.writeText(payload.content); setButtonState('Copied'); + setTimeout(() => setButtonState(originalLabel), 2000); return; } } catch { // fallback below } const copied = fallbackCopy(payload.content); setButtonState(copied ? 'Copied' : 'Copy failed'); + setTimeout(() => setButtonState(originalLabel), 2000); });
439-490: Indentation is off inside the message event handler —trybody is not indented relative to thetry.Lines 441–485 are indented at the same level as the
trykeyword on line 440, making the block harder to read. Thecatchat line 486 is at the correct level.Fix indentation
window.addEventListener('message', (event) => { try { - if (rpcClient.handleMessageEvent(event)) { - return; - } + if (rpcClient.handleMessageEvent(event)) { + return; + }(and so on for the rest of the try body)
src/ui/resources.ts (1)
64-75: No caching forgetFilePreviewResourceText— every call reads three files from disk.Each
resources/readrequest triggersfs.readFilefor the HTML template, CSS, and JS runtime, plus the inlining pipeline. If this resource is fetched frequently (e.g., on everyread_filetool call), this could become a performance concern. A lazy-initialized cache would be straightforward.Optional: add memoization
+let cachedFilePreviewHtml: string | undefined; + export async function getFilePreviewResourceText(): Promise<string> { - return readInlinedResourceHtml(DIST_FILE_PREVIEW_DIR, 'preview-runtime.js'); + if (!cachedFilePreviewHtml) { + cachedFilePreviewHtml = await readInlinedResourceHtml(DIST_FILE_PREVIEW_DIR, 'preview-runtime.js'); + } + return cachedFilePreviewHtml; }
| hljs.registerLanguage('toml', yaml); | ||
| hljs.registerLanguage('ini', yaml); |
There was a problem hiding this comment.
TOML and INI are registered with the YAML grammar, which will produce incorrect highlighting.
TOML has fundamentally different syntax from YAML (e.g., [table] headers, inline tables, triple-quoted strings). Aliasing it to YAML will miscolor valid TOML. highlight.js doesn't ship a built-in TOML grammar, but a community grammar exists (highlightjs-lang/toml). Alternatively, fall back to plain text for TOML rather than mislabel it.
INI is closer to YAML structurally but still not a match — consider the same treatment.
🔧 Minimal fix: remove the misleading aliases and let highlightAuto handle them
hljs.registerLanguage('yaml', yaml);
-hljs.registerLanguage('toml', yaml);
-hljs.registerLanguage('ini', yaml);With these lines removed, highlightSource will fall through to hljs.highlightAuto(code) for TOML/INI, which may or may not produce good results but won't actively misrepresent the grammar.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| hljs.registerLanguage('toml', yaml); | |
| hljs.registerLanguage('ini', yaml); |
🤖 Prompt for AI Agents
In `@src/ui/file-preview/src/components/highlighting.ts` around lines 36 - 37,
Remove the incorrect aliasing of TOML and INI to the YAML grammar: stop calling
hljs.registerLanguage('toml', yaml) and hljs.registerLanguage('ini', yaml) so we
don't mis-highlight those formats; instead either register proper community
grammars for TOML/INI (e.g., use highlightjs-lang/toml or an appropriate ini
grammar) by registering them with hljs.registerLanguage('toml', tomlGrammar) /
hljs.registerLanguage('ini', iniGrammar), or remove the aliases so
highlightSource falls back to hljs.highlightAuto(code) or plain text for those
filetypes; update the registration logic around the hljs.registerLanguage calls
to reflect this change.
| highlight(code: string, language: string): string { | ||
| const normalizedLanguage = (language || 'text').toLowerCase(); | ||
| const highlighted = highlightSource(code, normalizedLanguage); | ||
| return `<pre class="code-viewer"><code class="hljs language-${normalizedLanguage}">${highlighted}</code></pre>`; | ||
| } |
There was a problem hiding this comment.
Minor XSS vector: unescaped language name in class attribute.
The normalizedLanguage value originates from user-authored markdown fenced code blocks (e.g., ```some"lang). Injecting it directly into an HTML class attribute without sanitization could allow attribute injection. Consider stripping non-alphanumeric characters.
🛡️ Proposed fix
highlight(code: string, language: string): string {
- const normalizedLanguage = (language || 'text').toLowerCase();
+ const normalizedLanguage = (language || 'text').toLowerCase().replace(/[^a-z0-9_-]/g, '');
const highlighted = highlightSource(code, normalizedLanguage);🤖 Prompt for AI Agents
In `@src/ui/file-preview/src/components/markdown-renderer.ts` around lines 26 -
30, The class attribute uses raw normalizedLanguage from user markdown; sanitize
it in highlight(code, language) by stripping any characters except [a-z0-9_-]
(lowercase) and falling back to "text" if the result is empty so attribute
injection cannot occur; update the normalizedLanguage computation in the
highlight method (and any use of normalizedLanguage in the returned template) to
use the sanitized value.
| initialize: () => { | ||
| void rpcClient.request('ui/initialize', { | ||
| app: { name: appName, version: appVersion }, | ||
| capabilities: {}, | ||
| }).catch(() => { | ||
| // Initialization handshake failure should not break rendering. | ||
| }); | ||
| rpcClient.notify('ui/notifications/initialized', {}); | ||
| }, |
There was a problem hiding this comment.
initialized notification fires before the handshake resolves — potential race.
rpcClient.request('ui/initialize', ...) is fire-and-forget (void + .catch()), but rpcClient.notify('ui/notifications/initialized', {}) on line 47 executes synchronously after the request is dispatched, not after it resolves. If the host expects the handshake to complete before receiving the initialized signal, this is a sequencing bug.
If the host is tolerant of ordering, this is fine. Otherwise:
Proposed fix
initialize: () => {
- void rpcClient.request('ui/initialize', {
+ rpcClient.request('ui/initialize', {
app: { name: appName, version: appVersion },
capabilities: {},
}).catch(() => {
// Initialization handshake failure should not break rendering.
+ }).finally(() => {
+ rpcClient.notify('ui/notifications/initialized', {});
});
- rpcClient.notify('ui/notifications/initialized', {});
},🤖 Prompt for AI Agents
In `@src/ui/shared/host-lifecycle.ts` around lines 40 - 48, The initialize method
currently dispatches rpcClient.request('ui/initialize', ...) and immediately
calls rpcClient.notify('ui/notifications/initialized', {}), causing the
initialized notification to fire before the handshake resolves; change
initialize (the initialize function in host-lifecycle.ts) to await the request
call inside a try/catch and only call
rpcClient.notify('ui/notifications/initialized', {}) after the await completes
(in the success path if the host requires ordering, or in both success and the
catch if you still want best-effort notification), ensuring any errors are
caught and logged but do not crash rendering.
| for (const candidate of rawMapCandidates) { | ||
| if (!isObject(candidate)) { | ||
| continue; | ||
| } | ||
|
|
||
| const next: Record<string, string> = {}; | ||
| for (const [rawKey, rawValue] of Object.entries(candidate)) { | ||
| if (typeof rawValue !== 'string') { | ||
| continue; | ||
| } | ||
| const key = rawKey.startsWith('--') ? rawKey : `--${rawKey}`; | ||
| if (!isSafeCssVariableName(key)) { | ||
| continue; | ||
| } | ||
| next[key] = rawValue.trim(); | ||
| } | ||
| return next; | ||
| } |
There was a problem hiding this comment.
extractCssVariableMap returns on the first non-empty candidate — is that intentional?
The return next at line 111 fires as soon as the first isObject candidate is found, even if that candidate yields zero valid CSS variable entries (since next could be {}). This means if the first object-typed candidate has no string values, you'll get {} back instead of checking subsequent candidates.
If the intent is to return the first candidate that has actual variables, move the return inside a length check:
Proposed fix
const next: Record<string, string> = {};
for (const [rawKey, rawValue] of Object.entries(candidate)) {
...
}
- return next;
+ if (Object.keys(next).length > 0) {
+ return next;
+ }
}🤖 Prompt for AI Agents
In `@src/ui/shared/theme-adaptation.ts` around lines 95 - 112, The current loop in
extractCssVariableMap returns `next` as soon as it processes the first
object-typed `candidate`, which can yield an empty `{}` and prevent checking
later candidates; change the logic so you only return `next` when it contains at
least one valid variable (e.g., check `Object.keys(next).length > 0`) and
otherwise continue to the next `candidate`, and after the loop return an empty
map if no candidates produced variables; reference symbols:
`extractCssVariableMap`, `rawMapCandidates`, `isObject`,
`isSafeCssVariableName`.
| .icon-button:focus-visible { | ||
| outline: 2px solid var(--border); | ||
| outline-offset: 2px; | ||
| } |
There was a problem hiding this comment.
Focus ring uses --border instead of --focus-ring.
The base CSS defines a --focus-ring token specifically for focus indicators. Using --border here misses the opportunity for distinct focus styling and may not meet WCAG contrast requirements against the border color.
Suggested fix
.icon-button:focus-visible {
- outline: 2px solid var(--border);
+ outline: 2px solid var(--focus-ring);
outline-offset: 2px;
}🤖 Prompt for AI Agents
In `@src/ui/styles/components/tool-header.css` around lines 162 - 165, The focus
ring CSS for .icon-button:focus-visible is using --border; update the rule to
use the dedicated focus token --focus-ring (with a safe fallback to --border) so
focus indicators use the intended color and better meet contrast requirements;
adjust the declaration for .icon-button:focus-visible to reference
var(--focus-ring, var(--border)) while keeping outline and outline-offset as-is.
| async function testTrackUiEventCall() { | ||
| console.log('\n--- Test: track_ui_event call ---'); | ||
| const callToolHandler = getRequestHandler('tools/call'); | ||
| const response = await callToolHandler({ | ||
| method: 'tools/call', | ||
| params: { | ||
| name: 'track_ui_event', | ||
| arguments: { | ||
| event: 'widget_expanded', | ||
| component: 'file_preview', | ||
| params: { | ||
| file_type: 'markdown', | ||
| line_count: 36, | ||
| expanded: true | ||
| } | ||
| } | ||
| } | ||
| }, {}); | ||
|
|
||
| assert.ok(response, 'tools/call should return a response'); | ||
| assert.ok(Array.isArray(response.content), 'track_ui_event should return content array'); | ||
| assert.strictEqual(response.isError, undefined, 'track_ui_event should not return isError'); | ||
| assert.ok(response.content[0].text.includes('Tracked UI event'), 'track_ui_event should acknowledge event tracking'); | ||
| console.log('✓ track_ui_event call works'); | ||
| } |
There was a problem hiding this comment.
Test sends real analytics data via capture() to Google Analytics.
handleTrackUiEvent calls capture(...) which fires a real HTTP request to GA4. This pollutes production analytics with test events and will fail or hang if the network is unavailable. Consider mocking capture or adding a test/dry-run mode.
🤖 Prompt for AI Agents
In `@test/test-ui-event-tracking.js` around lines 17 - 41, The test currently
invokes handleTrackUiEvent via testTrackUiEventCall which calls capture(...) and
sends real GA4 requests; update the test to avoid firing real network calls by
mocking or stubbing the capture function used by handleTrackUiEvent (or enable a
dry-run/test mode) so capture does not perform HTTP requests. Locate
handleTrackUiEvent and the test helper getRequestHandler / testTrackUiEventCall,
replace or inject a mock capture implementation that records calls and returns a
dummy resolved value, or toggle a test flag so handleTrackUiEvent skips real
network calls, then assert against the mocked call results instead of relying on
real analytics.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/ui/file-preview/src/app.ts`:
- Around line 146-162: The Windows branch in buildOpenInFolderCommand only
escapes quotes and is vulnerable to cmd.exe metacharacter injection when
terminalManager.executeCommand runs via cmd.exe; update
buildOpenInFolderCommand's Windows branch to stop returning a raw cmd line and
instead return a PowerShell-based invocation that treats the file path as a
single, safely quoted/escaped argument (or change the callsite to pass args as
an array if supported) so metacharacters like & | ^ % > < are not interpreted;
modify the Windows branch in buildOpenInFolderCommand to produce a
powershell.exe invocation that calls Start-Process (or equivalent) with the path
passed as a single argument, preserving the existing behavior of selecting the
file, and keep the mac/Linux branches unchanged.
In `@src/ui/file-preview/src/components/html-renderer.ts`:
- Around line 46-55: The CSS values from resolveThemeFrameStyles are injected
raw into the frameDocument in renderSandboxedHtmlFrame, allowing a crafted theme
value (palette.background, palette.text, palette.fontFamily) to break out of the
style block; validate or escape these three values before interpolation (e.g.,
run them through a sanitizer that strips or rejects characters like ; } < > and
quotes, or apply a strict whitelist/regex for valid CSS color/font tokens) and
use the sanitized results when building frameDocument (keep using escapeHtml for
srcdoc); update the code paths that reference palette.background, palette.text,
and palette.fontFamily to use the sanitized/validated variables so no untrusted
characters can close the rule or inject new declarations.
- Around line 12-29: The regex in sanitizeHtml currently defined by
blockedTagPattern misses dangerous tags like style and svg (and others such as
math, template), so update the blockedTagPattern in the sanitizeHtml function to
include "style" and "svg" (and consider adding "math", "template", "picture",
"source" or any other inline container tags you deem risky) so those tags are
stripped; also ensure the pattern still uses the same flags (/gi) and matches
both opening and closing tags for blockedTagPattern to reliably remove them
before proceeding with attribute filtering.
In `@src/ui/styles/base.css`:
- Line 18: The CSS custom property --shadow-sm is self-referential (uses
var(--shadow-sm, none)), creating a cyclic/invalid value; fix it by replacing
the self-reference with either a direct default (set --shadow-sm: none;) or
point it to a distinct host-provided token (e.g., use the host token name
instead of --shadow-sm in the var() call) so the variable resolves correctly;
update the declaration where --shadow-sm is defined to use the
non-self-referential value.
🧹 Nitpick comments (23)
tsconfig.json (1)
17-19:"src/**/*.d.ts"is redundant —"src/**/*.ts"already matches.d.tsfiles.The glob
*.tsmatches any filename ending in.ts, which includes.d.tsfiles. The second pattern has no effect. It's harmless, but if clarity of intent was the goal, a brief comment might serve better than a redundant glob.package.json (2)
93-95:highlight.jsandmarkdown-itare only consumed at build time — consider moving todevDependencies.These libraries are bundled into the UI runtime artifact by esbuild during the build step. They aren't imported at Node.js server runtime. Keeping them in
dependenciesinflates the production install footprint for end-users who install the published package (wheredist/already contains the bundled output).
34-34: Build command is getting unwieldy — consider extracting to a shell script or splitting into named npm scripts.The
buildcommand is now a single very long&&-chained line. This makes it hard to read, maintain, and debug when one step fails. Consider breaking it into composable sub-scripts (e.g.,build:ts,build:copy,build:ui) composed vianpm-run-allor a simple shell script.src/ui/shared/host-lifecycle.ts (1)
33-38:ResizeObserveris never disconnected — no disposal path exposed.
observeResize()creates aResizeObserverthat lives forever. TheUiHostLifecycleinterface has nodisposemethod, so the observer can never be cleaned up. While the browser will GC it when the page unloads, if the lifecycle is recreated (e.g., hot-reload), observers accumulate.Consider returning the observer or adding a
disposemethod to the interface.src/ui/file-preview/src/components/markdown-renderer.ts (1)
4-6: Use@types/markdown-itto eliminate unnecessary@ts-expect-errorsuppression.The project uses markdown-it v14.1.0, which has compatible typings available via the
@types/markdown-itpackage (v14.1.2). Installing this package would allow you to remove the@ts-expect-errorsuppression on line 5 and the manual type definitions and casting on lines 9–20, simplifying the file significantly.src/types.ts (1)
84-84:Record<string, unknown>widensstructuredContentto accept any object.The union
FilePreviewStructuredContent | Record<string, unknown>effectively erases all type safety since any object satisfiesRecord<string, unknown>. If the intent is to allow future extensibility, a discriminated union or a base interface with atypefield would preserve compile-time checks for known shapes.Not blocking, but worth noting for future maintainability.
test/test-file-handlers.js (1)
285-340: Good coverage of the preview metadata contract.The test exercises markdown, text, HTML, image, and null-args paths. A couple of observations:
Implicit dependency on
_meta.uiexistence (Line 307, 315, 323, 332): Accessing_meta.ui.resourceUriwill throw aTypeErrorif_metaexists butuiis absent, rather than producing a clean assertion failure. Consider asserting_meta.uiexists before drilling into it, or accept the thrown error as a sufficient test failure signal.Script-tag test data (Line 297): Writing
<script>alert(1)</script>as test data is fine for type inference testing. No security concern since it stays on disk and is read back as text.src/ui/styles/apps/file-preview.css (3)
95-98: Stylelint flags multi-declaration single-line keyframes.Each keyframe stop has multiple declarations on one line. While compact, Stylelint's
declaration-block-single-line-max-declarationsrule flags this. Expanding to multi-line silences the linter.Suggested fix
`@keyframes` pulse-dot { - 0%, 100% { opacity: 0.35; transform: scale(0.9); } - 50% { opacity: 1; transform: scale(1); } + 0%, 100% { + opacity: 0.35; + transform: scale(0.9); + } + 50% { + opacity: 1; + transform: scale(1); + } }
195-257: HLJS selector names are external — Stylelint kebab-case warnings are false positives.The
.hljs-built_in,.language_,.class_,.function_selectors match highlight.js output and cannot be renamed. Consider adding a Stylelint disable comment or configuringselector-class-patternto allow these patterns in this file.
268-289: Consider modern range notation for the media query.Stylelint's
media-feature-range-notationrule suggests using context (range) syntax. If browser support allows:Suggested fix
-@media (max-width: 720px) { +@media (width <= 720px) {src/ui/styles/components/tool-header.css (2)
141-145: Use lowercasecurrentcolorper CSS spec.Stylelint's
value-keyword-caserule expectscurrentcolor(lowercase). Both work, but consistency with the spec is preferred.Suggested fix
.icon-button svg { width: 14px; height: 14px; - fill: currentColor; + fill: currentcolor; }
51-73: Three file-pill variants are identical — intentional placeholder?
.file-pill--md,.file-pill--html, and.file-pill--jsonshare identical declarations (transparent background,var(--text)color,var(--border)border-color). If these are placeholders for future differentiation, a brief comment would clarify intent. Otherwise, they can be collapsed into a single rule.src/ui/shared/tool-header.ts (1)
30-32:actionsHtmlis injected as raw HTML — ensure it's always trusted.Unlike all other config fields that go through
escapeHtml,actionsHtmlis inserted verbatim. This is fine for server-generated button markup, but any future caller must ensure this value is never derived from user input without sanitization.A brief JSDoc note on
actionsHtmlin the interface would make the trust boundary explicit.src/handlers/history-handlers.ts (1)
56-77: Verify thatparamsvalues routed tocaptureare free of PII.The
paramsbag is an openRecord<string, string | number | boolean | null>forwarded directly to the analytics pipeline. If any client ever includes identifiers (file paths with usernames, etc.) in params, they'll be persisted.Consider either:
- Allowlisting expected param keys in the schema, or
- Documenting the trust boundary so callers know not to include PII.
Not blocking — the current UI event use-cases (expand, collapse, copy) look safe.
src/handlers/filesystem-handlers.ts (1)
147-163: Text content is duplicated in bothcontent[0].textandstructuredContent.content.For large files (up to
fileReadLineLimitlines), this doubles the response payload. The duplication is understandable for backward compatibility, but for files approaching the line limit, the size impact may be non-trivial.Consider either:
- Omitting
contentfromstructuredContentfor text files (the UI client can read fromcontent[0].text), or- Documenting this as intentional so future maintainers don't re-derive it.
src/ui/file-preview/src/components/toolbar.ts (1)
7-23:inferFilePilldoesn't handle the'text'or'unsupported'file types explicitly.This works because those types fall through to the extension-based path, which is reasonable. However, for
'unsupported'file types (e.g., a binary with no extension),extensionMatchwill benull, defaulting to'txt'label withfile-pill--textclass — slightly misleading since the file isn't text.Consider adding an explicit early return for
'unsupported':Suggested improvement
function inferFilePill(payload: PreviewStructuredContent): { label: string; className: string } { + if (payload.fileType === 'unsupported') { + return { label: 'FILE', className: 'file-pill--text' }; + } if (payload.fileType === 'markdown') { return { label: 'MD', className: 'file-pill--md' }; }src/ui/file-preview/shared/preview-file-types.ts (2)
41-47: Dotfile variants like.env.local,.env.productionwon't match.
TEXT_PREVIEW_BASENAMESmatches exact basenames, so.envmatches but.env.localdoesn't. Its extension (.local) isn't inTEXT_PREVIEW_EXTENSIONSeither, so these common config files fall to'unsupported'. Consider a startsWith/prefix check or adding common compound dotfile extensions.One approach: match basename prefix for dotenv files
+const TEXT_PREVIEW_BASENAME_PREFIXES = ['.env']; + export function resolvePreviewFileType(filePath: string): PreviewFileType { const normalizedPath = filePath.toLowerCase(); const extension = path.extname(normalizedPath); const basename = path.basename(normalizedPath); if (MARKDOWN_PREVIEW_EXTENSIONS.has(extension)) { return 'markdown'; } if (HTML_PREVIEW_EXTENSIONS.has(extension)) { return 'html'; } - if (TEXT_PREVIEW_EXTENSIONS.has(extension) || TEXT_PREVIEW_BASENAMES.has(basename)) { + if (TEXT_PREVIEW_EXTENSIONS.has(extension) || TEXT_PREVIEW_BASENAMES.has(basename) || TEXT_PREVIEW_BASENAME_PREFIXES.some(p => basename.startsWith(p))) { return 'text'; } return 'unsupported'; }
11-39: Several common text-based extensions are missing.Extensions like
.c,.cpp,.h,.cs,.php,.swift,.kt,.vue,.svelte,.csv,.conf,.cfg,.bat,.ps1, etc. will fall to'unsupported'. These are all plaintext-renderable. Consider expanding the set or switching to an allowlist + denylist approach (block known binary extensions instead).This isn't blocking since the fallback is graceful, but users reading
.cor.cppfiles won't get previews.src/ui/file-preview/src/app.ts (5)
335-335:body.noticeis interpolated as raw HTML without escaping.Currently all notice strings are developer-controlled literals, so there's no immediate XSS. But this is fragile — if any future code path produces a notice containing user-supplied text, it becomes an injection vector.
Defensive fix
- const notice = body.notice ? `<div class="notice">${body.notice}</div>` : ''; + const notice = body.notice ? `<div class="notice">${escapeHtml(body.notice)}</div>` : '';
15-19: Module-level mutable state couples app lifecycle to module scope.Five
letbindings at module scope (isExpanded,onRender,trackUiEvent,rpcCallTool,shellController) create implicit shared state. This is acceptable for a single-instance embedded app, but if this module is ever imported into tests or instantiated multiple times, it will cause state leakage.Consider grouping these into a single state object or a factory function that returns an app instance — no urgency, just noting for future extensibility.
35-46: Type guard doesn't validatefileTypeagainst thePreviewFileTypeunion.
isPreviewStructuredContentaccepts any string forfileType. A payload withfileType: 'evil'would pass validation and reachrenderBody, where it would be treated as a non-markdown text file (line 180). This isn't a security issue but weakens the type guarantee.Optional tightening
+const VALID_FILE_TYPES = new Set(['markdown', 'text', 'html', 'unsupported']); + function isPreviewStructuredContent(value: unknown): value is PreviewStructuredContent { if (!isObject(value)) { return false; } return ( typeof value.fileName === 'string' && typeof value.filePath === 'string' && - typeof value.fileType === 'string' && + typeof value.fileType === 'string' && VALID_FILE_TYPES.has(value.fileType) && typeof value.content === 'string' ); }
432-436: Magic timeout values (140ms, 120ms) without explanation.Lines 434 and 461 use
setTimeoutwith 140ms and 120ms delays respectively. These appear to be timing heuristics for letting the host settle, but without comments they're opaque and fragile.Add brief comments explaining why these specific delays exist and what they're waiting for (e.g., host frame readiness, RPC handshake completion).
Also applies to: 459-462
439-490: Indentation misalignment inside themessageevent handler.The
tryblock at line 440 opens, but the body (lines 441–488) is not indented relative to it. This makes the control flow harder to follow —catchon line 486 looks like it's at the same level asifstatements that are logically inside thetry.Fix indentation
window.addEventListener('message', (event) => { try { - if (rpcClient.handleMessageEvent(event)) { - return; - } + if (rpcClient.handleMessageEvent(event)) { + return; + } // ... (indent all lines within the try block consistently)
| function sanitizeHtml(rawHtml: string): string { | ||
| const blockedTagPattern = /<\/?(script|iframe|object|embed|link|meta|base|form)[^>]*>/gi; | ||
| let safe = rawHtml.replace(blockedTagPattern, ''); | ||
|
|
||
| safe = safe.replace(/\son[a-z]+\s*=\s*(".*?"|'.*?'|[^\s>]+)/gi, ''); | ||
| safe = safe.replace(/\s(href|src)\s*=\s*(".*?"|'.*?'|[^\s>]+)/gi, (match, attr, value) => { | ||
| const strippedValue = String(value).replace(/^['"]|['"]$/g, '').trim().toLowerCase(); | ||
| if (strippedValue.startsWith('javascript:')) { | ||
| return ` ${attr}="#"`; | ||
| } | ||
| if (strippedValue.startsWith('data:text/html')) { | ||
| return ` ${attr}="#"`; | ||
| } | ||
| return match; | ||
| }); | ||
|
|
||
| return safe; | ||
| } |
There was a problem hiding this comment.
Regex-based HTML sanitizer has known bypass vectors — <style> and <svg> tags are not blocked.
The blocklist on Line 13 misses style (CSS-based data exfiltration/keylogging), svg (can host inline <script> or event handlers), and other dangerous elements. Since this is defense-in-depth behind the iframe sandbox + CSP, this isn't critical, but widening the blocklist is cheap:
🛡️ Suggested improvement
- const blockedTagPattern = /<\/?(script|iframe|object|embed|link|meta|base|form)[^>]*>/gi;
+ const blockedTagPattern = /<\/?(script|iframe|object|embed|link|meta|base|form|style|svg|math|noscript)[^>]*>/gi;🤖 Prompt for AI Agents
In `@src/ui/file-preview/src/components/html-renderer.ts` around lines 12 - 29,
The regex in sanitizeHtml currently defined by blockedTagPattern misses
dangerous tags like style and svg (and others such as math, template), so update
the blockedTagPattern in the sanitizeHtml function to include "style" and "svg"
(and consider adding "math", "template", "picture", "source" or any other inline
container tags you deem risky) so those tags are stripped; also ensure the
pattern still uses the same flags (/gi) and matches both opening and closing
tags for blockedTagPattern to reliably remove them before proceeding with
attribute filtering.
| function renderSandboxedHtmlFrame(content: string, allowUnsafeScripts: boolean): string { | ||
| const htmlContent = allowUnsafeScripts ? content : sanitizeHtml(content); | ||
| const csp = allowUnsafeScripts | ||
| ? '' | ||
| : `<meta http-equiv="Content-Security-Policy" content="default-src 'none'; img-src https: http: data:; style-src 'unsafe-inline';">`; | ||
| const sandbox = allowUnsafeScripts ? 'allow-scripts allow-forms allow-popups' : ''; | ||
| const palette = resolveThemeFrameStyles(); | ||
| const frameDocument = `<!doctype html><html><head><meta charset="utf-8" />${csp}<style>html,body{margin:0;padding:0;background:${palette.background};color:${palette.text};}body{font-family:${palette.fontFamily};padding:16px;line-height:1.5;}img{max-width:100%;height:auto;}</style></head><body>${htmlContent}</body></html>`; | ||
| return `<iframe class="html-rendered-frame" title="Rendered HTML preview" sandbox="${sandbox}" referrerpolicy="no-referrer" srcdoc="${escapeHtml(frameDocument)}"></iframe>`; | ||
| } |
There was a problem hiding this comment.
CSS values from resolveThemeFrameStyles are interpolated into the frame document without escaping.
On Line 53, palette.background, palette.text, and palette.fontFamily are injected directly into a <style> block. While these originate from getComputedStyle, a malicious host context could set CSS variables containing ; or } to break out of the style rule. The srcdoc is escaped on Line 54, but the breakout happens inside the frame's own CSS before rendering.
Consider escaping or validating these values (e.g., stripping ;, }, <, >).
🤖 Prompt for AI Agents
In `@src/ui/file-preview/src/components/html-renderer.ts` around lines 46 - 55,
The CSS values from resolveThemeFrameStyles are injected raw into the
frameDocument in renderSandboxedHtmlFrame, allowing a crafted theme value
(palette.background, palette.text, palette.fontFamily) to break out of the style
block; validate or escape these three values before interpolation (e.g., run
them through a sanitizer that strips or rejects characters like ; } < > and
quotes, or apply a strict whitelist/regex for valid CSS color/font tokens) and
use the sanitized results when building frameDocument (keep using escapeHtml for
srcdoc); update the code paths that reference palette.background, palette.text,
and palette.fontFamily to use the sanitized/validated variables so no untrusted
characters can close the rule or inject new declarations.
| --accent-hover: var(--color-accent-primary-hover, var(--accent)); | ||
| --accent-soft: var(--color-accent-subtle, var(--panel-subtle)); | ||
| --focus-ring: var(--color-border-focused, var(--accent)); | ||
| --shadow-sm: var(--shadow-sm, none); |
There was a problem hiding this comment.
Self-referential CSS variable — --shadow-sm references itself, which is invalid.
var(--shadow-sm, none) inside the declaration of --shadow-sm creates a cyclic reference. Per the CSS spec, this makes the value "guaranteed-invalid" at computed-value time, so the fallback none is never reached. Unlike the other tokens (e.g., --panel referencing --color-background-primary), this one needs a distinct host-provided variable name or should just default to none.
🐛 Proposed fix
- --shadow-sm: var(--shadow-sm, none);
+ --shadow-sm: var(--shadow-elevation-sm, none);(Use whatever the host's shadow token name actually is, or simply --shadow-sm: none; if no host override is expected.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --shadow-sm: var(--shadow-sm, none); | |
| --shadow-sm: var(--shadow-elevation-sm, none); |
🤖 Prompt for AI Agents
In `@src/ui/styles/base.css` at line 18, The CSS custom property --shadow-sm is
self-referential (uses var(--shadow-sm, none)), creating a cyclic/invalid value;
fix it by replacing the self-reference with either a direct default (set
--shadow-sm: none;) or point it to a distinct host-provided token (e.g., use the
host token name instead of --shadow-sm in the var() call) so the variable
resolves correctly; update the declaration where --shadow-sm is defined to use
the non-self-referential value.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/ui/file-preview/src/app.ts`:
- Around line 227-251: The button title/aria-label is overwritten by
setButtonState('Copied') or 'Copy failed' and never restored; capture the
original label (read from copyButton.getAttribute('title') / aria-label) before
changing it in the copyButton click handler, then after the copy attempt
schedule a short timeout (e.g. 1.5–2s) to call setButtonState(originalLabel) to
restore it; store and clear a pending timer id so repeated clicks cancel the
previous restore timer. Use the existing setButtonState,
copyButton.addEventListener, stripReadStatusLine and fallbackCopy locations to
implement this behavior.
🧹 Nitpick comments (2)
src/ui/file-preview/src/app.ts (2)
447-497: Inconsistent indentation inside themessageevent listener.The
tryblock body (lines 449–494) is indented at the same level as thetrykeyword itself (line 448), making the structure harder to follow.Fix indentation
window.addEventListener('message', (event) => { - try { - if (rpcClient.handleMessageEvent(event)) { - return; - } + try { + if (rpcClient.handleMessageEvent(event)) { + return; + }(apply matching indentation fix throughout the block)
442-444: Magic timeout values (140 ms, 120 ms) lack rationale.These delays likely work around a host rendering or message timing quirk, but without a comment explaining why these specific values were chosen, they'll be a maintenance hazard.
Also applies to: 467-469
| const setButtonState = (label: string): void => { | ||
| copyButton.setAttribute('title', label); | ||
| copyButton.setAttribute('aria-label', label); | ||
| }; | ||
|
|
||
| copyButton.addEventListener('click', async () => { | ||
| const cleanedContent = stripReadStatusLine(payload.content); | ||
| trackUiEvent?.('copy_clicked', { | ||
| file_type: payload.fileType, | ||
| file_extension: getFileExtensionForAnalytics(payload.filePath) | ||
| }); | ||
|
|
||
| try { | ||
| if (navigator.clipboard?.writeText) { | ||
| await navigator.clipboard.writeText(cleanedContent); | ||
| setButtonState('Copied'); | ||
| return; | ||
| } | ||
| } catch { | ||
| // fallback below | ||
| } | ||
|
|
||
| const copied = fallbackCopy(cleanedContent); | ||
| setButtonState(copied ? 'Copied' : 'Copy failed'); | ||
| }); |
There was a problem hiding this comment.
Copy button label never resets after "Copied".
Once the copy succeeds (or fails), setButtonState updates the title/aria-label but never restores it to the original value. The button permanently shows "Copied" (or "Copy failed") for the lifetime of the current render.
Proposed fix — reset after a short delay
copyButton.addEventListener('click', async () => {
const cleanedContent = stripReadStatusLine(payload.content);
trackUiEvent?.('copy_clicked', {
file_type: payload.fileType,
file_extension: getFileExtensionForAnalytics(payload.filePath)
});
+ const originalLabel = copyButton.getAttribute('title') ?? 'Copy source';
+ const flashLabel = (label: string): void => {
+ setButtonState(label);
+ window.setTimeout(() => setButtonState(originalLabel), 2000);
+ };
try {
if (navigator.clipboard?.writeText) {
await navigator.clipboard.writeText(cleanedContent);
- setButtonState('Copied');
+ flashLabel('Copied');
return;
}
} catch {
// fallback below
}
const copied = fallbackCopy(cleanedContent);
- setButtonState(copied ? 'Copied' : 'Copy failed');
+ flashLabel(copied ? 'Copied' : 'Copy failed');
});🤖 Prompt for AI Agents
In `@src/ui/file-preview/src/app.ts` around lines 227 - 251, The button
title/aria-label is overwritten by setButtonState('Copied') or 'Copy failed' and
never restored; capture the original label (read from
copyButton.getAttribute('title') / aria-label) before changing it in the
copyButton click handler, then after the copy attempt schedule a short timeout
(e.g. 1.5–2s) to call setButtonState(originalLabel) to restore it; store and
clear a pending timer id so repeated clicks cancel the previous restore timer.
Use the existing setButtonState, copyButton.addEventListener,
stripReadStatusLine and fallbackCopy locations to implement this behavior.
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
|
CodeAnt AI Incremental review completed. |
Use a PowerShell encoded invocation that passes the selected file path as a single explorer argument, preventing cmd metacharacter injection while preserving /select behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
User description
- resources/list now exposes preview UI resource
- resources/read now serves inlined preview HTML runtime
interaction signals
runtime.cjs + build script hook)
Summary by CodeRabbit
New Features
Style
Tests
Chores
CodeAnt-AI Description
Add inline file preview UI, structured read_file preview payloads, and UI event tracking
What Changed
Impact
✅ Inline file previews for read_file responses✅ Clearer fallback previews and messages for images/unsupported files✅ Recorded preview interactions for analytics💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.