fix: center active search match in CodeMirror viewport#7918
fix: center active search match in CodeMirror viewport#7918DevRajah wants to merge 1 commit intousebruno:mainfrom
Conversation
DevRajah
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
WalkthroughCodeMirrorSearch's scroll behavior is enhanced to center matching search results in the viewport instead of simply scrolling them into view, addressing issue ChangesCodeMirror Search Centering Algorithm
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bruno-app/src/components/CodeMirrorSearch/index.js`:
- Around line 159-174: The current centering uses
editor.heightAtLine(activeMatch.from.line, 'local') which ignores wrapped-line
offsets; replace that with editor.charCoords(activeMatch.from, 'local') and use
coords.top (the actual on-screen Y for the match) when computing targetScrollTop
so wrapped matches are centered correctly; update the code that defines matchTop
(or remove it) to call editor.charCoords(activeMatch.from, 'local') and use the
returned .top value in the existing centering calculation before calling
editor.scrollTo.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 19325b30-8e23-43ee-8571-d8624726d0f4
📒 Files selected for processing (1)
packages/bruno-app/src/components/CodeMirrorSearch/index.js
| const activeMatch = matches[matchIndex]; | ||
| const scroller = editor.getScrollerElement(); | ||
| const lineHeight = editor.defaultTextHeight(); | ||
|
|
||
| // CodeMirror's heightAtLine gives us the vertical position of the line | ||
| // inside the full editor document, not just the visible screen. | ||
| const matchTop = editor.heightAtLine(activeMatch.from.line, 'local'); | ||
|
|
||
| // We subtract half the viewport height, then add a few line heights so | ||
| // the highlighted line is visually centred instead of being too high. | ||
| const targetScrollTop = Math.max( | ||
| 0, | ||
| matchTop - scroller.clientHeight / 2 + lineHeight * 3 | ||
| ); | ||
| // my line of work ends here | ||
| editor.scrollTo(null, targetScrollTop); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In CodeMirror 5, what is the difference between heightAtLine(line, 'local')andcharCoords(pos, 'local'), and which API reflects the actual vertical position of a match inside a wrapped line?
💡 Result:
In CodeMirror 5, heightAtLine(line, "local") and charCoords(pos, "local") both accept "local" as the coordinate reference mode, but they answer different geometry questions: 1) heightAtLine(line, "local") - Returns the y position (height from the top of the editor viewport) of the top of the given logical line. - It’s line-level and based on CodeMirror’s internal measurement model for line heights (which can be approximated when the line has not actually been rendered due to viewport/lazy rendering) [1]. - For wrapped lines, this is about the logical line’s top (not a specific visual wrap “sub-line”), so it does not directly tell you where a particular wrapped segment starts/ends. 2) charCoords(pos, "local") - Returns the rectangle for a specific character position in the document, including top/bottom in the chosen coordinate system (here, "local") [2][3]. - Because it’s character-based, it reflects the actual layout of the visual line wrapping for that position (including which wrapped segment the character is on). Which API reflects the actual vertical position of a match inside a wrapped line? - Use charCoords(pos, "local") for match rendering/positioning. Since matches are character ranges (positions) and wrapping is visual/segment-based, the character rectangle returned by charCoords is the API that corresponds to the actual on-screen vertical placement of the match within wrapped lines [2][3]. Important nuance about "local" - There have been historical issues where charCoords(..., "local") could be inconsistent/off by padding (and related fixes landed) [4]. So if you’re comparing to other local measurements (or doing pixel-perfect positioning), you should validate against getScrollInfo()/wrapper padding in your exact setup—but the character-based nature of charCoords remains what makes it the correct choice for “vertical position of a match inside a wrapped line.” [4][2]
Citations:
- 1: heightAtLine or any line's top position which out of viewport might not accurate codemirror/codemirror5#4045
- 2: https://codemirror.net/docs/ref/
- 3: Update cursor position while dragging (or get cursor position on-demand) codemirror/codemirror5#4719
- 4: charCoords inconsistency codemirror/codemirror5#1557
Use charCoords() to reflect the actual position of wrapped matches.
heightAtLine() returns only the logical line's top, which doesn't account for the actual vertical position of a match within a wrapped line. charCoords() is character-position aware and reflects the true on-screen placement.
🔧 Suggested change
- const matchTop = editor.heightAtLine(activeMatch.from.line, 'local');
+ const matchTop = editor.charCoords(activeMatch.from, 'local').top;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-app/src/components/CodeMirrorSearch/index.js` around lines 159
- 174, The current centering uses editor.heightAtLine(activeMatch.from.line,
'local') which ignores wrapped-line offsets; replace that with
editor.charCoords(activeMatch.from, 'local') and use coords.top (the actual
on-screen Y for the match) when computing targetScrollTop so wrapped matches are
centered correctly; update the code that defines matchTop (or remove it) to call
editor.charCoords(activeMatch.from, 'local') and use the returned .top value in
the existing centering calculation before calling editor.scrollTo.
1 participant
Description
This PR improves the positioning of search results in the JSON response view.
Previously, when navigating search matches, the highlighted result was often positioned near the bottom of the viewport. This made it difficult to view surrounding JSON context and required additional manual scrolling.
What changed
Why this matters
This aligns with expected behavior seen in tools like VS Code, where search navigation keeps results within a comfortable viewing context.
Screenshot
Summary by CodeRabbit