From 8ca81674670b5845841e02b037945088cf7275aa Mon Sep 17 00:00:00 2001 From: SuZhou-Joe Date: Fri, 1 Nov 2024 12:22:09 +0800 Subject: [PATCH 1/5] fix: staled closure inside chrome_service Signed-off-by: SuZhou-Joe --- src/core/public/chrome/chrome_service.test.ts | 35 ++++++++++++++++++- src/core/public/chrome/chrome_service.tsx | 24 ++++++++++--- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/src/core/public/chrome/chrome_service.test.ts b/src/core/public/chrome/chrome_service.test.ts index 082ffbfa16e..25fb9d35b54 100644 --- a/src/core/public/chrome/chrome_service.test.ts +++ b/src/core/public/chrome/chrome_service.test.ts @@ -46,6 +46,7 @@ import { HeaderVariant } from './constants'; class FakeApp implements App { public title: string; + public appRoute: string; public mount = () => () => {}; constructor( @@ -54,6 +55,7 @@ class FakeApp implements App { public headerVariant?: HeaderVariant ) { this.title = `${this.id} App`; + this.appRoute = this.id; } } const store = new Map(); @@ -78,12 +80,18 @@ function defaultStartDeps(availableApps?: App[]) { uiSettings: uiSettingsServiceMock.createStartContract(), overlays: overlayServiceMock.createStartContract(), workspaces: workspacesServiceMock.createStartContract(), + updateApplications: (() => {}) as (applications?: App[]) => void, }; if (availableApps) { - deps.application.applications$ = new Rx.BehaviorSubject>( + const applications$ = new Rx.BehaviorSubject>( new Map(availableApps.map((app) => [app.id, getAppInfo(app) as PublicAppInfo])) ); + deps.application.applications$ = applications$; + deps.updateApplications = (applications?: App[]) => + applications$.next( + new Map(applications?.map((app) => [app.id, getAppInfo(app) as PublicAppInfo])) + ); } return deps; @@ -285,6 +293,31 @@ describe('start', () => { ] `); }); + + it('should use correct current app id to tell if hidden', async () => { + const apps = [new FakeApp('alpha', true), new FakeApp('beta', false)]; + const startDeps = defaultStartDeps(apps); + const { navigateToApp } = startDeps.application; + const { chrome } = await start({ startDeps }); + const visibleChangedArray: boolean[] = []; + const visible$ = chrome.getIsVisible$(); + visible$.subscribe((visible) => visibleChangedArray.push(visible)); + + await navigateToApp('alpha'); + + await navigateToApp('beta'); + startDeps.updateApplications(apps); + + expect(visibleChangedArray).toMatchInlineSnapshot(` + Array [ + false, + false, + true, + true, + true, + ] + `); + }); }); describe('header variant', () => { diff --git a/src/core/public/chrome/chrome_service.tsx b/src/core/public/chrome/chrome_service.tsx index 39f9f0fbc32..10613b9a2f0 100644 --- a/src/core/public/chrome/chrome_service.tsx +++ b/src/core/public/chrome/chrome_service.tsx @@ -160,18 +160,32 @@ export class ChromeService { const isEmbedded = new URL(location.hash.slice(1), location.origin).searchParams.has('embed'); this.isForceHidden$ = new BehaviorSubject(isEmbedded); + /** + * There is a staled closure issue here. + * For example, when currentAppId$ is going through A -> B -> C and + * the application.applications$ just get changed in B, then it will always use B as the currentAppId + * even though the latest appId now is C. + */ + let currentAppId: string | undefined; + const appHidden$ = merge( // For the isVisible$ logic, having no mounted app is equivalent to having a hidden app // in the sense that the chrome UI should not be displayed until a non-chromeless app is mounting or mounted of(true), application.currentAppId$.pipe( - flatMap((appId) => - application.applications$.pipe( + flatMap((appId) => { + // Update the currentAppId to latest + currentAppId = appId; + return application.applications$.pipe( map((applications) => { - return !!appId && applications.has(appId) && !!applications.get(appId)!.chromeless; + return ( + !!currentAppId && + applications.has(currentAppId) && + !!applications.get(currentAppId)!.chromeless + ); }) - ) - ) + ); + }) ) ); this.isVisible$ = combineLatest([appHidden$, this.isForceHidden$]).pipe( From 2503d6de380c540e96f8d913ff70565c44c54d91 Mon Sep 17 00:00:00 2001 From: "opensearch-changeset-bot[bot]" <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Date: Fri, 1 Nov 2024 04:28:17 +0000 Subject: [PATCH 2/5] Changeset file for PR #8783 created/updated --- changelogs/fragments/8783.yml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelogs/fragments/8783.yml diff --git a/changelogs/fragments/8783.yml b/changelogs/fragments/8783.yml new file mode 100644 index 00000000000..0109a60191a --- /dev/null +++ b/changelogs/fragments/8783.yml @@ -0,0 +1,2 @@ +fix: +- Staled closure inside chrome_service ([#8783](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8783)) \ No newline at end of file From 88f485ae207ba63a448bcfa31e8a9e944981ccd9 Mon Sep 17 00:00:00 2001 From: SuZhou-Joe Date: Fri, 1 Nov 2024 17:44:02 +0800 Subject: [PATCH 3/5] feat: optimize code Signed-off-by: SuZhou-Joe --- src/core/public/chrome/chrome_service.test.ts | 1 - src/core/public/chrome/chrome_service.tsx | 39 ++++++++----------- 2 files changed, 16 insertions(+), 24 deletions(-) diff --git a/src/core/public/chrome/chrome_service.test.ts b/src/core/public/chrome/chrome_service.test.ts index 25fb9d35b54..e930aa8931c 100644 --- a/src/core/public/chrome/chrome_service.test.ts +++ b/src/core/public/chrome/chrome_service.test.ts @@ -314,7 +314,6 @@ describe('start', () => { false, true, true, - true, ] `); }); diff --git a/src/core/public/chrome/chrome_service.tsx b/src/core/public/chrome/chrome_service.tsx index 10613b9a2f0..b890fdfb880 100644 --- a/src/core/public/chrome/chrome_service.tsx +++ b/src/core/public/chrome/chrome_service.tsx @@ -41,7 +41,7 @@ import { ReplaySubject, Subscription, } from 'rxjs'; -import { flatMap, map, takeUntil } from 'rxjs/operators'; +import { map, switchMap, takeUntil } from 'rxjs/operators'; import { EuiLink } from '@elastic/eui'; import { mountReactNode } from '../utils/mount'; import { InternalApplicationStart } from '../application'; @@ -160,32 +160,25 @@ export class ChromeService { const isEmbedded = new URL(location.hash.slice(1), location.origin).searchParams.has('embed'); this.isForceHidden$ = new BehaviorSubject(isEmbedded); - /** - * There is a staled closure issue here. - * For example, when currentAppId$ is going through A -> B -> C and - * the application.applications$ just get changed in B, then it will always use B as the currentAppId - * even though the latest appId now is C. - */ - let currentAppId: string | undefined; - const appHidden$ = merge( // For the isVisible$ logic, having no mounted app is equivalent to having a hidden app // in the sense that the chrome UI should not be displayed until a non-chromeless app is mounting or mounted of(true), application.currentAppId$.pipe( - flatMap((appId) => { - // Update the currentAppId to latest - currentAppId = appId; - return application.applications$.pipe( - map((applications) => { - return ( - !!currentAppId && - applications.has(currentAppId) && - !!applications.get(currentAppId)!.chromeless - ); - }) - ); - }) + /** + * There is a staled closure issue here. + * For example, when currentAppId$ is going through A -> B -> C and + * the application.applications$ just get changed in B, then it will always use B as the currentAppId + * even though the latest appId now is C. + */ + switchMap((appId) => + application.applications$.pipe( + map( + (applications) => + !!appId && applications.has(appId) && !!applications.get(appId)!.chromeless + ) + ) + ) ) ); this.isVisible$ = combineLatest([appHidden$, this.isForceHidden$]).pipe( @@ -198,7 +191,7 @@ export class ChromeService { this.headerVariantOverride$ = new BehaviorSubject(undefined); const appHeaderVariant$ = application.currentAppId$.pipe( - flatMap((appId) => + switchMap((appId) => application.applications$.pipe( map( (applications) => From 25f4aa140c3e9c4f4f6142a528ad07f5e5d83094 Mon Sep 17 00:00:00 2001 From: SuZhou-Joe Date: Fri, 1 Nov 2024 17:45:34 +0800 Subject: [PATCH 4/5] feat: optimize code Signed-off-by: SuZhou-Joe --- src/core/public/chrome/chrome_service.tsx | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/core/public/chrome/chrome_service.tsx b/src/core/public/chrome/chrome_service.tsx index b890fdfb880..e07f99772b6 100644 --- a/src/core/public/chrome/chrome_service.tsx +++ b/src/core/public/chrome/chrome_service.tsx @@ -173,10 +173,9 @@ export class ChromeService { */ switchMap((appId) => application.applications$.pipe( - map( - (applications) => - !!appId && applications.has(appId) && !!applications.get(appId)!.chromeless - ) + map((applications) => { + return !!appId && applications.has(appId) && !!applications.get(appId)!.chromeless; + }) ) ) ) From be0da36b0e266627104d3e2bc7c297bb9879c074 Mon Sep 17 00:00:00 2001 From: SuZhou-Joe Date: Fri, 1 Nov 2024 17:49:49 +0800 Subject: [PATCH 5/5] feat: update Signed-off-by: SuZhou-Joe --- src/core/public/chrome/chrome_service.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/public/chrome/chrome_service.tsx b/src/core/public/chrome/chrome_service.tsx index e07f99772b6..51bb77ccc32 100644 --- a/src/core/public/chrome/chrome_service.tsx +++ b/src/core/public/chrome/chrome_service.tsx @@ -166,7 +166,7 @@ export class ChromeService { of(true), application.currentAppId$.pipe( /** - * There is a staled closure issue here. + * Using flatMap here will introduce staled closure issue. * For example, when currentAppId$ is going through A -> B -> C and * the application.applications$ just get changed in B, then it will always use B as the currentAppId * even though the latest appId now is C.