[AntonioKL]

ELI5

When you search inside a terminal, the find bar highlights every match and shows which one you're on. Clicking the find bar's arrows or pressing Enter worked. Pressing Cmd+G or Cmd+Shift+G (Ctrl+G on Windows/Linux) did move to the next match, but the orange "current match" highlight stayed put, the 3/47 counter froze, and highlights stopped updating when new output came in. The shortcut was sending the search without the "please highlight" settings. Now the shortcut and the find bar send the same settings, so they behave the same.

What Changed

• Before: Cmd+G / Cmd+Shift+G moved the xterm selection, but the active-match highlight, the find-bar counter, and the re-highlight on new output all stopped updating.
• After: Cmd+G / Cmd+Shift+G move the active-match highlight, keep the counter in step, and keep highlights refreshing, the same as the find bar's arrows and Enter.
• Mechanism:
  • New src/renderer/src/components/terminal-search-options.ts with one buildTerminalSearchOptions(). It holds the decoration palette, moved unchanged from TerminalSearch.tsx.
  • TerminalSearch.tsx builds its options with this helper. Its behavior doesn't change.
  • runTerminalSearchNavigation in terminal-keyboard-shortcut-matching.ts now passes the decorated options to findNext/findPrevious. Before, it passed only { caseSensitive, regex }.
  • The two runTerminalSearchNavigation tests now check that the shortcut sends decorations. They previously asserted the buggy bare options.

Why

@xterm/addon-search decides three things from whether the last find call included decorations:

1. _selectResult only creates the active-match decoration when it gets options.
2. fireResultsChanged(!!decorations) returns early without them, so onDidChangeResults (which drives the counter) never fires.
3. _updateMatches checks lastSearchOptions?.decorations before it re-highlights after new output.

The shortcut's bare options replaced lastSearchOptions, which turned all three off. Having one options builder for both paths means they can't drift apart again.

This version is smaller than the original PR. main already has a match counter subscribed to onDidChangeResults (including the N+ overflow formatting), so this PR leaves out the original's counter, highlightLimit pin, and JSDoc additions. With this fix, the existing counter now follows Cmd+G.

Linked Issue

Fixes #18843

Visual Proof

No screenshot or video. The proof is a before/after probe that runs a real xterm Terminal + SearchAddon in happy-dom. NEEDLE appears only in scrollback rows 0-45 of 350:

== origin/main ==
EVENTS [{"resultIndex":0,"resultCount":200}]                       # Cmd+G x2: no result events after the first
PREV EVENTS [{"resultIndex":0,"resultCount":10}] activeRow undefined selRow 40   # active highlight never moves
REFRESH EVENTS []                                                  # new output: no re-highlight
6 failed

== this branch ==
EVENTS [{"resultIndex":0,...},{"resultIndex":1,...},{"resultIndex":2,...}]
PREV EVENTS [{"resultIndex":0},{"resultIndex":9},{"resultIndex":8}] activeRow 40 selRow 40   # highlight follows selection
REFRESH EVENTS [{"resultIndex":1,"resultCount":11}]
63 passed

Testing

• I manually tested these changes locally

• Automated tests added/updated, or explained why not below

• Reproduced the bug on origin/main with the probe above. Event index stuck at 0, no active decoration, no refresh after output, and the regex + case-sensitive shortcut path also stopped emitting results. All pass on this branch.

• vitest run keyboard-handlers.test.ts TerminalSearch*: 31/31 pass after rebasing on current main.

• oxlint and oxfmt --check are clean on the 4 touched files.

• No Electron run. The change is renderer-only and has no platform-specific code (the shortcut is already platform-aware upstream of this function), so macOS, Linux and Windows behave the same.

Review

• Reviewed with Claude and Codex, both approved at low risk. The test assertions now check for decorations directly instead of rebuilding the expected value from the production helper, so they'd catch a regression.
• Tradeoff: after Cmd+G, the addon's 200 ms debounced re-highlight keeps running on new output. The find bar already does this, so it's the same cost, bounded by highlightLimit.

Agent skill upstream boundary

• Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

• Security: no new inputs or IPC.
• Cross-platform: renderer only, with no platform branches.
• Remote SSH and mixed versions: unaffected, since search runs in the client's xterm.
• Mobile: no terminal find surface involved.
• Folder workspaces: unaffected.
• Performance: one small options object per find call, the same as the find bar.

Checklist

• This PR is small and focused
• I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
• Before/after screenshots or videos attached for UI changes, or N/A with reason
• Self-reviewed for correctness, security, and performance
• Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
• pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

Original fix by @AntonioKL in #18844. Their authorship is kept on the squashed commit.

[coderabbitai]

No actionable comments were generated in the recent review.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: f4ea4268-b335-4207-96bc-43fef71f9e95

📥 Commits

Reviewing files that changed from the base of the PR and between cc7444c and 0d5833f.

📒 Files selected for processing (5)

• src/renderer/src/components/TerminalSearch.test.tsx
• src/renderer/src/components/TerminalSearch.tsx
• src/renderer/src/components/terminal-pane/keyboard-handlers.test.ts
• src/renderer/src/components/terminal-pane/terminal-keyboard-shortcut-matching.ts
• src/renderer/src/lib/pane-manager/pane-dom-creation.ts

🚧 Files skipped from review as they are similar to previous changes (5)

• src/renderer/src/components/terminal-pane/keyboard-handlers.test.ts
• src/renderer/src/lib/pane-manager/pane-dom-creation.ts
• src/renderer/src/components/TerminalSearch.tsx
• src/renderer/src/components/TerminalSearch.test.tsx
• src/renderer/src/components/terminal-pane/terminal-keyboard-shortcut-matching.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

Walkthrough

Terminal search options now come from a shared buildTerminalSearchOptions helper. The helper includes search flags, incremental state, and match decorations. The terminal pane applies a shared highlight limit to SearchAddon. TerminalSearch listens for search-result events, tracks the active index and total count, discards stale results, and renders a match counter. Tests cover result events, counter formatting, clearing, shared options, and active-match decorations.

Merge Risk: Minimal · up to 0d583

The change centralizes terminal search options and adds match-result display behavior without any identified merge-blocking risk.

Pre-merge checks: 5

Check name	Status	Explanation
Linked Issues check		The changes satisfy issue [#18843]. Shortcut navigation now uses the same decoration options as find-bar searches, active-match highlights remain synchronized during navigation and terminal output, an…
Out of Scope Changes check		The changed production files and tests are directly related to terminal search navigation, search decorations, match-result reporting, highlight-limit configuration, and regression coverage. The added…
Docstring Coverage		Docstring coverage is 81.82% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 6 files.
Title check		The title clearly identifies the main fix: preserving find-bar highlights during Cmd+G and Cmd+Shift+G navigation.
Description check		The description is detailed and covers the user impact, implementation, rationale, linked issue, testing, compatibility considerations, and checklist. It provides before-and-after probe results instea…

Full details: Linked Issues check

Explanation

The changes satisfy issue [#18843]. Shortcut navigation now uses the same decoration options as find-bar searches, active-match highlights remain synchronized during navigation and terminal output, and the match counter reports results outside the viewport. Regression tests cover the affected behavior.

Full details: Out of Scope Changes check

Explanation

The changed production files and tests are directly related to terminal search navigation, search decorations, match-result reporting, highlight-limit configuration, and regression coverage. The added documentation comments support the touched code and do not introduce unrelated behavior.

Full details: Description check

Explanation

The description is detailed and covers the user impact, implementation, rationale, linked issue, testing, compatibility considerations, and checklist. It provides before-and-after probe results instead of screenshot or video attachments, and manual testing plus the full command suite remain unchecked, but the description is otherwise mostly complete.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[pullfrog]

ℹ️ No critical issues — one edge case in the match counter and one coverage gap worth a look.

Reviewed changes

• Shared buildTerminalSearchOptions builder — extracts the decoration palette and search flags into terminal-search-options.ts so both the find bar and the Cmd+G shortcut construct the same options, eliminating the drift that caused the bug.
• Shortcut navigation now carries decorations — runTerminalSearchNavigation builds options through the shared helper, so Cmd+G moves the active-match decoration, fires onDidChangeResults, and keeps highlights refreshing on write.
• Match counter — subscribes to searchAddon.onDidChangeResults and renders a 3/47 counter (red 0/0 for no matches), cleared on empty query and pane switch; subscription effect is declared before the search effect.
• Tests — regression test asserting shortcut navigation passes decorations.activeMatchBackground, plus counter tests for reported position/total and clearing.

The root-cause analysis checks out: in SearchAddon.ts, _selectResult only creates the active decoration when searchOptions?.decorations is set, _fireResults passes !!searchOptions?.decorations to fireResultsChanged (which returns early when false), and _updateMatches gates on lastSearchOptions?.decorations. Passing decorations on the navigation path is exactly the right fix.

ℹ️ Nitpicks

• The red 0/0 (zero-match) branch is the one behavior the PR body calls out that has no test. The matches.count === 0 ? 0 : ... ternary in the render is a distinct branch — a test emitting { resultIndex: -1, resultCount: 0 } would pin it.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

[AntonioKL]

Ready for review — every template section is filled in, and before/after evidence is now embedded in the description.

One thing that needs a maintainer: this is a fork PR from a first-time contributor, so the lint/typecheck/test/build workflows are held pending approval. Only pullfrog and track-community-pr have run so far. Happy to rebase or adjust anything.

[pullfrog]

✅ No new issues found.

Reviewed changes

• formatMatchCount counter formatter — extracts the index < 0 branch into a pure helper so a selection past the highlight limit renders 1000+ (or the bare total when no match is selected) instead of the misleading 0/1000, resolving the prior review's counter edge case.
• Addon/query-tagged match state — MatchCount now carries the SearchAddon and requestQuery it was reported for, and currentMatches is derived in render, so a pane switch or emptied/edited query invalidates the counter without the removed setMatches(null) resets (which also cleared the react-doctor finding).
• Pinned highlight limit — TERMINAL_SEARCH_HIGHLIGHT_LIMIT = 1000 is set at addon construction in pane-dom-creation.ts, matching the addon's verified default so the counter knows where resultCount is truncated.
• Expanded tests — new assertions pin the red 0/0, the 1000+ cap, and the bare-total-when-unselected branches, closing the prior zero-match coverage gap.

The prior inline finding is fully addressed: formatMatchCount branches on index < 0 before falling through to index + 1 / count, and the 1000+/bare-total tests exercise exactly that path. The highlight limit is confirmed identical to the addon's Constants.DEFAULT_HIGHLIGHT_LIMIT (1000) and is not altered by the vendored patch, so the constructor pin is behavior-neutral as claimed.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}