fix: refine compare grid header layout#2072
fix: refine compare grid header layout#2072mihaizaurus wants to merge 1 commit intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe ComparisonGrid component was refactored to use a single CSS variable ( Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/nuxt/components/compare/ComparisonGrid.spec.ts (1)
63-103: Consider adding a direct no-overflow assertion for the 4-column case.Current checks confirm
--package-count, but they do not directly guard the issue goal (preventing horizontal scroll). A browser-layout assertion (scrollWidth <= clientWidth) would make this regression-proof.Possible test addition
+ it('does not horizontally overflow with four package columns', async () => { + const component = await mountSuspended(ComparisonGrid, { + props: { + columns: cols('very-long-package-name-a@1.0.0', 'very-long-package-name-b@1.0.0', 'very-long-package-name-c@1.0.0', 'very-long-package-name-d@1.0.0'), + }, + }) + + const gridEl = component.find('.comparison-grid').element as HTMLElement + expect(gridEl.scrollWidth).toBeLessThanOrEqual(gridEl.clientWidth) + })Based on learnings: in
test/nuxt/, tests run in Vitest browser mode with Playwright, soscrollWidth/clientWidthassertions are reliable for layout checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b691b81d-3839-46ba-ac26-59668f1bf8df
📒 Files selected for processing (2)
app/components/Compare/ComparisonGrid.vuetest/nuxt/components/compare/ComparisonGrid.spec.ts
| v-for="col in columns" | ||
| :key="col.name" | ||
| class="comparison-cell comparison-cell-header min-w-0" |
There was a problem hiding this comment.
Use a unique v-for key for versioned comparisons.
:key="col.name" can collide when two columns share the same package name but differ by version, causing unstable DOM reuse on updates.
Suggested fix
- :key="col.name"
+ :key="`${col.name}@${col.version ?? ''}`"📝 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.
| v-for="col in columns" | |
| :key="col.name" | |
| class="comparison-cell comparison-cell-header min-w-0" | |
| v-for="col in columns" | |
| :key="`${col.name}@${col.version ?? ''}`" | |
| class="comparison-cell comparison-cell-header min-w-0" |
There was a problem hiding this comment.
My understanding is that the PackageSelector.vue already prevents duplicate package names before it gets to the Comparison Grid.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find PackageSelector.vue
fd -t f 'PackageSelector.vue'
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for duplicate prevention logic in PackageSelector.vue
rg -n "duplicate|already|exist|includes|has\b|filter|unique|Set" app/components/Compare/PackageSelector.vue -A2 -B2
Length of output: 2175
@mihaizaurus, you're correct. Looking at app/components/Compare/PackageSelector.vue, there are two independent guards that make duplicate names impossible:
filteredResultscomputed — already-selected package names are filtered out of the typeahead dropdown before they can be chosen again (Line 59:.filter(r => !packages.value.includes(r.name))).addPackageguard — an explicit early-return (Line 82:if (packages.value.includes(name)) return) prevents adding a name that's already present.
Since columns can never contain two entries with the same col.name, :key="col.name" is safe and my suggestion was unwarranted here. Apologies for the noise!
✏️ Learnings added
Learnt from: mihaizaurus
Repo: npmx-dev/npmx.dev PR: 2072
File: app/components/Compare/ComparisonGrid.vue:38-40
Timestamp: 2026-03-14T08:57:22.084Z
Learning: In `app/components/Compare/ComparisonGrid.vue`, using `:key="col.name"` in the `v-for` over columns is safe and correct. `app/components/Compare/PackageSelector.vue` enforces two independent deduplication guards that make it impossible for duplicate package names to reach the grid: (1) `filteredResults` filters out already-selected names from the typeahead, and (2) `addPackage` has an explicit `if (packages.value.includes(name)) return` guard. Do not flag `:key="col.name"` as a potential collision risk.
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: graphieros
Repo: npmx-dev/npmx.dev PR: 2032
File: app/components/Package/TrendsChart.vue:1356-1361
Timestamp: 2026-03-13T12:43:11.121Z
Learning: In `app/components/Package/TrendsChart.vue`, the asymmetry between `chartHeight` and the wrapper height class is intentional. `chartHeight` only reduces (from 600 to 494) when `showCorrectionControls.value && props.inModal` is true, but the wrapper height class (Lines ~1857-1863) compacts from `h-[567px]` to `h-[491px]` whenever `showCorrectionControls` is true regardless of `props.inModal`. Do not flag this as a bug or suggest aligning both conditions to `shouldCompactChart`.
Learnt from: jellydeck
Repo: npmx-dev/npmx.dev PR: 904
File: app/components/Package/AccessControls.vue:253-253
Timestamp: 2026-02-04T05:34:20.527Z
Learning: In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css with the rule: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements in Vue templates or components (e.g., AccessControls.vue). Rely on the global rule for consistency and maintainability; only use inline focus-visible utilities when styling non-button/select elements or in exceptional cases outside the global scope.
Learnt from: jellydeck
Repo: npmx-dev/npmx.dev PR: 904
File: app/components/Package/Versions.vue:332-332
Timestamp: 2026-02-04T05:34:54.335Z
Learning: In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css with: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Therefore, individual buttons or selects in Vue components should not rely on inline focus-visible utility classes like focus-visible:outline-accent/70. Ensure components do not add per-element focus-visible utilities; rely on the global rule. If a component seems to require a different focus state, adjust only through global CSS or maintain accessibility with native focus styles, rather than inserting inline utility classes.
Learnt from: userquin
Repo: npmx-dev/npmx.dev PR: 1170
File: app/components/Package/MetricsBadges.vue:66-66
Timestamp: 2026-02-08T15:02:02.232Z
Learning: In Vue components that use UnoCSS with the preset-icons collection, prefer colon-syntax for icons (e.g., i-carbon:checkmark) over the dash-separated form (i-carbon-checkmark). This aids UnoCSS in resolving the correct collection directly, which improves performance for long icon names. Apply this pattern to all Vue components (e.g., app/components/**/*.vue) where UnoCSS icons are used; ensure UnoCSS is configured with the preset-icons collection.
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, ButtonBase (used via app/components/ButtonBase.vue or similar) provides default classes: border border-border. When styling ButtonBase instances in Vue components (e.g., app/components/Compare/FacetSelector.vue and other files under app/components), avoid duplicating the border class to prevent the HTML validator error no-dup-class and CI failures. If styling overrides are needed, ensure only unique classes are applied (remove redundant border classes or adjust via props) so the default border remains intact without duplication.
Learnt from: abbeyperini
Repo: npmx-dev/npmx.dev PR: 1049
File: app/components/Settings/Toggle.client.vue:22-29
Timestamp: 2026-02-11T00:01:33.121Z
Learning: In Vue 3.4 and later, you can use same-name shorthand for attribute bindings: use :attributeName instead of :attributeName="attributeName" when binding to a variable with the same name in scope. For example, :id is equivalent to :id="id" when an id variable exists. Apply this shorthand in .vue components (notably in Settings/Toggle.client.vue) to simplify templates. Ensure the bound variable exists and that you are using a Vue version that supports this shorthand.
Learnt from: alexdln
Repo: npmx-dev/npmx.dev PR: 1845
File: app/components/InstantSearch.vue:6-11
Timestamp: 2026-03-03T09:42:52.533Z
Learning: Maintain the established prehydration pattern across the project: use JSON.parse(localStorage.getItem('npmx-settings') || '{}') without per-call try-catch blocks. Do not introduce try-catch error handling for this pattern unless a coordinated, project-wide refactor of all onPrehydrate readers is planned and executed.
Learnt from: graphieros
Repo: npmx-dev/npmx.dev PR: 1974
File: app/components/Compare/FacetBarChart.vue:174-178
Timestamp: 2026-03-07T12:00:18.120Z
Learning: When reviewing Compare/FacetBarChart.vue (or other Vue components in app/components), treat config.datapoint as a runtime object that can include extra properties from the dataset item (e.g., formattedValue, custom metadata). The TypeScript typings for config.datapoint may be incomplete, but that should not be flagged as a runtime error. Base reviews on actual runtime behavior and data presence, not on missing type checks; focus on correct usage and data-driven effects.
mihaizaurus
force-pushed
the
compare-grid-header-layout
branch
from
4171f59 to
e784d3a
Compare
mihaizaurus
changed the title
mihaizaurus
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/components/Compare/ComparisonGrid.vue (1)
37-40:⚠️ Potential issue | 🟡 MinorUse a version-aware
v-forkey.Line 39 uses
:key="col.name", which can collide when the same package is compared at different versions, causing unstable node reuse.Proposed fix
- :key="col.name" + :key="`${col.name}@${col.version ?? ''}`"
🧹 Nitpick comments (1)
test/nuxt/components/compare/ComparisonGrid.spec.ts (1)
95-103: Remove the duplicate--package-counttest.This case overlaps with the earlier
--package-countassertions and doesn’t add new coverage.Suggested simplification
- it('sets --package-count CSS variable', async () => { - const component = await mountSuspended(ComparisonGrid, { - props: { - columns: cols('a', 'b', 'c'), - }, - }) - const grid = component.find('.comparison-grid') - expect(grid.attributes('style')).toContain('--package-count: 3') - })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47d1a78a-9dc4-4622-9a70-ce322356192b
📒 Files selected for processing (3)
app/components/Compare/ComparisonGrid.vuetest/e2e/interactions.spec.tstest/nuxt/components/compare/ComparisonGrid.spec.ts
| <LinkBase | ||
| :to="packageRoute(col.name, col.version)" | ||
| class="text-sm truncate" | ||
| block | ||
| class="flex min-w-0 flex-col items-center text-center text-sm" | ||
| :title="col.version ? `${col.name}@${col.version}` : col.name" | ||
| > |
There was a problem hiding this comment.
title may not be forwarded to the rendered anchor.
Line 46 passes :title to LinkBase, but app/components/Link/Base.vue currently forwards only declared props via v-bind="props" and does not declare title. That can drop the attribute and break this PR’s tooltip/title contract.
Proposed fix (in app/components/Link/Base.vue)
const props = withDefaults(
defineProps<{
+ /** Native title attribute forwarded to the underlying link element */
+ title?: string
/** Disabled links will be displayed as plain text */
disabled?: boolean
...
}>(),
{ variant: 'link', size: 'medium' },
)There was a problem hiding this comment.
Searching for the LinkBase in other components, I notice the same pattern in a few packages like Package/Header.vue, Package/Dependencies.vue and the page for [[org]]/[name].vue. If the risk is really Major, it would impact those areas as well and would indeed need to be strengthened.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the full LinkBase component
fd -e vue "Base.vue" --search-path app/components/Link/ | xargs cat
# Also check for inheritAttrs usage
rg -n "inheritAttrs" app/components/Link/
# Check title attribute usage in the mentioned files
rg -n ":title" app/components/Package/Header.vue app/components/Package/Dependencies.vue
rg -n ":title" app/pages/ --type vue
Length of output: 5751
@mihaizaurus, you're right to connect the dots — the concern is real and does affect all those locations.
Here's why: app/components/Link/Base.vue renders a v-if/v-else pair at the template root (a <span> when disabled, a <NuxtLink> otherwise). Vue 3 treats this as a fragment, and fragments do not support automatic fallthrough attribute inheritance — Vue will silently drop title (or emit a runtime warning in dev mode). Additionally, the <NuxtLink> uses v-bind="props", which only forwards explicitly declared props, and title is not one of them.
So every call site that passes :title to LinkBase — whether in ComparisonGrid.vue, Package/Header.vue, Package/Dependencies.vue, or [[org]]/[name].vue — currently has the attribute silently dropped.
The cleanest fix is to declare title as a prop and forward it explicitly on both branches:
// app/components/Link/Base.vue
const props = withDefaults(
defineProps<{
+ /** Native title attribute (tooltip) forwarded to the underlying element */
+ title?: string
/** Disabled links will be displayed as plain text */
disabled?: boolean
...
}>(),
{ variant: 'link', size: 'medium' },
)And in the template, add :title="title" to both the <span> (disabled branch) and ensure it flows through v-bind="props" to <NuxtLink> (it will automatically once declared as a prop):
- <span v-if="disabled" ...>
+ <span v-if="disabled" :title="title" ...>The <NuxtLink> branch already uses v-bind="props", so declaring title as a prop is sufficient for it — no additional change needed there.
Would you like me to open a follow-up issue to track fixing LinkBase globally, or would you prefer to address it in this PR since it directly affects the contract this PR relies on?
✏️ Learnings added
Learnt from: mihaizaurus
Repo: npmx-dev/npmx.dev PR: 2072
File: app/components/Compare/ComparisonGrid.vue:43-47
Timestamp: 2026-03-14T09:05:41.326Z
Learning: In `app/components/Link/Base.vue` (npmx.dev project), the component renders a `v-if`/`v-else` fragment (disabled `<span>` vs `<NuxtLink>`), which means Vue 3 does NOT automatically apply fallthrough attributes. `v-bind="props"` on `<NuxtLink>` only forwards declared props. Therefore, any HTML attribute like `title` passed to `LinkBase` must be explicitly declared as a prop in `defineProps` and manually forwarded to both template branches (`:title="title"` on the `<span>`, and automatically via `v-bind="props"` on `<NuxtLink>` once declared). This affects all usages across the codebase including `Package/Header.vue`, `Package/Dependencies.vue`, and `Compare/ComparisonGrid.vue`.
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: mihaizaurus
Repo: npmx-dev/npmx.dev PR: 2072
File: app/components/Compare/ComparisonGrid.vue:38-40
Timestamp: 2026-03-14T08:57:22.084Z
Learning: In Vue components, when using v-for, provide a stable unique key (e.g., :key="col.name"). This learning notes that in app/components/Compare/ComparisonGrid.vue, using :key="col.name" is safe because duplicates are prevented upstream: app/components/Compare/PackageSelector.vue enforces two guards (filteredResults excludes already-selected names; addPackage early-returns if the name already exists). Do not flag :key="col.name" as a collision risk in this context. If applying this guideline elsewhere, ensure a unique key is guaranteed by logic or consider fallbacks (e.g., index or composite keys) only if duplicates could be introduced.
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: 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.
Learnt from: graphieros
Repo: npmx-dev/npmx.dev PR: 2032
File: app/components/Package/TrendsChart.vue:1356-1361
Timestamp: 2026-03-13T12:43:11.121Z
Learning: In `app/components/Package/TrendsChart.vue`, the asymmetry between `chartHeight` and the wrapper height class is intentional. `chartHeight` only reduces (from 600 to 494) when `showCorrectionControls.value && props.inModal` is true, but the wrapper height class (Lines ~1857-1863) compacts from `h-[567px]` to `h-[491px]` whenever `showCorrectionControls` is true regardless of `props.inModal`. Do not flag this as a bug or suggest aligning both conditions to `shouldCompactChart`.
Learnt from: jellydeck
Repo: npmx-dev/npmx.dev PR: 904
File: app/components/Package/AccessControls.vue:253-253
Timestamp: 2026-02-04T05:34:20.527Z
Learning: In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css with the rule: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements in Vue templates or components (e.g., AccessControls.vue). Rely on the global rule for consistency and maintainability; only use inline focus-visible utilities when styling non-button/select elements or in exceptional cases outside the global scope.
Learnt from: jellydeck
Repo: npmx-dev/npmx.dev PR: 904
File: app/components/Package/Versions.vue:332-332
Timestamp: 2026-02-04T05:34:54.335Z
Learning: In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css with: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Therefore, individual buttons or selects in Vue components should not rely on inline focus-visible utility classes like focus-visible:outline-accent/70. Ensure components do not add per-element focus-visible utilities; rely on the global rule. If a component seems to require a different focus state, adjust only through global CSS or maintain accessibility with native focus styles, rather than inserting inline utility classes.
Learnt from: userquin
Repo: npmx-dev/npmx.dev PR: 1170
File: app/components/Package/MetricsBadges.vue:66-66
Timestamp: 2026-02-08T15:02:02.232Z
Learning: In Vue components that use UnoCSS with the preset-icons collection, prefer colon-syntax for icons (e.g., i-carbon:checkmark) over the dash-separated form (i-carbon-checkmark). This aids UnoCSS in resolving the correct collection directly, which improves performance for long icon names. Apply this pattern to all Vue components (e.g., app/components/**/*.vue) where UnoCSS icons are used; ensure UnoCSS is configured with the preset-icons collection.
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, ButtonBase (used via app/components/ButtonBase.vue or similar) provides default classes: border border-border. When styling ButtonBase instances in Vue components (e.g., app/components/Compare/FacetSelector.vue and other files under app/components), avoid duplicating the border class to prevent the HTML validator error no-dup-class and CI failures. If styling overrides are needed, ensure only unique classes are applied (remove redundant border classes or adjust via props) so the default border remains intact without duplication.
Learnt from: abbeyperini
Repo: npmx-dev/npmx.dev PR: 1049
File: app/components/Settings/Toggle.client.vue:22-29
Timestamp: 2026-02-11T00:01:33.121Z
Learning: In Vue 3.4 and later, you can use same-name shorthand for attribute bindings: use :attributeName instead of :attributeName="attributeName" when binding to a variable with the same name in scope. For example, :id is equivalent to :id="id" when an id variable exists. Apply this shorthand in .vue components (notably in Settings/Toggle.client.vue) to simplify templates. Ensure the bound variable exists and that you are using a Vue version that supports this shorthand.
Learnt from: alexdln
Repo: npmx-dev/npmx.dev PR: 1845
File: app/components/InstantSearch.vue:6-11
Timestamp: 2026-03-03T09:42:52.533Z
Learning: Maintain the established prehydration pattern across the project: use JSON.parse(localStorage.getItem('npmx-settings') || '{}') without per-call try-catch blocks. Do not introduce try-catch error handling for this pattern unless a coordinated, project-wide refactor of all onPrehydrate readers is planned and executed.
Learnt from: graphieros
Repo: npmx-dev/npmx.dev PR: 1974
File: app/components/Compare/FacetBarChart.vue:174-178
Timestamp: 2026-03-07T12:00:18.120Z
Learning: When reviewing Compare/FacetBarChart.vue (or other Vue components in app/components), treat config.datapoint as a runtime object that can include extra properties from the dataset item (e.g., formattedValue, custom metadata). The TypeScript typings for config.datapoint may be incomplete, but that should not be flagged as a runtime error. Base reviews on actual runtime behavior and data presence, not on missing type checks; focus on correct usage and data-driven effects.
|
Thank you @mihaizaurus :) I think this is a clear improvement vs the current layout when comparing 4 packages. There have been talks about potentially increasing the max number of compared packages, which would probably require other adaptations for the table (sticky first col, etc), but this is probably for a later iteration. You might want to check / address the coderabbit's remarks. |
|
I left comments on both, it also responded with the first minor issue not being required, the second one requiring a bigger discussion. I can tackle that change as well but it felt a bit beyond the scope of this specific issue as it would impact other components that use the Link |
2 participants
🔗 Linked issue
Resolves #1627
🧭 Context
This change updates the compare grid header layout so long package names and versions behave more predictably in 2/3/4-package comparisons, ensuring coherent visuals across all columns.
📚 Description
With this change I have:
columns-2/3/4layout classes with a single generic grid definition using--package-countname@versionin the linktitleattributemin-w-0to the header cell chain so long labels can shrink instead of distorting the layout