diff --git a/app/api/csp-report/route.ts b/app/api/csp-report/route.ts index f88e7671b..1e08e352b 100644 --- a/app/api/csp-report/route.ts +++ b/app/api/csp-report/route.ts @@ -6,9 +6,10 @@ * we accept either that or plain JSON since the spec is in flux. * * The route does NOT require auth — anyone can post a CSP report (the - * browser is the originator, not the user). We rate-limit via the - * existing `spamLimiter` keyed on IP to keep a hostile receiver from - * flooding our logs. + * browser is the originator, not the user). We rate-limit on IP to keep a + * hostile poster from flooding our logs, but with a ceiling sized for + * browser-generated traffic: see cspReportLimiter for why the old + * spamLimiter budget made this endpoint useless. * * The report is logged as a structured event (`event: "csp_violation"`) * so an operator scanning `console` output during the report-only @@ -19,7 +20,7 @@ */ import { NextResponse, type NextRequest } from "next/server"; -import { applyRateLimit, spamLimiter } from "@/lib/rate-limit"; +import { applyRateLimit, cspReportLimiter } from "@/lib/rate-limit"; export async function POST(req: NextRequest) { const ip = @@ -27,7 +28,7 @@ export async function POST(req: NextRequest) { req.headers.get("x-real-ip") ?? "unknown"; - const rl = await applyRateLimit(spamLimiter, `csp:${ip}`); + const rl = await applyRateLimit(cspReportLimiter, `csp:${ip}`); if (rl) return rl; const body = await req.json().catch(() => null); diff --git a/docs/enterprise/20-iam-and-security/05-security-headers.md b/docs/enterprise/20-iam-and-security/05-security-headers.md index ffa9b0641..c56407f06 100644 --- a/docs/enterprise/20-iam-and-security/05-security-headers.md +++ b/docs/enterprise/20-iam-and-security/05-security-headers.md @@ -47,6 +47,8 @@ flowchart LR LOG --> OP["operator scans during rollout, fixes allow-list"] ``` +**The observation window only works if the reports actually arrive.** `/api/csp-report` originally shared `spamLimiter`, which allows 5 requests per hour — a budget sized for a human deciding to file a support ticket. A browser emits one report per violated directive per navigation, so a single person opening a few dashboard pages exhausted the hour in seconds and every subsequent report was rejected with a `429`. The rollout was therefore blind in precisely the situation it exists to observe. The endpoint now has its own limiter (`cspReportLimiter`) sized for browser-generated volume. If you add a report sink in future, size its limiter by who generates the traffic, not by how much you want to receive. + ## Header inventory (production) All seven production headers, their values, and what each one defends against, are listed in the table below. @@ -71,11 +73,27 @@ and the public marketplace pages alike. Anything outside the directives below will be blocked once `ENABLE_CSP_ENFORCE=true`, so each external origin earns its place by being load-bearing for a real product surface. -The `script-src` directive keeps `'self' 'unsafe-inline' 'unsafe-eval'`, which is non-negotiable until Next.js 16 ships hashed inline runtime chunks. Its external origins are Razorpay's checkout CDN (`https://checkout.razorpay.com`, the payment SDK), Sentry (`https://*.sentry.io`, error reporting), Stream.io (`https://*.getstream.io`, the call widget), and Supabase (`https://*.supabase.co`, storage signed URLs). +The `script-src` directive keeps `'self' 'unsafe-inline' 'unsafe-eval'`, which is non-negotiable until Next.js 16 ships hashed inline runtime chunks. Its external origins are Razorpay's checkout CDN (`https://checkout.razorpay.com`, the payment SDK), Stripe (`https://js.stripe.com`), Sentry (`https://*.sentry.io`, error reporting), Stream.io (`https://*.getstream.io`), and Supabase (`https://*.supabase.co`, storage signed URLs). The Stream entry is inherited rather than observed — the SDK is bundled from npm and self-served, so no `script-src` fetch to Stream was seen in practice. + +The `connect-src` directive governs XHR, fetch, and WebSocket targets. It opens `https://api.razorpay.com` for payments, the three Stream domains described below, `https://*.supabase.co` and `https://*.upstash.io` for storage and Redis, `https://*.sentry.io` for error reporting, `https://api.resend.com` for transactional email, and `https://*.novu.co` plus `wss://*.novu.co` for the notification inbox. + +The `media-src` directive serves Stream.io recording and call audio/video, so it requires `blob:` for local recording playback alongside Stream's CDN and API origins. + +### Stream.io does not run on getstream.io + +This is the mistake the allow-list originally made, and it is worth stating plainly because it is easy to repeat: `getstream.io` is Stream's marketing and documentation domain. No SDK traffic goes there. The clients talk to three unrelated domains, and a CSP host wildcard does not span them: + +| Domain | Carries | +| --- | --- | +| `*.stream-io-api.com` | REST calls and both websockets (`wss://video.stream-io-api.com`, `wss://chat.stream-io-api.com`) | +| `*.stream-io-video.com` | the edge-latency hint (`hint.stream-io-video.com`) the client fetches before a call to choose an SFU, then the SFU edge itself | +| `*.stream-io-cdn.com` | call recordings and chat attachments | + +Because only `*.getstream.io` was listed, every dashboard load filed violation reports for traffic the product cannot function without, and video calling would have failed outright the moment `ENABLE_CSP_ENFORCE=true` was set. The domains above were confirmed against a real browser network log on a deploy preview rather than read off Stream's docs, which is the only way to catch this class of drift. -The `connect-src` directive governs XHR, fetch, and WebSocket targets. It opens `https://api.razorpay.com` for payments, both `wss://*.getstream.io` and `https://*.getstream.io` for Stream call signalling and media, `https://*.supabase.co` and `https://*.upstash.io` for storage and Redis, `https://*.sentry.io` for error reporting, and `https://api.resend.com` for transactional email. +`*.getstream.io` remains in the list: Stream still serves some static assets from it, and dropping it is a separate change with its own unobserved blast radius. -The `media-src` directive serves Stream.io recording and call audio/video, so it requires both `blob:` (local recording playback) and the getstream.io CDN. +`worker-src` is deliberately absent. Nothing in the app constructs a `Worker`, and Stream's background-filter and noise-cancellation add-ons — the features that would need `blob:` workers and `wasm-unsafe-eval` — are not installed. If they are ever adopted, `worker-src` is the directive that breaks first, and it will fall back to `default-src 'self'`. The `frame-src` directive allows Razorpay's checkout iframe; without this entry, payments break the moment CSP is flipped to enforce mode. diff --git a/docs/stream/03-provider-authentication.md b/docs/stream/03-provider-authentication.md index 53d833832..4de4090e4 100644 --- a/docs/stream/03-provider-authentication.md +++ b/docs/stream/03-provider-authentication.md @@ -18,16 +18,33 @@ ### Dual-Client Design Pattern -**File:** `/providers/StreamProvider.tsx` (Lines 1-382) +**File:** `providers/StreamProviderImpl.tsx` (the SDK-free shell lives in `providers/StreamProvider.tsx`) -StreamProvider implements a **dual-client architecture**, managing two independent SDK clients simultaneously: +StreamProvider manages two SDK clients — one for chat, one for video — but holds them in a **single piece of state**: ```typescript -// Two separate client instances -const [chatClient, setChatClient] = useState(null); -const [videoClient, setVideoClient] = useState(null); +// One settled value, not two independent ones +interface SettledStreamClients { + chat: StreamChat | null; + video: StreamVideoClient | null; +} +const [clients, setClients] = useState(null); ``` +`null` means "not settled yet", which is deliberately distinct from a settled result whose `chat` or `video` is `null` because that particular connect failed. + +#### Why one state and not two + +This is the most important thing to understand before changing this file, because the obvious refactor — a `useState` per client — is the bug. + +The provider wraps its children in `` and `` only once a client exists. With two independent states set by two async connects that race, the element occupying that wrapper slot changed **type** between renders: `children`, then ``, then ``, in whichever order the sockets happened to settle. React cannot reconcile a change of element type in place — it unmounts the old tree and mounts a new one — and the subtree here is the entire dashboard. + +The user-visible symptom was a join button that appeared to do nothing: the click started a join, the dashboard remounted underneath it as the second client connected, and the in-flight join was destroyed. People pressed it repeatedly. + +Committing both clients in one `setClients` makes the tree shape a pure function of one value, so it changes exactly once per session: unwrapped while connecting, then wrapped once both connects settle. A connect that genuinely _fails_ can still cost a second change if a later retry succeeds; that is accepted, because withholding the client that did connect would break the sidebar's chat-unread badge on every route (#248). + +The nesting order is fixed — `` outside, `` inside — for the same reason. Order must not depend on arrival order. + #### 1. Chat Client (`StreamChat`) **Package:** `stream-chat` @@ -105,9 +122,8 @@ export default function StreamProvider({ enableChat = true, enableVideo = true, }: StreamProviderProps) { - // Connection state - const [chatClient, setChatClient] = useState(null); - const [videoClient, setVideoClient] = useState(null); + // Connection state — both clients in ONE value, see "Why one state and not two" + const [clients, setClients] = useState(null); const [chatConnected, setChatConnected] = useState(false); const [videoConnected, setVideoConnected] = useState(false); const [isConnecting, setIsConnecting] = useState(false); @@ -132,22 +148,20 @@ export default function StreamProvider({ }; }, [userDetails, isLoading, apiKey]); + // Built up in a fixed order from the single settled value, so the tree shape + // never depends on which socket connected first. + let content = children; + if (clients?.video) { + content = {content}; + } + if (clients?.chat) { + content = {content}; + } + return ( - {enableVideo && videoClient ? ( - - {enableChat && chatClient ? ( - {children} - ) : ( - children - )} - - ) : enableChat && chatClient ? ( - {children} - ) : ( - children - )} + {content} ); @@ -158,7 +172,9 @@ export default function StreamProvider({ - Outermost: `StreamErrorBoundary` (error handling) - Middle: `StreamConnectionContext` (connection state) -- Inner: `StreamVideo` → `Chat` → `children` (SDK providers) +- Inner: `Chat` → `StreamVideo` → `children` (SDK providers) + +Note that the connection _flags_ (`chatConnected`, `videoConnected`, `isConnecting`) are still separate state. That is fine and intentional: they feed the context value, which changes what consumers render but not the shape of the tree above them. --- @@ -475,11 +491,26 @@ const connectServices = useCallback(async () => { setError(null); try { - const promises = []; - if (enableChat && !chatConnected) promises.push(connectChat()); - if (enableVideo && !videoConnected) promises.push(connectVideo()); + // allSettled, not all: `all` rejects on the first failure and abandons the + // other promise's result, so a chat failure threw away a good video client. + // Each connect RESOLVES to its client (or null) rather than setting state, + // so both land in one commit below. + const [chatResult, videoResult] = await Promise.allSettled([ + connectChat(), + connectVideo(), + ]); + + setClients({ + chat: chatResult.status === "fulfilled" ? chatResult.value : null, + video: videoResult.status === "fulfilled" ? videoResult.value : null, + }); + + // Retry is still driven by a rejection, so re-throw the first one. + const failure = [chatResult, videoResult].find( + (result) => result.status === "rejected", + ); + if (failure?.status === "rejected") throw failure.reason; - await Promise.all(promises); // Parallel execution setConnectionAttempts(0); // Reset on success } catch (error) { const errorMessage = @@ -620,25 +651,21 @@ export function ConnectionStatus() { ### Loading States -Provider shows loading UI while connecting (Lines 321-334): +**The provider no longer blocks children behind a spinner.** It used to: while either client was unconnected it returned a spinner instead of `children`, which gated the entire dashboard on two websocket handshakes even on routes with no chat or video UI at all. + +Children now render immediately and the wrappers appear around them once the clients settle. Video consumers already guard a null client, and chat consumers only render on the chat route, underneath ``. + +The only spinner left is in the SDK-free shell (`providers/StreamProvider.tsx`), shown during the brief window where the lazy impl chunk is still downloading: ```typescript -if ( - (enableChat && !chatClient && !error) || - (enableVideo && !videoClient && !error) || - isConnecting -) { - return ( -
-
- {isConnecting && ( -

Connecting to Stream...

- )} -
- ); -} +const LazyStreamProviderImpl = dynamic( + () => import("@/providers/StreamProviderImpl"), + { ssr: false, loading: () => }, +); ``` +If you are tempted to reintroduce a connection gate here, note that it interacts badly with the single-commit design above: gating on "all clients ready" turns one shape change into a shape change plus an unmount, and a permanently failing service would hold the whole dashboard hostage. + ### Error States Error UI shown after max retries (Lines 337-352): @@ -775,37 +802,35 @@ See: [Troubleshooting - Token Expiry Race Condition](./troubleshooting.md#token- ### Cache Invalidation (Lines 276-298) +Disconnection is **not** owned by the provider and does **not** happen on unmount. The clients live in module-level refs in `lib/stream/disconnect.ts` — an SDK-free module so that callers which only need to disconnect (Navbar, UserDropdown, the org/admin/staff layouts) do not statically link the heavy SDK into their bundles. + ```typescript -const disconnect = useCallback(async () => { - const promises = []; - - if (chatClient) { - promises.push( - chatClient.disconnectUser().then(() => { - console.log("Chat client disconnected"); - setChatClient(null); - setChatConnected(false); - }), - ); - } +export async function disconnectStreamClients(): Promise { + const promises: Promise[] = []; + if (globalChatClient) promises.push(globalChatClient.disconnectUser().then(...)); + if (globalVideoClient) promises.push(globalVideoClient.disconnectUser().then(...)); + + // allSettled (not all): a rejected disconnectUser() must NOT skip the global + // teardown below — stale refs after a failed logout would let the next login + // adopt the prior user's connection. + await Promise.allSettled(promises); + + globalChatClient = null; + globalVideoClient = null; + currentUserId = null; + clearAllStreamCaches(); +} +``` - if (videoClient) { - // Note: StreamVideoClient doesn't have explicit disconnect method - setVideoClient(null); - setVideoConnected(false); - } +**Why unmount does not disconnect:** the clients are deliberately kept alive across remounts and tab switches, so navigating between dashboard routes does not pay for a fresh websocket handshake each time. The provider adopts an existing global client for the same user rather than building a new one. - await Promise.all(promises); - setTokenCache({}); // Clear token cache -}, [chatClient, videoClient]); -``` +**The one case that must tear down** is a _different_ user appearing — on a fresh mount the provider's local `clients` state is `null` while the globals still point at the previous user. The provider therefore calls `disconnectStreamClients()` (global refs) rather than any local teardown, which would no-op and leak the prior user's connection for the next connect to adopt. -**Cache cleared on:** +**Disconnect happens on:** -- User logout -- Component unmount -- Manual disconnect -- Connection error (after max retries) +- User logout (the primary path) +- A different user mounting the provider +- Not on unmount, and not on connection error — the retry loop owns that --- @@ -1070,26 +1095,32 @@ const { retryConnection } = useStreamConnection(); ## Advanced Topics -### Cleanup on Unmount (Lines 301-309) +### Cleanup on Unmount -```typescript -useEffect(() => { - if (!isLoading && userDetails && apiKey) { - connectServices(); - } +Unmount cancels _pending work_ and nothing else. It does not disconnect. - return () => { - disconnect(); // Cleanup function - }; -}, [userDetails, isLoading, apiKey]); +```typescript +return () => { + // Cancel the scheduled idle connect and any pending retry so nothing calls + // setState after unmount. + if (idleHandle !== undefined) cancelIdleCallback(idleHandle); + if (timeoutHandle) clearTimeout(timeoutHandle); + if (retryTimeoutRef.current) clearTimeout(retryTimeoutRef.current); + + // Intentionally NOT calling disconnect() here. + // Global clients are reused across component remounts. +}; ``` -**Cleanup Process:** +**Cleanup process:** + +1. Cancel the deferred `requestIdleCallback` connect (#248) +2. Clear the retry backoff timer +3. Leave the clients connected + +The third point is the whole design. Disconnecting here would mean a websocket handshake on every dashboard navigation, and — before the single-commit change described above — the provider remounted on its own during connection anyway, so an unmount-disconnect would have torn down the connection it had just established. -1. Disconnect chat client (`chatClient.disconnectUser()`) -2. Nullify video client (no explicit disconnect method) -3. Clear token cache -4. Reset connection states +Actual disconnection happens on logout, via `disconnectStreamClients()`. See §Disconnection. ### Connection State Persistence diff --git a/docs/stream/troubleshooting.md b/docs/stream/troubleshooting.md index 7e32047ec..abe1386a8 100644 --- a/docs/stream/troubleshooting.md +++ b/docs/stream/troubleshooting.md @@ -392,6 +392,45 @@ curl -X GET "https://chat.stream-io-api.com/health" ## Connection Issues +### Issue: The dashboard flickers or reloads as the page settles, and the first Join click does nothing + +#### Symptoms + +- The dashboard visibly remounts a second or two after load +- Clicking Join appears to do nothing, so the user clicks it several more times +- Component state (open dialogs, scroll position, half-filled forms) resets on its own +- Possibly a React error #310 — "rendered more hooks than during the previous render" — pointing into Stream SDK internals + +#### Cause + +The provider held the chat and video clients in two independent `useState`s. Their connects race, so the element wrapping the dashboard changed *type* between renders (`children` → `` → ``, in socket-arrival order). React cannot reconcile a type change in place, so it remounted the whole subtree — destroying any in-flight join. + +#### Fix + +Both clients are committed in a single `setClients` via `Promise.allSettled`, so the tree shape is a pure function of one settled value. See §Why one state and not two in `docs/stream/03-provider-authentication.md`. + +**If you see this again**, the first thing to check is whether someone has reintroduced a second source of truth for client state, or made the wrapper nesting order depend on which client arrived first. + +### Issue: Video works locally but fails in production, or the console fills with CSP violations + +#### Symptoms + +- `Refused to connect to 'https://hint.stream-io-video.com/…'` or `'wss://video.stream-io-api.com/…'` +- Calls connect locally (where CSP is often not exercised) but not on a deploy preview or production +- `POST /api/csp-report` returning `429` + +#### Cause + +Stream does **not** use `getstream.io` at runtime — that is the marketing domain. The SDKs talk to `*.stream-io-api.com`, `*.stream-io-video.com`, and `*.stream-io-cdn.com`. An allow-list containing only `*.getstream.io` does not match any of them. + +Separately, `/api/csp-report` was rate-limited at 5/hour, so the violation reports that would have revealed this were themselves being dropped. + +#### Fix + +Both are corrected in `next.config.mjs` and `lib/rate-limit.ts`. The full domain breakdown is in `docs/enterprise/20-iam-and-security/05-security-headers.md`. + +**Verify with the browser, not the docs.** This class of drift is only visible in a real network log; Stream's documentation does not enumerate the SFU and hint domains in one place. + ### Issue: "Chat connection failed" #### Symptoms diff --git a/lib/rate-limit.ts b/lib/rate-limit.ts index f35e93e04..01a9a9874 100644 --- a/lib/rate-limit.ts +++ b/lib/rate-limit.ts @@ -8,6 +8,7 @@ * - waitlistLimiter: 3/hr per IP — POST /api/waitlist (newsletter signup spam) * - referralApplyLimiter: 3/24h per user — POST /api/referrals/apply (farming) * - spamLimiter: 5/hr per user — support-tickets, feedbacks, reviews, report + * - cspReportLimiter: 120/min per IP — POST /api/csp-report (browser-generated) * - trialRequestLimiter: 3/24h per user — POST /api/trials (spam prevention) * - requestApprovalLimiter: 10/hr per user — POST /api/slots/request-for-approval * - searchLimiter: 60/min per IP — GET /api/user/consultants, /api/consultants/search @@ -69,6 +70,22 @@ export const referralApplyLimiter = makeLimiter(3, "24 h", "rl:referral-apply"); /** 5 per hour — support-tickets, feedbacks, reviews, report (scope key by route) */ export const spamLimiter = makeLimiter(5, "1 h", "rl:spam"); +/** + * 120 per minute per IP — POST /api/csp-report. + * + * Was on spamLimiter's 5/hr, which is sized for a HUMAN deciding to file a + * support ticket. A CSP report is emitted by the browser, unprompted, once per + * violated directive per page load — so one person opening a few dashboard + * pages exhausted the hour's quota in seconds and every report after that was + * dropped with a 429. The report-only rollout was therefore blind in exactly + * the situation it exists to observe: a directive drifting on a real user. + * + * Sized for a page that violates a handful of directives on every navigation, + * with headroom, while still capping a hostile poster. Reports are logged, not + * stored, so the cost of a generous ceiling is log volume rather than writes. + */ +export const cspReportLimiter = makeLimiter(120, "1 m", "rl:csp-report"); + /** 3 per 24 hours — POST /api/trials (prevents flooding consultant inboxes) */ export const trialRequestLimiter = makeLimiter(3, "24 h", "rl:trial-request"); diff --git a/next.config.mjs b/next.config.mjs index 964c6f50b..9b28ad5c2 100644 --- a/next.config.mjs +++ b/next.config.mjs @@ -32,14 +32,37 @@ const withBundleAnalyzer = * added here AND in the matching client. * - `frame-src` allows Razorpay's + Stripe's checkout iframes. * - `media-src` is the load-bearing entry for Stream call audio / - * video / recording playback (`blob:` + getstream.io). + * video / recording playback. + * + * Stream.io does NOT run on getstream.io at runtime + * ------------------------------------------------- + * `getstream.io` is the marketing/docs domain. The SDKs actually talk to + * three separate domains, none of which `*.getstream.io` matches, so every + * dashboard load was filing violation reports: + * + * - `*.stream-io-api.com` REST + the chat/video websockets + * (`wss://video.stream-io-api.com`) + * - `*.stream-io-video.com` the edge-latency hint (`hint.…`) the client + * fetches BEFORE a call to pick an SFU, then the + * SFU edge itself + * - `*.stream-io-cdn.com` recordings and chat attachments + * + * Confirmed against a real network log on a deploy preview, not inferred from + * docs. `*.getstream.io` stays because Stream still serves some static assets + * there and removing it is a separate, unobserved risk. + * + * Not added, deliberately: `worker-src`. Nothing in this app constructs a + * Worker and the Stream background-filter/noise-cancellation add-ons (the + * things that would need `blob:` workers and `wasm-unsafe-eval`) are not + * installed. If those are ever enabled, this is the directive that will break + * first, and `default-src 'self'` is what it will fall back to. */ const CSP_DIRECTIVES = [ "default-src 'self'", "script-src 'self' 'unsafe-inline' 'unsafe-eval' https://checkout.razorpay.com https://js.stripe.com https://*.sentry.io https://*.getstream.io https://*.supabase.co", - "connect-src 'self' https://*.getstream.io wss://*.getstream.io https://*.supabase.co https://*.upstash.io https://api.razorpay.com https://api.stripe.com https://*.sentry.io https://api.resend.com https://*.novu.co wss://*.novu.co", + "connect-src 'self' https://*.getstream.io wss://*.getstream.io https://*.stream-io-api.com wss://*.stream-io-api.com https://*.stream-io-video.com wss://*.stream-io-video.com https://*.stream-io-cdn.com https://*.supabase.co https://*.upstash.io https://api.razorpay.com https://api.stripe.com https://*.sentry.io https://api.resend.com https://*.novu.co wss://*.novu.co", "img-src 'self' data: https: blob:", - "media-src 'self' blob: https://*.getstream.io", + "media-src 'self' blob: https://*.getstream.io https://*.stream-io-cdn.com https://*.stream-io-api.com", "style-src 'self' 'unsafe-inline'", "frame-src 'self' https://checkout.razorpay.com https://js.stripe.com https://hooks.stripe.com", "font-src 'self' data:", diff --git a/providers/StreamProviderImpl.tsx b/providers/StreamProviderImpl.tsx index 3d8c5c609..a05d093a8 100644 --- a/providers/StreamProviderImpl.tsx +++ b/providers/StreamProviderImpl.tsx @@ -10,12 +10,7 @@ // dynamic()-wrapping a component defined in the same file does not code-split, // since its static imports stay in the parent chunk. See StreamProvider.tsx. -import { - useCallback, - useEffect, - useState, - useRef, -} from "react"; +import { useCallback, useEffect, useState, useRef } from "react"; import { StreamChat } from "stream-chat"; import { Chat } from "stream-chat-react"; import { StreamVideo, StreamVideoClient } from "@stream-io/video-react-sdk"; @@ -58,18 +53,30 @@ const clientSyncCompletedUsers = new Set(); const apiKey = process.env.NEXT_PUBLIC_STREAM_API_KEY; +/** + * The two clients as ONE value, deliberately. Held separately they were set by + * two async connects that race, so the element in the wrapper slot below + * changed TYPE between renders — `children`, then ``, then + * ``, in whichever order the sockets happened to settle. React cannot + * reconcile a type change in place: it unmounts and remounts the entire + * subtree, which here is the whole dashboard. That is the remount storm behind + * "I pressed Join ten times" — an in-flight join was torn down under the user. + * + * `null` means "not settled yet", which is distinct from a settled result whose + * `chat` or `video` is null because that connect failed. + */ +interface SettledStreamClients { + chat: StreamChat | null; + video: StreamVideoClient | null; +} + const StreamProviderImpl = ({ children, userId, enableChat = true, enableVideo = true, }: StreamProviderProps) => { - // Connection states - always initialize to null/false - // Let the connection functions handle global client detection - const [chatClient, setChatClient] = useState(null); - const [videoClient, setVideoClient] = useState( - null, - ); + const [clients, setClients] = useState(null); const [chatConnected, setChatConnected] = useState(false); const [videoConnected, setVideoConnected] = useState(false); const [isConnecting, setIsConnecting] = useState(false); @@ -144,17 +151,20 @@ const StreamProviderImpl = ({ return Math.min(1000 * Math.pow(2, attempt), 30000); // Max 30 seconds }, []); + // connectChat/connectVideo RESOLVE to their client (or null) instead of each + // setting its own state, so the caller can commit both at once and the tree + // changes shape a single time. See SettledStreamClients. const connectChat = useCallback(async () => { - if (!enableChat || !userDetails || !apiKey) return; + if (!enableChat || !userDetails || !apiKey) return null; // Check if we already have a global client for this user - adopt it - if (getCurrentStreamUserId() === userDetails.id && getGlobalChatClient()) { + const adoptable = getGlobalChatClient(); + if (getCurrentStreamUserId() === userDetails.id && adoptable) { streamLogger.debug("Adopting existing chat client", { userId: userDetails.id, }); - setChatClient(getGlobalChatClient()); setChatConnected(true); - return; + return adoptable; } // Prevent concurrent connectUser calls (e.g. connectVideo re-render race) @@ -162,7 +172,7 @@ const StreamProviderImpl = ({ streamLogger.debug("Chat connection already in progress, skipping", { userId: userDetails.id, }); - return; + return getGlobalChatClient(); } isChatConnectingRef.current = true; @@ -182,9 +192,8 @@ const StreamProviderImpl = ({ }); setGlobalChatClient(client); setCurrentStreamUserId(userDetails.id); - setChatClient(client); setChatConnected(true); - return; + return client; } // Ensure user exists in Stream's database (only if not synced before) @@ -218,7 +227,6 @@ const StreamProviderImpl = ({ setGlobalChatClient(client); setCurrentStreamUserId(userDetails.id); - setChatClient(client); setChatConnected(true); // Initial channel sync — once per user per browser session. @@ -261,6 +269,7 @@ const StreamProviderImpl = ({ streamLogger.info("Chat connection established", { userId: userDetails.id, }); + return client; } catch (error) { streamLogger.warn("Chat connection failed (will retry)", { userId: userDetails.id, @@ -273,16 +282,16 @@ const StreamProviderImpl = ({ }, [enableChat, userDetails, getCachedToken]); const connectVideo = useCallback(async () => { - if (!enableVideo || !userDetails || !apiKey) return; + if (!enableVideo || !userDetails || !apiKey) return null; // Check if we already have a global client for this user - adopt it - if (getCurrentStreamUserId() === userDetails.id && getGlobalVideoClient()) { + const adoptable = getGlobalVideoClient(); + if (getCurrentStreamUserId() === userDetails.id && adoptable) { streamLogger.debug("Adopting existing video client", { userId: userDetails.id, }); - setVideoClient(getGlobalVideoClient()); setVideoConnected(true); - return; + return adoptable; } try { @@ -304,11 +313,11 @@ const StreamProviderImpl = ({ setGlobalVideoClient(client); setCurrentStreamUserId(userDetails.id); - setVideoClient(client); setVideoConnected(true); streamLogger.info("Video connection established", { userId: userDetails.id, }); + return client; } catch (error) { streamLogger.error("Video connection failed", error, { userId: userDetails.id, @@ -326,12 +335,25 @@ const StreamProviderImpl = ({ setError(null); try { - const promises = []; - // Always try to connect - the functions will handle global client detection - if (enableChat) promises.push(connectChat()); - if (enableVideo) promises.push(connectVideo()); + // allSettled, not all: `all` rejects on the first failure and abandons the + // other client's result, so a chat failure discarded a perfectly good + // video client. Both outcomes are now committed together, which is also + // what keeps the tree from changing shape twice. + const [chatResult, videoResult] = await Promise.allSettled([ + connectChat(), + connectVideo(), + ]); + + setClients({ + chat: chatResult.status === "fulfilled" ? chatResult.value : null, + video: videoResult.status === "fulfilled" ? videoResult.value : null, + }); + + const failure = [chatResult, videoResult].find( + (result) => result.status === "rejected", + ); + if (failure?.status === "rejected") throw failure.reason; - await Promise.all(promises); connectionAttemptsRef.current = 0; // Reset on success setRetryCount(0); } catch (error) { @@ -362,15 +384,10 @@ const StreamProviderImpl = ({ } finally { setIsConnecting(false); } - }, [ - isLoading, - userDetails, - enableChat, - enableVideo, - connectChat, - connectVideo, - getRetryDelay, - ]); + // enableChat/enableVideo are not read here any more: each connect returns + // null when its own flag is off, and both are already deps of those + // callbacks, so listing them again only invalidates this one needlessly. + }, [isLoading, userDetails, connectChat, connectVideo, getRetryDelay]); const retryConnection = useCallback(() => { connectionAttemptsRef.current = 0; @@ -399,11 +416,11 @@ const StreamProviderImpl = ({ const run = () => { // Check if user changed - if so, disconnect old user first. - // Use disconnectStreamClients (global refs) rather than the local - // disconnect() here: on a fresh remount for a different user, local - // chatClient/videoClient state is null while the GLOBAL clients still - // point at the PREVIOUS user. local disconnect() would no-op and leak - // the prior user's connection, which the new connect would then adopt. + // Use disconnectStreamClients (global refs) rather than any local + // teardown: on a fresh remount for a different user the local `clients` + // state is null while the GLOBAL clients still point at the PREVIOUS + // user, so a local teardown would no-op and leak the prior user's + // connection, which the new connect would then adopt. if ( getCurrentStreamUserId() && getCurrentStreamUserId() !== userDetails.id @@ -420,9 +437,12 @@ const StreamProviderImpl = ({ .catch((err) => { // Never block the new user's connect on a prior-user disconnect // failure; disconnectStreamClients already clears global refs. - streamLogger.warn("Prior-user disconnect failed, connecting anyway", { - error: err, - }); + streamLogger.warn( + "Prior-user disconnect failed, connecting anyway", + { + error: err, + }, + ); }) .finally(() => { connectServices(); @@ -488,17 +508,24 @@ const StreamProviderImpl = ({ // render children immediately; the Stream context providers wrap them once the // clients are ready, and the video/chat consumers already guard a null client. - // Render providers + // The wrapper set is derived from ONE settled value and nested in a fixed + // order — Chat outside, StreamVideo inside — so the shape here is a pure + // function of `clients` rather than of which socket won the race. In the + // normal case that means exactly one shape change for the whole session: + // unwrapped while connecting, then wrapped once both connects settle. + // + // A connect that genuinely FAILS still costs a second change if a later retry + // succeeds. That is accepted: it is a degraded path, the retry loop is capped + // at 5 attempts, and withholding the client that did connect would break the + // sidebar's chat-unread badge on every route (#248). let content = children; - // Wrap with video provider if enabled and connected - if (enableVideo && videoClient) { - content = {content}; + if (clients?.video) { + content = {content}; } - // Wrap with chat provider if enabled and connected - if (enableChat && chatClient) { - content = {content}; + if (clients?.chat) { + content = {content}; } // The connection-failed banner renders alongside children (not in place of