fix(search): don't re-center visible matches on find navigation (SD-3315)#3572
Conversation
…315)
Search next/previous re-centered the active match on every move via
scrollToPosition({ block: 'center' }), jumping the document ~50px even when
the match was already on screen. Add an opt-in `ifNeeded` mode that downgrades
to 'nearest' (a no-op) when the match is fully visible, keeping center for
off-screen matches. goToFirstMatch still centers the initial result.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaa582479a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
No issues found across 4 files
Linked issue analysis
Linked issue: SD-3315: Bug: Find & replace scrolls the page ~50px on every match navigation, even when the match is already visible
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Per-match (next/previous) navigation does not re-center an already-fully-visible match (no ~50px jump). | PresentationEditor.scrollToPosition gained an ifNeeded mode that downgrades to 'nearest' when the precise target element is fully visible, and search.next/previous calls opt into ifNeeded. Tests verify the 'nearest' call for a fully-visible match. |
| ✅ | Off-screen or partially-clipped matches still center (preserve expected centering when needed). | The implementation only downgrades to 'nearest' for a fully-visible target; off-screen or partially-clipped elements keep the requested block (center). Tests cover both off-screen and partially-clipped cases and expect centering. |
| ✅ | Initial/first-match navigation still recenters (first-match jump unchanged). | The change is explicitly limited to per-match navigation—first-match logic was not modified and remains centered. The PR description and code changes show only next/previous opting into ifNeeded. |
| ✅ | The fix does not reintroduce the hidden-editor selection-sync jump: scroll path retains the RAF re-assert (no early-return). | The implementation intentionally downgrades to 'nearest' rather than early-returning so the existing scrollIntoView + RAF re-assert behavior remains in place and continues to defend against the hidden editor's selection-sync scroll. |
Tip: cubic could auto-approve low-risk PRs like this, if it thinks it's safe to merge. Learn more
Re-trigger cubic
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…315) The ifNeeded scroll removed the per-match re-center, but exposed a second scroll: navigating via the find UI restores focus to the find input, which fires a selectionUpdate that reverts the editor selection to its pre-search caret; selection-sync then scrolled the viewport to that stale caret and the RAF re-assert yanked it back — a jump/flash on every navigation. Add an opt-in `suppressSelectionSyncScroll` to scrollToPosition: while a search-owned scroll is in flight (set before its scroll, cleared in its RAF re-assert), #scrollActiveEndIntoView is skipped so it can't fight the search scroll. The selection overlay still renders; normal keyboard/pointer selection scroll resumes the next frame. Active-match tracking and replace are storage-owned, so the selection revert does not affect them.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7ccb7b334
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
cubic analysis
1 issue found across 4 files
Linked issue analysis
Linked issue: SD-3315: Bug: Find & replace scrolls the page ~50px on every match navigation, even when the match is already visible
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Do not re-center an already-visible match when navigating next/previous (no ~50px jump) | scrollToPosition gained an ifNeeded mode that downgrades to 'nearest' for fully-visible match elements; search navigation passes ifNeeded so already-visible matches are not re-centered. |
| ✅ | Keep centering behavior for off-screen matches and initial jump | ifNeeded only downgrades when the precise match element is fully visible; off-screen or partially-clipped matches still use the requested 'center' block. The PR also preserves plain centering for the initial jump. |
| Prevent selection-sync from fighting search navigation (avoid selection-scroll flash/jitter during find nav) | The PR implements a suppressSelectionSyncScroll option and a #suppressSelectionScrollUntilRaf flag which is set/cleared around the RAF re-assert and causes #scrollActiveEndIntoView to early-return while the search-owned scroll is in flight. This matches the intended suppression behavior, but the change is difficult to unit-test in the mock harness and the PR notes the runtime verification is instrumented rather than fully covered by unit tests / cross-browser bits need re-run, so a follow-up E2E regression is recommended. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…315) scrollToPositionAsync forwarded ifNeeded:true into its post-mount retry. A match that was off-screen at call time (hence the async/mount path) could end up edge-visible after the page scrolls into view, then downgrade to 'nearest' and skip the centering it should get. Force ifNeeded:false on the retry (keeping suppressSelectionSyncScroll); the fast path keeps ifNeeded for already-mounted matches. Flagged in review by Codex and cubic.
|
🎉 This PR is included in superdoc-cli v0.15.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.14.0 |
|
🎉 This PR is included in @superdoc-dev/mcp v0.10.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.38.0 The release is available on GitHub release |
|
🎉 This PR is included in @superdoc-dev/react v1.9.0 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.10.0 |
2 participants
Find navigation felt unstable: the page jumped on every next/previous. This fixes it in two layers, both in search navigation only.
1. Stop re-centering an already-visible match.
goToSearchResultscrolled withblock: 'center'every time, so the document jumped ~50px even when the match was already on screen.scrollToPositiongainsifNeeded: when the match element is already fully inside the scroll container it downgrades tonearest(a no-op); off-screen/clipped matches still center.goToFirstMatchkeeps plain centering for the initial jump.2. Stop the selection-sync scroll from fighting search nav. Fixing (1) exposed a second scroll: navigating via the find UI restores focus to the find input, which fires a selectionUpdate that reverts the editor selection to its pre-search caret; selection-sync then scrolled the viewport to that stale caret and the RAF re-assert yanked it back — a visible flash on every navigation.
scrollToPositiongainssuppressSelectionSyncScroll: while a search-owned scroll is in flight (set before its scroll, cleared in its RAF re-assert)#scrollActiveEndIntoViewis skipped, so it can't fight the search scroll. The selection overlay still renders; normal keyboard/pointer selection scroll resumes the next frame. Active-match tracking and Replace are storage-owned, so the selection revert doesn't affect them.Both options are opt-in and only set by search next/previous; other callers of
scrollToPosition(AI nav, PositionTracker, public navigateTo) are unchanged.Reported in #3500.
Review: focus on the two new options in
scrollToPosition/scrollToPositionAsync, the#scrollActiveEndIntoViewearly-return, and thatgoToFirstMatchstill centers. Lint warnings inPresentationEditor.tsare pre-existing.Verified: instrumented runtime trace through the real find UI — visible match = 0px scroll, off-screen = centered, 30 consecutive navigations with zero flash; full search + scroll unit suites (208) and
tsc -bpass. Cross-browser Playwright matrix was green on layer (1); needs a re-run for (2).Test note: the selection-sync suppression is verified at runtime; it is not cleanly unit-testable in the super-editor mock harness (the gate is a private, event-driven method and the jitter needs the superdoc find-UI focus dynamics). A find-UI E2E regression test is a recommended fast-follow.