From 8ec74dffcc1fd648191e06a13c859621df18240c Mon Sep 17 00:00:00 2001 From: Your Name Date: Tue, 8 Sep 2026 05:27:16 +0800 Subject: [PATCH] fix(web): code review round 3 -- surviving races and a shape bug Round 2's dedup fixes for connect()/openConversation left two gaps that only showed up under closer scrutiny (and, for the first one, actual live reproduction): - closeConversation() awaited the shared in-flight subscribe promise before calling leave(), but didn't check whether a NEWER openConversation() had since claimed the same topic while it was waiting -- under React 19 StrictMode's mount->cleanup->remount, the stale cleanup's leave() would detach the subscription the surviving mount had just established, silently killing live updates for whatever conversation you'd just opened. Fixed with a per-topic generation counter: closeConversation only actually leaves if no newer open has since claimed the topic. - prefetchPreview (chat-list preview subscribe) never joined the same in-flight tracking as openConversation, so opening a conversation while its own preview subscribe was still resolving could still send two overlapping {sub} packets at the same topic. - resetTinode() didn't clear connectPromise/restorePromise or disconnect a connection that was still mid-handshake (only guarded on isConnected()) -- switching tenants while the old connection was slow to respond could leave a new login silently waiting on the abandoned old connection's outcome. - subscribeMeTopic() got the same in-flight dedup as the others, for consistency (no concrete concurrent caller today, cheap insurance against a future one). Also: - getBlocks()'s response type was a flat Friend shape; the real contract (confirmed against both the protocol doc's JSON example and the old app's own consumer) nests the user under `.user`, so "already blocked" state in DetailColumn was silently always false. - useBootstrap() had no tenant in its query key and staleTime:Infinity, so switching tenants without a full reload kept serving the PREVIOUS tenant's message_policy/features/permissions/security forever. Co-Authored-By: Claude Sonnet 5 --- web/src/app/RequireAuth.tsx | 38 +++++++++++++++++++++--- web/src/lib/api/client.ts | 16 ++++++---- web/src/lib/api/friends.ts | 10 ++++++- web/src/lib/api/hooks.ts | 13 +++++++-- web/src/lib/tinode/connection.ts | 16 +++++++--- web/src/lib/tinode/topics.ts | 50 ++++++++++++++++++++++++++++++-- 6 files changed, 124 insertions(+), 19 deletions(-) diff --git a/web/src/app/RequireAuth.tsx b/web/src/app/RequireAuth.tsx index 6a74f565..5b847fdf 100644 --- a/web/src/app/RequireAuth.tsx +++ b/web/src/app/RequireAuth.tsx @@ -2,10 +2,17 @@ import { useEffect, useState } from 'react'; import { Navigate, Outlet } from 'react-router-dom'; +import { Toast } from '@/app/Toast'; import { restoreSession } from '@/lib/tinode/connection'; import { subscribeMeTopic } from '@/lib/tinode/topics'; import { useSessionStore } from '@/stores/session'; +/** No connect-timeout exists anywhere in the SDK (its WS promise only settles on the underlying + * socket's own onopen/onerror) -- past this, assume the host is unreachable in a way that won't + * self-resolve (stalled handshake, silently-dropped middlebox) rather than hang the restore screen + * forever with no escape route. */ +const RESTORE_TIMEOUT_MS = 10_000; + /** * Gate for everything inside AppShell: needs a resolved tenant and an authenticated user. * @@ -22,33 +29,56 @@ export function RequireAuth() { const setMyUserId = useSessionStore((s) => s.setMyUserId); const [hasToken] = useState(() => !!sessionStorage.getItem('im-session-token')); const [restoreFailed, setRestoreFailed] = useState(false); + const [restoreTimedOut, setRestoreTimedOut] = useState(false); + const [contactsError, setContactsError] = useState(''); useEffect(() => { if (myUserId || !tenant || !hasToken) return undefined; const token = sessionStorage.getItem('im-session-token'); if (!token) return undefined; let cancelled = false; + const timeout = window.setTimeout(() => { + if (!cancelled) setRestoreTimedOut(true); + }, RESTORE_TIMEOUT_MS); restoreSession(token) .then((uid) => { if (cancelled) return; setMyUserId(uid); - subscribeMeTopic().catch(() => {}); + subscribeMeTopic().catch(() => setContactsError('会话列表加载失败，请刷新页面重试')); }) .catch(() => { if (!cancelled) { sessionStorage.removeItem('im-session-token'); setRestoreFailed(true); } + }) + .finally(() => { + window.clearTimeout(timeout); }); return () => { cancelled = true; + window.clearTimeout(timeout); }; }, [tenant, myUserId, hasToken, setMyUserId]); if (!tenant) return ; - if (myUserId) return ; - if (hasToken && !restoreFailed) { - return
正在恢复登录状态…
; + if (myUserId) { + return ( + <> + + setContactsError('')} /> + + ); + } + if (hasToken && !restoreFailed && !restoreTimedOut) { + return ( +
+ 正在恢复登录状态… + + 改为重新登录 + +
+ ); } return ; } diff --git a/web/src/lib/api/client.ts b/web/src/lib/api/client.ts index 2e7a912c..2e9c498a 100644 --- a/web/src/lib/api/client.ts +++ b/web/src/lib/api/client.ts @@ -165,7 +165,7 @@ export async function imRequest( ); } } - if (!response.ok || (payload?.code && payload.code !== 0)) { + if (!response.ok || payload?.code) { const known = translateReason(payload.reason, payload.details); throw new ImApiError( known || payload.message || payload.reason || response.statusText || '请求失败', @@ -196,16 +196,22 @@ export const imDelete = (path: string, opts: RequestOptions = {}) = * only ever fetch page 1 (e.g. "am I this group's owner/admin" going undetected because the real * answer was on page 2). Empty/repeated next_cursor means done. */ +// Guards against a server-side cursor bug (e.g. two values that cycle back and forth) turning this +// into an infinite fetch loop -- 200 pages at a 20-50 item page size is already far more than any +// realistic friends/blocks/search list, so hitting this is itself a signal something's wrong server- +// side, not a real user's data legitimately needing more. +const MAX_PAGES = 200; + export async function fetchAllPages( fetchPage: (cursor: string) => Promise<{ items?: T[]; next_cursor?: string }>, startCursor = '', ): Promise { let cursor = startCursor; let items: T[] = []; - for (;;) { - const page = await fetchPage(cursor); - items = items.concat(page?.items || []); - const nextCursor = page?.next_cursor || ''; + for (let page = 0; page < MAX_PAGES; page++) { + const result = await fetchPage(cursor); + items = items.concat(result?.items || []); + const nextCursor = result?.next_cursor || ''; if (!nextCursor || nextCursor === cursor) break; cursor = nextCursor; } diff --git a/web/src/lib/api/friends.ts b/web/src/lib/api/friends.ts index c40d3d3c..641a027d 100644 --- a/web/src/lib/api/friends.ts +++ b/web/src/lib/api/friends.ts @@ -114,6 +114,14 @@ export function unblockUser(userId: string) { return imDelete(`/blocks/${encodeURIComponent(userId)}`); } +export interface BlockedUser { + user: { user_id: string; display_name?: string; avatar_file_id?: string }; + blocked_at: string; +} + +/** Unlike `/friends`, this endpoint nests the user under `.user` (docs/IM-前后端对接文档.md:1379,1428, + * both the prose and the JSON example agree, and the old app's own consumer reads `item.user. + * display_name` -- not a flat item like Friend). */ export function getBlocks(cursor = '') { - return imGet<{ items: Friend[]; next_cursor?: string }>(`/blocks${cursor ? `?cursor=${encodeURIComponent(cursor)}` : ''}`); + return imGet<{ items: BlockedUser[]; next_cursor?: string }>(`/blocks${cursor ? `?cursor=${encodeURIComponent(cursor)}` : ''}`); } diff --git a/web/src/lib/api/hooks.ts b/web/src/lib/api/hooks.ts index c59e36f5..c0ed63aa 100644 --- a/web/src/lib/api/hooks.ts +++ b/web/src/lib/api/hooks.ts @@ -1,8 +1,17 @@ import { useQuery } from '@tanstack/react-query'; +import { useSessionStore } from '@/stores/session'; + import { getBootstrap } from './bootstrap'; -/** Bootstrap config barely changes within a session -- fetch once, keep indefinitely. */ +/** + * Bootstrap config barely changes within a session -- fetch once, keep indefinitely. Keyed by + * tenant code, not just `['bootstrap']`, so switching tenants (TenantEntryPage's "切换企业", no full + * page reload) fetches the NEW tenant's own message_policy/features/permissions/security instead of + * silently keeping the previous tenant's frozen (staleTime: Infinity) response around -- the + * queryClient itself is a single app-wide instance that's never cleared on tenant switch. + */ export function useBootstrap() { - return useQuery({ queryKey: ['bootstrap'], queryFn: getBootstrap, staleTime: Infinity }); + const tenantCode = useSessionStore((s) => s.tenant?.code); + return useQuery({ queryKey: ['bootstrap', tenantCode], queryFn: getBootstrap, staleTime: Infinity }); } diff --git a/web/src/lib/tinode/connection.ts b/web/src/lib/tinode/connection.ts index 9627fe06..1ebbac0a 100644 --- a/web/src/lib/tinode/connection.ts +++ b/web/src/lib/tinode/connection.ts @@ -189,12 +189,20 @@ export function getTinode(): Tinode { return tinode; } -/** Tears down the singleton so the next getTinode() rebuilds against a freshly-resolved tenant. */ +/** Tears down the singleton so the next getTinode() rebuilds against a freshly-resolved tenant. + * Unconditional `disconnect()` (not just when `isConnected()`) -- the SDK's own WS disconnect() is + * a safe no-op with no socket, but a connection still mid-handshake (not yet `isConnected()`) needs + * tearing down too, or it keeps trying to connect in the background against the OLD tenant/host + * after this call. Also drops connectPromise/restorePromise: without this, a stale in-flight + * connect()/restoreSession() for the tenant just abandoned here would still be what a *new* + * connect() call (against the newly-created Tinode instance for the newly-selected tenant) returns, + * since those are keyed by nothing but "is there currently a pending one" -- confirmed reachable via + * TenantEntryPage.tsx's "切换企业" flow racing a slow-to-respond old host. */ export function resetTinode() { - if (tinode?.isConnected()) { - tinode.disconnect(); - } + tinode?.disconnect(); tinode = null; + connectPromise = null; + restorePromise = null; setStatus('disconnected'); } diff --git a/web/src/lib/tinode/topics.ts b/web/src/lib/tinode/topics.ts index 8af5e4db..53842c54 100644 --- a/web/src/lib/tinode/topics.ts +++ b/web/src/lib/tinode/topics.ts @@ -71,8 +71,15 @@ export function getMessagesSnapshot(topicName: string): StoredMessage[] { return cached; } +/** Deduped the same way openConversation/connect are -- subscribing the same `me` topic twice + * concurrently is the same non-idempotent-WS-call shape as those (currently reachable from + * RequireAuth.tsx/LoginPage.tsx/RegisterPage.tsx, which don't overlap today, but this is cheap + * insurance against a future call site that does, e.g. a manual "reload contacts" retry button). */ +let inFlightMeSubscribe: Promise | null = null; + /** Call once at session start (after login) -- chat-list metadata only, no message bodies. */ export function subscribeMeTopic(): Promise { + if (inFlightMeSubscribe) return inFlightMeSubscribe; const me = getTinode().getMeTopic(); wireTopicCallbacks(me); // Read/recv/unread updates (e.g. from noteRead() when opening a conversation) travel through @@ -80,9 +87,19 @@ export function subscribeMeTopic(): Promise { // doesn't touch it. Without this, the chat list's unread badge/ordering goes stale after reading // a conversation until some unrelated topic event happens to invalidate its cached snapshot. me.onContactUpdate = () => notifyTopic(me.name); - return me.subscribe(me.startMetaQuery().withLaterSub().withDesc().withTags().withCred().build()); + inFlightMeSubscribe = me.subscribe(me.startMetaQuery().withLaterSub().withDesc().withTags().withCred().build()).finally(() => { + inFlightMeSubscribe = null; + }); + return inFlightMeSubscribe; } +/** Tracks prefetchPreview's own in-flight subscribe per topic, so openConversation (below) can wait + * for it to settle first instead of firing a second overlapping `{sub}` packet at the same topic -- + * this is a real path, not just theoretical: ChatListColumn prefetches every visible contact's + * preview on render, and a deep link or fast click can open one of those same topics before its + * preview subscribe has resolved. */ +const inFlightPreview = new Map>(); + /** * One-shot per topic: subscribes just enough to learn its latest message for the chat-list preview. * Deliberately never left afterward (matches the old app) -- simpler than tracking teardown for a @@ -93,9 +110,13 @@ export function prefetchPreview(topicName: string): void { previewPrefetched.add(topicName); const topic = getTinode().getTopic(topicName); wireTopicCallbacks(topic); - topic.subscribe(topic.startMetaQuery().withLaterData(1).build()).catch(() => { + const promise = topic.subscribe(topic.startMetaQuery().withLaterData(1).build()).catch(() => { previewPrefetched.delete(topicName); }); + inFlightPreview.set(topicName, promise); + promise.finally(() => { + if (inFlightPreview.get(topicName) === promise) inFlightPreview.delete(topicName); + }); } /** @@ -109,6 +130,20 @@ export function prefetchPreview(topicName: string): void { */ const inFlightSubscribe = new Map>(); +/** + * Bumped by every openConversation() call, checked by closeConversation() after it's done waiting -- + * without this, StrictMode's mount->cleanup->remount still breaks a freshly (re)opened conversation: + * cleanup-1 awaits the SAME shared inFlightSubscribe promise mount-2 is also relying on (they're + * deduped to one promise, per the comment above), and once it resolves, cleanup-1's queued + * continuation calls leave() unconditionally -- detaching the subscription mount-2 just established + * a moment earlier, with no error surfaced anywhere (confirmed by reading tinode.dev.js: `_attached` + * only flips true on a successful subscribe, and leave(false) while `_attached` is true really does + * send `{leave}` and reset it). The topic silently stops receiving live messages/receipts. Capturing + * the generation synchronously at closeConversation's call time and re-checking it after all awaits + * settle detects "a newer open has since claimed this topic" and skips the stale leave(). + */ +const topicGeneration = new Map(); + /** * Opens a topic for full conversation view: fetches a page of history and wires live updates. * Uses withData(undefined, undefined, limit), NOT withLaterData(limit) -- if prefetchPreview already @@ -120,9 +155,14 @@ const inFlightSubscribe = new Map>(); * never used) -- not yet replicated here since the new-conversation picker flow doesn't exist yet. */ export function openConversation(topicName: string): Promise { + topicGeneration.set(topicName, (topicGeneration.get(topicName) || 0) + 1); const existing = inFlightSubscribe.get(topicName); if (existing) return existing; const promise = (async () => { + // Let any in-flight chat-list preview subscribe for this same topic settle first, rather than + // firing a second overlapping `{sub}` packet at it (see inFlightPreview above). + const previewPending = inFlightPreview.get(topicName); + if (previewPending) await previewPending.catch(() => {}); const topic = getTinode().getTopic(topicName); wireTopicCallbacks(topic); if (topic.isSubscribed()) { @@ -141,10 +181,14 @@ export function openConversation(topicName: string): Promise { /** Soft-leave (detach, not unsubscribe) -- cached messages stay in the SDK's local buffer. Waits out * any in-flight openConversation() for this same topic first, so a cleanup that fires before the - * subscribe it's undoing has even resolved doesn't just silently no-op (see inFlightSubscribe above). */ + * subscribe it's undoing has even resolved doesn't just silently no-op (see inFlightSubscribe above) + * -- then, per topicGeneration's comment above, skips the actual leave() if a NEWER openConversation + * call has since claimed this topic while this one was waiting. */ export async function closeConversation(topicName: string): Promise { + const myGeneration = topicGeneration.get(topicName) || 0; const pending = inFlightSubscribe.get(topicName); if (pending) await pending.catch(() => {}); + if (topicGeneration.get(topicName) !== myGeneration) return; await getTinode().getTopic(topicName).leave(false).catch(() => {}); }