Repository navigation
fix(desktop): classify ENOENT spawn failures as PRECONDITION_FAILED, not 500 - #6388
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe external helper now maps missing application launchers to a structured ChangesSpawn error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/external/helpers.ts (1)
388-402: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd error-path tests for external procedures.
external.openInAppandexternal.openFileInEditorpreserve theTRPCError, and the formatter exposesdata.cause.kind. Add tests inhelpers.test.tsforENOENTand non-ENOENTfailures.🤖 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 `@apps/desktop/src/lib/trpc/routers/external/helpers.ts` around lines 388 - 402, Add error-path tests in helpers.test.ts covering external.openInApp and external.openFileInEditor for both ENOENT and non-ENOENT spawn failures. Assert ENOENT preserves the TRPCError with PRECONDITION_FAILED and formatter data.cause.kind set to APP_NOT_INSTALLED, while non-ENOENT failures retain the generic error behavior.Source: MCP tools
🤖 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.
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/external/helpers.ts`:
- Around line 388-402: Add error-path tests in helpers.test.ts covering
external.openInApp and external.openFileInEditor for both ENOENT and non-ENOENT
spawn failures. Assert ENOENT preserves the TRPCError with PRECONDITION_FAILED
and formatter data.cause.kind set to APP_NOT_INSTALLED, while non-ENOENT
failures retain the generic error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f9ffd5e-db21-44fb-bbce-0d5f5af122cf
📒 Files selected for processing (1)
apps/desktop/src/lib/trpc/routers/external/helpers.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No issues found across 1 file
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Architecture diagram
sequenceDiagram
participant UI as Renderer UI
participant TRPC as tRPC Router
participant Spawn as spawnAsync Helper
participant Child as Child Process
participant Middleware as Sentry Middleware
Note over UI,Middleware: External App Launch Flow
UI->>TRPC: invoke "openInApp" / "openFileInEditor"
TRPC->>Spawn: call openPathInApp(command, args)
Spawn->>Child: child_process.spawn()
alt ENOENT error (app not installed / not on PATH)
Child-->>Spawn: error event with code="ENOENT"
Spawn->>Spawn: Build user-facing message
Spawn-->>TRPC: NEW: Reject with TRPCError PRECONDITION_FAILED + cause: APP_NOT_INSTALLED
TRPC-->>Middleware: Pass error (PRECONDITION_FAILED)
Middleware->>Middleware: Skip Sentry capture (non-500)
TRPC-->>UI: Return PRECONDITION_FAILED error
UI->>UI: Show toast with unchanged error message
else Other spawn errors (EACCES, crash, non-zero exit)
Child-->>Spawn: error event with other code
Spawn->>Spawn: Build user-facing message
Spawn-->>TRPC: Reject with plain Error
TRPC-->>Middleware: Pass error (INTERNAL_SERVER_ERROR)
Middleware->>Middleware: Capture to Sentry
TRPC-->>UI: Return INTERNAL_SERVER_ERROR
UI->>UI: Show toast with unchanged error message
end
Note over Spawn,Child: Normal successful exit path unchanged
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
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
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.
Problem
Triggering "Open in " for an editor whose CLI launcher is not installed (or not on PATH) reports to Sentry as an
INTERNAL_SERVER_ERRORonexternal.openInApp(DESKTOP-QZ, 10 events on 1.20.2). A missing external application is an expected user-environment condition, not a bug — the guard and its user-facing message are correct, only the classification is wrong.Root cause
spawnAsyncinapps/desktop/src/lib/trpc/routers/external/helpers.tswraps everychild.on("error")into a plainnew Error(...), dropping theENOENTcode. The tRPC boundary then defaults the plain error toINTERNAL_SERVER_ERROR, which the dumb 500=report Sentry middleware (apps/desktop/src/lib/trpc/index.ts) always captures.Fix
In the shared
spawnAsyncerror handler (used by bothexternal.openInAppandexternal.openFileInEditorviaopenPathInApp), when the incoming spawn error hascode === "ENOENT", reject withnew TRPCError({ code: "PRECONDITION_FAILED", cause: { kind: "APP_NOT_INSTALLED", command } })carrying the exact same message string. Non-ENOENT spawn failures (EACCES, crashes, non-zero exits) keep the plain-Error path and continue reporting as before.The renderer surfaces this error via
toast.error(...error.message...)in every caller (OpenInMenuButton, V2OpenInMenuButton, ClickablePath, usePathActions), and the message is unchanged, so the user-visible toast is identical. No Sentry-side filtering — classification happens at the throw site per the PR #6164 contract.Verification
Refs DESKTOP-QZ
https://claude.ai/code/session_0968498a-7639-4840-b859-e8aab9fa335f
Summary by cubic
Classifies ENOENT spawn failures from “Open in ” as PRECONDITION_FAILED instead of INTERNAL_SERVER_ERROR, so expected “app not installed/not on PATH” cases no longer get reported to Sentry. Previously we wrapped spawn errors as plain
Error, lost theENOENTcode, and tRPC returned a 500; now ENOENT throws aTRPCErrorwithPRECONDITION_FAILED. User-facing toast text is unchanged; non-ENOENT errors behave as before.Review notes
spawnAsyncinapps/desktop/src/lib/trpc/routers/external/helpers.ts, shared byexternal.openInAppandexternal.openFileInEditor.new TRPCError({ code: "PRECONDITION_FAILED", cause: { kind: "APP_NOT_INSTALLED", command } })from@trpc/server; other spawn errors keep the existing path.Written for commit 6b1593d. Summary will update on new commits.
Summary by CodeRabbit