Terminal search #1654
Terminal search #1654
Conversation
|
Warning Rate limit exceeded@esimkowitz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces search functionality across multiple files in the frontend application. The changes primarily focus on integrating a search feature into terminal and webview components by adding the In the terminal view ( The In the webview component ( Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (7)
frontend/app/view/term/term.tsx (4)
76-76: Optional property can create branching
searchAtoms?: SearchAtoms;might require handling undefined states in calling code. Ensure that all references tosearchAtomsare safely guarded.
795-819: Search callbacks are well-structured
The incremental search option and callback definitions look good. However, consider a short-circuit if the terminal,searchAddon, orsearchTextis not ready.
851-851: Allowing proposed API
EnablingallowProposedApimay introduce future deprecations as xterm evolves. Keep track of breaking changes in xterm’s release notes.
864-868: Event handler for search results
The dynamic update of result indices is a solid approach. Consider adding a cleanup mechanism (e.g., unsubscribing from the event) if future refactoring moves initialization outsideinitTerminal.frontend/app/view/term/termwrap.ts (1)
157-159: Handling search result changes
Binding the callback conditionally prevents errors if the consumer omitsonSearchResultsDidChange. Consider removing the listener ondispose()to prevent potential memory leaks.frontend/app/view/webview/webview.tsx (2)
566-566: “Next” search logic
Meets typical forward-search needs. If you want cyclical navigation (after last match goes to first), consider handling that.
574-574: “Previous” search logic
Same note as “Next.” Including cyclical search might improve user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
frontend/app/view/term/term.tsx(9 hunks)frontend/app/view/term/termwrap.ts(4 hunks)frontend/app/view/webview/webview.tsx(3 hunks)package.json(1 hunks)
🔇 Additional comments (10)
frontend/app/view/term/term.tsx (4)
6-6: Import usage is correct
The Search and useSearch imports appear aligned with the new search functionality.
28-28: Validate usage of new imports
boundNumber and fireAndForget are imported. Verify that they are indeed required and that no other utility function covers similar functionality.
367-370: Conditional focus behavior
This code prevents focusing the terminal if the search UI is open, which seems logical. Confirm that returning true here (i.e., “focus handled”) doesn’t cause unintended UI states in other areas.
923-923: Integrated Search UI
Embedding <Search {...searchProps} /> at this level is consistent with the terminal layout. Good approach for a cohesive user experience.
frontend/app/view/term/termwrap.ts (3)
12-12: Added SearchAddon import
This import aligns with the new search functionality. Double-check that the appropriate version is installed and consistent with @xterm/xterm.
54-54: Introducing searchable terminal properties
The searchAddon and onSearchResultsDidChange properties facilitate dynamic search integration. Ensure onSearchResultsDidChange is only invoked when listeners are defined.
Also applies to: 61-61
78-79: Loading SearchAddon
Loading the searchAddon before other addons is fine. Just confirm that order of loading does not conflict with the fitAddon or serializeAddon for any edge cases.
frontend/app/view/webview/webview.tsx (2)
555-555: Search callback
Good usage of try/catch. Consider logging corner cases (e.g., undefined webviewRef or empty string handling) for improved debugging.
763-763: Search component usage
Placing <Search {...searchProps} /> directly within the webview layout is consistent with the approach in term.tsx.
package.json (1)
96-96: New addon dependency
"@xterm/addon-search": "^0.15.0" looks correct given the xterm version. Confirm no incompatibility with core xterm or third-party addons.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/app/element/search.tsx (1)
134-134: Consider removing or gating debug logs.
The usage ofconsole.log("offsetCalc", yOffsetCalc, xOffsetCalc);on lines above may be useful for development, but it can clutter the console in production. Consider removing or conditionally executing them (e.g., behind an environment check).- console.log("offsetCalc", yOffsetCalc, xOffsetCalc); + if (process.env.NODE_ENV !== "production") { + console.log("offsetCalc", yOffsetCalc, xOffsetCalc); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/element/search.tsx(2 hunks)frontend/app/view/term/term.scss(2 hunks)
🔇 Additional comments (3)
frontend/app/view/term/term.scss (2)
129-136: LGTM! Clear documentation of width adjustment.
The comment clearly explains the 18px width calculation, and the z-index positioning ensures proper layering with the xterm viewport overlay.
Line range hint 139-146: LGTM! Consistent width adjustment.
The width calculation has been properly synchronized with the show observer's width to maintain consistent layout behavior.
frontend/app/element/search.tsx (1)
1-1: All good on the new imports.
Your imports from @floating-ui/react, clsx, and other libraries look correct. They appear to be used appropriately in the component, and this aligns well with your approach to floating UI elements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/app/view/term/termwrap.ts (2)
177-181: Consider logging disposal errors.While the try-catch block prevents errors from breaking the disposal chain, consider logging the errors for debugging purposes.
this.toDispose.forEach((d) => { try { d.dispose(); - } catch (_) {} + } catch (error) { + console.debug('Failed to dispose addon:', error); + } });
Line range hint
367-370: Use debug logger for consistency.For consistency with the rest of the codebase, consider using the debug logger instead of console.log.
if (this.searchAtoms && globalStore.get(this.searchAtoms.isOpenAtom)) { - console.log("search is open, not giving focus"); + dlog("search is open, not giving focus"); return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/app/element/search.scss(1 hunks)frontend/app/element/search.tsx(6 hunks)frontend/app/view/term/term.tsx(9 hunks)frontend/app/view/term/termwrap.ts(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/app/element/search.scss
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/element/search.tsx
🔇 Additional comments (5)
frontend/app/view/term/termwrap.ts (2)
12-12: LGTM: SearchAddon integration is properly implemented.
The SearchAddon is correctly imported, initialized as a class property, and loaded into the terminal instance.
Also applies to: 54-54, 79-80
61-61: LGTM: Search results callback is well-defined.
The callback property is properly typed and matches the xterm.js SearchAddon API specification.
frontend/app/view/term/term.tsx (3)
795-863: LGTM: Search implementation is comprehensive and follows best practices.
The search functionality is well-implemented with:
- Proper state management using atoms
- Memoized callbacks for performance
- Comprehensive search options (regex, whole word, case-sensitive)
- Well-configured search decorations
895-895: LGTM: Terminal configuration properly updated for search support.
The allowProposedApi option is correctly added with a clear explanatory comment.
908-911: LGTM: Search results callback is properly implemented.
The callback correctly updates the search UI state with result index and count information.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
frontend/app/element/search.stories.tsx (2)
19-42: Ensure user-friendliness with the default open state
Having the search open by default might obscure the user experience or prevent them from seeing the initial closed state of the search box. Consider whether to keep it closed at first or to provide a separate story illustrating the closed-to-open transition.
128-130: Repeat usage pattern
This block is nearly identical to the previous story's pattern. If necessary, consider abstracting duplicated logic out into a helper function for maintainability.frontend/app/element/iconbutton.scss (1)
36-47: LGTM! Consider enhancing hover state feedback.The toggle button styles are well-structured and provide good visual feedback. However, consider combining the hover state with the active state for better visual distinction.
&.toggle { border-radius: 3px; padding: 1px; &.active { opacity: 1; border: 1px solid var(--accent-color); padding: 0; + &:hover { + background: var(--highlight-bg-color); + opacity: 0.9; + } } &:hover { background: var(--highlight-bg-color); } }frontend/app/element/iconbutton.tsx (1)
38-39: Consider memoizing activeAtom initialization.The activeAtom initialization could be optimized to prevent unnecessary recreations.
-const activeAtom = useMemo(() => decl.active ?? atom(false), [decl.active]); +const activeAtom = useMemo(() => { + if (decl.active) return decl.active; + return atom(false); +}, [decl.active]);frontend/app/element/search.tsx (1)
Line range hint
164-184: Consider adding error handling for search operations.The search functionality should handle potential errors during search operations.
<Input placeholder="Search" value={search} - onChange={setSearch} + onChange={(value) => { + try { + if (regexAtom && value) { + // Validate regex syntax + new RegExp(value); + } + setSearch(value); + } catch (e) { + // Handle invalid regex + console.error('Invalid regex pattern:', e); + } + }} onKeyDown={onKeyDown} autoFocus />frontend/app/view/term/term.tsx (3)
76-76: Consider improving type safety and logging.The changes look good, but could benefit from these improvements:
- Add type annotation for
searchAtomsproperty- Make the console log message more descriptive
- searchAtoms?: SearchAtoms; + searchAtoms?: SearchAtoms | undefined; - console.log("search is open, not giving focus"); + console.log("[TermViewModel] Search dialog is open, skipping terminal focus");Also applies to: 367-370
795-845: Enhance search implementation maintainability.The search implementation is solid, but consider these improvements:
- Extract decoration colors as constants
- Memoize searchProps callbacks using useCallback
- Add JSDoc comments for search options
+const SEARCH_DECORATION_COLORS = { + MATCH_OVERVIEW: '#e0e0e0', + ACTIVE_MATCH_BORDER: '#58c142', + MATCH_BORDER: '#e0e0e0', +} as const; +/** + * Search options for the terminal + * @property {boolean} incremental - Enable incremental search + * @property {boolean} regex - Enable regex search + * @property {boolean} wholeWord - Match whole words only + * @property {boolean} caseSensitive - Case sensitive search + * @property {object} decorations - Search result highlighting options + */ const searchOpts: ISearchOptions = React.useMemo( () => ({ incremental: true, regex, wholeWord, caseSensitive, decorations: { - matchOverviewRuler: "#e0e0e0", - activeMatchColorOverviewRuler: "#e0e0e0", - activeMatchBorder: "#58c142", - matchBorder: "#e0e0e0", + matchOverviewRuler: SEARCH_DECORATION_COLORS.MATCH_OVERVIEW, + activeMatchColorOverviewRuler: SEARCH_DECORATION_COLORS.MATCH_OVERVIEW, + activeMatchBorder: SEARCH_DECORATION_COLORS.ACTIVE_MATCH_BORDER, + matchBorder: SEARCH_DECORATION_COLORS.MATCH_BORDER, }, }), [regex, wholeWord, caseSensitive] );
877-877: Improve type safety and documentation.Consider these enhancements:
- Add more descriptive comment for allowProposedApi
- Add type safety for search results handler
- allowProposedApi: true, // needed for search + allowProposedApi: true, // Required by @xterm/addon-search for advanced search features + interface SearchResults { + resultIndex: number; + resultCount: number; + } + - termWrap.onSearchResultsDidChange = (results) => { + termWrap.onSearchResultsDidChange = (results: SearchResults) => {Also applies to: 890-894
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/app/block/blockframe.tsx(2 hunks)frontend/app/element/iconbutton.scss(1 hunks)frontend/app/element/iconbutton.tsx(2 hunks)frontend/app/element/search.scss(2 hunks)frontend/app/element/search.stories.tsx(4 hunks)frontend/app/element/search.tsx(6 hunks)frontend/app/store/keymodel.ts(1 hunks)frontend/app/view/term/term.tsx(9 hunks)frontend/app/view/webview/webview.tsx(5 hunks)frontend/types/custom.d.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/app/element/search.scss
- frontend/app/view/webview/webview.tsx
🔇 Additional comments (16)
frontend/app/element/search.stories.tsx (3)
44-46: Nice addition of advanced search capabilities
Using { regex: true, caseSensitive: true, wholeWord: true } broadens search functionality. Good approach to future-proof the search options.
72-73: Verify consistency of naming
Renaming numResultsAtom to resultsCount in the story is consistent with the new search property naming. Looks good.
99-101: Expanded property usage
This reinforces the refactor from searchAtom to searchValue. Keep an eye on naming consistency across the rest of the codebase.
frontend/app/store/keymodel.ts (2)
329-329: Event-driven approach to opening search
Activating the search via globalStore.set(...) is straightforward. Be sure all relevant keyboard or button events funnel through here to avoid confusion from multiple triggers.
337-338: Guard clause complements
Great that the condition checks both the existence of searchAtoms and the isOpen state. This helps prevent errors if for some reason searchAtoms isn't populated.
frontend/types/custom.d.ts (5)
146-146: New 'ToggleIconButtonDecl' in header elements
Adding the toggle icon button type to the union broadens the header’s flexibility. Good extension.
154-157: IconButtonCommon enhancements
The extended fields (iconColor, iconSpin, etc.) provide clearer customization for icons. Make sure to handle any potential styling conflicts gracefully.
164-168: Refined IconButtonDecl definition
The redefinition using IconButtonCommon is a clean approach to avoid duplication. This is consistent with good DRY practices.
170-174: ToggleIconButtonDecl
Introducing an active atom is a solid, reactive design pattern for toggling states in the UI. Nicely done.
241-247: Renamed search atoms
Renaming the properties to searchValue, resultsIndex, resultsCount, isOpen, etc., is more descriptive. The optional booleans for advanced search (regex, caseSensitive, wholeWord) align neatly with the rest of the code changes.
frontend/app/block/blockframe.tsx (2)
33-33: Newly imported ToggleIconButton
Importing the toggle variant is aligned with the expanded search UI patterns.
281-282: Conditional rendering of toggle buttons
Using toggleiconbutton in the HeaderTextElem function helps unify button rendering logic. Verify that any toggled state is stored consistently and remains modular for future expansions.
frontend/app/element/iconbutton.tsx (1)
25-25: LGTM! Good accessibility improvement.
Adding aria-label enhances accessibility for screen readers.
frontend/app/element/search.tsx (2)
Line range hint 80-91: LGTM! Well-implemented floating UI setup.
The floating UI configuration is well-structured and handles positioning correctly.
143-150:
Fix incorrect condition in wholeWordDecl.
The wholeWordDecl is using caseSensitiveAtom instead of wholeWordAtom in its condition.
-const wholeWordDecl: ToggleIconButtonDecl = caseSensitiveAtom
+const wholeWordDecl: ToggleIconButtonDecl = wholeWordAtom
? {
elemtype: "toggleiconbutton",
icon: "w",
title: "Whole Word",
active: wholeWordAtom,
}
: null;Likely invalid or redundant comment.
frontend/app/view/term/term.tsx (1)
949-949: LGTM!
The Search component is correctly positioned within the terminal view.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/app/element/search.tsx (1)
210-230:⚠️ Potential issueAdd cleanup and error handling in useSearch hook.
The cleanup issue identified in the previous review is still relevant.
Additionally, the useMemo dependency array should include the relevant options to ensure proper reactivity.
Apply these changes:
export function useSearch(options?: SearchOptions): SearchProps { const searchAtoms: SearchAtoms = useMemo( () => ({ searchValue: atom(""), resultsIndex: atom(0), resultsCount: atom(0), isOpen: atom(false), regex: options?.regex !== undefined ? atom(options.regex) : undefined, caseSensitive: options?.caseSensitive !== undefined ? atom(options.caseSensitive) : undefined, wholeWord: options?.wholeWord !== undefined ? atom(options.wholeWord) : undefined, }), - [] + [options?.regex, options?.caseSensitive, options?.wholeWord] ); const anchorRef = options?.anchorRef ?? useRef(null); useEffect(() => { if (options?.viewModel) { options.viewModel.searchAtoms = searchAtoms; + return () => { + options.viewModel.searchAtoms = undefined; + }; } }, [options?.viewModel]); return { ...searchAtoms, anchorRef }; }
🧹 Nitpick comments (1)
frontend/app/element/search.tsx (1)
137-160: Consider simplifying toggle button declarations.The toggle button declarations follow a repetitive pattern that could be simplified for better maintainability.
Consider refactoring to use a map or array of configurations:
- const regexDecl: ToggleIconButtonDecl = regexAtom - ? { - elemtype: "toggleiconbutton", - icon: "asterisk", - title: "Regex Search", - active: regexAtom, - } - : null; - const wholeWordDecl: ToggleIconButtonDecl = caseSensitiveAtom - ? { - elemtype: "toggleiconbutton", - icon: "w", - title: "Whole Word", - active: wholeWordAtom, - } - : null; - const caseSensitiveDecl: ToggleIconButtonDecl = caseSensitiveAtom - ? { - elemtype: "toggleiconbutton", - icon: "font-case", - title: "Case Sensitive", - active: caseSensitiveAtom, - } - : null; + const toggleButtons = [ + { + atom: regexAtom, + icon: "asterisk", + title: "Regex Search", + }, + { + atom: wholeWordAtom, + icon: "w", + title: "Whole Word", + }, + { + atom: caseSensitiveAtom, + icon: "font-case", + title: "Case Sensitive", + }, + ].map(({ atom, icon, title }) => + atom + ? { + elemtype: "toggleiconbutton" as const, + icon, + title, + active: atom, + } + : null + );Then update the render section:
- <div className="right-buttons additional"> - {caseSensitiveDecl && <ToggleIconButton decl={caseSensitiveDecl} />} - {wholeWordDecl && <ToggleIconButton decl={wholeWordDecl} />} - {regexDecl && <ToggleIconButton decl={regexDecl} />} - </div> + <div className="right-buttons additional"> + {toggleButtons.map((decl, index) => + decl && <ToggleIconButton key={index} decl={decl} /> + )} + </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/element/search.tsx(7 hunks)
🔇 Additional comments (1)
frontend/app/element/search.tsx (1)
22-28: LGTM! Props naming improvements and new search options.
The renaming of props (e.g., searchAtom → searchValue) improves clarity, and the new optional props align well with the PR's objective of adding advanced search capabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/app/element/search.tsx (1)
212-230:⚠️ Potential issueAdd cleanup and error handling in useSearch hook
The cleanup issue from the previous review is still valid.
export function useSearch(options?: SearchOptions): SearchProps { const searchAtoms: SearchAtoms = useMemo( () => ({ searchValue: atom(""), resultsIndex: atom(0), resultsCount: atom(0), isOpen: atom(false), regex: options?.regex !== undefined ? atom(options.regex) : undefined, caseSensitive: options?.caseSensitive !== undefined ? atom(options.caseSensitive) : undefined, wholeWord: options?.wholeWord !== undefined ? atom(options.wholeWord) : undefined, }), [] ); const anchorRef = options?.anchorRef ?? useRef(null); useEffect(() => { if (options?.viewModel) { options.viewModel.searchAtoms = searchAtoms; + return () => { + options.viewModel.searchAtoms = undefined; + }; } }, [options?.viewModel]); return { ...searchAtoms, anchorRef }; }
🧹 Nitpick comments (2)
frontend/app/element/search.tsx (2)
46-50: Consider using object destructuring for cleaner reset logicThe reset logic can be more concise using object destructuring.
- if (!isOpen) { - setSearch(""); - setIndex(0); - setNumResults(0); - } + if (!isOpen) { + const resetState = { search: "", index: 0, numResults: 0 }; + Object.entries(resetState).forEach(([key, value]) => { + const setter = { setSearch, setIndex, setNumResults }[`set${key.charAt(0).toUpperCase()}${key.slice(1)}`]; + setter(value); + }); + }
139-162: Consider extracting toggle button declarations into a helperThe toggle button declarations follow a similar pattern and could be extracted into a helper function to reduce repetition.
+ const createToggleButton = ( + atom: typeof regexAtom | typeof caseSensitiveAtom | typeof wholeWordAtom, + icon: string, + title: string + ): ToggleIconButtonDecl => + atom + ? { + elemtype: "toggleiconbutton", + icon, + title, + active: atom, + } + : null; + - const regexDecl: ToggleIconButtonDecl = regexAtom - ? { - elemtype: "toggleiconbutton", - icon: "asterisk", - title: "Regex Search", - active: regexAtom, - } - : null; + const regexDecl = createToggleButton(regexAtom, "asterisk", "Regex Search"); - const wholeWordDecl: ToggleIconButtonDecl = caseSensitiveAtom - ? { - elemtype: "toggleiconbutton", - icon: "w", - title: "Whole Word", - active: wholeWordAtom, - } - : null; + const wholeWordDecl = createToggleButton(wholeWordAtom, "w", "Whole Word"); - const caseSensitiveDecl: ToggleIconButtonDecl = caseSensitiveAtom - ? { - elemtype: "toggleiconbutton", - icon: "font-case", - title: "Case Sensitive", - active: caseSensitiveAtom, - } - : null; + const caseSensitiveDecl = createToggleButton(caseSensitiveAtom, "font-case", "Case Sensitive");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/element/search.stories.tsx(4 hunks)frontend/app/element/search.tsx(8 hunks)frontend/app/view/webview/webview.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/app/element/search.stories.tsx
- frontend/app/view/webview/webview.tsx
🔇 Additional comments (1)
frontend/app/element/search.tsx (1)
204-210: LGTM: Well-structured type definitions
The SearchOptions type is well-defined with optional properties that align with the new search functionality requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/app/element/iconbutton.tsx (1)
Line range hint
36-63: Consider performance optimizations for ToggleIconButtonThe implementation looks good and follows React best practices. Consider these performance optimizations:
- Memoize the onClick handler using useCallback
- Memoize the title string computation
Apply these optimizations:
export const ToggleIconButton = memo( forwardRef<HTMLButtonElement, ToggleIconButtonProps>(({ decl, className }, ref) => { const activeAtom = useMemo(() => decl.active ?? atom(false), [decl.active]); const [active, setActive] = useAtom(activeAtom); ref = ref ?? useRef<HTMLButtonElement>(null); const spin = decl.iconSpin ?? false; - const title = `${decl.title}${active ? " (Active)" : ""}`; + const title = useMemo(() => `${decl.title}${active ? " (Active)" : ""}`, [decl.title, active]); const disabled = decl.disabled ?? false; + const handleClick = useCallback(() => setActive(!active), [active, setActive]); return ( <button ref={ref} className={clsx("wave-iconbutton", "toggle", className, decl.className, { active, disabled, "no-action": decl.noAction, })} title={title} aria-label={title} style={{ color: decl.iconColor ?? "inherit" }} - onClick={() => setActive(!active)} + onClick={handleClick} disabled={disabled} >Don't forget to add the useCallback import:
-import { forwardRef, memo, useMemo, useRef } from "react"; +import { forwardRef, memo, useMemo, useRef, useCallback } from "react";frontend/app/element/search.tsx (3)
46-50: Consider using object state reset for cleaner cleanup.The current implementation resets states individually. Consider using a single object reset for better maintainability.
- if (!isOpen) { - setSearch(""); - setIndex(0); - setNumResults(0); - } + if (!isOpen) { + const initialState = { search: "", index: 0, numResults: 0 }; + setSearch(initialState.search); + setIndex(initialState.index); + setNumResults(initialState.numResults); + }
163-167: Enhance accessibility for search options.The toggle buttons should have ARIA labels to improve screen reader support.
<div className="right-buttons additional"> - {caseSensitiveDecl && <ToggleIconButton decl={caseSensitiveDecl} />} - {wholeWordDecl && <ToggleIconButton decl={wholeWordDecl} />} - {regexDecl && <ToggleIconButton decl={regexDecl} />} + {caseSensitiveDecl && <ToggleIconButton decl={caseSensitiveDecl} aria-label="Toggle case sensitive search" />} + {wholeWordDecl && <ToggleIconButton decl={wholeWordDecl} aria-label="Toggle whole word search" />} + {regexDecl && <ToggleIconButton decl={regexDecl} aria-label="Toggle regex search" />} </div>
216-228: Enhance type safety for toggle button declarations.Consider using a more specific return type and adding type guards.
const createToggleButtonDecl = ( atom: WritableAtom<boolean, [boolean], void> | undefined, icon: string, title: string -): ToggleIconButtonDecl => +): ToggleIconButtonDecl | null => atom ? { elemtype: "toggleiconbutton", icon, title, active: atom, } : null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/element/iconbutton.tsx(2 hunks)frontend/app/element/search.tsx(8 hunks)
🔇 Additional comments (4)
frontend/app/element/iconbutton.tsx (2)
7-8: LGTM: Required imports added correctly
The new imports for jotai state management and React hooks are appropriate for the implementation.
17-17: Improved accessibility and disabled state handling
The changes address previous review feedback by:
- Properly handling the disabled state with nullish coalescing
- Adding aria-label for better accessibility
Also applies to: 22-22, 26-26, 28-34
frontend/app/element/search.tsx (2)
22-28: LGTM! Props naming improvements and new search options.
The renamed props provide better clarity, and the new search option props align well with the PR objectives.
206-210: Cleanup implementation in useSearch hook.
The cleanup implementation in useEffect addresses the previous review comment about proper cleanup handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
frontend/app/element/search.tsx (3)
Line range hint
22-50: Add error handling for search operations.While the search implementation is solid, consider adding try-catch blocks around the search operations to gracefully handle potential regex syntax errors.
const handleOpenChange = useCallback((open: boolean) => { + try { setIsOpen(open); + } catch (e) { + console.warn("Error in search operation:", e); + // Optionally reset search state or show user feedback + } }, []);
Line range hint
795-857: Extract search decoration colors as constants.Consider extracting the search decoration colors into named constants for better maintainability and reusability.
+const SEARCH_DECORATION_COLORS = { + MATCH_OVERVIEW: '#e0e0e0', + ACTIVE_MATCH_BORDER: '#58c142', + MATCH_BORDER: '#e0e0e0', +} as const; const searchOpts: ISearchOptions = React.useMemo( () => ({ incremental: true, regex, wholeWord, caseSensitive, decorations: { - matchOverviewRuler: "#e0e0e0", - activeMatchColorOverviewRuler: "#e0e0e0", - activeMatchBorder: "#58c142", - matchBorder: "#e0e0e0", + matchOverviewRuler: SEARCH_DECORATION_COLORS.MATCH_OVERVIEW, + activeMatchColorOverviewRuler: SEARCH_DECORATION_COLORS.MATCH_OVERVIEW, + activeMatchBorder: SEARCH_DECORATION_COLORS.ACTIVE_MATCH_BORDER, + matchBorder: SEARCH_DECORATION_COLORS.MATCH_BORDER, }, }), [regex, wholeWord, caseSensitive] );
Line range hint
889-906: Consider adding error boundary for terminal initialization.The terminal initialization with search addon could benefit from error boundary protection to gracefully handle initialization failures.
Consider implementing a custom error boundary:
class TerminalErrorBoundary extends React.Component { componentDidCatch(error: Error) { console.error('Terminal initialization failed:', error); // Implement fallback UI or recovery logic } render() { return this.props.children; } }And wrap the terminal initialization:
-fireAndForget(termWrap.initTerminal.bind(termWrap)); +<TerminalErrorBoundary> + {fireAndForget(termWrap.initTerminal.bind(termWrap))} +</TerminalErrorBoundary>frontend/app/view/term/term.tsx (2)
367-370: Remove or replace console.log with proper logging.Consider using a proper logging utility instead of console.log for better debugging capabilities.
if (this.searchAtoms && globalStore.get(this.searchAtoms.isOpen)) { - console.log("search is open, not giving focus"); + dlog("search is open, not giving focus"); return true; }
889-889: Document the purpose of allowProposedApi flag.Add a comment explaining why the allowProposedApi flag is required for the search functionality.
- allowProposedApi: true, // needed for search + // The allowProposedApi flag is required by xterm.js to enable the search addon + // See: https://github.com/xtermjs/xterm.js/blob/master/addons/search/README.md + allowProposedApi: true,frontend/app/element/iconbutton.tsx (2)
40-41: Consider adding a controlled mode optionThe component currently uses internal state management with jotai. Consider adding support for a controlled mode where the parent component can manage the state directly, making the component more flexible.
type ToggleIconButtonProps = { decl: ToggleIconButtonDecl; className?: string }; +interface ControlledToggleIconButtonProps extends Omit<ToggleIconButtonProps, 'decl'> { + decl: Omit<ToggleIconButtonDecl, 'active'>; + isActive: boolean; + onToggle: (active: boolean) => void; +}
44-44: Consider i18n support for the active state textThe "(Active)" suffix should be internationalized for better localization support.
- const title = `${decl.title}${active ? " (Active)" : ""}`; + const title = `${decl.title}${active ? t(" (Active)") : ""}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/app/element/iconbutton.tsx(2 hunks)frontend/app/element/search.scss(2 hunks)frontend/app/element/search.tsx(8 hunks)frontend/app/view/term/term.tsx(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/element/search.scss
🔇 Additional comments (6)
frontend/app/element/search.tsx (1)
Line range hint 1-21: LGTM! Well-structured search interface.
The search implementation has a clean architecture with proper type definitions and clear separation of concerns.
frontend/app/view/term/term.tsx (1)
795-857: LGTM! Robust search implementation with proper error handling.
The search implementation includes:
- Proper error handling for search operations
- Efficient memoization of search options
- Proper cleanup of search decorations
frontend/app/element/iconbutton.tsx (4)
7-8: LGTM: Import statements are appropriate
The added imports for jotai state management and React hooks are necessary for implementing the toggle functionality.
17-17: LGTM: Improved accessibility and disabled state handling
The changes improve the component by:
- Adding proper aria-label for accessibility
- Implementing consistent disabled state handling
- Using TypeScript's nullish coalescing for safer defaults
Also applies to: 22-22, 26-26, 28-28
49-53: LGTM: Proper class composition
The class composition using clsx is well-structured and follows the same pattern as IconButton for consistency.
57-57: Verify event handler behavior across browsers
The onClick handler is correctly used instead of onPointerDown as suggested in past reviews. However, let's verify its behavior across different browsers and input methods.
✅ Verification successful
Let me gather more information about the button implementation to ensure consistent behavior.
Let me try a different approach to gather information about the button implementation.
onClick handler implementation is appropriate and consistent
After reviewing the complete implementation of the ToggleIconButton component and other button implementations in the codebase, the onClick handler is appropriately used for toggling the active state. The implementation:
- Uses React's standard event handling pattern
- Is consistent with other toggle buttons in the codebase
- Properly manages the active state through Jotai atoms
- Includes proper ARIA attributes that update with the active state
- Has appropriate visual feedback through CSS classes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar button implementations to ensure consistency
rg -A 2 -B 2 "onClick.*setActive|setActive.*onClick"
Length of output: 433
Script:
#!/bin/bash
# Search for the IconButton component implementation to understand its full context
ast-grep --pattern 'function IconButton($_) {
$$$
}'
# Also check for any other button implementations that might set active state
rg -A 5 -B 5 "button.*active|active.*button" --type tsx
Length of output: 142
Script:
#!/bin/bash
# Search for the IconButton component implementation
cat frontend/app/element/iconbutton.tsx
# Look for other button implementations with active state, without file type restriction
rg -A 5 -B 5 "button.*active|active.*button"
Length of output: 7038
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/app/element/search.tsx (4)
46-50: Consider adding state validation in cleanupWhile the state reset on search close is good, consider adding validation to ensure the state is properly cleaned up even if the component unmounts during an active search.
if (!isOpen) { setSearch(""); setIndex(0); setNumResults(0); + // Ensure any pending search operations are cancelled + return () => { + setSearch(""); + setIndex(0); + setNumResults(0); + }; }
Line range hint
82-91: Document floating UI configurationThe floating UI setup is correct but would benefit from documentation explaining the placement strategy and offset calculations.
+// Configure floating UI with top-end placement and dynamic offset adjustments +// to ensure the search box remains visible within viewport bounds const { refs, floatingStyles } = useFloating({ placement: "top-end", open: isOpen, onOpenChange: handleOpenChange, whileElementsMounted: autoUpdate, middleware, elements: { reference: anchorRef!.current, }, });
163-169: Add ARIA labels for search option togglesThe search option toggles should include ARIA labels for better accessibility.
{(caseSensitiveDecl || wholeWordDecl || regexDecl) && ( - <div className="additional-buttons"> + <div className="additional-buttons" role="group" aria-label="Search Options"> {caseSensitiveDecl && <ToggleIconButton decl={caseSensitiveDecl} />} {wholeWordDecl && <ToggleIconButton decl={wholeWordDecl} />} {regexDecl && <ToggleIconButton decl={regexDecl} />} </div> )}
208-214: Add error boundary for viewModel operationsThe viewModel cleanup is good, but consider adding error handling for edge cases.
if (options?.viewModel) { + try { options.viewModel.searchAtoms = searchAtoms; return () => { + try { options.viewModel.searchAtoms = undefined; + } catch (error) { + console.error('Failed to cleanup search atoms:', error); + } }; + } catch (error) { + console.error('Failed to set search atoms:', error); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/fontawesome/webfonts/custom-icons.woff2is excluded by!**/*.woff2
📒 Files selected for processing (3)
frontend/app/element/search.scss(2 hunks)frontend/app/element/search.tsx(8 hunks)public/fontawesome/css/custom-icons.min.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/element/search.scss
🔇 Additional comments (3)
public/fontawesome/css/custom-icons.min.css (1)
1-1: LGTM: New Font Awesome icons added correctly
The new icons for search functionality (case-sensitive, regex, whole-word) are properly integrated while maintaining existing icons and font settings.
frontend/app/element/search.tsx (2)
22-28: LGTM: Clean prop destructuring with atomic state management
The component properly destructures the search-related atoms, maintaining a clean separation of concerns.
193-216: Verify xterm search addon integration
Let's verify the integration with @xterm/addon-search across the codebase.
✅ Verification successful
Search functionality is properly integrated with error handling
The verification shows that the xterm search addon is correctly integrated:
- The search addon is properly imported in
frontend/app/view/term/term.tsxandfrontend/app/view/term/termwrap.ts - Search addon is correctly initialized with
new SearchAddon()intermwrap.ts - All search operations (
findNextandfindPrevious) include proper error handling with try-catch blocks - The search functionality is well-integrated with the
useSearchhook being reviewed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify xterm search addon integration
# Check for proper import and usage of search addon
rg -l "@xterm/addon-search"
# Check for search addon initialization
ast-grep --pattern 'new SearchAddon()'
# Look for potential missing error handling in search operations
rg -A 5 'searchAddon\.(findNext|findPrevious)'
Length of output: 1518
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/app/view/term/term.tsx (4)
76-76: Consider adding TypeScript type for searchAtomsThe
searchAtomsproperty would benefit from a proper TypeScript type definition to improve code maintainability and type safety.- searchAtoms?: SearchAtoms; + searchAtoms?: { + isOpen: jotai.PrimitiveAtom<boolean>; + searchValue: jotai.PrimitiveAtom<string>; + caseSensitive: jotai.PrimitiveAtom<boolean>; + wholeWord: jotai.PrimitiveAtom<boolean>; + regex: jotai.PrimitiveAtom<boolean>; + resultsIndex: jotai.PrimitiveAtom<number>; + resultsCount: jotai.PrimitiveAtom<number>; + };
808-822: Consider memoizing searchOpts separatelyThe search options object could be split into two memoized objects: one for the core options and another for decorations. This would prevent unnecessary re-renders when only the core options change.
+ const coreSearchOpts = React.useMemo<Pick<ISearchOptions, 'regex' | 'wholeWord' | 'caseSensitive'>>(() => ({ + regex, + wholeWord, + caseSensitive, + }), [regex, wholeWord, caseSensitive]); + + const searchDecorations = React.useMemo(() => ({ + matchOverviewRuler: "#000000", + activeMatchColorOverviewRuler: "#000000", + activeMatchBorder: "#FF9632", + matchBorder: "#FFFF00", + }), []); + const searchOpts = React.useMemo<ISearchOptions>( () => ({ incremental: true, - regex, - wholeWord, - caseSensitive, - decorations: { - matchOverviewRuler: "#000000", - activeMatchColorOverviewRuler: "#000000", - activeMatchBorder: "#FF9632", - matchBorder: "#FFFF00", - }, + ...coreSearchOpts, + decorations: searchDecorations, }), - [regex, wholeWord, caseSensitive] + [coreSearchOpts, searchDecorations] );
823-850: Refactor search callbacks to reduce code duplicationThe error handling in search callbacks is duplicated. Consider extracting it into a utility function.
+ const handleSearchError = React.useCallback((e: Error) => { + console.warn("search error:", e); + // TODO: Consider showing a user-friendly error message + }, []); + + const executeSearch = React.useCallback((searchText: string, direction: 'next' | 'previous') => { + if (searchText === "") { + model.termRef.current?.searchAddon.clearDecorations(); + return; + } + try { + model.termRef.current?.searchAddon[direction === 'next' ? 'findNext' : 'findPrevious'](searchText, searchOpts); + } catch (e) { + handleSearchError(e); + } + }, [searchOpts, handleSearchError]); + searchProps.onSearch = React.useCallback( - (searchText: string) => { - if (searchText == "") { - model.termRef.current?.searchAddon.clearDecorations(); - return; - } - try { - model.termRef.current?.searchAddon.findPrevious(searchText, searchOpts); - } catch (e) { - console.warn("search error:", e); - } - }, - [searchOpts] + (searchText: string) => executeSearch(searchText, 'previous'), + [executeSearch] ); + searchProps.onPrev = React.useCallback( - () => { - try { - model.termRef.current?.searchAddon.findPrevious(searchVal, searchOpts); - } catch (e) { - console.warn("search error:", e); - } - }, - [searchVal, searchOpts] + () => executeSearch(searchVal, 'previous'), + [executeSearch, searchVal] ); + searchProps.onNext = React.useCallback( - () => { - try { - model.termRef.current?.searchAddon.findNext(searchVal, searchOpts); - } catch (e) { - console.warn("search error:", e); - } - }, - [searchVal, searchOpts] + () => executeSearch(searchVal, 'next'), + [executeSearch, searchVal] );
895-895: Enhance the comment for allowProposedApiThe current comment could be more descriptive about why the proposed API is needed specifically for search functionality.
- allowProposedApi: true, // needed for search + allowProposedApi: true, // Required by @xterm/addon-search to enable search functionality and decorations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/store/keymodel.ts(1 hunks)frontend/app/view/term/term.tsx(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/store/keymodel.ts
🔇 Additional comments (2)
frontend/app/view/term/term.tsx (2)
908-911: LGTM: Search results callback implementation
The implementation correctly updates the global store with search results, maintaining synchronization between the search addon and the UI.
967-967: LGTM: Search component integration
The Search component is properly integrated into the terminal view with all necessary props passed through.
This adds support for searching the terminal buffer using the
@xterm/addon-searchlibrary. It also adds three options for searching: regex, case-sensitive, and whole-word. These can be included or excluded from the search options foruseSearchdepending on whether the search backend supports it.I didn't like any of the Font Awesome icons for these toggles so until we have time to make some of our own icons that better match the Font Awesome style, I've appropriated VSCode's icons from their codicons font.
To implement the toggle-able buttons for these options, I've introduced a new HeaderElem component,
ToggleIconButton. This is styled similarly toIconButton, but when you hover over it, it also shows a highlighted background and when active, it shows as fully-opaque and with an accented border.Also removes the
useDismissbehavior for the search box to better match behavior in other apps.Also fixes the scrollbar observer from my previous PR so it's wider.