Skip to content

Commit

Permalink
fix: staled closure inside chrome_service (#8783)
Browse files Browse the repository at this point in the history
  • Loading branch information
SuZhou-Joe authored Nov 1, 2024
1 parent 92e0766 commit e11b401
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 4 deletions.
2 changes: 2 additions & 0 deletions changelogs/fragments/8783.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- Staled closure inside chrome_service ([#8783](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8783))
34 changes: 33 additions & 1 deletion src/core/public/chrome/chrome_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import { HeaderVariant } from './constants';

class FakeApp implements App {
public title: string;
public appRoute: string;
public mount = () => () => {};

constructor(
Expand All @@ -54,6 +55,7 @@ class FakeApp implements App {
public headerVariant?: HeaderVariant
) {
this.title = `${this.id} App`;
this.appRoute = this.id;
}
}
const store = new Map();
Expand All @@ -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<Map<string, PublicAppInfo>>(
const applications$ = new Rx.BehaviorSubject<Map<string, PublicAppInfo>>(
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;
Expand Down Expand Up @@ -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', () => {
Expand Down
12 changes: 9 additions & 3 deletions src/core/public/chrome/chrome_service.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand All @@ -184,7 +190,7 @@ export class ChromeService {
this.headerVariantOverride$ = new BehaviorSubject<HeaderVariant | undefined>(undefined);

const appHeaderVariant$ = application.currentAppId$.pipe(
flatMap((appId) =>
switchMap((appId) =>
application.applications$.pipe(
map(
(applications) =>
Expand Down

0 comments on commit e11b401

Please sign in to comment.