[Discover] fix: Clean up sync URL subscription in Discover plugin topNav #9316

Maosaic · 2025-01-31T18:47:05Z

Description

Properly clean up sync URL subscription in Discover plugin topNav component.

Changes this PR contains:

Create useSyncQueryStateWithUrl hook which automatically cancel sync URL subscription.
Invoke useSyncQueryStateWithUrl from topNav component, so the URL subscription will be properly clean up when user navigate away from Discover page.
~~Rewrite SavedObjectSaveModal as functional component.~~

Issues Resolved

fixes #9309

Screenshot

Before

Screen.Recording.2025-01-29.at.2.56.41.PM.mp4

After

Screen.Recording.2025-01-31.at.10.43.38.AM.mp4

Testing the changes

Changelog

fix: Clean up sync URL subscription in Discover plugin topNav

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

codecov · 2025-01-31T19:02:36Z

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 61.71%. Comparing base (ec6d7df) to head (2a0bfad).
Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
...plication/components/top_nav/get_top_nav_links.tsx	0.00%	3 Missing ⚠️
...lic/application/view_components/canvas/top_nav.tsx	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9316   +/-   ##
=======================================
  Coverage   61.70%   61.71%           
=======================================
  Files        3816     3817    +1     
  Lines       91829    91841   +12     
  Branches    14543    14545    +2     
=======================================
+ Hits        56666    56677   +11     
- Misses      31507    31508    +1     
  Partials     3656     3656

Flag	Coverage Δ
Linux_1	`28.98% <6.66%> (-0.01%)`	⬇️
Linux_2	`56.46% <ø> (ø)`
Linux_3	`39.19% <73.33%> (+0.01%)`	⬆️
Linux_4	`28.90% <6.66%> (-0.01%)`	⬇️
Windows_1	`29.00% <6.66%> (-0.01%)`	⬇️
Windows_2	`56.41% <ø> (ø)`
Windows_3	`39.19% <73.33%> (+0.01%)`	⬆️
Windows_4	`28.90% <6.66%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/plugins/data/public/query/state_sync/use_sync_state_with_url.test.ts

src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx

Signed-off-by: Joey Liu <[email protected]>

kavilla · 2025-02-06T00:09:57Z

do you mind checking the ci failures? is the ci failing on main?

Maosaic · 2025-02-06T18:35:50Z

do you mind checking the ci failures? is the ci failing on main?

I see the same test error in other PR, I don't think that's related to my change

abbyhu2000 · 2025-02-12T19:34:59Z

@Maosaic one comment but not blocking.

I see we use useSyncQueryStateWithUrl to wrap syncQueryStateWithUrl for discover plugin.

Other plugins also uses syncQueryStateWithUrl
*dashboard

OpenSearch-Dashboards/src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx

Line 131 in 4da33a1

const { stop: stopSyncingQueryServiceStateWithUrl } = syncQueryStateWithUrl(

OpenSearch-Dashboards/src/plugins/dashboard/public/application/components/dashboard_listing/dashboard_listing.tsx

Line 51 in 4da33a1

const { stop } = syncQueryStateWithUrl(query, osdUrlStateStorage);

*visbuilder

OpenSearch-Dashboards/src/plugins/vis_builder/public/application/app.tsx

Line 45 in 4da33a1

const { stop } = syncQueryStateWithUrl(query, osdUrlStateStorage);

*visualization

OpenSearch-Dashboards/src/plugins/visualize/public/application/app.tsx

Line 63 in 4da33a1

const { stop } = syncQueryStateWithUrl(query, osdUrlStateStorage);

should we also replace those with useSyncQueryStateWithUrl as well?

…Nav (#9316) * [Discover] fix: Clean up sync URL subscription in Discover plugin topNav Signed-off-by: Joey Liu <[email protected]> * Changeset file for PR #9316 created/updated * Update unit test Signed-off-by: Joey Liu <[email protected]> * Revert save modal change Signed-off-by: Joey Liu <[email protected]> --------- Signed-off-by: Joey Liu <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 8cc5f84) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Maosaic · 2025-02-13T02:12:23Z

@Maosaic one comment but not blocking.

I see we use useSyncQueryStateWithUrl to wrap syncQueryStateWithUrl for discover plugin.

Other plugins also uses syncQueryStateWithUrl *dashboard

OpenSearch-Dashboards/src/plugins/dashboard/public/application/utils/create_dashboard_app_state.tsx

Line 131 in 4da33a1

const { stop: stopSyncingQueryServiceStateWithUrl } = syncQueryStateWithUrl(

OpenSearch-Dashboards/src/plugins/dashboard/public/application/components/dashboard_listing/dashboard_listing.tsx

Line 51 in 4da33a1

const { stop } = syncQueryStateWithUrl(query, osdUrlStateStorage);

*visbuilder

OpenSearch-Dashboards/src/plugins/vis_builder/public/application/app.tsx

Line 45 in 4da33a1

const { stop } = syncQueryStateWithUrl(query, osdUrlStateStorage);

*visualization

OpenSearch-Dashboards/src/plugins/visualize/public/application/app.tsx

Line 63 in 4da33a1

const { stop } = syncQueryStateWithUrl(query, osdUrlStateStorage);

should we also replace those with useSyncQueryStateWithUrl as well?

We can, but not necessary, those are manually calling stop

github-actions bot added the repeat-contributor label Jan 31, 2025

opensearch-changeset-bot bot added a commit to Maosaic/OpenSearch-Dashboards that referenced this pull request Jan 31, 2025

Changeset file for PR opensearch-project#9316 created/updated

9adae0f

ashwin-pc reviewed Feb 4, 2025

View reviewed changes

src/plugins/data/public/query/state_sync/use_sync_state_with_url.test.ts Show resolved Hide resolved

kavilla added backport 2.x discover for discover reinvent labels Feb 5, 2025

kavilla reviewed Feb 5, 2025

View reviewed changes

src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx Outdated Show resolved Hide resolved

Maosaic and others added 4 commits February 5, 2025 21:58

[Discover] fix: Clean up sync URL subscription in Discover plugin topNav

60dd66e

Signed-off-by: Joey Liu <[email protected]>

Changeset file for PR opensearch-project#9316 created/updated

12c1957

Update unit test

39f4f2c

Signed-off-by: Joey Liu <[email protected]>

Revert save modal change

2a0bfad

Signed-off-by: Joey Liu <[email protected]>

Maosaic force-pushed the topnav branch from 4592edd to 2a0bfad Compare February 5, 2025 22:42

kavilla approved these changes Feb 6, 2025

View reviewed changes

kavilla assigned kavilla and abbyhu2000 Feb 6, 2025

abbyhu2000 approved these changes Feb 12, 2025

View reviewed changes

abbyhu2000 merged commit 8cc5f84 into opensearch-project:main Feb 12, 2025
69 of 71 checks passed

opensearch-trigger-bot bot mentioned this pull request Feb 12, 2025

[Backport 2.x] [Discover] fix: Clean up sync URL subscription in Discover plugin topNav #9376

Open

Maosaic deleted the topnav branch February 13, 2025 02:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] fix: Clean up sync URL subscription in Discover plugin topNav #9316

[Discover] fix: Clean up sync URL subscription in Discover plugin topNav #9316

Maosaic commented Jan 31, 2025 •

edited

Loading

codecov bot commented Jan 31, 2025 •

edited

Loading

kavilla commented Feb 6, 2025

Maosaic commented Feb 6, 2025

abbyhu2000 commented Feb 12, 2025

Maosaic commented Feb 13, 2025

[Discover] fix: Clean up sync URL subscription in Discover plugin topNav #9316

[Discover] fix: Clean up sync URL subscription in Discover plugin topNav #9316

Conversation

Maosaic commented Jan 31, 2025 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Jan 31, 2025 • edited Loading

Codecov Report

kavilla commented Feb 6, 2025

Maosaic commented Feb 6, 2025

abbyhu2000 commented Feb 12, 2025

Maosaic commented Feb 13, 2025

Maosaic commented Jan 31, 2025 •

edited

Loading

codecov bot commented Jan 31, 2025 •

edited

Loading