fix(gui): clear a stale install failure once that action succeeded (#1245) - #1275
Conversation
…1245) The Startup Safety page kept showing "Installation failed" after startup protection was already in place. A user who hit a failing install and then fixed it another way saw "Restart protected" and "Installation failed" at the same time, with no way to dismiss the contradiction. installResult was only reset when the page itself started an action, so a refresh replaced the health data underneath a notice describing a past attempt. fetchStartup now retires a failed notice only when health independently shows that attempt's goal is met. Three conditions, each added after reproducing a case where a looser rule hid something true: - per action, not overall status: a machine protected by the service must not erase a failed shim install, which is still true and actionable - serviceViable, not installed-and-running: a stale or conflicting service can be both while the page reports it unhealthy, so a failed repair would vanish while the UI still said Stale - native routing retires only installs made under a local routing dependency, recorded on the error at action time; a native machine is still offered the shim button, so an optional shim failure there stands Success notices are left alone - those are the receipt for an action the user just ran. Five regressions drive the real page through button clicks and fetch responses: two assert clearing, three assert survival. Every branch has a test that fails when that branch is removed or widened. Buttons are selected by accessible name because both installs read "Install" and only aria-label separates them. gui: 684 pass / 0 fail; lint:gui and typecheck clean.
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughStartup install failures now record local-routing context. Health refreshes clear failures only when the matching service, shim, or routing condition is healthy. New tests verify clearing and retention across recovery and native-routing transitions. ChangesStartup reconciliation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant StartupPage
participant StartupHealthAPI
participant InstallResultState
StartupPage->>StartupHealthAPI: Request startup health
StartupHealthAPI-->>StartupPage: Return service, shim, and routing status
StartupPage->>InstallResultState: Reconcile failed install notices
InstallResultState-->>StartupPage: Retain or clear notices by matching health state
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@gui/tests/startup-install-result-reconciliation.test.tsx`:
- Around line 157-181: Add a test alongside the existing shim-install
reconciliation test that first records a failed shim installation, then
refreshes with both shimInstalled and shimHealthy set to true, and asserts the
shim failure notice is removed. Reuse the existing mount, click, settle, and
cleanup helpers and target the reconciliation behavior in Startup.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f29cca6a-d786-46ac-ab94-e69b840881a5
📒 Files selected for processing (2)
gui/src/pages/Startup.tsxgui/tests/startup-install-result-reconciliation.test.tsx
| test("a failed shim install survives a refresh that only proves the service is healthy (#1245)", async () => { | ||
| let health = atRiskHealth(); | ||
| const root = await mount(() => health, () => Response.json({ error: "shim install failed" }, { status: 500 })); | ||
|
|
||
| await act(async () => { | ||
| clickTarget(/shim.*install/i).dispatchEvent(new testWindow.Event("click", { bubbles: true })); | ||
| }); | ||
| await settle(); | ||
| expect(container().textContent).toContain("shim install failed"); | ||
|
|
||
| // Restart safety is now covered BY THE SERVICE. The shim is still not installed, | ||
| // so the shim failure is still true — clearing it here would hide a real problem | ||
| // behind an unrelated success. | ||
| health = { ...atRiskHealth(), status: "protected", protection: "service", serviceInstalled: true, serviceViable: true, serviceEnabled: true, serviceRunning: true, rebootSafe: true }; | ||
| await act(async () => { | ||
| clickTarget(/refresh/i).dispatchEvent(new testWindow.Event("click", { bubbles: true })); | ||
| }); | ||
| await settle(); | ||
|
|
||
| expect(container().textContent).toContain("shim install failed"); | ||
|
|
||
| await act(async () => { root.unmount(); }); | ||
| containerEl!.remove(); | ||
| containerEl = null; | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a successful shim-reconciliation test.
gui/src/pages/Startup.tsx Lines 126-131 clear an install-shim failure only when shimInstalled and shimHealthy are both true. This test verifies retention when only the service becomes viable, but it does not verify that the shim failure clears after the shim becomes healthy.
Add a test that fails a shim install, refreshes with both fields true, and asserts that the failure notice is removed. This protects the action-specific shim predicate from regression.
🤖 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 `@gui/tests/startup-install-result-reconciliation.test.tsx` around lines 157 -
181, Add a test alongside the existing shim-install reconciliation test that
first records a failed shim installation, then refreshes with both shimInstalled
and shimHealthy set to true, and asserts the shim failure notice is removed.
Reuse the existing mount, click, settle, and cleanup helpers and target the
reconciliation behavior in Startup.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44a0040138
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| setInstallResult(current => { | ||
| if (current?.kind !== "error") return current; |
There was a problem hiding this comment.
When a user clicks Refresh while a long-running install request is still pending, the refreshed health can report the action's goal as satisfied before the POST eventually returns an error. This updater then sees installResult === null and does nothing; the later catch stores the failure, leaving the page showing healthy state alongside the stale error until another refresh. The Refresh button is disabled only by loading, not installBusy, so this ordering is possible. Reconcile whenever either health or the failure changes (or compare the failure against the latest health when storing it), and add a test where the health response completes before the failed action response.
Useful? React with 👍 / 👎.
1 participant
Summary
The Startup Safety page kept showing "Installation failed" after startup protection was already in place. A user who hit a failing install and then fixed it another way — the CLI, the service manager, a retry the page did not initiate — saw "Restart protected" and "Installation failed" at the same time, with no way to dismiss the contradiction.
installResultwas only reset when the page itself started an action, so a refresh replaced the health data underneath a notice describing a past attempt.fetchStartupnow retires a failed notice only when health independently shows that attempt's goal is met. Three conditions, each added after reproducing a case where a looser rule hid something true:serviceViable, not installed-and-running — a stale or conflicting service can be both while the page reports it unhealthy, so a failed repair would vanish while the UI still said "Stale"Success notices are left alone — those are the receipt for an action the user just ran.
Closes #1245.
GUI change
Before — a failed install, then the service comes up another way. Both messages on screen:
After — the stale failure is gone, and the shim correctly still reads "Not installed":
Verification
bun test tests(gui) — 684 pass / 0 failbun run lint:gui— cleanbun run typecheck— cleanChecklist
Summary by CodeRabbit
Bug Fixes
Tests