Feat/eng 2974#724
Open
RyanNg1403 wants to merge 9 commits into
Open
Conversation
… in SETTINGS_REGISTRY Adds EnumSettingDescriptor to the settings type system and registers the two language settings (mode, code) under category 'language'. The wire layer (CLI parser, transport DTO, validator, store, handler) now accepts and round-trips string-valued enum settings end-to-end; TUI and WebUI renderers will pick them up in follow-up commits. - SettingDescriptor union grows with EnumSettingDescriptor (options field) - SettingItem.current / default widen to boolean | number | string - SettingsItemDTO exposes 'options' on enum-typed items - brv settings set <enum-key> rejects invalid values with the allowed list - SettingsValidator.validate routes enum values through validateEnum - SETTINGS_KEYS gains LANGUAGE_MODE + LANGUAGE_CODE - registry test: enum narrowing, options shape, language category - handler test: LIST exposes options, SET rejects non-string to enum keys
Surfaces language.mode / language.code via the existing settings UIs. - shared/language/language-names.ts: extracted single source of truth for the ISO-639-1 → English-name map (was inline in language-clause.ts); reused by settings registry, WebUI display labels, and language-clause - shared SettingsRow / format-settings: adds enum row builder with options + display formatting "[ auto ]" - TUI settings-page: Left/Right cycles options in edit mode; LANGUAGE group appears below UPDATES per CATEGORY_ORDER - WebUI enum-settings-row.tsx: select dropdown driven by descriptor options; language.code rows render "ja — Japanese" via LANGUAGE_NAMES - WebUI LanguagePanel wires into Configuration > General between TaskHistoryPanel and UpdatesPanel - tests: enum row builder + parseRowInput + category ordering
…onfig set language.*
Switches the curate kickoff/continuation language read site from project
config to the daemon settings store, and points users at the new
brv settings set surface for any future change. Project config remains
a fallback for users who still have a per-project override from
ENG-2691 so no one loses their setting on upgrade.
- curate/index.ts: resolveLanguagePreference now calls FileSettingsStore
first; falls back to ProjectConfigStore for backward compat
- config/set.ts: brv config set language.{mode,code} now fails with a
clear "moved to brv settings set" message; pure dispatcher unchanged
so the validation path still covers other project-config keys
…ange + integer parsed.value Pre-push tsc rejected: - enum-settings-row: Select.onValueChange signature is (string | null) under the component lib's types; the null branch is unreachable for a non- cleared single-select but the type system needs an explicit guard. - settings-row IntegerSettingsRow: parsed.value widened to (number | string) after enum support landed; narrow to number before passing to toastValue/setMutation so the integer branch keeps compiling under strict.
The oclif settings command kept a duplicate local CATEGORY_ORDER + HEADERS that wasn't extended when 'language' was added to the canonical shared registry, so language rows silently dropped from text output even though the JSON form exposed them. Promote CATEGORY_HEADERS plus a CATEGORY_ORDER-derived toRowCategory helper into the shared types module so the oclif and shared-utils formatters consume a single canonical source. Add a regression test that iterates every category in CATEGORY_ORDER, gating the broader "new category added but normalizer not updated" bug class.
`oclif/curate/index.ts` was importing FileSettingsStore + SETTINGS_KEYS directly from server/ to resolve the language preference, violating CLAUDE.md's "oclif and tui never import from server" boundary. Route through SettingsEvents.LIST via withDaemonRetry — same pattern used by `brv settings get/list`. Add an intent comment on resolveLanguagePreference explaining the daemon-vs-project-config precedence semantics for the ENG-2691 → ENG-2974 migration window so a future reader doesn't try to "fix" the deliberate fallback. Distinguishing "user explicitly set mode=auto" from "user never touched settings" requires raw-overrides access the transport doesn't expose today, and the case can't manifest in production until project-config language ships.
The round-3 transport refactor inlined `'language.mode'` and `'language.code'` as local string-literal consts in `oclif/commands/curate/index.ts` to avoid the oclif → server boundary slip. That workaround reproduced the brittleness the original `SETTINGS_KEYS` constant was designed to prevent — a rename in the registry would no longer be a typecheck error at the curate call site (and the same trap existed in two WebUI files). Move `SETTINGS_KEYS` to `src/shared/types/settings-keys.ts` so every consumer (server registry / validator / bootstrap, agent, oclif curate, WebUI labels + enum-settings-row) imports one canonical map. Rename safety is now machine checked. WebUI consumers that previously inlined the wire keys also switch to the constant. Also export `readLanguageFromSettings` with an optional `DaemonClientOptions` parameter and add three unit tests covering: daemon returns fixed → returns the language; daemon returns auto → undefined; daemon throws → undefined. The function was the surface that just changed and silently swallows daemon errors, so direct coverage is warranted.
RyanNg1403
requested review from
bao-byterover,
cuongdo-byterover,
danhdoan,
leehpham,
ncnthien,
ngduyanhece and
wzlng
as code owners
Contributor
|
Claude finished @RyanNg1403's task in 7m 0s —— View job Code Review — feat/ENG-2974 (Settings: language.mode/code via SETTINGS_REGISTRY)Overall score: 4/5 — solid, well-tested feature work. The discriminated-union widening from Main concern: the kickoff-path daemon round-trip ( Findings (inline)
What's good
What was checked but is fine
Checklist
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…ings PR Five inline-comment items on PR #724's auto-review against feat/ENG-2974: 1. Tight retry budget on readLanguageFromSettings — kickoff was inheriting withDaemonRetry's MAX_RETRIES=10 × 1s default, making every `brv curate` block ~9s when the daemon was unreachable before falling back to project config. Now passes {maxRetries:1, retryDelayMs:0, ...options} so a missing daemon trips the fallback immediately. Test asserts call count stays at most 2 (was 10 with the old default; test would have run for ~9s, now completes in 6ms). 2. Surface enum options on `brv settings get` — text mode now prints an `allowed: a, b, c` line for enum keys; JSON mode includes the `options` field. Mirrors how integer keys surface `min`/`max`/`range`. Scripted consumers can discover the legal value set without grepping source. 3. Clean stale `brv config set` surface — help text, args description, and examples no longer advertise the now-deprecated `language.mode` / `language.code` (the command-level interceptor catches those upstream with a deprecation-redirect). Examples point users at `brv settings set` instead. SETTERS dispatcher emptied with a comment that future project-config keys wire in by adding one entry; unreachable setLanguageMode / setLanguageCode functions removed; their dispatcher tests collapsed to skeleton coverage. 4. Collapse language-clause re-export — replaced the `export {LANGUAGE_NAMES} from ...` + `import {... as LANGUAGE_NAMES_LOCAL}` double-binding with a single `import {LANGUAGE_NAMES}` directly from the canonical shared module. `oclif/commands/config/set.ts` and the language-clause test repoint to the same canonical path. 5. file-settings-store — source `restartRequired` from the registry descriptor instead of hardcoding `true`. The hardcoded value was dead in the transport-routed path (the handler already overrides via the descriptor) but misrepresented boolean/enum keys whose descriptors carry `restartRequired: false` (update.checkForUpdates, language.mode, language.code) for any in-process caller reading the store directly. Also refresh the `SettingsFile.values` doc that still said "set writes only validated numeric values" — now mentions booleans and enums. Verification: 8871 npm test passing; ESLint + typecheck + build clean; 3 sub-agents PASS on settings-get / curate-pipeline / config-deprecation surfaces. Codex round-5 review APPROVED.
Contributor
|
Claude finished @RyanNg1403's task in 3m 48s —— View job Code Review — feat/ENG-2974 (re-review after follow-ups)Overall score: 4.5/5 — the previous round's 6 findings are addressed cleanly. One residual doc-rot from finding #1 remains (couldn't post inline — the comment block isn't in the diff). Everything else is shipped or properly deferred. Status of previous findings
What's good in this round
Tests / buildPer PR description: 8871 passing, 16 pending; lint clean; build succeeds. Spot-checked the new and modified test files — assertions are tight (call-count cap, enum option propagation, deprecation-redirect dispatcher). Checklist
|
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
2 participants
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
brv config set language.*(project-config flat file). TUI users and WebUI users had no way to discover or change their language — the panels listed bybrv settings listweren't aware of language at all.brv settings. Language being the lone outlier in project config made it invisible to non-CLI users and forced docs to explain two separate "settings" surfaces.SETTINGS_REGISTRYtype system with anenumdescriptor variant, registeredlanguage.mode(auto/fixed) andlanguage.code(24 ISO-639-1 options) under a new'language'category, and wired the wire/CLI/TUI/WebUI layers to render and mutate them. The curate read site now consults daemon settings first;brv config set language.*is sunset with a clear pointer to the new command.--langflag, per-domain language overrides, first-run language wizard, and the MCPbrv-curate-tooltool description (still built at server-boot from the auto clause — separate follow-up). The 24-entryLANGUAGE_NAMESmap is unchanged; it was extracted tosrc/shared/language/language-names.tsso settings + WebUI + language-clause share one source.Type of change
Scope (select all touched areas)
Linked issues
brv config set language.*)Root cause (bug fixes only, otherwise write
N/A)N/A — feature work.
Test plan
test/unit/core/domain/entities/settings-registry.test.ts— enum narrowing,language.*entries, category propagationtest/unit/infra/transport/handlers/settings-handler.test.ts— LIST exposesoptions, SET rejects non-string to enum keystest/unit/shared/utils/format-settings.test.ts— enum row builder + parser + category orderingtest/unit/infra/storage/file-settings-store.test.ts— keyset assertion updatedfindSettingDescriptor('language.mode')narrows toEnumSettingDescriptorwithoptions: ['auto', 'fixed']SettingsHandler.LISTincludes both language entries;optionsis omitted from non-enum DTOsSettingsHandler.SETrejectslanguage.mode: 5withexpected: 'enum', acceptslanguage.mode: 'fixed'buildSettingsRowsproduces enum row withdisplayCurrent: '[ auto ]', category-ordered afterupdatesparseRowInputaccepts a valid option, rejects unknown options with the allowed listUser-visible changes
brv settings listadds aLANGUAGEsection with two rows:language.mode([ auto ]/[ fixed ]) andlanguage.code(e.g.[ en ]).brv settings set language.mode fixed— accepted; rejects invalid values withexpected one of [auto, fixed].brv settings set language.code ko— accepted; rejects unknown codes with the full ISO list in the error.brv settings get language.code— printscurrent,default,scopelike other settings.LANGUAGEgroup; Enter enters edit mode; Left/Right cycles options; Enter saves; Esc cancels.Languagepanel (between Task history and Updates) with Select dropdowns.language.coderows showja — Japanesefor readability.brv config set language.mode | language.codenow fails with'language.{mode,code}' has moved to global settings. Run: brv settings set language.{mode,code} <value>. Other project-config keys (none today; dispatcher kept for future use) keep working unchanged.brv curatereads language from daemon settings first, falls back to.brv/config.jsonfor users who haven't migrated — no setting is lost on upgrade.Evidence
before any changes:
$ brv settings list | grep -i language
(no output — language was invisible to settings UI)
after:
$ brv settings list | grep -i language
language.mode auto (default auto)
language.code en (default en)
$ brv settings set language.mode fixed
Setting saved: language.mode = fixed.
$ brv settings set language.code xx
language.code expected one of [ar, de, el, en, es, fi, fr, he, hi, id, it, ja, ko, nl, no, pl, pt, ru, sv, th, tr, uk, vi, zh], got 'xx'.
$ brv config set language.mode fixed
'language.mode' has moved to global settings. Run: brv settings set language.mode fixed
$ npm test
8869 passing (21s)
16 pending
$ npm run lint
279 problems (0 errors, 279 warnings) # all pre-existing warnings in unrelated test files
$ npm run build
✓ tsc -b
✓ vite build src/webui (built in 7.32s)