---
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

- [x] `no-undef` rule enabled in `eslint.config.mjs`
- [x] All 3 surfaced bugs fixed (not silenced with disable-comments)
- [x] `npm run lint` clean (modulo the 1 pre-existing unrelated warning on `CollectionsPageView.js`)
- [x] `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)