fix(studio): prevent auth monitor reload loop (#7118)
This commit is contained in:
Long Yixing
GitHub
parent
2f3eae9846
commit
c80e7d317a
7 changed files with 175 additions and 17 deletions
|
|
@ -44,6 +44,7 @@ declare module "@tanstack/react-router" {
|
|||
interface StaticDataRouteOption {
|
||||
title?: string;
|
||||
titleKey?: TranslationKey;
|
||||
isAuthFlow?: boolean;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -103,6 +104,9 @@ function RootLayout() {
|
|||
const t = useT();
|
||||
const pathname = useRouterState({ select: (s) => s.location.pathname });
|
||||
const hideNavbar = HIDDEN_NAVBAR_ROUTES.includes(pathname);
|
||||
const isAuthFlowRoute = useMatches({
|
||||
select: (matches) => matches.some((match) => match.staticData.isAuthFlow),
|
||||
});
|
||||
// Exact match: a prefix would treat /chatty as chat, hiding its not-found UI.
|
||||
const isChatRoute = pathname === "/chat";
|
||||
const { pinned, setPinned, togglePinned } = useSidebarPin();
|
||||
|
|
@ -161,7 +165,8 @@ function RootLayout() {
|
|||
});
|
||||
|
||||
const settingsDialogOpen = useSettingsDialogStore((s) => s.open);
|
||||
const documentTitle = settingsDialogOpen ? t("settings.title") : matchedTitle;
|
||||
const documentTitle =
|
||||
settingsDialogOpen && !isAuthFlowRoute ? t("settings.title") : matchedTitle;
|
||||
|
||||
useLayoutEffect(() => {
|
||||
document.title = documentTitle
|
||||
|
|
@ -170,9 +175,13 @@ function RootLayout() {
|
|||
}, [documentTitle]);
|
||||
|
||||
useEffect(() => {
|
||||
if (isAuthFlowRoute) {
|
||||
useSettingsDialogStore.getState().closeDialog();
|
||||
}
|
||||
const handler = (e: KeyboardEvent) => {
|
||||
if (e.defaultPrevented) return;
|
||||
if ((e.metaKey || e.ctrlKey) && e.key === ",") {
|
||||
if (isAuthFlowRoute) return;
|
||||
e.preventDefault();
|
||||
useSettingsDialogStore.getState().openDialog();
|
||||
return;
|
||||
|
|
@ -196,7 +205,7 @@ function RootLayout() {
|
|||
};
|
||||
window.addEventListener("keydown", handler);
|
||||
return () => window.removeEventListener("keydown", handler);
|
||||
}, [navigate]);
|
||||
}, [isAuthFlowRoute, navigate]);
|
||||
|
||||
useEffect(() => {
|
||||
if (isChatRoute) return;
|
||||
|
|
@ -219,7 +228,7 @@ function RootLayout() {
|
|||
return (
|
||||
<AppProvider>
|
||||
<PersonalizationSyncMount />
|
||||
<SettingsDialog />
|
||||
{!isAuthFlowRoute && <SettingsDialog />}
|
||||
<RemoteCodeConsentDialog />
|
||||
{hideNavbar ? (
|
||||
<main className="flex-1 pt-[var(--studio-hidden-route-top-inset,0px)] [--studio-titlebar-height:var(--studio-hidden-route-top-inset,0px)]">
|
||||
|
|
|
|||
|
|
@ -15,7 +15,7 @@ const ChangePasswordPage = lazy(() =>
|
|||
export const Route = createRoute({
|
||||
getParentRoute: () => rootRoute,
|
||||
path: "/change-password",
|
||||
staticData: { title: "Change Password" },
|
||||
staticData: { title: "Change Password", isAuthFlow: true },
|
||||
beforeLoad: () => requirePasswordChangeFlow(),
|
||||
component: ChangePasswordPage,
|
||||
});
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ const LoginPage = lazy(() =>
|
|||
export const Route = createRoute({
|
||||
getParentRoute: () => rootRoute,
|
||||
path: "/login",
|
||||
staticData: { title: "Login" },
|
||||
staticData: { title: "Login", isAuthFlow: true },
|
||||
beforeLoad: () => requireGuest(),
|
||||
component: LoginPage,
|
||||
});
|
||||
|
|
|
|||
|
|
@ -17,7 +17,7 @@ const WizardLayout = lazy(() =>
|
|||
export const Route = createRoute({
|
||||
getParentRoute: () => rootRoute,
|
||||
path: "/onboarding",
|
||||
staticData: { title: "Onboarding" },
|
||||
staticData: { title: "Onboarding", isAuthFlow: true },
|
||||
beforeLoad: () => requireAuth(),
|
||||
validateSearch: (search: Record<string, unknown>): OnboardingSearch => ({
|
||||
redirectTo: typeof search.redirectTo === "string" ? search.redirectTo : undefined,
|
||||
|
|
|
|||
|
|
@ -82,6 +82,10 @@ async function redirectToAuth(): Promise<void> {
|
|||
// Fall through to /login on error
|
||||
}
|
||||
|
||||
if (window.location.pathname === target) {
|
||||
isRedirecting = false;
|
||||
return;
|
||||
}
|
||||
window.location.href = target;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -160,6 +160,15 @@ with sync_playwright() as p:
|
|||
# collapse run startViewTransition() which can leave <html> intercepting
|
||||
# pointer events for a beat after each route swap. See _playwright_robust.py.
|
||||
install_view_transition_killer(ctx)
|
||||
system_requests: list[str] = []
|
||||
ctx.on(
|
||||
"request",
|
||||
lambda request: (
|
||||
system_requests.append(request.url)
|
||||
if request.url.split("?", 1)[0].endswith("/api/system")
|
||||
else None
|
||||
),
|
||||
)
|
||||
page = ctx.new_page()
|
||||
# 60s default (was 30s): macos-14 under --single-process Chromium is
|
||||
# slow enough that renders/webfonts/lazy routes routinely crowd 30s.
|
||||
|
|
@ -1259,11 +1268,10 @@ with sync_playwright() as p:
|
|||
)
|
||||
|
||||
# ─────────────────────────────────────────────────────
|
||||
# 17. Shutdown via the account menu. The "Stop server" action
|
||||
# POSTs /api/shutdown, swaps in the "Unsloth Studio has stopped"
|
||||
# placeholder, and /api/health goes unreachable shortly after.
|
||||
# 17. Persisted monitor auth boundary, then shutdown. A monitor left open
|
||||
# must stay dormant on /login and resume after successful authentication.
|
||||
# ─────────────────────────────────────────────────────
|
||||
step("Shutdown via account menu")
|
||||
step("persisted monitor stays dormant on /login and resumes after auth")
|
||||
# Start fresh after the CLI rotation invalidates this browser session.
|
||||
# Stay in the SAME context: macOS Chromium runs --single-process, where
|
||||
# closing the last context kills the browser and a second context cannot
|
||||
|
|
@ -1273,6 +1281,13 @@ with sync_playwright() as p:
|
|||
ctx.clear_cookies()
|
||||
except Exception as exc:
|
||||
info(f"WARN clearing stale session cookies failed: {exc!r}")
|
||||
robust_evaluate(
|
||||
page,
|
||||
"""() => localStorage.setItem(
|
||||
"unsloth_monitor_overlay",
|
||||
JSON.stringify({ state: { isOpen: true, isMinimized: false }, version: 0 })
|
||||
)""",
|
||||
)
|
||||
# Auth tokens live in localStorage, and /login's guest guard redirects on
|
||||
# their mere presence, so drop them before navigating.
|
||||
try:
|
||||
|
|
@ -1291,6 +1306,7 @@ with sync_playwright() as p:
|
|||
except Exception:
|
||||
pass
|
||||
page = _fresh_page
|
||||
login_system_request_count = len(system_requests)
|
||||
|
||||
# Re-login with NEW2 for a valid /api/shutdown token. Route changes can
|
||||
# still abort or interrupt this navigation, so the field wait below is the
|
||||
|
|
@ -1311,6 +1327,14 @@ with sync_playwright() as p:
|
|||
info(f"goto /login interrupted ({exc!r}); password-field wait will confirm /login")
|
||||
pw_field = page.locator("#password")
|
||||
pw_field.wait_for(state = "visible", timeout = 60_000)
|
||||
page.keyboard.press("Control+,")
|
||||
page.wait_for_timeout(5_500)
|
||||
if len(system_requests) != login_system_request_count:
|
||||
raise AssertionError(
|
||||
"persisted monitor requested /api/system while /login was active"
|
||||
)
|
||||
if "/login" not in page.url:
|
||||
raise AssertionError(f"login route reloaded or redirected unexpectedly: {page.url}")
|
||||
pw_field.fill(NEW2)
|
||||
# Wait on the login POST so a transient 4xx/5xx is caught and retried
|
||||
# here, not swallowed until the out-of-loop composer wait.
|
||||
|
|
@ -1384,8 +1408,17 @@ with sync_playwright() as p:
|
|||
# merely-slow composer look like a broken login.
|
||||
composer = page.locator('textarea[aria-label="Message input"]')
|
||||
composer.wait_for(state = "visible", timeout = 60_000)
|
||||
monitor_deadline = time.time() + 10
|
||||
while len(system_requests) == login_system_request_count and time.time() < monitor_deadline:
|
||||
page.wait_for_timeout(100)
|
||||
if len(system_requests) == login_system_request_count:
|
||||
fail("persisted monitor did not resume /api/system polling after login")
|
||||
if page.get_by_role("dialog", name = re.compile(r"^Settings$")).count() != 0:
|
||||
fail("settings shortcut on /login left the dialog open after authentication")
|
||||
info("OK persisted monitor stayed dormant on /login and resumed after authentication")
|
||||
shoot("18-relogin-with-NEW2")
|
||||
|
||||
step("Shutdown via account menu")
|
||||
acct_btn = page.locator('button[aria-label$=" account menu"]').first
|
||||
if acct_btn.count() == 0:
|
||||
fail("account menu button missing -- can't reach Shutdown")
|
||||
|
|
|
|||
|
|
@ -1,22 +1,27 @@
|
|||
# SPDX-License-Identifier: AGPL-3.0-only
|
||||
# Copyright 2026-present the Unsloth AI Inc. team. All rights reserved.
|
||||
|
||||
"""Pin the auth-form input-count contract on the change-password page.
|
||||
"""Fast source and runtime contracts for Studio's frontend authentication flows.
|
||||
|
||||
PR #5490 added a third "Current password" input, regressing first-boot UX to
|
||||
three inputs; PR #5545 restores two by rendering it only when BOOTSTRAP is absent.
|
||||
These tests inspect the source directly (no Studio/browser/network); runtime is
|
||||
covered by tests/studio/playwright_chat_ui.py."""
|
||||
Issue #7114 covers auth redirects and the persisted System monitor; its browser
|
||||
lifecycle remains covered by tests/studio/playwright_chat_ui.py."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import re
|
||||
import shutil
|
||||
import subprocess
|
||||
import textwrap
|
||||
from pathlib import Path
|
||||
|
||||
AUTH_FORM = (
|
||||
Path(__file__).resolve().parents[2]
|
||||
/ "studio/frontend/src/features/auth/components/auth-form.tsx"
|
||||
)
|
||||
import pytest
|
||||
|
||||
REPO = Path(__file__).resolve().parents[2]
|
||||
FRONTEND = REPO / "studio/frontend/src"
|
||||
AUTH_FORM = FRONTEND / "features/auth/components/auth-form.tsx"
|
||||
AUTH_API = FRONTEND / "features/auth/api.ts"
|
||||
|
||||
CONDITIONAL_OPENER = "{!hasBootstrapPassword && ("
|
||||
|
||||
|
|
@ -161,3 +166,110 @@ def test_login_jsx_declares_exactly_one_password_input():
|
|||
assert len(pw_ids) == 1, (
|
||||
f"login JSX must declare exactly one password-typed input; " f"found {pw_ids!r}"
|
||||
)
|
||||
|
||||
|
||||
def test_auth_flow_routes_do_not_mount_global_settings():
|
||||
root = (FRONTEND / "app/routes/__root.tsx").read_text()
|
||||
assert "{!isAuthFlowRoute && <SettingsDialog />}" in root
|
||||
assert "useSettingsDialogStore.getState().closeDialog();" in root
|
||||
assert "if (isAuthFlowRoute) return;" in root
|
||||
for route in ("login", "change-password", "onboarding"):
|
||||
assert "isAuthFlow: true" in (FRONTEND / f"app/routes/{route}.tsx").read_text()
|
||||
|
||||
|
||||
def test_auth_redirect_targets_are_idempotent_and_concurrent(tmp_path: Path):
|
||||
if shutil.which("node") is None:
|
||||
pytest.skip("node not available")
|
||||
probe = subprocess.run(
|
||||
["node", "--experimental-strip-types", "--version"],
|
||||
capture_output = True,
|
||||
text = True,
|
||||
timeout = 5,
|
||||
)
|
||||
if probe.returncode != 0:
|
||||
pytest.skip("node --experimental-strip-types not available")
|
||||
|
||||
source = (
|
||||
AUTH_API.read_text()
|
||||
.replace('from "@/lib/api-base"', 'from "./stubs.mjs"')
|
||||
.replace('from "./session"', 'from "./stubs.mjs"')
|
||||
)
|
||||
(tmp_path / "api.ts").write_text(source)
|
||||
(tmp_path / "stubs.mjs").write_text(
|
||||
textwrap.dedent("""
|
||||
let access = null, refresh = null, passwordChange = false;
|
||||
export const apiUrl = (path) => path;
|
||||
export const isTauri = false;
|
||||
export const reset = (a = null, r = null) => { access = a; refresh = r; passwordChange = false; };
|
||||
export const clearAuthTokens = () => { access = null; refresh = null; };
|
||||
export const getAuthToken = () => access;
|
||||
export const getRefreshToken = () => refresh;
|
||||
export const mustChangePassword = () => passwordChange;
|
||||
export const setMustChangePassword = (value) => { passwordChange = value; };
|
||||
export const storeAuthTokens = (a, r) => { access = a; refresh = r; };
|
||||
""")
|
||||
)
|
||||
script = textwrap.dedent("""
|
||||
import assert from "node:assert/strict";
|
||||
import { reset } from "./stubs.mjs";
|
||||
const response = (status, value) => new Response(
|
||||
value && JSON.stringify(value), { status }
|
||||
);
|
||||
const settle = () => new Promise((resolve) => setImmediate(resolve));
|
||||
const load = (name) => import(`./api.ts?${name}`);
|
||||
const locationAt = (pathname) => {
|
||||
const assigned = [];
|
||||
globalThis.window = { location: { pathname,
|
||||
set href(value) { assigned.push(value); this.pathname = value; }
|
||||
}};
|
||||
return assigned;
|
||||
};
|
||||
async function redirectCase(path, requiresChange, name, repeats = 1) {
|
||||
reset();
|
||||
const assigned = locationAt(path);
|
||||
let statusCalls = 0;
|
||||
globalThis.fetch = async (input) => {
|
||||
if (input === "/api/auth/status") {
|
||||
statusCalls += 1;
|
||||
return response(200, { requires_password_change: requiresChange });
|
||||
}
|
||||
return response(401);
|
||||
};
|
||||
const { authFetch } = await load(name);
|
||||
for (let i = 0; i < repeats; i += 1) {
|
||||
await authFetch("/api/system");
|
||||
await settle();
|
||||
}
|
||||
return { assigned, statusCalls };
|
||||
}
|
||||
const login = await redirectCase("/login", false, "login", 2);
|
||||
assert.deepEqual(login, { assigned: [], statusCalls: 2 });
|
||||
const change = await redirectCase("/chat", true, "change");
|
||||
assert.deepEqual(change.assigned, ["/change-password"]);
|
||||
|
||||
reset("expired", "refresh");
|
||||
const assigned = locationAt("/chat");
|
||||
const calls = { refresh: 0, status: 0 };
|
||||
globalThis.fetch = async (input) => {
|
||||
if (input === "/api/auth/refresh") calls.refresh += 1;
|
||||
if (input === "/api/auth/status") {
|
||||
calls.status += 1;
|
||||
return response(200, { requires_password_change: false });
|
||||
}
|
||||
return response(401);
|
||||
};
|
||||
const { authFetch } = await load("concurrent");
|
||||
await Promise.all([authFetch("/api/system"), authFetch("/api/system")]);
|
||||
await settle();
|
||||
assert.deepEqual(calls, { refresh: 1, status: 1 });
|
||||
assert.deepEqual(assigned, ["/login"]);
|
||||
""")
|
||||
result = subprocess.run(
|
||||
["node", "--experimental-strip-types", "--no-warnings", "--input-type=module"],
|
||||
input = script,
|
||||
cwd = tmp_path,
|
||||
capture_output = True,
|
||||
text = True,
|
||||
timeout = 30,
|
||||
)
|
||||
assert result.returncode == 0, f"stderr: {result.stderr}\nstdout: {result.stdout}"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue