Skip to content

Fix iOS native RCTLog triple record and E_TOKEN on a blank launch token - #66

Open
krassx wants to merge 2 commits into
mainfrom
fix/rctlog-dup-and-etoken
Open

krassx wants to merge 2 commits into
mainfrom
fix/rctlog-dup-and-etoken

Conversation

@krassx

@krassx krassx commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Fixes the two wrapper bugs the campaign API tests found (#63, campaign-prep-api.md W1 and W2).

W1 — iOS: a native-only RCTLog line was recorded and filtered three times

In a Debug build a native RCTLogWarn reached the report as the wrapper's Custom (98) line plus two stderr (source 2) echoes, and the log filter ran three times:

  1. React Native's own delivery through the app's log function. The example mirrors RCTLog to stderr, so the SDK captured that copy.
  2. _RCTLogNativeInternal also calls RCTLog.logIfNoNativeHook under RCT_DEBUG. LogBox's warning handler passes a warn to the console.warn it saved before any patch, so the same text comes back through RCTLog as RCTLogSourceJavaScript and is mirrored again.

BGSRNConsoleCapture armed the echo drop (#47) only for console.* lines, through noteConsoleEcho. The hook also used RCTAddLogFunction, which runs the existing function first, so a drop armed inside the hook would come after the echo was written.

Fix (ios/BGSRNConsoleCapture.mm):

  • The hook wraps the current log function, so it runs before the rest of the chain.
  • A native line arms the same echo drop a console line does (a stderr stamp and one raw stdio line), then is recorded once on the channel as Custom, inside Begin/EndChannelLine.
  • Under RCT_DEBUG, a native warning also expects its JavaScript relog. When that delivery arrives, it arms one more drop.
  • Nothing new is recorded: with capture.logs off nothing is armed, and JavaScript-source lines are still left to the JS patch.

W2 — launch('') rejected without code: 'E_TOKEN'

The JS assertUsableToken check runs before the bridge on both platforms and threw a plain Error. The native modules already reject a blank token with E_TOKEN (implementation plan, Task 1.3). The JS error now carries code: 'E_TOKEN' and the same message. This one change covers both platforms.

Tests

  • Unit: lifecycle.test.ts: blank and non-string tokens reject with {code: 'E_TOKEN', message}. It failed before the fix.
  • e2e (rebased on e2e(campaign): API, option and appearance device tests (N-02..N-09, N-11, N-13, N-14) #63): the pins this fix closes are flipped to plain it: N-05 [FLOW-13] a native RCTLog warning with no console call lands once and is filtered once (console-levels) and N-08 [API-01c] the rejection carries code E_TOKEN (api-arguments). The earlier failing-first native case in console-dedup duplicated FLOW-13 and was dropped. Before the fix it ran 3 filter calls on the simulator.
  • Stryker (src, index.ts assertUsableToken): 14/14 killed.
  • Manual native mutant: without the relog arming the simulator gives nativeCalls=2 and the test fails.

Device results (first round, before the rebase; the rebased round is in a comment below)

Target Suites Result
iOS Simulator 6FA9B3E8 (Debug) console-dedup, log-filter, console, wrapper-channel 10/10, twice
iOS Simulator, with #63 merged locally and both pins flipped console-levels, api-arguments 28 pass, 4 skipped (as in #63)
iPhone XS (Debug, run-ios.sh), #63 merged locally, pins flipped console-dedup, console-levels, api-arguments, log-filter 39/39
iPhone XS (Release) console-dedup E2E_RELEASE=1 4/4
WOD_LX1 (Debug), #63 merged locally, pins flipped console-dedup, console-levels, api-arguments, log-filter 37/39. API-11d/11e: the scenario never started, with another app in front. They passed on the rerun (4/4).

🤖 Generated with Claude Code

@cursor cursor 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.

Stale comment

Deep code review

Reviewed 240d490 against main (888586d). Intent is two wrapper bugs from campaign API tests (W1 / W2): iOS Debug recorded a native-only RCTLogWarn three times (Custom + two stderr echoes) and ran the log filter three times; Bugsee.launch('') threw a plain Error with no code, so callers matching E_TOKEN never saw it.

I did not review the diff in isolation. Surrounding code: BGSRNDropConsoleEcho / channel-line keep, JS noteConsoleEcho + console/capture.ts, native E_TOKEN on both modules, RN 0.87.1 RCTLog.mm + Libraries/Utilities/RCTLog.js + LogBox, the example RCTSetLogFunction stderr mirror, and the open #63 pins this PR says it unblocks. I did not run Jest or native tests here (node_modules is not installed). CI on this head was still pending at review time.

No P0–P3 findings. Every candidate I tried to break either matches RN 0.87 behavior, matches existing console-echo design, or is merge coordination with #63 rather than a defect in this diff.

What I pressure-tested

W1 — native RCTLog triple record

  • _RCTLogNativeInternal calls the log function synchronously, then (under RCT_DEBUG) logIfNoNativeHook. Wrapping with RCTSetLogFunction so the drop is armed before existing(...) is the right fix: RCTAddLogFunction always runs the current function first, which is why a drop armed inside the hook was too late.
  • RN 0.87 logIfNoNativeHook: if nativeLoggingHook exists (normal device/simulator Debug), only level === 'warn' hits LogBox’s warning handler, which calls the console.warn saved before Bugsee’s patch. Error/fatal do not take that path. Gating BGSRNExpectJsRelog on RCT_DEBUG && Warning matches that. A JS-debugger session with no nativeLoggingHook re-console.*s every level; that is RN’s debugger fallback, not this hook.
  • JS-source lines are still not recorded. Relog delivery only arms another echo drop. Native recording uses Begin/EndChannelLine the same way wrapperLog does, so the Custom line is kept and consumes the equal-text claim after stderr has already taken the stdio credit.
  • Install order in the example is safe: AppDelegate sets the stderr mirror in didFinishLaunching before RN starts; BugseeModule init wraps whatever is current. A later RCTSetLogFunction still replaces the chain (same as before RCTAddLogFunction). A later RCTAddLogFunction keeps this wrapper first.
  • rctLog is a void TurboModule method (queued). UPLOAD_AFTER_MS is 2s, same as the echo window; the new e2e case is in that existing envelope, not a tighter race.

W2 — E_TOKEN

  • JS assertUsableToken already ran before the bridge. Native iOS/Android reject with E_TOKEN / Bugsee.launch requires a non-empty app token and the same trim rule ('', whitespace, non-string). Attaching code: 'E_TOKEN' on the JS Error is the same shape as other JS-side codes in this package (Object.assign(new Error(...), { code }) in report tests). Callers matching .code now see one answer.
  • launch is async; the throw still becomes a rejection, same as before. Exception handlers and registerWrapper still do not run on a blank token.

Tests

  • Unit: blank and non-string tokens now assert { code: 'E_TOKEN', message }. Overlaps the older toThrow cases; that is redundant, not wrong.
  • e2e: console-dedup adds a native-only rctLog('warn') and requires nativeCalls === 1 and one kept event (iOS: Custom 98, Warning 2). That is the W1 regression. Android is correctly endsWith plus length 1 (logcat, one pass).

Merge coordination (not a defect in this diff)

#63 is still open and pins:

  • it.failing('[API-01c] the rejection carries code E_TOKEN')
  • const itNativeOnly = ON_IOS ? it.failing : it for FLOW-13

This PR documents flipping those after #63 merges. If both land without that flip, Jest it.failing will error on an unexpected pass. Flip in whichever PR is second; do not need a change on this branch until those files exist here.


1. Overall risk: Low

2. Merge recommendation: Approve

Safe to merge from a correctness standpoint. I would still wait for lint, typecheck, unit and iOS unit/e2e on this head (CI was pending at review).

3. Most important issues to fix

None in this diff. When merging with #63, convert those two it.failing pins to plain it.

4. Positive observations

  • Replacing RCTAddLogFunction with a wrap that arms, then calls existing, then records, matches how the JS patch arms noteConsoleEcho before the original console write (#47).
  • Warning-only JS relog is justified by RN 0.87 LogBox (setWarningHandler + logIfNoNativeHook only calling it for 'warn' when the native hook exists). Not an under-fix for RCTLogError.
  • E_TOKEN is a one-line JS change that covers both platforms because the JS guard always runs first; native rejects stay as the backstop for a direct TurboModule call.
  • The new e2e case is the actual W1 failure (nativeCalls=3 before, 2 without relog arming, 1 with it), not a proxy assertion.
Open in Web View Automation 

Sent by Cursor Automation: Bugsee code review

krassx added 2 commits October 9, 2026 18:18
The JS check runs before the bridge on both platforms and threw a plain
Error, so a caller never saw the E_TOKEN code the native modules use for
the same refusal (implementation plan, Task 1.3). It now carries
code E_TOKEN and the native message. Flips the N-08 [API-01c] pin.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
A native RCTLog line was recorded three times in a Debug build: the
wrapper's Custom line, plus two stderr echoes the SDK captured (RN's own
delivery through the app's log function, and LogBox re-logging the
warning from JS as RCTLogSourceJavaScript). The console echo drop was
armed only for console.* lines, so the filter ran three times too.

The RCTLog hook now wraps the log function instead of appending to it,
arms the echo drop for a native line before the rest of the chain runs,
and arms one more drop for the JavaScript relog of a native warning.
Flips the N-05 FLOW-13 'lands once and is filtered once' pin.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
@krassx
krassx force-pushed the fix/rctlog-dup-and-etoken branch from 240d490 to 9124567 Compare October 9, 2026 13:30

@cursor cursor 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.

Deep code review

Re-reviewed 9124567 against main (d957f55, #63). Prior pass was 240d490 (2026-10-07): no P0–P3, with merge coordination to convert #63’s it.failing pins once that PR landed.

Intent is unchanged: campaign W1 (iOS Debug recorded a native-only RCTLogWarn three times — Custom + two stderr echoes — and ran the log filter three times) and W2 (Bugsee.launch('') threw a plain Error with no code, so callers matching E_TOKEN never saw it).

I did not review the diff in isolation. Surrounding code: BGSRNDropConsoleEcho / Begin/EndChannelLine, JS noteConsoleEcho + console/capture.ts + LogBox’s saved console.warn, native E_TOKEN on both modules (BGSRNTokenIsUsable / BugseeTokens.isUsable), RN 0.81.0 and 0.87.1 RCTLog.mm + Libraries/Utilities/RCTLog.js (logIfNoNativeHook only calls the warning handler for 'warn' when nativeLoggingHook exists), the example RCTSetLogFunction stderr mirror (AppDelegate before startReactNative), and the #63 pins this rebase now flips. I did not run Jest or native tests here (node_modules is not installed). CI on this head was still queued at review (run 37937362460).

No remaining P0–P3. Native BGSRNConsoleCapture.mm is identical to the previously reviewed 240d490. The only material follow-through from last time is the #63 pin flip, which is done.

What I pressure-tested

W1 — native RCTLog triple record

  • _RCTLogNativeInternal calls the log function synchronously, then (under RCT_DEBUG) logIfNoNativeHook with the same message string. Wrapping with RCTSetLogFunction so BGSRNNoteConsoleEcho / BGSRNExpectJsRelog run before existing(...) is the right fix: RCTAddLogFunction always runs the current function first, which is why a drop armed inside an appended hook was too late for the example’s stderr mirror.
  • RN 0.81 and 0.87 logIfNoNativeHook match: if nativeLoggingHook exists (normal device/simulator Debug), only level === 'warn' hits LogBox’s warning handler. That handler calls the console.warn saved before Bugsee’s patch (LogBox.install in InitializeCore), so the relog does not go through console/capture.ts and must be dropped natively. Error/fatal do not take that path. Gating BGSRNExpectJsRelog on RCT_DEBUG && Warning matches that. A JS-debugger session with no nativeLoggingHook re-console.*s every level; that is RN’s debugger fallback, not this hook.
  • JS-source lines are still not recorded. Relog delivery only arms another echo drop. Native recording uses Begin/EndChannelLine the same way wrapperLog does, so the Custom line is kept and consumes the equal-text claim after stderr has already taken the stdio credit.
  • Install order in the example is safe: application:didFinishLaunching sets the stderr mirror before the scene starts RN; BugseeModule init wraps whatever is current. A later RCTSetLogFunction still replaces the chain (same as before RCTAddLogFunction). A later RCTAddLogFunction keeps this wrapper first.
  • Same-text collision inside the 2s echo window (a console.warn of the same string stealing the relog claim) is the existing echo-ledger class, not a new hole in this wrap.
  • rctLog is a void TurboModule method (queued). The FLOW-13 scenario waits 3s before reading filter counts, which is outside the 2s echo window but far longer than the JS relog hop.

W2 — E_TOKEN

  • JS assertUsableToken already ran before the bridge. Native iOS/Android reject with E_TOKEN / Bugsee.launch requires a non-empty app token and the same trim rule ('', whitespace, non-string). Attaching code: 'E_TOKEN' on the JS Error is the same shape as other JS-side codes in this package (Object.assign(new Error(...), { code })). launch is async; the throw still becomes a rejection. Exception handlers and registerWrapper still do not run on a blank token. Native rejects stay as the backstop for a direct TurboModule call.

Tests / #63 coordination (previously open)

  • examples/bare/e2e/console-levels.test.ts: [FLOW-13] ... lands once and is filtered once is now a plain it (was ON_IOS ? it.failing : it).
  • examples/bare/e2e/api-arguments.test.ts: [API-01c] the rejection carries code E_TOKEN is now a plain it (was it.failing).
  • Unit: blank and non-string tokens now assert { code: 'E_TOKEN', message }. The older toThrow cases remain; that is redundant, not wrong.
  • The native-only case that lived on console-dedup at 240d490 was dropped as a duplicate of FLOW-13. The pin that actually failed on device (counts.rct === 1 / one kept event) is the one that remains.

Findings

None.


1. Overall risk: Low

2. Merge recommendation: Approve

Safe to merge from a correctness standpoint. I would still wait for lint, typecheck, unit and iOS unit/e2e on this head (CI was queued at review).

3. Most important issues to fix

None. The #63 it.failing flip called out last time is in this rebase.

4. Positive observations

  • Replacing RCTAddLogFunction with a wrap that arms, then calls existing, then records, matches how the JS patch arms noteConsoleEcho before the original console write (#47).
  • Warning-only JS relog is justified by RN 0.81/0.87 LogBox (setWarningHandler + logIfNoNativeHook only calling it for 'warn' when the native hook exists). Not an under-fix for RCTLogError.
  • E_TOKEN is a one-line JS change that covers both platforms because the JS guard always runs first.
  • Campaign pins are now the regression tests, not a parallel it.failing that would explode on merge.
Open in Web View Automation 

Sent by Cursor Automation: Bugsee code review

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