Skip to content

Fix three issues found reviewing the 1.0.1 lifecycle work - #73

Merged
yosriady merged 4 commits into
mainfrom
fix/crash-chain-and-screen-url-encoding
Aug 2, 2026
Merged

Fix three issues found reviewing the 1.0.1 lifecycle work#73
yosriady merged 4 commits into
mainfrom
fix/crash-chain-and-screen-url-encoding

Conversation

@yosriady

@yosriady yosriady commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #70 and #71, from a Codex review sweep. All three are real; one is a genuine severity concern.

1. CrashReporter could sever the handler chain

The installed handler read this.previousHandler, a mutable field that cleanup() cleared. With two reporters alive — the provider re-initialising, or a customer's own reporter wrapping ours:

A.start()   → A wraps original
B.start()   → B wraps A
A.cleanup() → cannot restore (B is installed), clears A.previousHandler
crash       → B reports → calls A → A reports AGAIN → forwards to undefined

Net effect: the real RN/default handler never runs (no redbox, no crash-reporter chain) and a stopped reporter emits a duplicate Application Crashed.

Fixed by capturing the previous handler in the closure at install time, not clearing it on cleanup, and having a stopped reporter forward without reporting.

2. buildScreenUrl let the screen name change URL structure

screen("Checkout?coupon=SUMMER") produced app://bundle/Checkout?coupon=SUMMER, which parses with pathname /Checkout — the rest of the screen name silently dropped. '?' and '#' are now percent-encoded. '/' deliberately is not: router-style names like /tabs/leaderboard are meant to be path segments.

Dot segments (a/../Admin) are knowingly left alone. Encoding them is not possible — the URL spec decodes %2E before resolving path segments, so a JS new URL() collapses them either way. It also doesn't matter: the ingestion pipeline parses with ClickHouse's path(), which performs no dot-segment normalisation. Verified directly against the real parser:

path('app://com.acme.wallet/a/../Admin')  ->  /a/../Admin

3. Deep Link Opened was disabled by an unrelated flag

The event was gated on attribution.deeplinks, because that check also guarded the Linking hook both features share. So autocapture: { deepLinks: true } with attribution: { deeplinks: false } emitted nothing, with no indication that a different setting was responsible.

The hook is now installed if either consumer needs it, and each behaviour checks its own flag independently.

Tests

297 passing (6 new), typecheck / lint / build clean. The crash tests cover the nested-reporter sequence specifically: the original handler is still reached after the inner reporter is cleaned up, and the cleaned-up reporter does not double-report.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

yosriady and others added 3 commits August 2, 2026 08:42
1. CrashReporter could sever the handler chain. The installed handler read
   `this.previousHandler`, which cleanup() cleared. With two reporters — the
   provider re-initialising, or a customer's own reporter wrapping ours — the
   sequence A.start, B.start, A.cleanup, crash left B forwarding into A, and A
   forwarding into `undefined`: the real RN/default handler never ran, and A
   reported a duplicate despite being stopped. The previous handler is now
   captured in the closure at install time, cleanup no longer clears it, and a
   stopped reporter forwards without reporting.

2. buildScreenUrl let the screen name change the URL's structure.
   screen("Checkout?coupon=X") parsed with pathname "/Checkout" and the rest
   silently dropped. '?' and '#' are now percent-encoded; '/' deliberately is
   not, since router-style names are meant to be path segments.

   Dot segments are knowingly left alone: the URL spec decodes %2E before
   resolving path segments, so JS collapses "a/../Admin" either way — and it
   does not matter, because the pipeline parses with ClickHouse's path(), which
   performs no dot-segment normalisation. Verified against the real parser.

3. Deep Link Opened was gated on attribution.deeplinks rather than its own
   autocapture flag, because that check also guarded the Linking hook both
   features share. Turning attribution off silently disabled the event with no
   indication an unrelated setting was responsible. The hook is now installed
   if either consumer needs it, and each behaviour checks its own flag.

297 tests (6 new), typecheck, lint and build clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa993d915e

ℹ️ 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".

Comment on lines +167 to +168
.replace(/\?/g, "%3F")
.replace(/#/g, "%23");

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 Badge Escape literal percent signs before encoding delimiters

When a screen name already contains the literal text %3F or %23, it now produces exactly the same page_url as a distinct name containing ? or # (for example, Checkout%3Fx and Checkout?x both become app://<bundle>/Checkout%3Fx). This causes URL-based screen analytics to merge distinct screen names; encode existing % characters first, or encode each slash-separated path segment with a standard component encoder.

Useful? React with 👍 / 👎.

Review follow-up. Encoding '?' as %3F without first escaping '%' made the
transform non-injective: a screen literally named "Checkout%3Fx" and one named
"Checkout?x" both produced ".../Checkout%3Fx", silently merging two distinct
screens in the analytics.

'%' is now encoded first, so "Checkout?x" -> "Checkout%3Fx" and
"Checkout%3Fx" -> "Checkout%253Fx". Test asserts a mixed set of names maps to
the same number of distinct URLs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@yosriady
yosriady merged commit c4d3bc4 into main Aug 2, 2026
11 checks passed
@yosriady
yosriady deleted the fix/crash-chain-and-screen-url-encoding branch August 2, 2026 15:17
This was referenced Aug 3, 2026
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