fix(admin): keep hook order stable on content list when a refetch errors (#1415)#1570
Conversation
…eturns (emdash-cms#1415) The inline `React.useCallback` passed to `onLoadMore` sat below the `!manifest` / `!collectionConfig` / `error` guards. A render that took one of those early returns (e.g. when a sort change or fast navigation triggers a refetch that errors) ran one fewer hook than the preceding full render, tripping React emdash-cms#300 "Rendered fewer hooks than expected" — the same class as emdash-cms#439 / emdash-cms#467, which lifted other hooks above the guards. Lift the callback to `handleLoadMore` alongside the `items` useMemo so every render runs the same hooks regardless of which branch it takes. Adds a regression test driving a content refetch into an error after a successful render and asserting the ErrorScreen renders instead of crashing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: c8b09c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
area/admin
size/M
review/needs-review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approach
This is the right fix for a real bug, done the idiomatic way. ContentListPage declared onLoadMore={React.useCallback(...)} inline in its JSX, below three early returns (!manifest, !collectionConfig, error). A render that took a guard therefore ran one fewer hook than a full render → React #300 "Rendered fewer hooks than expected." Hoisting the useCallback above the guards is the canonical fix, and the added comment documents the invariant. Scope is correctly limited to ContentListPage (#1415); no drive-by edits.
What I verified
- The hook baseline is real.
ContentListPageis a child ofadminLayoutRoute, whoseRootComponentfetches the shared["manifest"]query and only renders<Shell><Outlet/></Shell>once it resolves (router.tsx:243-265). So onContentListPage's first mount the manifest is already cached →!manifest/!collectionConfig/errorare all false → the first render reaches the JSX and establishes the full hook count. A later erroring refetch then takes theerrorguard; in the buggy version that skipped the inlineuseCallback→ #300. The fix makes every render path run the same hooks. Confirmed correct. - The fix is complete. Reading the full function (lines 297–534), after this change no hooks remain below the early returns — only
const collectionConfig, thehandleLocaleChangearrow, and the JSX return. Nothing else was missed. - The test reproduces the crash. It waits for the "Add New" link (proving the full-hook baseline was established on a successful render), swaps the content handler to a 500, forces
queryClient.refetchQueries, and asserts the Retry button renders. React Query v5.90 setserroron a failed refetch even when cached data exists, so theif (error)guard fires — in the buggy version that render throws #300 and the Retry button never appears. Thescreen.getByRole(...)→await expect.element(...)query pattern matches the existing locale tests in the same file (nofindByRoleis used anywhere in the suite), so it's consistent with the codebase's waiting idiom. - No recurrence elsewhere. Grepped for inline
useCallback-in-JSX acrosspackages/admin/src; this was the only instance. - Conventions. Tabs, no new user-facing strings (Lingui/RTL N/A), no SQL/API changes. Changeset targets
@emdash-cms/adminpatch — correct.
Conclusion
Implementation is clean and correct, with a valid regression test. The only thing worth a look is one line of changeset prose that drifts into implementation mechanics — non-blocking.
| "@emdash-cms/admin": patch | ||
| --- | ||
|
|
||
| Fixes a crash on the content list when an action that refetches (changing the sort, fast navigation) coincides with a load error. The `onLoadMore` callback hook now sits above the page's early returns, so an error render runs the same number of hooks as a normal render instead of throwing React error #300. Closes #1415. |
There was a problem hiding this comment.
[suggestion] Per AGENTS.md, a changeset is release notes for someone upgrading — lead with the observable effect and leave out internal mechanics (file names, refactors, how you implemented it). The middle sentence here ("The onLoadMore callback hook now sits above the page's early returns, so an error render runs the same number of hooks as a normal render instead of throwing React error #300") is exactly the implementation detail a user reading the changelog doesn't need. The first sentence already states the observable effect; the mechanics sentence can go.
| Fixes a crash on the content list when an action that refetches (changing the sort, fast navigation) coincides with a load error. The `onLoadMore` callback hook now sits above the page's early returns, so an error render runs the same number of hooks as a normal render instead of throwing React error #300. Closes #1415. | |
| Fixes a crash on the content list when an action that refetches (changing the sort, fast navigation) coincides with a load error. Closes #1415. |
github-actions
Bot
added
review/approved
1 participant
What does this PR do?
Fixes a crash (React #300 "Rendered fewer hooks than expected") on the content list page when an action that triggers a refetch errors.
ContentListPagedeclared theonLoadMoreReact.useCallbackinline in JSX, below three early returns (!manifest,!collectionConfig,error). On the render that takes theerrorguard, that hook never runs, so the render executes one fewer hook than a successful render — which is exactly what React #300 forbids. Any refetch that fails (e.g. a transient API error after the list has loaded) crashed the whole page instead of showing the error state.Fix: lift the callback above the early returns so the hook count is identical on every render path. Added a comment documenting the invariant.
Closes #1415.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
New test in
packages/admin/tests/router.test.tsx: renders the page, swaps the content handler to a 500, refetches, and asserts the Retry button renders (the page no longer crashes). Targeted admin + core suites green; typecheck/lint/format clean.