Reviewing Source Code Changes
Guide for reviewing PRs and source code changes in /library/src/.
When to Use This Guide
- Reviewing pull requests modifying library source
- Validating implementation patterns before merging
- Checking code quality, types, documentation, and tests
Review Process
- Understand the change — Read PR description, identify affected files
- Check patterns — Verify code follows existing conventions
- Verify types — Ensure type safety and proper inference
- Review docs — Confirm JSDoc is complete and accurate
- Check tests — Validate runtime and type test coverage
What to Review
Code Quality
| Check | Requirement |
|---|---|
| Naming | Matches existing patterns (StringSchema, minLength, _parse) |
| Purity annotation | // @__NO_SIDE_EFFECTS__ before pure factory functions |
| Import extensions | All imports use .ts extension |
| Interface vs type | Use interface for object shapes, type for unions/aliases |
| Folder structure | Each API has: name.ts, name.test.ts, name.test-d.ts, index.ts |
Good — purity annotation:
// @__NO_SIDE_EFFECTS__
export function string(message?: ErrorMessage<StringIssue>): StringSchema {
return {
/* ... */
};
}Bad — missing annotation:
export function string(message?: ErrorMessage<StringIssue>): StringSchema {
return {
/* ... */
};
}Type Safety
| Check | Requirement |
|---|---|
| Generic inference | Types infer correctly without explicit annotations |
| Constraints | Generic parameters have appropriate extends clauses |
| Return types | Explicit return types on exported functions |
| Type tests | .test-d.ts file covers type inference scenarios |
Good — constrained generic:
export function minLength<
TInput extends LengthInput,
TRequirement extends number,
>(
requirement: TRequirement,
message?: ErrorMessage<MinLengthIssue<TInput, TRequirement>>
): MinLengthAction<TInput, TRequirement>;Documentation
| Check | Requirement |
|---|---|
| JSDoc present | All exported functions have JSDoc |
| First line | Action verb matching function purpose (see below) |
@param tags | Every parameter documented |
@returns tag | Return value documented |
| Overloads | Every overload has its own complete JSDoc block |
First line patterns by category:
| Category | Pattern |
|---|---|
| Schemas | Creates a... schema. |
| Actions | Creates a... action. |
| Parse methods | Parses... |
| Type guards | Checks if... |
| Unwrap methods | Unwraps... |
| Other methods | Creates a..., Returns..., Forwards... |
See repo-source-code-document skill for full documentation rules.
Tests
| Check | Requirement |
|---|---|
| Runtime tests | .test.ts covers success cases, failure cases, edge cases |
| Type tests | .test-d.ts validates type inference with expectTypeOf |
| Issue messages | Tests verify correct error messages and issue structure |
Common Issues
| Issue | What to Look For |
|---|---|
| Missing purity annotation | Factory function without // @__NO_SIDE_EFFECTS__ |
| Incomplete JSDoc | Missing @param or @returns, wrong description format |
| No type tests | New API without .test-d.ts file |
| Wrong import extension | Imports without .ts suffix |
| Inconsistent naming | Schema not ending in Schema, action not ending in Action |
| Side effects in pure code | Mutations, I/O, or global state in schema/action creation |
Checklist
- Implementation follows existing patterns in similar files
// @__NO_SIDE_EFFECTS__on pure factory functions- All imports use
.tsextension interfaceused for object shapes- JSDoc complete on all exports
- Runtime tests in
.test.ts - Type tests in
.test-d.ts - Naming conventions followed
Related Skills
repo-structure-navigate— Navigate the codebaserepo-source-code-document— JSDoc requirements