Skip to content

Fix iOS notification deep link to post (read trigger.payload)#264

Merged
nyomanjyotisa merged 1 commit into
mainfrom
claude/fix-ios-notification-deep-link
Jun 8, 2026
Merged

Fix iOS notification deep link to post (read trigger.payload)#264
nyomanjyotisa merged 1 commit into
mainfrom
claude/fix-ios-notification-deep-link

Conversation

@nyomanjyotisa

@nyomanjyotisa nyomanjyotisa commented Jun 8, 2026

Copy link
Copy Markdown
Member

Summary

Tapping a Gumroad push notification on iOS opened the home tab instead of the linked post (follow-up to the cold-start work in #258).

Root cause: for remote pushes on iOS, expo-notifications leaves request.content.data null and places the APNs payload (installment_id, purchase_id, …) under request.trigger.payload. The handler only read content.data, so it never found installment_id and fell back to the default tab. (Verified against the real backend payload in gumroadpost_sendgrid_api.rb sends those keys top-level on iOS / in the FCM data field on Android.)

Fix: consumeNotificationRoute now reads the payload from both request.content.data (Android FCM data field / Expo push) and request.trigger.payload (iOS remote), so the route resolves on both platforms. Added Sentry breadcrumbs at the cold-start and parse sites to surface the payload shape if a future notification ever fails to route.

Changes

  • components/use-push-notifications.ts — read payload from content.data and trigger.payload; breadcrumb when no route is built.
  • app/index.tsx — breadcrumb recording cold-start response/route.
  • tests/components/use-push-notifications.test.ts — cover the iOS trigger.payload shape, the Android FCM payload (with tag/message ignored), and multi-notification routing.

Test plan

Verified end-to-end on iOS simulator and Android emulator against production data:

  • Cold-start tap → opens /post/{installment_id} (not the home tab).
  • Installment content loads (200) once purchase_id is included (matches the real push payload).
  • Android: multiple distinct notifications stack as separate tray entries; tapping one routes correctly and leaves the others (complements the backend tag fix Set unique tag per Android push to stop tray collapse gumroad#5289).
  • tsc --noEmit, expo lint, and jest (200/200) all pass.

🤖 Generated with Claude Code


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.


Note

Low Risk
Narrow change to notification routing and observability; Android behavior stays on content.data first, with no auth or data-model changes.

Overview
Fixes iOS push notification deep links so tapping a notification opens the linked post instead of the default home tab.

consumeNotificationRoute now resolves routing data from request.content.data (Android / Expo) and request.trigger.payload (iOS remote APNs), trying each until a /post/{installment_id} route can be built. When parsing fails, it records a Sentry warning breadcrumb with both payload shapes; cold-start routing in app/index.tsx adds an info breadcrumb for the last response and resolved route.

Tests cover the iOS trigger.payload shape, Android FCM payloads with extra tag/message fields, and routing multiple distinct notifications.

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

Tapping a Gumroad push notification on iOS opened the home tab instead
of the linked post. The cause was where the payload is read: for remote
pushes on iOS, expo-notifications leaves content.data null and puts the
APNs payload (installment_id, purchase_id, ...) under
request.trigger.payload. The handler only read content.data, so it never
found installment_id and fell back to the default tab.

consumeNotificationRoute now reads the payload from both
request.content.data (Android FCM data field / Expo push) and
request.trigger.payload (iOS remote), so the route resolves on both
platforms. Adds Sentry breadcrumbs at the cold-start and parse sites to
surface the payload shape if a future notification fails to route.

Verified end-to-end on the iOS simulator and Android emulator against
production data: tapping the notification opens /post/{id} and the
installment content loads (200) once purchase_id is included.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nyomanjyotisa nyomanjyotisa self-assigned this Jun 8, 2026
@nyomanjyotisa nyomanjyotisa merged commit bfabfef into main Jun 8, 2026
2 checks passed
@nyomanjyotisa nyomanjyotisa deleted the claude/fix-ios-notification-deep-link branch June 8, 2026 10:28
@greptile-apps

greptile-apps Bot commented Jun 8, 2026

Copy link
Copy Markdown

Greptile Summary

Fixes iOS push notification deep-linking by reading the APNs payload from request.trigger.payload in addition to request.content.data, which expo-notifications leaves null for remote iOS pushes. Sentry breadcrumbs are added at both the cold-start routing site and the no-route fallback to ease future debugging.

  • consumeNotificationRoute now iterates [content.data, trigger.payload] and uses the first payload that resolves to a post route, covering both Android FCM and iOS APNs delivery paths.
  • app/index.tsx records a cold-start breadcrumb (response presence + resolved route) after calling consumeNotificationRoute.
  • New tests exercise the iOS trigger-payload shape, Android FCM extra fields (tag, message), and multi-notification dedup by distinct identifiers.

Confidence Score: 4/5

Safe to merge — the change is well-scoped to notification routing with no mutations to shared state or auth paths.

The dual-payload fallback logic is correct and the new tests cover the iOS and Android cases. Two minor concerns: the Sentry mock is missing addBreadcrumb, so the new warning breadcrumbs are silently swallowed rather than asserted on in tests; and the type cast makes content optional on NotificationRequest even though the SDK guarantees it is always present, which could mask future type errors.

The type cast in components/use-push-notifications.ts (lines 92–96) and the Sentry mock in the test file deserve a second look before merge.

Important Files Changed

Filename Overview
components/use-push-notifications.ts Core fix: consumeNotificationRoute now reads both content.data (Android/Expo) and trigger.payload (iOS APNs) to resolve the post route; adds a Sentry warning breadcrumb when neither yields a route. Type cast broadens content to optional, which may mask future SDK-type errors.
app/index.tsx Adds a Sentry info breadcrumb recording the cold-start notification response presence and resolved route — purely additive observability with no logic changes.
tests/components/use-push-notifications.test.ts New tests cover iOS trigger-payload routing, Android FCM extra fields, and multi-notification dedup. Sentry mock omits addBreadcrumb, so the new breadcrumb paths are exercised via optional chaining but never asserted on.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Notification tapped] --> B[consumeNotificationRoute]
    B --> C{response is null?}
    C -- Yes --> D[return null]
    C -- No --> E{identifier already handled?}
    E -- Yes --> D
    E -- No --> F[payloads = content.data + trigger.payload]
    F --> G[Try content.data first]
    G --> H{installment_id present?}
    H -- Yes --> I[buildNotificationRoute]
    I --> J[Add to handledIdentifiers]
    J --> K[return route]
    H -- No --> L[Try trigger.payload - iOS APNs]
    L --> M{installment_id present?}
    M -- Yes --> I
    M -- No --> N[Sentry warning breadcrumb]
    N --> D
Loading

Comments Outside Diff (1)

  1. tests/components/use-push-notifications.test.ts, line 34 (link)

    P2 Sentry addBreadcrumb not mocked — new breadcrumbs go untested

    The Sentry mock exports only captureException, so Sentry.addBreadcrumb is undefined in every test. consumeNotificationRoute now calls Sentry.addBreadcrumb?.({...}) when no route is found (e.g. "returns null when the data has no installment_id"), and app/index.tsx does the same on every cold-start. Because optional chaining silently returns undefined for a missing property, no test currently verifies that the breadcrumbs are actually recorded with the expected fields. Adding addBreadcrumb: jest.fn() to the mock and asserting on it in the no-route and cold-start tests would close this gap.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "Fix iOS notification deep link to post (..." | Re-trigger Greptile

Comment on lines +92 to +96
const request = response.notification.request as {
identifier: string;
content?: { data?: Record<string, any> | null };
trigger?: { payload?: Record<string, any> | null } | null;
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Overly-broad type cast makes content optional when the SDK guarantees it is always present

The cast widens request to a custom interface where content is optional (content?:). In the expo-notifications SDK, NotificationRequest.content is always defined — making it optional in the cast can mask TypeScript errors if future code accesses request.content without null-guarding. A safer approach is to cast only the trigger sub-field so the rest of the SDK-provided typing is preserved.

Suggested change
const request = response.notification.request as {
identifier: string;
content?: { data?: Record<string, any> | null };
trigger?: { payload?: Record<string, any> | null } | null;
};
const request = response.notification.request;
const trigger = request.trigger as { payload?: Record<string, any> | null } | null | undefined;

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant