Add search function within patch viewer by rosemarie-o · Pull Request #2 · igsilya/leadlight

rosemarie-o · 2026-07-15T22:09:40Z

Add a vim-like expression search /
Supports regex like vim and has the same n/N controls

Assisted-by: Claude Sonnet 4.5, Claude Code

dceara

Thanks @roseoriorden for this feature, I tried it locally and it seems to work quite nicely!

Here's some comments on the actual code from my side. Please take them with a grain of salt as it's the first time I really look closely at the leadlight code so some of them might be completely off.

Thanks,
Dumitru

dceara · 2026-08-07T10:51:24Z

+	viewportLoading  bool
+	viewportOffset   int
+	viewExpanded     bool
+	searching        bool


Nit: Should all this search related state be part of a separate group of fields, lower. I mean, it applies to the compare mode too and that has its own separate section of fields.

dceara · 2026-08-07T11:11:37Z

+func (m *Model) handleSearchInputMode(msg tea.KeyMsg, updateMatches func()) (tea.Model, tea.Cmd) {
+	key := msg.String()
+	switch key {
+	case "enter":


I'm not sure I like the mixing of search logic with key handling. keys.go seems like the appropriate place to handle all input keys for all modes. Would it make sense to keep it like that?

dceara · 2026-08-07T11:16:56Z

+	viewportLoading  bool
+	viewportOffset   int
+	viewExpanded     bool
+	searching        bool


searching is a bit misleading IMO. It actually is used to reflect the "the user is typing the search query" state. Only once the search is "committed" (enter is hit) we actually are in searching state.

dceara · 2026-08-07T12:02:19Z

 		right = comparePadLine(right, rightWidth,
 			compareDiffKind(m.compare[1].kinds, idx))
+
+		left = m.highlightLineIfMatched(left, idx)


Here, if the match is only on one side (left or right) but not on the other we'll try to highlight on both sides and fail for the side that doesn't have the match.

But does that mean that we'd also potentially scroll incorrectly? Shouldn't the searchMatches be part of the compareSide struct instead?

Which means, shouldn't we have a let's call it SearchMatcher struct, that stores all matches. And we could potentially use it in three places:

for the viewportLines (in patch view mode)

for the left lines (in compare view mode)

for the right lines (in compare view mode)

Maybe that will also remove the need for the slightly unnatural if m.viewMode == viewCompare { checks in search.go, what do you think?

igsilya · 2026-08-07T17:56:44Z

Some LLM review comments:

Commit 1: 9314b07 — "tui: search: Add vim-like expression search /<expr>." (3 issues, medium)

\C always overrides \c — In parseVimSearchPattern, the \C check runs after \c, so \C always wins when both flags appear. Vim uses last-flag-wins semantics, so foo\C\c should be case-insensitive but isn't.
Compare view duplicate matches — updateCompareSearchMatches indexes both sides of a compare view with the same lineIdx, producing duplicate matches for identical lines. Pressing n visits the same visual line twice. Also, highlightLineIfMatched doesn't distinguish panels, so both sides show current-match highlighting simultaneously.
Stale searchMatches after content rebuild — switchToComment and the WindowSizeMsg handler rebuild viewport content without clearing or re-running search. Pressing n after navigating to a different comment or resizing jumps to stale line indices.

Commit 2: 0f38713 — "tui: search: Add search history." (2 issues, low)

N to recall last search navigates to first match — recallLastSearch() always sets searchIdx = 0, so pressing N (which should go backward) starts at the first match instead of the last.
Up arrow discards in-progress search text — Pressing up to browse history overwrites partially-typed input without saving it. Pressing down back to index -1 restores an empty string instead of the original partial text.

Commit 3: 4af6be2 — "tui: search: Make esc exit patch, q clear search then exit." (2 issues, low)

Duplicated viewport exit cleanup — The "q" and "esc" cases in both handleViewportKey and handleCompareKey contain identical 7-field cleanup code. Should be extracted to a helper to avoid divergence.
Typo in commit message — "serach" should be "search".

I didn't look too deep into the code myself, but one of the big things I tried to avoid in different places in leadlight is parsing and/or updating what was already rendered. This is fragile and hard to maintain. The search logic should be applied to the original text and then the rendering pass should already highlight the findings. This may require extra translation between original and rendered line indexes due to possible wrapping, but would solve a few problems like inability to find wrapped text or the need to re-apply the search and re-highlighting after resize or comment switching that the LLM review flagged above.

igsilya

Marking as 'changes requested'.

The diff path expands tabs and then wraps, but the body path wrapped first and expanded each segment afterwards. A tab grows to the next 8-column stop, so wrapping an unexpanded line undercounts its width: the styled result can come out wider than the terminal, which then wraps it a second time and throws off the viewport's line accounting. A tabbed body line at width 40 overflowed by up to 12 columns. Expand on the whole line before wrapping, in both the mbox body and the comment content, and drop the per-segment expansion. Expanding a segment was wrong on its own terms too, since tab stops are counted from the start of the line, not from the start of a segment. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Rosemarie O'Riorden <rosemarie@redhat.com>

The patch and compare views stored their content as a []string of finished terminal rows: already wrapped to the width and with the lipgloss escape sequences baked into the text. That is a lossy representation. Once a row has been built there is no way to tell which source line it came from, where in that line it starts, or which parts of it are text as opposed to colour, so anything that wants to reason about the content has no choice but to parse the rendering back apart. This is groundwork for the search that follows. That search has to match against the text as it was written rather than as it was drawn, and has to know which row on screen shows a given position in it; neither question can be answered from a finished row. Nothing here changes what ends up on the terminal. Introduce renderedDoc, which keeps both halves: the logical text, the lines as they read before wrapping and without styling, and the visual rows they were laid out into, along with the mapping between them. A row is a list of (text, style) spans rather than a string, and spans that are chrome the formatter invented rather than source text, such as the wrap arrow or a column's padding, are marked as such. Escape sequences are now produced at the very last moment, for the rows actually on screen, instead of for the whole document up front. The formatters are rewritten to build documents. FormatMbox, FormatMboxParts, FormatComment and FormatChecks keep their signatures and become flatteners over them, so the existing output assertions go on covering them unchanged. writeHeader, writeStyledLine, styleBodyLine and styleDiffLine fold into appendHeaderDoc, appendBodyLineDoc and appendDiffLineDoc, which both views now share. In the compare view the per-row diff tint moves from a parallel []diffLineKind onto the row itself, which retires compareDiffKind and padToEqualWithKinds. wrapLine and wrapHeaderValue gain variants that report the rune offset each segment starts at. The offsets are exact because neither wrapper rewrites text, only cuts it: a segment is always a contiguous run of its source line. That holds for header values too, where the comma a folded line ends with is the source's own separator and only the space after it is dropped. Verified against a corpus of 180 header, body, diff, comment and check renderings at five widths with collapsing both ways: the visible text is byte for byte what it was. Two styling differences are deliberate. A continuation arrow is emitted as its own span, so a wrapped line carries one more escape pair than before, with the same colours and the same text; the arrow has to be separable from the source text for anything to reason about which columns hold what. And a body line that literally starts with the arrow character is no longer mistaken for a continuation and styled as one, now that continuations are structural rather than guessed at from a prefix. Two latent bugs turned up while doing this and are fixed here. wrapHeaderValue's overflow path sliced by byte offset, so a multi-byte character in a long header could be cut in half and the lines it produced were narrower than the terminal in proportion to how much non-ASCII they held. And documents do not carry the trailing empty row that splitLines left behind, which removes one blank line from the end of the patch viewport and of the fallback compare rendering. The renderer reads rows straight out of the document with Line, and setViewportDoc is a plain setter. Both are the shapes the search needs later: it wraps the row rendering to apply a highlight, and hooks setViewportDoc so that the matches are recomputed whenever the content is replaced. Neither hook exists yet because there is nothing to hook. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Rosemarie O'Riorden <rosemarie@redhat.com>

Search within the patch and compare views the way vim does: "/" opens a prompt, the query is a regular expression, and n and N step forward and backward through the matches with the current one highlighted differently from the rest. The \c and \C flags force case-insensitive and case-sensitive matching and, as in vim, may appear anywhere in the pattern with the last one winning. A pattern that is not a valid regular expression is matched literally, so an unfinished one still finds the text that was typed. Match colours differ between the dark and light themes. The search runs against the logical text of a document, not against the rows on screen. A match is recorded as a rune range inside an unwrapped source line, which has two consequences worth stating. A term is found even when a wrap falls in the middle of it, and it is then highlighted on both of the rows it spans. And the coordinates do not move when the window is resized, because re-wrapping changes the rows and not the source. Highlighting happens as a row is rendered, by giving part of a span a different style. Nothing inspects or edits an escape sequence, and the compare view applies the highlight before the column padding, so a match at the end of a line is not thrown off by the padding after it. Each document gets its own matcher: one for the patch viewport and one for each compare column. The columns therefore do not share a row index, and a match belongs to exactly one of them, so only that column lights up. searchOrder merges the matches of every column into reading order, down the screen and left column before right, which is what n and N walk; the view mode is consulted in exactly one place, searchTargets, which answers which documents are being searched. The matches are recomputed whenever the content changes. Every assignment to the patch document goes through setViewportDoc and buildCompareContent defers the rescan, so a document cannot be shown beside matches that were found in a previous one. That is what keeps the highlighting correct across a resize, a comment switch, a sync or an expand toggle, rather than remembering to re-run the search at each of those call sites. Matches are indexed by source line, so rendering a row is a map lookup rather than a scan of the whole match list. A query that finds nothing is dropped rather than left active, since there would be nothing to step through or highlight, and the status line says so for a few seconds. While the query is still being typed the prompt reports "[no matches]" instead; saying it out loud on every keystroke would fire on almost every prefix of a real query. A pattern that is not a valid regular expression is treated the same way: no matches, and the message. It is tempting to fall back to searching for the pattern as literal text, but that answers a question the user did not ask and does it silently, so "/*.c" looks simply absent rather than rejected. Glob syntax is not regular expression syntax and is not accepted as such; ".c" files are found with /\.c and not with /*.c, exactly as in vim. n and N are offered in the help bar only when there is something to step to, and fall through to the view's own bindings when there is no search at all. The prompt takes any printable rune, including non-ASCII, and backspace removes a whole rune rather than a byte. Searching is also how q and esc differ in these views. esc leaves outright. q drops an active search on the first press and leaves on the second, so that a search can be dismissed without losing your place; when nothing matched there is no search to dismiss and q leaves at once. Two things this touches rather than adds. The patch and compare renderers stop reading rows straight out of the document and go through renderDocRow instead, which is the same thing plus the highlight. And setViewportDoc, which until now only assigned the field, gains the rescan. Both hooks had nowhere to attach before there was a search; neither replaces anything that was wrong. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Rosemarie O'Riorden <rosemarie@redhat.com>

Remember the queries that have been run, so that a search can be repeated without retyping it. At the "/" prompt the up and down arrows walk the list, most recent first, the way they do in a shell. A query being typed is stashed on the way into the history and restored on the way back out, so browsing never loses it. Re-running an earlier query moves it to the front rather than adding a second copy of it. Pressing n or N with no search active re-runs the most recent query outright, without going through the prompt. N puts the cursor on the last match rather than the first, since it means "go backwards". If the recalled query no longer matches anything in the content now being viewed it is dropped, the same as a query that is typed and matches nothing. The history is kept in the project database, in the sync_state table that already holds durable state of this kind, so it outlives the session. That also makes it per-project, which is what you want: a query that is useful on one list rarely is on another. The config is not an option, since leadlight only ever reads it and it is shared between projects anyway. Fifty entries are kept. The list is only ever walked one entry at a time, with no way to search within it, so a longer tail would just sit in the database unread. A query that matched nothing is remembered too, since a typo is exactly what you want to recall and correct. A stored value that cannot be decoded is dropped and logged rather than reported, as an unusable history is not worth interrupting startup over; the next committed search overwrites it. Everything still works with no database at all, which is how NewModelWithData and much of the test suite build a model. Assisted-by: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Rosemarie O'Riorden <rosemarie@redhat.com>

rosemarie-o · 2026-10-13T15:03:34Z

I just updated this PR with a v2

rosemarie-o · 2026-10-19T15:27:12Z

something I need to fix is when a search is initiated, it should not jump to the top. it should start the search from where the user is in the patch.

rosemarie-o · 2026-10-19T15:29:28Z

Oh no I found another bug. While typing the string to search, the user should be able to use the arrow keys to move around in the string to edit it.

rosemarie-o force-pushed the main branch 3 times, most recently from e3679b1 to 4af6be2 Compare July 20, 2026 19:17

dceara reviewed Aug 7, 2026

View reviewed changes

igsilya requested changes Sep 25, 2026

View reviewed changes

rosemarie-o closed this Oct 13, 2026

rosemarie-o force-pushed the main branch from 4af6be2 to 7f8d710 Compare October 13, 2026 14:05

rosemarie-o added 4 commits October 13, 2026 10:54

rosemarie-o reopened this Oct 13, 2026

Navigation Menu

Repository navigation

Add search function within patch viewer - #2

Add search function within patch viewer#2
rosemarie-o wants to merge 4 commits into
igsilya:mainfrom
rosemarie-o:main

rosemarie-o commented Jul 15, 2026 •

edited

Loading

Uh oh!

dceara left a comment

Uh oh!

dceara Aug 7, 2026

Uh oh!

dceara Aug 7, 2026

Uh oh!

dceara Aug 7, 2026

Uh oh!

dceara Aug 7, 2026 •

edited

Loading

Uh oh!

igsilya commented Aug 7, 2026

Uh oh!

igsilya left a comment

Uh oh!

rosemarie-o commented Oct 13, 2026

Uh oh!

rosemarie-o commented Oct 19, 2026

Uh oh!

rosemarie-o commented Oct 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

rosemarie-o commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dceara left a comment

Choose a reason for hiding this comment

Uh oh!

dceara Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

dceara Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

dceara Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

dceara Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igsilya commented Aug 7, 2026

Uh oh!

igsilya left a comment

Choose a reason for hiding this comment

Uh oh!

rosemarie-o commented Oct 13, 2026

Uh oh!

rosemarie-o commented Oct 19, 2026

Uh oh!

rosemarie-o commented Oct 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rosemarie-o commented Jul 15, 2026 •

edited

Loading

dceara Aug 7, 2026 •

edited

Loading