Skip to content

chore: test foreground push notifications#32100

Open
kirillzyusko wants to merge 3 commits into
mainfrom
fix/foreground-push-notifications
Open

chore: test foreground push notifications#32100
kirillzyusko wants to merge 3 commits into
mainfrom
fix/foreground-push-notifications

Conversation

@kirillzyusko

@kirillzyusko kirillzyusko commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Description

Changelog

CHANGELOG entry:

Related issues

Fixes:

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Before

After

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Medium Risk
Changes FCM bootstrap (setBackgroundMessageHandler in index.js) and core notification handling paths; the visible debug UI may ship to all users unless removed or gated before merge.

Overview
Adds local FCM push instrumentation so engineers can trace token registration, foreground handling, background/headless delivery, and notification-open flows while testing pushes.

A new pushDebugLog helper persists up to 100 typed events in AsyncStorage via logPushEvent, with safe no-op error handling. FCMService and index.js now emit events at key points (e.g. onMessage, parse/display/drop paths, token creation, opened-from-killed/background). A PushDebugPanel on the Notifications screen lists entries with refresh/clear and expandable JSON detail.

Note: The debug panel is wired unconditionally into the Notifications view (no __DEV__ gate in this diff).

Reviewed by Cursor Bugbot for commit 4eb4ec4. Bugbot is set up for automated code reviews on this repo. Configure here.

@kirillzyusko kirillzyusko self-assigned this Jun 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@github-actions github-actions Bot added the pr-not-ready-for-e2e Skip E2E and block merging. Remove this label once the PR is ready to run the E2E tests. label Jun 19, 2026
@mm-token-exchange-service

Copy link
Copy Markdown

PR template — items to address before "Ready for review"

Blocking — these items fail the workflow until fixed:

  • Changelog section has an empty CHANGELOG entry: line. Fill in a description, write CHANGELOG entry: null, or apply the no-changelog label.

Warnings — informational, address before merging:

  • Description section is empty. Describe what changed and why.
  • Related issues section is empty. Add Fixes: #123 / Closes: <URL> / Refs: <Jira key>, or write a short rationale after the colon.
  • Manual testing steps still contain template content (the Gherkin example title or a [...] placeholder). Replace with real steps, or write N/A — <reason>.
  • Screenshots/Recordings section is empty. Add an image/video for user-facing changes, logs/console output for non-user-facing changes, or write N/A if no evidence is applicable.
  • Pre-merge author checklist has unchecked items (e.g. "I've followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards."). Every box must be consciously checked — see docs/readme/ready-for-review.md.

See docs/readme/ready-for-review.md for the full Definition of Ready for Review.

@kirillzyusko kirillzyusko marked this pull request as ready for review June 19, 2026 15:43
@kirillzyusko kirillzyusko requested a review from a team as a code owner June 19, 2026 15:43
},
]}
/>
<PushDebugPanel />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug panel ships in production

Medium Severity

PushDebugPanel is always rendered on the notifications screen with no __DEV__, feature flag, or internal-only guard. Production users get a developer push debug strip (Refresh/Clear, internal FCM event types) above the notification list, which matches temporary test tooling rather than end-user UI.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3c1f9a3. Configure here.

Comment thread index.js
dataKeys: Object.keys(remoteMessage.data ?? {}),
},
);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Background handler drops debug writes

Medium Severity

The new setBackgroundMessageHandler calls logPushEvent without await. In headless/background Android contexts the handler promise can finish before AsyncStorage read/write completes, so FCM_BACKGROUND_RECEIVED entries may never persist—the main signal this PR adds for killed/background FCM.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3c1f9a3. Configure here.

@github-actions github-actions Bot added the risk:medium AI analysis: medium risk label Jun 19, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4eb4ec4. Configure here.

logPushEvent(
'FCM_FOREGROUND_PROCESSING',
`Foreground notification parsed: ${notification?.id}`,
{ id: notification?.id, type: (notification as { type?: string })?.type },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handler runs without notification

Medium Severity

Optional chaining on the foreground debug logs stops a throw when processNotification returns a nullish value. The flow still calls handler with that value afterward, whereas before the log line’s notification.id access threw and skipped the handler inside the existing catch path.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4eb4ec4. Configure here.

@kirillzyusko kirillzyusko removed the pr-not-ready-for-e2e Skip E2E and block merging. Remove this label once the PR is ready to run the E2E tests. label Jun 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: None (no tests recommended)
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: low
  • AI Confidence: 82%
click to see 🤖 AI reasoning details

E2E Test Selection:
The PR introduces a push notification debug panel and logging infrastructure:

  1. PushDebugPanel.tsx (new): A debug UI component unconditionally rendered in the Notifications view. It reads from AsyncStorage to display FCM event logs. While it's always rendered, it's a passive display component that doesn't affect notification logic.

  2. pushDebugLog.ts (new): A pure debug logging utility using AsyncStorage. No impact on core wallet functionality.

  3. FCMService.ts: Only additive logPushEvent calls added around existing notification handling logic. The core notification processing logic is unchanged — no conditional branches altered, no return values changed.

  4. Notifications/index.tsx: Adds <PushDebugPanel /> to the view. This could affect the Notifications screen layout but doesn't change notification delivery or processing.

  5. index.js: Adds a setBackgroundMessageHandler that only logs via logPushEvent. The handler doesn't process or modify notifications — it just logs debug info.

Why no E2E tags are selected:

  • No existing E2E test tags cover push notification delivery or the Notifications view specifically
  • The changes are purely additive debug logging — no core wallet flows (accounts, confirmations, swaps, staking, identity sync, network management, etc.) are affected
  • The FCM notification processing logic itself is unchanged
  • The PushDebugPanel is a passive read-only debug component
  • No controllers, Engine, or critical wallet infrastructure is modified
  • The changes don't affect any user-facing flows that existing E2E tests cover (onboarding, account management, transactions, etc.)

The risk is low as these are debug instrumentation additions that don't alter any functional behavior.

Performance Test Selection:
The changes are debug logging additions to the push notification system. They add AsyncStorage reads/writes for debug logging, but these are fire-and-forget operations that don't affect any measured performance flows (app launch, login, onboarding, swaps, asset loading, account list, perps, predictions). The PushDebugPanel reads from AsyncStorage on mount in the Notifications view, but this is not a performance-sensitive path covered by any performance test tag.

View GitHub Actions results

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
58.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:medium AI analysis: medium risk size-M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant