Repository navigation
fix(settings): resolve reactive category sync, credential reference restoration, and locale fallback (#514) - #515
Conversation
…estoration, and locale fallback (#514)
…ences, and harden formShape
Update: Additional hardening & bug fixes pushed in commit
|
PeterGuy326
left a comment
There was a problem hiding this comment.
Reviewed at caf24e9 in a clean worktree (npm ci → tsc -b → node scripts/bundle-configuration.mjs, the last two are needed before typecheck/tests will resolve @roleweave/shared and src/vendor/jsonc-parser.cjs).
Verified locally
| Check | Result |
|---|---|
run-renderer-tests.mjs on the 4 settings suites |
47/47 pass, exit 0 |
node --test on the 6 desktop .cjs suites |
61/61 pass, exit 0 |
typecheck:renderer / typecheck:ui |
both exit 0 |
| antd deprecation warnings | 7 → 0 |
act(...) warnings |
11 before, 11 after — pre-existing, not from this PR |
The fixes are real, not just test-shaped. I reverted src/settings/ + src/locales/ to origin/main while keeping the new tests: exactly the 6 new cases go red, and they go green again with the PR applied. The baseline warning is verbatim Warning: [antd: Alert] \message` is deprecated. Please use `title` instead.— and antd is 6.6.1 here, sotitleis the correct target. Grepped the rest ofapps/: no other ` left, so the migration is complete rather than partial.
Also confirmed the reactive-category fix can't be defeated by React bailing out on an unchanged initialCategory: SettingsModule is conditionally rendered on activeModule === "settings" (App.tsx:2259), so it unmounts on navigation and remounts with fresh state.
Non-blocking — none of this needs to hold the merge
- Two real behaviour changes aren't in the issue or the PR description: the
formShaperelaxation (host sub-objects are now optional) and therequest()clearKeys semantics. Worth adding to the description so the change is discoverable later; theformShapeone at least has a test. request()now emitsnullfor every key inclearKeysregardless of whether an input is registered for it. Today the panels stay mounted so it's equivalent, but this is the credential-deletion path and nothing asserts it. A test onsave()sendinghostChanges.X === nullwould pin it down.- The
probe.checkedAtguard is unreachable — the field is a requiredstringand every producer writesnew Date().toISOString(). If it ever did fire, the fallback renders the raw malformed value (probe.checkedAt || "—") instead of"—"; plain"—"would be the safer output. initialCategoryis widened toConfigurationCategoryand the type is exported, but the only producer (App.tsx:155) is stilluseState<"experiments" | undefined>. Either narrow App.tsx to match or leave it — just noting the extra surface has no caller yet.- Nits:
const input = secretInputs.current.get(key)shadows the outerinput()render helper at ConfigurationSettings.tsx:179 and :199; and thesetClearKeysupdater at :82-94 now carries side effects (field(...),input.value = '') which React may double-invoke under StrictMode — idempotent today, cleaner moved out. - Behaviour note, not a defect: checking "Explicitly clear this credential" now wipes whatever was typed, and unchecking restores the reference but not the typed secret. Reads as intended; a sentence in the description would save a future bug report.
Merge order: this and #513 both touch ServiceConnections.tsx (#513 at lines 34–131, this PR at ~137). Adjacent, not overlapping, so either order should merge clean — whoever lands second just needs to re-run the settings suites.
…updater, and date fallback
|
Thanks @PeterGuy326 for the thorough review and approval! Pushed follow-up commit
Local test suites and TypeScript checks are all green (48 renderer tests, 61 desktop unit tests, clean typecheck). |
…eliminate act warning
PeterGuy326
left a comment
There was a problem hiding this comment.
Reviewed exact head ef59e53.
Technical read of all 8 items:
- initialCategory sync via useEffect + widened ConfigurationCategory type — correct; the reactively-updated tab has a regression test.
- clearSecret / secretChanged now treat the state updater as pure (DOM write and field() moved out, so StrictMode double-invocation cannot duplicate them) and restore the credential reference with a safe fallback (secret:host/ / secret:service/) when the raw text carried none. This is the right fix for the dropped-reference bug.
- request() iterating clearKeys to emit null in hostChanges/serviceChanges removes the dependency on which inputs happen to be mounted; the preview modal unioning secretInputs keys with clearKeys keeps the diff honest.
- formShape host validation is relaxed to match validateConfigurationText (omitted non-essential hosts allowed). Deliberate and consistent, not a hole.
- english falls back to useOwbLocale() so English users stop seeing Chinese loading/error text before the snapshot lands.
- Mint / Ant Blue now go through copy() with zh translations.
- Alert message -> title clears the antd deprecation path.
- probe.checkedAt guarded against malformed dates.
CI on this head: 10/10 green (ubuntu + macOS Node 24, both staging smoke, both installers, CodeQL x2, Dependency review, Scorecard). MERGEABLE, not draft. Approving; merging as squash with Fixes #514 per the PR body.
Summary
Fixes #514.
An audit of the desktop client settings module (
apps/desktop/renderer/src/settings/) identified several issues and optimization points affecting state consistency, localization, error resilience, and framework warnings:initialCategorysync inConfigurationSettings:useState(initialCategory ?? 'general')only runs on mount. When navigation targets a specific category (e.g., jumping from an external feature notice to experiments viainitialCategory="experiments"while settings is already mounted), the tab did not update reactively. Added auseEffectto syncinitialCategory, reset to'general'whenundefined, widenedinitialCategorytoConfigurationCategory, and typedApp.tsxstate accordingly.clearSecret) & pure state updater:snapshot.config.hosts[id]without null-safety and passedundefinedif the reference was not previously in the raw text, permanently dropping the reference. Added safe fallbacks(snapshot.config.hosts[id] as Record<string, string | undefined>)?.[refFields[f.key]!] ?? \secret:host/${f.key}`andsnapshot.config.services[kind]?.tokenRef ?? `secret:service/${kind}``.setClearKeysstate updater to avoid duplicate execution under StrictMode.secretChangednow cleans up the reference (field(path, undefined)) if the credential was not previously configured, avoiding phantomsecret:host/...references left in JSON drafts.clearKeyshandling inrequest():Save request explicitly iterates over all keys in
clearKeysto emitnullinhostChanges/serviceChangesrather than relying solely on DOM inputs insecretInputs.current. Diff preview modal incorporatesclearKeysto faithfully show explicitly cleared credentials. Added a test assertinghostChangesreceivesnullon save.formShape):Relaxed
formShapehost checks to permit omitted non-essential host entries in JSONC, aligning withvalidateConfigurationText.englishcheck inConfigurationSettingsdefaulted to Chinese whensnapshotis null, causing English desktop users to see Chinese loading spinners and error alerts. It now falls back to the active application locale fromuseOwbLocale().Color profile labels "Mint" and "Ant Blue" were hardcoded in English. Added translations ("薄荷绿", "经典蓝") and wrapped with
copy().SettingsModule.tsx:Migrated deprecated
<Alert message={...} />to<Alert title={...} />.ServiceConnections.tsx:Defensive check on
probe.checkedAtfalls back safely to"—"on malformed date strings.Test Plan
node scripts/run-renderer-tests.mjs apps/desktop/renderer/test/settings-module.test.tsx apps/desktop/renderer/test/configuration-settings.test.tsx apps/desktop/renderer/test/experimental-settings.test.tsx apps/desktop/renderer/test/service-connections.test.tsx(48 passed)node --test apps/desktop/test/configuration.test.cjs apps/desktop/test/configuration-ipc.test.cjs apps/desktop/test/credential-settings.test.cjs apps/desktop/test/service-connections.test.cjs apps/desktop/test/experiments-ipc.test.cjs apps/desktop/test/runtime-settings.test.cjs(61 passed)npm run typecheck:renderer(clean)npm run typecheck:ui(clean)