Feat/UI improvements

[Kaiohz]

No description provided.

[Kaiohz]

Code Review — PR #17 "Feat/UI improvements"

Score : 5/10 — Refactor UI substantiel et bien intentionné (a11y, formatting, code-splitting), mais la PR a un gros problème structurel qui doit être résolu avant merge, et plusieurs petits points qualité.

---

🔴 Bloquant — Rebase requis (le contenu est déjà sur main)

La branche feat/ui-improvements est diverged par rapport à main : 3 commits en avance, 2 en retard. Quand on regarde les commits behind (ce qui est sur main mais pas dans la PR) :

Commit main (pas dans la PR)	Message
1d49d55	Improve accessibility, style, and code-splitting
ffa9387	Add editorconfig and prettierrc; format codebase

Ces deux commits ont strictement le même message que les 2 commits de tête de la PR (05c30c7 et 40711e0). En pratique, merger cette PR = no-op (ou conflit pur) sur les 100 fichiers de refactor.

Le seul vrai delta une fois le rebase fait, c'est le commit 8a83083 ("Remove cache-to option from Docker build") qui supprime une ligne du cd.yaml :

-          cache-to: type=gha,mode=max

Action : git rebase main puis squash en un seul commit ciblé sur le retrait du cache-to. Les 100 fichiers / +2355/-2100 lignes de refactor sont déjà sur main — ils ne doivent pas être re-mergés.

Note : c'est le même pattern récurrent que sur pickpro-front#176 (cf. review du 8 juillet). À fixer au niveau process (pre-merge hook ou check CI) plutôt que PR par PR.

---

🟡 Description manquante

Le body de la PR est vide (null). Pour une PR de 100 fichiers, c'est un problème de revue :

• Pas de contexte sur l'objectif métier des changements a11y
• Pas de description des nouveaux composants partagés (SegmentedToggle, formConstants, StatusBadge refactor)
• Pas de note sur la stratégie de code-splitting

Action : ajouter au minimum un résumé des changements et un lien vers une éventuelle issue / discussion.

---

🟡 Pas de nouveaux tests pour les comportements ajoutés

J'ai noté 81 lignes ajoutées dans ChatMessage.test.tsx et 58 dans MainLayout.test.tsx, ce qui est bien. Mais les nouveaux comportements n'ont pas de couverture dédiée :

• SegmentedToggle.tsx (nouveau composant, 76 lignes) — 0 test
• IndexActionMenu.tsx refactoré (keyboard nav, outside click, focus management) — testé mais c'est le test existant (151 lignes ajoutées), il faut vérifier qu'il couvre les 3 nouveaux comportements :
  • Fermeture sur outside click (useEffect + mousedown)
  • Navigation clavier (ArrowUp / ArrowDown / Escape / Enter)
  • Focus restoration vers buttonRef après action
• ThreadSidebar.tsx — ajout d'un useEffect sur Escape (alors qu'un <Dialog> est utilisé et le gère déjà) → doublon potentiel, à clarifier

---

🟢 Points positifs

Sur le fond, le refactor est de bonne qualité :

• SegmentedToggle (nouveau) : bonne abstraction a11y avec 3 patterns WAI-ARIA (tab, toggle, radio), typage générique <V extends string>, props Readonly<> → 👍
• IndexActionMenu : passage d'un menu basique à un menu WAI-ARIA compliant (focus management, keyboard nav, outside click, aria-haspopup, aria-expanded, aria-hidden sur l'icône). C'est du vrai travail d'a11y, pas du cosmetic.
• formatDate : passage de toLocaleDateString à Intl.DateTimeFormat → bonne pratique (instance réutilisable).
• useCreateThread onError : ajout d'un onError: () => setShowAgentDialog(false) qui ferme le dialog en cas d'échec → 👍 évite un dialog bloqué ouvert.
• .editorconfig + .prettierrc ajoutés : standardise le formatting pour les futures contributions.
• Suppression cache-to: type=gha,mode=max : passthrough cache-from (read) mais pas cache-to (write) évite l'upload vers le cache GitHub pour un job de scan Trivy qui n'a pas besoin d'être caché. Bonne optimisation CI.

---

🟡 Questions / points à clarifier

1. ThreadSidebar — useEffect Escape redondant ? Le useEffect ajouté pour fermer le dialog sur Escape fait doublon avec le Dialog shadcn (qui gère déjà Escape + focus trap). Le commentaire dit "Dialog handles it, but ensure state sync" — si le Dialog n'appelle pas le onOpenChange proprement, c'est un bug du Dialog, pas un truc à doubler côté consumer.

2. getThreadPreview : la fonction renvoie Thread · ${thread.id.slice(0, 8)} avec un commentaire "for now show a short id-based preview". Si c'est un placeholder, il vaudrait mieux throw new Error("not implemented") ou // TODO(...) explicite + une card pour le signaler dans la PR, plutôt qu'un slice d'ID qui n'apporte rien à l'UX.

3. formConstants.ts : 2 constantes (AGENT_CONFIG_FORM_ID, AGENT_YAML_FORM_ID). C'est bien de les extraire, mais il n'y a pas de test qui vérifie qu'elles sont bien consommées. Une recherche rapide dans la PR pour confirmer que les <form id={...}> et les <form data-testid={...}> pointent bien dessus ?

---

📋 Résumé

Item	Statut
Fonctionnalité a11y / refactor UI	✅ Bon travail, shipped sur main
Description de la PR	❌ Vide
Stratégie de merge	❌ Branche diverged, rebase requis
Test coverage nouveaux composants	⚠️ À vérifier (SegmentedToggle non testé)
Doublon Escape dans ThreadSidebar	⚠️ À clarifier
Fix cache-to dans cd.yaml	✅ Bonne optimisation

Verdict : Changes Requested. Rebase + description de PR + clarification sur l'Escape du ThreadSidebar. Une fois ces 3 points OK, on peut merger.