getUser is also refresh the user reference in the currentSession#1315
Open
stefanschaller wants to merge 3 commits intosupabase:mainfrom
Open
getUser is also refresh the user reference in the currentSession#1315stefanschaller wants to merge 3 commits intosupabase:mainfrom
stefanschaller wants to merge 3 commits intosupabase:mainfrom
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Comment |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/gotrue/lib/src/gotrue_client.dart`:
- Around line 720-728: The code emits userUpdated even when there is no local
session because currentSession?.copyWith(...) returns null; fix by
early-returning when _currentSession is null: after constructing userResponse,
if _currentSession is null return userResponse (so no copyWith or
notifyAllSubscribers is called); otherwise update _currentSession with
_currentSession.copyWith(user: userResponse.user) and call
notifyAllSubscribers(AuthChangeEvent.userUpdated). Ensure you reference
userResponse, _currentSession, copyWith, notifyAllSubscribers, and
AuthChangeEvent.userUpdated.
ℹ️ Review info
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/gotrue/lib/src/gotrue_client.dart
Comment on lines
720
to
728
| final userResponse = UserResponse.fromJson(response); | ||
|
|
||
| // np need to update the local user when the user is the same | ||
| if (userResponse.user == _currentSession?.user) return userResponse; | ||
|
|
||
| _currentSession = currentSession?.copyWith(user: userResponse.user); | ||
| notifyAllSubscribers(AuthChangeEvent.userUpdated); | ||
|
|
||
| return userResponse; |
Contributor
There was a problem hiding this comment.
Bug: userUpdated event emitted incorrectly when no session exists.
When getUser(jwt) is called with a custom JWT but no local session (_currentSession is null), the code reaches line 725 where currentSession?.copyWith(...) evaluates to null, and then emits a userUpdated event with a null session. This is incorrect behavior—no state actually changed.
Guard against this case:
Proposed fix
final userResponse = UserResponse.fromJson(response);
- // np need to update the local user when the user is the same
- if (userResponse.user == _currentSession?.user) return userResponse;
+ // No need to update the local user when there's no session or user is the same
+ if (_currentSession == null || userResponse.user == _currentSession?.user) {
+ return userResponse;
+ }
_currentSession = currentSession?.copyWith(user: userResponse.user);
notifyAllSubscribers(AuthChangeEvent.userUpdated);
return userResponse;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/gotrue/lib/src/gotrue_client.dart` around lines 720 - 728, The code
emits userUpdated even when there is no local session because
currentSession?.copyWith(...) returns null; fix by early-returning when
_currentSession is null: after constructing userResponse, if _currentSession is
null return userResponse (so no copyWith or notifyAllSubscribers is called);
otherwise update _currentSession with _currentSession.copyWith(user:
userResponse.user) and call notifyAllSubscribers(AuthChangeEvent.userUpdated).
Ensure you reference userResponse, _currentSession, copyWith,
notifyAllSubscribers, and AuthChangeEvent.userUpdated.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/gotrue/lib/src/gotrue_client.dart (1)
720-728:⚠️ Potential issue | 🟠 MajorGuard against emitting
userUpdatedwhen no local session exists.If
getUser(jwt)is called while_currentSessionis null,currentSession?.copyWith(...)yields null and you still emituserUpdatedwith no session. This is incorrect and can mislead subscribers. Please return early when_currentSessionis null.🛠️ Proposed fix
- if (userResponse.user == null) return userResponse; - - // np need to update the local user when the user is the same - if (userResponse.user == _currentSession?.user) return userResponse; + if (userResponse.user == null) return userResponse; + if (_currentSession == null) return userResponse; + + // no need to update the local user when the user is the same + if (userResponse.user == _currentSession?.user) return userResponse; _currentSession = currentSession?.copyWith(user: userResponse.user); notifyAllSubscribers(AuthChangeEvent.userUpdated);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gotrue/lib/src/gotrue_client.dart` around lines 720 - 728, The code updates _currentSession and emits notifyAllSubscribers(AuthChangeEvent.userUpdated) even when there is no local session; change the logic in the block after UserResponse.fromJson(response) to return early if _currentSession is null (i.e., check _currentSession == null and return userResponse) before calling currentSession?.copyWith(...) and notifyAllSubscribers so you never emit a userUpdated event without an existing local session.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/gotrue/lib/src/gotrue_client.dart`:
- Around line 720-728: The code updates _currentSession and emits
notifyAllSubscribers(AuthChangeEvent.userUpdated) even when there is no local
session; change the logic in the block after UserResponse.fromJson(response) to
return early if _currentSession is null (i.e., check _currentSession == null and
return userResponse) before calling currentSession?.copyWith(...) and
notifyAllSubscribers so you never emit a userUpdated event without an existing
local session.
ℹ️ Review info
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/gotrue/lib/src/gotrue_client.dart
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Bug fix/feature -> Depending on who you ask 😅
What is the current behavior?
The
getUsercall only gets the user without doing anything with the userWhat is the new behavior?
The old user object is also updated in the
currentSessionAdditional context
We update the user object in an edgefunction and want to refresh the user afterwards.