fix: flickering of inactive collaborator #4053
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4053 +/- ##
==========================================
+ Coverage 88.83% 88.84% +0.01%
==========================================
Files 422 422
Lines 19118 19118
==========================================
+ Hits 16984 16986 +2
+ Misses 2134 2132 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
doc-han
requested review from
elias-ba,
stuartc,
taylordowns2000 and
theroinaochieng
|
Previous conversations at #3951 |
doc-han
force-pushed
the
3931-inactive-collaborators
branch
from
1749227 to
d81a0c9
Compare
| const awayUserCache = new Map< | ||
| string, | ||
| { user: AwarenessUser; expiresAt: number } | ||
| >(); | ||
|
|
||
| const AWAY_CACHE_DURATION = 60000; // 60 seconds |
There was a problem hiding this comment.
I am wondering if we shouldn't move this into AwarenessStore so that we can properly scope it and clean it up to avoid cache pollutions and memory leaks when users open workflows in multiple tabs. Wouldn't the fact that it's module-level Map make it shared across all component instances and persists across React re-renders ?
| if (visibilityProps) { | ||
| const handler = activityStateChangeHandler.bind(undefined, setState); | ||
| // initial call | ||
| handler(); | ||
| document.addEventListener(visibilityProps.visibilityChange, handler); | ||
|
|
||
| // return cleanup function | ||
| return () => { | ||
| document.removeEventListener(visibilityProps.visibilityChange, handler); | ||
| }; |
There was a problem hiding this comment.
What happens when visibilityProps is null ? Wouldn't the function return undefined instead of a no-op cleanup function ? Wouldn't this cause issues in StoreProvider.tsx:160 where the cleanup is called unconditionally ?
| { user: AwarenessUser; expiresAt: number } | ||
| >(); | ||
|
|
||
| const AWAY_CACHE_DURATION = 60000; // 60 seconds |
There was a problem hiding this comment.
The 60-second duration looks arbitrary. If that's the case, shouldn't we consider documenting why this specific value was chosen. Is it related to typical browser throttling behavior? Should it be configurable?
| userMap.set(userId, user); | ||
| } | ||
| connectionCounts.set(userId, (connectionCounts.get(userId) || 0) + 1); | ||
| (userMap.get(userId) || userMap.set(userId, []).get(userId)!).push(user); |
There was a problem hiding this comment.
This looks dense. No way to simplify ?
|
Moving this PR to draft since its priority at the moment has been changed to low |
|
@doc-han to work out and propose next steps for this |
Description
Better implements
active|away. also deals with when they switch from the tab and back.lastSeen.What was causing the flickering?
setIntervalwhich updates the user'slastSeenevery 10s, just so we are in the 30s intervalsetIntervalstart to miss their interval calls. eg. a 10s interval call might be actually triggered after 60s.Approach taken
lastStatecontrolled by the browser visibility API to track whether a user isactiveorawayHow do we smoothing the presence of a user to avoid the flickering?
away. (throttling likely happens to away users)Closes #3931
Validation steps
Additional notes for the reviewer
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
:owner,:admin,:editor,:viewer)