Conversation
Preview deployments |
There was a problem hiding this comment.
Pull request overview
This PR implements a feature to preserve the scroll position of a focused section when the "Show only" checkbox is toggled in the card search interface. When users check/uncheck "Show only", sections expand or collapse, which would normally cause the focused section to shift on screen. This feature maintains the visual position of the focused section during these DOM changes.
Changes:
- Added a new
ScrollAnchormodifier that monitors DOM mutations and adjusts scroll position to keep an anchor element in place - Integrated the modifier into the search-content component with dynamic anchor selection based on focused section
- Added comprehensive test coverage for the scroll preservation behavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/host/app/modifiers/scroll-anchor.ts | New modifier that tracks element positions and adjusts scroll to preserve anchor position during DOM mutations |
| packages/host/app/components/card-search/search-content.gts | Integrates ScrollAnchor modifier, adds data-section-sid attributes, and manages scrollAnchorSid state |
| packages/host/tests/integration/components/operator-mode-card-catalog-test.gts | Adds test to verify scroll position preservation when toggling "Show only" checkbox |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.#element = element; | ||
| this.#anchorSelector = anchorSelector ?? null; | ||
| this.#trackSelector = trackSelector ?? null; | ||
|
|
||
| if (!this.#observer) { | ||
| this.#scrollHandler = this.capturePositions.bind(this); | ||
| element.addEventListener('scroll', this.#scrollHandler, { | ||
| passive: true, | ||
| }); | ||
|
|
||
| this.#observer = new MutationObserver(this.handleMutations.bind(this)); | ||
| this.#observer.observe(element, { | ||
| childList: true, | ||
| subtree: true, | ||
| attributes: true, | ||
| attributeFilter: ['class'], | ||
| }); | ||
|
|
||
| this.capturePositions(); | ||
| } |
There was a problem hiding this comment.
When the anchorSelector or trackSelector arguments change on a subsequent modify() call, the positions are not immediately recaptured. The modifier only calls capturePositions() during initial setup (line 50). If these selectors change dynamically (which they do based on scrollAnchorSid in search-content.gts), the position map may not include the new anchor element until the next scroll event or mutation. Consider calling capturePositions() after updating the selectors if they have changed.
There was a problem hiding this comment.
I can’t say I understand this, does it seem like a problem?
| scrollContainer.scrollTop = 50; | ||
| scrollContainer.dispatchEvent(new Event('scroll')); | ||
| await settled(); |
There was a problem hiding this comment.
The test manually dispatches a scroll event and then immediately awaits settled(). However, the scroll event listener is registered with the passive option, meaning it won't block. The test might not give enough time for the capturePositions callback to execute before checking positions. Consider waiting for a microtask or animation frame after dispatching the scroll event, or verify that settled() properly waits for all pending microtasks.
| ); | ||
|
|
||
| // Uncheck: sections expand, position should still be preserved | ||
| scrollContainer.dispatchEvent(new Event('scroll')); |
There was a problem hiding this comment.
This scroll event dispatch appears unnecessary at this point in the test. The event is dispatched after unchecking the show-only checkbox but before capturing the "before" position for the second assertion. If the intent is to ensure positions are captured, this should happen before the first click at line 912, not here. Consider removing this line or clarifying its purpose with a comment.
| scrollContainer.dispatchEvent(new Event('scroll')); |
backspace
left a comment
backspace
left a comment
There was a problem hiding this comment.
I tried this in the preview deployment, works as expected. There‘s still some jumping when I’ve scrolled to an extreme and there aren’t that many realms collapsing, but that seems unavoidable to me.
| this.#element = element; | ||
| this.#anchorSelector = anchorSelector ?? null; | ||
| this.#trackSelector = trackSelector ?? null; | ||
|
|
||
| if (!this.#observer) { | ||
| this.#scrollHandler = this.capturePositions.bind(this); | ||
| element.addEventListener('scroll', this.#scrollHandler, { | ||
| passive: true, | ||
| }); | ||
|
|
||
| this.#observer = new MutationObserver(this.handleMutations.bind(this)); | ||
| this.#observer.observe(element, { | ||
| childList: true, | ||
| subtree: true, | ||
| attributes: true, | ||
| attributeFilter: ['class'], | ||
| }); | ||
|
|
||
| this.capturePositions(); | ||
| } |
There was a problem hiding this comment.
I can’t say I understand this, does it seem like a problem?
Screen.Recording.2026-02-26.at.15.45.56.mov