Skip to content

fix(typing): reset typing on foreground/reconnect + refresh conversation on reconnect#188

Merged
yujiawei merged 1 commit into
mainfrom
fix/typing-background-reconnect-reset
Jun 1, 2026
Merged

fix(typing): reset typing on foreground/reconnect + refresh conversation on reconnect#188
yujiawei merged 1 commit into
mainfrom
fix/typing-background-reconnect-reset

Conversation

@yujiawei
Copy link
Copy Markdown
Contributor

@yujiawei yujiawei commented Jun 1, 2026

问题

octo-web 群聊 typing indicator 在 tab 切后台或 WebSocket 重连后卡死不消失,且当前打开的会话窗口不刷新断连期间的消息。

根因:typing 状态进入路径与清除路径不对称。typing 唯一清除点挂在实时 WS push 的全局 messageListener 上,而 App 后台/断连期间 bot 真实回复经 HTTP sync 落库,该路径从不触发 messageListener → typing 永不清。SDK 重连只 reSubscribe,不补拉离线消息/CMD,导致当前会话窗口也不刷新。三端同构(iOS octo-ios#10 / Android octo-android#17 已合入对应修复)。

修复(两层防御,对齐三端)

  • TypingManagerService/TypingManager.tsx):typingMap value 改为 { typing, channel },支持 Channel 反查;新增 resetAll()——快照所有 channel、stop timer、clear map,再逐个 notifyTypingListener(channel, false) 广播清除(先快照后 clear,避免遍历中改 map)。
  • 回前台 resetApp.tsx visibilitychange,第一层):复用既有 handler,document.hidden===false 时除原有 refresh() 外追加 TypingManager.shared.resetAll()。对应 iOS appDidBecomeActive
  • 重连 resetApp.tsx 全局 connectStatusListener,第二层):新增 ConnectStatus.Connected 分支调 resetAll()。放全局 App 单例(生命周期最长),不放页面 VM。
  • 重连补刷当前会话Components/Conversation/vm.ts):didMount 注册 connectStatusListener,ConnectedrequestMessagesOfFirstPage(0) 补刷首屏,5s 去抖(参考 remoteConfig foreground refresh pattern);didUnMount 成对 removeConnectStatusListener,避免页面切换累积 listener → 内存泄漏 + 重连多实例并发拉首屏。

测试

  • 新增 Service/__tests__/TypingManager.test.ts:覆盖 resetAll() 清除并广播每个会话、空 map no-op、stop timer 防止延迟触发(3 用例全过)。
  • dmworkbase 既有用例不回归(预存在的 jsdom/canvas/lottie 环境失败与本改动无关,本分支失败数较 base 更少)。
  • tsc 对改动文件零新增类型错误(既有 16 项错误为 base 预存在的 ambient 类型噪声)。

验证点

  1. 群聊 bot 回复 → 切后台等回复完成 → 切回前台:typing 立即消失,bot 回复显示在当前会话。
  2. 断网重连后:typing 不残留,当前会话补刷断连期间消息。
  3. 正常前台聊天 typing 显示/消失不受影响。
  4. 反复切换会话页面不泄漏 listener(didUnMount 已成对注销)。

Closes #187

…tion on reconnect

Typing indicators got stuck after the tab went to background or after a
WebSocket reconnect, because typing is only cleared on the realtime push
path (global messageListener). Bot replies that arrive while backgrounded/
disconnected are persisted via the HTTP sync path, which never notifies the
message listener, so typing was never cleared. The SDK reconnect also does
not pull offline messages, so the currently-open conversation did not
refresh.

Two-layer defense aligned with iOS#10 / Android#17:
- TypingManager: store { typing, channel } so resetAll() can reverse-look-up
  the channel and broadcast notifyTypingListener(channel, false). Snapshot
  channels before clearing the map.
- App.tsx visibilitychange (foreground) -> resetAll() (layer 1).
- App.tsx global connectStatusListener Connected -> resetAll() (layer 2),
  kept on the app-level singleton, not the page VM.
- ConversationVM: register a connectStatusListener that refreshes the first
  page on reconnect, with a 5s debounce; paired removeConnectStatusListener
  in didUnMount to avoid listener leaks / concurrent first-page loads.

Adds unit tests for TypingManager.resetAll().
@yujiawei yujiawei requested a review from a team as a code owner June 1, 2026 08:52
@github-actions github-actions Bot added the size/M PR size: M label Jun 1, 2026
Copy link
Copy Markdown
Contributor

@lml2468 lml2468 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Fixes stuck typing indicators after reconnect/foreground and refreshes the active conversation on reconnect. Two-layer defense: TypingManager.resetAll() on both visibilitychange (foreground) and ConnectStatus.Connected (reconnect). Adds a reconnect-triggered first-page refresh in ConversationVM with 5s debounce.

Findings

No blocking issues.

  • P2 (nit): check-sprint CI fails due to Sprint mismatch on the linked issue (same project-board metadata issue as other recent PRs). Not a code problem.

Verification

  • TypingManager refactored from Map<string, Typing> to Map<string, TypingEntry> so resetAll() can notify per-channel — all existing methods (addTyping, getFakeTypingMessage, getTyping, removeTyping) correctly adapted.
  • resetAll() uses snapshot-then-clear pattern to avoid iterator invalidation during notification. Stops all timers before clearing to prevent ghost callbacks.
  • connectStatusListener in vm.ts is properly paired: added in didMount (line 876), removed in didUnMount (line 929). No listener leak.
  • requestMessagesOfFirstPage(0) is the existing method already used for initial load — consistent and safe.
  • 5s debounce via lastReconnectRefreshAt prevents rapid reconnect storms from hammering the server.
  • 3 unit tests cover: resetAll clears+notifies, no-op when empty, stopped timers do not fire late. Mocking strategy for heavy deps (App, lottie) is correct.
  • Build CI passed.

Verdict

APPROVED — clean implementation, proper lifecycle management, good test coverage.

Copy link
Copy Markdown
Contributor Author

@yujiawei yujiawei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — PR #188 (octo-web)

Summary

This PR fixes the group-chat typing indicator that stayed stuck after the tab was backgrounded or the WebSocket reconnected, plus refreshes the open conversation after reconnect. The root cause is correctly identified: typing has only one clearing path (the real-time WS-push messageListener), while bot replies that arrive during background/disconnect are persisted via the HTTP sync path, which never triggers that listener — so typing is never cleared. The fix mirrors the already-landed iOS (#10) and Android (#17) approach with a clean two-layer defense.

Verdict: APPROVED — no correctness, security, or data-loss issues. A few non-blocking P2/UX notes below.

What was reviewed

  • Service/TypingManager.tsxtypingMap value changed to { typing, channel }, new resetAll().
  • App.tsxvisibilitychange (layer 1, foreground) and global connectStatusListener Connected branch (layer 2, reconnect) both call resetAll().
  • Components/Conversation/vm.ts — per-conversation reconnect refresh with 5s debounce + paired listener removal.
  • Service/__tests__/TypingManager.test.ts — new unit tests.

Verification

  • resetAll() is iteration-safe. It snapshots channels first, then clear(), then broadcasts notifyTypingListener(channel, false) — no map mutation during traversal, and timers are stopped before clearing (TypingManager.tsx:69-86). The empty-map short-circuit makes it a true no-op when idle, so the initial (non-reconnect) Connected event is harmless.
  • Listener lifecycle is paired. connectStatusListener is registered in didMount (vm.ts:865-876) and removed in didUnMount (vm.ts:929), preventing the leak / multi-instance concurrent first-page fetch the PR description warns about. typingListener removal is likewise intact.
  • Reconnect refresh debounce is correct. lastReconnectRefreshAt gate (vm.ts:869-873) reuses the 5s pattern from refreshRemoteConfigOnForeground (App.tsx:731-738); repeated disconnect/reconnect storms won't repeatedly hammer the first-page sync.
  • App.tsx branch ordering is safe. The new ConnectStatus.Connected branch sits after ConnectKick and reasonCode === 2 (auth-fail) (App.tsx:672-682). A successful Connected never carries reasonCode === 2, so the auth-fail path can't shadow it.
  • getFakeTypingMessage / getTyping updated for the new entry shape (TypingManager.tsx:42-59); all other call sites (ConversationList/index.tsx, module.tsx) go through hasTyping/getTyping/removeTyping and need no change.
  • Tests cover the three meaningful behaviors of resetAll(): clears + broadcasts per channel, no-op on empty, and stops the 8s timer so a stale removal can't fire late. Heavy DOM deps are mocked appropriately for unit isolation.

Findings

P2 — Reconnect refresh forces scroll-to-bottom even when the user is scrolled up

requestMessagesOfFirstPage(0)syncMessages(0)refreshAndLocateMessages(..., scrollBottom = true) (vm.ts:1505, 1538-1548), which calls scrollToBottom. If a user is reading older history when a reconnect lands, the view will jump to the bottom. Note that the existing typingListener deliberately guards this case with an early if (this.showScrollToBottomBtn) return (vm.ts:844), but the new reconnect path has no equivalent guard. Reconnects are relatively rare and debounced, so this is a UX nit rather than a blocker — consider suppressing the auto-scroll (or skipping the refresh) when showScrollToBottomBtn is true, to keep behavior consistent with the typing path.

P2 (pre-existing, not introduced here) — resetAll broadcast is swallowed in-chat when scrolled up

When resetAll() broadcasts notify(channel, false), the ConversationVM.typingListener early-returns if showScrollToBottomBtn is true (vm.ts:844-846), so the in-chat typing bubble may linger in the rendered list until the next refresh. The sidebar (ConversationList) listener calls setState({}) unconditionally (ConversationList/index.tsx:308-311) and clears correctly. This is a property of the pre-existing showScrollToBottomBtn early-return (the same F1 limitation already present before this PR), not something this change introduces — flagging only for awareness. The reconnect refresh added here actually mitigates it, since the subsequent first-page reload re-renders the list without the stale typing entry.

Nit — comment accuracy

The App.tsx:678-681 comment ("SDK 重连只 reSubscribe,不补拉离线消息/CMD") is accurate per the prior cross-platform investigation; no action needed, just confirming the rationale is sound.

Conclusion

The change is correct, minimal, well-commented, properly tears down its listeners, and is symmetric with the iOS/Android fixes. The two P2 items are UX refinements around scroll behavior and do not block merge.

Copy link
Copy Markdown
Contributor

@Jerry-Xin Jerry-Xin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary: The PR is in scope for octo-web and addresses the typing lifecycle/reconnect gap with a reasonable two-layer reset plus current-conversation refresh. I found no blocking correctness, security, or architecture issues.

💬 Non-blocking

  • 🟡 Warning: packages/dmworkbase/src/Components/Conversation/vm.ts:865 starts a first-page refresh on every Connected event without checking whether another syncMessages() call is already in flight. Existing code already lacks a request sequencing guard, but this new reconnect path increases the chance of overlapping full-page loads during reconnect/mount/user refresh. Consider a small in-flight guard or request token if this area has shown out-of-order UI refreshes.

  • 🔵 Suggestion: packages/dmworkbase/src/Service/__tests__/TypingManager.test.ts:37, :61, and :70 register singleton listeners and remove them only at the end of each test body. If an assertion throws before cleanup, later tests can inherit stale listeners. A try/finally or afterEach cleanup, plus vi.useRealTimers(), would make the tests more robust.

✅ Highlights

  • packages/dmworkbase/src/Service/TypingManager.tsx:72 snapshots channels, stops timers, clears the map, then broadcasts removals. That avoids mutation-during-iteration problems and prevents late timer callbacks.

  • packages/dmworkbase/src/Components/Conversation/vm.ts:929 correctly unregisters the reconnect listener on unmount, avoiding the listener accumulation risk described in the PR.

  • packages/dmworkbase/src/App.tsx:677 and :721 place typing reset in long-lived app lifecycle paths, which matches the bug’s asymmetric clear-path root cause.

Verification note: I attempted to run the focused Vitest test, but the local workspace does not have the vitest binary available through pnpm exec, so I could not execute it in this checkout.

@yujiawei yujiawei merged commit 7a42c23 into main Jun 1, 2026
16 of 17 checks passed
@yujiawei yujiawei deleted the fix/typing-background-reconnect-reset branch June 1, 2026 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M PR size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Web] typing indicator 后台/重连卡死 + 当前会话消息不刷新

3 participants