stop background flash on tab switches, unblock Audit/Usage demos (#6562)

This commit is contained in:
Anthony Stirling
2026-06-09 17:59:53 +01:00
committed by GitHub
parent 1e739b6f6f
commit 1a0beaffc2
3 changed files with 141 additions and 53 deletions
@@ -30,12 +30,26 @@ interface AppConfigModalProps {
onClose: () => void; 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<AppConfigModalProps> = ({ const AppConfigModalInner: React.FC<AppConfigModalProps> = ({
opened, opened,
onClose, onClose,
}) => { }) => {
const { t } = useTranslation(); const { t } = useTranslation();
const [active, setActive] = useState<NavKey>("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<NavKey>(
() => getSectionFromPath(window.location.pathname) ?? "general",
);
const isMobile = useIsMobile(); const isMobile = useIsMobile();
const navigate = useNavigate(); const navigate = useNavigate();
const location = useLocation(); const location = useLocation();
@@ -44,17 +58,10 @@ const AppConfigModalInner: React.FC<AppConfigModalProps> = ({
const { confirmIfDirty } = useUnsavedChanges(); const { confirmIfDirty } = useUnsavedChanges();
const closeButtonRef = useRef<HTMLButtonElement>(null); const closeButtonRef = useRef<HTMLButtonElement>(null);
// Extract section from URL path (e.g., /settings/people -> people) // Sync active state with URL path. Runs on open, on external URL changes,
const getSectionFromPath = (pathname: string): NavKey | null => { // and on the redirect path below - NOT on intra-modal tab clicks, because
const match = pathname.match(/\/settings\/([^/]+)/); // those update the URL via `history.replaceState` directly and never push
if (match && match[1]) { // a new React Router location.
const section = match[1] as NavKey;
return VALID_NAV_KEYS.includes(section as NavKey) ? section : null;
}
return null;
};
// Sync active state with URL path
useEffect(() => { useEffect(() => {
const section = getSectionFromPath(location.pathname); const section = getSectionFromPath(location.pathname);
if (opened && section) { if (opened && section) {
@@ -76,18 +83,41 @@ const AppConfigModalInner: React.FC<AppConfigModalProps> = ({
} }
}, [opened]); }, [opened]);
// Handle custom events for backwards compatibility. // Switch tab without forcing every `useLocation()` subscriber (HomePage and
// Use replace when already on /settings/* so external tab-switches // its FileSidebar/Workbench/RightSidebar/FileManager tree) to re-render.
// don't pile up history entries that would break close-by-back. //
// 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(() => { useEffect(() => {
const handler = (ev: Event) => { const handler = (ev: Event) => {
const detail = (ev as CustomEvent).detail as { key?: NavKey } | undefined; const detail = (ev as CustomEvent).detail as { key?: NavKey } | undefined;
if (detail?.key) { if (detail?.key) {
const alreadyInSettings = switchSection(detail.key);
window.location.pathname.startsWith("/settings");
navigate(`/settings/${detail.key}`, {
replace: alreadyInSettings,
});
} }
}; };
window.addEventListener("appConfig:navigate", handler as EventListener); window.addEventListener("appConfig:navigate", handler as EventListener);
@@ -96,7 +126,7 @@ const AppConfigModalInner: React.FC<AppConfigModalProps> = ({
"appConfig:navigate", "appConfig:navigate",
handler as EventListener, handler as EventListener,
); );
}, [navigate]); }, [switchSection]);
const colors = useMemo( const colors = useMemo(
() => ({ () => ({
@@ -165,23 +195,9 @@ const AppConfigModalInner: React.FC<AppConfigModalProps> = ({
async (key: NavKey) => { async (key: NavKey) => {
const canProceed = await confirmIfDirty(); const canProceed = await confirmIfDirty();
if (!canProceed) return; if (!canProceed) return;
switchSection(key);
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 });
}, },
[confirmIfDirty, navigate], [confirmIfDirty, switchSection],
); );
return ( return (
@@ -121,6 +121,81 @@ test.describe("Settings dialog", () => {
await restored.click(); 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<typeof window.history.pushState>),
);
};
window.history.replaceState = function (...args) {
w.__historyOps.replace++;
return origReplace(
...(args as Parameters<typeof window.history.replaceState>),
);
};
});
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 ({ test("close returns to origin URL even after switching tabs (no history pile-up)", async ({
page, page,
}) => { }) => {
@@ -71,10 +71,6 @@ export const useConfigNavSections = (
"settings.tooltips.enableLoginFirst", "settings.tooltips.enableLoginFirst",
"Enable login mode first", "Enable login mode first",
); );
const requiresEnterpriseTooltip = t(
"settings.tooltips.requiresEnterprise",
"Requires Enterprise license",
);
// Workspace // Workspace
sections.push({ sections.push({
@@ -199,10 +195,10 @@ export const useConfigNavSections = (
label: t("settings.licensingAnalytics.audit", "Audit"), label: t("settings.licensingAnalytics.audit", "Audit"),
icon: "fact-check-rounded", icon: "fact-check-rounded",
component: <AdminAuditSection />, component: <AdminAuditSection />,
disabled: !runningEE || requiresLogin, // Non-Enterprise users can still click in: AdminAuditSection
disabledTooltip: requiresLogin // renders a demo preview when `!hasEnterpriseLicense`.
? enableLoginTooltip disabled: requiresLogin,
: requiresEnterpriseTooltip, disabledTooltip: requiresLogin ? enableLoginTooltip : undefined,
}, },
{ {
key: "adminUsage", key: "adminUsage",
@@ -212,10 +208,9 @@ export const useConfigNavSections = (
), ),
icon: "analytics-rounded", icon: "analytics-rounded",
component: <AdminUsageSection />, component: <AdminUsageSection />,
disabled: !runningEE || requiresLogin, // Same demo-preview story as adminAudit above.
disabledTooltip: requiresLogin disabled: requiresLogin,
? enableLoginTooltip disabledTooltip: requiresLogin ? enableLoginTooltip : undefined,
: requiresEnterpriseTooltip,
}, },
], ],
}); });
@@ -441,20 +436,22 @@ export const createConfigNavSections = (
label: "Audit", label: "Audit",
icon: "fact-check-rounded", icon: "fact-check-rounded",
component: <AdminAuditSection />, component: <AdminAuditSection />,
disabled: !runningEE || requiresLogin, // Non-Enterprise users can click in to see the demo preview.
disabled: requiresLogin,
disabledTooltip: requiresLogin disabledTooltip: requiresLogin
? "Enable login mode first" ? "Enable login mode first"
: "Requires Enterprise license", : undefined,
}, },
{ {
key: "adminUsage", key: "adminUsage",
label: "Usage Analytics", label: "Usage Analytics",
icon: "analytics-rounded", icon: "analytics-rounded",
component: <AdminUsageSection />, component: <AdminUsageSection />,
disabled: !runningEE || requiresLogin, // Non-Enterprise users can click in to see the demo preview.
disabled: requiresLogin,
disabledTooltip: requiresLogin disabledTooltip: requiresLogin
? "Enable login mode first" ? "Enable login mode first"
: "Requires Enterprise license", : undefined,
}, },
], ],
}); });