fix(typecheck): type Doctor screen state#1522
Conversation
📝 WalkthroughWalkthroughDoctor screen gains explicit TypeScript annotations, extracts dist-tag resolution into ChangesDoctor Screen Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 `@src/screens/Doctor.test.tsx`:
- Around line 98-108: The teardown function restoreActualModules currently
re-registers mocks via mock.module(...) which leaks module-mock state across
tests; instead remove the mock.module(...) calls from restoreActualModules and
ensure teardown only restores original modules by stopping or clearing mocks
(e.g., call the test runner's mock stop/reset APIs or restore the saved actual*
variables without re-registering hooks). Update restoreActualModules and any
afterEach cleanup so they do not call mock.module('../utils/doctorDiagnostic.js'
/ '../utils/autoUpdater.js' / '../utils/nativeInstaller/pidLock.js') and use
proper mock stop/reset (or jest.resetModules()/mock.restore equivalents) to
fully isolate tests.
🪄 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: 909a4226-acf7-4a6f-8dc5-3f428ea1eb48
📒 Files selected for processing (3)
src/screens/Doctor.test.tsxsrc/screens/Doctor.tsxsrc/utils/doctorContextWarnings.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: smoke-and-tests
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/screens/Doctor.tsxsrc/utils/doctorContextWarnings.tssrc/screens/Doctor.test.tsx
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/screens/Doctor.test.tsx
🔇 Additional comments (2)
src/screens/Doctor.tsx (1)
22-22: LGTM!Also applies to: 60-62, 105-105, 115-115, 119-122
src/utils/doctorContextWarnings.ts (1)
2-2: LGTM!Also applies to: 146-146, 269-269, 350-350
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)
src/screens/Doctor.test.tsx (1)
200-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a native-install dist-tag source regression test.
Both tests keep
installationType: 'npm-global', so thenative -> getGcsDistTagsbranch introduced in Doctor remains unverified. Please add a focused case that setsinstallationType: 'native'and asserts GCS tags are rendered (and npm tags are not used).As per coding guidelines, "Review tests for meaningful coverage of the changed behavior ... Block when risky runtime changes lack focused regression coverage."
🤖 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 `@src/screens/Doctor.test.tsx` around lines 200 - 263, Add a focused regression test in Doctor.test.tsx that covers the native-install branch by rendering <Doctor ... deps={doctorDeps} /> with installationType set to 'native' (override doctorDeps or construct a new deps object) and mock getGcsDistTags to return deterministic dist-tags while ensuring getNpmDistTags is not used; then assert the output contains the GCS/`native` tags (e.g., "Latest version" / "Stable version" from the GCS mock) and that npm tag strings are not present. Locate the test block near the existing Doctor tests, reference the Doctor component and the helper mocks getGcsDistTags and getNpmDistTags, and clean up streams exactly as the other tests do (root.unmount, stdin.end, stdout.end, await Bun.sleep(0)).
🤖 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 `@src/screens/Doctor.test.tsx`:
- Around line 200-263: Add a focused regression test in Doctor.test.tsx that
covers the native-install branch by rendering <Doctor ... deps={doctorDeps} />
with installationType set to 'native' (override doctorDeps or construct a new
deps object) and mock getGcsDistTags to return deterministic dist-tags while
ensuring getNpmDistTags is not used; then assert the output contains the
GCS/`native` tags (e.g., "Latest version" / "Stable version" from the GCS mock)
and that npm tag strings are not present. Locate the test block near the
existing Doctor tests, reference the Doctor component and the helper mocks
getGcsDistTags and getNpmDistTags, and clean up streams exactly as the other
tests do (root.unmount, stdin.end, stdout.end, await Bun.sleep(0)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7d98571a-bfeb-411a-acf2-b1272db9fb61
📒 Files selected for processing (2)
src/screens/Doctor.test.tsxsrc/screens/Doctor.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/screens/Doctor.test.tsxsrc/screens/Doctor.tsx
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/screens/Doctor.test.tsx
🔇 Additional comments (1)
src/screens/Doctor.tsx (1)
39-56: LGTM!Also applies to: 125-129, 143-146, 177-223
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 `@src/screens/Doctor.tsx`:
- Around line 125-129: The diagnostic promise can reject and leave the Doctor
screen stuck; update the places where getDoctorDiagnostic is invoked (both the
direct call around lines 160-162 and the useMemo that creates distTagsPromise
via resolveDoctorDistTags) to catch errors and return a safe fallback so no
promise remains unhandled: wrap the getDoctorDiagnostic call in a try/catch or
append .catch(() => fallbackDiagnostic) and ensure resolveDoctorDistTags is
called with a diagnostic-resolving function that never throws (or returns a
resolved fallback), and propagate a sensible default from distTagsPromise so the
UI switches out of loading even when diagnostics fail.
🪄 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: 989d0960-230e-4e2e-8a5a-ff8f87799cc5
📒 Files selected for processing (3)
src/screens/Doctor.tsxsrc/screens/doctorDistTags.test.tssrc/screens/doctorDistTags.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/screens/doctorDistTags.tssrc/screens/doctorDistTags.test.tssrc/screens/Doctor.tsx
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/screens/doctorDistTags.test.ts
🔇 Additional comments (3)
src/screens/Doctor.tsx (1)
35-35: LGTM!Also applies to: 106-118, 170-170, 180-180, 186-189, 205-205
src/screens/doctorDistTags.ts (1)
4-27: LGTM!src/screens/doctorDistTags.test.ts (1)
1-88: LGTM!
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 `@src/screens/Doctor.tsx`:
- Around line 163-166: The PR currently implies it is fully merge-ready despite
repo-wide typecheck failures; update any status text or logic around the
diagnostic load in this file so it doesn't claim readiness when checks are
failing: ensure loadDoctorDiagnostic is invoked normally (keep the call using
loadDoctorDiagnostic with getDoctorDiagnostic) but change the post-call handling
to explicitly surface CI/typecheck failure state instead of marking success—use
setDiagnosticLoadFailed(true) when the repo-wide typecheck baseline or CI checks
are failing and avoid calling setDiagnostic in that case; also update any
user-visible "merge-ready" or success messaging in this file to mention required
GitHub Checks and human review must pass before merge.
🪄 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: 567319f2-f411-4d01-8551-611fed1f10f1
📒 Files selected for processing (5)
src/screens/Doctor.tsxsrc/screens/doctorDiagnosticLoad.test.tssrc/screens/doctorDiagnosticLoad.tssrc/screens/doctorDistTags.test.tssrc/screens/doctorDistTags.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: smoke-and-tests
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/screens/doctorDiagnosticLoad.tssrc/screens/doctorDistTags.tssrc/screens/doctorDistTags.test.tssrc/screens/doctorDiagnosticLoad.test.tssrc/screens/Doctor.tsx
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/screens/doctorDistTags.test.tssrc/screens/doctorDiagnosticLoad.test.ts
🔇 Additional comments (5)
src/screens/doctorDistTags.ts (1)
20-30: LGTM!src/screens/doctorDistTags.test.ts (1)
78-85: LGTM!src/screens/Doctor.tsx (1)
35-35: LGTM!Also applies to: 122-122, 163-166, 245-247
src/screens/doctorDiagnosticLoad.ts (1)
1-23: LGTM!src/screens/doctorDiagnosticLoad.test.ts (1)
1-59: LGTM!
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable issues from my review.
3 participants
Summary
never/nullduring typecheck.Toolsalias in Doctor context-warning helpers instead of requiring mutableTool[].Impact
Testing
bun test src/screens/Doctor.test.tsx src/services/mcp/doctor.test.ts src/commands/mcp/doctorCommand.test.ts src/__tests__/doctorContextWarnings.test.ts src/__tests__/statusNoticeLocalModel.test.tsenv -u CLAUDE_CODE_USE_OPENAI -u OPENAI_API_KEY -u OPENAI_BASE_URL -u OPENAI_MODEL -u CLAUDE_CODE_MAX_RETRIES -u CLAUDE_CODE_UNATTENDED_RETRY bun run checkbun run test:providerenv -u CLAUDE_CODE_USE_OPENAI -u OPENAI_API_KEY -u OPENAI_BASE_URL -u OPENAI_MODEL -u CLAUDE_CODE_MAX_RETRIES -u CLAUDE_CODE_UNATTENDED_RETRY npm run test:provider-recommendationbun run web:typecheckgit diff --checkNotes
bun run typecheckstill exits 2 because of the existing repo-wide baseline; the post-change log has 1639 TypeScript diagnostics and no diagnostics forsrc/screens/Doctor.tsx,src/screens/Doctor.test.tsx, orsrc/utils/doctorContextWarnings.ts.Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Tests