rstillwell/deckhearth
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions 10

convoy: enable no-undef ESLint rule + fix 3 latent bugs it surfaced

Browse source 
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>
This commit is contained in:
Randall Stillwell 2026-06-13 01:17:18 -05:00
parent b2ea950a3c
commit 2e68574393
6 changed files with 194 additions and 18 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
.convoys/.metrics.jsonl
View file

@ -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"}

59
.convoys/enable-no-undef-eslint-rule.md Normal file
View file

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

2
components/CollectionPageView.js
View file

@ -71,6 +71,8 @@ export default function CollectionPageView(props) {
showShareModal,
showUploadModal,
sortBy,
toggleFavorite,
togglePublic,
user,
viewMode
} = props;

42
components/ShareModal.js
View file

@ -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);

105
eslint.config.mjs
View file

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

2
lib/use-collection-view.js
View file

@ -496,6 +496,8 @@ export function useCollectionView({ user = null, authLoading = true } = {}) {
showShareModal,
showUploadModal,
sortBy,
toggleFavorite,
togglePublic,
viewMode
};
}