-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix: emit sync events when clicking on region annotations #8604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
fix: emit sync events when clicking on region annotations #8604
Conversation
When clicking on region annotations (either directly on the plot or in the Regions sidebar/outliner), the view would scroll to show the region but would NOT synchronize with other synced media components. This broke multi-modal data exploration workflows where users expect all synchronized components (Audio, TimeSeries, etc.) to seek to the same time position. For example, clicking on a TimeSeries region labeled "Walking" would only scroll the TimeSeries view but would not seek the Audio player or other synced TimeSeries to that time. Changes: - Modified TimeSeriesRegion.selectRegion() to emit sync events after scrolling to the region - Added setCursor() call to update the cursor/playhead position - Added syncSend() call with the region's start time to notify all synced components in the same sync group - Handles both date-formatted and numeric time columns properly - Applies to both direct region clicks and Regions sidebar/outliner clicks Benefits: - Clicking any region (plot or sidebar) now syncs all components - Consistent behavior between clicking empty plot vs clicking regions - Improved multi-modal data exploration and navigation - Users can now easily navigate synchronized data by clicking annotations Impact: - Affects all TimeSeries annotation tasks with synchronized media - Critical for multi-sensor data workflows (IoT, wearables, autonomous vehicles) - Requires FF_TIMESERIES_SYNC feature flag to be enabled Bug demonstration: https://www.loom.com/share/2584716884114820a7e4b5b76a3316b3 Fix demonstration: https://www.loom.com/share/9a6421fca42d424bb068779d9a4d7021 Files changed: - web/libs/editor/src/regions/TimeSeriesRegion.js
👷 Deploy request for heartex-docs pending review.Visit the deploys page to approve it
|
👷 Deploy request for label-studio-docs-new-theme 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. |
| "on-headers": "1.1.0", | ||
| "form-data": "4.0.4", | ||
| "stylus": "0.0.1-security" | ||
| "stylus": "0.64.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems unrelated - was that intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was done so I could build it, "stylus": "0.0.1-security" was not being resolved, not sure if its just me or not.
If its me, we can simply revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the whole override can probably be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch, @cloudmark! I think it's best to take it from this PR to avoid mixing concerns.
Let's review this separately @yyassi-heartex, as @nass600 said this can probably be removed since it's coming from the npm incident.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cloudmark we discussed internally, please leave the stylus version unchanged - we'll be creating a PR internally to remove it anyways
|
/jira create
|
|
you let me know what you want to do, if you want to remove Ill remove,
alternatively feel free to remove it while you merge internally
…On Tue, Oct 21, 2025 at 5:47 PM Raul Martin ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In web/package.json
<#8604 (comment)>
:
> @@ -249,7 +249,7 @@
"moment": "2.29.4",
"on-headers": "1.1.0",
"form-data": "4.0.4",
- "stylus": "0.0.1-security"
+ "stylus": "0.64.0"
Great catch, @cloudmark <https://github.com/cloudmark>! I think it's best
to take it from this PR to avoid mixing concerns.
Let's review this separately @yyassi-heartex
<https://github.com/yyassi-heartex>, as @nass600
<https://github.com/nass600> said this can probably be removed since it's
coming from the npm incident.
—
Reply to this email directly, view it on GitHub
<#8604 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANKHJSCA6WHWV5CH37P6SL3YZIQ7AVCNFSM6AAAAACITM5YR2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGNRRGQ4DAMRZGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Here's a comprehensive PR description for the region synchronization fix:
PR Description
Reason for Change
Problem:
When clicking on region annotations (either directly on the plot or in the Regions sidebar/outliner), the view scrolls to show the region but does NOT synchronize with other synced media components. This breaks multi-modal data exploration workflows where users expect all synchronized components (Audio, TimeSeries, etc.) to seek to the same time position.
For example:
Root Cause:
The
selectRegion()method inTimeSeriesRegion.jsonly callsscrollToRegion()which adjusts the view but does not:setCursor())syncSend()) to notify other componentsThis is inconsistent with how clicking directly on empty plot areas works, which DOES emit sync events.
Solution:
Modified
TimeSeriesRegion.selectRegion()to emit sync events after scrolling to the region. The method now:setCursor()syncSend()to notify all components in the same sync groupVideos
Bug demonstration: https://www.loom.com/share/2584716884114820a7e4b5b76a3316b3
Fix demonstration: https://www.loom.com/share/9a6421fca42d424bb068779d9a4d7021
Rollout Strategy
No special rollout needed - This fix is gated by existing feature flag:
FF_TIMESERIES_SYNCfeature flag to be enabledTesting
Manual Testing Performed:
Basic Region Click Test
Regions Sidebar Test
Edge Cases
Interaction Consistency
Multi-Component Synchronization
Code Quality:
setCursor()andsyncSend()methodsRisks
Risk Level: LOW
Performance:
Backward Compatibility:
FF_TIMESERIES_SYNCis enabledSecurity:
User Experience:
Reviewer Notes
Key Points to Review:
Implementation Location (
web/libs/editor/src/regions/TimeSeriesRegion.js)selectRegion()method (lines ~72-93)Code Logic:
Consistency with Existing Code:
handleMainAreaClick()andplotClickHandler()setCursor()andsyncSend()methodsTesting the Fix:
General Notes
Implementation Details:
The fix adds synchronization logic to
TimeSeriesRegion.selectRegion():Time Conversion:
Feature Flag:
FF_TIMESERIES_SYNCfeature flagBenefits:
Files Changed:
web/libs/editor/src/regions/TimeSeriesRegion.js- Added sync logic toselectRegion()No Migration Required - This is a transparent bug fix that improves behavior without requiring any user action or configuration changes.
Related Context
Related to Issue: #8603