Skip to content

Comments

fix(ui): hide awkward empty state for weekly downloads for new packages#1054

Merged
serhalp merged 4 commits intonpmx-dev:mainfrom
wojtekmaj:empty-weekly-downloads
Feb 8, 2026
Merged

fix(ui): hide awkward empty state for weekly downloads for new packages#1054
serhalp merged 4 commits intonpmx-dev:mainfrom
wojtekmaj:empty-weekly-downloads

Conversation

@wojtekmaj
Copy link
Contributor

Closes #1044

Too new packages now have Weekly downloads hidden:

image

Well established packages of course still have the section:

image

Copilot AI review requested due to automatic review settings February 5, 2026 22:46
@vercel
Copy link

vercel bot commented Feb 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Feb 7, 2026 11:26pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 7, 2026 11:26pm
npmx-lunaria Ignored Ignored Feb 7, 2026 11:26pm

Request Review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 5, 2026

📝 Walkthrough

Walkthrough

The WeeklyDownloadStats component now tracks loading and data presence with two new state properties. Rendering is conditional: a sparkline or loading skeleton shows only while loading or when data exists; an explicit "No download data available" message appears for empty data. The chart modal and analyse button are prevented from opening/displaying when no weekly-download data exists. Unit tests cover both empty and populated scenarios.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description directly addresses the changeset by referencing issue #1044 and demonstrating the UI fix with visual comparisons of before and after states.
Linked Issues check ✅ Passed The pull request implements the core requirement from issue #1044 by hiding the empty weekly downloads chart and displaying a 'No download data available' message instead, aligning with the suggested solution.
Out of Scope Changes check ✅ Passed All changes are scoped to the WeeklyDownloadStats component and its corresponding unit tests, directly addressing the linked issue without introducing unrelated modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Feb 5, 2026

Codecov Report

❌ Patch coverage is 68.00000% with 8 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
app/components/Package/WeeklyDownloadStats.vue 68.00% 6 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the awkward empty state for weekly downloads on new packages by conditionally hiding the entire downloads section when no data is available. The implementation adds loading state tracking and data presence checks to determine when to show or hide the section, following the pattern used by npmjs.com.

Changes:

  • Added computed property to check if weekly downloads data exists
  • Added loading state to track data fetch progress
  • Conditionally render the downloads section based on loading state or data presence
  • Guard modal opening and button visibility with data presence checks

@serhalp
Copy link
Member

serhalp commented Feb 6, 2026

My hunch is it's more confusing to hide this entirely. We might get future bug reports like "the downloads chart is missing". Could we keep just the heading and show "No downloads yet"? 🤔

@wojtekmaj
Copy link
Contributor Author

My hunch is it's more confusing to hide this entirely. We might get future bug reports like "the downloads chart is missing". Could we keep just the heading and show "No downloads yet"? 🤔

Good idea, I'll do this today

<!-- Data label (covers ~42% width) -->
<div class="w-[42%] flex items-center ps-0.5">
<SkeletonInline class="h-7 w-24" />
<template v-if="isLoadingWeeklyDownloads || hasWeeklyDownloads">
Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean if we're loading the downloads but ultimately have none in the end, we'll flash the chart up then remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we're optimistically assuming it WILL load, as it does for 99,9% of cases

@43081j
Copy link
Contributor

43081j commented Feb 6, 2026

also agree we should show a "no data" message or something instead, like a subtle overlay of some sort

i see you're already planning on changing it so will re-review once you push 👍

@ghostdevv ghostdevv marked this pull request as draft February 6, 2026 22:49
@ghostdevv
Copy link
Contributor

(marked as draft while you're making changes)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/components/Package/WeeklyDownloadStats.vue (1)

104-119: ⚠️ Potential issue | 🟠 Major

Reset weekly download state before fetching to avoid stale charts and surprise modal re-open.

When switching packages, the previous weeklyDownloads array remains until the new request resolves, so the sparkline/analyse button can briefly show old data. If the modal was open, isChartModalOpen can also stay true and re-open when data returns. Clearing the data and resetting modal state at the start of each load prevents that.

💡 Proposed fix
 async function loadWeeklyDownloads() {
   if (!import.meta.client) return
 
   isLoadingWeeklyDownloads.value = true
+  weeklyDownloads.value = []
+  if (isChartModalOpen.value) {
+    isChartModalOpen.value = false
+    hasChartModalTransitioned.value = false
+  }
   try {
     const result = await fetchPackageDownloadEvolution(
       () => props.packageName,
       () => props.createdIso,
       () => ({ granularity: 'week' as const, weeks: 52 }),
     )

Copy link
Member

@serhalp serhalp left a comment

Choose a reason for hiding this comment

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

🎉

@serhalp serhalp added this pull request to the merge queue Feb 8, 2026
Merged via the queue into npmx-dev:main with commit 188e887 Feb 8, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Awkward empty state for weekly downloads section

4 participants