shipshitdev Structural Review
Opinionated structural and maintainability review rubric. Report-only — this skill produces findings; it does not mutate files or open PRs.
Core Principle
Every structural finding must answer: "What exactly should the author delete, collapse, or rename — and which specific file/line is the canonical landing zone?" Vague nits ("this could be cleaner") are noise. High-conviction, actionable findings only.
This rubric is explicitly complementary to /code-review. Do not re-flag correctness bugs or repo instruction-file rule violations already covered there. Own the orthogonal structural/maintainability/devex dimensions. Use codebase-design vocabulary (module, interface, depth, seam) when judging whether a change made a module deeper or shallower.
Primary Review Questions
Ask these first, before scanning for individual issues:
- Does this PR make the codebase smaller or larger? If larger: is each new abstraction earning its keep, or is it complexity added on faith?
- Where is the decision logic? Is it in the canonical layer (server action, domain service, hook) or scattered across render trees and event handlers?
- Can a reader understand this module in isolation? Or does understanding it require tracking state/logic across 4+ files?
- Is every new type structurally placed? Or are bare
unknown, andas Xcasts hiding future breakage, and are interfaces inlined where they should be in*.types.ts? - Does the PR leave the stack cleaner than it found it? Tailwind v3 debris,
npminvocations, raw HTML in a shadcn project,middleware.tsin a Next.js 16 app — these are regressions, not style notes.
Axes
1. File Size — The ~1000-Line Rule
A file crossing ~1000 lines is a presumptive blocker. It is not a hard numeric law but a strong prior that the module has taken on too many responsibilities.
Flag when:
- Any file touched by this PR is now >1000 lines.
- The PR itself adds enough lines to push a previously-marginal file over the threshold.
Preferred remedy: Split into colocated slices — *.service.ts, *.queries.ts, *.types.ts, *.hooks.ts — before the PR lands. Not after.
Phrase: "File is now 1 180 lines. Split the query layer into *.queries.ts before this ships — the 1 000-line rule exists exactly for PRs like this one."
Severity: Blocker.
2. Abstraction Earns Its Keep
Every new function, hook, class, or file must justify its existence. Thin wrappers, identity helpers, and passthrough re-exports fail this test unconditionally.
Flag when:
- A new helper is a one-liner rename or a call-through with no added invariants.
- A new file re-exports a symbol from another file without transformation.
- A hook wraps a single
useStatewith no co-located logic. - An interface is defined once and used once at the call site — inline it.
Preferred remedy: Delete the abstraction; inline the real symbol at the call site. Bun + TS path aliases make the original easy to reach.
Phrase: "This helper is an identity function over formatDate — inline it and delete the file. Code-judo."
Severity: Request Changes.
3. Spaghetti Branching
Ad-hoc forks bolted onto existing flows are the primary mechanism by which a clean codebase becomes unmaintainable.
Flag when:
- The same
if (isAdmin)/if (featureFlag)/if (isPremium)fork appears in 2+ files changed by this PR. - A new branch is added to an existing render function that already has 3+ conditional paths.
- Feature-flag forks are spread across component render paths instead of being resolved at a single decision point.
Preferred remedy: Collapse all branches into one canonical decision point — a hook for client state, a server action or domain service for mutations. Branches then disappear from the components.
Phrase: "Three if (isAdmin) forks scattered across the render tree. Collapse the decision into one hook or server action; the branches can then disappear from the component."
Severity: Request Changes.
4. Canonical Layer Discipline
In this stack, logic belongs in a specific layer. Violating the layer model is a structural defect, not a preference.
Layer map:
- Mutations → server actions or domain services. Not in raw
<form onSubmit>, not inuseEffect. - Derived client state → hooks. Not recomputed inline in JSX.
- Interactive elements → shadcn/ui primitives or
packages/uicomponents. Not raw<button>,<input>,<dialog>. - Route middleware →
proxy.ts. Notmiddleware.ts(Next.js 16).
Flag when:
- A mutation is wired directly to a DOM event handler when a server action exists in the project.
- A computation that could be a hook is inlined in JSX and repeated in 2+ components.
- A raw HTML element appears in a file that already imports from
packages/uior shadcn.
Phrase: "<button> in a component that already imports Button from packages/ui — swap it; raw HTML is a regression in this codebase."
Severity: Request Changes; Blocker when the element is a raw interactive element in a shadcn/packages/ui project.
5. Type Structural Discipline
This axis covers structural placement and shape issues — not any existence, which is owned by the /code-review harness.
Violations (flag all):
- Bare
unknownwithout an adjacent type guard — this is deferredany, structurally equivalent to leaving the shape unresolved. as Xcasts without a comment explaining why the type system cannot infer the narrowing.- Inline interfaces defined inside a component or service file — place in colocated
*.types.tsorpackages/types.
Preferred remedy: Define the shape in *.types.ts. Add the type guard at the boundary where the unknown enters.
Phrase: "Bare unknown without a type guard is deferred any. Define the shape in *.types.ts or add the guard here."
Severity: Bare unknown without guard → Blocker. Inline interface → Request Changes. as X without comment → Minor.
6. Non-Atomic Mutations
Sequential await calls that each write to the database are a correctness-adjacent structural defect. A crash between writes leaves the system in a half-written state.
Flag when:
- Two or more
await db.update()/await db.insert()calls appear in the same function without a wrapping transaction. - A function performs a write, then calls an external service, then does a second write — with no compensation/rollback path.
Preferred remedy: Wrap in a DB transaction or model as a single atomic write. If the external call cannot be inside a transaction, add explicit compensation logic.
Phrase: "These two await db.update() calls are non-atomic. A failure between them leaves the row in an invalid state — wrap in a transaction or model as a single atomic write."
Severity: Blocker.
7. Sequential Orchestration Smell
Functions that orchestrate 5+ sequential steps with no intermediate abstraction are a future maintenance hazard. They are hard to test, hard to reuse, and hard to understand in isolation.
Flag when:
- A new function has 5+ sequential
awaitcalls with no intermediate named steps or helper functions. - A route handler or server action reads, transforms, validates, writes, and notifies — all inline.
Preferred remedy: Extract named phases. Each phase is testable in isolation. The orchestrator becomes a readable list of intents.
Severity: Request Changes.
8. Stack Hygiene (Hard Regressions)
These are not style notes — they are regressions introduced by the PR that must be fixed before merge.
| Issue | Rule | Phrase |
|---|---|---|
middleware.ts created or left in place |
Next.js 16 renamed the entry point to proxy.ts; the old name is silently ignored |
"This is middleware.ts. Rename to proxy.ts — Next.js 16 renamed the entry point." |
tailwind.config.ts or tailwind.config.js added |
v4 uses CSS-based config (@theme in CSS); JS config is v3 |
"tailwind.config.ts created by this PR — that's v3. Delete it and move the theme tokens into the CSS @theme block per v4." |
@apply directive or bg-opacity-* class added |
v3 patterns; must migrate to v4 slash syntax | Cite the exact line and the v4 equivalent. |
npm run, npx, yarn add, pnpm exec in scripts, CI YAML, or docs |
Bun is the only package manager; replace with bun run / bunx / bun add |
"npx prisma migrate two lines below must be bunx prisma migrate." |
Severity: All Blocker.
9. Design Purity — Same Behavior, Cleaner Shape
The sharpest structural question is not "is this correct?" but "could this same behavior be expressed with materially less structure?" A change that ships the right behavior on top of avoidable complexity is a missed simplification, and missed simplifications compound.
Flag when:
- A new state machine, config object, or branch tree encodes a decision that a single derived value or default would express.
- A special case is added where collapsing it into the general path (a sensible default, an early return) would delete the branch entirely.
- A refactor reshuffles code without reducing the number of moving parts a reader must hold at once.
Preferred remedy: Reframe the state model. Collapse the special case into a default. Deletion and collapse beat polishing — "code-judo" the complexity category out of existence rather than tidying it.
Phrase: "This three-state flag collapses to one derived boolean — the same behavior with a whole branch deleted. Reframe rather than polish."
Severity: Request Changes (Blocker when the simpler form also removes a correctness footgun).
10. Directness vs Magic
Prefer the obvious mechanism over the clever one. Over-generic abstractions, hidden assumptions, and indirection that hides control flow cost more to read than they save to write.
Flag when:
- A generic/parameterized mechanism is introduced for a single concrete caller ("speculative generality").
- Behavior depends on a hidden assumption — implicit ordering, a global, a naming convention, reflection/metaprogramming — that a reader cannot see at the call site.
- Indirection (dynamic dispatch, event indirection, deep config) replaces a direct call without an invariant that earns it.
Preferred remedy: Inline to the direct form for the one real caller. Make the assumption explicit at the boundary, or remove the magic.
Phrase: "Generic registry for a single handler — delete it, call the handler directly. Add the abstraction back when the second caller actually arrives."
Severity: Request Changes.
What to Ignore
- Style preferences not rooted in a structural defect (indentation, naming micro-variations).
- Correctness bugs and test coverage gaps — these belong in
/code-review. - Security issues — these belong in
/security-audit. - Repo instruction-file rule violations already covered by the correctness review harness.
- Documentation gaps (inline docs, README updates) — off unless explicitly requested.
- Coverage percentage floors — flag missing tests for new critical paths only, not a specific number.
Output Format
Order findings by severity:
## Structural Review
### Blockers (must fix before merge)
[File-size violations, non-atomic mutations, stack regressions (middleware.ts, tailwind config, npm/yarn usage), bare unknown without guard, design-purity simplifications that also remove a correctness footgun]
### Request Changes (significant structural debt)
[Abstraction-earns-keep failures, canonical-layer violations, spaghetti branching, sequential orchestration smells, inline interfaces, missed design-purity simplifications, speculative generality / magic indirection]
### Minor (low-debt, flag for awareness)
[as X casts without comments, small layer suggestions with obvious inlines]
### Approved Axes
[Axes with no findings — confirm clean]
Include for each finding:
- File + line range (exact, not approximate).
- One sentence stating the structural defect.
- One sentence stating the specific remedy.
- A reviewer phrase (use the examples above or adapt them to the specific code).
Approval Bar
Blocker and request-changes criteria are embedded in each axis above. When ambiguous, default to Request Changes — the ambiguity tax is paid by the author, not the codebase.