review-changes

name	review-changes
description	[Review & Quality] Review all uncommitted changes before commit

Code Review: Uncommitted Changes

Perform a comprehensive code review of all uncommitted git changes following EasyPlatform standards.

Summary

Goal: Review all uncommitted changes via a report-driven four-phase process before commit.

Phase	Action	Key Notes
0	Collect changes & create report	git status, git diff HEAD, create plans/reports/code-review-*.md
1	File-by-file review	Read each diff, update report with summary/purpose/issues
2	Holistic review	Re-read accumulated report for architecture coherence
3	Finalize findings	Critical issues, recommendations, suggestions, commit message

Key Principles:

• Be skeptical. Critical thinking. Everything needs traced proof. — Never accept code at face value; verify claims against actual behavior, trace data flow end-to-end, and demand evidence (file:line references, grep results, runtime confirmation) for every finding
• Ensure code quality: no flaws, no bugs — Verify correctness of logic, data flow, edge cases, and error handling. Flag anything that could fail at runtime
• Clean code and DRY — No duplication, clear naming, single responsibility, early returns. Code should be self-documenting
• Follow existing conventions — Match project patterns, naming style, file organization, and architectural decisions already established in the codebase. Grep for similar implementations before flagging deviations
• Docs must match code — If changes affect behavior, APIs, or features, verify related docs are updated: feature docs (docs/business-features/), test specs (docs/test-specs/), CHANGELOG, README, architecture docs, and inline code comments. Flag any doc that describes old behavior
• Build report incrementally — update after EACH file, not at the end
• Check logic placement in lowest layer (Entity > Service > Component)
• Always suggest conventional commit message based on changes

Review Approach (Report-Driven Four-Phase - CRITICAL)

⛔ MANDATORY FIRST: Create Todo Tasks for Review Phases Before starting, call TodoWrite with:

• [Review Phase 0] Get git changes and create report file - in_progress
• [Review Phase 1] Review file-by-file and update report - pending
• [Review Phase 2] Re-read report for holistic assessment - pending
• [Review Phase 3] Generate final review findings - pending

Update todo status as each phase completes. This ensures review is tracked.

---

Phase 0: Get Changes & Create Report

0.1 Get Change Summary

# See all changed files
git status

# See actual changes (staged and unstaged)
git diff HEAD

0.2 Create Report File

• Create plans/reports/code-review-{date}-{slug}.md
• Initialize with Scope (list of changed files), Change Type (feature/bugfix/refactor)

---

Phase 1: File-by-File Review (Build Report Incrementally)

For EACH changed file, read the diff and immediately update report with:

• File path and change type (added/modified/deleted)
• Change Summary: what was modified/added/deleted
• Purpose: why this change exists (infer from context)
• Issues Found: naming, typing, responsibility, patterns
• Continue to next file, repeat

Review Checklist Per File

Architecture Compliance

• Follows Clean Architecture layers (Domain, Application, Persistence, Service)
• Uses correct repository pattern (I{Service}RootRepository)
• CQRS pattern: Command/Query + Handler + Result in ONE file
• No cross-service direct database access

Code Quality

• Single Responsibility Principle
• No code duplication (DRY)
• Appropriate error handling with PlatformValidationResult
• No magic numbers/strings (extract to named constants)
• Type annotations on all functions
• No implicit any types
• Early returns/guard clauses used

Naming Conventions

• Names reveal intent (WHAT not HOW)
• Specific names, not generic (employeeRecords not data)
• Methods: Verb + Noun (getEmployee, validateInput)
• Booleans: is/has/can/should prefix (isActive, hasPermission)
• No cryptic abbreviations (employeeCount not empCnt)

Platform Patterns

• Uses platform validation fluent API (.And(), .AndAsync())
• No direct side effects in command handlers (use entity events)
• DTO mapping in DTO classes, not handlers
• Static expressions for entity queries

Security

• No hardcoded credentials or secrets
• Proper authorization checks
• Input validation at boundaries
• No SQL injection risks

Performance

• No O(n²) complexity (use dictionary for lookups)
• No N+1 query patterns (batch load related entities)
• Project only needed properties (don't load all then select one)
• Pagination for all list queries (never get all without paging)
• Parallel queries for independent operations
• Appropriate use of async/await
• Entity query expressions have database indexes configured
• MongoDB collections have Ensure*IndexesAsync() methods
• EF Core migrations include indexes for WHERE clause columns

Backend-Specific Checks

• CQRS patterns followed correctly
• Repository usage (no direct DbContext access)
• Entity DTO mapping patterns
• Validation using PlatformValidationResult

Frontend-Specific Checks

• Component base class inheritance correct (AppBase*)
• State management via PlatformVmStore
• Memory leaks (missing .pipe(this.untilDestroyed()))
• Template binding issues
• BEM classes on ALL template elements

Common Anti-Patterns to Flag

• Unused imports or variables
• Console.log/Debug.WriteLine left in code
• Hardcoded values that should be configuration
• Missing async/await keywords
• Incorrect exception handling
• Missing validation

Test-Specific Checks (When Diff Includes Test Files)

Apply these checks ONLY to files matching *.Tests.*/*.cs or e2e/tests/**/*.spec.ts:

Assertion Quality

• Every mutation test (create/update/delete) asserts at least one domain field, not just HTTP status
• Preferred: follow-up query verification after mutations (proves DB round-trip)
• Validation error tests parse response body and inspect error content (not just IsSuccessStatusCode.Should().BeFalse())
• Setup steps (arrange phase HTTP calls) have status assertion with descriptive because string
• Domain boolean flags verified where applicable (wasCreated, wasSoftDeleted, wasRestored)
• E2E update tests include post-mutation field re-read (not just waitForLoading())

Data Verification

• After create: asserts id not null + at least 1 domain field matches input
• After update: asserts same id retained + at least 1 changed field
• After delete: asserts domain flag or absence from list
• Search tests: verify matched item contains search term (not just count > 0)

Test Structure

• TC-ID annotation present ([Trait("TestCase", "...")] or TC-ID in test title)
• No hardcoded test data (uses TestDataHelper / createTestSnippet())
• Cleanup tracked in currentTestData (E2E) or UUID-isolated (integration)
• because strings on FluentAssertions calls (C#) are descriptive and unique

Anti-Patterns to Flag in Tests

• Status-only assertions: response.StatusCode.Should().Be(OK) with no body check or follow-up query
• Missing intermediate assertions: setup calls without status verification
• E2E tests ending at waitForLoading() with no field verification
• Validation tests checking only IsSuccessStatusCode.Should().BeFalse()
• Identical because strings across multiple assertions

---

Phase 2: Holistic Review (Review the Accumulated Report)

After ALL files reviewed, re-read the report to see big picture:

• Overall technical approach makes sense?
• Solution architecture coherent as unified plan?
• New files in correct layers (Domain/Application/Presentation)?
• Logic in LOWEST appropriate layer?
• Backend: mapping in Command/DTO (not Handler)?
• Frontend: constants/columns in Model (not Component)?
• No duplicated logic across changes?
• Service boundaries respected?
• No circular dependencies?
• Test assertion density adequate? (each test verifies meaningful domain state, not just HTTP status)
• No "placeholder" tests that only check HTTP status and defer real assertions?
• Assertion patterns consistent across test files in the same feature area?
• Related documentation updated? (feature docs, test specs, CHANGELOG, README, architecture docs)
• No docs describe old/removed behavior that conflicts with current changes?
• New APIs, entities, or features have corresponding doc entries?

---

Phase 3: Generate Final Review Result

Update report with final sections:

Output Format

Summary: Brief overall assessment of the changes

Critical Issues: (Must fix before commit)

• Issue 1: Description and suggested fix
• Issue 2: Description and suggested fix

High Priority: (Should fix)

• Issue 1: Description
• Issue 2: Description

Suggestions: (Nice to have)

• Suggestion 1
• Suggestion 2

Positive Notes:

• What was done well

Architecture Recommendations: (If applicable)

• Recommendation 1

Suggested Commit Message: Based on changes (conventional commit format)

<type>(<scope>): <description>

<body - what and why>

IMPORTANT Task Planning Notes

• Always plan and break many small todo tasks
• Always add a final review todo task to review the works done at the end to find any fix or enhancement needed