fix: clear search navbar input only when focused on esc key event#39464
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughThe Escape key handling in the NavBarSearch component has been refined to only trigger the search close logic when the overlay is open or focus is on the trigger element, replacing the previous behavior of always preventing default and invoking close logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
No issues found across 1 file
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/navbar/NavBarSearch/NavBarSearch.tsx (1)
64-68: Logic is sound, but consider addingstateto the dependency array for clarity.The condition correctly restricts Escape handling to when the overlay is open or the input is focused. The dependency on
stateis implicitly maintained viahandleEscSearch, but addingstatedirectly to line 75's dependency array would make this relationship explicit and avoid subtle stale-closure risks ifhandleEscSearchis refactored.♻️ Suggested improvement
- }, [focusManager, handleEscSearch, setFocus]); + }, [focusManager, handleEscSearch, setFocus, state]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/navbar/NavBarSearch/NavBarSearch.tsx` around lines 64 - 68, The Escape key handler references `state` (and `triggerRef`) but the effect that registers this handler only lists `handleEscSearch` in its dependency array; update the effect that adds the 'Escape' keyboard binding in NavBarSearch to include `state` in its dependency array so the closure sees the latest `state` values (this is the same effect that uses the 'Escape' mapping and calls `handleEscSearch`), keeping `handleEscSearch` and `triggerRef` as-is to avoid stale-closure bugs if `handleEscSearch` is refactored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/client/navbar/NavBarSearch/NavBarSearch.tsx`:
- Around line 64-68: The Escape key handler references `state` (and
`triggerRef`) but the effect that registers this handler only lists
`handleEscSearch` in its dependency array; update the effect that adds the
'Escape' keyboard binding in NavBarSearch to include `state` in its dependency
array so the closure sees the latest `state` values (this is the same effect
that uses the 'Escape' mapping and calls `handleEscSearch`), keeping
`handleEscSearch` and `triggerRef` as-is to avoid stale-closure bugs if
`handleEscSearch` is refactored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c0d7e5c-96b5-46de-a292-8a5b9e64be5b
📒 Files selected for processing (1)
apps/meteor/client/navbar/NavBarSearch/NavBarSearch.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/navbar/NavBarSearch/NavBarSearch.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
🔇 Additional comments (1)
apps/meteor/client/navbar/NavBarSearch/NavBarSearch.tsx (1)
64-68: PR objectives mismatch: linked issue#39461appears unrelated.The linked issue discusses deprecation warnings for anonymous read/write settings (
Accounts_AllowAnonymousRead,Accounts_AllowAnonymousWrite), while this PR modifies Escape key handling in the search navbar. Please verify the correct issue is linked.
fix: Prevent global Escape key press from unexpectedly clearing search
Proposed changes (including videos or screenshots)
The Problem:
Currently, the
Escapekey listener within NavBarSearch.tsx is attached globally to thewindowobject usingtinykeys. This causes an issue where pressingEscapeanywhere in the application triggers thehandleEscSearch()function, clearing and resetting the search even when the search input is not focused or the search dropdown list is completely closed.The Fix:
This PR modifies the event handler to verify whether the search component is currently active before taking action. It verifies that the search input is focused (
document.activeElement === triggerRef.current) or the search dropdown is visibly open (state.isOpen). Theevent.preventDefault()andhandleEscSearch()functions are now appropriately restricted to execute only when the user is actively interacting with the search.https://github.com/user-attachments/assets/b46069ec-7b9a-4749-9f39-24926a1340e1
Issue(s)
Closes #39462
Steps to test or reproduce
Escapekey.Escapekey.Further comments
This change preserves the intended accessibility shortcuts of NavBarSearch without unintentionally interfering with other components that may rely on
Escapekey functionality globally.Summary by CodeRabbit