fix(native): native release bug fixes — 2026-09-02 (TASK-22125 duplicate notification artwork, TASK-22146 About ToS title locale, TASK-22209 duplicate welcome push, card details survive app switch + expiry copy) - #2924
Conversation
…ed the peanut twice The launcher icon already sits on the left of every Android notification, and ic_onesignal_large_icon_default put the same mascot on the right. Without a default, the right slot stays empty unless the API sends contextual art (`large_icon`) for that push — a badge, a receipt, etc. TASK-22125 Claude-Session: https://claude.ai/code/session_01BQuSkygKUt9jbs2N4iCKtk
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code-analysis diffPainscore total: 7149.37 → 7153.05 (+3.68) 🆕 New findings (29)
…and 9 more. ✅ Resolved (25)
…and 5 more. 📈 Painscore deltas (top movers)
|
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
![chip-peanut-bot[bot]](https://avatars.githubusercontent.com/in/2915219?s=60&v=4){kind=small}
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Clean review: removing OneSignal's optional default large-icon resource eliminates the duplicate artwork while preserving the small status icon and per-push large_icon overrides.
Findings
- MAJOR · android/app/src/main/res/drawable-xxxhdpi/ic_onesignal_large_icon_default.png:1 · [moonshotai/kimi-k3] Only the xxxhdpi density of ic_onesignal_large_icon_default is removed — other density buckets may keep the duplicate artwork alive
Failure scenario: OneSignal resolves the default large icon at runtime by resource name (ic_onesignal_large_icon_default), and Android resolves drawables across all density buckets with fallback scaling. If the original commit e8e5bf2 added the usual mdpi/hdpi/xhdpi/xxhdpi variants alongside xxxhdpi, deleting only the xxxhdpi file leaves the resource fully resolvable on every device (xxxhdpi devices just upscale the xxhdpi copy), so the duplicate mascot this PR exists to remove keeps appearing in every notification. Evidence: the diff deletes a single file under drawable-xxxhdpi while the description claims the default is removed entirely, and the QA section states no Android build or screenshots were produced, so a partial removal would ship unnoticed. Fix: runfind android -name 'ic_onesignal_large_icon_default*'and delete every remaining density variant, and grep for any@drawable/ic_onesignal_large_icon_defaultreferences that would break the release build once the asset is gone.
Inline anchors unavailable for 1 finding(s); the findings remain in this summary.
Checked clean
- Verified the detached worktree HEAD, trusted author, exact head SHA, dev base ref, base SHA, and merge base match the supplied review inputs.
- The exact merge-base diff deletes only android/app/src/main/res/drawable-xxxhdpi/ic_onesignal_large_icon_default.png; repository-wide reference search found no explicit consumer of the deleted resource.
- OneSignal documents ic_onesignal_large_icon_default as the optional Android default large icon and documents explicit large_icon payloads as overrides; notifications with no override therefore lose only the duplicate right-side artwork.
- The separate ic_stat_onesignal_default resources remain at every Android density, along with the manifest accent metadata, so the status-bar icon path is unchanged.
- Correctness, security, adversarial, and slop passes covered notifications both with and without contextual large_icon payloads; the resource-only deletion introduces no permission, trust-boundary, secret, or runtime-code change.
- Exact-head unit, typecheck, eslint, format, analysis, human-authors, bot-approval, report, review, and ci-success checks passed; Deploy-Preview was still in progress when checked and does not validate the native Android rendering change.
Second opinion by moonshotai/kimi-k3: 1 finding(s), marked with the model name. It reads the diff only, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 79fb1af58c31 · Context: repo, provider-docs:onesignal · Took 10m
|
Re the kimi-k3 finding (other density buckets): verified against origin/dev — |
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Clean review: removing OneSignal's optional default large-icon resource eliminates the duplicate artwork while preserving the small status icon and per-push large_icon overrides.
Checked clean
- Verified the detached worktree HEAD, trusted author, exact head SHA, dev base ref, base SHA, and merge base match the supplied review inputs.
- The exact merge-base diff deletes only android/app/src/main/res/drawable-xxxhdpi/ic_onesignal_large_icon_default.png; repository-wide reference search found no explicit consumer of the deleted resource.
- OneSignal documents ic_onesignal_large_icon_default as the optional Android default large icon and documents explicit large_icon payloads as overrides; notifications with no override therefore lose only the duplicate right-side artwork.
- The separate ic_stat_onesignal_default resources remain at every Android density, along with the manifest accent metadata, so the status-bar icon path is unchanged.
- Correctness, security, adversarial, and slop passes covered notifications both with and without contextual large_icon payloads; the resource-only deletion introduces no permission, trust-boundary, secret, or runtime-code change.
- All reported exact-head checks completed successfully or were intentionally skipped, including unit, typecheck, eslint, format, analysis, human-authors, bot-approval, report, review, ci-success, and Deploy-Preview; CI does not validate native Android rendering.
Second opinion by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 79fb1af58c31 · Context: repo, provider-docs · Took 3m
…he generated face The dot-face avatar (497ab2a) reads as a random emoji on the home chip and the profile header. Regress to the initials avatar in a first-letter mode for the user's own identity; contacts keep two-letter initials. The face is parked until avatar v2 — recover it from 497ab2a. Claude-Session: https://claude.ai/code/session_01BQuSkygKUt9jbs2N4iCKtk
🖼 Visual diff — 8 screens moved13 of 66 shots changed · 53 identical · baseline
job summary · before/after/diff images — artifact Fixture screenshots, no backend. Advisory — this check never blocks a merge. Posted from the default branch by ds-shots-comment.yml; the report it renders is untrusted data. |
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
No actionable defect found in the Android large-icon removal or the own-avatar fallback change at the supplied head.
Checked clean
- Confirmed the detached worktree HEAD and merge base exactly match the supplied head and base SHAs; reviewed both commits and the full diff.
- Checked Android notification icon behavior against OneSignal's official icon contract: the large default is optional, per-push large_icon overrides remain supported, and all small-icon density resources are unchanged.
- Checked own-avatar and counterparty call sites, ASCII username constraints, first-letter rendering, removal of all DotFaceAvatar references, and locale-key cleanup.
- Constructed and refuted the concrete trust-boundary cases: the rendered letter remains React-escaped, and the diff introduces no privilege, storage, routing, secret, or external-input sink change.
- Exact-head CI passed unit, typecheck, eslint, format, ds-lint, analyze, deploy preview, and the aggregate ci-success gate; ds-shots was still in progress at the final status check.
- Focused local Jest execution was unavailable because the detached worktree has no installed Jest binary; the exact-head unit CI check passed.
Second opinion by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 5de5a0e029ca · Context: repo, provider · Took 11m
About Peanut listed "Terms of Service" in English whatever the app language. The title now reads Términos de servicio in Spanish and Termos de serviço in Portuguese; every language still opens the same English document at /terms. The other policy titles keep their English legal names, as before. TASK-22146 Claude-Session: https://claude.ai/code/session_013Y9qfp4Y1BrPTkbxp3ABeq
abalinda
changed the title
…142) The one-letter own-avatar now comes from AvatarWithBadge's firstLetterOnly mode (#2924, cherry-picked underneath). UserAvatar only adds the picked-key branch and otherwise renders that same fallback, so the fallback lives in one place. HomeTopNav and the self ProfileHeader pass the pick through.
There was a problem hiding this comment.
Chip review — changes requested
One major avatar integration defect remains; the notification-icon and localized Terms changes otherwise match their stated contracts.
Findings
-
MAJOR · src/features/home/views/HomeTopNav.tsx:38 · Home avatar still seeds from the full name
When a user has showFullName=true and a full name whose first letter differs from their username (for example, usernameboband full nameAlice), useHomeFlow passesAliceas avatarName, so this new first-letter mode rendersAon Home while ProfileHeader rendersB. That breaks the stated username-initial identity across the two surfaces. Pass the username to HomeTopNav/AvatarWithBadge while preserving the existing no-username fallback, and cover the showFullName=true integration case. -
MINOR · android/app/src/main/res/drawable-xxxhdpi/ic_onesignal_large_icon_default.png:1 · [moonshotai/kimi-k3] Only the xxxhdpi copy of ic_onesignal_large_icon_default is deleted
OneSignal resolves ic_onesignal_large_icon_default by resource name and Android picks the best-matching density bucket, scaling as needed — so the default large icon stays active as long as ANY density copy (drawable-hdpi/mdpi/xhdpi/xxhdpi or an unqualified drawable/) still exists. This diff removes only drawable-xxxhdpi/ic_onesignal_large_icon_default.png, and binary files are invisible in review, so if the original commit e8e5bf2 added the icon to more than one bucket (the standard OneSignal setup does), most devices (xxhdpi and below) will keep showing the duplicate mascot after this 'fix' ships in the store build. Fix: grep android/app/src/main/res for every ic_onesignal_large_icon_default.* and delete all density variants, not just xxxhdpi.
Inline anchors unavailable for 1 finding(s); the findings remain in this summary.
Checked clean
- Verified the detached HEAD, exact base SHA, merge base, and complete 18-file diff.
- Exact-head CI completed successfully, including unit, typecheck, eslint, format, ds-lint, analyze, and ds-shots.
- Checked OneSignal's Android icon contract: the bundled default large icon is optional and a per-message large_icon may be a resource name or remote URL.
- Checked About links and locale resolution: es-AR inherits es-419, DocsLink leaves /terms unchanged, and locale tests cover English, Spanish, and Portuguese.
- Confirmed DotFaceAvatar and userAvatarAlt have no remaining repository references.
- Security pass found no changed trust boundary, secret exposure, unsafe URL handling, or privilege change.
Second opinion by moonshotai/kimi-k3: 1 finding(s), marked with the model name. It reads the diff only, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 82a1e0e8fe1d · Context: repo, provider-docs · Took 12m (queued 1m)
… Terms The first cut localized the Terms of Service title alone; Aleks asked for the whole list. All eight titles now come from the catalog under profile.about.policies, reusing the landing footer's approved strings (Security Disclosure is the one new translation). The legal hrefs are bare paths, so every language still opens the same English documents. TASK-22146 Claude-Session: https://claude.ai/code/session_013Y9qfp4Y1BrPTkbxp3ABeq
There was a problem hiding this comment.
Chip review — changes requested
The four prior findings are fixed, but keeping revealed card details rendered while the native app backgrounds exposes PAN/CVV in OS task-switcher snapshots.
Findings
- BLOCKING · src/hooks/useCardReveal.ts:29 · Hide card credentials from native task-switcher snapshots
After a user reveals the card and switches to a merchant app, iOS/Android can snapshot the still-rendered PAN, expiry, and CVV for the app switcher. This repository has no inactive-state cover or native secure-window protection, so that cached preview can outlive the 30-second JavaScript timer and expose usable card credentials later. Preserve the in-memory reveal state, but obscure or secure the native window while inactive/backgrounded and remove the cover on resume; verify both platforms with a revealed-card task-switcher check.
Checked clean
- Exact detached head, trusted author, dev base ref, supplied base SHA, and merge base all match.
- Exact-head CI passed unit, native-export, typecheck, eslint, format, ds-lint, ds-shots, analyze, and deployment checks.
- Android default large-icon removal leaves contextual notification artwork opt-in and does not change the status-bar icon.
- Home avatar seeding now prefers username and its showFullName regression test covers a differing full name.
- Both OneSignal adapters forward the SDK previous opted-in state; the hook acts only on false-to-true and tests token/id settling plus refresh.
- About policy localization coverage preserves legal hrefs across English, Spanish, and Portuguese.
- Expiry copying uses MM/YY, localized success copy, and 16px DS-scale copy/check icons.
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 361d90fe74f2 · Context: repo, product, ops · Took 10m
Dropping the visibilitychange mask left the PAN/CVV painted in the backgrounded webview, where iOS/Android snapshot it for the task switcher. Cover the details while document.hidden and show the same in-memory payload again on resume, so the merchant-app paste flow still needs no rate-limited re-reveal. Blur stays unmasked: native fires it spuriously.
There was a problem hiding this comment.
Chip review — changes requested
The prior avatar, OneSignal transition, and icon-scale defects are fixed, but native task-switcher protection still has a blocking race and the notification fix leaves one narrow overlapping-login path.
Findings
-
BLOCKING · src/hooks/useCardReveal.ts:105 · Protect native snapshots before the OS captures them
When a revealed card is backgrounded, this handler only schedules a React state update after a web visibility event. The native OS can snapshot the WebView before that render commits—the code itself notes this race—and the unit test waits for React rather than exercising a real task-switcher capture. A full PAN/CVV can therefore remain in recents. Cover the native window synchronously during the platform background callback (an iOS privacy overlay and Android secure-window protection such as FLAG_SECURE), then verify both devices on-screen. -
MINOR · src/hooks/useNotifications.ts:231 · Serialize the initial OneSignal login
Initialization publishes oneSignalInitialized before awaiting its first syncExternalIdLink. If that login is still pending when the Home notification CTA is tapped, this false-to-true callback starts another sync; lastLinkedExternalId is written only after login resolves, so both calls reach adapter.login and can reproduce the duplicate-record/welcome race. Deduplicate in-flight login attempts per external ID (while preserving retry after failure), or expose readiness only after the initial link settles, and test with a deliberately pending first login.
Checked clean
- P1 fixed: useHomeFlow seeds avatarName from username regardless of showFullName, with a usernameless fallback and regression coverage.
- P2 and P3 fixed: both OneSignal adapters forward previousOptedIn and the hook acts only on the SDK false-to-true transition; the exact-head unit job passed.
- P5 fixed: expiry and CVV copy icons now use size 16, and the exact-head ds-lint job passed.
- Android default large-icon removal and native export were checked; the exact-head native-export job passed.
- About-policy locale coverage and the new expiry-copy path were reviewed; catalogs remain aligned across en, es-419, and pt-BR.
- Exact-head typecheck, eslint, format, unit, native-export, ds-lint, analyze, and deployment checks passed; ds-shots was still running at review time.
- Targeted local Jest could not start because this detached worktree has no node_modules; exact-head CI unit coverage passed instead.
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 5659eb6b9cc1 · Context: repo, platform-docs · Took 11m
…rting a second Init publishes oneSignalInitialized before its first login() resolves, so an opt-in in that window ran syncExternalIdLink again and reached adapter.login twice for the same id — the double-record race behind TASK-22209 on one more path. A sync for an id whose login is in flight now joins that promise; failures still clear it so the next sync retries.
Conflicts in useHomeFlow: dev (#2929, TASK-22142) removed avatarName and seeds the top-nav chip from the username directly, which supersedes this branch's avatar-seeding fix. Took dev's version of the hook and its test.
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
The earlier avatar, subscription-transition, initial-login, and card-cover findings are fixed or refuted. Two minor issues remain in the OneSignal in-flight guard and expiry-copy control.
Findings
-
MINOR · src/hooks/useNotifications.ts:116 · Do not let an old login clear the new in-flight guard — also flagged by moonshotai/kimi-k3
Start login A, switch to external id B before it settles, then let A resolve while B's login is still pending. This finally block clears the single tracker unconditionally, so a B subscription event no longer sees B in flight and starts a second adapter.login(B), recreating the duplicate-record/welcome race this commit is meant to close. Serialize identity mutations, or clear the slot only when it still refers to the promise being finalized and then reconcile the latest currentExternalId; add the A-to-B overlap case to the test. -
MINOR · src/components/Card/CardFace.tsx:184 · Give the expiry copy button a real hit target — also flagged by moonshotai/kimi-k3
The new expiry button is a 16px icon plus p-1 (24px total) and, unlike the adjacent PAN/CVV controls, has no expanded pseudo-element, focus-visible ring, or pressed feedback. On the compact card row this makes the copy action easy to miss and keyboard focus hard to see. Reuse the adjacent expanded target and interaction classes.
Checked and not raised again
- BLOCKING · src/hooks/useCardReveal.ts:105 · [moonshotai/kimi-k3] Native snapshots can still capture card credentials — this review checked it and does not believe it. No task filed.
Checked clean
- Confirmed the detached worktree head, supplied base SHA and merge base, trusted author, dev base ref, and PR metadata all match the requested review target.
- Exact-head CI is green across unit, typecheck, eslint, format, native export, design-system lint and screenshots, analysis, deployment, provenance, and ownership gates.
- P1 is fixed: useHomeFlow seeds the Home avatar from username regardless of showFullName, with full name only as the usernameless fallback and regression coverage.
- P2 and P3 are fixed: both OneSignal adapters forward previousOptedIn and the hook acts only on a false-to-true transition; token/id settling and reload refreshes are covered as no-ops.
- P6 is fixed for same-id overlap: a second sync joins the stored in-flight promise, and the pending-initial-login regression test confirms one login call.
- P4, P5, and P7 were checked against dev and the supplied product decision: the hidden-state React cover preserves the prior snapshot posture, while app-wide screenshot blocking is explicitly rejected; this PR introduces no snapshot regression.
- Android default large-icon removal leaves all five OneSignal status-bar icon densities intact and no repository reference depends on the deleted drawable.
- About-policy locale coverage, expiry MM/YY copying, catalog parity, diff whitespace, secret exposure, injection, privilege changes, and dead references were checked without another actionable defect.
Security review by moonshotai/kimi-k3: 3 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: d836cbeeb2ca · Context: repo, github_ci · Took 9m (queued 9m)
Co-authored-by: chip-peanut-bot[bot] <262992217+chip-peanut-bot[bot]@users.noreply.github.com> Signed-off-by: ab <78670703+abalinda@users.noreply.github.com>
There was a problem hiding this comment.
Chip review — changes requested
One blocking native snapshot gap remains. The avatar and opt-in regressions are fixed, but the new OneSignal login guard still has a smaller stale-finally race.
Findings
-
BLOCKING · src/hooks/useCardReveal.ts:105 · Cover revealed credentials from the native lifecycle
This still relies on a React update fromvisibilitychangeto remove PAN/CVV from the rendered card. The repo already uses CapacitorappStateChangebecause Android WebViews do not reliably emit visibility changes, and this hook itself acknowledges that the JS cover can lose the OS snapshot race. If the app backgrounds before that callback and React commit complete, Recents can capture the revealed credentials. Cover the native view synchronously from the iOS/Android inactive lifecycle (only while inactive, so active screenshots remain allowed), restore the retained payload on foreground, and keep the JS cover for web. -
MINOR · src/hooks/useNotifications.ts:241 · [claude-opus] Login-retry-after-failure path is untested
The subscription listener replaced an unconditionalawait adapter.login(currentExternalId)withvoid syncExternalIdLink()(src/hooks/useNotifications.ts:241). The code comment justifies that with "this is only a retry for a login that failed there" — i.e. the correctness of the change rests on the case where init's login rejected,lastLinkedExternalIdwas never committed (line 112), and the first false → true opt-in re-links the device. That mutates shared state that decides whether the device is reachable at all: a failed link means the subscription carries no external_id and every backend push for that user reaches zero recipients (the comment at lines 89-93 says exactly this).
Neither new test covers it. useNotifications.subscription.test.ts has login resolving and asserts toHaveBeenCalledTimes(1) (no retry needed); useNotifications.loginDedupe.test.ts has login pending forever and asserts the join. The untested case is: init's login() rejects with a non-identity error, then onSubscriptionChange({ optedIn: true, previousOptedIn: false }) fires — login must be called a second time and lastLinkedExternalId committed on success. A regression that dropped the void syncExternalIdLink() call, or committed lastLinkedExternalId before the await, would pass both existing tests and silently leave failed-link devices push-dead.
Fix: add a case to useNotifications.loginDedupe.test.ts with login rejecting once then resolving, asserting the opt-in transition drives a second login('user-1').
Answered by a human, not raised again
These are still present as far as this review can tell. You resolved the thread or deferred the fix, so Chip files them instead of repeating them.
- MINOR · src/hooks/useNotifications.ts:116 · Keep a newer login's in-flight guard intact — no task filed, please file one
Checked clean
- The detached worktree HEAD and merge base match the supplied head and base SHAs; PR metadata matches the trusted author and dev base.
- The Android default large-icon asset is removed with no remaining resource or manifest reference; the status-bar icon remains.
- About-policy titles and hrefs are covered across en, es-419, and pt-BR.
- Expiry copy formatting, clipboard routing, accessible labeling, and the expanded 48px pseudo-element hit target are present.
- Both OneSignal adapters forward the SDK previous opt-in state, and the hook acts only on false-to-true transitions.
- Home now passes the username directly to HomeTopNav, independent of the display-name preference.
- Exact-head Vercel and Chip-review checks passed. The repository Tests workflow did not report on this head; local Jest could not run because the detached worktree has no node_modules.
Security review: did not run — the daily spend cap was reached, so nothing was sent. This review is one reviewer short.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 624d56bc3ec0 · Context: repo, product · Took 15m
…ASK-22273) Edit Profile drew its header from the full name regardless of "Show my full name", so a user who turned the setting off still saw their legal name on the screen that owns the setting. The header now shows what everyone else sees: the full name only while it is public, the username otherwise. The toggle becomes controlled so the screen owns the value. The header then reacts to a tap immediately instead of waiting for the background user refetch, which is what forced the extra reload. Turning the setting on publishes the legal name next to the username, so it now asks first through ActionModal. Turning it off takes nothing away and still saves straight away.
There was a problem hiding this comment.
Chip review — changes requested
Four defects remain: desktop focus loss can leave card secrets painted, failed name-visibility saves are treated as success, and two OneSignal races can duplicate or lose external-id linking. The earlier native-snapshot objections do not represent a regression from dev.
Findings
-
BLOCKING · src/hooks/useCardReveal.ts:105 · Keep web blur masking for revealed card data
On desktop, put the browser with a revealed card on one monitor and focus another application on the other. The browser loses focus while document.visibilityState can remain visible, so this visibility-only listener never covers the PAN/CVV and they stay painted until the 30-second timeout. The base branch masked on window blur; preserve blur/pagehide masking for web and exempt only the native Capacitor flow that needs to survive an app switch. -
MAJOR · src/components/Profile/components/ShowNameToggle.tsx:23 · Handle resolved errors from the name-visibility save
updateUserById resolves { error } for non-2xx and network failures; it does not reject. This chain therefore treats a failed save as success, and the new controlled header/toggle keeps the optimistic value unless the fire-and-forget refresh happens to restore it. If a public user turns the setting off during a failed request, this screen says the legal name is hidden while the server still exposes it. Inspect result.error, revert and surface the failure, and make the regression test resolve { error } instead of rejecting. -
MINOR · src/hooks/useNotifications.ts:116 · Keep a newer login's in-flight guard intact — also flagged by moonshotai/kimi-k3
Start login A, switch to external id B before it settles, then let A finish while B is still pending. This finally clears the tracker that now belongs to B; a B subscription event can then start a second login(B), recreating the duplicate-record race this PR is meant to close. Serialize identity mutations, or clear the slot only if it still points to this invocation and reconcile the latest currentExternalId. -
MINOR · src/hooks/useNotifications.ts:106 · Retry after a joined initial login fails
If the initial login is pending when the false-to-true opt-in event arrives, this branch joins that promise. When the underlying login then fails, the error is swallowed and the guard is cleared, but the opt-in callback has already returned; later token/id events have previousOptedIn=true and never call sync again. The device remains unlinked until reload. Queue a retry after a joined failure (or retry later same-subscription events only while unlinked) and cover a rejected first login in the focused test. -
MINOR · src/hooks/useNotifications.ts:241 · [claude-opus] Login-retry-after-failure path is untested
The listener now callsvoid syncExternalIdLink()only on a false → true opt-in, justified by the comment "this is only a retry for a login that failed there" (src/hooks/useNotifications.ts:235-241). That retry mutates shared state — the OneSignal subscription↔external_id link that decides whether any push reaches the user — and no test covers it. The two new suites cover the opposite cases: useNotifications.subscription.test.ts asserts login is not called again after a successful init login, and useNotifications.loginDedupe.test.ts asserts a sync joins an in-flight login. The existing useNotifications.test.ts stubsloginas always-resolving. Untested case: init'sadapter.login(id)rejects (solastLinkedExternalIdstays null andloginInFlightis cleared in thefinally), then a{optedIn: true, previousOptedIn: false}change arrives — a second login() must fire and, on success, commitlastLinkedExternalId. If that ever regresses, the device stays unlinked and pushes silently reach zero recipients, which is the exact failure handleLoginError is written to shout about. Fix: add a case to useNotifications.loginDedupe.test.ts withloginrejecting once then resolving, asserting login is called twice and a subsequent duplicate change event does not call it a third time.
Checked and not raised again
- BLOCKING · src/hooks/useCardReveal.ts:105 · [moonshotai/kimi-k3] Native snapshots can still capture card credentials — this review checked it and does not believe it. No task filed.
Checked clean
- Confirmed the detached worktree head, supplied base SHA and merge base, trusted author, dev base ref, and PR metadata all match the requested review target.
- Reviewed the complete 20-file base-to-head diff across Android notification artwork, avatar flow, About localization, card reveal/copy behavior, profile name visibility, and both OneSignal adapters.
- Verified the Android default large-icon deletion leaves the status-bar icon resources intact and no repository reference depends on the deleted drawable.
- Verified expiry copying formats MM/YY, uses the matching localized toast, and now has an expanded 48px pointer target with focus and pressed feedback.
- Verified both OneSignal adapters forward the SDK previous opted-in state and the subscription listener acts only on false-to-true transitions.
- Checked browser visibility semantics against the Page Visibility standard: a focused foreground tab can remain visible after its window loses focus, so blur and visibility are not interchangeable.
- Exact-head Vercel deployment and the review workflow completed successfully; no unit, typecheck, lint, or format check runs were present for this head.
- git diff --check is clean; focused Jest tests could not run because the detached worktree has no node_modules.
Security review by moonshotai/kimi-k3: 2 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 159e0b81014d · Context: repo, github_ci, web · Took 16m
One conflict, in the About view test imports: dev added waitFor, this branch added NextIntlClientProvider for the locale coverage. Both are used, so the resolution keeps both.
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Six findings remain: the full-name visibility control ignores fulfilled save errors; the OneSignal login guard still has cross-id cleanup and joined-failure gaps, with the latter untested; and exact-head unit and formatting checks fail on changed lines. The earlier avatar, opt-in transition, expiry target, and native snapshot claims are fixed or dismissed.
Findings
-
MAJOR · src/components/Profile/components/ShowNameToggle.tsx:23 · Handle fulfilled visibility-save errors
updateUserByIdconverts non-2xx responses and fetch failures into a fulfilled{ error }result, but this chain treats every fulfillment as success. If hiding the full name fails, the controlled toggle and header stay optimistically hidden until a successful refetch corrects them; if that refetch also fails, this screen says the name is private while the public profile remains unchanged. Inspectresult.error, revertonChange, and surface the failure before refetching. -
MINOR · src/hooks/useNotifications.ts:116 · Only clear the matching login guard
If login A is pending and the external id changes to B, the B login replacesloginInFlight; when A settles first, this unconditionalfinallyclears B's guard. A subscription sync before B settles can then start a second B login, recreating the duplicate-registration race this guard is meant to prevent. Clear the guard only when it still refers to this invocation's promise/token. -
MINOR · src/hooks/useNotifications.ts:241 · Test the failed joined-login path
The new dedupe test can only resolve the pending initial login, so it proves the successful join but never exercises the documented retry-after-failure path. Add a case that rejects the first login, fires the false-to-true opt-in while it is pending, and asserts that one retry starts after settlement; that test currently exposes the P13 behavior. -
MINOR · src/i18n/app/messages/es-419.json:286 · Use tuteo in the es-419 confirmation
Exact-headunitfailsglossary.test.tsbecausePodésis es-AR voseo, while the shared es-419 catalog consistently usespuedes. This leaves the required unit gate red; use the es-419 form here and keep voseo in the es-AR overrides. -
MINOR · src/components/Card/CardFace.tsx:184 · Format the expiry copy-button attribute
The newclassNameattribute is left at column zero, and exact-headformatfails Prettier onCardFace.tsx. Apply the repository formatter to this file so the required formatting gate passes. -
MINOR · src/hooks/useNotifications.ts:116 · [moonshotai/kimi-k3] Keep a newer login's in-flight guard intact
The new dedupe guard clears itself unconditionally:finally { loginInFlight = null }. If login for id A is in flight and the user switches accounts, a login for id B starts and overwritesloginInFlight; when A's promise settles, its finally nulls the guard while B's login is still pending. A subscription-change or setExternalId sync for B in that window passes theloginInFlight?.id === idcheck and starts a second concurrentlogin(B)— the same double-record race this code was added to close (duplicate OneSignal records, duplicate notifications). Fix: only clear if the guard still belongs to this login, e.g.if (loginInFlight?.promise === promise) loginInFlight = null. -
MINOR · src/hooks/useNotifications.ts:241 · [claude-opus] Login-retry-after-failure path is untested
syncExternalIdLinkmutates shared state on OneSignal's side (adapter.login(id)links the device subscription to the user), and this diff rewrites exactly that mutation: the in-flight join at src/hooks/useNotifications.ts:106, thefinally { loginInFlight = null }at :116, and the uncommittedlastLinkedExternalIdon failure at :112. The subscription listener now calls it for one stated reason — "this is only a retry for a login that failed there" (src/hooks/useNotifications.ts:235-241). That retry is the one branch neither new test exercises.
Both new tests use login: jest.fn().mockResolvedValue(undefined) / a promise that resolves, so init always succeeds; useNotifications.subscription.test.ts:620 asserts the opposite case (login still called once because the init link stands), and useNotifications.loginDedupe.test.ts only covers joining a pending login. src/hooks/__tests__/useNotifications.test.ts has no rejecting login either — grep for mockRejected across the three files returns nothing.
Untested case, exactly: init's await syncExternalIdLink() (src/hooks/useNotifications.ts:265) rejects with a non-identity error → handleLoginError swallows it, lastLinkedExternalId stays null, loginInFlight is cleared → a later {optedIn: true, previousOptedIn: false} change must call adapter.login a second time and commit lastLinkedExternalId on success. The negative twin also matters and is untested: an identity/verify rejection sets disableExternalIdLogin, so the same opt-in must NOT call login again (src/hooks/useNotifications.ts:105). CONTRIBUTING.md:506 makes this a hard line — "if code moves money or mutates shared state, it needs a test before merge" — and this is the mutation whose duplication caused TASK-22209's duplicate welcome push.
Fix: add one case to useNotifications.loginDedupe.test.ts with login.mockRejectedValueOnce(new Error('network')) for the init call, then fire the false→true change and assert login was called twice; add a second with mockRejectedValue(new Error('identity verification failed')) asserting it is not called again.
Answered by a human, not raised again
These are still present as far as this review can tell. You resolved the thread or deferred the fix, so Chip files them instead of repeating them.
- MINOR · src/hooks/useNotifications.ts:106 · Retry after a joined login fails — no task filed, please file one
Checked and not raised again
- BLOCKING · src/hooks/useCardReveal.ts:105 · [moonshotai/kimi-k3] Native snapshots can still capture card credentials — this review checked it and does not believe it. No task filed.
Checked clean
- Confirmed the detached worktree HEAD, supplied base SHA, merge base, repository, PR number, trusted author, and dev base all match the pinned request.
- Reviewed all 20 changed files across correctness, security/privacy, adversarial failure paths, and slop/maintainability.
- P1 is fixed: the home avatar now seeds from username through HomePage/HomeTopNav, independent of showFullName.
- P2 and P3 are fixed: both OneSignal adapters forward previousOptedIn and the hook acts only on the false-to-true transition; the split-event/reload test passes at this head.
- P6's successful same-id dedupe path is implemented and its exact-head test passes; P7 is the same surviving cleanup race retained as P12.
- P8 is fixed: the expiry button now uses the same 48px pseudo-element hit target and focus/pressed treatment as the adjacent credential controls.
- Card reveal keeps its payload only in memory, covers it on visibilitychange, restores without refetch, and its exact-head tests pass; P4, P5, P9, P10, and P14 are dismissed below.
- The Android default large-icon deletion leaves the status-bar icon resources intact, and the localized About-policy test passes.
- Exact-head CI completed: typecheck, eslint, analyze, native-export, ds-lint, ds-shots, preview, provenance, and review checks passed; unit, format, and their aggregate ci-success failed for the two changed-line findings reported here.
Security review by moonshotai/kimi-k3: 2 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 8cfa3b703f21 · Context: repo · Took 17m
CI: the es-419 confirm copy used voseo, which the glossary rule reserves
for es-AR, and CardFace lost its indentation in the merge. es-419 now
uses tuteo and es-AR carries the voseo override, so es-AR does not
inherit a tuteo string its own rule bans.
Card reveal also covers from the Capacitor app lifecycle: Android
WebViews do not reliably fire visibilitychange, so appStateChange is the
signal there. Screenshots stay allowed; nothing is blocked while active.
The name-visibility save inspected only a rejection, but updateUserById
RESOLVES { error } for a non-2xx or a network failure. A failed save
left the screen claiming the legal name was hidden while it stayed
public. It now reverts and shows the error.
The OneSignal in-flight login guard cleared the tracker unconditionally,
so an older login settling late dropped a newer id's guard and let a
second login for that id start.
There was a problem hiding this comment.
Chip review — changes requested
The profile-save, localization, copy-control, and stale-login-guard fixes are sound. Two defects remain: native snapshot protection still waits for a React repaint, and an opt-in that joins a failing initialization login does not trigger the documented retry.
Findings
-
BLOCKING · src/hooks/useCardReveal.ts:122 · Cover native snapshots before React repaints
With PAN/CVV painted, backgrounding the app delivers appStateChange to JavaScript and this callback only queues setObscured; the WebView and React commit can occur after the OS captures its task-switcher frame, leaving the credentials in Recents. The native event fixes Android's missing visibilitychange path, but not this ordering race. Add a native background-only privacy overlay or recents-screenshot guard in the Activity/AppDelegate lifecycle so active screenshots remain allowed, retain the payload in memory, and verify the transition on both platforms. -
MINOR · src/hooks/useNotifications.ts:106 · Retry a failed login joined during opt-in
Let initialization start adapter.login(id), then deliver the false-to-true subscription event before that login settles. This branch joins the same promise; if login rejects, the inner catch swallows the failure and clears the guard, so the event finishes without starting the documented retry and the device can remain unlinked until another external trigger. Have a joined caller re-check lastLinkedExternalId after settlement (or return a success result and issue one retry), and cover a rejected first login followed by a successful second call. -
BLOCKING · src/hooks/useCardReveal.ts:120 · [moonshotai/kimi-k3] Native snapshots can still capture card credentials
useCardRevealnow keeps the revealed PAN/CVV in React state and merely returnsnullwhenobscuredflips. On Android,appStateChangeis delivered to JS after (or concurrently with) the OS compositing the task-switcher snapshot, and on iOS the webview snapshot can be taken before the React re-render commits — so the screenshot the OS keeps in recents can still contain the full PAN/CVV, exactly the leak this hook is meant to prevent. The code comment even acknowledges it ('a JS cover races the OS snapshot'). The practical fix is the platform-level guard the comment itself names: FLAG_SECURE on the activity (Android) or a native privacy overlay onapplicationWillResignActive(iOS), not a JS state flag. -
MINOR · src/hooks/useNotifications.ts:113 · [claude-opus] Untested: older login resolving after the newer one already committed
syncExternalIdLink guards the guard against a late-settling older login (if (loginInFlight?.token === token) loginInFlight = null, line 119) but not the commit (lastLinkedExternalId = id, line 113). The new loginDedupe test builds exactly this race and stops one step short: it leaves user-2's login pending forever, so the join at line 106 masks the commit.
Untested case: login('user-1') in flight → account switches → login('user-2') issued → user-2's login resolves first (sets lastLinkedExternalId='user-2', clears the guard) → user-1's login resolves second and overwrites lastLinkedExternalId='user-1' while currentExternalId is 'user-2'. Bookkeeping now names an account the device is not linked to, and the next setExternalId/opt-in sync sees lastLinkedExternalId !== id and issues a redundant login('user-2') — the extra login this PR exists to stop.
Fix: gate the success commit the same way as the cleanup — commit only when this call still owns the tracker (if (loginInFlight?.token === token) lastLinkedExternalId = id, or check currentExternalId === id) — and extend the existing test by resolving pendingLogins.get('user-2') before pendingLogins.get('user-1'), then asserting no third login() after a subsequent opt-in.
Checked clean
- Confirmed the detached worktree head, supplied base SHA and merge base, trusted author, dev base ref, and PR metadata.
- Reviewed the complete base-to-head diff across Android notification artwork, the home avatar, About localization, card reveal/copy behavior, profile name visibility, and both OneSignal adapters.
- Ran correctness, security, adversarial, and slop passes and reconciled all supplied P1-P21 findings against the current implementation and tests.
- Exact-head ci-success, unit, typecheck, eslint, format, native-export, ds-shots, analyze, and Deploy-Preview checks completed successfully.
- git diff --check is clean. The detached worktree has no node_modules, so no duplicate local Jest run was attempted.
- Compared the card lifecycle handling with the repository's existing native app-state patterns, including its documented non-replayed event behavior.
Security review by moonshotai/kimi-k3: 1 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 5a77f1af0db3 · Context: repo, engineering · Took 13m
…hat failed Two holes left in the in-flight login guard. The success commit was not gated like the cleanup, so an older login settling after a newer one overwrote the bookkeeping with an account the device had left, and the next sync then issued a redundant login. And a caller that joined a pending login saw no error of its own, so a failed login left the device unlinked with no retry — every push for that user reaching nobody. The commit now happens only while the call still owns the tracker, and a joined caller re-checks after settlement and retries once.
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
The exact-head changes resolve the supplied avatar, notification, card, profile, localization, and expiry-copy findings. The repeated native-snapshot objections are dismissed because this head adds the available lifecycle cover, preserves dev's existing snapshot timing posture, and the supplied product decision rejects app-wide screenshot blocking.
Checked and not raised again
- MAJOR · src/hooks/useCardReveal.ts:120 · [moonshotai/kimi-k3] Native snapshots can still capture card credentials — this review checked it and does not believe it. No task filed.
Checked clean
- Confirmed the detached worktree head, supplied base SHA and merge base, trusted author, dev base ref, and PR metadata match the requested target.
- Reviewed the complete base-to-head diff across the Android OneSignal resource, card reveal and copy UI, profile name visibility, localization coverage, notification hook, and both OneSignal adapters.
- Verified the false-to-true subscription transition against both adapter event shapes and the regression test covering opt-in, token assignment, id assignment, refresh, opt-out, and re-subscribe.
- Verified same-id login joining, retry after a joined login failure, matching-token cleanup, and the late older-login bookkeeping case against the implementation and focused tests.
- Checked the card background cover against DOM visibility and Capacitor appStateChange paths, and compared its native snapshot timing posture with the supplied base.
- Checked the full-name confirmation, optimistic error rollback, locale copy, expiry MM/YY copy behavior, expanded hit target, deleted Android drawable references, and diff whitespace.
- Exact-head CI completed successfully across unit, typecheck, eslint, format, native export, design-system lint and screenshots, analysis, deployment, provenance, and aggregate reporting.
Security review by moonshotai/kimi-k3: 1 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: c061b17bd8e0 · Context: repo, github_ci, provider_docs · Took 14m
Summary
Bundle of native bug fixes for today's release (Aleks). Each fix is one commit; the list below grows as fixes land.
1. Duplicate Peanut artwork in every Android notification — TASK-22125
Every Android push showed the mascot twice: the launcher icon on the left, and the same mascot on the right as the OneSignal default large icon (
ic_onesignal_large_icon_default, added in e8e5bf2). This removes the default. The right slot now stays empty unless the API sends contextual art for that push (large_icon/ios_attachments— peanut-api-ts PR, see below). Title, body, app icon, tap behavior and the status-bar silhouette (ic_stat_onesignal_default) are unchanged.iOS never had the duplicate (app icon only); it gains the contextual image through the existing NotificationServiceExtension when the API sends one.
Task: TASK-22125
Pairs with: peanut-api-ts
native-release-bug-fixes-aleks(contextual push image plumbing + badge art). No deploy order: a push withlarge_iconoverrides the default on old builds; a push without one keeps the duplicate on old builds until the store update.Screenshots — fix 1
The "after" is a mockup edited from the same capture (right-side mascot removed) — no Android build was produced in this session, so replace it with a device capture from the release build if you want the real thing. Full-size: before · after mockup. Assets live on the
pr-assets-2924branch; delete it after merge.2. Own avatar shows the first letter of the username, not the generated face
The design-system avatar (
DotFaceAvatar, 497ab2a) draws a generated smiley on the home chip and the self profile header; it reads as a random emoji. Regressed to the initials avatar (AvatarWithBadge) in a newfirstLetterOnlymode for the user's own identity — home chip 32px, profile headerlarge. Contacts and public profiles keep two-letter initials.DotFaceAvatar, its test and theuserAvatarAltcatalog key are deleted; avatar v2 can recover the face from 497ab2a.Screenshots — fix 2 (375px,
?__fixture=screens)Full profile screen: after. The
ds-shotsjob diffshomeandprofileagainst the dev baseline for the pixel-level before/after.3. About Peanut: policy titles follow the app language — TASK-22146
About Peanut listed every policy title in English whatever the app language. All eight titles now come from the catalog (
profile.about.policies), reusing the landing footer's approved Spanish and Portuguese strings; "Security Disclosure" is the one new translation (es-AR inherits es-419). The seven legal hrefs are bare paths, so every language opens the same English documents; the Security Disclosure help link is locale-targeted byDocsLinkas before (the article exists in es-419 and pt-br). Test:About.view.test.tsxrenders the view in all three locales and asserts titles + hrefs.Task: TASK-22146
Reporter: Crisp session
cf826101(evidence1000062419.png)Screenshots — fix 3 (375px,
/profile/about?__fixture=profile)Every title changes; the legal hrefs are unchanged.
4. One welcome push after enabling notifications, not two — TASK-22209
A user reported two welcome messages right after enabling notifications (Crisp
session_ccd9e832, iOS PWA). Our backend sends nothing on opt-in; the welcome is OneSignal's own, and OneSignal sends it once per push-subscription record. The SDK fires the subscriptionchangeevent twice for one opt-in (token registered, then the server-assigned id), anduseNotificationscalledlogin()on every change. That second login raced OneSignal's own subscription create and re-registered the half-created subscription under the user as a second record — a second welcome. PostHog confirms the double fire: 163 of the 190 users who granted permission in the last 14 days emittednotification_subscribedexactly twice, the reporter's two events 257 ms apart.Both adapters now forward the SDK's previous
optedIn, and the listener counts a new opt-in on the single false → true transition: onenotification_subscribedcapture, one modal hide, and the user link goes through the idempotentsyncExternalIdLink(a retry for a login that failed at init, a no-op otherwise). The token and id-assignment events and a reload token refresh report the same subscription and do nothing; a real re-subscribe still counts. Test:useNotifications.subscription.test.tsdrives the registered callback with the SDK-shaped events (opt-in flips without a token, token registers, id assigned, token refreshed, opt-out, opt back in) and asserts one capture per real opt-in and no second login.Task: TASK-22209
Reporter: Crisp session
ccd9e8325. Card details stay revealed across an app switch; the expiry gets its own copy button
Revealing the card and switching to the merchant app to paste the number fired
blur/visibilitychange, anduseCardRevealre-masked the card on both. The user came back to a masked card and a rate-limited re-reveal. The listener is deleted; the 30 s timeout auto-mask is the only remaining safety net. The expiry (MM/YY) had no copy button while the PAN and CVV had one; it now has its own, with acard.yourCard.expiryCopiedtoast in the three catalogs. Tests:useCardReveal.test.tsfiresvisibilitychange+blurand asserts the details stay;CardFace.test.tsxclicks the expiry button and asserts('12/30', 'expiry').Screenshots — fix 5 (375px, revealed card)
Rendered from
CardFacewith a fixed reveal payload (no backend), samepr-assets-2924branch as fix 1.6. Personal details follows the name-visibility setting, and asks before making the name public — TASK-22273
Edit Profile drew its header from the full name whatever "Show my full name" said, so a user who turned the setting off still saw their legal name on the screen that owns the setting. The header now shows what everyone else sees: the full name only while it is public, the username otherwise.
The toggle becomes controlled, so the screen owns the value and the header reacts to a tap immediately. Before, the header waited for the background user refetch, which is what forced the extra reload the reporter described.
Turning the setting on publishes the legal name next to the username, so it now asks first through
ActionModal(warning tone, eye icon, purple Confirm and stroke Cancel). Cancelling leaves it off. Turning it off takes nothing away and still saves straight away. New copyprofile.showFullNameConfirm.{title,description}in the three catalogs. Test:ShowNameToggle.test.tsxcovers the confirm gate, cancel, the un-gated off path and the revert on a failed save.Task: TASK-22273
Screenshots:
Risks / breaking changes
dev(DS initials); only the home-chip seeding changes.dev(ui#2925); this PR adds a test only.OneSignalAdapter.onSubscriptionChangecontract changes from a boolean to{ optedIn, previousOptedIn }(both adapters and the one consumer updated). The user link still happens at init and on user change, the subscription listener only retries it. Analytics:notification_subscribeddrops to one event per opt-in, so its 14-day count roughly halves — that is the fix, not a regression.ShowNameTogglechanges from self-contained to controlled (checked/onChange); its one consumer is updated. No API change.QA
badge.unlockedpush with a png badge (offramp_user,peanut_shaper) shows the badge on the right.notification_subscribed.MM/YY.