Skip to content

Latest commit

 

History

History
792 lines (646 loc) · 40.6 KB

ts-react.md

File metadata and controls

792 lines (646 loc) · 40.6 KB

TypeScript + React Coding Rules

Design rules for TypeScript + React code, for people and for coding agents. The same rules drive the ts-react-linter-driven-development plugin; this file is the version you can read in ten minutes and keep open while you work, with or without the plugin. Import it from your CLAUDE.md or AGENTS.md, or read it once before your first PR.

1. Mindset

Before a rule fires, ask the question behind it:

  • Tell, don't ask. What will the caller do with the value it is requesting — and does that decision belong on the type that owns the value?
  • Make illegal states unrepresentable. Can this type hold a value its methods would have to defend against? Delete the possibility, not the symptom.
  • Parse, don't validate. Does this check produce a more-typed value (a parse function that returns the value or fails), or just a boolean the next caller must remember? Validation that returns proof is parsing; validation that returns advice is a latent re-check.
  • Every indirection must earn its keep. What does this indirection own — a validation, a decision, a second production implementation, a deleted duplication, a real race, a real escaping alias? If the answer is nothing, it is ceremony: delete it.
  • Duplication is far cheaper than the wrong abstraction. Is this extraction earning its indirection today, with the callers in hand — or is it a bet on imagined futures?
  • Three strikes and you refactor. How many real occurrences exist right now? One is an instance, two is a coincidence, three is a pattern.
  • If a test is hard to write, the design is wrong. What is the test's pain telling you? Huge fixtures → the unit is too big; global mutation → a seam is missing; mocks everywhere → the boundaries are wrong. Never silence test pain with test machinery.
  • Clear is better than clever. Will a reader get this in 10–15 seconds? Cleverness is a cost paid by every future reader.
  • When in Rome, code as the Romans do. Does this diff arrive in the host repo's existing style, or does it import mine? A new test mechanism, dependency, framework, or convention — however good — is an adoption decision that belongs to the repo owner, not a side effect of a feature PR. Strong opinions travel as a discussion or a separate PR, never as a bundled surprise. Reviewers forgive imperfect code in the house style far more readily than perfect code in a foreign one.

2. The twelve rules

Each rule is stated once, with one TypeScript + React example and the names of the moves that fix it. The moves are shared vocabulary across every language these rules are rendered for: reviewers cite them by name. Where a rule cites R1's scorecard, the short form is: a type earns its keep by owning validation, behavior or an invariant, and scores zero when its only method unwraps the primitive; the full scorecard lives in the plugin's R1 rule.

R1 — Primitive Obsession

Domain concepts must not travel as raw strings, numbers, booleans or lists. When a primitive carries validation rules, behavior, or a domain name, it becomes a type with a validating constructor and named methods. The inverse binds equally: a wrapper that adds no validation, no logic, and no invariant is over-abstraction — score before you wrap.

// ❌ the rule lives at the call site, twice, and 0 means "none"
function managementPort(spec: ServiceSpec): number {
  for (const p of spec.ports) {
    if (p.name === 'weka-api' && p.port > 0 && p.port <= 65535) return p.port
  }
  for (const p of spec.ports) {
    if (p.port > 0 && p.port <= 65535) return p.port
  }
  return 0
}

// ✅ a Port cannot exist out of range; "first valid" collapses to "first"
export interface Port {
  readonly name: string
  readonly number: number
}

export function parsePort(name: string, number: number): Port {
  if (!Number.isInteger(number) || number <= 0 || number > 65535) {
    throw new RangeError(`port '${name}': ${number} out of range 1-65535`)
  }
  return { name, number }
}

export class Ports {
  constructor(private readonly items: readonly Port[]) {}
  firstNamed(name: string): Port | undefined {
    return this.items.find((p) => p.name === name)
  }
  first(): Port | undefined {
    return this.items[0]
  }
}

// ❌ a shape crosses the boundary; the caller pokes the method out of it
const BEARER = 'Bearer '

function bearerToken(headers: Record<string, string>): string | undefined {
  for (const [name, value] of Object.entries(headers)) {
    if (name === 'Authorization' && value.startsWith(BEARER)) return value.slice(BEARER.length)
  }
  return undefined
}

// ✅ the container has a name, and the loop is its method
export class RequestHeaders {
  private readonly raw: ReadonlyMap<string, string>
  constructor(raw: Iterable<readonly [string, string]>) {
    this.raw = new Map(raw)
  }
  authToken(): string | undefined {
    const value = this.raw.get('Authorization')
    return value?.startsWith(BEARER) ? value.slice(BEARER.length) : undefined
  }
}

In TypeScript: absence is a declared Port | undefined that every caller narrows, checked by strict; Map.get and Array.prototype.find are the model. A 0, '', -1 or null returned from a signature that promises Port is a sentinel and a finding, and as Port, a ! or an @ts-expect-error is its silenced form. Never a [Port, boolean] tuple. A bare type PortNumber = number, or a brand without a validating factory, scores zero on the scorecard: it is erased and admits every literal. A Record<string, string> or a tuple returned from a function is a shape, not a concept: read what the receivers do with it, and name the type. A nested annotation (Record<string, string[]>, [Record<string, unknown>, string]) is always a missing named type; readonly T[] is one level, not nesting.

Moves: Replace Primitive with Domain Type · Extract Collection Type · Replace Sentinel with Declared Absence · Name enum strings · Introduce Parameter Object · Name the Container

R2 — Self-Validating Types

A type validates its own invariants in its constructor — the only way to obtain a value — and every method thereafter trusts the receiver. Validation ownership never sits upstream: a type that relies on callers to have validated for it is not self-validating, whatever its fields look like.

// ❌ an assertion is not validation; whatever the server sent is now typed Device
export async function fetchDevice(id: DeviceId): Promise<Device> {
  const data = await apiClient.get<Device>(`/devices/${id}`)
  return data
}

// ✅ the boundary parses unknown once, with type guards; the domain only sees a Device
export function parseDevice(raw: unknown): Device {
  if (!isRecord(raw) || typeof raw.id !== 'string' || typeof raw.hostname !== 'string') {
    throw new ApiError('device: malformed response')
  }
  return { id: parseDeviceId(raw.id), hostname: raw.hostname }
}

export async function fetchDevice(id: DeviceId): Promise<Device> {
  return parseDevice(await apiClient.get(`/devices/${id}`))
}

// ❌ optional collaborator kept undefined-able; every method re-asks the question
class Reporter {
  constructor(private readonly sink?: Sink) {}

  record(ev: UiEvent): void {
    if (this.sink !== undefined) this.sink.write(ev)
  }
}

// ✅ absence is a named value bound once; no argument is ever undefined
const NULL_SINK: Sink = { write() {} }

class Reporter {
  constructor(private readonly sink: Sink = NULL_SINK) {}

  record(ev: UiEvent): void {
    this.sink.write(ev) // no guard anywhere
  }
}

In TypeScript: the self-validating type is the readonly shape with the one factory that builds it, shown under R1, or a class with a private constructor and a static parse. Types are erased, so the invariant lives in that factory: readonly closes assignment after construction, and the factory is the only production site that spells the literal. The finding is a mutable interface carrying invariants with no factory, data as Device, an apiClient.get<T>() taken on faith at the boundary, or an as unknown as. Where the repository already uses a schema library, schema.parse at the boundary is the constructor and the inferred type its shape; a refactor never introduces one. The second fence is the optional-collaborator case: a Null Object bound once as a module constant, the parameter typed Sink and never Sink | undefined, supplied by a default.

Moves: Add validating constructor · Hoist method checks into the constructor · Introduce Null Object · Delete re-validation of composed types · Separate Failure from Absence

R3 — Storifying

A top-level function reads like a story: every step is a named call at the same conceptual level, and the whole flow is graspable at a glance. Method calls never mix with string/index manipulation in the same body. A comment that names a block of code is a function name waiting to be extracted.

// ❌ flags track the loop, comments name the blocks, the policy is nowhere stated
export function alignIpConfig(config: Config, iface: ReportedInterface): void {
  let addrIp4Added = false
  let addrIp6Added = false
  for (const a of iface.addresses) {
    if (a.scope !== 'global') continue
    if (a.family === 'inet6') { // validate IP6
      if (addrIp6Added) continue // already added. skip
      if (!parseIp6(config, a.address)) throw new Error(`IP6 "${config.ipv6}" is not valid`)
      addrIp6Added = true
      continue
    }
    // ... the IPv4 twin of the block above, then the "nothing added" check
  }
}

// ✅ the story in two named steps; the loop moved onto a type that owns it
export function alignIpConfig(config: Config, iface: ReportedInterface): void {
  const ipConfig = collectIpConfigFrom(iface.addresses)
  alignIps(config, ipConfig)
}

function collectIpConfigFrom(addresses: readonly ReportedAddress[]): IPConfig {
  const ipConfig = new IPConfig()
  for (const a of addresses) ipConfig.addAddress(a)
  return ipConfig
}

In TypeScript: a name reveals its side effect: align/upsert/set mutate, parse/validate/is never do. A parseIp4 that writes config.ipv4 is a storifying bug even when the flow reads well. In React the fat function is as often a component body or an effect: the same let flags and nested ifs inside a useEffect, or a render tree whose ternaries nest three deep. The extracted leaf is a pure function or a custom hook, and react/no-unstable-nested-components fires when the extraction is done in place inside the component.

Moves: Extract Function named after the comment · Extract Leaf Type · Replace Nesting with Early Returns · Split Phase · Honest Rename

R4 — Helper Placement

Every extraction raises a second question: where does the helper live? The answer is decided by two axes — juiciness (the scorecard in R1; cite it, never re-derive it) and scope (feature-specific versus domain-generic). Three rungs: internal in place, feature sub-package, shared domain package. Never test privates, and never export a helper into its parent package just so a test can reach it.

// ❌ exported from the page module only so the test can import it
import { parseK3sArgument } from './ClusterPage'

// ✅ rung 1: one caller, no vocabulary of its own; non-exported, at module scope,
//    covered through the module's exports
function parseK3sArgument(arg: string): readonly [key: string, value: string] | undefined {
  const [key, ...rest] = arg.split('=')
  return rest.length === 0 ? undefined : [key, rest.join('=')]
}

// ✅ rung 2: the page folder's own module, named for its vocabulary, by relative path
import { managementPort } from './managementPort'
import { useClusterHealth } from './hooks/useClusterHealth'

// ✅ rung 3: networking vocabulary with several callers → a shared module,
//    named for the domain, through the path alias
import { parsePort, Ports } from '@/networking/ports'

In TypeScript: export is the wall: a non-exported function cannot be imported, so the urge to test it shows up as an export added for the test or a barrel index.ts that re-exports a private name. The urge is a placement signal, and the helper wants its own module with a public name. Rung 1 is beside its only caller at module scope, never inside the component body, where it is re-created every render; rung 2 is the page folder's own module or its hooks/; rung 3 a shared src/<domain>/ module. There is no internal/. Extract Custom Hook climbs the same ladder: beside its only caller, then the page's hooks/, then src/hooks/ only when two pages share it.

Moves: Demote (rung 1) · Promote to feature sub-package (rung 2) · Promote to domain package (rung 3) · Split policy from vocabulary during promotion · Move Method to the Envied Type

R5 — Vertical Slice

Group code by feature and role, not by technical layer: bad — domain/rotator, services/rotator; good — rotator/parser.ts, rotator/handler.ts. All code for a feature lives in one package, internally separated by role within it. Package names are flatcase domain vocabulary — never a layer or role name.

❌ by layer                          ✅ by feature, roles inside
src/components/SnapshotsTable.tsx    src/pages/Snapshots/SnapshotsPage.tsx       # the route's component: the story
src/hooks/useSnapshots.ts            src/pages/Snapshots/SnapshotsTable.tsx      # role: presentation
src/services/snapshotsApi.ts         src/pages/Snapshots/useSnapshots.ts         # role: server state (the query hook)
src/types/snapshot.ts                src/pages/Snapshots/snapshotsApi.ts         # role: HTTP, over apiClient
src/pages/SnapshotsPage.tsx          src/pages/Snapshots/snapshot.ts             # domain type + parseSnapshot
                                     src/pages/Snapshots/SnapshotsPage.test.tsx  # MSW handlers answer the real fetch

In TypeScript: the folder name is the feature noun, Snapshots, never services or hooks. utils/, common/ and helpers/ name no vocabulary at all: the first function that lands there has no owner, and the next lands beside it because the first did. Role names (snapshotsApi.ts, useSnapshots.ts) live in the file names inside the slice. Each type with logic sits in its own module named after the type. The router imports SnapshotsPage and nothing else from the folder, so the module split inside it is private to it; top-level components/, hooks/, services/ and types/ hold only what two pages share.

Moves: Slice out a feature · Rename layer files by role during the move · Split a generic package by owner

R6 — Test-Only Interfaces

A seam that exists only so a test can substitute a double — an interface with one production implementer, a patched attribute, an injection parameter no production caller varies — is deleted; depend on the concrete collaborator. Don't create interfaces until you need them; a test fake is not a need. An interface is justified only by a real second production implementation or a verified import cycle.

// ❌ one production implementer; the interface and the context exist for the double in the test
export interface DeviceRepository {
  findLatest(id: DeviceId): Promise<Device>
}
export const DeviceRepositoryContext = createContext<DeviceRepository | undefined>(undefined)

// ❌ the same seam with no declaration to grep for
vi.mock('../hooks/useDevices', () => ({
  useDevices: () => ({ data: [device], isLoading: false }),
}))

// ✅ concrete dependency; the test wires the REAL hook over the REAL fetch, and MSW answers it
export async function fetchLatestDevice(client: ApiClient, id: DeviceId): Promise<Device> {
  return parseDevice(await client.get(`/devices/${id}`))
}

it('shows the latest device', async () => {
  server.use(http.get('/api/devices/:id', () => HttpResponse.json(deviceFixture)))

  renderWithProviders(<DevicesPage />)

  expect(await screen.findByRole('heading', { name: deviceFixture.hostname })).toBeInTheDocument()
})

In TypeScript (opinionated): vi.mock of a hook or service you own, and a vi.fn() object shaped like a service, are this smell with no interface to point at; an interface with one implementation injected through context "for testing" is the same finding. MSW is the real layer for HTTP: one handler file per API domain, started once in setup. Mocking the true boundary is fine: the router, the auth SDK, the clock (vi.useFakeTimers), matchMedia. A TypeScript interface is structural, so the one-implementer test transfers unchanged.

Moves: Delete the Test Seam · Rewrite the test around real collaborators · Delete the double

R7 — Test Placement

Every behavior is tested at the lowest rung of the composition ladder that contains it: rung 0 is pure leaf types (unit tests with literal inputs, 100% coverage, public API only, imported as a consumer would); each rung above adds exactly one real production layer; only the true external boundary is ever faked. Orchestrating types get integration-style tests that cover the seams between their real collaborators — some overlap with leaf coverage is fine; leaf behavior tested only from above is not. On a leaf type, coverage is the floor and the mutation score is the claim: a leaf's tests must fail when its logic is changed, and a mutant that survives them is a missing row or dead logic.

// ❌ success and error fused into one table, with a branch inside the case
it.each([
  { raw: '3x100ms', expectError: false },
  { raw: '0x100ms', expectError: true },
])('parsePolicy($raw)', ({ raw, expectError }) => {
  if (expectError) {
    expect(() => parsePolicy(raw)).toThrow()
  } else {
    expect(parsePolicy(raw).maxAttempts).toBe(3)
  }
})

// ✅ two tables, one shape each, every row named
describe('parsePolicy', () => {
  it.each([
    { name: 'plain', raw: '3x100ms', maxAttempts: 3 },
    { name: 'single attempt', raw: '1x100ms', maxAttempts: 1 },
  ])('accepts $name', ({ raw, maxAttempts }) => {
    expect(parsePolicy(raw).maxAttempts).toBe(maxAttempts)
  })

  it.each([
    { name: 'zero attempts', raw: '0x100ms' },
    { name: 'missing delay', raw: '3x' },
  ])('rejects $name', ({ raw }) => {
    expect(() => parsePolicy(raw)).toThrow(PolicyError)
  })
})

In TypeScript: a name field on every row and $name in the title, so a failure names its case; import as a consumer would, from '@/retry/policy', never a symbol exported for the test. No setTimeout wait: findBy* or waitFor, or vi.useFakeTimers with advanceTimersByTime. Pages and hooks are tested through renderWithProviders over the real hook, with MSW handlers answering the fetch. Leaf modules, and only they, are the mutation targets where the repository configures Stryker: a surviving mutant is a missing row or dead logic.

Moves: Move the behavior down a rung · Split Success and Error Tables · Kill the surviving mutant · Replace doubles with real collaborators · Replace sleep with synchronization · Delete private-function tests

R8 — No Globals

Dependencies are passed down from the caller, never reached sideways: no package-level mutable state, no import-time initialization writing state, no singletons fetched from inside business logic, no library code that manufactures its own root cancellation — cancellation flows from caller to callee. Globals are acceptable only at the composition root — the program's entry point, handler setup, application wiring — where they are read once and injected downward.

// ❌ a module-level config built at import, reached from a leaf
import { env } from '../config/env'

export const apiClient = new ApiClient(env.apiUrl) // built at import, from a global

export async function renameDevice(id: DeviceId, name: string): Promise<void> {
  await apiClient.post(`/devices/${id}`, { name })
}

// ✅ read once in the composition root, pushed down as a value
export class DevicesApi {
  constructor(private readonly client: ApiClient) {}

  async rename(id: DeviceId, name: string): Promise<void> {
    await this.client.post(`/devices/${id}`, { name })
  }
}

// main.tsx — the environment is read ONLY here
const config: AppConfig = readAppConfig(import.meta.env, window.__RUNTIME_ENV__)
const queryClient = new QueryClient()
const services: Services = { devices: new DevicesApi(new ApiClient(config.apiUrl)) }

createRoot(rootElement).render(
  <QueryClientProvider client={queryClient}>
    <ServicesProvider value={services}>
      <App />
    </ServicesProvider>
  </QueryClientProvider>,
)

In TypeScript: silent everywhere: constants, as const enums, types, pure functions, the createContext key (the value lives in a provider), style imports. Silent only in the composition root (main.tsx / App.tsx): new QueryClient(), the router, the provider tree, import.meta.env and window.__RUNTIME_ENV__ read once into an AppConfig. Reported elsewhere: a module-level let, new ApiClient() at import, a QueryClient stashed in a module so non-React code can reach it, import.meta.env inside a hook or service, localStorage read at module scope, a lazy singleton behind a getter. React context is the composition mechanism; a module singleton is the finding when a provider would do. A test that vi.stubEnvs or vi.mocks a config module is evidence against the production code, not a fix for the test.

Moves: Extract Clean Island · Push the Global Up One Level · Replace Import-Time Initialization with a Constructor · Pass Cancellation Down

R9 — Repo Brain

Documentation is a network ranked by the documentation ladder: storified code → JSDoc comments → repo docs → the index, each fact placed at the lowest rung that can carry it, higher rungs summarizing and pointing down, never duplicating. Two invariants hold the network together: reachability (every doc is reachable from the root: CLAUDE.md → index.md → doc — no orphans) and bidirectionality (code points up at its feature doc; docs point down at code via greppable symbols; the index points everywhere).

// ❌ an exported class with no JSDoc; the comment narrates WHAT the loop does
export class Policy {
  async do(op: () => Promise<void>): Promise<void> {
    // loop over attempts and back off between failures
    for (let attempt = 1; attempt <= this.maxAttempts; attempt += 1) { /* … */ }
  }
}

// ❌ a JSDoc that restates the type is deleted
export interface Cluster {
  /** The cluster id. */
  readonly clusterId: string
}

/**
 * @param raw the raw string
 * @returns the policy
 */
export function parsePolicy(raw: string): Policy { /* … */ }

// ✅ the summary line states the contract, the body says why,
//    the doc points down at code and code points up at the doc
/**
 * A capped exponential-backoff retry policy with full jitter.
 *
 * Jitter is deliberate: synchronized clients retrying in lockstep re-overloaded
 * the upstream API after every blip.
 * See docs/retry-policy.md for the incident and the cap math.
 */
export class Policy { /* … */ }

In TypeScript: an exported symbol gets a JSDoc when the WHY budget has something to say; the type system carries the WHAT, so /** The cluster id */ on clusterId: string, @param and @returns tags that restate types, and @type tags are noise and are deleted. The summary line is the contract and never a restatement finding. A non-exported symbol gets no comment by default. A component's props doc the prop whose meaning its type does not carry: units, ownership, who calls it. Where the repository's jsdoc/require-jsdoc demands one, a WHAT-comment is rewritten, never deleted. The doc points down with greppable symbols (Policy.do, parsePolicy), never a path and a line number.

Moves: Push the fact down a rung · Convert WHAT to WHY or delete · Rewire orphan doc · Wire the root · Add missing frontmatter · Update the stale doc with the behavior change

R10 — Concurrency Safety

Every async task has an owner and a provable exit path; shared mutable state is owned by one type and guarded where it lives; production code never sleeps to pace or synchronize cancellable work. Concurrency is designed at construction time — who owns the state, who stops the async task — never patched in afterward.

// ❌ no owner, no exit; the interval outlives the component, and the slowest response lands last
useEffect(() => {
  setInterval(() => {
    void fetchStatus(id).then(setStatus)
  }, POLL_MS)
}, [id])

// ✅ the effect that starts the timer returns the cleanup that stops it; each tick retires the last
useEffect(() => {
  let inflight: AbortController | undefined
  const timer = setInterval(() => {
    inflight?.abort()                       // a slow response never lands after a newer one
    inflight = new AbortController()
    void fetchStatus(id, inflight.signal).then(setStatus).catch(ignoreAbort)
  }, POLL_MS)
  return () => {
    clearInterval(timer)
    inflight?.abort()
  }
}, [id])

// ✅ where the repository has TanStack Query, the query owns the fetch and the polling
const { data: status } = useQuery({
  queryKey: ['device-status', id],
  queryFn: ({ signal }) => fetchStatus(id, signal),
  refetchInterval: POLL_MS,
})

In TypeScript: one thread, no locks. The units are promises, effects, timers, subscriptions and queries; the race is the stale closure and the out-of-order response, retired by abort() or an ignore flag set in the cleanup, and check-then-act on a ref across an await is the split guard. Sleep is setTimeout-based polling or backoff that nothing cancels; refetchInterval, or an interval cleared in the cleanup, is the cancellable wait. A fetch in useEffect in a repository that has the query layer is the finding, and a disabled react-hooks/exhaustive-deps is a suppression of this rule.

Moves: Inject the Exit Path · Make Concurrent Work Joinable · Extract Synchronized Owner · Replace Sleep with Cancellable Wait · Delete Unearned Guards

R11 — Conditional Dispatch (Anti-IF)

A conditional that asks what a value is — a type switch, or a switch/if-chain on a kind/status/mode discriminator — may exist once. The second copy of that discriminator is a missing polymorphic type: the variants want to be implementations of an interface (or entries in a dispatch map), chosen once at the boundary, so downstream code tells the value what to do instead of asking what it is. One well-placed, exhaustive switch is not a defect; a duplicated one always is.

// ❌ the same discriminator in NotifyPanel.tsx, validateChannel.ts and alertsApi.ts
function send(alert: Alert): Promise<void> {
  switch (alert.channel) {
    case 'email': return sendEmail(alert.recipient, renderEmail(alert))
    case 'slack': return postSlack(alert.recipient, renderSlack(alert))
    default: return Promise.reject(new Error(`unknown channel ${alert.channel}`))
  }
}

// ✅ chosen once at the boundary; everything downstream tells
export type Channel = 'email' | 'slack' | 'webhook'

export interface ChannelSender {
  send(a: Alert): Promise<void>
  validate(recipient: string): boolean
  retryPolicy(): RetryPolicy
}

const CHANNEL_SENDERS: Record<Channel, ChannelSender> = { // complete, or it does not compile
  email: emailSender,
  slack: slackSender,
  webhook: webhookSender,
}

export function parseChannel(raw: string): ChannelSender { // the ONE place the raw string is inspected
  if (!isChannel(raw)) throw new ApiError(`unknown channel ${raw}`)
  return CHANNEL_SENDERS[raw]
}

// ✅ the one kept switch is closed by the compiler, not by an "unknown" arm
function channelIcon(channel: Channel): ReactNode {
  switch (channel) {
    case 'email': return <MailIcon />
    case 'slack': return <SlackIcon />
    case 'webhook': return <WebhookIcon />
    default: return assertNever(channel)
  }
}

In TypeScript: the kept switch is over a discriminated union, closed by default: return assertNever(x) or satisfies never; that arm is the completeness proof tsc checks, not an unknown-kind default, and a default: return null in a render switch is the finding. Prefer a Record<Kind, …> of handlers or components first, a strategy object second, a class hierarchy last. Boolean props: isLoading/isError/isEmpty triplets become a status union, and three or more boolean props on one component is a Split Flag Argument candidate (T3).

Moves: Replace Duplicated Switch with Interface Dispatch · Replace If-Chain with Strategy Map · Introduce Null Object · Split Flag Argument · Keep the Single Exhaustive Switch

R12 — Mutation Discipline

A validated value changes state only through methods that own its invariants — never through leaked internals. Constructors copy the collections they are given; queries return copies (or iterators), not the internal reference; a method is a query or a modifier, not both; and a type with a validating constructor exposes no setter that skips the validation. This rule adapts Fowler's Mutable Data smell family (Refactoring, 2nd ed.: Encapsulate Collection, Separate Query from Modifier, Remove Setting Method, Split Variable) to languages where collections are passed by reference into shared backing storage.

// ❌ returns a mutable alias into validated state; a distant caller sorts it in place
export class Grants {
  private readonly perms: Permission[]

  constructor(raw: Iterable<string>) {
    this.perms = dedupeAndValidate(raw) // non-empty, deduplicated
  }

  all(): Permission[] {
    return this.perms
  }
}

const perms = user.grants.all()
perms.sort((a, b) => a.weight - b.weight) // reorders internal state; no method called

// ✅ copy on the way in; hand out a readonly view, not the storage
export class Grants {
  private constructor(private readonly perms: readonly Permission[]) {}

  static parse(raw: Iterable<string>): Grants {
    return new Grants([...dedupeAndValidate(raw)]) // freshly built here — no shared alias
  }

  all(): readonly Permission[] { // tsc rejects sort() and index assignment on it
    return this.perms
  }
}

const sorted = user.grants.all().toSorted((a, b) => a.weight - b.weight) // the caller's own copy

In TypeScript: readonly is a compile-time wall, and tsc is where an alias is caught cheapest: a spread or Array.from on the way in, readonly T[], ReadonlyMap or ReadonlySet on the way out, readonly fields so nothing is assigned in between. as const freezes the type, not the value, and Object.freeze is shallow. The React forms of the leak: a selector that .sort()s query data in place, an updater that mutates the previous state and returns the same reference so React skips the render, a prop array stored in a ref and mutated; the fix is toSorted or a spread copy, and an updater that returns a new object.

Moves: Copy on the Way In · Copy on the Way Out / Encapsulate Collection · Separate Query from Modifier · Remove Setting Method · Split Variable

3. House rules

Rules that are not one of the twelve but hold in every language, then the TypeScript + React house rules. Same shape as above, with their own numbers so a review can cite them.

H1 — A suppression is a review finding, not a tool

A // eslint-disable-next-line directive is never added on your own: fix the code, and when the finding is a true false positive, propose the exclusion in the linter's configuration and get it reviewed. A new suppression in a diff is itself a finding, and no automated lint-fix pass adds one or edits the configuration.

In TypeScript: // eslint-disable-next-line <rule>, // eslint-disable-line, the block form /* eslint-disable <rule> */ … /* eslint-enable */, @ts-expect-error, @ts-ignore, @ts-nocheck and // prettier-ignore are the same thing. A bare eslint-disable names no rule, so read for the rule id; of the type suppressions, @ts-expect-error is preferred over @ts-ignore because it fails the build when the error goes away. An override block in eslint.config.* or a tsconfig option is the reviewed place for a true false positive.

Review: Did the diff add an eslint-disable, @ts-expect-error, @ts-ignore, @ts-nocheck or prettier-ignore, or edit eslint.config.*, tsconfig*.json or .prettierrc*?

H2 — Errors carry the context of the layer that saw them

Handle an error once: wrap it with the context of the boundary it crossed and its cause, or handle it, never both log it and pass it on. Catch narrowly, the failure you can handle, never everything. Failure vocabulary belongs to the package that raises it: one named error per thing that can go wrong, made public only when a caller decides on it.

In TypeScript: throw an Error subclass with its cause attached, throw new ApiError('device: malformed response', { cause }), never a string. A catch (error: unknown) narrows with instanceof and never by message; the one place a broad catch belongs is the boundary that renders the failure, an error boundary or the query layer's onError. A promise is awaited or returned, never dropped (promise/catch-or-return, @typescript-eslint/no-floating-promises); console.error and a rethrow in the same catch is handling it twice. A catch that returns null or undefined is R2 Q5: Separate Failure from Absence.

Review: Does any catch swallow the error, return null for a failure, both console.error and rethrow, inspect message, or throw without its cause?

Rules marked (opinionated) are stances, not TypeScript or React community norms; the departure is deliberate.

T1 — The type system is the contract (opinionated)

strict is on and the type checker passes. No any on a public signature (@typescript-eslint/no-explicit-any, the no-unsafe-* family), no as outside the boundary parser, no ! (@typescript-eslint/no-non-null-assertion). An object- or unknown-valued container crossing a boundary (Record<string, unknown>, Map<string, object>) is the same suppression one level down: name the mapping (R1, Name the Container). The types are what let T | undefined be a declared absence instead of a hope.

Review: Does any public signature carry an any, an as or ! outside the boundary parser, or an object- or unknown-valued container?

T2 — Props and returned data are readonly

A component takes Readonly<Props> (sonarjs/prefer-read-only-props), a hook or a service returns readonly T[], ReadonlyMap or ReadonlySet, and a lookup table is as const. A mutable prop or return type is an invitation to the in-place .sort() and .push() that R12 hunts.

Review: Is any props type, returned array or table mutable?

T3 — A component is a named function declaration, one per file (opinionated)

function DevicesPage(props: Readonly<Props>), named-exported (react/function-component-definition, react/no-multi-comp). A hook is useX, beside its only caller until a second page needs it (R4). No component is defined inside another (react/no-unstable-nested-components): the inner one is re-created every render and loses its state. Three or more boolean props on one component are a status union or two components (R11, Split Flag Argument).

Review: Is any component an arrow, a second component in a file, or defined inside another component's body?

T4 — MSW is the boundary (opinionated)

HTTP is answered by MSW handlers, one file per API domain, started once in setup; vi.mock of a module you own is R6's seam. The router, the auth SDK and the clock (vi.useFakeTimers) are the boundaries a test may stub. Queries go by what the user sees: getByRole and getByLabelText first, getByText next, getByTestId last.

Review: Does any test vi.mock a module the repository owns, or reach for a test id where a role or a label would do?

T5 — An effect returns its cleanup, and a fetch belongs to the query layer (opinionated)

Every setInterval, subscription and addEventListener started in an effect has a cleanup that stops it, and a fetch started there owns an AbortController. A fetch in useEffect is a finding where the repository has TanStack Query: the query owns the lifecycle, the cache and the abort. react-hooks/exhaustive-deps is never disabled; a dependency the lint wants is a design question (R10), not a lint problem.

Review: Does any effect start a timer, subscription or fetch without a cleanup, fetch where a query hook would do, or disable react-hooks/exhaustive-deps?

4. Self-review

Before you ask for review, answer each with a file and line, not a feeling. The plugin's reviewers ask every rule question with a detection command behind it; these are the ones that catch the most. The house-rule questions are review questions only.

  • R1 Does the diff validate a primitive inline instead of constructing a type? · Is the same predicate enforced in more than one place? · Does any function return a sentinel to mean "not found / invalid"? · Does a flat container of primitives cross a function boundary?
  • R2 Can the type exist in an invalid state? · Does anything return or accept undefined or null as a value?
  • R3 Does one body mix abstraction levels? · Do block comments narrate sections inside a function body?
  • R4 Are module-private helpers tested directly? · Does a new shared module have a role name?
  • R5 Is any directory named after a layer or role? · Is one feature's code spread across ≥2 layer directories?
  • R6 Is the only other implementer a test double? · Does the diff justify a new interface with "for testing" or "import cycle"?
  • R7 Does any test case body contain a conditional? · Does any test reach past the public surface? · Does any test sleep to synchronize?
  • R8 Does any module declare mutable state at module level? · Does deep code read a global config?
  • R9 Does a JSDoc on an exported symbol state WHAT instead of WHY? · Did behavior change silently under an existing doc?
  • R10 Does every timer, subscription or fetch started in the diff have a provable exit path? · Does production code sleep?
  • R11 Is the same discriminator inspected in more than one place? · Does a default: (or trailing else) handle "unknown kind" away from the boundary? · Does a boolean parameter select between behaviors?
  • R12 Does a method return an internal array, Map or Set by reference? · Can a validated type be mutated around its constructor?
  • House rules Did the diff add an eslint-disable, @ts-expect-error, @ts-ignore, @ts-nocheck or prettier-ignore, or edit eslint.config.*, tsconfig*.json or .prettierrc*? · Does any catch swallow the error, return null for a failure, both console.error and rethrow, inspect message, or throw without its cause? · Does any public signature carry an any, an as or ! outside the boundary parser, or an object- or unknown-valued container? · Is any props type, returned array or table mutable? · Is any component an arrow, a second component in a file, or defined inside another component's body? · Does any test vi.mock a module the repository owns, or reach for a test id where a role or a label would do? · Does any effect start a timer, subscription or fetch without a cleanup, fetch where a query hook would do, or disable react-hooks/exhaustive-deps?

5. Mechanics

Test npx vitest run
Lint npx tsc --noEmit && npx eslint .
Lint and fix npx tsc --noEmit && npx eslint . --fix && npx prettier --write .
Suppression // eslint-disable-next-line — never added on your own; a new one in a diff is a review finding
Docs JSDoc: short, says why, not what; long-form under docs/
Type check tsc -b where tsconfig.json has references, else tsc --noEmit; the lint command runs it first, so ESLint's --fix and Prettier never see a type error
Type suppression @ts-expect-error over @ts-ignore — the same rule as eslint-disable, see H1
TypeScript / React the examples assume TypeScript 5 under strict and React 18; satisfies needs 4.9+, toSorted the ES2023 lib (else [...xs].sort())
Tests Vitest collects *.test.ts and *.test.tsx colocated with the module; renderWithProviders in src/test-utils/; MSW started once in the setup file
Package manager read from the lockfile (yarn.lock, pnpm-lock.yaml, package-lock.json, bun.lock or the older bun.lockb); every script runs through it
Mutation Stryker over leaf modules only, where the repository configures it
Workflow RED → GREEN → REFACTOR per behavior; package-scoped lint every cycle; review per finished slice; commit only a green tree