Skip to content

feat(web): add four ESLint rules for previously-vibes-checked conventions (LUM-2070)#32659

Closed
ashleeradka wants to merge 1 commit into
mainfrom
ashlee/lum-2070-eslint-conventions
Closed

feat(web): add four ESLint rules for previously-vibes-checked conventions (LUM-2070)#32659
ashleeradka wants to merge 1 commit into
mainfrom
ashlee/lum-2070-eslint-conventions

Conversation

@ashleeradka
Copy link
Copy Markdown
Contributor

Closes LUM-2070.

Why

A codebase-wide audit following PRs #32641 and #32654 found that 12 of ~80 documented conventions are automatically enforced — the other 85% rely on manual review and decay over time. The systemic issue isn't that we lack good conventions; it's that we lack mechanisms to keep them in force. This PR closes four of the highest-ROI gaps with custom ESLint rules.

The four rules picked have something in common: each catches either a concrete current-state issue OR prevents a regression of a recently-established convention that human review missed (twice during PR #32654's cleanup, until Codex caught it).

What

Four new rules in apps/web/eslint-rules/, following the same pattern as the existing no-cross-domain-imports.mjs:

Rule Level What it catches
local/no-barrel-files error index.ts / index.tsx outside src/generated/
local/no-usereducer-for-client-state error useReducer calls outside src/domains/terminal/ (the documented exception)
local/no-outlet-context-outside-layouts error useOutletContext() outside *-layout.{ts,tsx}
local/max-file-lines warn (300) Non-test source files exceeding 300 lines

Each ships with a unit-test file (43 total cases) and a docstring explaining the rule's rationale, exemption policy, and reference to the corresponding doc section. Rule registration goes in apps/web/eslint.config.mjs alongside the existing local/no-cross-domain-imports.

Why each level

  • Three at error because the codebase already complies. They're defensive guards — they don't break anything today; they prevent silent regression.
  • max-file-lines at warn because 116 existing files would otherwise fail CI. The existing offenders are tracked: chat-page.tsx and chat-route-content.tsx (LUM-2071), settings/ai files (LUM-2072), chat-layout.tsx (LUM-2065), the long tail (LUM-2073). The warning surfaces each one as a CI signal; once the worst offenders are split, the threshold can be tightened or flipped to error.

Two genuinely-interlocked state machines (lifecycle-service.ts 643 lines, turn-store.ts 879 lines) carry an inline /* eslint-disable local/max-file-lines -- <reason> */ at the file top with a one-line justification. That's the documented escape hatch for code where splitting hurts cohesion more than file length hurts readability.

Why safe

  • tsc --noEmit clean
  • bun run lint clean (0 errors, 116 warnings — all from max-file-lines)
  • bun test eslint-rules/ — 43/43 rule cases pass
  • bun run test:ci — 181/181 test files pass
  • No source code behavior change. The only src/ edits are the two eslint-disable comments on the state-machine files.

References

What I considered and did NOT do, and why

Did not add a require-atomic-selectors rule for the "atomic selectors via createSelectors, not useShallow" convention. The codebase has zero useShallow violations in non-test code — adoption is already 100%, so a lint rule would catch only hypothetical regressions. Lower priority than the four shipping here.

Did not add a conversationId vs conversationKey rule. The convention is documented (CONVENTIONS.md L318-360, AGENTS.md L18) and 36 conversationKey references still exist in non-generated code. A lint rule would require detecting "is this an API-call argument," which needs heuristics. File separately if the lingering refs become an active failure mode.

Did not flip max-file-lines to error in this PR. Doing so would block CI on 116 existing files. The forcing function for splits comes from the warning signal + the tracking tickets. Once those land, the threshold can be tightened.

Did not bulk-edit existing files to add eslint-disable comments. Only two state-machine files (lifecycle-service.ts, turn-store.ts) got the inline disable — files where splitting genuinely hurts cohesion and there's an explicit justification on record. The other 114 warning-firing files are tracked for actual decomposition, not silenced.

Did not extract the rule pattern into a higher abstraction. Each rule is short (50-100 lines including docstring) and self-contained. A shared base class or helpers would obscure the rule logic without removing meaningful duplication.

Stepping back

The single biggest finding from the audit was that the 85%-vibes ratio is the systemic issue, not any individual unenforced rule. This PR moves four rules from the unenforced 85% into the enforced 15%. The next-most-tractable enforcement candidates (if any later prove worthwhile) are the conversationId rule and possibly a require-createSelectors rule, but neither has the cost-benefit of these four.

https://claude.ai/code/session_01NjQuUAiXsS4DVEmKPsb1uE


Generated by Claude Code

A codebase-wide audit (post-#32641 / #32654) found that 12 of ~80
documented conventions are automatically enforced — the rest rely
on manual review and decay. This PR closes four of the highest-ROI
gaps as custom ESLint rules:

**`local/no-barrel-files`** (error) — bans `index.ts` /
`index.tsx` re-export files in app code (exempts `src/generated/`,
where barrels are emitted by codegen). The codebase already
complies; rule is defensive against re-export creep.

**`local/no-usereducer-for-client-state`** (error) — bans
`useReducer` outside `src/domains/terminal/` (the documented
exception pending migration). Codebase already complies; rule
prevents the documented exception from quietly multiplying.

**`local/no-outlet-context-outside-layouts`** (error) — bans
`useOutletContext()` calls outside `*-layout.{ts,tsx}` files. PR
#32641 documented the gate-drops-context failure mode; this rule
turns the AGENTS.md convention into a hard guard. After #32657
removed the last stale violation, the codebase is at compliance.

**`local/max-file-lines`** (warn, default threshold 300) — flags
non-test source files exceeding 300 lines. Exempts
`src/generated/`, `*-catalog.ts` / `*-catalog-data.ts`, and test
files. Ships as `warn` so adoption can be incremental — 116 files
currently fire, tracked under LUM-2071/2072/2073/2065. State
machines that genuinely don't split (`lifecycle-service.ts`,
`turn-store.ts`) carry an inline `/* eslint-disable */` with a
one-line reason.

Test coverage:
- One unit test file per rule, following the existing
  `no-cross-domain-imports.test.mjs` pattern.
- `bun test eslint-rules/` runs all 43 cases.

Docs updates:
- `AGENTS.md`: outlet-context bullet now cites the lint rule.
- `CONVENTIONS.md`: no-barrel-files and file-size sections cite
  their respective rules.
- `STATE_MANAGEMENT.md`: the useReducer section cites the rule.

The previously-empty enforcement column for these four rules is
the systemic issue the audit named: 85% of documented conventions
were vibes-checked. This PR moves four of them into the 15% that
survive without human review.

Closes LUM-2070.
@linear
Copy link
Copy Markdown

linear Bot commented May 30, 2026

LUM-2070

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants