fix: profile avatar cache busting and refresh mechanism #6797
Conversation
WalkthroughImage remounting and cache policy were added to Avatar; the ETag observation hook now runs unconditionally and tracks more dependencies; avatarETag updates for the logged-in user are synchronized to the servers database after updateAvatar events. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Avatar as Avatar Component
participant Hook as useAvatarETag Hook
participant DB as App DB
participant ServerDB as Servers DB
participant Connect as connect service
User->>Avatar: update avatar
Avatar->>Hook: effect re-evaluates (deps: username,text,id,rid,type)
Hook->>DB: observeAvatarETag (unconditional)
DB-->>Hook: record observable
Hook-->>Avatar: avatarETag updated
Avatar->>Avatar: Image remount (new key)
Avatar-->>User: render updated avatar
Note over Connect,ServerDB: sync logged-in user's ETag
DB->>Connect: emit updateAvatar event
Connect->>ServerDB: update avatarETag for logged-in user (if username matches)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/views/ProfileView/index.tsx (1)
286-299: Avatar refresh on every focus is intentional and defensive, though conditional refresh could be optimized.The implementation correctly refreshes the avatar whenever ProfileView gains focus by incrementing
avatarRefreshKeyand clearing the image cache. While this happens on every focus (including returns from unrelated screens like ChangePasswordView), the overhead is minimal and the defensive approach ensures the avatar stays fresh.If profiling later reveals this causes issues, consider conditionally refreshing only after returning from ChangeAvatarView by checking navigation history or passing a flag through route params.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
app/containers/Avatar/Avatar.tsx(3 hunks)app/containers/Avatar/useAvatarETag.ts(1 hunks)app/lib/services/connect.ts(1 hunks)app/views/ProfileView/index.tsx(3 hunks)app/views/WorkspaceView/ServerAvatar.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/lib/services/connect.ts (2)
app/lib/store/auxStore.ts (1)
store(6-6)app/actions/login.ts (1)
setUser(91-96)
app/views/WorkspaceView/ServerAvatar.tsx (3)
app/containers/LoginServices/styles.ts (1)
BORDER_RADIUS(6-6)app/theme.tsx (1)
useTheme(29-29)app/containers/CustomIcon/index.tsx (1)
CustomIcon(38-38)
app/containers/Avatar/useAvatarETag.ts (3)
app/definitions/ISubscription.ts (1)
TSubscriptionModel(121-124)app/definitions/IUser.ts (1)
TUserModel(179-179)app/definitions/ILoggedUser.ts (1)
TLoggedUserModel(41-41)
app/containers/Avatar/Avatar.tsx (1)
app/lib/methods/helpers/getAvatarUrl.ts (1)
getAvatarURL(12-66)
🔇 Additional comments (7)
app/lib/services/connect.ts (1)
230-236: LGTM! Redux state synchronization is well-implemented.The addition correctly syncs the logged-in user's
avatarETagin Redux when anupdateAvatarevent occurs, following the same pattern used for user status updates. This ensures that components and hooks dependent on Redux state will immediately detect avatar changes.app/views/WorkspaceView/ServerAvatar.tsx (2)
56-73: Loading state and handlers are well-implemented.The state management logic correctly handles the image loading lifecycle with appropriate initial values and event handlers.
76-103: Skeleton and fallback rendering logic is solid.The conditional rendering appropriately shows a skeleton during loading and a fallback icon on error or when no image is provided. The dark mode handling is also correctly implemented.
app/containers/Avatar/Avatar.tsx (2)
39-86: Avatar URI memoization is correctly implemented.The memoized
avatarUricomputation properly handles static avatars, constructs the URL viagetAvatarURL, and adds cache-busting parameters. The comprehensive dependencies array ensures the URI updates when any relevant prop changes.
88-93: Image key generation ensures proper cache invalidation.The memoized
imageKeycorrectly combinesavatarUri,avatarETag, andavatarto force Image component remounting when avatar data changes, addressing the cache busting requirement.app/containers/Avatar/useAvatarETag.ts (1)
25-65: Hook refactoring correctly addresses the avatar refresh issues.The changes ensure that:
- The initial
avatarETagvalue is set immediately (line 50)- Changes are always observed, not just when
avatarETagis falsy (lines 53-56)- The effect re-runs when any relevant dependency changes (line 65)
These modifications align with the PR objective to fix the
useAvatarETaghook not re-running and to ensure immediate avatar updates.app/views/ProfileView/index.tsx (1)
308-308: Dynamic key correctly triggers avatar container remount.The key based on
avatarRefreshKeyensures the avatar container remounts when returning to the ProfileView, working in conjunction with other avatar refresh mechanisms introduced in this PR.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/views/WorkspaceView/ServerAvatar.tsx (1)
41-55: Cache-busting URI logic looks correct; minor comment wording nitUsing
useMemoto deriveimageUrifromurlandimage, and appending_cbbased onbaseUri.includes('?'), is a solid, deterministic cache-busting approach that avoidsDate.now()and keeps URLs stable across renders. EncodingimagewithencodeURIComponentis appropriate since it may contain/,?, etc.Tiny nit: the comment says “using image value hash” but the code uses the raw (encoded)
imagestring. Either update the comment or actually hash the value if you want that behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/views/WorkspaceView/ServerAvatar.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/WorkspaceView/ServerAvatar.tsx (2)
app/containers/LoginServices/styles.ts (1)
BORDER_RADIUS(6-6)app/theme.tsx (1)
useTheme(29-29)
deepakbhagatiitr
force-pushed
the
fix/profile-avatar-cache-busting
branch
from
2146bbf to
1b64c9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/views/WorkspaceView/ServerAvatar.tsx (3)
61-68: Cache-busting URL construction is correct; confirmimageis always relativeUsing
useMemoto buildimageUriwith a_cbquery param based onimageand choosing the separator frombaseUrishould effectively bust caches when the image value changes. This aligns with the PR’s cache-busting goals. One thing to confirm: this logic assumesimageis a path segment relative tourl(not an already fully-qualified URL). If callers might pass a full URL asimage, this\${url}/${image}`` composition could yield malformed URIs.
70-95: Loading/error reset logic works;previousImageUriRefis likely unnecessary complexityThe
useEffect+useStatecombination correctly resetsloading/errorwhenimageUrichanges, addressing the earlier “stuck error” issue and allowing recovery on new URLs/ETags. Given that React already only re-runs the effect whenimageUrichanges, thepreviousImageUriRefguard is probably redundant and could be removed to simplify the code (the effect would still not loop, since state updates don’t changeimageUri).
107-124: Fallback rendering is robust; consider optional a11y/test hooksThe fallback branch (error or no
imageUri) is themed correctly and usesCustomIconas a clear workspace placeholder. If you want to improve testability and accessibility, you could optionally add anaccessibilityLabeland/ortestIDto the fallback container or icon so tests and screen readers can distinguish “no image / error” from the normal avatar case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
-
app/views/WorkspaceView/ServerAvatar.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/WorkspaceView/ServerAvatar.tsx (2)
app/containers/LoginServices/styles.ts (1)
BORDER_RADIUS(6-6)app/theme.tsx (1)
useTheme(29-29)
🔇 Additional comments (3)
app/views/WorkspaceView/ServerAvatar.tsx (3)
27-47: Styles for fallback, image container, and skeleton overlay look consistentThe layout for
fallbackContainer,imageContainer, andskeletonOverlayis coherent with the existingcontainer/imagestyles and should render the avatar, fallback icon, and skeleton overlay correctly without layout glitches.
96-106: Per-URL loading/error handling looks sound
handleLoadandhandleErrornow simply fliploadingoff and seterroronly on failures, with the effect handling reset on URL changes. Combined withshowFallback = error || !imageUri, this gives a clear and predictable state machine for success, failure, and retry-on-new-URL cases.
126-152: Image + skeleton overlay behavior matches the intended loading lifecycleAlways rendering
<Image>whenimageUriis truthy and overlaying the skeleton whileloadingavoids the previous “permanent skeleton / no image” issue and combines well with the cache-bustedimageUriandkey={${imageUri}-${image}}to ensure remounts on avatar changes. The border and background theming also look correct for both light and dark modes.
|
This change is done by AI and i don't think you have done the testing in iOS and Android.
|
|
It was showing the old avatar due to cache... so i clear cache after avatar change so that it request fresh fetch of avatar from server... Screen.Recording.2025-11-18.at.3.36.06.AM.mp4here's the recording Thanks |
- Fixed useAvatarETag hook to always observe avatarETag changes instead of only when empty - Added key prop to Avatar Image component with avatarETag for cache-busting - Updated updateAvatar stream handler to also update logged-in user's avatarETag in servers database - Avatar now updates immediately when navigating back from ChangeAvatarView without requiring manual refresh
deepakbhagatiitr
force-pushed
the
fix/profile-avatar-cache-busting
branch
from
1b64c9f to
9669b4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/lib/services/connect.ts (1)
233-248: Servers DB avatarETag sync for logged-in user looks correctThe additional block correctly:
- Checks that the
updateAvatarevent is for the logged-in user vialoggedUser && loggedUser.username === username.- Uses
database.serversand theuserscollection to updateavatarETaginside a WatermelonDB write, mirroring the existing pattern fordatabase.active.- Gracefully no-ops if the servers user record is missing.
This aligns with the updated
useAvatarETaglogic that reads fromdatabase.serversfor the logged-in user and should fix the stale-avatar bug without introducing new correctness issues. You might optionally add debug logging in thecatchto aid future diagnostics, but it’s not required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/containers/Avatar/Avatar.tsx(1 hunks)app/containers/Avatar/useAvatarETag.ts(1 hunks)app/lib/services/connect.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/containers/Avatar/Avatar.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/lib/services/connect.ts (4)
app/lib/store/auxStore.ts (1)
store(6-6)app/sagas/rooms.js (2)
serversDB(15-15)serversDB(32-32)app/sagas/login.js (7)
serversDB(62-62)serversDB(93-93)serversDB(236-236)serversDB(310-310)serversDB(339-339)serversDB(377-377)u(238-251)app/sagas/encryption.js (1)
serversDB(25-25)
- Added isSubscribed flag to prevent subscriptions after cleanup - Made subscription explicitly optional (Subscription | undefined) - Check isSubscribed before creating subscription to avoid memory leaks - Inlined isDirect() check to fix linter warning
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/containers/Avatar/useAvatarETag.ts (1)
32-46: Consider adding validation or dev-mode logging.The record resolution logic correctly handles three distinct contexts. However, when
username === text(line 33), the code callsfind(id)without validating thatidis defined and non-empty. Ifidis invalid, thefind()will throw and be silently caught. While this doesn't break functionality, it could mask issues during development.Similarly, the empty
catchblock (lines 44-46) suppresses all errors, not just "record not found" cases. Consider logging errors in development mode to aid debugging.You could add a guard or dev-mode logging like:
let record; try { if (username === text) { + if (!id) throw new Error('Missing id for logged-in user'); const serversDB = database.servers; const userCollections = serversDB.get('users'); const user = await userCollections.find(id); record = user; } else if (type === 'd') { const [user] = await usersCollection.query(Q.where('username', text)).fetch(); record = user; } else if (rid) { record = await subsCollection.find(rid); } -} catch { +} catch (error) { + if (__DEV__) { + console.warn('[useAvatarETag] Failed to find record:', error); + } // Record not found }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/containers/Avatar/useAvatarETag.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/containers/Avatar/useAvatarETag.ts (4)
app/lib/database/model/Subscription.js (1)
Subscription(8-225)app/definitions/ISubscription.ts (1)
TSubscriptionModel(121-124)app/definitions/IUser.ts (1)
TUserModel(179-179)app/definitions/ILoggedUser.ts (1)
TLoggedUserModel(41-41)
🔇 Additional comments (2)
app/containers/Avatar/useAvatarETag.ts (2)
24-25: Subscription leak concern properly addressed.The addition of the
isSubscribedflag and the guard at line 48 (if (record && isSubscribed)) correctly prevents subscription creation after the effect has been cleaned up. The cleanup function now safely setsisSubscribed = falseand unsubscribes if present. This matches the recommendations from the previous review and resolves the async subscription leak risk.Also applies to: 48-48, 56-61
62-62: Dependencies are comprehensive and correct.The effect dependencies
[username, text, id, rid, type]correctly capture all parameters that influence which record is observed. This ensures the subscription re-establishes whenever any identifying property changes, keeping theavatarETagin sync with the correct source.
2 participants
I fixed an issue where updating a profile avatar wasn’t showing the new image immediately after going back to the ProfileView. The old cached image would keep showing until the user saved again or manually refreshed.
Issue(s)
Fixes bug: #6794
Problem
When a user updates their avatar and goes back to the ProfileView, the old avatar still appears. This happens because:
avatarETagwasn't updating correctly across databasesuseAvatarETaghook didn’t re-run when the value changedWhat I Did
I added a few coordinated fixes to make sure the avatar updates instantly:
useAvatarETag
Avatar component
_cb=${avatarETag}query param to the image URLkeybased onavatarETagso the image remounts when updateduseMemoto generate the final avatar URLProfileView
connect.ts
avatarETagin ReduxSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.