UI language toggle and static string lookup (frontend-only)#2187
Open
zerofrip wants to merge 2 commits into
Open
UI language toggle and static string lookup (frontend-only)#2187zerofrip wants to merge 2 commits into
zerofrip wants to merge 2 commits into
Conversation
Frontend-only pilot using a pure t() function and flat-key JSON dictionaries. No backend, build pipeline, or persistence changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Consolidates display-language selection into Settings as the single UI entry point. Navigation subscribes via a lightweight window event for OnPush tooltip refresh. No backend or i18n architecture changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
📝 WalkthroughWalkthroughA new static translation module ( ChangesStatic i18n with EN/JA language switcher
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
desktop/angular/src/app/pages/settings/settings.ts (1)
54-54: ⚡ Quick winExtract the language-change event name into a shared constant.
'portmaster-ui-lang-change'is part of a cross-component contract (Settings dispatch, Navigation subscribe). Keeping it as duplicated string literals is brittle; a shared exported constant would prevent silent breakage from typos.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/angular/src/app/pages/settings/settings.ts` at line 54, Extract the hardcoded event name string 'portmaster-ui-lang-change' used in the window.dispatchEvent call into a shared exported constant in a common constants file or shared utilities module. Then import and use this constant in the window.dispatchEvent call in the Settings component instead of the string literal. Also update any other components (such as Navigation) that subscribe to this same event to import and use the same shared constant instead of duplicating the string literal, ensuring consistent event naming across all components.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/angular/src/app/pages/settings/settings.html`:
- Around line 15-20: The EN and JA language toggle buttons lack accessibility
attributes for screen reader users to determine the active language state. Add
aria-pressed attribute bindings to both buttons using [attr.aria-pressed] that
dynamically bind to the condition matching the current uiLanguage value. For the
EN button, bind aria-pressed to uiLanguage === 'en', and for the JA button, bind
it to uiLanguage === 'ja' so screen readers can announce the selected state.
---
Nitpick comments:
In `@desktop/angular/src/app/pages/settings/settings.ts`:
- Line 54: Extract the hardcoded event name string 'portmaster-ui-lang-change'
used in the window.dispatchEvent call into a shared exported constant in a
common constants file or shared utilities module. Then import and use this
constant in the window.dispatchEvent call in the Settings component instead of
the string literal. Also update any other components (such as Navigation) that
subscribe to this same event to import and use the same shared constant instead
of duplicating the string literal, ensuring consistent event naming across all
components.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 142bc819-e6ea-4892-8fe6-69eb99978d35
📒 Files selected for processing (9)
assets/data/i18n/en.jsonassets/data/i18n/ja.jsondesktop/angular/src/app/i18n/static-translate.spec.tsdesktop/angular/src/app/i18n/static-translate.tsdesktop/angular/src/app/layout/navigation/navigation.htmldesktop/angular/src/app/layout/navigation/navigation.tsdesktop/angular/src/app/pages/settings/settings.htmldesktop/angular/src/app/pages/settings/settings.tsdesktop/angular/tsconfig.json
Comment on lines
+15
to
+20
| <button type="button" class="px-2 py-1 rounded bg-gray-300 hover:bg-gray-200" | ||
| [class.font-semibold]="uiLanguage === 'en'" | ||
| (click)="setLanguage('en')">EN</button> | ||
| <button type="button" class="px-2 py-1 rounded bg-gray-300 hover:bg-gray-200" | ||
| [class.font-semibold]="uiLanguage === 'ja'" | ||
| (click)="setLanguage('ja')">JA</button> |
Contributor
There was a problem hiding this comment.
Expose selected language state for assistive tech.
The EN/JA toggle currently signals selection only visually. Add aria-pressed bindings so screen-reader users can detect the active language.
Suggested change
- <button type="button" class="px-2 py-1 rounded bg-gray-300 hover:bg-gray-200"
+ <button type="button" class="px-2 py-1 rounded bg-gray-300 hover:bg-gray-200"
+ [attr.aria-pressed]="uiLanguage === 'en'"
[class.font-semibold]="uiLanguage === 'en'"
(click)="setLanguage('en')">EN</button>
- <button type="button" class="px-2 py-1 rounded bg-gray-300 hover:bg-gray-200"
+ <button type="button" class="px-2 py-1 rounded bg-gray-300 hover:bg-gray-200"
+ [attr.aria-pressed]="uiLanguage === 'ja'"
[class.font-semibold]="uiLanguage === 'ja'"
(click)="setLanguage('ja')">JA</button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/angular/src/app/pages/settings/settings.html` around lines 15 - 20,
The EN and JA language toggle buttons lack accessibility attributes for screen
reader users to determine the active language state. Add aria-pressed attribute
bindings to both buttons using [attr.aria-pressed] that dynamically bind to the
condition matching the current uiLanguage value. For the EN button, bind
aria-pressed to uiLanguage === 'en', and for the JA button, bind it to
uiLanguage === 'ja' so screen readers can announce the selected state.
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
Presentation-layer update: a small UI language toggle in Settings (EN/JA) drives existing static string lookup for a handful of nav tooltips and one settings heading. No backend, integration, build pipeline, or service-layer changes.
UI state consolidation
setLanguage()is the only UI mutation entry pointt(), refreshes on a lightweight window event (OnPush)Scope (intentionally limited)
assets/data/i18n/en.json,ja.jsont()lookup instatic-translate.ts— no services, pipes, HTTP, or persistenceTest plan
ng buildsucceedsstatic-translatelogic validated (6 tests via direct execution)Note: Karma coverage reporter is not installed in the local environment; tests were validated via direct execution.
Reversibility
Fully frontend-only. Removable without backend or configuration impact.
Made with Cursor
Summary by CodeRabbit
日本語要約
概要
プレゼンテーション層のみの小さな変更です。Settings ページの UI 言語トグル（EN/JA）が、既存の静的文字列 lookup（
t()）を通じて、少数の Navigation ツールチップと Settings 見出しの表示を切り替えます。バックエンド・integration・ビルドパイプライン・サービス層への変更はありません。UI 状態の整理
setLanguage()）t()でツールチップを表示し、軽量 window イベントで OnPush 再描画スコープ（意図的に限定）
assets/data/i18n/en.json,ja.jsont()（static-translate.ts）— サービス・pipe・HTTP・永続化なしテスト
ng build成功static-translateロジック検証（直接実行、6 テスト）補足: ローカル環境に Karma coverage reporter が未インストールのため、テストは直接実行で検証済みです。
可逆性
フロントエンドのみの変更。バックエンドや設定への影響なく削除可能です。