fix(desktop): make the local runtime invisible in the normal flow - #5200
fix(desktop): make the local runtime invisible in the normal flow#5200haniakrim wants to merge 2 commits into
Conversation
BootCheckGate used to show a "Select a Runtime" picker (local vs. cloud) and per-failure screens (daemon detected, outdated, port conflict, ...) each with their own manual action buttons — on desktop there is only ever one real runtime (the embedded local core), so all of that was friction with no actual choice behind it. Desktop (isTauri()) now: - Never renders the picker. Mode is silently committed to `local` (mirrors the fallback oauthAuthReadiness.ts already used), logged once. - Auto-runs the same remediation a button used to require (daemon cleanup, core restart, port-conflict recovery) for up to 2 attempts before giving up, so a first-run daemon/outdated-core hiccup self-heals with no click. - Collapses every failure into one generic error screen with a single Retry button. A foreign (non-OpenHuman) process holding the port is the one case that never auto-resolves — killing an unrelated process without consent stays a human decision, surfaced in the error text instead of a dedicated button. - Welcome's "Select a Runtime" escape hatch is gone — there's no picker to return to anymore. Runtime crash mid-session (not just at boot) now also self-heals: DaemonHealthService's existing disconnect watchdog triggers one `restartCoreProcess()` attempt automatically instead of just flipping a status flag, and that restart now clears the cached RPC URL (not just the token) so a fallback-port restart is rediscovered instead of pinging a dead port forever. Web builds are unchanged — a URL/token is still real server configuration there (no local process to spawn), not a "runtime" choice. Rust core process lifecycle (start/reuse/restart/port-fallback/shutdown) was already solid and untouched; this is a frontend-only change. Co-authored-by: Orca <help@stably.ai>
Co-authored-by: Orca <help@stably.ai>
📝 WalkthroughWalkthroughDesktop now selects the embedded local runtime without showing a picker, consolidates boot failures into retry handling, adds guarded automatic core recovery, removes the Welcome runtime selector, and updates unit and E2E coverage while preserving web runtime selection. ChangesDesktop runtime flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
Pull request overview
This PR streamlines the desktop (Tauri) boot/runtime experience by removing the runtime-picker concept from the normal UX flow, automatically attempting self-healing remediation on boot failures, and improving mid-session recovery by restarting the embedded core when health snapshots stop arriving. It keeps the web build behavior intact, where URL/token selection remains genuine server configuration.
Changes:
- Desktop boot now auto-commits to local mode (no picker UI) and collapses failures into a single generic error + Retry after bounded auto-remediation attempts.
- Desktop disconnect watchdog now attempts an automatic core restart once a health timeout marks the daemon disconnected.
restartCoreProcess()now clears cached RPC URL as well as token to support fallback-port restarts, with tests updated accordingly.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
HANDOFF.md |
Adds a session handoff note describing the work and verification status. |
app/test/e2e/specs/runtime-picker-login.spec.ts |
Updates the desktop E2E spec to assert the runtime picker is never shown and keeps login/logout phases. |
app/src/services/daemonHealthService.ts |
Adds a desktop-only one-shot auto-restart attempt when the health watchdog times out. |
app/src/services/coreProcessControl.ts |
Clears cached RPC URL on core restart to rediscover fallback ports. |
app/src/services/__tests__/daemonHealthService.test.ts |
Adds unit tests covering the new desktop-only auto-restart behavior and non-reentrancy. |
app/src/services/__tests__/coreProcessControl.test.ts |
Adds coverage for clearing the RPC URL cache during restart. |
app/src/pages/Welcome.tsx |
Removes the “Select a Runtime” escape hatch button and its reset logic. |
app/src/pages/__tests__/Welcome.test.tsx |
Updates tests to assert the runtime escape hatch is not present. |
app/src/components/BootCheckGate/BootCheckGate.tsx |
Implements desktop no-picker flow, bounded auto-remediation, and a single generic desktop error screen. |
app/src/components/BootCheckGate/__tests__/BootCheckGate.test.tsx |
Refactors component tests into desktop vs web behaviors and updates assertions for the new UX. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| if (!isDesktop || coreMode.kind !== 'unset' || autoModeAppliedRef.current) { | ||
| return; |
| # Session Handoff | ||
|
|
||
| **Branch:** `feat/invisible-desktop-runtime` (pushed to fork, not merged) | ||
| **Date:** 2026-07-25 | ||
|
|
| it('auto-commits to local mode and runs the boot check without any click', async () => { | ||
| mockRunBootCheck.mockResolvedValue({ kind: 'match' }); | ||
|
|
||
| renderGate(); | ||
| fireEvent.click(screen.getByRole('button', { name: 'Continue' })); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByTestId('app-content')).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('BootCheckGate — daemonDetected', () => { | ||
| it('shows daemon detection screen', async () => { | ||
| mockRunBootCheck.mockResolvedValue({ kind: 'daemonDetected' }); | ||
|
|
||
| renderGate(); | ||
| fireEvent.click(screen.getByRole('button', { name: 'Continue' })); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Legacy Background Runtime Detected')).toBeInTheDocument(); | ||
| expect(screen.getByRole('button', { name: 'Remove and Continue' })).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('BootCheckGate — outdatedLocal', () => { | ||
| it('shows outdated local screen', async () => { | ||
| mockRunBootCheck.mockResolvedValue({ kind: 'outdatedLocal' }); | ||
|
|
||
| renderGate(); | ||
| fireEvent.click(screen.getByRole('button', { name: 'Continue' })); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Local Runtime Needs a Restart')).toBeInTheDocument(); | ||
| expect(screen.getByRole('button', { name: 'Restart Runtime' })).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('BootCheckGate — outdatedCloud', () => { | ||
| it('shows outdated cloud screen', async () => { | ||
| mockRunBootCheck.mockResolvedValue({ kind: 'outdatedCloud' }); | ||
|
|
||
| const store = makeStore({ kind: 'cloud', url: 'https://core.example.com/rpc' }); | ||
| // Trigger the check by rendering with an already-set mode | ||
| mockRunBootCheck.mockResolvedValue({ kind: 'outdatedCloud' }); | ||
| render( | ||
| <Provider store={store}> | ||
| <BootCheckGate> | ||
| <div data-testid="app-content">App Content</div> | ||
| </BootCheckGate> | ||
| </Provider> | ||
| expect(mockRunBootCheck).toHaveBeenCalledWith( | ||
| expect.objectContaining({ kind: 'local' }), | ||
| expect.any(Object) | ||
| ); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Cloud Runtime Needs an Update')).toBeInTheDocument(); | ||
| expect(screen.getByRole('button', { name: 'Update Cloud Runtime' })).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('BootCheckGate — noVersionMethod', () => { | ||
| it('shows no version method screen', async () => { | ||
| mockRunBootCheck.mockResolvedValue({ kind: 'noVersionMethod' }); | ||
|
|
||
| renderGate(); | ||
| fireEvent.click(screen.getByRole('button', { name: 'Continue' })); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Runtime Version Check Failed')).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('BootCheckGate — unreachable', () => { | ||
| it('shows unreachable screen with quit and switch mode buttons', async () => { | ||
| mockRunBootCheck.mockResolvedValue({ kind: 'unreachable', reason: 'Connection refused' }); | ||
|
|
||
| renderGate(); | ||
| fireEvent.click(screen.getByRole('button', { name: 'Continue' })); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText("Can't Reach the Runtime")).toBeInTheDocument(); | ||
| expect(screen.getByRole('button', { name: 'Quit' })).toBeInTheDocument(); | ||
| expect(screen.getByRole('button', { name: 'Pick a Different Runtime' })).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
|
|
||
| it("returns to picker when 'Pick a Different Runtime' is clicked", async () => { | ||
| mockRunBootCheck.mockResolvedValue({ kind: 'unreachable', reason: 'Connection refused' }); | ||
|
|
||
| renderGate(); | ||
| fireEvent.click(screen.getByRole('button', { name: 'Continue' })); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByRole('button', { name: 'Pick a Different Runtime' })).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| fireEvent.click(screen.getByRole('button', { name: 'Pick a Different Runtime' })); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Select a Runtime')).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('BootCheckGate — pre-set mode (subsequent launches)', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/services/daemonHealthService.ts (1)
175-181: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRe-arm the watchdog after automatic restart. After the timeout fires,
healthTimeoutIdis cleared andattemptAutoRecovery()only callsrestartCoreProcess(); it never callsensureWatchdogArmed()orstartHealthTimeout(). A restarted desktop core that never produces anotherapp_state_snapshottherefore stays without another watchdog unless another caller can arm it, so disconnect detection can get lost for that session.🤖 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 `@app/src/services/daemonHealthService.ts` around lines 175 - 181, The watchdog is not re-armed after automatic recovery, leaving restarted cores without disconnect detection. Update the timeout recovery flow in `attemptAutoRecovery()` and the surrounding watchdog methods so a successful automatic restart invokes the existing `ensureWatchdogArmed()` or `startHealthTimeout()` mechanism, while preserving the cleared timeout state before recovery.
🧹 Nitpick comments (5)
app/src/components/BootCheckGate/__tests__/BootCheckGate.test.tsx (2)
293-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLines 293-295 are a no-op. The
findByPlaceholderText(...).dispatchEvent(new Event('input'))call happens before any value is set and is immediately superseded bysetValue(urlInput, ...). Drop it and keep the awaited lookup only if you need it to gate on the picker being rendered.♻️ Proposed cleanup
- (await screen.findByPlaceholderText(/https:\/\/core\.example\.com/)).dispatchEvent( - new Event('input', { bubbles: true }) - ); - const urlInput = screen.getByPlaceholderText( + const urlInput = (await screen.findByPlaceholderText( /https:\/\/core\.example\.com/ - ) as HTMLInputElement; + )) as HTMLInputElement;🤖 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 `@app/src/components/BootCheckGate/__tests__/BootCheckGate.test.tsx` around lines 293 - 306, Remove the standalone input event dispatch from the initial findByPlaceholderText call in the BootCheckGate test, since it occurs before a value is assigned. Retain the awaited element lookup only if it is needed to wait for the picker to render, then use setValue for the actual URL update.
242-259: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis test doesn't actually prove the budget was reset.
mockRunBootCheckis switched to{ kind: 'match' }before the click, so a single check fromhandleRetrysatisfiescalls.length > callsBeforeRetryeven ifautoRecoveryAttemptsRefwere never zeroed. Keep the mock failing after the click and assert that the call count grows by the retry plusAUTO_RECOVERY_MAX_ATTEMPTSauto attempts.🤖 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 `@app/src/components/BootCheckGate/__tests__/BootCheckGate.test.tsx` around lines 242 - 259, The “Retry resets the auto-recovery budget” test must verify the retry-triggered auto-recovery attempts, not just a successful retry. In the test around renderGate and generic-retry-btn, keep mockRunBootCheck returning unreachable after the click, then assert the call count increases by the retry check plus AUTO_RECOVERY_MAX_ATTEMPTS auto-recovery checks; retain the initial failing cycle setup.app/src/components/BootCheckGate/BootCheckGate.tsx (2)
603-623: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
actionErroris dropped on desktop.GenericErrorScreennever rendersactionError, so a failed remediation (e.g.bootCheck.portConflictFixFailed, or a thrownrestart_core_process) leaves the user with only the stale genericdescribeGenericFailuretext. Consider threadingactionErrorin and preferring it over the derived message when set.🤖 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 `@app/src/components/BootCheckGate/BootCheckGate.tsx` around lines 603 - 623, The GenericErrorScreen component drops the remediation failure details held in actionError. Thread actionError into GenericErrorScreen from its caller and prefer displaying it over describeGenericFailure(result, t) when present, while retaining the derived message as the fallback.
863-882: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo delay between auto-recovery attempts. For a plain
unreachableresulthandleActionmatches no remediation branch and simply re-runsrunBootCheck, so both budgeted attempts fire back-to-back with zero backoff — a core that is merely slow to come up (cold start) will burn the budget in milliseconds and land on the generic error screen. A short delay (or an exponential step) before each auto attempt would make the retry budget actually useful.🤖 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 `@app/src/components/BootCheckGate/BootCheckGate.tsx` around lines 863 - 882, The auto-recovery effect around handleAction retries immediately for plain unreachable results, exhausting AUTO_RECOVERY_MAX_ATTEMPTS before a slow core can recover. Add a short backoff delay before invoking handleAction, while preserving the existing attempt counter and cap; ensure the delayed callback is cancelled on effect cleanup or dependency changes.app/src/services/__tests__/daemonHealthService.test.ts (1)
180-215: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing the case that matters: a second watchdog fire after the first restart settles. Both new tests keep the restart pending or resolve it at the very end, so nothing covers "restart completed, core still dead, watchdog fires again" — which is exactly where
autoRecoveryInFlightstops guarding (seeapp/src/services/daemonHealthService.tslines 183-206). Add a test that resolves the first restart, re-ingests, advances another window, and asserts the intended call count.🤖 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 `@app/src/services/__tests__/daemonHealthService.test.ts` around lines 180 - 215, The daemon health tests do not cover a second watchdog firing after the first restart completes. Add a test near the existing restart-guard tests that mocks the first restart as controllably resolved, ingests the initial health snapshot, advances one watchdog window, resolves the restart, re-ingests the disconnected state, advances another window, and asserts the intended restart call count for the still-dead core.
🤖 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 `@app/src/components/BootCheckGate/BootCheckGate.tsx`:
- Around line 851-882: Update the desktop foreign-owner path in BootCheckGate
and GenericErrorScreen so a result with foreignOwner exposes an explicit consent
action wired to handleForceQuit, alongside Retry. Preserve the current
auto-recovery exclusion for foreign port conflicts, and ensure the force-quit
control is available only when a foreign owner is present.
In `@app/src/services/daemonHealthService.ts`:
- Around line 183-206: Cap automatic recovery in attemptAutoRecovery to one
restart per health cycle, rather than only preventing concurrent calls. Add an
attempted-state guard that remains set after the restart completes or fails, and
reset that state in the next successful ingestHealthSnapshot so a later
unrelated disconnect can receive one automatic restart.
In `@HANDOFF.md`:
- Around line 3-4: Update the PR status in the HANDOFF.md branch/status section
to identify this work as PR `#5200` under review, replacing the indication that no
PR has been opened. Ensure the next-step guidance tells downstream reviewers to
continue reviewing PR `#5200` rather than reopen or duplicate it.
- Around line 25-28: Reconcile the pnpm test status in HANDOFF.md by accurately
distinguishing the 8799 passing tests from the two failures and stating whether
those failures are pre-existing and reproduced or exempted in CI. Clarify
whether Rust and E2E coverage actually ran and completed as release validation,
rather than implying the full suite passed when it did not.
---
Outside diff comments:
In `@app/src/services/daemonHealthService.ts`:
- Around line 175-181: The watchdog is not re-armed after automatic recovery,
leaving restarted cores without disconnect detection. Update the timeout
recovery flow in `attemptAutoRecovery()` and the surrounding watchdog methods so
a successful automatic restart invokes the existing `ensureWatchdogArmed()` or
`startHealthTimeout()` mechanism, while preserving the cleared timeout state
before recovery.
---
Nitpick comments:
In `@app/src/components/BootCheckGate/__tests__/BootCheckGate.test.tsx`:
- Around line 293-306: Remove the standalone input event dispatch from the
initial findByPlaceholderText call in the BootCheckGate test, since it occurs
before a value is assigned. Retain the awaited element lookup only if it is
needed to wait for the picker to render, then use setValue for the actual URL
update.
- Around line 242-259: The “Retry resets the auto-recovery budget” test must
verify the retry-triggered auto-recovery attempts, not just a successful retry.
In the test around renderGate and generic-retry-btn, keep mockRunBootCheck
returning unreachable after the click, then assert the call count increases by
the retry check plus AUTO_RECOVERY_MAX_ATTEMPTS auto-recovery checks; retain the
initial failing cycle setup.
In `@app/src/components/BootCheckGate/BootCheckGate.tsx`:
- Around line 603-623: The GenericErrorScreen component drops the remediation
failure details held in actionError. Thread actionError into GenericErrorScreen
from its caller and prefer displaying it over describeGenericFailure(result, t)
when present, while retaining the derived message as the fallback.
- Around line 863-882: The auto-recovery effect around handleAction retries
immediately for plain unreachable results, exhausting AUTO_RECOVERY_MAX_ATTEMPTS
before a slow core can recover. Add a short backoff delay before invoking
handleAction, while preserving the existing attempt counter and cap; ensure the
delayed callback is cancelled on effect cleanup or dependency changes.
In `@app/src/services/__tests__/daemonHealthService.test.ts`:
- Around line 180-215: The daemon health tests do not cover a second watchdog
firing after the first restart completes. Add a test near the existing
restart-guard tests that mocks the first restart as controllably resolved,
ingests the initial health snapshot, advances one watchdog window, resolves the
restart, re-ingests the disconnected state, advances another window, and asserts
the intended restart call count for the still-dead core.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b355fbec-d07d-421d-84f0-c28703c5eb7a
📒 Files selected for processing (10)
HANDOFF.mdapp/src/components/BootCheckGate/BootCheckGate.tsxapp/src/components/BootCheckGate/__tests__/BootCheckGate.test.tsxapp/src/pages/Welcome.tsxapp/src/pages/__tests__/Welcome.test.tsxapp/src/services/__tests__/coreProcessControl.test.tsapp/src/services/__tests__/daemonHealthService.test.tsapp/src/services/coreProcessControl.tsapp/src/services/daemonHealthService.tsapp/test/e2e/specs/runtime-picker-login.spec.ts
💤 Files with no reviewable changes (1)
- app/src/pages/Welcome.tsx
| // ------------------------------------------------------------------ | ||
| // Desktop: automatically run the same remediation `handleAction` performs | ||
| // on a button click — daemon cleanup, core restart, port-conflict | ||
| // recovery — without waiting for the user to press anything. A foreign | ||
| // (non-OpenHuman) process holding the port is the one case this never | ||
| // auto-resolves: killing an unrelated process without consent is exactly | ||
| // the kind of destructive action a human must approve, so that still | ||
| // waits for an explicit click even on desktop (surfaced within the | ||
| // generic error screen below). Capped at AUTO_RECOVERY_MAX_ATTEMPTS so a | ||
| // core that can never come up still resolves to a single visible error | ||
| // instead of retrying silently forever. | ||
| // ------------------------------------------------------------------ | ||
| useEffect(() => { | ||
| if (!isDesktop || phase !== 'result' || !result || result.kind === 'match') { | ||
| return; | ||
| } | ||
| const isForeignPortConflict = result.kind === 'unreachable' && Boolean(result.foreignOwner); | ||
| if (isForeignPortConflict) return; | ||
| if (autoRecoveryAttemptsRef.current >= AUTO_RECOVERY_MAX_ATTEMPTS) return; | ||
| autoRecoveryAttemptsRef.current += 1; | ||
| log( | ||
| '[boot-check] gate — desktop: auto-recovery attempt %d/%d for result=%s', | ||
| autoRecoveryAttemptsRef.current, | ||
| AUTO_RECOVERY_MAX_ATTEMPTS, | ||
| result.kind | ||
| ); | ||
| void handleAction(); | ||
| // handleAction is intentionally excluded: it changes identity on every | ||
| // result update (its own deps include `result`), which would re-run this | ||
| // effect before the attempt counter above ever gets a chance to cap it. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isDesktop, phase, result]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Desktop foreign port-conflict is now a dead end. The comment says the foreign-owner case "still waits for an explicit click even on desktop (surfaced within the generic error screen below)", but GenericErrorScreen renders only Retry — there is no path to handleForceQuit, and Retry just re-runs the same check that will keep returning the same foreignOwner result. The test at BootCheckGate.test.tsx lines 224-240 locks this in (one button, forceQuitPortOwner never called), so a desktop user whose port is held by an unrelated process has no remedy at all.
Either surface the consent force-quit button in the desktop screen when result.foreignOwner is present, or update the comment to state that the case is deliberately unrecoverable from the UI.
🛠️ Sketch: pass the foreign-owner consent action into the desktop screen
if (isDesktop) {
return (
<GenericErrorScreen
result={result ?? { kind: 'unreachable', reason: 'Unknown error' }}
onRetry={handleManualRetry}
+ onForceQuit={handleForceQuit}
retrying={actionBusy}
/>
);
}🤖 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 `@app/src/components/BootCheckGate/BootCheckGate.tsx` around lines 851 - 882,
Update the desktop foreign-owner path in BootCheckGate and GenericErrorScreen so
a result with foreignOwner exposes an explicit consent action wired to
handleForceQuit, alongside Retry. Preserve the current auto-recovery exclusion
for foreign port conflicts, and ensure the force-quit control is available only
when a foreign owner is present.
| /** | ||
| * One-shot, desktop-only self-heal for a core that goes quiet mid-session | ||
| * (crashed, killed, wedged) — the counterpart to BootCheckGate's own | ||
| * auto-recovery, which only ever runs at startup. Web builds talk to a | ||
| * core they don't own the lifecycle of, so there is nothing local to | ||
| * restart there. Not re-entrant: if a restart is already in flight when | ||
| * the watchdog fires again, this is a no-op rather than a pile-up of | ||
| * concurrent `restart_core_process` calls. | ||
| */ | ||
| private async attemptAutoRecovery(): Promise<void> { | ||
| if (!isTauri() || this.autoRecoveryInFlight) { | ||
| return; | ||
| } | ||
| this.autoRecoveryInFlight = true; | ||
| console.warn('[DaemonHealth] core disconnected — attempting automatic restart'); | ||
| try { | ||
| await restartCoreProcess(); | ||
| console.debug('[DaemonHealth] automatic core restart invoked successfully'); | ||
| } catch (error) { | ||
| console.error('[DaemonHealth] automatic core restart failed:', error); | ||
| } finally { | ||
| this.autoRecoveryInFlight = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
"One-shot" isn't enforced — this is single-flight, not capped. autoRecoveryInFlight is cleared in finally, so every subsequent watchdog fire triggers another restartCoreProcess(). A core that keeps dying yields an unbounded restart loop (one every HEALTH_TIMEOUT_MS), each of which also invalidates the RPC token/URL caches and disrupts live consumers. Both the module doc (lines 12-14) and the PR description say a single automatic restart; BootCheckGate caps itself with AUTO_RECOVERY_MAX_ATTEMPTS, and this path has no equivalent.
🛡️ Proposed attempt cap
private autoRecoveryInFlight = false;
+ private autoRecoveryAttempted = false;
@@
private async attemptAutoRecovery(): Promise<void> {
- if (!isTauri() || this.autoRecoveryInFlight) {
+ if (!isTauri() || this.autoRecoveryInFlight || this.autoRecoveryAttempted) {
return;
}
+ this.autoRecoveryAttempted = true;
this.autoRecoveryInFlight = true;(Reset autoRecoveryAttempted on the next successful ingestHealthSnapshot so a later, unrelated disconnect still gets one attempt.)
🤖 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 `@app/src/services/daemonHealthService.ts` around lines 183 - 206, Cap
automatic recovery in attemptAutoRecovery to one restart per health cycle,
rather than only preventing concurrent calls. Add an attempted-state guard that
remains set after the restart completes or fails, and reset that state in the
next successful ingestHealthSnapshot so a later unrelated disconnect can receive
one automatic restart.
| **Branch:** `feat/invisible-desktop-runtime` (pushed to fork, not merged) | ||
| **Date:** 2026-07-25 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the PR status.
This is PR #5200, but the handoff says no PR has been opened. Update the branch/next-step status so downstream reviewers do not reopen or duplicate the PR. As per PR objectives, this review is for PR #5200.
Also applies to: 39-39
🤖 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 `@HANDOFF.md` around lines 3 - 4, Update the PR status in the HANDOFF.md
branch/status section to identify this work as PR `#5200` under review, replacing
the indication that no PR has been opened. Ensure the next-step guidance tells
downstream reviewers to continue reviewing PR `#5200` rather than reopen or
duplicate it.
| - `npx eslint` clean on every changed file (repo-wide `pnpm lint` has ~60-70 pre-existing errors/warnings in unrelated files — not touched, not introduced by this session). | ||
| - `npx tsc --noEmit` — clean, 0 errors. | ||
| - `pnpm test` (full suite) — 8799 passed, 2 pre-existing failures unrelated to this work (`navConfig.test.ts`, `desktopDeepLinkListener.test.ts` — both expect `VITE_BILLING_DASHBOARD_URL` to resolve to `https://tinyhumans.ai/dashboard`, but that env var isn't set in this local shell so it falls back to the dev placeholder; pre-existing from the prior `f95e8c3d4` commit, not something this session touched). | ||
| - Pre-push hook (full repo build + lint + format + Rust checks) passed; branch pushed to `fork/feat/invisible-desktop-runtime`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)HANDOFF\.md$|(^|/)README|PR|PR_OBJECTIVES|objectives|Handoff|handoff' || true
echo "== HANDOFF relevant section =="
if [ -f HANDOFF.md ]; then
nl -ba HANDOFF.md | sed -n '1,80p'
fi
echo "== search PR objectives / full suite passed =="
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'full[test| suite| test suite]|test suite|8799 passed|70 pre-existing errors|VITE_BILLING_DASHBOARD_URL|desktopDeepLinkListener|navConfig.test' . || trueRepository: tinyhumansai/openhuman
Length of output: 5297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo summary =="
git rev-parse --show-toplevel
git status --short
git diff --stat || true
git log --oneline -5 || true
echo "== tracked HANDOFF candidates =="
git ls-files | rg -i '(^|/)(handoff|hand-off|handoff|changes|changelog|objectives|target|release|validation|readme|pr|pr_).*\.md$|(^|/)README.*\.md$' || trueRepository: tinyhumansai/openhuman
Length of output: 8450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== HANDOFF.md line numbers and relevant section =="
awk '{printf "%5d %s\n", NR, $0}' HANDOFF.md | sed -n '1,80p'
echo "== HEAD summary =="
git show --stat HEAD
echo "== search objective/validation/test-status mentions in repo text =="
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'PR_OBJECTIVES|PR objectives|objectives|full suite passed|full test suite|full suite|test suite passed|8799 passed|70 pre-existing|VITE_BILLING_DASHBOARD_URL|desktopDeepLinkListener|navConfig\.test|pnpm test' $(git ls-files | tr '\n' ' ')Repository: tinyhumansai/openhuman
Length of output: 35994
Reconcile the pnpm test pass/fail status.
HANDOFF says pnpm test is a full suite with 8799 passed, but it also records two failures. Align this entry with the actual test command/result, including whether the failures are pre-existing/released from CI and whether Rust/E2E coverage completed as release validation.
🤖 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 `@HANDOFF.md` around lines 25 - 28, Reconcile the pnpm test status in
HANDOFF.md by accurately distinguishing the 8799 passing tests from the two
failures and stating whether those failures are pre-existing and reproduced or
exempted in CI. Clarify whether Rust and E2E coverage actually ran and completed
as release validation, rather than implying the full suite passed when it did
not.
|
| Filename | Overview |
|---|---|
| HANDOFF.md | New file: AI session handoff document committed to the repo — contains internal agent notes, unverified steps, and internal debugging guidance that do not belong in production source control. |
| app/src/components/BootCheckGate/BootCheckGate.tsx | Desktop path now silently commits to local mode and auto-remediates failures up to 2 times before collapsing to a single GenericErrorScreen; web path is unchanged. Logic is correct; one minor issue: actionError set by handleAction on failure is never displayed in GenericErrorScreen. |
| app/src/services/daemonHealthService.ts | Adds desktop-only one-shot auto-restart on watchdog disconnect; re-entrancy guard prevents concurrent restarts. Unlike BootCheckGate's 2-attempt cap, this path retries indefinitely (one restart per 120s watchdog fire) which is a design divergence from 'one automatic attempt' in the PR description. |
| app/src/services/coreProcessControl.ts | Adds clearCoreRpcUrlCache() call after restart so fallback-port restarts are rediscovered rather than pinging a stale cached URL. Minimal, correct change. |
| app/src/components/BootCheckGate/tests/BootCheckGate.test.tsx | Desktop picker tests replaced with auto-selection/auto-recovery assertions; web picker tests moved to dedicated describe. Good coverage for the happy path and retry-budget reset. CTA link test retained but drops the target=_blank and rel=noopener assertions from the original. |
| app/src/pages/Welcome.tsx | Removes the 'Select a Runtime' escape-hatch button and all associated imports/handler. Clean removal; 'Continue Locally' auth path untouched. |
| app/test/e2e/specs/runtime-picker-login.spec.ts | Phase 1 rewritten from 'drive the picker' to 'assert picker never shows.' Phases 2 and 3 (login/logout) unchanged. Note: updated spec not yet executed per PR description. |
| app/src/pages/tests/Welcome.test.tsx | Removes tests for the deleted 'Select a Runtime' button and its state-clearing behavior; renames describe block. Correctly mirrors the Welcome.tsx change. |
| app/src/services/tests/coreProcessControl.test.ts | Adds mock for clearCoreRpcUrlCache and a new test asserting it's called after restart. Correctly mirrors the production change. |
| app/src/services/tests/daemonHealthService.test.ts | New describe covers auto-restart on desktop, web no-op, re-entrancy guard, and error swallowing. Solid coverage for the new behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([App mounts — BootCheckGate]) --> B{isTauri?}
B -- No web --> C[phase = picker\ncoreMode = unset]
C --> D[ModePicker shown\nuser enters URL + token]
D --> E[runCheck cloud mode]
E --> F{result?}
F -- match --> G([Children rendered])
F -- failure --> H[ResultScreen\nper-kind buttons]
B -- Yes desktop --> I[phase = checking\nauto-mode effect fires]
I --> J[dispatch setCoreMode local\nstoreRpcUrl ''\nclearCaches]
J --> K[runCheck local mode]
K --> L{result?}
L -- match --> G
L -- failure non-foreign --> M{autoRecoveryAttempts < 2?}
M -- Yes --> N[handleAction\ndaemon cleanup / core restart / port recovery]
N --> K
M -- No --> O[GenericErrorScreen\nRetry button only]
L -- failure foreignOwner --> O
O -- user clicks Retry --> P[reset counter to 0\nrunCheck again]
P --> L
subgraph Mid-session
Q([DaemonHealthService\n120s watchdog fires]) --> R{isTauri?}
R -- Yes --> S{autoRecoveryInFlight?}
S -- No --> T[restartCoreProcess\nclearToken + clearUrl]
T --> U([New snapshots re-arm watchdog])
S -- Yes --> V([no-op])
R -- No --> V
end
Comments Outside Diff (3)
-
HANDOFF.md, line 1-46 (link)AI session artifact committed to production source
HANDOFF.mdis an internal agent session document — it describes what the AI did, what wasn't manually verified ("No live Claude-in-Chrome verification"), and lists next steps for a follow-up session. It has no value as production source and actively telegraphs unverified states ("The updated E2E spec was rewritten … but not executed — flag this before treating it as passing") to any reviewer who might miss the caveat. This file should be removed from the PR and added to.gitignoreif the agent workflow generates it routinely.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
app/src/components/BootCheckGate/BootCheckGate.tsx, line 96-110 (link)actionErroris silently dropped in the desktop generic error screenWhen
handleActioncatches an exception it callssetActionError(...), butGenericErrorScreennever rendersactionError— it only showsdescribeGenericFailure(result, t). So if the remediation step itself fails (e.g., therestart_core_processTauri command throws, or theservice_stopRPC rejects), the user sees the same generic description with no indication that the fix attempt also failed. The webResultScreendoes exposeactionError; adding it here would keep the desktop flow consistent and aid manual diagnosis. -
app/src/components/BootCheckGate/__tests__/BootCheckGate.test.tsx, line 1211-1222 (link)target="_blank"andrel="noopener"assertions dropped from the CTA link testThe original test verified both
link?.getAttribute('target') === '_blank'andlink?.getAttribute('rel')containing"noopener"— these are the security-relevant attributes that prevent tab-napping via the external GitHub releases link. The new test only checks thehrefpattern. If the production component loses those attributes in a future change, no test will catch it.
Reviews (1): Last reviewed commit: "docs: refresh session handoff for invisi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| useEffect(() => { | ||
| if (!isDesktop || phase !== 'result' || !result || result.kind === 'match') { | ||
| return; | ||
| } | ||
| const isForeignPortConflict = result.kind === 'unreachable' && Boolean(result.foreignOwner); | ||
| if (isForeignPortConflict) return; | ||
| if (autoRecoveryAttemptsRef.current >= AUTO_RECOVERY_MAX_ATTEMPTS) return; | ||
| autoRecoveryAttemptsRef.current += 1; | ||
| log( | ||
| '[boot-check] gate — desktop: auto-recovery attempt %d/%d for result=%s', | ||
| autoRecoveryAttemptsRef.current, | ||
| AUTO_RECOVERY_MAX_ATTEMPTS, | ||
| result.kind | ||
| ); | ||
| void handleAction(); | ||
| // handleAction is intentionally excluded: it changes identity on every | ||
| // result update (its own deps include `result`), which would re-run this | ||
| // effect before the attempt counter above ever gets a chance to cap it. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isDesktop, phase, result]); |
There was a problem hiding this comment.
actionBusy bail-out
autoRecoveryAttemptsRef.current is incremented unconditionally before void handleAction() is called, but handleAction immediately returns without doing anything if actionBusy is true (if (!result || actionBusy) return). In practice this path is very unlikely — actionBusy should be false whenever phase transitions back to 'result' — but if it ever fires while an action is still in flight, one recovery budget slot is silently consumed without an actual attempt, and the effect won't re-fire (its deps [isDesktop, phase, result] didn't change). Consider only incrementing after confirming actionBusy is false, or checking the flag here before calling.
| private async attemptAutoRecovery(): Promise<void> { | ||
| if (!isTauri() || this.autoRecoveryInFlight) { | ||
| return; | ||
| } | ||
| this.autoRecoveryInFlight = true; | ||
| console.warn('[DaemonHealth] core disconnected — attempting automatic restart'); | ||
| try { | ||
| await restartCoreProcess(); | ||
| console.debug('[DaemonHealth] automatic core restart invoked successfully'); | ||
| } catch (error) { | ||
| console.error('[DaemonHealth] automatic core restart failed:', error); | ||
| } finally { | ||
| this.autoRecoveryInFlight = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
attemptAutoRecovery — diverges from the PR description
The PR description says the watchdog "triggers one automatic restartCoreProcess() attempt on desktop." The re-entrancy guard (autoRecoveryInFlight) prevents concurrent restarts, but once that attempt settles (the finally block clears the flag), the next watchdog fire (120 s later) will attempt another restart. If the core crashes repeatedly this cycles indefinitely — unlike BootCheckGate's AUTO_RECOVERY_MAX_ATTEMPTS = 2 cap that surfaces the error screen after two failed tries. That may be intentional (mid-session crashes are transient and worth retrying indefinitely), but it's worth explicitly documenting the divergence since the PR description implies a single attempt.
Summary
BootCheckGateno longer shows a runtime picker on desktop (isTauri()). Mode is silently committed tolocalon mount (mirrors the fallback already used inoauthAuthReadiness.ts), logged once.daemonHealthService's existing disconnect watchdog now also triggers one automaticrestartCoreProcess()attempt on desktop instead of just flipping a status flag, so a core that goes quiet mid-session recovers without the user needing to relaunch.restartCoreProcess()now also clears the cached RPC URL (not just the token), so a restart landing on a fallback port (existing Rust-side port-fallback logic) is rediscovered instead of pinging a dead port forever.test/e2e/specs/runtime-picker-login.spec.ts(WDIO/Appium) to match: Phase 1 now asserts the picker is never shown instead of driving it; login/logout phases unchanged.No Rust changes — the embedded core's lifecycle (start/reuse/restart/port-fallback/stale-listener takeover/shutdown-on-quit) was already solid; this is frontend-only.
Test plan
npx eslintclean on every changed filenpx tsc --noEmit— 0 errorspnpm test(full suite) — 8799 passed; 2 pre-existing failures unrelated to this change (missingVITE_BILLING_DASHBOARD_URLenv var locally, from a prior unrelated commit)runtime-picker-login.spec.ts) — spec source updated to match the new behavior, but not executed here (no Appium/tauri-driver harness available in this sandboxed session); CI's own desktop E2E lane only runs against thereleasebranch per this repo's two-lane model, not on thismain-targeted PR.Made with Orca 🐋
Summary by CodeRabbit
New Features
Bug Fixes
Compatibility