Reduce flickering on pageload and plot controls changes#99
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRidgelinePlot component refines its "no data" display condition to require both empty selections and completion of data loading, removes immediate watcher execution, and expands test coverage with categorical-scale assertions and deterministic loading-state timing tests. ChangesRidgelinePlot component and tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## types #99 +/- ##
=======================================
Coverage 98.32% 98.32%
=======================================
Files 39 39
Lines 897 897
Branches 256 257 +1
=======================================
Hits 882 882
Misses 10 10
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
david-mears-2
force-pushed
the
fix-alert-on-pageload-2
branch
from
37ddc35 to
def2d48
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![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 `@src/components/RidgelinePlot.vue`:
- Line 146: The legend stays visible during data fetches because noDataToDisplay
is set false while loading; update the logic so loading counts as "no data" — in
RidgelinePlot.vue change the assignment for noDataToDisplay to include
dataStore.isLoading (e.g., set noDataToDisplay = selectedLines.length === 0 ||
dataStore.isLoading) so LegendContainer (which is rendered when
!noDataToDisplay) is hidden during fetches; ensure you update the reactive/noRef
usage consistent with existing selectedLines and dataStore access.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1fc6fea5-2f5b-4ebb-83a8-a442e8440f07
📒 Files selected for processing (2)
src/components/RidgelinePlot.vuetests/unit/components/RidgelinePlot.spec.ts
EmmaLRussell
left a comment
EmmaLRussell
left a comment
There was a problem hiding this comment.
I struggled to see any flickering with the deployed vaxviz from my desktop, but did see some issues from my laptop and when looking at vaxviz running locally (in dev mode, which may cause other issues). This definitely seems smoother!
So during all the time when loading is still going on, the loading spinner will be displayed rather than the momentary "No data" message..?
| }, | ||
| })); | ||
|
|
||
| // Assert that all diseases (the current data dimension for the plot-rows) are in the correct order, |
There was a problem hiding this comment.
Did you remove this comment because you've added some previously missing, similar assertions above? This could still be a useful comments, so could move and edit it to the first such assertion.
There was a problem hiding this comment.
It duplicates a comment at the definition of the function called here.
Yes |
2 participants
To see what I mean by flickering, go to https://vaxviz.vaccineimpact.org/ and click 'disable cache' in developer tools. Then pay attention to the plot area when you (a) initially load/reload the page (you'll likely see the 'no data' alert flash up for a fraction of a second) and (b) when you first switch focus from 'Geography' to 'Disease' (you might see that alert again, and also, what's worse, an updated version of the plot that has only one plot row, which is then instantly replaced with a correct plot).
The degree of noticeability of flicker probably depends on your own computer's speed.
I reduce the flickering of plot updates in two ways in this PR.
First: the commit Don't show 'no data' alert if data is still loading. We had needlessly been displaying the 'no data' alert for fractions of a second while data was still loading. There is already a dataStore.isLoading flag we can trivially look at to prevent his.
Secondly: the commit Increase debounce duration to reduce flickering papers over the rapid plot updates. 25ms was not enough, since the
constructLinesfunction can sometimes take up to around 100ms (at least on my laptop).A general solution to the issue would involve trying to make sure that individual changes to the plot controls ultimately trigger only one call to constructLines and only one call to updateChart (removing the need for debouncing). But this would be a deep refactor that isn't currently justified given we don't know how much future development vaxviz will see. https://mrc-ide.myjetbrains.com/youtrack/issue/mrc-6857/Reduce-number-of-watchers