feat(pos-app): unified Update keys screen with QR import/export + PIN gate#517
feat(pos-app): unified Update keys screen with QR import/export + PIN gate#517ignaciosantise wants to merge 5 commits into
Conversation
…r key Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…agement-ux # Conflicts: # dapps/pos-app/app/settings.tsx
- Gate the Update keys screen with a PIN on entry (usePinGate); saving no longer prompts separately - Add web-only Export keys screen (QR of merchant ID + customer key) and a native Import keys scanner, with a shared device-setup QR codec + tests - Apply UX-writing audit fixes to the new copy and minor UI polish (equal button heights, web focus-ring padding, back after save) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR refactors merchant credential management in pos-app into a single PIN-gated “Update keys” flow and adds QR-based provisioning (web export → native import) backed by a shared encode/decode codec.
Changes:
- Added a shared
device-setup-qrcodec (+ unit tests) for exporting/importing merchant ID + customer API key via QR payloads. - Introduced a reusable
usePinGatehook and reworked the merchant flow + settings navigation to use a dedicatedupdate-keysscreen instead of bottom sheets. - Added native QR scanning via
expo-camera, plus a web QR export screen (PIN-gated) and related routing/config updates.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| dapps/pos-app/utils/device-setup-qr.ts | Implements encode/decode for device provisioning QR payloads. |
| dapps/pos-app/utils/device-setup-qr.test.ts | Adds tests to keep QR producer/consumer in sync. |
| dapps/pos-app/store/useSettingsStore.ts | Adds transient scannedSetup handoff and ensures it’s not persisted. |
| dapps/pos-app/package.json | Adds expo-camera dependency for QR scanning. |
| dapps/pos-app/package-lock.json | Locks new expo-camera and transitive dependencies. |
| dapps/pos-app/hooks/use-pin-gate.ts | New shared PIN gate logic for sensitive screens/actions. |
| dapps/pos-app/hooks/use-merchant-flow.ts | Simplifies to “form + save” (PIN gate moved to screen entry). |
| dapps/pos-app/app/update-keys.tsx | New unified Update keys screen with PIN gate, import, export entry points. |
| dapps/pos-app/app/settings.tsx | Replaces separate merchant/key sheets with a single “Update keys” entry. |
| dapps/pos-app/app/scan-customer-key.tsx | New QR scanner screen (native provisioning) using expo-camera. |
| dapps/pos-app/app/export-keys.tsx | New QR export screen (web provisioning) that renders the setup QR. |
| dapps/pos-app/app/_layout.tsx | Registers new routes/screens for update/export/scan flows. |
| dapps/pos-app/app.json | Bumps Android versionCode and adds expo-camera permission config. |
Files not reviewed (1)
- dapps/pos-app/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if (isLockedOut()) { | ||
| setActiveModal("none"); | ||
| showErrorToast(formatLockoutMessage()); | ||
| } else { | ||
| const attemptsLeft = MAX_PIN_ATTEMPTS - pinFailedAttempts; | ||
| setPinError( | ||
| `Incorrect PIN. ${attemptsLeft} attempt${attemptsLeft !== 1 ? "s" : ""} remaining.`, | ||
| ); |
| const requireAuth = useCallback( | ||
| (onSuccess: () => void | Promise<void>) => { | ||
| if (isLockedOut()) { | ||
| showErrorToast(formatLockoutMessage()); | ||
| return; | ||
| } | ||
| onSuccessRef.current = onSuccess; | ||
| setPinError(null); | ||
| setActiveModal(isPinSet() ? "pin-verify" : "pin-setup"); | ||
| }, |
| import { encodeDeviceSetupQr } from "@/utils/device-setup-qr"; | ||
| import { router } from "expo-router"; | ||
| import { useCallback, useEffect, useRef, useState } from "react"; | ||
| import { StyleSheet, View } from "react-native"; |
| useEffect(() => { | ||
| if (startedRef.current) return; | ||
| startedRef.current = true; | ||
| requireAuth(() => setUnlocked(true)); | ||
| }, [requireAuth]); |
| useEffect(() => { | ||
| if (!permission?.granted) { | ||
| requestPermission(); | ||
| } | ||
| }, [permission, requestPermission]); |
|
@claude review |
|
Claude finished @ignaciosantise's task in 1m 34s —— View job PR Review
License compliance: ✅ All new deps (expo-camera + transitive) are MIT — no issues. Found 8 issue(s)Issue 1:
|
…agement-ux # Conflicts: # dapps/pos-app/app/settings.tsx
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| import { StyleSheet, View } from "react-native"; | ||
| import { ScrollView } from "react-native-gesture-handler"; | ||
|
|
||
| export default function ExportKeysScreen() { |
There was a problem hiding this comment.
🤖 Auto Review Issue: export-keys screen accessible on native via deep link — credentials can leak
Severity: HIGH
Category: security
Tool: Claude Auto Review
Context:
- Pattern:
ExportKeysScreenrenders unconditionally on any platform; noPlatform.OSguard exists. Theupdate-keys.tsxcorrectly hides the "Export keys" button with{isWeb && ...}, but the route itself is registered and reachable on native viarouter.push('/export-keys')or a deep link. - Risk: The PR's stated security model ("native builds have no export path") is only one navigational hop away from being violated. Any deep link or programmatic navigation on a native device reaches the export screen and shows the credentials QR after PIN.
- Impact: Credentials exfiltration from a native POS device.
- Trigger: Any call to
router.push('/export-keys')on a native build.
Recommendation:
export default function ExportKeysScreen() {
// Web-only: native builds must never expose an export path.
useEffect(() => {
if (Platform.OS !== 'web') {
router.replace('/update-keys');
}
}, []);
if (Platform.OS !== 'web') return null;
// ...rest of component| k: string; | ||
| } | ||
|
|
||
| export function encodeDeviceSetupQr(payload: DeviceSetupPayload): string { |
There was a problem hiding this comment.
🤖 Auto Review Issue: customerApiKey encoded as plaintext JSON in QR payload
Severity: HIGH
Category: security / data_classification
Tool: Claude Auto Review
Context:
- Pattern:
encodeDeviceSetupQrserializescustomerApiKeydirectly into a plain JSON string (JSON.stringify(raw)). The resulting string is displayed as a QR code on screen with no timeout. - Risk: The
customerApiKeygrants full merchant account access. As plaintext JSON it is immediately usable by anyone who captures the QR — screenshot, photo, screen recording, shoulder surfing, or a security camera. - Impact: Full merchant account compromise from a single photo. The QR stays on screen indefinitely; there is no auto-dismiss or hide toggle.
- Trigger: Any time the export screen is displayed (after PIN unlock on web).
Recommendation: At minimum, add an auto-dismiss timeout (e.g. 30–60 s) and a "tap to hide/show" toggle. Longer term, consider one-time-code symmetric encryption: importing device shows a short pairing code; exporting device encrypts the payload with it; the QR is useless without the matching code.
| setActiveModal("none"); | ||
| showErrorToast(formatLockoutMessage()); | ||
| } else { | ||
| const attemptsLeft = MAX_PIN_ATTEMPTS - pinFailedAttempts; |
There was a problem hiding this comment.
🤖 Auto Review Issue: pinFailedAttempts off-by-one — wrong attempt count shown to user
Severity: HIGH
Category: code_quality / security
Tool: Claude Auto Review
Context:
- Pattern:
const attemptsLeft = MAX_PIN_ATTEMPTS - pinFailedAttemptsuses the pre-increment snapshot ofpinFailedAttemptscaptured from the store selector at render time.verifyPin()on line 65 incrementspinFailedAttemptsinside the store synchronously, but the closure still holds the old value. - Risk: After the first failure,
pinFailedAttemptsin the store is 1, but the closure sees 0, so the message reads "3 attempts remaining" instead of "2". After the second failure it shows "2" instead of "1". This makes the lockout warning misleading and could cause a user to expect one more attempt than they actually have. - Impact: User is surprised by lockout because the displayed count was wrong.
- Trigger: Every failed PIN attempt.
Recommendation: Read the current count from the store after verifyPin() resolves:
const isValid = await verifyPin(pin);
if (isValid) {
await runSuccess();
} else if (isLockedOut()) {
setActiveModal("none");
showErrorToast(formatLockoutMessage());
} else {
// Read fresh value — verifyPin() already incremented it
const currentAttempts = useSettingsStore.getState().pinFailedAttempts;
const attemptsLeft = MAX_PIN_ATTEMPTS - currentAttempts;
setPinError(`Incorrect PIN. ${attemptsLeft} attempt${attemptsLeft !== 1 ? "s" : ""} remaining.`);
}|
|
||
| const requireAuth = useCallback( | ||
| (onSuccess: () => void | Promise<void>) => { | ||
| if (isLockedOut()) { |
There was a problem hiding this comment.
🤖 Auto Review Issue: Blank screen after lockout toast dismisses — no retry UI
Severity: MEDIUM
Category: code_quality
Tool: Claude Auto Review
Context:
- Pattern: When
isLockedOut()is true,requireAuth()shows an error toast and returns early. Bothupdate-keysandexport-keysstay on the screen withunlocked = false, which hides the content behind the PIN modal. But no modal is shown and after the toast fades the user is left on a blank screen with no way to retry or navigate away. - Risk: Users on locked-out devices see an empty screen with no feedback or action.
- Impact: Dead-end UX; user must background/foreground the app or force-quit.
- Trigger: Any user who has been locked out navigating to update-keys or export-keys.
Recommendation: Return a boolean or expose a lockedOut state so callers can render a lockout message or navigate away:
// Option A: return false so screens can router.back()
const requireAuth = useCallback((onSuccess) => {
if (isLockedOut()) {
showErrorToast(formatLockoutMessage());
return false;
}
// ...
return true;
}, [...]);
// In update-keys.tsx / export-keys.tsx:
useEffect(() => {
if (startedRef.current) return;
startedRef.current = true;
const granted = requireAuth(() => setUnlocked(true));
if (!granted) router.back();
}, [requireAuth]);| const hasScannedRef = useRef(false); | ||
| const lastInvalidToastRef = useRef(0); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
🤖 Auto Review Issue: Camera permission re-requested in denied state on every re-render
Severity: MEDIUM
Category: code_quality
Tool: Claude Auto Review
Context:
- Pattern:
useEffectcallsrequestPermission()whenever!permission?.granted, which includes bothundeterminedanddeniedstates. After a user explicitly denies the camera permission, re-renders (e.g. state updates) trigger repeatedrequestPermission()calls. - Risk: On Android, repeated requests after denial can permanently suppress the permission dialog (auto-deny). On iOS the second request is silently no-opped, but the effect still runs unnecessarily.
- Impact: Users who deny permission may be unable to grant it later via a re-prompt; they'd need to go to device settings.
- Trigger: Any state update that causes a re-render while camera permission is denied.
Recommendation:
useEffect(() => {
if (permission?.status === 'undetermined') {
requestPermission();
}
}, [permission?.status, requestPermission]);| contentContainerStyle={styles.content} | ||
| showsVerticalScrollIndicator={false} | ||
| > | ||
| {!isLoading && !qrValue ? ( |
There was a problem hiding this comment.
🤖 Auto Review Issue: QR renders with undefined uri during async key load
Severity: LOW
Category: code_quality
Tool: Claude Auto Review
Context:
- Pattern: The render logic is
{!isLoading && !qrValue ? <error> : <QRCode uri={qrValue ?? undefined} />}. WhenisLoadingistrue,qrValueisnull, so the QR component renders withuri={undefined}— the loading/error branch is not shown, the QR branch is shown with no data. - Risk: The QR component may render a broken/empty state before the key loads.
- Trigger: Brief window between unlock and key retrieval.
Recommendation:
{isLoading ? (
<ActivityIndicator />
) : !qrValue ? (
// error state
) : (
<QRCode size={280} uri={qrValue} arenaClear />
)}| if (merchantIdChanged) { | ||
| setMerchantId(trimmedMerchantId); | ||
| addLog( | ||
| "info", |
There was a problem hiding this comment.
🤖 Auto Review Issue: merchantId logged verbatim — account identifier in log store
Severity: LOW
Category: data_classification
Tool: Claude Auto Review
Context:
- Pattern:
addLog("info", \Merchant ID updated to: ${trimmedMerchantId}`, ...)` writes the full merchant ID into the log store on every save. Logs are viewable via "View logs" in settings and potentially exported in bug reports. - Impact: Merchant account identifier disclosed in any log export or support ticket.
Recommendation: Log the event without the value: addLog("info", "Merchant ID updated", "settings", "handleUpdateKeysConfirm").
- Guard export-keys to web-only (native navigates away, never reveals the QR) - Fix off-by-one in PIN "attempts remaining" (read post-increment count) - Only auto-request camera when permission is undetermined (no re-prompt loop) - On lockout, send the user back instead of leaving a blank gated screen Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Reworks merchant credential management in pos-app into a single, PIN-protected Update keys screen, and adds QR-based device provisioning so keys flow web → POS without manual typing.
update-keysscreen. The merchant ID and customer key are edited together and saved in one step.usePinGatehook; the form stays hidden until unlocked, and saving no longer re-prompts.useMerchantFlowis slimmed to form + save only.expo-camerascanner that reads a setup QR and fills both fields; rejects non-setup QRs.device-setup-qrcodec (with unit tests) keeps producer and scanner in sync.Provisioning flow
flowchart LR subgraph Web["Web (admin)"] UKw["Update keys (PIN)"] --> EXP["Export keys\nQR of merchant ID + customer key"] end subgraph POS["POS device (native)"] UKn["Update keys (PIN)"] --> IMP["Import keys\nscan QR"] IMP --> FILL["Both fields filled"] --> SAVE["Save keys"] end EXP -. "scan" .-> IMPTesting
npx tsc --noEmit,eslint,prettierclean (only 2 pre-existing unrelated tsc errors).device-setup-qrcodec tests + store tests pass. Fulljestrun is OOM-limited in the sandbox; per-suite runs pass.expo.android.versionCodebumped;expo-cameraconfig plugin added.🤖 Generated with Claude Code