fix(ui): version selector older groups and versions history link#2276
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- cover version page links for scoped and unscoped packages - add regression tests for collapsing groups, prop resets, and loading guards
📝 WalkthroughWalkthroughReplaces inline route-construction in components with a new exported helper Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
🧹 Nitpick comments (2)
app/components/VersionSelector.vue (2)
328-346: Make the first all-versions load single-flight.Line 328 only blocks repeat clicks on the same row. If a second row is toggled before the first request resolves,
loadAllVersions()will still start another fetch. Sharing one in-flight promise at component scope keeps the new “load once” behaviour true under parallel interaction.Possible approach
+let allVersionsPromise: Promise<PackageVersionInfo[]> | null = null + async function loadAllVersions(): Promise<PackageVersionInfo[]> { if (allVersionsCache.value) return allVersionsCache.value + if (allVersionsPromise) return allVersionsPromise - isLoadingAll.value = true - try { - const versions = await fetchAllPackageVersions(props.packageName) - allVersionsCache.value = versions - hasLoadedAll.value = true - return versions - } finally { - isLoadingAll.value = false - } + allVersionsPromise = (async () => { + isLoadingAll.value = true + try { + const versions = await fetchAllPackageVersions(props.packageName) + allVersionsCache.value = versions + hasLoadedAll.value = true + return versions + } finally { + isLoadingAll.value = false + allVersionsPromise = null + } + })() + + return allVersionsPromise }
679-680: Please add a direct assertion for the new footer route.This now switches the footer to the version-history page, but
test/nuxt/components/VersionSelector.spec.ts:144-159still only checks the label text. Anhrefassertion here would keep the routing fix covered.As per coding guidelines, "Write unit tests for core functionality using `vitest`."Small test addition
it('shows "View all X versions" link', async () => { ... await button.trigger('click') expect(component.text()).toContain('View all 3 versions') + expect( + component.find('a[href="/package/test-package/versions"]').exists(), + ).toBe(true) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2526e043-faf3-4804-b656-f52a6a49bb10
📒 Files selected for processing (4)
app/components/Package/Versions.vueapp/components/VersionSelector.vueapp/utils/router.tstest/nuxt/components/VersionSelector.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5ece50e-5fe7-40b5-bb6e-90b67360bd79
📒 Files selected for processing (2)
test/nuxt/components/Package/Versions.spec.tstest/nuxt/components/VersionSelector.spec.ts
✅ Files skipped from review due to trivial changes (1)
- test/nuxt/components/VersionSelector.spec.ts
|
Sorry this took a few iterations, it worked on my machine! 🤣 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/nuxt/components/VersionSelector.spec.ts (2)
429-469: Strengthen the hidden-before-expand precondition in this regression test.This case can still pass if
0.9.xis visible too early, because it never asserts initial hidden state and starts with0.9.0already present inversions.Proposed test hardening
const component = await mountSuspended(VersionSelector, { props: { packageName: 'test-package', currentVersion: '1.0.0', - versions: { '1.0.0': {}, '0.9.0': {} }, + versions: { '1.0.0': {} }, distTags: { latest: '1.0.0' }, urlPattern: '/package-docs/test-package/v/{version}', }, }) const button = component.find('button[aria-haspopup="listbox"]') await button.trigger('click') + expect(component.find('[role="listbox"]').text()).not.toContain('0.9') const expandButton = component.find('[role="listbox"] button[aria-expanded="false"]') await expandButton.trigger('click')
550-579: Prefer asserting reset on a semanticdistTagschange, not just object identity.
setProps({ distTags: { latest: '1.0.0' } })reuses equivalent values, so the test can pass even if only reference changes are observed.Proposed adjustment
- await component.setProps({ distTags: { latest: '1.0.0' } }) + await component.setProps({ distTags: { latest: '1.0.0', next: '0.9.0' } })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bbb0672-2173-41f9-9193-a386a8b1f3a3
📒 Files selected for processing (3)
app/components/VersionSelector.vuetest/nuxt/components/Package/Versions.spec.tstest/nuxt/components/VersionSelector.spec.ts
✅ Files skipped from review due to trivial changes (2)
- test/nuxt/components/Package/Versions.spec.ts
- app/components/VersionSelector.vue
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/nuxt/components/VersionSelector.spec.ts (2)
517-548: Consider explicitly focusing the group item before ArrowLeft.The
handleListboxKeydownhandler forArrowLeftonly acts whenfocusedIndexpoints to a group item. This test relies on the dropdown's initial focus being set to a group, which is implicit behaviour. If the focus initialisation logic changes, this test could become flaky.Consider adding an explicit keyboard navigation step (e.g.,
HomeorArrowUp/ArrowDown) to ensure focus is on the tagged group row before triggeringArrowLeft, making the test more robust.
574-574: Use a clearly different prop value to improve test clarity.Setting
distTagsto{ latest: '1.0.0' }when the initial value is also{ latest: '1.0.0' }works (due to reference change triggering the watcher), but it's confusing. A reader might wonder why an apparently identical value causes a state reset.Consider using a distinctly different value to make the test's intent explicit:
🔧 Suggested change
- await component.setProps({ distTags: { latest: '1.0.0' } }) + await component.setProps({ distTags: { latest: '1.0.0', beta: '0.9.0' } })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 077d4d17-8194-4923-940e-79ac60962547
📒 Files selected for processing (2)
test/nuxt/components/Package/Versions.spec.tstest/nuxt/components/VersionSelector.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/nuxt/components/Package/Versions.spec.ts
2 participants
🔗 Linked issue
Resolves #2247
🧭 Context
The package version dropdown could not meaningfully expand when the visible "group" was a single tagged release but older versions existed without tags: there was nothing to expand, yet users still needed a way to load and show those older rows. The "View all versions" link in the selector also pointed at the package overview instead of the dedicated version history page.
A follow-up issue also existed in the first-load expansion path: clicking a tagged row with nested versions could unintentionally reveal every hidden untagged group, even when that row was only supposed to expand its own nested versions.
📚 Description
Version selector
showAllGroupsandvisibleVersionGroupshide untagged groups until the user expands the controlling tagged row, and collapse them again on second toggle.toggleGroupis refactored so loading runs once, nested multi-version groups still expand/collapse, and the expand control stays correct for loading, nested, and “show more groups” cases.showAllGroupsis only enabled when the reloaded row actually controls additional hidden groups. Expanding a tagged row with nested versions now opens only that row instead of also revealing unrelated untagged groups.isGroupOpen/canToggleGroupso Enter matches the new semantics; ArrowLeft can collapse the additional-groups state.aria-expanded/ labels derived fromisGroupOpen.distTags,versions, orcurrentVersionchange,showAllGroupsresets so stale UI does not stick around.Routing
packageVersionsRoute(packageName)inapp/utils/router.tsfor the package-versions route.Versions.vueuses that helper instead of duplicating org/name parsing.packageVersionsRouteso “view all versions” goes to version history, not the main package route.Tests
test/nuxt/components/VersionSelector.spec.ts: single tagged release + fetch returns an older version; expand shows0.9, collapse hides it again.test/nuxt/components/VersionSelector.spec.tscovering a tagged row with nested same-major versions plus an unrelated older group; expanding that tagged row no longer reveals the unrelated hidden group.