Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
skoshx
force-pushed
the
chore/browser-mcp-cleanup
branch
from
31982af to
0676fb4
Compare
skoshx
force-pushed
the
chore/browser-mcp-cleanup
branch
from
0676fb4 to
712bd74
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 100 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
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/playwright.ts">
<violation number="1" location="packages/browser/src/playwright.ts:237">
P1: Duplicate `context.addInitScript(RUNTIME_SCRIPT)` call. The init script is registered twice on the same context, so it will execute twice on every page load/navigation. This can cause duplicate rrweb listeners and other side effects from the runtime script. Remove the second call.</violation>
</file>
<file name="packages/browser/src/mcp-server.ts">
<violation number="1" location="packages/browser/src/mcp-server.ts:117">
P2: The `cookies` parameter is accepted in the input schema but never used in the handler. Users toggling this flag will have no effect—cookie injection is driven solely by the `browserProfile` env config.</violation>
<violation number="2" location="packages/browser/src/mcp-server.ts:319">
P2: CLS is omitted from the `hasMetrics` check, despite being listed in the tool's description. A page with only CLS data (or CLS = 0) will be incorrectly reported as having no metrics. Also note that falsy checks won't work for CLS since `0` is a valid "good" score—consider checking for `!== undefined` instead.</violation>
</file>
<file name="apps/cli/src/hooks/use-installed-browsers.ts">
<violation number="1" location="apps/cli/src/hooks/use-installed-browsers.ts:15">
P2: Handle `ListBrowsersError` before `runPromise`. Without a catch, browser detection failures reject the query, leaving the UI with no data and no fallback handling.</violation>
</file>
<file name="apps/cli/package.json">
<violation number="1" location="apps/cli/package.json:89">
P1: `@expect/website` is used at runtime but was added to `devDependencies`; it should be a production dependency to avoid runtime module resolution failures.</violation>
</file>
<file name="packages/supervisor/src/tail.ts">
<violation number="1" location="packages/supervisor/src/tail.ts:14">
P1: Handle file truncation by resetting the read offset when file size shrinks; otherwise tailing can stop emitting new data after rotation/truncate.</violation>
</file>
<file name="apps/cli/src/data/execution-atom.ts">
<violation number="1" location="apps/cli/src/data/execution-atom.ts:57">
P2: Forward `onConfigOptions` into the execute options; otherwise config option updates never reach the UI.</violation>
</file>
<file name="apps/cli/src/components/screens/testing-screen.tsx">
<violation number="1" location="apps/cli/src/components/screens/testing-screen.tsx:417">
P3: Remove the debug console.error. It adds noisy stderr output in normal CLI runs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const readNewBytes = Effect.gen(function* () { | ||
| const stat = yield* fileSystem.stat(filePath).pipe(Effect.orDie); | ||
| const size = Number(stat.size); | ||
| if (size <= offset) return new Uint8Array(0); |
There was a problem hiding this comment.
P1: Handle file truncation by resetting the read offset when file size shrinks; otherwise tailing can stop emitting new data after rotation/truncate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/supervisor/src/tail.ts, line 14:
<comment>Handle file truncation by resetting the read offset when file size shrinks; otherwise tailing can stop emitting new data after rotation/truncate.</comment>
<file context>
@@ -0,0 +1,46 @@
+ const readNewBytes = Effect.gen(function* () {
+ const stat = yield* fileSystem.stat(filePath).pipe(Effect.orDie);
+ const size = Number(stat.size);
+ if (size <= offset) return new Uint8Array(0);
+ const chunks = yield* fileSystem
+ .stream(filePath, { offset, bytesToRead: size - offset })
</file context>
packages/browser/src/mcp-server.ts
Outdated
| const pw = yield* Playwright; | ||
| const page = yield* pw.getPage; | ||
| const metrics = yield* evaluateRuntime(page, "getPerformanceMetrics"); | ||
| const hasMetrics = metrics.fcp || metrics.lcp || metrics.inp; |
There was a problem hiding this comment.
P2: CLS is omitted from the hasMetrics check, despite being listed in the tool's description. A page with only CLS data (or CLS = 0) will be incorrectly reported as having no metrics. Also note that falsy checks won't work for CLS since 0 is a valid "good" score—consider checking for !== undefined instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/browser/src/mcp-server.ts, line 319:
<comment>CLS is omitted from the `hasMetrics` check, despite being listed in the tool's description. A page with only CLS data (or CLS = 0) will be incorrectly reported as having no metrics. Also note that falsy checks won't work for CLS since `0` is a valid "good" score—consider checking for `!== undefined` instead.</comment>
<file context>
@@ -0,0 +1,360 @@
+ const pw = yield* Playwright;
+ const page = yield* pw.getPage;
+ const metrics = yield* evaluateRuntime(page, "getPerformanceMetrics");
+ const hasMetrics = metrics.fcp || metrics.lcp || metrics.inp;
+ if (!hasMetrics) return textResult("No performance metrics available yet.");
+ return jsonResult(metrics);
</file context>
| const browsers = yield* browsersService.list; | ||
| const defaultBrowser = yield* browsersService.defaultBrowser(); | ||
| return { default: defaultBrowser, browsers }; | ||
| }).pipe(Effect.provide(layerLive), Effect.provide(NodeServices.layer), Effect.runPromise), |
There was a problem hiding this comment.
P2: Handle ListBrowsersError before runPromise. Without a catch, browser detection failures reject the query, leaving the UI with no data and no fallback handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/cli/src/hooks/use-installed-browsers.ts, line 15:
<comment>Handle `ListBrowsersError` before `runPromise`. Without a catch, browser detection failures reject the query, leaving the UI with no data and no fallback handling.</comment>
<file context>
@@ -1,48 +1,16 @@
+ const browsers = yield* browsersService.list;
+ const defaultBrowser = yield* browsersService.defaultBrowser();
+ return { default: defaultBrowser, browsers };
+ }).pipe(Effect.provide(layerLive), Effect.provide(NodeServices.layer), Effect.runPromise),
});
</file context>
| }).pipe(Effect.provide(layerLive), Effect.provide(NodeServices.layer), Effect.runPromise), | |
| }).pipe( | |
| Effect.provide(layerLive), | |
| Effect.provide(NodeServices.layer), | |
| Effect.tapCause((cause) => Effect.logWarning("Browser detection failed", { cause })), | |
| Effect.catchCause(() => | |
| Effect.succeed({ default: Option.none<Browser>(), browsers: [] as Browser[] }), | |
| ), | |
| Effect.runPromise, | |
| ), |
| ), | ||
| ); | ||
|
|
||
| const finalExecuted = yield* executor.execute(input.options).pipe( |
There was a problem hiding this comment.
P2: Forward onConfigOptions into the execute options; otherwise config option updates never reach the UI.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/cli/src/data/execution-atom.ts, line 57:
<comment>Forward `onConfigOptions` into the execute options; otherwise config option updates never reach the UI.</comment>
<file context>
@@ -180,35 +41,23 @@ export const executeAtomFn = cliAtomRuntime.fn(
+ ),
+ );
+
+ const finalExecuted = yield* executor.execute(input.options).pipe(
Stream.tap((executed) =>
- Effect.gen(function* () {
</file context>
| const finalExecuted = yield* executor.execute(input.options).pipe( | |
| const finalExecuted = yield* executor.execute({ | |
| ...input.options, | |
| onConfigOptions: input.onConfigOptions, | |
| }).pipe( |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
packages/browser/src/mcp-server.ts
Outdated
| const pw = yield* Playwright; | ||
| const page = yield* pw.getPage; | ||
| const metrics = yield* evaluateRuntime(page, "getPerformanceMetrics"); | ||
| const hasMetrics = metrics.fcp || metrics.lcp || metrics.inp; |
There was a problem hiding this comment.
CLS is omitted from the hasMetrics check, causing pages with only CLS data to be incorrectly reported as having no metrics
packages/browser/src/mcp-server.ts
Outdated
| ), | ||
| }, | ||
| }, | ||
| ({ url, headed, cookies, waitUntil, cdp }, { signal }) => |
There was a problem hiding this comment.
The cookies parameter is accepted in the open tool's input schema but was never used in the handler, breaking the documented feature to control cookie reuse
There was a problem hiding this comment.
3 issues found across 20 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="apps/cli/src/utils/run-test.ts">
<violation number="1" location="apps/cli/src/utils/run-test.ts:65">
P1: `ExecutionTimeoutError` is never caught, so `outputReporter.onTimeout()` is never called. On timeout the user will see an unhandled-rejection stack trace instead of the clean timeout output the `OutputReporter` provides.</violation>
</file>
<file name="apps/cli/src/utils/is-headless.ts">
<violation number="1" location="apps/cli/src/utils/is-headless.ts:1">
P1: Returning `true` unconditionally forces the CLI into headless/non-interactive mode and breaks normal interactive execution paths.</violation>
</file>
<file name="apps/cli/src/index.tsx">
<violation number="1" location="apps/cli/src/index.tsx:108">
P2: `--output` is now ignored in headless mode because the call stopped forwarding it. If this flag is still supported, map it to the reporter selection (or remove the option) to avoid a silent behavior change.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 6 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/typescript-sdk/src/expect.ts">
<violation number="1" location="packages/typescript-sdk/src/expect.ts:326">
P2: User-supplied cookies are ignored because cookieImportProfiles is always set to an empty array. This prevents profile-based cookie import and breaks authenticated tests relying on `cookies` or config defaults.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| isHeadless, | ||
| cookieBrowserKeys: [...resolved.browserKeys], | ||
| // @todo(Aiden): add back, we have to pass in the specific profile we wanna use | ||
| cookieImportProfiles: [], |
There was a problem hiding this comment.
P2: User-supplied cookies are ignored because cookieImportProfiles is always set to an empty array. This prevents profile-based cookie import and breaks authenticated tests relying on cookies or config defaults.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/typescript-sdk/src/expect.ts, line 326:
<comment>User-supplied cookies are ignored because cookieImportProfiles is always set to an empty array. This prevents profile-based cookie import and breaks authenticated tests relying on `cookies` or config defaults.</comment>
<file context>
@@ -311,7 +322,8 @@ const runExecution = (
isHeadless,
- cookieBrowserKeys: [...resolved.browserKeys],
+ // @todo(Aiden): add back, we have to pass in the specific profile we wanna use
+ cookieImportProfiles: [],
baseUrl: url,
};
</file context>
There was a problem hiding this comment.
1 issue found across 12 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="apps/cli/src/live-viewer-server.ts">
<violation number="1" location="apps/cli/src/live-viewer-server.ts:38">
P1: WebSocket proxy uses `node:http` for all upstreams, which breaks when `replayHost` is HTTPS (the default). Use `node:https.request` for `https:` origins.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
There was a problem hiding this comment.
1 issue found across 5 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="apps/website/app/api/version/route.ts">
<violation number="1">
P1: This API route is incompatible with the app’s static export build (`output: "export"`), so `/api/version` won’t be served in production.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
There was a problem hiding this comment.
1 issue found across 2 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/cookies/tests/cdp-client.test.ts">
<violation number="1" location="packages/cookies/tests/cdp-client.test.ts:76">
P2: Safari extraction is executed twice, causing unnecessary work and potential test flakiness.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
JiwaniZakir
left a comment
JiwaniZakir
left a comment
There was a problem hiding this comment.
In e2e-website.ts, the CLI is invoked with both --verbose and --reporter json (lines 63–71). If --verbose writes anything to stdout rather than stderr, the collected stdout string will contain mixed verbose log lines and JSON, causing Schema.decodeEffect(TestReport.json)(stdout) to fail at parse time. It would be safer to either drop --verbose when using --reporter json, or parse only the last non-empty line of stdout (where the JSON report is likely emitted).
Additionally, the old validate-scenario.sh explicitly validated that artifacts is an object and checked status/step consistency (e.g., status === "passed" with failed steps was an error), but the new e2e-website.ts skips both of those checks — report.assertSuccess() may not cover the same invariants depending on its implementation in TestReport. Worth confirming those edge cases are still caught somewhere.
There was a problem hiding this comment.
6 issues found across 10 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/rrvideo.ts">
<violation number="1" location="packages/browser/src/rrvideo.ts:270">
P2: `convertEvents` substantially duplicates `convert` logic; this should be consolidated to avoid divergence and double-maintenance.</violation>
<violation number="2" location="packages/browser/src/rrvideo.ts:274">
P2: `resolutionRatio` needs a lower bound check in `convertEvents`; `0` or negative values can generate invalid viewport sizes and break video rendering.</violation>
<violation number="3" location="packages/browser/src/rrvideo.ts:295">
P1: Validate `speed` in `convertEvents` before dividing by it; zero/negative values can produce invalid timeout calculations.</violation>
</file>
<file name=".github/scripts/e2e.ts">
<violation number="1" location=".github/scripts/e2e.ts:136">
P2: Server provisioning condition does not match instruction selection; non-`dogfood` cases can run website tests without starting the website server.</violation>
</file>
<file name="apps/cli/src/layers.ts">
<violation number="1" location="apps/cli/src/layers.ts:40">
P1: Unsanitized `testId` is used as a file path component, enabling path traversal via `--test-id`.</violation>
</file>
<file name="packages/supervisor/src/reporter.ts">
<violation number="1" location="packages/supervisor/src/reporter.ts:85">
P2: Avoid force-casting replay events to `unknown[]`; validate/normalize decoded events before calling `convertEvents`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| options: Omit<ConvertOptions, "inputPath"> & { events: RrwebEvent[] }, | ||
| ) { | ||
| const { events } = options; | ||
| const ratio = Math.min(options.resolutionRatio ?? DEFAULT_RESOLUTION_RATIO, 1); |
There was a problem hiding this comment.
P2: resolutionRatio needs a lower bound check in convertEvents; 0 or negative values can generate invalid viewport sizes and break video rendering.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/browser/src/rrvideo.ts, line 274:
<comment>`resolutionRatio` needs a lower bound check in `convertEvents`; `0` or negative values can generate invalid viewport sizes and break video rendering.</comment>
<file context>
@@ -235,36 +257,116 @@ export class RrVideo extends ServiceMap.Service<RrVideo>()("@expect/browser/RrVi
+ options: Omit<ConvertOptions, "inputPath"> & { events: RrwebEvent[] },
+ ) {
+ const { events } = options;
+ const ratio = Math.min(options.resolutionRatio ?? DEFAULT_RESOLUTION_RATIO, 1);
+ const scaleFactor = ratio * MAX_SCALE_VALUE;
+ if (events.length === 0) {
</file context>
| const ratio = Math.min(options.resolutionRatio ?? DEFAULT_RESOLUTION_RATIO, 1); | |
| const ratio = Math.max(0.01, Math.min(options.resolutionRatio ?? DEFAULT_RESOLUTION_RATIO, 1)); |
skoshx
force-pushed
the
chore/browser-mcp-cleanup
branch
from
d9d3dcc to
de7d156
Compare
skoshx
force-pushed
the
chore/browser-mcp-cleanup
branch
2 times, most recently
from
d9d3dcc to
04af48e
Compare
skoshx
force-pushed
the
chore/browser-mcp-cleanup
branch
from
04af48e to
d39c9f8
Compare
There was a problem hiding this comment.
4 issues found across 32 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/shared/src/constants.ts">
<violation number="1" location="packages/shared/src/constants.ts:13">
P2: This changes the shared default replay host to an environment-specific domain, creating inconsistent CLI defaults and likely broken default routing for `expect watch` users.</violation>
</file>
<file name="packages/browser/src/mcp-server.ts">
<violation number="1" location="packages/browser/src/mcp-server.ts:334">
P2: `clear` currently wipes all artifact types, not just network requests. This can unexpectedly delete console/performance artifacts from the session.</violation>
</file>
<file name="packages/supervisor/src/artifact-store.ts">
<violation number="1" location="packages/supervisor/src/artifact-store.ts:89">
P1: `Effect.orDie` in `listTests` turns per-file decode/read failures into defects, so one corrupted artifact can fail the entire test listing.</violation>
<violation number="2" location="packages/supervisor/src/artifact-store.ts:100">
P2: `listTests` uses unbounded concurrency for per-file streaming reads, which can exhaust file descriptors on large artifact sets.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| return fileSystem.stream(filePath).pipe( | ||
| Stream.pipeThroughChannel(Ndjson.decodeSchema(ArtifactPayloadJson)()), | ||
| Stream.runHead, | ||
| Effect.orDie, |
There was a problem hiding this comment.
P1: Effect.orDie in listTests turns per-file decode/read failures into defects, so one corrupted artifact can fail the entire test listing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/supervisor/src/artifact-store.ts, line 89:
<comment>`Effect.orDie` in `listTests` turns per-file decode/read failures into defects, so one corrupted artifact can fail the entire test listing.</comment>
<file context>
@@ -0,0 +1,114 @@
+ return fileSystem.stream(filePath).pipe(
+ Stream.pipeThroughChannel(Ndjson.decodeSchema(ArtifactPayloadJson)()),
+ Stream.runHead,
+ Effect.orDie,
+ Effect.flatMap((head) =>
+ Effect.gen(function* () {
</file context>
packages/shared/src/constants.ts
Outdated
| export const LIVE_VIEWER_RPC_URL = `ws://localhost:${LIVE_VIEWER_RPC_PORT}/rpc`; | ||
| export const LIVE_VIEWER_STATIC_PORT = 38931; | ||
| export const LIVE_VIEWER_STATIC_URL = `http://localhost:${LIVE_VIEWER_STATIC_PORT}`; | ||
| export const DEFAULT_REPLAY_HOST = "https://expect-jfxubhoxd.ami.construction"; |
There was a problem hiding this comment.
P2: This changes the shared default replay host to an environment-specific domain, creating inconsistent CLI defaults and likely broken default routing for expect watch users.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/shared/src/constants.ts, line 13:
<comment>This changes the shared default replay host to an environment-specific domain, creating inconsistent CLI defaults and likely broken default routing for `expect watch` users.</comment>
<file context>
@@ -10,4 +10,4 @@ export const LIVE_VIEWER_RPC_PORT = 38930;
export const LIVE_VIEWER_STATIC_PORT = 38931;
export const LIVE_VIEWER_STATIC_URL = `http://localhost:${LIVE_VIEWER_STATIC_PORT}`;
-export const DEFAULT_REPLAY_HOST = "https://expect.dev";
+export const DEFAULT_REPLAY_HOST = "https://expect-jfxubhoxd.ami.construction";
</file context>
| export const DEFAULT_REPLAY_HOST = "https://expect-jfxubhoxd.ami.construction"; | |
| export const DEFAULT_REPLAY_HOST = "https://expect.dev"; |
| Effect.catchTag("NoSuchElementError", () => Effect.succeed(undefined)), | ||
| ); | ||
| }, | ||
| { concurrency: "unbounded" }, |
There was a problem hiding this comment.
P2: listTests uses unbounded concurrency for per-file streaming reads, which can exhaust file descriptors on large artifact sets.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/supervisor/src/artifact-store.ts, line 100:
<comment>`listTests` uses unbounded concurrency for per-file streaming reads, which can exhaust file descriptors on large artifact sets.</comment>
<file context>
@@ -0,0 +1,114 @@
+ Effect.catchTag("NoSuchElementError", () => Effect.succeed(undefined)),
+ );
+ },
+ { concurrency: "unbounded" },
+ ).pipe(Effect.map(Arr.filter(Predicate.isNotUndefined)));
+
</file context>
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
3 participants
No description provided.