[GIT-260] fix(user): Profile cover image uses stale image after update#9285
[GIT-260] fix(user): Profile cover image uses stale image after update#9285codingwolf-at wants to merge 1 commit into
Conversation
Updated the `updateCurrentUser` method in `UserStore` to clone the current user data before making updates, ensuring that the original data remains unchanged during the update process. Additionally, added logic to update the local state with the new user data after a successful update. fix(cover-image): return absolute URLs for cover images Modified the `handleCoverImageChange` function to return absolute URLs for cover images, ensuring compatibility with the expected format. This change includes handling both uploaded images and new images, providing a consistent return structure.
|
|
codingwolf-at
changed the title
📝 WalkthroughWalkthroughTwo targeted bug fixes: ChangesUser Store and Cover Image Data Handling Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/core/store/user/index.ts (1)
174-180:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap rollback logic in
runInActionfor MobX reactivity.The rollback at lines 176-179 mutates the observable
this.databut is not wrapped inrunInAction. This is inconsistent with the success path (lines 165-172) and MobX best practices. MobX may not properly track these mutations, leading to missed re-renders or reactivity issues.🔧 Proposed fix
} catch (error) { - if (currentUserData) { - Object.keys(currentUserData).forEach((key: string) => { - const userKey: keyof IUser = key as keyof IUser; - if (this.data) set(this.data, userKey, currentUserData[userKey]); - }); - } runInAction(() => { + if (currentUserData) { + Object.keys(currentUserData).forEach((key: string) => { + const userKey: keyof IUser = key as keyof IUser; + if (this.data) set(this.data, userKey, currentUserData[userKey]); + }); + } this.error = { status: "user-update-error", message: "Failed to update current user", }; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/core/store/user/index.ts` around lines 174 - 180, The rollback logic in the catch block mutates the observable this.data property without wrapping it in runInAction, which is inconsistent with the success path and violates MobX best practices. Wrap the entire forEach loop that restores currentUserData keys back to this.data (where set is called for each userKey) inside a runInAction function call to ensure MobX properly tracks these mutations and maintains reactivity.
🧹 Nitpick comments (1)
apps/web/core/store/user/index.ts (1)
158-163: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffRemove redundant optimistic update or wrap in
runInAction.Lines 158-163 apply an optimistic update using the request payload before the API call, but lines 165-172 immediately overwrite those fields with the API response. This double-update is redundant.
According to the stack context and AI summary, the intent was to sync from the API response instead of applying the request payload before the call. The optimistic update should likely be removed entirely.
If immediate UI feedback is required and the optimistic update is intentional, it must be wrapped in
runInAction(() => { ... })to ensure MobX tracks the state changes properly, consistent with the pattern used at lines 165-172 and elsewhere in this file (e.g., lines 125-129).♻️ Recommended fix: remove optimistic update
updateCurrentUser = async (data: Partial<IUser>): Promise<IUser> => { const currentUserData = cloneDeep(this.data); try { - if (currentUserData) { - Object.keys(data).forEach((key: string) => { - const userKey: keyof IUser = key as keyof IUser; - if (this.data) set(this.data, userKey, data[userKey]); - }); - } const user = await this.userService.updateUser(data); if (user && this.data) { runInAction(() => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/core/store/user/index.ts` around lines 158 - 163, The forEach loop that iterates through data keys and updates this.data using the set function is redundant because the API response immediately overwrites those same fields in the subsequent block. Either remove this entire optimistic update block entirely, or if immediate UI feedback is required, wrap the forEach loop and its contents in runInAction(() => { ... }) to ensure MobX properly tracks the state changes, consistent with the pattern used elsewhere in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/web/core/store/user/index.ts`:
- Around line 174-180: The rollback logic in the catch block mutates the
observable this.data property without wrapping it in runInAction, which is
inconsistent with the success path and violates MobX best practices. Wrap the
entire forEach loop that restores currentUserData keys back to this.data (where
set is called for each userKey) inside a runInAction function call to ensure
MobX properly tracks these mutations and maintains reactivity.
---
Nitpick comments:
In `@apps/web/core/store/user/index.ts`:
- Around line 158-163: The forEach loop that iterates through data keys and
updates this.data using the set function is redundant because the API response
immediately overwrites those same fields in the subsequent block. Either remove
this entire optimistic update block entirely, or if immediate UI feedback is
required, wrap the forEach loop and its contents in runInAction(() => { ... })
to ensure MobX properly tracks the state changes, consistent with the pattern
used elsewhere in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 229c73f3-5559-4efb-9dff-cf3f9816e8eb
📒 Files selected for processing (2)
apps/web/core/store/user/index.tsapps/web/helpers/cover-image.helper.ts
2 participants
Description
Updated the
updateCurrentUsermethod inUserStoreto clone the current user data before making updates, ensuring that the original data remains unchanged during the update process. Additionally, added logic to update the local state with the new user data after a successful update.fix(cover-image): return absolute URLs for cover images
Modified the
handleCoverImageChangefunction to return absolute URLs for cover images, ensuring compatibility with the expected format. This change includes handling both uploaded images and new images, providing a consistent return structure.Type of Change
Screenshots and Media (if applicable)
Before
Before.mov
After
After.mov
Test Scenarios
References
NA
Summary by CodeRabbit
Bug Fixes