diff --git a/docs/prs/PR-settings-locale-select-report.md b/docs/prs/PR-settings-locale-select-report.md new file mode 100644 index 0000000..4320165 --- /dev/null +++ b/docs/prs/PR-settings-locale-select-report.md @@ -0,0 +1,193 @@ +# PR — un seul contrôle de langue, un seul écrivain + +**Date** : 26 juillet 2026 +**Auteur** : @cc-ankora +**Branche** : `fix/settings-locale-select` +**Revue de plan** : `plan-reviewer` — 🟡 ×2 (v1 puis v2), 13 corrections au total, toutes intégrées +**Audits** : `i18n-auditor` ✅ GO · `ui-auditor` (cf. §7) +**Décision produit** : @thierry, 26/07 — « juste FR - EN point, pas de précision spécifique, cela complique trop les choses pour rien » + +--- + +## 1. Le défaut + +Le sélecteur de langue de Réglages › Profil était cassé de deux façons cumulées, et +préexistait à tout ce qui a été livré cette semaine. + +**Il ne pouvait rien enregistrer d'autre que le français.** Le `` maison, c'est-à-dire d'écrire une +**troisième** implémentation du même contrôle. Réutiliser celui qui existe supprime d'un coup +le double `onSubmit`, les clés `localeOptions`, et la question de la valeur initiale. + +### Le contrôle sort du formulaire + +Il ne s'agit pas d'un brouillon qu'on soumet : le switcher persiste immédiatement puis navigue +vers l'URL localisée, ce qui remonte la carte. Le laisser dans le `
` aurait suggéré que +« Enregistrer » s'y applique — et une saisie de nom en cours aurait disparu au changement de +langue, sans explication. Il est donc sur sa propre ligne, séparée par une bordure, avec la +grammaire libellé-gauche / contrôle-droite du toggle de thème du `MoreSheet`. + +### Un ``, et un nom accessible distinct — correction issue de l'audit UI + +`LocaleSwitcher` est un `radiogroup` sans contrôle labelable unique : un `htmlFor` pendrait +dans le vide, d'où le ``. + +Ma première version laissait le switcher garder son propre `aria-label`. **`ui-auditor` a +relevé que c'était une régression que j'introduisais** : à ≥1024px, `/app/settings` monte +aussi le switcher du `HeaderNav` (son bloc `hidden lg:flex` est bien dans le DOM et +focusable). Deux `radiogroup` annoncés « Changer de langue » deviennent indiscernables dans +la liste des éléments d'un lecteur d'écran. Avant ce diff, un seul exemplaire coexistait par +viewport. + +Corrigé plutôt que laissé en arbitrage : `LocaleSwitcher` accepte désormais un +`labelledById` optionnel, et le champ des Réglages nomme le groupe par son libellé visible +« Langue ». Les instances du header et du `MoreSheet` sont inchangées — sans la prop, le +comportement d'origine est conservé. Bénéfice secondaire : le nom accessible **égale** +désormais le texte visible au lieu de simplement le contenir (WCAG 2.5.3), ce que l'audit +signalait comme conforme mais fragile et non testé. + +### Le piège des deux clés d'erreur quasi identiques + +`settings.locale.invalid` (émise par le champ retiré du schéma) devenait orpheline et a été +supprimée des 5 fichiers. `errors.locale.invalid`, elle, est émise par `setLocaleAction` et a +été **préservée** — les confondre aurait cassé le chemin d'erreur du switcher. +`i18n-auditor` a vérifié les deux : orpheline absente partout, celle du switcher intacte et +toujours atteignable, parité des 5 locales conservée. + +Le groupe `app.settings.profile.localeOptions` a été supprimé : c'était un doublon de +`ui.localeSwitcher.options.*`, et ce doublon est exactement ce qui avait permis à `fr-FR` et +`en-GB` de diverger de l'enum serveur. + +## 3. Ce qui n'a délibérément pas bougé + +- **`users.locale` reste écrit** par `setLocaleAction` et **reste lu** par `src/i18n/request.ts` + quand `requestLocale` est absent. Je m'étais trompé en écrivant que la colonne ne pilotait + plus grand-chose : c'est le filet cross-device / cookie perdu. +- **`profileUpdateSchema` reste un `z.object` nu, jamais `.strict()`.** Pendant un déploiement, + les onglets sur l'ancien bundle continuent d'envoyer `{ displayName, locale }` ; Zod strippe + les clés inconnues, donc ils dégradent proprement. `.strict()` les rejetterait sèchement, + pour une propreté que personne ne demande. Un test verrouille ce comportement. +- **Les trois éditions tiennent dans un seul commit.** Le schéma portait `.default('fr-BE')` : + si le client avait cessé d'envoyer `locale` avant que le schéma et l'action ne changent, + chaque sauvegarde de profil aurait silencieusement réinitialisé la langue de l'utilisateur. + +## 4. Preuve + +### Smoke en direct, session connectée + +Joué sur la page Réglages réelle avec le fixture seedé : + +``` +label : Langue +options : ["FR","EN"] +ancien ` disparu ; nom accessible du groupe distinct de celui du header ; +et sauvegarder le nom ne touche pas à la langue (la séparation d'avec le formulaire). Joué en +local : **3/3 sur iPhone 14**. Il s'auto-skippe en CI, c'est écrit en tête du fichier. + +`tests/schemas/settings.test.ts` — les deux specs qui asservissaient l'acceptation de `locale` +et le défaut `fr-BE` ont été **réécrites**, pas supprimées : elles asserent désormais le +contrat inverse. Les effacer aurait fait disparaître la seule trace que ce contrat a changé +volontairement. + +### Ce que la CI ne prouvera pas + +Aucun spec Playwright ne couvre ce contrôle : `e2e/smoke.spec.ts` ne teste que la redirection +vers `/login`, et les parcours authentifiés s'auto-skippent en CI faute de +`SUPABASE_SERVICE_ROLE_KEY` (un seul projet Supabase, la clé `service_role` ne doit pas y +atteindre). **Une CI verte ne vaut donc pas validation de ce correctif** — d'où le smoke seedé +local ci-dessus, exigé par la revue. + +À consigner aussi : `setLocaleAction` n'a **aucun rate limit** là où `updateProfileAction` +consomme un jeton `rateLimit('mutation')`. Inchangé par rapport au `LocaleSwitcher` du header, +donc pas une régression de cette PR, mais le déplacer dans les Réglages en fait un chemin de +plus vers une action non limitée. + +## 5. Note d'i18n consignée pour ne pas être re-débattue + +Le libellé **visible** est « FR » / « EN », conforme à la décision de @thierry. Le **nom +accessible** des options vient de `ui.localeSwitcher.options.*`, où `fr-BE` vaut +« Français (BE) » — la mention régionale subsiste donc pour les technologies d'assistance. +`i18n-auditor` le qualifie de conforme : un sélecteur de langue affiche l'endonyme, et le code +`fr-BE` est précisément la valeur envoyée à `setLocaleAction`. + +## 6. Definition of DONE + +| # | Critère | Preuve | +| --- | ---------------------------------------------- | --------------------------------- | +| 1 | CI verte | cf. checks de la PR | +| 2 | Sourcery muet sur le dernier commit | `gh api …/comments` → sortie vide | +| 3 | Approbation humaine @thierry + threads résolus | à la revue | +| 4 | Pas de conflit avec `main` | `mergeStateStatus: CLEAN` | +| 5 | Rapport livré | ce fichier | + +## 7. Audits + +- **`i18n-auditor`** : ✅ **GO**. Parité vérifiée sur les 5 fichiers, clés supprimées sans + consommateur résiduel, `errors.locale.invalid` intacte et distincte de l'orpheline, aucun + résidu `fr-FR` / `en-GB`, aucun lecteur de `users.locale` cassé. +- **`ui-auditor`** : ✅ **GO conditionnel**, avec un P1 réel — la duplication du contrôle sur + desktop, corrigée dans cette PR (cf. §2), et un P1 « aucun test de non-régression sur le + nouveau champ », corrigé par `e2e/mobile-ios/settings-locale-field.spec.ts`. + Contraste, cible tactile, ordre de tabulation et parité i18n : conformes. + + **Restent en P2, non traités ici et volontairement** : + - Pas d'`aria-describedby` « s'applique immédiatement » sur le switcher. Le signal est + visuel (bordure de séparation). Risque faible — convention établie pour les sélecteurs de + langue — mais réel pour un utilisateur non-voyant, dans ce nouveau contexte de carte de + formulaire. + - La relation « libellé visible ⊂ nom accessible » n'est verrouillée par aucun test unitaire + sur les 5 locales. Sans objet pour le champ des Réglages depuis la correction ci-dessus + (les deux sont désormais la même chaîne), mais toujours vrai pour les autres instances. + - `CardTitle` rend un `
` et jamais un vrai titre : ça concerne la hiérarchie de titres + de toute l'app, pas cette carte. À tracer séparément, hors périmètre. diff --git a/e2e/mobile-ios/settings-locale-field.spec.ts b/e2e/mobile-ios/settings-locale-field.spec.ts new file mode 100644 index 0000000..293d326 --- /dev/null +++ b/e2e/mobile-ios/settings-locale-field.spec.ts @@ -0,0 +1,83 @@ +import { expect } from '@playwright/test'; + +import { test } from './fixtures/mobile-test'; + +/** + * Settings › Profile — the language field. + * + * The control it replaced was broken twice over: it offered `fr-FR` and + * `en-GB`, neither of which exists in `LOCALES`, so every choice but `fr-BE` + * failed server validation with a generic toast; and `updateProfileAction` + * wrote `users.locale` without touching the NEXT_LOCALE cookie or revalidating, + * so even an accepted `fr-BE` changed nothing on screen — the rendered locale + * comes from the URL prefix alone. Two divergent writers for one preference. + * + * ⚠️ This spec is `seededUser`-gated and AUTO-SKIPS in CI (no + * `SUPABASE_SERVICE_ROLE_KEY` there — one Supabase project, the service_role + * key must not reach CI). A green pipeline does NOT mean these assertions ran. + * Run locally: load `.env.local` into the environment, start a server, then + * `E2E_BASE_URL= npx playwright test e2e/mobile-ios/settings-locale-field.spec.ts`. + */ +test.describe('Settings — language field', () => { + test.beforeEach(async ({ page, seededUser }) => { + await page.goto('/login'); + await page.getByLabel('Email').fill(seededUser.email); + await page.getByLabel('Mot de passe').fill(seededUser.password); + await page.getByRole('button', { name: /^se connecter$/i }).click(); + await page.waitForURL(/\/app\b/, { timeout: 30_000 }); + await page.goto('/app/settings', { waitUntil: 'networkidle' }); + }); + + test('offers exactly FR and EN, and nothing else', async ({ page }) => { + // Locators MUST be scoped through the wrapper: `Header variant="app"` also + // renders a LocaleSwitcher (its `hidden lg:flex` block is in the DOM at + // every viewport), so the switcher's own testids are ambiguous here. + const field = page.getByTestId('settings-locale-field'); + await expect(field).toBeVisible(); + + await expect(field.getByRole('radio')).toHaveCount(2); + expect((await field.getByRole('radio').allTextContents()).map((t) => t.trim())).toEqual([ + 'FR', + 'EN', + ]); + + // The old - - - - - {tOptions('fr-BE')} - {tOptions('fr-FR')} - {tOptions('en-GB')} - - -
+ + {/* + Language sits OUTSIDE the form on purpose. It is not a draft the user + submits — `LocaleSwitcher` persists the choice immediately and then + navigates to the localised URL, which remounts this card. Leaving the + control inside the form would suggest "Save" applies to it, and a name + typed but not yet saved would vanish on switch with no explanation. + Same label-left / control-right grammar as the theme toggle in + `MoreSheet`. + + `data-testid` on the WRAPPER, not on the switcher: `/app/settings` + mounts `
` (which renders a LocaleSwitcher in a + `hidden lg:flex` block — present in the DOM at every viewport) and, + on mobile, `MoreSheet` renders another. There are therefore two to + three instances on this page, all carrying the same internal testids. + Any Playwright locator for this one must scope through this wrapper, + or hit a strict-mode violation. + */} +
+ {/* + A , not a
); diff --git a/src/app/[locale]/app/settings/page.tsx b/src/app/[locale]/app/settings/page.tsx index fc71bef..825c804 100644 --- a/src/app/[locale]/app/settings/page.tsx +++ b/src/app/[locale]/app/settings/page.tsx @@ -21,7 +21,7 @@ export default async function SettingsPage() { const supabase = await createClient(); const [profileRes, factorsRes, deletionRes, consentRes] = await Promise.all([ - supabase.from('users').select('display_name, locale, email').eq('id', user.id).maybeSingle(), + supabase.from('users').select('display_name, email').eq('id', user.id).maybeSingle(), supabase.auth.mfa.listFactors(), supabase .from('deletion_requests') @@ -52,7 +52,6 @@ export default async function SettingsPage() { const { error } = await supabase .from('users') - .update({ - display_name: parsed.data.displayName, - locale: parsed.data.locale, - }) + // Display name only — the language preference is owned by `setLocaleAction`, + // which also writes the cookie and revalidates the root layout. + .update({ display_name: parsed.data.displayName }) .eq('id', user.id); if (error) return { ok: false, errorCode: 'errors.settings.profileUpdateFailed' }; diff --git a/src/lib/schemas/settings.ts b/src/lib/schemas/settings.ts index d0aefcb..0285918 100644 --- a/src/lib/schemas/settings.ts +++ b/src/lib/schemas/settings.ts @@ -1,15 +1,30 @@ import { z } from 'zod'; -import { LOCALES } from '@/i18n/routing'; import { normalizeEmail } from '@/lib/i18n/formatters'; +/** + * Profile form payload. + * + * `locale` is deliberately absent. The language preference has a single writer, + * `setLocaleAction` — it writes the NEXT_LOCALE cookie, updates `users.locale` + * AND revalidates the root layout, none of which `updateProfileAction` did. + * Having two writers is what left the Settings selector unable to save anything + * but `fr-BE`: it offered `fr-FR` / `en-GB`, neither of which is in `LOCALES`, + * so every other choice failed validation with a generic toast — and even an + * accepted `fr-BE` changed nothing on screen, since the rendered locale comes + * from the URL prefix alone (cf. `src/i18n/routing.ts`). + * + * Keep this a plain `z.object`, NEVER `.strict()`. During a deploy, tabs still + * running the previous bundle keep posting `{ displayName, locale }`; Zod + * strips unknown keys, so they degrade gracefully. `.strict()` would reject + * them outright, for tidiness nobody asked for. + */ export const profileUpdateSchema = z.object({ displayName: z .string() .trim() .min(1, { message: 'settings.displayName.required' }) .max(80, { message: 'settings.displayName.tooLong' }), - locale: z.enum(LOCALES, { error: 'settings.locale.invalid' }).default('fr-BE'), }); export const factorIdSchema = z.string().uuid({ message: 'settings.factorId.invalid' }); diff --git a/tests/actions/settings-profile.test.ts b/tests/actions/settings-profile.test.ts new file mode 100644 index 0000000..c36c857 --- /dev/null +++ b/tests/actions/settings-profile.test.ts @@ -0,0 +1,96 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +/** + * `updateProfileAction` — the language preference must not travel through it. + * + * It used to write `users.locale` alongside the display name, which made it a + * second writer of a preference `setLocaleAction` already owned. Only the + * latter writes the NEXT_LOCALE cookie and revalidates the root layout, so a + * language saved from Settings changed a database column and nothing else — + * the rendered locale comes from the URL prefix alone since the routing config + * stopped reading the cookie. + * + * The Settings selector was the visible casualty: it offered `fr-FR` and + * `en-GB`, neither of which exists in `LOCALES`, so every choice but `fr-BE` + * failed validation with a generic error toast. + * + * These specs pin the payload actually sent to Supabase, which is the only + * place the regression could come back unnoticed — the schema no longer even + * types `locale`, so a plain type check would not catch a hand-written update. + */ + +const updateSpy = vi.hoisted(() => + vi.fn<(payload: Record) => { eq: () => Promise<{ error: null }> }>(() => ({ + eq: async () => ({ error: null }), + })), +); +const revalidateAppPathSpy = vi.hoisted(() => vi.fn()); + +// `@/lib/log` pulls in `@/lib/env`, which throws at module load without the +// Supabase env vars — the same stub the auth guard suites use. +vi.mock('@/lib/log', () => ({ + log: { + trace: vi.fn(), + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + fatal: vi.fn(), + child: vi.fn(), + }, +})); + +vi.mock('@/lib/security/rate-limit', () => ({ + rateLimit: async () => ({ success: true, limit: 10, remaining: 9, reset: 0 }), +})); + +vi.mock('@/lib/actions/revalidate', () => ({ + revalidateAppPath: (...args: unknown[]) => revalidateAppPathSpy(...args), + revalidateDashboard: vi.fn(), +})); + +vi.mock('@/lib/supabase/server', () => ({ + createClient: async () => ({ + auth: { + async getUser() { + return { data: { user: { id: 'user-123', email: 'thierry@example.test' } }, error: null }; + }, + }, + from: () => ({ update: updateSpy }), + }), +})); + +import { updateProfileAction } from '@/lib/actions/settings'; + +beforeEach(() => { + updateSpy.mockClear(); + revalidateAppPathSpy.mockClear(); +}); + +describe('updateProfileAction', () => { + it('writes the display name', async () => { + const res = await updateProfileAction({ displayName: 'Thierry' }); + + expect(res).toEqual({ ok: true }); + expect(updateSpy).toHaveBeenCalledWith({ display_name: 'Thierry' }); + }); + + it('never writes a locale, even when an older client still sends one', async () => { + // Deploy skew: a tab running the previous bundle keeps posting `locale`. + // Zod strips it, and the update payload must stay clean — otherwise the + // second writer is back and the user's language gets overwritten on every + // profile save. + await updateProfileAction({ displayName: 'Thierry', locale: 'en' }); + + expect(updateSpy).toHaveBeenCalledWith({ display_name: 'Thierry' }); + const [payload] = updateSpy.mock.calls[0] ?? []; + expect(payload as Record | undefined).not.toHaveProperty('locale'); + }); + + it('rejects an empty display name without touching the database', async () => { + const res = await updateProfileAction({ displayName: ' ' }); + + expect(res.ok).toBe(false); + expect(updateSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/schemas/settings.test.ts b/tests/schemas/settings.test.ts index c5529e2..4a27ebf 100644 --- a/tests/schemas/settings.test.ts +++ b/tests/schemas/settings.test.ts @@ -6,28 +6,48 @@ import { } from '@/lib/schemas/settings'; describe('profileUpdateSchema', () => { - it('accepts trimmed display name and known locale', () => { - const res = profileUpdateSchema.parse({ - displayName: ' Thierry ', - locale: 'fr-BE', - }); + it('accepts and trims the display name', () => { + const res = profileUpdateSchema.parse({ displayName: ' Thierry ' }); expect(res.displayName).toBe('Thierry'); - expect(res.locale).toBe('fr-BE'); }); - it('defaults locale to fr-BE when omitted', () => { - const res = profileUpdateSchema.parse({ displayName: 'Name' }); - expect(res.locale).toBe('fr-BE'); + /** + * The language preference used to travel through this schema, which gave it + * two writers: this one wrote `users.locale` only, while `setLocaleAction` + * wrote the cookie AND revalidated the root layout. The Settings selector was + * the casualty — it offered `fr-FR` / `en-GB`, absent from `LOCALES`, so every + * choice but `fr-BE` failed validation, and even `fr-BE` changed nothing on + * screen. `setLocaleAction` is now the single writer. + * + * These two specs replace the ones that asserted the opposite (locale + * accepted, defaulted to `fr-BE`). Deleting them outright would have removed + * the only trace that this contract changed on purpose. + */ + it('ignores a locale sent by an older client instead of rejecting the request', () => { + // Deploy skew: tabs on the previous bundle keep posting `locale`. The schema + // is a plain `z.object`, so Zod strips unknown keys. Adding `.strict()` + // would turn those tabs into hard validation failures. + const res = profileUpdateSchema.parse({ displayName: 'Thierry', locale: 'fr-BE' }); + expect(res.displayName).toBe('Thierry'); + expect(res as Record).not.toHaveProperty('locale'); + }); + + it('never writes a default locale back', () => { + // The old schema carried `.default('fr-BE')`. Left in place while the client + // stopped sending the field, it would have silently reset the language on + // every profile save. + const res = profileUpdateSchema.parse({ displayName: 'Thierry' }); + expect(res as Record).not.toHaveProperty('locale'); }); it('rejects empty name', () => { expect(profileUpdateSchema.safeParse({ displayName: ' ' }).success).toBe(false); }); - it('rejects unknown locale', () => { - expect(profileUpdateSchema.safeParse({ displayName: 'N', locale: 'xx-ZZ' }).success).toBe( - false, - ); + it('accepts a request whatever the stray locale value, since the field is gone', () => { + // Formerly "rejects unknown locale". The rejection now belongs to + // `setLocaleAction`, which validates against `LOCALES` on its own path. + expect(profileUpdateSchema.safeParse({ displayName: 'N', locale: 'xx-ZZ' }).success).toBe(true); }); });