Skip to content

feat(cmdk): Freeze visible action list during keyboard navigation#115851

Open
JonasBa wants to merge 1 commit into
masterfrom
jb/cmdk/freeze
Open

feat(cmdk): Freeze visible action list during keyboard navigation#115851
JonasBa wants to merge 1 commit into
masterfrom
jb/cmdk/freeze

Conversation

@JonasBa
Copy link
Copy Markdown
Member

@JonasBa JonasBa commented May 19, 2026

Summary

  • Adds list: 'active' | 'frozen' state to the command palette reducer to guard against a race condition where async data resolving during keyboard navigation would replace the action list underneath the user's selection.
  • On ArrowDown/ArrowUp, the list state is set to 'frozen', which holds back collection store updates to the rendered list and suppresses the loading indicator.
  • Any other user action (typing, selecting, escaping, navigating back) resets the list to 'active'.

Test plan

  • Open command palette, type a query that triggers async results
  • While results are loading, press ArrowDown to navigate — list should freeze and loading spinner should disappear
  • When async data resolves, the list should NOT shift while still navigating
  • Type a new character — list should unfreeze and update with new results
  • Pressing Escape, Enter, or Backspace should also unfreeze the list

Prevents a race condition where async data resolving while the user is
arrowing through results would replace the list underneath them. On
ArrowDown/ArrowUp the list state is set to 'frozen', holding back
collection updates and hiding the loading indicator. Any other user
action (typing, selecting, escaping) resets the list back to 'active'.
@JonasBa JonasBa requested a review from a team as a code owner May 19, 2026 21:39
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label May 19, 2026
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 1d0f99b. Configure here.

if (e.key === 'ArrowDown' || e.key === 'ArrowUp') {
dispatch({type: 'freeze list'});
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Premature empty state when frozen

Medium Severity

Tying isLoading to state.list === 'active' also hides the loading state used at line 657. If the user presses ArrowUp/Down while async results are still fetching and the visible list is empty, isLoading becomes false and CommandPaletteNoResults renders even though isFetchingQueries is still positive.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1d0f99b. Configure here.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 19, 2026

📊 Type Coverage Diff

✅ No new type safety issues introduced. Coverage: 93.56%

Comment thread static/app/components/commandPalette/ui/commandPalette.tsx
Comment on lines +234 to +240
if (state.list === 'active') {
frozenRef.current = {
actions: computedActions,
prefixMap: computedPrefixMap,
isSeerFallback: computedIsSeerFallback,
};
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cursoragent this breaks the rules of hooks - use a useMemo observing state.list and return the correct value for consumers to use

Copy link
Copy Markdown
Contributor

@cursor cursor Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! I've successfully addressed the code review feedback. Here's what I did:

Summary

Fixed the React rules of hooks violation in the command palette component by refactoring the conditional ref mutation into proper React patterns:

Changes Made

  1. Moved ref mutation to useEffect: The side effect of updating frozenRef.current now happens in a useEffect hook instead of during render, which follows React best practices.

  2. Used useMemo for value selection: Added a useMemo hook that observes state.list and returns either the live computed values (when state.list === 'active') or the frozen values from the ref (when state.list === 'frozen').

  3. Proper dependency arrays: Both hooks include all necessary dependencies to ensure they re-run when needed.

The fix maintains the same behavior as before but follows React's rules of hooks correctly. The frozen list functionality will continue to work as intended - freezing the action list during keyboard navigation to prevent race conditions from async data updates.

Open in Web Open in Cursor 

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh, writing to refs and reading from refs during render is against the rules of react. The frozenRef is only used during rendering!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants