diff --git a/.changeset/session-resume-skeleton.md b/.changeset/session-resume-skeleton.md
new file mode 100644
index 0000000000000..d969665f612a2
--- /dev/null
+++ b/.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
diff --git a/apps/meteor/client/hooks/useStoredItem.spec.ts b/apps/meteor/client/hooks/useStoredItem.spec.ts
new file mode 100644
index 0000000000000..1d1ca8d347e7a
--- /dev/null
+++ b/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);
+});
diff --git a/apps/meteor/client/hooks/useStoredItem.ts b/apps/meteor/client/hooks/useStoredItem.ts
new file mode 100644
index 0000000000000..49352ab490703
--- /dev/null
+++ b/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,
+ );
diff --git a/apps/meteor/client/lib/sdk/storage.ts b/apps/meteor/client/lib/sdk/storage.ts
index b2aa37dd3d5bf..bcc691a176a78 100644
--- a/apps/meteor/client/lib/sdk/storage.ts
+++ b/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;
};
diff --git a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx
new file mode 100644
index 0000000000000..d1669e205445e
--- /dev/null
+++ b/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: () => login-page
}));
+jest.mock('../../home/HomeSkeleton', () => ({ __esModule: true, default: () => home-skeleton
}));
+
+const renderGate = (root: ReturnType) =>
+ render(conference, { 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,
+ dispatch: () => undefined,
+ };
+
+ renderGate(
+ mockAppRoot()
+ .withAnonymous()
+ .wrap((children) => {children}),
+ );
+
+ 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) => {children}),
+ );
+
+ 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 {children};
+};
+
+// 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(
+
+
+ conference
+
+ ,
+ );
+
+ expect(screen.getByText('conference')).toBeInTheDocument();
+
+ rerender(
+
+
+ conference
+
+ ,
+ );
+
+ expect(screen.getByText('login-page')).toBeInTheDocument();
+ expect(screen.queryByText('home-skeleton')).not.toBeInTheDocument();
+});
diff --git a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx
index 41e0d9bdb6d7f..37276ae272000 100644
--- a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx
+++ b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx
@@ -1,10 +1,14 @@
-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, useRef, useState } from 'react';
import LoggedInArea from './LoggedInArea';
import LoginPage from './LoginPage';
import UsernameCheck from './UsernameCheck';
+import { useStoredItem } from '../../../hooks/useStoredItem';
+import { STORAGE_KEYS } from '../../../lib/sdk/storage';
+import HomeSkeleton from '../../home/HomeSkeleton';
/*
* Anonymous and guest are similar in some way
@@ -17,11 +21,81 @@ 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['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 resume is something that happens *before* the first user of a mount, never after one. Once a user has been
+ * seen, a user that goes away again is a session that ended — a logout, an account deleted from under itself,
+ * a token the server revoked — and the form, not a skeleton, is the answer. Latching this is what keeps the
+ * skeleton bounded by something the component owns, rather than by every path that drops a session
+ * remembering to clear the stored token on its way out: deleting your own account does not, and left the
+ * skeleton up for good.
+ */
+ const hasSeenUser = useRef(false);
+ if (user) {
+ hasSeenUser.current = true;
+ }
+
+ /**
+ * 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 on an explicit logout or a rejected 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.
+ *
+ * Subscribed to rather than read per render, because the writes that matter here are same-tab ones and those
+ * announce themselves to nobody: `clearStoredCredentials()` ends by nulling a connection userId that is
+ * *already* null on a resume that never got a user, so neither the Tracker dep nor the userId store fires and
+ * there is no next render to fall through on. See `useStoredItem`.
+ */
+ const loginToken = useStoredItem(STORAGE_KEYS.LOGIN_TOKEN);
+
+ const isResumingSession = !user && !hasSeenUser.current && !forceLogin && !unreachable && !!loginToken;
+
+ if (isResumingSession) {
+ return ;
+ }
+
if (user) {
return (