Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 37 additions & 24 deletions app/components/Compare/ComparisonGrid.vue
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const props = defineProps<{
/** Total column count including the optional no-dep column */
const totalColumns = computed(() => props.columns.length + (props.showNoDependency ? 1 : 0))
const visibleColumns = computed(() => Math.min(totalColumns.value, 4))
/** Compute plain-text tooltip for a replacement column */
function getReplacementTooltip(col: ComparisonGridColumn): string {
Expand All @@ -30,32 +31,43 @@ function getReplacementTooltip(col: ComparisonGridColumn): string {
<div class="overflow-x-auto">
<div
class="comparison-grid"
:class="[totalColumns === 4 ? 'min-w-[800px]' : 'min-w-[600px]', `columns-${totalColumns}`]"
:style="{ '--columns': totalColumns }"
:style="{
'--package-count': totalColumns,
'--visible-columns': visibleColumns,
}"
>
<!-- Header row -->
<div class="comparison-header">
<div class="comparison-label" />
<div class="comparison-label relative bg-bg" />

<!-- Package columns -->
<div v-for="col in columns" :key="col.name" class="comparison-cell comparison-cell-header">
<span class="inline-flex items-center gap-1.5 truncate">
<div
v-for="col in columns"
:key="col.name"
class="comparison-cell comparison-cell-header min-w-0"
>
<div class="flex items-start justify-center gap-1.5 min-w-0">
<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"
>
Comment on lines 50 to 54
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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' },
 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

🧩 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.

{{ col.name }}<template v-if="col.version">@{{ col.version }}</template>
<span class="min-w-0 break-words line-clamp-1">
{{ col.name }}
</span>
<span v-if="col.version" class="text-fg-muted line-clamp-1">
@{{ col.version }}
</span>
</LinkBase>

<TooltipApp v-if="col.replacement" :text="getReplacementTooltip(col)" position="bottom">
<span
class="i-lucide:lightbulb w-3.5 h-3.5 text-amber-500 shrink-0 cursor-help"
class="i-lucide:lightbulb mt-0.5 h-3.5 w-3.5 shrink-0 cursor-help text-amber-500"
role="img"
:aria-label="$t('package.replacement.title')"
/>
</TooltipApp>
</span>
</div>
</div>

<!-- "No dep" column (always last) -->
Expand Down Expand Up @@ -100,29 +112,30 @@ function getReplacementTooltip(col: ComparisonGridColumn): string {

<style scoped>
.comparison-grid {
--label-column-width: 140px;
--package-column-width: calc((100% - var(--label-column-width)) / var(--visible-columns));
display: grid;
gap: 0;
}
.comparison-grid.columns-2 {
grid-template-columns: minmax(120px, 180px) repeat(2, 1fr);
}
.comparison-grid.columns-3 {
grid-template-columns: minmax(120px, 160px) repeat(3, 1fr);
}
.comparison-grid.columns-4 {
grid-template-columns: minmax(100px, 140px) repeat(4, 1fr);
grid-template-columns:
var(--label-column-width)
repeat(var(--package-count), minmax(var(--package-column-width), var(--package-column-width)));
}
.comparison-header {
display: contents;
}
.comparison-header > .comparison-label {
padding: 0.75rem 1rem;
border-bottom: 1px solid var(--color-border);
z-index: 3;
}
.comparison-label {
position: sticky;
left: 0;
z-index: 2;
inline-size: var(--label-column-width);
min-inline-size: var(--label-column-width);
isolation: isolate;
}
.comparison-header > .comparison-cell-header {
Expand Down
14 changes: 13 additions & 1 deletion app/components/Compare/FacetRow.vue
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@ function isCellLoading(index: number): boolean {
<template>
<div class="contents">
<!-- Label cell -->
<div class="comparison-label flex items-center gap-1.5 px-4 py-3 border-b border-border">
<div
class="comparison-label relative bg-bg flex items-center gap-1.5 px-4 py-3 border-b border-border"
>
<span class="text-xs text-fg-muted uppercase tracking-wider">{{ label }}</span>
<TooltipApp v-if="description" :text="description" position="top">
<span class="i-lucide:info w-3 h-3 text-fg-subtle cursor-help" aria-hidden="true" />
Expand Down Expand Up @@ -151,3 +153,13 @@ function isCellLoading(index: number): boolean {
</div>
</div>
</template>

<style lang="css" scoped>
.comparison-label {
position: sticky;
left: 0;
z-index: 2;
inline-size: var(--label-column-width);
min-inline-size: var(--label-column-width);
}
</style>
9 changes: 5 additions & 4 deletions app/pages/compare.vue
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const { locale } = useI18n()
const router = useRouter()
const canGoBack = useCanGoBack()
const { copied, copy } = useClipboard({ copiedDuring: 2000 })
const maxPackages = 4

// Sync packages with URL query param (stable ref - doesn't change on other query changes)
const packagesParam = useRouteQuery<string>('packages', '', { mode: 'replace' })
Expand All @@ -23,7 +24,7 @@ const packages = computed({
.split(',')
.map(p => p.trim())
.filter(p => p.length > 0)
.slice(0, 4)
.slice(0, maxPackages)
},
set(value) {
packagesParam.value = value.length > 0 ? value.join(',') : ''
Expand Down Expand Up @@ -61,12 +62,12 @@ const gridColumns = computed(() =>

// Whether we can add the no-dep column (not already added and have room)
const canAddNoDep = computed(
() => packages.value.length < 4 && !packages.value.includes(NO_DEPENDENCY_ID),
() => packages.value.length < maxPackages && !packages.value.includes(NO_DEPENDENCY_ID),
)

// Add "no dependency" column to comparison
function addNoDep() {
if (packages.value.length >= 4) return
if (packages.value.length >= maxPackages) return
if (packages.value.includes(NO_DEPENDENCY_ID)) return
packages.value = [...packages.value, NO_DEPENDENCY_ID]
}
Expand Down Expand Up @@ -191,7 +192,7 @@ useSeoMeta({
<h2 id="packages-heading" class="text-xs text-fg-subtle uppercase tracking-wider mb-3">
{{ $t('compare.packages.section_packages') }}
</h2>
<ComparePackageSelector v-model="packages" :max="4" />
<ComparePackageSelector v-model="packages" :max="maxPackages" />

<!-- "No dep" replacement suggestions (native, simple) -->
<div v-if="noDepSuggestions.length > 0" class="mt-3 space-y-2">
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/interactions.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ test.describe('Compare Page', () => {
await expect(page).toHaveURL(/packages=vue,nuxt,__no_dependency__/)

// Verify column order in the grid: vue, nuxt, then no-dep
const headerLinks = grid.locator('.comparison-cell-header a.truncate')
const headerLinks = grid.locator('.comparison-cell-header a[title]')
await expect(headerLinks).toHaveCount(2)
await expect(headerLinks.nth(0)).toContainText('vue')
await expect(headerLinks.nth(1)).toContainText('nuxt')
Expand Down
53 changes: 21 additions & 32 deletions test/nuxt/components/compare/ComparisonGrid.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ describe('ComparisonGrid', () => {
columns: cols('lodash@4.17.21', 'underscore@1.13.6'),
},
})
expect(component.text()).toContain('lodash@4.17.21')
expect(component.text()).toContain('underscore@1.13.6')
expect(component.text()).toContain('lodash')
expect(component.text()).toContain('@4.17.21')
expect(component.text()).toContain('underscore')
expect(component.text()).toContain('@1.13.6')
})

it('renders correct number of header cells', async () => {
Expand All @@ -43,74 +45,61 @@ describe('ComparisonGrid', () => {
expect(component.find('.comparison-cell-nodep').exists()).toBe(true)
})

it('truncates long header text with title attribute', async () => {
it('renders package name and version on separate clamped lines with a full title attribute', async () => {
const longName = 'very-long-package-name@1.0.0-beta.1'
const component = await mountSuspended(ComparisonGrid, {
props: {
columns: cols(longName, 'short'),
},
})
const links = component.findAll('a.truncate')
const longLink = links.find(a => a.text() === longName)
expect(longLink?.attributes('title')).toBe(longName)

const link = component.find(`a[title="${longName}"]`)
expect(link.exists()).toBe(true)
expect(link.attributes('title')).toBe(longName)
expect(link.findAll('.line-clamp-1')).toHaveLength(2)
})
})

describe('column layout', () => {
it('applies columns-2 class for 2 columns', async () => {
it('sets --package-count to the number of package columns', async () => {
const component = await mountSuspended(ComparisonGrid, {
props: {
columns: cols('a', 'b'),
},
})
expect(component.find('.columns-2').exists()).toBe(true)
const grid = component.find('.comparison-grid')
expect(grid.attributes('style')).toContain('--package-count: 2')
})

it('applies columns-3 class for 2 packages + no-dep', async () => {
it('includes the no-dependency column in --package-count', async () => {
const component = await mountSuspended(ComparisonGrid, {
props: {
columns: cols('a', 'b'),
showNoDependency: true,
},
})
expect(component.find('.columns-3').exists()).toBe(true)
})

it('applies columns-4 class for 4 columns', async () => {
const component = await mountSuspended(ComparisonGrid, {
props: {
columns: cols('a', 'b', 'c', 'd'),
},
})
expect(component.find('.columns-4').exists()).toBe(true)
const grid = component.find('.comparison-grid')
expect(grid.attributes('style')).toContain('--package-count: 3')
})

it('sets min-width for 4 columns to 800px', async () => {
it('supports four package columns with the generic grid layout', async () => {
const component = await mountSuspended(ComparisonGrid, {
props: {
columns: cols('a', 'b', 'c', 'd'),
},
})
expect(component.find('.min-w-\\[800px\\]').exists()).toBe(true)
})

it('sets min-width for 2-3 columns to 600px', async () => {
const component = await mountSuspended(ComparisonGrid, {
props: {
columns: cols('a', 'b'),
},
})
expect(component.find('.min-w-\\[600px\\]').exists()).toBe(true)
const grid = component.find('.comparison-grid')
expect(grid.attributes('style')).toContain('--package-count: 4')
})

it('sets --columns CSS variable', async () => {
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('--columns: 3')
expect(grid.attributes('style')).toContain('--package-count: 3')
})
})

Expand Down
Loading