From a029ee63cbd55caefc25cf6a48f706c540eefdbc Mon Sep 17 00:00:00 2001 From: simoleo89 Date: Tue, 19 May 2026 17:57:28 +0200 Subject: [PATCH] fix(catalog,ci): catch hook-order violations + add CI gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups to the CatalogPurchaseWidgetView fix (6bf3366): 1. CatalogItemGridWidgetView had the same shape — four useCallback declarations (handleDragStart / handleDragOver / handleDrop / handleDragEnd) sat below an `if(!currentPage) return null` early return. When currentPage flipped from null to a real page the hook count jumped by 4 and React would have thrown "Rendered more hooks than during the previous render" the moment any consumer rendered the grid in admin mode. Moved the four useCallback declarations above the early-return; their bodies are safe pre-load (only currentPage?.offers is accessed inside handleDrop, optional-chained already). 2. CI gate — the existing GitHub Actions workflow runs `yarn typecheck` and `yarn test`, but NOT `yarn eslint`. That's why this pattern slipped through twice in a row: ESLint flags it locally but no PR check enforces it. Full `yarn eslint` emits ~900 pre-existing baseline errors (brace-style, indentation, recommended TS rules — out of scope for this branch), so a blanket step would always fail. Instead added a focused `eslint.hooks.config.mjs` + `yarn lint:hooks` script that runs ESLint with ONLY `react-hooks/rules-of-hooks: error`. Wired into ci.yml between `typecheck` and `test`. The local repo now has zero violations of the rule. 3. useSessionSnapshots.test.tsx — added eslint-disable-next-line comments on the three lines that intentionally violate the rule (they're the assertions that the broken pattern crashes). Without the comments the new CI gate would fail on the regression-guard suite. Verification: yarn lint:hooks green, yarn typecheck clean, yarn test 209/209. --- .github/workflows/ci.yml | 9 ++++ eslint.hooks.config.mjs | 43 +++++++++++++++++++ package.json | 1 + .../widgets/CatalogItemGridWidgetView.tsx | 21 ++++++--- .../session/useSessionSnapshots.test.tsx | 7 +++ 5 files changed, 74 insertions(+), 7 deletions(-) create mode 100644 eslint.hooks.config.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ca03d52..c4981b2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -82,6 +82,15 @@ jobs: working-directory: Nitro-V3 run: yarn typecheck + # Hook-order lint gate — the full yarn eslint emits ~900 pre-existing + # baseline errors (brace style, indentation), so we use a focused + # config that asserts only react-hooks/rules-of-hooks. Catches the + # "hook below early-return" pattern that produced two production + # crashes this session (CatalogPurchaseWidgetView, CatalogItemGridWidgetView). + - name: ESLint (hook-order gate) + working-directory: Nitro-V3 + run: yarn lint:hooks + - name: Vitest working-directory: Nitro-V3 run: yarn test --run diff --git a/eslint.hooks.config.mjs b/eslint.hooks.config.mjs new file mode 100644 index 0000000..376ea5e --- /dev/null +++ b/eslint.hooks.config.mjs @@ -0,0 +1,43 @@ +// Minimal ESLint config focused on the Rules of Hooks. +// +// The full eslint.config.mjs runs the project's full lint baseline, +// which currently emits ~900 pre-existing errors (brace style, +// indentation, recommended TS rules) — those are tracked separately +// and would drown a CI signal. This config strips down to just the +// rule we care about as a gate: react-hooks/rules-of-hooks. +// +// Wired up as `yarn lint:hooks` (see package.json) and called from +// .github/workflows/ci.yml so a hook-order violation breaks the +// build the same way a typecheck or test failure would. + +import typescriptEslintParser from '@typescript-eslint/parser'; +import reactHooksPlugin from 'eslint-plugin-react-hooks'; +import path from 'path'; +import { fileURLToPath } from 'url'; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); + +export default [ + { + files: ['**/*.jsx', '**/*.js', '**/*.tsx', '**/*.ts'], + plugins: { + 'react-hooks': reactHooksPlugin + }, + languageOptions: { + parser: typescriptEslintParser, + ecmaVersion: 'latest', + parserOptions: { + sourceType: 'module', + project: './tsconfig.json', + tsconfigRootDir: __dirname, + ecmaFeatures: { + jsx: true + } + } + }, + rules: { + 'react-hooks/rules-of-hooks': 'error' + } + } +]; diff --git a/package.json b/package.json index 6c8a5a2..97bbc56 100644 --- a/package.json +++ b/package.json @@ -12,6 +12,7 @@ "build:prod": "npx browserslist@latest --update-db && yarn build", "preview": "vite preview --host", "eslint": "eslint ./src", + "lint:hooks": "eslint --config eslint.hooks.config.mjs ./src", "typecheck": "tsgo --noEmit", "test": "vitest run", "test:watch": "vitest" diff --git a/src/components/catalog/views/page/widgets/CatalogItemGridWidgetView.tsx b/src/components/catalog/views/page/widgets/CatalogItemGridWidgetView.tsx index deff50a..e34dbb8 100644 --- a/src/components/catalog/views/page/widgets/CatalogItemGridWidgetView.tsx +++ b/src/components/catalog/views/page/widgets/CatalogItemGridWidgetView.tsx @@ -26,13 +26,13 @@ export const CatalogItemGridWidgetView: FC = pro if(elementRef && elementRef.current) elementRef.current.scrollTop = 0; }, [ currentPage ]); - if(!currentPage) return null; - - const selectOffer = (offer: IPurchasableOffer) => - { - selectCatalogOffer(offer); - }; - + // Drag-and-drop handlers — hooks MUST run unconditionally so the + // hook order stays stable when currentPage flips from null to a + // real value (the `if(!currentPage) return null` below would + // otherwise hide these from the first render and React would flag + // "Rendered more hooks than during the previous render"). Bodies + // are safe to evaluate pre-load: currentPage? optional chaining + // already guards the only access inside handleDrop. const handleDragStart = useCallback((index: number) => { setDragIndex(index); @@ -68,6 +68,13 @@ export const CatalogItemGridWidgetView: FC = pro setDropIndex(null); }, []); + if(!currentPage) return null; + + const selectOffer = (offer: IPurchasableOffer) => + { + selectCatalogOffer(offer); + }; + return ( { currentPage.offers && (currentPage.offers.length > 0) && currentPage.offers.map((offer, index) => diff --git a/src/hooks/session/useSessionSnapshots.test.tsx b/src/hooks/session/useSessionSnapshots.test.tsx index c4c0acc..55b6af2 100644 --- a/src/hooks/session/useSessionSnapshots.test.tsx +++ b/src/hooks/session/useSessionSnapshots.test.tsx @@ -59,6 +59,7 @@ describe('use-between + useSyncExternalStore incompatibility', () => const Broken = () => { + // eslint-disable-next-line react-hooks/rules-of-hooks -- intentional: this test asserts the runtime crash useBetween(() => useSyncExternalStore(() => () => undefined, () => 'v', () => 'v')); return null; }; @@ -85,9 +86,15 @@ describe('use-between + useSyncExternalStore incompatibility', () => { const sharedState = () => ({ count: 0 }); + // Lowercase intentionally — this is a custom hook named like a + // regular function so the test reproduces the exact call shape + // a refactor might land on. The eslint disable below silences + // the "hooks must start with use" lint that flags the body. const safeHook = () => { + // eslint-disable-next-line react-hooks/rules-of-hooks -- intentional: function named like a hook to mirror real call sites const shared = useBetween(sharedState); + // eslint-disable-next-line react-hooks/rules-of-hooks -- intentional: same reason as above const external = useSyncExternalStore(() => () => undefined, () => 'value', () => 'value'); return { ...shared, external };