fix(file-preview): attribute events to source tools#479
Open
edgarsskore wants to merge 2 commits into
Open
Conversation
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.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds sourceTool and default-editor metadata to file preview and edit responses, implements getDefaultEditorMetadata(...) (macOS osascript + cache), and updates UI wiring and telemetry to consume payload-provided editor metadata instead of performing client-side detection. ChangesEditor metadata enrichment and refactoring
Sequence DiagramsequenceDiagram
participant Client
participant ServerHandler
participant getDefaultEditorMetadata
participant StructuredContent
participant UI
Client->>ServerHandler: request file preview / edit
ServerHandler->>getDefaultEditorMetadata: getDefaultEditorMetadata(filePath)
getDefaultEditorMetadata-->>ServerHandler: { defaultEditorName?, defaultEditorPath? } or {}
ServerHandler->>StructuredContent: set sourceTool + merge editor metadata
StructuredContent-->>Client: return enriched payload
Client->>UI: render payload (UI seeds cache from payload, uses sourceTool for telemetry)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/types.ts (1)
79-79: ⚡ Quick winNarrow
sourceToolto a literal union to prevent telemetry drift.
FilePreviewStructuredContent.sourceToolis currentlystring, but all in-repo producers assign only'read_file','write_file', or'edit_block', so tightening the type prevents silent typos at compile time.Proposed diff
export interface FilePreviewStructuredContent { fileName: string; filePath: string; fileType: PreviewFileType; - sourceTool?: string; + sourceTool?: 'read_file' | 'write_file' | 'edit_block'; defaultEditorName?: string; defaultEditorPath?: string; content?: string; imageData?: string; mimeType?: string; }🤖 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/types.ts` at line 79, Change the FilePreviewStructuredContent.sourceTool property from a generic string to a literal union of the allowed values to avoid telemetry drift: replace the type of sourceTool with 'read_file' | 'write_file' | 'edit_block' (i.e., FilePreviewStructuredContent.sourceTool?: 'read_file' | 'write_file' | 'edit_block';) so any future mistyped values fail at compile time.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/handlers/filesystem-handlers.ts`:
- Around line 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).
In `@src/tools/edit.ts`:
- Around line 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.
In `@src/tools/filesystem.ts`:
- Around line 1062-1083: The code only writes to defaultEditorCache on
successful osascript detection, so repeated failures keep spawning
execFileAsync('osascript', ...) on every call; modify the get-default-editor
flow (use symbols defaultEditorCache, cacheKey, execFileAsync,
escapeAppleScriptString) to record and return a cached negative result when
detection fails (e.g., cache an empty metadata object or a sentinel value with
appropriate TTL) inside the try/catch path so subsequent lookups short-circuit
and avoid repeated process spawns; ensure the same cached shape is used by
callers and that successful detections continue to overwrite the negative cache
when appropriate.
---
Nitpick comments:
In `@src/types.ts`:
- Line 79: Change the FilePreviewStructuredContent.sourceTool property from a
generic string to a literal union of the allowed values to avoid telemetry
drift: replace the type of sourceTool with 'read_file' | 'write_file' |
'edit_block' (i.e., FilePreviewStructuredContent.sourceTool?: 'read_file' |
'write_file' | 'edit_block';) so any future mistyped values fail at compile
time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19bffc2e-d823-4a3a-82a6-cf055ede2d1b
📒 Files selected for processing (7)
src/handlers/filesystem-handlers.tssrc/tools/edit.tssrc/tools/filesystem.tssrc/types.tssrc/ui/file-preview/src/app.tssrc/ui/file-preview/src/host/external-actions.tstest/test-file-handlers.js
💤 Files with no reviewable changes (1)
- src/ui/file-preview/src/host/external-actions.ts
Comment on lines
+141
to
+142
| sourceTool: 'read_file', | ||
| ...await getDefaultEditorMetadata(resolvedFilePath), |
Contributor
There was a problem hiding this comment.
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).
Comment on lines
+230
to
+231
| sourceTool: 'edit_block', | ||
| ...await getDefaultEditorMetadata(resolvedEditPath), |
Contributor
There was a problem hiding this comment.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 cached lookup. This avoids repeated hidden start_process/osascript calls from the preview and keeps command execution in server code.
Summary by CodeRabbit
New Features
Improvements
Tests