feat(core): Wire TurboModulePerfLogger on iOS and Android - #6307
4 issues
code-review: Found 4 issues (4 low)
Low
Redundant `node` subprocess invocations at Gradle configure time in android/build.gradle - `packages/core/android/build.gradle:117-140`
isReactNativePrefabAvailable() is cached into needsPrefab at line 100, but the function is re-invoked uncached at line 121 and line 143, and resolveReactNativeDir() is called a fourth time directly at line 144. isReactNativePrefabAvailable() calls resolveReactNativeDir() (line 22), which—when no REACT_NATIVE_NODE_MODULES_DIR override is set and the New Architecture is enabled—spawns a node --print require.resolve(...) subprocess via providers.exec. As a result up to four node subprocesses are spawned per configure phase, adding latency on every Gradle sync/build. Consider reusing the cached needsPrefab value and resolving the React Native dir once into a shared variable.
No tests cover the new `initNativeSdk` promise-rejection path or `enableTurboModuleTracking` wiring - `packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java:196-243`
The RNSentryTurboModulePerfTracker unit tests validate the tracker in isolation, but no test exercises the initNativeSdk integration: (1) startWithOptions throwing causing promise.reject, and (2) the enableTurboModuleTracking option correctly toggling the tracker.
ForwardingLogger → sink dispatch path has no automated test - `packages/core/cpp/SentryTurboModulePerfLogger.cpp:47-80`
The forwarding path in ForwardingLogger (the SENTRY_FORWARD* macro bodies in SentryTurboModulePerfLogger.cpp) that routes a RN callback to the installed sink is not covered by any test in this PR. RNSentryTurboModulePerfControllerTests.mm declares a RecordingSink with moduleCreateStartCalls/syncMethodCallStartCalls counters but never asserts they increment after a callback, and the file closes with an explicit NOTE deferring end-to-end forwarding coverage to follow-up sink PRs. The practical impact is limited: no sink is installed by production code in this PR, the macro bodies are uniform forwarders where a signature mismatch would fail to compile, and the isEnabled()/sink() guards are independently exercised by testSetEnabledTogglesIsEnabled and testSetSinkRoundTrip. Still, a regression in the forward order or guard logic inside the macros (e.g. dropping the isEnabled() early-return) would go undetected. Consider adding an integration-level test once a sink ships, or exposing the logger for a direct dispatch assertion.
Also found at:
packages/core/cpp/SentryTurboModulePerfLogger.cpp:212-214
`testSetEnabledFalseDoesNotInstall` assertion is tautological and cannot validate its stated install-side invariant - `packages/core/cpp/SentryTurboModulePerfLogger.h:91-92`
SentryTurboModulePerfController::isEnabled() returns enabled_ && installed_, and setUp/tearDown only reset enabled_ (via setEnabled(false)) and the sink — there is no test-reset for the sticky installAttempted_/installed_ fields (unlike Android's RNSentryTurboModulePerfTracker.resetNativeUnavailableForTests()). As a result testSetEnabledFalseDoesNotInstall asserts only isEnabled() == false, which is already guaranteed by the enabled_ reset in setUp regardless of install state, so the test cannot validate its documented claim that a freshly reset controller "never claims the perf-logger slot." The test comment itself acknowledges install introspection is not exposed and is covered only "contractually." This is a deterministic coverage gap, not a flaky/order-dependent failure.
⏱ 6m 52s · 1.6M in / 84.8k out · $3.37