Framework Code Review
Use this skill when reviewing or writing framework-level code: handler, parser, protocol, router, CLI runtime, FFI, or state management. The checklist is generic - apply every section to every PR that touches core logic.
Complements: perf (speed and memory optimization), quality-gate (automated checks), ffi (C ABI boundary safety).
1 - Cross-layer contract parity
Every behavioral contract that spans more than one layer (parser → protocol → handler → client) must be identical in semantics. Mismatches cause silent divergence between SSR and client rendering.
| Check |
Why |
| Encoding / decoding policy |
If one layer decodes (e.g., decodeURIComponent), all layers that consume the value must agree on whether the value is decoded or raw. |
| Matching semantics |
Optional, splat, exact, and wildcard rules must produce the same result on server and client for the same input. |
| Tie-breaking determinism |
When multiple candidates match with equal rank, the selection rule must be documented and identical across layers. |
| Edge values |
Empty strings, /, encoded slashes (%2F), Unicode, and values containing delimiters (., :, *) must be tested at every boundary. |
| Error representation |
When one layer returns an error, the other layers must handle or propagate it consistently - no silent swallowing. |
| Spec-vs-code parity |
If DESIGN.md specifies behavior (e.g., array indexing items.0.name, string .length semantics), the implementation must match. Tests that lock in spec-violating behavior are bugs, not proof of correctness. |
| Binding surface parity |
Node, WASM, and CLI bindings must expose the same logical API. If one uses protobuf bytes and another uses JSON strings, transparent fallback is impossible. |
| Type coercion consistency |
If == uses strict equality but > coerces strings to numbers, document the coercion matrix. Operators on the same pair of operands should not produce contradictory outcomes. |
| Literal grammar completeness |
If the expression engine supports true, false, "strings", and numbers as literals, it must also support null. Missing literals create dead-end conditions (e.g., foo == null fails). |
| Fallback contract validity |
If a function falls back to a CLI subprocess or alternative backend, the result shape must satisfy the same postconditions as the primary path. An empty buffer from a fallback is a contract violation. |
How to check
- Write a table: "Input X → Layer A produces Y₁, Layer B produces Y₂". If Y₁ ≠ Y₂, file a parity bug.
- Ensure tests exist that exercise the same input through all layers.
- Diff the function signatures between Node/WASM/TS bindings - arguments and return types must match logically.
- Search for tests that assert spec-violating behavior (e.g.,
test_array_index_not_resolved) - these are bugs masquerading as tests.
2 - Determinism
Non-deterministic behavior is a correctness bug, even if it "usually works."
| Anti-pattern |
Fix |
Iterating HashMap / HashSet for order-dependent logic (ranking, tie-breaking, first-match) |
Use BTreeMap, IndexMap, or add an explicit ordering field. |
| Relying on allocation or hash seed order across process restarts |
Use deterministic comparison (insertion order, lexicographic, priority field). |
| Floating-point comparison for equality |
Use integer arithmetic or epsilon-aware comparison. |
rand / Uuid::new_v4 in deterministic paths |
Inject randomness explicitly; keep core logic pure. |
How to check
- Search for
HashMap or HashSet usage where iteration order affects output.
- Run the same input twice in different process instances - output must be identical.
3 - Concurrency and race safety
Rust (server / CLI)
| Check |
Why |
| Lock scope |
Hold Mutex / RwLock only long enough to copy or read what you need. Never hold a lock across I/O, rendering, or network calls. |
| Snapshot consistency |
If a request reads multiple fields from shared state, read them all under one lock acquisition. Two separate locks can produce mixed old/new data if a writer updates between them. |
std::sync::Mutex on async paths |
Blocking mutexes on async paths can starve the executor. Prefer tokio::sync::RwLock or Arc<ArcSwap<T>> for read-heavy shared state. |
| Atomics |
Use AtomicU64 for simple counters/versions instead of locking. |
| File watcher error recovery |
If a rebuild fails, the watcher must NOT update its "last seen" timestamps - otherwise it won't retry when the error is transient. Keep a retry/backoff path. |
| Cache concurrent writes |
Temp files used for atomic-replace must have unique names (PID + random suffix). Two writers using the same {key}.tmp will clobber each other. |
| Event-driven vs polling |
Polling every N ms scales poorly. Prefer event-driven file notifications (notify crate) or at least directory-level mtime checks before deep scans. |
Rust (FFI / WASM boundaries)
| Check |
Why |
catch_unwind on FFI exports |
Every extern "C" function must wrap its body in std::panic::catch_unwind(). Panics from transitive dependencies (serde, prost) crossing the FFI boundary are UB in unwind mode and abrupt termination in abort mode. |
| WASM panic hooks |
Install console_error_panic_hook (or equivalent) so panics produce diagnosable errors instead of opaque host traps. |
| Callback error propagation |
If a host callback (e.g., JS chunk writer) fails, stop processing immediately. Continuing to render after the sink has failed wastes CPU and can produce partial/corrupt output. |
TypeScript (client)
| Check |
Why |
| In-flight request cancellation |
Rapid user actions (navigations, searches) can launch multiple async operations. Use AbortController + sequence IDs to discard stale results. |
| Event ordering |
DOM events and CustomEvent dispatches can interleave with async completions. Ensure state mutations are guarded by a generation counter or similar. |
| Feature detection |
Browser APIs (Navigation API, View Transitions) may not exist. Guard with typeof / 'navigation' in window before use. |
Node.js bindings
| Check |
Why |
| Event loop blocking |
Synchronous native calls that do parsing, rendering, or I/O block the Node.js event loop. Expose async variants via napi::Task/AsyncTask for any operation that takes > 1ms. |
4 - Allocation and clone discipline
Every unnecessary allocation is CPU time and GC pressure. Apply these rules in hot paths (request handling, rendering, matching, serialization).
Rust
| Anti-pattern |
Fix |
format!() in loops or hot paths |
Use write!(buf, ...) into a pre-allocated buffer, or push_str. |
String::from(ch) for single characters |
Use buf.push(ch) or write!(buf, "{ch}"). |
.clone() on a value used only for read access |
Pass &T or &str instead. |
.clone() to insert into HashSet after a .contains() check |
Reorder: do the lookup / get first, then insert(owned_value) by move. |
.to_string() on Cow<str> |
Write the Cow directly; .to_string() defeats zero-copy. |
collect::<Vec<_>>() when sequential iteration suffices |
Iterate the iterator directly. |
Vec<T> when max size is small and known |
Use SmallVec<[T; N]> or a stack array to avoid heap allocation. |
| Deep-cloning large structures (protocol, state trees) per request |
Use Arc<T> with clone-on-write or snapshot swapping. |
| Re-parsing static data on every request |
Cache the parsed form alongside the raw data. |
to_vec() before join() |
Slice the original and join directly: segments[idx..].join("/"). |
| Recompiling parser queries per call |
Cache compiled queries in OnceLock<Query> or extract all attributes once into a struct per element. |
chars[i..].iter().collect::<String>() for substring comparison |
Compare without allocation: use byte-level starts_with or slice equality. |
Zero-capacity String::new() for output buffers |
Pre-allocate with String::with_capacity(estimated_size). For HTML output, 4096 is a reasonable starting point. |
Cloning JSON Value subtrees on lookup |
Add a borrowed lookup API (-> &Value) for the hot path; keep owned API as a thin wrapper. |
| Full-tree validation pre-pass on every evaluation |
Validate once at AST construction time and cache the result, not on every evaluation call. |
TypeScript
| Anti-pattern |
Fix |
Array.filter().length for counting |
Use a for loop with a counter, or reduce. |
Repeated DOM queries (querySelectorAll) on unchanged DOM |
Cache the result; invalidate on mutation. |
JSON.parse(JSON.stringify(x)) for deep copy |
Use structuredClone() or avoid the copy entirely. |
| Rebuilding derived data every call when inputs haven't changed |
Memoize or cache with invalidation. |
5 - Data structure selection
Choose the structure that matches the access pattern. A wrong choice is a design bug.
| Access pattern |
Preferred structure |
| Order-dependent iteration (ranking, priority) |
BTreeMap, Vec sorted by key, IndexMap |
| Membership test only |
HashSet, bloom filter (but beware false positives - see below) |
| Frequent insert + lookup by key |
HashMap (if order doesn't matter) |
| Small fixed-size collections (< 16 elements) |
Stack array, SmallVec, ArrayVec |
| Append-only log |
Vec with push |
| Probabilistic membership (bloom filter) |
Use ≥ 2 independent hash functions. A single-hash bloom filter has unacceptable false-positive rates at moderate load. Document the false-positive bound. |
How to check
- For every
HashMap in the diff: "Does any consumer iterate this and depend on order?" If yes, switch.
- For every
Vec: "Is the capacity known?" If yes, use with_capacity.
- For every bloom filter: "What is the false-positive rate at expected load?" If > 1%, redesign.
6 - API surface and design deduplication
| Check |
Why |
| No duplicate logic across layers |
If server and client both implement matching, both must call equivalent code, not independent reimplementations. Any behavioral change must update both. |
| No duplicate calls in the same request |
If match_route() is called twice on the same path in one request flow, extract the result and pass it through. |
| Free functions vs. methods |
If standalone helper functions duplicate methods on a type (e.g., activateRoute(el) and el.activate()), remove one. Keep the method; delete the free function. |
| Caching static computations |
Parsing a template string into segments is pure. If the template set is static, parse once and cache. |
| Minimal public surface |
Use pub(crate) for internal helpers. Every public API is a maintenance commitment. |
| Error paths are actionable |
Errors must say what went wrong AND what the caller can do. "Parse error" is not actionable. "Invalid parameter name 'id-x' in path '/users/:id-x': only alphanumeric and underscore allowed" is. |
7 - Type safety, correctness idioms, and architectural rules
Rust - banned patterns
| Pattern |
Preference |
unwrap() / expect() in library code |
Banned. Use ? with typed errors via thiserror. |
unsafe without // SAFETY: comment |
Banned. Document the invariant upheld. |
| Recursion in core algorithms |
Banned. Use iterative loops with an explicit stack. Recursion blows the call stack on deep/adversarial inputs and defeats branch prediction. |
as any casts (in FFI or interop) |
Minimize. Use typed wrappers or trait objects. |
| Raw pointer arithmetic |
Prefer safe abstractions. Use unsafe only when necessary, with safety proof. |
Rust - required annotations
| Pattern |
Preference |
#[must_use] |
Add to all public constructors, serialization/deserialization methods, and functions returning Option/Result whose value should not be silently discarded. |
#[allow(dead_code)] |
Remove the annotation AND the dead code. If the field/function is planned for future use, delete it now and re-add when needed (with a TODO referencing an issue). |
Rust - error handling precision
| Pattern |
Preference |
Silent fallbacks (unwrap_or(0), unwrap_or_default()) |
Replace with explicit error propagation. A silent default can mask a real mismatch (e.g., unbalanced start/end markers). If a default is genuinely correct, add a comment explaining why. |
Option<T> return for lookups that can fail in multiple ways |
Use Result<T, E> with a typed error enum distinguishing missing-key, invalid-path, type-mismatch, etc. Collapsing all failures to None makes debugging impossible. |
Error variants that abuse a generic Validation(String) |
Add dedicated variants (Decode, Encode, MissingField) so callers can match programmatically. |
| Byte vs char index confusion |
When scanning strings with chars() but slicing with byte indices, panics occur on non-ASCII input. Always use char_indices() or work exclusively with byte offsets. |
Rust - numeric precision
| Pattern |
Preference |
Forcing all numbers through f64 for comparisons |
Preserve exact integer comparisons (i64/u64) when both operands are integral. Only fall back to f64 for genuine floats. Large integers above 2^53 lose precision in f64. |
Rust - build-time validation
| Pattern |
Preference |
| Deferring validation to render-time |
Validate at parse/build time and fail fast. If a route references a component that doesn't exist, or a path is empty, catch it during parsing - not when a user hits the route. |
| Accepting invalid inputs that pass through |
If a <route> element is missing path or references an unknown component, the parser must reject it with an actionable error. |
bytes[i] as char on UTF-8 strings |
This silently corrupts multi-byte characters. Use str slicing or char_indices() to preserve UTF-8 correctness. |
Rust - diagnostics & error presentation
See skills/diagnostics/SKILL.md for the full workflow. On review, check:
| Pattern |
Preference |
panic!/unwrap on recoverable (template/CLI/state) input |
Banned. Return Result - panic = "abort" kills FFI/WASM/Node hosts. Authoring mistakes return ParserError::Template(Box<Diagnostic>). |
Authoring error without a stable code, location, or help: |
A Diagnostic must carry a diagnostic::codes constant, --> owner:line:column, snippet, and an actionable help:. Codes are a stable API - flag renames. |
| Color/ANSI in a library, host, browser, or JSON channel |
Color belongs only in webui-cli (console::style()). Libraries emit plain data; FFI/WASM/Node/browser get plain Display. Split terminal-vs-machine renderings (e.g. RebuildError { display, message }) rather than leaking ANSI. |
| A single SGR span wrapping newlines |
Style each line independently - a span across \n bleeds when lines are re-prefixed ([server]). |
| Error construction inlined into a hot function |
Mark error builders #[cold]/#[inline(never)]; keep the hot fast-path inlinable. Layout perturbation from cold error code is a real regression - confirm with cargo bench vs the base branch. |
serde_json::json! macro in CLI output |
Use the serde_json::Map API; the macro unwraps internally and trips disallowed_methods. |
| Tests asserting on error prose |
Assert on the stable code (and that JSON output contains no \x1b), not the human message. |
TypeScript
| Pattern |
Preference |
(x as any) |
Use proper type narrowing, generics, or unknown with guards. |
Untyped expando properties ((el as any)._data = ...) |
Use WeakMap<Element, T> or typed fields on a subclass. |
var |
Banned. Use const by default, let only when reassignment is needed. |
== null |
Use === null || === undefined or optional chaining (?.). |
| Callback-based async |
Use async/await. |
arguments object |
Use rest parameters (...args). |
Loose union types (state: object | string) |
Use precise types. If Rust accepts serde_json::Value, define a recursive JsonValue type on the TS side. If plugin accepts "fast" only, use a string literal union. |
8 - Style, comments, and documentation
| Check |
Rule |
| Dead code |
Remove unused functions, imports, and variables. Don't comment them out. |
| Commented-out code |
Delete it. Git has history. |
| Obvious comments |
Remove comments that restate the code: // increment counter above counter += 1. |
| Missing comments |
Add comments for non-obvious decisions: "We use BTreeMap here for deterministic iteration order." |
TODO / FIXME / HACK |
Acceptable only with an associated issue number: // TODO(#123): switch to streaming parser. |
| Naming |
Rust: snake_case functions, PascalCase types, SCREAMING_SNAKE constants. TS: camelCase variables/functions, PascalCase classes/types. |
| Magic numbers |
Extract to named constants with a doc comment. |
| File length |
When a file exceeds ~400 lines, split by concern. |
| Function arguments |
Max 5 parameters. Use a config/options struct beyond that. |
| Unused dependencies |
Remove deps listed in Cargo.toml that are never imported. anyhow is for binary crates only - library crates use thiserror. |
| Placeholder / no-op APIs |
If a public function always returns empty/default, either implement it or remove it behind a feature gate. Silent no-ops mislead callers. |
| Tests that lock in wrong behavior |
A test named test_feature_not_supported that asserts None for spec-required behavior is a bug, not a test. Delete or fix it. |
9 - Server runtime, proxy, and discovery correctness
When the change touches dev-server, API proxy, request handling, or package discovery:
| Check |
Why |
| HTTP semantics |
Accept header parsing must respect quality weights (q=). Substring matching (contains("application/json")) is incorrect per RFC 7231. |
| Status codes |
A "no match" must return 404, not 200 with empty body. Clients cannot distinguish "valid empty" from "not found" otherwise. |
| Proxy header forwarding |
Forward a safe allowlist: Authorization, Cookie, Accept, Content-Type, X-Request-ID, etc. Don't forward Host or hop-by-hop headers. |
| Path handling |
Paths with dots (e.g., /api/v2.1/users) are valid routes, not file requests. Don't use contains('.') as a file-vs-route heuristic. |
| Resource reuse |
HTTP clients, TLS contexts, and connection pools are expensive to create. Reuse them across requests. |
| HMR injection |
Search for closing tags case-insensitively. </BODY> and </body> are both valid HTML. |
| Input validation |
CLI commands must validate that input paths are directories (not files) before proceeding. Actionable error early beats cryptic failure later. |
| Cache invalidation scope |
Cache keys must include hashes/mtimes of ALL files that affect the cached result - not just package.json. If template HTML or CSS changes, the cache must invalidate. |
| Cache write safety |
Use per-write unique temp file names (PID + random suffix) and atomic rename. Concurrent writers must not clobber each other's temp files. |
| Symlink policy |
Package resolvers must allow workspace-linked packages (symlinks outside node_modules). Blocking all external symlinks breaks common monorepo patterns. |
| File-size guardrails |
Discovery that reads arbitrary files must cap read sizes. A 100MB HTML file shouldn't cause OOM during component scanning. |
10 - Protocol and schema hygiene
When the change touches .proto files, serialization, or the binary protocol:
| Check |
Why |
| Field additions are backward-compatible |
New fields must have defaults that preserve old behavior. |
Removed fields use reserved |
Prevents accidental reuse of field numbers. |
| Enum variants are exhaustive |
Match arms must handle unknown variants (use a default/unknown variant or _ with a log). |
| No unnecessary nesting |
Flat structures decode faster than deeply nested ones. |
| Measure payload size |
Before/after byte counts for representative inputs. Extra fields add decode overhead even when empty. |
| Prioritize decode speed |
Breaking field changes are allowed when they improve performance measurably. Remove unused fields and message shapes that add decode overhead. |
| Cascade all layers |
Schema changes affect the whole stack: protocol -> handler -> FFI -> CLI. Update all in the same change. Run cargo xtask build && cargo xtask test to validate. |
| Update DESIGN.md |
Protocol behavior changes must update the protocol sections of DESIGN.md in the same commit. |
10b - FFI boundary safety
When the change touches crates/webui-ffi or C ABI signatures:
| Check |
Why |
# Safety doc on every extern "C" fn |
Documents the invariants the caller must uphold. |
// SAFETY: on every unsafe block |
Explains why the unsafe operation is sound. |
catch_unwind wraps all FFI bodies |
Panics from transitive deps (serde, prost) crossing the boundary are UB in unwind mode. On panic, set last-error and return null. |
| Validate all foreign inputs |
Null pointers, invalid UTF-8, out-of-range values - check before dereferencing. |
No unwrap() or expect() |
Anywhere in the FFI code path, including error-handling helpers. |
| Minimal stable surface |
Prefer opaque pointers and integer error codes over exposing Rust layouts. |
| Header sync |
If any #[no_mangle] signature changes, verify crates/webui-ffi/include/webui_ffi.h. |
11 - Documentation sync
Every behavioral change must include corresponding documentation updates. Missing docs are a review finding.
| Check |
Why |
| DESIGN.md updated |
If the change modifies public APIs, protocol fields, behavioral contracts, error variants, or SSR markers, DESIGN.md must be updated in the same commit. |
| User-facing docs updated |
If the change affects CLI flags, template syntax, component authoring, routing, or integration behavior, docs/ must be updated. |
| AI reference updated |
If the change affects anything a code-generation AI would need to know, docs/ai.md must be updated. |
| README links to docs portal |
Package READMEs should defer to the docs portal, not duplicate content. |
| No stale examples |
Code examples in docs must use current API signatures, flag names, and marker formats. |
| docs build passes |
cd docs && pnpm build must succeed (catches broken links, unescaped {{, missing pages). |
See docs-sync skill for the full file mapping of what-changed to which-docs-to-update.
Review output format
When producing a code review, report findings as:
### ISSUE-NNN: <short title>
**Severity**: 🔴 Bug | 🟠 Security | 🟡 Performance | 🔵 Design/Style
**Files**: `path/to/file.rs` (lines N-M)
**Description**: What is wrong, with a code snippet showing the problem.
**Impact**: What breaks or degrades, and under what conditions.
**Fix**: Concrete suggestion, with a code snippet when helpful.
Each finding must be self-contained - readable without context from other findings. This allows direct conversion to GitHub issues.
1---2name: code-review-33description: Framework code review checklist - correctness, performance, concurrency, design, and style.4---56# Framework Code Review78Use this skill when reviewing or writing framework-level code: handler, parser, protocol, router, CLI runtime, FFI, or state management. The checklist is generic - apply every section to every PR that touches core logic.910> **Complements**: `perf` (speed and memory optimization), `quality-gate` (automated checks), `ffi` (C ABI boundary safety).1112---1314## 1 - Cross-layer contract parity1516Every behavioral contract that spans more than one layer (parser → protocol → handler → client) must be **identical** in semantics. Mismatches cause silent divergence between SSR and client rendering.1718| Check | Why |19|-------|-----|20| **Encoding / decoding policy** | If one layer decodes (e.g., `decodeURIComponent`), all layers that consume the value must agree on whether the value is decoded or raw. |21| **Matching semantics** | Optional, splat, exact, and wildcard rules must produce the same result on server and client for the same input. |22| **Tie-breaking determinism** | When multiple candidates match with equal rank, the selection rule must be documented and identical across layers. |23| **Edge values** | Empty strings, `/`, encoded slashes (`%2F`), Unicode, and values containing delimiters (`.`, `:`, `*`) must be tested at every boundary. |24| **Error representation** | When one layer returns an error, the other layers must handle or propagate it consistently - no silent swallowing. |25| **Spec-vs-code parity** | If `DESIGN.md` specifies behavior (e.g., array indexing `items.0.name`, string `.length` semantics), the implementation must match. Tests that lock in spec-violating behavior are bugs, not proof of correctness. |26| **Binding surface parity** | Node, WASM, and CLI bindings must expose the same logical API. If one uses protobuf bytes and another uses JSON strings, transparent fallback is impossible. |27| **Type coercion consistency** | If `==` uses strict equality but `>` coerces strings to numbers, document the coercion matrix. Operators on the same pair of operands should not produce contradictory outcomes. |28| **Literal grammar completeness** | If the expression engine supports `true`, `false`, `"strings"`, and numbers as literals, it must also support `null`. Missing literals create dead-end conditions (e.g., `foo == null` fails). |29| **Fallback contract validity** | If a function falls back to a CLI subprocess or alternative backend, the result shape must satisfy the same postconditions as the primary path. An empty buffer from a fallback is a contract violation. |3031### How to check3233- Write a table: "Input X → Layer A produces Y₁, Layer B produces Y₂". If Y₁ ≠ Y₂, file a parity bug.34- Ensure tests exist that exercise the same input through all layers.35- Diff the function signatures between Node/WASM/TS bindings - arguments and return types must match logically.36- Search for tests that assert spec-violating behavior (e.g., `test_array_index_not_resolved`) - these are bugs masquerading as tests.3738---3940## 2 - Determinism4142Non-deterministic behavior is a correctness bug, even if it "usually works."4344| Anti-pattern | Fix |45|-------------|-----|46| Iterating `HashMap` / `HashSet` for order-dependent logic (ranking, tie-breaking, first-match) | Use `BTreeMap`, `IndexMap`, or add an explicit ordering field. |47| Relying on allocation or hash seed order across process restarts | Use deterministic comparison (insertion order, lexicographic, priority field). |48| Floating-point comparison for equality | Use integer arithmetic or epsilon-aware comparison. |49| `rand` / `Uuid::new_v4` in deterministic paths | Inject randomness explicitly; keep core logic pure. |5051### How to check5253- Search for `HashMap` or `HashSet` usage where iteration order affects output.54- Run the same input twice in different process instances - output must be identical.5556---5758## 3 - Concurrency and race safety5960### Rust (server / CLI)6162| Check | Why |63|-------|-----|64| **Lock scope** | Hold `Mutex` / `RwLock` only long enough to copy or read what you need. Never hold a lock across I/O, rendering, or network calls. |65| **Snapshot consistency** | If a request reads multiple fields from shared state, read them all under **one** lock acquisition. Two separate locks can produce mixed old/new data if a writer updates between them. |66| **`std::sync::Mutex` on async paths** | Blocking mutexes on async paths can starve the executor. Prefer `tokio::sync::RwLock` or `Arc<ArcSwap<T>>` for read-heavy shared state. |67| **Atomics** | Use `AtomicU64` for simple counters/versions instead of locking. |68| **File watcher error recovery** | If a rebuild fails, the watcher must NOT update its "last seen" timestamps - otherwise it won't retry when the error is transient. Keep a retry/backoff path. |69| **Cache concurrent writes** | Temp files used for atomic-replace must have unique names (PID + random suffix). Two writers using the same `{key}.tmp` will clobber each other. |70| **Event-driven vs polling** | Polling every N ms scales poorly. Prefer event-driven file notifications (`notify` crate) or at least directory-level mtime checks before deep scans. |7172### Rust (FFI / WASM boundaries)7374| Check | Why |75|-------|-----|76| **`catch_unwind` on FFI exports** | Every `extern "C"` function must wrap its body in `std::panic::catch_unwind()`. Panics from transitive dependencies (serde, prost) crossing the FFI boundary are UB in unwind mode and abrupt termination in abort mode. |77| **WASM panic hooks** | Install `console_error_panic_hook` (or equivalent) so panics produce diagnosable errors instead of opaque host traps. |78| **Callback error propagation** | If a host callback (e.g., JS chunk writer) fails, stop processing immediately. Continuing to render after the sink has failed wastes CPU and can produce partial/corrupt output. |7980### TypeScript (client)8182| Check | Why |83|-------|-----|84| **In-flight request cancellation** | Rapid user actions (navigations, searches) can launch multiple async operations. Use `AbortController` + sequence IDs to discard stale results. |85| **Event ordering** | DOM events and `CustomEvent` dispatches can interleave with async completions. Ensure state mutations are guarded by a generation counter or similar. |86| **Feature detection** | Browser APIs (`Navigation API`, `View Transitions`) may not exist. Guard with `typeof` / `'navigation' in window` before use. |8788### Node.js bindings8990| Check | Why |91|-------|-----|92| **Event loop blocking** | Synchronous native calls that do parsing, rendering, or I/O block the Node.js event loop. Expose async variants via `napi::Task`/`AsyncTask` for any operation that takes > 1ms. |9394---9596## 4 - Allocation and clone discipline9798Every unnecessary allocation is CPU time and GC pressure. Apply these rules in hot paths (request handling, rendering, matching, serialization).99100### Rust101102| Anti-pattern | Fix |103|-------------|-----|104| `format!()` in loops or hot paths | Use `write!(buf, ...)` into a pre-allocated buffer, or `push_str`. |105| `String::from(ch)` for single characters | Use `buf.push(ch)` or `write!(buf, "{ch}")`. |106| `.clone()` on a value used only for read access | Pass `&T` or `&str` instead. |107| `.clone()` to insert into `HashSet` after a `.contains()` check | Reorder: do the lookup / get first, then `insert(owned_value)` by move. |108| `.to_string()` on `Cow<str>` | Write the `Cow` directly; `.to_string()` defeats zero-copy. |109| `collect::<Vec<_>>()` when sequential iteration suffices | Iterate the iterator directly. |110| `Vec<T>` when max size is small and known | Use `SmallVec<[T; N]>` or a stack array to avoid heap allocation. |111| Deep-cloning large structures (protocol, state trees) per request | Use `Arc<T>` with clone-on-write or snapshot swapping. |112| Re-parsing static data on every request | Cache the parsed form alongside the raw data. |113| `to_vec()` before `join()` | Slice the original and join directly: `segments[idx..].join("/")`. |114| Recompiling parser queries per call | Cache compiled queries in `OnceLock<Query>` or extract all attributes once into a struct per element. |115| `chars[i..].iter().collect::<String>()` for substring comparison | Compare without allocation: use byte-level `starts_with` or slice equality. |116| Zero-capacity `String::new()` for output buffers | Pre-allocate with `String::with_capacity(estimated_size)`. For HTML output, 4096 is a reasonable starting point. |117| Cloning JSON `Value` subtrees on lookup | Add a borrowed lookup API (`-> &Value`) for the hot path; keep owned API as a thin wrapper. |118| Full-tree validation pre-pass on every evaluation | Validate once at AST construction time and cache the result, not on every evaluation call. |119120### TypeScript121122| Anti-pattern | Fix |123|-------------|-----|124| `Array.filter().length` for counting | Use a `for` loop with a counter, or `reduce`. |125| Repeated DOM queries (`querySelectorAll`) on unchanged DOM | Cache the result; invalidate on mutation. |126| `JSON.parse(JSON.stringify(x))` for deep copy | Use `structuredClone()` or avoid the copy entirely. |127| Rebuilding derived data every call when inputs haven't changed | Memoize or cache with invalidation. |128129---130131## 5 - Data structure selection132133Choose the structure that matches the access pattern. A wrong choice is a design bug.134135| Access pattern | Preferred structure |136|---------------|-------------------|137| Order-dependent iteration (ranking, priority) | `BTreeMap`, `Vec` sorted by key, `IndexMap` |138| Membership test only | `HashSet`, bloom filter (but beware false positives - see below) |139| Frequent insert + lookup by key | `HashMap` (if order doesn't matter) |140| Small fixed-size collections (< 16 elements) | Stack array, `SmallVec`, `ArrayVec` |141| Append-only log | `Vec` with `push` |142| Probabilistic membership (bloom filter) | Use ≥ 2 independent hash functions. A single-hash bloom filter has unacceptable false-positive rates at moderate load. Document the false-positive bound. |143144### How to check145146- For every `HashMap` in the diff: "Does any consumer iterate this and depend on order?" If yes, switch.147- For every `Vec`: "Is the capacity known?" If yes, use `with_capacity`.148- For every bloom filter: "What is the false-positive rate at expected load?" If > 1%, redesign.149150---151152## 6 - API surface and design deduplication153154| Check | Why |155|-------|-----|156| **No duplicate logic across layers** | If server and client both implement matching, both must call equivalent code, not independent reimplementations. Any behavioral change must update both. |157| **No duplicate calls in the same request** | If `match_route()` is called twice on the same path in one request flow, extract the result and pass it through. |158| **Free functions vs. methods** | If standalone helper functions duplicate methods on a type (e.g., `activateRoute(el)` and `el.activate()`), remove one. Keep the method; delete the free function. |159| **Caching static computations** | Parsing a template string into segments is pure. If the template set is static, parse once and cache. |160| **Minimal public surface** | Use `pub(crate)` for internal helpers. Every public API is a maintenance commitment. |161| **Error paths are actionable** | Errors must say what went wrong AND what the caller can do. "Parse error" is not actionable. "Invalid parameter name 'id-x' in path '/users/:id-x': only alphanumeric and underscore allowed" is. |162163---164165## 7 - Type safety, correctness idioms, and architectural rules166167### Rust - banned patterns168169| Pattern | Preference |170|---------|-----------|171| `unwrap()` / `expect()` in library code | **Banned.** Use `?` with typed errors via `thiserror`. |172| `unsafe` without `// SAFETY:` comment | **Banned.** Document the invariant upheld. |173| Recursion in core algorithms | **Banned.** Use iterative loops with an explicit stack. Recursion blows the call stack on deep/adversarial inputs and defeats branch prediction. |174| `as any` casts (in FFI or interop) | Minimize. Use typed wrappers or trait objects. |175| Raw pointer arithmetic | Prefer safe abstractions. Use `unsafe` only when necessary, with safety proof. |176177### Rust - required annotations178179| Pattern | Preference |180|---------|-----------|181| `#[must_use]` | Add to all public constructors, serialization/deserialization methods, and functions returning `Option`/`Result` whose value should not be silently discarded. |182| `#[allow(dead_code)]` | Remove the annotation AND the dead code. If the field/function is planned for future use, delete it now and re-add when needed (with a TODO referencing an issue). |183184### Rust - error handling precision185186| Pattern | Preference |187|---------|-----------|188| Silent fallbacks (`unwrap_or(0)`, `unwrap_or_default()`) | Replace with explicit error propagation. A silent default can mask a real mismatch (e.g., unbalanced start/end markers). If a default is genuinely correct, add a comment explaining why. |189| `Option<T>` return for lookups that can fail in multiple ways | Use `Result<T, E>` with a typed error enum distinguishing missing-key, invalid-path, type-mismatch, etc. Collapsing all failures to `None` makes debugging impossible. |190| Error variants that abuse a generic `Validation(String)` | Add dedicated variants (`Decode`, `Encode`, `MissingField`) so callers can match programmatically. |191| Byte vs char index confusion | When scanning strings with `chars()` but slicing with byte indices, panics occur on non-ASCII input. Always use `char_indices()` or work exclusively with byte offsets. |192193### Rust - numeric precision194195| Pattern | Preference |196|---------|-----------|197| Forcing all numbers through `f64` for comparisons | Preserve exact integer comparisons (`i64`/`u64`) when both operands are integral. Only fall back to `f64` for genuine floats. Large integers above 2^53 lose precision in `f64`. |198199### Rust - build-time validation200201| Pattern | Preference |202|---------|-----------|203| Deferring validation to render-time | Validate at parse/build time and fail fast. If a route references a component that doesn't exist, or a path is empty, catch it during parsing - not when a user hits the route. |204| Accepting invalid inputs that pass through | If a `<route>` element is missing `path` or references an unknown component, the parser must reject it with an actionable error. |205| `bytes[i] as char` on UTF-8 strings | This silently corrupts multi-byte characters. Use `str` slicing or `char_indices()` to preserve UTF-8 correctness. |206207### Rust - diagnostics & error presentation208209See `skills/diagnostics/SKILL.md` for the full workflow. On review, check:210211| Pattern | Preference |212|---------|-----------|213| `panic!`/`unwrap` on recoverable (template/CLI/state) input | **Banned.** Return `Result` - `panic = "abort"` kills FFI/WASM/Node hosts. Authoring mistakes return `ParserError::Template(Box<Diagnostic>)`. |214| Authoring error without a stable `code`, location, or `help:` | A `Diagnostic` must carry a `diagnostic::codes` constant, `--> owner:line:column`, snippet, and an actionable `help:`. Codes are a **stable API** - flag renames. |215| Color/ANSI in a library, host, browser, or JSON channel | Color belongs **only** in `webui-cli` (`console::style()`). Libraries emit plain data; FFI/WASM/Node/browser get plain `Display`. Split terminal-vs-machine renderings (e.g. `RebuildError { display, message }`) rather than leaking ANSI. |216| A single SGR span wrapping newlines | Style each line independently - a span across `\n` bleeds when lines are re-prefixed (`[server]`). |217| Error construction inlined into a hot function | Mark error builders `#[cold]`/`#[inline(never)]`; keep the hot fast-path inlinable. Layout perturbation from cold error code is a real regression - confirm with `cargo bench` vs the base branch. |218| `serde_json::json!` macro in CLI output | Use the `serde_json::Map` API; the macro `unwrap`s internally and trips `disallowed_methods`. |219| Tests asserting on error prose | Assert on the stable `code` (and that JSON output contains no `\x1b`), not the human message. |220221### TypeScript222223| Pattern | Preference |224|---------|-----------|225| `(x as any)` | Use proper type narrowing, generics, or `unknown` with guards. |226| Untyped expando properties (`(el as any)._data = ...`) | Use `WeakMap<Element, T>` or typed fields on a subclass. |227| `var` | **Banned.** Use `const` by default, `let` only when reassignment is needed. |228| `== null` | Use `=== null \|\| === undefined` or optional chaining (`?.`). |229| Callback-based async | Use `async/await`. |230| `arguments` object | Use rest parameters (`...args`). |231| Loose union types (`state: object \| string`) | Use precise types. If Rust accepts `serde_json::Value`, define a recursive `JsonValue` type on the TS side. If plugin accepts `"fast"` only, use a string literal union. |232233---234235## 8 - Style, comments, and documentation236237| Check | Rule |238|-------|------|239| **Dead code** | Remove unused functions, imports, and variables. Don't comment them out. |240| **Commented-out code** | Delete it. Git has history. |241| **Obvious comments** | Remove comments that restate the code: `// increment counter` above `counter += 1`. |242| **Missing comments** | Add comments for non-obvious decisions: "We use BTreeMap here for deterministic iteration order." |243| **`TODO` / `FIXME` / `HACK`** | Acceptable only with an associated issue number: `// TODO(#123): switch to streaming parser`. |244| **Naming** | Rust: `snake_case` functions, `PascalCase` types, `SCREAMING_SNAKE` constants. TS: `camelCase` variables/functions, `PascalCase` classes/types. |245| **Magic numbers** | Extract to named constants with a doc comment. |246| **File length** | When a file exceeds ~400 lines, split by concern. |247| **Function arguments** | Max 5 parameters. Use a config/options struct beyond that. |248| **Unused dependencies** | Remove deps listed in `Cargo.toml` that are never imported. `anyhow` is for binary crates only - library crates use `thiserror`. |249| **Placeholder / no-op APIs** | If a public function always returns empty/default, either implement it or remove it behind a feature gate. Silent no-ops mislead callers. |250| **Tests that lock in wrong behavior** | A test named `test_feature_not_supported` that asserts `None` for spec-required behavior is a bug, not a test. Delete or fix it. |251252---253254## 9 - Server runtime, proxy, and discovery correctness255256When the change touches dev-server, API proxy, request handling, or package discovery:257258| Check | Why |259|-------|-----|260| **HTTP semantics** | `Accept` header parsing must respect quality weights (`q=`). Substring matching (`contains("application/json")`) is incorrect per RFC 7231. |261| **Status codes** | A "no match" must return 404, not 200 with empty body. Clients cannot distinguish "valid empty" from "not found" otherwise. |262| **Proxy header forwarding** | Forward a safe allowlist: `Authorization`, `Cookie`, `Accept`, `Content-Type`, `X-Request-ID`, etc. Don't forward `Host` or hop-by-hop headers. |263| **Path handling** | Paths with dots (e.g., `/api/v2.1/users`) are valid routes, not file requests. Don't use `contains('.')` as a file-vs-route heuristic. |264| **Resource reuse** | HTTP clients, TLS contexts, and connection pools are expensive to create. Reuse them across requests. |265| **HMR injection** | Search for closing tags case-insensitively. `</BODY>` and `</body>` are both valid HTML. |266| **Input validation** | CLI commands must validate that input paths are directories (not files) before proceeding. Actionable error early beats cryptic failure later. |267| **Cache invalidation scope** | Cache keys must include hashes/mtimes of ALL files that affect the cached result - not just `package.json`. If template HTML or CSS changes, the cache must invalidate. |268| **Cache write safety** | Use per-write unique temp file names (PID + random suffix) and atomic rename. Concurrent writers must not clobber each other's temp files. |269| **Symlink policy** | Package resolvers must allow workspace-linked packages (symlinks outside `node_modules`). Blocking all external symlinks breaks common monorepo patterns. |270| **File-size guardrails** | Discovery that reads arbitrary files must cap read sizes. A 100MB HTML file shouldn't cause OOM during component scanning. |271272---273274## 10 - Protocol and schema hygiene275276When the change touches `.proto` files, serialization, or the binary protocol:277278| Check | Why |279|-------|-----|280| **Field additions are backward-compatible** | New fields must have defaults that preserve old behavior. |281| **Removed fields use `reserved`** | Prevents accidental reuse of field numbers. |282| **Enum variants are exhaustive** | Match arms must handle unknown variants (use a default/unknown variant or `_` with a log). |283| **No unnecessary nesting** | Flat structures decode faster than deeply nested ones. |284| **Measure payload size** | Before/after byte counts for representative inputs. Extra fields add decode overhead even when empty. |285| **Prioritize decode speed** | Breaking field changes are allowed when they improve performance measurably. Remove unused fields and message shapes that add decode overhead. |286| **Cascade all layers** | Schema changes affect the whole stack: protocol -> handler -> FFI -> CLI. Update all in the same change. Run `cargo xtask build && cargo xtask test` to validate. |287| **Update DESIGN.md** | Protocol behavior changes must update the protocol sections of `DESIGN.md` in the same commit. |288289---290291## 10b - FFI boundary safety292293When the change touches `crates/webui-ffi` or C ABI signatures:294295| Check | Why |296|-------|-----|297| **`# Safety` doc on every `extern "C" fn`** | Documents the invariants the caller must uphold. |298| **`// SAFETY:` on every `unsafe` block** | Explains why the unsafe operation is sound. |299| **`catch_unwind` wraps all FFI bodies** | Panics from transitive deps (serde, prost) crossing the boundary are UB in unwind mode. On panic, set last-error and return null. |300| **Validate all foreign inputs** | Null pointers, invalid UTF-8, out-of-range values - check before dereferencing. |301| **No `unwrap()` or `expect()`** | Anywhere in the FFI code path, including error-handling helpers. |302| **Minimal stable surface** | Prefer opaque pointers and integer error codes over exposing Rust layouts. |303| **Header sync** | If any `#[no_mangle]` signature changes, verify `crates/webui-ffi/include/webui_ffi.h`. |304305---306307## 11 - Documentation sync308309Every behavioral change must include corresponding documentation updates. Missing docs are a review finding.310311| Check | Why |312|-------|-----|313| **DESIGN.md updated** | If the change modifies public APIs, protocol fields, behavioral contracts, error variants, or SSR markers, `DESIGN.md` must be updated in the same commit. |314| **User-facing docs updated** | If the change affects CLI flags, template syntax, component authoring, routing, or integration behavior, `docs/` must be updated. |315| **AI reference updated** | If the change affects anything a code-generation AI would need to know, `docs/ai.md` must be updated. |316| **README links to docs portal** | Package READMEs should defer to the docs portal, not duplicate content. |317| **No stale examples** | Code examples in docs must use current API signatures, flag names, and marker formats. |318| **docs build passes** | `cd docs && pnpm build` must succeed (catches broken links, unescaped `{{`, missing pages). |319320See `docs-sync` skill for the full file mapping of what-changed to which-docs-to-update.321322---323324## Review output format325326When producing a code review, report findings as:327328```329### ISSUE-NNN: <short title>330331**Severity**: 🔴 Bug | 🟠 Security | 🟡 Performance | 🔵 Design/Style332**Files**: `path/to/file.rs` (lines N-M)333**Description**: What is wrong, with a code snippet showing the problem.334**Impact**: What breaks or degrades, and under what conditions.335**Fix**: Concrete suggestion, with a code snippet when helpful.336```337338Each finding must be **self-contained** - readable without context from other findings. This allows direct conversion to GitHub issues.
Microsoft