Code Review Reference Guide#

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.

Fire-and-Forget Error Handling (unawaited() + Error)#

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

bloc_signals 전용 점검#

DCM 의 bloc 규칙군은 package:bloc 타입을 대상으로 하므로 bloc_signals 코드에서는 발화하지 않습니다. 자동 검사에서 빠지는 항목을 리뷰에서 봅니다 (상세: cc-dcm:dcm-bloc).

• 모든 await 뒤에 if (isClosed) return; — close() 는 진행 중 핸들러를 취소하지 않음
• 상태를 새 인스턴스로 방출 — in-place 변경 후 재방출은 자동 de-dup 에 조용히 삼켜짐
• 상태 타입이 불변이고 의미 있는 == 를 가짐
• 핸들러 내 상태 읽기는 stateValue (state 는 ReadonlySignal<S>)
• .listen 구독을 close() 오버라이드에서 cancel + await super.close() — restartable() 은 emission 만 버리고 구독을 취소하지 않음
• context.watch<T>().stateValue 없음 (상태를 구독하지 않음 → BlocSignalBuilder 사용)
• 전역/최상위 컨테이너 인스턴스 없음 (SSR·테스트 격리 위험)
• expect 리스트에 동일 상태 연속 중복 없음 (de-dup 으로 방출되지 않음)

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.

9.1 Optional-call safety (omission compiles)#

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.

9.2 Primitive obsession (sibling drift)#

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.

9.3 Derived state stored instead of computed#

❌ 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.

9.4 Incomplete fixes declared "out of scope"#

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.

9.5 State living outside the code SSOT#

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)
• 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 → dcm analyze)
melos run lint:check

# Formatting (kobic uses dcm format, not dart format)
melos run format
# Underlying command: dcm format .

Testing#

# Full test suite
melos run test

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