Skip to content

Commit bd968fa

Browse files
marthakellyclaude
andcommitted
fix(inspector/telemetry): fix banner_viewed race and dead opt-out check
- Defer trackBannerViewed until runtime connection is established via pendingBannerViewed + flushPendingBannerViewed(), preventing the race where the CDN response beats the /info handshake and fires the event before core.telemetryDisabled is known - Remove dead isTelemetryOptedOut() short-circuit from track(); opt-out is enforced at call sites via core.telemetryDisabled, not localStorage Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent aee390a commit bd968fa

3 files changed

Lines changed: 25 additions & 12 deletions

File tree

packages/web-inspector/src/index.ts

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2436,6 +2436,7 @@ export class WebInspectorElement extends LitElement {
24362436
// to per-mount than per-tab (sessionStorage), but for the inspector the
24372437
// distinction is academic — inspector instances rarely outlive the page.
24382438
private viewedBannerTimestamps: Set<string> = new Set();
2439+
private pendingBannerViewed: { banner_id: string; cta_label?: string } | null = null;
24392440
// Per-instance dedup for `oss.inspector.banner_clicked` (keyed by
24402441
// `${bannerId}:${cta}`) so copy-button retries and accidental multi-clicks
24412442
// don't inflate funnel counts beyond one signal per intent type per banner.
@@ -2634,6 +2635,7 @@ export class WebInspectorElement extends LitElement {
26342635
ensureTelemetryDistinctId();
26352636
maybeShowDisclosure();
26362637
}
2638+
this.flushPendingBannerViewed();
26372639
for (const agentId of this._ownedThreadStores.keys()) {
26382640
this.refreshOwnedThreadStore(agentId);
26392641
}
@@ -7401,6 +7403,16 @@ ${prettyEvent}</pre
74017403
</div>`;
74027404
}
74037405

7406+
private flushPendingBannerViewed(): void {
7407+
if (!this.pendingBannerViewed || this.core?.telemetryDisabled) {
7408+
this.pendingBannerViewed = null;
7409+
return;
7410+
}
7411+
if (this.runtimeStatus !== "connected") return;
7412+
trackBannerViewed(this.pendingBannerViewed);
7413+
this.pendingBannerViewed = null;
7414+
}
7415+
74047416
private ensureAnnouncementLoading(): void {
74057417
if (
74067418
this.announcementPromise ||
@@ -7484,19 +7496,20 @@ ${prettyEvent}</pre
74847496
this.announcementHtml = await this.convertMarkdownToHtml(markdown);
74857497
this.announcementLoaded = true;
74867498

7487-
// banner_viewed: gate on actual visibility (hasUnseenAnnouncement)
7488-
// and per-mount de-dup so re-fetches don't double-count.
7499+
// banner_viewed: gate on actual visibility and per-mount dedup.
7500+
// Store as pending rather than firing immediately — telemetryDisabled
7501+
// may not be known yet if /info hasn't returned. Flushed in
7502+
// onRuntimeConnectionStatusChanged once the handshake completes.
74897503
if (
74907504
this.hasUnseenAnnouncement &&
74917505
!this.viewedBannerTimestamps.has(timestamp)
74927506
) {
74937507
this.viewedBannerTimestamps.add(timestamp);
7494-
if (!this.core?.telemetryDisabled) {
7495-
trackBannerViewed({
7496-
banner_id: timestamp,
7497-
cta_label: ctaLabel ?? undefined,
7498-
});
7499-
}
7508+
this.pendingBannerViewed = {
7509+
banner_id: timestamp,
7510+
cta_label: ctaLabel ?? undefined,
7511+
};
7512+
this.flushPendingBannerViewed();
75007513
}
75017514

75027515
this.requestUpdate();

packages/web-inspector/src/lib/__tests__/telemetry.test.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ describe("track()", () => {
8686
expect(typeof body.ts).toBe("number");
8787
});
8888

89-
it("does not send when the user is opted out", async () => {
89+
it("sends regardless of localStorage opt-out — callers gate on core.telemetryDisabled", async () => {
9090
setTelemetryOptOut(true);
9191
expect(isTelemetryOptedOut()).toBe(true);
9292

@@ -96,7 +96,9 @@ describe("track()", () => {
9696
});
9797
await Promise.resolve();
9898

99-
expect(fetchMock).not.toHaveBeenCalled();
99+
// track() no longer short-circuits on localStorage; opt-out is enforced
100+
// at the call site via core.telemetryDisabled before track*() is invoked.
101+
expect(fetchMock).toHaveBeenCalledTimes(1);
100102
});
101103

102104
it("swallows fetch failures (telemetry is best-effort)", async () => {

packages/web-inspector/src/lib/telemetry.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,6 @@ export function track(
6464
event: TelemetryEvent,
6565
properties: Record<string, unknown> = {},
6666
): void {
67-
if (isTelemetryOptedOut()) return;
68-
6967
const distinctId = getOrCreateTelemetryDistinctId();
7068
const body = JSON.stringify({
7169
event,

0 commit comments

Comments
 (0)