fix: DM sidebar tooltip shows empty while presence loads#41107
fix: DM sidebar tooltip shows empty while presence loads#41107ricardogarim wants to merge 2 commits into
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
|
| Layer / File(s) | Summary |
|---|---|
UserStatusTooltipContent component and hook wiring apps/meteor/client/hooks/useUserStatusTooltip.tsx |
Adds an internal component that fetches presence via useUserPresence and renders a Skeleton or UserStatusText accordingly; updates onMouseEnter to render this component within UserPresenceContext.Provider and changes the callback's dependency to userPresence. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested labels
type: bug
Suggested reviewers
- dougfabris
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly describes the main fix: the DM sidebar tooltip no longer appears empty while presence is loading. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
Warning
Review ran into problems
🔥 Problems
Errors were encountered while retrieving linked issues.
Errors (1)
- CORE-2359: Request failed with status code 401
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 help to get the list of available commands.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.6.0 #41107 +/- ##
================================================
Coverage ? 70.17%
================================================
Files ? 3371
Lines ? 130429
Branches ? 22642
================================================
Hits ? 91523
Misses ? 35588
Partials ? 3318
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
|
||
| openTooltip( | ||
| <UserStatusText status={presence?.status} statusText={presence?.statusText} statusExpiresAt={presence?.statusExpiresAt} />, | ||
| <UserPresenceContext.Provider value={userPresence}> |
There was a problem hiding this comment.
We already have a UserPresenceProvider lets use it, also would be better to wrap around UserStatusText instead of here
There was a problem hiding this comment.
good call to reach for it — i actually tried wiring the tooltip up with UserPresenceProvider first, but hit a few differences worth flagging.
because the tooltip content mounts fresh on every hover, using UserPresenceProvider there re-mounts the provider each time and since it's our app-root singleton, that re-runs the whole presence engine on every hover. what I saw:
- it clears the presence cache and refetches
/v1/users.presenceon each hover (setStatus→reset()→store.clear()) - it re-registers the away-detection listeners (
userPresence.use()) - it opens a second
stream-allsubscription
so I ended up keeping the existing bridge — <UserPresenceContext.Provider value={useContext(UserPresenceContext)}> — which reuses the presence the app already has loaded, so there's no extra fetch and no duplicated engine.
think we could still refactor this in the future to keep a single provider, but for this task it doesn't feel worth the effort.
2 participants
Proposed changes (including videos or screenshots)
Hovering a DM in the sidebar while the partner's presence was still loading showed an empty tooltip.
useUserStatusTooltipopened the tooltip with a static<UserStatusText>snapshot, and while presence wasundefinedthat snapshot rendered nothing.The hook now opens a small self-subscribing component (
UserStatusTooltipContent) that:Throbberwhile presence is loading, thenSince the tooltip renders above
UserPresenceProvider, the hook bridgesUserPresenceContextinto the tooltip so the content'suseUserPresenceresolves inside the portal. The whole fix is contained inuseUserStatusTooltip.tsx; no shared components were touched.Regression introduced by #40469.
Issue(s)
Steps to test or reproduce
/v1/users.presence) so presence loads slowly.Before: tooltip appears empty, only showing the status after a re-hover.
After: tooltip shows a loading spinner, then live-updates to the status (e.g. "Offline") in place — no re-hover.
Further comments
Summary by CodeRabbit