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 <noreply@anthropic.com>
2026-09-08 05:27:16 +08:00
parent c9f4615c40
commit 8ec74dffcc
6 changed files with 124 additions and 19 deletions
@@ -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 <Navigate to="/tenant" replace />;
-  if (myUserId) return <Outlet />;
-  if (hasToken && !restoreFailed) {
-    return <div className="grid h-dvh place-items-center bg-surface-page text-sm text-text-tertiary-3">正在恢复登录状态…</div>;
+  if (myUserId) {
+    return (
+      <>
+        <Outlet />
+        <Toast message={contactsError} variant="error" onDone={() => setContactsError('')} />
+      </>
+    );
+  }
+  if (hasToken && !restoreFailed && !restoreTimedOut) {
+    return (
+      <div className="flex h-dvh flex-col items-center justify-center gap-3 bg-surface-page text-sm text-text-tertiary-3">
+        正在恢复登录状态…
+        <a href="/login" className="text-accent no-underline">
+          改为重新登录
+        </a>
+      </div>
+    );
  }
  return <Navigate to="/login" replace />;
 }
@@ -165,7 +165,7 @@ export async function imRequest<T = unknown>(
      );
    }
  }
-  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 = <T = unknown>(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<T>(
  fetchPage: (cursor: string) => Promise<{ items?: T[]; next_cursor?: string }>,
  startCursor = '',
 ): Promise<T[]> {
  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;
  }
@@ -114,6 +114,14 @@ export function unblockUser(userId: string) {
  return imDelete<void>(`/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)}` : ''}`);
 }
@@ -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 });
 }
@@ -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');
 }

@@ -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<unknown> | null = null;
+
 /** Call once at session start (after login) -- chat-list metadata only, no message bodies. */
 export function subscribeMeTopic(): Promise<unknown> {
+  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<unknown> {
  // 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<string, Promise<unknown>>();
+
 /**
 * 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<string, Promise<Topic>>();

+/**
+ * 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<string, number>();
+
 /**
 * 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<string, Promise<Topic>>();
 * never used) -- not yet replicated here since the new-conversation picker flow doesn't exist yet.
 */
 export function openConversation(topicName: string): Promise<Topic> {
+  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<Topic> {

 /** 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<void> {
+  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(() => {});
 }