Randall Stillwell 2e68574393 convoy: enable no-undef ESLint rule + fix 3 latent bugs it surfaced

PR #144 (`31da384`, 2026-06-13) shipped a `ReferenceError: useFocusTrap
is not defined` to production because the flat ESLint config did NOT
enable the core `no-undef` rule — only `react/jsx-no-undef` (which
catches undefined JSX components, not plain JS identifier references).
This PR closes that gap, narrowly.

## What changes

- `eslint.config.mjs`: enable `no-undef: 'error'` for source files +
  define the ~40 browser / Node / Vitest globals the rule needs.
  Hand-curated globals list (rejected pulling in the `globals` npm
  package for one config block).
- 3 latent bugs surfaced + fixed (NOT silenced with disables):

  | Site | Bug | Fix |
  |------|-----|-----|
  | `components/CollectionPageView.js:238` | `onClick={toggleFavorite}` — fn defined in `lib/use-collection-view.js:269` (collection-level favorite) but missing from the hook's `return {}` | Added to hook return + component destructure |
  | `components/CollectionPageView.js:532` | `onTogglePublic={togglePublic}` — same pattern, fn at line 315 of the hook | Same shape: hook return + destructure |
  | `components/ShareModal.js:99` | `fetchInvitedUsers()` scoped inside the useEffect body but called from `handleInvite` outside | Extracted to component scope via `useCallback`; effect dep array updated |

  Bugs 1 + 2 broke the "Favorite collection" button and the public-toggle
  in the Share modal on the collection-detail page. Bug 3 broke the
  "refresh invitee list" path after a successful invite. None had been
  flagged because the operator hadn't exercised those exact flows since
  the relevant hooks were last refactored.
- `components/ShareModal.js`: also adds an eslint-disable for
  `react-hooks/set-state-in-effect` on the moved `fetchInvitedUsers()`
  call. Matches the canonical pattern in `pages/profile.js:90` —
  async fetch; setState fires post-resolve, not synchronously to the
  effect body.

## Why not pull in @eslint/js/recommended wholesale?

The recommended bundle also enables `no-unused-vars`,
`no-prototype-builtins`, `no-empty`, `no-cond-assign`, and ~10 others
— each would generate dozens of pre-existing violations on this
codebase. The right rule-by-rule sweep is the deferred
`adopt-eslint-recommended-set` convoy. This PR is scoped to the one
rule that would have caught PR #144's bug class.

## Test plan

- [x] `npm run lint` — clean (1 pre-existing unrelated warning on
      `CollectionsPageView.js`'s `eslint-disable` directive — out of
      scope)
- [x] `npm run test:run` — 25 files / 123 tests pass
- [ ] CI on this PR
- [ ] Post-merge: exercise the three formerly-broken paths (favorite a
      collection from its detail page; toggle a collection public via
      Share modal; invite a user and confirm the invitee list refreshes)

## Convoy doc

`.convoys/enable-no-undef-eslint-rule.md` documents the surfaced bugs,
D1 (no-undef only vs recommended bundle), D2 (hand-curated globals vs
`globals` package), risks, and acceptance.

Co-authored-by: Cursor <cursoragent@cursor.com>

2026-06-13 01:17:18 -05:00

5.2 KiB

Raw Blame History

slug: enable-no-undef-eslint-rule status: shipping opened: 2026-06-13 owner: rstillw related:

PR #144 (31da384, 2026-06-13) — the runtime useFocusTrap is not defined ReferenceError that motivated this rule
eslint.config.mjs — flat config that needed the rule added

enable-no-undef-eslint-rule

Problem

PR #144 shipped a production ReferenceError: useFocusTrap is not defined because components/ScanDisambiguationDialog.js called a hook it never imported. Lint didn't catch it. The flat ESLint config (eslint.config.mjs) only extended eslint-config-next/core-web-vitals, which enables react/jsx-no-undef (undefined JSX components) but NOT the core no-undef rule that catches plain JS identifier references like useFocusTrap(...) in a hook call.

What ships

Add the no-undef: 'error' rule directly to the flat config + define the browser / Node / Vitest globals it needs. Not pulling in @eslint/js/recommended wholesale — that bundle also enables no-unused-vars, no-prototype-builtins, and several others that would surface a flood of pre-existing violations and risk derailing the hotfix-class spirit of this change.

Latent bugs surfaced + fixed in this PR

Enabling the rule against the current codebase surfaced 3 real bugs (NOT false positives) all in the same convoy that built the collection-view split:

#	Site	Bug	Fix
1	`components/CollectionPageView.js:238`	`onClick={toggleFavorite}` — `toggleFavorite` (collection-level favorite, defined at `lib/use-collection-view.js:269`) was missing from the hook's `return {}` block. Different fn from `handleToggleFavorite` (per-card, line 375).	Added `toggleFavorite` to both the hook's return + the component's destructure.
2	`components/CollectionPageView.js:532`	`onTogglePublic={togglePublic}` — same pattern: `togglePublic` defined at `lib/use-collection-view.js:315`, missing from return.	Added `togglePublic` to both the hook's return + the component's destructure.
3	`components/ShareModal.js:99`	`fetchInvitedUsers()` scoped inside the `useEffect` body but called from `handleInvite` (outside the effect) after a successful invite.	Extracted `fetchInvitedUsers` to component scope wrapped in `useCallback(... , [collectionId])`; effect dep array updated.

Bugs 1 + 2 broke the "Favorite this collection" button and the public-toggle in the Share modal on the collection-detail page. Bug 3 broke the "refresh invitee list" path after a successful invite. All three would have crashed at runtime under normal usage; none had crashed yet because the broken paths sat in flows the operator hadn't exercised since the relevant hooks were extracted.

Decisions

D1. no-undef only vs @eslint/js/recommended wholesale. Chose no-undef only. Pulling the full recommended set would have added no-unused-vars, no-prototype-builtins, no-empty, no-cond-assign, and ~10 others — each generating dozens of pre-existing violations. The right rule-by-rule sweep is a separate convoy (adopt-eslint-recommended-set) if and when we want it. This convoy is scoped to the one rule that would have caught the PR #144 bug.
D2. Globals: hand-curated list vs globals/browser / globals/node packages. Chose hand-curated. The list is ~40 identifiers; pulling in the globals npm package adds a dep purely for one config block. Maintenance cost: when a new browser/Node global is referenced and the rule false-positives, add it to the list. Trade-off accepted.

Risks

#	Risk	Mitigation
1	A new file uses a global I forgot to add (e.g. `IndexedDB`, `WebGLRenderingContext`) and CI red-X's	Add to the `languageOptions.globals` block in the same PR. Low-cost.
2	The `react-hooks/set-state-in-effect` rule starts firing on additional sites because moving `fetchInvitedUsers` out of the useEffect made its setState call more visible to the rule tracker	Already happened on the new `fetchInvitedUsers` site; disable-comment with rationale (matches the canonical pattern in `pages/profile.js:90`). No other sites affected this PR.
3	A future PR re-introduces the same class of bug (unimported identifier) but somehow bypasses lint	Lint is a required CI gate (`Lint` job in `ci.yml`); `no-undef` is now on by default. Bypassing would require disabling the rule, which would show in PR review.

Acceptance

no-undef rule enabled in eslint.config.mjs
All 3 surfaced bugs fixed (not silenced with disable-comments)
npm run lint clean (modulo the 1 pre-existing unrelated warning on CollectionsPageView.js)
npm run test:run — 25 files / 123 tests pass
CI on the PR green
Smoke test post-merge: trigger the three previously-broken paths (favorite a collection, toggle a collection public, invite a user) and confirm no console errors.

Non-goals

Adopting the full @eslint/js/recommended rule set (adopt-eslint-recommended-set follow-up)
Adding eslint-plugin-jsx-a11y or other broader rule packages
Fixing react-hooks/set-state-in-effect violations across the codebase systematically (they're already advisory; the rule fires today on many sites with explicit eslint-disable-next-line comments that document the async-fetch pattern)

5.2 KiB Raw Blame History