Skip to content
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 56 additions & 26 deletions app/frontend/src/layoutWrapper.tsx
Original file line number Diff line number Diff line change
@@ -1,45 +1,75 @@
import { AccountInfo, EventType, PublicClientApplication } from "@azure/msal-browser";
import { AuthenticationResult, EventType, PublicClientApplication } from "@azure/msal-browser";
import { checkLoggedIn, msalConfig, useLogin } from "./authConfig";
import { useEffect, useState } from "react";
import { useEffect, useMemo, useRef, useState } from "react";
import { MsalProvider } from "@azure/msal-react";
import { LoginContext } from "./loginContext";
import Layout from "./pages/layout/Layout";

const LayoutWrapper = () => {
const [loggedIn, setLoggedIn] = useState(false);
if (useLogin) {
var msalInstance = new PublicClientApplication(msalConfig);
// Create a stable MSAL instance (avoid re-init/duplicate listeners; single shared client for MsalProvider).
const msalInstance = useMemo(() => new PublicClientApplication(msalConfig), []);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cline's suggested fix did not use useMemo, but Copilot+GPT5 thought it was a good idea, to avoid re-construction across renders.

Copy link

@DanWahlin DanWahlin Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs show this general type of pattern for accomplishing that (without useMemo()), but useMemo() should achieve the goal of ensuring it's only created once. I don't know enough about the app to understand the overall flow and how often the component is rerendered. But, assuming it is, useMemo should help there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an SPA - my understanding is that this wrapper holds all the content displayed and shouldn't be re-rendered unless the page is refreshed

const [initialized, setInitialized] = useState(false);
// Track mount state so we don't call setState after unmount if async init resolves late
const mounted = useRef<boolean>(true);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new mounted ref is to avoid setState calls inside the async init() below if the component unmounts during all that. Don't know how common that is.


// Default to using the first account if no account is active on page load
if (!msalInstance.getActiveAccount() && msalInstance.getAllAccounts().length > 0) {
// Account selection logic is app dependent. Adjust as needed for different use cases.
msalInstance.setActiveAccount(msalInstance.getActiveAccount());
}
useEffect(() => {
// React StrictMode in development invokes effects twice (mount -> cleanup -> mount).
// Reset the flag here so this run is considered mounted.
mounted.current = true;
const init = async () => {
try {
await msalInstance.initialize();

// Listen for sign-in event and set active account
msalInstance.addEventCallback(event => {
if (event.eventType === EventType.LOGIN_SUCCESS && event.payload) {
const account = event.payload as AccountInfo;
msalInstance.setActiveAccount(account);
}
});
// Default to using the first account if no account is active on page load
if (!msalInstance.getActiveAccount() && msalInstance.getAllAccounts().length > 0) {
// Account selection logic is app dependent. Adjust as needed for different use cases.
msalInstance.setActiveAccount(msalInstance.getAllAccounts()[0]);
}

useEffect(() => {
const fetchLoggedIn = async () => {
setLoggedIn(await checkLoggedIn(msalInstance));
// Listen for sign-in event and set active account
msalInstance.addEventCallback(event => {
if (event.eventType === EventType.LOGIN_SUCCESS && event.payload) {
const result = event.payload as AuthenticationResult;
if (result.account) {
msalInstance.setActiveAccount(result.account);
}
}
});

if (mounted.current) {
const isLoggedIn = await checkLoggedIn(msalInstance).catch(e => {
// Swallow check error but still allow app to render
console.error("checkLoggedIn failed", e);
return false;
});
setLoggedIn(isLoggedIn);
}
} catch (e) {
console.error("MSAL initialize failed", e);
} finally {
if (mounted.current) {
setInitialized(true);
}
}
};
init();
return () => {
// On unmount: flag as unmounted so any pending async in init() doesn't call setState
// This avoids React warnings about setting state on an unmounted component.
mounted.current = false;
};
}, [msalInstance]);

fetchLoggedIn();
}, []);
if (!initialized) {
// Lightweight placeholder while MSAL initializes
return <p>Loading authentication…</p>;
}

return (
<MsalProvider instance={msalInstance}>
<LoginContext.Provider
value={{
loggedIn,
setLoggedIn
}}
>
<LoginContext.Provider value={{ loggedIn, setLoggedIn }}>
<Layout />
</LoginContext.Provider>
</MsalProvider>
Expand Down
Loading