Reviewing Source Code Changes
Guide for reviewing PRs and source code changes in packages/ and frameworks/.
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 (FormStore, useField, createForm) |
| 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 |
Methods: name.ts, index.ts. Primitives/components in their folder |
Good — purity annotation:
// @__NO_SIDE_EFFECTS__
export function useField<TSchema, TFieldPath>(
form: FormStore<TSchema>,
config: UseFieldConfig<TSchema, TFieldPath>
): FieldStore<TSchema, TFieldPath> {
return {
/* ... */
};
}
Bad — missing annotation:
export function useField<TSchema, TFieldPath>(
form: FormStore<TSchema>,
config: UseFieldConfig<TSchema, TFieldPath>
): FieldStore<TSchema, TFieldPath> {
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 useField<
TSchema extends FormSchema,
TFieldPath extends RequiredPath<TSchema>,
>(
form: FormStore<TSchema>,
config: UseFieldConfig<TSchema, TFieldPath>
): FieldStore<TSchema, TFieldPath>;
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 |
| Primitives |
Creates a ... |
| Methods |
Focuses ..., Resets ..., Validates ... |
| Components |
Renders a ... |
| Utilities |
Returns ..., Gets ..., Sets ... |
Tests
| Check |
Requirement |
| Runtime tests |
.test.ts covers success cases, failure cases, edge cases |
| Type tests |
.test-d.ts validates type inference with expectTypeOf |
| Error handling |
Tests verify correct error messages and validation |
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 |
Primitives not using create/use prefix, wrong Store suffix |
| Side effects in pure code |
Mutations, I/O, or global state in primitive/method creation |
Checklist
Related Skills
repo-structure-navigate — Navigate the codebase
repo-source-code-document — JSDoc requirements
1---2name: repo-source-code-review3description: Review PRs and source code changes in Formisch packages/ and frameworks/. Use when reviewing pull requests, validating implementation patterns, or checking code quality before merging.4---56# Reviewing Source Code Changes78Guide for reviewing PRs and source code changes in `packages/` and `frameworks/`.910## When to Use This Guide1112- Reviewing pull requests modifying library source13- Validating implementation patterns before merging14- Checking code quality, types, documentation, and tests1516## Review Process17181. **Understand the change** — Read PR description, identify affected files192. **Check patterns** — Verify code follows existing conventions203. **Verify types** — Ensure type safety and proper inference214. **Review docs** — Confirm JSDoc is complete and accurate225. **Check tests** — Validate runtime and type test coverage2324## What to Review2526### Code Quality2728| Check | Requirement |29| ----------------- | --------------------------------------------------------------------- |30| Naming | Matches existing patterns (`FormStore`, `useField`, `createForm`) |31| Purity annotation | `// @__NO_SIDE_EFFECTS__` before pure factory functions |32| Import extensions | All imports use `.ts` extension |33| Interface vs type | Use `interface` for object shapes, `type` for unions/aliases |34| Folder structure | Methods: `name.ts`, `index.ts`. Primitives/components in their folder |3536**Good — purity annotation:**3738```typescript39// @__NO_SIDE_EFFECTS__40export function useField<TSchema, TFieldPath>(41 form: FormStore<TSchema>,42 config: UseFieldConfig<TSchema, TFieldPath>43): FieldStore<TSchema, TFieldPath> {44 return {45 /* ... */46 };47}48```4950**Bad — missing annotation:**5152```typescript53export function useField<TSchema, TFieldPath>(54 form: FormStore<TSchema>,55 config: UseFieldConfig<TSchema, TFieldPath>56): FieldStore<TSchema, TFieldPath> {57 return {58 /* ... */59 };60}61```6263### Type Safety6465| Check | Requirement |66| ----------------- | ----------------------------------------------------- |67| Generic inference | Types infer correctly without explicit annotations |68| Constraints | Generic parameters have appropriate `extends` clauses |69| Return types | Explicit return types on exported functions |70| Type tests | `.test-d.ts` file covers type inference scenarios |7172**Good — constrained generic:**7374```typescript75export function useField<76 TSchema extends FormSchema,77 TFieldPath extends RequiredPath<TSchema>,78>(79 form: FormStore<TSchema>,80 config: UseFieldConfig<TSchema, TFieldPath>81): FieldStore<TSchema, TFieldPath>;82```8384### Documentation8586| Check | Requirement |87| -------------- | ------------------------------------------------- |88| JSDoc present | All exported functions have JSDoc |89| First line | Action verb matching function purpose (see below) |90| `@param` tags | Every parameter documented |91| `@returns` tag | Return value documented |92| Overloads | Every overload has its own complete JSDoc block |9394**First line patterns by category:**9596| Category | Pattern |97| ---------- | -------------------------------------------- |98| Primitives | `Creates a ...` |99| Methods | `Focuses ...`, `Resets ...`, `Validates ...` |100| Components | `Renders a ...` |101| Utilities | `Returns ...`, `Gets ...`, `Sets ...` |102103### Tests104105| Check | Requirement |106| -------------- | ---------------------------------------------------------- |107| Runtime tests | `.test.ts` covers success cases, failure cases, edge cases |108| Type tests | `.test-d.ts` validates type inference with `expectTypeOf` |109| Error handling | Tests verify correct error messages and validation |110111## Common Issues112113| Issue | What to Look For |114| ------------------------- | -------------------------------------------------------------- |115| Missing purity annotation | Factory function without `// @__NO_SIDE_EFFECTS__` |116| Incomplete JSDoc | Missing `@param` or `@returns`, wrong description format |117| No type tests | New API without `.test-d.ts` file |118| Wrong import extension | Imports without `.ts` suffix |119| Inconsistent naming | Primitives not using `create`/`use` prefix, wrong Store suffix |120| Side effects in pure code | Mutations, I/O, or global state in primitive/method creation |121122## Checklist123124- [ ] Implementation follows existing patterns in similar files125- [ ] `// @__NO_SIDE_EFFECTS__` on pure factory functions126- [ ] All imports use `.ts` extension127- [ ] `interface` used for object shapes128- [ ] JSDoc complete on all exports129- [ ] Runtime tests in `.test.ts`130- [ ] Type tests in `.test-d.ts`131- [ ] Naming conventions followed132- [ ] Cross-framework consistency for shared APIs133134## Related Skills135136- `repo-structure-navigate` — Navigate the codebase137- `repo-source-code-document` — JSDoc requirements