feat: add semver range filter and element title support#2176
feat: add semver range filter and element title support#2176ShroXd wants to merge 9 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (5)
📝 WalkthroughWalkthroughReplaces per-group lazy full-metadata fetch with an immediate watcher on Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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 Tip You can customize the tone of the review comments and chat replies.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/pages/package/[[org]]/[name]/versions.vue (1)
521-521:⚠️ Potential issue | 🟡 MinorSSR fallback lacks title attribute.
The main template (line 409) adds
:title="item.versions[0]"to the version span, but the SSR fallback omits it. For consistency:-<span class="text-xs text-fg-muted" dir="ltr">{{ item.versions[0] }}</span> +<span class="text-xs text-fg-muted" :title="item.versions[0]" dir="ltr">{{ item.versions[0] }}</span>
🧹 Nitpick comments (1)
app/pages/package/[[org]]/[name]/versions.vue (1)
409-411: Consider defensive access for array index.Whilst
filteredGroupsfilters out empty groups (line 149), the coding guidelines recommend always checking when accessing array values by index. Consider using optional chaining for defensive coding:-<span class="text-xs text-fg-muted" :title="item.versions[0]" dir="ltr">{{ - item.versions[0] -}}</span> +<span class="text-xs text-fg-muted" :title="item.versions[0] ?? ''" dir="ltr">{{ + item.versions[0] ?? '' +}}</span>As per coding guidelines: "ensure you always check when accessing an array value by index".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2eef2dd3-3a98-4cd5-aa58-00da50acb7a7
📒 Files selected for processing (1)
app/pages/package/[[org]]/[name]/versions.vue
|
Works well. Try After filtering, this icons keeps spinning. Maybe replace with a fixed "funnel" icon indicating that the list was filtered. 
My final wish: I'm testing with 
Similarly, if a tag points to a deprecated version, we could add a "deprecated" badge in the tag list: 
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/nuxt/pages/PackageVersionsPage.spec.ts (2)
144-153: Assert fetch sequencing, not just fetch count.At the moment, this only proves “called once”. It does not verify the “after phase 1 completes” part from the test title. Add an explicit order assertion to lock in the intended behaviour.
Suggested test hardening
it('fetches full metadata automatically after phase 1 completes, exactly once', async () => { - mockGetVersions.mockResolvedValue(makeVersionData(['2.0.0', '1.0.0'], { latest: '2.0.0' })) - mockFetchAllPackageVersions.mockResolvedValue([ + const callOrder: string[] = [] + mockGetVersions.mockImplementation(async () => { + callOrder.push('phase1') + return makeVersionData(['2.0.0', '1.0.0'], { latest: '2.0.0' }) + }) + mockFetchAllPackageVersions.mockImplementation(async () => { + callOrder.push('phase2') + return [ { version: '2.0.0', time: '2024-01-15T00:00:00.000Z', hasProvenance: false }, { version: '1.0.0', time: '2024-01-10T00:00:00.000Z', hasProvenance: false }, - ]) + ] + }) await mountPage() - await vi.waitFor(() => expect(mockFetchAllPackageVersions).toHaveBeenCalledTimes(1)) + await vi.waitFor(() => { + expect(mockGetVersions).toHaveBeenCalledTimes(1) + expect(mockFetchAllPackageVersions).toHaveBeenCalledTimes(1) + expect(callOrder).toEqual(['phase1', 'phase2']) + }) })
170-170: Prefer a more specific selector for the semver input.
input[autocomplete="off"]can become ambiguous if another input with the same attribute is added. A dedicated attribute (e.g.title,aria-label, ordata-testid) would make this test less brittle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dabc6865-b9da-431e-ba05-e5a802c1c56e
📒 Files selected for processing (2)
app/pages/package/[[org]]/[name]/versions.vuetest/nuxt/pages/PackageVersionsPage.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/pages/package/[[org]]/[name]/versions.vue
ShroXd
force-pushed
the
feat/semver-range-filter
branch
from
3ea5f92 to
b69ef86
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/pages/package/[[org]]/[name]/versions.vue (1)
431-433: Consider guarding array access for strict type-safety.
item.versions[0]is accessed without an explicit bounds check. WhilefilteredGroupsfilters out empty groups (ensuring this is safe in practice), TypeScript cannot guarantee the array is non-empty at compile time.♻️ Optional: Add fallback for stricter safety
-<span class="text-xs text-fg-muted" :title="item.versions[0]" dir="ltr">{{ - item.versions[0] -}}</span> +<span class="text-xs text-fg-muted" :title="item.versions[0] ?? ''" dir="ltr">{{ + item.versions[0] ?? '' +}}</span>As per coding guidelines: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b81a0f4b-33c4-4f2b-8faf-daabd75ae3e2
📒 Files selected for processing (2)
app/pages/package/[[org]]/[name]/versions.vuetest/nuxt/pages/PackageVersionsPage.spec.ts
✅ Files skipped from review due to trivial changes (1)
- test/nuxt/pages/PackageVersionsPage.spec.ts
| watch( | ||
| versionSummary, | ||
| async summary => { | ||
| if (summary) { | ||
| await ensureFullDataLoaded() | ||
| } finally { | ||
| loadingGroup.value = null | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| { immediate: true }, | ||
| ) |
There was a problem hiding this comment.
Unhandled promise rejection in async watch callback.
If fetchAllPackageVersions throws (e.g. network error), the error will be silently swallowed or cause an unhandled rejection. Consider adding error handling to improve resilience.
🛡️ Proposed fix to handle errors gracefully
watch(
versionSummary,
async summary => {
if (summary) {
- await ensureFullDataLoaded()
+ try {
+ await ensureFullDataLoaded()
+ } catch (err) {
+ console.error('[versions] Failed to load full metadata:', err)
+ }
}
},
{ immediate: true },
)As per coding guidelines: "Use error handling patterns consistently".
📝 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.
| watch( | |
| versionSummary, | |
| async summary => { | |
| if (summary) { | |
| await ensureFullDataLoaded() | |
| } finally { | |
| loadingGroup.value = null | |
| } | |
| } | |
| } | |
| }, | |
| { immediate: true }, | |
| ) | |
| watch( | |
| versionSummary, | |
| async summary => { | |
| if (summary) { | |
| try { | |
| await ensureFullDataLoaded() | |
| } catch (err) { | |
| console.error('[versions] Failed to load full metadata:', err) | |
| } | |
| } | |
| }, | |
| { immediate: true }, | |
| ) |
|
Hi @mbtools, thanks for the feedback! I have updated my PR. Because of the design of fetching, the information about whether a version is deprecated can only be known after phase 2 is finished. Therefore I changed the behavior of the network request, making phase 2 fire automatically after phase 1 is finished. This puts additional rendering pressure on the main thread. The performance profiling shows a significant number of |
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
|
|
I tried on my phone and it's super fast even with over a thousand versions in a package like @nx/js. LGTM 👍 PS wishlist: two toggle buttons to hide deprecated and pre-release versions. I would hide them by default since these versions are only relevant for maintainers or early adopters ie a fraction of the users. |
|
@mbtools Happy to hear you like it! The idea of a toggle button to hide some versions is great! I would move it to a separate PR because although the implementation is simple, some business logic needs to be discussed, like:
I can write an issue later. Once we gather enough thoughts from the community, I can finish the remaining things. |
3 participants
🔗 Linked issue
n/a
🧭 Context
Following work of #2105 , was mentioned in the PR review comment.
cc: @mbtools
📚 Description
Add description for semver range filtering and validator. The normal error message would make the page shake during input, so an inline icon is used to indicate the error state instead.
Screen.Recording.2026-03-21.at.17.02.21.mov
Also added titles to elements.