From a17c554c98e4e56bd926637b8324a59302f57198 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 01:40:42 +0000 Subject: [PATCH] Telemetry: report when the consent browser actually opens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `broker_connect_failed` with `OAUTH_TIMEOUT` currently covers two different outcomes that call for opposite responses: the user saw the Robinhood consent page and walked away, or never got a usable page at all. The events are identical, so the funnel can't tell a copy/UX problem from a defect. Adds `broker_consent_opened`, emitted from `redirectToAuthorization` after the browser is actually launched — so a launch that throws is not recorded as a page the user saw, and a silent connect (which throws `ConsentRequired` at the gate, before `openBrowser`) reports nothing. An `OAUTH_TIMEOUT` preceded by this event means abandonment; one without it means the user never saw a page. Its `armed_ms` measures the second thing worth knowing: the loopback's timeout starts at bind, before the SDK registers the client and opens the page, so a slow round-trip silently eats the consent window. The provider stamps the arming time in `beginAuthorization` (called immediately after the bind) and reports the delta at open. If that number is routinely large, the deadline is armed in the wrong place; if it's small, the timeout is doing its job and the drop-off is behavioural. Allowlist invariants hold: one `z.strictObject` with a single non-negative integer duration — no free-form string, nothing identifying, and nothing about the user's account or the authorization URL. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0128hXeESThVH1BrjUp2MbUP --- .../services/broker/robinhood/oauth.test.ts | 39 ++++++++++++++++++- .../main/services/broker/robinhood/oauth.ts | 13 +++++++ app/src/shared/analytics.ts | 9 +++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/app/src/main/services/broker/robinhood/oauth.test.ts b/app/src/main/services/broker/robinhood/oauth.test.ts index 964d088..555450b 100644 --- a/app/src/main/services/broker/robinhood/oauth.test.ts +++ b/app/src/main/services/broker/robinhood/oauth.test.ts @@ -1,8 +1,9 @@ import { Database } from "bun:sqlite"; -import { describe, expect, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; import { drizzle } from "drizzle-orm/bun-sqlite"; import type { Db } from "../../../db/client"; import * as schema from "../../../db/schema"; +import { analytics } from "../../analytics"; import { BrokerOAuthProvider, ConsentRequired } from "./oauth"; function memDb(): Db { @@ -11,6 +12,17 @@ function memDb(): Db { return drizzle(sqlite, { schema }) as unknown as Db; } +// Spy on the singleton rather than starting it with a fake client: `start` is idempotent, +// so in a full-suite run whichever file starts it first owns it and a second fake client +// would silently receive nothing. The spy asserts the call this file is about. +let tracked: ReturnType>; +beforeEach(() => { + tracked = spyOn(analytics, "track").mockImplementation(() => {}); +}); +afterEach(() => { + tracked.mockRestore(); +}); + function provider() { const opened: string[] = []; const p = new BrokerOAuthProvider({ db: memDb(), openBrowser: (u) => opened.push(u) }); @@ -60,4 +72,29 @@ describe("BrokerOAuthProvider", () => { expect(() => p.redirectToAuthorization(new URL("https://x"))).toThrow(ConsentRequired); expect(opened).toHaveLength(1); }); + + test("opening the browser reports broker_consent_opened with the budget already spent", () => { + const { p, opened } = provider(); + p.beginAuthorization("http://127.0.0.1:50123/callback"); + p.redirectToAuthorization(new URL("https://robinhood.com/oauth")); + + expect(opened).toHaveLength(1); + const calls = tracked.mock.calls.filter(([event]) => event === "broker_consent_opened"); + expect(calls).toHaveLength(1); + // How much of the consent timeout was gone before the user could act. Bounded by the + // test's own runtime, so assert the shape the allowlist requires, not a value. + const armed = (calls[0][1] as { armed_ms: number }).armed_ms; + expect(Number.isInteger(armed)).toBe(true); + expect(armed).toBeGreaterThanOrEqual(0); + }); + + test("a browser that never opened reports nothing — that's the case the event separates", () => { + const { p } = provider(); + // Silent connect: the gate throws before `openBrowser`, so an OAUTH_TIMEOUT with no + // `broker_consent_opened` means the user was never shown a page. + expect(() => p.redirectToAuthorization(new URL("https://robinhood.com/oauth"))).toThrow( + ConsentRequired, + ); + expect(tracked.mock.calls.filter(([e]) => e === "broker_consent_opened")).toHaveLength(0); + }); }); diff --git a/app/src/main/services/broker/robinhood/oauth.ts b/app/src/main/services/broker/robinhood/oauth.ts index 3e54d11..647104d 100644 --- a/app/src/main/services/broker/robinhood/oauth.ts +++ b/app/src/main/services/broker/robinhood/oauth.ts @@ -1,6 +1,7 @@ import { eq } from "drizzle-orm"; import type { Db } from "../../../db/client"; import { settings } from "../../../db/schema"; +import { analytics } from "../../analytics"; /** * Read/write the settings kv. Values are stored as plaintext JSON: the backend @@ -110,6 +111,12 @@ export class BrokerOAuthProvider { * as the browser gate: the SDK may only open a browser while this is set. */ private activeRedirectUrl: string | null = null; + /** + * When the current consent's loopback was bound — which is also when its timeout + * started running. Used only to measure how much of that budget was gone by the time + * the browser opened (`broker_consent_opened`). + */ + private armedAt: number | null = null; constructor(private opts: OAuthProviderOptions) { this.store = new SecureStore(opts.db); @@ -140,11 +147,13 @@ export class BrokerOAuthProvider { this.store.clear(K_CLIENT); this.store.clear(K_VERIFIER); this.activeRedirectUrl = redirectUrl; + this.armedAt = Date.now(); } /** The interactive consent ended (however it ended): the browser gate closes again. */ endAuthorization() { this.activeRedirectUrl = null; + this.armedAt = null; } state() { @@ -176,6 +185,10 @@ export class BrokerOAuthProvider { redirectToAuthorization(url: URL) { if (!this.activeRedirectUrl) throw new ConsentRequired(); this.opts.openBrowser(url.toString()); + // After the open, so a browser we failed to launch isn't recorded as one the user saw. + analytics.track("broker_consent_opened", { + armed_ms: this.armedAt === null ? 0 : Math.max(0, Date.now() - this.armedAt), + }); } saveCodeVerifier(verifier: string) { diff --git a/app/src/shared/analytics.ts b/app/src/shared/analytics.ts index bd79809..5951009 100644 --- a/app/src/shared/analytics.ts +++ b/app/src/shared/analytics.ts @@ -160,6 +160,15 @@ export const TELEMETRY_EVENTS = { * is attempts with no outcome event: superseded by a later click, still pending, or a * silent connect that found a dead grant and quietly stayed disconnected. */ broker_connect_started: z.strictObject({ mode: z.enum(["interactive", "silent"]) }), + /** + * The consent browser actually opened. Splits the two ways an interactive connect + * ends in `OAUTH_TIMEOUT`: the user saw the page and walked away (this event, then + * the timeout), or never got one at all (the timeout with no such event). + * `armed_ms` is the slice of the consent budget already spent when the browser + * opened — the loopback's timer starts at bind, before the SDK registers the client + * and opens the page, so a slow round-trip eats the user's window. + */ + broker_consent_opened: z.strictObject({ armed_ms: z.number().int().nonnegative() }), broker_connected: z.strictObject({}), broker_connect_failed: z.strictObject({ error_name: errorName,