fix(server,frontend): make Advanced Settings own analyzer endpoint and phase models - #3192
Open
dudarenok-maker wants to merge 7 commits into
Open
fix(server,frontend): make Advanced Settings own analyzer endpoint and phase models#3192dudarenok-maker wants to merge 7 commits into
dudarenok-maker wants to merge 7 commits into
Conversation
…config resolver Refs #3141
… reject them on save Refs #3141
Refs #3141 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Refs #3141 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lout from #3141 - Add analyzerPhasePicks: {} to the two pre-existing UiState test literals in theme-toggle.test.tsx / use-theme.test.tsx now that step 5 made it a required field. - Change account.backups.test.tsx's SERVER_FIXTURE.analyzerPhase1MinLagChapters from null to 10 (DEFAULT_PHASE1_MIN_LAG_CHAPTERS) now that the openapi.yaml step-2 change dropped nullable from that response field. - Re-export AnalyseResponse from src/lib/api.ts so analysing.phase-model-picks.test.tsx's type-only import resolves. - Allowlist the config/resolver.ts <-> workspace/user-settings.ts import cycle in server/madge-cycles-allowlist.json. user-settings.ts already carries a header comment (added with step 1's configValue import) documenting this as a deliberate, function-body-only cycle. Breaking it for real would mean moving getResolvedOllamaUrl/getResolvedOllamaModel out of user-settings.ts to stop it depending on the resolver directly, which touches 7+ call-site files outside this rework's scope. - Update the stale analysing.tsx comment claiming per-phase picks persist to UserSettings; step 5 made them a per-run-only pick. npm run typecheck and npm run check:cycles are both clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Merge-order note:
server/src/config/registry-knob-read.guard.test.ts(added by #3139) does not exist yet onorigin/main—git ls-tree origin/mainreturns nothing for that path. #3139 has not merged, so there is noKNOWN_UNREADallowlist entry to reconcile against right now. Whichever of the two PRs merges second must removeanalyzer.ollama.url,analyzer.ollama.model,analyzer.phase0.model,analyzer.phase1.model,analyzer.phase1.minLagChaptersfrom that guard'sKNOWN_UNREADlist, since this branch makes all five keys read. If this PR merges first, #3139 needs the follow-up; if #3139 merges first, this PR does.Found in passing: none beyond what #3152 and #3167 already reported (the typecheck/cycle fallout and stale comment, both fixed by #3167 and reconfirmed here).
Test plan
getResolvedOllamaUrlto read the retired AccountollamaUrlfield is caught bytsc(TS2339: Property 'ollamaUrl' does not exist) since the field is gone from the settings type; (b) removing the never-clobber guard in the legacy-field migration turns red 1 vitest assertion (does not clobber an override that already exists for one of the four target keys); (c) re-addingollamaUrlto the Account save payload is caught bytsc(TS2353: Object literal may only specify known properties) sinceUserSettingsPatchno longer has it; (d) dropping the per-runphaseModelpriority fromselectAnalyzerForPhaseturns red 1 vitest assertion (phaseModel beats opts.model and a saved Advanced Settings override); (e) re-adding aputUserSettingscall to the phase-model swap control turns red 2 vitest assertions asserting it is never called. All five reverted cleanly (git status --porcelainempty after each).e2e/analyzer-settings-ownership.spec.ts— 3 passed (warmup + both scenarios),PLAYWRIGHT_PORT=5324npm run typecheck,npm run check:cycles,npm run config:check,npm run lint, targeted vitest suites — all clean: rootsrc/components src/views src/store(194 files / 3196 tests passed), serversrc/analyzer src/workspace src/config src/routes/user-settings.test.ts(140 files / 2257 tests passed),check:cyclesreports 16 circular dependencies, all allowlistedpr-review-gatepass (operator)Closes #3141
🤖 Generated with Claude Code