fix: eliminate unread badge dispatch storm that crashed TabBar rendering - #3437
Conversation
On webapp boot the server fires one unread-changed-by-subscription event per room. Each event dispatched a badge update to the root window's Redux store, the servers reducer minted a new array even for no-op patches, and every server-subscribed component re-rendered per dispatch — with unread rooms present at startup, React aborted TabBar with 'Maximum update depth exceeded' on every launch. - injected.ts: coalesce badge recomputes into a 100ms trailing-edge timer and skip setBadge entirely when the resolved value did not change - preload badge.ts: skip dispatching consecutive identical badge values (also covers the pre-7.8.0 Session autorun path) - servers reducers: upsert/update now preserve object and array identity when a patch would not change any field, so no-op actions no longer re-render every consumer - bootWatchdog: arm the boot deadline on the first committed navigation instead of on attach — webviews that legitimately never navigate (lazy or error panes) produced false boot-deadline-exceeded reports
WalkthroughThe PR coalesces unread badge updates and suppresses unchanged dispatches. It defers boot watchdog deadlines until navigation. It also preserves server state identity for no-op updates. ChangesUnread badge update flow
Boot watchdog lifecycle
Server reducer identity preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/servers/preload/badge.ts (1)
6-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for the deduplication contract.
The adjacent test only verifies one dispatch. Add cases for the first badge value, a repeated equal value, and a changed value. Reset the module-scoped
hasDispatchedandlastBadgebetween tests so one test does not hide another.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/servers/preload/badge.ts` around lines 6 - 18, Add regression tests covering setBadge’s first dispatch, suppression of a repeated equal badge, and dispatch of a changed badge. Reset the module-scoped hasDispatched and lastBadge state between tests so each case independently verifies the deduplication contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/injected.ts`:
- Around line 540-541: Share badge deduplication across the event and pre-7.8
Tracker.autorun producers: route both paths through a single helper that updates
the same lastSentBadge state, or remove the local guard and rely on the shared
deduplication in preload/badge.ts. Ensure alternating badge values from either
producer are applied correctly without stale local state suppressing updates.
---
Nitpick comments:
In `@src/servers/preload/badge.ts`:
- Around line 6-18: Add regression tests covering setBadge’s first dispatch,
suppression of a repeated equal badge, and dispatch of a changed badge. Reset
the module-scoped hasDispatched and lastBadge state between tests so each case
independently verifies the deduplication contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b11a535-fa5e-47e3-a875-bc1cf87998fb
📒 Files selected for processing (4)
src/injected.tssrc/servers/bootWatchdog.tssrc/servers/preload/badge.tssrc/servers/reducers.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: build (windows-latest, windows)
- GitHub Check: build (macos-latest, mac)
- GitHub Check: build (ubuntu-latest, linux)
- GitHub Check: check (macos-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
- GitHub Check: Analyze (javascript)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires something Fuselage does not provide.
CheckTheme.d.tsfor valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local.d.tsfiles instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
Files:
src/servers/reducers.tssrc/servers/preload/badge.tssrc/servers/bootWatchdog.tssrc/injected.ts
🔇 Additional comments (3)
src/servers/bootWatchdog.ts (1)
212-214: LGTM!src/injected.ts (1)
530-539: LGTM!Also applies to: 582-595, 614-614, 623-623
src/servers/reducers.ts (1)
80-100: LGTM!Also applies to: 110-113
| let lastSentBadge: number | '•' | undefined; | ||
| let hasSentBadge = false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Share badge deduplication state across both producers.
lastSentBadge tracks only values sent by resolveBadge. The pre-7.8 Tracker.autorun at Line [500-503] calls window.RocketChatDesktop.setBadge directly and does not update this state. If the event path sends 3, the Session path sends 0, and the event path computes 3 again, this guard returns and leaves the actual badge at 0.
Route both producers through one shared helper, or remove this second guard and rely on the shared deduplication in src/servers/preload/badge.ts.
Suggested fix
- let lastSentBadge: number | '•' | undefined;
- let hasSentBadge = false;
...
- if (hasSentBadge && badge === lastSentBadge) {
- return;
- }
- hasSentBadge = true;
- lastSentBadge = badge;
window.RocketChatDesktop.setBadge(badge);Also applies to: 573-579
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/injected.ts` around lines 540 - 541, Share badge deduplication across the
event and pre-7.8 Tracker.autorun producers: route both paths through a single
helper that updates the same lastSentBadge state, or remove the local guard and
rely on the shared deduplication in preload/badge.ts. Ensure alternating badge
values from either producer are applied correctly without stale local state
suppressing updates.
macOS installer download |
1 participant
Summary
Root-causes and fixes the long-standing intermittent
Maximum update depth exceededstorm (first reported as 119 errors within ~1 second, ~20s after startup). The trigger turned out to be data-dependent, not code-dependent — which is why it always looked random.How it was found
After #3435 and #3436 merged, the error storm still reproduced — now deterministically, ~2s after every startup, at reduced intensity (10–14 errors). Two techniques pinned it down:
1. Build bisection. Five builds were launched under identical conditions and the error count measured after startup:
setVersiondispatchEvery build reproduced, including the untouched baseline — so no merged PR caused it. The environment had changed instead: the storm only fires when servers have unread rooms at boot. Earlier same-day runs with no unread state were consistently clean, which is also why the bug historically appeared intermittent.
2. react-dom instrumentation. Patching
getRootForUpdatedFiber(locally, dev only) to print the fiber chain at the moment of the throw identified the crashing subscriber:Root cause
On webapp boot, the server fires one
unread-changed-by-subscriptionevent per room. The chain amplified each one:injected.tsrecomputed and calledsetBadgefor every event — no coalescing, no value dedupepreload/badge.tsdispatchedWEBVIEW_UNREAD_CHANGEDunconditionally, even for identical valuesupsert/updateminted a new array (and server object) even when the patch changed nothingTabBarand abortedChanges
src/injected.ts— badge recomputes are coalesced into a single 100ms trailing-edge call, andsetBadgeis skipped when the resolved value is unchangedsrc/servers/preload/badge.ts— consecutive identical badge values no longer dispatch (also covers the pre-7.8.0 Session autorun path)src/servers/reducers.ts—upsert/updatepreserve object and array identity for no-op patches, so no-op actions no longer re-render every consumersrc/servers/bootWatchdog.ts— the boot deadline now arms on the first committed navigation instead of on attach; webviews that legitimately never navigate (lazy or error panes) were producing falseboot-deadline-exceededreportsValidation
npx tsc --noEmitclean,yarn lintcleanyarn test: 155 suites, 1666 passed, 2 skippedyarn buildclean (externals check passed)Relationship to previous PRs
#3435's fixes remain valid and necessary — they reduced the storm's amplification (119 → ~12) by stabilizing subscribers. This PR removes the storm at its source. Whether the badge storm also explains the intermittent boot wedge (stuck throbber, #3436) is not yet confirmed; the boot watchdog from #3436 remains in place to capture forensic reports if a wedge recurs.
Summary by CodeRabbit