From 7149e693465db0c12a9bcc041368272bd9796ff5 Mon Sep 17 00:00:00 2001 From: SuZhou-Joe Date: Sat, 2 Nov 2024 00:23:39 +0800 Subject: [PATCH] fix: staled closure inside chrome_service (#8783) --- changelogs/fragments/8783.yml | 2 ++ src/core/public/chrome/chrome_service.test.ts | 34 ++++++++++++++++++- src/core/public/chrome/chrome_service.tsx | 12 +++++-- 3 files changed, 44 insertions(+), 4 deletions(-) create mode 100644 changelogs/fragments/8783.yml diff --git a/changelogs/fragments/8783.yml b/changelogs/fragments/8783.yml new file mode 100644 index 000000000000..0109a60191a8 --- /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 diff --git a/src/core/public/chrome/chrome_service.test.ts b/src/core/public/chrome/chrome_service.test.ts index 082ffbfa16ed..e930aa8931ce 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,30 @@ 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, + ] + `); + }); }); describe('header variant', () => { diff --git a/src/core/public/chrome/chrome_service.tsx b/src/core/public/chrome/chrome_service.tsx index 39f9f0fbc320..51bb77ccc323 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'; @@ -165,7 +165,13 @@ export class ChromeService { // 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) => + /** + * 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. + */ + switchMap((appId) => application.applications$.pipe( map((applications) => { return !!appId && applications.has(appId) && !!applications.get(appId)!.chromeless; @@ -184,7 +190,7 @@ export class ChromeService { this.headerVariantOverride$ = new BehaviorSubject(undefined); const appHeaderVariant$ = application.currentAppId$.pipe( - flatMap((appId) => + switchMap((appId) => application.applications$.pipe( map( (applications) =>