Repository navigation
Conversation
There was a problem hiding this comment.
Stale comment
Deep code review
Reviewed
240d490againstmain(888586d). Intent is two wrapper bugs from campaign API tests (W1 / W2): iOS Debug recorded a native-onlyRCTLogWarnthree times (Custom + two stderr echoes) and ran the log filter three times;Bugsee.launch('')threw a plainErrorwith nocode, so callers matchingE_TOKENnever saw it.I did not review the diff in isolation. Surrounding code:
BGSRNDropConsoleEcho/ channel-line keep, JSnoteConsoleEcho+console/capture.ts, nativeE_TOKENon both modules, RN 0.87.1RCTLog.mm+Libraries/Utilities/RCTLog.js+ LogBox, the exampleRCTSetLogFunctionstderr mirror, and the open #63 pins this PR says it unblocks. I did not run Jest or native tests here (node_modulesis 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
_RCTLogNativeInternalcalls the log function synchronously, then (underRCT_DEBUG)logIfNoNativeHook. Wrapping withRCTSetLogFunctionso the drop is armed beforeexisting(...)is the right fix:RCTAddLogFunctionalways runs the current function first, which is why a drop armed inside the hook was too late.- RN 0.87
logIfNoNativeHook: ifnativeLoggingHookexists (normal device/simulator Debug), onlylevel === 'warn'hits LogBox’s warning handler, which calls theconsole.warnsaved before Bugsee’s patch. Error/fatal do not take that path. GatingBGSRNExpectJsRelogonRCT_DEBUG && Warningmatches that. A JS-debugger session with nonativeLoggingHookre-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/EndChannelLinethe same waywrapperLogdoes, 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:
AppDelegatesets the stderr mirror indidFinishLaunchingbefore RN starts;BugseeModuleinitwraps whatever is current. A laterRCTSetLogFunctionstill replaces the chain (same as beforeRCTAddLogFunction). A laterRCTAddLogFunctionkeeps this wrapper first.rctLogis a void TurboModule method (queued).UPLOAD_AFTER_MSis 2s, same as the echo window; the new e2e case is in that existing envelope, not a tighter race.W2 —
E_TOKEN
- JS
assertUsableTokenalready ran before the bridge. Native iOS/Android reject withE_TOKEN/Bugsee.launch requires a non-empty app tokenand the same trim rule ('', whitespace, non-string). Attachingcode: 'E_TOKEN'on the JSErroris the same shape as other JS-side codes in this package (Object.assign(new Error(...), { code })in report tests). Callers matching.codenow see one answer.launchisasync; the throw still becomes a rejection, same as before. Exception handlers andregisterWrapperstill do not run on a blank token.Tests
- Unit: blank and non-string tokens now assert
{ code: 'E_TOKEN', message }. Overlaps the oldertoThrowcases; that is redundant, not wrong.- e2e:
console-dedupadds a native-onlyrctLog('warn')and requiresnativeCalls === 1and one kept event (iOS: Custom 98, Warning 2). That is the W1 regression. Android is correctlyendsWithplus 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 : itfor FLOW-13This PR documents flipping those after #63 merges. If both land without that flip, Jest
it.failingwill 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, unitand 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.failingpins to plainit.4. Positive observations
- Replacing
RCTAddLogFunctionwith a wrap that arms, then callsexisting, then records, matches how the JS patch armsnoteConsoleEchobefore the original console write (#47).- Warning-only JS relog is justified by RN 0.87 LogBox (
setWarningHandler+logIfNoNativeHookonly calling it for'warn'when the native hook exists). Not an under-fix forRCTLogError.- 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=3before,2without relog arming,1with it), not a proxy assertion.Sent by Cursor Automation: Bugsee code review
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)
240d490 to
9124567
Compare
There was a problem hiding this comment.
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
_RCTLogNativeInternalcalls the log function synchronously, then (underRCT_DEBUG)logIfNoNativeHookwith the same message string. Wrapping withRCTSetLogFunctionsoBGSRNNoteConsoleEcho/BGSRNExpectJsRelogrun beforeexisting(...)is the right fix:RCTAddLogFunctionalways 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
logIfNoNativeHookmatch: ifnativeLoggingHookexists (normal device/simulator Debug), onlylevel === 'warn'hits LogBox’s warning handler. That handler calls theconsole.warnsaved before Bugsee’s patch (LogBox.installin InitializeCore), so the relog does not go throughconsole/capture.tsand must be dropped natively. Error/fatal do not take that path. GatingBGSRNExpectJsRelogonRCT_DEBUG && Warningmatches that. A JS-debugger session with nonativeLoggingHookre-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/EndChannelLinethe same waywrapperLogdoes, 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:didFinishLaunchingsets the stderr mirror before the scene starts RN;BugseeModuleinitwraps whatever is current. A laterRCTSetLogFunctionstill replaces the chain (same as beforeRCTAddLogFunction). A laterRCTAddLogFunctionkeeps this wrapper first. - Same-text collision inside the 2s echo window (a
console.warnof the same string stealing the relog claim) is the existing echo-ledger class, not a new hole in this wrap. rctLogis 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
assertUsableTokenalready ran before the bridge. Native iOS/Android reject withE_TOKEN/Bugsee.launch requires a non-empty app tokenand the same trim rule ('', whitespace, non-string). Attachingcode: 'E_TOKEN'on the JSErroris the same shape as other JS-side codes in this package (Object.assign(new Error(...), { code })).launchisasync; the throw still becomes a rejection. Exception handlers andregisterWrapperstill 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 onceis now a plainit(wasON_IOS ? it.failing : it).examples/bare/e2e/api-arguments.test.ts:[API-01c] the rejection carries code E_TOKENis now a plainit(wasit.failing).- Unit: blank and non-string tokens now assert
{ code: 'E_TOKEN', message }. The oldertoThrowcases remain; that is redundant, not wrong. - The native-only case that lived on
console-dedupat240d490was 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
RCTAddLogFunctionwith a wrap that arms, then callsexisting, then records, matches how the JS patch armsnoteConsoleEchobefore the original console write (#47). - Warning-only JS relog is justified by RN 0.81/0.87 LogBox (
setWarningHandler+logIfNoNativeHookonly calling it for'warn'when the native hook exists). Not an under-fix forRCTLogError. - 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.failingthat would explode on merge.
Sent by Cursor Automation: Bugsee code review


Fixes the two wrapper bugs the campaign API tests found (#63,
campaign-prep-api.mdW1 and W2).W1 — iOS: a native-only RCTLog line was recorded and filtered three times
In a Debug build a native
RCTLogWarnreached the report as the wrapper's Custom (98) line plus two stderr (source 2) echoes, and the log filter ran three times:_RCTLogNativeInternalalso callsRCTLog.logIfNoNativeHookunderRCT_DEBUG. LogBox's warning handler passes awarnto theconsole.warnit saved before any patch, so the same text comes back through RCTLog asRCTLogSourceJavaScriptand is mirrored again.BGSRNConsoleCapturearmed the echo drop (#47) only forconsole.*lines, throughnoteConsoleEcho. The hook also usedRCTAddLogFunction, which runs the existing function first, so a drop armed inside the hook would come after the echo was written.Fix (
ios/BGSRNConsoleCapture.mm):Begin/EndChannelLine.RCT_DEBUG, a native warning also expects its JavaScript relog. When that delivery arrives, it arms one more drop.capture.logsoff nothing is armed, and JavaScript-source lines are still left to the JS patch.W2 —
launch('')rejected withoutcode: 'E_TOKEN'The JS
assertUsableTokencheck runs before the bridge on both platforms and threw a plainError. The native modules already reject a blank token withE_TOKEN(implementation plan, Task 1.3). The JS error now carriescode: 'E_TOKEN'and the same message. This one change covers both platforms.Tests
lifecycle.test.ts: blank and non-string tokens reject with{code: 'E_TOKEN', message}. It failed before the fix.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.index.tsassertUsableToken): 14/14 killed.nativeCalls=2and the test fails.Device results (first round, before the rebase; the rebased round is in a comment below)
E2E_RELEASE=1🤖 Generated with Claude Code