Improve search functionality #2712
Conversation
carl2027
commented
Dec 23, 2025
carl2027
commented
- Fixed: The search dialog now correctly receives focus when opened in the current block.
- Implemented direct search for selected text.
|
|
WalkthroughThis change enhances the search functionality by introducing ref management and keyboard shortcuts. The first file adds an optional 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
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/app/element/search.tsx (1)
149-149: Consider conditionally rendering thedata-blockidattribute.When
blockIdis undefined, this will renderdata-blockid=""ordata-blockid="undefined". The selector inkeymodel.ts(.search-container[data-blockid="${blockId}"]) would fail to match if blockId is undefined there, which is fine since there's a fallback. However, for cleaner DOM output, you could conditionally add the attribute.🔎 Optional: Conditionally render attribute
- <div className="search-container" style={{ ...floatingStyles }} ref={refs.setFloating} data-blockid={blockId}> + <div className="search-container" style={{ ...floatingStyles }} ref={refs.setFloating} {...(blockId && { "data-blockid": blockId })}>frontend/app/store/keymodel.ts (1)
631-652: Consider adding a guard for missing block component model.The
getSelectedTextfunction accessesbcm.viewModelwithout checking ifbcmis null first. While this is currently only called fromactivateSearchwherebcmexists, the function could be reused elsewhere. Also, theas anycast loses type safety.🔎 Optional: Add defensive check
function getSelectedText(): string { // Check for terminal selection first const bcm = getBlockComponentModel(getFocusedBlockInStaticTab()); + if (!bcm?.viewModel) { + return window.getSelection()?.toString().trim() || ""; + } if (bcm?.viewModel?.viewType === "term") { const termViewModel = bcm.viewModel as any;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/app/element/search.tsxfrontend/app/store/keymodel.tsfrontend/app/view/term/term.tsxfrontend/app/view/webview/webview.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/view/webview/webview.tsx (1)
frontend/app/element/search.tsx (1)
useSearch(196-219)
⏰ 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: merge-gatekeeper
🔇 Additional comments (7)
frontend/app/view/term/term.tsx (1)
181-181: LGTM!Correctly wires
blockIdto theuseSearchhook, enabling block-scoped search context for the terminal view. This aligns with the broader PR pattern.frontend/app/view/webview/webview.tsx (1)
824-824: LGTM!Correctly passes
model.blockIdtouseSearch, ensuring the webview search context is block-aware and consistent with the terminal view implementation.frontend/app/element/search.tsx (2)
19-19: LGTM!Adding optional
blockIdtoSearchPropsenables block-scoped search context.
218-218: LGTM!Properly propagates
blockIdfrom options to the return value ofuseSearch.frontend/app/store/keymodel.ts (3)
654-681: LGTM with a minor observation.The
setTimeout(..., 0)pattern is appropriate here to ensure the DOM has updated after React renders the search component. The fallback to any.search-container inputprovides good resilience.
683-713: LGTM!The enhanced
activateSearchfunction correctly handles both opening the search dialog with selected text and resetting the search state when already open. The logic properly distinguishes between the two cases and resetsresultsIndexandresultsCountwhen the search is reset.
714-721: LGTM!Clean implementation of
deactivateSearchthat properly closes the search dialog when open and returnstrueto indicate the event was handled.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/app/store/keymodel.ts (1)
631-652: Consider improving type safety.The function correctly prioritizes terminal selection over browser selection. However, the type cast to
anyat line 635 bypasses type safety.💡 Consider adding proper typing
If the terminal view model has a consistent interface, consider defining a proper type instead of using
any:+interface TerminalViewModel { + viewType: string; + termRef?: React.RefObject<{ + terminal: { + getSelection: () => string; + }; + }>; +} + function getSelectedText(): string { // Check for terminal selection first const bcm = getBlockComponentModel(getFocusedBlockInStaticTab()); if (bcm?.viewModel?.viewType === "term") { - const termViewModel = bcm.viewModel as any; + const termViewModel = bcm.viewModel as TerminalViewModel; if (termViewModel.termRef?.current?.terminal) {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/app/element/search.tsxfrontend/app/store/keymodel.tsfrontend/app/view/term/term.tsxfrontend/app/view/webview/webview.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/app/view/term/term.tsx
- frontend/app/view/webview/webview.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/store/keymodel.ts (2)
frontend/app/store/global.ts (2)
getBlockComponentModel(853-853)globalStore(865-865)pkg/vdom/vdom_types.go (1)
WaveKeyboardEvent(233-247)
⏰ 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: merge-gatekeeper
🔇 Additional comments (6)
frontend/app/element/search.tsx (4)
19-19: LGTM: Clean addition of block-scoped search support.The optional
blockIdprop is properly typed and destructured, enabling block-specific search contexts without breaking existing usage.Also applies to: 36-36
149-149: LGTM: DOM attribute enables block-scoped search targeting.The
data-blockidattribute correctly propagates the block identifier to the DOM, allowing the key handler inkeymodel.tsto focus the appropriate search input viaquerySelector.
193-193: LGTM: Type definition aligned with SearchProps.The addition of
blockIdtoSearchOptionsmaintains consistency withSearchPropsand supports the block-scoped search feature.
218-218: LGTM: Completes blockId propagation chain.The
useSearchhook correctly returnsblockIdfrom options, completing the propagation path that enables block-scoped search behavior.frontend/app/store/keymodel.ts (2)
683-713: LGTM: Enhanced search activation with selection support.The updated
activateSearchfunction correctly implements the PR objective:
- Pre-fills search with selected text when opening the dialog
- Resets search to current selection when already open
- Properly focuses the search input in both scenarios
The guard for Ctrl+f in terminal blocks (lines 686-688) is preserved, maintaining existing behavior.
714-732: LGTM: Clean search activation/deactivation integration.The
deactivateSearchfunction correctly closes the search dialog when open, and the key bindings are properly integrated:
- Cmd:f opens/resets search with selected text
- Escape closes search (after checking for modals first)
The priority order for Escape (modals → search → no-op) provides good UX.
carl2027
force-pushed
the
impr_find
branch
2 times, most recently
from
eb678b6 to
7afb70d
Compare
- Fixed: The search dialog now correctly receives focus when opened in the current block. - Implemented direct search for selected text.
|
ah, missed your PR with the holidays. thanks for submitting, will take a look |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/store/keymodel.ts (1)
656-677: Fix indentation inconsistency.The logic is correct and the setTimeout delay allows the search UI to render before focusing. However, lines 663-665 use tab characters for indentation instead of spaces, which is inconsistent with the rest of the file.
🔧 Proposed fix
if (bcm.viewModel.searchAtoms) { - let selectedText = getSelectedText(); - globalStore.set(bcm.viewModel.searchAtoms.isOpen, true); - globalStore.set(bcm.viewModel.searchAtoms.searchValue, selectedText); + let selectedText = getSelectedText(); + globalStore.set(bcm.viewModel.searchAtoms.isOpen, true); + globalStore.set(bcm.viewModel.searchAtoms.searchValue, selectedText); // Focus the search input using the exposed searchInputRef
🧹 Nitpick comments (1)
frontend/app/element/search.tsx (1)
212-224: Consider improving type safety.The implementation correctly exposes
searchInputReffor external focus control and properly cleans up on unmount. However, using(options.viewModel as any)to dynamically add properties bypasses TypeScript's type checking.Consider extending the
ViewModelinterface to include an optionalsearchInputRef?: React.RefObject<HTMLInputElement>property for better type safety. This is a minor concern since the current implementation works correctly, but it would prevent potential runtime errors if the viewModel structure changes.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/element/search.tsxfrontend/app/store/keymodel.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/store/keymodel.ts (3)
frontend/app/store/global.ts (2)
getBlockComponentModel(853-853)globalStore(865-865)frontend/app/view/term/term-model.ts (1)
TermViewModel(37-905)pkg/vdom/vdom_types.go (1)
WaveKeyboardEvent(233-247)
🔇 Additional comments (5)
frontend/app/store/keymodel.ts (3)
24-24: LGTM!Type import is correctly used for the type assertion in
getSelectedText.
632-654: LGTM!The function correctly prioritizes terminal selection over browser text selection with appropriate fallbacks. The type guard at line 635 ensures the type assertion at line 636 is safe.
678-696: LGTM!The
deactivateSearchfunction correctly closes the search dialog, and the Escape key handler properly prioritizes closing modals before deactivating search.frontend/app/element/search.tsx (2)
14-14: LGTM!The optional
searchInputRefprop is correctly typed and allows external components to control input focus.
31-31: LGTM!The ref handling properly supports both external control (via
providedInputRef) and internal management (vialocalInputRef). In React 19, passingrefas a regular prop is supported, so this will work correctly.Also applies to: 38-39, 153-153
3 participants
