Skip to content

Conversation

@userquin
Copy link
Contributor

@userquin userquin commented Feb 10, 2026

compare page comparing client only, and so this PR adding it with prerender: true:

  • added some client only to avoid hydration missmatch errors
  • added some skeletons
  • restore facets from url
  • added js required via useHead
  • tests changed

@vercel
Copy link

vercel bot commented Feb 10, 2026

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

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Feb 10, 2026 11:38pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 10, 2026 11:38pm
npmx-lunaria Ignored Ignored Feb 10, 2026 11:38pm

Request Review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 10, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a static prerender route for "/compare" and refactors compare-related UI and data readiness. FacetSelector styling is simplified; FacetRow uses SkeletonInline for missing values; usePackageComparison introduces a suspense-aware readiness flag and returns nulls while not ready. compare.vue now guards package access, truncates long SEO titles, and wraps multiple interactive sections in ClientOnly with skeleton fallbacks; CompareLineChart is rendered conditionally when data exists. No exported API signatures were changed.

Possibly related PRs

Suggested labels

front

Suggested reviewers

  • danielroe
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description is directly related to the changeset, explaining the addition of the compare page to routeRules with prerender: true.

✏️ 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

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

@codecov
Copy link

codecov bot commented Feb 10, 2026

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
app/composables/usePackageComparison.ts 83.33% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@userquin
Copy link
Contributor Author

uhmmm, what's happening here 🤔 ?

@userquin
Copy link
Contributor Author

userquin commented Feb 10, 2026

Trying to patch nuxt html validator, ignore option being ignored, check nuxt-modules/html-validator#768

No idea why css classes being reported as duplicated when checking /compare page (prerendered), I need to check what's the issue 🤔

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)
nuxt.config.ts (1)

223-228: Extract the duplicated condition to a variable for maintainability.

The condition !isCI || (provider !== 'vercel' && !!process.env.VALIDATE_HTML) is duplicated between enabled and ignore. This creates maintenance burden and risk of the conditions diverging. Consider extracting to a descriptive variable.

Also, could you clarify why /compare needs to be ignored from validation when prerendering is enabled? If there's a known validation issue with the compare page, a brief comment would help future maintainers understand the intent.

♻️ Proposed refactor
+const shouldValidateHtml = !isCI || (provider !== 'vercel' && !!process.env.VALIDATE_HTML)
+
 export default defineNuxtConfig({
   // ... other config
 
   htmlValidator: {
-    enabled: !isCI || (provider !== 'vercel' && !!process.env.VALIDATE_HTML),
-    ignore:
-      !isCI || (provider !== 'vercel' && !!process.env.VALIDATE_HTML) ? ['/compare'] : undefined,
+    enabled: shouldValidateHtml,
+    // TODO: Add comment explaining why /compare is excluded from validation
+    ignore: shouldValidateHtml ? ['/compare'] : undefined,
     failOnError: true,
   },

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: 1

nuxt.config.ts Outdated
htmlValidator: {
enabled: !isCI || (provider !== 'vercel' && !!process.env.VALIDATE_HTML),
ignore:
!isCI || (provider !== 'vercel' && !!process.env.VALIDATE_HTML) ? ['/compare'] : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this for? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will be removed on next commit; I was trying to ignore that page, but it is a problem at CompareFacetSelector component (BaseButton now has some styles duplicated at CompareFacetSelector )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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: 1

Comment on lines +72 to 78
class="gap-1 px-1.5 rounded"
:class="
facet.comingSoon
? 'text-fg-subtle/50 bg-bg-subtle border-border-subtle cursor-not-allowed'
: isFacetSelected(facet.id)
? 'text-fg-muted bg-bg-muted border-border'
? 'text-fg-muted bg-bg-muted'
: 'text-fg-subtle bg-bg-subtle border-border-subtle hover:text-fg-muted hover:border-border'
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 10, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Avoid a potential size shift between selected and unselected buttons.

Line 77 removes the border from the selected state while the unselected state still uses border colour classes; if ButtonBase doesn’t enforce a fixed border width, toggling could cause a subtle layout shift. Consider keeping a consistent border width (e.g., transparent border on selected).

Suggested tweak
-              ? 'text-fg-muted bg-bg-muted'
+              ? 'text-fg-muted bg-bg-muted border border-transparent'
📝 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.

Suggested change
class="gap-1 px-1.5 rounded"
:class="
facet.comingSoon
? 'text-fg-subtle/50 bg-bg-subtle border-border-subtle cursor-not-allowed'
: isFacetSelected(facet.id)
? 'text-fg-muted bg-bg-muted border-border'
? 'text-fg-muted bg-bg-muted'
: 'text-fg-subtle bg-bg-subtle border-border-subtle hover:text-fg-muted hover:border-border'
class="gap-1 px-1.5 rounded"
:class="
facet.comingSoon
? 'text-fg-subtle/50 bg-bg-subtle border-border-subtle cursor-not-allowed'
: isFacetSelected(facet.id)
? 'text-fg-muted bg-bg-muted border border-transparent'
: 'text-fg-subtle bg-bg-subtle border-border-subtle hover:text-fg-muted hover:border-border'

Copy link
Contributor Author

@userquin userquin Feb 10, 2026

Choose a reason for hiding this comment

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

BaseButton has border border-border: html validator error => error Class "border-border" duplicated no-dup-class, and so, CI will fail

Copy link
Contributor

Choose a reason for hiding this comment

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

@userquin, thank you for the clarification! You're absolutely right—if ButtonBase already provides border border-border by default, then my suggestion to add border border-transparent would indeed duplicate the border class and fail the HTML validator.

Given that ButtonBase enforces a consistent border width across all states, there's no layout shift concern. The selected state uses the default border color from ButtonBase, while the unselected state simply overrides it with border-border-subtle. Your current implementation is correct.

I apologize for not considering the ButtonBase component's default styles when making my initial suggestion.

🐰


✏️ Learnings added
Learnt from: userquin
Repo: npmx-dev/npmx.dev PR: 1335
File: app/components/Compare/FacetSelector.vue:72-78
Timestamp: 2026-02-10T15:47:33.467Z
Learning: In the npmx.dev project, the `ButtonBase` component (app/components/ButtonBase.vue or similar) provides default `border border-border` styles. When styling ButtonBase instances, avoid adding duplicate `border` classes as this triggers the HTML validator error `no-dup-class` and fails CI.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: skaldebane
Repo: npmx-dev/npmx.dev PR: 760
File: app/components/Compare/FacetCard.vue:134-136
Timestamp: 2026-02-06T07:14:04.242Z
Learning: In `app/components/Compare/FacetCard.vue`, the facet values use `dir="auto"` rather than `dir="ltr"` because the values are not always numbers/sizes and can contain mixed content types.

Learnt from: jellydeck
Repo: npmx-dev/npmx.dev PR: 904
File: app/components/Package/Versions.vue:332-332
Timestamp: 2026-02-04T05:35:00.078Z
Learning: In the npmx.dev project, focus-visible styling for buttons and select elements is applied globally via main.css using the rule: `button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }`. Therefore, individual buttons and selects don't need inline focus-visible utility classes like `focus-visible:outline-accent/70` added to them.

Learnt from: jellydeck
Repo: npmx-dev/npmx.dev PR: 904
File: app/components/Package/AccessControls.vue:253-253
Timestamp: 2026-02-04T05:34:26.711Z
Learning: In the npmx.dev project, focus-visible styling for button and select elements is applied globally via app/assets/main.css using `button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }`. Individual inline utility classes like `focus-visible:outline-accent/70` are not needed on these elements.

Learnt from: alexdln
Repo: npmx-dev/npmx.dev PR: 838
File: app/pages/package/[...package].vue:445-449
Timestamp: 2026-02-03T13:59:33.392Z
Learning: The copy button pattern in app/pages/package/[...package].vue may be made into a reusable component or pattern in the future, but currently it's acceptable to keep it inline with the CSS-only approach for smooth animations.

@danielroe
Copy link
Member

danielroe commented Feb 10, 2026

this might leads to hydration mismatch unless we only perform data fetching on client-side. this will also mean the compare page will not work without JS (an acceptable trade-off). looks like there's also a hydration mismatch on main to investigate - can you have a look? 🙏

this will also be true for the search page, btw, but we do want that one to work without JS

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

@github-actions
Copy link

github-actions bot commented Feb 10, 2026

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

File Note
lunaria/files/en-GB.json Localization changed, will be marked as complete. 🔄️
lunaria/files/en-US.json Source changed, localizations will be marked as outdated.
Warnings reference
Icon Description
🔄️ The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

@danielroe danielroe changed the title chore: add missing compare page at routeRules (prerender) chore: prerender /compare page Feb 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants