Skip to content

Feat/preview UI redesign #338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wonderwhy-er merged 7 commits into from
Feb 16, 2026
Merged

Feat/preview UI redesign #338

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3cafa97
Better UI for read preview
wonderwhy-er Feb 14, 2026
5c28bf5
markdown style
wonderwhy-er Feb 14, 2026
a737552
UI: breadcrumb one-line layout, load more lines, markdown typography,…
wonderwhy-er Feb 14, 2026
55af1de
improvements to widget
wonderwhy-er Feb 14, 2026
be64eea
Fix last line
wonderwhy-er Feb 14, 2026
3f896ff
Few more fixes
wonderwhy-er Feb 16, 2026
d75d40e
hint fix
wonderwhy-er Feb 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 100 additions & 2 deletions src/ui/file-preview/src/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,13 @@ function stripReadStatusLine(content: string): string {
return content.replace(/^\[Reading [^\]]+\]\r?\n?/, '');
}

function countContentLines(content: string): number {
const cleaned = stripReadStatusLine(content);
if (cleaned === '') return 0;
const lines = cleaned.split('\n');
return lines[lines.length - 1] === '' ? lines.length - 1 : lines.length;
}

interface ReadRange {
fromLine: number;
toLine: number;
Expand Down Expand Up @@ -351,6 +358,79 @@ function attachOpenInFolderHandler(payload: PreviewStructuredContent): void {
});
}

function attachLoadAllHandler(
container: HTMLElement,
payload: PreviewStructuredContent,
htmlMode: HtmlPreviewMode
): void {
const beforeBtn = document.getElementById('load-before') as HTMLButtonElement | null;
const afterBtn = document.getElementById('load-after') as HTMLButtonElement | null;
if (!beforeBtn && !afterBtn) {
return;
}

const range = parseReadRange(payload.content);
if (!range?.isPartial) return;

const currentContent = stripReadStatusLine(payload.content);

const loadLines = async (btn: HTMLButtonElement, direction: 'before' | 'after'): Promise<void> => {
const originalText = btn.textContent;
btn.textContent = 'Loading…';
btn.disabled = true;

trackUiEvent?.(direction === 'before' ? 'load_lines_before' : 'load_lines_after', {
file_type: payload.fileType,
file_extension: getFileExtensionForAnalytics(payload.filePath)
});

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 - 1}`} (total: ${range.totalLines} lines, ${remaining} remaining)]\n`

@codeant-ai codeant-ai Bot Feb 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Suggested change
? `[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.
👍 | 👎

: '';

const mergedPayload: PreviewStructuredContent = {
...payload,
content: statusLine + merged
};
renderApp(container, mergedPayload, htmlMode, isExpanded);
Comment on lines +388 to +420

@coderabbitai coderabbitai Bot Feb 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

"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.

} else {
btn.textContent = 'Failed to load';
setTimeout(() => { btn.textContent = originalText; btn.disabled = false; }, 2000);
}
} catch {
btn.textContent = 'Failed to load';
setTimeout(() => { btn.textContent = originalText; btn.disabled = false; }, 2000);
}
};

beforeBtn?.addEventListener('click', () => void loadLines(beforeBtn, 'before'));
afterBtn?.addEventListener('click', () => void loadLines(afterBtn, 'after'));
}

function renderStatusState(container: HTMLElement, message: string): void {
container.innerHTML = `
<main class="shell">
Expand Down Expand Up @@ -398,7 +478,7 @@ export function renderApp(

const breadcrumb = buildBreadcrumb(payload.filePath);
const range = parseReadRange(payload.content);
const lineCount = range ? range.toLine - range.fromLine + 1 : payload.content.split('\n').length;
const lineCount = range ? range.toLine - range.fromLine + 1 : countContentLines(payload.content);
const fileTypeLabel = payload.fileType === 'markdown' ? 'MARKDOWN'
: payload.fileType === 'html' ? 'HTML'
: fileExtension !== 'none' ? fileExtension.toUpperCase()
Expand All @@ -418,6 +498,18 @@ export function renderApp(
const copyIcon = `<svg width="14" height="14" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><rect x="9" y="9" width="13" height="13" rx="2" ry="2"/><path d="M5 15H4a2 2 0 0 1-2-2V4a2 2 0 0 1 2-2h9a2 2 0 0 1 2 2v1"/></svg>`;
const folderIcon = `<svg width="14" height="14" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><path d="M22 19a2 2 0 0 1-2 2H4a2 2 0 0 1-2-2V5a2 2 0 0 1 2-2h5l2 3h9a2 2 0 0 1 2 2z"/></svg>`;

const loadAllButton = '';

// Content-area banners for missing lines
const hasMissingBefore = range?.isPartial && range.fromLine > 1;
const hasMissingAfter = range?.isPartial && range.toLine < range.totalLines;
const loadBeforeBanner = hasMissingBefore
? `<button class="load-lines-banner" id="load-before">↑ Load lines 1–${range!.fromLine - 1}</button>`
: '';
const loadAfterBanner = hasMissingAfter
? `<button class="load-lines-banner" id="load-after">↓ Load lines ${range!.toLine + 1}–${range!.totalLines}</button>`
: '';

container.innerHTML = `
<main id="tool-shell" class="shell tool-shell ${isExpanded ? 'expanded' : 'collapsed'}">
<div class="compact-row compact-row--ready" id="compact-toggle" role="button" tabindex="0" aria-expanded="${isExpanded}">
Expand All @@ -435,7 +527,11 @@ export function renderApp(
</span>
</div>
${notice}
${body.html}
<div class="panel-content-wrapper">
${loadBeforeBanner}
${body.html}
${loadAfterBanner}
</div>
<div class="panel-footer">
<span>${footerLabel}</span>
</div>
Expand All @@ -446,6 +542,7 @@ export function renderApp(
attachCopyHandler(payload);
attachHtmlToggleHandler(container, payload, htmlMode);
attachOpenInFolderHandler(payload);
attachLoadAllHandler(container, payload, htmlMode);

// Compact row click toggles expand/collapse
const compactRow = document.getElementById('compact-toggle');
Expand Down Expand Up @@ -538,6 +635,7 @@ export function bootstrapApp(): void {

onRender?.();
themeAdapter.applyFromData((window as any).__MCP_HOST_CONTEXT__);

const renderAndSync = (payload?: PreviewStructuredContent): void => {
if (payload) {
widgetState.write(payload); // Persist for refresh recovery (cross-host)
Expand Down
Loading