Code Review Standards

Last Updated: 2026-02-16 Status: Active

Purpose

This document defines the code review criteria for all Zus contributions. Reviews assess compliance with project architecture, coding standards, and documentation expectations. Every issue that passes through the "Review" column on the project board must satisfy these standards before moving to "Done."

Core Policy: No Technical Debt

Zus operates under a no technical debt policy. Every defect, shortcut, and deviation from standards is addressed when found, not deferred.

• Don't walk by garbage. If we see a problem during review, e.g., hardcoded colors, DOM coupling, missing tokens, suboptimal patterns, then it gets fixed in the current issue or a new issue is opened immediately. Unless there is a critical deadline or emergency fix that must be released urgently, we don't defer issues for later.
• Minor issues are not optional. Minor findings are low priority relative to blockers and major issues, but they are still required fixes. "Low priority" means they can be fixed quickly and should be, not that they can be ignored.
• No severity level means "defer indefinitely." Blockers and majors block the issue from Done. Minors must be fixed (in the current issue or a tracked follow-up) before the related code area is considered complete. Notes are informational observations that may inform future design but do not require immediate action.

This policy keeps the codebase clean as a baseline condition, not as a periodic cleanup effort.

Review Checklist

1. Architectural Integrity

• ZUI framework compliance — Shell layout uses zui view descriptors and the panel registry. Components that represent content areas should be registered panels (registerPanel), not ad-hoc sub-components rendered by other panels. Layout decisions belong in .zui.ts view definitions, not in panel code.
• Observable workspace pattern — Shared UI state lives in workspace.ts. Panels read via useObservable(workspace) during render; writes go directly to workspace.* in callbacks. No useState for state that crosses panel boundaries.
• Plugin architecture — Panels, status providers, commands, and actions are registered through the plugin/registration API. Avoid hardwiring components that should be pluggable.
• Transition orchestration — All animated transitions read a single phase clock (workspace.transition.phase). Any new transition code must participate in the existing orchestration model — it must not create independent timers, per-component animation state, or standalone hooks that run outside the phase clock. The gate question: "Does this create a new clock, or does it participate in the existing one?" If it creates a new clock, it must be rejected. See Transition Architecture.
• Animation fill mode — All CSS animations participating in transitions must use animation-fill-mode: both — never forwards alone. The backwards component applies the from keyframe before the first paint frame, preventing single-frame flashes. The forwards component holds the to keyframe after completion. Using only forwards leaves the element's default state exposed for one compositor frame between DOM insertion and animation start.
• Stable tree positions — Components that persist across state transitions (e.g., DocumentBrowserPanel in ContentPanel) must remain at the same React tree position in all render paths. Multiple return branches with the same component at different positions cause React to unmount and remount the instance, producing a one-frame flash. Use a single return with conditional visibility instead of branching returns.
• Separation of concerns — View definitions declare structure. The framework handles behavior. The theme handles appearance. Panels handle content. These layers should not leak into each other.
• Data model alignment — Documents are markdown strings. Storage is IndexedDB. Data loading should be centralized, not duplicated across panels.

2. Theming Compliance

• CSS custom properties for all colors — Never hardcode colors (#F5A623, rgba(255, 255, 255, 0.4), etc.) in inline styles or component code. Use var(--color-*) tokens from the theme system. If a token doesn't exist, define one.
• CSS custom properties for typography — Font families, sizes, weights, and spacing should reference theme variables (var(--font-family-sans), var(--font-size-*), etc.).
• No inline styles for themeable properties — Use CSS classes and custom properties for anything that should change with the theme. Inline styles are acceptable only for truly dynamic, computed values (e.g., width from drag resize).
• Accent color from theme — The accent/brand color must come from a CSS variable, not be hardcoded. The value may change per theme.

3. TypeScript & React Best Practices

• TypeScript erasable subset — No enums, no decorators, no namespace. Use type and interface for type definitions, union types for variants.
• Stable references — Callbacks passed as props should use useCallback. Objects/arrays created during render that are used as deps or props should be stabilized with useMemo.
• Effect cleanup — All useEffect hooks that create subscriptions, timers, or async operations must return cleanup functions (cancellation flags for async, unsubscribe for listeners).
• No render-time allocation of throwaway functions — Noop functions, event handlers, and formatters should be module-level constants or memoized, not recreated every render.
• Parallel async operations — When loading multiple independent resources, use Promise.all() or Promise.allSettled() instead of sequential await in a loop.

4. Code Quality

• DRY within reason — Identical logic in multiple files (e.g., the same IndexedDB loading sequence) should be extracted to a shared hook or utility. But don't over-abstract for a single use.
• No magic strings for cross-boundary identifiers — Panel IDs, folder IDs, document prefixes, and command IDs used in multiple files should be defined as named constants in a shared location.
• Fragile coupling — Watch for coupling to implementation details of other layers (e.g., filtering documents by ID prefix instead of metadata). Flag as design smell even if acceptable short-term.

5. Documentation (Literate Coding Standard)

This project targets a documentation level suitable for teaching. Code should be understandable by both AI and human collaborators encountering it for the first time.

• File-level docstring — Every file starts with a block comment explaining: what the module does, its role in the architecture, its data sources, and its key dependencies.
• Type documentation — Interfaces and type aliases that represent domain concepts should have doc comments explaining the concept, not just the shape.
• Extension point documentation — Where the code is designed to be extended (new virtual node types, new folder kinds, new panel types), document how. Include "To add X, do Y" comments.
• Non-obvious logic — Any conditional, filter, or mapping that encodes a business rule must have a comment explaining the rule.
• Pure UUIDs — All entity IDs must be pure UUIDs (crypto.randomUUID()). Never encode semantics in ID prefixes. Filtering uses metadata fields or explicit key stores, not ID parsing.
• Architecture relationship comments — When a component participates in a larger pattern (master-detail, observable → render, panel registry), explain how this piece fits.

6. Pointer Event Interactions

• Pointer Events over HTML5 DnD — Custom drag interactions use the Pointer Events API for full control over drag image, thresholds, and animation. Never use HTML5 dragstart/dragover/drop.
• Native drag suppression — <a> and <img> elements are natively draggable. Any custom drag on these elements must set draggable={false} and onDragStart={preventDefault} to prevent browser hijacking (Chrome tab-splitting, bookmark creation).
• Pointer capture vs hit-testing — setPointerCapture directs all events to the capturing element, which prevents elementFromPoint from seeing elements underneath. Release capture before hit-testing drop targets.
• Click suppression after drag — Browsers fire a click event after pointerup. After a drag or marquee gesture, this click must be suppressed via a module-level flag to prevent unintended actions (clearing selection, opening documents).
• Selection state in workspace — Multi-select state (selectedDocIds) belongs in workspace.document, not local component state, so drag hooks and multiple panels can read it. Selection must clear on: folder change, Escape key, plain click on empty area, and successful drop.

7. Accessibility & UX

• Keyboard navigation — Interactive elements (folders, buttons, cards) should be focusable and operable via keyboard.
• ARIA roles — Tree views should use role="tree", role="treeitem", aria-expanded. Lists should use semantic <ul>/<li> or equivalent ARIA.
• Focus management — Selection changes should move focus appropriately. Focus outlines must not visually double up with selection borders (suppress outline on .selected elements).

Severity Levels

Per the no-technical-debt policy: blockers, majors, and minors all require resolution. The distinction is urgency (blockers/majors gate the current issue; minors can be a fast follow-up), not whether they get fixed.

Pre-Review Gate: npm run check

Before moving any issue to Review, the dev must run npm run check and confirm it passes. This script runs the full build (tsc -b + Vite) and all tests (vitest run). A clean check is a prerequisite — do not submit for review with a broken build or failing tests.

npm run check

If check fails, fix the errors before submitting. The PM/Architect will reject any review submission where npm run check does not pass.

Review Workflow

1. Dev runs npm run check and confirms it passes
2. Dev moves issue to Review column
3. PM/Architect reads changed files and evaluates against this checklist
4. Findings are posted as a comment on the issue
5. If no blockers/majors/minors: Move to Done
6. If blockers or majors exist: Leave in Review for fixes in the current issue
7. If only minors exist: Move to Done only if a follow-up issue is opened immediately to track the fixes. Otherwise, leave in Review for fixes in the current issue.
8. Tony provides final visual/UX sign-off independently

This is a living document. Update as standards evolve.