From 898b90986e68f45c8a403e3154ba9e5beb21db60 Mon Sep 17 00:00:00 2001 From: Rodrigo Nascimento Date: Tue, 25 Aug 2026 18:03:33 -0300 Subject: [PATCH 1/7] fix(client): show a loading skeleton instead of the login form while resuming a session 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" flashed the login form for the few hundred milliseconds the resume took, at someone who never asked for one. Gate on the stored login token alone. 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. `isLoggingIn` deliberately plays no part: it is true of any login in flight, a person typing their password at the form included, and using it here unmounted the form mid-attempt. A stale token cannot strand anyone on the skeleton -- every path that rejects a stored token removes it (`makeClientLoggedOut` via Meteor's reconnect hook, and `clearStoredCredentials()` from `ensureConnectedAndAuthenticated` and `runUserDataSync`), and `forceLogin` short-circuits the gate outright. Co-Authored-By: Claude Fable 5 --- .changeset/session-resume-skeleton.md | 5 ++ .../MainLayout/AuthenticationCheck.spec.tsx | 84 +++++++++++++++++++ .../root/MainLayout/AuthenticationCheck.tsx | 25 ++++++ 3 files changed, 114 insertions(+) create mode 100644 .changeset/session-resume-skeleton.md create mode 100644 apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx 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/views/root/MainLayout/AuthenticationCheck.spec.tsx b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx new file mode 100644 index 0000000000000..666961fba6154 --- /dev/null +++ b/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: () =>
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(); + }); + + // 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(); +}); diff --git a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx index 41e0d9bdb6d7f..915e40bd759d4 100644 --- a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx +++ b/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 ; + } + if (user) { return ( From 51abe0cfcecce9a247e26c55e1a62448d207f056 Mon Sep 17 00:00:00 2001 From: Rodrigo Nascimento Date: Wed, 26 Aug 2026 11:13:18 -0300 Subject: [PATCH 2/7] fix(client): fall through to the login form when the connection gives up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A stored token is only cleared when a server rejects it, so a server that never answers — a dropped network, a captive portal, a workspace that is down — cleared nothing and left the resume skeleton up for good. Bound it on the connection status, latched so a flapping retry cannot flap the form back into a skeleton, and only on the states where the connection has stopped trying: 'connecting' is the ordinary first moment of a page load. Co-Authored-By: Claude Fable 5 --- .../MainLayout/AuthenticationCheck.spec.tsx | 21 ++++++++++++++ .../root/MainLayout/AuthenticationCheck.tsx | 28 +++++++++++++++++-- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx index 666961fba6154..f3817c3368907 100644 --- a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx +++ b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx @@ -45,6 +45,27 @@ describe('while the session is being resumed', () => { 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('shows the login page once the connection has given up', async () => { + localStorage.setItem(STORAGE_KEYS.LOGIN_TOKEN, 'a-stored-token'); + + renderGate(mockAppRoot().withAnonymous().withServerContext({ connected: false, status: 'waiting' })); + + expect(await screen.findByText('login-page')).toBeInTheDocument(); + expect(screen.queryByText('home-skeleton')).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. diff --git a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx index 915e40bd759d4..3b7bfe3941f23 100644 --- a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx +++ b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx @@ -1,6 +1,7 @@ -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'; @@ -24,6 +25,26 @@ const AuthenticationCheck = ({ children, guest }: AuthenticationCheckProps) => { 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. + * + * `connecting` is not that: it is the ordinary first moment of every page load, and falling through on it is + * exactly the login-form flash this exists to remove. Only the states where the connection has stopped trying + * count, and once one 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. + */ + const [unreachable, setUnreachable] = useState(false); + + useEffect(() => { + if (status === 'waiting' || status === 'failed' || status === 'offline') { + 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 @@ -39,9 +60,10 @@ const AuthenticationCheck = ({ children, guest }: AuthenticationCheckProps) => { * 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. + * `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 && !!getStoredItem(STORAGE_KEYS.LOGIN_TOKEN); + const isResumingSession = !user && !forceLogin && !unreachable && !!getStoredItem(STORAGE_KEYS.LOGIN_TOKEN); if (isResumingSession) { return ; From b783071a54b2daa9497eb7d727fed2df23d96052 Mon Sep 17 00:00:00 2001 From: Rodrigo Nascimento Date: Wed, 26 Aug 2026 11:28:14 -0300 Subject: [PATCH 3/7] fix(client): seed the unreachable latch from the current connection status A window that mounts while the connection has already given up rendered a frame of skeleton before the effect flipped the latch. Seeding the state from the status picks the login form on the first render instead. Co-Authored-By: Claude Fable 5 --- .../root/MainLayout/AuthenticationCheck.spec.tsx | 8 +++++--- .../views/root/MainLayout/AuthenticationCheck.tsx | 11 +++++++++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx index f3817c3368907..27df4a48b6445 100644 --- a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx +++ b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx @@ -47,12 +47,14 @@ describe('while the session is being resumed', () => { // 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('shows the login page once the connection has given up', async () => { + // Synchronously: a window that mounts with the connection already given up must pick the form on its first + // render, rather than showing a frame of skeleton on the way to it. + it.each(['waiting', 'failed', 'offline'] 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: 'waiting' })); + renderGate(mockAppRoot().withAnonymous().withServerContext({ connected: false, status })); - expect(await screen.findByText('login-page')).toBeInTheDocument(); + 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 3b7bfe3941f23..de7d14fc13fdf 100644 --- a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx +++ b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx @@ -20,6 +20,10 @@ import HomeSkeleton from '../../home/HomeSkeleton'; */ export type AuthenticationCheckProps = { children: ReactNode; guest?: boolean }; +/** The connection states in which the server has stopped trying to reach us, as opposed to not having arrived yet. */ +const hasGivenUp = (status: ReturnType['status']): boolean => + status === 'waiting' || status === 'failed' || status === 'offline'; + const AuthenticationCheck = ({ children, guest }: AuthenticationCheckProps) => { const user = useUser(); const allowAnonymousRead = useSetting('Accounts_AllowAnonymousRead'); @@ -36,11 +40,14 @@ const AuthenticationCheck = ({ children, guest }: AuthenticationCheckProps) => { * exactly the login-form flash this exists to remove. Only the states where the connection has stopped trying * count, and once one 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(false); + const [unreachable, setUnreachable] = useState(() => hasGivenUp(status)); useEffect(() => { - if (status === 'waiting' || status === 'failed' || status === 'offline') { + if (hasGivenUp(status)) { setUnreachable(true); } }, [status]); From 938b345a2e2e2f9a36931de0acc73fb682b188d0 Mon Sep 17 00:00:00 2001 From: Rodrigo Nascimento Date: Wed, 26 Aug 2026 11:46:11 -0300 Subject: [PATCH 4/7] fix(client): stop treating an idle connection as one that gave up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DDP SDK begins every page load 'idle' and `sdkStatusToMeteor` reports that as 'offline', so counting 'offline' as a give-up state latched on healthy reloads — showing the login form for the whole resume, which is the bug this branch set out to remove. The bound now rests on 'waiting' and 'failed', which both mean a connection was made for and lost. Co-Authored-By: Claude Fable 5 --- .../MainLayout/AuthenticationCheck.spec.tsx | 15 ++++++++++++--- .../root/MainLayout/AuthenticationCheck.tsx | 19 ++++++++++++------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx index 27df4a48b6445..1967bf35aaa76 100644 --- a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx +++ b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx @@ -47,9 +47,7 @@ describe('while the session is being resumed', () => { // 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. - // Synchronously: a window that mounts with the connection already given up must pick the form on its first - // render, rather than showing a frame of skeleton on the way to it. - it.each(['waiting', 'failed', 'offline'] as const)('shows the login page when the connection has given up (%s)', (status) => { + 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 })); @@ -58,6 +56,17 @@ describe('while the session is being resumed', () => { 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'); diff --git a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx index de7d14fc13fdf..bfb5a6959716d 100644 --- a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx +++ b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx @@ -20,9 +20,14 @@ import HomeSkeleton from '../../home/HomeSkeleton'; */ export type AuthenticationCheckProps = { children: ReactNode; guest?: boolean }; -/** The connection states in which the server has stopped trying to reach us, as opposed to not having arrived yet. */ -const hasGivenUp = (status: ReturnType['status']): boolean => - status === 'waiting' || status === 'failed' || status === 'offline'; +/** + * 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(); @@ -36,10 +41,10 @@ const AuthenticationCheck = ({ children, guest }: AuthenticationCheckProps) => { * 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. * - * `connecting` is not that: it is the ordinary first moment of every page load, and falling through on it is - * exactly the login-form flash this exists to remove. Only the states where the connection has stopped trying - * count, and once one 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. + * 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. From 2f1cdace08780762b71a4adc6181ab7226407216 Mon Sep 17 00:00:00 2001 From: Tasso Evangelista Date: Wed, 26 Aug 2026 21:32:53 -0300 Subject: [PATCH 5/7] refactor(client): make stored-item reads observable Co-Authored-By: Claude Opus 5 (1M context) --- .../meteor/client/hooks/useStoredItem.spec.ts | 63 +++++++++++++++++++ apps/meteor/client/hooks/useStoredItem.ts | 24 +++++++ apps/meteor/client/lib/sdk/storage.ts | 33 +++++++++- 3 files changed, 118 insertions(+), 2 deletions(-) create mode 100644 apps/meteor/client/hooks/useStoredItem.spec.ts create mode 100644 apps/meteor/client/hooks/useStoredItem.ts diff --git a/apps/meteor/client/hooks/useStoredItem.spec.ts b/apps/meteor/client/hooks/useStoredItem.spec.ts new file mode 100644 index 0000000000000..8996430300f43 --- /dev/null +++ b/apps/meteor/client/hooks/useStoredItem.spec.ts @@ -0,0 +1,63 @@ +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(); +}); + +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..d46743a8e34f1 100644 --- a/apps/meteor/client/lib/sdk/storage.ts +++ b/apps/meteor/client/lib/sdk/storage.ts @@ -32,11 +32,39 @@ 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. + */ +const listeners = new Set<() => void>(); + +const notify = (): void => { + for (const listener of listeners) { + listener(); + } +}; + +export const subscribeStoredItem = (listener: () => void): (() => void) => { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +}; + 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 +78,7 @@ export const setStorageBackend = (backend: StorageBackend): boolean => { } storageBackend = backend; + notify(); return true; }; From 04a5462eb16ae247ae90393e29b2dcf52c375b8f Mon Sep 17 00:00:00 2001 From: Tasso Evangelista Date: Wed, 26 Aug 2026 21:32:55 -0300 Subject: [PATCH 6/7] fix(client): show the login form when a session ends with its token still stored Co-Authored-By: Claude Opus 5 (1M context) --- .../MainLayout/AuthenticationCheck.spec.tsx | 63 ++++++++++++++++++- .../root/MainLayout/AuthenticationCheck.tsx | 33 +++++++--- 2 files changed, 84 insertions(+), 12 deletions(-) diff --git a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx index 1967bf35aaa76..d1669e205445e 100644 --- a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx +++ b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.spec.tsx @@ -1,11 +1,12 @@ 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 { 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 } from '../../../lib/sdk/storage'; +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. @@ -45,6 +46,24 @@ describe('while the session is being resumed', () => { 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) => { @@ -114,3 +133,41 @@ it('keeps the login page up while someone is logging in at it', () => { 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 bfb5a6959716d..37276ae272000 100644 --- a/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx +++ b/apps/meteor/client/views/root/MainLayout/AuthenticationCheck.tsx @@ -1,12 +1,13 @@ 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 { useEffect, useRef, useState } from 'react'; import LoggedInArea from './LoggedInArea'; import LoginPage from './LoginPage'; import UsernameCheck from './UsernameCheck'; -import { STORAGE_KEYS, getStoredItem } from '../../../lib/sdk/storage'; +import { useStoredItem } from '../../../hooks/useStoredItem'; +import { STORAGE_KEYS } from '../../../lib/sdk/storage'; import HomeSkeleton from '../../home/HomeSkeleton'; /* @@ -57,25 +58,39 @@ const AuthenticationCheck = ({ children, guest }: AuthenticationCheckProps) => { } }, [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 only on an explicit logout or a failed resume, so it covers the resume from end to end. Asking + * 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. * - * 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. + * 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 isResumingSession = !user && !forceLogin && !unreachable && !!getStoredItem(STORAGE_KEYS.LOGIN_TOKEN); + const loginToken = useStoredItem(STORAGE_KEYS.LOGIN_TOKEN); + + const isResumingSession = !user && !hasSeenUser.current && !forceLogin && !unreachable && !!loginToken; if (isResumingSession) { return ; From f0c3c9858b581bd6869c79f696382851e78f62ba Mon Sep 17 00:00:00 2001 From: Tasso Evangelista Date: Wed, 26 Aug 2026 22:06:14 -0300 Subject: [PATCH 7/7] fix(client): notice a stored item another tab changed Co-Authored-By: Claude Opus 5 (1M context) --- .../meteor/client/hooks/useStoredItem.spec.ts | 35 +++++++++++++++++++ apps/meteor/client/lib/sdk/storage.ts | 24 +++++++++++++ 2 files changed, 59 insertions(+) diff --git a/apps/meteor/client/hooks/useStoredItem.spec.ts b/apps/meteor/client/hooks/useStoredItem.spec.ts index 8996430300f43..1d1ca8d347e7a 100644 --- a/apps/meteor/client/hooks/useStoredItem.spec.ts +++ b/apps/meteor/client/hooks/useStoredItem.spec.ts @@ -44,6 +44,41 @@ it('re-renders when the value is removed', () => { 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'); diff --git a/apps/meteor/client/lib/sdk/storage.ts b/apps/meteor/client/lib/sdk/storage.ts index d46743a8e34f1..bcc691a176a78 100644 --- a/apps/meteor/client/lib/sdk/storage.ts +++ b/apps/meteor/client/lib/sdk/storage.ts @@ -38,6 +38,12 @@ const getStorage = (): Storage | undefined => { * `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>(); @@ -47,10 +53,28 @@ const notify = (): void => { } }; +/** + * 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); + } }; };