fix: analytics data loss + add live Chrome support#68
fix: analytics data loss + add live Chrome support#68
Conversation
- Gate /api/version ping to interactive-only (skip CI/agent), send machine ID as query param for correlation with PostHog distinctId - Fix captureImmediate: Effect.sync → Effect.tryPromise so the HTTP promise is properly awaited instead of fire-and-forget - Fix flush: remove Effect.ignore, handle errors at Analytics service layer with logging instead of silent swallow - Fix session-analytics: use ManagedRuntime so machineId() runs once and distinctId stays stable across calls, remove redundant error swallowing
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
aidenybai
changed the title
commit: |
Test Results❌ Website Test: failed8 passed, 3 failed out of 11 steps — 602s
Session Recordinghttps://github.com/millionco/expect/releases/download/ci-pr-68/2a919a4baed09c26223889391a69a92f.webm |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/browser/src/mcp/server.ts">
<violation number="1" location="packages/browser/src/mcp/server.ts:118">
P2: The open-tool response can incorrectly claim "(system Chrome)" even when system Chrome was not used.</violation>
</file>
<file name="packages/browser/src/chrome-launcher.ts">
<violation number="1" location="packages/browser/src/chrome-launcher.ts:203">
P2: The `Effect.callback` polling loop doesn't return a cleanup finalizer, so fiber interruption leaks the `setTimeout` chain (up to 30 seconds of unnecessary file I/O). Capture the timer handle and return a cleanup `Effect` to cancel it on interruption.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Fix systemChrome suffix showing when CDP URL was provided (system Chrome is ignored in that case) - Add cleanup finalizer to Effect.callback polling loop in chrome-launcher to prevent setTimeout leak on fiber interruption - Add Effect.catchCause error handling to analytics trackEvent/flush to prevent unhandled promise rejections from fire-and-forget calls
Add Microsoft Edge executable paths (LOCALAPPDATA, PROGRAMFILES, PROGRAMFILES(X86)) to Windows system Chrome discovery, matching parity with macOS and Linux which already include Edge.
| import { usePreferencesStore } from "../stores/use-preferences"; | ||
|
|
||
| const analyticsLayer = Analytics.layerPostHog; | ||
| const analyticsRuntime = ManagedRuntime.make(Analytics.layerPostHog); |
There was a problem hiding this comment.
Use Effect.catchCause instead of Effect.catchTag("UnknownError") for consistency in error handling
The error cleanup path skipped browser.close() whenever cdpEndpoint was truthy, which was designed for user-provided CDP endpoints (externally managed Chrome). System Chrome sets cdpEndpoint from its own launch, so the Playwright CDP client was never released on failure. Now only skip browser.close() for user-provided CDP (when no chromeProcess).
The @expect/browser package had no testTimeout config, defaulting to 5000ms. Cross-browser E2E tests (webkit, firefox launches) and cookie injection tests exceed this on Windows CI. Set testTimeout: 0 to match the project convention documented in AGENTS.md.
| import { usePreferencesStore } from "../stores/use-preferences"; | ||
|
|
||
| const analyticsLayer = Analytics.layerPostHog; | ||
| const analyticsRuntime = ManagedRuntime.make(Analytics.layerPostHog); |
- Early return in Analytics when telemetry disabled (skip machineId, capture, track) - Replace ternary with early return in Browser.createPage - Use async fs (fs.promises) in chrome-launcher instead of sync - Namespace fs/path imports in llms.txt route
There was a problem hiding this comment.
4 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/browser/src/chrome-launcher.ts">
<violation number="1" location="packages/browser/src/chrome-launcher.ts:197">
P1: Do not ignore all errors when deleting `DevToolsActivePort`; swallowing removal failures can allow stale CDP port data to be reused.</violation>
</file>
<file name="apps/cli/src/utils/session-analytics.ts">
<violation number="1">
P2: Using `Effect.runPromise` with `Effect.provide(analyticsLayer)` per call rebuilds the Analytics layer each invocation; this can change `distinctId` across events on fallback paths and breaks stable session analytics identity.</violation>
</file>
<file name="packages/shared/src/analytics/analytics.ts">
<violation number="1">
P2: `flush` is piped through `Effect.ignore`, so flush failures are silently swallowed. This hides analytics delivery problems.</violation>
<violation number="2">
P1: `capture` wraps `captureImmediate` in `Effect.sync`, which drops async completion/failure tracking. Use a promise-aware Effect so capture is actually awaited.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| yield* Effect.tryPromise(() => | ||
| fs.promises.rm(path.join(userDataDir, "DevToolsActivePort"), { force: true }), | ||
| ).pipe(Effect.catchTag("UnknownError", () => Effect.void)); |
There was a problem hiding this comment.
P1: Do not ignore all errors when deleting DevToolsActivePort; swallowing removal failures can allow stale CDP port data to be reused.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/browser/src/chrome-launcher.ts, line 197:
<comment>Do not ignore all errors when deleting `DevToolsActivePort`; swallowing removal failures can allow stale CDP port data to be reused.</comment>
<file context>
@@ -183,13 +183,18 @@ export const launchSystemChrome = Effect.fn("launchSystemChrome")(function* (opt
- fs.rmSync(path.join(userDataDir, "DevToolsActivePort"), { force: true });
+ yield* Effect.tryPromise(() =>
+ fs.promises.rm(path.join(userDataDir, "DevToolsActivePort"), { force: true }),
+ ).pipe(Effect.catchTag("UnknownError", () => Effect.void));
const args = buildLaunchArgs({ headless: options.headless, userDataDir });
</file context>
| ).pipe(Effect.catchTag("UnknownError", () => Effect.void)); | |
| ).pipe(Effect.catchTag("UnknownError", Effect.die)); |
| @@ -1,6 +1,6 @@ | |||
| import { Config, Effect, Layer, Option, ServiceMap } from "effect"; | |||
There was a problem hiding this comment.
P1: capture wraps captureImmediate in Effect.sync, which drops async completion/failure tracking. Use a promise-aware Effect so capture is actually awaited.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/shared/src/analytics/analytics.ts, line 40:
<comment>`capture` wraps `captureImmediate` in `Effect.sync`, which drops async completion/failure tracking. Use a promise-aware Effect so capture is actually awaited.</comment>
<file context>
@@ -37,13 +37,13 @@ export class AnalyticsProvider extends ServiceMap.Service<
static layerPostHog = Layer.succeed(this)({
capture: (event) =>
- Effect.tryPromise(() =>
+ Effect.sync(() => {
posthogClient.captureImmediate({
event: event.eventName,
</file context>
| @@ -1,6 +1,6 @@ | |||
| import { Config, Effect, Layer, Option, ServiceMap } from "effect"; | |||
There was a problem hiding this comment.
P2: flush is piped through Effect.ignore, so flush failures are silently swallowed. This hides analytics delivery problems.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/shared/src/analytics/analytics.ts, line 60:
<comment>`flush` is piped through `Effect.ignore`, so flush failures are silently swallowed. This hides analytics delivery problems.</comment>
<file context>
@@ -54,7 +54,10 @@ export class AnalyticsProvider extends ServiceMap.Service<
+ flush: Effect.tryPromise({
+ try: () => posthogClient.flush(),
+ catch: (cause) => cause,
+ }).pipe(Effect.ignore),
});
</file context>
Simplifies browser connection to two clear modes: Playwright bundled Chromium (default) or connect to an already-running Chrome (liveChrome). Removes the confusing middle ground of launching a fresh system Chrome binary as a fallback.
| sourcemap: true, | ||
| }, | ||
| test: { | ||
| testTimeout: 0, |
There was a problem hiding this comment.
Test timeout disabled globally in committed config
Medium Severity
testTimeout: 0 disables test timeouts entirely for the browser package. Any test that hangs (common with browser/CDP tests) will block CI indefinitely instead of failing after a reasonable threshold.
| if (!cached) { | ||
| cached = getRawMachineId(); | ||
| } | ||
| return cached; |
There was a problem hiding this comment.
Rejected machineId promise is permanently cached
Low Severity
If getRawMachineId() rejects (e.g., transient filesystem error), the rejected promise is stored in cached and returned on every subsequent call. There's no retry path — the module is permanently broken for the process lifetime.
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/browser/src/browser.ts">
<violation number="1" location="packages/browser/src/browser.ts:351">
P2: Avoid closing externally supplied CDP sessions on setup errors. With `options.cdpUrl`, `isExternalBrowser` is false, so this cleanup now calls `browser.close()` and can shut down a user-provided Chrome instance.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return yield* setupPage.pipe( | ||
| Effect.tapError(() => { | ||
| if (cdpEndpoint) return Effect.void; | ||
| if (isExternalBrowser) return Effect.void; |
There was a problem hiding this comment.
P2: Avoid closing externally supplied CDP sessions on setup errors. With options.cdpUrl, isExternalBrowser is false, so this cleanup now calls browser.close() and can shut down a user-provided Chrome instance.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/browser/src/browser.ts, line 351:
<comment>Avoid closing externally supplied CDP sessions on setup errors. With `options.cdpUrl`, `isExternalBrowser` is false, so this cleanup now calls `browser.close()` and can shut down a user-provided Chrome instance.</comment>
<file context>
@@ -322,20 +343,16 @@ export class Browser extends ServiceMap.Service<Browser>()("@browser/Browser", {
- ),
- ),
+ Effect.tapError(() => {
+ if (isExternalBrowser) return Effect.void;
+ return Effect.tryPromise(() => browser.close()).pipe(
+ Effect.catchTag("UnknownError", () => Effect.void),
</file context>
| if (isExternalBrowser) return Effect.void; | |
| if (isExternalBrowser || cdpEndpoint) return Effect.void; |
There was a problem hiding this comment.
Additional Suggestions:
- flush operation silently swallows all failures due to Effect.ignore, preventing error logging and hiding analytics delivery problems
- capture function uses Effect.sync which doesn't wait for PostHog's captureImmediate Promise to complete, causing analytics events to be fire-and-forget
aidenybai
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
JiwaniZakir
left a comment
JiwaniZakir
left a comment
There was a problem hiding this comment.
In apps/website/app/llms.txt/route.ts, both readFileSync calls happen at module load time rather than inside the GET handler. In a serverless/edge environment, process.cwd() may not resolve to the monorepo root as expected, and any missing file will crash the module on cold start instead of returning a graceful HTTP error response — wrapping these reads inside the handler (or using try/catch with a fallback NextResponse) would be more resilient.
In apps/cli/src/index.tsx, the promise chain .catch(() => "unknown").then((mid) => { fetch(...) }) is functionally correct but the ordering is unconventional — typically .then() precedes .catch(), so this reads as if the fetch only fires on failure. Reversing to .then((mid) => fetch(...)).catch(() => {}) would be clearer and also silences the dangling fetch rejection in one step.
For the live Chrome support in browser.ts, the isExternalBrowser flag is set from Option.isSome(liveBrowser) but the diff is truncated before showing how it's used. It's worth confirming that browser cleanup differentiates between browser.disconnect() (for externally-connected CDP sessions) and browser.close() (for launched browsers) — conflating these can leave the user's live Chrome instance in a closed state unexpectedly.
3 participants
Why
Agents couldn't test against authenticated state. When an agent runs
open_browser, it launches Playwright's bundled Chromium — a clean browser with no cookies, sessions, or extensions. If you're testing a change behind auth (e.g. a dashboard, settings page, internal tool), the agent sees a login wall and can't proceed. The user's real Chrome already has all the sessions and state needed.What changed
Live Chrome support:
liveChromeoption onopen_browserMCP tool — connects to the user's already-running Chrome via CDP auto-discoverycloseonly closes the page (not the whole browser)Other:
@expect/shared/machine-idmodule used by analytics and CLI version ping/api/versionping to interactive sessions only (skip CI/agent), send machine ID for correlationNote
Medium Risk
Touches browser session creation/teardown and CDP connection behavior, which can cause flaky automation or unexpected tab/browser lifecycle effects across platforms despite added tests and fallbacks.
Overview
Adds live Chrome support to the browser MCP
openflow via a newliveChromeoption, which auto-discovers a running Chrome CDP endpoint, connects viachromium.connectOverCDP, and adjusts lifecycle handling (don’t reuse existing tabs;closeonly closes the page and skips closing the external browser).Hardens CDP discovery by parsing
DevToolsActivePortvia a shared utility, expands supported user-data dirs, and introduces achrome-launcherutility (system Chrome discovery/launch + cleanup) plus new error types and timeouts. Separately, analytics/version pings now use a cached shared@expect/shared/machine-idmodule and the CLI gates version pings to interactive runs while includingmid; the websitellms.txtroute now serves combined repoREADME.md+ skill content.Written by Cursor Bugbot for commit f348d3e. This will update automatically on new commits. Configure here.