feat(appLock): add logout flow to app lock modal [WPB-23186]#20391
feat(appLock): add logout flow to app lock modal [WPB-23186]#20391
Conversation
|
🔗 Download Full Report Artifact 🧪 Playwright Test Summary
specs/AccountSettingsSpecs/accountSettings.spec.ts (❌ 0 failed,
|
V-Gira
force-pushed
the
v/applock-sign-in-WPB-23186
branch
from
8199f56 to
1d6d8f2
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements a new logout flow for the app lock feature, replacing the previous client deletion flow with a standard logout that preserves the client on the backend.
Changes:
- Users who forget their applock passcode can now log out using the standard flow instead of deleting their client
- The applock feature is disabled (if not enforced) when logging out via the forgot passcode flow
- UI has been refactored to use @wireapp/react-ui-kit components (Button, Input, Checkbox, Link)
- Removed the client deletion flow (WIPE_CONFIRM and WIPE_PASSWORD states)
- Added new LOGOUT state with an option to clear local data
- Updated localization strings to reflect the new flow
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/webapp/src/types/i18n.d.ts | Updated type definitions for new and modified i18n strings |
| apps/webapp/src/i18n/en-US.json | Added/updated localization strings for new logout flow, removed old wipe-related strings |
| apps/webapp/src/script/repositories/user/AppLockRepository.ts | Extracted disableFeature method for reuse in logout flow |
| apps/webapp/src/script/page/AppLock/Applock.styles.ts | New styles file for AppLock component using CSS-in-JS |
| apps/webapp/src/script/page/AppLock/AppLock.tsx | Major refactor: removed WIPE states, added LOGOUT state, migrated to UI-Kit components, implemented new logout flow |
| <Link | ||
| variant={LinkVariant.PRIMARY} | ||
| type="button" | ||
| css={applockStyles.linkStyle} | ||
| className="button-reset-default block modal__cta" | ||
| data-uie-name="go-forgot-passphrase" | ||
| onClick={onClickForgot} | ||
| > | ||
| {t('modalAppLockLockedForgotCTA')} | ||
| </button> | ||
|
|
||
| <div className="modal__buttons"> | ||
| <button | ||
| type="submit" | ||
| className="modal__button modal__button--primary modal__button--full" | ||
| data-uie-name="do-action" | ||
| > | ||
| {t('modalAppLockLockedUnlockButton')} | ||
| </button> | ||
| </div> | ||
| </Link> |
There was a problem hiding this comment.
The Link component is used with type="button" which creates a semantic mismatch. A Link component should be used for navigation (anchor tags), not for button actions like onClick handlers. This component triggers onClickForgot which changes state rather than navigating.
Consider using a Button component with ButtonVariant.TERTIARY or similar styling instead of Link for this action, or use a native <button> element styled as a link. This will ensure proper semantics for screen readers and keyboard navigation.
| <p>{t('modalAppLockForgotMessage')}</p> | ||
| <br /> |
There was a problem hiding this comment.
Using <br /> for spacing (line 483) is semantically incorrect and bad for accessibility. The <br /> element should only be used for line breaks within content (like in addresses or poems), not for spacing between paragraphs.
Instead, use proper paragraph elements with CSS margins for spacing:
<p css={{marginBottom: '16px'}}>{t('modalAppLockForgotMessage')}</p>
<p>{t('modalAppLockForgotSecondMessage')}</p>Or define the spacing in the styles file and apply it to the paragraph elements.
| <p>{t('modalAppLockForgotMessage')}</p> | |
| <br /> | |
| <p css={{marginBottom: '16px'}}>{t('modalAppLockForgotMessage')}</p> |
| if (isTemporaryClient) { | ||
| await clientRepository.logoutClient(); | ||
| } else { | ||
| setState(APPLOCK_STATE.LOGOUT); |
There was a problem hiding this comment.
Missing error handling in async function onClickLogout. Line 237 calls await clientRepository.logoutClient() but there's no try-catch block to handle potential errors.
If logoutClient() throws an error, it will be an unhandled promise rejection which could leave the UI in an inconsistent state (modal still showing, user confused about their logged-in status).
Add proper error handling:
const onClickLogout = async () => {
try {
if (isTemporaryClient) {
await clientRepository.logoutClient();
} else {
setState(APPLOCK_STATE.LOGOUT);
}
} catch (error) {
// Handle error appropriately - show error message or log
logger.error('Failed to logout client', error);
// Optionally show user-facing error
}
};| if (isTemporaryClient) { | |
| await clientRepository.logoutClient(); | |
| } else { | |
| setState(APPLOCK_STATE.LOGOUT); | |
| try { | |
| if (isTemporaryClient) { | |
| await clientRepository.logoutClient(); | |
| } else { | |
| setState(APPLOCK_STATE.LOGOUT); | |
| } | |
| } catch (error) { | |
| // [Important] Handle logout errors to avoid unhandled promise rejections and inconsistent UI state. | |
| // Consider surfacing a user-visible error message if appropriate for this UI. | |
| // eslint-disable-next-line no-console | |
| console.error('Failed to logout client', error); |
| {state === APPLOCK_STATE.LOGOUT && ( | ||
| <Fragment> | ||
| <div className="modal__text" data-uie-name="label-applock-wipe-confirm-text"> | ||
| {t('modalAppLockWipeConfirmMessage')} | ||
| </div> | ||
|
|
||
| <div className="modal__buttons"> | ||
| <button onClick={onGoBack} className="modal__button modal__button--secondary" data-uie-name="do-go-back"> | ||
| {t('modalAppLockWipeConfirmGoBackButton')} | ||
| </button> | ||
|
|
||
| <button | ||
| onClick={onClickWipeConfirm} | ||
| className="modal__button modal__button--primary modal__button--alert" | ||
| data-uie-name="do-action" | ||
| > | ||
| {t('modalAppLockWipeConfirmConfirmButton')} | ||
| </button> | ||
| </div> | ||
| </Fragment> | ||
| )} | ||
|
|
||
| {state === APPLOCK_STATE.WIPE_PASSWORD && ( | ||
| <form onSubmit={onWipeDatabase}> | ||
| <input | ||
| aria-label={t('modalAppLockWipePasswordTitle', {brandName: Config.getConfig().BRAND_NAME})} | ||
| autoFocus | ||
| className="modal__input" | ||
| type="password" | ||
| name="password" | ||
| autoComplete="new-password" | ||
| placeholder={t('modalAppLockWipePasswordPlaceholder')} | ||
| onKeyDown={clearWipeError} | ||
| data-uie-name="input-applock-wipe" | ||
| /> | ||
|
|
||
| <p className="modal__input__error" style={{height: 20}} data-uie-name="label-applock-wipe-error"> | ||
| {wipeError} | ||
| </p> | ||
|
|
||
| <div className="modal__buttons"> | ||
| <button | ||
| type="button" | ||
| <Checkbox | ||
| checked={clearData} | ||
| data-uie-name="modal-option-checkbox" | ||
| id="clear-data-checkbox" | ||
| onChange={(event: React.ChangeEvent<HTMLInputElement>) => { | ||
| const value = event.target.checked; | ||
| setClearData(value); | ||
| }} | ||
| > | ||
| <CheckboxLabel className="label-xs" htmlFor="clear-data-checkbox"> | ||
| {t('modalAccountLogoutOption')} | ||
| </CheckboxLabel> | ||
| </Checkbox> | ||
|
|
||
| <div css={applockStyles.buttonGroupStyle}> | ||
| <Button | ||
| css={applockStyles.buttonStyle} | ||
| variant={ButtonVariant.SECONDARY} | ||
| onClick={onGoBack} | ||
| className="modal__button modal__button--secondary" | ||
| data-uie-name="do-go-back" | ||
| > | ||
| {t('modalAppLockWipePasswordGoBackButton')} | ||
| </button> | ||
| {t('modalAppLockLogoutCancelButton')} | ||
| </Button> | ||
|
|
||
| <button | ||
| type="submit" | ||
| className="modal__button modal__button--primary modal__button--alert" | ||
| data-uie-name="do-action" | ||
| > | ||
| {t('modalAppLockWipePasswordConfirmButton')} | ||
| </button> | ||
| <Button css={applockStyles.buttonStyle} onClick={() => onLogout(clearData)} data-uie-name="do-action"> | ||
| {t('modalAccountLogoutAction')} | ||
| </Button> | ||
| </div> | ||
| </form> | ||
| </Fragment> | ||
| )} |
There was a problem hiding this comment.
Missing test coverage for new APPLOCK_STATE.LOGOUT state. The new logout flow introduced in this PR (lines 46, 259, 501-532) is a significant change to the applock behavior, but there are no corresponding tests in AppLock.test.tsx.
Add tests to cover:
- Transitioning to LOGOUT state when "Forgot passcode?" → "Log Out" is clicked
- The checkbox for clearing data functionality
- The "Cancel" button returning to LOCKED state
- The "Log Out" button triggering logout with correct clearData value
- Temporary client handling (direct logout without showing LOGOUT state)
This is important because the logout flow affects user data and authentication state.
| 'isAppLockEnforced', | ||
| ]); | ||
|
|
||
| const isTemporaryClient = clientState.currentClient?.isTemporary(); |
There was a problem hiding this comment.
Potential null pointer exception: clientState.currentClient?.isTemporary() on line 87 can return undefined if currentClient is not set. The code checks isTemporaryClient on line 236 without null handling.
If clientState.currentClient is undefined, isTemporaryClient will be undefined, which will cause the condition if (isTemporaryClient) to evaluate to false. However, this creates ambiguity - undefined being treated as false may mask cases where the client state hasn't been initialized properly.
Consider adding explicit null checks or default values, such as:
const isTemporaryClient = clientState.currentClient?.isTemporary() ?? false;
| const isTemporaryClient = clientState.currentClient?.isTemporary(); | |
| const isTemporaryClient = clientState.currentClient?.isTemporary() ?? false; |
| <p | ||
| className="modal__text" | ||
| dangerouslySetInnerHTML={{__html: t('modalAppLockSetupMessage', undefined, {br: '<br><br>'})}} | ||
| data-uie-name="label-applock-set-text" | ||
| /> |
There was a problem hiding this comment.
Unsafe use of dangerouslySetInnerHTML without sanitization on line 301. While this uses a localization string (t('modalAppLockSetupMessage')), the {br: '<br><br>'} substitution injects raw HTML without sanitization.
If the localization strings are controlled and trusted, document this with a comment explaining:
- That the content is from trusted localization files
- That only
<br>tags are allowed in the substitution
However, consider using safer alternatives like:
- React components for line breaks
- CSS for spacing
- A safe HTML sanitizer like DOMPurify if HTML is necessary
This same pattern appears throughout the component (lines 301, 399) and should be addressed consistently.
| data-uie-name="input-applock-unlock" | ||
| autoComplete="new-password" | ||
| autoComplete="current-password" | ||
| error={ErrorMessage()} |
There was a problem hiding this comment.
The ErrorMessage component is defined as a function component but is used as a function call error={ErrorMessage()} on line 460, rather than as a React element error={<ErrorMessage />}.
While this works in this case since it returns JSX, this is an anti-pattern in React. Components should be used as JSX elements, not called as functions. This affects proper React reconciliation, hooks behavior, and component lifecycle.
Change the usage to pass the component as JSX:
error={<ErrorMessage />}Or if the Input component expects a function that returns JSX, rename ErrorMessage to renderErrorMessage or getErrorMessage to indicate it's a render function, not a React component.
| error={ErrorMessage()} | |
| error={<ErrorMessage />} |
| @@ -365,26 +360,28 @@ | |||
| {t('modalAppLockSetupSpecial')} | |||
| </p> | |||
|
|
|||
| <div className="modal__buttons"> | |||
| <div css={applockStyles.buttonGroupStyle}> | |||
| {!isAppLockEnforced && ( | |||
| <button | |||
| <Button | |||
| css={applockStyles.buttonStyle} | |||
| variant={ButtonVariant.SECONDARY} | |||
| type="button" | |||
| className="modal__button modal__button--secondary" | |||
| data-uie-name="do-cancel-applock" | |||
| onClick={onCancelAppLock} | |||
| > | |||
| {t('modalConfirmSecondary')} | |||
| </button> | |||
| </Button> | |||
| )} | |||
|
|
|||
| <button | |||
| <Button | |||
| css={!isAppLockEnforced ? applockStyles.buttonStyle : undefined} | |||
| block={isAppLockEnforced} | |||
| type="submit" | |||
| className="modal__button modal__button--primary modal__button--full" | |||
| data-uie-name="do-action" | |||
| disabled={!isSetupPassphraseValid} | |||
| > | |||
| {t('modalAppLockSetupAcceptButton')} | |||
| </button> | |||
| </Button> | |||
| </div> | |||
| </form> | |||
| )} | |||
| @@ -403,17 +400,15 @@ | |||
| data-uie-name="label-applock-set-text" | |||
| /> | |||
|
|
|||
| <div className="modal__text modal__label" data-uie-name="label-applock-unlock-text"> | |||
| {t('modalAppLockPasscode')} | |||
| </div> | |||
|
|
|||
| <input | |||
| <Input | |||
| aria-label={t('modalAppLockSetupChangeTitle', {brandName: Config.getConfig().BRAND_NAME})} | |||
| autoFocus | |||
| className="modal__input" | |||
| label={t('modalAppLockPasscode')} | |||
| type="password" | |||
| placeholder={t('modalAppLockInputPlaceholder')} | |||
| value={setupPassphrase} | |||
| onChange={event => setSetupPassphrase(event.target.value)} | |||
| onChange={(event: React.ChangeEvent<HTMLInputElement>) => setSetupPassphrase(event.target.value)} | |||
| data-uie-status={isSetupPassphraseValid ? 'valid' : 'invalid'} | |||
| data-uie-name="input-applock-set-a" | |||
| autoComplete="new-password" | |||
| @@ -442,147 +437,98 @@ | |||
| </p> | |||
|
|
|||
| <div className="modal__buttons"> | |||
| <button | |||
| type="submit" | |||
| className="modal__button modal__button--primary modal__button--full" | |||
| data-uie-name="do-action" | |||
| disabled={!isSetupPassphraseValid} | |||
| > | |||
| <Button block type="submit" data-uie-name="do-action" disabled={!isSetupPassphraseValid}> | |||
| {t('modalAppLockSetupAcceptButton')} | |||
| </button> | |||
| </Button> | |||
| </div> | |||
| </form> | |||
| )} | |||
|
|
|||
| {state === APPLOCK_STATE.LOCKED && ( | |||
| <form onSubmit={onUnlock}> | |||
| <div className="modal__text modal__label" data-uie-name="label-applock-unlock-text"> | |||
| {t('modalAppLockPasscode')} | |||
| </div> | |||
|
|
|||
| <input | |||
| <Input | |||
| aria-label={t('modalAppLockLockedTitle', {brandName: Config.getConfig().BRAND_NAME})} | |||
There was a problem hiding this comment.
Redundant aria-label on Input component. Lines 307, 404, and 450 all use both aria-label and label props on the Input component.
The label prop from the UI-Kit Input component already provides proper labeling through an associated <label> element. Adding aria-label creates redundancy and can confuse screen readers about which label to announce.
Remove the aria-label prop and rely on the label prop:
<Input
label={t('modalAppLockPasscode')}
type="password"
// ... other props
/>The aria-label should only be used when you need to override or provide a label that differs from the visible label, which doesn't appear to be the case here.
V-Gira
force-pushed
the
v/applock-sign-in-WPB-23186
branch
from
b416a9e to
cd4bdf4
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #20391 +/- ##
==========================================
+ Coverage 45.25% 45.26% +0.01%
==========================================
Files 1633 1634 +1
Lines 40245 40249 +4
Branches 8325 8326 +1
==========================================
+ Hits 18212 18219 +7
+ Misses 20105 20102 -3
Partials 1928 1928
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
| const onClickLogout = async () => { | ||
| if (isTemporaryClient) { | ||
| await clientRepository.logoutClient(); | ||
| } else { | ||
| setState(APPLOCK_STATE.LOGOUT); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The isTemporaryClient check uses optional chaining which will return undefined if currentClient is undefined. In the onClickLogout function at line 236, this means if (isTemporaryClient) will be false when isTemporaryClient is undefined, which may not be the intended behavior.
Consider making the condition more explicit:
if (isTemporaryClient === true) {
await clientRepository.logoutClient();
} else {
setState(APPLOCK_STATE.LOGOUT);
}This ensures we only call logoutClient when we explicitly know the client is temporary, and show the logout modal in all other cases (including when currentClient is undefined).
| /* | ||
| * Wire | ||
| * Copyright (C) 2026 Wire Swiss GmbH | ||
| * | ||
| * This program is free software: you can redistribute it and/or modify | ||
| * it under the terms of the GNU General Public License as published by | ||
| * the Free Software Foundation, either version 3 of the License, or | ||
| * (at your option) any later version. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| * GNU General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU General Public License | ||
| * along with this program. If not, see http://www.gnu.org/licenses/. | ||
| * | ||
| */ | ||
|
|
||
| import {CSSObject} from '@emotion/react'; | ||
|
|
||
| const buttonGroupStyle: CSSObject = { | ||
| margin: '16px 0', | ||
| display: 'flex', | ||
| justifyContent: 'space-around', | ||
| }; | ||
|
|
||
| const buttonStyle: CSSObject = { | ||
| width: '160px', | ||
| }; | ||
|
|
||
| const headerStyle: CSSObject = { | ||
| marginBottom: '32px', | ||
| fontSize: 'var(--font-size-large)', | ||
| textTransform: 'initial', | ||
| }; | ||
|
|
||
| const linkStyle: CSSObject = { | ||
| margin: '32px 0', | ||
| fontSize: 'var(--font-size-base)', | ||
| fontWeight: 'var(--font-weight-bold)', | ||
| display: 'flex', | ||
| justifyContent: 'center', | ||
| }; | ||
|
|
||
| const unlockButtonStyle: CSSObject = { | ||
| margin: '16px 0', | ||
| }; | ||
|
|
||
| export const applockStyles = { | ||
| buttonGroupStyle, | ||
| buttonStyle, | ||
| headerStyle, | ||
| linkStyle, | ||
| unlockButtonStyle, | ||
| }; |
There was a problem hiding this comment.
File name has inconsistent casing. The file is named Applock.styles.ts (lowercase 'l') while the main component is AppLock.tsx (uppercase 'L'). For consistency, consider renaming to AppLock.styles.ts to match the component name casing.
| "modalAppLockForgotWipeCTA": "Reset this client", | ||
| "modalAppLockForgotGoBackButton": "Back", | ||
| "modalAppLockForgotMessage": "The data stored on this device can only be accessed with your app lock passcode.", | ||
| "modalAppLockForgotSecondMessage":"If you have forgotten your passcode, you can log out of this account and set a new passcode the next time you log in.", |
There was a problem hiding this comment.
Missing space after the colon in the JSON property. Should be "modalAppLockForgotSecondMessage": "If you have forgotten..." (note the space after the colon).
| "modalAppLockForgotSecondMessage":"If you have forgotten your passcode, you can log out of this account and set a new passcode the next time you log in.", | |
| "modalAppLockForgotSecondMessage": "If you have forgotten your passcode, you can log out of this account and set a new passcode the next time you log in.", |
| @@ -236,12 +229,16 @@ const AppLock = ({ | |||
| const isSetupPassphraseLength = passwordRegexLength.test(setupPassphrase); | |||
| const isSetupPassphraseSpecial = passwordRegexSpecial.test(setupPassphrase); | |||
|
|
|||
| const clearWipeError = () => setWipeError(''); | |||
| const clearUnlockError = () => setUnlockError(''); | |||
| const onGoBack = () => setState(APPLOCK_STATE.LOCKED); | |||
| const onClickForgot = () => setState(APPLOCK_STATE.FORGOT); | |||
| const onClickWipe = () => setState(APPLOCK_STATE.WIPE_CONFIRM); | |||
| const onClickWipeConfirm = () => setState(APPLOCK_STATE.WIPE_PASSWORD); | |||
| const onClickLogout = async () => { | |||
| if (isTemporaryClient) { | |||
| await clientRepository.logoutClient(); | |||
| } else { | |||
| setState(APPLOCK_STATE.LOGOUT); | |||
| } | |||
| }; | |||
There was a problem hiding this comment.
The new LOGOUT state and onLogout flow lack test coverage. Tests should verify:
- When a temporary client clicks "Forgot passcode", it directly calls clientRepository.logoutClient()
- When a non-temporary client clicks "Forgot passcode", it transitions to the LOGOUT state
- The onLogout function disables app lock only when not enforced
- The onLogout function removes the passcode code
- The onLogout function publishes the correct SIGN_OUT event with the clearData flag
Consider adding test cases covering these scenarios to ensure the logout flow works correctly for both temporary and permanent clients.
2 participants
Pull Request
Summary
What is the change?
Implement a different logout flow when a user forgets their applock passcode
Previously:
Now:
Code changes
Security Checklist (required)
Accessibility (required)
Standards Acknowledgement (required)
Screenshots or demo (if the user interface changed)
Forgot Passcode and log out
Create Passcode (Team enforced)
Create Passcode (not enforced)
Notes for reviewers