Skip to content

fix: change default sorting to downloads-week-desc as in parseSortOption #2477

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.

Sign up for GitHub

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

Open
t128n wants to merge 2 commits into
base: main
Choose a base branch
from
+3 −3
Open

fix: change default sorting to downloads-week-desc as in parseSortOption #2477

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
943660b
fix: change default sorting to downloads-week-desc as in parseSortOption
t128n Apr 11, 2026
f34bac3
fix: change sorting method in org-change watch
t128n Apr 12, 2026
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
4 changes: 2 additions & 2 deletions app/pages/org/[org].vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const {
setSort,
} = useStructuredFilters({
packages,
initialSort: (normalizeSearchParam(route.query.sort) as SortOption) ?? 'updated-desc',
initialSort: (normalizeSearchParam(route.query.sort) as SortOption) ?? 'downloads-week-desc',
})
Comment on lines 58 to 61
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

Suggested change
} = 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)),
})

Copy link
Copy Markdown
Contributor

@ghostdevv ghostdevv Apr 13, 2026

Choose a reason for hiding this comment

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

did you see this? @t128n not sure if it makes sense to do this in this pr

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 13, 2026

Choose a reason for hiding this comment

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

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!

Copy link
Copy Markdown
Contributor Author

@t128n t128n Apr 13, 2026

Choose a reason for hiding this comment

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

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.

@coderabbitai

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 13, 2026

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


// Pagination state
Expand Down Expand Up @@ -86,7 +86,7 @@ const updateUrl = debounce((updates: { filter?: string; sort?: string }) => {
query: {
...route.query,
q: updates.filter || undefined,
sort: updates.sort && updates.sort !== 'updated-desc' ? updates.sort : undefined,
sort: updates.sort && updates.sort !== 'downloads-week-desc' ? updates.sort : undefined,
},
})
}, 300)
Expand Down
Loading