feat(nav): rendre l'oubli d'une destination impossible (Phase 1, lot 1)#262
Merged
Conversation
…missing one /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<AppDestinationId, …>, 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.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
status:review-needed
Contributor
|
🧙 Sourcery examine votre pull request ! Conseils et commandesInteragir avec Sourcery
Personnaliser votre expérienceAccédez à votre dashboard pour :
Obtenir de l’aide
Original review guide in English🧙 Sourcery is reviewing your pull request! Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
thierryvm
added a commit
that referenced
this pull request
Jul 26, 2026
…SEO (#265) Handoff de fin de session. Documentation uniquement, 1 fichier. **Livré** : #261 (redirections localisées), #262 (registre des destinations de nav), #263 (sélecteur de langue), #264 (SEO) — toutes mergées. **Reste à faire, ordonné** : le lot 2 de la refonte UX en tête, avec les trois limites que le lot 1 s'est explicitement interdit de traiter reprises comme entrées plutôt que laissées à re-découvrir. Puis la reconnexion fantôme, sans diagnostic à ce jour. Puis les dettes tracées. **Frictions d'environnement consignées** parce qu'elles ont coûté du temps deux fois chacune : `playwright.config.ts` ne charge pas `.env.local`, `e2e:auth` sert un build prod où le rate limit échoue en fermé sur l'Upstash factice, la variable est `E2E_BASE_URL`, et le port 3000 appartient à un autre projet. Miroir dans le vault Obsidian, slug vérifié via `_index.md`.
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.
Premier lot de la Phase 1 de la refonte UX. Aucun changement visuel — il ferme la cause structurelle du bug que tu as signalé, et ajoute le lien manquant.
Le défaut
/app/commitments(« Engagements ») était inatteignable depuis la nav mobile : déclaré dans le header desktop (hidden lg:flex) et nulle part ailleurs. Hors du cockpit, la page n'existait plus sur téléphone.La cause n'est pas le lien manquant, c'est qu'on pouvait l'oublier. Les destinations étaient déclarées trois fois — tableau privé dans
BottomTabBar, JSX en dur dansMoreSheet, JSX en dur dansHeader— sans rien pour les relier.Le correctif
src/components/layout/app-destinations.ts, module server-safe, déclare les 7 destinations une seule fois. Les trois surfaces le consomment.Le test lit le système de fichiers dans les deux sens : une route sans entrée échoue, et une entrée pointant vers une route supprimée échoue aussi (sinon on rendrait un lien 404 sur les trois surfaces sans que rien ne bronche). Falsifiabilité vérifiée : registre amputé d'Engagements → rouge ; restauré → vert.
Décisions que
plan-reviewera corrigées avant que j'écrive une lignemobilePlacement, passurface— le header desktop rend la liste complète en l'ignorant. Un champ nommésurfaceaurait invité un futur lecteur à filtrer dessus et à faire disparaître des destinations du desktop.bills→/app/charges,simulate→/app/simulator, parce qu'ils sont gravés dans lesdata-testidassertés par les suites unitaire et e2e.Ce que la validation e2e a révélé
Un test existant a échoué à cause de ce changement : le lien ajouté rehausse le sheet, et Playwright clique le centre d'un élément par défaut. Le backdrop étant
fixed inset-0, son centre est le milieu du viewport — désormais couvert par le sheet. Le clic atterrissait sur le sheet.L'intention du test était juste, son point de clic était devenu faux : il vise maintenant la bande exposée en haut, ce que fait un utilisateur.
À retenir pour le lot de redesign : le sheet grandit avec son contenu et occupe déjà une part notable d'un iPhone SE. La répartition barre/sheet devra être arbitrée sur pièces.
Preuve
npm run testtypecheck·lint·lint:use-serverbottom-tab-bariPhone 14bottom-tab-bariPhone SECes specs s'auto-skippent en CI (pas de
SUPABASE_SERVICE_ROLE_KEY), donc la CI ne peut pas valider ce lot : la passe locale était exigée par la revue, elle a été faite. Deux frictions consignées dans le rapport —playwright.config.tsne charge pas.env.local, ete2e:authsert un build prod où le rate limit échoue en fermé sur l'Upstash factice.Limite assumée
Sur
/app/commitments, aucun onglet ne portearia-current— même comportement qu'/adminaujourd'hui. Résolu au lot de redesign.Rapport :
docs/prs/PR-nav-destination-registry-report.mdSummary by Sourcery
Introduire un registre unique, sûr côté serveur, pour les destinations de navigation du cockpit et relier toutes les surfaces de navigation à celui-ci afin d’éviter que certaines routes deviennent inaccessibles, notamment en exposant Engagements sur mobile sans modifier la conception visuelle.
New Features:
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
Introduce a single, server-safe registry for cockpit navigation destinations and wire all navigation surfaces to it to prevent routes from becoming unreachable, notably exposing Engagements on mobile without changing the visual design.
New Features:
Enhancements:
Documentation:
Tests:
Nouvelles fonctionnalités :
Améliorations :
Documentation :
Tests :
/appet le registre de destinations de navigation, et qui valident les invariants de placement mobile.Original summary in English
Summary by Sourcery
Introduire un registre unique, sûr côté serveur, pour les destinations de navigation du cockpit et relier toutes les surfaces de navigation à celui-ci afin d’éviter que certaines routes deviennent inaccessibles, notamment en exposant Engagements sur mobile sans modifier la conception visuelle.
New Features:
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
Introduce a single, server-safe registry for cockpit navigation destinations and wire all navigation surfaces to it to prevent routes from becoming unreachable, notably exposing Engagements on mobile without changing the visual design.
New Features:
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
Introduire un registre unique, sûr côté serveur, pour les destinations de navigation du cockpit et relier toutes les surfaces de navigation à celui-ci afin d’éviter que certaines routes deviennent inaccessibles, notamment en exposant Engagements sur mobile sans modifier la conception visuelle.
New Features:
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
Introduce a single, server-safe registry for cockpit navigation destinations and wire all navigation surfaces to it to prevent routes from becoming unreachable, notably exposing Engagements on mobile without changing the visual design.
New Features:
Enhancements:
Documentation:
Tests:
Nouvelles fonctionnalités :
Améliorations :
Documentation :
Tests :
/appet le registre de destinations de navigation, et qui valident les invariants de placement mobile.Original summary in English
Summary by Sourcery
Introduire un registre unique, sûr côté serveur, pour les destinations de navigation du cockpit et relier toutes les surfaces de navigation à celui-ci afin d’éviter que certaines routes deviennent inaccessibles, notamment en exposant Engagements sur mobile sans modifier la conception visuelle.
New Features:
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
Introduce a single, server-safe registry for cockpit navigation destinations and wire all navigation surfaces to it to prevent routes from becoming unreachable, notably exposing Engagements on mobile without changing the visual design.
New Features:
Enhancements:
Documentation:
Tests: