From 8ca81674670b5845841e02b037945088cf7275aa Mon Sep 17 00:00:00 2001
From: SuZhou-Joe <suzhou@amazon.com>
Date: Fri, 1 Nov 2024 12:22:09 +0800
Subject: [PATCH] fix: staled closure inside chrome_service

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
---
 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 082ffbfa16ed..25fb9d35b54b 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<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;
@@ -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 39f9f0fbc320..10613b9a2f04 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(