Skip to content

Conversation

@tusharpandey13
Copy link
Contributor

@tusharpandey13 tusharpandey13 commented Oct 29, 2025

Fixes: #2370

beforeSessionSaved was not provided with the updated access token in getAccessToken() and connectAccount(), leading to it operating on stale AT.

Changes

  • auth-client.ts: merged sessionChanges with existing session and THEN passed to finalizeSession (this calls beforeSessionSaved)
  • beforeSessionSaved-token-refresh-flow.test.ts: added flow test cases that ensure that beforeSessionSaved is called with updated tokens after token refresh.

References

Tests

PASSING

@tusharpandey13 tusharpandey13 requested a review from a team as a code owner October 29, 2025 14:47
@tusharpandey13 tusharpandey13 changed the title fix: make sure session is updated with latest accesstoken before calling finalizeSession fix: make sure session is updated with latest accessToken before calling finalizeSession Oct 29, 2025
@tusharpandey13
Copy link
Contributor Author

@claude please review this PR

@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.65%. Comparing base (df1725e) to head (6e41313).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2387      +/-   ##
==========================================
+ Coverage   87.61%   87.65%   +0.04%     
==========================================
  Files          37       37              
  Lines        4086     4084       -2     
  Branches      809      809              
==========================================
  Hits         3580     3580              
+ Misses        503      501       -2     
  Partials        3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2025

Claude finished @tusharpandey13's task —— View job


PR Review

  • Read PR diff
  • Read project documentation
  • Analyze changed files
  • Perform code quality review
  • Perform security review
  • Perform performance review
  • Provide inline feedback
  • Post overall assessment

Overall Assessment

✅ This PR is ready to be merged. The fix correctly addresses a bug where beforeSessionSaved was receiving stale session data instead of the updated session with refreshed tokens. The implementation is clean, the test coverage is comprehensive, and no security or performance concerns were identified.

What was fixed:

  • Root cause: In handleAccessToken and completeConnectAccountCallback, the finalizeSession method was called with the original session object, then the sessionChanges were merged afterward. This meant beforeSessionSaved hook received outdated token data.
  • Solution: Merge sessionChanges into session before calling finalizeSession, ensuring hooks receive the complete updated session with refreshed tokens.
  • Test coverage: Added a comprehensive integration test (164 lines) that validates the fix across HTTP, session hook, and persistence layers.

Changes reviewed:

  • src/server/auth-client.ts: Two locations fixed (lines 936-940 and 1059-1067)
  • src/server/auth-client.test.ts: Comprehensive test added with multi-layer validation

Job Run | Branch: bugfix/beforesessionsaved-behaviour

@tusharpandey13 tusharpandey13 changed the title fix: make sure session is updated with latest accessToken before calling finalizeSession fix: make sure beforeSessionSaved hook gets the updated token after refresh Oct 29, 2025
@tusharpandey13 tusharpandey13 merged commit a483859 into main Oct 30, 2025
9 checks passed
@tusharpandey13 tusharpandey13 deleted the bugfix/beforesessionsaved-behaviour branch October 30, 2025 05:43
This was referenced Oct 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

beforeSessionSaved behaves differently when called after login or after token refresh

4 participants