chore(gui): React Doctor 100, 0.9.2 pin, block findings - #589
Conversation
Clear the post-cleanup regressions, pin react-doctor 0.9.2, and fail CI/prepush on any doctor finding so the score cannot silently drift again.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/src/components/MemoryObservabilityCard.tsx (1)
238-260: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNon-2xx
/healthzresponses never trigger the give-up timeout — reconnect can hang forever.Line 240 collapses
!res.okandcancelledinto a single early return, so a resolved-but-not-ok response (e.g. a fronting proxy returning 502/503 while the backend restarts) exits the.thenhandler without ever checkingRECONNECT_GIVE_UP_MS. The give-up-to-error transition only exists in the success branch (line 257) and in.catch()(line 264), which only fires on network-level rejection, not on a resolved non-ok response. If the proxy keeps returning non-ok status codes,restartPhasestays"reconnecting"indefinitely, the restart button stays disabled (busyat line 324), and the user never seesdash.mem.restartFailed.🐛 Proposed fix: apply the give-up check to non-ok responses too
void fetch(`${apiBase}/healthz`, { cache: "no-store", signal }) .then(async (res) => { - if (!res.ok || cancelled) return; + if (cancelled) return; + if (!res.ok) { + if (Date.now() - started >= RECONNECT_GIVE_UP_MS) { + setRestartPhase("error"); + setRestartError(t("dash.mem.restartFailed")); + } + return; + } let replaced = restartFromPid == null;🤖 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/src/components/MemoryObservabilityCard.tsx` around lines 238 - 260, Update the health-check callback in the restart/reconnect flow so resolved non-2xx responses also evaluate the RECONNECT_GIVE_UP_MS timeout and transition to restartPhase "error" with the existing dash.mem.restartFailed message. Keep the cancelled early return intact, and preserve the existing success/replacement handling for ok responses.
🤖 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/src/components/MemoryObservabilityCard.tsx`:
- Around line 175-179: Extract the duplicated AbortSignal feature detection into
a shared boundedSignal helper accepting an AbortController and timeout in
milliseconds. Replace both signal-construction blocks in the polling logic with
this helper, preserving the existing 10_000 and 5_000 timeout values and
fallback behavior.
In `@gui/src/pages/ClaudeDesktop.tsx`:
- Around line 211-219: Update the poll function and its cleanup logic in
ClaudeDesktop so status requests cannot overlap: track the active request with
an AbortController, skip a new poll while one is in flight (or abort the
previous request), pass its signal to fetch, and abort/clear the controller when
cancelled during cleanup. Preserve the existing cancelled guard and status
update behavior.
In `@gui/src/pages/Storage.tsx`:
- Around line 492-495: Update the error handling around the Storage page’s
response check so backend error codes such as “not_found” and “list_failed” are
mapped to the appropriate i18n key, including storage.trash.listFailed, before
localizedCatch renders them. Add or reuse the corresponding translation entries
in the locale files and ensure no raw backend code is exposed to users.
- Around line 1051-1057: Update the percent field state and its onChange handler
to preserve the raw input as a string, including an empty draft, instead of
rejecting blank values or converting immediately with Number. Ensure buildBody()
remains responsible for numeric validation and savePolicy() submits the draft so
cleared or invalid input is reported rather than silently retaining the previous
percentage.
In `@scripts/doctor-gui-if-changed.ts`:
- Around line 81-90: Separate unavailable-command failures from genuine React
Doctor results in scripts/doctor-gui-if-changed.ts around the catch handling:
soft-skip unavailable or offline execution with exit code 0, while propagating
only the nonzero status from a successfully launched doctor process. Update
tests/ci-workflows.test.ts lines 1992-2004 to assert zero for the unavailable
case and use a successfully launched command that exits 1 to verify findings
gate the push.
In `@tests/ci-workflows.test.ts`:
- Around line 1915-1933: Update the React Doctor workflow test to inspect active
YAML rather than comments, especially the blocking input assertion, so it only
matches the action’s configured value. In the test around “React Doctor workflow
is SHA-pinned, engine-pinned, gating, and read-only,” replace the
mutable-reference check with validation that every active uses: reference ends
in a full 40-character commit SHA, rejecting refs such as dev and latest while
preserving the existing pin assertions.
---
Outside diff comments:
In `@gui/src/components/MemoryObservabilityCard.tsx`:
- Around line 238-260: Update the health-check callback in the restart/reconnect
flow so resolved non-2xx responses also evaluate the RECONNECT_GIVE_UP_MS
timeout and transition to restartPhase "error" with the existing
dash.mem.restartFailed message. Keep the cancelled early return intact, and
preserve the existing success/replacement handling for ok responses.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2cd39c6a-562d-4245-ad7c-0431d6f7eac2
📒 Files selected for processing (14)
.github/workflows/react-doctor.ymlgui/README.mdgui/doctor.config.jsongui/package.jsongui/src/combo-workspace-data.tsgui/src/components/MemoryObservabilityCard.tsxgui/src/components/provider-workspace/AnthropicAccountPoolSettings.tsxgui/src/pages/Claude.tsxgui/src/pages/ClaudeDesktop.tsxgui/src/pages/Grok.tsxgui/src/pages/Logs.tsxgui/src/pages/Storage.tsxscripts/doctor-gui-if-changed.tstests/ci-workflows.test.ts
Keep Response-like test doubles working, preserve Grok/message error text, honor reconnect deadlines on non-OK healthz, and soft-skip offline npx/registry failures without letting real findings through.
Extract boundedSignal, serialize Claude Desktop status polls, keep storage percent as a draft string, map trash list errors through i18n, and tighten React Doctor workflow assertions to active YAML.
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/src/components/MemoryObservabilityCard.tsx (1)
235-246: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCap reconnect polling by the remaining give-up window.
boundedSignal(controller, 5_000)can keep/healthzin flight pastRECONNECT_GIVE_UP_MS, andinFlightblocks the next tick from advancing the phase sooner. Compute the remaining time before each request and clamp the abort timeout to it (or drive the phase with a dedicated deadline timer) so"reconnecting"cannot outlive the configured deadline atgui/src/components/MemoryObservabilityCard.tsx:235-274.🤖 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/src/components/MemoryObservabilityCard.tsx` around lines 235 - 246, Update the reconnect polling flow around the healthz fetch and its inFlight handling to compute the remaining RECONNECT_GIVE_UP_MS window before each request, then clamp the AbortSignal timeout to that remaining duration instead of always using 5 seconds. Ensure an in-flight request cannot delay the deadline transition, so the reconnecting phase changes to error by the configured give-up deadline.
🤖 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/fetch-json.test.ts`:
- Around line 27-34: Extend the test “readJsonOrThrow prefers error, then
message, then fallback” to assert that readJsonOrThrow rejects with the supplied
fallback when the response contains neither a usable error nor message string.
Preserve the existing precedence assertions.
In `@scripts/doctor-gui-if-changed.ts`:
- Around line 22-24: Restrict looksLikeDoctorInfraFailure to concrete engine,
command, or registry error codes/signatures by removing bare-word matches such
as network, registry, offline, SSL, and TLS while retaining explicit patterns.
In tests/ci-workflows.test.ts at lines 1969-1972, add a negative classifier
assertion showing findings text such as “Network requests > 1 errors” is not
treated as infrastructure failure and therefore remains gating.
---
Outside diff comments:
In `@gui/src/components/MemoryObservabilityCard.tsx`:
- Around line 235-246: Update the reconnect polling flow around the healthz
fetch and its inFlight handling to compute the remaining RECONNECT_GIVE_UP_MS
window before each request, then clamp the AbortSignal timeout to that remaining
duration instead of always using 5 seconds. Ensure an in-flight request cannot
delay the deadline transition, so the reconnecting phase changes to error by the
configured give-up deadline.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 83b22aea-e07e-448a-842f-776773e4c9ff
📒 Files selected for processing (7)
gui/src/components/MemoryObservabilityCard.tsxgui/src/fetch-json.tsgui/tests/fetch-json.test.tsscripts/doctor-gui-if-changed.tsscripts/fixtures/doctor-findings-exit.tsscripts/fixtures/doctor-offline-exit.tstests/ci-workflows.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6f493cb5c
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gui/src/pages/Storage.tsx (1)
809-819: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSynchronize draft fields with the saved policy response.
buildBody()floors the percent before sending it, but successful saves only updatepolicy;percentremains the original draft. For example, entering12.9submits12, while the input continues displaying12.9, leaving the GUI inconsistent with the management API. Reuse the same response-to-draft normalization used byloadPolicy()after bothsavePolicy()and the initial save inrunNow().As per path instructions, GUI state changes must stay consistent with management API responses.
Also applies to: 848-859
🤖 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/src/pages/Storage.tsx` around lines 809 - 819, Synchronize draft fields with the normalized policy returned by the management API. In both the successful save path and the initial save path in runNow(), reuse the response-to-draft normalization used by loadPolicy() to update percent and related draft fields alongside setPolicy, so floored values such as 12.9→12 are reflected in the GUI.Source: Path instructions
gui/src/pages/ClaudeDesktop.tsx (1)
154-159: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate management API payload shapes before using them.
readJsonIfOkonly handles HTTP/JSON parsing errors; a200 {}is still returned. The status poll stores that object, then Lines 360-366 dereferencestatus.health, crashing the page. The load guard also appliesinto truthy primitives and accepts{ profile: null, models: null }, exposing internal errors instead ofclaudeDesktop.loadFail.
gui/src/pages/ClaudeDesktop.tsx#L154-L159: validate the complete response shape required bynormalizeProfilebefore accessing nested fields; reject invalid data with the localized load failure.gui/src/pages/ClaudeDesktop.tsx#L219-L223: add anisDesktopStatusguard and ignore invalid successful payloads rather than callingsetStatus.As per path instructions, GUI state changes must stay consistent with management API responses.
🤖 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/src/pages/ClaudeDesktop.tsx` around lines 154 - 159, In gui/src/pages/ClaudeDesktop.tsx#L154-L159, strengthen the load response validation before normalizeProfile by requiring a non-null object with the complete profile and models shape, rejecting invalid payloads with claudeDesktop.loadFail. In gui/src/pages/ClaudeDesktop.tsx#L219-L223, add and use an isDesktopStatus guard so invalid successful status responses are ignored instead of passed to setStatus.Source: Path instructions
🤖 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 `@tests/ci-workflows.test.ts`:
- Around line 1920-1921: Update the action-reference regex in the workflow
pinning assertion to require exactly 40 hexadecimal characters followed by
end-of-line or whitespace, not merely a word boundary. Ensure refs with a
40-character SHA plus suffixes are rejected while valid standalone SHA pins
remain accepted.
---
Outside diff comments:
In `@gui/src/pages/ClaudeDesktop.tsx`:
- Around line 154-159: In gui/src/pages/ClaudeDesktop.tsx#L154-L159, strengthen
the load response validation before normalizeProfile by requiring a non-null
object with the complete profile and models shape, rejecting invalid payloads
with claudeDesktop.loadFail. In gui/src/pages/ClaudeDesktop.tsx#L219-L223, add
and use an isDesktopStatus guard so invalid successful status responses are
ignored instead of passed to setStatus.
In `@gui/src/pages/Storage.tsx`:
- Around line 809-819: Synchronize draft fields with the normalized policy
returned by the management API. In both the successful save path and the initial
save path in runNow(), reuse the response-to-draft normalization used by
loadPolicy() to update percent and related draft fields alongside setPolicy, so
floored values such as 12.9→12 are reflected in the GUI.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 59fa86dc-20a7-4ca5-8846-cc4ce80faf83
📒 Files selected for processing (4)
gui/src/components/MemoryObservabilityCard.tsxgui/src/pages/ClaudeDesktop.tsxgui/src/pages/Storage.tsxtests/ci-workflows.test.ts
Make apply failures keep server message text, cover the fetch-json fallback path, and only soft-skip prepush on concrete registry/network signatures so findings with words like network still gate.
|
@codex review @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37b3cbcf3f
ℹ️ 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".
Restore manual timeout fallbacks via shared createBoundedFetch, raise doctor maxBuffer and fail on ENOBUFS, recognize ECONNRESET registry failures, and tighten action SHA pinning assertions.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb11a7f4e0
ℹ️ 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".
Give npm ERR! its own alternative so trailing whitespace after the bang still counts as an offline/registry failure instead of gating the push.
Also recognize Node's ERR_CHILD_PROCESS_STDIO_MAXBUFFER so oversized doctor output cannot soft-skip the prepush findings gate.
|
CI is green on the latest push. Remaining inline threads look stale relative to current
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Replace execFileSync so prepush classification uses spawnSync status/error channels and cannot soft-skip oversized doctor output.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/bounded-fetch.ts`:
- Around line 12-27: Add focused unit coverage for createBoundedFetch that
verifies signal.aborted becomes true after controller.abort() in both the native
AbortSignal.any/timeout branch and the manual setTimeout fallback branch. In the
fallback test, temporarily disable or mock AbortSignal.any and
AbortSignal.timeout on the global AbortSignal, then restore them after the test.
In `@scripts/doctor-gui-if-changed.ts`:
- Around line 98-134: Add a focused regression fixture and test for the
hard-fail branch in the doctor execution flow: create an overflow fixture that
emits more than DOCTOR_MAX_BUFFER, then extend the existing CI workflow tests to
run the script with that fixture and assert a nonzero exit code plus the
“exceeded buffer” stderr message. Ensure the test exercises the
ENOBUFS/ERR_CHILD_PROCESS_STDIO_MAXBUFFER handling in the spawnSync result.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 03211aee-273a-40ea-935d-aba7f692f77c
📒 Files selected for processing (7)
gui/src/bounded-fetch.tsgui/src/components/MemoryObservabilityCard.tsxgui/src/pages/ClaudeDesktop.tsxgui/src/pages/Grok.tsxgui/tests/fetch-json.test.tsscripts/doctor-gui-if-changed.tstests/ci-workflows.test.ts
Add regression tests CodeRabbit asked for, and check AbortSignal helpers with typeof so the manual timeout fallback actually runs when any/timeout are missing.
Summary
0.9.1→0.9.2ingui/package.jsonscripts and.github/workflows/react-doctor.yml.blocking: warningingui/doctor.config.json+ CI workflow; prepush now propagates doctor exit codes (offline spawn still soft-skips).Test plan
npx react-doctor@0.9.2 --scope full --no-supply-chainingui/→ 0 issues, score 100, exit 0bun test tests/ci-workflows.test.tsbun run typecheckbun run lintingui/Summary by CodeRabbit
blocking: warning), aligned across GUI instructions, CI, and pre-push checks.