feat(presence): persist status across reload (3/4) - #436
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
3af7112 to
89d0e90
Compare
89d0e90 to
e217295
Compare
motirebuma
left a comment
There was a problem hiding this comment.
Hey @niamao - this is very well thought out, using preferred status to resolve the "stale cleanup erases your chosen status" dilemma is the right approach.
What's good
-
preferredStatus/preferredStatusMessage- by introducing this layer of indirection we enable the stale cleanup to mark people as offline without destroying their desired status for peers, while updating the Presence model(schema type, common types) appropriately. -
updateHeartbeatrestoration logic - looks good, after being stale, we restore the preferred status upon next heartbeat instead of blindly settingonline. The$setOnInsertfor a new document andexisting.save()upon return are correctly handled, including the edge case whenpreferred === 'offline'falls back to'online'. -
updatePresencemutation - the allowed statuses are explicitly checked (usingALLOWED_STATUSESset), the status message is correctly limited to 200 chars (trimmed), pubsub publishing is there, and thepreferredStatusis set toonlinewhen user setsoffline(avoids looping over the statuses). The resolver itself is correct. -
cleanupStalePresencepreservation - now correctly preserving preferred status and message before marking the presence as offline. -
Client
-
areGraphqlSubscriptionsEnabled()now disables WS on localhost, preventing the noisy reconnect loop on dev start -
Noop subscription link for missing WS transport is implemented correctly
-
The error link only shows the auth modal on mutations, not on background queries - which is correct and prevents the scary-modal-from-nowhere scenario
-
hasActiveSession()correctly checks the store (in addition to JWT) to prevent flashes of logged-in state before the initial query resolves
- Tests - the heartbeat resolver tests auth, success, invalid status, status message truncation, and pubsub - looks good. Auth utilities tests now also test the new
hasActiveSession()resolver.
What to fix
-
Presence.updateHeartbeat = updateHeartbeatImpl.bind(Presence)after model creation - this is probably a hotfix of sorts, to rebind the staticupdateHeartbeatafter the model is created anew. It would be helpful to document why this is done, or perhaps move all of the statics intoPresence, so that upon model binding all statics are binded as well (if this is a one-off, then the same mistake in the future may lead to the same bug if another static is added later and thisbindis not updated). -
hasActiveSession()could potentially show expired tokens as active - if the Zustand store still contains data from the previous session (persisted in localStorage), thenhasActiveSession()will return true even if the JWT is expired. This could lead to a user thinking he is logged in (interactive elements available) until the first GraphQL request fails with UNAUTHENTICATED, which triggers logout and clears the store. Said UX hiccup is noted inuseGuestGuardwith a// TODO, however, it is still correct as the only way to get a valid JWT is through an auth flow that sets it, and this is the intended behavior. -
usePresenceHeartbeat- floating promises - you addedvoid sendHeartbeat()...to avoid the warning, which is correct - however,applyPresenceFromHeartbeat()does a state comparison before attempting to set the presence. If thechat.userStatusis initially undefined (as it is for the first store subscriber), it will always update it to the presence value ('online'). Should the initial value be'online'instead to avoid extra renders? Or is it desired to always update the presence even if it is the same as the current one?
Nits
-
toPublicPresencehelper in the heartbeat resolver is a very nice touch to normalize the_idfromObjectIdtostring. -
Using
import { Observable } from 'rxjs'for the Noop subscription link is correct as well (Apollo hides the constructor in their types).
Final verdict: approved, assuming that the CI passes (once again).
e217295 to
2ab2799
Compare
Keep preferredStatus/message through stale offline cleanup, restore on heartbeat, expose updatePresence, and rehydrate the client from GET_USER and heartbeat responses. Co-authored-by: Cursor <[email protected]>
…ates Re-bind all Presence statics after model resolve so hot reload cannot leave stale methods, and coalesce chat status defaults before applying heartbeat payloads to avoid no-op setState when values already match. Co-authored-by: Cursor <[email protected]>
|
Addressed the review notes:
|
motirebuma
left a comment
There was a problem hiding this comment.
Hey @niamao, I believe all review comments are addressed.
Previously raised items - resolved
Static rebind pattern - FIXED
Extracted a bindPresenceStatics helper that binds all statics (findByUserId, updateHeartbeat) under a single clearly documented comment that explains why it's necessary for hot-reloaded apps. Now, when new statics are added, there's no chance of forgetting to rebind them in hot reload development environment
Heartbeat initial status comparison - FIXED
Now coalescing chat.userStatus || 'online' and chat.userStatusMessage || '', which ensures that an initial undefined store state won't trigger a setUserStatus call on every heartbeat
hasActiveSession() masking expired token - ACKNOWLEDGED
The error link clears the store on UNAUTHENTICATED responses. By default, useGuestGuard will utilize hasActiveSession() to avoid unnecessary auth popups if the session has expired for a short moment between refresh and reauthorization. I think it's a reasonable UX tradeoff and it's now documented.
What's good (not changed from previous review)
-
preferredStatus/preferredStatusMessage persistence through stale cleanup
-
updateHeartbeat restoring preferred status on return
-
updatePresence mutation with validation, truncation and pubsub
-
cleanupStalePresence preserving preferred values
-
Client: WS disabled on local host, noop subscription link, auth guard only on mutations
-
Tests covered heartbeat resolver and auth utilities
Thank you @niamao
Stack
Part 3 of 4 — merge after Part 2 (#435 /
avatar-consistency).profile-bioavatar-consistencypresence-persistapi-wiringAfter Part 2 merges, retarget this PR’s base to
main(or rebase ontomain).Related issue: #362
Summary
preferredStatus/preferredStatusMessagesurvive stale offline cleanupupdatePresencemutation wired; client rehydrates from GET_USER + heartbeatTest plan