fix(l10n): align port forwarding naming with 1.x - #1097
Conversation
- Rename "Port Triggering" to "Port Range Triggering" across all locales - Update tab labels to full names without count (moved count to section titles) - Update related strings: add/edit dialogs, empty state messages Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
AustinChangLinksys
left a comment
There was a problem hiding this comment.
🤖 Automated Review — Round 1 · 6108b1f..3edb7ee (full)
Verdict: 💬 Self-review (comment only) — No Critical issues; 7 Warnings and 2 Suggestions documented for reference. GitHub prohibits self-approval; review posted as comment only.
| Conf. | Where | Issue (one-liner) | |
|---|---|---|---|
| 🟢High | usp_single_port_tab.dart:33, usp_port_range_tab.dart:32, usp_port_triggering_tab.dart:32 |
[both reviewers] Hardcoded string interpolation '${loc} (${rules.length})' bypasses l10n; format is non-localizable for RTL / long-string locales |
|
| 🟢High | golden_runner.dart:93,143 |
[both reviewers] Double Localizations wrapper outside a MaterialApp.router that already sets locale + delegates — redundant and behavior undefined with alchemist |
|
| 🟢High | lib/l10n/gen/ (gitignored) |
gen/ excluded from PR; CI must run flutter gen-l10n or build fails after ARB key removals |
|
| 🟢High | usp_port_forwarding_card.dart:57 |
portTriggering value renamed silently updates Dashboard Card section title — unmentioned in PR scope |
|
| 🟢High | usp_port_forwarding_detail_view.dart:77-80 + 3 tab components |
Count display responsibility split: Tab bar = static (no count) vs. tab content = dynamic count — violates single responsibility, adds maintenance burden | |
| 🟡Med | app_en.arb (new *Tab keys) |
New singlePortForwardingTab / portRangeForwardingTab / portRangeTriggeringTab may duplicate existing keys — 26 locales would need double maintenance |
|
| 🟢High | test/ (missing) |
No widget tests for changed count display location; existing goldens will fail CI after text changes | |
| 💡 | 🟢High | All 26 .arb + usp_port_forwarding_card.dart |
[both reviewers] portTriggering rename affects dashboard card goldens; include updated goldens or explicit --update-goldens note |
| 💡 | 🟢High | usp_single_port_tab.dart:33, usp_port_range_tab.dart:32, usp_port_triggering_tab.dart:32 |
Same count format string repeated in 3 files — extract shared helper or use parameterized l10n key |
Confidence: 🟢High = code-verified · 🟡Med = located + reasoned, not fully confirmed · ⚪Low = speculative, please double-check.
Items marked [both reviewers] were independently flagged by two agents → higher confidence.
⚠️ Warning Details
W1 · [both reviewers] Hardcoded count interpolation bypasses l10n 🟢High
usp_single_port_tab.dart:33, usp_port_range_tab.dart:32, usp_port_triggering_tab.dart:32
// usp_single_port_tab.dart:33 (post-PR)
AppText.titleMedium('${loc(context).singlePortForwarding} (${rules.length})')
// same pattern in the other two tab filesThe format (N) is hardcoded — parentheses, spacing and number position cannot be customised per locale. Issues:
- RTL (Arabic, Hebrew):
(N)is appended on the logical right, but renders on the left in RTL context — inverted from user expectation. - Long-string locales (German, Finnish):
AppText.titleMediumis not wrapped inExpandedinside itsRow, so appending extra characters risks overflow / truncation. - PR simultaneously deletes the parameterized
singlePortWithCount/portRangeWithCount/triggeringWithCountkeys, which were the correct l10n mechanism for this pattern.
Fix: Re-introduce parameterized l10n keys (or repurpose the deleted ones with updated values), so each locale controls number placement. Also wrap AppText.titleMedium in Expanded.
W2 · [both reviewers] Double Localizations wrapper in golden_runner 🟢High
test/golden_test/golden_framework/golden_runner.dart:93,143
// PR adds outer Localizations around widget (golden_runner.dart:93)
await tester.pumpWidget(
Localizations(
locale: locale,
delegates: AppLocalizations.localizationsDelegates,
child: widget,
),
);
// But _buildGoldenWidget() already builds MaterialApp.router with:
// locale: locale, localizationsDelegates: AppLocalizations.localizationsDelegateswidget at that call site is already a MaterialApp.router (produced by _buildGoldenWidget) which sets locale and localizationsDelegates internally. The outer Localizations wrapper is logically redundant and interacts with alchemist's own FlutterGoldenTestWrapper localizations fallback in an undefined way. If this fixes a specific test failure (e.g. loc(context) unavailable in dialogs opened via root navigator), the fix belongs in _buildGoldenWidget or in the MediaQueryData scaffold, not at pumpWidget level. Please add a comment explaining the root cause.
W3 · gen/ files not in PR — CI build risk 🟢High
lib/l10n/gen/ (gitignored)
All lib/l10n/gen/app_localizations_*.dart are gitignored. The PR removes singlePortWithCount, portRangeWithCount, triggeringWithCount from all 26 ARBs and adds singlePortForwardingTab / portRangeForwardingTab / portRangeTriggeringTab. If CI starts from a clean state without running flutter gen-l10n, the generated Dart stubs will not match and the build will fail. Confirm CI pipeline regenerates l10n before compile.
W4 · portTriggering rename silently updates Dashboard Card 🟢High
lib/page/port_forwarding/cards/usp_port_forwarding_card.dart:57
// usp_port_forwarding_card.dart:57 (head version)
CardSection(
title: loc(context).portTriggering, // <- Dashboard Card section title
...
)The key portTriggering is used in two places: the tab content title (usp_port_triggering_tab.dart) and the Dashboard Card section header (usp_port_forwarding_card.dart:57). The PR renames its value to "Port Range Triggering" across all 26 locales. The Dashboard Card will silently pick up the new string. If this is intentional (aligning 1.x naming end-to-end), confirm explicitly in the PR description. If not intentional, the Card needs its own dedicated key.
W5 · Count responsibility split across 4 files 🟢High
usp_port_forwarding_detail_view.dart:77-80 (Tab bar: no count) vs. usp_single_port_tab.dart:33, usp_port_range_tab.dart:32, usp_port_triggering_tab.dart:32 (content titles: count)
Previously count was shown in one place (Tab bar labels, detail_view.dart). Post-PR, Tab bar is static and three sub-components each independently construct "${label} (N)". Any future requirement to change count display (show/hide, change format, add tooltip) now requires touching 4 files instead of 1. Document the intentional design decision.
W6 · New *Tab keys may duplicate existing keys 🟡Med
app_en.arb — new singlePortForwardingTab, portRangeForwardingTab, portRangeTriggeringTab
If the new Tab keys carry the same values as singlePortForwarding / portRangeForwarding / portTriggering, that is 6 keys where 3 suffice — every translator must maintain both sets. If values differ, Tab bar and content-area headings will display different strings for the same feature. Confirm whether distinct keys are truly necessary, and document the intended values explicitly in the PR.
W7 · Missing widget tests for count display change 🟢High
No tests added covering:
- Tab bar shows static text (no count)
- Tab content title shows
"${label} (N)"format
Existing golden tests (usp_port_forwarding_detail_view_test.dart, dashboard card goldens) reference portTriggering and tab label text — these will fail CI after this PR without golden regeneration.
✅ What looks good
- No security issues — no hardcoded secrets, no injection vectors, no permission/access-control changes, no JNAP/USP response handling modifications.
- Removed l10n key cleanup is complete —
singlePortWithCount,portRangeWithCount,triggeringWithCountare confirmed to have no other usages outside ofdetail_view.dart(which was updated). - Consistent naming update across all 26 locales —
addPortTriggering,editPortTriggering,noPortTriggeringRules,portTriggeringall updated in lock-step; no locale was missed. - ARB file trailing comma fix — all 26 locale files fix the missing trailing comma before the new keys at end-of-object.
- Localizations fix intent is sound — wrapping golden pump with
AppLocalizations.localizationsDelegatesto support dialogs opened via root navigator is a valid problem to solve; implementation approach just needs refinement.
Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.
PeterJhongLinksys
left a comment
There was a problem hiding this comment.
Verified l10n key integrity across all 26 locales:
- Each new key (singlePortForwardingTab, portRangeForwardingTab, portRangeTriggeringTab) is added in all 26 ARB files; each removed key (singlePortWithCount, portRangeWithCount, triggeringWithCount) is deleted in all 26 — no locale missed.
- Template metadata (@singlePortWithCount, etc.) removed cleanly from app_en.arb.
- The only Dart consumer of the removed keys (usp_port_forwarding_detail_view.dart) is updated to the new keys in this PR — no dangling references.
- New keys carry no placeholders; removed keys' {count} placeholders removed cleanly — no broken/mismatched placeholders.
- portTriggering / noPortTriggeringRules / add/editPortTriggering are value-only renames (keys preserved), so no usage breakage.
No blocking issues found. The naming/architecture and test-coverage points (hardcoded count interpolation, double Localizations wrapper, gen/ CI regeneration, dashboard-card value pickup, golden regeneration) are already captured in the automated Round-1 review and are non-blocking for merge.
PeterJhongLinksys
left a comment
There was a problem hiding this comment.
Verified all 26 ARB locales add/remove the port-forwarding tab keys in lock-step (removed singlePortWithCount/portRangeWithCount/triggeringWithCount, added singlePortForwardingTab/portRangeForwardingTab/portRangeTriggeringTab), the sole Dart consumer (usp_port_forwarding_detail_view.dart) is updated with no dangling references, and placeholders are handled cleanly. No blocking issues.
Non-critical (cosmetic): a couple of translation-consistency nits — app_es_ar.arb uses "Añadir" for addPortTriggering while the rest of the file uses "Agregar"; a few locales (fi, ko, pl, pt_pt, th, vi) phrase noPortTriggeringRules slightly differently from their sibling "no…Rules" strings. Wording only, no functional impact.
…ing-naming-consistency # Conflicts: # lib/l10n/app_ar.arb # lib/l10n/app_da.arb # lib/l10n/app_de.arb # lib/l10n/app_el.arb # lib/l10n/app_es.arb # lib/l10n/app_es_ar.arb # lib/l10n/app_fi.arb # lib/l10n/app_fr.arb # lib/l10n/app_fr_ca.arb # lib/l10n/app_id.arb # lib/l10n/app_it.arb # lib/l10n/app_ja.arb # lib/l10n/app_ko.arb # lib/l10n/app_nb.arb # lib/l10n/app_nl.arb # lib/l10n/app_pl.arb # lib/l10n/app_pt.arb # lib/l10n/app_pt_pt.arb # lib/l10n/app_ru.arb # lib/l10n/app_sv.arb # lib/l10n/app_th.arb # lib/l10n/app_tr.arb # lib/l10n/app_vi.arb # lib/l10n/app_zh.arb # lib/l10n/app_zh_TW.arb
…ing-naming-consistency
|
Naming consistency: the new The three new tab keys are, in English, identical to keys that already exist and are still used as the section titles inside each tab on the same screen:
Since the tab label is just the section name without the count, we could reuse the existing keys directly instead of adding three new ones. Beyond the redundancy, because the new keys were translated independently, their translations diverge from the existing section keys in most locales — so the same concept renders two different ways on one screen (tab vs. the section title inside it). A few examples:
Recommendation: drop the three new |
|
🤖 Automated Review — Oversize PR This round's changes exceed the automated-review limit (14146 lines / 190 files, limit 6000 lines / 100 files); AI review was not run. Manual review recommended. First 15 changed files (for a quick scan): |
…#1097 review) Drop the three new *Tab keys (singlePortForwardingTab, portRangeForwardingTab, portRangeTriggeringTab) and reuse the existing section keys (singlePortForwarding, portRangeForwarding, portTriggering) for the tab labels. The new keys duplicated existing keys with identical English values but were translated independently, causing the same concept to render two different ways on one screen (e.g. zh_TW tab "轉發" vs. section title "轉寄"). Reusing the section keys removes the duplication and the translation divergence in one step. English UI is unchanged (identical values); other locales now use one consistent translation per concept. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Good catch — adopted the recommendation in 973aad1. Dropped the three new English UI is unchanged since the values were identical, and the diff now shrinks by 78 lines (26 locales × 3 keys removed). |
HankYuLinksys
left a comment
There was a problem hiding this comment.
Verified the fix in 973aad1 — the three *Tab keys are fully removed across all 26 locales and the tab labels now reuse the existing section keys (singlePortForwarding / portRangeForwarding / portTriggering). This resolves both the duplication and the per-locale translation divergence. English UI unchanged; no dangling references remain.
Naming-consistency issue is fully addressed. LGTM 👍
Summary
AppLocalizationsFixes #1081
Changes
pumpWidgetwithLocalizationsto fix dialog localization in golden testsTest plan
dart formatpassedflutter analyzepassed🤖 Generated with Claude Code