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 Schema,
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 codebaserepo-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---5
6# Reviewing Source Code Changes
7
8Guide for reviewing PRs and source code changes in `packages/` and `frameworks/`.
9
10## When to Use This Guide
11
12- Reviewing pull requests modifying library source
13- Validating implementation patterns before merging
14- Checking code quality, types, documentation, and tests
15
16## Review Process
17
181. **Understand the change** — Read PR description, identify affected files
192. **Check patterns** — Verify code follows existing conventions
203. **Verify types** — Ensure type safety and proper inference
214. **Review docs** — Confirm JSDoc is complete and accurate
225. **Check tests** — Validate runtime and type test coverage
23
24## What to Review
25
26### Code Quality
27
28| 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 |
35
36**Good — purity annotation:**
37
38```typescript
39// @__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```
49
50**Bad — missing annotation:**
51
52```typescript
53export function useField<TSchema, TFieldPath>(
54 form: FormStore<TSchema>,
55 config: UseFieldConfig<TSchema, TFieldPath>
56): FieldStore<TSchema, TFieldPath> {
57 return {
58 /* ... */
59 };
60}
61```
62
63### Type Safety
64
65| 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 |
71
72**Good — constrained generic:**
73
74```typescript
75export function useField<
76 TSchema extends Schema,
77 TFieldPath extends RequiredPath<TSchema>,
78>(
79 form: FormStore<TSchema>,
80 config: UseFieldConfig<TSchema, TFieldPath>
81): FieldStore<TSchema, TFieldPath>;
82```
83
84### Documentation
85
86| 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 |
93
94**First line patterns by category:**
95
96| Category | Pattern |
97| ---------- | -------------------------------------------- |
98| Primitives | `Creates a ...` |
99| Methods | `Focuses ...`, `Resets ...`, `Validates ...` |
100| Components | `Renders a ...` |
101| Utilities | `Returns ...`, `Gets ...`, `Sets ...` |
102
103### Tests
104
105| 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 |
110
111## Common Issues
112
113| 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 |
121
122## Checklist
123
124- [ ] Implementation follows existing patterns in similar files
125- [ ] `// @__NO_SIDE_EFFECTS__` on pure factory functions
126- [ ] All imports use `.ts` extension
127- [ ] `interface` used for object shapes
128- [ ] JSDoc complete on all exports
129- [ ] Runtime tests in `.test.ts`
130- [ ] Type tests in `.test-d.ts`
131- [ ] Naming conventions followed
132- [ ] Cross-framework consistency for shared APIs
133
134## Related Skills
135
136- `repo-structure-navigate` — Navigate the codebase
137- `repo-source-code-document` — JSDoc requirements
majiayu000