-
-
Notifications
You must be signed in to change notification settings - Fork 238
chore: prerender /compare page
#1335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
chore: prerender /compare page
#1335
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:
📝 WalkthroughWalkthroughAdds 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
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
uhmmm, what's happening here 🤔 ? |
|
Trying to patch nuxt html validator, No idea why css classes being reported as duplicated when checking |
There was a problem hiding this 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 betweenenabledandignore. This creates maintenance burden and risk of the conditions diverging. Consider extracting to a descriptive variable.Also, could you clarify why
/compareneeds 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, },
There was a problem hiding this 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this for? 👀
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this 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
| 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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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' |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
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 |
# Conflicts: # nuxt.config.ts
There was a problem hiding this 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
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
routeRules (prerender)/compare page
comparepage comparing client only, and so this PR adding it withprerender: true: