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-dev:swr-pattern] §16). A codebase-wide audit of unawaited( call sites found 11 confirmed reproductions of this pattern — see [cc-dev-cycle: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
• BlocBuilder buildWhen used
• Refined with BlocSelector
• Image cacheWidth/cacheHeight applied
• Parallelizable tasks parallelized
• debounce/throttle applied
• Resources released in dispose
• Stream subscriptions cancelled

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

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