fix(desktop): onboarding i18n audit + locale-reactive switching#39
Merged
fix(desktop): onboarding i18n audit + locale-reactive switching#39
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Findings
- [Major] Validation effect re-runs on every render due to unstable
tdependency — this can repeatedly call provider detection/key validation and thrash UI state, causing unnecessary network load and racey UX. Evidence:apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:158addstto the dependency list, whileuseT()currently returns a new function each render.
Suggested fix:// packages/i18n/src/index.ts import { useCallback } from 'react'; export function useT(): (key: string, options?: Record<string, unknown>) => string { const { t } = useTranslation(); return useCallback((key, options) => t(key, options ?? {}) as string, [t]); }
Summary
- Review mode: initial
- 1 issue found (Major): onboarding validation effect dependency now causes repeated effect execution.
- Context docs check:
docs/VISION.mdanddocs/PRINCIPLES.mdare Not found in repo/docs in this checkout. - Residual testing gap: no component-level test proving onboarding validation requests are not retriggered on no-op re-renders.
Testing
- Not run (automation):
pnpmis unavailable in this runner, so Vitest/Playwright could not be executed here.
open-codesign Bot
|
|
||
| return () => window.clearTimeout(handle); | ||
| }, [trimmed, trimmedBaseUrl]); | ||
| }, [trimmed, trimmedBaseUrl, t]); |
Contributor
There was a problem hiding this comment.
is included in this effect dependency list, but currently returns a new function each render. That makes this effect fire repeatedly even when / are unchanged, which can re-trigger provider detection + key validation in a loop. Please stabilize (e.g., ) or remove this function dependency and depend on a stable locale signal instead.
Contributor
There was a problem hiding this comment.
Findings
- No actionable issues found in added/modified lines.
Summary
- Review mode: follow-up after new commits
- No new issues found in the latest head diff.
- Residual risk/testing gap: changes are covered at i18n-key level, but no component-level test verifies onboarding validation behavior on locale switch/re-render.
- Context note:
docs/VISION.mdanddocs/PRINCIPLES.mdare Not found in repo/docs in this checkout.
Testing
- Not run (automation)
open-codesign Bot
All three onboarding screens (Welcome, PasteKey, ChooseModel) now use useT() so they re-render immediately when the user toggles the language toggle in the onboarding header. - Welcome.tsx: title, subtitle, path-button labels/subtitles, "where to get a key" label all go through t() - PasteKey.tsx: title, description, all field labels, preset hint, connection-test button/status, error messages, back/continue buttons all go through t(); getErrorHint and getValidationErrorMessage now accept t() so they can return translated strings at call time - ChooseModel.tsx: title, description, model-picker labels/hints, cost notes, custom base URL note, back/saving/finish buttons all go through t() - en.json / zh-CN.json: replaced the placeholder stubs with full, accurate keys matching the actual UI copy; added ~30 new keys (statusIdle, statusDetecting, statusValidating, statusOk, error codes, advanced toggle, preset placeholder, apiKey label, baseUrl label, etc.) - packages/i18n/src/i18n.test.ts: added two new describe blocks that assert onboarding keys return correct English and Chinese strings after setLocale() — covers Welcome, PasteKey, and ChooseModel Signed-off-by: hqhq1025 <[email protected]>
react-i18next's t identity is not stable across renders. When useT()'s returned function was used in useEffect deps (e.g. PasteKey validation effect), the effect re-ran every render — repeatedly triggering provider detection and key validation. Wrap the return in useCallback keyed on i18n.language so identity stays stable until the locale changes. Signed-off-by: hqhq1025 <[email protected]>
hqhq1025
force-pushed
the
wt/fix-i18n-onboarding
branch
from
58bdfa5 to
0f25bc3
Compare
Contributor
There was a problem hiding this comment.
Findings
- No actionable issues found in added/modified lines.
Summary
- Review mode: follow-up after new commits
- No new issues found in the latest head diff.
- Constraint checks on changed lines: no direct provider SDK imports, no new dependencies/licenses, no new silent-fallback regression found, and no new hardcoded color/font-size literals were introduced.
- Not found in repo/docs:
docs/VISION.md,docs/PRINCIPLES.md,docs/RESEARCH_QUEUE.md. - Residual risk/testing gap: added tests validate locale keys/values, but there is still no component-level test covering onboarding locale-toggle behavior with connection-test status transitions.
Testing
- Not run (automation)
open-codesign Bot
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
useT()— they were previously 100% hardcoded English with no i18n wiring at allenandzh-CNlocales (~30 net-new keys added, old placeholder stubs replaced with accurate copy)getErrorHintandgetValidationErrorMessagein PasteKey now acceptt()so translated error messages are returned at render time, not at module loadLanguageToggle(already present in the onboarding header) is clickedCompatibility, upgradeability, no bloat, elegance
useT()hook pattern used in every other componenti18n audit findings
Hardcoded strings fixed in this PR:
Welcome.tsx: title, subtitle, 3x PathButton title+subtitle pairs, "where to get a key" labelPasteKey.tsx: title, description, preset label/placeholder/hint, API key label, base URL label, advanced toggle/description, all status messages (idle/detecting/validating/ok), all error messages, connection-test button/states, back/continue buttonsChooseModel.tsx: title, description, primary/fast model picker labels and hints (both standard and free-tier variants), custom base URL note, cost notes, back/saving/finish buttonsOut of scope (pre-existing, no change):
Settings.tsxhas hardcoded<SectionTitle>strings but nouseT()at all — tracked separately.Test plan
pnpm -r test→ all 11 i18n tests pass, including 2 new onboarding locale-switching tests