Skip to content

Commit cf857bf

Browse files
edgarsskoreedgarsskore
authored
fix(file-preview): attribute events to source tools (#479)
* fix(file-preview): attribute events to source tools File preview UI events were always reported as read_file, which hid whether the preview came from write_file or edit_block. Carry sourceTool through structured content so analytics reflects the initiating tool. Move default editor detection out of the browser UI and onto the server as a lazy macOS-only cached lookup. This avoids repeated hidden start_process/osascript calls from the preview and keeps command execution in server code. * fix(file-preview): harden editor metadata lookup Narrow sourceTool to the known file-preview tool names so telemetry attribution cannot drift silently. Cache failed default-editor detections briefly and keep getDefaultEditorMetadata best-effort so editor metadata lookup cannot repeatedly spawn osascript or break successful file operations.
1 parent f513fe9 commit cf857bf

7 files changed

Lines changed: 105 additions & 68 deletions

File tree

‎src/handlers/filesystem-handlers.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
moveFile,
88
getFileInfo,
99
writePdf,
10+
getDefaultEditorMetadata,
1011
type FileResult,
1112
type MultiFileResult
1213
} from '../tools/filesystem.js';
@@ -137,6 +138,8 @@ export async function handleReadFile(args: unknown): Promise<ServerResult> {
137138
fileName: path.basename(resolvedFilePath),
138139
filePath: resolvedFilePath,
139140
fileType: 'unsupported' as const,
141+
sourceTool: 'read_file',
142+
...await getDefaultEditorMetadata(resolvedFilePath),
140143
content: pdfContent
141144
.filter((item): item is { type: "text"; text: string } => item.type === "text")
142145
.map((item) => item.text)
@@ -170,6 +173,8 @@ export async function handleReadFile(args: unknown): Promise<ServerResult> {
170173
fileName: path.basename(resolvedFilePath),
171174
filePath: resolvedFilePath,
172175
fileType: 'image',
176+
sourceTool: 'read_file',
177+
...await getDefaultEditorMetadata(resolvedFilePath),
173178
content: imageData,
174179
imageData,
175180
mimeType: fileResult.mimeType
@@ -189,6 +194,8 @@ export async function handleReadFile(args: unknown): Promise<ServerResult> {
189194
fileName: path.basename(resolvedFilePath),
190195
filePath: resolvedFilePath,
191196
fileType,
197+
sourceTool: 'read_file',
198+
...await getDefaultEditorMetadata(resolvedFilePath),
192199
content: textContent,
193200
},
194201
};
@@ -309,6 +316,8 @@ export async function handleWriteFile(args: unknown): Promise<ServerResult> {
309316
fileName: path.basename(resolvedWritePath),
310317
filePath: resolvedWritePath,
311318
fileType: resolvePreviewFileType(resolvedWritePath),
319+
sourceTool: 'write_file',
320+
...await getDefaultEditorMetadata(resolvedWritePath),
312321
},
313322
};
314323
} catch (error) {

‎src/tools/edit.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
* 3. Unify the editRange() signature to handle both text search/replace and structured edits
1616
*/
1717

18-
import { readFile, writeFile, readFileInternal, validatePath } from './filesystem.js';
18+
import { getDefaultEditorMetadata, readFile, writeFile, readFileInternal, validatePath } from './filesystem.js';
1919
import fs from 'fs/promises';
2020
import { ServerResult } from '../types.js';
2121
import { recursiveFuzzyIndexOf, getSimilarityRatio } from './fuzzySearch.js';
@@ -227,6 +227,8 @@ RECOMMENDATION: For large search/replace operations, consider breaking them into
227227
fileName: path.basename(resolvedEditPath),
228228
filePath: resolvedEditPath,
229229
fileType: resolvePreviewFileType(resolvedEditPath),
230+
sourceTool: 'edit_block',
231+
...await getDefaultEditorMetadata(resolvedEditPath),
230232
},
231233
};
232234
}
@@ -438,6 +440,8 @@ export async function handleEditBlock(args: unknown): Promise<ServerResult> {
438440
fileName: path.basename(resolvedRangePath),
439441
filePath: resolvedRangePath,
440442
fileType: resolvePreviewFileType(resolvedRangePath),
443+
sourceTool: 'edit_block',
444+
...await getDefaultEditorMetadata(resolvedRangePath),
441445
},
442446
};
443447
} catch (error) {
@@ -476,6 +480,8 @@ export async function handleEditBlock(args: unknown): Promise<ServerResult> {
476480
fileName: path.basename(resolvedEditRangePath),
477481
filePath: resolvedEditRangePath,
478482
fileType: resolvePreviewFileType(resolvedEditRangePath),
483+
sourceTool: 'edit_block',
484+
...await getDefaultEditorMetadata(resolvedEditRangePath),
479485
},
480486
};
481487
}
@@ -492,4 +498,4 @@ export async function handleEditBlock(args: unknown): Promise<ServerResult> {
492498
search: parsed.old_string,
493499
replace: parsed.new_string
494500
}, parsed.expected_replacements);
495-
}
501+
}

‎src/tools/filesystem.ts‎

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ import fs from "fs/promises";
22
import path from "path";
33
import os from 'os';
44
import fetch from 'cross-fetch';
5+
import { execFile } from 'child_process';
6+
import { promisify } from 'util';
57
import { capture } from '../utils/capture.js';
68
import { withTimeout } from '../utils/withTimeout.js';
79
import { configManager } from '../config-manager.js';
@@ -1041,4 +1043,54 @@ export async function writePdf(
10411043
} else {
10421044
throw new Error('Invalid content type for writePdf. Expected string (markdown) or array of operations.');
10431045
}
1044-
}
1046+
}
1047+
1048+
const execFileAsync = promisify(execFile);
1049+
type DefaultEditorMetadata = { defaultEditorName?: string; defaultEditorPath?: string };
1050+
type DefaultEditorCacheEntry = { metadata: DefaultEditorMetadata; expiresAt?: number };
1051+
const DEFAULT_EDITOR_NEGATIVE_CACHE_MS = 5 * 60 * 1000;
1052+
const defaultEditorCache = new Map<string, DefaultEditorCacheEntry>();
1053+
1054+
function escapeAppleScriptString(value: string): string {
1055+
return value.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
1056+
}
1057+
1058+
export async function getDefaultEditorMetadata(filePath: string): Promise<DefaultEditorMetadata> {
1059+
if (os.platform() !== 'darwin') {
1060+
return {};
1061+
}
1062+
1063+
let cacheKey = '';
1064+
try {
1065+
const extension = path.extname(filePath).toLowerCase();
1066+
cacheKey = extension || path.basename(filePath).toLowerCase();
1067+
const cached = defaultEditorCache.get(cacheKey);
1068+
if (cached) {
1069+
if (!cached.expiresAt || cached.expiresAt > Date.now()) {
1070+
return cached.metadata;
1071+
}
1072+
defaultEditorCache.delete(cacheKey);
1073+
}
1074+
1075+
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`;
1076+
const { stdout } = await execFileAsync('osascript', ['-e', script], { timeout: 12000 });
1077+
const lines = stdout.split('\n').map((line) => line.trim()).filter(Boolean);
1078+
const defaultEditorName = lines[lines.length - 2]?.replace(/\.app$/i, '') ?? '';
1079+
const defaultEditorPath = lines[lines.length - 1] ?? '';
1080+
1081+
if (defaultEditorName && defaultEditorPath.startsWith('/')) {
1082+
const metadata = { defaultEditorName, defaultEditorPath };
1083+
defaultEditorCache.set(cacheKey, { metadata });
1084+
return metadata;
1085+
}
1086+
1087+
defaultEditorCache.set(cacheKey, { metadata: {}, expiresAt: Date.now() + DEFAULT_EDITOR_NEGATIVE_CACHE_MS });
1088+
} catch {
1089+
if (cacheKey) {
1090+
defaultEditorCache.set(cacheKey, { metadata: {}, expiresAt: Date.now() + DEFAULT_EDITOR_NEGATIVE_CACHE_MS });
1091+
}
1092+
// Generic UI fallback is good enough if detection fails.
1093+
}
1094+
1095+
return {};
1096+
}

‎src/types.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,9 @@ export interface FilePreviewStructuredContent {
7676
fileName: string;
7777
filePath: string;
7878
fileType: PreviewFileType;
79+
sourceTool?: 'read_file' | 'write_file' | 'edit_block';
80+
defaultEditorName?: string;
81+
defaultEditorPath?: string;
7982
content?: string;
8083
imageData?: string;
8184
mimeType?: string;

‎src/ui/file-preview/src/app.ts‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import { attachDirectoryHandlers } from './directory-controller.js';
1111
import { buildDocumentLayout } from './document-layout.js';
1212
import { getDocumentFullscreenAvailability, parseReadRange, stripReadStatusLine } from './document-workspace.js';
1313
import { getFileTypeCapabilities, renderPayloadBody } from './file-type-handlers.js';
14-
import { buildOpenInEditorCommand, buildOpenInFolderCommand, detectDefaultMarkdownEditor, renderMarkdownEditorAppIcon } from './host/external-actions.js';
14+
import { buildOpenInEditorCommand, buildOpenInFolderCommand, renderMarkdownEditorAppIcon } from './host/external-actions.js';
1515
import { attachSelectionContext } from './host/selection-context.js';
1616
import { createMarkdownController } from './markdown/controller.js';
1717
import {
@@ -47,7 +47,10 @@ let inlinePayloadBeforeFullscreen: RenderPayload | undefined;
4747
let directoryBackPayload: RenderPayload | undefined;
4848
let selectionAbortController: AbortController | null = null;
4949
const markdownEditorAppCache = new Map<string, { appName: string; appPath?: string }>();
50-
const markdownEditorAppPending = new Set<string>();
50+
51+
function getTelemetryToolName(payload: RenderPayload | undefined): string {
52+
return typeof payload?.sourceTool === 'string' ? payload.sourceTool : 'read_file';
53+
}
5154

5255
async function callToolIfReady(name: string, args: Record<string, unknown>): Promise<unknown | undefined> {
5356
return rpcCallTool ? rpcCallTool(name, args) : undefined;
@@ -168,7 +171,10 @@ async function readAndResolvePayload(
168171
try {
169172
const freshPayload = await markdownController.readPayload(payload.filePath);
170173
if (freshPayload) {
171-
onReady(freshPayload);
174+
onReady({
175+
...freshPayload,
176+
sourceTool: payload.sourceTool ?? freshPayload.sourceTool,
177+
});
172178
if (freshPayload.fileType === 'markdown') {
173179
void markdownController.refreshFromDisk(freshPayload);
174180
}
@@ -244,21 +250,16 @@ export function renderApp(
244250
availableDisplayModes: getAvailableDisplayModes(),
245251
}).canFullscreen;
246252

253+
if (payload.fileType === 'markdown' && payload.defaultEditorName) {
254+
markdownEditorAppCache.set(payload.filePath, {
255+
appName: payload.defaultEditorName,
256+
appPath: payload.defaultEditorPath,
257+
});
258+
}
259+
247260
const defaultMarkdownEditor = payload.fileType === 'markdown'
248261
? markdownEditorAppCache.get(payload.filePath)
249262
: undefined;
250-
if (payload.fileType === 'markdown' && !defaultMarkdownEditor) {
251-
void detectDefaultMarkdownEditor({
252-
filePath: payload.filePath,
253-
editorAppCache: markdownEditorAppCache,
254-
editorAppPending: markdownEditorAppPending,
255-
callTool: callToolIfReady,
256-
extractToolText,
257-
onDetected: () => {
258-
rerenderCurrent?.();
259-
},
260-
});
261-
}
262263

263264
const layout = buildDocumentLayout({
264265
payload,
@@ -477,13 +478,16 @@ export function bootstrapApp(): void {
477478
const result = await app.requestDisplayMode({ mode });
478479
return typeof result.mode === 'string' ? result.mode : null;
479480
};
480-
trackUiEvent = createUiEventTracker(
481+
const filePreviewUiEvent = createUiEventTracker(
481482
(name, args) => app.callServerTool({ name, arguments: args }),
482483
{
483484
component: 'file_preview',
484-
baseParams: { tool_name: 'read_file' },
485485
}
486486
);
487+
trackUiEvent = (event, params = {}) => filePreviewUiEvent(event, {
488+
tool_name: getTelemetryToolName(currentPayload ?? hostPayload),
489+
...params,
490+
});
487491

488492
app.ontoolinput = (params) => {
489493
const requestedPath = typeof params.arguments?.path === 'string' ? params.arguments.path : undefined;

‎src/ui/file-preview/src/host/external-actions.ts‎

Lines changed: 0 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -65,53 +65,6 @@ export function buildOpenInEditorCommand(
6565
return `xdg-open ${shellQuote(trimmedPath)}`;
6666
}
6767

68-
export async function detectDefaultMarkdownEditor(options: {
69-
filePath: string;
70-
editorAppCache: Map<string, { appName: string; appPath?: string }>;
71-
editorAppPending: Set<string>;
72-
callTool?: (name: string, args: Record<string, unknown>) => Promise<unknown | undefined>;
73-
extractToolText: (value: unknown) => string | undefined;
74-
onDetected?: () => void;
75-
}): Promise<void> {
76-
const trimmedPath = options.filePath.trim();
77-
if (!trimmedPath || options.editorAppCache.has(trimmedPath) || options.editorAppPending.has(trimmedPath)) {
78-
return;
79-
}
80-
81-
const userAgent = navigator.userAgent.toLowerCase();
82-
if (!userAgent.includes('mac')) {
83-
return;
84-
}
85-
86-
options.editorAppPending.add(trimmedPath);
87-
try {
88-
const detectCommand = `osascript -e ${shellQuote(`set appAlias to default application of (info for POSIX file "${trimmedPath.replace(/"/g, '\\"')}")
89-
return (name of (info for appAlias)) & linefeed & POSIX path of appAlias`)}`;
90-
const detectResult = await options.callTool?.('start_process', {
91-
command: detectCommand,
92-
timeout_ms: 12000,
93-
});
94-
const text = options.extractToolText(detectResult) ?? '';
95-
if (!text || text.toLowerCase().includes('error') || text.toLowerCase().includes('execution')) {
96-
return;
97-
}
98-
const lines = text.split('\n').map((line) => line.trim()).filter(Boolean);
99-
const appName = lines[lines.length - 2]?.replace(/\.app$/i, '') ?? '';
100-
const appPath = lines[lines.length - 1] ?? '';
101-
if (appName && appPath.startsWith('/')) {
102-
options.editorAppCache.set(trimmedPath, {
103-
appName,
104-
appPath,
105-
});
106-
options.onDetected?.();
107-
}
108-
} catch {
109-
// Fall back to generic editor label.
110-
} finally {
111-
options.editorAppPending.delete(trimmedPath);
112-
}
113-
}
114-
11568
export function renderMarkdownEditorAppIcon(): string {
11669
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>';
11770
}

‎test/test-file-handlers.js‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import { fileURLToPath } from 'url';
1818
import assert from 'assert';
1919
import { readFile, writeFile, getFileInfo } from '../dist/tools/filesystem.js';
2020
import { getFileHandler } from '../dist/utils/files/factory.js';
21-
import { handleReadFile } from '../dist/handlers/filesystem-handlers.js';
21+
import { handleReadFile, handleWriteFile } from '../dist/handlers/filesystem-handlers.js';
2222
import { handleEditBlock } from '../dist/handlers/edit-search-handlers.js';
2323

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

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

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

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

336340
const svgResult = await handleReadFile({ path: SVG_FILE });
337341
assert.ok(Array.isArray(svgResult.content), 'SVG result should include content array');
@@ -341,6 +345,11 @@ async function testReadFilePreviewMetadata() {
341345
assert.strictEqual(svgResult.structuredContent.mimeType, 'image/svg+xml', 'SVG structured payload should include SVG mimeType');
342346
assert.strictEqual(typeof svgResult.structuredContent.imageData, 'string', 'SVG structured payload should include imageData');
343347
assert.ok(svgResult.structuredContent.imageData.length > 0, 'SVG structured payload should include non-empty imageData');
348+
assert.strictEqual(svgResult.structuredContent.sourceTool, 'read_file', 'SVG preview should preserve source tool');
349+
350+
const writeResult = await handleWriteFile({ path: TEXT_FILE, content: 'written through handler' });
351+
assert.ok(writeResult.structuredContent, 'write_file should include structuredContent');
352+
assert.strictEqual(writeResult.structuredContent.sourceTool, 'write_file', 'write_file preview should preserve source tool');
344353

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

0 commit comments

Comments
 (0)