메뉴
Pull Request (PR) Review skill for reviewing code changes following Flutter/Dart best practices, Clean Architecture, and Security standards.
기본적으로 소스를 먼저 확인하는 Prompt가 선택됩니다. 직접 명령으로 전환하거나 로컬 사본을 다운로드할 수도 있습니다.
설치 여부를 결정하기 전에 SKILL.md와 SkillsMP에 표시된 보조 파일을 읽어 보세요.
Codex 또는 Claude로 설치 이 Prompt를 복사해 Codex, Claude 또는 다른 어시스턴트에 붙여 넣으면 Skill 페이지를 검토하고 설치를 진행할 수 있습니다.
직접 명령은 검토 Prompt를 거치지 않습니다. 실행하기 전에 소스를 확인하세요.
npx skills add https://github.com/DanhDue/bloc_digital_wallet --skill pr-review명령은 한 줄로 유지됩니다. 복사하기 전에 가로로 스크롤해 전체 내용을 확인하세요.
로컬 사본을 원하시나요? SkillsMP에서 현재 제공할 수 있는 파일을 다운로드하세요.
Merges workspace keybindings into IDE user profile configurations (e.g., ~/antigravity-profile-2/User/keybindings.json).
Parse JSON and create freezed object classes following project conventions.
Merges workspace keybindings (.vscode/keybindings.json) into the user's global IDE configuration (VS Code, Cursor, Windsurf, Antigravity, etc.).
SKILL.md 표시 중
SOC 직업 분류 기준
| name | pr-review |
| description | Pull Request (PR) Review skill for reviewing code changes following Flutter/Dart best practices, Clean Architecture, and Security standards. |
This skill transforms the AI agent into a Principal Flutter Engineer specializing in Mobile Security and Clean Architecture to review Pull Requests for the project.
You are a Principal Flutter Engineer specializing in Mobile Security and Clean Architecture. Your task is to review the Pull Request for this project.
| Rule | Description |
|---|---|
| UpperCamelCase | Types, extensions, enums use UpperCamelCase |
| lowerCamelCase | Variables, functions, parameters, constants use lowerCamelCase |
| lowercase_with_underscores | Packages, directories, source files use snake_case |
| Import Ordering | Order: dart: → package: → relative. Sort alphabetically within sections |
| Curly Braces | Use curly braces for all flow control statements |
| Line Length | Prefer lines 80 characters or fewer |
Examples:
// ❌ BAD - Wrong casing
class user_entity {}
const MAX_RETRY = 3;
// ✅ GOOD - Correct casing
class UserEntity {}
const maxRetry = 3;
| Rule | Description |
|---|---|
| Don't Init Null | Don't explicitly initialize variables to null |
| Collection Literals | Use [], {}, <>{} instead of List(), Map(), Set() |
| isEmpty/isNotEmpty | Use .isEmpty instead of .length == 0 |
| Avoid forEach | Prefer for-in over Iterable.forEach() with function literals |
| Use whereType | Use whereType<T>() to filter by type instead of where + cast |
| Avoid cast() | Avoid using .cast(), prefer type-safe alternatives |
| Tear-offs | Use list.map(toUpper) instead of list.map((s) => toUpper(s)) |
| Final Fields | Prefer final for read-only properties |
| Avoid Color.withValues | Use withOpacity() instead of withValues() for SDK < 3.27.0 compatibility |
Examples:
// ❌ BAD
String? name = null;
if (list.length == 0) {}
items.forEach((item) { process(item); });
// ✅ GOOD
String? name;
if (list.isEmpty) {}
if (list.isEmpty) {}
for (final item in items) { process(item); }
// ❌ BAD - Requires Flutter 3.27.0+
color.withValues(alpha: 0.5);
// ✅ GOOD - Compatible with Flutter 3.10.7+
color.withOpacity(0.5);
| Rule | Description |
|---|---|
| Avoid Abbreviations | Use buttonText instead of btnTxt |
| Descriptive Noun Last | Use pageCount not countOfPages |
| Boolean Names | Use non-imperative verbs: isEnabled, hasValue, canClose |
| Positive Names | Use isVisible instead of isHidden (prefer positive) |
| Avoid get Prefix | Use user property instead of getUser() method |
| to___ / as___ | Use toJson() for copies, asList() for views |
| Rule | Description |
|---|---|
| Intention-Revealing Names | Variable/function names must answer: Why it exists, what it does, and how it is used |
| BLoC Event Semantics | Events must use past tense or intent-based nouns (e.g., TopUpSubmitted, PinChanged) |
| BLoC State Semantics | States must describe the current UI status (e.g., BalanceLoading, PaymentSuccess) |
| Searchable Names | Avoid "Magic Numbers" or "Magic Strings". Use named constants (e.g., maxPinAttempts instead of 3) |
Examples:
// ❌ BAD
var d; // days elapsed
// ✅ GOOD
var daysSinceLastTransaction;
[!WARNING] Watch for these common code smells that indicate deeper problems.
| Smell | Description | Fix |
|---|---|---|
| Long Method | Function > 20 lines | Extract smaller functions |
| Large Class | Class doing too much | Split into focused classes |
| Primitive Obsession | Using primitives instead of small objects | Create value objects (e.g., Money, Email) |
| Long Parameter List | > 3 parameters | Use parameter object or builder |
| Data Clumps | Same group of data appearing together | Extract into a class |
| Smell | Description | Fix |
|---|---|---|
| Switch Statements | Complex switch/if-else chains | Use polymorphism or strategy pattern |
| Temporary Field | Fields only used in certain situations | Extract class or use null object |
| Refused Bequest | Subclass doesn't use inherited methods | Replace inheritance with delegation |
| Smell | Description | Fix |
|---|---|---|
| Divergent Change | One class changed for different reasons | Split by responsibility |
| Shotgun Surgery | One change requires editing many classes | Move related code together |
| Smell | Description | Fix |
|---|---|---|
| Dead Code | Unreachable or unused code | Delete it |
| Duplicate Code | Same code in multiple places | Extract method/class |
| Lazy Class | Class that does too little | Inline or merge |
| Speculative Generality | Unused abstractions "for future" | Remove until needed |
| Smell | Description | Fix |
|---|---|---|
| Feature Envy | Method uses another class's data more than its own | Move method to that class |
| Message Chains | a.b().c().d() chains | Hide delegation, Law of Demeter |
| Middle Man | Class delegates everything | Remove or inline |
[!TIP] These rules are derived from the official Flutter AI Rules to ensure modern, performant, and maintainable code.
| Rule | Description |
|---|---|
| Premium Feel | Apply subtle noise texture to backgrounds; use multi-layered drop shadows for depth. |
| Typography | Use font sizes and weights (hero text, section headlines) to guide understanding. |
| Interactive Glow | Interactive elements (buttons, sliders) should have shadows/colors that create a "glow" effect. |
| Rule | Description |
|---|---|
| Const Constructors | Use const variables and constructors extensively to reduce widget rebuilds. |
| List Performance | Always use ListView.builder or SliverList for long/lazy-loaded lists. |
| Isolates | Use compute() for expensive calculations (e.g., JSON parsing) to avoid blocking the UI. |
| Build Method | Keep build() pure and fast. Move complex logic or network calls out of build(). |
| Callback Optimization | Move expensive operations (FFI, I/O, parsing) outside callbacks that execute frequently. |
// ❌ BAD - FFI call on every callback invocation
client.callback = () {
fingerprints = loadFromFFI(); // Called every time!
validate(fingerprints);
};
// ✅ GOOD - Load once, capture in closure
fingerprints = loadFromFFI();
client.callback = () {
validate(fingerprints); // Uses captured value
};
| Rule | Description |
|---|---|
| Composition | Favor composition over inheritance. Build complex UIs from smaller, private Widget classes (not helper methods). |
| Immutability | Widgets (especially StatelessWidget) should be immutable. |
| State Management | Separate ephemeral state (UI) from app state (Business Logic). Use Bloc/Cubit for app state as per project standard. |
| Testing | Prefer package:checks for expressive assertions if applicable. Write code with testing in mind. |
| Rule | Requirement |
|---|---|
| Small & Single Responsibility (SRP) | Max 20 lines per function. If it has "And", split it |
| Monadic/Dyadic Arguments | Max 2 arguments. If 3+, wrap into a Data Class/Entity |
| Command Query Separation (CQS) | A function should either perform an action (Command) OR return data (Query), never both |
| Rule | Description |
|---|---|
| Law of Demeter | Modules should not know the inner details of objects they manipulate |
| Layer Purity (Domain) | Strictly NO imports from package:flutter/material.dart or external SDKs |
| Layer Purity (Data) | Must map DTOs (JSON) to Domain Entities before returning to Repository |
| Tell, Don't Ask | Don't pull data out to process it. Tell the object to perform its own logic |
Examples:
// ❌ BAD - Violates Law of Demeter
user.wallet.balance.currency.symbol
// ✅ GOOD
user.getCurrencySymbol()
| Rule | Description |
|---|---|
| Strict Immutability | All states must be final. Use @freezed or Equatable. Never mutate; always copyWith |
| Functional Error Handling | Use Either<Failure, Success> for UseCases. Force the caller to handle failures |
| Side Effect Isolation | Use BlocListener for navigation/dialogs. Keep BlocBuilder pure for UI rendering |
| Rule | Description |
|---|---|
| Don't Return/Pass Null | Return empty collections [] or Null Objects instead of null |
| Contextual Exceptions | Throw domain-specific exceptions (e.g., InsufficientFundsException) over generic errors |
| Type Safety on Dynamic Data | Always verify type before casting from Map<String, dynamic> or JSON data |
| Guard Before API Calls | Check for null/empty values before making network requests |
| Guard Widget Generation | Don't generate widgets (e.g., QR codes) with invalid/empty data |
Examples:
// ❌ BAD - No type check, can throw runtime error
if (data.containsKey('message')) {
message = data['message']; // Fails if value is int, null, etc.
}
// ✅ GOOD - Type-safe access
if (data['message'] is String) {
message = data['message'] as String;
}
// ❌ BAD - Empty string fallback may cause invalid API calls
final result = await useCase(entity.address ?? '');
// ✅ GOOD - Guard before API call
final address = entity.address;
if (address == null || address.isEmpty) {
return entity; // Skip processing for invalid data
}
final result = await useCase(address);
// ❌ BAD - Generates useless widget with empty data
final qrCode = QrCode.fromData(data: address ?? '');
// ✅ GOOD - Guard widget generation, update on changes
QrImage? _qrImage;
void _generateQrImage() {
if (address == null || address.isEmpty) {
_qrImage = null;
return;
}
_qrImage = QrImage(QrCode.fromData(data: address));
}
[!CAUTION] CRITICAL: These are non-negotiable security requirements based on OWASP Mobile Top 10.
| Risk | Description | Verification |
|---|---|---|
| M1: Improper Credential Usage | Hardcoded secrets, API keys in code | No secrets in source code; use env vars or secure storage |
| M2: Inadequate Supply Chain Security | Vulnerable dependencies | Run flutter pub outdated; audit third-party packages |
| M3: Insecure Authentication/Authorization | Weak authentication flows | Use proper token management; validate on server-side |
| M4: Insufficient Input/Output Validation | Injection, XSS, path traversal | Sanitize all user inputs; validate before processing |
| M5: Insecure Communication | HTTP, no cert pinning | HTTPS only; implement certificate pinning |
| M6: Inadequate Privacy Controls | Excessive data collection, PII exposure | No logging of PII, card numbers, wallet addresses |
| M7: Insufficient Binary Protections | Reverse engineering, tampering | Enable code obfuscation (--obfuscate) |
| M8: Security Misconfiguration | Debug mode in production, insecure defaults | Disable debug flags; review AndroidManifest/Info.plist |
| M9: Insecure Data Storage | Plaintext sensitive data | Use flutter_secure_storage for all secrets/tokens |
| M10: Insufficient Cryptography | Weak algorithms, hardcoded keys | Use platform crypto; never hardcode encryption keys |
| Risk | Description |
|---|---|
| M1: Improper Platform Usage | Misuse of platform features or security controls |
| M2: Insecure Data Storage | Unprotected data in SQLite, logs, plist, cookies |
| M3: Insecure Communication | Lack of TLS, weak handshake, cleartext traffic |
| M4: Insecure Authentication | Anonymous auth, weak passwords |
| M5: Insufficient Cryptography | Using deprecated algorithms (MD5, SHA1) |
| M6: Insecure Authorization | IDOR, missing role checks |
| M7: Client Code Quality | Buffer overflows, format strings |
| M8: Code Tampering | Binary patching, method swizzling |
| M9: Reverse Engineering | Disassembly, string table analysis |
| M10: Extraneous Functionality | Hidden backdoors, debug flags |
| Risk | Description |
|---|---|
| M1: Weak Server Side Controls | Insecure API endpoints |
| M2: Insecure Data Storage | Local storage vulnerabilities |
| M3: Insufficient Transport Layer Protection | Missing/broken TLS |
| M4: Unintended Data Leakage | Logs, clipboard, cache |
| M5: Poor Authorization and Authentication | Weak identity controls |
| M6: Broken Cryptography | Hardcoded keys, weak ciphers |
| M7: Client Side Injection | SQLi, XSS in WebViews |
| M8: Security Decisions Via Untrusted Inputs | Intent/URL scheme abuse |
| M9: Improper Session Handling | Long sessions, insecure tokens |
| M10: Lack of Binary Protections | No obfuscation, debugging enabled |
Flutter-Specific Checks:
// ❌ BAD - Hardcoded API key (M1)
const apiKey = 'sk_live_abc123...';
// ✅ GOOD - From secure storage
final apiKey = await secureStorage.read(key: 'api_key');
// ❌ BAD - Debug-only configuration without guard (M8)
class DebugSslConfiguration {
const DebugSslConfiguration(); // Can be used in production!
}
// ✅ GOOD - Runtime assertion prevents production usage
class DebugSslConfiguration {
DebugSslConfiguration() {
assert(kDebugMode, 'Must only be used in debug mode.');
}
}
// ❌ BAD - Insecure default (M8)
DioFactory({SslConfiguration ssl = const NoSslPinning()});
// ✅ GOOD - Secure default with auto-selection
DioFactory({SslConfiguration ssl = const AutoSslConfiguration()});
// ❌ BAD - Logging sensitive security data (M6)
talker.debug('Fingerprints: $fingerprints');
talker.debug('Token: $authToken');
// ✅ GOOD - Log counts, not values; gate debug logs
talker.debug('Fingerprints loaded: ${fingerprints.length} entries');
assert(() {
talker.debug('Debug fingerprint: $fingerprint');
return true;
}());
| Principle | Description |
|---|---|
| Fast | Tests must run quickly |
| Independent | Tests should not depend on each other |
| Repeatable | Must pass in any environment (Local/CI) |
| Self-Validating | Clear Boolean output (Pass/Fail) |
| Timely | Write tests alongside or before code (TDD mindset) |
[!IMPORTANT] Resource Cleanup: All Blocs and Cubits MUST be closed in the
tearDown()method to prevent stream/subscription leaks.
| Rule | Requirement | Reason |
|---|---|---|
| Close in tearDown | MUST call bloc.close() or cubit.close() in tearDown() | Prevents memory leaks from unclosed streams/subscriptions |
| Setup/TearDown Pattern | Create bloc in setUp(), close in tearDown() | Ensures proper lifecycle management |
| Late Variables | Use late keyword for bloc/cubit declarations | Allows initialization in setUp() |
Example:
// ❌ BAD - Bloc never closed, causes resource leak
void main() {
late TransactionBloc bloc;
late MockGetTransactionUseCase mockUseCase;
setUp(() {
mockUseCase = MockGetTransactionUseCase();
bloc = TransactionBloc(mockUseCase);
});
// Missing tearDown - RESOURCE LEAK!
test('should emit success state', () {
// test code...
});
}
// ✅ GOOD - Bloc properly closed in tearDown
void main() {
late TransactionBloc bloc;
late MockGetTransactionUseCase mockUseCase;
setUp(() {
mockUseCase = MockGetTransactionUseCase();
bloc = TransactionBloc(mockUseCase);
});
tearDown(() {
bloc.close(); // ← REQUIRED: Close bloc to prevent leaks
});
test('should emit success state', () {
// test code...
});
}
Why This Matters:
[!IMPORTANT] Verify all freezed models follow the project's strict conventions. These are mandatory for all data classes.
| Rule | Requirement | Example |
|---|---|---|
| Abstract Class | MUST use abstract class for all freezed models | abstract class UserModel with _$UserModel |
| Private Constructor | MUST include const ClassName._(); | const UserModel._(); |
| @JsonSerializable | SHOULD use @JsonSerializable(includeIfNull: false) | On factory constructor |
| @JsonKey on ALL fields | MUST use @JsonKey(name: 'field_name') for EVERY field | Even when names match |
| Nullable Fields | PREFER nullable types (String?, int?) | For optional API fields |
| One Object, One File | MUST have each class in its own dedicated file | Never multiple classes per file |
| Import | Purpose |
|---|---|
package:flutter/foundation.dart | Required for debugFillProperties support |
package:freezed_annotation/freezed_annotation.dart | Freezed annotations |
[!NOTE] The
// coverage:ignore-filecomment is ONLY for generated files and freezed models. DO NOT add// coverage:ignore-fileto test files (files intest/directories).
// Copyright (c) 2026, one of DanhDue ExOICTIF projects. All rights reserved.
// coverage:ignore-file ← ONLY for generated/model files, NOT for test files
// ignore_for_file: invalid_annotation_target
import 'package:flutter/foundation.dart';
import 'package:freezed_annotation/freezed_annotation.dart';
part '{model_name}.freezed.dart';
part '{model_name}.g.dart';
@freezed
abstract class ModelName with _$ModelName {
const ModelName._(); // ← REQUIRED: Private constructor
@JsonSerializable(includeIfNull: false)
const factory ModelName({
@JsonKey(name: 'field_name') String? fieldName, // ← @JsonKey on ALL fields
@JsonKey(name: 'count') int? count,
}) = _ModelName;
factory ModelName.fromJson(Map<String, Object?> json) =>
_$ModelNameFromJson(json);
}
// ❌ BAD - Missing abstract, private constructor, and @JsonKey
@freezed
class UserModel with _$UserModel {
const factory UserModel({
required String id, // Missing @JsonKey
required String name,
}) = _UserModel;
}
// ✅ GOOD - Complete freezed implementation
@freezed
abstract class UserModel with _$UserModel {
const UserModel._(); // Private constructor
@JsonSerializable(includeIfNull: false)
const factory UserModel({
@JsonKey(name: 'id') required String id,
@JsonKey(name: 'name') required String name,
@JsonKey(name: 'email') String? email,
}) = _UserModel;
factory UserModel.fromJson(Map<String, Object?> json) =>
_$UserModelFromJson(json);
}
| Aspect | Verification |
|---|---|
| Freezed Models | All entities and models follow Freezed Standards (Section 13) |
| Import Convention | Always use full package paths (e.g., import 'package:bloc_digital_wallet/...') instead of relative imports |
| Boilerplate | Identify code that could be generated using Mason Bricks (mvi_feature, mvi_subfeature) |
| Testing Coverage | New logic is accompanied by Unit Tests for UseCases/BLoCs |
| One Object, One File | Each freezed class MUST have its own dedicated .dart file |
Use the @pr-review skill to review commit: abc123def
The agent will run git show <COMMIT_ID> to fetch the diff and review it.
Use the @pr-review skill to review this PR:
<PASTE PR DIFF HERE>
Paste the diff content directly into your request.
# Fetch PR diff and copy to clipboard
gh pr diff <PR_NUMBER> | pbcopy
# Then paste into your request with the skill trigger
# Review a specific commit
git show <COMMIT_ID>
# Review changes between commits
git diff <BASE_COMMIT> <HEAD_COMMIT>
# Review changes between branches
git diff main..feature-branch
Copy the output and paste into your review request.
[!IMPORTANT] MANDATORY FIRST STEP: Before reviewing any code changes, you MUST perform these quality checks in order.
Run melos genAlls to ensure all generated code is up-to-date and license headers are applied:
melos genAlls
This command will:
build_runner to generate freezed models, JSON serialization, and other code-generated filesRun fvm dart analyze to detect code quality issues, lints, and potential bugs:
fvm dart analyze
Review the output for:
Check that the code adheres to project-specific quality standards:
lib/ files use package imports (not relative imports)@freezed with @JsonKey annotationsbaseUrl parameters.infinity, .maxFinite, .zero where applicableIf changes affect core functionality, verify the build succeeds:
# For code generation (freezed, retrofit, etc.)
melos build_runner
# For full app build
flutter build apk --debug
# or
melos build_apk
[!CAUTION] If any of these checks fail, STOP and fix the issues before proceeding with the PR review. Do not review code that doesn't pass basic quality gates.
Your response MUST be structured exactly as follows:
### 📝 Summary of Changes
(A brief description of what this PR introduces)
### 🚨 Critical Issues (Architecture/Security/Logic)
- **[File Name] : [Line Number]**: [Issue Description] -> [Suggested Fix]
### 💡 Refactoring & Style (Flutter/Dart)
- [Suggestions for cleaner code, better performance, or UI optimization]
### ✅ Final Verdict
(Choose one: 🟢 LGTM / 🟡 Needs Work / 🔴 Request Changes)
### 🚨 Critical Issues (Architecture/Security/Logic)
- **wallet_bloc.dart : Line 45**: Logging wallet address value -> Remove `debugPrint('Address: $walletAddress')` or mask sensitive data
- **transfer_usecase.dart : Line 23**: UseCase imports Flutter's BuildContext -> Move context-dependent logic to Presentation layer
- **home_page.dart : Line 112**: Hardcoded color `Color(0xFF123456)` -> Use `context.appThemes.colorScheme.primary`
- **auth_repository_impl.dart : Line 67**: Token stored in SharedPreferences -> Use `FlutterSecureStorage` for sensitive credentials
- **payment_bloc.dart : Line 34**: Magic number `3` used for retry count -> Use named constant `MAX_RETRY_ATTEMPTS`
- **user_entity.dart : Line 15**: Law of Demeter violation `user.wallet.balance.amount` -> Create `user.getBalanceAmount()` method
### 💡 Refactoring & Style (Flutter/Dart)
- Consider using `const` constructor for `TransactionCard` widget at `transaction_card.dart:15` to prevent unnecessary rebuilds
- The `TransactionEntity` could be generated via freezed. Current manual implementation at `transaction_entity.dart` increases maintenance burden
- Replace `BlocBuilder` with `BlocSelector` at `home_page.dart:78` to only rebuild when specific state property changes
- Use `context.t.homeTitle` instead of hardcoded string "Home" at `home_page.dart:45`
- Function `processPaymentAndUpdateBalance()` at `payment_usecase.dart:28` violates SRP - split into `processPayment()` and `updateBalance()`
- Event name `Load` at `home_bloc.dart:12` is too generic -> Rename to `HomeDataRequested` for clarity
### ✅ Final Verdict
🟢 **LGTM** - Code follows best practices, no critical issues found.
🟡 **Needs Work** - Minor issues found that should be addressed before merging.
🔴 **Request Changes** - Critical security/architecture violations must be fixed.