feat(ui): add multi-select and bulk actions for packages#1672
feat(ui): add multi-select and bulk actions for packages#1672MatteoGabriele wants to merge 37 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
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
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
MatteoGabriele
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/composables/usePackageSelection.ts (1)
10-22: Canonicalise selection values before persisting to query state.
selectedPackagesreads a capped list, but Line 20-Line 21 can still write duplicates or over-limit values into the URL. Keeping read/write logic symmetrical avoids query-state drift.♻️ Suggested normalisation patch
const selectedPackages = computed<string[]>({ get() { const raw = selectedPackagesParam.value if (!raw) return [] - return raw - .split(',') - .map(p => String(p).trim()) - .filter(Boolean) - .slice(0, MAX_PACKAGE_SELECTION) + return [...new Set( + raw + .split(',') + .map(p => String(p).trim()) + .filter(Boolean), + )].slice(0, MAX_PACKAGE_SELECTION) }, set(pkgs: string[]) { - // Ensure all items are strings before joining - const validPkgs = (Array.isArray(pkgs) ? pkgs : []).map(p => String(p).trim()).filter(Boolean) + const validPkgs = [...new Set( + (Array.isArray(pkgs) ? pkgs : []) + .map(p => String(p).trim()) + .filter(Boolean), + )].slice(0, MAX_PACKAGE_SELECTION) selectedPackagesParam.value = validPkgs.length > 0 ? validPkgs.join(',') : '' }, })
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/components/Package/ActionBar.vueapp/components/Package/Card.vueapp/components/Package/ListToolbar.vueapp/components/Package/SelectionView.vueapp/components/Package/Table.vueapp/components/Package/TableRow.vueapp/composables/usePackageSelection.tsapp/pages/search.vueapp/router.options.tsi18n/locales/en.jsoni18n/locales/it-IT.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonlunaria/files/it-IT.jsonshared/types/npm-registry.tstest/unit/shared/types/index.spec.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- lunaria/files/en-US.json
- i18n/locales/en.json
- app/components/Package/ActionBar.vue
- app/components/Package/SelectionView.vue
- app/components/Package/ListToolbar.vue
- app/components/Package/Table.vue
- i18n/schema.json
| <div | ||
| v-if="result.downloads?.weekly" | ||
| class="text-fg-subtle gap-2 flex items-center justify-end" | ||
| class="text-fg-subtle gap-2 flex items-center sm:justify-end" | ||
| > |
There was a problem hiding this comment.
Prevent duplicate weekly-downloads rendering on small screens.
Line 157 renders this downloads row on all breakpoints, while Lines 121-133 already render a mobile-only downloads row. On small screens, users will see the same metric twice.
Proposed fix
- <div
- v-if="result.downloads?.weekly"
- class="text-fg-subtle gap-2 flex items-center sm:justify-end"
- >
+ <div
+ v-if="result.downloads?.weekly"
+ class="hidden sm:flex text-fg-subtle gap-2 items-center sm:justify-end"
+ >📝 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.
| <div | |
| v-if="result.downloads?.weekly" | |
| class="text-fg-subtle gap-2 flex items-center justify-end" | |
| class="text-fg-subtle gap-2 flex items-center sm:justify-end" | |
| > | |
| <div | |
| v-if="result.downloads?.weekly" | |
| class="hidden sm:flex text-fg-subtle gap-2 items-center sm:justify-end" | |
| > |
app/components/Package/TableRow.vue
Outdated
| :title=" | ||
| isMaxSelected && !isSelected | ||
| ? $t('package.card.select_maximum', MAX_PACKAGE_SELECTION) | ||
| : undefined |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the TableRow.vue file around the specified lines
cat -n app/components/Package/TableRow.vue | sed -n '210,225p'Repository: npmx-dev/npmx.dev
Length of output: 902
🏁 Script executed:
# Find translation files to check the message definition
find . -type f \( -name "*.json" -o -name "*.yaml" -o -name "*.yml" \) | grep -i "i18n\|translation\|locale" | head -20Repository: npmx-dev/npmx.dev
Length of output: 572
🏁 Script executed:
# Search for the specific translation key
rg "select_maximum" --type json --type yamlRepository: npmx-dev/npmx.dev
Length of output: 381
🏁 Script executed:
# Check other usages of $t with MAX_PACKAGE_SELECTION or similar pattern
rg "\$t\(" app/components/Package/TableRow.vue -A 2 -B 2Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check the package.json to see vue-i18n version
cat package.json | grep -A 2 -B 2 "vue-i18n"Repository: npmx-dev/npmx.dev
Length of output: 241
🏁 Script executed:
# Get the full package.json to find vue-i18n version
cat package.json | grep "vue-i18n" | head -5Repository: npmx-dev/npmx.dev
Length of output: 93
🏁 Script executed:
# Check the i18n schema to understand the expected format
cat i18n/schema.json | grep -A 10 "select_maximum"Repository: npmx-dev/npmx.dev
Length of output: 331
🏁 Script executed:
# Check other similar $t() calls in the codebase to see the pattern used
rg "\$t\(" app/components/ -B 1 -A 1 | grep -E "\$t\(|count|MAX" | head -30Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Search for vue-i18n in dependencies more carefully
grep -E '"vue-i18n"' package.json -A 1 -B 1Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Look for examples of $t calls with parameters in the entire codebase
rg "\$t\(" --type vue -A 1 | grep -E "\$t\(|count|MAX_|{" | head -40Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Check the i18n configuration/setup
find . -name "i18n*" -type f | grep -E "\.(ts|js|config)" | head -10Repository: npmx-dev/npmx.dev
Length of output: 165
🏁 Script executed:
# Look for plural or named parameters in translation calls
rg "\$t\(" -A 1 | grep -B 1 "{" | head -30Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check full package.json for vue-i18n dependency
cat package.json | jq '.dependencies, .devDependencies' | grep -A 1 -B 1 "vue-i18n"Repository: npmx-dev/npmx.dev
Length of output: 150
🏁 Script executed:
# Look at i18n configuration
cat i18n/i18n.config.tsRepository: npmx-dev/npmx.dev
Length of output: 400
🏁 Script executed:
# Check for other $t calls with parameters
rg '\$t\(' app/ -A 1 -B 1 --max-count 50Repository: npmx-dev/npmx.dev
Length of output: 50374
🏁 Script executed:
# Look at the i18n-loader plugin
cat app/plugins/i18n-loader.client.tsRepository: npmx-dev/npmx.dev
Length of output: 629
Pass named interpolation params to select_maximum.
Line 218 currently passes a number directly to $t; the translation message uses the {count} placeholder, which requires a named params object. This is consistent with every similar usage throughout the codebase (e.g. $t('org.public_packages', { count: $n(results.total) }, results.total)).
✅ Suggested fix
- :title="
- isMaxSelected && !isSelected
- ? $t('package.card.select_maximum', MAX_PACKAGE_SELECTION)
- : undefined
- "
+ :title="
+ isMaxSelected && !isSelected
+ ? $t('package.card.select_maximum', { count: MAX_PACKAGE_SELECTION })
+ : undefined
+ "
app/router.options.ts
Outdated
| // If only query parameters changed (same path), don't scroll | ||
| if (to.path === from.path) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Vue Router 4 scrollBehavior path hash query difference to.path to.fullPath
💡 Result:
In Vue Router 4, scrollBehavior(to, from, savedPosition) receives normalized route locations (to/from) that contain (among others) path, fullPath, query, and hash. (router.vuejs.org)
to.path vs to.fullPath
to.path: URL pathname only (no?query, no#hash). Use this when you only care whether you changed “page/route” (same component/record, different query, etc.).to.fullPath: the whole URL for the SPA location, including query string and hash (“search and hash”), and it’s percent-encoded as a string. Use this when you care about exact URL changes (query/hash changes included). (router.vuejs.org)
Practical implication:
- Changing
?query=...usually keepsto.paththe same, but changesto.fullPath. - Changing
#hashkeepsto.paththe same, but changesto.fullPath.
to.query vs to.hash
to.query: the parsed query object (what comes after?).to.hash: the fragment (what comes after#), and in Router 4 it’s designed to be usable directly (e.g., for anchor scrolling). (router.vuejs.org)
How this matters in scrollBehavior
Common patterns:
1) Scroll to anchor when there’s a hash
scrollBehavior(to, from, savedPosition) {
if (savedPosition) return savedPosition
if (to.hash) return { el: to.hash }
return { top: 0 }
}(Using el: to.hash is the Router 4 anchor-scrolling style.) (vue-router.netlify.app)
2) Don’t scroll when only query/hash changed within the same path
scrollBehavior(to, from, savedPosition) {
if (savedPosition) return savedPosition
if (to.hash) return { el: to.hash }
// same “page”, only query changed
if (to.path === from.path) return {}
return { top: 0 }
}This works because path ignores query/hash, while fullPath changes when they change. (router.vuejs.org)
Citations:
- 1: https://router.vuejs.org/zh/guide/advanced/scroll-behavior.html?utm_source=openai
- 2: https://router.vuejs.org/api/interfaces/routelocationnormalizedgeneric?utm_source=openai
- 3: https://router.vuejs.org/guide/migration/?utm_source=openai
- 4: https://vue-router.netlify.app/guide/advanced/scroll-behavior.html?utm_source=openai
- 5: https://router.vuejs.org/api/interfaces/routelocationnormalizedgeneric?utm_source=openai
🏁 Script executed:
# First, let's find and examine the actual file
fd app/router.options.tsRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Read the actual file to see the full context
cat -n app/router.options.tsRepository: npmx-dev/npmx.dev
Length of output: 1001
Hash-anchor navigation is being blocked on same-path routes.
At Line 11, to.path === from.path matches hash-only navigations (for example /search#foo → /search#bar). Because this returns false before the hash handling at Line 16, anchor scrolling is skipped.
Proposed fix
- // If only query parameters changed (same path), don't scroll
- if (to.path === from.path) {
+ // If only query parameters changed (same path + same hash), don't scroll
+ if (to.path === from.path && to.hash === from.hash && to.fullPath !== from.fullPath) {
return false
}2 participants
🔗 Linked issue
resolves #1509
🧭 Context
Added a multi-select feature to the search page that allows users to select multiple packages and perform bulk actions on them. Currently supports comparing selected packages, with the possibility of adding more actions in the future.
📚 Description
At the moment, the logic is locked at a maximum of 4 selectable items. This won't scale, but for now, to reduce complexity, it will mimic what's needed by the Compare page, which is the only current functionality available in the multi-select.
My take is to re-think this along the way, when and if another action gets added.
Screen.Recording.2026-02-27.at.18.33.58.mov