review-before-commit

Par divinevideo · divine-mobile

Examinez toutes les modifications non committées avant de pousser. Vérifie le code mort, les commentaires obsolètes, les violations des règles CLAUDE.md, les imports inutilisés et les incohérences introduites pendant la session en cours. À invoquer avec /review-before-commit.

npx skills add https://github.com/divinevideo/divine-mobile --skill review-before-commit

Skill d'examen du code

Objectif

Examen de code final de tous les changements non commités avant de pousser. Capture les problèmes qui sont faciles à introduire lors du développement itératif : commentaires obsolètes référençant du code supprimé, code mort, violations de règles et incohérences.

Comment examiner

Étape 1 : Identifier les fichiers modifiés

Exécute git diff --name-only et git diff --cached --name-only pour obtenir la liste complète des fichiers modifiés (staged + unstaged). Si l'utilisateur a fourni un argument de chemin, filtre pour ne garder que les fichiers sous ce chemin.

Étape 2 : Lire tous les fichiers modifiés

Lis chaque fichier modifié en entier. Pour chaque fichier, vérifie les éléments ci-dessous.

Étape 3 : Exécuter les vérifications

Pour chaque fichier modifié, vérifie :

Code mort

  • Imports inutilisés (import non référencé nulle part dans le fichier)
  • Champs privés, méthodes ou getters inutilisés
  • Code inaccessible après des retours précoces
  • Blocs de code commentés (doivent être supprimés, pas commentés)

Références obsolètes

  • Commentaires ou docstrings référençant des méthodes, classes ou variables qui n'existent plus dans le codebase (utilise Grep pour vérifier que les références sont toujours valides)
  • Commentaires ABOUTME qui ne décrivent plus précisément le fichier
  • Commentaires TODO pour du travail déjà complété dans cette session

Docs de réflexion

Tout fichier sous mobile/docs/brainstorm/ (généralement datés comme YYYY-MM-DD-issueNNNN-…-brainstorm.md) est un artefact de travail, pas un livrable. Au moment où la PR est prête, la justification doit figurer dans la description de la PR ; le fichier ne doit pas être livré avec le commit de fusion.

Signale chaque fichier staged sous ce chemin et demande à l'utilisateur si le document de réflexion doit être supprimé ou converti sur place :

  • Le supprimer (par défaut) — git rm le fichier. Utilise un commit docs: drop … brainstorm doc séparé pour que la suppression soit auditable.
  • Le convertir en décision record — uniquement quand le doc a une valeur durable au-delà de la PR (p.ex. capture une approche rejetée que les lecteurs futurs proposeront continuellement). Renomme, déplace hors de brainstorm/, et réduis aux parties durables. Précédent : cd723075e docs(notifications): convert badge-desync brainstorm to decision record.

Précédents pour suppression pure et simple : 380bf50c1 docs: drop PR #4229 brainstorm doc, et le suivi de l'examen de la PR #4234.

Violations des règles CLAUDE.md

Lis et applique TOUTES les règles de .claude/CLAUDE.md et .claude/rules/. Ne les code pas en dur ici — consulte toujours la source de vérité dans ces fichiers.

Patterns Flutter — Découvertes d'examen connues

Ces patterns ont émergé des cycles d'examen précédents sur ce repo et ne figurent pas dans les règles toujours chargées (pour garder la fenêtre de contexte global allégée). Vérifie-les explicitement lors de l'examen du code.

Design System

  • Widget sur mesure s'écarte de divine_ui sans note de docstring. Tout widget qui s'écarte délibérément d'un composant divine_ui (taille différente, structure différente, variante contournée) doit dire pourquoi dans sa docstring de classe — quel composant du design system s'en rapproche et qu'est-ce qui a forcé la divergence. Sans cela, les relecteurs relèveront « pourquoi ne pas simplement utiliser DivineIconButton ? »

    // Bon
    /// Visuellement équivalent à [DivineIconButton] en style ghost mais dimensionné 64×64
    /// avec une icône 32 px au lieu des présets 40×40/56×56 de DivineIconButton,
    /// car le spec Figma (node 15314:53971) demande une cible tactile plus grande
    /// que n'importe quelle taille standard de DivineIconButton.
    class CenterPlaybackControl extends StatelessWidget { ... }

Widget API Design

  • Paramètres spéculatifs sur les widgets réutilisables. Avant d'ajouter un paramètre à quoi que ce soit dans divine_ui ou lib/utils/, confirme que la branche qu'il déverrouille est accessible depuis au moins un appelant. Un paramètre Builder/Callback/Resolver avec exactement un appelant qui le fournit toujours est une branche morte. Supprime-la — l'ajouter à nouveau plus tard est bon marché ; les branches mortes confondent les lecteurs et gonflent la surface API.

    // Mauvais — initialChildSizeBuilder n'est appelé que depuis une surface où
    // le clavier n'est jamais ouvert, donc la branche est inaccessible.
    static Future<T?> show<T>({
      double initialChildSize = 0.6,
      double Function(BuildContext)? initialChildSizeBuilder,
    }) { ... }
    
    // Bon — branche spéculative supprimée.
    static Future<T?> show<T>({double initialChildSize = 0.6}) { ... }
  • Injection d'ancêtre via une fermeture builder unique. Quand un site d'appel doit mettre un BlocProvider / InheritedWidget au-dessus de chaque slot d'une modal/sheet/route, ajoute un paramètre contentWrapper à la cible au lieu de passer une fermeture Widget Function(BuildContext, Widget) au site d'appel. La fermeture est un _buildFoo en disguise et manque silencieusement les nouveaux slots ajoutés plus tard.

    // Mauvais — fermeture builder cuite au site d'appel.
    showMySheet(context, builder: (ctx, child) => BlocProvider<MyBloc>(
      create: (_) => MyBloc(), child: child));
    
    // Bon — la cible expose contentWrapper ; le provider couvre chaque slot.
    showMySheet(context,
      contentWrapper: (ctx, child) => BlocProvider<MyBloc>(
        create: (_) => MyBloc(), child: child));

State Management

  • Bloc limité à la modal instancié au site d'appel avec try/finally close(). Mets le BlocProvider à l'intérieur du sous-arbre de la modal (via contentWrapper ou équivalent). BlocProvider gère la fermeture automatiquement au démontage — toute route qui dépile via un chemin autre que le retour d'await fuit silencieusement le bloc dans le pattern try/finally.

    // Mauvais — cycle de vie géré au site d'appel.
    final bloc = CommentsBloc()..add(const CommentsLoadRequested());
    try {
      await showSheet(title: BlocProvider.value(value: bloc, child: _Title()));
    } finally {
      await bloc.close();
    }
    
    // Bon — BlocProvider possède la fermeture au démontage.
    return showSheet(
      contentWrapper: (ctx, child) => BlocProvider<CommentsBloc>(
        create: (_) => CommentsBloc()..add(const CommentsLoadRequested()),
        child: child),
      title: _Title());

Commentaires

  • Commentaires de rationale de conception multilignes pour le bénéfice de Claude. Les commentaires inline de longueur paragraphe dérivent dès que le code change, et les LLMs les citent comme faisant autorité même quand ils sont obsolètes. Si l'explication dépasse une phrase, déplace-la vers la description de la PR ou un fichier de règles et laisse un pointeur // See PR #N inline — pas le paragraphe complet.

    // Mauvais — paragraphe au-dessus de ClipRRect qui dérivera silencieusement.
    // La coquille extérieure utilise des coins inférieurs 30 px et le conteneur
    // onglets interne utilise des coins supérieurs 32 px pour que la surface
    // interne s'imbrique visiblement ...
    ClipRRect(borderRadius: BorderRadius.vertical(bottom: Radius.circular(30)));
    
    // Bon — la rationale vit dans la constante nommée.
    // Le rayon interne est 2 px plus grand pour que le conteneur onglets s'imbriques visiblement.
    ClipRRect(borderRadius: BorderRadius.vertical(
      bottom: Radius.circular(VineTheme.shellCornerRadius)));

UI / Animations

  • ValueKey redondant sur les branches AnimatedSwitcher qui sont déjà de types runtime différents. AnimatedSwitcher compare runtimeType + key. Les branches qui retournent déjà des types de widget différents (Center, IgnorePointer, SizedBox, …) sont déjà distinguables — ajouter ValueKey est redondant et risque les assertions « duplicate GlobalKey » quand le widget est réutilisé. Ajoute un ValueKey uniquement quand (a) deux branches sont du même type runtime, ou (b) un test s'ancre sur la clé via find.byKey(...) (laisse un commentaire le disant).

  • Timer (ou Future.delayed) pour les timages UI. Timer continue de s'exécuter après le démontage du widget sauf annulation manuelle ; il ne s'interrompt pas avec la route ; il ne respecte pas MediaQuery.disableAnimations ; et peut déclencher un setState en milieu de frame. Utilise AnimationController + FadeTransition / AnimatedBuilder pour les flashes transitoires, pulsations de badge ou overlays style snackbar. Atteins Timer uniquement quand l'effet est véritablement hors du pipeline de rendu (p.ex. debounce réseau, délai analytique).

    // Bon
    late final AnimationController _feedbackController = AnimationController(
      vsync: this, duration: const Duration(milliseconds: 550));
    void _triggerFeedback() => _feedbackController.forward(from: 0);
    // In build:
    FadeTransition(
      opacity: Tween(begin: 1.0, end: 0.0).animate(_feedbackController),
      child: const FeedbackIcon());

Tests

  • Bornes absolues de temps mur (<100 ns, <100 ms). Les runners CI partagés sont bruyants et substantiellement plus lents que les laptops de dev — ceux-ci deviennent flaky. Utilise une comparaison relative (expect(fastMs, lessThan(slowMs))), enveloppe dans fakeAsync pour les vérifications de durée logique, ou skip avec skip: true + un commentaire TODO(any):. Ne relève jamais le seuil — cela retarde juste le prochain flake.

  • expect(tester.takeException(), isNotNull) comme signal d'effet de bord. very_good test --optimization fusionne les fichiers de test ; l'état keepAlive Riverpod fuyant d'un test antérieur peut silencieusement résoudre la dépendance, supprimant l'exception. Affirme uniquement le contrat nommé du test ; vide les erreurs incidentes sans assertion :

    tester.takeException(); // vide erreur incidente — pas d'assertion
  • tester.tapAt(Offset(...)) pour interaction modale. Les taps basés sur les coordonnées sont sensibles au flag expand de la modal : DraggableScrollableSheet(expand: false) a un espace vide transparent au-dessus de son contenu, tandis que expand: true remplit ce rectangle avec le hit-testing de la sheet. Préfère find.text(...) / find.byType(...) / find.bySemanticsIdentifier(...). Quand tapAt est inévitable, documente l'hypothèse de layout inline et garde l'offset bien à l'intérieur de la région prévue.

  • DateTime.now() dans le code comparé à tester.pump(Duration). L'horloge du test Flutter avance via pump ; DateTime.now() lit l'horloge murale du host — ils ne concordent pas. Utilise clock.now() de package:clock dans le code de production et withClock(Clock(() => now), () async { ... }) dans le test pour que les deux horloges avancent ensemble. (clock est déjà une dépendance transitive — aucun changement pubspec.yaml nécessaire.)

    // Code testé :
    import 'package:clock/clock.dart';
    if (clock.now().difference(_pausedAt!) >= _minPauseForFeedback) {
      _triggerUnpauseFeedback();
    }
    
    // Test :
    var now = DateTime(2026);
    await withClock(Clock(() => now), () async {
      now = now.add(const Duration(milliseconds: 220));
      await tester.pump(const Duration(milliseconds: 220));
      expect(find.byType(UnpauseFeedback), findsOneWidget);
    });

Test Consistency

  • Si le code de production a changé, vérifie que les tests correspondants existent et correspondent toujours
  • Vérifie les assertions de test qui référencent des champs ou méthodes supprimés
  • Vérifie que les configurations de mock correspondent aux signatures de méthode actuelles

Étape 4 : Exécuter l'analyseur et les tests

  1. Exécute mcp__dart__analyze_files sur tous les fichiers modifiés
  2. Exécute mcp__dart__run_tests sur tous les fichiers de test modifiés
  3. Signale les défaillances

Étape 5 : Rapporter les découvertes

Présente les découvertes groupées par sévérité :

## Résumé de l'examen

### À corriger obligatoirement
- [file.dart:42](path/to/file.dart#L42) - Description du problème

### À corriger de préférence
- [file.dart:15](path/to/file.dart#L15) - Description du problème

### Détail mineur
- [file.dart:7](path/to/file.dart#L7) - Description du problème

### Aucun problème
Si aucun problème trouvé, confirme : « Aucun problème trouvé. Prêt à pousser. »

Définitions de sévérité :

  • À corriger obligatoirement : Causera des bugs, défaillances de test ou défaillances CI
  • À corriger de préférence : Viole les règles du projet, code mort, commentaires obsolètes
  • Détail mineur : Préférences de style, améliorations mineures

Après signalement, corrige automatiquement tous les éléments « À corriger obligatoirement » et « À corriger de préférence ». Demande à l'utilisateur avant de corriger les éléments « Détail mineur ».

Skills similaires