fix(file-annotator): fix upward autoscroll on search previous navigation#76
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the FileAnnotator search navigation autoscroll to work reliably in both directions by replacing scrollIntoView with explicit scrollBy math and making match lookup robust to special characters in IDs. Sequence diagram for updated FileAnnotator search autoscrollsequenceDiagram
participant FileAnnotator
participant Window
participant Container
participant Element
FileAnnotator->>Window: setTimeout
Window->>Container: querySelectorAll
Window->>Element: getAttribute
opt [no matching element or container]
Window-->>FileAnnotator: return
end
Window->>Container: getBoundingClientRect
Window->>Element: getBoundingClientRect
Window->>Container: scrollBy
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Instead of querying all
[data-search-match-id]elements and filtering in JS, you could useCSS.escape(activeSearchMatchId)with the attribute selector (e.g.document.querySelector(`[data-search-match-id="${CSS.escape(activeSearchMatchId)}"]`)) to keep the lookup O(1) while safely handling special characters. - The scroll offset calculation assumes no transforms or padding on the scroll container; if those can vary, consider using
element.offsetToprelative to the container’s scrollTop to compute the target position more robustly within the scrolling context.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of querying all `[data-search-match-id]` elements and filtering in JS, you could use `CSS.escape(activeSearchMatchId)` with the attribute selector (e.g. ``document.querySelector(`[data-search-match-id="${CSS.escape(activeSearchMatchId)}"]`)``) to keep the lookup O(1) while safely handling special characters.
- The scroll offset calculation assumes no transforms or padding on the scroll container; if those can vary, consider using `element.offsetTop` relative to the container’s scrollTop to compute the target position more robustly within the scrolling context.
## Individual Comments
### Comment 1
<location path="src/renderer/src/components/file-annotator/index.tsx" line_range="187-188" />
<code_context>
+ containerRect.height / 2 +
+ elementRect.height / 2;
+
+ container.scrollBy({
+ top: scrollOffset,
behavior: "smooth",
- block: "center",
</code_context>
<issue_to_address>
**question (bug_risk):** Dropping horizontal scroll alignment may change behavior for horizontally scrollable content.
Previously, `scrollIntoView({ inline: "nearest" })` could adjust horizontal scroll for overflowed content. Now we only scroll vertically via `top`, so matches near the left/right edges may remain partially hidden in horizontally scrollable annotators. If horizontal overflow can occur here, consider adding horizontal alignment or confirm that this behavior change is acceptable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| container.scrollBy({ | ||
| top: scrollOffset, |
There was a problem hiding this comment.
question (bug_risk): Dropping horizontal scroll alignment may change behavior for horizontally scrollable content.
Previously, scrollIntoView({ inline: "nearest" }) could adjust horizontal scroll for overflowed content. Now we only scroll vertically via top, so matches near the left/right edges may remain partially hidden in horizontally scrollable annotators. If horizontal overflow can occur here, consider adding horizontal alignment or confirm that this behavior change is acceptable.
There was a problem hiding this comment.
Pull request overview
This PR fixes auto-scrolling in the file annotator search navigation so that moving to the previous match reliably scrolls upward, and match lookup no longer breaks when match IDs contain CSS-special characters.
Changes:
- Replaced
element.scrollIntoView()with an explicit scroll offset calculation andcontainer.scrollBy()to reliably scroll both up and down in Electron/Chromium. - Replaced CSS attribute selector lookup with a DOM attribute comparison (
querySelectorAll+getAttribute) to avoidSyntaxErrorwhen IDs contain characters that invalidate CSS selectors.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Problema
Al buscar una palabra y navegar entre las ocurrencias, el botón de "subir" no hacía nada, solo el botón de "bajar" disparaba el autoscroll hacia la ocurrencia correspondiente.
La causa raíz tenía dos partes:
element.scrollIntoView(), que puede fallar silenciosamente al desplazarse hacia arriba dentro de un contenedoroverflow-y: scrollen el renderer de Electron/Chromium.paragraph.idusa el texto completo del párrafo como identificador, por lo quequerySelector('[data-search-match-id="..."]')lanzaba unSyntaxErrorcuando el texto contenía caracteres especiales de CSS (",/,,, etc.).Solución
scrollIntoViewpor un cálculo explícito concontainer.scrollBy()usandofileRef, garantizando un scroll confiable en ambas direcciones.querySelectorAll+getAttribute), haciendo la búsqueda robusta sin importar los caracteres especiales en el ID del match.Issue relacionado
Issue #69
Summary by Sourcery
Ensure the file annotator reliably auto-scrolls to the active search match when navigating between occurrences.
Bug Fixes: