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/hooks/useStoredItem.spec.ts

@@ -0,0 +1,98 @@
+import { act, renderHook } from '@testing-library/react';
+
+import { useStoredItem } from './useStoredItem';
+import { STORAGE_KEYS, removeStoredItem, setStoredItem } from '../lib/sdk/storage';
+
+afterEach(() => {
+	localStorage.clear();
+});
+
+it('reads what is already stored', () => {
+	localStorage.setItem(STORAGE_KEYS.LOGIN_TOKEN, 'a-stored-token');
+
+	const { result } = renderHook(() => useStoredItem(STORAGE_KEYS.LOGIN_TOKEN));
+
+	expect(result.current).toBe('a-stored-token');
+});
+
+it('reads null for a key with nothing under it', () => {
+	const { result } = renderHook(() => useStoredItem(STORAGE_KEYS.LOGIN_TOKEN));
+
+	expect(result.current).toBeNull();
+});
+
+// The whole point of the hook: `localStorage` announces a same-tab write to nobody, so the writers have to.
+it('re-renders when the value is written', () => {
+	const { result } = renderHook(() => useStoredItem(STORAGE_KEYS.LOGIN_TOKEN));
+
+	act(() => {
+		setStoredItem(STORAGE_KEYS.LOGIN_TOKEN, 'a-fresh-token');
+	});
+
+	expect(result.current).toBe('a-fresh-token');
+});
+
+it('re-renders when the value is removed', () => {
+	localStorage.setItem(STORAGE_KEYS.LOGIN_TOKEN, 'a-token-on-its-way-out');
+
+	const { result } = renderHook(() => useStoredItem(STORAGE_KEYS.LOGIN_TOKEN));
+
+	act(() => {
+		removeStoredItem(STORAGE_KEYS.LOGIN_TOKEN);
+	});
+
+	expect(result.current).toBeNull();
+});
+
+// The other half of the same problem: a `storage` event is delivered only to the *other* tabs, so it is the only
+// notice a window resuming a session gets that another tab has logged out from under it.
+it('re-renders when another tab changes the value', () => {
+	localStorage.setItem(STORAGE_KEYS.LOGIN_TOKEN, 'a-token-another-tab-drops');
+
+	const { result } = renderHook(() => useStoredItem(STORAGE_KEYS.LOGIN_TOKEN));
+
+	act(() => {
+		// jsdom does not raise `storage` for writes made in the same window — which is right, and why the same-tab
+		// path needs `notify` — so stand in for the other tab by removing the value and raising the event by hand.
+		localStorage.removeItem(STORAGE_KEYS.LOGIN_TOKEN);
+		window.dispatchEvent(
+			new StorageEvent('storage', { key: STORAGE_KEYS.LOGIN_TOKEN, oldValue: 'a-token-another-tab-drops', newValue: null }),
+		);
+	});
+
+	expect(result.current).toBeNull();
+});
+
+it('lets go of the storage listener with its last subscriber', () => {
+	const addEventListener = jest.spyOn(window, 'addEventListener');
+	const removeEventListener = jest.spyOn(window, 'removeEventListener');
+
+	const { unmount } = renderHook(() => useStoredItem(STORAGE_KEYS.LOGIN_TOKEN));
+
+	expect(addEventListener).toHaveBeenCalledWith('storage', expect.any(Function));
+
+	unmount();
+
+	expect(removeEventListener).toHaveBeenCalledWith('storage', expect.any(Function));
+
+	addEventListener.mockRestore();
+	removeEventListener.mockRestore();
+});
+
+it('ignores a write to another key', () => {
+	localStorage.setItem(STORAGE_KEYS.LOGIN_TOKEN, 'a-stored-token');
+
+	const renders = jest.fn();
+	renderHook(() => {
+		renders();
+		return useStoredItem(STORAGE_KEYS.LOGIN_TOKEN);
+	});
+
+	const rendersBefore = renders.mock.calls.length;
+
+	act(() => {
+		setStoredItem(STORAGE_KEYS.USER_ID, 'john.doe');
+	});
+
+	expect(renders).toHaveBeenCalledTimes(rendersBefore);
+});

apps/meteor/client/hooks/useStoredItem.ts

@@ -0,0 +1,24 @@
+import { useCallback, useSyncExternalStore } from 'react';
+
+import type { StorageKey } from '../lib/sdk/storage';
+import { getStoredItem, subscribeStoredItem } from '../lib/sdk/storage';
+
+const getServerSnapshot = (): string | null => null;
+
+/**
+ * Reads a value out of the client-side persistent storage, and re-renders when it changes.
+ *
+ * `getStoredItem` on its own is a plain `localStorage` read: a component that calls it during render picks up a
+ * write only if something *else* it subscribes to changes at the same time. Where the stored value is what
+ * decides the render — `AuthenticationCheck` reading the login token — that coincidence is not something to
+ * depend on, hence this.
+ *
+ * The snapshot is the stored string itself, so React's `Object.is` bail-out compares it by value and a `notify`
+ * for some other key costs nothing beyond the read.
+ */
+export const useStoredItem = (key: StorageKey): string | null =>
+	useSyncExternalStore(
+		subscribeStoredItem,
+		useCallback(() => getStoredItem(key), [key]),
+		getServerSnapshot,
+	);

apps/meteor/client/lib/sdk/storage.ts

@@ -32,11 +32,63 @@ const getStorage = (): Storage | undefined => {
 	return getStorageForBackend(storageBackend);
 };
 
+/**
+ * A same-tab write to `localStorage` announces itself to nobody: the DOM `storage` event is only ever delivered
+ * to the *other* tabs. Anything that renders from a stored value — the session-resume gate in
+ * `AuthenticationCheck` above all — therefore has to be told, or it reads a stale value and keeps it until some
+ * unrelated state change happens to render it again. Every mutation below goes through `notify`, and
+ * `subscribeStoredItem` is what `useSyncExternalStore` consumers hook into.
+ *
+ * The `storage` event is the other half of that, and covers exactly the writes `notify` cannot see: a logout in
+ * another tab takes the token out from under a window that is still resuming, and that window has no other way
+ * to hear about it. `ensureConnectedAndAuthenticated` reads the token only once it has a connection, so a token
+ * that left in the meantime makes it return without a user, without clearing anything, and without a resume to
+ * fail — leaving a gate that never re-renders sitting on a token that is no longer there.
+ */
+const listeners = new Set<() => void>();
+
+const notify = (): void => {
+	for (const listener of listeners) {
+		listener();
+	}
+};
+
+/**
+ * Attached with the first subscriber and dropped with the last, so imperative readers pay nothing for it. The
+ * event is not filtered by key or by storage area: `notify` is keyless anyway, and a subscriber that reads a
+ * key nobody touched re-reads an unchanged snapshot, which React discards.
+ */
+const handleStorageEvent = (): void => {
+	notify();
+};
+
+export const subscribeStoredItem = (listener: () => void): (() => void) => {
+	if (listeners.size === 0 && typeof window !== 'undefined') {
+		window.addEventListener('storage', handleStorageEvent);
+	}
+
+	listeners.add(listener);
+
+	return () => {
+		listeners.delete(listener);
+
+		if (listeners.size === 0 && typeof window !== 'undefined') {
+			window.removeEventListener('storage', handleStorageEvent);
+		}
+	};
+};
+
 export const getStoredItem = (key: StorageKey): string | null => getStorage()?.getItem(key) ?? null;
 
-export const setStoredItem = (key: StorageKey, value: string): void => getStorage()?.setItem(key, value);
+export const setStoredItem = (key: StorageKey, value: string): void => {
+	getStorage()?.setItem(key, value);
+	notify();
+};
 
-export const removeStoredItem = (key: StorageKey): void => getStorage()?.removeItem(key);
+export const removeStoredItem = (key: StorageKey): void => {
+	getStorage()?.removeItem(key);
+	notify();
+};
 
 let storageBackend: StorageBackend = 'local';
 
@@ -50,6 +102,7 @@ export const setStorageBackend = (backend: StorageBackend): boolean => {
 	}
 
 	storageBackend = backend;
+	notify();
 	return true;
 };
 

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

@@ -0,0 +1,173 @@
+import { mockAppRoot } from '@rocket.chat/mock-providers';
+import type { AuthenticationContextValue, SessionContextValue } from '@rocket.chat/ui-contexts';
+import { AuthenticationContext, SessionContext, UserContext } from '@rocket.chat/ui-contexts';
+import { act, render, screen } from '@testing-library/react';
+import type { ReactNode } from 'react';
+import { useContext } from 'react';
+
+import AuthenticationCheck from './AuthenticationCheck';
+import { STORAGE_KEYS, removeStoredItem } 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 stored token is read through a subscription, not per render, and this is why: a rejected resume clears
+	// the credentials without anything else about the gate changing — no user arrives, the status stays healthy —
+	// so a per-render read would sit on the stale token with no next render to correct it.
+	it('falls through to the login page when the stored token is cleared under it', () => {
+		localStorage.setItem(STORAGE_KEYS.LOGIN_TOKEN, 'a-token-the-server-rejects');
+
+		renderGate(mockAppRoot().withAnonymous());
+
+		expect(screen.getByText('home-skeleton')).toBeInTheDocument();
+
+		act(() => {
+			removeStoredItem(STORAGE_KEYS.LOGIN_TOKEN);
+		});
+
+		expect(screen.getByText('login-page')).toBeInTheDocument();
+		expect(screen.queryByText('home-skeleton')).not.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();
+});
+
+// Overrides whatever user the mocked root supplies, without changing the shape of the tree — the gate has to keep
+// the same instance across the switch for the test to be about anything.
+const MaybeSessionEnded = ({ ended, children }: { ended: boolean; children: ReactNode }) => {
+	const value = useContext(UserContext);
+
+	return <UserContext.Provider value={ended ? { ...value, userId: undefined, user: null } : value}>{children}</UserContext.Provider>;
+};
+
+// Deleting your own account ends the session server-side but clears nothing locally: the stored token stays put,
+// and reading it alone the gate answered "a resume is in flight" and held the skeleton up for good. A user that
+// has already been seen going away is an ended session, whatever storage still says.
+it('shows the login page when the user goes away with a token still stored', () => {
+	localStorage.setItem(STORAGE_KEYS.LOGIN_TOKEN, 'a-token-nobody-cleared');
+
+	const Root = mockAppRoot().withJohnDoe().build();
+
+	const { rerender } = render(
+		<Root>
+			<MaybeSessionEnded ended={false}>
+				<AuthenticationCheck>conference</AuthenticationCheck>
+			</MaybeSessionEnded>
+		</Root>,
+	);
+
+	expect(screen.getByText('conference')).toBeInTheDocument();
+
+	rerender(
+		<Root>
+			<MaybeSessionEnded ended={true}>
+				<AuthenticationCheck>conference</AuthenticationCheck>
+			</MaybeSessionEnded>
+		</Root>,
+	);
+
+	expect(screen.getByText('login-page')).toBeInTheDocument();
+	expect(screen.queryByText('home-skeleton')).not.toBeInTheDocument();
+});