Review Skill

Purpose

Final code review of all uncommitted changes before pushing. Catches issues that are easy to introduce during iterative development: stale comments referencing removed code, dead code, rule violations, and inconsistencies.

How to Review

Step 1: Identify changed files

Run git diff --name-only and git diff --cached --name-only to get the full list of modified files (staged + unstaged). If the user provided a path argument, filter to only files under that path.

Step 2: Read all changed files

Read every changed file in full. For each file, check the items below.

Step 3: Run checks

For each changed file, check for:

Dead Code

• Unused imports (import not referenced anywhere in the file)
• Unused private fields, methods, or getters
• Unreachable code after early returns
• Commented-out code blocks (should be deleted, not commented)

Stale References

• Comments or doc strings referencing methods, classes, or variables that no longer exist in the codebase (use Grep to verify references are still valid)
• ABOUTME comments that no longer accurately describe the file
• TODO comments for work that was already completed in this session

Brainstorm Docs

Any file under mobile/docs/brainstorm/ (typically dated like YYYY-MM-DD-issueNNNN-…-brainstorm.md) is a working artefact, not a deliverable. By the time the PR is ready, the rationale belongs in the PR description; the file should not ship with the merge commit.

Flag every staged file under that path and ask the user whether the brainstorm should be dropped or converted in place:

• Drop it (default) — git rm the file. Use a separate docs: drop … brainstorm doc commit so the deletion is auditable.
• Convert it to a decision record — only when the doc has lasting value beyond the PR (e.g. captures a rejected approach future readers will keep proposing). Rename, move out of brainstorm/, and trim to the durable bits. Precedent: cd723075e docs(notifications): convert badge-desync brainstorm to decision record.

Precedents for outright deletion: 380bf50c1 docs: drop PR #4229 brainstorm doc, and the follow-up to PR #4234 review.

AGENTS.md Rule Violations

Read and apply ALL rules from AGENTS.md and .claude/rules/. Do not hardcode specific rules here — always check the source of truth in those files.

Flutter Patterns — Known Review Findings

These patterns emerged from past review cycles on this repo and are not in the always-loaded rules (to keep the global context window lean). Check for them explicitly during code review.

Design System

• Bespoke widget diverges from divine_ui without a docstring note. Any widget that deliberately differs from a divine_ui component (different size, different structure, bypassed variant) must say why in its class docstring — which design-system component it's close to and what forced the divergence. Without it, reviewers will re-raise "why not just use DivineIconButton?"

  // Good
  /// Visually equivalent to a [DivineIconButton] in ghost style but sized 64×64
  /// with a 32 px icon instead of DivineIconButton's 40×40/56×56 presets,
  /// because the Figma spec (node 15314:53971) calls for a larger tap target
  /// than any standard DivineIconButton size.
  class CenterPlaybackControl extends StatelessWidget { ... }
  

Widget API Design

• Speculative parameters on reusable widgets. Before adding a parameter to anything in divine_ui or lib/utils/, confirm the branch it unlocks is reachable from at least one caller. A Builder/Callback/Resolver parameter with exactly one caller that always supplies it is a dead branch. Delete it — adding it back later is cheap; dead branches confuse readers and inflate the API surface.

  // Bad — initialChildSizeBuilder is only called from a surface where
  // the keyboard is never open, so the branch is unreachable.
  static Future<T?> show<T>({
    double initialChildSize = 0.6,
    double Function(BuildContext)? initialChildSizeBuilder,
  }) { ... }
  
  // Good — speculative branch deleted.
  static Future<T?> show<T>({double initialChildSize = 0.6}) { ... }
  

• Ancestor injection via a one-off builder closure. When a call site needs to put a BlocProvider / InheritedWidget above every slot of a modal/sheet/route, add a contentWrapper parameter to the target instead of passing a Widget Function(BuildContext, Widget) closure at the call site. The closure is a _buildFoo in disguise and silently misses new slots added later.

  // Bad — builder closure baked into the call site.
  showMySheet(context, builder: (ctx, child) => BlocProvider<MyBloc>(
    create: (_) => MyBloc(), child: child));
  
  // Good — target exposes contentWrapper; provider covers every slot.
  showMySheet(context,
    contentWrapper: (ctx, child) => BlocProvider<MyBloc>(
      create: (_) => MyBloc(), child: child));
  

State Management

• **Modal-scoped bloc instantiated at the call site with try/finally close(). ** Put the BlocProvider inside the modal's subtree (via contentWrapper or equivalent). BlocProvider handles the close automatically on unmount — any route that pops via a path other than await returning silently leaks the bloc in the try/finally pattern.

  // Bad — lifecycle managed at call site.
  final bloc = CommentsBloc()..add(const CommentsLoadRequested());
  try {
    await showSheet(title: BlocProvider.value(value: bloc, child: _Title()));
  } finally {
    await bloc.close();
  }
  
  // Good — BlocProvider owns the close on unmount.
  return showSheet(
    contentWrapper: (ctx, child) => BlocProvider<CommentsBloc>(
      create: (_) => CommentsBloc()..add(const CommentsLoadRequested()),
      child: child),
    title: _Title());
  

Comments

• Multi-line design-rationale comments for Codex's benefit. Paragraph- length inline comments drift the moment the code changes, and LLMs cite them as authoritative even when stale. If the explanation is more than one sentence, move it to the PR description or a rule file and leave a // See PR #N pointer inline — not the full paragraph.

  // Bad — paragraph above ClipRRect that will silently drift.
  // The outer shell uses 30 px bottom corners and the inner tabs container
  // uses 32 px top corners so that the inner surface visibly nests ...
  ClipRRect(borderRadius: BorderRadius.vertical(bottom: Radius.circular(30)));
  
  // Good — rationale lives in the named constant.
  // Inner radius is 2 px larger so the tabs container visibly nests.
  ClipRRect(borderRadius: BorderRadius.vertical(
    bottom: Radius.circular(VineTheme.shellCornerRadius)));
  

UI / Animations

• Redundant ValueKey on AnimatedSwitcher branches that are already different runtime types. AnimatedSwitcher compares runtimeType + key. Branches that already return different widget types (Center, IgnorePointer, SizedBox, …) are already distinguishable — adding ValueKey is redundant and risks "duplicate GlobalKey" assertions when the widget is reused. Only add a ValueKey when (a) two branches are the same runtime type, or (b) a test anchors on the key via find.byKey(...) (leave a comment saying so).

• Timer (or Future.delayed) for UI timing. Timer keeps firing after the widget is disposed unless manually cancelled; it doesn't pause with the route; it doesn't respect MediaQuery.disableAnimations; and it can land a setState mid-frame. Use AnimationController + FadeTransition / AnimatedBuilder for transient flashes, badge pulses, or snackbar-like overlays. Reach for Timer only when the effect is genuinely outside the rendering pipeline (e.g. network debounce, analytics delay).

  // Good
  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

• Absolute wall-clock timing bounds (<100 ns, <100 ms). Shared CI runners are noisy and substantially slower than dev laptops — these flake. Use a relative comparison (expect(fastMs, lessThan(slowMs))), wrap in fakeAsync for logical-duration checks, or skip with skip: true + a TODO(any): comment. Never bump the threshold — that just delays the next flake.

• expect(tester.takeException(), isNotNull) as a side-effect signal. very_good test --optimization merges test files; leaked Riverpod keepAlive state from an earlier test can silently resolve the dependency, suppressing the exception. Assert only the test's named contract; drain incidental errors without an assertion:

  tester.takeException(); // drain incidental error — no assertion
  

• tester.tapAt(Offset(...)) for modal interaction. Coordinate-based taps are sensitive to the modal's expand flag: DraggableScrollableSheet(expand: false) has transparent empty space above its content, while expand: true fills that rectangle with the sheet's hit-testing. Prefer find.text(...) / find.byType(...) / find.bySemanticsIdentifier(...). When tapAt is unavoidable, document the layout assumption inline and keep the offset well inside the intended region.

• DateTime.now() in code compared against tester.pump(Duration). The Flutter test clock advances via pump; DateTime.now() reads the host wall clock — they don't agree. Use package:clock's clock.now() in production code and withClock(Clock(() => now), () async { ... }) in the test so both clocks advance together. (clock is already a transitive dependency — no pubspec.yaml change needed.)

  // Code under 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

• If production code changed, verify corresponding tests exist and still match
• Check for test assertions that reference removed fields or methods
• Verify mock setups match current method signatures

Step 4: Run analyzer and tests

1. Run mcp__dart__analyze_files on all changed files
2. Run mcp__dart__run_tests on all changed test files
3. Report any failures

Step 5: Report findings

Present findings grouped by severity:

## Review Summary

### Must Fix
- [file.dart:42](path/to/file.dart#L42) - Description of the issue

### Should Fix
- [file.dart:15](path/to/file.dart#L15) - Description of the issue

### Nitpick
- [file.dart:7](path/to/file.dart#L7) - Description of the issue

### All Clear
If no issues found, confirm: "No issues found. Ready to push."

Severity definitions:

• Must Fix: Will cause bugs, test failures, or CI failures
• Should Fix: Violates project rules, dead code, stale comments
• Nitpick: Style preferences, minor improvements

After reporting, fix all "Must Fix" and "Should Fix" items automatically. Ask the user before fixing "Nitpick" items.