fix(client): show a loading skeleton instead of the login form while resuming a session

.changeset/session-resume-skeleton.md

@@ -0,0 +1,5 @@
+---
+'@rocket.chat/meteor': patch
+---
+
+Shows the loading skeleton instead of flashing the login form while a stored session is being resumed

apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx

@@ -0,0 +1,84 @@
+import { mockAppRoot } from '@rocket.chat/mock-providers';
+import type { AuthenticationContextValue, SessionContextValue } from '@rocket.chat/ui-contexts';
+import { AuthenticationContext, SessionContext } from '@rocket.chat/ui-contexts';
+import { render, screen } from '@testing-library/react';
+import type { ReactNode } from 'react';
+
+import AuthenticationCheck from './AuthenticationCheck';
+import { STORAGE_KEYS } from '../../../lib/sdk/storage';
+
+// The chain below this gate reaches for a great deal of app; what this spec is about is which of the three
+// outcomes the gate picks, so stand the rest of it down.
+jest.mock('./LoggedInArea', () => ({ __esModule: true, default: ({ children }: { children: ReactNode }) => <>{children}</> }));
+jest.mock('./UsernameCheck', () => ({ __esModule: true, default: ({ children }: { children: ReactNode }) => <>{children}</> }));
+jest.mock('./LoginPage', () => ({ __esModule: true, default: () => <div>login-page</div> }));
+jest.mock('../../home/HomeSkeleton', () => ({ __esModule: true, default: () => <div>home-skeleton</div> }));
+
+const renderGate = (root: ReturnType<typeof mockAppRoot>) =>
+	render(<AuthenticationCheck>conference</AuthenticationCheck>, { wrapper: root.build() });
+
+afterEach(() => {
+	localStorage.clear();
+});
+
+it('renders the route for a user who is logged in', () => {
+	renderGate(mockAppRoot().withJohnDoe());
+
+	expect(screen.getByText('conference')).toBeInTheDocument();
+});
+
+it('shows the login page to a visitor with no session', () => {
+	renderGate(mockAppRoot().withAnonymous());
+
+	expect(screen.getByText('login-page')).toBeInTheDocument();
+});
+
+// The bug this guards: a window that opens with a session already stored — a call popout, or any plain reload —
+// has no user until the login is resumed from that token, and showed a login form in the meantime.
+describe('while the session is being resumed', () => {
+	it('shows the skeleton instead of the login page when a stored token is about to be used', () => {
+		localStorage.setItem(STORAGE_KEYS.LOGIN_TOKEN, 'a-stored-token');
+
+		renderGate(mockAppRoot().withAnonymous());
+
+		expect(screen.queryByText('login-page')).not.toBeInTheDocument();
+		expect(screen.getByText('home-skeleton')).toBeInTheDocument();
+	});
+
+	// Someone who was logged out, or whose session the server rejected, must reach the form — otherwise a stale
+	// token in storage would leave them staring at a skeleton forever. The mocked app root has no session
+	// support, so this one supplies the context itself.
+	it('shows the login page when a login is being forced', () => {
+		localStorage.setItem(STORAGE_KEYS.LOGIN_TOKEN, 'a-stale-token');
+
+		const forcingLogin = {
+			query: () => [() => () => undefined, () => true] as ReturnType<SessionContextValue['query']>,
+			dispatch: () => undefined,
+		};
+
+		renderGate(
+			mockAppRoot()
+				.withAnonymous()
+				.wrap((children) => <SessionContext.Provider value={forcingLogin}>{children}</SessionContext.Provider>),
+		);
+
+		expect(screen.getByText('login-page')).toBeInTheDocument();
+	});
+});
+
+// The regression that came with the guard above, and the reason it asks only about the stored token: `isLoggingIn`
+// is true of *any* login in flight, a person typing their password at the form included. Swapping the form for a
+// skeleton mid-attempt lost the rejection — the form came back blank, with neither field marked invalid — and
+// iframe login never appeared at all, since the flow that fetches its URL runs from inside `LoginPage`.
+it('keeps the login page up while someone is logging in at it', () => {
+	const loggingIn = { isLoggingIn: true } as AuthenticationContextValue;
+
+	renderGate(
+		mockAppRoot()
+			.withAnonymous()
+			.wrap((children) => <AuthenticationContext.Provider value={loggingIn}>{children}</AuthenticationContext.Provider>),
+	);
+
+	expect(screen.getByText('login-page')).toBeInTheDocument();
+	expect(screen.queryByText('home-skeleton')).not.toBeInTheDocument();
+});

apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx

@@ -5,6 +5,8 @@ import type { ReactNode } from 'react';
 import LoggedInArea from './LoggedInArea';
 import LoginPage from './LoginPage';
 import UsernameCheck from './UsernameCheck';
+import { STORAGE_KEYS, getStoredItem } from '../../../lib/sdk/storage';
+import HomeSkeleton from '../../home/HomeSkeleton';
 
 /*
  * Anonymous and guest are similar in some way
@@ -22,6 +24,29 @@ const AuthenticationCheck = ({ children, guest }: AuthenticationCheckProps) => {
 	const allowAnonymousRead = useSetting('Accounts_AllowAnonymousRead');
 	const forceLogin = useSession('forceLogin');
 
+	/**
+	 * A window that opens with a session already stored — a call popout, or any plain reload — has no user until
+	 * the login is resumed from that token. Treating "no user yet" as "not logged in" showed a login form for the
+	 * few hundred milliseconds it took, to someone who never asked for one.
+	 *
+	 * The stored token is the whole of the test, and deliberately so. It is written before the window loads and
+	 * removed only on an explicit logout or a failed resume, so it covers the resume from end to end. Asking
+	 * `isLoggingIn` as well looked like it covered the same ground more directly, but it is true of *any* login in
+	 * flight, including one someone is making at the form right now: that unmounted the form mid-attempt, so a
+	 * rejected password came back to a blank form with nothing marked invalid, and iframe login — which runs from
+	 * inside `LoginPage` — could never get as far as showing its own form at all.
+	 *
+	 * The token is read per render rather than subscribed to, so a stale one only falls through on the next
+	 * render. That is safe because every path that rejects a stored token removes it — `makeClientLoggedOut` via
+	 * Meteor's reconnect hook, and `clearStoredCredentials()` from `ensureConnectedAndAuthenticated` and
+	 * `runUserDataSync` — and the expired-token page load clears it before React even mounts.
+	 */
+	const isResumingSession = !user && !forceLogin && !!getStoredItem(STORAGE_KEYS.LOGIN_TOKEN);
+
+	if (isResumingSession) {
+		return <HomeSkeleton />;
+	}
+
 	if (user) {
 		return (
 			<LoggedInArea>