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,116 @@
+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();
+	});
+
+	// The one way a stored token could strand someone: it is cleared when a server *rejects* it, so a server that
+	// never answers at all clears nothing. Once the connection stops trying, the form has to be reachable.
+	it.each(['waiting', 'failed'] as const)('shows the login page when the connection has given up (%s)', (status) => {
+		localStorage.setItem(STORAGE_KEYS.LOGIN_TOKEN, 'a-stored-token');
+
+		renderGate(mockAppRoot().withAnonymous().withServerContext({ connected: false, status }));
+
+		expect(screen.getByText('login-page')).toBeInTheDocument();
+		expect(screen.queryByText('home-skeleton')).not.toBeInTheDocument();
+	});
+
+	// `offline` is not a give-up state, however much it reads like one: the DDP SDK starts every page load `idle`
+	// and that is reported as `offline`, so counting it would show the form on healthy reloads — the very bug.
+	it('keeps the skeleton up while the connection is merely idle', () => {
+		localStorage.setItem(STORAGE_KEYS.LOGIN_TOKEN, 'a-stored-token');
+
+		renderGate(mockAppRoot().withAnonymous().withServerContext({ connected: false, status: 'offline' }));
+
+		expect(screen.getByText('home-skeleton')).toBeInTheDocument();
+		expect(screen.queryByText('login-page')).not.toBeInTheDocument();
+	});
+
+	// ...but not while it is still the ordinary first connect of a page load, which is the flash this removes.
+	it('keeps the skeleton up while the connection is still being made', () => {
+		localStorage.setItem(STORAGE_KEYS.LOGIN_TOKEN, 'a-stored-token');
+
+		renderGate(mockAppRoot().withAnonymous().withServerContext({ connected: false, status: 'connecting' }));
+
+		expect(screen.getByText('home-skeleton')).toBeInTheDocument();
+		expect(screen.queryByText('login-page')).not.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

@@ -1,10 +1,13 @@
-import { useSession, useUser, useSetting } from '@rocket.chat/ui-contexts';
+import { useConnectionStatus, useSession, useUser, useSetting } from '@rocket.chat/ui-contexts';
 import RegistrationRoute from '@rocket.chat/web-ui-registration';
 import type { ReactNode } from 'react';
+import { useEffect, useState } 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
@@ -17,11 +20,67 @@ import UsernameCheck from './UsernameCheck';
  */
 export type AuthenticationCheckProps = { children: ReactNode; guest?: boolean };
 
+/**
+ * The connection states that mean a server was reached for and lost, as opposed to not having answered yet.
+ *
+ * `offline` is deliberately not one of them, however much it sounds like the plainest case: the DDP SDK begins
+ * every page load `idle`, and `sdkStatusToMeteor` reports `idle` as `offline`. Counting it would give up on the
+ * connection before it had been attempted, on exactly the healthy reloads this exists to protect.
+ */
+const hasGivenUp = (status: ReturnType<typeof useConnectionStatus>['status']): boolean => status === 'waiting' || status === 'failed';
+
 const AuthenticationCheck = ({ children, guest }: AuthenticationCheckProps) => {
 	const user = useUser();
 	const allowAnonymousRead = useSetting('Accounts_AllowAnonymousRead');
 	const forceLogin = useSession('forceLogin');
 
+	const { status } = useConnectionStatus();
+
+	/**
+	 * A resume needs a server to answer it, and waiting on one that never will is the one way this can strand
+	 * someone: the token is only cleared when a server *rejects* it, so an unreachable server — a dropped network,
+	 * a captive portal, a workspace that is down — clears nothing and would leave the skeleton up for good.
+	 *
+	 * Which states count is the whole of the care here — see `hasGivenUp`, and note that neither `connecting` nor
+	 * `offline` is one of them. Once a state that counts has been seen it stands: a retry flapping between
+	 * `waiting` and `connecting` must not flap the form back into a skeleton. A resume that succeeds later brings
+	 * a user with it, which wins anyway.
+	 *
+	 * Seeded from the status rather than latched from `false`, so a mount that is *already* offline picks the form
+	 * on its first render instead of showing a frame of skeleton on the way to it.
+	 */
+	const [unreachable, setUnreachable] = useState(() => hasGivenUp(status));
+
+	useEffect(() => {
+		if (hasGivenUp(status)) {
+			setUnreachable(true);
+		}
+	}, [status]);
+
+	/**
+	 * 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. Those are all
+	 * rejections, though, which is why the unreachable case above is bounded separately.
+	 */
+	const isResumingSession = !user && !forceLogin && !unreachable && !!getStoredItem(STORAGE_KEYS.LOGIN_TOKEN);
+
+	if (isResumingSession) {
+		return <HomeSkeleton />;
+	}
+
 	if (user) {
 		return (
 			<LoggedInArea>