Repository navigation
fix(host-service): surface breaking failures instead of swallowing them - #5493
Merged
Merged
Conversation
- db.ts: let a failed migration throw instead of returning a half-migrated DB; add busy_timeout - coordinator: alert (native dialog + Restart) when a running host-service child crashes, instead of only emitting a status event - renderer: surface start-mutation failures via onError instead of dropping them into unread state
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Ready to review this PR? Stage has broken it down into 4 individual chapters for you:
Chapters generated by Stage for commit c0b68b4 on Jul 8, 2026 6:52pm UTC. |
Contributor
📝 WalkthroughWalkthroughHost-service crash handling now shows a native error dialog after unexpected child exits, start failures now surface as unauthorized TRPC errors or renderer toasts, and database initialization now sets a busy timeout and throws on migration failure. ChangesDesktop host-service error handling
Host-service database initialization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/host-service-coordinator.ts (1)
543-551: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestart click can silently no-op when no auth token is present.
If
loadToken()returns no token, the handler returns without any feedback, so the user who explicitly chose "Restart" sees nothing happen. Consider surfacing a follow-up dialog (or logging + re-alerting) so the failed precondition isn't invisible.🤖 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/main/lib/host-service-coordinator.ts` around lines 543 - 551, The restart flow in host-service-coordinator’s prompt handler can silently exit after loadToken() returns no token, leaving the user with no feedback. Update the .then(async ({ response }) => { ... }) path to handle the missing-token branch explicitly by showing a follow-up dialog or re-alerting/logging before returning, so the Restart action in this flow is never an invisible no-op.
🤖 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 `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 414-425: The status transition in host-service-coordinator.ts is
using a hard-coded previous status of "running" when calling emitStatus, but
current.status can still be "starting" in this exit path. Update the logic
around the current.status check and the emitStatus call in
HostServiceCoordinator so it preserves the actual prior state and passes that
value instead of always "running", using the existing current.status/ crashed
flow to locate the fix.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 543-551: The restart flow in host-service-coordinator’s prompt
handler can silently exit after loadToken() returns no token, leaving the user
with no feedback. Update the .then(async ({ response }) => { ... }) path to
handle the missing-token branch explicitly by showing a follow-up dialog or
re-alerting/logging before returning, so the Restart action in this flow is
never an invisible no-op.
🪄 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: 45694c8a-89af-4b72-a731-32e29e60edd5
📒 Files selected for processing (3)
apps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/renderer/routes/_authenticated/providers/LocalHostServiceProvider/LocalHostServiceProvider.tsxpackages/host-service/src/db/db.ts
Contributor
Greptile SummaryThis PR un-swallows breaking host-service failures at three distinct layers: migration errors now crash the child process loudly instead of returning a half-migrated DB, unexpected crashes of a running child surface via a native dialog with a Restart button, and renderer-side start failures fire a toast instead of silently settling into unread React Query state.
Confidence Score: 4/5Safe to merge — all three changes move in the right direction and the core crash-surfacing logic is sound. The migration-throw change in db.ts is clean and correct. The crash dialog logic correctly distinguishes running-state deaths from startup deaths, and the stop() guard prevents false dialogs on deliberate shutdown. Two gaps remain in the coordinator: the dialog is shown without a parent window (can appear behind the app on Windows), and if the user clicks Restart and the restart fails, the error is only written to electron-log with no user-visible feedback. The auth-error substring filter in the renderer is fragile but functional for the current error message shape. apps/desktop/src/main/lib/host-service-coordinator.ts — the crash dialog UX and its restart error handling are the areas most likely to need a follow-up.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/host-service-coordinator.ts | Adds crash detection and native dialog (Restart/Dismiss) when a running child exits unexpectedly; restart failures after user clicks Restart are only logged to electron-log with no user-facing feedback, and the dialog is shown without a parent BrowserWindow reference. |
| apps/desktop/src/renderer/routes/_authenticated/providers/LocalHostServiceProvider/LocalHostServiceProvider.tsx | Adds onError handler to the start mutation to surface failures as toasts; auth-error suppression relies on a fragile substring check rather than a typed error code. |
| packages/host-service/src/db/db.ts | Removes the migration try/catch so failures crash the child loudly, and adds busy_timeout = 5000 pragma for concurrent-access safety — both straightforward and correct changes. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant R as Renderer (LocalHostServiceProvider)
participant M as Main (HostServiceCoordinator)
participant C as Child (host-service)
participant DB as SQLite DB
R->>M: start mutation
M->>C: spawn child process
C->>DB: createDb() - migrate()
alt migration fails (NEW: throws instead of swallow)
DB-->>C: throws
C-->>M: exit(1)
M-->>R: onError - toast.error (NEW)
else migration succeeds
DB-->>C: ok
C-->>M: health check passes
M-->>R: Connection port+secret
Note over C,M: service running
alt child crashes unexpectedly (NEW)
C-->>M: "exit event status=running"
M->>M: alertChildCrashed()
M-->>R: native dialog Host service crashed
alt user clicks Restart
M->>M: loadToken()
M->>C: restart - spawn new child
else user clicks Dismiss
Note over M: stopped, no restart
end
end
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant R as Renderer (LocalHostServiceProvider)
participant M as Main (HostServiceCoordinator)
participant C as Child (host-service)
participant DB as SQLite DB
R->>M: start mutation
M->>C: spawn child process
C->>DB: createDb() - migrate()
alt migration fails (NEW: throws instead of swallow)
DB-->>C: throws
C-->>M: exit(1)
M-->>R: onError - toast.error (NEW)
else migration succeeds
DB-->>C: ok
C-->>M: health check passes
M-->>R: Connection port+secret
Note over C,M: service running
alt child crashes unexpectedly (NEW)
C-->>M: "exit event status=running"
M->>M: alertChildCrashed()
M-->>R: native dialog Host service crashed
alt user clicks Restart
M->>M: loadToken()
M->>C: restart - spawn new child
else user clicks Dismiss
Note over M: stopped, no restart
end
end
end
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/desktop/src/main/lib/host-service-coordinator.ts:533-557
**Dialog shown without parent window**
`dialog.showMessageBox` is called without a `BrowserWindow` argument. On Windows this causes the dialog to appear as a standalone system window that can easily be obscured behind the focused app window, leading users to believe the Restart button does nothing. On macOS the behavior is acceptable but still suboptimal. Passing the focused (or any visible) `BrowserWindow` pins the dialog to the front of the app's window stack.
Consider using `BrowserWindow.getFocusedWindow() ?? BrowserWindow.getAllWindows()[0]` as the first argument to `dialog.showMessageBox`.
### Issue 2 of 3
apps/desktop/src/main/lib/host-service-coordinator.ts:543-557
**Silent restart failure after user clicks "Restart"**
When `this.restart()` throws (e.g., the newly spawned child fails its health check), the error is swallowed in the `.catch()` and only written to `electron-log`. The user clicked "Restart", the dialog closes, but the service never comes up and no feedback is given. The `onError` toast in `LocalHostServiceProvider` only fires for mutations triggered from the renderer; this restart originates in the main process and bypasses that path entirely.
Consider calling `dialog.showErrorBox` or a second `dialog.showMessageBox` inside the `.catch` to tell the user that the restart itself failed.
### Issue 3 of 3
apps/desktop/src/renderer/routes/_authenticated/providers/LocalHostServiceProvider/LocalHostServiceProvider.tsx:44-45
**Fragile string-match auth-error filter**
The filter `error.message.toLowerCase().includes("auth token")` is a plain substring match. If the error message wording changes (e.g., "missing token", "invalid token", "no token available"), real startup-race noise slips through as a spurious toast. Conversely, any genuine failure whose message happens to contain "auth token" (e.g., "Failed to exchange auth token with upstream") is silently suppressed. A typed error code or a dedicated `TRPCClientError` subtype would make the boundary explicit and rename-proof.
Reviews (1): Last reviewed commit: "fix(host-service): surface breaking fail..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+533
to
+557
| void dialog | ||
| .showMessageBox({ | ||
| type: "error", | ||
| title: "Host service crashed", | ||
| message: "The Superset host service stopped unexpectedly.", | ||
| detail: `Workspaces and terminals for this organization are unavailable until it restarts (${cause}).`, | ||
| buttons: ["Restart", "Dismiss"], | ||
| defaultId: 0, | ||
| cancelId: 1, | ||
| }) | ||
| .then(async ({ response }) => { | ||
| if (response !== 0) return; | ||
| const { token } = await loadToken(); | ||
| if (!token) return; | ||
| await this.restart(organizationId, { | ||
| authToken: token, | ||
| cloudApiUrl: mainEnv.NEXT_PUBLIC_API_URL, | ||
| }); | ||
| }) | ||
| .catch((error) => { | ||
| log.error( | ||
| `[host-service:${organizationId}] crash-alert restart failed:`, | ||
| error, | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
dialog.showMessageBox is called without a BrowserWindow argument. On Windows this causes the dialog to appear as a standalone system window that can easily be obscured behind the focused app window, leading users to believe the Restart button does nothing. On macOS the behavior is acceptable but still suboptimal. Passing the focused (or any visible) BrowserWindow pins the dialog to the front of the app's window stack.
Consider using BrowserWindow.getFocusedWindow() ?? BrowserWindow.getAllWindows()[0] as the first argument to dialog.showMessageBox.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/host-service-coordinator.ts
Line: 533-557
Comment:
**Dialog shown without parent window**
`dialog.showMessageBox` is called without a `BrowserWindow` argument. On Windows this causes the dialog to appear as a standalone system window that can easily be obscured behind the focused app window, leading users to believe the Restart button does nothing. On macOS the behavior is acceptable but still suboptimal. Passing the focused (or any visible) `BrowserWindow` pins the dialog to the front of the app's window stack.
Consider using `BrowserWindow.getFocusedWindow() ?? BrowserWindow.getAllWindows()[0]` as the first argument to `dialog.showMessageBox`.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+543
to
+557
| .then(async ({ response }) => { | ||
| if (response !== 0) return; | ||
| const { token } = await loadToken(); | ||
| if (!token) return; | ||
| await this.restart(organizationId, { | ||
| authToken: token, | ||
| cloudApiUrl: mainEnv.NEXT_PUBLIC_API_URL, | ||
| }); | ||
| }) | ||
| .catch((error) => { | ||
| log.error( | ||
| `[host-service:${organizationId}] crash-alert restart failed:`, | ||
| error, | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
When this.restart() throws (e.g., the newly spawned child fails its health check), the error is swallowed in the .catch() and only written to electron-log. The user clicked "Restart", the dialog closes, but the service never comes up and no feedback is given. The onError toast in LocalHostServiceProvider only fires for mutations triggered from the renderer; this restart originates in the main process and bypasses that path entirely.
Consider calling dialog.showErrorBox or a second dialog.showMessageBox inside the .catch to tell the user that the restart itself failed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/host-service-coordinator.ts
Line: 543-557
Comment:
**Silent restart failure after user clicks "Restart"**
When `this.restart()` throws (e.g., the newly spawned child fails its health check), the error is swallowed in the `.catch()` and only written to `electron-log`. The user clicked "Restart", the dialog closes, but the service never comes up and no feedback is given. The `onError` toast in `LocalHostServiceProvider` only fires for mutations triggered from the renderer; this restart originates in the main process and bypasses that path entirely.
Consider calling `dialog.showErrorBox` or a second `dialog.showMessageBox` inside the `.catch` to tell the user that the restart itself failed.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+44
to
+45
| // Auth preconditions resolve once the token lands; not a real failure. | ||
| if (error.message.toLowerCase().includes("auth token")) return; |
Contributor
There was a problem hiding this comment.
The filter error.message.toLowerCase().includes("auth token") is a plain substring match. If the error message wording changes (e.g., "missing token", "invalid token", "no token available"), real startup-race noise slips through as a spurious toast. Conversely, any genuine failure whose message happens to contain "auth token" (e.g., "Failed to exchange auth token with upstream") is silently suppressed. A typed error code or a dedicated TRPCClientError subtype would make the boundary explicit and rename-proof.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/providers/LocalHostServiceProvider/LocalHostServiceProvider.tsx
Line: 44-45
Comment:
**Fragile string-match auth-error filter**
The filter `error.message.toLowerCase().includes("auth token")` is a plain substring match. If the error message wording changes (e.g., "missing token", "invalid token", "no token available"), real startup-race noise slips through as a spurious toast. Conversely, any genuine failure whose message happens to contain "auth token" (e.g., "Failed to exchange auth token with upstream") is silently suppressed. A typed error code or a dedicated `TRPCClientError` subtype would make the boundary explicit and rename-proof.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 3 files
Architecture diagram
sequenceDiagram
participant Renderer as Renderer Process
participant LocalHS as LocalHostServiceProvider
participant Coordinator as HostServiceCoordinator (Main)
participant DB as Host Service DB
participant Child as Host Service Child
Note over Renderer,Child: Starting the host service
Renderer->>LocalHS: Use start mutation
LocalHS->>LocalHS: Call electronTrpc.hostServiceCoordinator.start mutation
LocalHS->>Coordinator: Invoke start()
Coordinator->>Coordinator: Spawn child process (host-service)
Coordinator->>Child: Execute
alt Child process fails to start
Coordinator-->>LocalHS: Rejection with error
LocalHS->>LocalHS: NEW: onError handler
alt Error is not "auth token" precondition
LocalHS->>LocalHS: toast.error("Host service failed to start")
end
LocalHS-->>Renderer: No status update
else Child starts successfully
Coordinator-->>LocalHS: Success
LocalHS-->>Renderer: Status = "running"
end
Note over Coordinator,Child: Startup phase - DB migration
Child->>DB: Create database
DB->>DB: Set busy_timeout = 5000
DB->>DB: Run migrations
alt Migration fails
DB-->>Child: NEW: Throw error (no catch)
Child->>Child: Crash (exit with error)
Child-->>Coordinator: Unexpected exit
else Migration succeeds
DB-->>Child: Ready
Child-->>Coordinator: Running normally
end
Note over Coordinator,Child: Runtime - unexpected child crash
alt Child crashes while "running"
Child-->>Coordinator: Exit event (code, signal)
Coordinator->>Coordinator: Check status !== "stopped"
Coordinator->>Coordinator: NEW: alertChildCrashed()
Coordinator->>Coordinator: dialog.showMessageBox(error, buttons["Restart", "Dismiss"])
Coordinator-->>Coordinator: User selects "Restart"
Coordinator->>Coordinator: loadToken()
Coordinator->>Coordinator: restart(organizationId, {authToken, cloudApiUrl})
Coordinator->>Child: Spawn new child
Child-->>Coordinator: New child running
else Deliberate stop()
Coordinator->>Coordinator: Set status = "stopped"
Coordinator->>Child: Kill child
Child-->>Coordinator: Exit event
Coordinator->>Coordinator: Early return (no alert)
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
- drop Restart button (silent no-op when no token, flagged in review); recovery is the existing tray Restart. removes loadToken/env plumbing. - emitStatus now reports the real prior status instead of hardcoded 'running' (child can die during the starting/health-poll window).
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/host-service-coordinator.ts (1)
523-535: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider identifying the affected organization in the crash dialog.
The dialog message says "unavailable ... for this organization" but never names the org, so with multiple concurrent host-service instances the user won't know which one crashed just from the alert.
💡 Optional improvement
log.error(`[host-service:${organizationId}] crashed (${cause})`); dialog.showErrorBox( "Host service crashed", - `The Superset host service stopped unexpectedly (${cause}). Workspaces and terminals for this organization are unavailable until it restarts — use the Superset tray menu > Host Service > Restart.`, + `The Superset host service for organization ${organizationId} stopped unexpectedly (${cause}). Workspaces and terminals for this organization are unavailable until it restarts — use the Superset tray menu > Host Service > Restart.`, );🤖 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/main/lib/host-service-coordinator.ts` around lines 523 - 535, The crash dialog in alertChildCrashed only says “this organization” and does not identify which org stopped, which is ambiguous when multiple host-service instances are running. Update the dialog text built in alertChildCrashed to include the affected organizationId alongside the existing cause so the user can tell which organization’s workspaces and terminals are unavailable. Keep the log line as-is, but ensure the dialog message itself names the organization clearly.
🤖 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/main/lib/host-service-coordinator.ts`:
- Around line 523-535: The crash dialog in alertChildCrashed only says “this
organization” and does not identify which org stopped, which is ambiguous when
multiple host-service instances are running. Update the dialog text built in
alertChildCrashed to include the affected organizationId alongside the existing
cause so the user can tell which organization’s workspaces and terminals are
unavailable. Keep the log line as-is, but ensure the dialog message itself names
the organization clearly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8ca0ea4-ce4b-490b-b57b-f666e95ad866
📒 Files selected for processing (1)
apps/desktop/src/main/lib/host-service-coordinator.ts
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/lib/host-service-coordinator.ts">
<violation number="1" location="apps/desktop/src/main/lib/host-service-coordinator.ts:533">
P2: On Windows/Linux, the crash dialog points users to a recovery path that does not exist because the Superset tray is only initialized on macOS. Consider using platform-neutral recovery text or gating the tray-specific instruction to macOS.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| log.error(`[host-service:${organizationId}] crashed (${cause})`); | ||
| dialog.showErrorBox( | ||
| "Host service crashed", | ||
| `The Superset host service stopped unexpectedly (${cause}). Workspaces and terminals for this organization are unavailable until it restarts — use the Superset tray menu > Host Service > Restart.`, |
Contributor
There was a problem hiding this comment.
P2: On Windows/Linux, the crash dialog points users to a recovery path that does not exist because the Superset tray is only initialized on macOS. Consider using platform-neutral recovery text or gating the tray-specific instruction to macOS.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/host-service-coordinator.ts, line 533:
<comment>On Windows/Linux, the crash dialog points users to a recovery path that does not exist because the Superset tray is only initialized on macOS. Consider using platform-neutral recovery text or gating the tray-specific instruction to macOS.</comment>
<file context>
@@ -530,31 +528,10 @@ export class HostServiceCoordinator extends EventEmitter {
- });
+ dialog.showErrorBox(
+ "Host service crashed",
+ `The Superset host service stopped unexpectedly (${cause}). Workspaces and terminals for this organization are unavailable until it restarts — use the Superset tray menu > Host Service > Restart.`,
+ );
}
</file context>
Suggested change
| `The Superset host service stopped unexpectedly (${cause}). Workspaces and terminals for this organization are unavailable until it restarts — use the Superset tray menu > Host Service > Restart.`, | |
| `The Superset host service stopped unexpectedly (${cause}). Workspaces and terminals for this organization are unavailable until it restarts — restart Superset to recover${process.platform === "darwin" ? ", or use the Superset tray menu > Host Service > Restart" : ""}.`, |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
…ondition Replaces the fragile error.message substring match with a TRPCError code so the auth-precondition filter is rename-proof (greptile P2).
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/host-service-coordinator.ts (1)
516-533: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
showMessageBoxif this dialog needs a Restart action
dialog.showErrorBoxonly renders a blocking OK dialog, so it can’t support the stated Restart flow. Switch toshowMessageBoxwith explicit buttons and handle the selected action from a layer that has restart context.🤖 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/main/lib/host-service-coordinator.ts` around lines 516 - 533, The crash alert in alertChildCrashed currently uses dialog.showErrorBox, which only supports a blocking OK dialog and cannot offer the Restart flow described in the message. Replace it with dialog.showMessageBox in host-service-coordinator.ts, and wire the response to a restart-capable handler outside alertChildCrashed so the user can choose Restart explicitly; keep the existing cause logging and message text, but move the action handling to the layer that has restart context.
🤖 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.
Outside diff comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 516-533: The crash alert in alertChildCrashed currently uses
dialog.showErrorBox, which only supports a blocking OK dialog and cannot offer
the Restart flow described in the message. Replace it with dialog.showMessageBox
in host-service-coordinator.ts, and wire the response to a restart-capable
handler outside alertChildCrashed so the user can choose Restart explicitly;
keep the existing cause logging and message text, but move the action handling
to the layer that has restart context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 363940bb-8c64-4747-b073-2442c0c61709
📒 Files selected for processing (3)
apps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/renderer/routes/_authenticated/providers/LocalHostServiceProvider/LocalHostServiceProvider.tsxpackages/host-service/src/db/db.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/routes/_authenticated/providers/LocalHostServiceProvider/LocalHostServiceProvider.tsx
- packages/host-service/src/db/db.ts
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/host-service-coordinator/index.ts (1)
20-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: Extract the duplicated unauthorized-error throw into a helper.
The same
TRPCError({ code: "UNAUTHORIZED", message: "No auth token available — user must be logged in" })block is repeated instart,restart, andreset. A small helper would keep the message and code in one place and make future changes (e.g., different message or code) a single-line edit.♻️ Suggested helper extraction
+function assertAuthToken(token: string | null): asserts token { + if (!token) { + throw new TRPCError({ + code: "UNAUTHORIZED", + message: "No auth token available — user must be logged in", + }); + } +}Then in each mutation:
const { token } = await loadToken(); -if (!token) { - throw new TRPCError({ - code: "UNAUTHORIZED", - message: "No auth token available — user must be logged in", - }); -} +assertAuthToken(token);Also applies to: 45-48, 60-63
🤖 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/host-service-coordinator/index.ts` around lines 20 - 23, The same unauthorized TRPCError throw is duplicated across start, restart, and reset in host-service-coordinator; extract it into a shared helper so the code and message live in one place. Add a small function or factory near the existing mutation handlers in index.ts, then replace each inline TRPCError({ code: "UNAUTHORIZED", message: "No auth token available — user must be logged in" }) with that helper to keep future edits centralized.
🤖 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/host-service-coordinator/index.ts`:
- Around line 20-23: The same unauthorized TRPCError throw is duplicated across
start, restart, and reset in host-service-coordinator; extract it into a shared
helper so the code and message live in one place. Add a small function or
factory near the existing mutation handlers in index.ts, then replace each
inline TRPCError({ code: "UNAUTHORIZED", message: "No auth token available —
user must be logged in" }) with that helper to keep future edits centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a01bfb04-ae86-4275-b7b0-4efc64496e99
📒 Files selected for processing (2)
apps/desktop/src/lib/trpc/routers/host-service-coordinator/index.tsapps/desktop/src/renderer/routes/_authenticated/providers/LocalHostServiceProvider/LocalHostServiceProvider.tsx
saddlepaddle
added a commit
that referenced
this pull request
Jul 9, 2026
… test #5493 added dialog.showErrorBox to the coordinator but the test file's electron mock only exported app, so the suite fails at import. Broke on main between the 17:17Z and 18:59Z CI runs.
saddlepaddle
added a commit
that referenced
this pull request
Jul 9, 2026
* feat(mobile): EAS TestFlight build and submit configuration - eas.json: preview/production profiles pull EXPO_PUBLIC_* from EAS environment variables; submit profile pinned to the App Store Connect app (ascAppId) and Apple team - app.config.ts: expo-image-picker plugin with photo library usage string (camera/microphone disabled), expo-document-picker plugin - store.config.json: EAS Metadata store listing config - assets/icon.png: flatten unused alpha channel (App Store rejects 1024px icons with alpha) - .env.example: document mobile EXPO_PUBLIC_* variables * feat(mobile): populate EXPO_PUBLIC_* env vars in local dev setup setup.local.sh and .env.local.example now provide the mobile app's required env vars. Electric points at the plain-HTTP electric-proxy wrangler server since React Native fetch rejects Caddy's self-signed certificate. Also switch App Review contact to support@superset.sh. * fix(desktop): add dialog to electron mock in host-service-coordinator test #5493 added dialog.showErrorBox to the coordinator but the test file's electron mock only exported app, so the suite fails at import. Broke on main between the 17:17Z and 18:59Z CI runs.
This was referenced Jul 14, 2026
This was referenced Jul 27, 2026
This was referenced Aug 5, 2026
Merged
This branch was previously deployed
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
Breaking host-service failures never reached the user. The confirmed 1.12.0 crash is the canonical case: a migration fails, the failure is swallowed, and the service limps on a half-migrated DB — then fails later in a confusing, unrelated way. Failures were being swallowed at multiple layers with no loud surface.
What changed
Three targeted fixes, each un-swallowing a breaking failure (legitimate best-effort catches — teardown, fan-out guards, optional features — were deliberately left alone):
db.ts— migration throws. Removed thetry/catchthat logged-and-continued aroundmigrate(), so a failed migration crashes the child at startup (before the safety net is installed →main().catch → exit(1)) with the real stack, instead of returning a half-migrated DB. Also addedbusy_timeout = 5000.host-service-coordinator.ts— alert on child crash. When a running child dies unexpectedly, the coordinator now shows a native dialog ("Host service crashed" with exit code/signal) and a Restart button, rather than only emitting a passivestoppedstatus event nothing reacts to. Deliberatestop()/quit set status tostoppedfirst, so they return early and never alert; startup-phase deaths are surfaced by (3), so no double-alert.LocalHostServiceProvider.tsx— surface start failures. The fire-and-forgetstartmutation now has anonErrorthat logs + toasts the failure instead of dropping the rejection into unread React Query state (auth-token preconditions filtered to avoid startup-race noise).Deliberately out of scope
reset()/ per-org separation).exit()on every uncaught error — crashing the whole session-supervisor on a stray post-startup throw, with no auto-restart, is worse than logging it.Verification
createDbtests pass.Summary by cubic
Stop swallowing breaking host-service failures and surface them clearly to users. Failed migrations now crash early, crashes show a native error dialog, and start errors show a toast.
packages/host-service/src/db/db.ts: Let failed migrations throw instead of returning a half‑migrated DB; setbusy_timeout = 5000.apps/desktop/src/main/lib/host-service-coordinator.ts: Show a native “Host service crashed” error box with exit code/signal on unexpected exits; emit the real previous status; alert only for crashes of running children; recovery via tray menu Restart.apps/desktop/src/renderer/.../LocalHostServiceProvider.tsx: AddonErrorto the start mutation to log and toast failures; ignore typedUNAUTHORIZEDauth‑token preconditions.apps/desktop/src/lib/trpc/routers/host-service-coordinator: ThrowTRPCErrorwith codeUNAUTHORIZEDwhen no auth token is present so the renderer can reliably filter expected preconditions.Written for commit c0b68b4. Summary will update on new commits.
Summary by CodeRabbit