Skip to content

Conversation

@MatteoGabriele
Copy link
Contributor

@MatteoGabriele MatteoGabriele commented Feb 9, 2026

Adds query parameters to support the downloads modal permalink.
When present, the modal opens on page load and persists across refreshes.
Included parameters: granularity, facet, start date, and end date.

link to a modal

Screen.Recording.2026-02-09.at.22.19.01.mov

closes #516

@vercel
Copy link

vercel bot commented Feb 9, 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 11, 2026 3:55am
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 11, 2026 3:55am
npmx-lunaria Ignored Ignored Feb 11, 2026 3:55am

Request Review

@codecov
Copy link

codecov bot commented Feb 9, 2026

Codecov Report

❌ Patch coverage is 83.33333% with 5 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 63.63% 4 Missing ⚠️
app/composables/usePermalink.ts 92.85% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 9, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This pull request introduces a centralised type system for chart data structures and implements route-driven modal state management. A new app/types/chart.ts module defines types for chart granularities, data points, and evolution options. Types are migrated from useCharts.ts to this centralised location. A new usePermalink composable enables toggling between route query parameters and local state. WeeklyDownloadStats.vue is updated to control modal visibility through route parameters. TrendsChart.vue adds a permalink prop and switches internal state to use permalink-backed values for granularity, date range, and metric selection.

Possibly related PRs

Suggested reviewers

  • danielroe
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly outlines the purpose of adding query parameters for the downloads modal permalink, including specific parameters (granularity, facet, start date, end date) and expected behavior.
Linked Issues check ✅ Passed The PR successfully addresses issue #516 by implementing route-based modal state management. The changes enable the modal to open via query parameters and persist across page refreshes, fulfilling the requirement for a route-driven modal.
Out of Scope Changes check ✅ Passed All changes are directly aligned with implementing query parameter support for the downloads modal. The modifications to state management, type definitions, and component props are all necessary for the stated objective.

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

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

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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

@graphieros graphieros marked this pull request as draft February 9, 2026 22:00
@graphieros
Copy link
Contributor

graphieros commented Feb 9, 2026

This introduces a regression, the data fetched does not correspond to the expected range:

  • when loading the modal it should display 52 weeks
  • when switching to monthly, 12 months

Also, the refresh button next to the date inputs, that shows when changing the date or granularity, stays visible when used, yet it should only appear when the selection or granularity is different than the default 52 weeks

@graphieros
Copy link
Contributor

In the meantime, @serhalp has made a PR that generalizes and refactors the component. You might consider taking it from here (can't imagine the conflicts otherwise...)

@MatteoGabriele
Copy link
Contributor Author

@graphieros, would you mind giving this another look? I think the graph is now working as expected, and the reset button only shows up when the dates change and disappears when pressed. wdyt? 🙏

@MatteoGabriele MatteoGabriele marked this pull request as ready for review February 10, 2026 19:08
@graphieros
Copy link
Contributor

Looks good to me :)

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.

Actionable comments posted: 3

🧹 Nitpick comments (2)
app/components/Package/TrendsChart.vue (1)

1535-1543: Remove inline focus-visible utility class from the reset button.

The project applies focus-visible styling for buttons globally via main.css. Inline utilities like focus-visible:outline-accent/70 should be avoided for consistency. Based on learnings: "focus-visible styling for buttons and selects is applied globally via main.css... individual buttons or selects in Vue components should not rely on inline focus-visible utility classes."

♻️ Proposed fix
         class="self-end flex items-center justify-center px-2.5 py-1.75 border border-transparent rounded-md text-fg-subtle hover:text-fg transition-colors hover:border-border focus-visible:outline-accent/70 sm:mb-0"
+        class="self-end flex items-center justify-center px-2.5 py-1.75 border border-transparent rounded-md text-fg-subtle hover:text-fg transition-colors hover:border-border sm:mb-0"
app/components/Package/WeeklyDownloadStats.vue (1)

253-262: Remove inline focus-visible utility class from the button.

The project applies focus-visible styling for buttons globally via main.css. Inline utilities like focus-visible:outline-accent/70 should be avoided for consistency. Based on learnings: "focus-visible styling for buttons and selects is applied globally via main.css... individual buttons or selects in Vue components should not rely on inline focus-visible utility classes."

♻️ Proposed fix
-          class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 focus-visible:outline-accent/70 rounded"
+          class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 rounded"

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.

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/components/Package/TrendsChart.vue (1)

322-360: ⚠️ Potential issue | 🟠 Major

Prevent permalink mode from activating when the prop is unset.

Passing permanent: props.permalink means undefined falls back to true inside usePermalink, so non-permalink contexts will still sync to the URL. Default the prop to false (or coalesce) before passing it into the composable.

✅ Suggested fix (single source of truth)
+const permalinkEnabled = props.permalink ?? false
+
-const selectedGranularity = usePermalink<ChartTimeGranularity>('granularity', DEFAULT_GRANULARITY, {
-  permanent: props.permalink,
-})
+const selectedGranularity = usePermalink<ChartTimeGranularity>('granularity', DEFAULT_GRANULARITY, {
+  permanent: permalinkEnabled,
+})

-const startDate = usePermalink<string>('start', '', {
-  permanent: props.permalink,
-})
+const startDate = usePermalink<string>('start', '', {
+  permanent: permalinkEnabled,
+})
-const endDate = usePermalink<string>('end', '', {
-  permanent: props.permalink,
-})
+const endDate = usePermalink<string>('end', '', {
+  permanent: permalinkEnabled,
+})

-const selectedMetric = usePermalink<MetricId>('facet', DEFAULT_METRIC_ID, {
-  permanent: props.permalink,
-})
+const selectedMetric = usePermalink<MetricId>('facet', DEFAULT_METRIC_ID, {
+  permanent: permalinkEnabled,
+})

Also applies to: 586-588

@danielroe danielroe added this pull request to the merge queue Feb 10, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Feb 11, 2026
@vinnymac
Copy link
Contributor

Now I want our permalinks to produce opengraph images of the charts 😈

@ghostdevv ghostdevv enabled auto-merge February 11, 2026 03:54
@ghostdevv ghostdevv added this pull request to the merge queue Feb 11, 2026
Merged via the queue into npmx-dev:main with commit 54e08d0 Feb 11, 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.

Convert ChartModal to a modal route

5 participants