Repository navigation
Fix providers filter persistence and settings translations - #970
diegosouzapw merged 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements persistent storage for the "show configured only" preference on the providers page using localStorage. It also updates the English and Simplified Chinese translation files with new keys for LKGP and maintenance settings, and adds corresponding unit tests. Feedback suggests optimizing the storage update logic to avoid redundant writes on initialization and ensuring all relevant translation keys are included in the Chinese locale.
| useEffect(() => { | ||
| if (!configuredOnlyPreferenceReady) return; | ||
|
|
||
| writeConfiguredOnlyPreference(showConfiguredOnly); | ||
| }, [configuredOnlyPreferenceReady, showConfiguredOnly]); |
There was a problem hiding this comment.
The current implementation causes a redundant write to localStorage on every page load. When the component mounts, the first useEffect reads the preference and updates showConfiguredOnly. This update, combined with configuredOnlyPreferenceReady becoming true, triggers this second useEffect, which then writes the same value back to storage. It is better to check if the value has actually changed before writing.
useEffect(() => {
if (!configuredOnlyPreferenceReady) return;
if (showConfiguredOnly !== readConfiguredOnlyPreference()) {
writeConfiguredOnlyPreference(showConfiguredOnly);
}
}, [configuredOnlyPreferenceReady, showConfiguredOnly]);
| "lkgpCacheClearFailed": "清除 LKGP 缓存失败", | ||
| "maintenance": "维护", |
There was a problem hiding this comment.
Several translation keys related to LKGP (days, lkgp, lkgpDesc) are present in the English settings block but were omitted from the Simplified Chinese version in this PR. Since the objective is to fix settings translations, these should be included to ensure a complete localized experience.
"lkgpCacheClearFailed": "清除 LKGP 缓存失败",
"days": "天",
"lkgp": "LKGP 模式",
"lkgpDesc": "最后已知良好提供商（可预测的韧性）",
"maintenance": "维护",
There was a problem hiding this comment.
Pull request overview
This PR persists the Providers page “Configured only” filter across reloads and fills in missing Settings i18n strings related to LKGP and maintenance actions, with unit tests to guard against regressions.
Changes:
- Persist
Configured onlyon the Providers page via a small localStorage wrapper. - Add missing
settings.*translation keys for LKGP + maintenance UI inenandzh-CN. - Add unit tests for provider filter preference parsing/storage and presence of required settings keys.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/settings-i18n-keys.test.mjs | Adds a unit test asserting required settings.* translation keys exist (currently only EN + ZH-CN). |
| tests/unit/providers-page-utils.test.mjs | Adds tests for configured-only preference parsing and storage round-trip behavior. |
| src/i18n/messages/zh-CN.json | Adds missing Simplified Chinese settings.* strings for cache/maintenance/LKGP UI. |
| src/i18n/messages/en.json | Adds missing English settings.* strings for cache/maintenance/LKGP UI. |
| src/app/(dashboard)/dashboard/providers/providerPageStorage.ts | Introduces localStorage helpers for reading/writing the configured-only preference. |
| src/app/(dashboard)/dashboard/providers/page.tsx | Reads configured-only preference on mount and writes changes back to storage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (enabled) { | ||
| storage.setItem(SHOW_CONFIGURED_ONLY_STORAGE_KEY, "true"); | ||
| return; | ||
| } | ||
|
|
||
| storage.removeItem(SHOW_CONFIGURED_ONLY_STORAGE_KEY); |
There was a problem hiding this comment.
localStorage.setItem / removeItem can throw (e.g., Safari private mode or quota exceeded). Even if getBrowserStorage() returns a storage object, an unhandled DOMException here would crash the Providers page. Wrap these storage writes in try/catch and treat failures as a no-op.
| if (enabled) { | |
| storage.setItem(SHOW_CONFIGURED_ONLY_STORAGE_KEY, "true"); | |
| return; | |
| } | |
| storage.removeItem(SHOW_CONFIGURED_ONLY_STORAGE_KEY); | |
| try { | |
| if (enabled) { | |
| storage.setItem(SHOW_CONFIGURED_ONLY_STORAGE_KEY, "true"); | |
| return; | |
| } | |
| storage.removeItem(SHOW_CONFIGURED_ONLY_STORAGE_KEY); | |
| } catch { | |
| return; | |
| } |
| export function readConfiguredOnlyPreference(storage: StorageReader | null = getBrowserStorage()) { | ||
| if (!storage) return false; | ||
|
|
||
| return parseConfiguredOnlyPreference(storage.getItem(SHOW_CONFIGURED_ONLY_STORAGE_KEY)); |
There was a problem hiding this comment.
storage.getItem(...) can also throw in some browser modes. Consider wrapping the read in try/catch and returning the default (false) on failure, so initialization can't crash the page when storage is blocked/unavailable.
| return parseConfiguredOnlyPreference(storage.getItem(SHOW_CONFIGURED_ONLY_STORAGE_KEY)); | |
| try { | |
| return parseConfiguredOnlyPreference(storage.getItem(SHOW_CONFIGURED_ONLY_STORAGE_KEY)); | |
| } catch { | |
| return false; | |
| } |
| test("settings translations include LKGP and maintenance keys in English and Simplified Chinese", () => { | ||
| for (const key of requiredSettingsKeys) { | ||
| assert.equal(typeof en.settings?.[key], "string", `en.settings.${key} should exist`); | ||
| assert.equal(typeof zhCn.settings?.[key], "string", `zh-CN.settings.${key} should exist`); | ||
| } |
There was a problem hiding this comment.
This test only checks en and zh-CN, but the app supports many locales and loads one messages/<locale>.json per request. Since the new settings keys are not present in the other locale JSON files, consider iterating through all locale message files and asserting these keys exist (or explicitly enforce a fallback strategy).
| "lkgpToggleTitle": "Last Known Good Provider (LKGP)", | ||
| "lkgpToggleDesc": "When enabled, the router remembers which provider last served a successful response and tries it first on subsequent requests.", | ||
| "clearLkgpCache": "Clear LKGP Cache", | ||
| "lkgpCacheCleared": "LKGP cache cleared successfully", | ||
| "lkgpCacheClearFailed": "Failed to clear LKGP cache", |
There was a problem hiding this comment.
These newly added settings.* keys (LKGP/maintenance) currently exist only in en.json and zh-CN.json (verified via repo-wide search), but the app supports many locales under src/i18n/messages/. For other locales, these messages will still be missing and may cause runtime missing-message errors. Please add the same keys to the other locale JSON files (placeholders OK) or implement a deterministic fallback.
rdself
changed the title
|
Thanks @rdself for this clean contribution! 🎉 The providers filter persistence and missing i18n keys have been merged into main and will be part of v3.5.1. Great job on the DI-friendly storage module and comprehensive tests. We appreciate your effort! |
Summary
Configured onlytoggle with localStorage so the filter survives refreshessettingstranslation keys for LKGP and maintenance-related UI in English and Simplified ChineseRoot Cause
Configured onlystate only lived in component memory, so it reset tofalseafter every page reloadsettingstranslation keys that were never added to the locale message files, so the UI fell back to inline strings or showed missing localized contentImpact
Validation
node --import tsx/esm --test tests/unit/providers-page-utils.test.mjs tests/unit/settings-i18n-keys.test.mjsnpx eslint 'src/app/(dashboard)/dashboard/providers/page.tsx' 'src/app/(dashboard)/dashboard/providers/providerPageStorage.ts' 'tests/unit/providers-page-utils.test.mjs' 'tests/unit/settings-i18n-keys.test.mjs'npm run typecheck:core -- --pretty false