Fix three issues found reviewing the 1.0.1 lifecycle work (#73)

* 1.0.1

* Fix three issues found reviewing the 1.0.1 lifecycle work

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>

* Escape '%' before encoding URL delimiters, keeping screen names distinct

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>

---------

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

Author: yosriady

src/FormoAnalytics.ts

@@ -153,7 +153,16 @@ export class FormoAnalytics implements IFormoAnalytics {
     // is a fast native bridge call; the Android Play Install Referrer is a fast
     // one-shot native call (and no-ops instantly when the native module or the
     // platform isn't Android), so awaiting it does not meaningfully delay init.
-    if (analytics.isAttributionEnabled("deeplinks")) {
+    // Hook Linking if EITHER consumer needs it. The two flags control different
+    // things — attribution.deeplinks parses UTMs into context, autocapture
+    // .deepLinks emits Deep Link Opened — but both depend on observing the link
+    // in the first place. Gating the hook on attribution alone silently
+    // disabled the event for anyone who turned attribution off, with no
+    // indication that an unrelated setting was responsible.
+    if (
+      analytics.isAttributionEnabled("deeplinks") ||
+      analytics.isAutocaptureEnabled("deepLinks")
+    ) {
       try {
         await analytics.startDeepLinkCapture();
       } catch (error) {
@@ -220,7 +229,9 @@ export class FormoAnalytics implements IFormoAnalytics {
     try {
       const url = await Linking.getInitialURL();
       if (url) {
-        this.setTrafficSourceFromUrl(url);
+        if (this.isAttributionEnabled("deeplinks")) {
+          this.setTrafficSourceFromUrl(url);
+        }
         // Held, not emitted: this runs before lifecycle tracking starts, and
         // the Segment spec orders `Deep Link Opened` after `Application
         // Opened`. Emitted by trackInitialDeepLink() once lifecycle has fired.
@@ -233,7 +244,11 @@ export class FormoAnalytics implements IFormoAnalytics {
     // Runtime deep links (foreground opens, universal links).
     this.linkingSubscription = Linking.addEventListener("url", (event) => {
       if (!event?.url) return;
-      this.setTrafficSourceFromUrl(event.url);
+      // Each behaviour checks its own flag: the hook may exist because only one
+      // of them is enabled.
+      if (this.isAttributionEnabled("deeplinks")) {
+        this.setTrafficSourceFromUrl(event.url);
+      }
       void this.trackDeepLinkOpened(event.url);
     });
   }

src/__tests__/crashReporter.test.ts

@@ -158,3 +158,72 @@ describe("CrashReporter", () => {
     expect(analytics.track).not.toHaveBeenCalled();
   });
 });
+
+describe("CrashReporter chained with another reporter", () => {
+  // The failure this guards: two reporters exist (e.g. the provider
+  // re-initialising, or a customer's own reporter wrapping ours), the first is
+  // cleaned up, then the app crashes. If our installed handler reads
+  // `this.previousHandler` at crash time — which cleanup() had cleared — the
+  // chain terminates at us and the real RN/default handler never runs.
+  type Handler = (error: Error, isFatal?: boolean) => void;
+
+  let original: jest.Mock;
+  let current: Handler | undefined;
+  let analyticsA: { track: jest.Mock; flush: jest.Mock };
+  let analyticsB: { track: jest.Mock; flush: jest.Mock };
+
+  const g = globalThis as {
+    ErrorUtils?: {
+      getGlobalHandler: () => Handler | undefined;
+      setGlobalHandler: (h: Handler) => void;
+    };
+  };
+
+  const mkAnalytics = () => ({
+    track: jest.fn().mockResolvedValue(undefined),
+    flush: jest.fn().mockResolvedValue(undefined),
+  });
+
+  beforeEach(() => {
+    original = jest.fn();
+    current = original;
+    g.ErrorUtils = {
+      getGlobalHandler: () => current,
+      setGlobalHandler: (h: Handler) => {
+        current = h;
+      },
+    };
+    analyticsA = mkAnalytics();
+    analyticsB = mkAnalytics();
+  });
+
+  afterEach(() => {
+    delete g.ErrorUtils;
+  });
+
+  it("still reaches the original handler after the inner reporter is cleaned up", () => {
+    const a = new CrashReporter(analyticsA);
+    const b = new CrashReporter(analyticsB);
+    a.start();
+    b.start();
+    a.cleanup(); // b's chain still points at a's installed handler
+
+    const error = new Error("boom");
+    current!(error, true);
+
+    expect(original).toHaveBeenCalledWith(error, true);
+  });
+
+  it("does not double-report from the cleaned-up reporter", () => {
+    const a = new CrashReporter(analyticsA);
+    const b = new CrashReporter(analyticsB);
+    a.start();
+    b.start();
+    a.cleanup();
+
+    current!(new Error("boom"), true);
+
+    expect(analyticsB.track).toHaveBeenCalledTimes(1);
+    expect(analyticsA.track).not.toHaveBeenCalled();
+  });
+});

src/__tests__/deepLinkEvent.test.ts

@@ -87,8 +87,25 @@ describe("Deep Link Opened", () => {
     expect(trackedEvents).not.toContain("Deep Link Opened");
   });
 
-  it("does not subscribe at all when deep-link attribution is disabled", async () => {
+  it("still emits the event when only attribution is disabled", async () => {
+    // The two flags are independent: attribution.deeplinks parses UTMs into
+    // context, autocapture.deepLinks emits the event. Turning attribution off
+    // must not silently take the event with it — the Linking hook has to be
+    // installed if EITHER consumer needs it.
     await init({ attribution: { deeplinks: false } });
+    const handler = addEventListener.mock.calls.at(-1)?.[1];
+    expect(handler).toBeDefined();
+
+    await handler({ url: "myapp://product" });
+
+    expect(trackedEvents).toContain("Deep Link Opened");
+  });
+
+  it("does not subscribe when BOTH deep-link flags are disabled", async () => {
+    await init({
+      attribution: { deeplinks: false },
+      autocapture: { deepLinks: false },
+    });
 
     expect(addEventListener).not.toHaveBeenCalled();
   });

src/__tests__/screenEvent.test.ts

@@ -126,3 +126,67 @@ describe("generateScreenEvent bundle id resolution", () => {
     );
   });
 });
+
+describe("buildScreenUrl structural characters", () => {
+  // The URL is parsed downstream with standard URL functions, so any character
+  // that changes URL STRUCTURE silently truncates or merges screen names.
+  it("keeps a '?' inside the screen name instead of starting a query", () => {
+    const url = buildScreenUrl("com.acme.wallet", "Checkout?coupon=SUMMER");
+    expect(url).toBe("app://com.acme.wallet/Checkout%3Fcoupon=SUMMER");
+    expect(new URL(url).pathname).toBe("/Checkout%3Fcoupon=SUMMER");
+    expect(new URL(url).search).toBe("");
+  });
+
+  it("keeps a '#' inside the screen name instead of starting a fragment", () => {
+    const url = buildScreenUrl("com.acme.wallet", "Order#123");
+    expect(new URL(url).hash).toBe("");
+    expect(new URL(url).pathname).toBe("/Order%23123");
+  });
+
+  it("passes dot segments through verbatim", () => {
+    // Deliberately NOT encoded: the URL spec decodes %2E before resolving path
+    // segments, so a JS `new URL()` collapses "a/../Admin" to "/Admin" either
+    // way. The ingestion pipeline uses ClickHouse's path(), which does no
+    // dot-segment normalisation, so the raw string is what matters here.
+    expect(buildScreenUrl("com.acme.wallet", "a/../Admin")).toBe(
+      "app://com.acme.wallet/a/../Admin",
+    );
+  });
+
+  it("still treats '/' as a real path separator for router-style names", () => {
+    const url = buildScreenUrl("com.acme.wallet", "/tabs/leaderboard");
+    expect(url).toBe("app://com.acme.wallet/tabs/leaderboard");
+    expect(new URL(url).pathname).toBe("/tabs/leaderboard");
+  });
+
+  it("leaves ordinary names untouched", () => {
+    expect(buildScreenUrl("com.acme.wallet", "Home")).toBe(
+      "app://com.acme.wallet/Home",
+    );
+  });
+});
+
+describe("buildScreenUrl encoding is injective", () => {
+  // Encoding '?' as %3F without first escaping '%' would make a screen named
+  // "Checkout%3Fx" indistinguishable from "Checkout?x", silently merging two
+  // distinct screens in the analytics.
+  it("does not collide a literal %3F with an encoded ?", () => {
+    const fromQuestionMark = buildScreenUrl("com.acme.wallet", "Checkout?x");
+    const fromLiteral = buildScreenUrl("com.acme.wallet", "Checkout%3Fx");
+    expect(fromQuestionMark).not.toBe(fromLiteral);
+    expect(fromQuestionMark).toBe("app://com.acme.wallet/Checkout%3Fx");
+    expect(fromLiteral).toBe("app://com.acme.wallet/Checkout%253Fx");
+  });
+
+  it("does not collide a literal %23 with an encoded #", () => {
+    expect(buildScreenUrl("com.acme.wallet", "Order#1")).not.toBe(
+      buildScreenUrl("com.acme.wallet", "Order%231"),
+    );
+  });
+
+  it("keeps distinct names distinct across a mixed set", () => {
+    const names = ["Home", "Home?x", "Home%3Fx", "Home#y", "Home%23y", "a/b", "a%2Fb"];
+    const urls = names.map((n) => buildScreenUrl("com.acme.wallet", n));
+    expect(new Set(urls).size).toBe(names.length);
+  });
+});

src/lib/crash/index.ts

@@ -64,18 +64,31 @@ export class CrashReporter {
       return;
     }
 
-    this.previousHandler = errorUtils.getGlobalHandler();
+    // Captured in the closure, NOT read from `this` at crash time. If another
+    // reporter wraps us afterwards our handler stays reachable through its
+    // chain, and cleanup() clearing the field would otherwise sever the chain
+    // at the moment it matters: A.start, B.start, A.cleanup, crash — B calls
+    // our handler, which would then forward to `undefined` and the real
+    // RN/default handler would never run.
+    const previousHandler = errorUtils.getGlobalHandler();
+    this.previousHandler = previousHandler;
 
     const handler: ErrorHandler = (error, isFatal) => {
       // Nothing in here may throw: this runs while the app is already failing,
       // and an exception would replace the real crash with ours.
-      try {
-        this.report(error, isFatal);
-      } catch (reportingError) {
-        logger.debug("CrashReporter: failed to report crash", reportingError);
+      //
+      // `started` is checked so a cleaned-up reporter still FORWARDS (keeping
+      // the chain intact) but no longer reports — otherwise the sequence above
+      // would emit a duplicate Application Crashed from the stopped instance.
+      if (this.started) {
+        try {
+          this.report(error, isFatal);
+        } catch (reportingError) {
+          logger.debug("CrashReporter: failed to report crash", reportingError);
+        }
       }
 
-      this.previousHandler?.(error, isFatal);
+      previousHandler?.(error, isFatal);
     };
 
     this.installedHandler = handler;
@@ -123,9 +136,10 @@ export class CrashReporter {
       errorUtils.setGlobalHandler(this.previousHandler);
     }
 
+    // previousHandler is deliberately NOT cleared: if another reporter wrapped
+    // us, our installed handler is still in its chain and must keep forwarding.
     this.started = false;
     this.installedHandler = undefined;
-    this.previousHandler = undefined;
     logger.info("CrashReporter: Cleaned up");
   }
 }

src/lib/event/EventFactory.ts

@@ -156,7 +156,20 @@ interface DeviceInfoResult {
  *   ("/tabs/leaderboard") does not produce a doubled separator.
  */
 export function buildScreenUrl(bundleId: string, name: string): string {
-  const screen = (name ?? "").replace(/^\/+/, "");
+  const screen = (name ?? "")
+    .replace(/^\/+/, "")
+    // Percent-encode the characters that would otherwise change the URL's
+    // STRUCTURE rather than its path. '?' starts a query and '#' a fragment, so
+    // screen("Checkout?coupon=X") would parse with pathname "/Checkout" and the
+    // rest silently dropped from the screen name. '/' is deliberately NOT
+    // encoded — router-style names like "/tabs/leaderboard" are meant to be
+    // path segments.
+    // '%' FIRST, so the transform stays injective. Without it a screen literally
+    // named "Checkout%3Fx" and one named "Checkout?x" both produce
+    // ".../Checkout%3Fx" and two distinct screens merge in the analytics.
+    .replace(/%/g, "%25")
+    .replace(/\?/g, "%3F")
+    .replace(/#/g, "%23");
   return `app://${bundleId ?? ""}/${screen}`;
 }