Skip to content

Commit 7ef5366

Browse files
frano-mclaude
andauthored
chore!: enable react-hooks v7 rules and address surfaced anti-patterns (#941) (#950)
* chore: enable react-hooks/immutability and enforce _ prefix for unused destructure siblings (#941) - Re-enable the react-hooks/immutability rule and fix the one violation in VariableSizeListItem: replace delete style.height (prop mutation) with a non-mutating destructure. - Tighten @typescript-eslint/no-unused-vars: drop the implicit ignoreRestSiblings allowance and instead require unused destructure siblings to be _-prefixed (argsIgnorePattern + varsIgnorePattern = ^_). This forces intentional-omit cases to be visibly marked. - Apply the convention to the existing intentional-omit sites and drop the now-redundant eslint-disable directives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: enable react-hooks/refs (#941) - useAsync: per-site suppress on the useReducer initialStateRef access. useReducer reads its second arg only on first render, at which point initialStateRef.current is the same `state` value the ref was just initialized with — safe. - FilterTag: per-site suppress on the overflow-detection block. The current ref-during-render measurement is genuinely broken (not reactive to resize / label change). Proper fix tracked in #949. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: document react-hooks/incompatible-library as not applicable (#941) The rule is a React-Compiler-aware check (e.g. TanStack Table / Virtual return values carry hidden mutable state). findable-ui does not use the React Compiler, so the rule is not earning its keep. Keep it off with an inline comment explaining the decision and pointing at the revisit trigger (Compiler adoption). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: address simple set-state-in-effect sites (#941) Five sites improved + one unused hook deleted. Rule itself stays off in this commit; it'll be flipped on once the remaining sites (MarkdownRenderer, UseTabs, useSessionTimeout, loginGuard, four terra/* hooks) are addressed. - useLocalStorage: migrate to useSyncExternalStore. Returns undefined from the server snapshot to preserve the "not loaded yet" sentinel that CookieBanner depends on; null or a string on the client. Reacts to cross-tab storage events. - useFeatureFlag: migrate to useSyncExternalStore. Returns false on the server, the FLAG.TRUE comparison on the client. - useResetableState: delete. Workspace audit found no consumers. The "sync prop to state via effect" shape it implemented is also an anti-pattern per current React docs. - EllipsisContent: drop the redundant mount effect that transitioned state from undefined to NONE (behaviorally identical to starting at NONE). Tighten helper signatures now that ellipsisMode is always defined. - CollapsableSection: replace the breakpoint useEffect with the React-docs adjust-during-render pattern. Replace the setTimeout-based transition-duration "settle" trick with Collapse's onEntered and onExited callbacks. Drop the outer JSX ternary; Collapse stays mounted and `in` is driven by `disabled || expanded` so desktop is force-open. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: enable react-hooks/set-state-in-effect (#941) Final batch of the set-state-in-effect cluster. All flagged sites addressed; rule now turned on at "error" level. Mix of structural fixes (preferred) and per-site suppressions (where the pattern is correct but the rule is overzealous). Structural fixes: - useHash: migrate to useSyncExternalStore subscribing to `hashchange`. Now SSR-safe (no hydration mismatch on URLs with fragments) and reactive to hash changes — both pre-existing gaps the old non-subscribing read had. - useTabs: drop the useState+useEffect indirection; derive `value` directly from `useHash()` + outline. Initial value is correct on first render — no DEFAULT_TAB_VALUE flash. - MarkdownRenderer: move the `setError(null)` reset into the fetch success handler so there is no synchronous setState in the effect body. Trade-off: stale error stays visible one parse cycle longer; net win in error→error transitions (no zero-error flash between). - useSessionTimeout: drop useLocation, use useSearchParams from next/navigation. Sticky semantics preserved via adjust-during-render with a prev-value tracker — the banner only goes false via clearSessionTimeout, not via unrelated navigation that happens to drop the URL param (the state-sync manager does this). - loginGuard provider: split the auth-success effect into adjust-during-render (setOpen(false)) + a separate effect that fires the stored callback. Clarifies pure-state vs side-effect. - useFetchProfiles: drop useState+useEffect; derive `status` directly during render via getProfileStatus(...). Per-site suppressions (with justification comments): - useFetchTerraNIHProfile, useFetchTerraProfile, useFetchTerraTermsOfService: async fetch pattern with a synchronous "set to PENDING" before the network call. Refactor would require a data-fetching library or external store. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: enable react-hooks/static-components (#941) Final rule from the v7 cluster. Codebase has zero violations, so this just turns the rule on at error level to prevent future regressions (the rule catches components defined inside other components, which create new component identity on every render). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: address copilot review on #941 markdown renderer - Move setError(null) out of the isValidElement branch so any successful parse clears a stale error, even when the result isn't a renderable element. - Set element to null in the non-element case so we don't leave a stale previous element rendering. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address copilot review on #941 useSessionTimeout init Initialize both isSessionTimeout and prevSessionTimeout from the current sessionTimeout value, so the adjust-during-render block doesn't fire on first mount when the URL param is absent (searchParams.get returns null in that case, which differed from the prior `undefined` init and forced a discarded render). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Fran McDade <18710366+frano-m@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 83903dd commit 7ef5366

23 files changed

Lines changed: 190 additions & 149 deletions

File tree

eslint.config.mjs

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,10 @@ const config = [
3838
],
3939
rules: {
4040
"@eslint-community/eslint-comments/require-description": "error",
41+
"@typescript-eslint/no-unused-vars": [
42+
"error",
43+
{ argsIgnorePattern: "^_", varsIgnorePattern: "^_" },
44+
],
4145
"jsdoc/check-alignment": "error",
4246
"jsdoc/check-param-names": "error",
4347
"jsdoc/require-description": "error",
@@ -50,15 +54,17 @@ const config = [
5054
"perfectionist/sort-enums": "error",
5155
"perfectionist/sort-interfaces": "error",
5256
"react-hooks/exhaustive-deps": "error",
53-
// The remaining react-hooks v7 rules below are React-Compiler-aware
54-
// checks that surface real anti-patterns in the codebase. Disabled here
55-
// to keep this PR scoped to the tooling upgrade; revisit alongside any
56-
// future React Compiler adoption.
57-
"react-hooks/immutability": "off",
57+
"react-hooks/immutability": "error",
58+
// react-hooks/incompatible-library is a React-Compiler-aware check
59+
// that flags library hooks whose return values carry hidden mutable
60+
// state the Compiler can't safely memoize (e.g. TanStack Table /
61+
// Virtual). findable-ui does not use the React Compiler, so the rule
62+
// is not earning its keep today. Revisit alongside any future React
63+
// Compiler adoption.
5864
"react-hooks/incompatible-library": "off",
59-
"react-hooks/refs": "off",
60-
"react-hooks/set-state-in-effect": "off",
61-
"react-hooks/static-components": "off",
65+
"react-hooks/refs": "error",
66+
"react-hooks/set-state-in-effect": "error",
67+
"react-hooks/static-components": "error",
6268
// sonarjs v1 dropped `cognitive-complexity` from its recommended preset
6369
// (the rule's meta lost its `recommended` flag during the v1 rewrite,
6470
// a discrepancy with the package README). Restore at default threshold

src/components/Filter/components/Filter/components/DrawerTransition/drawerTransition.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import { DrawerTransitionProps } from "./types";
1313
*/
1414
export const DrawerTransition = ({
1515
children,
16-
// eslint-disable-next-line @typescript-eslint/no-unused-vars -- destructured out so it doesn't spread onto the DOM Slide element
1716
placement: _placement,
1817
ref,
1918
...props

src/components/Filter/components/FilterTag/filterTag.tsx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,12 @@ export const FilterTag = ({
3333
}: FilterTagProps): JSX.Element => {
3434
const Tag = superseded ? SupersededTag : Chip;
3535
const tagRef = useRef<HTMLDivElement>(null);
36+
/* eslint-disable react-hooks/refs -- ref-during-render measurement; needs proper ResizeObserver-in-effect rewrite. Tracked in #949. */
3637
const tagLabelElement =
3738
tagRef.current?.querySelector<HTMLElement>(".MuiChip-label");
3839
const isOverflowed =
3940
(tagLabelElement?.offsetWidth ?? 0) < (tagLabelElement?.scrollWidth ?? 0);
41+
/* eslint-enable react-hooks/refs -- end suppression for #949. */
4042
return (
4143
<Tooltip
4244
arrow

src/components/Filter/components/SearchAllFilters/components/OutlinedInput/outlinedInput.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,12 @@ import { isButtonIn } from "./utils";
1010
export const OutlinedInput = (
1111
props: AutocompleteRenderInputParams,
1212
): JSX.Element => {
13-
// eslint-disable-next-line @typescript-eslint/no-unused-vars -- Intended behavior, destructure InputLabelProps, as they are not used on the component.
14-
const { InputLabelProps, InputProps, ...outlinedInputProps } = props;
13+
// Destructure InputLabelProps out so it isn't spread onto the component.
14+
const {
15+
InputLabelProps: _InputLabelProps,
16+
InputProps,
17+
...outlinedInputProps
18+
} = props;
1519
const { onClear, open, searchTerm, surfaceType } = useAutocomplete();
1620
return (
1721
<StyledOutlinedInput

src/components/Filter/components/VariableSizeListItem/variableSizeListItem.tsx

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,9 @@ export default function VariableSizeListItem({
3838
labelRanges,
3939
value: { count, key, label, selected },
4040
} = matchedItem;
41-
delete style.height; // Remove height style to allow variable size list to set item height.
41+
// Strip height so the variable-size list can drive item height via
42+
// onUpdateItemSizeByItemKey, without mutating the parent-owned style prop.
43+
const { height: _height, ...itemStyle } = style;
4244

4345
// Sets map of list item key to its height.
4446
useEffect(() => {
@@ -55,7 +57,7 @@ export default function VariableSizeListItem({
5557
ref={listItemRef}
5658
onClick={handleItemClicked}
5759
selected={selected}
58-
style={style}
60+
style={itemStyle}
5961
>
6062
<Checkbox
6163
checked={selected}
Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,33 @@
1-
import { isSSR } from "../../../../../../utils/ssr";
1+
import { useSyncExternalStore } from "react";
22
import { UseHash } from "./types";
33

44
/**
55
* Hook to get the current URL hash without the leading '#' character.
6-
*
7-
* @description
8-
* Extracts the hash from window.location.hash and removes the leading '#'.
9-
* Returns empty string when running in SSR environment.
10-
*
11-
* @returns Object containing the hash without leading '#', or empty string if SSR.
6+
* SSR-safe: returns an empty string on the server and during the hydration
7+
* pass; reads `window.location.hash` on the client. Reacts to `hashchange`
8+
* events so consumers re-render when the URL hash changes.
9+
* @returns Object containing the hash without leading '#', or empty string
10+
* on the server.
1211
*/
1312
export function useHash(): UseHash {
14-
if (isSSR()) return { hash: "" };
15-
const { hash } = window.location;
16-
return { hash: hash.replace(/^#/, "") };
13+
const hash = useSyncExternalStore(
14+
subscribe,
15+
() => window.location.hash.replace(/^#/, ""),
16+
() => "",
17+
);
18+
return { hash };
19+
}
20+
21+
/**
22+
* Subscribe a callback to the window's `hashchange` event.
23+
* Module-scope so its identity stays stable across renders (required by
24+
* useSyncExternalStore).
25+
* @param callback - Listener invoked when the URL hash changes.
26+
* @returns Unsubscribe function.
27+
*/
28+
function subscribe(callback: () => void): () => void {
29+
window.addEventListener("hashchange", callback);
30+
return (): void => {
31+
window.removeEventListener("hashchange", callback);
32+
};
1733
}

src/components/Layout/components/Outline/hooks/UseTabs/hook.ts

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,21 @@
11
import { TabsProps } from "@mui/material";
22
import Router from "next/router";
3-
import { SyntheticEvent, useCallback, useEffect, useState } from "react";
3+
import { SyntheticEvent, useCallback } from "react";
44
import { TABS_PROPS } from "../../../../../../styles/common/mui/tabs";
55
import { OutlineItem } from "../../types";
66
import { useHash } from "../UseHash/hook";
7-
import { DEFAULT_TAB_VALUE } from "./constants";
87
import { getNextValue } from "./utils";
98

109
export function useTabs(
1110
outline: OutlineItem[],
1211
): Pick<TabsProps, "indicatorColor" | "onChange" | "orientation" | "value"> {
13-
const [value, setValue] = useState<TabsProps["value"]>(DEFAULT_TAB_VALUE);
1412
const { hash } = useHash();
13+
const value = getNextValue(hash, outline);
1514

1615
const onChange = useCallback((_event: SyntheticEvent, hash: string): void => {
1716
Router.push({ hash });
1817
}, []);
1918

20-
useEffect(() => {
21-
setValue(getNextValue(hash, outline));
22-
}, [hash, outline]);
23-
2419
return {
2520
indicatorColor: value
2621
? TABS_PROPS.INDICATOR_COLOR.PRIMARY

src/components/MarkdownRenderer/markdownRenderer.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ export const MarkdownRenderer = ({
3535

3636
useEffect(() => {
3737
let cancelled = false;
38-
setError(null);
3938

4039
const processor = unified()
4140
.use(remarkParse)
@@ -55,7 +54,8 @@ export const MarkdownRenderer = ({
5554
.process(value)
5655
.then((file: VFile) => {
5756
if (cancelled) return;
58-
if (isValidElement(file.result)) setElement(file.result);
57+
setError(null);
58+
setElement(isValidElement(file.result) ? file.result : null);
5959
})
6060
.catch((err: Error) => {
6161
if (!cancelled) setError(err.message);

src/components/TableCreator/common/utils.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,10 @@ import { ColumnConfig } from "../../../config/entities";
1212
export function buildBaseColumnDef<T>(
1313
baseColumnConfig: ColumnConfig<T>,
1414
): ColumnDef<T> {
15+
// Destructure componentConfig out so it isn't passed through to the columnDef.
1516
const {
1617
columnPinned,
17-
// eslint-disable-next-line @typescript-eslint/no-unused-vars -- Destructure to avoid passing it to the columnDef.
18-
componentConfig,
18+
componentConfig: _componentConfig,
1919
header,
2020
id,
2121
meta,

src/components/common/EllipsisContent/ellipsisContent.tsx

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@ export const EllipsisContent = ({
2323
const ellipsisRef = useRef<HTMLDivElement>(null);
2424
const { height } =
2525
useResizeObserver(ellipsisRef, getBorderBoxSizeHeight) || {};
26-
const [ellipsisMode, setEllipsisMode] = useState<EllipsisMode | undefined>();
26+
const [ellipsisMode, setEllipsisMode] = useState<EllipsisMode>(
27+
EllipsisMode.NONE,
28+
);
2729
const isEllipsis = ellipsisMode === EllipsisMode.ON;
2830

2931
/**
@@ -38,14 +40,6 @@ export const EllipsisContent = ({
3840
});
3941
};
4042

41-
/**
42-
* Ellipsis mode state is initialized to "NONE" only when the component mounts,
43-
* so we guarantee that `ellipsisRef` gets the element handle for resize observer hook.
44-
*/
45-
useEffect(() => {
46-
setEllipsisMode(EllipsisMode.NONE);
47-
}, []);
48-
4943
/**
5044
* Ellipsis mode state updates with changes to the rendered content height.
5145
*/
@@ -119,7 +113,7 @@ function calculateRenderedLineCount(
119113
function getEllipsisMode(
120114
element: HTMLDivElement | null,
121115
maxLineCount: number,
122-
currentMode: EllipsisMode = EllipsisMode.NONE,
116+
currentMode: EllipsisMode,
123117
): EllipsisMode {
124118
// Calculate line count.
125119
const lineCount = calculateRenderedLineCount(element);
@@ -149,7 +143,7 @@ function getEllipsisMode(
149143
* @param currentMode - current ellipsis mode.
150144
* @returns string for display as button text.
151145
*/
152-
function getModeText(currentMode?: EllipsisMode): string {
146+
function getModeText(currentMode: EllipsisMode): string {
153147
if (currentMode === EllipsisMode.ON) {
154148
return "Read More";
155149
}
@@ -189,8 +183,8 @@ function getTextNodeDOMRects(node: Node, DOMRects: DOMRect[] = []): DOMRect[] {
189183
* @param currentMode - current ellipsis mode.
190184
* @returns true when mode is not "NONE".
191185
*/
192-
function isModeActivated(currentMode?: EllipsisMode): boolean {
193-
return !!currentMode && currentMode !== EllipsisMode.NONE;
186+
function isModeActivated(currentMode: EllipsisMode): boolean {
187+
return currentMode !== EllipsisMode.NONE;
194188
}
195189

196190
/**

0 commit comments

Comments
 (0)