eryao/.trellis/spec/frontend/quality-guidelines.md

# Quality Guidelines

> Code quality standards for Flutter app development.

---

## Overview

This app enforces quality through:

- **Linting**: Flutter/Dart analysis
- **Architecture**: Feature-first with clear boundaries
- **Testing**: Widget tests, unit tests, integration tests
- **Code review**: Checklist-based reviews

---

## Forbidden Patterns

### ❌ Hardcoded Colors

```dart
// WRONG: Hardcoded hex color
Container(
  color: Color(0xFF2196F3),
)

// WRONG: Using Colors.*
Container(
  color: Colors.blue,
)
```

**Right: Use semantic colors:**

```dart
// Semantic colors from theme
Container(
  color: Theme.of(context).colorScheme.primary,
)

// Brand palette colors
Container(
  color: Theme.of(context).extension<AppColorPalette>()!.brandPrimary,
)
```

### ❌ Hardcoded Spacing

```dart
// WRONG: Magic numbers
Padding(
  padding: EdgeInsets.all(16.0),
)

// WRONG: Hardcoded values
SizedBox(height: 24.0)
```

**Right: Use `AppSpacing` / `AppRadius`:**

```dart
import 'shared/theme/design_tokens.dart';

Padding(
  padding: AppSpacing.allMedium,
)

SizedBox(height: AppSpacing.large)
```

### ❌ Feature Data Import from Other Features

```dart
// WRONG: Direct import from another feature
import 'package:app/features/auth/data/repositories/auth_repository.dart';
```

**Right: Access via app-level facade or DI:**

```dart
final authRepository = ServiceLocator.authRepository;
```

### ❌ Creating Per-Widget State Instances

```dart
// WRONG: New instance per build
class MyWidget extends StatelessWidget {
  final authBloc = AuthBloc(); // New instance every time
}
```

**Right: Use singleton from DI:**

```dart
class MyWidget extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    final authBloc = ServiceLocator.authBloc; // Singleton
  }
}
```

### ❌ Place Feature Repositories in Shared Data Layer

```dart
// WRONG: Feature repository in shared data/
// data/repositories/auth_repository.dart
class AuthRepositoryImpl implements AuthRepository {}
```

**Right: Keep in feature's data layer:**

```dart
// features/auth/data/repositories/auth_repository.dart
class AuthRepositoryImpl implements AuthRepository {}
```

### ❌ New Second-Level Directories Under `lib/`

```dart
// WRONG: Ad-hoc directories
lib/
  utils/
  helpers/
  constants/
```

**Right: Use allowed directories only:**

```dart
lib/
  app/      # Bootstrap, DI, router
  core/     # Cross-feature infrastructure
  data/     # Shared infrastructure
  features/ # Feature modules
  shared/   # Reusable widgets
  l10n/     # Localization
```

---

## Required Patterns

### ✅ Semantic Color System

```dart
import 'package:flutter/material.dart';

// Use semantic colors
ThemeData lightTheme = ThemeData(
  colorScheme: ColorScheme.light(
    primary: Color(0xFF2196F3),
    secondary: Color(0xFF03DAC6),
    surface: Color(0xFFFFFFFF),
    error: Color(0xFFB00020),
  ),
);

// Access via theme
Container(
  color: Theme.of(context).colorScheme.primary,
)
```

### ✅ Design Tokens for Spacing

```dart
// shared/theme/design_tokens.dart
class AppSpacing {
  static const double xsmall = 4.0;
  static const double small = 8.0;
  static const double medium = 16.0;
  static const double large = 24.0;
  static const double xlarge = 32.0;
  
  static EdgeInsets get allMedium => EdgeInsets.all(medium);
  static EdgeInsets get horizontalMedium => EdgeInsets.symmetric(horizontal: medium);
}

class AppRadius {
  static const double small = 4.0;
  static const double medium = 8.0;
  static const double large = 12.0;
}
```

### ✅ Repository Pattern with DI

```dart
// app/di/di.dart
class ServiceLocator {
  static late AuthRepository authRepository;
  static late AuthBloc authBloc;

  static void setup() {
    authRepository = AuthRepositoryImpl(
      api: AuthApiImpl(),
      sessionStore: SessionStore(),
    );
    
    authBloc = AuthBloc(repository: authRepository);
  }
}
```

### ✅ Immutable State with copyWith

```dart
class AuthState {
  const AuthState({
    required this.status,
    this.user,
    this.errorMessage,
  });

  final AuthStatus status;
  final AuthUser? user;
  final String? errorMessage;

  AuthState copyWith({
    AuthStatus? status,
    AuthUser? user,
    String? errorMessage,
  }) {
    return AuthState(
      status: status ?? this.status,
      user: user ?? this.user,
      errorMessage: errorMessage ?? this.errorMessage,
    );
  }
}
```

### ✅ Error Logging in Try-Catch

```dart
try {
  await _repository.operation();
} catch (e, stackTrace) {
  _logger.error(
    message: 'Operation failed',
    error: e,
    stackTrace: stackTrace,
  );
  rethrow;
}
```

### ✅ Logger Module Naming

```dart
class AuthBloc extends ChangeNotifier {
  final Logger _logger = getLogger('features.auth.bloc');
}

class HomeRepository {
  final Logger _logger = getLogger('features.home.repository');
}

class HttpClient {
  final Logger _logger = getLogger('core.network.http_client');
}
```

---

## Testing Requirements

### Test Organization

```
apps/test/
├── unit/              # Unit tests
├── widgets/           # Widget tests
├── integration/       # Integration tests
└── test_utils/        # Test utilities
```

### Test Requirements from AGENTS.md

1. **Prioritize tests for**: model parsing, service logic, high-regression flows
2. **Auth/Home/Cache changes**: must include targeted regression tests
3. **Simple static UI changes**: may skip tests
4. **Test credentials**: use environment config, never hardcode

### Unit Testing

```dart
test('AuthBloc login success', () async {
  final mockRepo = MockAuthRepository();
  final bloc = AuthBloc(repository: mockRepo);
  
  when(mockRepo.loginWithEmailOtp(
    email: 'test@example.com',
    otp: '123456',
  )).thenAnswer((_) async => AuthUser(id: '123', email: 'test@example.com'));
  
  await bloc.loginWithOtp(email: 'test@example.com', otp: '123456');
  
  expect(bloc.state.status, AuthStatus.authenticated);
  expect(bloc.state.user?.id, '123');
});
```

### Widget Testing

```dart
testWidgets('Login screen shows error on failed login', (tester) async {
  await tester.pumpWidget(MyApp());
  
  await tester.enterText(find.byKey(Key('email_field')), 'test@example.com');
  await tester.enterText(find.byKey(Key('otp_field')), 'wrong');
  await tester.tap(find.byKey(Key('login_button')));
  
  await tester.pumpAndSettle();
  
  expect(find.text('Request failed, please try again'), findsOneWidget);
});
```

### Integration Testing

```dart
testWidgets('User can login with OTP', (tester) async {
  await tester.pumpWidget(MyApp());
  
  // Navigate to login
  await tester.tap(find.text('Login'));
  await tester.pumpAndSettle();
  
  // Enter credentials
  await tester.enterText(find.byKey(Key('email_field')), 'user@example.com');
  await tester.tap(find.byKey(Key('send_otp_button')));
  await tester.pumpAndSettle();
  
  // Enter OTP
  await tester.enterText(find.byKey(Key('otp_field')), '123456');
  await tester.tap(find.byKey(Key('login_button')));
  await tester.pumpAndSettle();
  
  // Verify success
  expect(find.text('Welcome'), findsOneWidget);
});
```

---

## Code Review Checklist

### Architecture

- [ ] Follows `features/<feature>/data/` and `features/<feature>/presentation/` structure
- [ ] Shared infrastructure in `data/` (not feature repositories/models)
- [ ] Reusable widgets in `shared/widgets/` (not feature-specific)
- [ ] Cross-feature code in `core/`
- [ ] No new second-level directories under `lib/`

### State Management

- [ ] Uses `ChangeNotifier` + immutable state
- [ ] State classes have `copyWith` method
- [ ] Singleton blocs via DI (not per-widget instances)
- [ ] Errors are logged before state changes

### UI

- [ ] Uses semantic colors from `Theme.of(context).colorScheme`
- [ ] Uses `AppSpacing` / `AppRadius` (no hardcoded values)
- [ ] Follows design system tokens
- [ ] Toast/Banner for user feedback (no `print()`)

### Data

- [ ] Repository pattern for data access
- [ ] `ApiProblem` for error handling
- [ ] Error codes mapped to l10n keys
- [ ] 401 handled via global callback

### Logging

- [ ] Logger initialized with module path (`features.<feature>.<component>`)
- [ ] All exceptions logged with `error`, `stackTrace`, and `message`
- [ ] No PII/tokens/passwords in logs
- [ ] Info logs only for milestones (login, logout, run completed)

### Testing

- [ ] Unit tests for bloc logic
- [ ] Widget tests for UI
- [ ] Integration tests for critical flows
- [ ] Mocks use `when/thenAnswer/thenReturn`

---

## High-Risk Modules Checklist

From AGENTS.md, these modules require extra attention:

### Auth

- [ ] `AuthBloc` is single source of truth
- [ ] 401 invalidation goes through global callback
- [ ] No feature-level token clearing or direct login navigation

### Home Message Viewport

- [ ] Auto-scroll/anchor restore is event-driven
- [ ] Viewport preserved during history prepend
- [ ] User reading position maintained

### Cache / Repository

- [ ] Reads/writes go through repository layer
- [ ] Cache keys/invalidation in repository (not UI/Bloc)
- [ ] Feature-scoped TTL policy defined in repository
- [ ] No per-screen/per-widget cache store instances
- [ ] Cross-feature access via app-level facade

---

## Common Mistakes

### ❌ Mutable State

```dart
// WRONG: Direct mutation
class AuthBloc extends ChangeNotifier {
  AuthUser? user; // Mutable
  
  void login() {
    user = fetchUser(); // Direct mutation
    notifyListeners();
  }
}
```

**Right: Immutable state:**

```dart
class AuthBloc extends ChangeNotifier {
  AuthState _state = AuthState.initial;
  
  AuthState get state => _state;
  
  void login() async {
    final user = await fetchUser();
    _state = _state.copyWith(user: user, status: AuthStatus.authenticated);
    notifyListeners();
  }
}
```

### ❌ Missing Error Logging

```dart
// WRONG: No logging
try {
  await operation();
} catch (e) {
  state = ErrorState();
}
```

**Right: Log before state change:**

```dart
try {
  await operation();
} catch (e, stackTrace) {
  _logger.error(message: 'Operation failed', error: e, stackTrace: stackTrace);
  state = ErrorState();
}
```

### ❌ Hardcoded Strings

```dart
// WRONG: User-facing string
Text('Login Failed')
```

**Right: Use l10n:**

```dart
Text(AppLocalizations.of(context)!.loginFailed)
```

### ❌ Bypassing Design System

```dart
// WRONG: Magic numbers
Padding(
  padding: EdgeInsets.fromLTRB(16, 8, 16, 8),
)
```

**Right: Use tokens:**

```dart
Padding(
  padding: EdgeInsets.symmetric(
    horizontal: AppSpacing.medium,
    vertical: AppSpacing.small,
  ),
)
```

---

## Flutter-Specific Quality

### Widget Best Practices

- Use `const` constructors for immutable widgets
- Prefer `StatelessWidget` over `StatefulWidget` when possible
- Use `const` when creating widgets in build methods
- Avoid `print()` - use Logger instead

### Performance

- Use `ListView.builder` for long lists
- Avoid rebuilding expensive widgets unnecessarily
- Use `const` widgets to prevent rebuilds
- Use `RepaintBoundary` for complex animations

### Code Style

- Follow Dart style guide
- Use `flutter analyze` to catch issues
- Run `dart format .` before commits
- Follow naming conventions: `lowerCamelCase` for variables, `UpperCamelCase` for types