code-review

# Code Review Workflow ## Overview This skill defines the canonical code review standards for Clypra. In a desktop video editor, subtle concurrency glitches, unreleased resources, and violated timeline invariants can lead to permanent render freezes, corrupted projects, or AV desynchronization. Reviews must be technical, evidence-based, and prioritized by risk. --- ## When to Use This Skill - Reviewing git diffs or pull requests before merging. - Evaluating changes generated by another AI agent. - Pre-commit verification of local working-tree modifications. ## When NOT to Use This Skill - Authoring new features or bug fixes directly (use `feature-implementation` or `bugfix-regression`). - Measuring performance benchmarks (use benchmark tools). --- ## Review Priorities (Strict Hierarchy) Reviews must assess changes in this exact order: 1. **Functional Defects & Invariants Violation** - Does the change break the single time authority (`PlaybackClock`)? - Does any render path bypass `evaluateTimelineSceneCached()` or `EvaluatedScene`? - Are timeline mutations bypassing `historyStore.dispatch(command)`? - Is `asset.path` accessed before hydration/probe completes? - In `NativeProgramPreview.tsx`, is `renderInFlight` guaranteed to reset across all exit paths? 2. **Asynchronous Races & Lifecycle Errors** - Can out-of-order IPC responses or promise resolutions overwrite fresher state? - Are Web Workers or native resources leaking on unmount/dispose? - Is cancellation handled when user scrubs or switches projects rapidly? 3. **Error Handling & Resource Leakage** - Are errors silently caught without fallback or logging? - Are GPU textures, buffers, or audio nodes disposed cleanly? 4. **Security & Data Integrity** - Does Tauri IPC path handling validate incoming file paths against traversal attacks? - Is project JSON deserialization robust against corrupted or legacy schemas? 5. **Test Coverage & Verification** - Does a bug fix include a corresponding automated regression test? - Are assertions meaningful rather than trivial existence checks? - Do all modified files compile clean under `npx tsc --noEmit`? 6. **Performance & Compute Boundaries** - Are heavy computations (>2ms) kept off the main UI thread? - Are IPC calls batched or debounced rather than called per animation frame? - Is frame batching used for export rendering? 7. **Maintainability & Documentation** - Were existing comments and docstrings preserved? - Are public types well-documented and backward-compatible? --- ## Review Procedure ### 1. Diff Inspection - Run `git diff` against base branch (`master` or target commit). - Check modified files, deleted lines, and new dependencies. - Ensure no accidental edits, leftover debug statements, or secrets were committed. ### 2. Static Analysis & Compilation Check Verify compiler diagnostics: ```bash npx tsc --noEmit npm run docs:check ``` ### 3. Verification of Tests Verify that test assertions directly exercise the changed behavior and that existing suites still pass: ```bash npx vitest run src/components/editor/preview/__tests__/ProgramPreview.renderLoop.test.ts ``` ### 4. Structure the Review Report Format findings clearly: - **Summary**: Concise overview of the change and overall recommendation (Approve, Request Changes, Discuss). - **Critical Issues (Blockers)**: Confirmed bugs, invariant violations, data-loss risks, or broken tests. Include file and line numbers. - **Risks & Edge Cases**: Potential race conditions or lifecycle concerns requiring verification. - **Improvements (Non-blocking)**: Suggestions for clarity, maintainability, or performance. **Important Rule**: The default review mode must never modify code or push commits unless explicitly authorized by the user.