fix: search bar clipboard and focus improvements#3025
fix: search bar clipboard and focus improvements#3025sawka merged 2 commits intowavetermdev:mainfrom
Conversation
Navigating search results in the terminal causes xterm.js to change the selection programmatically, which was triggering copy-on-select and clobbering the clipboard. Skip the clipboard write when an element inside .search-container is the active element.
|
|
WalkthroughChanges add a focus-control mechanism for the search input and adjust copy-on-select behavior. keymodel.ts now increments a block-specific focusInput counter when activating an already-open search instead of always reopening it. search.tsx introduces a focusInput atom, threads it through the Search component and useSearch API, and uses an input ref to programmatically focus/select the input when focusInput is incremented; it also resets the counter on close. termwrap.ts skips copy-on-select when the focused element is inside a search container. frontend/types/custom.d.ts adds the focusInput type to SearchAtoms. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
SummaryThis PR implements two UX improvements for the in-app search:
Note on Existing CommentThe existing inline comment about |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/app/store/keymodel.ts (1)
698-706: Global DOM query may select wrong search input when multiple blocks have search open.
document.querySelector(".search-container input")returns the first matching element in DOM order, not necessarily the one belonging to the currently focused block. If multiple blocks have their search panels open, this could focus the wrong input.Consider scoping the query to the focused block's container, or adding a ref to
SearchAtomsfor direct access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/store/keymodel.ts` around lines 698 - 706, The DOM query using document.querySelector in the block that checks globalStore.get(bcm.viewModel.searchAtoms.isOpen) may pick the first .search-container input in the whole page, so change it to target the input for the currently focused block instead: locate the block/container that owns the active search (e.g., use the focused block element or a block-specific identifier) and run querySelector on that container, or better, add and use a ref on the SearchAtoms component to directly access its input; update the logic around globalStore.set(bcm.viewModel.searchAtoms.isOpen) and the focus/select calls to use that scoped container or ref so the correct input is focused and selected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/app/store/keymodel.ts`:
- Around line 698-706: The DOM query using document.querySelector in the block
that checks globalStore.get(bcm.viewModel.searchAtoms.isOpen) may pick the first
.search-container input in the whole page, so change it to target the input for
the currently focused block instead: locate the block/container that owns the
active search (e.g., use the focused block element or a block-specific
identifier) and run querySelector on that container, or better, add and use a
ref on the SearchAtoms component to directly access its input; update the logic
around globalStore.set(bcm.viewModel.searchAtoms.isOpen) and the focus/select
calls to use that scoped container or ref so the correct input is focused and
selected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a0451ce5-992e-4a9a-9214-c5465e55cc65
📒 Files selected for processing (2)
frontend/app/store/keymodel.tsfrontend/app/view/term/termwrap.ts
There was a problem hiding this comment.
Pull request overview
This PR improves the in-app search UX by preventing unintended clipboard writes during search-result navigation in terminal blocks, and by ensuring repeated Cmd+F refocuses the existing search input.
Changes:
- Skip terminal copy-on-select when focus is inside the search UI, avoiding clipboard pollution during search navigation.
- When search is already open, Cmd+F now focuses and selects the search input instead of being a no-op.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| frontend/app/view/term/termwrap.ts | Prevents copy-on-select from firing while the search UI is focused (avoids overwriting clipboard during result navigation). |
| frontend/app/store/keymodel.ts | Enhances Cmd+F behavior to refocus/select the search input if search is already open. |
When the search bar was already open, Cmd+F was a no-op because setting the isOpen atom to true again had no effect. Fix uses a per-instance focusInput atom (a monotonic counter) on SearchAtoms: activateSearch increments it for the focused block's search instance, and SearchComponent watches it to call focus()+select() on its own inputRef — avoiding a global DOM query that would target the wrong block when multiple searches are open simultaneously. The counter is reset to 0 on close so re-opening doesn't re-trigger the focus effect.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/app/element/search.tsx (1)
63-70: Consider addingisOpento the dependency array or suppressing the lint warning.The effect reads
isOpenbut only listsfocusInputCounteras a dependency. While the logic appears intentional (initial open is handled byautoFocus, and this effect only reacts to explicit focus requests), this will trigger ESLint'sreact-hooks/exhaustive-depsrule. AddingisOpento the dependency array is safe here since the counter resets to 0 on close, making the conditionfocusInputCounter > 0 && isOpeneffectively guarded.♻️ Suggested fix
useEffect(() => { if (focusInputCounter > 0 && isOpen) { inputRef.current?.focus(); inputRef.current?.select(); } - }, [focusInputCounter]); + }, [focusInputCounter, isOpen]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/element/search.tsx` around lines 63 - 70, The useEffect that focuses the input (the effect referencing focusInputCounter, isOpen, and inputRef.current) currently only lists focusInputCounter in its dependencies which triggers react-hooks/exhaustive-deps; update the dependency array to include isOpen as well (i.e., useEffect(..., [focusInputCounter, isOpen])) so the hook correctly re-evaluates when open state changes, or alternatively add an explicit eslint-disable-next-line comment if you intentionally want to suppress the rule; locate the effect in the Search component (the useEffect that focuses and selects inputRef.current) and apply the dependency change or lint suppression accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/app/element/search.tsx`:
- Around line 63-70: The useEffect that focuses the input (the effect
referencing focusInputCounter, isOpen, and inputRef.current) currently only
lists focusInputCounter in its dependencies which triggers
react-hooks/exhaustive-deps; update the dependency array to include isOpen as
well (i.e., useEffect(..., [focusInputCounter, isOpen])) so the hook correctly
re-evaluates when open state changes, or alternatively add an explicit
eslint-disable-next-line comment if you intentionally want to suppress the rule;
locate the effect in the Search component (the useEffect that focuses and
selects inputRef.current) and apply the dependency change or lint suppression
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bb523fdf-0389-4c32-8a60-f60443825165
📒 Files selected for processing (3)
frontend/app/element/search.tsxfrontend/app/store/keymodel.tsfrontend/types/custom.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/store/keymodel.ts
|
@Shay12tg thanks for this thoughtful and targeted contribution. Just tested, works great. |
Two small fixes to the in-app search feature:
search results in the terminal, the clipboard kept getting overwritten with
the matched text on every step. This was annoying on its own, but also
polluted paste history managers. The root cause was
that xterm.js updates the terminal selection programmatically to highlight
each match, which triggered the copy-on-select handler. The fix skips the
clipboard write whenever an element inside
.search-containeris the activeelement.
bar was already open was a no-op (setting the
isOpenatom totrueagainhas no effect). The fix detects the already-open case and directly calls
focus()+select()on the input, so the user can immediately type a newquery.