fix: BROS-128: Correct multi-channel rendering and dynamic scaling #7790
+83
−120
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
👷 Deploy request for label-studio-docs-new-theme pending review.Visit the deploys page to approve it
|
👷 Deploy request for heartex-docs pending review.Visit the deploys page to approve it
|
✅ Deploy Preview for label-studio-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for label-studio-playground ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
hlomzik
reviewed
Jun 19, 2025
| @@ -910,7 +884,15 @@ class TimeSeriesVisualizerD3 extends React.Component { | |||
| } | |||
| } | |||
|
|
|||
| this.renderBrushes(this.props.ranges, flushBrushes); | |||
| // Batch brush rendering to prevent visual artifacts | |||
There was a problem hiding this comment.
Do you expect multiple renders here? Or what is the problem here?
cloudmark
force-pushed
the
fix/7789-multichannel-timeseries-fix
branch
2 times, most recently
from
cb0cfac to
c5e11b0
Compare
- Remove complex optimization code (useOptimizedData, optimizedSeries, dual paths) - Fix zoom to use updateTR() instead of throttledRangeUpdate() for immediate response - Add MobX reactions for instant overview brush and channel visibility updates - Simplify rendering to always use lineSlice with single path approach - Fix initial waveform rendering by using channel.line instead of this.line - Consolidate all reactive setup in componentDidMount for better organization Resolves issues with delayed updates during zoom/pan operations and overview brush changes.
cloudmark
force-pushed
the
fix/7789-multichannel-timeseries-fix
branch
from
c5e11b0 to
8ed311b
Compare
|
Hey Andrew (@hlomzik) I cleaned up and fixed all issues, here is the before and after Before: New: What was broken:
What was fixed:
The TimeSeriesVisualizer now responds immediately to all user interactions and provides smooth, reliable performance! I would recommend that we review this rather soon because the change is currently on develop and anyone using the timeseries is suffering from this bug. |
makseq
changed the title
This was referenced Jun 20, 2025
|
Closing @Gondragos did a better fix, which keeps all the optimisation. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses and fixes several critical bugs within the
<TimeSeries>component, enabling the correct rendering and dynamic scaling of multi-channel charts, particularly with large datasets. The changes ensure that the component is robust, performant, and behaves as expected when handling complex time series visualizations.This solves issue #7789.
Reason for change
The
<TimeSeries>component was failing to render data correctly when using a<MultiChannel>configuration or themultiaxis="true"attribute. The primary symptoms were a blank main chart view despite the overview scrubber being populated, and a Y-axis that would not scale dynamically on zoom.This was caused by three distinct bugs:
this.lineinstead ofchannel.line) inTimeSeriesVisualizer.jsxprevented the SVG path for channels from ever being drawn.Channel.jsxcomponent was not correctly activating theTimeSeriesVisualizerfor multi-channel views, leading to multiple, conflicting chart instances being rendered. The logic is now corrected to render a single, unified visualizer.Video
Before:
The main chart area is blank and does not render the channel data. The Y-axis is not scaled correctly.
https://www.loom.com/share/50fc240f1bae4d72ad421bdb9c2c8dfa
After:
All channels are rendered correctly in a single, unified chart. The Y-axis now scales dynamically and accurately as the user zooms and pans through the data.
https://www.loom.com/share/7d50d09befce4d19ad3da847ca2d6be7
Rollout strategy
These changes are bug fixes to core functionality and can be rolled out directly. There is no feature flag associated with this fix.
Testing
Verification was performed by:
Risks
There are no known security or performance risks associated with this change. The fixes improve rendering performance and correctness for the targeted use case. The changes are scoped to the
TimeSeriescomponent and do not affect other parts of the application.Reviewer notes
The core of the fix lies in three places:
TimeSeriesVisualizer.jsx'sinitializeChannelmethod.TimeSeriesVisualizer.jsx'ssetChannelRangeWithScalingmethod.Channel.jsx'sHtxChannelViewD3function, which now properly usesTimeSeriesVisualizerwhenmultiaxis="true".Please review these three areas to confirm the logic is sound and correctly addresses the issues outlined in the linked ticket.