code-review

name	code-review
description	Framework code review checklist - correctness, performance, concurrency, design, and style.

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.

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/SKILL.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.