feat: vault-core based logger#395
Conversation
|
@nickgr2 please review. Ignore any inconsistency in package.json, it's not final, pending vault-core PR merge |
nickgr2
left a comment
There was a problem hiding this comment.
Reviewed at 47a36d3. The Diagnostics screen uses the kit cleanly (PageHeader, ToggleSwitch, Button, Text, useTheme, rawTokens), strings go through Lingui, interactive elements have testIDs, and the README addition is a clear public-facing statement of the Sentry boundary. A few things to address before merge.
Blocker
@sentry/react-native is referenced but missing from package.json / package-lock.json.
src/utils/sentry.js:import * as Sentry from '@sentry/react-native'app.config.ts:plugins.push('@sentry/react-native/expo')
Neither declares @sentry/react-native as a dependency, and the package isn't in the lockfile. Nightly builds (PEARPASS_DISTRIBUTION=nightly) will fail at prebuild when Expo tries to load the plugin, and initSentry() will throw at runtime. Please add @sentry/react-native (and update the lockfile) — preferably the version you tested with on the simulator/Pixel 9.
Other findings
loadLogConfiguration() doesn't notify subscribers when storage resolves. registerRootComponent(Main) is intentionally ordered before bootstrap() (per the Android-race comment in index.js), so it's possible for things to read getLogLevelSync() / mount useLogLevel before AsyncStorage returns. The first read sees the default ('off' outside nightly, 'debug' on nightly); the listener registered in useLogLevel's useEffect only fires on setLogLevel, never on the storage-load completion, so the stored level isn't observed until the user toggles. Same applies to the subscribeLogLevel callback wired in src/worklet/index.js after setLogOptions(buildLogOptions(getLogLevelSync())) — if vault-core is created before bootstrap finishes, it gets the default instead of the stored level until the user toggles. Suggest firing all listeners at the end of loadLogConfiguration once cached is updated, so existing consumers reconcile.
getDistributionChannel default changed 'standard' → 'production' in src/constants/distribution.js, but app.config.ts still falls back to 'standard'. The two paths now disagree on the fallback string. Functionally harmless because isFdroid()/isNightly() are the only consumers, but worth aligning to avoid a footgun if anything ever reads the channel string directly.
Minor
src/worklet/index.js: thesubscribeLogLevelcallback is never unsubscribed. One client per app lifetime so currently safe, but worth revisiting if the client can ever be torn down and recreated.createFileLoggerallocates a freshTextEncoderper log line — could be hoisted module-level. Style nit.app.config.tsadds the Sentry plugin wheneverisNightly, even ifPEARPASS_SENTRY_DSNis unset. The runtimeinitSentrydoes checkif (!dsn) return, so capturing is safe; just a heads-up that the Expo plugin will still be loaded into the build in that case.
|
@nickgr2 please review |
nickgr2
left a comment
There was a problem hiding this comment.
Re-reviewed at 993f8c6 (3 commits since 47a36d3).
Blocker — resolved
@sentry/react-native@8.9.2now inpackage.jsonand lockfile.EXPO_PUBLIC_SENTRY_DSNenv var replacesConstants.expoConfig.extra.sentryDsnin bothsrc/utils/sentry.jsandsrc/worklet/index.js, so the gating is consistent across host and worklet.- The new
defaultIntegrations: false+ explicit list ininitSentrylooks well-thought-out — the comment explaining the Metro module-table failure is exactly the kind of context I want.
Other findings
loadLogConfiguration() still doesn't notify subscribers when AsyncStorage resolves. Same observation as last round — loadLogConfiguration updates cached but never calls for (const fn of listeners) fn(cached). With registerRootComponent(Main) running before bootstrap() awaits storage, any consumer that registered via useLogLevel / subscribeLogLevel before bootstrap settles will continue seeing the default ('off' outside nightly, 'debug' on nightly) until the user toggles. createPearpassVaultClient reads getLogLevelSync() synchronously when first called and uses it for setLogOptions; if it races bootstrap on a cold start, vault-core gets the wrong sink configuration until a toggle reconciles it. Three-line fix at the end of loadLogConfiguration.
Minor
src/utils/errorReporter.jswrites[reported] {stack}to the log sink wheneverlogSinkImplis set, regardless of whethercaptureExceptionImplis registered. On non-nightly distributions nothing is actually reported anywhere, butmain.logstill gets the[reported] …line. Suggest gating that log emission oncaptureExceptionImplbeing set so the prefix stays truthful.README.md: typo "Loggin is off by default" — should be "Logging".package.jsonswaps the@tetherto/pearpass-lib-ui-kitspec fromgit+https://…#design-system-v2togithub:tetherto/…#design-system-v2. Both resolve to the same commit; just an inconsistency vs every other tetherto dep on the next line — easy to align.metro.config.cjsnow usesgetSentryExpoConfigfor all builds, not just nightly. Functionally fine (source-map upload is gated onSENTRY_AUTH_TOKEN), worth a heads-up that the Sentry Metro wrapper is in the build path for standard/F-Droid too.
The split between bundle:* (--defer sentry-bare) and bundle:*:nightly for the bare bundles is a clean way to keep sentry-bare out of public worklet bundles.
Requirements
Allows extracting logs from main process + worklet for external support.
Changes
isNightly()only.Testing Notes
Tested on iOS Simulator and Pixel 9
Things reviewers should pay attention to
Screenshots/Recordings
dependencies
tetherto/pearpass-lib-vault-core#55