fix: change default sorting to downloads-week-desc as in parseSortOption#2477
fix: change default sorting to downloads-week-desc as in parseSortOption#2477t128n wants to merge 2 commits 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! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe default sorting option on the organisation page has been changed from Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/pages/org/[org].vue (1)
84-90:⚠️ Potential issue | 🟠 MajorDefault sort is still inconsistent on org-switch flow (Line 115).
Line 89 now treats
'downloads-week-desc'as default for URL sync, but Line 115 still resets to'updated-desc'. On client-side org navigation, that can reintroduce mismatch and persistsort=updated-descunexpectedly.Suggested fix
+const ORG_DEFAULT_SORT: SortOption = 'downloads-week-desc' @@ - initialSort: parseSortOption(normalizeSearchParam(route.query.sort)), + initialSort: parseSortOption(normalizeSearchParam(route.query.sort)) ?? ORG_DEFAULT_SORT, @@ - sort: updates.sort && updates.sort !== 'downloads-week-desc' ? updates.sort : undefined, + sort: updates.sort && updates.sort !== ORG_DEFAULT_SORT ? updates.sort : undefined, @@ - setSort('updated-desc') + setSort(ORG_DEFAULT_SORT)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f107d08-ee6f-4ae7-99d4-b0343f9ad45b
📒 Files selected for processing (1)
app/pages/org/[org].vue
| } = useStructuredFilters({ | ||
| packages, | ||
| initialSort: (normalizeSearchParam(route.query.sort) as SortOption) ?? 'updated-desc', | ||
| initialSort: (normalizeSearchParam(route.query.sort) as SortOption) ?? 'downloads-week-desc', | ||
| }) |
There was a problem hiding this comment.
Use parseSortOption (not a direct cast) for query-derived sort.
(normalizeSearchParam(route.query.sort) as SortOption) trusts arbitrary query input. An invalid ?sort= value can enter state and bypass the normal defaulting/parsing behaviour described in the PR objective.
Suggested fix
-import type { FilterChip, SortOption } from '#shared/types/preferences'
+import { parseSortOption, type FilterChip, type SortOption } from '#shared/types/preferences'
@@
- initialSort: (normalizeSearchParam(route.query.sort) as SortOption) ?? 'downloads-week-desc',
+ initialSort: parseSortOption(normalizeSearchParam(route.query.sort)),As per coding guidelines: "Ensure you write strictly type-safe code."
📝 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.
| } = useStructuredFilters({ | |
| packages, | |
| initialSort: (normalizeSearchParam(route.query.sort) as SortOption) ?? 'updated-desc', | |
| initialSort: (normalizeSearchParam(route.query.sort) as SortOption) ?? 'downloads-week-desc', | |
| }) | |
| } = useStructuredFilters({ | |
| packages, | |
| initialSort: parseSortOption(normalizeSearchParam(route.query.sort)), | |
| }) |
There was a problem hiding this comment.
did you see this? @t128n not sure if it makes sense to do this in this pr
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Yeah, I saw that, wasn't sure of its relevancy.
As the URL parameter is passed to PackageListToolbar,
and that component
does use parseSortOption once the prop is passed to it,
https://github.com/npmx-dev/npmx.dev/blob/main/app/components/Package/ListToolbar.vue#L61
before doing work on it. To not effectively do it twice, I decided to wait for feedback yet. I guess one could do it here as well if it does provide value.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check useStructuredFilters composable
fd -i "useStructuredFilters" --extension vue --extension ts | head -20
ast-grep --pattern 'function useStructuredFilters($_) { $$$ }'
rg -n "useStructuredFilters" --type ts --type vue -A5 -B2
Length of output: 29324
🏁 Script executed:
#!/bin/bash
# Check parseSortOption definition
rg -n "parseSortOption" -A10 -B2
Length of output: 13809
🏁 Script executed:
#!/bin/bash
# Check ListToolbar line 61 area
fd -i "ListToolbar" --extension vue | head -5
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Get the full parseSortOption implementation
cat shared/types/preferences.ts | grep -A 30 "export function parseSortOption"
Length of output: 983
@t128n, thanks for the detailed explanation — after tracing through the code that's a fair point. parseSortOption does have an explicit fallback:
// Fallback to default sort option
return { key: 'downloads-week', direction: 'desc' }So any invalid ?sort= value is silently corrected to downloads-week-desc wherever parseSortOption is used — both inside comparePackages (line 315 of useStructuredFilters.ts) and in ListToolbar.vue (line 61). That makes your current approach functionally safe.
There is one subtle edge case worth knowing about: the updateUrl debounce checks:
sort: updates.sort && updates.sort !== 'downloads-week-desc' ? updates.sort : undefined,If someone visits ?sort=garbage-value, the raw invalid string would be stored in sortOption and since 'garbage-value' !== 'downloads-week-desc', it would get written back into the URL as-is, keeping the bad value in the address bar even though sorting is actually happening correctly. Calling parseSortOption at the entry point would also fix this, since it would normalise the value to 'downloads-week-desc' immediately and the URL check would then suppress it.
That said, this is a minor cosmetic issue and not a correctness problem. I'll leave the call to you on whether it's worth the extra normalisation here.
✏️ Learnings added
Learnt from: t128n
Repo: npmx-dev/npmx.dev PR: 2477
File: app/pages/org/[org].vue:58-61
Timestamp: 2026-04-13T19:31:51.202Z
Learning: In npmx-dev/npmx.dev, `parseSortOption` (shared/types/preferences.ts) has an explicit fallback `return { key: 'downloads-week', direction: 'desc' }` for invalid/unknown sort values. Both `useStructuredFilters.comparePackages` (app/composables/useStructuredFilters.ts line 315) and `ListToolbar.vue` (line 61) call `parseSortOption` before using the sort value, so invalid URL sort params are handled gracefully. The raw cast `(normalizeSearchParam(route.query.sort) as SortOption)` in org/[org].vue is therefore functionally safe, though it can cause an invalid value to be reflected back into the URL by the updateUrl debounce check.
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: ShroXd
Repo: npmx-dev/npmx.dev PR: 2025
File: app/pages/package/[[org]]/[name].vue:210-211
Timestamp: 2026-03-15T04:00:46.905Z
Learning: In app/pages/package/[[org]]/[name].vue, the `isVersionsRoute` computed flag intentionally gates only template rendering (not data fetches) for the `package-versions` child route. Skipping data fetches on the versions route would cause them to never re-fire on navigation back to the main package page, because `immediate` is evaluated once at composable setup time. All heavyweight fetch/composable initialisation therefore runs unconditionally; only the rendered output is suppressed via `v-if="!isVersionsRoute"`. Do not flag this as a performance issue or suggest conditionally blocking fetches for this route.
Learnt from: ShroXd
Repo: npmx-dev/npmx.dev PR: 2115
File: app/pages/search.vue:365-368
Timestamp: 2026-03-17T06:53:49.002Z
Learning: In npmx-dev/npmx.dev (app/pages/search.vue), the `showClaimPrompt` computed intentionally returns `true` during a `status === 'pending'` fetch even when `avail.name` doesn't match `committedQuery.value.trim()`. This is by design to prevent the claim prompt from flickering/disappearing while the user is typing (avoiding layout shifts). The claim button is separately guarded with `:disabled="status === 'pending'"` to prevent accidental clicks on stale data. Do not suggest adding an `avail.name === committedQuery` guard to the pending branch.
Learnt from: alexdln
Repo: npmx-dev/npmx.dev PR: 1845
File: app/components/InstantSearch.vue:6-11
Timestamp: 2026-03-03T09:42:56.622Z
Learning: In the npmx.dev project, `onPrehydrate` callbacks consistently use `JSON.parse(localStorage.getItem('npmx-settings') || '{}')` without try-catch error handling. This is the established pattern across all components that read settings during prehydration (e.g., app/components/Settings/BgThemePicker.vue, app/components/Settings/AccentColorPicker.vue, app/components/CollapsibleSection.vue, app/components/InstantSearch.vue). Do not suggest adding try-catch blocks to this pattern unless refactoring all instances project-wide.
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: 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: 900
File: app/pages/cookie-policy.vue:6-14
Timestamp: 2026-02-05T03:17:30.849Z
Learning: In Nuxt projects using nuxtjs/i18n v8, rely on the auto-imported $t() in script setup (including in useSeoMeta and defineOgImageComponent) and do not destructure t from useI18n(). This pattern should apply across all page components under app/pages (e.g., index.vue, about.vue, settings.vue, search.vue, cookie-policy.vue). Ensure you consistently use the auto-imported $t() instead of importing or extracting t from useI18n().
Learnt from: userquin
Repo: npmx-dev/npmx.dev PR: 900
File: app/pages/cookie-policy.vue:6-14
Timestamp: 2026-02-05T03:23:42.731Z
Learning: In this Nuxt 4 project with nuxtjs/i18n v10, $t() and other globals like $n, $d are exposed in <script setup> and work inside callbacks (including those passed to useSeoMeta and defineOgImageComponent). This pattern applies across all pages (e.g., index.vue, about.vue, settings.vue, search.vue, cookie-policy.vue). Do not destructure t from useI18n(); rely on the global provided by Nuxt i18n in script setup.
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: 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: ShroXd
Repo: npmx-dev/npmx.dev PR: 2025
File: app/components/Package/Versions.vue:533-540
Timestamp: 2026-03-15T03:59:32.483Z
Learning: In Vue 3, templates that use v-if/v-else render exactly one root element at runtime. Attributes (data-testid, title, etc.) will fall through to the active root without needing explicit prop forwarding. Do not flag attribute forwarding as a bug for this pattern; reserve review for cases where multiple roots render simultaneously or where explicit forwarding is required by design. This applies broadly to Vue single-file components using v-if/v-else.
Learnt from: graphieros
Repo: npmx-dev/npmx.dev PR: 2419
File: app/components/Compare/FacetQuadrantChart.vue:387-431
Timestamp: 2026-04-08T13:59:06.437Z
Learning: In the npmx-dev/npmx.dev project, vue-data-ui’s DOM-to-PNG export flow automatically ignores/hides any DOM elements marked with the `data-dom-to-png-ignore` attribute. Therefore, when reviewing Vue templates (including `#svg` slot content like in `app/components/Compare/FacetQuadrantChart.vue`), do not flag the absence of additional `v-if="!svg.isPrintingSvg && !svg.isPrintingImg"` guards solely on the basis that elements are already intended to be excluded from the export via `data-dom-to-png-ignore`.
ghostdevv
left a comment
ghostdevv
left a comment
There was a problem hiding this comment.
could you take a look at code rabbit's comment?
Totally missed that 2nd comment - just added another commit that fixes it |
| } = useStructuredFilters({ | ||
| packages, | ||
| initialSort: (normalizeSearchParam(route.query.sort) as SortOption) ?? 'updated-desc', | ||
| initialSort: (normalizeSearchParam(route.query.sort) as SortOption) ?? 'downloads-week-desc', | ||
| }) |
There was a problem hiding this comment.
did you see this? @t128n not sure if it makes sense to do this in this pr
2 participants
🔗 Linked issue
Resolves #2471
🧭 Context
Describe the bug
https://npmx.dev/org/arnaud-barre-> Sort dropdown isDownloads/wkLast published, no URL changeDownloads/wk,?sort=downloads-week-descis append to the URLThe makes it annoying in the following flow:
Changed default sorting on
/org/pages to match the default sorting method used innpmx.dev/shared/types/preferences.ts
Lines 133 to 149 in 0164064
📚 Description
/org/[org]pages usedupdated-descas the default sorting methodnpmx.dev/app/pages/org/[org].vue
Line 60 in 0164064
The
ListToolbarcomponent used on that page, relies on the fnparseSortOption, which usesdownloads-week-descas the default sorting method though, as defined innpmx.dev/shared/types/preferences.ts
Lines 133 to 149 in 0164064
So when
/org/used the default sorting method, it didn't save it in the URL query params. The/org/page thought thatupdated-desc iwas used, butListToolbarthought thatdownloads-weeks-desc iwas used. This caused the desync as outlined in the issue.I adjusted the default on the
/org/page to align with the default set inshared/types/preferences.ts.