Skip to content

fix(settings): resolve reactive category sync, credential reference restoration, and locale fallback (#514) - #515

Merged
PeterGuy326 merged 4 commits into
mainfrom
fix/514-client-settings-fixes
Sep 27, 2026
Merged

PeterGuy326 merged 4 commits into
mainfrom
fix/514-client-settings-fixes

Conversation

@Bindy-lbb

@Bindy-lbb Bindy-lbb commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator

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:

  1. Reactive initialCategory sync in ConfigurationSettings:
    useState(initialCategory ?? 'general') only runs on mount. When navigation targets a specific category (e.g., jumping from an external feature notice to experiments via initialCategory="experiments" while settings is already mounted), the tab did not update reactively. Added a useEffect to sync initialCategory, reset to 'general' when undefined, widened initialCategory to ConfigurationCategory, and typed App.tsx state accordingly.
  2. Safe host/service credential reference restoration (clearSecret) & pure state updater:
    • Checking and then unchecking "Explicitly clear this credential" accessed snapshot.config.hosts[id] without null-safety and passed undefined if 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}``.
    • Checking "Explicitly clear this credential" wipes whatever was typed in the secret input, and unchecking restores the reference (not the typed secret). Side effects are kept outside setClearKeys state updater to avoid duplicate execution under StrictMode.
    • When an uncommitted secret input is erased back to empty, secretChanged now cleans up the reference (field(path, undefined)) if the credential was not previously configured, avoiding phantom secret:host/... references left in JSON drafts.
  3. Hardened clearKeys handling in request():
    Save request explicitly iterates over all keys in clearKeys to emit null in hostChanges/serviceChanges rather than relying solely on DOM inputs in secretInputs.current. Diff preview modal incorporates clearKeys to faithfully show explicitly cleared credentials. Added a test asserting hostChanges receives null on save.
  4. Form view resilience (formShape):
    Relaxed formShape host checks to permit omitted non-essential host entries in JSONC, aligning with validateConfigurationText.
  5. Locale fallback during loading / load failure:
    english check in ConfigurationSettings defaulted to Chinese when snapshot is null, causing English desktop users to see Chinese loading spinners and error alerts. It now falls back to the active application locale from useOwbLocale().
  6. Untranslated color profile options:
    Color profile labels "Mint" and "Ant Blue" were hardcoded in English. Added translations ("薄荷绿", "经典蓝") and wrapped with copy().
  7. Ant Design v6 deprecation warning in SettingsModule.tsx:
    Migrated deprecated <Alert message={...} /> to <Alert title={...} />.
  8. Defensive formatting in ServiceConnections.tsx:
    Defensive check on probe.checkedAt falls back safely to "—" on malformed date strings.

Test Plan

  • Run renderer settings suite:
    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)
  • Run desktop unit tests:
    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)
  • TypeScript checks:
    npm run typecheck:renderer (clean)
    npm run typecheck:ui (clean)

@Bindy-lbb

Copy link
Copy Markdown
Collaborator Author

Update: Additional hardening & bug fixes pushed in commit caf24e9

  1. Category reset & widening:
    • Fixed category reset issue when initialCategory becomes undefined (e.g. clicking the settings rail icon). useEffect now resets to 'general' instead of ignoring undefined.
    • Exported ConfigurationCategory and widened initialCategory prop on both ConfigurationSettings and SettingsModule to support deep linking to any tab category.
  2. Phantom secret reference cleanup:
    • When an uncommitted secret input is erased back to empty, secretChanged now correctly cleans up the reference (field(path, undefined)) if the credential was not previously configured, avoiding phantom secret:host/... references left in JSON drafts.
  3. Hardened clearKeys handling:
    • Save request explicitly iterates over all keys in clearKeys rather than relying on DOM inputs in secretInputs.current.
    • Diff preview modal incorporates clearKeys to faithfully show explicitly cleared credentials.
  4. Form view resilience:
    • Relaxed formShape host checks to permit omitted non-essential host entries in JSONC, aligning with validateConfigurationText.

PeterGuy326
PeterGuy326 previously approved these changes Sep 26, 2026

@PeterGuy326 PeterGuy326 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Two real behaviour changes aren't in the issue or the PR description: the formShape relaxation (host sub-objects are now optional) and the request() clearKeys semantics. Worth adding to the description so the change is discoverable later; the formShape one at least has a test.
  2. request() now emits null for every key in clearKeys regardless 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 on save() sending hostChanges.X === null would pin it down.
  3. The probe.checkedAt guard is unreachable — the field is a required string and every producer writes new Date().toISOString(). If it ever did fire, the fallback renders the raw malformed value (probe.checkedAt || "—") instead of "—"; plain "—" would be the safer output.
  4. initialCategory is widened to ConfigurationCategory and the type is exported, but the only producer (App.tsx:155) is still useState<"experiments" | undefined>. Either narrow App.tsx to match or leave it — just noting the extra surface has no caller yet.
  5. Nits: const input = secretInputs.current.get(key) shadows the outer input() render helper at ConfigurationSettings.tsx:179 and :199; and the setClearKeys updater at :82-94 now carries side effects (field(...), input.value = '') which React may double-invoke under StrictMode — idempotent today, cleaner moved out.
  6. 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.

@Bindy-lbb

Copy link
Copy Markdown
Collaborator Author

Thanks @PeterGuy326 for the thorough review and approval!

Pushed follow-up commit 63bb5c5 to address all your non-blocking suggestions:

  1. PR Description updated: Documented the formShape relaxation (optional host sub-objects), request() clearKeys semantics, and the intentional UX behavior when checking/unchecking "Explicitly clear this credential".
  2. clearKeys deletion test added: Added a test in configuration-settings.test.tsx verifying that explicitly clearing a credential emits hostChanges.OPENAI_API_KEY === null in the save payload.
  3. probe.checkedAt fallback: Changed the fallback to plain "—" instead of probe.checkedAt || "—".
  4. Pure state updater & nit cleanup: Moved side effects (field(...), resetting secret input value) out of the setClearKeys updater callback to prevent issues under React StrictMode. Renamed shadowed const input to const secretInput.
  5. Typed settingsInitialCategory in App.tsx: Re-exported ConfigurationCategory from SettingsModule and updated useState<ConfigurationCategory | undefined>() in App.tsx.

Local test suites and TypeScript checks are all green (48 renderer tests, 61 desktop unit tests, clean typecheck).

@PeterGuy326 PeterGuy326 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head ef59e53.

Technical read of all 8 items:

  1. initialCategory sync via useEffect + widened ConfigurationCategory type — correct; the reactively-updated tab has a regression test.
  2. 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.
  3. 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.
  4. formShape host validation is relaxed to match validateConfigurationText (omitted non-essential hosts allowed). Deliberate and consistent, not a hole.
  5. english falls back to useOwbLocale() so English users stop seeing Chinese loading/error text before the snapshot lands.
  6. Mint / Ant Blue now go through copy() with zh translations.
  7. Alert message -> title clears the antd deprecation path.
  8. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(settings): resolve reactive initial category sync, credential reference restoration, and locale fallback in client settings

2 participants