Conversation
- Background: required-fields routing duplicated LLM provider rules instead of using the TUI schema. - Changes: add boolean/predicate required metadata, mark provider fields in the schema, validate active visible fields generically, and support persisted secret-location references safely. - Tests: cover conditional and hidden requirements, empty values, keyless Ollama, secret pointers, matching legacy provider blocks, and entry routing. - Verification: targeted Vitest (34 tests), npm run lint, npm run build, git diff --check, and independent review passed. Full npm test reached 8,361 passing tests; 18 unrelated environment/timing failures remained in browser, REPL, health, and packaged-binary suites. Fixes KnockOutEZ#257
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe TUI schema now defines required fields and provider-dependent API-key requirements. ChangesRequired field validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PersistedConfig
participant hasRequiredFields
participant CATALOG
PersistedConfig->>hasRequiredFields: provide persisted configuration
hasRequiredFields->>CATALOG: collect active required fields
CATALOG-->>hasRequiredFields: return field definitions
hasRequiredFields-->>PersistedConfig: return completion status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
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/cli/tui/state/required-fields.ts`:
- Around line 47-51: Update the legacy key-location check in the required-field
predicate to use hasCompleteValue instead of only checking string type and
length, so whitespace-only values are rejected consistently with regular
secret-location references. Add a regression test covering
config.provider.keyLocation set to whitespace.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dca11e68-1c66-4044-8dd4-f384bb20999c
📒 Files selected for processing (6)
src/cli/tui/schema/llm.tssrc/cli/tui/schema/types.tssrc/cli/tui/state/required-fields.tstests/unit/cli/tui/entry-required-fields.test.tstests/unit/cli/tui/schema/llm.test.tstests/unit/cli/tui/state/required-fields.test.ts
- Background: CodeRabbit found that whitespace-only legacy key locations could satisfy setup completion. - Changes: validate legacy provider key-location references through the same field-aware completion helper and add a regression test. - Verification: targeted Vitest (35 tests), npm run lint, and git diff --check passed.
|
CI follow-up: all lint/build/unit jobs, CodeRabbit, and every clean-machine smoke job passed on commit |
|
Hi @w-up — thank you for this PR, and I'm sorry it's been sitting here without a proper review. I've been heads-down building some pretty big features and improvements for wigolo, and as a solo dev, shipping that while also reviewing and maintaining every issue and PR has been genuinely hard. This one isn't forgotten — it just hasn't had the attention it deserves yet. I'd really appreciate your patience here. I'm going to work through the open PRs and issues properly over the next few weeks (sooner if I can free up), and I'll follow up right here. In the meantime, the wigolo Discord is open if you'd like to follow what's being built, ask questions, or nudge me directly: https://discord.gg/BkUUgz2bNF Thanks again for contributing, and for understanding — it genuinely means a lot. 🙏 |
Summary
Derive first-run required-field checks from the TUI schema instead of hardcoding LLM provider paths in
hasRequiredFields.Background
src/cli/tui/state/required-fields.tsduplicated setup rules forllmProviderandllmApiKey. This made the entry-routing logic drift-prone and left the schema unable to express unconditional or conditional requirements.Changes
FieldDef.requiredas either a boolean or a predicate overCtx.llmProvideras always required andllmApiKeyas required only for providers other than Ollama.CATALOG, ignoring fields hidden in the current schema context.<settingsPath>KeyLocationsecret references without reading secret values.llmApiKeyprovider.Verification
npm test -- --run tests/unit/cli/tui/state/required-fields.test.ts tests/unit/cli/tui/schema/llm.test.ts tests/unit/cli/tui/entry-required-fields.test.ts— 34 tests passed.npm run lint— passed.npm run build— passed.git diff --check— passed.env -u NODE_ENV npm test— 8,361 tests passed; 18 unrelated environment/timing failures remained in browser teardown, REPL readline, daemon health, REST timeout, and packaged-binary suites.Fixes #257
Summary by CodeRabbit
New Features
Bug Fixes