docs(34): add code review report
This commit is contained in:
294
.planning/phases/34-i18n-foundation/34-REVIEW.md
Normal file
294
.planning/phases/34-i18n-foundation/34-REVIEW.md
Normal file
@@ -0,0 +1,294 @@
|
||||
---
|
||||
phase: 34-i18n-foundation
|
||||
reviewed: 2026-04-17T00:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 23
|
||||
files_reviewed_list:
|
||||
- src/client/components/ClassificationBadge.tsx
|
||||
- src/client/components/ImageUpload.tsx
|
||||
- src/client/components/ImpactDeltaBadge.tsx
|
||||
- src/client/components/PlanningView.tsx
|
||||
- src/client/components/PublicSetupCard.tsx
|
||||
- src/client/components/SetupImpactSelector.tsx
|
||||
- src/client/components/ThreadCard.tsx
|
||||
- src/client/components/ThreadTabs.tsx
|
||||
- src/client/components/TotalsBar.tsx
|
||||
- src/client/locales/de/collection.json
|
||||
- src/client/locales/de/common.json
|
||||
- src/client/locales/de/onboarding.json
|
||||
- src/client/locales/de/settings.json
|
||||
- src/client/locales/de/setups.json
|
||||
- src/client/locales/de/threads.json
|
||||
- src/client/locales/en/collection.json
|
||||
- src/client/locales/en/common.json
|
||||
- src/client/locales/en/settings.json
|
||||
- src/client/locales/en/setups.json
|
||||
- src/client/locales/en/threads.json
|
||||
- src/client/routes/index.tsx
|
||||
- src/client/routes/profile.tsx
|
||||
- src/client/routes/settings.tsx
|
||||
findings:
|
||||
critical: 0
|
||||
warning: 7
|
||||
info: 4
|
||||
total: 11
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 34: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-04-17T00:00:00Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 23
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
This review covers the i18n foundation work: wiring components to `react-i18next`, locale JSON files for English and German, and three routes (`index`, `profile`, `settings`). The EN locale is complete and internally consistent. The DE locale has significant gaps — several keys used by components exist only in EN, meaning German users will see raw key strings or English fallback text in multiple places. Additionally, one component hardcodes a locale in a `toLocaleDateString` call (bypassing i18n entirely), one component shadows the `t` translation function with a filter callback variable, and two hardcoded English strings in `ImageUpload` were not extracted to the locale.
|
||||
|
||||
---
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: DE locale missing `tabs.setups`, `totals`, and `classificationBadge` keys
|
||||
|
||||
**File:** `src/client/locales/de/collection.json`
|
||||
**Issue:** The EN `collection.json` contains three top-level sections absent from the DE file: `tabs` (`tabs.setups`), `totals` (`totals.totalWeight`, `totals.totalCost`), and `classificationBadge` (`classificationBadge.base`, `classificationBadge.worn`, `classificationBadge.consumable`). `CollectionTabs` calls `t("tabs.setups")` and `ClassificationBadge` calls `t("classificationBadge.base")` as a `defaultValue`. When the language is German, these keys resolve to the raw key string unless a fallback language is configured in i18next.
|
||||
**Fix:** Add the missing keys to `de/collection.json`:
|
||||
```json
|
||||
"tabs": {
|
||||
"setups": "Setups"
|
||||
},
|
||||
"totals": {
|
||||
"totalWeight": "Gesamtgewicht",
|
||||
"totalCost": "Gesamtkosten"
|
||||
},
|
||||
"classificationBadge": {
|
||||
"base": "Basisgewicht",
|
||||
"worn": "Getragen",
|
||||
"consumable": "Verbrauchsmaterial"
|
||||
}
|
||||
```
|
||||
|
||||
### WR-02: DE locale missing `card.by`, `card.anonymous`, `impact.compareWith` in setups
|
||||
|
||||
**File:** `src/client/locales/de/setups.json`
|
||||
**Issue:** The EN `setups.json` contains `card.by`, `card.anonymous`, and `impact.compareWith`. The DE file omits all three. `PublicSetupCard` calls `t("card.by", { name: ... })` and `t("card.anonymous")` unconditionally; `SetupImpactSelector` calls `t("impact.compareWith")` as the blank option label. German users will see raw key strings in these locations.
|
||||
**Fix:** Add to `de/setups.json`:
|
||||
```json
|
||||
"card": {
|
||||
"items": "{{count}} Gegenstände",
|
||||
"items_one": "{{count}} Gegenstand",
|
||||
"weight": "Gewicht",
|
||||
"price": "Preis",
|
||||
"by": "von {{name}}",
|
||||
"anonymous": "Anonym"
|
||||
},
|
||||
"impact": {
|
||||
"title": "Auswirkungsvorschau",
|
||||
"adding": "Hinzufügen",
|
||||
"removing": "Entfernen",
|
||||
"compareWith": "Mit Setup vergleichen..."
|
||||
}
|
||||
```
|
||||
|
||||
### WR-03: DE locale missing `card.candidates` and `planning.*` keys in threads
|
||||
|
||||
**File:** `src/client/locales/de/threads.json`
|
||||
**Issue:** The EN `threads.json` contains `card.candidates` (with pluralisation) and the entire `planning.*` subtree (8 keys). `ThreadCard` calls `t("card.candidates", { count: candidateCount })` and `PlanningView` calls `t("threads:planning.title")`, `t("threads:planning.emptyTitle")`, `t("threads:planning.createFirst")`, and three sets of step titles/descriptions. All of these fall back to raw key strings in German.
|
||||
**Fix:** Add to `de/threads.json`:
|
||||
```json
|
||||
"card": {
|
||||
"candidates": "{{count}} Kandidaten",
|
||||
"candidates_one": "{{count}} Kandidat"
|
||||
},
|
||||
"planning": {
|
||||
"title": "Planungs-Threads",
|
||||
"emptyTitle": "Planen Sie Ihren nächsten Kauf",
|
||||
"createFirst": "Ersten Thread erstellen",
|
||||
"step1Title": "Thread erstellen",
|
||||
"step1Description": "Starten Sie einen Recherche-Thread für Ausrüstung, die Sie in Betracht ziehen",
|
||||
"step2Title": "Kandidaten hinzufügen",
|
||||
"step2Description": "Fügen Sie Produkte hinzu, die Sie mit Preisen und Gewichten vergleichen",
|
||||
"step3Title": "Gewinner wählen",
|
||||
"step3Description": "Schließen Sie den Thread ab und der Gewinner wird Ihrer Sammlung hinzugefügt"
|
||||
}
|
||||
```
|
||||
|
||||
### WR-04: DE locale missing `currency.suggestion`, `currency.switch`, and `showConversions` keys in settings
|
||||
|
||||
**File:** `src/client/locales/de/settings.json`
|
||||
**Issue:** The EN `settings.json` contains `currency.suggestion`, `currency.switch`, and the entire `showConversions` block. `SettingsPage` renders a currency suggestion banner using `t("currency.suggestion", { symbol, code })` and `t("currency.switch")`, and the "Show Converted Prices" toggle uses `t("showConversions.title")` and `t("showConversions.description")`. German users see raw key strings for all four of these.
|
||||
**Fix:** Add to `de/settings.json`:
|
||||
```json
|
||||
"currency": {
|
||||
"title": "Währung",
|
||||
"description": "Ändert das angezeigte Währungssymbol. Werte werden nicht umgerechnet.",
|
||||
"suggestion": "Basierend auf Ihrer Region empfehlen wir {{symbol}} ({{code}})",
|
||||
"switch": "Wechseln"
|
||||
},
|
||||
"showConversions": {
|
||||
"title": "Umgerechnete Preise anzeigen",
|
||||
"description": "Ungefähre Umrechnungen anzeigen, wenn kein lokaler Preis verfügbar ist"
|
||||
}
|
||||
```
|
||||
|
||||
### WR-05: DE locale missing `home.*`, `imageUpload.*`, and `profile.*` keys in common
|
||||
|
||||
**File:** `src/client/locales/de/common.json`
|
||||
**Issue:** The EN `common.json` contains three top-level sections absent from DE: `home` (3 keys used by `index.tsx`), `imageUpload` (4 keys used by `ImageUpload.tsx`), and `profile` (22 keys used by `profile.tsx`). These are the most user-facing gaps — the entire profile page renders raw key strings in German.
|
||||
**Fix:** Add the missing sections to `de/common.json`. The `profile` section is large; key translations include:
|
||||
```json
|
||||
"home": {
|
||||
"popularSetups": "Beliebte Setups",
|
||||
"recentlyAdded": "Kürzlich hinzugefügt",
|
||||
"trendingCategories": "Beliebte Kategorien"
|
||||
},
|
||||
"imageUpload": {
|
||||
"clickToAdd": "Klicken zum Hinzufügen eines Fotos",
|
||||
"invalidType": "Bitte wählen Sie ein JPG-, PNG- oder WebP-Bild.",
|
||||
"tooLarge": "Das Bild muss kleiner als 5 MB sein.",
|
||||
"uploadFailed": "Upload fehlgeschlagen. Bitte versuchen Sie es erneut."
|
||||
},
|
||||
"profile": {
|
||||
"title": "Profil",
|
||||
"account": "Konto",
|
||||
"accountInfo": "Ihre Kontoinformationen",
|
||||
"email": "E-Mail",
|
||||
"noEmail": "Keine E-Mail-Adresse hinterlegt",
|
||||
"change": "Ändern",
|
||||
"newEmailPlaceholder": "Neue E-Mail-Adresse",
|
||||
"updating": "Wird aktualisiert...",
|
||||
"updateEmail": "E-Mail aktualisieren",
|
||||
"emailUpdated": "E-Mail aktualisiert",
|
||||
"memberSince": "Mitglied seit",
|
||||
"security": "Sicherheit",
|
||||
"managePassword": "Ihr Passwort verwalten",
|
||||
"currentPassword": "Aktuelles Passwort",
|
||||
"newPassword": "Neues Passwort",
|
||||
"password": "Passwort",
|
||||
"confirmPassword": "Passwort bestätigen",
|
||||
"passwordRequirements": "Das Passwort muss mindestens 8 Zeichen mit Groß-, Kleinbuchstaben und einer Zahl enthalten.",
|
||||
"passwordUpdated": "Passwort aktualisiert",
|
||||
"changingPassword": "Wird geändert...",
|
||||
"changePassword": "Passwort ändern",
|
||||
"setPassword": "Passwort festlegen",
|
||||
"dangerZone": "Gefahrenzone",
|
||||
"dangerZoneDescription": "Löschen Sie Ihr Konto und alle persönlichen Daten. Öffentliche Setups werden \"Gelöschter Benutzer\" zugeordnet.",
|
||||
"deleteAccount": "Konto löschen",
|
||||
"deleteConfirmMessage": "Diese Aktion ist dauerhaft. Geben Sie LÖSCHEN ein, um zu bestätigen.",
|
||||
"deleteConfirmPlaceholder": "LÖSCHEN eingeben, um zu bestätigen"
|
||||
}
|
||||
```
|
||||
|
||||
### WR-06: `ThreadCard.formatDate` hardcodes `"en-US"` locale
|
||||
|
||||
**File:** `src/client/components/ThreadCard.tsx:21`
|
||||
**Issue:** `formatDate` calls `d.toLocaleDateString("en-US", ...)` with a hardcoded locale. Regardless of the user's language setting, thread card dates will always format in English (e.g. "Apr 17" not "17. Apr"). This is a locale bypass that survives even a correct i18next setup.
|
||||
**Fix:** Pass `undefined` (or the active i18n language) so the browser respects the user's locale:
|
||||
```ts
|
||||
function formatDate(iso: string): string {
|
||||
const d = new Date(iso);
|
||||
return d.toLocaleDateString(undefined, { month: "short", day: "numeric" });
|
||||
}
|
||||
```
|
||||
Alternatively, import `i18n` from `../lib/i18n` and use `i18n.language` as the first argument.
|
||||
|
||||
### WR-07: `t` variable shadowed by filter callback parameter in `PlanningView`
|
||||
|
||||
**File:** `src/client/components/PlanningView.tsx:32`
|
||||
**Issue:** The file destructures `{ t }` from `useTranslation` at line 11, then on line 32 uses `t` as the name of the filter callback parameter:
|
||||
```ts
|
||||
const filteredThreads = (threads ?? [])
|
||||
.filter((t) => t.status === activeTab)
|
||||
.filter((t) => (categoryFilter ? t.categoryId === categoryFilter : true));
|
||||
```
|
||||
The inner `t` shadows the outer translation function. While JS resolves this correctly inside the arrow functions, it makes the code misleading and will cause a lint warning (or error with strict shadowing rules). A future developer editing the filter body could accidentally call `t("some.key")` expecting a translation, and instead receive an object.
|
||||
**Fix:** Rename the filter parameter:
|
||||
```ts
|
||||
const filteredThreads = (threads ?? [])
|
||||
.filter((thread) => thread.status === activeTab)
|
||||
.filter((thread) => (categoryFilter ? thread.categoryId === categoryFilter : true));
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: Two hardcoded English strings in `ImageUpload` not extracted to locale
|
||||
|
||||
**File:** `src/client/components/ImageUpload.tsx:119,136`
|
||||
**Issue:** Line 119 has `alt="Item"` and line 136 has `title="Adjust framing"` as hardcoded English strings. The component already imports `useTranslation("common")` and uses it for other strings. These two are not in the EN locale file and will not be translated.
|
||||
**Fix:** Add keys to `en/common.json` (and `de/common.json`) under `imageUpload`:
|
||||
```json
|
||||
"imageUpload": {
|
||||
"clickToAdd": "Click to add photo",
|
||||
"invalidType": "...",
|
||||
"tooLarge": "...",
|
||||
"uploadFailed": "...",
|
||||
"altText": "Item photo",
|
||||
"adjustFraming": "Adjust framing"
|
||||
}
|
||||
```
|
||||
Then use them in the component:
|
||||
```tsx
|
||||
// line 119
|
||||
alt={t("imageUpload.altText")}
|
||||
// line 136
|
||||
title={t("imageUpload.adjustFraming")}
|
||||
```
|
||||
|
||||
### IN-02: `CollectionTabs` uses inconsistent key depth for first two tabs
|
||||
|
||||
**File:** `src/client/components/ThreadTabs.tsx:13-15`
|
||||
**Issue:** The tabs array mixes key depths:
|
||||
```ts
|
||||
{ key: "gear", label: t("gear") },
|
||||
{ key: "planning", label: t("planning") },
|
||||
{ key: "setups", label: t("tabs.setups") },
|
||||
```
|
||||
`"gear"` and `"planning"` are looked up at the root of the `collection` namespace, while `"setups"` is nested under `"tabs"`. This asymmetry is accidental — if `"gear"` or `"planning"` ever gain sub-keys, lookups will break silently. Consistent nesting (all under `"tabs"`) is cleaner and mirrors standard i18n patterns.
|
||||
**Fix:** Move all three into a `tabs` grouping in the locale files and look them up uniformly:
|
||||
```ts
|
||||
{ key: "gear", label: t("tabs.gear") },
|
||||
{ key: "planning", label: t("tabs.planning") },
|
||||
{ key: "setups", label: t("tabs.setups") },
|
||||
```
|
||||
|
||||
### IN-03: `profile.tsx` silently swallows account-deletion error after logging to console
|
||||
|
||||
**File:** `src/client/routes/profile.tsx:327-332`
|
||||
**Issue:** `handleDelete` catches errors with `console.error` but provides no feedback to the user. If `deleteAccount.mutateAsync()` throws, the user sees nothing — no error message, no state change.
|
||||
**Fix:** Add error state and display an error message:
|
||||
```tsx
|
||||
const [deleteError, setDeleteError] = useState<string | null>(null);
|
||||
|
||||
async function handleDelete() {
|
||||
setDeleteError(null);
|
||||
try {
|
||||
await deleteAccount.mutateAsync();
|
||||
window.location.href = "/logout";
|
||||
} catch (err) {
|
||||
setDeleteError((err as Error).message || t("errors.somethingWentWrong"));
|
||||
}
|
||||
}
|
||||
```
|
||||
Render `{deleteError && <p className="text-sm text-red-600">{deleteError}</p>}` in the danger zone form.
|
||||
|
||||
### IN-04: `de/threads.json` uses "Abgeschlossen" for `status.resolved` but `empty.noThreads` says "noch keine" vs EN "No threads found"
|
||||
|
||||
**File:** `src/client/locales/de/threads.json:42`
|
||||
**Issue:** The DE `empty.noThreads` reads "Noch keine Recherche-Threads" (meaning "No research threads yet") while the EN version reads "No threads found". These are used for two different states: the EN string is shown when a category filter returns no matches (not an "empty collection" state), but the DE string implies the collection is empty. When filtered to a category with no results, German users receive a misleading message.
|
||||
**Fix:** Align the DE translation with the EN intent:
|
||||
```json
|
||||
"empty": {
|
||||
"noThreads": "Keine Threads gefunden",
|
||||
"noCandidates": "Noch keine Kandidaten"
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-04-17T00:00:00Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Reference in New Issue
Block a user