Skip to content

Commit

Permalink
Merge pull request #17722 from mozilla/FXA-10404
Browse files Browse the repository at this point in the history
bug(settings): Fix bug in handle window focus where apollo cache could be empty
  • Loading branch information
dschom authored Oct 2, 2024
2 parents feb22c5 + 667f16a commit 784980d
Show file tree
Hide file tree
Showing 4 changed files with 246 additions and 7 deletions.
163 changes: 163 additions & 0 deletions packages/functional-tests/tests/settings/multitab.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

import { BrowserContext } from '@playwright/test';
import { expect, Page, test } from '../../lib/fixtures/standard';
import { BaseTarget } from '../../lib/targets/base';

test.describe('severity-1 #smoke', () => {
test('settings opens in multiple tabs with same account', async ({
context,
target,
page,
pages: { signin, settings },
testAccountTracker,
}) => {
const pages = [signin, settings];
const credentials1 = await testAccountTracker.signUp();
await page.goto(target.contentServerUrl);
await signin.fillOutEmailFirstForm(credentials1.email);
await signin.fillOutPasswordForm(credentials1.password);
await expect(settings.settingsHeading).toBeVisible();

// Signin on new tab, and then sign out
await openNewTab(context, target, pages);
await signin.signInButton.click();
await expect(settings.settingsHeading).toBeVisible();
await settings.signOut();
await expect(signin.emailFirstHeading).toBeVisible();

// Switch focus to 1st tab. Page should logout, since
// account on other tab logged out.
await activateTab(page, pages);
await expect(signin.emailFirstHeading).toBeVisible();
});

test('settings opens in multiple tabs with different accounts', async ({
context,
target,
page,
pages: { signin, settings, signup, confirmSignupCode },
testAccountTracker,
}) => {
const pages = [signin, settings, signup, confirmSignupCode];
const credentials = await testAccountTracker.signUp();
await page.goto(target.contentServerUrl);
await signin.fillOutEmailFirstForm(credentials.email);
await signin.fillOutPasswordForm(credentials.password);
await expect(settings.settingsHeading).toBeVisible();

// Signup on new tab
const email2 = credentials.email.replace('@', '.2@');
const password2 = credentials.password;

await openNewTab(context, target, pages);
await signin.useDifferentAccountLink.click();

await signup.fillOutEmailForm(email2);
await signup.fillOutSignupForm(password2, '21');
const code = await target.emailClient.getVerifyShortCode(email2);
await confirmSignupCode.fillOutCodeForm(code);
await expect(settings.settingsHeading).toBeVisible();

// Signout of 2nd tab
await settings.signOut();
await expect(signin.passwordFormHeading).toBeVisible();

// Switch focus back to 1st tab. Page should NOT logout,
// since this account is unaffected
await activateTab(page, pages);
await expect(settings.settingsHeading).toBeVisible();
});

test('settings opens in multiple tabs user clears local storage', async ({
context,
target,
page,
pages: { signin, settings },
testAccountTracker,
}) => {
const pages = [signin, settings];
const credentials = await testAccountTracker.signUp();
await page.goto(target.contentServerUrl);
await signin.fillOutEmailFirstForm(credentials.email);
await signin.fillOutPasswordForm(credentials.password);
await expect(settings.settingsHeading).toBeVisible();

// Open new tab, and clear localstorage
const newPage = await openNewTab(context, target, pages);
await newPage.evaluate(() => {
localStorage.removeItem('__fxa_storage.accounts');
});

// Switch focus to 1st tab. Page should logout, since
// local storage has been wiped clean
await activateTab(page, pages);
await expect(signin.emailFirstHeading).toBeVisible();
});

test('settings opens in multiple tabs and apollo account cache is dropped', async ({
context,
target,
page,
pages: { signin, settings },
testAccountTracker,
}) => {
test.skip(
!/localhost/.test(target.contentServerUrl),
'Access to apollo client is only available during in dev mode, which requires running on localhost.'
);

const credentials = await testAccountTracker.signUp();
await page.goto(target.contentServerUrl);
await signin.fillOutEmailFirstForm(credentials.email);
await signin.fillOutPasswordForm(credentials.password);
await expect(settings.settingsHeading).toBeVisible();

// Signin on new tab
await openNewTab(context, target, [signin, settings]);
await signin.signInButton.click();
await expect(settings.settingsHeading).toBeVisible();

// Mutate apollo cache on page 1, and refocus
await page.evaluate(() => {
// @ts-ignore
const client = window.__APOLLO_CLIENT__;
if (client) {
client.cache.modify({
id: 'ROOT_QUERY',
fields: {
account: () => {
return undefined;
},
},
broadcast: false,
});
}
});

await activateTab(page, [signin, settings]);
await expect(signin.cachedSigninHeading).toBeVisible();
});

async function openNewTab(
context: BrowserContext,
target: BaseTarget,
pages: Array<{ page: Page }>
) {
const page = await context.newPage();
pages.forEach((x) => {
x.page = page;
});
await page.goto(target.contentServerUrl);
return page;
}
async function activateTab(page: Page, pages: Array<{ page: Page }>) {
pages.forEach((x) => {
x.page = page;
});
await page.bringToFront();
await page.dispatchEvent('body', 'focus');
}
});
62 changes: 57 additions & 5 deletions packages/fxa-settings/src/components/Settings/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import PageRecoveryKeyCreate from './PageRecoveryKeyCreate';
import { hardNavigate } from 'fxa-react/lib/utils';
import { SettingsIntegration } from './interfaces';
import { currentAccount } from '../../lib/cache';
import { setCurrentAccount } from '../../lib/storage-utils';
import { hasAccount, setCurrentAccount } from '../../lib/storage-utils';
import GleanMetrics from '../../lib/glean';
import Head from 'fxa-react/components/Head';

Expand All @@ -60,13 +60,65 @@ export const Settings = ({
]);

useEffect(() => {
/**
* If we have an active session, we need to handle the possibility
* that it will reflect the session for the current tab. It's
* important to note that there is also a cache in local storage, and
* as such it is shared between all tabs. So, in the event a user has
* account A signed in on tab 1, and account B signed in on tab 2, local
* storage will reflect the account uid of whichever account was the last
* to be sign in.
*
* By noting the window focus, we actively swap the current account uid
* in local storage so that it matches the apollo cache's account uid,
* which is held in page memory, thereby fixing this discrepancy.
*
* Having multiple things cached in multiple places is never great, so we
* have a ticket in the backlog for cleaning this up, and avoiding this
* hack in the future. See FXA-9875 for more info.
*/
function handleWindowFocus() {
if (account.uid !== currentAccount()?.uid) {
setCurrentAccount(account.uid);
}
if (session.isDestroyed) {
// Try to retrieve the active account uid from the apollo cache.
const accountUidFromApolloCache = (() => {
try {
return account.uid;
} catch {}
return undefined;
})();

// During normal usage, we should not see this. However, if this happens many
// functions on the page would be broken, because it indicates the apollo
// for the active account was cleared. In this case, navigate back to the
// signin page
if (accountUidFromApolloCache === undefined) {
console.warn('Could not access account.uid from apollo cache!');
hardNavigate('/');
return;
}

// If the current account in local storage matches the account in the
// apollo cache, the state is syncrhonized and no action is required.
if (currentAccount()?.uid === accountUidFromApolloCache) {
return;
}

// If there is not a match, and the state exists in local storage, swap
// the active account, so apollo cache and localstorage are in sync.
if (hasAccount(accountUidFromApolloCache)) {
setCurrentAccount(accountUidFromApolloCache);
return;
}

// We have hit an unexpected state. The account UID reflected by the apollo
// cache does not match any known account state in local storage.
// This is could occur if:
// - The same account was signed out on another tab
// - A user localstorage was manually cleared.
//
// Either way, we cannot reliable sync up apollo cache and localstorage, so
// we will direct back to the login page.
console.warn('Could not locate current account in local storage');
hardNavigate('/');
}
window.addEventListener('focus', handleWindowFocus);
return () => window.removeEventListener('focus', handleWindowFocus);
Expand Down
10 changes: 8 additions & 2 deletions packages/fxa-settings/src/lib/gql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ export function createApolloClient(gqlServerUri: string) {
// errorLink handles error responses from the server
const errorLink = onError(errorHandler);

apolloClientInstance = new ApolloClient({
const apolloClientConfig = {
cache,
link: from([
errorLink,
Expand All @@ -158,7 +158,13 @@ export function createApolloClient(gqlServerUri: string) {
httpLink,
]),
typeDefs,
});
connectToDevTools: window.location.host === 'localhost:3030',
devtools: {
enabled: window.location.host === 'localhost:3030',
name: window.location.host === 'localhost:3030' ? 'settings' : '',
},
};
apolloClientInstance = new ApolloClient(apolloClientConfig);

return apolloClientInstance;
}
18 changes: 18 additions & 0 deletions packages/fxa-settings/src/lib/storage-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,29 @@ export function persistAccount(accountData: StoredAccountData) {
storage.set('accounts', accounts);
}

/**
* Checks to see there is an account stored in local storage for the give uid
* @param uid An account id
*/
export function hasAccount(uid: string) {
const storage = localStorage();
let accounts = storage.get('accounts') || {};
return !!accounts[uid];
}

/**
* Sets the current account uid, aka the 'active' account id.
* @param uid
*/
export function setCurrentAccount(uid: string) {
const storage = localStorage();
storage.set('currentAccountUid', uid);
}

/**
* Stores account data in local storage
* @param accountData
*/
export function storeAccountData(accountData: StoredAccountData) {
persistAccount(accountData);
setCurrentAccount(accountData.uid);
Expand Down

0 comments on commit 784980d

Please sign in to comment.