Code Review

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