From 1a0beaffc21d6db99b4f6dfa84e9efeaefc9c9a2 Mon Sep 17 00:00:00 2001 From: Anthony Stirling <77850077+Frooodle@users.noreply.github.com> Date: Tue, 9 Jun 2026 17:59:53 +0100 Subject: [PATCH] stop background flash on tab switches, unblock Audit/Usage demos (#6562) --- .../core/components/shared/AppConfigModal.tsx | 90 +++++++++++-------- .../src/core/tests/stubbed/settings.spec.ts | 75 ++++++++++++++++ .../shared/config/configNavSections.tsx | 29 +++--- 3 files changed, 141 insertions(+), 53 deletions(-) diff --git a/frontend/editor/src/core/components/shared/AppConfigModal.tsx b/frontend/editor/src/core/components/shared/AppConfigModal.tsx index a03489838..f4efe6d51 100644 --- a/frontend/editor/src/core/components/shared/AppConfigModal.tsx +++ b/frontend/editor/src/core/components/shared/AppConfigModal.tsx @@ -30,12 +30,26 @@ interface AppConfigModalProps { onClose: () => void; } +// Extract section from URL path (e.g., /settings/people -> people) +const getSectionFromPath = (pathname: string): NavKey | null => { + const match = pathname.match(/\/settings\/([^/]+)/); + if (match && match[1]) { + const section = match[1] as NavKey; + return VALID_NAV_KEYS.includes(section as NavKey) ? section : null; + } + return null; +}; + const AppConfigModalInner: React.FC = ({ opened, onClose, }) => { const { t } = useTranslation(); - const [active, setActive] = useState("general"); + // Initialize from the URL so a deep link (`/settings/people`) lands on the + // right tab without a one-frame "general" flicker. + const [active, setActive] = useState( + () => getSectionFromPath(window.location.pathname) ?? "general", + ); const isMobile = useIsMobile(); const navigate = useNavigate(); const location = useLocation(); @@ -44,17 +58,10 @@ const AppConfigModalInner: React.FC = ({ const { confirmIfDirty } = useUnsavedChanges(); const closeButtonRef = useRef(null); - // Extract section from URL path (e.g., /settings/people -> people) - const getSectionFromPath = (pathname: string): NavKey | null => { - const match = pathname.match(/\/settings\/([^/]+)/); - if (match && match[1]) { - const section = match[1] as NavKey; - return VALID_NAV_KEYS.includes(section as NavKey) ? section : null; - } - return null; - }; - - // Sync active state with URL path + // Sync active state with URL path. Runs on open, on external URL changes, + // and on the redirect path below - NOT on intra-modal tab clicks, because + // those update the URL via `history.replaceState` directly and never push + // a new React Router location. useEffect(() => { const section = getSectionFromPath(location.pathname); if (opened && section) { @@ -76,18 +83,41 @@ const AppConfigModalInner: React.FC = ({ } }, [opened]); - // Handle custom events for backwards compatibility. - // Use replace when already on /settings/* so external tab-switches - // don't pile up history entries that would break close-by-back. + // Switch tab without forcing every `useLocation()` subscriber (HomePage and + // its FileSidebar/Workbench/RightSidebar/FileManager tree) to re-render. + // + // First entry into /settings/* still goes through React Router so HomePage's + // location-watching effect opens the modal and pushes a real history entry - + // so the back button can close us. Subsequent tab clicks bypass React Router + // and mutate the URL bar via `history.replaceState`. The browser sees the + // URL update (deep-link / refresh still work) but React Router never fires a + // location change, so the layer behind the Mantine overlay never repaints + // and the backdrop-filter blur stops flashing. + const switchSection = useCallback( + (key: NavKey) => { + setActive(key); + const alreadyInSettings = + window.location.pathname.startsWith("/settings"); + if (alreadyInSettings) { + window.history.replaceState( + window.history.state, + "", + `/settings/${key}`, + ); + } else { + navigate(`/settings/${key}`); + } + }, + [navigate], + ); + + // Backwards-compat: external `appConfig:navigate` events route through the + // same switchSection path so they get the no-flash treatment too. useEffect(() => { const handler = (ev: Event) => { const detail = (ev as CustomEvent).detail as { key?: NavKey } | undefined; if (detail?.key) { - const alreadyInSettings = - window.location.pathname.startsWith("/settings"); - navigate(`/settings/${detail.key}`, { - replace: alreadyInSettings, - }); + switchSection(detail.key); } }; window.addEventListener("appConfig:navigate", handler as EventListener); @@ -96,7 +126,7 @@ const AppConfigModalInner: React.FC = ({ "appConfig:navigate", handler as EventListener, ); - }, [navigate]); + }, [switchSection]); const colors = useMemo( () => ({ @@ -165,23 +195,9 @@ const AppConfigModalInner: React.FC = ({ async (key: NavKey) => { const canProceed = await confirmIfDirty(); if (!canProceed) return; - - setActive(key); - // First in-modal nav (when current path isn't `/settings/*` yet) must - // PUSH so the originating page stays in history and close-by-back can - // return to it. Subsequent tab switches REPLACE so they don't pile up - // history entries that handleClose's navigate(-1) can't unwind. - // - // Read window.location.pathname directly (not the React hook's - // location.pathname) so rapid successive clicks pick up the URL - // change from the previous click immediately. The hook snapshot is - // stale between render cycles - relying on it lets a second click - // PUSH again before React re-renders, producing a history pile-up. - const alreadyInSettings = - window.location.pathname.startsWith("/settings"); - navigate(`/settings/${key}`, { replace: alreadyInSettings }); + switchSection(key); }, - [confirmIfDirty, navigate], + [confirmIfDirty, switchSection], ); return ( diff --git a/frontend/editor/src/core/tests/stubbed/settings.spec.ts b/frontend/editor/src/core/tests/stubbed/settings.spec.ts index 0bfd7b3ef..cb95a1171 100644 --- a/frontend/editor/src/core/tests/stubbed/settings.spec.ts +++ b/frontend/editor/src/core/tests/stubbed/settings.spec.ts @@ -121,6 +121,81 @@ test.describe("Settings dialog", () => { await restored.click(); }); + test("intra-modal tab switching updates URL via replaceState, never pushState", async ({ + page, + }) => { + // Mechanism test for the "background flash" fix. Before the fix, every + // tab click called `navigate(...)` which fired React Router's location + // subscribers - HomePage, QuickAccessBar, FileManagerView, ... - and the + // layer behind the Mantine overlay repainted, causing backdrop-filter + // blur to recompute and visibly flash. After the fix, only the very + // first nav into /settings/* is allowed to go through React Router (so + // HomePage's location-watching effect opens the modal and the back + // button has a real history entry to pop). Every subsequent tab click + // updates the URL bar via raw `window.history.replaceState`, which + // React Router does NOT subscribe to. We assert this directly by + // counting calls. + await page.addInitScript(() => { + const w = window as unknown as { + __historyOps: { push: number; replace: number }; + }; + w.__historyOps = { push: 0, replace: 0 }; + const origPush = window.history.pushState.bind(window.history); + const origReplace = window.history.replaceState.bind(window.history); + window.history.pushState = function (...args) { + w.__historyOps.push++; + return origPush( + ...(args as Parameters), + ); + }; + window.history.replaceState = function (...args) { + w.__historyOps.replace++; + return origReplace( + ...(args as Parameters), + ); + }; + }); + + await page.goto("/", { waitUntil: "domcontentloaded" }); + await openSettings(page); + + const generalNav = page.locator('[data-tour="admin-general-nav"]').first(); + const hotkeysNav = page.locator('[data-tour="admin-hotkeys-nav"]').first(); + await expect(generalNav).toBeVisible({ timeout: 5_000 }); + + // First nav into /settings/* takes the React Router path (push). We + // snapshot both counters AFTER this to isolate the intra-modal delta. + await generalNav.click(); + await page.waitForURL(/\/settings\/general/, { timeout: 5_000 }); + const baseline = await page.evaluate(() => { + const w = window as unknown as { + __historyOps: { push: number; replace: number }; + }; + return { ...w.__historyOps }; + }); + + // Now do 4 round-trips between two tabs - 8 intra-modal clicks total. + for (let i = 0; i < 4; i++) { + await hotkeysNav.click(); + await page.waitForURL(/\/settings\/hotkeys/, { timeout: 5_000 }); + await generalNav.click(); + await page.waitForURL(/\/settings\/general/, { timeout: 5_000 }); + } + + const after = await page.evaluate(() => { + const w = window as unknown as { + __historyOps: { push: number; replace: number }; + }; + return { ...w.__historyOps }; + }); + + // Zero pushes during 8 intra-modal clicks - the regression would show + // up here as `after.push - baseline.push >= 1`. + expect(after.push - baseline.push).toBe(0); + // Exactly 8 replaces - one per click. + expect(after.replace - baseline.replace).toBe(8); + }); + test("close returns to origin URL even after switching tabs (no history pile-up)", async ({ page, }) => { diff --git a/frontend/editor/src/proprietary/components/shared/config/configNavSections.tsx b/frontend/editor/src/proprietary/components/shared/config/configNavSections.tsx index 1ccfea9e7..19fe77559 100644 --- a/frontend/editor/src/proprietary/components/shared/config/configNavSections.tsx +++ b/frontend/editor/src/proprietary/components/shared/config/configNavSections.tsx @@ -71,10 +71,6 @@ export const useConfigNavSections = ( "settings.tooltips.enableLoginFirst", "Enable login mode first", ); - const requiresEnterpriseTooltip = t( - "settings.tooltips.requiresEnterprise", - "Requires Enterprise license", - ); // Workspace sections.push({ @@ -199,10 +195,10 @@ export const useConfigNavSections = ( label: t("settings.licensingAnalytics.audit", "Audit"), icon: "fact-check-rounded", component: , - disabled: !runningEE || requiresLogin, - disabledTooltip: requiresLogin - ? enableLoginTooltip - : requiresEnterpriseTooltip, + // Non-Enterprise users can still click in: AdminAuditSection + // renders a demo preview when `!hasEnterpriseLicense`. + disabled: requiresLogin, + disabledTooltip: requiresLogin ? enableLoginTooltip : undefined, }, { key: "adminUsage", @@ -212,10 +208,9 @@ export const useConfigNavSections = ( ), icon: "analytics-rounded", component: , - disabled: !runningEE || requiresLogin, - disabledTooltip: requiresLogin - ? enableLoginTooltip - : requiresEnterpriseTooltip, + // Same demo-preview story as adminAudit above. + disabled: requiresLogin, + disabledTooltip: requiresLogin ? enableLoginTooltip : undefined, }, ], }); @@ -441,20 +436,22 @@ export const createConfigNavSections = ( label: "Audit", icon: "fact-check-rounded", component: , - disabled: !runningEE || requiresLogin, + // Non-Enterprise users can click in to see the demo preview. + disabled: requiresLogin, disabledTooltip: requiresLogin ? "Enable login mode first" - : "Requires Enterprise license", + : undefined, }, { key: "adminUsage", label: "Usage Analytics", icon: "analytics-rounded", component: , - disabled: !runningEE || requiresLogin, + // Non-Enterprise users can click in to see the demo preview. + disabled: requiresLogin, disabledTooltip: requiresLogin ? "Enable login mode first" - : "Requires Enterprise license", + : undefined, }, ], });