Skip to content

fix(file-preview): attribute events to source tools #479

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 2 commits into from
Jun 3, 2026
Merged

fix(file-preview): attribute events to source tools #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c8f612
fix(file-preview): attribute events to source tools
edgarsskore May 24, 2026
644ab1c
fix(file-preview): harden editor metadata lookup
edgarsskore May 24, 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
9 changes: 9 additions & 0 deletions src/handlers/filesystem-handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
moveFile,
getFileInfo,
writePdf,
getDefaultEditorMetadata,
type FileResult,
type MultiFileResult
} from '../tools/filesystem.js';
Expand Down Expand Up @@ -137,6 +138,8 @@ export async function handleReadFile(args: unknown): Promise<ServerResult> {
fileName: path.basename(resolvedFilePath),
filePath: resolvedFilePath,
fileType: 'unsupported' as const,
sourceTool: 'read_file',
...await getDefaultEditorMetadata(resolvedFilePath),
Comment on lines +141 to +142

@coderabbitai coderabbitai Bot May 24, 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 | 🔴 Critical | ⚡ Quick win

Make editor metadata enrichment best-effort so it never flip-flops successful operations into errors.

getDefaultEditorMetadata() is optional enrichment, but it currently gates the full response. If it throws after writeFile succeeds, callers can receive an error and retry non-idempotent writes (notably append), causing duplicate content.

Proposed fix 
+const safeDefaultEditorMetadata = async (filePath: string) => {
+    try {
+        return await getDefaultEditorMetadata(filePath);
+    } catch {
+        return {};
+    }
+};
...
-                    ...await getDefaultEditorMetadata(resolvedFilePath),
+                    ...await safeDefaultEditorMetadata(resolvedFilePath),
...
-                    ...await getDefaultEditorMetadata(resolvedFilePath),
+                    ...await safeDefaultEditorMetadata(resolvedFilePath),
...
-                    ...await getDefaultEditorMetadata(resolvedFilePath),
+                    ...await safeDefaultEditorMetadata(resolvedFilePath),
...
-                ...await getDefaultEditorMetadata(resolvedWritePath),
+                ...await safeDefaultEditorMetadata(resolvedWritePath),

Also applies to: 170-171, 191-192, 313-314

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/handlers/filesystem-handlers.ts` around lines 141 - 142, The call to
getDefaultEditorMetadata currently can throw and turn successful
writeFile/append operations into failures; change enrichment to be best-effort
by wrapping each await getDefaultEditorMetadata(resolvedFilePath) in a try/catch
inside the filesystem handler functions (e.g., where sourceTool: 'read_file' is
assembled), so that on error you log/debug the metadata error and continue
returning the successful operation result without throwing; merge metadata only
if the call succeeds. Apply this pattern to all occurrences (the blocks around
the existing getDefaultEditorMetadata calls).

content: pdfContent
.filter((item): item is { type: "text"; text: string } => item.type === "text")
.map((item) => item.text)
Expand Down Expand Up @@ -164,6 +167,8 @@ export async function handleReadFile(args: unknown): Promise<ServerResult> {
fileName: path.basename(resolvedFilePath),
filePath: resolvedFilePath,
fileType: 'image',
sourceTool: 'read_file',
...await getDefaultEditorMetadata(resolvedFilePath),
content: imageData,
imageData,
mimeType: fileResult.mimeType
Expand All @@ -183,6 +188,8 @@ export async function handleReadFile(args: unknown): Promise<ServerResult> {
fileName: path.basename(resolvedFilePath),
filePath: resolvedFilePath,
fileType,
sourceTool: 'read_file',
...await getDefaultEditorMetadata(resolvedFilePath),
content: textContent,
},
};
Expand Down Expand Up @@ -303,6 +310,8 @@ export async function handleWriteFile(args: unknown): Promise<ServerResult> {
fileName: path.basename(resolvedWritePath),
filePath: resolvedWritePath,
fileType: resolvePreviewFileType(resolvedWritePath),
sourceTool: 'write_file',
...await getDefaultEditorMetadata(resolvedWritePath),
},
};
} catch (error) {
Expand Down
10 changes: 8 additions & 2 deletions src/tools/edit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* 3. Unify the editRange() signature to handle both text search/replace and structured edits
*/

import { readFile, writeFile, readFileInternal, validatePath } from './filesystem.js';
import { getDefaultEditorMetadata, readFile, writeFile, readFileInternal, validatePath } from './filesystem.js';
import fs from 'fs/promises';
import { ServerResult } from '../types.js';
import { recursiveFuzzyIndexOf, getSimilarityRatio } from './fuzzySearch.js';
Expand Down Expand Up @@ -227,6 +227,8 @@ RECOMMENDATION: For large search/replace operations, consider breaking them into
fileName: path.basename(resolvedEditPath),
filePath: resolvedEditPath,
fileType: resolvePreviewFileType(resolvedEditPath),
sourceTool: 'edit_block',
...await getDefaultEditorMetadata(resolvedEditPath),
Comment on lines +230 to +231

@coderabbitai coderabbitai Bot May 24, 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 | 🔴 Critical | ⚡ Quick win

Don’t let optional editor-metadata lookup fail successful edit operations.

These success paths perform the edit first, then await metadata. If metadata lookup fails, the tool returns an error despite a completed mutation, which can trigger duplicate retries.

Proposed fix 
+const safeDefaultEditorMetadata = async (filePath: string) => {
+    try {
+        return await getDefaultEditorMetadata(filePath);
+    } catch {
+        return {};
+    }
+};
...
-                ...await getDefaultEditorMetadata(resolvedEditPath),
+                ...await safeDefaultEditorMetadata(resolvedEditPath),
...
-                        ...await getDefaultEditorMetadata(resolvedRangePath),
+                        ...await safeDefaultEditorMetadata(resolvedRangePath),
...
-                        ...await getDefaultEditorMetadata(resolvedEditRangePath),
+                        ...await safeDefaultEditorMetadata(resolvedEditRangePath),

Also applies to: 443-444, 483-484

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/tools/edit.ts` around lines 230 - 231, The code currently awaits
getDefaultEditorMetadata(resolvedEditPath) and if that throws the entire edit
tool returns an error even though the edit completed; wrap the metadata lookup
in a try/catch around calls that populate metadata (e.g., where sourceTool:
'edit_block' and the other two occurrences at the same pattern), and on error
log a warning and set metadata to null/{} (or omit it) so the function returns
the successful edit result; ensure you only catch errors from
getDefaultEditorMetadata and do not change the edit/mutation result handling.

},
};
}
Expand Down Expand Up @@ -438,6 +440,8 @@ export async function handleEditBlock(args: unknown): Promise<ServerResult> {
fileName: path.basename(resolvedRangePath),
filePath: resolvedRangePath,
fileType: resolvePreviewFileType(resolvedRangePath),
sourceTool: 'edit_block',
...await getDefaultEditorMetadata(resolvedRangePath),
},
};
} catch (error) {
Expand Down Expand Up @@ -476,6 +480,8 @@ export async function handleEditBlock(args: unknown): Promise<ServerResult> {
fileName: path.basename(resolvedEditRangePath),
filePath: resolvedEditRangePath,
fileType: resolvePreviewFileType(resolvedEditRangePath),
sourceTool: 'edit_block',
...await getDefaultEditorMetadata(resolvedEditRangePath),
},
};
}
Expand All @@ -492,4 +498,4 @@ export async function handleEditBlock(args: unknown): Promise<ServerResult> {
search: parsed.old_string,
replace: parsed.new_string
}, parsed.expected_replacements);
}
}
54 changes: 53 additions & 1 deletion src/tools/filesystem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import fs from "fs/promises";
import path from "path";
import os from 'os';
import fetch from 'cross-fetch';
import { execFile } from 'child_process';
import { promisify } from 'util';
import { capture } from '../utils/capture.js';
import { withTimeout } from '../utils/withTimeout.js';
import { configManager } from '../config-manager.js';
Expand Down Expand Up @@ -1041,4 +1043,54 @@ export async function writePdf(
} else {
throw new Error('Invalid content type for writePdf. Expected string (markdown) or array of operations.');
}
}
}

const execFileAsync = promisify(execFile);
type DefaultEditorMetadata = { defaultEditorName?: string; defaultEditorPath?: string };
type DefaultEditorCacheEntry = { metadata: DefaultEditorMetadata; expiresAt?: number };
const DEFAULT_EDITOR_NEGATIVE_CACHE_MS = 5 * 60 * 1000;
const defaultEditorCache = new Map<string, DefaultEditorCacheEntry>();

function escapeAppleScriptString(value: string): string {
return value.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
}

export async function getDefaultEditorMetadata(filePath: string): Promise<DefaultEditorMetadata> {
if (os.platform() !== 'darwin') {
return {};
}

let cacheKey = '';
try {
const extension = path.extname(filePath).toLowerCase();
cacheKey = extension || path.basename(filePath).toLowerCase();
const cached = defaultEditorCache.get(cacheKey);
if (cached) {
if (!cached.expiresAt || cached.expiresAt > Date.now()) {
return cached.metadata;
}
defaultEditorCache.delete(cacheKey);
}

const script = `set appAlias to default application of (info for POSIX file "${escapeAppleScriptString(filePath)}")\nreturn (name of (info for appAlias)) & linefeed & POSIX path of appAlias`;
const { stdout } = await execFileAsync('osascript', ['-e', script], { timeout: 12000 });
const lines = stdout.split('\n').map((line) => line.trim()).filter(Boolean);
const defaultEditorName = lines[lines.length - 2]?.replace(/\.app$/i, '') ?? '';
const defaultEditorPath = lines[lines.length - 1] ?? '';

if (defaultEditorName && defaultEditorPath.startsWith('/')) {
const metadata = { defaultEditorName, defaultEditorPath };
defaultEditorCache.set(cacheKey, { metadata });
return metadata;
}

defaultEditorCache.set(cacheKey, { metadata: {}, expiresAt: Date.now() + DEFAULT_EDITOR_NEGATIVE_CACHE_MS });
} catch {
if (cacheKey) {
defaultEditorCache.set(cacheKey, { metadata: {}, expiresAt: Date.now() + DEFAULT_EDITOR_NEGATIVE_CACHE_MS });
}
// Generic UI fallback is good enough if detection fails.
}

return {};
}
3 changes: 3 additions & 0 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ export interface FilePreviewStructuredContent {
fileName: string;
filePath: string;
fileType: PreviewFileType;
sourceTool?: 'read_file' | 'write_file' | 'edit_block';
defaultEditorName?: string;
defaultEditorPath?: string;
content?: string;
imageData?: string;
mimeType?: string;
Expand Down
38 changes: 21 additions & 17 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 @@ -11,7 +11,7 @@ import { attachDirectoryHandlers } from './directory-controller.js';
import { buildDocumentLayout } from './document-layout.js';
import { getDocumentFullscreenAvailability, parseReadRange, stripReadStatusLine } from './document-workspace.js';
import { getFileTypeCapabilities, renderPayloadBody } from './file-type-handlers.js';
import { buildOpenInEditorCommand, buildOpenInFolderCommand, detectDefaultMarkdownEditor, renderMarkdownEditorAppIcon } from './host/external-actions.js';
import { buildOpenInEditorCommand, buildOpenInFolderCommand, renderMarkdownEditorAppIcon } from './host/external-actions.js';
import { attachSelectionContext } from './host/selection-context.js';
import { createMarkdownController } from './markdown/controller.js';
import {
Expand Down Expand Up @@ -47,7 +47,10 @@ let inlinePayloadBeforeFullscreen: RenderPayload | undefined;
let directoryBackPayload: RenderPayload | undefined;
let selectionAbortController: AbortController | null = null;
const markdownEditorAppCache = new Map<string, { appName: string; appPath?: string }>();
const markdownEditorAppPending = new Set<string>();

function getTelemetryToolName(payload: RenderPayload | undefined): string {
return typeof payload?.sourceTool === 'string' ? payload.sourceTool : 'read_file';
}

async function callToolIfReady(name: string, args: Record<string, unknown>): Promise<unknown | undefined> {
return rpcCallTool ? rpcCallTool(name, args) : undefined;
Expand Down Expand Up @@ -168,7 +171,10 @@ async function readAndResolvePayload(
try {
const freshPayload = await markdownController.readPayload(payload.filePath);
if (freshPayload) {
onReady(freshPayload);
onReady({
...freshPayload,
sourceTool: payload.sourceTool ?? freshPayload.sourceTool,
});
if (freshPayload.fileType === 'markdown') {
void markdownController.refreshFromDisk(freshPayload);
}
Expand Down Expand Up @@ -244,21 +250,16 @@ export function renderApp(
availableDisplayModes: getAvailableDisplayModes(),
}).canFullscreen;

if (payload.fileType === 'markdown' && payload.defaultEditorName) {
markdownEditorAppCache.set(payload.filePath, {
appName: payload.defaultEditorName,
appPath: payload.defaultEditorPath,
});
}

const defaultMarkdownEditor = payload.fileType === 'markdown'
? markdownEditorAppCache.get(payload.filePath)
: undefined;
if (payload.fileType === 'markdown' && !defaultMarkdownEditor) {
void detectDefaultMarkdownEditor({
filePath: payload.filePath,
editorAppCache: markdownEditorAppCache,
editorAppPending: markdownEditorAppPending,
callTool: callToolIfReady,
extractToolText,
onDetected: () => {
rerenderCurrent?.();
},
});
}

const layout = buildDocumentLayout({
payload,
Expand Down Expand Up @@ -477,13 +478,16 @@ export function bootstrapApp(): void {
const result = await app.requestDisplayMode({ mode });
return typeof result.mode === 'string' ? result.mode : null;
};
trackUiEvent = createUiEventTracker(
const filePreviewUiEvent = createUiEventTracker(
(name, args) => app.callServerTool({ name, arguments: args }),
{
component: 'file_preview',
baseParams: { tool_name: 'read_file' },
}
);
trackUiEvent = (event, params = {}) => filePreviewUiEvent(event, {
tool_name: getTelemetryToolName(currentPayload ?? hostPayload),
...params,
});

app.ontoolinput = (params) => {
const requestedPath = typeof params.arguments?.path === 'string' ? params.arguments.path : undefined;
Expand Down
47 changes: 0 additions & 47 deletions src/ui/file-preview/src/host/external-actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,53 +65,6 @@ export function buildOpenInEditorCommand(
return `xdg-open ${shellQuote(trimmedPath)}`;
}

export async function detectDefaultMarkdownEditor(options: {
filePath: string;
editorAppCache: Map<string, { appName: string; appPath?: string }>;
editorAppPending: Set<string>;
callTool?: (name: string, args: Record<string, unknown>) => Promise<unknown | undefined>;
extractToolText: (value: unknown) => string | undefined;
onDetected?: () => void;
}): Promise<void> {
const trimmedPath = options.filePath.trim();
if (!trimmedPath || options.editorAppCache.has(trimmedPath) || options.editorAppPending.has(trimmedPath)) {
return;
}

const userAgent = navigator.userAgent.toLowerCase();
if (!userAgent.includes('mac')) {
return;
}

options.editorAppPending.add(trimmedPath);
try {
const detectCommand = `osascript -e ${shellQuote(`set appAlias to default application of (info for POSIX file "${trimmedPath.replace(/"/g, '\\"')}")
return (name of (info for appAlias)) & linefeed & POSIX path of appAlias`)}`;
const detectResult = await options.callTool?.('start_process', {
command: detectCommand,
timeout_ms: 12000,
});
const text = options.extractToolText(detectResult) ?? '';
if (!text || text.toLowerCase().includes('error') || text.toLowerCase().includes('execution')) {
return;
}
const lines = text.split('\n').map((line) => line.trim()).filter(Boolean);
const appName = lines[lines.length - 2]?.replace(/\.app$/i, '') ?? '';
const appPath = lines[lines.length - 1] ?? '';
if (appName && appPath.startsWith('/')) {
options.editorAppCache.set(trimmedPath, {
appName,
appPath,
});
options.onDetected?.();
}
} catch {
// Fall back to generic editor label.
} finally {
options.editorAppPending.delete(trimmedPath);
}
}

export function renderMarkdownEditorAppIcon(): string {
return '<svg width="14" height="14" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"><path d="M12 20h9"/><path d="M16.5 3.5a2.1 2.1 0 1 1 3 3L7 19l-4 1 1-4Z"/></svg>';
}
12 changes: 11 additions & 1 deletion test/test-file-handlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { fileURLToPath } from 'url';
import assert from 'assert';
import { readFile, writeFile, getFileInfo } from '../dist/tools/filesystem.js';
import { getFileHandler } from '../dist/utils/files/factory.js';
import { handleReadFile } from '../dist/handlers/filesystem-handlers.js';
import { handleReadFile, handleWriteFile } from '../dist/handlers/filesystem-handlers.js';
import { handleEditBlock } from '../dist/handlers/edit-search-handlers.js';

// Get directory name
Expand Down Expand Up @@ -306,20 +306,23 @@ async function testReadFilePreviewMetadata() {
assert.ok(markdownResult.structuredContent, 'Markdown should include structuredContent');
assert.strictEqual(markdownResult.structuredContent.fileType, 'markdown', 'Markdown fileType should be markdown');
assert.strictEqual(markdownResult.structuredContent.filePath, MD_FILE, 'Markdown file path should be present');
assert.strictEqual(markdownResult.structuredContent.sourceTool, 'read_file', 'Markdown preview should preserve source tool');
assert.strictEqual(markdownResult.structuredContent.content, markdownResult.content[0].text, 'Markdown structuredContent should include returned content');

const textResult = await handleReadFile({ path: TEXT_FILE });
assert.ok(Array.isArray(textResult.content), 'Result should include content array');
assert.ok(textResult.content[0].text.includes(textContent), 'Legacy content should still include text body');
assert.ok(textResult.structuredContent, 'Text should include structuredContent');
assert.strictEqual(textResult.structuredContent.fileType, 'text', 'Text fileType should be text');
assert.strictEqual(textResult.structuredContent.sourceTool, 'read_file', 'Text preview should preserve source tool');
assert.strictEqual(textResult.structuredContent.content, textResult.content[0].text, 'Text structuredContent should include returned content');

const htmlResult = await handleReadFile({ path: HTML_FILE });
assert.ok(Array.isArray(htmlResult.content), 'Result should include content array');
assert.ok(htmlResult.content[0].text.includes('<h1>Preview</h1>'), 'Legacy content should still include html body');
assert.ok(htmlResult.structuredContent, 'HTML should include structuredContent');
assert.strictEqual(htmlResult.structuredContent.fileType, 'html', 'HTML fileType should be html');
assert.strictEqual(htmlResult.structuredContent.sourceTool, 'read_file', 'HTML preview should preserve source tool');
assert.strictEqual(htmlResult.structuredContent.content, htmlResult.content[0].text, 'HTML structuredContent should include returned content');

const imageResult = await handleReadFile({ path: IMAGE_FILE });
Expand All @@ -332,6 +335,7 @@ async function testReadFilePreviewMetadata() {
assert.strictEqual(imageResult.structuredContent.content, imageResult.structuredContent.imageData, 'Image structuredContent should include file content');
assert.strictEqual(imageResult.structuredContent.mimeType, 'image/png', 'Image structured payload should include mimeType');
assert.strictEqual(imageResult.structuredContent.filePath, IMAGE_FILE, 'Image file path should be present');
assert.strictEqual(imageResult.structuredContent.sourceTool, 'read_file', 'Image preview should preserve source tool');

const svgResult = await handleReadFile({ path: SVG_FILE });
assert.ok(Array.isArray(svgResult.content), 'SVG result should include content array');
Expand All @@ -341,6 +345,11 @@ async function testReadFilePreviewMetadata() {
assert.strictEqual(svgResult.structuredContent.mimeType, 'image/svg+xml', 'SVG structured payload should include SVG mimeType');
assert.strictEqual(typeof svgResult.structuredContent.imageData, 'string', 'SVG structured payload should include imageData');
assert.ok(svgResult.structuredContent.imageData.length > 0, 'SVG structured payload should include non-empty imageData');
assert.strictEqual(svgResult.structuredContent.sourceTool, 'read_file', 'SVG preview should preserve source tool');

const writeResult = await handleWriteFile({ path: TEXT_FILE, content: 'written through handler' });
assert.ok(writeResult.structuredContent, 'write_file should include structuredContent');
assert.strictEqual(writeResult.structuredContent.sourceTool, 'write_file', 'write_file preview should preserve source tool');

const nullArgsResult = await handleReadFile(null);
assert.ok(Array.isArray(nullArgsResult.content), 'Null-args result should include content array');
Expand Down Expand Up @@ -375,6 +384,7 @@ async function testMarkdownExactMatchSave() {
assert.strictEqual(result.content[0].type, 'text', 'edit_block result[0] should be text');
assert.ok(result.structuredContent, 'edit_block should return structuredContent');
assert.ok(result.structuredContent.filePath, 'edit_block structuredContent should include filePath');
assert.strictEqual(result.structuredContent.sourceTool, 'edit_block', 'edit_block preview should preserve source tool');
assert.match(
result.content[0].text,
/\[Reading \d+ lines? from/,
Expand Down
Loading