Repository navigation
Keep copilot sessions signed in across a transition link - #70
Closed
adhorodyski wants to merge 1 commit into
Closed
adhorodyski wants to merge 1 commit into
adhorodyski wants to merge 1 commit into
Conversation
isLoggingInAsNewUser compares the address a transition link was generated for against the session email. A copilot session stores the address of the account being worked on rather than the copilot's own address, so neither comparison matches for a link generated for the copilot. The predicate reported a different user signing in, and both callers on the transition path signed the live copilot session out mid-switch. Pass the copilot-session fact into the predicate and answer that no new user is signing in when the session is a live copilot session. LogOutPreviousUserPage and AuthScreensInitHandler forward isDelegateSession(session), which both already hold, so the decision stays in the predicate. The exitTo effect in LogOutPreviousUserPage now runs for a copilot, which it previously skipped while the copilot counted as a new user. Tests record what each transition link shape returns, and cover both callers against the real predicate.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
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
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.
Explanation of Change
isLoggingInAsNewUserdecides whether a transition link carries a different person than the one already signed in. It compares theemailanddelegatorEmailparams of the link against the session email. Both callers of that decision,LogOutPreviousUserPageandAuthScreensInitHandler, sign the current session out when it returns true.A copilot session is keyed to the account being worked on.
session.emailholds the delegator's address, not the copilot's own. So for a transition link generated for the copilot, neither param can equal the session email, the predicate returns true, and the transition path signs a working copilot session out in the middle of the switch. Because both callers run the same check on boot, the sign-out arrives as a pair a few milliseconds apart.The assumption behind this change is that a copilot who opens a link generated for a specific account expects to end up working in that account.
This change passes the copilot-session fact into
isLoggingInAsNewUserand answers that no new user is signing in when the live session is a copilot session.LogOutPreviousUserPageandAuthScreensInitHandlerforwardisDelegateSession(session), which readssession.authTokenTypeand which both callers already hold, so the decision stays inside the predicate and the callers only supply a session they have already read. TheemailanddelegatorEmailcomparisons are untouched for every other session, so an ordinary switch between two people still signs the previous user out.One side effect worth calling out. The second effect in
LogOutPreviousUserPageskips theexitTonavigation while the transition counts as a new user. A copilot now reaches that navigation, which is what the same predicate already decides for any returning user.The unit tests record what each transition link shape returns today. Reading them against the reported symptom shows the trigger is the session shape, not the link: a link naming the copilot with no
delegatorEmail, and a link naming the copilot in both params, both return true against a copilot session, while the only link shape that returns false is one whosedelegatorEmailequals the session email. Either way the session email is the odd one out, which is why the fix reads the session rather than the link.One follow-up is not in scope here.
DelegatorConnectGaterunsconnectonly when the link carries adelegatorEmail, and the link shape above carries none, so with the sign-out suppressed the page still signs the client into the account named byemail. Making a copilot land in the target account for that shape needs a change to that connect step.Fixed Issues
No issue is linked. This change comes from reading the transition path against a reported copilot symptom, and there is no issue to reference here.
$
PROPOSAL:
Tests
Automated coverage, which is the primary evidence for this change:
npx jest tests/unit/SessionUtilsTest.ts --maxWorkers=2. The newisLoggingInAsNewUserblock records each transition link shape against each session email, including the copilot shape, and asserts the copilot link is read as the same person.npx jest tests/unit/LogOutPreviousUserPageTest.tsx --maxWorkers=2. The first case seeds a copilot session and a link generated for the copilot, and assertssignOutAndRedirectToSignInis never called. The second case seeds an ordinary session the link does not name, and asserts the sign-out still happens withLOGIN_AS_NEW_USER.npx jest tests/ui/AuthScreensInitHandlerTest.tsx --maxWorkers=2. The new case points the mocked predicate at the real implementation, so the second caller is checked against real behaviour rather than a stub, for a copilot session and for an ordinary one.if (isCopilotSession)guard fromisLoggingInAsNewUserand the three copilot cases fail while all 57 others pass. Make that guard unconditional and the six genuine new-user cases fail instead, which is the check that the exemption stayed narrow.Manual check, which needs an OldDot session because NewDot does not generate the transition link itself:
Offline tests
This change reads a value already held in the session and adds no network call, so offline behaviour is unchanged. There is no offline state that reaches a transition link, because a transition link arrives over a live OldDot session.
QA Steps
Same as the Tests section. This path needs an OldDot session to produce the transition link, so it is not repeatable on staging alone, and there is no UI change to verify visually.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionLogOutPreviousUserPageTestand the ordinary-account row inAuthScreensInitHandlerTestboth assert that sign-out still happens.isLoggingInAsNewUsercarries a comment, because a session keyed to a different address than the link is not obvious from the code.isDelegateSession, which already answers "is this a copilot session" elsewhere in the app.Avatar, I verified the components usingAvatarare working as expected)isLoggingInAsNewUserhas exactly two callers, and both are in this diff. The existing suites for both are green.StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)/transitionis that page. The new cases cover it with a copilot session and with an ordinary session, and a signed-out session reachesLogInWithShortLivedAuthTokenPagerather than this page, so it is unaffected.Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
No UI change, so there is nothing to capture.
Android: mWeb Chrome
No UI change, so there is nothing to capture.
iOS: Native
No UI change, so there is nothing to capture.
iOS: mWeb Safari
No UI change, so there is nothing to capture.
MacOS: Chrome / Safari
No UI change, so there is nothing to capture.