|
| 1 | +--- |
| 2 | +description: Review the changes in the current branch (or specified branch) and provide feedback. |
| 3 | +--- |
| 4 | + |
| 5 | +# /review |
| 6 | + |
| 7 | +Perform a comprehensive code review of all changes in the current branch (or specified branch) against master (or main). |
| 8 | + |
| 9 | +## Process |
| 10 | + |
| 11 | +1. **Branch Diff Analysis** |
| 12 | + |
| 13 | + - Determine the name of the current branch to review |
| 14 | + - Run `git merge-base origin/master [branch]` to find the common ancestor commit |
| 15 | + - Run `git diff [common ancestor commit]..[branch]` to capture all changes in the branch |
| 16 | + - Run `git log [common ancestor commit]..[branch] --oneline --no-merges` to understand the commit progression |
| 17 | + - Use `--no-merges` flag to ignore merge commits |
| 18 | + - Identify all modified, added, and deleted files for comprehensive analysis |
| 19 | + |
| 20 | +2. **File-by-File Code Review** |
| 21 | + |
| 22 | + - Read each modified file completely to understand context and changes |
| 23 | + - Focus on the specific diff sections but maintain awareness of surrounding code |
| 24 | + |
| 25 | +3. **Quality Validation** |
| 26 | + |
| 27 | + - Verify the changes are covered with tests |
| 28 | + - Ensure critical code paths have unit tests and edge cases are covered |
| 29 | + - Check that new components have corresponding test files |
| 30 | + |
| 31 | +4. **Security and Performance Audit** |
| 32 | + |
| 33 | + - Scan for potential security vulnerabilities following OWASP Top 10 |
| 34 | + - **A01: Broken Access Control** - Authorization bypass, privilege escalation |
| 35 | + - **A02: Cryptographic Failures** - Weak encryption, exposed secrets, insecure data transmission |
| 36 | + - **A03: Injection** - SQL injection, XSS in components, unsafe database queries |
| 37 | + - **A04: Insecure Design** - Missing security controls, inadequate threat modeling |
| 38 | + - **A05: Security Misconfiguration** - Default configs, exposed debug info, missing security headers |
| 39 | + - **A06: Vulnerable Components** - Outdated npm packages, insecure gem versions |
| 40 | + - **A07: Authentication Failures** - Weak passwords, session management flaws |
| 41 | + - **A08: Data Integrity Failures** - Unsigned software updates, insecure deserialization |
| 42 | + - **A09: Logging/Monitoring Failures** - Missing audit logs, inadequate error tracking |
| 43 | + - **A10: Server-Side Request Forgery** - Unvalidated server-side requests |
| 44 | + - CSRF protection in forms and API endpoints |
| 45 | + - Proper sanitization of user inputs |
| 46 | + - Identify performance bottlenecks: |
| 47 | + - Unnecessary re-renders and missing memoization |
| 48 | + - Memory leaks |
| 49 | + - Inefficient database indexes |
| 50 | + - Check for proper error handling and logging practices |
| 51 | + |
| 52 | +## Review Standards |
| 53 | + |
| 54 | +### Guidelines |
| 55 | + |
| 56 | +- Be concise and specific |
| 57 | +- Provide **only** actionable feedback for all sections except "strengths" |
| 58 | + - Actionable feedback identifies specific issues that require code changes or improvements |
| 59 | + - Non-actionable feedback acknowledges good practices without suggesting changes |
| 60 | + - Example: "Missing error handling in submitForm() function" (actionable) vs "Good use of TypeScript types" (non-actionable) |
| 61 | + - Do not note what was changed in the actionable sections |
| 62 | + - Do not acknowledge good practices in the actionable sections |
| 63 | +- Focus on reviewing the code |
| 64 | + - Do not run tests or linters |
| 65 | + - Do not make changes |
| 66 | + |
| 67 | +### Code Quality |
| 68 | + |
| 69 | +- Includes appropriate tests (happy-path for new features, failing test for bugs) |
| 70 | +- **New `TODO`s**: include explanation for future work |
| 71 | +- **README impact**: update for new env vars, setup instructions, or dependencies |
| 72 | +- **`package-lock.json` changes**: correspond to `package.json` changes |
| 73 | + |
| 74 | +### Coding Standards |
| 75 | + |
| 76 | +#### Security & Environment Configuration |
| 77 | + |
| 78 | +- **Don't expose environment details client-side**: Read configuration from server-side instead of hardcoding environment-specific paths |
| 79 | +- **Environment-based credentials**: Never hardcode database credentials; always use environment variables |
| 80 | +- **API endpoint whitelisting**: Implement whitelists of allowed endpoints in proxy controllers to prevent unintended exposure |
| 81 | +- **Use correct HTTP status codes**: Return 403 Forbidden instead of 401 Unauthorized for permission-related access denials |
| 82 | + |
| 83 | +#### File Organization & Architecture |
| 84 | + |
| 85 | +- **Use proper shared component placement**: Place shared components in appropriate folders |
| 86 | +- **Extract constants for magic numbers**: Define hardcoded limits as named constants (e.g., `const DISPLAY_LIMIT = 4`) |
| 87 | + |
| 88 | +#### Code Quality |
| 89 | + |
| 90 | +- **`any` usage**: Validate type safety and eliminate `any` usage |
| 91 | +- **Use type guards over type assertions**: Prefer `Type.is(value) ? value.prop : undefined` over unsafe casting |
| 92 | +- **Avoid unnecessary optional chaining**: Only use when nullability is actually possible |
| 93 | +- **Provide unique ESLint disable comments**: Each disable should explain the specific violation |
| 94 | +- **Use descriptive variable names**: Avoid abbreviations; use full descriptive names (`index` not `idx`) |
| 95 | +- **Use `@ts-expect-error` over `@ts-ignore`**: Make type ignores explicit and catchable when no longer needed |
| 96 | +- **Remove redundant type suffixes**: Avoid adding "Cube" or similar redundant suffixes to type names |
| 97 | + |
| 98 | +#### Function Design & Immutability |
| 99 | + |
| 100 | +- **Avoid direct object mutation in utilities**: Functions should return new objects instead of modifying parameters |
| 101 | +- **Extract repeated inline logic**: Convert repeated patterns into reusable helper functions |
| 102 | + |
| 103 | +#### Error Handling & Loading States |
| 104 | + |
| 105 | +- **Independent loading states**: Loading states for unrelated features should not block each other |
| 106 | +- **Complete error and loading state handling**: Always implement error and loading states for hooks and API calls |
| 107 | +- **Prevent race conditions**: Disable interactions during loading states |
| 108 | +- **Handle division by zero explicitly**: Check for zero denominators before performing division operations |
| 109 | +- **Provide actionable error messages**: Include specific guidance on how to resolve issues |
| 110 | + |
| 111 | +#### Testing Requirements |
| 112 | + |
| 113 | +- **Mandatory test coverage**: All code changes must be covered by tests unless explicitly discussed with team |
| 114 | +- **Mocking with jest.spyOn**: Ensure proper mocking with `jest.spyOn` |
| 115 | +- **Test edge cases explicitly**: Include tests for empty strings, null values, undefined data |
| 116 | +- **Use proper testing methodology**: Don't use `renderHook` for non-hook functions |
| 117 | +- **Comprehensive test coverage**: When adding new nullable or undefinable props, test both presence and absence |
| 118 | +- **Use test factories**: Use existing factories from `__tests__/factories` instead of manually creating test objects |
| 119 | +- **Test with realistic data structures**: Update test data to match new data models when extending types |
| 120 | + |
| 121 | +#### Performance & UX Guidelines |
| 122 | + |
| 123 | +- **Implement limits for large datasets**: Add sensible display limits for user-generated content |
| 124 | +- **Real-time preview updates**: Interactive elements should provide immediate feedback |
| 125 | + |
| 126 | +### Suggestions Section |
| 127 | + |
| 128 | +Focus on code quality improvements that enhance maintainability: |
| 129 | + |
| 130 | +- Identify opportunities for code deduplication and suggest shared utilities or components |
| 131 | +- Suggest performance optimizations that aren't critical issues |
| 132 | +- Recommend better naming conventions or code organization |
| 133 | + |
| 134 | +## Output Format |
| 135 | + |
| 136 | +Deliver structured feedback using this format: |
| 137 | + |
| 138 | +``` |
| 139 | +## Code Review Results |
| 140 | +
|
| 141 | +## ✅ Strengths |
| 142 | +[Write **one short sentence** highlighting good practices and well-implemented features] |
| 143 | +
|
| 144 | +## 🚨 Critical Issues |
| 145 | +[Must be fixed before merge - include file_path:line_number references] |
| 146 | +
|
| 147 | +## ⚠️ Important Issues |
| 148 | +[Should be addressed - include file_path:line_number references] |
| 149 | +
|
| 150 | +## 💡 Suggestions |
| 151 | +[Improvements that enhance code quality - include file_path:line_number references] |
| 152 | +
|
| 153 | +**Overall Assessment:** [✅ Pass/⚠️ Needs Work/❌ Critical Issues] |
| 154 | +``` |
| 155 | + |
| 156 | +## Output Guidelines |
| 157 | + |
| 158 | +- Always include specific file paths with line numbers for actionable feedback. |
| 159 | +- Be direct and specific about required changes. |
| 160 | +- Do not include additional commentary outside the specified format. |
0 commit comments