From 2e685743932737b07977290d2909774a7df2163f Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Sat, 13 Jun 2026 01:17:18 -0500 Subject: [PATCH] convoy: enable no-undef ESLint rule + fix 3 latent bugs it surfaced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .convoys/.metrics.jsonl | 2 + .convoys/enable-no-undef-eslint-rule.md | 59 +++++++++++++ components/CollectionPageView.js | 2 + components/ShareModal.js | 42 ++++++---- eslint.config.mjs | 105 ++++++++++++++++++++++++ lib/use-collection-view.js | 2 + 6 files changed, 194 insertions(+), 18 deletions(-) create mode 100644 .convoys/enable-no-undef-eslint-rule.md diff --git a/.convoys/.metrics.jsonl b/.convoys/.metrics.jsonl index b39f1b8..f6f170a 100644 --- a/.convoys/.metrics.jsonl +++ b/.convoys/.metrics.jsonl @@ -70,3 +70,5 @@ {"ts": "2026-06-13T02:57:31Z", "role": "role-reviewer", "convoy": "harden-visual-diff-gate", "repo": "tcg-vault", "skip_flags": [], "brief": 2, "classification": "ci", "duration_s": 300, "outcome": "approved"} {"ts": "2026-06-13T04:14:48Z", "role": "role-architect", "convoy": "rotate-default-admin", "repo": "tcg-vault", "skip_flags": [], "classification": "security", "duration_s": 240, "outcome": "option-B-chosen"} {"ts": "2026-06-13T04:14:48Z", "role": "role-implementer", "convoy": "rotate-default-admin", "repo": "tcg-vault", "skip_flags": [], "classification": "security", "duration_s": 900, "outcome": "pr-open"} +{"ts": "2026-06-13T06:16:56Z", "role": "role-architect", "convoy": "enable-no-undef-eslint-rule", "repo": "tcg-vault", "skip_flags": [], "classification": "ci", "duration_s": 180, "outcome": "architecture-only"} +{"ts": "2026-06-13T06:16:56Z", "role": "role-implementer", "convoy": "enable-no-undef-eslint-rule", "repo": "tcg-vault", "skip_flags": [], "classification": "ci", "duration_s": 900, "outcome": "pr-open"} diff --git a/.convoys/enable-no-undef-eslint-rule.md b/.convoys/enable-no-undef-eslint-rule.md new file mode 100644 index 0000000..865dc1e --- /dev/null +++ b/.convoys/enable-no-undef-eslint-rule.md @@ -0,0 +1,59 @@ +--- +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) diff --git a/components/CollectionPageView.js b/components/CollectionPageView.js index 72fbdc3..98ae6e7 100644 --- a/components/CollectionPageView.js +++ b/components/CollectionPageView.js @@ -71,6 +71,8 @@ export default function CollectionPageView(props) { showShareModal, showUploadModal, sortBy, + toggleFavorite, + togglePublic, user, viewMode } = props; diff --git a/components/ShareModal.js b/components/ShareModal.js index 4f425ca..88fbf5f 100644 --- a/components/ShareModal.js +++ b/components/ShareModal.js @@ -1,4 +1,4 @@ -import { useState, useEffect } from 'react'; +import { useState, useEffect, useCallback } from 'react'; import { Modal, Button } from './ui'; export default function ShareModal({ @@ -15,6 +15,27 @@ export default function ShareModal({ const [currentUser, setCurrentUser] = useState(null); const [copySuccess, setCopySuccess] = useState(false); + // Component-scoped so `handleInvite` can call this after an + // invite succeeds (previously it was scoped inside the useEffect + // below, which silently threw a ReferenceError when handleInvite + // tried to refresh the list after invite — caught by `no-undef` + // post-PR #144). + const fetchInvitedUsers = useCallback(async () => { + try { + const response = await fetch(`/api/collections/${collectionId}/permissions`, { + headers: { + Authorization: `Bearer ${localStorage.getItem('auth_token')}`, + }, + }); + if (response.ok) { + const data = await response.json(); + setInvitedUsers(data.permissions || []); + } + } catch (error) { + console.error('Error fetching invited users:', error); + } + }, [collectionId]); + useEffect(() => { if (!isOpen) return; @@ -34,25 +55,10 @@ export default function ShareModal({ } }; - const fetchInvitedUsers = async () => { - try { - const response = await fetch(`/api/collections/${collectionId}/permissions`, { - headers: { - Authorization: `Bearer ${localStorage.getItem('auth_token')}`, - }, - }); - if (response.ok) { - const data = await response.json(); - setInvitedUsers(data.permissions || []); - } - } catch (error) { - console.error('Error fetching invited users:', error); - } - }; - + // eslint-disable-next-line react-hooks/set-state-in-effect -- async fetch; setState fires after the fetch resolves, not synchronously fetchInvitedUsers(); fetchCurrentUser(); - }, [isOpen, collectionId]); + }, [isOpen, collectionId, fetchInvitedUsers]); const handleSearch = async (query) => { setSearchQuery(query); diff --git a/eslint.config.mjs b/eslint.config.mjs index 1705fe9..d265de2 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -11,6 +11,91 @@ const eslintConfig = defineConfig([ 'next-env.d.ts', 'scripts/migrations/**', ]), + // The flat config from `eslint-config-next/core-web-vitals` does NOT + // enable the core `no-undef` rule for plain-JS identifier references + // — only `react/jsx-no-undef`, which catches undefined JSX components + // but NOT plain function/variable references like `useFocusTrap(...)` + // in a hook call. PR #144 (`31da384`) shipped a runtime + // ReferenceError to production because of this gap; that bug would + // have been caught at lint time with the rule on. We enable it + // directly rather than pulling in `@eslint/js/recommended` (which + // would also turn on `no-unused-vars`, `no-prototype-builtins`, and + // a handful of others that surface a flood of pre-existing + // violations and risk derailing this hotfix-class change). + // + // Browser + Node globals (window, document, process, Buffer, etc.) + // are sourced from the language-options block below. + { + rules: { + 'no-undef': 'error', + }, + languageOptions: { + globals: { + // Browser + window: 'readonly', + document: 'readonly', + navigator: 'readonly', + fetch: 'readonly', + FormData: 'readonly', + File: 'readonly', + Blob: 'readonly', + URL: 'readonly', + URLSearchParams: 'readonly', + localStorage: 'readonly', + sessionStorage: 'readonly', + location: 'readonly', + history: 'readonly', + alert: 'readonly', + confirm: 'readonly', + prompt: 'readonly', + atob: 'readonly', + btoa: 'readonly', + crypto: 'readonly', + WebSocket: 'readonly', + Image: 'readonly', + MediaStream: 'readonly', + MediaStreamTrack: 'readonly', + ImageCapture: 'readonly', + MediaRecorder: 'readonly', + AbortController: 'readonly', + Event: 'readonly', + CustomEvent: 'readonly', + HTMLElement: 'readonly', + HTMLInputElement: 'readonly', + HTMLImageElement: 'readonly', + HTMLVideoElement: 'readonly', + HTMLCanvasElement: 'readonly', + Element: 'readonly', + Node: 'readonly', + NodeList: 'readonly', + DOMException: 'readonly', + IntersectionObserver: 'readonly', + MutationObserver: 'readonly', + ResizeObserver: 'readonly', + requestAnimationFrame: 'readonly', + cancelAnimationFrame: 'readonly', + getComputedStyle: 'readonly', + // Timers (shared between browser + node) + setTimeout: 'readonly', + clearTimeout: 'readonly', + setInterval: 'readonly', + clearInterval: 'readonly', + queueMicrotask: 'readonly', + // Console + globals + console: 'readonly', + globalThis: 'readonly', + // Node / Next.js runtime + process: 'readonly', + Buffer: 'readonly', + __dirname: 'readonly', + __filename: 'readonly', + module: 'readonly', + require: 'readonly', + exports: 'readonly', + global: 'readonly', + }, + }, + }, { files: ['scripts/**/*.js'], rules: { @@ -20,6 +105,26 @@ const eslintConfig = defineConfig([ }], }, }, + // Vitest test files have an additional set of globals (describe, it, + // expect, vi, beforeEach, etc.). Defining them here keeps the test + // files from triggering no-undef while leaving the rule strict for + // source files. + { + files: ['test/**/*.js', '**/*.test.js', '**/*.test.ts', 'test/setup.js'], + languageOptions: { + globals: { + describe: 'readonly', + it: 'readonly', + test: 'readonly', + expect: 'readonly', + vi: 'readonly', + beforeEach: 'readonly', + afterEach: 'readonly', + beforeAll: 'readonly', + afterAll: 'readonly', + }, + }, + }, ]); export default eslintConfig; diff --git a/lib/use-collection-view.js b/lib/use-collection-view.js index 36880f8..f645489 100644 --- a/lib/use-collection-view.js +++ b/lib/use-collection-view.js @@ -496,6 +496,8 @@ export function useCollectionView({ user = null, authLoading = true } = {}) { showShareModal, showUploadModal, sortBy, + toggleFavorite, + togglePublic, viewMode }; }