refactor: useColors composable for charts#2696
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors color token resolution: removes configurable useCssVariables and introduces a fixed-set useColors composable. Seven chart components are migrated to useColors. Old tests removed and new unit tests added to cover useColors behaviour across client/server scenarios. ChangesuseColors Composable Refactoring
Sequence Diagram(s)sequenceDiagram
participant Watcher as selectedGranularity/startDate/endDate watcher
participant Reset as resetZoom()
participant Chart as chartRef
Watcher->>Reset: trigger resetZoom()
Reset->>Reset: set keepZoomState = false
Reset->>Chart: chartRef.resetZoom()
Reset->>Reset: restore keepZoomState = true (delayed)
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/composables/useColors.ts`:
- Line 1: The computed color map in useColors.ts only depends on the element ref
so DOM mutations (CSS variable changes) don't invalidate the cache; add a
Ref<number> called recomputeToken, include recomputeToken in the computed
getter's dependencies, and increment recomputeToken.value inside the observer
callbacks (the ones that currently read colors.value) so that changes from
watchHtmlAttributes or watchResize force recomputation; update references to
colors.value usage to ensure the callbacks still read colors if needed but also
trigger recomputeToken increments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64b19e40-ed13-4364-91e4-b5aa8f1c3aac
📒 Files selected for processing (10)
app/components/Chart/SplitSparkline.vueapp/components/Compare/FacetBarChart.vueapp/components/Compare/FacetScatterChart.vueapp/components/Package/TimelineChart.vueapp/components/Package/TrendsChart.vueapp/components/Package/VersionDistribution.vueapp/components/Package/WeeklyDownloadStats.vueapp/composables/useColors.tstest/nuxt/composables/use-colors.spec.tstest/unit/app/composables/use-colors.spec.ts
💤 Files with no reviewable changes (1)
- test/nuxt/composables/use-colors.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unit/app/composables/use-colors.spec.ts`:
- Around line 41-42: The test accesses
useResizeObserverMock.mock.calls?.[0]?.[1] as resizeCallback and invokes it
without asserting it exists; add an explicit check for the callback before
calling it (e.g., assert/expect that resizeCallback is defined/not null) and
only then invoke resizeCallback() to ensure a clear failure message if the mock
didn't record a second argument; update the test around the resizeCallback
retrieval to perform this existence assertion for useResizeObserverMock and
resizeCallback before the expect(() => resizeCallback()).not.toThrow() call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e51c5ff1-52bd-4ed2-889e-08dff4b15751
📒 Files selected for processing (2)
app/composables/useColors.tstest/unit/app/composables/use-colors.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/composables/useColors.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/components/Package/TrendsChart.vue`:
- Around line 1650-1653: The current watcher on normalisedDataset causes zoom
resets for any dataset change (filters, smoothing, prediction) rather than only
when the visible date range or granularity changes; change the watcher to
observe the specific reactive sources that represent the chart's date range and
granularity (e.g., startDate/endDate or dateRange and selectedGranularity)
instead of normalisedDataset, keep the isMounted.value check and await
resetZoom() inside the new watcher, and remove the old
watch([normalisedDataset], ...) so zoom is only reset when those
date/granularity refs change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 669f9c49-2af2-492f-b12d-3ab416b17e04
📒 Files selected for processing (1)
app/components/Package/TrendsChart.vue
graphieros
added
the
needs review
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/Package/TrendsChart.vue (1)
1631-1646: 💤 Low valueConfusing alias and a likely-redundant mount-time
resetZoom.Two small clarity/efficiency points on this block:
The alias
resetZoomStateis thestart()of auseTimeoutFnwhose callback restoreskeepZoomStatetotrue. Read against the siblingresetZoom()(which actively resets chart zoom), the call siteresetZoomState()reads as if it were resetting the zoom state, when it's actually scheduling the restoration of the keep-state flag. A name likescheduleKeepZoomStateRestore(orrestoreKeepZoomStateLater) would match its real effect.
onMounted(resetZoom)runs at first mount where there is no user zoom to clear, so thechartRef.value?.resetZoom?.()call is effectively a no-op, but it still toggleskeepZoomStatetofalsefor ~1s after mount and forces achartConfigrecomputation. The watcher on[selectedGranularity, startDate, endDate]already covers every subsequent reset case. Consider dropping the mount-time call, or gating it behind a "have we already rendered with zoom" check.♻️ Proposed naming + dropping the mount-time call
-const { start: resetZoomState } = useTimeoutFn( +const { start: scheduleKeepZoomStateRestore } = useTimeoutFn( () => { keepZoomState.value = true }, 1000, { immediate: false }, ) async function resetZoom() { keepZoomState.value = false await nextTick() chartRef.value?.resetZoom?.() - resetZoomState() + scheduleKeepZoomStateRestore() } -onMounted(resetZoom) - watch([selectedGranularity, startDate, endDate], async () => { if (!isMounted.value) return await resetZoom() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/Package/TrendsChart.vue` around lines 1631 - 1646, Rename the confusing alias from the useTimeoutFn start import (currently assigned to resetZoomState) to something like scheduleKeepZoomStateRestore and update the resetZoom() function to call that new name; then remove the onMounted(resetZoom) call (or gate it so resetZoom only runs when there is an actual user zoom) so we no longer toggle keepZoomState and force chartConfig recomputation on initial mount; the relevant symbols to change are useTimeoutFn, the alias currently named resetZoomState, the resetZoom() function, keepZoomState, chartRef, and the onMounted call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/components/Package/TrendsChart.vue`:
- Around line 1631-1646: Rename the confusing alias from the useTimeoutFn start
import (currently assigned to resetZoomState) to something like
scheduleKeepZoomStateRestore and update the resetZoom() function to call that
new name; then remove the onMounted(resetZoom) call (or gate it so resetZoom
only runs when there is an actual user zoom) so we no longer toggle
keepZoomState and force chartConfig recomputation on initial mount; the relevant
symbols to change are useTimeoutFn, the alias currently named resetZoomState,
the resetZoom() function, keepZoomState, chartRef, and the onMounted call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af57ec4d-e19b-4dc7-9de0-706866886671
📒 Files selected for processing (1)
app/components/Package/TrendsChart.vue
2 participants
This refactors the
useColorscomposable to remove code repetition in chart components by placing used css variables directly into the composable. Also adds a test for the composable.While I was at it, I also fixed another regression that persisted the state of the zoom when granularity or date ranges are changed in download charts. While keeping the zoom state is necessary when mutating the tooltip position dynamically, it must however be refreshed when the number of datapoints changes. To test this: