Code Review

Code review

Author: Bruno Balderrama (bmbalderrabano@gmail.com). Included in this collection with the author's permission.

These modular checks are also wired into the Standards axis of the `review` skill (/fly-review) when the diff touches UI/React files. Use this skill standalone for a develop-based UI audit; use review for the full two-axis Standards + Spec pass.

Goal

Produce a text-only audit of changes versus `develop` (never main as the comparison base). The user decides what to fix; do not apply edits unless they explicitly ask.

Modular checks

Apply every checklist file below against the diff. When adding a new theme, add checks/<id>.md and a bullet here.

Read each linked file before grading that category.

Step 1 — Scope the diff (repo root)

Base branch: develop (prefer origin/develop if local develop is missing or stale).

Suggested commands (read-only):

git fetch origin develop 2>$null
git branch --show-current
git log -1 --oneline origin/develop 2>$null; if (-not $?) { git log -1 --oneline develop }

Primary range (commits on this branch vs develop):

git diff origin/develop...HEAD

If origin/develop is unavailable, use develop...HEAD.

Include uncommitted changes in the audit when relevant:

git diff origin/develop

(or git diff develop)

Use the combined picture: symmetric range for “what the branch adds,” two-arg diff when local working tree matters. If both are empty, say so and stop.

Large changesets: use git diff --stat / path-scoped git diff origin/develop...HEAD -- path to stay thorough.

Step 2 — Grade each finding

For each issue, output:

• File (and line if visible in diff hunk)
• Check (ID from the table)
• Severity: low | medium | high (user impact / maintenance cost if ignored)
• Confidence: low | medium | high (how sure you are this is a real problem, not a false positive)
• Note: one or two sentences; optional suggestion (still no edits)

Severity guide

• high: Violates an explicit project rule, likely bugs, or entrenches architecture debt (e.g. legacy under system/).
• medium: Noise, inconsistency, or extra maintenance; should usually fix before merge.
• low: Style preference or uncertain redundancy; mention briefly.

Confidence guide

• high: Clear pattern in the hunk (e.g. cn("a b c") with no variables).
• medium: Likely issue; parent component not in diff.
• low: Needs design context; flag as “verify manually.”

Step 3 — Output shape (mandatory)

Plain text (or markdown) in this order:

1. Summary: 2–4 bullets (branch, base, scope used, rough counts). 2. Findings: grouped by severity (high → medium → low) or by file—pick one and stay consistent. 3. Clean pass: list check IDs with no issues as “OK” or “not applicable.” 4. Optional follow-ups: items that need runtime/design verification.

No auto-fixes, no commits, no PR creation.

Constraints

• Do not use main as the diff base unless the user explicitly overrides.
• Do not treat this audit as blocking CI; it complements human review.
• Prefer evidence from the diff; when inferring from incomplete context, lower confidence.
• Be extremely succinct and prioritize low token consumption.

Check: class-constants

What to flag

• const / let whose value is primarily a Tailwind class string (or array/object of class strings) used to avoid repeating markup—especially when the constant is only referenced once or twice.

Project expectation: repeat the class string at the call site when duplication is minor; extract a named component or CVA-based variant when abstraction is warranted—not a bare string constant.

Severity / confidence

• medium for new const styles = "..." or const X_CLASSES = "..." in TSX.
• low if the constant is shared across many sites in the same file and a component split is a larger refactor—suggest follow-up.

False positives

• cva(...) definitions (those are intentional variant systems).
• Non-CSS string constants (URLs, keys, copy).

Check: cn-static

What to flag

• cn(\...\) or cn("...") where all arguments are static string literals with no variables, no conditional expressions, and no className prop merge.

Preferred: put the string directly on className="..." (or template literal if needed for readability only when it includes expressions).

Keep cn() when

• Merging user/forwarded className with defaults.
• Conditional classes: cn(base, isActive && "active", className).
• Dynamic segments from props/state.

Severity / confidence

• low–medium by default (ergonomics and consistency).
• high only if static cn is used pervasively in new code in the same file (noise / convention break).

False positives

• cn() wrapping only to satisfy a typing quirk—rare; mention low confidence if unsure.

Check: inline-jsx-constants

What to flag

• A variable holding JSX (const Foo = <div>...</div>) that is only used once—or used as a stand-in for a tiny fragment that could live inline in the return tree.

Preferred: inline the JSX in return ( or extract a real function Subname() subcomponent when reuse or clarity warrants (per project UI rules: JSX subcomponents use function, not const arrows—see repo ui-component-spec).

Severity / confidence

• medium for needless indirection that hurts scanability.
• high if the pattern hides conditional logic or hooks incorrectly (rare—raise only with evidence).

Reference anti-pattern

Holding a block of JSX in a const then placing it under the return—integrate into the tree or promote to a proper function component.

False positives

• JSX assigned for arrays (items.map-style data) or legitimate reuse in multiple branches.

Check: legacy-in-system

What to flag

Any import in `src/components/system/** (or paths aliased as @/components/system/...) that resolves under **components/legacy/** or matches @/components/legacy`.

Rationale: legacy is slated for removal; system is the supported design-system surface. New coupling blocks deprecation.

Severity / confidence

• high for new or expanded legacy imports in system/ files.
• medium if the diff only touches an existing line—still call out debt.

Evidence

Quote the import line and both path segments (system + legacy).

False positives

• Imports from `system` into legacy (discouraged but opposite direction—optional note, lower severity).
• Non-component paths; adjust if the repo adds exceptions (document in this file when they exist).

Check: single-responsibility

What to flag

• A form component/file that also owns unrelated concerns: routing, global query orchestration, unrelated modals, or feature-level state that could live in a parent or hook next to the page.
• A list view that embeds heavy mutation / fetch logic that is not about rendering the list (e.g. whole “create assistant” flow inside the list component).
• Pages that mix many unrelated domains without clear composition boundaries.

Severity / confidence

• medium–high when the diff adds tight coupling (new imports of routers, mutations, or cross-feature stores into a presentational leaf).
• low when moving a few lines—might be acceptable; note “watch boundary.”

False positives

• Small local UI state (open/close, field focus) inside a form—expected.
• Co-located handlers that only serve the form’s submit/validate flow.

Guidance phrase for the audit

“Lift orchestration up; keep this component about one UI responsibility (form | list | card | layout).”

Check: tailwind-bloat

What to flag

• className strings with many utilities (rough guide: ≥ ~8 distinct tokens) especially when wrapping shadcn/system components that already take className and merge variants.
• Repeated responsive stacks (sm: md: lg:) on leaf nodes where a parent already defines layout/spacing.
• Duplication of the same utilities on siblings that could share a wrapper (only when obvious in the diff).

Severity / confidence

• high: Conflicting or redundant utilities that suggest misunderstanding of the child API (e.g. full padding/typography on a Button that already sets them via variants)—only when the diff makes it obvious.
• medium: Long className on simple wrappers; likely trimmable after checking the design system.
• low: Slightly verbose but readable; optional cleanup.

Confidence: lower when the imported component’s defaults are not in the diff—say “verify against component defaults.”

False positives

• One-off complex layouts that truly need many utilities.
• cva() variant strings (review variant design, not raw count).

License

This skill was created by Bruno Balderrama (bmbalderrabano@gmail.com) and is included in this collection with the author's permission.

Collection maintainer: Ender Puentes <Endev/> — https://enderpuentes.com

MIT License

Copyright (c) Bruno Balderrama

Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.