fix(ui): prevent scroll jump during pagination#1645
fix(ui): prevent scroll jump during pagination#1645marlonwq wants to merge 8 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces router-based URL updates with direct History API calls (window.history.replaceState) and debounced URL updates that are cancelled on unmount. Page query param is set only when page > 1. Introduces client-side displayResults and visibleResults to drive rendering and total/effectiveTotal logic. Tightens focusable-element selection to visible elements and updates focus/keyboard handling. Many conditional renderings shifted to rely on displayResults length (several v-if changed to v-show). Adds a search-page class, a results-layout wrapper and scoped styling. No public API signatures were changed. Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
marlonwq
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/pages/search.vue (3)
24-34: Nuxt route object will be stale afterreplaceState.Since
window.history.replaceStatebypasses the Nuxt router,route.query.pagewill not reflect the updated page number during the session. This is currently safe becauseinitialPage(line 53–56) is only consumed insideonMountedfor reload/restore. However, if any future code reactively readsroute.query.pagemid-session, it will get the stale value.Consider adding a short comment near the
initialPagecomputed (or here) noting this intentional divergence, so future contributors don't inadvertently rely onroute.query.pagefor the live page number.
553-556: Redundant condition:displayResults.length === 0is already insideshowSearching.The
showSearchingcomputed (line 265) already gates ondisplayResults.value.length === 0. Repeating the check in the template is harmless but adds noise and could drift ifshowSearchingis later updated.Proposed simplification
<LoadingSpinner - v-if="showSearching && displayResults.length === 0" + v-if="showSearching" :text="$t('search.searching')" />
747-755:min-height: 100vhmay produce excessive whitespace with few results.When only a handful of results are returned (e.g. a highly specific query with 2–3 matches),
.results-layoutwill still occupy at least the full viewport, leaving a noticeable empty gap below the content. Consider tying the minimum height to loading state or using a smaller fallback (e.g.min-height: 50vh) so that the layout reservation is enough to prevent collapse without creating a visual artefact.Also,
overflow-anchor: noneon the entire.search-pagedisables all browser scroll-anchoring, not just during pagination. If the page later gains above-the-fold dynamic content (e.g. banners), the user could experience unexpected scroll shifts. A narrower scope (e.g. on.results-layoutonly) would limit the blast radius.Proposed refinement
.search-page { - overflow-anchor: none; } .results-layout { - min-height: 100vh; + min-height: 50vh; + overflow-anchor: none; }
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/pages/search.vue (2)
449-452:⚠️ Potential issue | 🟡 MinorRemove debug
console.logbefore merging.This log fires on every
displayResultsupdate whenpendingEnterQueryis set, producing noise in the browser console for all end users.🐛 Proposed fix
- // eslint-disable-next-line no-console - console.log('[search] watcher fired', { - pending: pendingEnterQuery.value, - firstResult: firstResult?.package.name, - })
597-603:⚠️ Potential issue | 🟡 MinorRemove
focus-visible:outline-accent/70from both claim<button>elements.The project applies
button:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }globally inapp/assets/main.css. Adding per-elementfocus-visible:outline-accent/70on<button>tags is redundant and deviates from the project convention.♻️ Proposed fix (applies to both buttons)
- class="shrink-0 px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md motion-safe:transition-colors motion-safe:duration-200 hover:bg-fg/90 focus-visible:outline-accent/70" + class="shrink-0 px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md motion-safe:transition-colors motion-safe:duration-200 hover:bg-fg/90"- class="px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md transition-colors duration-200 hover:bg-fg/90 focus-visible:outline-accent/70" + class="px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md transition-colors duration-200 hover:bg-fg/90"Based on learnings: "In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css… individual buttons or selects in Vue components should not rely on inline focus-visible utility classes like
focus-visible:outline-accent/70."Also applies to: 703-710
🧹 Nitpick comments (1)
app/pages/search.vue (1)
40-62: Consider consolidating bothonMountedhooks into one.Two separate
onMountedcalls for closely related initialisation logic (page state + interaction flag) can be merged to reduce cognitive overhead.♻️ Proposed consolidation
-onMounted(() => { - // Small delay to let view transition complete - setTimeout(() => { - hasInteracted.value = true - }, 300) -}) - // Initialize current page from URL on mount onMounted(() => { + // Small delay to let view transition complete + setTimeout(() => { + hasInteracted.value = true + }, 300) + if (initialPage.value > 1) { currentPage.value = initialPage.value } })
package.json
Outdated
| "packageManager": "pnpm@10.30.1", | ||
| "description": "> A fast, modern browser for the npm registry.", | ||
| "main": "index.js", | ||
| "directories": { | ||
| "doc": "docs", | ||
| "test": "test" | ||
| }, | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "git+https://github.com/marlonwq/npmx.dev.git" | ||
| }, | ||
| "bugs": { | ||
| "url": "https://github.com/marlonwq/npmx.dev/issues" | ||
| }, | ||
| "homepage": "https://github.com/marlonwq/npmx.dev#readme" |
There was a problem hiding this comment.
These metadata additions are unrelated to the PR objective and appear to be npm init artefacts.
The PR is scoped to fixing a scroll-jump regression during pagination. All changes from line 167 onward (description, main, directories, repository, bugs, homepage) were not part of the stated fix and look like side-effects of running npm init on the fork. Please remove or split them into a separate, intentional PR.
package.json
Outdated
| }, | ||
| "packageManager": "pnpm@10.30.1" | ||
| "packageManager": "pnpm@10.30.1", | ||
| "description": "> A fast, modern browser for the npm registry.", |
There was a problem hiding this comment.
Strip the Markdown blockquote prefix from description.
The value "> A fast, modern browser for the npm registry." contains a literal > (Markdown blockquote syntax). It will appear verbatim in registry UIs, IDE tooltips, and any tooling that reads this field.
🐛 Proposed fix
- "description": "> A fast, modern browser for the npm registry.",
+ "description": "A fast, modern browser for the npm registry.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "description": "> A fast, modern browser for the npm registry.", | |
| "description": "A fast, modern browser for the npm registry.", |
package.json
Outdated
| "main": "index.js", | ||
| "directories": { | ||
| "doc": "docs", | ||
| "test": "test" | ||
| }, |
There was a problem hiding this comment.
main and directories are not applicable to a private Nuxt application.
"main": "index.js" implies a Node.js library entry point; no such file exists in a Nuxt SSR project and the package is already marked "private": true. directories is a legacy npm artefact. Both fields are likely unintentional npm init output and should be removed.
🐛 Proposed fix
- "main": "index.js",
- "directories": {
- "doc": "docs",
- "test": "test"
- },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "main": "index.js", | |
| "directories": { | |
| "doc": "docs", | |
| "test": "test" | |
| }, |
package.json
Outdated
| "repository": { | ||
| "type": "git", | ||
| "url": "git+https://github.com/marlonwq/npmx.dev.git" | ||
| }, | ||
| "bugs": { | ||
| "url": "https://github.com/marlonwq/npmx.dev/issues" | ||
| }, | ||
| "homepage": "https://github.com/marlonwq/npmx.dev#readme" |
There was a problem hiding this comment.
Fork-scoped URLs must not be merged into the upstream repository.
repository.url, bugs.url, and homepage all point to marlonwq/npmx.dev — the contributor's personal fork — rather than the canonical upstream npmx-dev/npmx.dev. If merged, these would direct tooling, registry links, and issue trackers to the wrong repository.
🐛 Proposed fix
- "repository": {
- "type": "git",
- "url": "git+https://github.com/marlonwq/npmx.dev.git"
- },
- "bugs": {
- "url": "https://github.com/marlonwq/npmx.dev/issues"
- },
- "homepage": "https://github.com/marlonwq/npmx.dev#readme"
+ "repository": {
+ "type": "git",
+ "url": "git+https://github.com/npmx-dev/npmx.dev.git"
+ },
+ "bugs": {
+ "url": "https://github.com/npmx-dev/npmx.dev/issues"
+ },
+ "homepage": "https://github.com/npmx-dev/npmx.dev#readme"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "repository": { | |
| "type": "git", | |
| "url": "git+https://github.com/marlonwq/npmx.dev.git" | |
| }, | |
| "bugs": { | |
| "url": "https://github.com/marlonwq/npmx.dev/issues" | |
| }, | |
| "homepage": "https://github.com/marlonwq/npmx.dev#readme" | |
| "repository": { | |
| "type": "git", | |
| "url": "git+https://github.com/npmx-dev/npmx.dev.git" | |
| }, | |
| "bugs": { | |
| "url": "https://github.com/npmx-dev/npmx.dev/issues" | |
| }, | |
| "homepage": "https://github.com/npmx-dev/npmx.dev#readme" |
1 participant
🔗 Linked issue
Resolves #1640
🧭 Context
Fixed the bug where scrolling jumps back to the top when the infinite pagination updates the URL.
📚 Description
router.replacewithwindow.history.replaceStateto update the page number without triggering Nuxt's default scroll-to-top behavior.currentPagefrom resetting if only the pagination changes, not the search text.v-showand CSS (overflow-anchor: none) to keep the results list in the DOM and prevent the page from collapsing while loading.