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.
|
|
WalkthroughAn optional Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)frontend/app/store/keymodel.ts (3)
🔇 Additional comments (6)
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.
2 participants
