fix(avatar): consistent avataaars rendering (2/4) - #435
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.
Hi @niamao - nice cleanup! This simplifies a lot of ad hoc avatar handling code into a consistent pattern.
What's good
-
Avatar component refactor - accepting
srcasstring | Record | nulland resolving it viaparseAvatarToUrlessentially makes all of its usesites pass the raw avatar value, which is nice and clean. Extracting AvatarMedia as a subcomponent withkey={resolvedSrc}correctly resets the loading/error state when the avatar changes. Good job! -
useSyncCurrentUserProfileis basically a hack to make sure that the nav bar avatar doesn't flicker when we log in - historically, the login response didn't include the avatar, so until we fetched it separately (in the profile page) we'd show a default avatar in the nav bar, but the profile page would show the real one. This hook basically syncs the avatar (as well as profile name and bio, email, and contributorBadge) from the server query to the Zustand store, which is exactly what we need. UsingJSON.stringifyto check if it changed is arguably a hack, but it's totally acceptable for a simple object like avatar. -
The presence restoration logic correctly favors
preferredStatusoverstatusand translatesoffline→onlinefor the current user (since they're obviously not actually offline if they're viewing the app). The test covers all four cases (default, set explicitly, offline → online, preferredStatus override). -
Updating all usesites of
avatarfromstringtostring | Recordis thorough - I don't see any lingeringtypeof avatar === 'string' ? avatar : undefinedcoercions in activity item, buddy list, chat types, component props, or post chat.
Items to address
-
The
useSyncCurrentUserProfilehook will run on every dashboard page (since it's called in theDashboardClientlayout), which means that every time you visit a dashboard page, it'll run a GET_USER query to update the profile. Since it's usingcache-and-network, the first time it'll use the cached value, but subsequent times it'll have to make a network request anyway. Not a huge problem, but it's not ideal - ideally we'd usecache-firstif we don't care about freshness, or implement some sort of debounce with a longer cache TTL. -
The
BuddyItemListavatar coercion is a type assertion, not a runtime check - ifitem.user.avataris something unexpected (a number, say), it would silently coerce tonull. Probably not a huge problem, but worth noting.
Nits
The AvatarMedia function component inside Avatar could be a named export if we wanted to test it, but it's probably fine as is since it's pretty simple and testing it would require testing Avatar anyway. ParseAvatarToUrl isn't in the diffs, but I assume that's a real function in @/lib/avatar that takes the qualities object and turns it into an avataaars.io URL.
Thank you @niamao
Parse avatar qualities vs URL in Avatar, pass raw avatar values through chat/comments/activity, sync login/nav avatar from GET_USER, and keep store avatar updates typed for objects. Co-authored-by: Cursor <[email protected]>
e217295 to
2ab2799
Compare
Use cache-first for useSyncCurrentUserProfile to avoid re-fetching GET_USER on every dashboard navigation, and coerce BuddyItemList avatars at runtime instead of type assertions. Cover coerceAvatarValue and resolveOwnStatus in tests. Co-authored-by: Cursor <[email protected]>
|
Addressed the review suggestions:
Also tightened presence tests: |
motirebuma
left a comment
There was a problem hiding this comment.
hi @niamao - all review items addressed cleanly.
Previously raised items - now resolved
-
useSyncCurrentUserProfilefetch policy - FIXED. Changed fromcache-and-networktocache-first. The hook now shares Apollo cache between dashboard mounts rather than fetching fresh profile on every page navigation. Nice comment explains the thinking. -
BuddyItemListavatar coercion - FIXED. Now usescoerceAvatarValue(item.user.avatar)which is a proper run-time check that returnsnullfor non-expected types (numbers, arrays, booleans) rather than type assertion. AddedcoerceAvatarValueunit test which covers strings, objects, null, undefined, numbers, arrays and booleans. -
resolveOwnStatusexported for testing - the presence resolution logic is now directly testable. Test covers default, explicit, offline->online and preferredStatus override cases.
What's good (unchanged from prior review)
-
Avatarcomponent refactor withparseAvatarToUrland keyedAvatarMediasubcomponent -
useSyncCurrentUserProfilesyncing avatar/name/bio/presence from GET_USER to Zustand -
Type unification across all avatar call sites (activity, buddy list, chat, components)
Verdict: approved - ready to merge
Thank you @niamao
Stack
Part 2 of 4 — merge after Part 1 (#434 /
profile-bio).profile-bioavatar-consistencypresence-persistapi-wiringAfter Part 1 merges to
main, retarget this PR’s base tomain(or rebase ontomain).Related issue: #362
Summary
Avatarparses qualities objects vs URL stringsuseSyncCurrentUserProfilekeeps nav/account avatar in sync with GET_USERTest plan