refactor(onboard): lower cognitive complexity ratchet to 245 - #5420
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com> (cherry picked from commit 41fcd6a)
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughExtracts NIM inference-selection logic from inline branches in ChangesNIM Selection Handler Extraction and Remote Model Validator
Biome Complexity Threshold
Sequence DiagramsequenceDiagram
participant setupNim as setupNim() Step 3
participant handler as handler function
participant validator as validateSelectedRemoteModel
participant remote as Remote Provider API
setupNim->>handler: create SetupNimSelectionState
handler->>validator: call with selected model
validator->>remote: probe/validate model
remote-->>validator: validation result
validator-->>handler: "selected" or "retry-*"
handler-->>setupNim: return updated state
alt retry-selection
setupNim->>setupNim: continue selection loop
else selected
setupNim->>setupNim: store provider/model/endpoint
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
PR Review AdvisorFindings: 0 needs attention, 7 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
E2E Advisor RecommendationRequired E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 27557046406
|
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 `@src/lib/onboard.ts`:
- Around line 3519-3522: When no NIM models fit the GPU VRAM and the code
returns "selected" while breaking out of selectionLoop, the state.model remains
null, causing setupNim() to return an incomplete provider/model selection.
Before returning "selected" in the fallback case, populate the cloud fallback
configuration by setting state.model to the appropriate cloud provider/model
selection so that setupNim() can properly fall back to cloud API inference
instead of returning incomplete results. Apply this fix at both the initial
fallback location and the secondary affected location mentioned in the comment.
- Around line 3626-3627: The retry paths that return "retry-selection" (when
validation.retry is "selection" or "model", or when validation.ok is false)
leave state.nimContainer set with a stale value. This stale container name
persists into the next iteration of the selection loop, causing non-NIM
selections to incorrectly use the failed NIM container. Before each return
statement that returns "retry-selection", clear state.nimContainer to ensure no
stale local-NIM state carries forward to subsequent provider selections.
🪄 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: CHILL
Plan: Enterprise
Run ID: 426eab54-be42-4cc5-b913-46b50c30fabb
📒 Files selected for processing (3)
biome.jsonsrc/lib/onboard.tssrc/lib/onboard/setup-nim-selection.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 27557870341
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Summary
Lower the cognitive-complexity ratchet from 255 to 245 by extracting the highest-complexity onboarding provider-selection paths out of
setupNim. This keeps the new Biome rule moving downward one offender at a time without adding ignore comments or per-file exceptions.Changes
complexity/noExcessiveCognitiveComplexityfrom255to245inbiome.json.src/lib/onboard/setup-nim-selection.tsto keepsrc/lib/onboard.tsnet-neutral for the growth guardrail.setupNiminto smaller helpers while preserving the existing mutable selection state.SetupNimSelectionStatetype fromsetup-nim-selection.tsto avoid duplicated selection-state contracts.nimContainervalues.Type of Change
Verification
npx @biomejs/biome lint --only=complexity/noExcessiveCognitiveComplexity --max-diagnostics=none .npx @biomejs/biome lint src/lib/onboard.ts src/lib/onboard/setup-nim-selection.ts src/lib/onboard/setup-nim-selection.test.ts biome.jsonnpm run typecheck:clinpx vitest run --project cli src/lib/onboard/providers.test.ts src/lib/onboard/provider-selection.test.ts src/lib/onboard/provider-selection-prompt.test.ts src/lib/onboard/vllm-menu.test.ts src/lib/onboard/routed-inference.test.ts src/lib/inference/nim.test.ts src/lib/onboard/setup-nim-selection.test.tsGit hooks passed during commit and push, or
npx prek run --from-ref main --to-ref HEADpassesTargeted tests pass for changed behavior
Tests added or updated for new or changed behavior
Full
npm testpasses (broad runtime changes only)No secrets, API keys, or credentials committed
Docs updated for user-facing behavior changes
npm run docsbuilds without warnings (doc changes only)Doc pages follow the style guide (doc changes only)
New doc pages include SPDX header and frontmatter (new pages only)
Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Improvements
Tests
Chores