Feat/preview UI redesign#338
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 · |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds partial-range loading and selection-context RPC to file preview: accurate line-counting, start-line-aware per-line code rendering, UI load-before/load-after banners with handlers to fetch/merge ranges, CSS for banners/typography, host initialize payload tweaks, and a new line-count unit test. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as File Preview UI
participant RPC as UI RPC Layer
participant Server as ReadFile Handler
participant FS as Filesystem
User->>UI: Select file / scroll preview
UI->>UI: Render content (show load-before/load-after banners if ranges missing)
UI->>RPC: ui/fetch-partial (request missing before/after ranges)
RPC->>Server: read_file (offset/length, partial flag)
Server->>FS: read file range
FS-->>Server: return bytes/content + range metadata
Server-->>RPC: respond with fetched content + totalLines/isPartial
RPC-->>UI: return fetched lines and totalLines
UI->>UI: merge fetched content, update range/status, re-render
User->>UI: Make text selection
UI->>RPC: ui/update-model-context (selection payload)
RPC-->>Server: deliver selection/context to host
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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:L
Nitpicks 🔍
|
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/ui/file-preview/src/app.ts`:
- Around line 541-567: Remove the development-only debug logging that collects
and prints CSS variables: the block that computes rootStyles, allProps, iterates
document.styleSheets and inlineStyle, and the console.log('[DC Theme Debug] CSS
variables:...'). Either delete this entire block or gate it behind a clearly
named runtime flag (e.g., a DEBUG_THEME_VARS or enableThemeDebug variable) and
only execute when that flag is true so rootStyles/getComputedStyle and the sheet
iteration do not run in production.
In `@src/ui/styles/apps/file-preview.css`:
- Around line 48-55: The stylelint error is due to the `fill: currentColor;`
value using incorrect case; update the `.compact-chevron` rule to use the
lowercase CSS keyword `currentcolor` (modify the `fill` declaration in the
`.compact-chevron` selector) so it complies with the `value-keyword-case` rule;
keep the rest of the declarations (`width`, `height`, `color`, `flex-shrink`,
`transition`) unchanged.
🧹 Nitpick comments (1)
src/ui/styles/apps/file-preview.css (1)
82-90: Consider consolidating duplicate dark-mode declarations.Both the
@media (prefers-color-scheme: dark)block and the[data-theme="dark"]selector apply the identical background rule. This is a valid dual-targeting pattern, but you could reduce duplication with a shared selector if your tooling supports it:♻️ Optional consolidation
-@media (prefers-color-scheme: dark) { - .panel { - background: color-mix(in srgb, var(--panel), white 6%); - } -} - -[data-theme="dark"] .panel { - background: color-mix(in srgb, var(--panel), white 6%); -} +@media (prefers-color-scheme: dark) { + .panel { + background: color-mix(in srgb, var(--panel), white 6%); + } +} +[data-theme="dark"] .panel { + background: color-mix(in srgb, var(--panel), white 6%); +}If future dark-mode overrides grow, consider extracting a utility class or CSS layer to avoid maintaining parallel blocks.
| .compact-chevron { | ||
| width: 14px; | ||
| height: 14px; | ||
| fill: currentColor; | ||
| color: var(--muted); | ||
| flex-shrink: 0; | ||
| transition: transform 200ms ease, color 120ms ease; | ||
| transition: transform 200ms ease, color 150ms ease; | ||
| } |
There was a problem hiding this comment.
Stylelint violation: currentColor → currentcolor.
Static analysis flags value-keyword-case on line 51. CSS keywords should be lowercase per your stylelint config.
🔧 Fix
.compact-chevron {
width: 14px;
height: 14px;
- fill: currentColor;
+ fill: currentcolor;
color: var(--muted);
flex-shrink: 0;
transition: transform 200ms ease, color 150ms ease;
}📝 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.
| .compact-chevron { | |
| width: 14px; | |
| height: 14px; | |
| fill: currentColor; | |
| color: var(--muted); | |
| flex-shrink: 0; | |
| transition: transform 200ms ease, color 120ms ease; | |
| transition: transform 200ms ease, color 150ms ease; | |
| } | |
| .compact-chevron { | |
| width: 14px; | |
| height: 14px; | |
| fill: currentcolor; | |
| color: var(--muted); | |
| flex-shrink: 0; | |
| transition: transform 200ms ease, color 150ms ease; | |
| } |
🧰 Tools
🪛 Stylelint (17.2.0)
[error] 51-51: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
In `@src/ui/styles/apps/file-preview.css` around lines 48 - 55, The stylelint
error is due to the `fill: currentColor;` value using incorrect case; update the
`.compact-chevron` rule to use the lowercase CSS keyword `currentcolor` (modify
the `fill` declaration in the `.compact-chevron` selector) so it complies with
the `value-keyword-case` rule; keep the rest of the declarations (`width`,
`height`, `color`, `flex-shrink`, `transition`) unchanged.
… line count fix - Breadcrumb + buttons on single line with ellipsis overflow - Removed responsive column breakpoint that broke iframe layout - Load more lines banners (before/after) inside scroll container - Each banner loads only missing lines, merges with existing content - Medium-inspired markdown: wider line-height, generous spacing, narrower column - Fixed off-by-one in countLines() for files with trailing newline - Added test/test-line-count.js with 6 test cases - Removed debug CSS variable logger - Tracks load_lines_before/after events in GA4
|
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:L
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const remaining = range.totalLines - newTo; | ||
| const isStillPartial = newFrom > 1 || newTo < range.totalLines; | ||
| const statusLine = isStillPartial | ||
| ? `[Reading ${lineCount} lines from ${newFrom === 1 ? 'start' : `line ${newFrom - 1}`} (total: ${range.totalLines} lines, ${remaining} remaining)]\n` |
There was a problem hiding this comment.
Suggestion: The constructed status header for merged content subtracts 1 from the starting line number, causing parseReadRange to think the range begins one line earlier than it actually does, which leads to incorrect labels and can result in subsequent "load before/after" requests skipping or duplicating lines. [off-by-one]
Severity Level: Major ⚠️
- ❌ Large-file previews show incorrect line ranges in footer.
- ⚠️ Subsequent load-more actions can skip or duplicate lines.
- ⚠️ Confusing UX when reviewing long files via read_file.| ? `[Reading ${lineCount} lines from ${newFrom === 1 ? 'start' : `line ${newFrom - 1}`} (total: ${range.totalLines} lines, ${remaining} remaining)]\n` | |
| ? `[Reading ${lineCount} lines from ${newFrom === 1 ? 'start' : `line ${newFrom}`} (total: ${range.totalLines} lines, ${remaining} remaining)]\n` |
Steps of Reproduction ✅
1. In the MCP host, invoke the `read_file` tool on a large text file so that the server
returns a partial range (tool defined in `src/server.ts:279,1337`, implemented by
`TextFileHandler.read()` in `src/utils/files/text.ts:53-60`).
2. For an offset-based read (e.g., offset > 0),
`TextFileHandler.generateEnhancedStatusMessage()` in `src/utils/files/text.ts:137-167`
builds a status header like `[Reading 100 lines from line 400 (total: 5000 lines, 4900
remaining)]`, which is included at the top of the tool result content.
3. The Desktop Commander File Preview UI (`bootstrapApp()` in
`src/ui/file-preview/src/app.ts:591-741`) receives this tool result via the
`ui/notifications/tool-result` message handler at `app.ts:682-733`, then calls
`renderApp()` at `app.ts:456-589`, which parses the header with `parseReadRange()` at
`app.ts:212-225` and renders partial content with "Load lines …" banners
(`loadBeforeBanner` / `loadAfterBanner` at `app.ts:503-511`).
4. Click the bottom "↓ Load lines …" banner (`#load-after` button created at
`app.ts:509-510`), which triggers `attachLoadAllHandler()` in `app.ts:361-432`. After
merging the newly fetched lines, it rebuilds the status header using the existing code at
`app.ts:405-413`, where the template uses ``line ${newFrom - 1}`` for non-`start` ranges.
On the next render, `parseReadRange()` interprets this header as starting at `newFrom - 1`
instead of the actual first line `newFrom`, causing the compact label and footer
(`footerLabel` and `compactLabel` at `app.ts:487-492`) to show an off-by-one line range
and making subsequent "load before/after" calls calculate offsets from this incorrect
range.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/app.ts
**Line:** 412:412
**Comment:**
*Off By One: The constructed status header for merged content subtracts 1 from the starting line number, causing `parseReadRange` to think the range begins one line earlier than it actually does, which leads to incorrect labels and can result in subsequent "load before/after" requests skipping or duplicating lines.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/file-preview/src/app.ts (1)
212-226:⚠️ Potential issue | 🔴 CriticalOff-by-one:
fromLineis wrong when the status line says "from line N" (0-based offset).
generateEnhancedStatusMessageintext.tswrites the raw 0-basedoffsetinto the status string (e.g.,"from line 50"when offset = 50). ButparseReadRangetreats that number as a 1-based line number — only the"start"branch gets this right (mapping to 1).Consequence: every downstream use of
fromLine/toLineis off by one for non-zero offsets:
What Current (offset 50, count 50, total 200) Correct fromLine50 51 toLine99 100 "Load before" length49 → misses line 50 50 "Load after" offset99 → overlaps last line 100 UI label "View lines 50–99" "View lines 51–100" 🐛 Proposed fix
function parseReadRange(content: string): ReadRange | undefined { const match = content.match(/^\[Reading (\d+) lines from (?:line )?(\d+|start) \(total: (\d+) lines/); if (!match) return undefined; const count = parseInt(match[1], 10); - const from = match[2] === 'start' ? 1 : parseInt(match[2], 10); + const from = match[2] === 'start' ? 1 : parseInt(match[2], 10) + 1; const total = parseInt(match[3], 10); return { fromLine: from, toLine: from + count - 1, totalLines: total, isPartial: count < total }; }
🧹 Nitpick comments (6)
src/ui/file-preview/src/app.ts (3)
198-203:countContentLinesduplicatesTextFileHandler.countLinesfromsrc/utils/files/text.ts.Both functions implement identical logic. Since this runs client-side and can't import server-side utilities, the duplication is acceptable. A brief comment noting the canonical source would help keep them in sync.
501-501: Unused variableloadAllButton.
loadAllButtonis assigned to''but never referenced in the template below. Remove it.🧹 Fix
- const loadAllButton = ''; - // Content-area banners for missing lines
361-432:attachLoadAllHandler— event listeners accumulate on each re-render.
renderApprebuilds the entire DOM viainnerHTML, so old listeners on old DOM nodes are garbage-collected. The handler is clean in that regard. However, verify thatrpcCallToolis always assigned before users can click the banners — it's set inbootstrapApp, so this should be safe.One functional note: on line 391, no
lengthargument is passed for the "after" direction. This means the handler defaults toconfig.fileReadLineLimit(typically 1000). For most files this is fine, but for very large files the banner could pull far more lines than the remaining gap. Consider passing the exact remaining count for tighter control:const readArgs = direction === 'before' ? { path: payload.filePath, offset: 0, length: range.fromLine - 1 } - : { path: payload.filePath, offset: range.toLine }; + : { path: payload.filePath, offset: range.toLine, length: range.totalLines - range.toLine };(After fixing the off-by-one,
range.toLineandrange.fromLinewill be 1-based, and the offset/length values should be adjusted accordingly.)test/test-line-count.js (3)
1-7: Tests import fromdist/— will fail if not built first.Importing from
../dist/handlers/filesystem-handlers.jscouples test execution to a prior build step. Consider adding a note (or aprebuildscript inpackage.json) to clarify this dependency, so CI and new contributors don't hit mysterious import failures.
16-18: Test artifacts are not cleaned up.
TEST_DIR(test/test_output/) is created but never removed. Over repeated runs, stale files accumulate. Add a teardown step:🧹 Suggested teardown
+async function teardown() { + await fs.rm(TEST_DIR, { recursive: true, force: true }); +} + setup().then(testLineCount).catch(err => { console.error('Test error:', err); process.exit(1); -}); +}).finally(() => teardown().catch(() => {}));
26-31:extractTotalLinesreturnsnullwhen the status line is absent — tests silently passnullintoassert.strictEqual.If
handleReadFileever stops emitting the status header (e.g.,includeStatusMessagechanges tofalse),extractTotalLineswould returnnulland the assertionassert.strictEqual(null, 100)would fail with a confusing message. Consider adding an explicit guard:if (total === null) throw new Error('Could not extract total line count from result');This way a format change surfaces immediately rather than as a mysterious equality failure.
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 388-420: The code assumes the second rpcCallTool('read_file', ...)
returned all remaining lines and sets newTo = range.totalLines; instead, inspect
the raw newText for its own "[Reading ...]" status header before calling
stripReadStatusLine so you can parse the actual fetched range (e.g. extract
from/to/total/remaining using a regex on newText's header) and compute
newFrom/newTo from that parsed header; if no header is present, fall back to the
existing logic (use range.fromLine/range.toLine/range.totalLines). Update the
variables used to build statusLine (newFrom, newTo, lineCount, remaining,
isStillPartial) so renderApp receives an accurate mergedPayload that reflects
the true fetched range; keep using rpcCallTool, stripReadStatusLine,
mergedPayload and renderApp as in the diff.
- Around line 618-619: The check for showing the "load after" banner is
off-by-one: update the hasMissingAfter condition (currently using
(range.totalLines - range.toLine) > 1) to treat any positive missing count as
missing (use > 0 or >= 1) so that when exactly one line remains it still shows
the banner; adjust the expression where hasMissingAfter is computed alongside
hasMissingBefore and keep the existing guards (range?.isPartial) and property
names (range.fromLine, range.toLine, range.totalLines) intact.
- Around line 450-544: attachTextSelectionHandler currently adds a
document.addEventListener('selectionchange', ...) on every call and never
removes it, leaking listeners and stale closures; update it to register the
listener with an AbortController (or keep a module-scoped reference to the
current controller/handler) so any previous controller is aborted (or previous
listener removed) before adding a new one, and ensure the controller is
stored/cleared so future calls to attachTextSelectionHandler can abort the old
listener; touch the functions and symbols selectionchange listener registration
inside attachTextSelectionHandler, and ensure
hideHint/lastSelectedText/rpcUpdateContext state remains correct when cleaning
up.
In `@src/ui/file-preview/src/components/code-viewer.ts`:
- Around line 66-78: The highlighted HTML from highlightSource contains
multi-line <span> tags that get broken by a naive split('\n'), so implement a
helper (e.g., splitWithSpanRepair) that walks highlighted HTML, splits on
newline while closing any currently open <span> tags at the end of each line and
reopening them at the start of the next line, then replace
highlighted.split('\n') with splitWithSpanRepair(highlighted) before the
trailing-empty-line pop and the lineHtml mapping; reference the helper name
splitWithSpanRepair and the variables highlighted, lines, and lineHtml so you
place the repair logic exactly where the split happens and preserve the existing
trailing-empty-line removal and line number computation (startLine + i).
🧹 Nitpick comments (2)
src/ui/file-preview/src/app.ts (1)
615-615: Dead code:loadAllButtonis declared but never referenced.This empty-string variable appears to be a leftover from removing a previous "Load All" button. It can be safely deleted.
- const loadAllButton = '';src/ui/styles/apps/file-preview.css (1)
295-312:cursor: pointeron.line-numsuggests clickability, but no click handler is wired.The line-number gutter has
cursor: pointerand a hover effect, implying interactivity (e.g., line selection or linking). However, no corresponding click handler exists in the JS code. This may confuse users who expect clicking a line number to trigger an action.Either wire up a click handler (e.g., to select/highlight the line) or change to
cursor: defaultuntil the interaction is implemented.
| try { | ||
| // Load only the missing portion | ||
| const readArgs = direction === 'before' | ||
| ? { path: payload.filePath, offset: 0, length: range.fromLine - 1 } | ||
| : { path: payload.filePath, offset: range.toLine }; | ||
|
|
||
| const result = await rpcCallTool?.('read_file', readArgs); | ||
| const resultObj = result as { content?: Array<{ text?: string }> } | undefined; | ||
| const newText = resultObj?.content?.[0]?.text; | ||
|
|
||
| if (newText && typeof newText === 'string') { | ||
| const cleanNew = stripReadStatusLine(newText); | ||
|
|
||
| // Merge: prepend or append the new lines | ||
| const merged = direction === 'before' | ||
| ? cleanNew + (cleanNew.endsWith('\n') ? '' : '\n') + currentContent | ||
| : currentContent + (currentContent.endsWith('\n') ? '' : '\n') + cleanNew; | ||
|
|
||
| // Build updated status line reflecting the new range | ||
| const newFrom = direction === 'before' ? 1 : range.fromLine; | ||
| const newTo = direction === 'after' ? range.totalLines : range.toLine; | ||
| const lineCount = newTo - newFrom + 1; | ||
| const remaining = range.totalLines - newTo; | ||
| const isStillPartial = newFrom > 1 || newTo < range.totalLines; | ||
| const statusLine = isStillPartial | ||
| ? `[Reading ${lineCount} lines from ${newFrom === 1 ? 'start' : `line ${newFrom}`} (total: ${range.totalLines} lines, ${remaining} remaining)]\n` | ||
| : ''; | ||
|
|
||
| const mergedPayload: PreviewStructuredContent = { | ||
| ...payload, | ||
| content: statusLine + merged | ||
| }; | ||
| renderApp(container, mergedPayload, htmlMode, isExpanded); |
There was a problem hiding this comment.
"Load after" assumes the second read_file fetches all remaining lines.
After the RPC call returns, newTo is set to range.totalLines (Line 408) and the status line is rebuilt to reflect the full range. However, read_file itself may paginate large results, returning only a portion of the remaining lines. If that happens, the reconstructed status header will claim all lines are present while the content is still truncated.
Consider parsing the returned newText for its own [Reading …] header to determine the actual range that was fetched, rather than assuming it covers everything from range.toLine to range.totalLines.
🤖 Prompt for AI Agents
In `@src/ui/file-preview/src/app.ts` around lines 388 - 420, The code assumes the
second rpcCallTool('read_file', ...) returned all remaining lines and sets newTo
= range.totalLines; instead, inspect the raw newText for its own "[Reading ...]"
status header before calling stripReadStatusLine so you can parse the actual
fetched range (e.g. extract from/to/total/remaining using a regex on newText's
header) and compute newFrom/newTo from that parsed header; if no header is
present, fall back to the existing logic (use
range.fromLine/range.toLine/range.totalLines). Update the variables used to
build statusLine (newFrom, newTo, lineCount, remaining, isStillPartial) so
renderApp receives an accurate mergedPayload that reflects the true fetched
range; keep using rpcCallTool, stripReadStatusLine, mergedPayload and renderApp
as in the diff.
| const hasMissingBefore = range?.isPartial && range.fromLine > 1; | ||
| const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines && (range.totalLines - range.toLine) > 1; |
There was a problem hiding this comment.
Off-by-one: "Load after" banner won't appear when exactly 1 line is missing.
(range.totalLines - range.toLine) > 1 requires more than 1 missing line. If a file has 100 total lines and lines 1–99 are loaded, 100 - 99 = 1, which fails the > 1 check, so no banner is shown and the user cannot load the final line.
Should this be > 0 (or >= 1)?
- const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines && (range.totalLines - range.toLine) > 1;
+ const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines;📝 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.
| const hasMissingBefore = range?.isPartial && range.fromLine > 1; | |
| const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines && (range.totalLines - range.toLine) > 1; | |
| const hasMissingBefore = range?.isPartial && range.fromLine > 1; | |
| const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines; |
🤖 Prompt for AI Agents
In `@src/ui/file-preview/src/app.ts` around lines 618 - 619, The check for showing
the "load after" banner is off-by-one: update the hasMissingAfter condition
(currently using (range.totalLines - range.toLine) > 1) to treat any positive
missing count as missing (use > 0 or >= 1) so that when exactly one line remains
it still shows the banner; adjust the expression where hasMissingAfter is
computed alongside hasMissingBefore and keep the existing guards
(range?.isPartial) and property names (range.fromLine, range.toLine,
range.totalLines) intact.
| const highlighted = highlightSource(code, normalizedLanguage); | ||
| return `<pre class="code-viewer"><code class="hljs language-${normalizedLanguage}">${highlighted}</code></pre>`; | ||
|
|
||
| // Wrap each line with line number gutter | ||
| const lines = highlighted.split('\n'); | ||
| // Remove trailing empty line from trailing newline | ||
| if (lines.length > 0 && lines[lines.length - 1] === '') { | ||
| lines.pop(); | ||
| } | ||
|
|
||
| const lineHtml = lines.map((line, i) => { | ||
| const lineNum = startLine + i; | ||
| return `<tr class="code-line" data-line="${lineNum}"><td class="line-num" data-line="${lineNum}">${lineNum}</td><td class="line-content">${line || ' '}</td></tr>`; | ||
| }).join('\n'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the highlightSource function
rg -n "highlightSource" src/ui/file-preview/src/components/code-viewer.ts -A 2 -B 2Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 645
🏁 Script executed:
# Also search for the function definition
ast-grep --pattern $'function highlightSource($_, $_) {
$$$
}'Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 58
🏁 Script executed:
# Look for where highlightSource is defined or imported
rg -n "highlightSource" src/ui/file-preview/src/components/ -B 3 -A 10Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 6270
🏁 Script executed:
cat -n src/ui/file-preview/src/components/highlighting.ts | head -70 | tail -35Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1511
🏁 Script executed:
# Check if highlight.js can produce spans spanning newlines by looking at test files
find src/ui/file-preview -name "*.test.*" -o -name "*.spec.*" | head -10Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 58
🏁 Script executed:
# Look for any tests related to code-viewer or highlighting
rg -l "code-viewer\|highlightSource" src/ui/file-preview --include="*.test.*" --include="*.spec.*"Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 517
🏁 Script executed:
# Check the package.json to see what version of highlight.js is used
rg -A 5 "highlight.js" package.jsonRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 243
🏁 Script executed:
# Let's also check how the highlighted output is actually being used -
# the key is whether the split fragments are ever directly compared or validated
rg -n "split.*\\\\n" src/ui/file-preview/src/components/ -B 3 -A 5Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 855
🏁 Script executed:
# Let's understand highlight.js behavior better by checking what the actual output looks like
# Search for any test files or examples that might show the output
find . -type f -name "*.ts" -o -name "*.js" | xargs grep -l "highlightSource\|hljs.highlight" | head -5Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 238
🏁 Script executed:
# Check if there are any example files or documentation about expected output
cat -n src/ui/file-preview/src/components/highlighting.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 2785
🏁 Script executed:
# Look for any existing issues or comments about line splitting
rg -n "split.*line\|newline\|line break" src/ui/file-preview/src/components/code-viewer.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 58
🌐 Web query:
highlight.js v11 output newlines in span tags multi-line tokens
💡 Result:
In Highlight.js v11 it’s normal for a single token (eg a multiline comment/string) to be emitted as one <span ...> whose text contains literal \n characters—so you’ll see “newlines inside span tags” when the grammar matches across lines. Highlight.js doesn’t have a built-in option to “split multiline tokens into per-line spans”; you have to post-process the generated HTML if you need that (most commonly for line-wrapping/line-number table layouts). [1]
Practical fix: post-process after highlighting
Use the plugin hook after:highlightElement to rewrite the already-generated HTML. [1]
Example (splitting multiline comment spans across lines, adapted from a common line-numbering workaround):
hljs.addPlugin({
'after:highlightElement': ({ el }) => {
// Split multiline comment spans so each line starts a new span
el.innerHTML = el.innerHTML.replace(
/<span class="hljs-comment">([\s\S]*?)<\/span>/g,
(m) => m.replace(/\r?\n/g, '\n<span class="hljs-comment">')
);
}
});This is the same basic approach used in line-number/table-row implementations: detect the <span class="hljs-comment">...</span> block and inject a new opening <span> after each newline so subsequent lines remain styled. [2]
If you only care about preserving line breaks (not per-line spans)
Make sure you’re using <pre><code> or CSS like white-space: pre; so line breaks render correctly; Highlight.js warns that non-pre containers need CSS to preserve linebreaks. [3]
Sources: Plugin hook and timing (after:highlightElement) [1], multiline span splitting workaround example [2], linebreak-preservation guidance [3].
Splitting highlighted HTML on \n breaks multi-line syntax tokens from highlight.js.
highlight.js v11 emits <span> tags containing literal newlines for multi-line constructs (e.g., multi-line comments, strings, block quotes). Splitting the highlighted output on \n produces unclosed/mismatched tags in table cells, causing lost syntax coloring and malformed HTML.
Fix by post-processing the highlighted HTML to close open <span> tags before each newline and reopen them on the next line:
Span-repair helper
function splitWithSpanRepair(highlighted: string): string[] {
const raw = highlighted.split('\n');
const result: string[] = [];
let openTags: string[] = [];
for (const line of raw) {
let patched = openTags.join('') + line;
// Track open spans and remove them from the line
const opens = (line.match(/<span[^>]*>/g) || []);
const closes = (line.match(/<\/span>/g) || []).length;
// Close any remaining open spans at line end
openTags = opens.slice(closes);
patched += '</span>'.repeat(Math.max(0, opens.length - closes));
result.push(patched);
}
return result;
}Then replace highlighted.split('\n') with splitWithSpanRepair(highlighted).
🤖 Prompt for AI Agents
In `@src/ui/file-preview/src/components/code-viewer.ts` around lines 66 - 78, The
highlighted HTML from highlightSource contains multi-line <span> tags that get
broken by a naive split('\n'), so implement a helper (e.g., splitWithSpanRepair)
that walks highlighted HTML, splits on newline while closing any currently open
<span> tags at the end of each line and reopening them at the start of the next
line, then replace highlighted.split('\n') with splitWithSpanRepair(highlighted)
before the trailing-empty-line pop and the lineHtml mapping; reference the
helper name splitWithSpanRepair and the variables highlighted, lines, and
lineHtml so you place the repair logic exactly where the split happens and
preserve the existing trailing-empty-line removal and line number computation
(startLine + i).
|
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:XL
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/ui/styles/apps/file-preview.css (1)
82-90: Duplicate dark-mode rule — consider consolidating.The
@media (prefers-color-scheme: dark)block (lines 82–86) and the[data-theme="dark"]rule (lines 88–90) apply the identical declaration. This is fine for supporting both auto-detect and manual theme override, but the duplicatedcolor-mix(...)expression could be maintained via a single shared custom property to avoid drift.♻️ Example consolidation
+/* shared dark panel tint */ +@media (prefers-color-scheme: dark) { + :root { --panel-dark-bg: color-mix(in srgb, var(--panel), white 6%); } +} +[data-theme="dark"] { + --panel-dark-bg: color-mix(in srgb, var(--panel), white 6%); +} + .panel { /* ... */ - background: var(--panel); + background: var(--panel-dark-bg, var(--panel)); } - -@media (prefers-color-scheme: dark) { - .panel { - background: color-mix(in srgb, var(--panel), white 6%); - } -} - -[data-theme="dark"] .panel { - background: color-mix(in srgb, var(--panel), white 6%); -}src/ui/file-preview/src/app.ts (1)
467-484: Non-null assertions oncontentWrapperinside nested closures.
contentWrapper!is asserted non-null at lines 471, 475, 479, 493, and 535, but the guard at line 454 only checked it at function entry time. If the DOM is modified between invocations (e.g.,renderApprebuilds the DOM while aselectionchangefires from a queued microtask),contentWrappercould reference a detached element. Since the reference is captured in the closure and won't become null itself, the!assertions are technically safe, but operations likegetBoundingClientRect()on a detached element will return zeroed rects, potentially positioning the hint at(0, 0).This is a minor edge-case concern — the abort controller will typically prevent stale closures from firing, so this is fine in practice.
| // Only act on selections within our content area | ||
| const anchorInContent = contentWrapper!.contains(selection.anchorNode); | ||
| const focusInContent = contentWrapper!.contains(selection.focusNode); | ||
| if (!anchorInContent && !focusInContent) return; |
There was a problem hiding this comment.
Suggestion: The selection hint and model context are never cleared when the user drags a new selection entirely outside the preview area (non-collapsed selection), so the floating "AI can see your selection" tooltip remains stuck near the old selection and the host continues to hold stale context, which is confusing and incorrect behavior. [logic error]
Severity Level: Major ⚠️
- ⚠️ Tooltip persists even after selecting outside preview content.
- ⚠️ Host retains stale selection in ui/update-model-context.| if (!anchorInContent && !focusInContent) return; | |
| if (!anchorInContent && !focusInContent) { | |
| if (lastSelectedText) { | |
| lastSelectedText = ''; | |
| rpcUpdateContext?.(''); | |
| hideHint(); | |
| } | |
| return; | |
| } |
Steps of Reproduction ✅
1. Open a file preview so `renderApp()` runs at `src/ui/file-preview/src/app.ts:579-713`
and renders the panel; this calls `attachTextSelectionHandler(payload)` at line 668, which
registers a `selectionchange` listener on `document` at
`src/ui/file-preview/src/app.ts:520-553`.
2. Inside the preview's `.panel-content-wrapper` (rendered in `renderApp()` at lines
636-657), select some code or markdown text; in the `selectionchange` handler, `selection`
is non-collapsed, `text` is non-empty, `anchorInContent`/`focusInContent` are true (lines
534-537), so `lastSelectedText` is set (line 539), `rpcUpdateContext` is called with a
context string (line 545) and the floating "AI can see your selection" hint is shown via
`showHint(selection)` (lines 486-497).
3. Without clicking to clear the selection, drag a new selection entirely outside
`.panel-content-wrapper` but still within the iframe document (e.g. select text in a
header or other UI above the panel); on the next `selectionchange`, `selection` is still
non-collapsed and `text` is non-empty (lines 521-532), but both `anchorInContent` and
`focusInContent` are now false for the preview content wrapper (line 535-536).
4. Because the code at lines 534-537 simply `return`s when `!anchorInContent &&
!focusInContent`, the handler exits without clearing `lastSelectedText`, without calling
`rpcUpdateContext?.('')`, and without invoking `hideHint()`. The stale tooltip remains
near the old selection inside the preview, and the host continues to hold the previous
selection text in `ui/update-model-context` until the selection is collapsed or the widget
is re-rendered.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/app.ts
**Line:** 537:537
**Comment:**
*Logic Error: The selection hint and model context are never cleared when the user drags a new selection entirely outside the preview area (non-collapsed selection), so the floating "AI can see your selection" tooltip remains stuck near the old selection and the host continues to hold stale context, which is confusing and incorrect behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
|
||
| // Content-area banners for missing lines | ||
| const hasMissingBefore = range?.isPartial && range.fromLine > 1; | ||
| const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines && (range.totalLines - range.toLine) > 1; |
There was a problem hiding this comment.
Suggestion: The hasMissingAfter condition now requires at least two lines to be missing at the end of the file before showing the "Load lines" banner; when exactly one line is missing (e.g., total 100 lines, previewing 1–99), no "load after" button is rendered, so the user can never load or see the final line of the file via the UI. [logic error]
Severity Level: Major ⚠️
- ⚠️ File preview cannot load final line in some partial views.
- ⚠️ Users may miss important trailing config or code lines.| const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines && (range.totalLines - range.toLine) > 1; | |
| const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines; |
Steps of Reproduction ✅
1. Open Desktop Commander and trigger any tool that returns a `PreviewStructuredContent`
payload for the file preview app (handled in `bootstrapApp()` at
`src/ui/file-preview/src/app.ts:715-777`), so that `renderApp()` is called at
`src/ui/file-preview/src/app.ts:579` with a `payload` representing a file.
2. Ensure the tool's content string represents a partial read of a file where exactly one
line is missing at the end, e.g. the text begins with `[Reading 99 lines from start
(total: 100 lines, 1 remaining)]` followed by lines 1–99 of a 100‑line file. This matches
the `parseReadRange()` pattern at `src/ui/file-preview/src/app.ts:213-226` and yields `{
fromLine: 1, toLine: 99, totalLines: 100, isPartial: true }`.
3. In `renderApp()` (`src/ui/file-preview/src/app.ts:595-613`), `range.isPartial` is true
and `range.toLine < range.totalLines` (99 < 100) but `(range.totalLines - range.toLine)`
equals `1`, so the `hasMissingAfter` condition at line 628 (`const hasMissingAfter =
range?.isPartial && range.toLine < range.totalLines && (range.totalLines - range.toLine) >
1;`) evaluates to `false`.
4. Because `hasMissingAfter` is false, `loadAfterBanner` is rendered as an empty string at
lines 632–633 and no "↓ Load lines 100–100" button appears in the DOM, so
`attachLoadAllHandler()` at `src/ui/file-preview/src/app.ts:362-433` finds no
`#load-after` element and cannot attach any handler to load the final line; the user
cannot load or view line 100 via the preview UI.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/file-preview/src/app.ts
**Line:** 628:628
**Comment:**
*Logic Error: The `hasMissingAfter` condition now requires at least two lines to be missing at the end of the file before showing the "Load lines" banner; when exactly one line is missing (e.g., total 100 lines, previewing 1–99), no "load after" button is rendered, so the user can never load or see the final line of the file via the UI.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI Incremental review completed. |
1 participant
User description
Summary by CodeRabbit
New Features
Improvements
Tests
CodeAnt-AI Description
Revamp file preview: accurate line counts, load-missing-lines, selection sharing, and UI polish
What Changed
Impact
✅ Ability to load missing lines without reloading entire file✅ Accurate total line counts in file previews✅ Selection shared with host AI context💡 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.