fix: prevent event listener accumulation during sync retries#278
Closed
Just-Insane wants to merge 29 commits intoSableClient:devfrom
Closed
fix: prevent event listener accumulation during sync retries#278Just-Insane wants to merge 29 commits intoSableClient:devfrom
Just-Insane wants to merge 29 commits intoSableClient:devfrom
Conversation
…report enhancements
- Add window.Sentry assignment for browser console access - Export Sentry from instrument module for app-wide use - Allows developers to test Sentry from browser console with Sentry.captureMessage() etc
- Add DSN, environment, and release logging on Sentry initialization - Show event IDs in test button alerts for verification - Add console logging for all Sentry test operations - Add 'Show Diagnostics' button to display full configuration - Improved error handling with clearer feedback messages - Makes it easier to debug why Sentry events aren't appearing
- Update tracesSampleRate to 100% for development and preview - Update replaysSessionSampleRate to 100% for development and preview - Update SentrySettings UI to reflect preview environment gets 100% sampling - Improves debugging experience for Cloudflare Pages preview deployments
- Add VITE_SENTRY_ENVIRONMENT to production workflow (cloudflare-web-deploy.yml) - Configure prepare-tofu action to pass through Sentry environment variables - Update SENTRY_INTEGRATION.md with comprehensive deployment configuration: - Environment variable documentation with sampling rate details - Deployment-specific configurations for production/preview/development - Sampling rate table showing 10% for production, 100% for preview/dev - Production builds from dev branch now properly set environment=production
…ser context
- Add browserProfilingIntegration + profilesSampleRate
- Add consoleLoggingIntegration, fix sendDefaultPii: false
- Wire Sentry.logger.* and error/warn count metrics in debugLogger
- slidingSync: sable.sync.{cycle,error,initial_ms,processing_ms}
- ClientRoot: sable.sync.time_to_ready_ms, pseudonymous SHA-256 hashed user ID
- RoomInput: sable.message.{send_latency_ms,send_error} + matrix.message span
- room.ts: sable.decryption.bulk_latency_ms + matrix.crypto span
- EncryptedContent: sable.decryption.{event_ms,failure} (5% sampled span)
- matrix.ts: sable.media.{upload_latency_ms,upload_bytes,upload_error}
- RoomTimeline: sable.{pagination,timeline}.* metrics + matrix.timeline span
- ClientNonUIFeatures: sable.notification.delivery_ms
- DirectDMsList: sable.roomlist.time_to_ready_ms
- Replace react-error-boundary with Sentry.ErrorBoundary in App.tsx so
crashes are auto-captured and an eventId is passed to the fallback
- ErrorPage: when Sentry captured the crash, show "Add Details to Report"
(Sentry.showReportDialog) as primary action and GitHub as secondary;
fall back to GitHub-only when Sentry is not configured
- BugReportModal: detect Sentry via isInitialized() at runtime
- Sentry enabled + bug: submit to Sentry by default, GitHub opt-in via
"Also create a GitHub issue" checkbox (default off)
- Sentry disabled / feature request: always open GitHub (unchanged UX)
- Error Tracking section hidden entirely when Sentry is not configured
- Submit button label changes to "Submit Report" when Sentry is active
Remove "Send Test Error", "Send Test Feedback", and "Attach Debug Logs" setting tiles and their handler functions from SentrySettings.tsx. Also drop the now-unused getDebugLogger import. The "Show Diagnostics" tile and the enable/replay toggles are kept.
…browser profiling
When a sliding-sync limited response fires RoomEvent.TimelineReset (or
the app fires RoomEvent.TimelineRefresh) the useLiveTimelineRefresh
callback was unconditionally calling setAtBottom(true) and incrementing
scrollToBottomRef, scrolling the user to the bottom even if they had
scrolled up to read history.
Three coordinated changes:
1. Add RoomEvent.TimelineReset handler to useLiveTimelineRefresh.
The SDK emits TimelineReset on the EventTimelineSet (not the Room)
when a limited sync response replaces the live EventTimeline. Without
this listener the stored linkedTimelines reference the old detached
chain; back-pagination silently no-ops, freezing the room.
2. Gate the viewport scroll on atBottomRef.current (prior position).
Capture wasAtBottom before calling setTimeline(getInitialTimeline).
Only call setAtBottom(true) and increment scrollToBottomRef when the
user was already at the bottom. When scrolled up we still reinit the
timeline (the old chain is gone) but avoid the forced scroll.
3. Add timelineJustResetRef to allow the self-heal effect to run when
atBottom=false after a reset. The SDK fires TimelineReset before
populating the fresh timeline, so getInitialTimeline sees range.end=0.
Without the atBottom guard bypass the range stays at {0,0} as events
arrive, leaving the virtual paginator rendering nothing. The ref is
cleared on first successful heal, after which normal atBottom-gated
behaviour resumes.
useLiveEventArrive, useRelationUpdate, useLiveTimelineRefresh, and
useThreadUpdate each passed their callback argument into the useEffect
dependency array. This caused the listener to be removed and re-added
every time the callback identity changed (e.g. when unreadInfo toggled
or when any other parent-scope value included in the useCallback deps
changed).
Under normal conditions React's synchronous effect cleanup guarantees
one listener at a time. But during sliding-sync error/retry cycles the
client emits MaxListenersExceededWarning (11 session_ended, 51
RoomState.events) confirming that listeners accumulate. When the
callbacks change fast enough — or React defers cleanup in concurrent
mode — multiple handleTimelineEvent instances are live simultaneously.
Each one independently calls setTimeline({ range: start+1, end+1 }) for
the same arriving event, producing the spurious scroll jumps reported.
Fix: use the stable callback ref pattern in all four hooks.
- Add a ref (onArriveRef / onRelationRef / onRefreshRef / onUpdateRef)
- Assign ref.current = callback on every render (always up-to-date)
- Remove the callback from the useEffect dep array (effect runs only
when `room` changes)
- Inside handlers, delegate via ref.current() instead of the captured
closure value
The event listener identity is now stable for the entire lifetime of
each room mount. Listener counts stay at 1 regardless of how many
re-renders occur or how many sync error/retry cycles fire.
Fix/timeline refresh scroll
# Conflicts: # .github/workflows/cloudflare-web-deploy.yml
Added Sentry configuration variables for deployment.
- Add unreadInfoRef in RoomTimeline; remove unreadInfo from useLiveEventArrive callback deps and LocalEchoUpdated effect deps to stop per-message listener churn - Fix CallEmbed .bind(this) leak: bind handlers once in start(), push cleanup to disposables so dispose() correctly removes all listeners - Add stable refs in useCallSignaling for mutedRoomId/ring callbacks; tighten effect deps to [mx, mDirects, setMutedRoomId] to prevent SessionEnded listener accumulation during sync retry cycles
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes event listener accumulation that caused
MaxListenerswarnings (11 session_ended,51 RoomState.events) and spurious scroll-to-bottom jumps during sync retries/reconnects.Changes
RoomTimeline.tsx—unreadInforefunreadInfoRefand removedunreadInfofromuseLiveEventArrivecallback deps andLocalEchoUpdatedeffect depsLocalEchoUpdatedlistener from being detached and re-attached on every message (sinceunreadInfostate transitions on each new event)CallEmbed.ts—.bind(this)leakstart(), bound each handler to a namedconstonce rather than calling.bind(this)inline at bothon()andoff()callsites.bind()creates a new function object each time, theoff()calls indispose()were no-ops — listeners were never removedthis.disposablessodispose()correctly removes all listenersuseCallSignaling.ts— stable refs for volatile depsmutedRoomIdRef,playRingingRef,stopRingingRef,playOutgoingRingingRef[mx, mDirects, playRinging, stopRinging, mutedRoomId, setMutedRoomId, playOutgoingRinging]to[mx, mDirects, setMutedRoomId]SessionEnded/RoomState.eventslisteners from being torn down and re-registered on every call state changeTesting
MaxListenersExceededWarningduring repeated sync retries