fix(ipc): graceful no-op on stale paths for shell + fs handlers#918
fix(ipc): graceful no-op on stale paths for shell + fs handlers#918pedramamini merged 1 commit intomainfrom
Conversation
shell:showItemInFolder, shell:trashItem, and fs:readFile rejected the IPC promise when given paths the user had already removed (or that pointed at a directory). Renderer callers fired-and-forget, so the rejections surfaced as unhandled rejections and were captured by Sentry as bugs even though the conditions are routine user state. Mirror the existing shell:openPath / fs:readFile-ENOENT pattern so each handler logs a warning and returns rather than throwing. Fixes MAESTRO-K1, MAESTRO-HN, MAESTRO-HS (shell:showItemInFolder "Path does not exist"), MAESTRO-JD, MAESTRO-JC (shell:trashItem "Path does not exist"), and MAESTRO-JP (fs:readFile EISDIR).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughIPC handler implementations and tests are updated to gracefully handle edge cases: file system read operations now return Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR converts three IPC handlers ( Confidence Score: 4/5Safe to merge; changes are narrowly scoped, well-tested, and consistent with established patterns in the codebase. Only P2 finding (missing observability log for EISDIR in fs:readFile); no logic bugs, no security concerns, tests cover all new branches. src/main/ipc/handlers/filesystem.ts — minor: EISDIR branch returns null silently with no log, unlike the shell handlers in this same PR. Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Renderer
participant IPC as IPC Handler
participant FS as Filesystem
Note over R,FS: shell:trashItem / shell:showItemInFolder (stale path)
R->>IPC: invoke(handler, stalePath)
IPC->>FS: existsSync(absolutePath)
FS-->>IPC: false
IPC->>IPC: logger.warn(...)
IPC-->>R: resolves undefined (no-op)
Note over R,FS: fs:readFile (directory path / EISDIR)
R->>IPC: invoke('fs:readFile', dirPath)
IPC->>FS: readFile(dirPath)
FS-->>IPC: throws EISDIR
IPC->>IPC: catch — error.code === 'EISDIR'
IPC-->>R: resolves null
Note over R,FS: fs:readFile (missing file / ENOENT) — existing behaviour
R->>IPC: invoke('fs:readFile', missingPath)
IPC->>FS: readFile(missingPath)
FS-->>IPC: throws ENOENT
IPC->>IPC: catch — error.code === 'ENOENT'
IPC-->>R: resolves null
Reviews (1): Last reviewed commit: "fix(ipc): graceful no-op on stale paths ..." | Re-trigger Greptile |
| if (error?.code === 'EISDIR') { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Missing observability log for EISDIR
The ENOENT branch is intentionally silent (established pattern), but EISDIR represents a different failure mode — the caller passed a directory path instead of a file path, which is a programming mistake rather than routine missing-file activity. Unlike the shell handlers in this same PR, fs:readFile returns null with no log, making it invisible in production logs when this condition fires.
Consider adding a logger.warn (or at minimum logger.debug) to match the diagnostic pattern used for shell:trashItem and shell:showItemInFolder.
| if (error?.code === 'EISDIR') { | |
| return null; | |
| } | |
| if (error?.code === 'EISDIR') { | |
| logger.warn(`fs:readFile - path is a directory, returning null: ${filePath}`, 'Filesystem'); | |
| return null; | |
| } |
Summary
Several IPC handlers reject the renderer's promise when the user supplies a path that was already removed (or that turned out to be a directory). Those callers are fire-and-forget, so the rejections surface as unhandled rejections in the renderer and Sentry captures them as bugs — even though the underlying state is routine user activity (file deleted between display and click, dropdown entry pointing at a folder, etc.).
This change mirrors the existing `shell:openPath` (MAESTRO-B3) and `fs:readFile`-ENOENT pattern so the same situations log a warning and return cleanly instead of throwing:
Tests updated to cover the new behavior. No renderer changes needed; existing fire-and-forget call sites simply stop rejecting.
Sentry issues addressed
Test plan
Summary by CodeRabbit
Bug Fixes
Tests