Skip to content

fix: added scroll preservation opt in to the compare page#2101

Merged
graphieros merged 5 commits intonpmx-dev:mainfrom
mihaizaurus:scroll-preservation-on-input-change
Mar 16, 2026
Merged

fix: added scroll preservation opt in to the compare page#2101
graphieros merged 5 commits intonpmx-dev:mainfrom
mihaizaurus:scroll-preservation-on-input-change

Conversation

@mihaizaurus
Copy link
Contributor

🔗 Linked issue

resolves #2098

🧭 Context

Fixes the compare page jumping back to the top when compare state is updated on the page.

On /compare, both package selection and facet selection sync state into the route query, being treated like normal navigations by the router scroll behavior, which caused the page to reset. This change makes scroll preservation opt-in at the page level and enables it for the compare page, so query-only updates keep the current scroll position.

Do note that this does not change normal page-to-page navigation scroll behavior, hash scrolling, or back/forward saved-position restoration.

I basically changed two files and added one test:

  • Added preserveScrollOnQuery: true to the compare page meta
  • Updated app/router.options.ts to return false from scrollBehavior for same-path, same-hash navigations on pages that opt in
  • Added router-level unit coverage for:
    • saved position restoration
    • scroll preservation on compare-style query updates
    • hash anchor scrolling
    • normal navigation scrolling to top

I wanted to implement this in a clean way that fixes both affected compare controls with one router-level change:

  • package updates via packages=...
  • facet updates via facets=...

I wanted to avoid duplicating special-case URL update logic in multiple compare components/composables, and thus keep the behavior scoped to pages that explicitly opt in.

Screen.Recording.2026-03-16.at.13.55.35.mov

@vercel
Copy link

vercel bot commented Mar 16, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docs.npmx.dev Ready Ready Preview, Comment Mar 16, 2026 9:01pm
npmx.dev Ready Ready Preview, Comment Mar 16, 2026 9:01pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
npmx-lunaria Ignored Ignored Mar 16, 2026 9:01pm

Request Review

@codecov
Copy link

codecov bot commented Mar 16, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
app/router.options.ts 66.66% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 16, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f5daa096-2dfb-41f7-b2be-17b9076d9a84

📥 Commits

Reviewing files that changed from the base of the PR and between 78d8cce and 9907685.

📒 Files selected for processing (3)
  • app/router.options.ts
  • app/types/index.ts
  • test/unit/app/router.options.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/router.options.ts

📝 Walkthrough

Walkthrough

Adds opt-in scroll preservation for query-only navigations. A new PageMeta flag preserveScrollOnQuery?: boolean is declared and set to true on the compare page. The router's scrollBehavior was updated to return false (preserve viewport) when navigating to the same path and hash and preserveScrollOnQuery is true; existing saved-position, hash-anchor (with scrollMargin), and default top-left scrolling behaviours remain. Unit tests for routerOptions.scrollBehavior were added to cover these cases.

Possibly related PRs

Suggested reviewers

  • danielroe
  • graphieros
  • serhalp
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description accurately describes the changeset, explaining the problem, solution approach, and specific files modified.
Linked Issues check ✅ Passed The pull request fully addresses the linked issue #2098 by implementing scroll preservation for query-only updates on the compare page via router-level opt-in.
Out of Scope Changes check ✅ Passed All changes are directly related to the linked issue: page meta flag, router scroll behaviour logic, type definitions, and comprehensive test coverage for the feature.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
test/unit/app/router.options.spec.ts (1)

22-53: Add explicit query-delta coverage for opt-in boundaries.

Current tests cover the happy path, but they don’t explicitly assert that only opted-in same-path query updates preserve scroll. Add:

  1. a same-path/hash test with different query values and preserveScrollOnQuery: true (expect false), and
  2. the same setup without opt-in (expect { left: 0, top: 0 }).

As per coding guidelines, "Write unit tests for core functionality using vitest".


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8d6f7f28-413e-447b-95ff-f530354c64b4

📥 Commits

Reviewing files that changed from the base of the PR and between 07a9ea9 and 78d8cce.

📒 Files selected for processing (3)
  • app/pages/compare.vue
  • app/router.options.ts
  • test/unit/app/router.options.spec.ts

Copy link
Contributor

@graphieros graphieros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works like a charm
LGTM

@graphieros graphieros requested a review from serhalp March 16, 2026 16:30
@graphieros graphieros added the needs review This PR is waiting for a review from a maintainer label Mar 16, 2026
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: _ prefix is for unused vars

Suggested change
scrollBehavior(to, from, savedPosition) {

@graphieros graphieros added this pull request to the merge queue Mar 16, 2026
Merged via the queue into npmx-dev:main with commit 31db8f8 Mar 16, 2026
20 checks passed
@mihaizaurus mihaizaurus deleted the scroll-preservation-on-input-change branch March 16, 2026 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs review This PR is waiting for a review from a maintainer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Changing Input in Compare Package shouldn't reset scroll

3 participants