2FA: Standalone 2FA Form#394
Conversation
|
@nickgr2 please review |
nickgr2
left a comment
There was a problem hiding this comment.
Two issues to address, plus a few minor cleanups.
note is required where peer screens make it optional
src/screens/CreateRecord/v2/CreateOrEditAuthenticatorContent.tsx:95
note: Validator.string().required(t`Comment is required`)The empty-state branch (line 285) renders a single InputField whose onChangeText is handleFirstCustomFieldChange. The first keystroke pushes [{ type: 'note', note: 'x' }], which switches the render to the mapped branch. The trash button only renders for customFieldsList.length > 1, so the user cannot remove that single entry — only clear it. Once cleared, validation fails on submit and the user has no escape other than retyping something.
Peer screens use note: Validator.string() (not required) — see CreateOrEditCustomContent.tsx:103 and CreateOrEditLoginContent.tsx:144. Match that pattern.
HeaderV2.tsx branch checks the prop, not the parameter
src/containers/Header/HeaderV2.tsx:42
const handleCreateRecord = (type: string) => {
if (type === 'password') { ... }
if (recordType === RECORD_TYPES.OTP) {
navigation.navigate('CreateRecord', { recordType: RECORD_TYPES.OTP })
return
}
navigation.navigate('CreateRecord', { recordType: type })
}The original branch checked the type parameter (type === 'authenticator'); the new branch checks the screen-level recordType prop. With the current call sites (addItemButton passes recordType; the context menu passes the user's selection, now RECORD_TYPES.OTP), the fallthrough at line 47 produces the same navigation, so the branch is dead. Either remove it, or change to type === RECORD_TYPES.OTP to mirror the previous line and keep the file consistent.
Minor
minor:src/screens/Authenticator/index.jsx:10-16— two consecutiveimportstatements from@tetherto/pearpass-lib-vault. Merge into one.minor:src/screens/CreateRecord/v2/CreateOrEditAuthenticatorContent.tsx:276—aria-label="Delete note"is the only user-facing string in the file not localized. Usearia-label={t\Delete note`}`.minor:src/types/pearpass-lib-vault.d.ts:6—OTP: "otp",uses double quotes and a trailing comma; every other entry uses single quotes and no trailing comma. Match the surrounding style.minor:src/screens/CreateRecord/v2/CreateOrEditAuthenticatorContent.tsx:125—initialRecord: initialRecord as objectcast isn't needed.useGetMultipleFilesis JS with JSDocObject; peer screens passinitialRecorddirectly.src/screens/CreateRecord/v2/CreateOrEditAuthenticatorContent.tsx:92—otpSecret: Validator.string()allows saving a "Standalone 2FA" record with no secret. Such a record won't appear in the Authenticator listing (r.otpPublicis null on submit). Is this intentional (user pre-creates the entry and pastes later), or should the secret be required?
|
@nickgr2 I've addressed your points, please review |
nickgr2
left a comment
There was a problem hiding this comment.
The two main issues from the last round are addressed (note field made optional, dead HeaderV2 branch removed). The two import/style nits are also addressed.
Withdrawing the aria-label nit — CreateOrEditCustomContent.tsx:305 uses the same unlocalized pattern, so this PR is consistent with the rest of the codebase.
Two open items, neither blocking:
src/screens/CreateRecord/v2/CreateOrEditAuthenticatorContent.tsx:125—initialRecord as objectcast still present; peer screens (e.g.CreateOrEditCustomContent.tsx:133) passinitialRecordtouseGetMultipleFilesdirectly. TheAuthenticatorRecordtype already satisfiesobject, so the cast is redundant.- The empty-
otpSecretquestion from the prior round wasn't answered — saving submits asRECORD_TYPES.LOGINwithotpInputundefined, which means a "Standalone 2FA" entry can be created without a secret and won't appear in the Authenticator listing. If that's the intended UX (entry now, paste secret later), no change needed; flagging in case it isn't.
Approving — these can land separately.
Requirements
2FA: otp create record
Changes
Testing Notes
Things reviewers should pay attention to
Screenshots/Recordings
Screen.Recording.2026-05-07.at.11.22.08.mov
dependencies