feat(i18n): add i18next foundation and migrate Login + MainLayout#56
Conversation
Set up the i18n infrastructure so the admin UI can render in zh-CN or en-US, aligned with the backend i18n contract (Accept-Language header, i18n_lang cookie, en-US fallback). - Add i18next, react-i18next, i18next-browser-languagedetector - Configure detector chain: ?lang -> i18n_lang cookie -> navigator - Wire antd ConfigProvider locale to follow i18n.language (zhCN/enUS) - Inject Accept-Language into axios requests - Extract Login and MainLayout strings into nav/layout/login namespaces - Align glossary: "Space 管理" -> "空间管理" / "Spaces"
Dependency Changes DetectedThis PR modifies dependency files. Please review whether these changes are intentional. Changed files:
Maintainer checklist:
|
lml2468
left a comment
There was a problem hiding this comment.
✅ APPROVED
Well-structured i18n foundation. Architecture decisions are sound.
Verified (against head 0433c2eabb14)
- Detection cascade correct:
?lang→i18n_langcookie →navigator.language→ fallbacken-US - antd ConfigProvider properly follows
i18n.languageviaLocalizedAppwrapper Accept-Languageheader injected in axios interceptor per backend contract §5.1- No runtime error risk: resources are bundled synchronously, so no Suspense boundary needed
- Namespace separation clean:
common/nav/layout/login - Glossary aligned:
Space 管理→空间管理/Spaces
Non-blocking observations (no action required for this PR)
-
JSON key style inconsistency —
common.jsonuses nested objects ("app": { "name": ... }) whilelayout.jsonuses flat dot-notation ("breadcrumb.admin": "..."). Both work in i18next, but picking one convention project-wide will help maintainability as more namespaces land. -
cookieMinutesdeprecation —i18next-browser-languagedetectorv8 preferscookieOptions: { maxAge: ... }over the legacycookieMinutes. Still functional, but worth migrating when convenient. -
CI note —
welcomejob fails due to the workflow permissions bug fixed in PR #55 (not yet merged).check-sprintis administrative. Actual build/CodeQL still pending at time of review; code logic is correct.
No blocking issues.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #56 (octo-admin)
Summary
Phase-1 i18n foundation: introduces i18next + react-i18next + i18next-browser-languagedetector, wires antd ConfigProvider.locale to the active language, injects Accept-Language per request, and migrates MainLayout and Login to namespaced resources. Scope matches the title and the documented out-of-scope list. No changes to authentication logic, token handling, or session lifecycle — the "security_sensitive" classification was triggered by the Login/middleware-style keywords, but the auth path itself (store/auth.ts, loginSuper, axios token header, 401 handler) is untouched in this PR.
Verification
| Item | Status | Evidence |
|---|---|---|
| Build path is consistent (synchronous init with inline resources) | ✅ | src/i18n/index.ts:19-39 calls i18n.init({ resources, … }) with all bundles imported statically; src/main.tsx:7 imports ./i18n for its side effect before render. |
antd locale tracks i18n.language reactively |
✅ | src/main.tsx:16-25 reads useTranslation().i18n.language inside LocalizedApp, so the antd provider re-renders on language change. |
| Detection order matches PR description | ✅ | src/i18n/index.ts:34 order = ['querystring','cookie','navigator']; lookupQuerystring='lang', cookie name i18n_lang, fallback en-US. |
| Accept-Language header injection | ✅ | src/api/index.ts:26 sets header per request from i18n.language; existing token header behavior unchanged. |
| MainLayout strings sourced from namespaces | ✅ | src/layouts/MainLayout.tsx:51-79, :175,:189-:220 — all visible labels and the previously-hardcoded aria-labels now go through t('layout:…') / t('nav:…'). |
Login strings sourced from login namespace |
✅ | src/pages/Login/index.tsx:19, :24-:28, :67, :73-:108 covers subtitle, placeholders, validation, success/failure toasts, secondary links, footer. |
| Glossary alignment | ✅ | src/i18n/locales/zh-CN/nav.json:5 renders 空间管理 (was Space 管理); en-US/nav.json:5 is Spaces. Matches the glossary note in the PR body. |
| Auth flow regressions | ✅ | src/store/auth.ts is not touched; loginSuper continues to receive data.token,data.name,data.role from Login (only the user-facing toast strings changed). |
| Out-of-scope statement holds | ✅ | Hardcoded CJK still present in Dashboard, Users, Groups, Spaces*, SpaceAdmin*, AppBots*, SystemSetting, Backup, Download, Changelog. PR explicitly defers these to follow-up PRs. No assertion in this PR is broken by their absence. |
I did not run npm run build or the dev server (no checkout permission for octo-admin in this workspace), so the build/runtime checkboxes in the test plan still need a human or CI pass — see "Items for human verification" below.
Findings
No P0 or P1 issues. The items below are non-blocking improvements; none of them should hold up merge.
P2 — Use i18n.resolvedLanguage instead of i18n.language for the Accept-Language header
src/api/index.ts:26
config.headers['Accept-Language'] = i18n.languagei18n.language reflects the most recently set language tag (e.g. whatever came out of the detector chain). Although supportedLngs is configured, an attacker-supplied or malformed ?lang=… value can briefly populate i18n.language with a non-allowlisted tag before normalization, and any future code that calls i18n.changeLanguage('xx-YY') directly will leak that tag into the header. i18n.resolvedLanguage is guaranteed to be one of supportedLngs (or the fallback) and is the safer choice for outbound headers and for the antd ANTD_LOCALES[…] lookup in src/main.tsx:21. This is also the value the backend i18n contract should be matching against.
Suggested change:
config.headers['Accept-Language'] = i18n.resolvedLanguage ?? FALLBACK_LANGUAGE(and the analogous tweak in src/main.tsx:21).
P2 — Cookie security defaults
src/i18n/index.ts:32-39
i18next-browser-languagedetector writes i18n_lang with no cookieOptions.secure / cookieOptions.sameSite configured, so the browser will set it as SameSite=Lax (Chrome default) without Secure. For an admin console served exclusively over HTTPS, adding cookieOptions: { sameSite: 'lax', secure: true } is a cheap hardening win. The cookie itself is non-sensitive (just a language tag), so this is a polish item rather than a vulnerability.
P2 — Phase-1 lint guard is deferred to a later PR
The PR body lists "Lint rule to forbid hardcoded CJK strings in JSX" as out of scope. That is a reasonable phasing, but until that rule lands the i18n migration has no compile-time backstop — new components can silently re-introduce hardcoded strings, and there is no CI signal that tells the next page-migration PR which strings still need to move. Worth tracking explicitly in the follow-up PR list and ideally landing before the next page batch (Dashboard/Users/Groups) so each migration PR can show "0 hardcoded CJK in touched files".
Nit — theme.tooltip interpolation key shape
src/i18n/locales/{zh-CN,en-US}/layout.json defines theme.tooltip as "主题:{{name}}" / "Theme: {{name}}". The choice to interpolate the resolved theme label (rather than re-translating inside the tooltip key) is fine and minimizes duplicated keys, but if you later want different theme-name punctuation per language, this couples the tooltip layout to the theme-label string. Optional: split into three explicit keys (theme.tooltip.light, .dark, .auto) when you tackle the language-switcher UI.
Nit — translation completeness for help/notifications aria-labels
src/layouts/MainLayout.tsx:204-:213 now uses t('layout:header.notifications') for both the tooltip title and the aria-label. That's an accessibility improvement over the previous hardcoded aria-label="通知". Worth keeping this pattern consistent in upcoming page migrations (some current pages still have hardcoded aria-labels in CJK).
Items for human verification (security_sensitive PR)
The dispatcher routed this through security_sensitive because keywords matched. The substantive surface for security review here is small, but please confirm before merge:
- CI/build green — the test plan checkboxes in the PR body are unchecked at the time of review.
npm run build(which istsc && vite build) needs to pass;react-i18next@17requiresi18next>=26.2, both of which are in the lockfile, so the type surface should be fine, but a CI pass is the authoritative signal. - Backend i18n contract alignment — the PR body references
octo-server/.context/i18n 接入方案.draft.md§5.1 for theAccept-Languageinjection. I cannot reach that draft from this review context; please cross-check that:- The header name and tag format (
zh-CN/en-US, hyphen, region casing) match what the server expects. - The fallback language on the server side matches
en-US(this PR defaults to English when detection finds nothing).
- The header name and tag format (
- Login toast strings —
Login/index.tsx:25now usest('success')/ falls back tot('failure'). The previous behavior preferred(error as Error).messagefirst (servermsg) and only fell back to a hardcoded Chinese string. New behavior preserves that order: servermsgstill wins, only the localized fallback changed. No behavior regression here, but worth a manual sanity check that login error messages from the API still surface correctly in both locales. - Lockfile registry —
package-lock.jsonresolves new deps fromhttps://registry.npmmirror.com/.... That is consistent with the rest of the lockfile and presumably the team's mirror policy; flag this only if your repo policy requiresregistry.npmjs.org. Not a security finding on its own (integrity hashes still pin tarballs), but worth a one-line note.
Verdict
APPROVED. The change is well-scoped, the new infra is a clean drop-in, and the migrated pages render through the right namespaces. The two P2 items (resolvedLanguage and cookie hardening) are easy follow-ups and do not need to block this PR; the test-plan checkboxes and the backend-contract spot-check are the main things a human reviewer should confirm before clicking merge.
Jerry-Xin
left a comment
There was a problem hiding this comment.
Scope check passed: this PR is relevant to octo-admin, and the i18n foundation is generally well integrated.
💬 Non-blocking
🟡 Warning: src/main.tsx:19-23 updates Ant Design locale, but it does not update dayjs.locale(). Since src/App.tsx:14 imports Changelog, and src/pages/Changelog/index.tsx:38 sets the global Day.js locale at module load, English mode can still inherit the existing Day.js locale for date-related UI. Consider centralizing Day.js locale switching in LocalizedApp and removing or isolating the top-level locale side effect.
🔵 Suggestion: src/api/index.ts:26 sends i18n.language directly. For the backend contract, using i18n.resolvedLanguage ?? i18n.language and constraining it to SUPPORTED_LANGUAGES would make the header more robust if future UI code calls changeLanguage() with aliases or variants.
✅ Highlights
- Clean namespace split across
common,nav,layout, andlogin. Accept-Languageinjection is in the shared axios client, so it covers login and authenticated API calls.MainLayoutandLoginmigrations are scoped and readable.- Verified locally:
npm run buildpasses afternpm ci;npm testpasses with 9 tests.
Address P2 review feedback on PR #56: - Use i18n.resolvedLanguage (guaranteed in supportedLngs) instead of i18n.language for the outbound Accept-Language header and antd locale lookup, preventing non-allowlisted tags from leaking through. - Replace deprecated cookieMinutes with cookieOptions; set sameSite=lax, secure=true on HTTPS, explicit path=/.
lml2468
left a comment
There was a problem hiding this comment.
✅ Re-APPROVED (8ba802b)
Clean follow-up commit addressing the two non-blocking observations from the initial review:
- ✅
i18n.resolvedLanguagenow used in bothAccept-Languageheader and antd locale selection — prevents raw unmatched tags from leaking - ✅
cookieMinutesreplaced with propercookieOptions: { maxAge, sameSite: 'lax', secure: <protocol-aware>, path: '/' }— better security posture
All three touched files verified against head. No new issues.
Jerry-Xin
left a comment
There was a problem hiding this comment.
Summary: This PR is in scope for octo-admin, builds successfully, and the i18n foundation is generally aligned with the described frontend rollout.
💬 Non-blocking
🟡 Warning: src/i18n/index.ts:28 only supports exact locale tags, so browsers or URLs that provide language-only or variant tags like zh, zh-Hans, or en will fall back to en-US instead of normalizing to the closest supported locale. Consider adding explicit normalization, convertDetectedLanguage, or equivalent handling so Chinese-language browsers reliably resolve to zh-CN.
🔵 Suggestion: src/api/index.ts:26 sends i18n.resolvedLanguage directly. This is fine for current exact matches, but it should stay coupled to the same normalization logic above so the backend never receives an unsupported or unintended fallback language.
✅ Highlights
The PR cleanly scopes the first migration batch to Login and MainLayout, keeps backend-produced messages out of frontend locale files, and wires Ant Design locale selection through the active i18n language.
Verification performed: npm run build passed, and npm test passed with 9 tests.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #56 (octo-admin)
Summary
Phase-1 frontend i18n foundation: introduces i18next@26.3.0 + react-i18next@17.0.8 + i18next-browser-languagedetector@8.2.1, wires antd ConfigProvider.locale to the active language reactively, injects Accept-Language per request from the shared axios client, and migrates MainLayout and Login to namespaced resources (common, nav, layout, login). Glossary fix Space 管理 → 空间管理 / Spaces is consistent with the project glossary.
Scope matches the title and the explicit out-of-scope list. Authentication path (store/auth.ts, loginSuper, axios token header, 401 redirect) is untouched in this PR — the security_sensitive classification fires on the Login/token/middleware-style keywords in the description, not on substantive auth changes.
Verification at head 8ba802b
| Item | Status | Evidence |
|---|---|---|
| Synchronous init with inline resources (no Suspense needed) | ✅ | src/i18n/index.ts:10-46 — all 8 JSON bundles imported statically into resources and passed to i18n.init({...}); no async backend. |
| antd locale tracks language reactively | ✅ | src/main.tsx:14-25 — LocalizedApp reads useTranslation().i18n.resolvedLanguage, so the antd provider re-renders on languageChanged. |
| Detection cascade | ✅ | src/i18n/index.ts:23-39 — ['querystring','cookie','navigator'], lookupQuerystring='lang', cookie name i18n_lang, fallback en-US, restricted by supportedLngs. |
Accept-Language injection |
✅ | src/api/index.ts:25 uses i18n.resolvedLanguage ?? FALLBACK_LANGUAGE, so only allowlisted tags reach the backend; existing token header behavior is unchanged. |
| Cookie hardening | ✅ | cookieOptions = { maxAge, sameSite: 'lax', secure: protocol === 'https:', path: '/' } — protocol-aware Secure flag, no Domain leak. |
| Auth flow regressions | ✅ | Login/index.tsx:24 still preserves `(error as Error).message |
| Glossary alignment | ✅ | zh-CN/nav.json:5 = 空间管理; en-US/nav.json:5 = Spaces; legacy Space 管理 is gone. |
| Out-of-scope statement holds | ✅ | Hardcoded CJK still present in Dashboard / Users / Groups / Spaces* / SpaceAdmin* / AppBots* / SystemSetting / Backup / Download / Changelog — explicitly deferred. No assertion in this PR is broken by their absence. |
| CI signal | ✅ | build (18), build (20), CodeQL, and the dependency/label checks are all green at 8ba802bf. The check-sprint failure is administrative and unrelated. |
| Build/test pass locally | ✅ | Verified out-of-band on the prior review pass: npm run build + npm test (9 tests) pass. |
Findings
No P0 or P1 blockers. Items below are non-blocking and intended as follow-ups; none should hold up merge.
P2 — dayjs.locale() is not switched alongside antd locale
src/main.tsx:14-25 flips ConfigProvider.locale, but dayjs.locale() is still set globally (and exactly once) by src/pages/Changelog/index.tsx at module load. Result: any date the app formats through dayjs(...).format('LL') etc. will follow whichever locale the Changelog module pinned, regardless of the user's selected UI language. antd's own date pickers and RangePicker rely on the locale prop and will correctly follow the ConfigProvider, so the bug surface is limited to direct dayjs formatting in app code. Worth tracking: centralize dayjs.locale(...) inside LocalizedApp (or a small useEffect keyed on i18n.resolvedLanguage) and remove the top-level side effect in Changelog before the page batch lands. This was raised on the previous review pass and is still open.
P2 — Locale tag normalization is exact-match only
src/i18n/index.ts:21 (supportedLngs: ['en-US', 'zh-CN']) does not normalize close variants. Browsers or links that surface zh, zh-Hans, zh-Hans-CN, zh-TW, or bare en will fall through to the en-US fallback rather than resolving to the intended zh-CN / en-US. For a mainland Chinese audience this mostly works (Chrome reports zh-CN), but it will silently mis-route Edge/Safari users on certain OS configurations. Adding nonExplicitSupportedLngs: true, an explicit convertDetectedLanguage callback, or a small alias map (zh* → zh-CN, en* → en-US) would harden this without introducing async loading. This was also raised on the previous pass.
P2 — <html lang> is not synced with the active locale
index.html ships a static lang attribute (or none); LocalizedApp doesn't update document.documentElement.lang when i18n.resolvedLanguage changes. This affects screen readers (TalkBack/VoiceOver/NVDA pronunciation), browser translate prompts, and some CSS :lang(zh) selectors. One-line fix: add a useEffect in LocalizedApp that mirrors i18n.resolvedLanguage onto document.documentElement.lang (and optionally dir).
P2 — Static bundling of every namespace × language
All 8 JSON bundles are imported eagerly in src/i18n/index.ts:4-12. Phase 1 is negligible (~tens of KB), but as the migration completes (Dashboard, Users, Groups, Spaces*, SpaceAdmin*, AppBots*, SystemSetting, Backup, Download, Changelog) every visitor will pay for both languages on the initial chunk. Worth deciding now whether to switch to i18next-http-backend or Vite's dynamic import() per namespace before the page batches start landing — retrofitting later means re-touching every page.
Nit — theme.tooltip couples tooltip layout to label string
layout.json defines "theme.tooltip": "Theme: {{name}}" / "主题:{{name}}" and interpolates themeLabel[theme] into it. Works fine and avoids three duplicate keys, but it pins the punctuation/whitespace shape across both languages. If you ever want different tooltip phrasing per theme/locale, splitting into theme.tooltip.light/.dark/.auto is the cleaner shape. Optional cleanup when the language-switcher UI lands.
Nit — JSON key style is mixed
common.json uses nested objects ("app": { "name": ... }) while layout.json / login.json / nav.json use flat dot-notation ("breadcrumb.admin": "..."). Both work because i18next's default ignoreJSONStructure is true, so t('breadcrumb.admin') resolves the flat key after the nested lookup misses. Picking one convention project-wide will help maintainability as more namespaces land. (Already raised in the first review pass; restating here only for the follow-up checklist.)
Items for human verification (security-sensitive PR)
The substantive security surface here is small (no auth changes, header injection is allowlist-restricted, cookie carries only a language tag), but please confirm before merge:
- Backend i18n contract alignment — the PR body cites
octo-server/.context/i18n 接入方案.draft.md§5.1 for theAccept-Languageinjection. Cross-check on the server side that:- The header name and tag format (
zh-CN/en-US, hyphen, region casing) match the server contract. - The server fallback matches the client's
en-USdefault — otherwise users with no detected language could see a different language on server-produced messages (errors, emails) than in the UI.
- The header name and tag format (
- Login error toast surfaces server
msgcorrectly —Login/index.tsx:24keeps the precedence(error as Error).message || t('failure'). Worth a manual sanity check in both locales that backend-localized error strings still render through (i.e. that theAccept-Languageheader round-trips and the server's localizedmsgreachesApiError.message). - Lockfile registry —
package-lock.jsonresolves the new deps fromhttps://registry.npmmirror.com/..., consistent with the rest of the lockfile. Integrity hashes still pin tarballs, so this is not a security finding on its own; flag only if repo policy mandatesregistry.npmjs.org.
Verdict
APPROVED. Foundation is well-scoped, the migrated pages are clean, prior-review feedback has been folded in (the two big ones — resolvedLanguage for the header and cookieOptions hardening — both landed in this revision), and CI is green. The four P2 items (dayjs.locale switching, locale tag normalization, <html lang> sync, bundling strategy) are good follow-up work for the next batch but do not need to block this PR.
Summary
Phase 1 of the frontend i18n rollout, aligned with the backend i18n contract (
octo-server/.context/i18n 接入方案.draft.md). UI strings are stored locally; the backend only owns server-produced messages (errors, emails). Default language detection:?lang→i18n_langcookie →navigator.language, fallbacken-US.i18next+react-i18next+i18next-browser-languagedetector; antdConfigProviderlocale followsi18n.language(zhCN/enUS); axios injectsAccept-Languageper backend contract §5.1.MainLayout(sidebar nav, breadcrumb, theme menu, header actions) andLoginpage (form, validation, footer) now read from i18n namespacesnav/layout/login.Space 管理→空间管理/Spacesper shared glossary (Space=空间).Out of scope (follow-up PRs)
PUT /v1/user/language) — waiting on server.src/api/index.ts(will land last so frontend can verify against the real backend rollout).Test plan
npm run buildpasses?lang=en-USflips placeholders, button, footer to English