Cleanup OsdUrlStateStorage subscription in TopNav #9167

Maosaic · 2025-01-09T21:43:32Z

Description

OsdUrlStateStorage subscription in TopNav is not properly cleaned up, which is causing ScopedHistory throwing error and potential memory leak.

Issues Resolved

-fixes #5476

Screenshot

Before

Screen.Recording.2025-01-09.at.1.19.16.PM.mp4

After

Screen.Recording.2025-01-09.at.1.13.41.PM.mp4

Testing the changes

Changelog

fix: Cleanup OsdUrlStateStorage subscription in 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

github-actions · 2025-01-09T21:44:05Z

❌ Invalid Prefix For Manual Changeset Creation

Invalid description prefix. Found "fix". Only "skip" entry option is permitted for manual commit of changeset files.

If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description.

angle943 · 2025-01-09T22:00:28Z

src/plugins/data/public/query/state_sync/connect_to_query_state.ts

@@ -49,7 +49,7 @@ import { validateTimeRange } from '../timefilter';
 * @param  OsdUrlStateStorage to use for syncing and store data
 * @param syncConfig app filter and query
 */
-export const connectStorageToQueryState = async (
+export const connectStorageToQueryState = (


will this function ever return anything other than the unsub callback function, besides it reaching the catch block? Several questions regarding that:

Based on the call pattern that you provided on the usage of the returning callback function, can't we just unsub from the end of this function instead of wrapping it with a useEffect by the caller?

If some subs were made and then getting to the catch block for some error, don't we want to unsub from the catch as well?

I think the intended behaviour hear is let the scope where invokes connectStorageToQueryState to maintain the life cycle of the subscription.

It make senses to me to unsub in the catch block, but currently the subscription is building at the end of the try block, it's unlikely to have any lingering subscription if it reach catch block.

d-rowe · 2025-01-09T22:45:16Z

src/plugins/discover/public/application/view_components/canvas/top_nav.tsx

@@ -65,7 +65,7 @@ export const TopNav = ({ opts, showSaveQuery, isEnhancementsEnabled }: TopNavPro
    ? getTopNavLinks(services, inspectorAdapters, savedSearch, isEnhancementsEnabled)
    : [];

-  connectStorageToQueryState(
+  const unSub = connectStorageToQueryState(


Change LGTM. One thing (I know it's outside of the scope of this PR) I wanted to discuss is that we're relying on folk's best intention and understanding of connectStorageToQuery to unsubscribe and avoid these memory leaks.

It may be nice to introduce a hook or HOC wrapper, which subscribes on mount and unsubscribes on unmount to abstract the subscription from the consumer.

+1 Good idea, any suggestion which folder to put this hook in?

Thanks! How about https://github.com/opensearch-project/OpenSearch-Dashboards/tree/32b0602cac0b0a5879d56b48e7d8587362e3b5eb/src/plugins/discover/public/application/view_components/utils?

Agree with that, should we move the connectStorageToQueryState into a useEffect? and we can unsubscribe the change inside that useEffect hook as well.

I have created the hook under same folder of plugins/data, since it's been using from src/plugins/discover/... and src/plugins/vis_builder/...

codecov · 2025-01-10T01:45:16Z

Codecov Report

Attention: Patch coverage is 9.09091% with 10 lines in your changes missing coverage. Please review.

Project coverage is 61.01%. Comparing base (82689bb) to head (8b6c70f).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...lic/application/view_components/canvas/top_nav.tsx	0.00%	5 Missing ⚠️
..._builder/public/application/components/top_nav.tsx	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9167      +/-   ##
==========================================
- Coverage   61.01%   61.01%   -0.01%     
==========================================
  Files        3812     3812              
  Lines       91385    91393       +8     
  Branches    14438    14440       +2     
==========================================
+ Hits        55762    55763       +1     
- Misses      32065    32073       +8     
+ Partials     3558     3557       -1

Flag	Coverage Δ
Linux_1	`29.07% <9.09%> (-0.01%)`	⬇️
Linux_2	`56.45% <ø> (ø)`
Linux_3	`38.03% <16.66%> (-0.01%)`	⬇️
Linux_4	`29.03% <16.66%> (-0.01%)`	⬇️
Windows_1	`29.08% <9.09%> (-0.01%)`	⬇️
Windows_2	`56.40% <ø> (ø)`
Windows_3	`38.03% <16.66%> (+<0.01%)`	⬆️
Windows_4	`29.03% <16.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.

kavilla · 2025-01-10T18:43:08Z

do you mind signing the commit on push?

kavilla · 2025-01-10T18:44:41Z

do you have the changelog bot setup i think we can have a changelog for this

src/plugins/data/public/query/state_sync/connect_to_query_state.ts

kavilla · 2025-01-10T22:42:06Z

src/plugins/data/public/query/state_sync/use_connect_to_query_state.ts

question:

do you think we should put this in a lib folder within the state_sync folder or would that be just too messy? i see some folders have it but i dont think there is a consistency within the project.

kavilla · 2025-01-10T22:42:37Z

src/plugins/data/public/query/state_sync/connect_to_query_state.ts

-    query: boolean;
-    dataSet?: boolean;
-  },
-  uiSettings?: CoreStart['uiSettings']


what about uiSettings?

It's a unused variable

kavilla · 2025-01-10T22:44:49Z

src/plugins/vis_builder/public/application/components/top_nav.tsx

  const { selected: indexPattern } = useIndexPatterns();
  const [config, setConfig] = useState<TopNavMenuData[] | undefined>();
  const originatingApp = useTypedSelector((state) => {
    return state.metadata.originatingApp;
  });

+  useConnectStorageToQueryState(services.data.query, services.osdUrlStateStorage, {


without diving too deep, do we know if the query state is changed and navigate to visbuilder if this gets set to after the the index patterns does it cause a difference in the selected: indexPattern?

…in TopNav Update src/plugins/data/public/query/state_sync/connect_to_query_state.ts Co-authored-by: Kawika Avilla <[email protected]> Signed-off-by: Joey Liu <[email protected]> Changeset file for PR opensearch-project#9167 created/updated

github-actions bot added the first-time-contributor label Jan 9, 2025

github-actions bot added failed changeset Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry and removed failed changeset labels Jan 9, 2025

angle943 reviewed Jan 9, 2025

View reviewed changes

d-rowe reviewed Jan 9, 2025

View reviewed changes

kavilla added the discover for discover reinvent label Jan 10, 2025

kavilla added the backport 2.x label Jan 10, 2025

Maosaic force-pushed the main branch from 6384a79 to 376b0c2 Compare January 10, 2025 20:20

kavilla reviewed Jan 10, 2025

View reviewed changes

src/plugins/data/public/query/state_sync/connect_to_query_state.ts Outdated Show resolved Hide resolved

kavilla reviewed Jan 10, 2025

View reviewed changes

github-actions bot removed the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Jan 10, 2025

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

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

2abbac0

Maosaic force-pushed the main branch from 2abbac0 to 5745e86 Compare January 10, 2025 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup OsdUrlStateStorage subscription in TopNav #9167

Cleanup OsdUrlStateStorage subscription in TopNav #9167

Maosaic commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025

angle943 Jan 9, 2025

Maosaic Jan 9, 2025 •

edited

Loading

d-rowe Jan 9, 2025

Maosaic Jan 9, 2025

d-rowe Jan 9, 2025

SuZhou-Joe Jan 10, 2025

Maosaic Jan 10, 2025

codecov bot commented Jan 10, 2025 •

edited

Loading

kavilla commented Jan 10, 2025

kavilla commented Jan 10, 2025

kavilla Jan 10, 2025

kavilla Jan 10, 2025

Maosaic Jan 10, 2025

kavilla Jan 10, 2025

Cleanup OsdUrlStateStorage subscription in TopNav #9167

Are you sure you want to change the base?

Cleanup OsdUrlStateStorage subscription in TopNav #9167

Conversation

Maosaic commented Jan 9, 2025 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

github-actions bot commented Jan 9, 2025

❌ Invalid Prefix For Manual Changeset Creation

Choose a reason for hiding this comment

Maosaic Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

kavilla commented Jan 10, 2025

kavilla commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maosaic commented Jan 9, 2025 •

edited

Loading

Maosaic Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading