code-review-checklist Reference

Detailed checklists and review criteria by category.

1. Architecture#

Clean Architecture Review#

Feature Module Independence#

// ✅ Correct dependency
import 'package:core/core.dart';
import 'package:feature_common_auth/auth.dart';

// ❌ Incorrect dependency (direct reference to another application feature)
import 'package:feature_application_home/home.dart';

Checklist#

• Domain → Data → Presentation dependency direction followed
• Business logic accessed through UseCases
• Repository interface separation (I prefix)
• No direct dependencies between feature modules
• Shared code separated into common or core
• No over-abstraction

2. State Management#

BLoC Pattern Review#

State Definition Pattern#

// ✅ Correct status definition (sealed class + Equatable, no codegen)
@immutable
sealed class HomeStatus extends Equatable {
  const HomeStatus();

  const factory HomeStatus.initial() = HomeInitial;
  const factory HomeStatus.loading() = HomeLoading;
  const factory HomeStatus.loaded(User user) = HomeLoaded;
  const factory HomeStatus.error(Failure failure) = HomeError;

  @override
  List<Object?> get props => const [];
}

// ✅ State class: immutable fields + copyWith + Equatable props
@immutable
final class HomeState extends Equatable {
  const HomeState({this.status = const HomeStatus.initial()});

  final HomeStatus status;

  HomeState copyWith({HomeStatus? status}) =>
      HomeState(status: status ?? this.status);

  @override
  List<Object?> get props => [status];
}

// ❌ Incorrect: mutable, ad-hoc flags, no Equatable/copyWith
class HomeState {
  bool isLoading = false;
  User? user;
  String? error;
}

kobic does NOT use @freezed. State/Event are plain sealed class ... extends Equatable with const factory sub-states and copyWith — no *.freezed.dart codegen.

Dart's Error (TypeError, StateError, NoSuchMethodError, JsonUnsupportedObjectError, ...) does not extend Exception, but most catch blocks conventionally use on Exception catch only. Behind a normal awaited call this is harmless — an uncaught Error propagates and surfaces as a crash/log. Behind unawaited(fn()) (fire-and-forget), a missed Error becomes an exception no code path observes; whether it also surfaces as a crash/log elsewhere depends on the execution zone (plain Dart isolate vs. a Flutter app's guarded zone) — but regardless, the specific consumer waiting on the result (BLoC/Cubit state, a ValueNotifier/useState-gated button or spinner, a StreamController) hangs forever — infinite loading spinner, permanently disabled button, dialog that never closes.

// ❌ Error thrown inside disappears completely — consumer hangs forever
unawaited(_revalidate(query, controller));

Future<void> _revalidate(...) async {
  try {
    await saveToCache(...);   // may throw a JsonUnsupportedObjectError / TypeError (an Error, not Exception)
  } on Exception catch (e, st) {   // ❌ Error passes straight through
    Log.e('failed', error: e, stackTrace: st);
  }
}

// ✅ on Object catches both Exception and Error
} on Object catch (e, st) {
  Log.e('failed', error: e, stackTrace: st);
}

Real case: kobic #7746 (SwrStrategyImpl._revalidate, see [cc-flutter:swr-pattern] §16). A codebase-wide audit of unawaited( call sites found 11 confirmed reproductions of this pattern — see [cc-dev:discovery-audit] for the triage→verify audit workflow.

Checklist#

• Event/State are sealed class extends Equatable (no @freezed)
• State branching with const factory sub-states + final class implementations
• Proper error handling
• Loading state management
• Resources released in dispose
• Global/local state properly distinguished
• Any new unawaited(fn()) call: fn()'s full call chain catches Error, not just Exception (see above)

3. Security#

Sensitive Information Review#

Input Validation#

// ✅ Correct validation
if (!EmailValidator.validate(email)) {
  return left(ValidationFailure('Invalid email'));
}

// ❌ Direct use without validation
final user = await api.login(email, password);

Checklist#

• No hardcoded API keys or secrets
• No sensitive information in logs
• User input sanitization
• SQL Injection, XSS prevention
• Protected endpoint access control
• Proper token management

4. Performance#

Rebuild Optimization#

Image Optimization#

// ✅ Correct image handling
CachedNetworkImage(
  imageUrl: url,
  cacheWidth: 200,
  cacheHeight: 200,
)

// ❌ Cache size not specified
Image.network(url)

Checklist#

• const widgets utilized
• BlocSignalBuilder buildWhen used
• Refined with BlocSignalSelector
• Image cacheWidth/cacheHeight applied
• Parallelizable tasks parallelized
• debounce/throttle applied
• Resources released in dispose
• Stream subscriptions cancelled

아래 항목은 analyzer 가 잡지 않는다. 리뷰에서 본다.

• 모든 await 뒤에 if (isClosed) return; — close() 는 진행 중 핸들러를 취소하지 않음
• 상태를 새 인스턴스로 방출 — in-place 변경 후 재방출은 자동 de-dup 에 조용히 삼켜짐
• 상태 타입이 불변이고 의미 있는 == 를 가짐
• 핸들러 내 상태 읽기는 stateValue (state 는 ReadonlySignal<S>)
• .listen 구독을 close() 오버라이드에서 cancel + await super.close() — restartable() 은 emission 만 버리고 구독을 취소하지 않음
• await cancel() 뒤에 listen 하는 재구독 경로에 세대 토큰이 있는가 (아래 §재구독 겹침) — 없으면 겹침 시 앞 구독이 취소 없이 덮이고, 맵에서 빠진 그 구독은 close() 의 순회 대상도 아니라 close() 로도 회수되지 않는다
• ⚠️ 그 재구독이 공유 mixin·헬퍼 안에 있으면 소비 BLoC 리뷰에는 호출 한 줄만 보인다 — 한 단계 내려가 확인했는가
• 상태 병합 규칙이 현재 스코프 밖 데이터를 되살리지 않는가 (스프레드·페이지·필터 등) — 구독 경합이 남긴 낡은 데이터는 이 규칙을 타고 화면으로 올라온다
• context.watch<T>().stateValue 없음 (상태를 구독하지 않음 → BlocSignalBuilder 사용)
• 전역/최상위 컨테이너 인스턴스 없음 (SSR·테스트 격리 위험)
• expect 리스트에 동일 상태 연속 중복 없음 (de-dup 으로 방출되지 않음)

restartable() 이 구독을 취소해 주지 않으므로, SWR/watch 계열은 "이전 구독을 await cancel() 한 뒤 새로 listen" 하는 재구독 코드를 갖게 됩니다. add() 는 핸들러를 await 하지 않으므로(기본 concurrent) 그 await 동안 같은 키의 호출이 뒤따라 들어올 수 있고, 그러면 둘 다 "이전 구독 없음" 으로 보고 각자 listen 한 뒤 나중 호출이 앞의 구독을 취소 없이 덮어씁니다. 덮인 구독은 맵에서 빠져 있어 close() 의 순회에도 걸리지 않은 채 계속 이벤트를 소비합니다(좀비 구독).

❌ WRONG — cancel 과 listen 사이에 await 창이 열려 있다:

Future<void> _subscribe(Object key) async {
  await _subscriptions.remove(key)?.cancel();
  if (isClosed) return;
  _subscriptions[key] = source().listen(_onData); // 겹치면 앞의 구독이 덮인다
}

✅ Right — 세대 토큰으로 마지막 호출만 구독을 연다:

final Map<Object, StreamSubscription<Object?>> _subscriptions = {};
final Map<Object, int> _generations = {};

int _nextGeneration(Object key) {
  final next = (_generations[key] ?? 0) + 1;
  _generations[key] = next;
  return next;
}

Future<void> _subscribe(Object key) async {
  final generation = _nextGeneration(key);
  await _subscriptions.remove(key)?.cancel();
  if (isClosed) return;
  // await 동안 같은 키의 호출이 뒤따라 들어왔으면 이 호출은 이미 낡았다 —
  // 구독을 열지 않고 물러난다.
  if (_generations[key] != generation) return;

  late final StreamSubscription<Object?> subscription;
  subscription = source().listen(
    _onData,
    onDone: () {
      // 더 새로운 구독의 등록을 지우지 않도록 자기 자신일 때만 해제한다.
      if (identical(_subscriptions[key], subscription)) {
        _subscriptions.remove(key);
      }
    },
  );
  // emit 이나 listen 이 동기적으로 같은 키의 재발행을 유발했을 수 있다 —
  // 등록 직전에 한 번 더 확인하고, 낡았으면 방금 연 구독을 닫는다.
  if (_generations[key] != generation) {
    await subscription.cancel();
    return;
  }
  final previous = _subscriptions[key];
  if (previous != null && !identical(previous, subscription)) {
    // 등록 교체는 이미 동기적으로 끝났고 이 Future 를 기다리는 소비자가 없다 —
    // §2 의 unawaited 규칙이 겨냥하는 "결과를 기다리다 멈추는 소비자" 가 없는 자리다.
    unawaited(previous.cancel());
  }
  _subscriptions[key] = subscription;
}

/// 명시적 취소도 **세대를 올린다** — 그러지 않으면 진행 중이던 호출이
/// 취소 뒤에 구독을 새로 연다.
Future<void> _cancelSubscription(Object key) {
  _nextGeneration(key);

  return _subscriptions.remove(key)?.cancel() ?? Future<void>.value();
}

이 로직의 정본은 공유 mixin 한 곳(SwrBlocSignalMixin / SwrCubitSignalMixin)이 소유합니다 — BLoC 마다 손으로 재구현하지 마세요. SWR 계약 자체는 [cc-flutter:swr-pattern] 참조.

실사고: unibook #13475 / PR #13478 — 도서 진입 시 pageChanged 두 건(페이지 복원 리스너와, 동기화 원천 판정을 await 한 뒤 재발행한 경로)이 겹쳐 SwrBlocSignalMixin.emitSwrStream 의 좌/우 페이지 키마다 좀비 구독이 남았습니다. 스티키 노트가 local-first 영구 watch 를 타기 시작하면서(그전 서버 SWR 스트림은 스스로 완료돼 좀비가 자연 소멸했습니다) 그 구독이 영구히 살아남았고, drift 는 테이블 단위 invalidation 이라 같은 테이블의 무관한 행 쓰기(읽기 진도 기록 등, 페이지를 넘길 때마다 발생)에도 모든 watch 가 재발화해 지난 스프레드의 노트가 화면에 되살아났습니다. 타이밍 의존이라 노출이 기기·보기 모드에 따라 갈렸고(양면 보기에서만 보고), 수정은 두 겹입니다 — mixin 의 세대 토큰(위 ✅)과, StickyNoteBloc._loadPageNotes 가 현재 스프레드 밖 페이지의 데이터·항목을 병합하지 않게 한 방어층.

진단 — 좀비 구독은 흔적을 남기게 만들어야 보입니다.

⚠️ 핫 리로드로는 보이지 않습니다 — 이미 열린 스트림의 콜백은 옛 클로저라 새로 심은 로그 코드를 타지 않아 "로그 없는 emit" 으로 보입니다. 계측 후 핫 리스타트로 다시 재현해야 좀비 구독이 드러납니다.

5. Testing#

Test Coverage#

Test Pattern#

// ✅ Correct test pattern (AAA)
test('should return user when repository succeeds', () async {
  // Arrange
  when(() => mockRepository.getUser(any()))
    .thenAnswer((_) async => Right(testUser));

  // Act
  final result = await useCase(GetUserParams(id: 1));

  // Assert
  expect(result, Right(testUser));
  verify(() => mockRepository.getUser(1)).called(1);
});

Checklist#

• UseCase unit tests
• Repository tests (mocked)
• BLoC tests
• Meaningful test cases
• Edge cases covered
• Arrange-Act-Assert pattern
• External dependencies isolated

6. Readability#

Naming Convention#

Code Structure#

// ✅ Single Responsibility Principle
class UserRepository implements IUserRepository {
  // User-related logic only
}

// ❌ Multiple responsibilities mixed
class UserRepository {
  void getUser() {}
  void sendEmail() {}  // Email should be a separate service
  void generateReport() {}  // Report should be separate too
}

Checklist#

• Clear and meaningful names
• Project convention followed
• Abbreviation usage minimized
• Appropriate file/class size
• Single Responsibility Principle
• Duplicate code removed
• No unnecessary comments
• Explanatory comments on complex logic

7. Internationalization (i18n)#

Translation Key Usage#

// ✅ Correct usage
Text(context.t.common.save)
Text(context.t.user.greeting(name: user.name))

// ❌ Hardcoded
Text('Save')
Text('Hello, ${user.name}!')

Checklist#

• All UI text uses translation keys
• context.t.* pattern used
• Proper pluralization
• Dynamic values parameterized
• RTL support (if needed)

8. Accessibility#

Semantics Applied#

// ✅ Correct application
Semantics(
  label: 'Add to cart',
  button: true,
  child: IconButton(
    icon: Icon(Icons.add_shopping_cart),
    onPressed: addToCart,
  ),
)

// ❌ No Semantics
IconButton(
  icon: Icon(Icons.add_shopping_cart),
  onPressed: addToCart,
)

Checklist#

• Appropriate semantic labels
• Screen reader support
• Minimum 48x48 touch target
• WCAG color contrast criteria met
• Information not conveyed by color alone

9. Structural Failure Modes#

Categories 1–8 ask "is this code correct?". This one asks a different question: "could a future change make this incorrect without anyone noticing?"

The pattern behind most repeat bugs is that a safety rule exists as a convention rather than as a type. Conventions have three failure modes, and all three are invisible to dart analyze:

1. Omission compiles. Forgetting the authorization call, the transformer:, or the cache-invalidation wiring produces working-looking code.
2. Sibling drift. A correct call site sits right next to the broken one — so review misses it, because the reviewer sees the surrounding code and assumes consistency.
3. Audits repeat. Because neither the compiler nor review catches it, the only remaining tool is a codebase-wide sweep — and the same sweep runs again months later.

Server frameworks that auto-expose public methods are the sharpest case — a new method becomes a public RPC the moment it is written, and the guard is a separate optional statement inside the body.

❌ WRONG — the guard is an optional statement; deleting it still compiles and still ships:

Future<List<Member>> listMembers(Session session) async {
  await requireAdmin(session);   // ← delete this line: compiles, deploys, leaks
  return MemberService.list(session);
}

✅ Right — the guard produces a value the handler cannot proceed without:

// AdminSession's constructor is private; requireAdmin() is the only producer.
Future<List<Member>> listMembers(Session session) async {
  final admin = await requireAdmin(session);
  return MemberService.list(admin);   // omitting the guard = compile error
}

Real case: kobic Epic #7415 / #8510 — 31 authorization fix commits over six weeks, including two confirmed PII exposures. The closing commit is titled "권한 가드 일괄 감사 마무리" (bulk audit finish), i.e. the sweep ran for the whole epic. A contributing factor was that four enforcement mechanisms coexisted (declarative scopes, mixins, explicit helper calls, manual null checks), which made "no guard at all" visually indistinguishable from the other three during review.

When a domain concept is an int, a String, or a bare optional parameter, every call site re-implements it — and one of them eventually re-implements it wrong.

❌ WRONG — three different identifier spaces, all int, freely assignable:

final uid = int.parse(session.userIdentifier);   // auth user-info id space
if (participant.userId == uid) { ... }           // app user id space — different sequence

✅ Right — zero-cost wrappers make the mix-up a compile error:

extension type AppUserId(int value) {}
extension type AuthUserInfoId(int value) {}
extension type IdpUserUuid(String value) {}

final AppUserId uid = await getCurrentAppUserId(session);
if (participant.userId == uid) { ... }   // only AppUserId is assignable here

Real case: kobic #7741 — an ownership check compared UserInfo.id against ChatParticipant.userId. Because the two sequences are independent, any logged-in user whose UserInfo.id happened to equal a participant row's User.id bypassed the check entirely. Six more bugs in the same family (#8931 FormatException on a UUID identifier, #8602 authUserId overwritten with NULL, #9076) share the root cause.

The paired symptom is the review tell: fix PRs in this family almost always contain a sentence like "the sibling call site already does this correctly — only this one was missing" (kobic #9077, #7738, #7331, #8158, #9152). If the correct example is right next to the bug, review will not catch the next one — only a type will.

The same drift arrives from outside when a dependency changes a default. Auditing only the reported call site leaves the rest of the family broken:

Real case: kobic #9162 — a file-picker library flipped allowMultiple from false to true in a minor release. The fix patched the one reported call site and its PR body noted "3 more remain"; a later count found 17 of 18 production call sites still unguarded. When an upgrade changes a default, the unit of work is the call-site family, not the reported instance.

❌ WRONG — a second copy that can disagree with the real source:

final selectedIndex = useState<int>(0);   // sidebar highlight
// ... navigationShell decides the actual visible branch elsewhere

✅ Right — derive it:

final selectedIndex = resolveUIIndexForBranch(navigationShell.currentIndex);

Real case: kobic #9092 — the sidebar highlight and the visible screen disagreed. Notably, the same defect had been "fixed" once before (#6925); that fix corrected the symptom on one entry path while the duplicated state remained, and it resurfaced through a different path. Five sibling instances: #7749, #9154, #9027, #9084, #9075.

Scoping a fix honestly is good practice. Leaving the remainder as prose is not — prose does not fail a build, and the remainder returns as a new bug report.

Real case: kobic UB-155 took four PRs (#7392 → #8260 → #8273 → #8351) because each one fixed the reproduced path and deferred the rest. PR #8275 states it plainly: "the PDF path was resolved in #7392/#8260, but the EPUB fixed-layout path was explicitly out of scope in both PRs and remained." The same shape repeats in class_chat ownership (4 PRs), page titles (3 PRs), ECR auth (2 PRs).

For exhaustiveness claims specifically: prefer pasting the command and its output. A claim of "grep confirmed 0 remaining" that is not reproducible in the PR body cannot be re-verified by the next reviewer, and stale claims are worse than none.

Bugs whose defining property is "works locally, fails only on a clean checkout or in production" — nothing in the local loop can observe them.

Real case: kobic #6945 — three generated model files were missed by a force-add, so the backend built locally (files existed on disk) but 404'd on a fresh Docker checkout. #7330 — a Step Functions field present in the repo was absent in the deployed state, causing infinite loading on large books. #8214 — SPF/DMARC records were never in Terraform at all; the existing DKIM records had been created by hand in the console.

Checklist#

• New public endpoint method: reaches an authorization guard by some mechanism, and the mechanism is the same one its siblings use
• New on<Event>() whose handler awaits: transformer: chosen deliberately (default concurrent() is rarely right for network events) — and if that handler opens a subscription, transformer: is not the remedy: restartable() gates emissions only, so an await cancel() → listen path still needs a generation token (§4 재구독 겹침)
• New repository mutation: wired to cache invalidation
• New identifier / index / unit-bearing value: has its own type rather than bare int/String
• New optional parameter on an existing call: checked whether sibling call sites need it too, with the search command recorded
• Dependency upgrade that changes a default or signature: all call sites of the affected API audited, not just the reported one
• Raw API that has a project wrapper (safe menu/avatar/cache helpers): used the wrapper, not the raw form
• New local widget state: not derivable from an existing source of truth
• Anything deferred as "out of scope": exists as an issue or @Skip('#nnn') test
• New generated/platform file the build needs: committed, and verified from a clean clone
• Exhaustiveness claims ("no other occurrences") include the command and its output

Automation Tools#

Static Analysis#

# Full analysis
melos run analyze

# Combined lint check (format-check → dart analyze)
melos run lint:check

# Formatting
dart format .

Testing#

# Full test suite
melos run test

# Coverage report
melos run test:with-html-coverage