fix(useGridState): prevent infinite loop restoring focus when no focusable row remains#10241
Open
RobHannay wants to merge 6 commits into
Open
Conversation
When the focused row is removed from the collection, the focus-restoration effect searches collection.rows for a non-disabled, non-headerrow row to move focus to. The single loop walked forward to the end, then jumped back to parentNode.index and decremented — but the next iteration's `index < rows.length - 1` check sent it forward again, so when every row in the search range was disabled (or a headerrow) it bounced between parentNode.index and the end forever and never reached `index < 0`. This hangs the main thread (e.g. a table whose only rows are disabled loading skeletons while a row is focused). Replace the bouncing loop with two bounded passes — forward from the start index, then backward — mirroring the already-correct walk in useListState. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Start the forward scan at Math.max(0, index) so an empty rows array (index === -1 from Math.min(..., rows.length - 1)) no longer reads rows[-1], matching the old `while (index >= 0)` no-op. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Removing the focused row while every remaining row is disabled previously infinite-looped in useGridState's focus restoration. Without the fix this test hangs; with it, focus is cleared and the render completes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e-focus-restoration-infinite-loop
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
useGridState's focus-restoration effect (which runs when the focused row is removed from the collection) searchescollection.rowsfor a non-disabled, non-headerrowrow to move focus to. It's meant to scan forward from the deleted position to the end, then fall back to scanning backward — but the forward/backward handoff is broken:After the forward pass reaches the end and resets to
parentNode.index/ decrements, the next iteration'sindex < rows.length - 1check sends it forward again. So when no focusable row remains (every row in range is disabled or aheaderrow),indexbounces betweenparentNode.indexand the end and never reachesindex < 0— an infinite loop that hangs the main thread.This is reachable whenever the focused row is removed and the new collection has no focusable row — e.g. a table whose rows are momentarily all-disabled loading skeletons while a row was focused. In our app it hard-froze the browser tab on filter/search/tab changes (Firefox "page is slowing down", Chrome unresponsive), independent of row count.
Fix
Replace the single bouncing loop with two bounded passes — forward from the start index, then backward — mirroring the already-correct walk in
useListState(getKeyAfter/getKeyBefore).Math.max(0, index)keeps an emptyrowsarray (index === -1) from readingrows[-1], matching the oldwhile (index >= 0)no-op.Only
useGridStateis affected;useListStateanduseTreeStatealready handle this case safely.How I tested
Added a regression test in
react-aria-components/test/Table.test.js: focus a row, then re-render with that row removed and every remaining row disabled.supports removing rowstest still passes, so focus restoration on normal removal is unchanged.🧢 Your Project
react-aria-components / @react-stately/grid (Table)