Merged
Conversation
Add search overlay to the embedded terminal, matching VS Code's behavior. Powered by @xterm/addon-search. - Cmd+F opens an overlay (input + match count + prev/next/close) - Enter = next match, Shift+Enter = previous, Escape = close - Input changes search incrementally - VS Code dark theme highlight colors Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an embedded terminal search feature (Cmd+F) using the Changes
Sequence DiagramsequenceDiagram
actor User
participant Terminal as Terminal Component
participant SearchAddon as `@xterm/addon-search`
participant UI as Search Overlay UI
User->>Terminal: Press Cmd+F
Terminal->>Terminal: Intercept key, open overlay
Terminal->>UI: Render overlay & focus input
User->>UI: Type query
UI->>SearchAddon: findNext(query)
SearchAddon->>Terminal: Highlight matches / return count
Terminal->>UI: Update match counter
User->>UI: Press Enter / Shift+Enter
UI->>SearchAddon: findNext / findPrevious
SearchAddon->>Terminal: Update highlight
User->>UI: Press Escape
UI->>SearchAddon: Clear decorations
Terminal->>Terminal: Close overlay, restore state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/terminal-tab.tsx (2)
95-97: Consider storing the disposable fromonDidChangeResults.The
onDidChangeResultsmethod returns anIDisposablethat could be stored and disposed during cleanup. However, since the terminal is a singleton that persists for the app lifetime, and the addon is tied to the terminal's lifecycle, this is a minor concern.🔧 Optional: Store disposable for completeness
+ const searchResultDisposableRef = useRef<{ dispose: () => void } | null>(null); // ...in useEffect after loading searchAddon: - searchAddon.onDidChangeResults((result) => { - setSearchResult(result); - }); + searchResultDisposableRef.current = searchAddon.onDidChangeResults((result) => { + setSearchResult(result); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/terminal-tab.tsx` around lines 95 - 97, The callback subscription returned by searchAddon.onDidChangeResults is an IDisposable but currently unused; capture its return value (e.g., const disposable = searchAddon.onDidChangeResults(...)) and ensure it is disposed during cleanup (call disposable.dispose() or push it into the component/terminal disposables array or the useEffect cleanup that manages terminal lifecycle). Update the code around searchAddon.onDidChangeResults and setSearchResult so the IDisposable is stored and disposed when the terminal/component cleans up to avoid leaking the subscription.
182-275: Search overlay implementation looks good.The UI correctly implements:
- Incremental search on input change
- Keyboard navigation (Escape, Enter, Shift+Enter)
- Match counter with proper 1-indexing and "No results" state
- Hover effects on buttons
💡 Optional: Add aria-label for accessibility
<input ref={searchInputRef} type="text" value={searchQuery} placeholder="Find" + aria-label="Search in terminal" onChange={(e) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/terminal-tab.tsx` around lines 182 - 275, Add accessible labels and roles: give the search input (bound to searchInputRef, value searchQuery) an aria-label like "Find in terminal" and ensure the match counter span (uses searchResult) has aria-live="polite" so screen readers announce changes; add aria-labels to the Previous/Next/Close buttons (that call findPrevious, findNext, closeSearch and use searchButtonStyle) such as "Previous match", "Next match", "Close search" and include title duplication only if needed for visual tooltips.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/terminal-tab.tsx`:
- Around line 95-97: The callback subscription returned by
searchAddon.onDidChangeResults is an IDisposable but currently unused; capture
its return value (e.g., const disposable = searchAddon.onDidChangeResults(...))
and ensure it is disposed during cleanup (call disposable.dispose() or push it
into the component/terminal disposables array or the useEffect cleanup that
manages terminal lifecycle). Update the code around
searchAddon.onDidChangeResults and setSearchResult so the IDisposable is stored
and disposed when the terminal/component cleans up to avoid leaking the
subscription.
- Around line 182-275: Add accessible labels and roles: give the search input
(bound to searchInputRef, value searchQuery) an aria-label like "Find in
terminal" and ensure the match counter span (uses searchResult) has
aria-live="polite" so screen readers announce changes; add aria-labels to the
Previous/Next/Close buttons (that call findPrevious, findNext, closeSearch and
use searchButtonStyle) such as "Previous match", "Next match", "Close search"
and include title duplication only if needed for visual tooltips.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62d2162b-f54f-4b15-88b6-2ea0f1c5f445
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
CHANGELOG.mdpackage.jsonsrc/terminal-tab.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/terminal-tab.tsx">
<violation number="1" location="src/terminal-tab.tsx:243">
P2: When the number of matches exceeds the search addon's highlight limit (default 1000), `resultIndex` is `-1` per the API docs, causing the counter to display "0 of N". Handle this case explicitly — e.g., show "N results" or "1000+ results" instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
- cubic P2: handle resultIndex=-1 (when matches exceed the search addon's highlight limit of 1000) — show "N+ results" instead of "0 of N" - CodeRabbit: store and dispose the onDidChangeResults IDisposable on component unmount - CodeRabbit: add aria-label to search input + buttons, aria-live="polite" to the match counter Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
Thanks for the nitpicks @coderabbitai — both addressed in c4392ba:
🤖 On behalf of @grimmerk — generated with Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add
Cmd+Fsearch to the embedded terminal, matching VS Code's terminal behavior. Powered by@xterm/addon-search.UX
Cmd+Fopens a search overlay (top-right of the terminal)Enterjumps to next match,Shift+Enterto previous,Escapecloses#515C6Afor matches,#A8AC94for active match)Why xterm.js doesn't ship a search UI
xterm.js (the library VS Code's terminal also uses) only provides search APIs through
@xterm/addon-search—findNext(),findPrevious(),onDidChangeResults. The UI is the host app's responsibility. This PR builds a small React overlay that calls into the addon.Changes
package.json— add@xterm/addon-search@^0.16.0src/terminal-tab.tsx:SearchAddonCmd+FinattachCustomKeyEventHandler(returnsfalseso the keystroke doesn't reach the PTY)onDidChangeResultsfor live match countsearchOpenstate) with input + counter + buttonscloseSearch()clears decorations and refocuses the terminalCHANGELOG.md+ version bump to 1.0.73Test plan
Cmd+Fopens the search overlayEnter/Shift+Entercycle through matchesEscapecloses the overlay and refocuses the terminalCmd+Foutside the Terminal tabCmd+Kclear,Shift+Entermultiline,Cmd+←/→) still work🤖 On behalf of @grimmerk — generated with Claude Code
Summary by cubic
Add
Cmd+Fsearch to the embedded terminal with a lightweight overlay and keyboard navigation, matching VS Code behavior. Uses@xterm/addon-searchfor search, decorations, and live result counts.New Features
Cmd+Fopens a search overlay with input, match counter, and prev/next/close; incremental search as you type.Enternext,Shift+Enterprevious,Escapecloses; highlights match VS Code dark theme and the keystroke isn’t sent to the PTY.Bug Fixes
onDidChangeResultslistener on unmount; add ARIA labels to input/buttons and aria-live to the counter.Written for commit c4392ba. Summary will update on new commits.
Summary by CodeRabbit