FabioFiorita/code-quality-review

Code Quality Review

A systematic audit framework for the Tastik codebase. Approach every review as a tech lead who owns long-term maintainability — not as a feature developer shipping fast.

Audit Process

1. Scope — Identify what to review (single file, folder, feature, or full sweep)
2. Explore — Read the code before forming any opinion (never suggest changes to unread files)
3. Classify — Tag each finding with a severity and category (see below)
4. Prioritize — Fix high-severity issues first; low-severity can be a backlog item
5. Fix — Apply changes one logical unit at a time; run bun typecheck && bun check:write after each batch
6. Verify — Run affected tests; add missing tests for changed logic

---

Severity Levels

| Level | Meaning | |-------|---------| | P0 | Bug risk, data loss, security issue — fix immediately | | P1 | Maintainability blocker, violates project conventions — fix in this session | | P2 | Refactor opportunity, DRY violation, premature complexity — prioritize | | P3 | Style/naming/minor consistency — fix opportunistically |

---

Review Checklist

1. Architecture & SOLID

• Single Responsibility: Each file/hook/function does one thing. Components only handle rendering; hooks handle logic.
• Open/Closed: Prefer composition over modifying existing components. Extend through props/slots.
• Dependency Inversion: Components depend on abstractions (hooks), not concrete implementations.
• No business logic in components: Derived state, formatting, error handling — all in hooks under src/hooks/.
• Action hooks under src/hooks/actions/: wrap useMutation with loading state, error handling, toast. Never return the raw mutation result.
• Query hooks under src/hooks/queries/: return only the data shape the UI needs; expose queryOptions for loader preloading.

2. DRY & Duplication

• Identical JSX structures appearing 3+ times → extract to a shared component
• Same logic duplicated across hooks → extract to a src/lib/utils/ pure function
• Same validation in multiple places → single src/lib/validation/ schema
• Inline constants that repeat → move to src/lib/constants/
• Inline types duplicating convex/schema.ts validators → use Infer<typeof validator> and place in src/lib/types/

3. Data Fetching — TanStack Query + Convex

Correct pattern (always prefer):

// hooks/queries/use-list-items.ts
export function listItemsQueryOptions(listId, includeCompleted, tagId?) {
  return convexQuery(api.items.getListItems, { listId, includeCompleted, tagId });
}

export function useListItems(listId, includeCompleted, tagId?, opts?) {
  const { data } = useQuery({
    ...listItemsQueryOptions(listId, includeCompleted, tagId),
    enabled: opts?.enabled !== false,
  });
  return data; // never return the full useQuery result
}

Anti-patterns to find and fix:

• useState + useEffect for fetching — replace with useQuery(convexQuery(...))
• Direct useQuery calls in components — move to a hook in src/hooks/queries/
• Returning raw useQuery result from a hook — return only .data
• useSuspenseQuery inside non-Suspense boundary without an error boundary
• Missing queryOptions export (needed for route loaders and SSR prefetch)
• Route loaders not calling context.queryClient.prefetchQuery(...) for secondary data

4. SSR & Route Loaders (TanStack Start)

• Critical data (the entity the page renders) → ensureQueryData in the loader (blocks navigation, shows pendingComponent)
• Secondary data (collaborators, tags) → fire-and-forget prefetchQuery (non-blocking)
• Never await non-critical prefetches in loaders — it blocks navigation unnecessarily
• Route params validation with parseConvexId before hitting the database
• Invalid params → throw redirect({ to: "/" }) immediately in the loader

loader: async ({ context, params }) => {
  const listId = parseConvexId<"lists">(params.listId);
  if (!listId) throw redirect({ to: "/" });

  const entity = await context.queryClient.ensureQueryData(entityQueryOptions(listId));
  if (!entity) throw redirect({ to: "/" });

  // fire-and-forget secondary data
  context.queryClient.prefetchQuery(secondaryQueryOptions(listId));

  return { listId };
},

5. React Memoization — Avoid Over-Engineering

Apply the react-memoization-guide skill rules. Key rules for Tastik:

• Do NOT memoize values that are cheap to recompute (primitives, simple derives)
• Do NOT memoize because "it might be slow" — measure first
• Do useCallback for stable references passed to child components with React.memo
• Do useMemo for expensive computations (sorting large arrays, complex filtering) — benchmark first
• Remove gratuitous useMemo wrapping simple object literals or array maps

6. Component Design

• One exported component per file; small helpers (<30 lines) can be unexported in the same file
• Components should not contain async/await logic directly — delegate to hooks
• Props interfaces: prefer explicit named types over React.FC<{...}> inline objects
• Avoid prop drilling > 2 levels — use composition or a context hook
• data-testid on all interactive elements and key UI landmarks (required)
• Use cn() from @/lib/utils/cn for conditional classes; never string concatenation
• Design tokens only: primary, muted, muted-foreground, foreground, background. No arbitrary Tailwind values.

7. Testability

Untestable code smells:

• Business logic inside components (can't test without rendering)
• Side effects mixed with pure computation
• Direct import of singletons without injection points
• Hooks that do too many things (single responsibility applies to hooks too)

For every non-trivial hook or util, check:

• Does a test file exist?
• Does it cover the happy path, error path, and edge cases?
• Are Convex mutations mocked using vi.fn() returned from the hook file's __mocks export?
• Are useNavigate, useCurrentUser, and other framework hooks mocked via src/lib/helpers/mocks?

See testing-setup skill for test infrastructure details.

8. Convex Backend Quality

Apply convex-guidelines skill. Key checks:

• Validators on all public function args and return types
• requireAuth / requireListAccess called at the top of every authenticated function
• No ctx.db.query without an index (full table scans forbidden in production)
• internalMutation / internalQuery for functions not meant to be called from client
• Error messages use ConvexError for user-facing errors, not generic throws
• Tests exist in convex/tests/ for all non-trivial mutations and queries

9. TypeScript Hygiene

• No any — use unknown and narrow, or use the correct generated Convex types
• No as casts without a comment explaining why it's safe
• No // @ts-ignore without justification
• Types inferred from schema validators via Infer<typeof validator> — no manual duplication
• Prefer type over interface for local types (consistency with project style)

10. Code Smells Catalogue

| Smell | Fix | |-------|-----| | useState(false) + useEffect fetching | Replace with useQuery(convexQuery(...)) | | Long component (>200 lines) | Extract sub-components or move logic to hooks | | Deeply nested ternaries in JSX | Extract to a variable or helper function | | Magic numbers/strings inline | Move to src/lib/constants/ | | console.log left in code | Remove or replace with proper error tracking | | Repeated try/catch blocks with same pattern | Extract to useHandleMutationError or useManagedAction | | window.location.href for navigation | Replace with useNavigate() from TanStack Router | | Promise ignored without error handling | Use useManagedAction or proper .catch() | | Mutation called directly in component | Move to an action hook in src/hooks/actions/ | | Unused imports/variables | Remove | | Optional chaining on things that can't be null | Remove unnecessary ?. |

---

Full Codebase Sweep Workflow

When asked to do a full sweep (automation-friendly):

1. Map the surface area

   Glob src/components/**/*.tsx → list all components
   Glob src/hooks/**/*.ts → list all hooks
   Glob convex/*.ts → list all Convex functions
   

2. Prioritize by risk — start with hooks/actions (most business logic), then routes (SSR/loader), then components

3. Per-file audit loop:

   • Read the file
   • Apply the checklist above
   • Classify findings with P0–P3
   • Apply P0/P1 fixes immediately
   • Log P2/P3 findings for a follow-up pass

4. After each batch of changes: bun typecheck && bun check:write

5. Test coverage check:

   Glob src/hooks/actions/*.ts → check each has a matching *.test.ts
   Glob convex/tests/*.test.ts → verify coverage of backend mutations
   

6. Summarize findings in a structured report:

   • Files changed
   • P0/P1 issues fixed
   • P2/P3 backlog items (with file:line references)
   • Test coverage gaps identified

---

Quick Reference: Tastik Stack Conventions

| Concern | Where | |---------|-------| | Convex data types | Infer<typeof ...Validator> from convex/schema.ts | | Query hooks | src/hooks/queries/ — export hook + queryOptions | | Action hooks | src/hooks/actions/ — wrap mutation, return { action, isPending } | | UI state hooks | src/hooks/ui/ | | Pure utils | src/lib/utils/ (no React, no side effects) | | Constants & static data | src/lib/constants/ | | Form validation schemas | src/lib/validation/ | | Domain types | src/lib/types/ | | Route loaders | src/routes/ — use ensureQueryData for critical, prefetchQuery for secondary | | Backend logic | convex/ — lib helpers in convex/lib/ | | Tests (frontend) | Co-located *.test.ts(x) or in src/hooks/.../*.test.ts | | Tests (Convex) | convex/tests/ |

---

References

• references/anti-patterns.md — extended examples of each anti-pattern with before/after code
• Use convex-guidelines skill when auditing Convex backend files
• Use testing-setup skill when writing missing tests
• Use react-memoization-guide skill for memoization decisions