From d1b9cdce3853587f0aa0c4f124c707e1f2576b1a Mon Sep 17 00:00:00 2001 From: thierryvm Date: Sun, 26 Jul 2026 00:55:33 +0200 Subject: [PATCH] feat(nav): make a forgotten destination impossible, not just add the missing one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /app/commitments shipped unreachable from mobile navigation: declared in the desktop header (hidden lg:flex) and nowhere else, so outside the cockpit card that links to it the page did not exist on a phone. Reported in production by @thierry. The defect worth fixing is not the missing link, it is that forgetting one was undetectable. Destinations were declared three times — a private array in BottomTabBar, hardcoded JSX in MoreSheet, hardcoded JSX in Header — with nothing tying them together. app-destinations.ts is now the single declaration, and its test reads the filesystem in both directions: a route with no entry fails, and an entry pointing at a deleted route fails too. Verified falsifiable — removing commitments from the registry turns it red. Icons and i18n keys deliberately stay out of the registry. It is server-safe (no React) and next-intl keys are typed against fr-BE.json, so only literals compile. Each surface keeps a Record, which gives the same exhaustiveness: a destination without an icon or a label does not build. Labels stay per-surface, and that is the point. The bar says Cockpit / Factures / Simuler where the desktop header says Tableau de bord / Charges / Simulateur. One shared key would have silently rewritten copy written for each context. The field is called mobilePlacement rather than surface because the desktop header renders the full list and ignores it. A field named surface on a registry consumed by three surfaces reads as "filter on me", and doing that in Header would make destinations vanish from desktop — the very bug class this module prevents. Ids are not folder names either: bills lives at charges, simulate at simulator, and both are baked into data-testids the e2e suite asserts on. Harmonising them would break two suites for nothing. Nothing else changes: same classes, same labels, same rendering, plus the Engagements link in the sheet. The visual redesign is the next lot. One existing e2e test failed because of this change, and it was informative. The extra link makes the sheet taller, and Playwright clicks an element's centre by default — the backdrop is fixed inset-0, so its centre is the viewport middle, now covered by the sheet. The click landed on the sheet and dismissed nothing. The test's intent was right, its click point had become wrong: it now aims at the exposed strip a user actually taps, and stays correct whatever the sheet's height. Worth carrying into the redesign lot: the sheet grows with its content and already takes a real share of an iPhone SE screen. Validated locally at 12/12 on iPhone 14 and iPhone SE, authenticated specs included — they are seededUser-gated and auto-skip in CI, so they cannot cover this lot automatically. Two frictions worth recording: playwright.config.ts does not load .env.local, so a direct run skips the seeded specs even when the key exists; and npm run e2e:auth serves a production build where rateLimit fails closed against the placeholder Upstash URL, breaking the first login. The pass was therefore run against the dev server, where it fails open — a fidelity gap that concerns Server Action compile timing, not navigation rendering. Known and accepted: /app/commitments lights up no tab (same as /admin today), resolved when the bar/sheet split is arbitrated in the redesign lot. --- .../prs/PR-nav-destination-registry-report.md | 162 ++++++++++++++++++ e2e/mobile-ios/bottom-tab-bar.spec.ts | 12 +- messages/de-DE.json | 1 + messages/en.json | 1 + messages/es-ES.json | 1 + messages/fr-BE.json | 1 + messages/nl-BE.json | 1 + src/components/layout/BottomTabBar.tsx | 94 +++++----- src/components/layout/Header.tsx | 61 ++++--- src/components/layout/MoreSheet.tsx | 55 ++++-- .../layout/__tests__/BottomTabBar.test.tsx | 32 ++++ .../layout/__tests__/app-destinations.test.ts | 135 +++++++++++++++ src/components/layout/app-destinations.ts | 101 +++++++++++ 13 files changed, 578 insertions(+), 79 deletions(-) create mode 100644 docs/prs/PR-nav-destination-registry-report.md create mode 100644 src/components/layout/__tests__/app-destinations.test.ts create mode 100644 src/components/layout/app-destinations.ts diff --git a/docs/prs/PR-nav-destination-registry-report.md b/docs/prs/PR-nav-destination-registry-report.md new file mode 100644 index 0000000..582b4de --- /dev/null +++ b/docs/prs/PR-nav-destination-registry-report.md @@ -0,0 +1,162 @@ +# PR — registre unique des destinations de navigation (Phase 1, lot 1) + +**Date** : 26 juillet 2026 +**Auteur** : @cc-ankora +**Branche** : `feat/nav-destination-registry` +**Revue de plan** : `plan-reviewer` — 🟡 puis ✅ APPROVED (8 corrections + 3 précisions, toutes intégrées) +**Programme** : Phase 1 de la refonte UX, **premier lot**. Le redesign visuel de la nav est le lot suivant. + +--- + +## 1. Le défaut + +@thierry a constaté en production que `/app/commitments` (« Engagements ») est inatteignable +depuis la navigation mobile. Vérifié : la route existe, la page fonctionne, mais elle n'était +référencée que par le header desktop (`hidden lg:flex`) et par une carte du cockpit. Hors du +cockpit, la page n'existait plus sur mobile. + +**La cause n'est pas le lien manquant, c'est qu'on pouvait l'oublier.** Les destinations +étaient déclarées **trois fois** — un tableau privé dans `BottomTabBar.tsx`, du JSX en dur +dans `MoreSheet.tsx`, du JSX en dur dans `Header.tsx` — sans rien pour les relier. Ajouter +une route et n'en câbler qu'une ou deux surfaces ne déclenchait aucune alerte. + +## 2. Le correctif + +`src/components/layout/app-destinations.ts` — module **server-safe** (pas de React, pas +d'import Next, même contrat que `bottom-tab-bar.routes.ts` voisin) qui déclare les 7 +destinations, leur `href`, leur stratégie de correspondance et leur placement **mobile**. + +Les trois surfaces le consomment désormais. Rien d'autre ne change : mêmes classes, mêmes +libellés, même rendu — plus le lien Engagements dans le sheet. + +### Ce qui reste volontairement hors du registre + +**Les icônes et les clés i18n**, parce que le module est server-safe et que les clés next-intl +sont typées contre `fr-BE.json` (seuls les littéraux compilent). Elles vivent dans chaque +surface sous forme de `Record` : ajouter une destination sans icône ni +libellé est une **erreur TypeScript**. Même exhaustivité, sans casser le contrat du module. + +**Les libellés sont différents par surface, et c'est voulu.** La barre dit « Cockpit » / +« Factures » / « Simuler » (`layout.bottomTab.*`) là où le header desktop dit « Tableau de +bord » / « Charges » / « Simulateur » (`common.nav.*`). Une clé unique aurait silencieusement +réécrit une copie rédigée pour chaque contexte — `plan-reviewer` l'a relevé avant que je +n'écrive la moindre ligne. + +### Le champ s'appelle `mobilePlacement`, pas `surface` + +Il gouverne le partage **mobile** (barre vs sheet). Le header desktop rend la liste +**complète** en l'ignorant. Un champ nommé `surface` sur un registre consommé par trois +surfaces se serait mal relu : le prochain lecteur aurait « corrigé » `Header` pour qu'il +filtre dessus, et fait disparaître des destinations du desktop — exactement la classe de bug +que ce module existe pour empêcher. Le nom porte l'invariant. + +### Les identifiants ne sont pas des noms de dossier + +`bills` pointe `/app/charges`, `simulate` pointe `/app/simulator`. Le décalage est délibéré : +ces ids sont gravés dans `data-testid="bottom-tab-bills"` / `bottom-tab-simulate` et assertés +par les suites unitaire **et** e2e. Les « harmoniser » aurait cassé les deux pour rien — et +comme les specs e2e sont `seededUser`-gated, la CI serait restée verte pendant qu'elles se +skippaient en silence. Documenté dans le module. + +## 3. Le test qui rend l'oubli impossible + +`src/components/layout/__tests__/app-destinations.test.ts` lit le système de fichiers et +vérifie **les deux sens** : + +- **dossier → registre** : chaque route sous `src/app/[locale]/app/` doit avoir une entrée. + C'est le bug d'Engagements. +- **registre → dossier** : chaque `href` doit pointer vers une route existante, `/app` + excepté (c'est la racine, pas un sous-dossier). Sans ce sens, une destination survivant à + la suppression de sa route produirait un lien 404 sur les trois surfaces sans que rien + n'échoue. + +La comparaison porte sur le **segment dérivé du `href`**, jamais sur l'`id` — sinon `bills` et +`simulate` échoueraient et pousseraient à les renommer, c'est-à-dire à provoquer la dérive +qu'on cherche à empêcher. + +Garde-fous : profondeur 1 uniquement (sinon `settings/deletion-status` serait exigé à tort), +`page.tsx` requis dans le dossier, exclusion des route groups `(x)`, dossiers privés `_x`, +routes parallèles `@x` et segments dynamiques `[x]`. `// @vitest-environment node` en tête, +le projet étant en jsdom par défaut. + +### Falsifiabilité vérifiée + +Registre amputé d'Engagements → le test échoue avec +`expected [ 'commitments' ] to deeply equal []`. Restauré → vert. Le test attrape bien le bug +qu'il prétend attraper. + +## 4. Ce que la validation e2e a révélé + +Un test existant a échoué **à cause de ce changement**, et c'était instructif : « More sheet +opens via tap and closes via backdrop click ». + +Le lien ajouté rehausse le sheet. Or Playwright clique le **centre** d'un élément par défaut, +et le backdrop est `fixed inset-0` — son centre est le milieu du viewport, désormais couvert +par le sheet. Le clic atterrissait donc sur le sheet et ne fermait rien. + +L'intention du test reste juste (« cliquer en dehors ferme ») ; c'est le point de clic qui +était devenu faux. Il vise maintenant explicitement la bande exposée en haut du backdrop — +ce que fait un utilisateur — et reste correct quelle que soit la hauteur du sheet. + +**À retenir pour le lot de redesign** : le sheet grandit avec son contenu. Sur iPhone SE il +occupe déjà une part notable de l'écran. La répartition barre/sheet devra être décidée sur +pièces, pas par accumulation. + +## 5. Preuve + +| Vérification | Résultat | +| ----------------------------------- | ------------------------------------------ | +| `npm run test` | **1659 / 1659** | +| `npm run typecheck` | 0 erreur | +| `npm run lint` | 0 erreur (7 warnings préexistants) | +| `npm run lint:use-server` | OK | +| e2e `bottom-tab-bar`, **iPhone 14** | **12 / 12**, specs authentifiées comprises | +| e2e `bottom-tab-bar`, **iPhone SE** | **12 / 12** | +| Falsifiabilité du test anti-dérive | rouge sans l'entrée, vert avec | + +### Comment les specs authentifiées ont été jouées, et ce que ça dit + +Elles sont `seededUser`-gated et **s'auto-skippent en CI** (pas de `SUPABASE_SERVICE_ROLE_KEY` +— un seul projet Supabase, la clé `service_role` ne doit pas atteindre la CI). Elles ne +peuvent donc pas valider ce lot automatiquement : `plan-reviewer` a exigé une passe locale, +elle a été faite. + +Deux frictions rencontrées, utiles à consigner : + +1. `playwright.config.ts` ne charge pas `.env.local`, donc un `npx playwright test` direct + skippe les specs seedées même quand la clé existe. Chargement fait côté terminal. +2. `npm run e2e:auth` construit et sert un build de **production**, où `rateLimit()` échoue en + fermé sur l'Upstash factice de `.env.local` — la première connexion casse (cf. #256). La + passe a donc été faite contre le serveur **dev**, où le rate limit échoue en ouvert. + Écart de fidélité assumé et déclaré : il porte sur le temps de compilation des Server + Actions, pas sur le rendu de la navigation, qui est l'objet de ce lot. + +## 6. Limites assumées, écrites avant merge plutôt que découvertes après + +- Sur `/app/commitments`, **aucun onglet ne porte `aria-current`** — même comportement + qu'`/admin` aujourd'hui. Coût UX assumé, résolu au lot de redesign quand la répartition + barre/sheet sera arbitrée. +- `AccountButton.tsx` pointe aussi `/app/settings` et **ne consomme délibérément pas** le + registre (menu desktop). Le registre couvre les destinations de **navigation**, pas tous + les liens vers `/app/*` de l'application. +- Les CTA contextuels du cockpit (`app/page.tsx`, `SimulatorClient.tsx`, + `ProchainesFacturesCard.tsx`) codent en dur des `/app/...` : hors périmètre, non touchés. +- `e2e/a11y/drawer-mobile-focus-trap.spec.ts` cible le drawer marketing, **pas** le MoreSheet. + Il n'est pas cité comme couverture de ce lot. + +## 7. i18n + +Nouvelle clé `layout.moreSheet.links.commitments`, ajoutée aux **5** fichiers de `messages/` +(une clé manquante déclenche un `MISSING_MESSAGE` à l'exécution sur les locales concernées). +Libellés repris à l'identique de `common.nav.commitments`, déjà traduits : Engagements, +Commitments, Verbintenissen, Verpflichtungen, Compromisos. + +## 8. 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 | Threads de review résolus | GraphQL `reviewThreads` → 0 non résolu | +| 4 | Pas de conflit avec `main` | `mergeStateStatus: CLEAN` | +| 5 | Rapport livré | ce fichier | diff --git a/e2e/mobile-ios/bottom-tab-bar.spec.ts b/e2e/mobile-ios/bottom-tab-bar.spec.ts index 4c98c08..cd44ea1 100644 --- a/e2e/mobile-ios/bottom-tab-bar.spec.ts +++ b/e2e/mobile-ios/bottom-tab-bar.spec.ts @@ -75,13 +75,21 @@ test.describe('BottomTabBar — iPhone Safari WebKit (PR-BETA-6 / THI-277)', () await expect(page.getByTestId('more-sheet')).toBeVisible(); await expect(page.getByRole('dialog', { name: 'Plus' })).toBeVisible(); - // Sheet content sanity: at least the canonical Accounts + Settings links. + // Sheet content sanity: the cockpit destinations the registry declares. await expect(page.getByTestId('more-sheet-link-accounts')).toBeVisible(); + await expect(page.getByTestId('more-sheet-link-commitments')).toBeVisible(); await expect(page.getByTestId('more-sheet-link-settings')).toBeVisible(); await expect(page.getByTestId('more-sheet-logout')).toBeVisible(); // Backdrop tap dismisses (Apple iOS sheet behaviour parity). - await page.getByTestId('more-sheet-backdrop').click(); + // + // Click near the TOP of the backdrop rather than letting Playwright aim at + // its centre. The backdrop is `fixed inset-0`, so its centre is the middle + // of the viewport — and the sheet is tall enough to cover that point, which + // makes a centre click land on the sheet itself and never dismiss. A user + // taps the exposed strip above the sheet; this reproduces that, and stays + // correct as the sheet's height changes with its content. + await page.getByTestId('more-sheet-backdrop').click({ position: { x: 20, y: 20 } }); await expect(page.getByTestId('more-sheet')).toBeHidden(); }); diff --git a/messages/de-DE.json b/messages/de-DE.json index ab19708..60e00d0 100644 --- a/messages/de-DE.json +++ b/messages/de-DE.json @@ -101,6 +101,7 @@ }, "links": { "accounts": "Konten", + "commitments": "Verpflichtungen", "settings": "Einstellungen", "admin": "Admin", "adminAriaLabel": "Adminbereich (nur Gründer)", diff --git a/messages/en.json b/messages/en.json index ffc0ef8..4b09fa0 100644 --- a/messages/en.json +++ b/messages/en.json @@ -101,6 +101,7 @@ }, "links": { "accounts": "Accounts", + "commitments": "Commitments", "settings": "Settings", "admin": "Admin", "adminAriaLabel": "Admin area (founder only)", diff --git a/messages/es-ES.json b/messages/es-ES.json index b1452eb..2b2b144 100644 --- a/messages/es-ES.json +++ b/messages/es-ES.json @@ -101,6 +101,7 @@ }, "links": { "accounts": "Cuentas", + "commitments": "Compromisos", "settings": "Ajustes", "admin": "Admin", "adminAriaLabel": "Zona admin (solo fundador)", diff --git a/messages/fr-BE.json b/messages/fr-BE.json index 2d0848d..3700e90 100644 --- a/messages/fr-BE.json +++ b/messages/fr-BE.json @@ -101,6 +101,7 @@ }, "links": { "accounts": "Comptes", + "commitments": "Engagements", "settings": "Paramètres", "admin": "Admin", "adminAriaLabel": "Espace admin (réservé fondateur)", diff --git a/messages/nl-BE.json b/messages/nl-BE.json index 7dcf478..c85853d 100644 --- a/messages/nl-BE.json +++ b/messages/nl-BE.json @@ -101,6 +101,7 @@ }, "links": { "accounts": "Rekeningen", + "commitments": "Verbintenissen", "settings": "Instellingen", "admin": "Admin", "adminAriaLabel": "Adminzone (alleen oprichter)", diff --git a/src/components/layout/BottomTabBar.tsx b/src/components/layout/BottomTabBar.tsx index 2b2a86e..c079c40 100644 --- a/src/components/layout/BottomTabBar.tsx +++ b/src/components/layout/BottomTabBar.tsx @@ -1,11 +1,27 @@ 'use client'; import { useState, useTransition, useCallback } from 'react'; -import { LayoutDashboard, Receipt, Wallet, Sparkles, Menu } from 'lucide-react'; +import { + HandCoins, + Landmark, + LayoutDashboard, + Menu, + Receipt, + Settings, + Sparkles, + Wallet, + type LucideIcon, +} from 'lucide-react'; import { useTranslations } from 'next-intl'; import { Link } from '@/i18n/navigation'; import { usePathname } from '@/i18n/navigation'; + +import { + MOBILE_TAB_DESTINATIONS, + isDestinationActive, + type AppDestinationId, +} from './app-destinations'; import { MoreSheet } from './MoreSheet'; /** @@ -56,43 +72,40 @@ import { MoreSheet } from './MoreSheet'; // `'use client'` boundary of this file (which crashes every page render // on Next.js 16 + React 19, observed on PR #182 preview Vercel). -type TabId = 'cockpit' | 'bills' | 'expenses' | 'simulate' | 'more'; - -type Tab = { - id: TabId; - href: string; - labelKey: 'cockpit' | 'bills' | 'expenses' | 'simulate' | 'more'; - // Mark routes that must use a startsWith comparison vs the exact-match - // `/app` root. We only have one exact-match tab today but keeping the - // shape explicit prevents a future refactor from quietly breaking the - // cockpit highlight. - match: 'exact' | 'startsWith'; - icon: typeof LayoutDashboard; +/** + * Icons and labels stay HERE, not in the registry: the registry is server-safe + * (no React) and next-intl message keys are typed against `fr-BE.json`, so only + * string literals type-check. Both maps are keyed by `AppDestinationId`, so + * adding a destination without an icon or a label is a TypeScript error — the + * same exhaustiveness the registry gives for the destinations themselves. + * + * Labels are per-surface on purpose. This bar says "Cockpit" / "Factures" / + * "Simuler" (`layout.bottomTab.*`) where the desktop header says "Tableau de + * bord" / "Charges" / "Simulateur" (`common.nav.*`). Sharing one key would have + * silently rewritten copy that was written for each context. + */ +const TAB_ICONS: Record = { + cockpit: LayoutDashboard, + bills: Receipt, + expenses: Wallet, + simulate: Sparkles, + commitments: HandCoins, + accounts: Landmark, + settings: Settings, }; -const TABS: readonly Tab[] = [ - { id: 'cockpit', href: '/app', labelKey: 'cockpit', match: 'exact', icon: LayoutDashboard }, - { id: 'bills', href: '/app/charges', labelKey: 'bills', match: 'startsWith', icon: Receipt }, - { - id: 'expenses', - href: '/app/expenses', - labelKey: 'expenses', - match: 'startsWith', - icon: Wallet, - }, - { - id: 'simulate', - href: '/app/simulator', - labelKey: 'simulate', - match: 'startsWith', - icon: Sparkles, - }, -] as const; - -function isActive(pathname: string, tab: Tab): boolean { - if (tab.match === 'exact') return pathname === tab.href; - return pathname === tab.href || pathname.startsWith(`${tab.href}/`); -} +type BottomTabLabelKey = 'cockpit' | 'bills' | 'expenses' | 'simulate'; + +const TAB_LABELS: Record = { + cockpit: 'cockpit', + bills: 'bills', + expenses: 'expenses', + simulate: 'simulate', + // Sheet-only destinations have no bottom-tab label. + commitments: null, + accounts: null, + settings: null, +}; function triggerHapticFeedback(): void { if (typeof navigator === 'undefined') return; @@ -155,9 +168,10 @@ export function BottomTabBar({ isAdmin = false }: BottomTabBarProps) { className="surface-overlay border-border/40 fixed right-0 bottom-0 left-0 z-40 border-t pb-[env(safe-area-inset-bottom)] md:hidden" >
- {TABS.map((tab) => { - const Icon = tab.icon; - const active = isActive(pathname, tab); + {MOBILE_TAB_DESTINATIONS.map((tab) => { + const Icon = TAB_ICONS[tab.id]; + const labelKey = TAB_LABELS[tab.id]; + const active = isDestinationActive(pathname, tab); return (