feat: Allow optionally displaying find with ctrl+f shortcut and allow close #501
Conversation
Signed-off-by: Adameska <[email protected]>
📝 WalkthroughWalkthroughAdds search UI and global shortcut handling to the terminal search controls, updates pod logs to pass the search prop to the TerminalWindow, and expands/rewrites related unit tests to mock Remote, simulate global key events, and validate incremental search, navigation, and close behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Window as Browser Window
participant Component as TerminalSearchControls
participant Remote as Remote(API_SYSTEM)
participant Terminal as TerminalInstance
participant Search as SearchAddon
Note over Component, Remote: mount -> query platform
Component->>Remote: request platform info (API_SYSTEM)
Remote-->>Component: platformName
Note over Window,Component: global key capture (capture phase)
Window->>Component: Ctrl/Cmd+F (keydown, capture)
Component-->>Window: preventDefault & stopPropagation (when handled)
Component->>Component: showSearch = true
Component->>Search: attach or create SearchAddon
Component->>Terminal: loadAddon(SearchAddon)
Component->>Component: focus find input
Component->>Search: findNext / findPrevious / findIncremental (on input / Enter / buttons)
Search-->>Terminal: highlight/match navigation
Terminal-->>Component: match count / no-results status
Component->>Component: update hasMatches -> show "No results" if none
Window->>Component: Escape (keydown)
Component->>Component: closeSearch (dispose SearchAddon, remove handler)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Potential attention points:
Pre-merge checks✅ Passed checks (3 passed)
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: 1
🧹 Nitpick comments (4)
packages/webview/src/component/terminal/TerminalSearchControls.svelte (2)
68-84: Search execution and match state handling are sound, with a small clarity tweak possibleGuarding
findNext/findPreviouswithif (searchTerm)and derivinghasMatchesfrom the addon’s boolean return avoids unnecessary searches and cleanly drives the “No results” UI. You might optionally also resethasMatchesinsidecloseSearch()to keep all search-related state in a single reset helper, even though the current UI logic doesn’t misbehave without it.
29-32: Consider unifying shortcut handling to avoid double-opening and reduce complexityRight now Ctrl/Cmd+F is handled both via
attachCustomKeyEventHandler(keydown) and the window keyup handler, each settingshowSearchand focusing the input. Functionally it works, but it’s redundant and a bit harder to reason about.A small simplification would be to have the terminal custom handler only block the shortcut from reaching xterm (return
falseon find combos) and delegate actually opening/focusing the search UI to the single windowkeyuppath. That keeps all “open search” behavior in one place while still preventing the terminal from consuming the shortcut.Also applies to: 95-103
packages/webview/src/component/terminal/TerminalWindows.spec.ts (1)
147-157: Search props test is a minimal smoke test; consider asserting search wiring if you want more confidenceThe test currently only verifies that
Terminalis constructed and the component renders whensearch: true. That’s fine as a smoke test, but if you later want stronger guarantees that the search controls are actually wired, you could trigger the find shortcut (e.g.,fireEvent.keyUp(window, { ctrlKey: true, key: 'f' })) and assert that the find input appears. Not mandatory given the dedicatedTerminalSearchControlstests, but worth considering.packages/webview/src/component/terminal/TerminalSearchControls.spec.ts (1)
241-264: Ctrl+F focus test could more directly validate focus and target the windowThe test currently waits for the “Find” input to appear after Ctrl+F, which confirms that the shortcut shows the search UI. If you want to tighten it, you might:
- Dispatch the shortcut on
windowrather thancontainerto more closely match<svelte:window on:keyup={onKeyUp}>, and- Assert focus explicitly, e.g.
expect(searchInput).toHaveFocus().Not required for correctness, but it would assert the intended behavior more precisely and avoid depending on bubbling details.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/webview/src/component/pods/PodLogs.svelte(1 hunks)packages/webview/src/component/terminal/TerminalSearchControls.spec.ts(4 hunks)packages/webview/src/component/terminal/TerminalSearchControls.svelte(3 hunks)packages/webview/src/component/terminal/TerminalWindows.spec.ts(2 hunks)
⏰ 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). (4)
- GitHub Check: Build / windows-2022
- GitHub Check: Build / macos-14
- GitHub Check: Build / ubuntu-24.04
- GitHub Check: Build extension OCI image
🔇 Additional comments (9)
packages/webview/src/component/pods/PodLogs.svelte (1)
109-109: Wiringsearchinto PodLogs’ TerminalWindow looks goodPassing the
searchboolean alongsideconvertEol/disableStdIncleanly opts PodLogs into the new search UI without altering existing behavior.packages/webview/src/component/terminal/TerminalSearchControls.svelte (3)
24-32: Remote/platform lookup and shortcut detection are reasonableFetching
platformNameviaRemote+API_SYSTEMand usingisFindShortcutto switch between Cmd+F on macOS and Ctrl+F elsewhere is a sensible, centralized approach.
34-53: SearchAddon lifecycle and terminal key handler are correctly integratedCreating/activating
SearchAddoninonMountand disposing it inonDestroy, plus usingattachCustomKeyEventHandlerto prevent the terminal from consuming the find shortcut, is aligned with xterm’s APIs and keeps addon management localized to this component.
114-150: Search UI rendering and accessibility look solidConditional rendering on
showSearch, explicitaria-labels for the input and buttons, and the “No results” message tied tosearchTerm && !hasMatchesprovide a clear, accessible UX that matches typical editor find widgets.packages/webview/src/component/terminal/TerminalWindows.spec.ts (1)
49-58: Expanded terminal mock matches the integration surfaceAdding
attachCustomKeyEventHandler,getSelection,clearSelection,write,open, andloadAddontocreateTerminalMockaligns the mock with how TerminalWindow and addons interact with xterm, and should prevent type/runtime issues during tests.packages/webview/src/component/terminal/TerminalSearchControls.spec.ts (4)
39-56: Remote context and platform mocking are correctly set upThe
mockSystemApi+mockRemote.getProxywiring and resetting of resolved values inbeforeEachensuregetPlatformNamebehaves predictably across tests, and the “search addon should be loaded” test nicely assertsSearchAddon.prototype.activateis called with the provided terminal.Also applies to: 58-70
87-121: Input typing test exercises incrementalfindNextbehavior correctlyTriggering Ctrl+F, waiting for the “Find” input, then typing and asserting
SearchAddon.prototype.findNext('hello', { incremental: true })matches the component’sonSearch→onSearchNext(true)flow and validates that incremental search is wired.
123-157: Enter-key behavior test aligns withonKeyPressedlogicThe
{Enter}path on the search input and the expectation thatfindNextis called with{ incremental: true }correctly mirror theonKeyPressedhandler that treats Enter as “next match (incremental)”.
159-198: Next/previous button tests cover SearchAddon wiring wellThe “arrow down” and “arrow up” tests wait for the search UI, type a term, then click the respective buttons and assert
findNext/findPreviousreceive the right term and options. This gives good coverage of the button handlers without over-coupling to implementation details.
packages/webview/src/component/terminal/TerminalSearchControls.svelte
Outdated
Show resolved
Hide resolved
Signed-off-by: Adameska <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/webview/src/component/terminal/TerminalSearchControls.svelte(3 hunks)
⏰ 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). (4)
- GitHub Check: Build / ubuntu-24.04
- GitHub Check: Build extension OCI image
- GitHub Check: Build / windows-2022
- GitHub Check: Build / macos-14
🔇 Additional comments (1)
packages/webview/src/component/terminal/TerminalSearchControls.svelte (1)
108-144: LGTM on the template structure.The conditional rendering, accessibility labels, and button handlers are well-implemented. The "No results" feedback provides good UX.
Signed-off-by: Adameska <[email protected]>
gastoner
left a comment
gastoner
left a comment
There was a problem hiding this comment.
Screencast_20251126_125316.webm
On Linux:
- When openning the logs it would be nice to not click in to terminal window when I have just opened it
- Cant exit the search with shortcut - esc will get me to list of all pods - I guess some propagating of events
…ombo we care about Signed-off-by: Adameska <[email protected]>
This was bugging me too and i gave up but spent some more time and hopefully was able to fix it and capture escape when the search is shown preventing the parent from capturing it. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/webview/src/component/terminal/TerminalSearchControls.svelte (1)
134-141: Useonkeydowninstead of deprecatedonkeypress.The
onkeypressevent is deprecated and doesn't reliably fire for all keys. SinceonKeyPressedonly handles Enter (a printable key), it works now, but usingonkeydownis the modern standard and will be more reliable across browsers.<Input bind:element={input} placeholder="Find" aria-label="Find" - onkeypress={onKeyPressed} + onkeydown={onKeyPressed} oninput={onSearch} value={searchTerm} />packages/webview/src/component/terminal/TerminalSearchControls.spec.ts (1)
264-395: LGTM!The test coverage is comprehensive, including keyboard shortcuts (Ctrl+F, Escape) and UI button interactions (Previous, Next, Close). The consistent test structure makes the suite maintainable.
Consider adding a test for the "No results" message visibility when
findNextreturnsfalse, though this is optional given the straightforward rendering logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/webview/src/component/terminal/TerminalSearchControls.spec.ts(3 hunks)packages/webview/src/component/terminal/TerminalSearchControls.svelte(2 hunks)
⏰ 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). (4)
- GitHub Check: Build / windows-2022
- GitHub Check: Build / macos-14
- GitHub Check: Build extension OCI image
- GitHub Check: Build / ubuntu-24.04
🔇 Additional comments (10)
packages/webview/src/component/terminal/TerminalSearchControls.svelte (7)
1-28: LGTM!The imports and state setup are well-structured. Good use of the Remote context pattern for platform detection, and the new reactive state variables (
showSearch,hasMatches,platformName) are properly declared with Svelte 5 runes.
29-38: LGTM!Platform-aware shortcut detection is correctly implemented. Using
tick()before focusing ensures the input is rendered first.
40-67: LGTM!The
onMountlifecycle is well-implemented:
- Capture phase for global keydown ensures shortcuts are intercepted early.
- Terminal's custom key event handler correctly prevents the terminal from consuming the find shortcut.
- Returning
falsefrom the handler properly signals xterm to ignore the event.
69-79: LGTM!Proper cleanup in
onDestroyensures no memory leaks. The Escape handling was correctly moved out ofonKeyPressed(addressing the previous inconsistency concern).
81-106: LGTM!Good implementation of search result tracking with
hasMatches. The incremental search pattern and empty term handling provide good UX feedback.
108-124: LGTM!The global keydown handler correctly uses
stopImmediatePropagation()to prevent event bubbling to other handlers. ThecloseSearch()function properly resets bothshowSearchandsearchTerm, ensuring consistent state.
142-163: LGTM!The search controls are well-structured with proper accessibility labels. The button handlers and Tailwind v4 styling are correct.
packages/webview/src/component/terminal/TerminalSearchControls.spec.ts (3)
31-51: LGTM!The mock setup properly covers the Remote context, system API, and terminal interfaces needed for testing. Good practice to mock
getProxyto returnmockSystemApi.
53-118: LGTM!The test scaffolding is well-designed:
- Capturing the window keydown handler while still calling the original is a solid pattern.
- Helper functions (
createKeyboardEvent,openSearchViaKeyboard,closeSearchViaEscape) reduce duplication and improve readability.- Using
tick()after triggering events ensures Svelte state updates are flushed.
120-262: LGTM!The test cases are well-structured with proper async handling using
vi.waitFor(). Good practice to wait for platform detection and handler registration before triggering keyboard interactions.
2 participants
The find menu works more similar to how other editors function (if enabled, ctrl/cmd+F will open and it is now closable).