Conversation
📝 WalkthroughWalkthroughImplements retriable, deduplicated push-registration with fingerprinting and Sentry breadcrumbs; adds probe-aware Sentry filters for server-action, malformed next-action, and web-streams probes and wires them into beforeSend; expands API error handling to support structured errors and retry options; updates related tests and a wave container ref stability change. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Capacitor as Capacitor PushNotifications
participant Backend as Backend Registration
participant Sentry as Sentry
App->>Capacitor: requestRegistration(fingerprint)
activate Capacitor
Capacitor->>Backend: POST /register (fingerprint)
activate Backend
alt 429 (Rate Limited)
Backend-->>Capacitor: 429 + Retry-After
Capacitor-->>App: error with retry metadata
App->>Sentry: addBreadcrumb("rate-limit")
App->>App: schedule retry (computePushRegistrationRetryDelayMs)
else Transient Error
Backend-->>Capacitor: 5xx / network error
Capacitor-->>App: error
App->>Sentry: captureException(tag="transient")
App->>App: retry according to transient rules
else Success
Backend-->>Capacitor: 200 + token
Capacitor-->>App: success
App->>App: cache lastSuccessfulRegistrationRef(fingerprint)
App->>Sentry: addBreadcrumb("registration-success")
end
deactivate Backend
deactivate Capacitor
sequenceDiagram
participant Event as Sentry Event
participant Tagger as tagSecurityProbes
participant Filter1 as filterServerActionProbeErrors
participant Filter2 as filterMalformedNextActionProbeErrors
participant Filter3 as filterWebStreamsProbeErrors
participant Sanitizer as sanitizeSentryEvent
participant Sentry as Sentry Backend
Event->>Tagger: beforeSend(event)
activate Tagger
Tagger-->>Filter1: taggedEvent
deactivate Tagger
Filter1->>Filter1: is probe-like?
alt probe-like
Filter1-->>Sentry: null (suppress)
else not probe-like
Filter1-->>Filter2: continue
Filter2->>Filter2: malformed next-action?
alt malformed
Filter2-->>Sentry: null (suppress)
else
Filter2-->>Filter3: continue
Filter3->>Filter3: web-streams probe?
alt probe-like
Filter3-->>Sentry: null (suppress)
else
Filter3-->>Sanitizer: continue
Sanitizer-->>Sentry: send sanitized event
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e770a83af9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
config/sentryProbes.ts (1)
170-206: SplitgetHeaderValueinto shape-specific helpers to reduce complexity.This block already tripped the cognitive-complexity threshold; extracting array/object parsing keeps behavior intact and lowers maintenance cost.
♻️ Refactor sketch
+function getHeaderValueFromTupleEntries( + headers: unknown[], + normalizedTarget: string +): string | undefined { + for (const entry of headers) { + if (!Array.isArray(entry) || entry.length < 2) continue; + const key = entry[0]; + const value = entry[1]; + if ( + typeof key === "string" && + key.toLowerCase() === normalizedTarget && + typeof value === "string" + ) { + return value; + } + } + return undefined; +} + +function getHeaderValueFromRecord( + headers: Record<string, unknown>, + normalizedTarget: string +): string | undefined { + for (const [key, value] of Object.entries(headers)) { + if (key.toLowerCase() !== normalizedTarget) continue; + return typeof value === "string" ? value : undefined; + } + return undefined; +} + function getHeaderValue( headers: unknown, headerName: string ): string | undefined { const normalizedTarget = headerName.toLowerCase(); if (Array.isArray(headers)) { - for (const entry of headers) { - if (!Array.isArray(entry) || entry.length < 2) { - continue; - } - const key = entry[0]; - const value = entry[1]; - if ( - typeof key === "string" && - key.toLowerCase() === normalizedTarget && - typeof value === "string" - ) { - return value; - } - } - return undefined; + return getHeaderValueFromTupleEntries(headers, normalizedTarget); } - if (!isRecord(headers)) { - return undefined; + if (isRecord(headers)) { + return getHeaderValueFromRecord(headers, normalizedTarget); } - for (const [key, value] of Object.entries(headers)) { - if (key.toLowerCase() !== normalizedTarget) { - continue; - } - return typeof value === "string" ? value : undefined; - } - return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/sentryProbes.ts` around lines 170 - 206, getHeaderValue is doing two shape-specific parsing flows (array and record) in one function which raised cognitive complexity; extract the array-handling and record-handling logic into two small helpers (e.g., getHeaderValueFromArray(headers: unknown[], headerName: string): string | undefined and getHeaderValueFromRecord(headers: Record<string, unknown>, headerName: string): string | undefined) and have getHeaderValue call them: if Array.isArray(headers) -> getHeaderValueFromArray, else if isRecord(headers) -> getHeaderValueFromRecord, otherwise return undefined; preserve all current checks (entry length, string key/value checks, case-insensitive compare using headerName.toLowerCase()) and return semantics exactly as implemented.sentry.edge.config.ts (1)
44-69: Consider extracting the sharedbeforeSendprobe pipeline.This sequence now duplicates
sentry.server.config.tsalmost line-for-line, which increases drift risk during future tuning.♻️ Suggested extraction pattern
+// config/sentryEventFilters.ts +import type { Event, EventHint } from "@sentry/nextjs"; +import { + filterMalformedNextActionProbeErrors, + filterServerActionProbeErrors, + filterTunnelRouteErrors, + filterWebStreamsProbeErrors, + tagSecurityProbes, +} from "@/config/sentryProbes"; + +export function applyProbeFilters<T extends Event>(event: T, hint?: EventHint): T | null { + const tunnelFiltered = filterTunnelRouteErrors(event, hint); + if (tunnelFiltered === null) return null; + + const tagged = tagSecurityProbes(tunnelFiltered); + const actionFiltered = filterServerActionProbeErrors(tagged); + if (actionFiltered === null) return null; + + const malformed = filterMalformedNextActionProbeErrors(actionFiltered); + if (malformed === null) return null; + + return filterWebStreamsProbeErrors(malformed); +}- beforeSend(event: Sentry.ErrorEvent, hint: Sentry.EventHint) { - const tunnelFiltered = filterTunnelRouteErrors(event, hint); - ... - return sanitizeSentryEvent(webStreamsFiltered); - }, + beforeSend(event: Sentry.ErrorEvent, hint: Sentry.EventHint) { + const filtered = applyProbeFilters(event, hint); + if (filtered === null) return null; + return sanitizeSentryEvent(filtered); + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sentry.edge.config.ts` around lines 44 - 69, The beforeSend pipeline in sentry.edge.config.ts duplicates logic from sentry.server.config.ts; extract the shared sequence into a single exported helper (e.g., buildProbeBeforeSend or runProbePipeline) that composes filterTunnelRouteErrors, tagSecurityProbes, filterServerActionProbeErrors, filterMalformedNextActionProbeErrors, filterWebStreamsProbeErrors, and sanitizeSentryEvent so both sentry.edge.config.ts and sentry.server.config.ts import and call that helper from their beforeSend functions to avoid duplication and drift.__tests__/components/notifications/NotificationsContext.test.tsx (2)
241-244: Mock the rate-limit failure as a string too (runtime-realistic path).
commonApiPostcan reject HTTP failures as plain strings, so this test should also cover string-based rate-limit errors to validate real retry classification behavior.Suggested diff
- const rateLimitError = new Error("Rate limit exceeded. Try again in 1 sec"); + const rateLimitError = "Too Many Requests. Try again in 1 sec";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/notifications/NotificationsContext.test.tsx` around lines 241 - 244, The test currently mocks a rate-limit error as an Error object (rateLimitError) but commonApiPost can also reject with plain strings at runtime, so update the test around commonApiPost and setupRegistrationCallback to also cover the string-rejection path: add an additional mock (or replace the mock) so commonApiPost.mockRejectedValue / mockRejectedValueOnce returns the string "Rate limit exceeded. Try again in 1 sec" (in addition to or instead of the Error instance) and assert that the registrationCallback retry classification behaves the same; reference commonApiPost and setupRegistrationCallback when making this change.
246-253: UseglobalThisfor timer spies.Switching from
globaltoglobalThisis more idiomatic and avoids environment-specific lint warnings.Suggested diff
- const setTimeoutSpy = jest.spyOn(global, "setTimeout").mockImplementation((( + const setTimeoutSpy = jest.spyOn(globalThis, "setTimeout").mockImplementation((( handler: TimerHandler ) => { if (typeof handler === "function") { handler(); } return 0 as unknown as NodeJS.Timeout; - }) as typeof global.setTimeout); + }) as typeof globalThis.setTimeout);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/notifications/NotificationsContext.test.tsx` around lines 246 - 253, Replace the environment-specific global reference when spying timers: change the jest.spyOn call that creates setTimeoutSpy from using global to globalThis so the spy is environment-agnostic (update the expression jest.spyOn(global, "setTimeout") to jest.spyOn(globalThis, "setTimeout") and keep the current mockImplementation/cast intact for setTimeoutSpy).__tests__/components/waves/drops/WaveDropsReverseContainer.test.tsx (1)
46-51: PreferglobalThisoverwindowin test mocks.Using
globalThis.requestAnimationFramemakes this setup more runtime-agnostic and aligns with the static-analysis warning.Suggested diff
- jest - .spyOn(window, "requestAnimationFrame") + jest + .spyOn(globalThis, "requestAnimationFrame") .mockImplementation((cb: any) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/drops/WaveDropsReverseContainer.test.tsx` around lines 46 - 51, Update the test's spy to use globalThis instead of window: change the jest.spyOn target from window to globalThis (i.e., jest.spyOn(globalThis, "requestAnimationFrame")...) and keep the same mockImplementation callback and return value so the test remains behaviorally identical; ensure the spy references requestAnimationFrame so static analysis warnings are resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/notifications/NotificationsContext.tsx`:
- Around line 127-144: The code assumes errors are object-shaped
(extractErrorStatusCode / extractRetryAfterMs) but commonApiPost in
services/api/common-api.ts rejects non-2xx responses as plain strings, so update
commonApiPost to reject with an Error-like object that preserves status and
headers (e.g., throw or reject { message, status: response.status, headers:
response.headers } or an Error instance augmented with status and headers) so
extractErrorStatusCode and extractRetryAfterMs can read status/response.status
and headers; alternatively, if you prefer changing the extractors, extend
extractErrorStatusCode and extractRetryAfterMs to detect string rejections from
commonApiPost and parse/unwrap an attached metadata object (status/headers) —
ensure the unique symbols mentioned (commonApiPost, extractErrorStatusCode,
extractRetryAfterMs) are updated accordingly so retry classification and
Retry-After backoff receive the status and header info.
---
Nitpick comments:
In `@__tests__/components/notifications/NotificationsContext.test.tsx`:
- Around line 241-244: The test currently mocks a rate-limit error as an Error
object (rateLimitError) but commonApiPost can also reject with plain strings at
runtime, so update the test around commonApiPost and setupRegistrationCallback
to also cover the string-rejection path: add an additional mock (or replace the
mock) so commonApiPost.mockRejectedValue / mockRejectedValueOnce returns the
string "Rate limit exceeded. Try again in 1 sec" (in addition to or instead of
the Error instance) and assert that the registrationCallback retry
classification behaves the same; reference commonApiPost and
setupRegistrationCallback when making this change.
- Around line 246-253: Replace the environment-specific global reference when
spying timers: change the jest.spyOn call that creates setTimeoutSpy from using
global to globalThis so the spy is environment-agnostic (update the expression
jest.spyOn(global, "setTimeout") to jest.spyOn(globalThis, "setTimeout") and
keep the current mockImplementation/cast intact for setTimeoutSpy).
In `@__tests__/components/waves/drops/WaveDropsReverseContainer.test.tsx`:
- Around line 46-51: Update the test's spy to use globalThis instead of window:
change the jest.spyOn target from window to globalThis (i.e.,
jest.spyOn(globalThis, "requestAnimationFrame")...) and keep the same
mockImplementation callback and return value so the test remains behaviorally
identical; ensure the spy references requestAnimationFrame so static analysis
warnings are resolved.
In `@config/sentryProbes.ts`:
- Around line 170-206: getHeaderValue is doing two shape-specific parsing flows
(array and record) in one function which raised cognitive complexity; extract
the array-handling and record-handling logic into two small helpers (e.g.,
getHeaderValueFromArray(headers: unknown[], headerName: string): string |
undefined and getHeaderValueFromRecord(headers: Record<string, unknown>,
headerName: string): string | undefined) and have getHeaderValue call them: if
Array.isArray(headers) -> getHeaderValueFromArray, else if isRecord(headers) ->
getHeaderValueFromRecord, otherwise return undefined; preserve all current
checks (entry length, string key/value checks, case-insensitive compare using
headerName.toLowerCase()) and return semantics exactly as implemented.
In `@sentry.edge.config.ts`:
- Around line 44-69: The beforeSend pipeline in sentry.edge.config.ts duplicates
logic from sentry.server.config.ts; extract the shared sequence into a single
exported helper (e.g., buildProbeBeforeSend or runProbePipeline) that composes
filterTunnelRouteErrors, tagSecurityProbes, filterServerActionProbeErrors,
filterMalformedNextActionProbeErrors, filterWebStreamsProbeErrors, and
sanitizeSentryEvent so both sentry.edge.config.ts and sentry.server.config.ts
import and call that helper from their beforeSend functions to avoid duplication
and drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31ededf2-6316-4452-95d6-a41400888358
📒 Files selected for processing (7)
__tests__/components/notifications/NotificationsContext.test.tsx__tests__/components/waves/drops/WaveDropsReverseContainer.test.tsxcomponents/notifications/NotificationsContext.tsxcomponents/waves/drops/WaveDropsReverseContainer.tsxconfig/sentryProbes.tssentry.edge.config.tssentry.server.config.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
__tests__/components/notifications/NotificationsContext.test.tsx (1)
246-253: PreferglobalThisoverglobalfor cross-environment compatibility.Static analysis correctly flags the use of
global. UsingglobalThisis the modern, standardized approach that works consistently across Node.js and browser environments.Proposed fix
- const setTimeoutSpy = jest.spyOn(global, "setTimeout").mockImplementation((( + const setTimeoutSpy = jest.spyOn(globalThis, "setTimeout").mockImplementation((( handler: TimerHandler ) => { if (typeof handler === "function") { handler(); } - return 0 as unknown as NodeJS.Timeout; - }) as typeof global.setTimeout); + return 0 as unknown as ReturnType<typeof setTimeout>; + }) as typeof globalThis.setTimeout);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/notifications/NotificationsContext.test.tsx` around lines 246 - 253, Replace usage of the Node-specific global with the standardized globalThis when spying on setTimeout: update the jest.spyOn call that creates setTimeoutSpy (the spy variable created with jest.spyOn(global, "setTimeout")) to use globalThis instead (i.e., jest.spyOn(globalThis, "setTimeout")), ensuring the mockImplementation and teardown logic remain unchanged and still target the same setTimeout function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/api/common-api.ts`:
- Around line 92-119: The code currently calls res.json() then res.text(), which
double-reads the response body; instead, read the response body once (call
res.text() a single time into a variable like rawContent), assign errorBody =
rawContent (if present), then attempt to JSON.parse(rawContent) to extract
bodyRecord/bodyError/bodyMessage and set errorMessage accordingly (falling back
to rawContent or res.statusText or "Something went wrong"); update the
error-parsing block around res, errorBody, errorMessage, bodyRecord, bodyError,
and bodyMessage to follow this single-read approach.
---
Nitpick comments:
In `@__tests__/components/notifications/NotificationsContext.test.tsx`:
- Around line 246-253: Replace usage of the Node-specific global with the
standardized globalThis when spying on setTimeout: update the jest.spyOn call
that creates setTimeoutSpy (the spy variable created with jest.spyOn(global,
"setTimeout")) to use globalThis instead (i.e., jest.spyOn(globalThis,
"setTimeout")), ensuring the mockImplementation and teardown logic remain
unchanged and still target the same setTimeout function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b64c7d2-87dd-4ade-a4dd-0d0323d4b8ae
📒 Files selected for processing (4)
__tests__/components/notifications/NotificationsContext.test.tsx__tests__/services/common-api.test.tscomponents/notifications/NotificationsContext.tsxservices/api/common-api.ts
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/services/common-api.test.ts (1)
119-169: Comprehensive test for structured error mode.Good coverage of the structured error contract: verifies
message,status,headers,response.status,response.headers, andresponse.bodyare all correctly populated. Theretry-afterheader assertion validates the header preservation needed for retry logic.Consider renaming the catch parameter per project conventions.
,
♻️ Optional: Rename catch parameter to follow naming convention
} catch (caught) { - error = caught as { + error = caught as unknown as {Or if the convention is
error_:- } catch (caught) { - error = caught as { + } catch (error_) { + error = error_ as {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/services/common-api.test.ts` around lines 119 - 169, Rename the catch parameter in the test from "caught" to the project's preferred convention (e.g., "error_" or "err") to match naming standards; update the try/catch block in the test case that calls commonApiPost (the variable currently declared as let error = null and the catch parameter named caught) so the caught exception is assigned using the new parameter name and all references to the caught variable within the catch block are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/services/common-api.test.ts`:
- Around line 119-169: Rename the catch parameter in the test from "caught" to
the project's preferred convention (e.g., "error_" or "err") to match naming
standards; update the try/catch block in the test case that calls commonApiPost
(the variable currently declared as let error = null and the catch parameter
named caught) so the caught exception is assigned using the new parameter name
and all references to the caught variable within the catch block are updated
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4358307-aa16-41cc-b205-777c3b49e400
📒 Files selected for processing (3)
__tests__/services/common-api.test.tscomponents/notifications/NotificationsContext.tsxservices/api/common-api.ts
prxt6529
approved these changes
Mar 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores