Web Search #1631
Web Search #1631
Conversation
WalkthroughThe pull request introduces comprehensive enhancements to the search functionality across multiple frontend files. The changes include updates to the The 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (4)
frontend/app/element/search.tsx (2)
39-43: Resetting state on isOpen change.
This ensures the search UI always starts fresh when it opens. However, if you want to preserve search text after closing, consider removing this reset.
53-63: Logging rects to console.
Consider removing or gating behind a debug flag for production builds to avoid cluttering logs.frontend/app/view/webview/webview.tsx (2)
546-552: Error handling in onSearch callback.
Consider providing user feedback or retry logic if searching fails. It currently just logs the error.try { model.webviewRef.current?.findInPage(search); } catch (e) { console.error("Failed to search", e); + // Potentially show a toast or user-visible error message here }
567-575: Strongly-type the event if possible.
You're currently typing the parameter asany. Consider a more precise type to ensure better readability and maintainability.- const onFoundInPage = useCallback((event: any) => { + interface FoundInPageEvent extends Event { + result?: { matches: number; activeMatchOrdinal: number }; + } + const onFoundInPage = useCallback((event: FoundInPageEvent) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/app/element/search.tsx(5 hunks)frontend/app/store/keymodel.ts(1 hunks)frontend/app/view/webview/webview.scss(1 hunks)frontend/app/view/webview/webview.tsx(6 hunks)frontend/types/custom.d.ts(2 hunks)frontend/util/keyutil.ts(1 hunks)
🔇 Additional comments (25)
frontend/app/element/search.tsx (9)
3-3: Use of Jotai imports looks good.
The import statement for atom and useAtom is appropriate for managing state.
9-15: Flexible callback props are well-introduced.
Declaring optional onSearch, onNext, and onPrev allows for more extensible usage without forcing the consumer to provide these callbacks. Good job keeping them optional.
26-28: Callbacks usage is consistent.
Using fallback behavior when callbacks are not provided maintains backward compatibility.
30-33: Atom state usage is clear.
The code properly destructures the required search states from Jotai. This is clean and readable.
45-50: onSearch triggered only for non-empty input.
This is a sensible approach to avoid sending extraneous updates, but ensure the caller also handles empty searches if needed.
83-91: Split behaviors for Previous/Next clicks is excellent.
The code uses onPrev or onNext if available; otherwise, it falls back to internal index control. This is a well-designed fallback structure.
106-106: Auto-focus on <Input> enhances usability.
This is a great user experience decision for immediate text entry.
128-128: Extended hook signature is cohesive.
Accepting viewModel and optionally assigning searchAtoms to it fosters better integration.
134-140: ViewModel synchronization looks correct.
Synchronizing searchAtoms with the provided viewModel ensures a single source of truth.
frontend/util/keyutil.ts (1)
81-83: Clean mapping of "Esc" to "Escape".
This is consistent with typical browser key naming. It improves clarity for event handling.
frontend/types/custom.d.ts (2)
231-236: Encapsulating search states into SearchAtoms.
This strongly groups relevant atoms, keeping the code organized and maintainable.
249-249: Optional searchAtoms in ViewModel
Provides a flexible mechanism for search integration. This maintains backward compatibility for other components not using search.
frontend/app/store/keymodel.ts (3)
324-333: activateSearch effectively opens the search UI.
Ensuring that we check if searchAtoms exist protects against null references. This approach gracefully degrades when search is not available.
334-342: deactivateSearch gracefully closes the search UI.
Similar to its counterpart, this function ensures toggling is done only if search is currently active.
343-345: Keyboard shortcuts are effectively mapped.
Binding "Cmd+f", "Ctrl+f", and "Escape" to search activation/deactivation is straightforward and intuitive for users.
frontend/app/view/webview/webview.tsx (9)
5-5: Imports look good.
It's great to see the Search and useSearch imports pulled in here, aligning with the PR objective of adding search functionality.
16-16: Verify Jotai dependencies.
Ensure that the appropriate version of Jotai (or any supporting library) is in the project's dependencies to avoid runtime issues.
540-545: Initialization of searchProps.
You’re cleanly retrieving the search atoms and handlers here. This structure is neat and sets up the search logic in a modular way.
553-559: Next-search callback looks good.
This effectively handles jump-to-next-result. Ensure that any wrapping logic (e.g., going from the last match back to the first) is handled according to UX requirements.
560-566: Previous-search callback looks good.
Similar to onSearchNext, confirm desired behavior when the user is at the first match.
576-584: Graceful search clearing.
Stopping the find operation when search is closed is correct. This prevents leftover highlights from persisting in the webview.
718-718: Correct event listener addition for "found-in-page".
This listener appropriately calls onFoundInPage to handle matched results.
734-734: Proper cleanup of the event listener.
Removing the "found-in-page" listener prevents memory leaks and ensures no stale references remain.
756-756: Search component usage is clear.
Providing the search callbacks and props here is straightforward and follows the newly introduced search pattern.
frontend/app/view/webview/webview.scss (1)
4-5: Unified styling for .webview and .webview-container.
Combining these ensures consistent appearance and layout. Verify if any container-specific properties are needed to differentiate the two in the future.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/app/view/webview/webview.tsx (1)
541-581: Implementation of search callbacksThis block introduces callbacks (
onSearch,onSearchNext,onSearchPrev, andonFoundInPage) to integrate searching within the webview. Overall, this is a solid approach, and the code appears correct. However, please consider the following points:
- Dependency arrays: The
useCallbackhooks foronSearchandonFoundInPagehave empty dependency arrays ([]), but they reference external values (model.webviewRef). Ensure thatmodelis stable or include it in the dependency array if needed.- Error handling: The try/catch blocks log errors via
console.error. That is acceptable for now, but consider whether you need more robust user-facing error handling.Other than that, the logic for updating search results and the currently selected match ordinal is well-structured.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/element/search.tsx(5 hunks)frontend/app/view/webview/webview.tsx(6 hunks)
🔇 Additional comments (18)
frontend/app/view/webview/webview.tsx (5)
5-5: Imported Search and useSearch
These imports are essential to integrate the new search functionality into the WebView. The usage appears correct.
16-16: Additional imports from jotai
The newly imported Jotai elements (Atom, PrimitiveAtom, etc.) facilitate reactive state management for search. This is well-aligned with the rest of the code's approach.
715-715: Adding found-in-page listener
Registering this listener is the key to integrating the search notifications. Good job hooking it into onFoundInPage.
731-731: Removing found-in-page listener on unmount
Properly cleaning up event listeners prevents potential memory leaks. This is a best practice.
753-753: Rendering the <Search> component
Embedding <Search> with onSearch, onNext, and onPrev props is consistent with the new integrated search logic. This approach is clean, ensures good separation of concerns, and clarifies the flow of data to the user.
frontend/app/element/search.tsx (13)
3-4: New imports
Importing the Jotai and React hooks ensures that the search component can manage and track state effectively. This is consistent with modern React usage.
9-15: Extending SearchProps with optional callbacks
The addition of onSearch, onNext, and onPrev enables customizable behavior. Keeping them optional is a good design choice, allowing flexible integration while preserving backward compatibility.
26-28: Destructuring callback props
Extracting onSearch, onNext, and onPrev early improves readability and prevents repeated prop lookups later in the code.
30-31: Atom usage for isOpen
You are using Jotai atoms to manage the open/close state of the search. This is straightforward and consistent with the rest of the file's approach.
39-43: Resetting state upon toggling search
Clearing out the search, index, and number of results whenever isOpen changes helps ensure fresh state each time the user opens the search panel.
45-49: Triggering search logic
Automatically calling onSearch whenever the user updates the search term is very intuitive. The fallback to clearing selection on empty input is well thought out.
52-60: Offset calculation logic in useCallback
Using the floating UI offset middleware to compute dynamic offsets is a clean approach. Logging the rects could help in debugging layout issues.
62-62: Pushing offset middleware
Appending the offset middleware to the array is correct. Ensures that the floating element is positioned relative to the anchor reference intelligently.
82-82: Implementing onPrev for search navigation
Using the optional callback onPrev?.() ?? setIndex(index - 1) makes the feature extensible while providing a fallback index decrement behavior.
90-90: Implementing onNext for search navigation
Similarly, onNext?.() ?? setIndex(index + 1) ensures consistent forward navigation logic with an easy fallback if no custom callback is provided.
105-105: Rendering the input box for search
Providing autoFocus on the input is a thoughtful touch that improves user experience, ensuring the user can start typing right away.
127-127: New optional parameter in useSearch
Adding viewModel?: ViewModel is a good approach, enabling broader integrations while keeping the function signature flexible.
133-139: Syncing atoms to viewModel
Assigning searchAtoms to viewModel.searchAtoms effectively centralizes search state. This is convenient if you need to retrieve or manipulate those atoms from elsewhere in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/app/view/webview/webview.tsx (4)
394-398: Consider removing or updating logging statements.While the logic to prevent focus when search is active is correct, the logging statements might be too verbose for production code. Consider either:
- Removing them
- Making them debug-level logs
- Adding more context about why the focus was prevented
- console.log("webview giveFocus"); + // console.debug("webview giveFocus"); if (this.searchAtoms && globalStore.get(this.searchAtoms.isOpenAtom)) { - console.log("search is open, not giving focus"); + // console.debug("webview: preventing focus due to active search"); return true; }
553-563: Enhance error handling in search operations.The current error handling only logs the error. Consider providing user feedback when search operations fail.
} catch (e) { - console.error("Failed to search", e); + console.error("Failed to search:", e); + // Notify user of search failure + if (model.webviewRef.current) { + model.webviewRef.current.stopFindInPage("clearSelection"); + } }
578-586: Improve type safety for found-in-page event handling.The event parameter is typed as
any. Consider creating a proper type definition for the event.+ interface FoundInPageEvent { + result: { + matches: number; + activeMatchOrdinal: number; + finalUpdate: boolean; + }; + } + - const onFoundInPage = useCallback((event: any) => { + const onFoundInPage = useCallback((event: FoundInPageEvent) => {
721-721: Consider clearing search on component unmount.To prevent potential memory leaks and ensure proper cleanup, consider clearing any active searches when the component unmounts.
return () => { + // Clear any active searches + if (model.webviewRef.current) { + model.webviewRef.current.stopFindInPage("clearSelection"); + } webview.removeEventListener("did-frame-navigate", navigateListener); // ... other removeEventListener calls ... webview.removeEventListener("found-in-page", onFoundInPage); };Also applies to: 737-737, 759-759
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/view/webview/webview.tsx(8 hunks)
🔇 Additional comments (1)
frontend/app/view/webview/webview.tsx (1)
5-5: LGTM: Search-related changes to imports and model.
The additions of Search-related imports and the optional searchAtoms property to WebViewModel are well-structured and align with the PR's objective of adding search functionality.
Also applies to: 16-17, 54-54
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/app/view/webview/webview.tsx (4)
54-54: Add null check for better defensive programming.While the search state management is correct, consider adding a null check before accessing
searchAtomsin thehandleNavigatemethod to make the code more robust.-if (this.searchAtoms) { +if (this.searchAtoms?.isOpenAtom) { globalStore.set(this.searchAtoms.isOpenAtom, false); }Also applies to: 301-303
397-401: Consider removing debug console.log statements.The search state check is correctly implemented, but consider removing or replacing the console.log statements with proper logging for production code.
-console.log("webview giveFocus"); if (this.searchAtoms && globalStore.get(this.searchAtoms.isOpenAtom)) { - console.log("search is open, not giving focus"); return true; }
550-565: Enhance error handling with specific error messages.The error handling could be more informative by including specific error details.
} catch (e) { - console.error("Failed to search", e); + console.error(`Failed to search with query "${search}":`, e); }
580-588: Consider adding validation for search results.The search index calculation assumes the
activeMatchOrdinalis always positive. Consider adding validation and handling edge cases.if (!result) { return; } +const activeMatch = Math.max(0, result.activeMatchOrdinal - 1); setNumSearchResults(result.matches); -setSearchIndex(result.activeMatchOrdinal - 1); +setSearchIndex(activeMatch); -console.log("found in page", result);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/view/webview/webview.tsx(9 hunks)
🔇 Additional comments (3)
frontend/app/view/webview/webview.tsx (3)
5-5: LGTM: Required imports for search functionality.
The imports for Search component, useSearch hook, and necessary types are correctly added.
Also applies to: 16-17
723-723: LGTM: Proper event listener management.
The found-in-page event listener is correctly added and cleaned up following React best practices.
Also applies to: 739-739
761-761: LGTM: Search component integration.
The Search component is correctly integrated with all necessary props for search functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/app/view/webview/webview.tsx (4)
54-54: Add null check for better defensive programming.While the logic is correct, consider adding a null check before accessing the searchAtoms property to make the code more robust.
-if (this.searchAtoms) { +if (this.searchAtoms?.isOpenAtom) { globalStore.set(this.searchAtoms.isOpenAtom, false); }Also applies to: 301-303
397-401: Remove debug console.log statements.Consider removing or replacing the console.log statements with proper logging for production code.
-console.log("webview giveFocus"); -if (this.searchAtoms && globalStore.get(this.searchAtoms.isOpenAtom)) { - console.log("search is open, not giving focus"); +if (this.searchAtoms && globalStore.get(this.searchAtoms.isOpenAtom)) { return true; }
555-565: Improve error handling in search function.The catch block could provide more specific error handling and user feedback.
} catch (e) { - console.error("Failed to search", e); + console.error("Failed to search:", e); + // Consider showing user feedback through a toast or error message + if (e instanceof Error) { + // Handle specific error types + } }
580-588: Type the event parameter properly.The
anytype for the event parameter should be replaced with a proper type definition.-const onFoundInPage = useCallback((event: any) => { +interface FindInPageResult { + result: { + matches: number; + activeMatchOrdinal: number; + }; +} + +const onFoundInPage = useCallback((event: FindInPageResult) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/element/search.tsx(5 hunks)frontend/app/view/webview/webview.tsx(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/element/search.tsx
🔇 Additional comments (3)
frontend/app/view/webview/webview.tsx (3)
5-5: LGTM: Required imports for search functionality.
The imports for Search component, useSearch hook, and necessary types are correctly added.
Also applies to: 16-17
723-723: LGTM: Proper event listener management.
The found-in-page event listener is correctly added and cleaned up following React best practices.
Also applies to: 739-739
761-761: LGTM: Search component integration.
The Search component is correctly integrated with all required props for search functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/app/view/webview/webview.tsx (3)
54-54: Add null check for better defensive programming.While the implementation is correct, consider adding a null check before accessing
searchAtoms.isOpenAtomfor better defensive programming.if (this.searchAtoms) { - globalStore.set(this.searchAtoms.isOpenAtom, false); + const { isOpenAtom } = this.searchAtoms; + if (isOpenAtom) { + globalStore.set(isOpenAtom, false); + } }Also applies to: 301-303
397-401: Remove debug console.log statements.Consider removing the debug console.log statements before merging to production.
-console.log("webview giveFocus"); if (this.searchAtoms && globalStore.get(this.searchAtoms.isOpenAtom)) { - console.log("search is open, not giving focus"); return true; }
550-592: Enhance search implementation with better error handling and constants.Consider the following improvements:
- Remove debug console.log statements
- Extract search options into constants
- Improve error handling with specific error messages
+const SEARCH_OPTIONS = { + NEXT: { findNext: false, forward: true }, + PREV: { findNext: false, forward: false }, +}; const onSearchNext = useCallback(() => { try { - console.log("search next", searchVal); - model.webviewRef.current?.findInPage(searchVal, { findNext: false, forward: true }); + model.webviewRef.current?.findInPage(searchVal, SEARCH_OPTIONS.NEXT); } catch (e) { - console.error("Failed to search next", e); + console.error(`Failed to search next: ${e.message}`); } }, [searchVal]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/element/search.tsx(5 hunks)frontend/app/view/webview/webview.tsx(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/element/search.tsx
🔇 Additional comments (3)
frontend/app/view/webview/webview.tsx (3)
5-5: LGTM: Required imports for search functionality.
The imports are correctly added and necessary for implementing the search feature.
Also applies to: 16-17
725-725: LGTM: Proper event listener management.
The found-in-page event listener is correctly added and cleaned up, following React best practices.
Also applies to: 741-741
763-763: LGTM: Clean Search component integration.
The Search component is properly integrated with all necessary props for search functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/app/element/search.tsx (1)
111-122: Consider adding keyboard shortcut hints to button titlesTo improve user experience, consider adding the keyboard shortcuts to the button titles.
const prevDecl: IconButtonDecl = { elemtype: "iconbutton", icon: "chevron-up", - title: "Previous Result", + title: "Previous Result (Shift+Enter)", click: onPrevWrapper, }; const nextDecl: IconButtonDecl = { elemtype: "iconbutton", icon: "chevron-down", - title: "Next Result", + title: "Next Result (Enter)", click: onNextWrapper, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/element/search.tsx(5 hunks)
🔇 Additional comments (4)
frontend/app/element/search.tsx (4)
3-4: LGTM: Type definitions and imports are well-structured
The new SearchProps type properly extends SearchAtoms and includes well-typed optional callbacks for search operations. The additional imports support the enhanced state management using Jotai atoms.
Also applies to: 9-15
39-49: LGTM: State management is properly implemented
The useEffect hooks appropriately handle state reset when search is closed and trigger the onSearch callback when search text changes.
165-177: LGTM: useSearch hook properly manages search state
The hook correctly initializes atoms and properly updates the viewModel when provided. The null coalescing for anchorRef provides a good fallback.
97-109: Verify keyboard shortcut implementation
The PR objectives mention adding Cmd+f, Ctrl+f, and Alt+f shortcuts, but these aren't handled in this component. Please verify if these shortcuts are implemented elsewhere in the codebase.
Adds support for Cmd:f, Ctrl:f, and Alt:f to activate search in the Web and Help widgets