Skip to content

Commit

Permalink
fix: staled closure inside chrome_service
Browse files Browse the repository at this point in the history
Signed-off-by: SuZhou-Joe <[email protected]>
  • Loading branch information
SuZhou-Joe committed Nov 1, 2024
1 parent 3381dcd commit 8ca8167
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 6 deletions.
35 changes: 34 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,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', () => {
Expand Down
24 changes: 19 additions & 5 deletions src/core/public/chrome/chrome_service.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit 8ca8167

Please sign in to comment.