Skip to content

fix(webview): drop caller sampler rows under producer-owned trace names - #160

Merged
krassx merged 2 commits into
mainfrom
fix/issue-158
Oct 29, 2026
Merged

krassx merged 2 commits into
mainfrom
fix/issue-158

Conversation

@krassx

@krassx krassx commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Fixes #158

launchWithSeams passed a caller systemMetricsSampler's rows to the system traces provider unfiltered, so names a running producer owns (battery, charging, the two battery times, route, view, url when a url producer is built) got a second writer. WEBVIEW_HOST_OWNED_TRACES only partly covered this: it never lists route/url, and it only applies while native owns the names (a hostOwnedTraces: [] reply hands the battery names and view back).

🤖 Generated with Claude Code

krassx and others added 2 commits October 29, 2026 18:47
…es (#158)

launchWithSeams passed systemMetricsSampler rows to the system traces provider
unfiltered, so battery/charging/route/view/url (and the battery times) got a
second writer, notably when native hands a host-owned name back. Add a shared
withoutTraceNames helper to @bugsee/capture and apply it in the webview launch.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…review)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 29, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 28 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a5fd6a91-9e95-468b-a76c-8cc1224c916d
Commits

Reviewing files that changed from the base of the PR and between 4d3cb27 and e0ffd13.

Files selected for processing: 5
  • packages/capture/src/index.ts
  • packages/capture/src/system-traces-provider.test.ts
  • packages/capture/src/system-traces-provider.ts
  • packages/webview/src/launch.test.ts
  • packages/webview/src/launch.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Claude finished @krassx's task in 27s —— View job


Review of PR #160: drop caller sampler rows under producer-owned trace names

I read the diff against the surrounding code and found no real bugs. I did not run the test suite, lint or typecheck; this is a static review.

What I checked

  • The owned set matches what is built. With sensors: false, createBrowserSystemTraceProducers returns only [battery, ...url, ...screen] (packages/browser/src/trace-producers.ts:757-762). That is exactly the set producerOwnedNames lists: the four battery names, route, view, and url when built.
  • route and view are always built here. screenTrackerFor never returns undefined (packages/core/src/screen-state.ts:13-20), so routeViewState is always set and ScreenProducer is always built.
  • url follows the producer. urlProducerDepsFor returns undefined when win is missing or navigationSourceFor returns nothing (packages/browser/src/url-producer.ts:148,166). urlDeps !== undefined therefore tracks whether a UrlProducer is really built, which is the point of the second commit. A sampler can still write url when no producer owns it, and the two new tests cover that.
  • Nothing is dropped twice. The provider still applies live suppression to the sampler's rows (system-traces-provider.ts:239). When native hands a name back with hostOwnedTraces: [], the producer is the only writer, which is the scenario the main test sets up.
  • Evaluation order is the same. urlProducerDepsFor now runs once, just before createSystemTracesProvider. The conditional that used to wrap it is unchanged: it is skipped when captureUrl is false.
  • Error handling is the same. A sampler that throws or returns malformed rows still throws inside the provider's guarded sample path. The onError handling around the sampler call (~line 213) is unchanged.

Overall risk: Low.

Merge recommendation: Approve. This is a small, scoped fix with tests for each owned name, for native handing names back, and for the two cases where url is left to the sampler.

Most important issues to fix: None.

Nit, no action needed: withoutTraceNames calls .filter, so a sampler returning a non-array iterable (a Set or a generator) would now throw where for…of used to accept it. The declared type is readonly TraceSample[] and the provider catches the throw, so I don't count it as a finding.

Positives

@krassx
krassx merged commit f0bd551 into main Oct 29, 2026
8 checks passed
@krassx
krassx deleted the fix/issue-158 branch October 29, 2026 16:11
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.

webview: launchWithSeams passes a caller systemMetricsSampler unfiltered, so producer-owned traces get two writers

1 participant