Merge pull request #5509 from nickgros/SWC-7049

SWC-7049 - Fix remounting/cleanup issue with React components
Sage-Bionetworks · Aug 30, 2024 · c86f830 · c86f830
2 parents 932d692 + 122660b
commit c86f830
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 24 deletions.
diff --git a/e2e/files.spec.ts b/e2e/files.spec.ts
@@ -356,7 +356,10 @@ testAuth.describe('Files', () => {
       await expect(
         userPage.getByRole('heading', { name: userProject.name }),
       ).toBeVisible()
-      await userPage.getByRole('link', { name: 'Files', exact: true }).click()
+      await userPage
+        .getByRole('link', { name: 'Files', exact: true })
+        .first()
+        .click()
     })
 
     await testAuth.step('upload file', async () => {

diff --git a/e2e/helpers/testUser.ts b/e2e/helpers/testUser.ts
@@ -134,13 +134,11 @@ export async function loginTestUser(
 
 export async function goToDashboard(page: Page) {
   await page.goto('/')
-  await waitForInitialPageLoad(page)
-
   const authenticatedUserBundleResponsePromise =
     getAuthenticatedUserBundleResponsePromise(page)
-  await page.getByRole('link', { name: 'View Your Dashboard' }).first().click()
 
   // wait for page to load
+  await waitForInitialPageLoad(page)
   await authenticatedUserBundleResponsePromise
   await expectAuthenticatedNavDrawerLoaded(page)
 }
@@ -189,7 +187,7 @@ export async function getUserIdFromLocalStorage(page: Page) {
 }
 
 export async function acceptSiteCookies(page: Page) {
-  await page.getByRole('button', { name: 'Accept and continue' }).click()
+  await page.getByRole('button', { name: 'ALLOW ALL' }).click()
 }
 
 export const dismissAlert = async (page: Page, alertText: string) => {
@@ -260,18 +258,14 @@ export const expectDiscussionThreadLoaded = async (
 
 export const toggleIntoExperimentalMode = async (page: Page) => {
   await test.step('toggle into experimental mode', async () => {
-    await expect(
-      page.getByRole('link', { name: 'Experimental Mode Off' }),
-    ).toBeVisible()
+    const experimentalModeLabel = page.getByText('Experimental Mode')
+    const experimentalModeInput = page.getByLabel('Experimental Mode')
 
-    await page.getByRole('link', { name: 'Experimental Mode' }).click()
-    await expect(
-      page.getByRole('heading', { name: 'Experimental Mode' }),
-    ).toBeVisible()
-    await page.getByRole('button', { name: 'OK' }).click()
+    await expect(experimentalModeInput).not.toBeChecked()
 
-    await expect(
-      page.getByRole('link', { name: 'Experimental Mode On' }),
-    ).toBeVisible()
+    // Because the input element is not visible, we need to click the label
+    await experimentalModeLabel.click()
+
+    await expect(experimentalModeInput).toBeChecked()
   })
 }
diff --git a/e2e/homepage.spec.ts b/e2e/homepage.spec.ts
@@ -11,7 +11,7 @@ test.describe('Homepage - Unauthenticated', () => {
 
     await expect(
       page.getByRole('link', { name: 'Log in to Synapse' }),
-    ).toHaveCount(2)
+    ).toHaveCount(1)
     await expect(
       page.getByRole('link', { name: 'View Your Dashboard' }),
     ).toHaveCount(0)
@@ -22,15 +22,15 @@ testAuth.describe('Homepage - Authenticated', () => {
   testAuth(
     'should show View Your Dashboard button when logged in',
     async ({ userPage }) => {
-      await userPage.goto('/')
+      await userPage.goto('/Home:x')
       await waitForInitialPageLoad(userPage)
 
       await expect(
         userPage.getByRole('link', { name: 'Log in to Synapse' }),
       ).toHaveCount(0)
       await expect(
         userPage.getByRole('link', { name: 'View Your Dashboard' }),
-      ).toHaveCount(2)
+      ).toHaveCount(1)
     },
   )
 })
diff --git a/src/main/java/org/sagebionetworks/web/client/widget/ReactComponent.java b/src/main/java/org/sagebionetworks/web/client/widget/ReactComponent.java
@@ -7,6 +7,7 @@
 import com.google.gwt.event.dom.client.ClickHandler;
 import com.google.gwt.event.dom.client.HasClickHandlers;
 import com.google.gwt.event.shared.HandlerRegistration;
+import com.google.gwt.user.client.Timer;
 import com.google.gwt.user.client.ui.ComplexPanel;
 import com.google.gwt.user.client.ui.Widget;
 import java.util.ArrayList;
@@ -142,15 +143,22 @@ private void injectChildWidgetsIntoComponent() {
 
   public void render(ReactNode<?> reactNode) {
     this.reactElement = reactNode;
-    maybeCreateRoot();
     injectChildWidgetsIntoComponent();
 
     // This component may be a React child of another component. If so, we must rerender the ancestor component(s) so
     // that they use the new ReactElement created in this render step.
     ReactComponent<?> componentToRender = getRootReactComponentWidget();
     if (componentToRender == this) {
-      // Resynchronize with the DOM
-      root.render(this.reactElement);
+      // Asynchronously schedule creating a root in case React is still rendering and may unmount the current root
+      Timer t = new Timer() {
+        @Override
+        public void run() {
+          maybeCreateRoot();
+          // Resynchronize with the DOM
+          root.render(reactElement);
+        }
+      };
+      t.schedule(0);
     } else {
       // Walk up the tree to the parent and repeat rerendering
       componentToRender.rerender();
@@ -175,9 +183,17 @@ protected void onLoad() {
   protected void onUnload() {
     super.onUnload();
     if (root != null) {
-      root.unmount();
+      // Asynchronously schedule unmounting the root to allow React to finish the current render cycle.
+      // https://github.com/facebook/react/issues/25675
+      Timer t = new Timer() {
+        @Override
+        public void run() {
+          root.unmount();
+          root = null;
+        }
+      };
+      t.schedule(0);
     }
-    root = null;
   }
 
   /**