diff --git a/frontend/editor/src/core/components/shared/AppConfigModal.tsx b/frontend/editor/src/core/components/shared/AppConfigModal.tsx index e01146d5ab..e26ebdf624 100644 --- a/frontend/editor/src/core/components/shared/AppConfigModal.tsx +++ b/frontend/editor/src/core/components/shared/AppConfigModal.tsx @@ -54,6 +54,21 @@ interface AppConfigModalProps { hiddenSectionKeys?: NavKey[]; } +/** + * Whether this session has a history entry behind the current one. + * + * react-router keeps its own entry index in `history.state.idx`, so reading it + * live is the non-stale equivalent of the `location.key === "default"` test + * this used to do ("default" = first entry, nothing to pop back to). It has to + * be live: `location` lags behind a pending transition, and intra-modal tab + * switches rewrite the URL through `history.replaceState`, which react-router + * never observes at all. + */ +const canUnwindHistory = (): boolean => { + const idx = (window.history.state as { idx?: number } | null)?.idx; + return typeof idx === "number" && idx > 0; +}; + // Extract section from URL path (e.g., /settings/people -> people) const getSectionFromPath = (pathname: string): NavKey | null => { const match = pathname.match(/\/settings\/([^/]+)/); @@ -89,10 +104,6 @@ const AppConfigModalInner: React.FC = ({ const licenseAlert = useLicenseAlert(); const { confirmIfDirty } = useUnsavedChanges(); const closeButtonRef = useRef(null); - // Whether this modal pushed its own /settings entry, so close knows there is - // one to pop. `location.key` can't answer that: react-router defers location - // updates through a transition and may still read the pre-open entry. - const pushedSettingsEntry = useRef(false); // 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 @@ -151,7 +162,6 @@ const AppConfigModalInner: React.FC = ({ withBasePath(`/settings/${key}`), ); } else { - pushedSettingsEntry.current = true; navigate(`/settings/${key}`); } }, @@ -235,8 +245,7 @@ const AppConfigModalInner: React.FC = ({ // the pre-open path here and skip the unwind entirely - leaving the URL on // /settings/* so the modal immediately re-opens. if (urlSync && isInSettings()) { - if (pushedSettingsEntry.current) { - pushedSettingsEntry.current = false; + if (canUnwindHistory()) { navigate(-1); } else { // Deep link or refresh straight into /settings: nothing to pop to. diff --git a/frontend/editor/src/core/pages/HomePage.tsx b/frontend/editor/src/core/pages/HomePage.tsx index b6714c9878..571bb5f116 100644 --- a/frontend/editor/src/core/pages/HomePage.tsx +++ b/frontend/editor/src/core/pages/HomePage.tsx @@ -105,11 +105,16 @@ export default function HomePage() { // Open the config modal whenever the URL is /settings/* (e.g. from the admin // tour's openConfigModal action which navigates to /settings/overview). // - // Read the live URL rather than `location.pathname`: react-router defers - // location updates through a transition, so under load it can still hold the - // pre-navigation path and re-open a modal the user just closed. + // Both halves read the live URL rather than `location.pathname`, and popstate + // is subscribed to directly: react-router defers location updates through a + // transition, so `location` can still hold the pre-navigation path. Deriving + // from it re-opens a modal the user just closed, and depending on it alone + // misses Back entirely when the push it should have committed never landed. useEffect(() => { - setConfigModalOpen(isInSettings()); + const syncFromUrl = () => setConfigModalOpen(isInSettings()); + syncFromUrl(); + window.addEventListener("popstate", syncFromUrl); + return () => window.removeEventListener("popstate", syncFromUrl); }, [location.pathname]); useEffect(() => { diff --git a/frontend/editor/src/core/tests/stubbed/settings.spec.ts b/frontend/editor/src/core/tests/stubbed/settings.spec.ts index a9b0bbaa17..2f6f394655 100644 --- a/frontend/editor/src/core/tests/stubbed/settings.spec.ts +++ b/frontend/editor/src/core/tests/stubbed/settings.spec.ts @@ -233,6 +233,54 @@ test.describe("Settings dialog", () => { .toBe(originPath); }); + test("browser Back closes the dialog and returns to the origin URL", async ({ + page, + }) => { + await page.goto("/compress", { waitUntil: "domcontentloaded" }); + const originPath = new URL(page.url()).pathname; + const dialog = await openSettings(page); + await dialog.locator('[data-tour="admin-general-nav"]').first().click(); + await page.waitForURL(/\/settings\/general/, { timeout: 5_000 }); + + // Back has to close the modal off the URL alone. react-router's location + // may never have committed the push that opened it, so an effect keyed + // only on `location.pathname` sees no change here and leaves it open. + await page.goBack(); + await expect( + page.locator(".mantine-Modal-content").first(), + ).not.toBeVisible({ timeout: 5_000 }); + await expect + .poll(() => new URL(page.url()).pathname, { timeout: 5_000 }) + .toBe(originPath); + }); + + test("close returns to origin URL when settings was opened from super search", async ({ + page, + }) => { + // Super search pushes /settings/
itself, without going through + // the modal's own tab nav. Close still has to pop that entry rather than + // fall back to the editor home, or the user loses the page they were on. + await page.goto("/compress", { waitUntil: "domcontentloaded" }); + const originPath = new URL(page.url()).pathname; + const input = page.locator("#super-search-input"); + await expect(input).toBeVisible({ timeout: 5_000 }); + await input.click(); + await input.fill("general"); + await page + .getByRole("option", { name: /General/ }) + .first() + .click(); + await expect(page.locator(".modal-container")).toBeVisible({ + timeout: 5_000, + }); + await page.waitForURL(/\/settings\/general/, { timeout: 5_000 }); + + await closeSettings(page); + await expect + .poll(() => new URL(page.url()).pathname, { timeout: 5_000 }) + .toBe(originPath); + }); + test("config sub-sections (System / Features / Endpoints / API Keys) are reachable when present", async ({ page, }) => {