fix(catalog,ci): catch hook-order violations + add CI gate

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.
This commit is contained in:
simoleo89
2026-05-19 17:57:28 +02:00
parent 6bf3366af7
commit a029ee63cb
5 changed files with 74 additions and 7 deletions
+9
View File
@@ -82,6 +82,15 @@ jobs:
working-directory: Nitro-V3 working-directory: Nitro-V3
run: yarn typecheck 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 - name: Vitest
working-directory: Nitro-V3 working-directory: Nitro-V3
run: yarn test --run run: yarn test --run
+43
View File
@@ -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'
}
}
];
+1
View File
@@ -12,6 +12,7 @@
"build:prod": "npx browserslist@latest --update-db && yarn build", "build:prod": "npx browserslist@latest --update-db && yarn build",
"preview": "vite preview --host", "preview": "vite preview --host",
"eslint": "eslint ./src", "eslint": "eslint ./src",
"lint:hooks": "eslint --config eslint.hooks.config.mjs ./src",
"typecheck": "tsgo --noEmit", "typecheck": "tsgo --noEmit",
"test": "vitest run", "test": "vitest run",
"test:watch": "vitest" "test:watch": "vitest"
@@ -26,13 +26,13 @@ export const CatalogItemGridWidgetView: FC<CatalogItemGridWidgetViewProps> = pro
if(elementRef && elementRef.current) elementRef.current.scrollTop = 0; if(elementRef && elementRef.current) elementRef.current.scrollTop = 0;
}, [ currentPage ]); }, [ currentPage ]);
if(!currentPage) return null; // Drag-and-drop handlers — hooks MUST run unconditionally so the
// hook order stays stable when currentPage flips from null to a
const selectOffer = (offer: IPurchasableOffer) => // real value (the `if(!currentPage) return null` below would
{ // otherwise hide these from the first render and React would flag
selectCatalogOffer(offer); // "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) => const handleDragStart = useCallback((index: number) =>
{ {
setDragIndex(index); setDragIndex(index);
@@ -68,6 +68,13 @@ export const CatalogItemGridWidgetView: FC<CatalogItemGridWidgetViewProps> = pro
setDropIndex(null); setDropIndex(null);
}, []); }, []);
if(!currentPage) return null;
const selectOffer = (offer: IPurchasableOffer) =>
{
selectCatalogOffer(offer);
};
return ( return (
<AutoGrid columnCount={ columnCount } innerRef={ elementRef } { ...rest }> <AutoGrid columnCount={ columnCount } innerRef={ elementRef } { ...rest }>
{ currentPage.offers && (currentPage.offers.length > 0) && currentPage.offers.map((offer, index) => { currentPage.offers && (currentPage.offers.length > 0) && currentPage.offers.map((offer, index) =>
@@ -59,6 +59,7 @@ describe('use-between + useSyncExternalStore incompatibility', () =>
const Broken = () => const Broken = () =>
{ {
// eslint-disable-next-line react-hooks/rules-of-hooks -- intentional: this test asserts the runtime crash
useBetween(() => useSyncExternalStore(() => () => undefined, () => 'v', () => 'v')); useBetween(() => useSyncExternalStore(() => () => undefined, () => 'v', () => 'v'));
return null; return null;
}; };
@@ -85,9 +86,15 @@ describe('use-between + useSyncExternalStore incompatibility', () =>
{ {
const sharedState = () => ({ count: 0 }); 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 = () => 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); const shared = useBetween(sharedState);
// eslint-disable-next-line react-hooks/rules-of-hooks -- intentional: same reason as above
const external = useSyncExternalStore(() => () => undefined, () => 'value', () => 'value'); const external = useSyncExternalStore(() => () => undefined, () => 'value', () => 'value');
return { ...shared, external }; return { ...shared, external };