T2: performance facade, typed upload mode, hide wire internals

[krassx]

Review-only PR. This change is already on main (the repo works directly on main; see CLAUDE.md). The base branch is the commit it forked from, so the diff below is exactly this task. Merging this PR changes nothing. Review findings become follow-up commits on main. Plan: docs/design/mobile-api-alignment.md.

• performanceUploadMode added to UmbrellaExtensionOptions (typed, tsc-checked).
• startTransaction / startSpan / getActiveSpan on the launched client (browser/node/bun/deno); no-ops on worker/edge entries and when performanceMonitoring is off.
• UploadOptions.trigger and Mechanism are no longer public; internal Internal* types and asInternalClient.

Review focus / known gaps

• performanceUploadMode is typed but nothing reads it (already true before this change).
• startSpan with no active span opens a new root transaction (my reading of mobile's behaviour).
• Internal* types are still exported from @bugsee/core (marked @internal), not from the umbrella.
• Worker/edge/workerd wrappers only tested under Node.

🤖 Generated with Claude Code

[cursor]

Deep review — T2 performance facade / typed upload mode / hide wire internals

Reviewed e588c92 (task commit 74a1dc9) against the surrounding client lifecycle, performance extension, uploader, and asInternalClient call sites — not the diff alone. No prior review threads on this PR.

T2 does three things: type performanceUploadMode, put startTransaction / startSpan / getActiveSpan on the launched umbrella client, and move trigger / mechanism behind Internal* + asInternalClient. The hide-internals pass is consistent (adapters and the report dialog were updated; umbrella type tests pin the public surface). The new facade and the newly public upload-mode option have two real correctness holes.

Findings

1. P2 — Facade ignores stopped / killed / isLaunched(). After stop(), ext('performance') is still registered, so the new top-level methods still create sampled transactions; composed flush() / a second stop() still uploads them.
2. P2 — performanceUploadMode: 'realtime' is now a typed, documented launch option and is never read. The uploader is interval-batch only. Before this change, TypeScript rejected the friendly name on launch().

Residuals (not blocking)

• createClient() now returns InternalBugseeClient, so @bugsee/core callers still typecheck trigger / mechanism. The umbrella surface is clean.
• startSpan with a parent does not become getActiveSpan() (no span stack — already documented as a later slice). Sequential startSpan calls are siblings under the current transaction, not nested.
• Pageload collection is not torn down in wirePerformance.stop() (pre-existing). Combined with (1), getActiveSpan() after stop() can still return the leftover pageload.

Did not run the suite (node_modules is not installed in this environment). Conclusions are from the implementation, the extension/uploader stop path, and the new tests (which cover "performance off" and repeat-launch, not stop/kill or realtime).

Verdict

1. Overall risk: Medium
2. Merge recommendation: Request changes
3. Fix first: gate the facade on client.isLaunched() (same as logException / upload); do not ship a working-looking performanceUploadMode until the uploader honors it — or drop it from the public options until then
4. What landed well: asInternalClient is a type-only view and the call-site sweep is complete; no-op facade on worker/edge matches "performance off"; lazy getApi() correctly picks up a late registerExt; idempotent in-place install is the right shape for the process singleton

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

P2 — Facade ignores stopped / killed / isLaunched()

performanceApiOf only treats "extension not registered" as absent. After stop() (and after KILL_SDK), ext('performance') still succeeds:

• createPerformanceExtension().stop() is a no-op (packages/performance/src/extension.ts)
• registerExt is never undone
• createPerformanceUploader().stop() only clearIntervals; flush() still store.drain() + send
• umbrella client.flush / client.stop always call that flush() before the core early-return (packages/bugsee/src/wire.ts composed stop / flush)

logException / upload / prepareUpload already no-op when stopped || killed (packages/core/src/client.ts). These new mobile-parity methods do not.

Impact: APM keeps recording after the app asked the SDK to stop, and a later flush() or a defensive second stop() uploads those traces.

Scenario:

1. const client = await launch(token, opts) (performance on).
2. Logout / privacy path: await client.stop() — carrier cleared, capture halted, uploader interval cleared, leftover pageload may still sit in the active slot.
3. Leftover instrumentation calls client.startTransaction('Checkout', 'ui.load').finish() (or startSpan under the leftover pageload). ext('performance') works → real sampled txn → store.add.
4. Cleanup calls stop() again, or flush(). Composed wrapper runs uploader.flush() and POSTs /v2/performance/transactions.

Same hole existed behind client.ext('performance') (PR 46 residual). This PR makes that path the public, recommended API and does not close it. Tests cover "performanceMonitoring: false" and repeat-launch, not stop/kill.

Fix: Resolve no-op when the client is not live, e.g.

function performanceApiOf(client: BugseeClient): PerformanceApi | undefined {
  if (!client.isLaunched()) return undefined;
  try {
    return client.ext('performance');
  } catch {
    return undefined;
  }
}

isLaunched() is already false after both stop() and enterKillState. Add a test: startTransaction after stop() is unsampled; a following flush() / stop() must not POST a performance batch.

[cursor]

P2 — performanceUploadMode is a public, documented option that nothing reads

This field is new on UmbrellaExtensionOptions. The docstring describes two working cadences. PERFORMANCE_OPTION_DEFINITIONS already maps the friendly name, and wireUmbrella does resolveLaunchOptions(..., PERFORMANCE_OPTION_DEFINITIONS), but the resolved PerformanceOption.UploadMode is never passed on.

wirePerformance / createPerformanceUploader have no upload-mode parameter. The only delivery path is scheduler.setInterval(..., flushIntervalMs) (default 30s). There is no per-transaction send.

Before this PR, launch(token, { performanceUploadMode: 'realtime' }) was an excess-property TypeScript error. Now it typechecks, is documented, and is pinned by public-surface.test-d.ts — as a silent no-op.

Impact: Callers who set 'realtime' (crash-before-next-flush, long-lived SPA that should not wait 30s, anything that is not calling flush() on a timer) still get batched uploads and can lose traces. The type system now encourages the broken setting.

Scenario: launch(token, { performanceUploadMode: 'realtime' }), finish a transaction, process dies or the tab crashes before the 30s tick and before an explicit flush(). The option promised an immediate upload; the store still waits for the interval.

Fix: Either implement it (on each sampled onFinish, if mode is realtime, void uploader.flush() — the uploader already sends whatever drain() returns) or keep the canonical key internal and drop this field from the public launch options until the uploader honors it. A type-only "known gap" should not look like a working API.

[krassx]

Review findings triaged against current main (this PR branch is a frozen snapshot, so its diff does not show the fixes): Facade now records nothing outside a launched client (5bf4791); performanceUploadMode 'realtime' is honoured (dc5fc29); uploader stops when the client is no longer launched (31208e3).

The overview PR #46 (head = main) was re-reviewed at ba608c4: no remaining findings, recommendation Approve.

🤖 Generated with Claude Code

[krassx]

Closing instead of merging: this review-only branch is a frozen snapshot that now conflicts with the review base branch after #37/#40/#43/#46 were merged. Its content is already on main, and every finding was addressed there (see the earlier triage comment; #46 was approved at the final review).

🤖 Generated with Claude Code