fix(web): do not render App on protected routes (#1366)

When visiting the root route (`/`), the App component is rendered even
if the user is not logged in. Of course, this causes many errors that
are not visible in the UI but in the network monitor.

Fixing this problem requires moving the code to handle phase and status
changes to a higher place in the hierarchy because, now, `App` is
rendered "too late" to observe status changes.

web/package/agama-web-ui.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Fri Jun 21 14:32:11 UTC 2024 - Imobach Gonzalez Sosa <[email protected]>
+
+- Avoid connection attempts when the user is not logged in
+  (gh#openSUSE/agama#1366).
+
 -------------------------------------------------------------------
 Thu Jun 20 05:33:29 UTC 2024 - Imobach Gonzalez Sosa <[email protected]>
 

web/src/App.jsx

@@ -19,13 +19,13 @@
  * find current contact information at www.suse.com.
  */
 
-import React, { useEffect, useState } from "react";
+import React from "react";
 import { Navigate, Outlet, useLocation } from "react-router-dom";
 import { Loading } from "./components/layout";
 import { Questions } from "~/components/questions";
 import { ServerError, Installation } from "~/components/core";
 import { useInstallerL10n } from "./context/installerL10n";
-import { useInstallerClient, useInstallerClientStatus } from "~/context/installer";
+import { useInstallerClientStatus } from "~/context/installer";
 import { useProduct } from "./context/product";
 import { CONFIG, INSTALL, STARTUP } from "~/client/phase";
 import { BUSY } from "~/client/status";
@@ -38,38 +38,10 @@ import { BUSY } from "~/client/status";
  *   error (3 by default). The component will keep trying to connect.
  */
 function App() {
-  const client = useInstallerClient();
   const location = useLocation();
-  const { connected, error } = useInstallerClientStatus();
+  const { connected, error, phase, status } = useInstallerClientStatus();
   const { selectedProduct, products } = useProduct();
   const { language } = useInstallerL10n();
-  const [status, setStatus] = useState(undefined);
-  const [phase, setPhase] = useState(undefined);
-
-  useEffect(() => {
-    if (client) {
-      return client.manager.onPhaseChange(setPhase);
-    }
-  }, [client, setPhase]);
-
-  useEffect(() => {
-    if (client) {
-      return client.manager.onStatusChange(setStatus);
-    }
-  }, [client, setStatus]);
-
-  useEffect(() => {
-    const loadPhase = async () => {
-      const phase = await client.manager.getPhase();
-      const status = await client.manager.getStatus();
-      setPhase(phase);
-      setStatus(status);
-    };
-
-    if (client) {
-      loadPhase().catch(console.error);
-    }
-  }, [client, setPhase, setStatus]);
 
   const Content = () => {
     if (error) return <ServerError />;

web/src/App.test.jsx

@@ -44,14 +44,16 @@ jest.mock("~/context/product", () => ({
   }
 }));
 
+const mockClientStatus = {
+  connected: true,
+  error: false,
+  phase: STARTUP,
+  status: BUSY
+};
+
 jest.mock("~/context/installer", () => ({
   ...jest.requireActual("~/context/installer"),
-  useInstallerClientStatus: () => {
-    return {
-      connected: true,
-      error: false
-    };
-  }
+  useInstallerClientStatus: () => mockClientStatus
 }));
 
 // Mock some components,
@@ -61,32 +63,12 @@ jest.mock("~/components/core/Installation", () => () => <div>Installation Mock</
 jest.mock("~/components/layout/Loading", () => () => <div>Loading Mock</div>);
 jest.mock("~/components/product/ProductSelectionProgress", () => () => <div>Product progress</div>);
 
-// this object holds the mocked callbacks
-const callbacks = {};
-const getStatusFn = jest.fn();
-const getPhaseFn = jest.fn();
-
-// capture the latest subscription to the manager#onPhaseChange for triggering it manually
-const onPhaseChangeFn = cb => {
-  callbacks.onPhaseChange = cb;
-};
-const onStatusChangeFn = cb => {
-  callbacks.onStatusChange = cb;
-};
-const changePhaseTo = phase => act(() => callbacks.onPhaseChange(phase));
-
 describe("App", () => {
   beforeEach(() => {
     // setting the language through a cookie
     document.cookie = "agamaLang=en-us; path=/;";
     createClient.mockImplementation(() => {
       return {
-        manager: {
-          getStatus: getStatusFn,
-          getPhase: getPhaseFn,
-          onPhaseChange: onPhaseChangeFn,
-          onStatusChange: onStatusChangeFn
-        },
         l10n: {
           locales: jest.fn().mockResolvedValue([["en_us", "English", "United States"]]),
           getLocales: jest.fn().mockResolvedValue(["en_us"]),
@@ -126,22 +108,10 @@ describe("App", () => {
     });
   });
 
-  describe("on the startup phase", () => {
-    beforeEach(() => {
-      getPhaseFn.mockResolvedValue(STARTUP);
-      getStatusFn.mockResolvedValue(BUSY);
-    });
-
-    it("renders the Loading screen", async () => {
-      installerRender(<App />, { withL10n: true });
-      await screen.findByText("Loading Mock");
-    });
-  });
-
   describe("when the service is busy during startup", () => {
     beforeEach(() => {
-      getPhaseFn.mockResolvedValue(STARTUP);
-      getStatusFn.mockResolvedValue(BUSY);
+      mockClientStatus.phase = STARTUP;
+      mockClientStatus.status = BUSY;
     });
 
     it("renders the Loading screen", async () => {
@@ -152,12 +122,12 @@ describe("App", () => {
 
   describe("on the CONFIG phase", () => {
     beforeEach(() => {
-      getPhaseFn.mockResolvedValue(CONFIG);
+      mockClientStatus.phase = CONFIG;
     });
 
     describe("if the service is busy", () => {
       beforeEach(() => {
-        getStatusFn.mockResolvedValue(BUSY);
+        mockClientStatus.status = BUSY;
       });
 
       it("redirects to product selection progress", async () => {
@@ -168,7 +138,7 @@ describe("App", () => {
 
     describe("if the service is not busy", () => {
       beforeEach(() => {
-        getStatusFn.mockResolvedValue(IDLE);
+        mockClientStatus.status = IDLE;
       });
 
       it("renders the application content", async () => {
@@ -180,7 +150,7 @@ describe("App", () => {
 
   describe("on the INSTALL phase", () => {
     beforeEach(() => {
-      getPhaseFn.mockResolvedValue(INSTALL);
+      mockClientStatus.phase = INSTALL;
       mockSelectedProduct = { id: "Fake product" };
     });
 
@@ -189,17 +159,4 @@ describe("App", () => {
       await screen.findByText("Installation Mock");
     });
   });
-
-  describe("when service phase changes", () => {
-    beforeEach(() => {
-      getPhaseFn.mockResolvedValue(CONFIG);
-    });
-
-    it("renders the Installation component on the INSTALL phase", async () => {
-      installerRender(<App />, { withL10n: true });
-      await screen.findByText(/Outlet Content/);
-      changePhaseTo(INSTALL);
-      await screen.findByText("Installation Mock");
-    });
-  });
 });

web/src/Protected.jsx

@@ -27,6 +27,9 @@ import { AppProviders } from "./context/app";
 export default function Protected() {
   const { isLoggedIn } = useAuth();
 
+  // It is not known yet whether the user is logged or not.
+  if (isLoggedIn === undefined) return;
+
   if (isLoggedIn === false) {
     return <Navigate to="/login" />;
   }

web/src/components/overview/SoftwareSection.test.jsx

@@ -56,7 +56,7 @@ beforeEach(() => {
   });
 });
 
-it("renders the required space and the selected patterns", async () => {
+it.only("renders the required space and the selected patterns", async () => {
   installerRender(<SoftwareSection />);
   await screen.findByText("500 MiB");
   await screen.findByText(kdePattern.summary);

web/src/context/installer.jsx

@@ -27,7 +27,9 @@ import { createDefaultClient } from "~/client";
 const InstallerClientContext = React.createContext(null);
 // TODO: we use a separate context to avoid changing all the codes to
 // `useInstallerClient`. We should merge them in the future.
-const InstallerClientStatusContext = React.createContext({ connected: false, error: false });
+const InstallerClientStatusContext = React.createContext({
+  connected: false, error: false, phase: undefined, status: undefined
+});
 
 /**
  * Returns the D-Bus installer client
@@ -77,6 +79,8 @@ function InstallerClientProvider({
   const [value, setValue] = useState(client);
   const [connected, setConnected] = useState(false);
   const [error, setError] = useState(false);
+  const [status, setStatus] = useState(undefined);
+  const [phase, setPhase] = useState(undefined);
 
   useEffect(() => {
     const connectClient = async () => {
@@ -99,6 +103,31 @@ function InstallerClientProvider({
     if (!value) connectClient();
   }, [setValue, value]);
 
+  useEffect(() => {
+    if (value) {
+      return value.manager.onPhaseChange(setPhase);
+    }
+  }, [value, setPhase]);
+
+  useEffect(() => {
+    if (value) {
+      return value.manager.onStatusChange(setStatus);
+    }
+  }, [value, setStatus]);
+
+  useEffect(() => {
+    const loadPhase = async () => {
+      const initialPhase = await value.manager.getPhase();
+      const initialStatus = await value.manager.getStatus();
+      setPhase(initialPhase);
+      setStatus(initialStatus);
+    };
+
+    if (value) {
+      loadPhase().catch(console.error);
+    }
+  }, [value, setPhase, setStatus]);
+
   useEffect(() => {
     if (!value) return;
 
@@ -115,7 +144,7 @@ function InstallerClientProvider({
 
   return (
     <InstallerClientContext.Provider value={value}>
-      <InstallerClientStatusContext.Provider value={{ connected, error }}>
+      <InstallerClientStatusContext.Provider value={{ connected, error, phase, status }}>
         {children}
       </InstallerClientStatusContext.Provider>
     </InstallerClientContext.Provider>

web/src/context/installer.test.jsx

@@ -24,16 +24,20 @@ import { act, screen } from "@testing-library/react";
 import { createDefaultClient } from "~/client";
 import { plainRender, createCallbackMock } from "~/test-utils";
 import { InstallerClientProvider, useInstallerClientStatus } from "./installer";
+import { STARTUP } from "~/client/phase";
+import { BUSY } from "~/client/status";
 
 jest.mock("~/client");
 
 // Helper component to check the client status.
 const ClientStatus = () => {
-  const { connected } = useInstallerClientStatus();
+  const { connected, phase, status } = useInstallerClientStatus();
 
   return (
     <ul>
       <li>{`connected: ${connected}`}</li>
+      <li>{`phase: ${phase}`}</li>
+      <li>{`status: ${status}`}</li>
     </ul>
   );
 };
@@ -43,7 +47,13 @@ describe("installer context", () => {
     createDefaultClient.mockImplementation(() => {
       return {
         onConnect: jest.fn(),
-        onDisconnect: jest.fn()
+        onDisconnect: jest.fn(),
+        manager: {
+          getPhase: jest.fn().mockResolvedValue(STARTUP),
+          getStatus: jest.fn().mockResolvedValue(BUSY),
+          onPhaseChange: jest.fn(),
+          onStatusChange: jest.fn()
+        }
       };
     });
   });
@@ -55,5 +65,7 @@ describe("installer context", () => {
       </InstallerClientProvider>
     );
     await screen.findByText("connected: false");
+    await screen.findByText("phase: 0");
+    await screen.findByText("status: 1");
   });
 });

web/src/context/installerL10n.test.jsx

@@ -35,6 +35,12 @@ const setUILocaleFn = jest.fn().mockResolvedValue();
 const client = {
   onConnect: jest.fn(),
   onDisconnect: jest.fn(),
+  manager: {
+    getPhase: jest.fn(),
+    getStatus: jest.fn(),
+    onPhaseChange: jest.fn(),
+    onStatusChange: jest.fn()
+  },
   l10n: {
     getUILocale: getUILocaleFn,
     setUILocale: setUILocaleFn,

web/src/test-utils.js

@@ -84,6 +84,18 @@ const Providers = ({ children, withL10n }) => {
     client.onDisconnect = noop;
   }
 
+  if (!client.manager) {
+    client.manager = {};
+  }
+
+  client.manager = {
+    getPhase: noop,
+    getStatus: noop,
+    onPhaseChange: noop,
+    onStatusChange: noop,
+    ...client.manager
+  };
+
   if (withL10n) {
     return (
       <InstallerClientProvider client={client}>