chore(lint): enable rules-of-hooks, set-state-in-effect, no-leaked-conditional-rendering

[carlosthe19916]

Tracking issue: #307
Task: Task 6 — React hooks safety rules

---

Summary

Enable three React safety rules and remove one duplicate (9 violations fixed):

• @eslint-react/rules-of-hooks — 1 false positive in e2e (Playwright fixture use(), not React). Inline-disabled.
• @eslint-react/set-state-in-effect — 8 warnings across 4 files. Refactored useFilterState.ts to use useRef instead of useState+useEffect for a one-time flag. The remaining 7 are legitimate patterns (prop sync, URL→state sync, default init) with inline disables.
• @eslint-react/no-leaked-conditional-rendering — 0 violations, clean.
• react-hooks/rules-of-hooks — removed entirely (duplicate of @eslint-react/rules-of-hooks).

Files changed:

• eslint.config.mjs — removed 4 rule entries
• useFilterState.ts — refactored useState+useEffect → useRef
• SearchFilterControl.tsx — inline disable for prop sync
• Explorer.tsx — inline disable for URL→state sync
• context.tsx (Rekor) — inline disable for default initialization
• e2e/tests/fixtures/coverage.ts — inline disable for Playwright fixture

Test plan

• npm run lint passes with zero warnings/errors
• npm run test — all 41 test files pass (422 tests)
• npm run build — production build succeeds
• Verify Rekor search, filter toolbar, and TrustRoot certificates table still work in the UI

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.48%. Comparing base (ebfe3e6) to head (141993a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #313      +/-   ##
==========================================
- Coverage   76.51%   76.48%   -0.04%     
==========================================
  Files         105      105              
  Lines        1554     1552       -2     
  Branches      466      465       -1     
==========================================
- Hits         1189     1187       -2     
  Misses        325      325              
  Partials       40       40              

Flag	Coverage Δ
e2e	65.09% <90.00%> (-0.06%)	⬇️
unit	61.68% <100.00%> (-0.05%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[tsd-agent-bot]

🤖 Automated review by tsd-agent-bot (tsd-agent-lab pr-review skill). Findings are advisory — please verify before acting on them.

Full review (53 lines)

PR #313 Review: chore(lint): enable rules-of-hooks, set-state-in-effect, no-leaked-conditional-rendering

Summary of Intent

This PR re-enables three previously-disabled ESLint rules (react-hooks/rules-of-hooks, @eslint-react/rules-of-hooks, @eslint-react/set-state-in-effect, and @eslint-react/no-leaked-conditional-rendering) in eslint.config.mjs, then resolves the resulting violations — three via targeted eslint-disable-line suppressions with justifying comments (SearchFilterControl.tsx, Explorer.tsx, context.tsx), one via suppression in an e2e fixture (coverage.ts), and one via an actual refactor that eliminates the underlying pattern rather than suppressing it (useFilterState.ts, replacing a useState+useEffect "is this initial render" latch with a useRef mutated during render).

Critical

None.

High

None.

Medium

1. Location: client/src/app/hooks/TableControls/filtering/useFilterState.ts, lines 34–39 (isInitialLoadRef.current = false inside the render body)
   Issue: Mutating ref.current directly during render is unconventional and, on first read, looks like the classic "impure render" anti-pattern that breaks under React Strict Mode's double-invocation of render functions in development. Tracing it through carefully: refs do persist their mutated value across Strict Mode's two same-mount invocations (this is real and documented), so the second invocation sees isInitialLoadRef.current === false and would compute initialFilterValues = {} instead of args.initialFilterValues. However, this doesn't matter for correctness, because useState(initialFilterValues) only ever consumes its argument on the hook's true first initialization for a given fiber — the second Strict-Mode invocation's useState call resolves against the already-created hook entry and discards whatever initialFilterValues it was just handed. The net effect is that filterValues is correctly seeded from args.initialFilterValues regardless of which invocation "wins," and this matches React's own documented exception to "don't mutate refs during render" — the idempotent, guarded, once-only lazy-initialization pattern (if (ref.current === null) ref.current = ...). Concurrent-mode render restarts before commit also don't threaten this, since a fully discarded/restarted mount gets a brand-new fiber and thus a brand-new ref (current reset to true), not a corrupted carryover.
   Recommendation: Not a bug, but the pattern is subtle enough that a future contributor could plausibly "fix" it into something unsafe (e.g., moving the ref mutation into a conditional that also reads other closure variables, or assuming it's equivalent to a plain instance flag). Add a short comment above isInitialLoadRef noting this is a one-time, idempotent lazy-latch consumed only by the subsequent useState initializer, and that its safety depends on useState never re-consulting its initial argument after mount. A unit test asserting that initialFilterValues is applied exactly once and ignored on subsequent prop/arg changes would also guard against regression.
   Confidence: Medium (the render/hook internals reasoning is sound but not verifiable without running React's actual dev build under Strict Mode + a profiler in this review context).

2. Location: eslint.config.mjs hunk (removal of the four "off" overrides) vs. the diff's file list
   Issue: @eslint-react/no-leaked-conditional-rendering is being turned on repo-wide, but no file in this diff touches a conditional-rendering expression (e.g., {count && <X/>} → {count > 0 && <X/>}). Either the codebase already has zero violations of this rule, or violations exist elsewhere and were missed, which would fail CI post-merge (or worse, be silently accepted if the rule is set to "warn" rather than "error" — not visible in this diff excerpt).
   Recommendation: Confirm the PR's CI lint job actually ran clean against the full repository (not just the changed files) before merging. If CI is green, this is a non-issue; flagging only because it can't be confirmed from the diff alone.
   Confidence: Medium.

3. Location: client/src/app/pages/Rekor/shared/utils/rekor/api/context.tsx, lines 32–38
   Issue: The suppressed pattern (if (baseUrl === undefined) { setBaseUrl(default) } inside a useEffect) is exactly the shape set-state-in-effect is designed to catch, and in many cases (if baseUrl's only source of "undefined" is the initial mount, not a later prop reset) it's fixable outright rather than suppressible — e.g., seeding the useState for baseUrl with the fallback value directly instead of defaulting it post-mount via an effect. The PR chooses suppression here instead of a fix, unlike useFilterState.ts where an actual refactor was done.
   Recommendation: Verify whether baseUrl can legitimately become undefined again after mount (e.g., via a prop that changes). If not, initialize the state with the fallback value directly and drop the effect entirely, avoiding the suppression. If it can, the current suppression + comment is appropriate and this is a non-issue.
   Confidence: Low (the full component/props definition is outside the diff context, so this is speculative).

Low

1. Location: client/src/app/pages/Rekor/RekorSearch/components/Explorer.tsx, four separate // eslint-disable-line comments with identical text
   Issue: Repeating the same justification on four consecutive lines inside one effect is verbose and slightly harder to maintain (a future edit to one line could leave a stray, now-orphaned disable comment on a line that no longer needs it).
   Recommendation: Consider a single eslint-disable-next-line/block-scoped disable-and-comment above the effect body, or (if the plugin supports it) disabling the rule for the whole effect via a wrapping comment, rather than four line-level duplicates.
   Confidence: Low.

2. Location: client/src/app/hooks/TableControls/filtering/useFilterState.ts, imports and useState call style
   Issue: The file now mixes a namespaced React.useState call with a bare named useRef import, a pre-existing inconsistency that this diff's import-line change (import React, { useRef } from "react") slightly draws more attention to.
   Recommendation: Optional stylistic unification (either fully namespaced or fully named imports); not worth blocking on.
   Confidence: Low.

3. Location: e2e/tests/fixtures/coverage.ts, line above await use(context)
   Issue: The @eslint-react/rules-of-hooks false positive on Playwright's conventional use fixture parameter is correctly suppressed with a clear comment, but if this pattern recurs across other e2e fixture files, per-line suppressions won't scale.
   Recommendation: If more instances exist elsewhere in e2e/, consider an ESLint config override that disables @eslint-react/rules-of-hooks (and react-hooks/rules-of-hooks) for the e2e/ directory wholesale, rather than sprinkling inline disables. Not necessary for this single occurrence.
   Confidence: Low.

Overall Assessment

This is a small, low-risk, mechanical lint-hardening PR. The one substantive logic change — the useFilterState.ts refactor from a useState+useEffect "initial load" flag to a useRef mutated during render — initially looks like a risky pattern given React Strict Mode's render double-invocation and concurrent-mode render restarts, but on careful tracing through React's hook-resolution semantics it is safe: it mirrors React's own documented "lazy ref initialization during render" exception, and correctness is protected by useState's well-known behavior of only ever consulting its initializer argument once per mount. As a side benefit, it also removes an unnecessary extra render that the old effect-driven flag flip caused. All other changed files are purely additive comments (eslint-disable-line/eslint-disable-next-line with justifications) with no logic changes, so they carry effectively no regression risk on their own.

Recommendation: approve, no blocking issues, contingent on confirming CI's full-repo lint run is green (particularly for @eslint-react/no-leaked-conditional-rendering, which has no corresponding code changes in this diff to point to). Suggested follow-ups (non-blocking): add a clarifying comment on the isInitialLoadRef pattern, double-check whether the context.tsx suppression could instead be a real fix, and consolidate the repeated suppression comments in Explorer.tsx.