|
| 1 | +--- |
| 2 | +description: Code Review Quality Control Workflow for TypeScript code in the ELCC project |
| 3 | +--- |
| 4 | + |
| 5 | +# Code Review Quality Control Workflow |
| 6 | + |
| 7 | +Use this workflow to review TypeScript code for the ELCC project. Apply every rule below as a hard gate — flag any violation as a blocking issue. For each issue found, quote the offending lines and state which rule they violate. |
| 8 | + |
| 9 | +**Complete workflow sequence:** This is step 2 of 4 in the complete PR creation process. Always use after jira-issue-management workflow and before pull-request-management workflow to ensure code quality standards are met. |
| 10 | + |
| 11 | +## Steps |
| 12 | + |
| 13 | +1. **Review TypeScript Strictness** |
| 14 | + - No `any` — not in source, not in tests, not in casts |
| 15 | + - No `!` (non-null assertion) — use `isNil` / optional chaining / explicit guard clauses instead |
| 16 | + - No `@ts-ignore` or `@ts-expect-error` |
| 17 | + - No relative imports — always `@/...` path aliases. Exception: barrel `index.ts` files may use relative imports for re-exports |
| 18 | + |
| 19 | +2. **Check Type Cast Placement** |
| 20 | + - Type casts with `as` must happen on assignment or creation, never at the point of use |
| 21 | + - **When a cast is needed at point of use, create a new type definition** that returns the correct type by design, eliminating the need for casts |
| 22 | + - Prefer proper type definitions over runtime type checks; only use guard clauses when runtime validation is genuinely needed, not just to satisfy TypeScript |
| 23 | + - A cast is only acceptable when the type system genuinely cannot infer a type that is known at runtime |
| 24 | + |
| 25 | +3. **Check Cyclomatic Complexity** |
| 26 | + - Each function or method should have at most one level of conditional nesting in its main body |
| 27 | + - Use guard clauses (early returns) to flatten logic |
| 28 | + - Each guard clause must be followed by a blank line before the next statement |
| 29 | + |
| 30 | +4. **Verify Test Completeness** |
| 31 | + - Every test must include explicit Arrange / Act / Assert comments and be fully self-contained |
| 32 | + - No shared state from `beforeEach` unless it is truly invariant setup (e.g., DB reset) |
| 33 | + - Service test assertions should target database state, not service return values, unless the return value is specifically what is under test |
| 34 | + - Spy assertions — use `expect(spy).not.toHaveBeenCalled()` without arguments. Never use `not.toHaveBeenCalledWith(...)` |
| 35 | + |
| 36 | +5. **Ensure One Expect Per Test** |
| 37 | + - Consolidate assertions into a single `expect` using `expect.objectContaining` |
| 38 | + - Exceptions: tests that assert something did NOT happen (e.g., `not.toHaveBeenCalled()`, `not.toBe()`) may be standalone single-line expects |
| 39 | + - When asserting "not any of N values", split into N separate tests rather than looping |
| 40 | + |
| 41 | +6. **Check Test Naming** |
| 42 | + - Tests must follow `"when [condition], [expected behaviour]"` with full English words (no abbreviations) |
| 43 | + - Numbered entities in test bodies: `user1`, `user2`, `workflowStep1`, `workflowStep2` — never `existingUser`, `newUser` |
| 44 | + - Describe hierarchy must mirror the source file path |
| 45 | + |
| 46 | +7. **Validate Error Messages** |
| 47 | + - `expect(bool).toBe(true)` is never acceptable — a failing test must tell you what went wrong without reading the source |
| 48 | + - Split complex boolean assertions into individual named assertions |
| 49 | + |
| 50 | +8. **Check Architecture Consistency** |
| 51 | + - When multiple classes serve similar purposes, ensure they have consistent APIs and clear differentiation in their type signatures |
| 52 | + - **When extending functionality, create new types rather than modifying existing ones to avoid breaking changes** |
| 53 | + - **Ensure similar classes follow the same patterns** |
| 54 | + - Flag inconsistent naming or patterns between similar classes |
| 55 | + |
| 56 | +9. **Check for Over-Engineering** |
| 57 | + - Flag additions that were not requested: |
| 58 | + - New abstractions, helpers, or utilities for a one-time use |
| 59 | + - Features, flags, or configurability that no current caller needs |
| 60 | + - Comments explaining what the code does when the names already say it |
| 61 | + - Error handling for states that cannot be reached |
| 62 | + |
| 63 | +10. **Check for Orphaned or Non-Sensical Code** |
| 64 | + - Flag functions, methods, or code paths that serve no purpose |
| 65 | + - Look for placeholder implementations that never get called |
| 66 | + - Identify dead code paths or unreachable logic |
| 67 | + - **Flag code with comments like "legacy path" or "no-op here" that should be removed** |
| 68 | + - **When removing features, remove all related types, imports, and exports** - don't leave partially implemented type systems |
| 69 | + - Verify that all exported functions are actually used somewhere in the codebase |
| 70 | + |
| 71 | +11. **Validate Import Organization** |
| 72 | + - Imports must follow PEP 8-style grouping with a blank line between each group: |
| 73 | + 1. Node.js built-ins (`path`, `fs`, etc.) |
| 74 | + 2. External packages from `node_modules` |
| 75 | + 3. Internal imports from `@/` |
| 76 | + - Within each group, alphabetical ordering is required. One import statement per module |
| 77 | + |
| 78 | +12. **Check Naming Conventions** |
| 79 | + - No abbreviations — `workflow` not `wf`, `migration` not `mig` |
| 80 | + - Fully qualified names at public boundaries — when data crosses a boundary (API response, email template, event payload), prefix with the parent model name to disambiguate |
| 81 | + - SQL — fully spell out table and column names; no abbreviated aliases |
| 82 | + - Function names — describe both trigger and behavior |
| 83 | + |
| 84 | +13. **Verify Expanded Style** |
| 85 | + - Avoid terse functional chains. Each logical step must be on its own line or extracted to a named variable |
| 86 | + - Extract and rename before constructing objects — never inline a property rename |
| 87 | + - No chained transformations — break chains at each step |
| 88 | + - Named constants — hoist every magic number or string to a named `const` at the top of the function or file |
| 89 | + |
| 90 | +14. **Check Service Pattern** |
| 91 | + - Services encapsulate business logic and are invoked exclusively via their static `perform()` method |
| 92 | + - Never instantiate a service directly outside of its own `static perform()` implementation |
| 93 | + - Red flags: `new SomeService(...)` at a call site, business logic in a controller that belongs in a service, a service calling a query directly instead of delegating to another service |
| 94 | + |
| 95 | +## Output Format |
| 96 | + |
| 97 | +For each issue: |
| 98 | + |
| 99 | +``` |
| 100 | +[RULE N] <rule name> |
| 101 | +File: <path>:<line> |
| 102 | +> <quoted offending code> |
| 103 | +Fix: <one-sentence description of what to change> |
| 104 | +``` |
| 105 | + |
| 106 | +After listing all issues, conclude with one of: |
| 107 | +- **APPROVED** — no blocking issues found. |
| 108 | +- **CHANGES REQUESTED** — N blocking issues listed above. |
| 109 | + |
| 110 | +## Related Workflows |
| 111 | + |
| 112 | +- [`./jira-issue-management.md`](./jira-issue-management.md) - Creating well-structured Jira issues |
| 113 | +- [`./pull-request-management.md`](./pull-request-management.md) - Create and update pull requests |
| 114 | +- [`./testing-instructions.md`](./testing-instructions.md) - Generate comprehensive testing instructions for pull requests |
0 commit comments