fix: eliminate unread badge dispatch storm that crashed TabBar rendering - #3437
fix: eliminate unread badge dispatch storm that crashed TabBar rendering#3437jeanfbrito merged 1 commit into
Conversation
Sorry, something went wrong.
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: type: bug Suggested reviewers: ggazzo 🚥 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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)src/servers/preload/badge.ts (1)🤖 Prompt for all review comments with AI agents6-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add 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 hasDispatched and lastBadge between tests so one test does not hide another.
🤖 Prompt for AI AgentsVerify 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.
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.
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
📥 CommitsReviewing files that changed from the base of the PR and between a5d10ec and 564f989.
📒 Files selected for processing (4)- src/injected.ts
- src/servers/bootWatchdog.ts
- src/servers/preload/badge.ts
- src/servers/reducers.ts
- 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)
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from @rocket.chat/fuselage for UI work unless the design requires something Fuselage does not provide.
Check Theme.d.ts for valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local .d.ts files 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 by Theme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such as process.getuid(), getgid(), geteuid(), and getegid().
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.ts files instead of assuming they are valid.
Files:
- src/servers/reducers.ts
- src/servers/preload/badge.ts
- src/servers/bootWatchdog.ts
- src/injected.ts
src/servers/bootWatchdog.ts (1)src/injected.ts (1)212-214: LGTM!
src/servers/reducers.ts (1)530-539: LGTM!
Also applies to: 582-595, 614-614, 623-623
80-100: LGTM!
Also applies to: 110-113
Sorry, something went wrong.
| 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 AgentsVerify 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.
Sorry, something went wrong.
Linux installer download |
Sorry, something went wrong.
macOS installer download |
Sorry, something went wrong.
Windows installer download |
Sorry, something went wrong.
Summary
Root-causes and fixes the long-standing intermittent Maximum update depth exceeded storm (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:
Every 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-subscription event per room. The chain amplified each one:
Changes
Validation
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