Stabilize preview browser surfaces, automation, and recording#3565
Merged
Conversation
- Prevent stale webview presenters from overwriting active state - Keep inactive Electron guests hidden offscreen during recording - Add store tests for lease ownership and release behavior
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Hidden webview during screencast start
- Moved the activeBrowserRecordingTabIdAtom update to before the startScreencast call so the React component removes visibility:hidden before Chromium needs to paint frames; existing error cleanup already resets the atom on failure.
Or push these changes by commenting:
@cursor push 4b0679ceee
Preview (4b0679ceee)
diff --git a/apps/web/src/browser/browserRecording.ts b/apps/web/src/browser/browserRecording.ts
--- a/apps/web/src/browser/browserRecording.ts
+++ b/apps/web/src/browser/browserRecording.ts
@@ -196,6 +196,9 @@
cause,
});
}
+ // Expose the recording tab to React before starting screencast so the
+ // webview removes `visibility: hidden` and Chromium can paint frames.
+ appAtomRegistry.set(activeBrowserRecordingTabIdAtom, tabId);
try {
await bridge.recording.startScreencast(tabId);
} catch (cause) {
@@ -220,7 +223,6 @@
),
});
}
- appAtomRegistry.set(activeBrowserRecordingTabIdAtom, tabId);
return startedAt;
}You can send follow-ups to the cloud agent here.
Contributor
ApprovabilityVerdict: Needs human review This PR introduces new concurrency control mechanisms (lease-based surface ownership, recording lifecycle phases) and adds explicit tab targeting across preview automation tools. These are significant runtime behavior changes that warrant human review. You can customize Macroscope's approvability policy. Learn more. |
- Split recording surface state from active recording state - Wait for two paints before `Page.startScreencast` - Keep inactive host webviews offscreen to avoid overflow
github-actions
Bot
added
size:L
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Stop during paint wait orphans recording
- Added a guard
if (active !== recording) return recording.startedAtafterawait waitForBrowserRecordingSurfacePaint()to bail out early ifstopBrowserRecordingcleared the active recording during the async paint wait, preventing orphaned screencast and stale atom state.
- Added a guard
Or push these changes by commenting:
@cursor push 40f6776d7d
Preview (40f6776d7d)
diff --git a/apps/web/src/browser/browserRecording.ts b/apps/web/src/browser/browserRecording.ts
--- a/apps/web/src/browser/browserRecording.ts
+++ b/apps/web/src/browser/browserRecording.ts
@@ -230,6 +230,9 @@
}
appAtomRegistry.set(browserRecordingSurfaceTabIdAtom, tabId);
await waitForBrowserRecordingSurfacePaint();
+ if (active !== recording) {
+ return recording.startedAt;
+ }
try {
await bridge.recording.startScreencast(tabId);
} catch (cause) {You can send follow-ups to the cloud agent here.
- Guard screencast startup against stop requests during surface paint - Ensure late-starting screencasts are shut down cleanly
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Idempotent start ignores stopping flag
- Added
!active.stoppingguard to the same-tab early return so a recording being torn down throws BrowserRecordingConflictError instead of returning a stale startedAt timestamp.
- Added
Or push these changes by commenting:
@cursor push f9ff0cd334
Preview (f9ff0cd334)
diff --git a/apps/web/src/browser/browserRecording.ts b/apps/web/src/browser/browserRecording.ts
--- a/apps/web/src/browser/browserRecording.ts
+++ b/apps/web/src/browser/browserRecording.ts
@@ -181,7 +181,7 @@
const bridge = previewBridge;
if (!bridge) throw new BrowserRecordingUnavailableError({ tabId });
if (active) {
- if (active.tabId === tabId) return active.startedAt;
+ if (active.tabId === tabId && !active.stopping) return active.startedAt;
throw new BrowserRecordingConflictError({
requestedTabId: tabId,
activeTabId: active.tabId,You can send follow-ups to the cloud agent here.
- Preserve the tab returned by open for follow-up preview requests - Route UI open-tab resolution through the pinned provider session tab
- Add tabId to preview tool schemas and handlers - Preserve tab selection across routed requests and recordings - Update tests for tab-specific routing and contracts
- Preserve the browsing tab when a global recording stop completes - Reject overlapping browser recording start/stop transitions
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Late tab assignment dropped
- Added an early return when resultTabId is undefined so that tab-unaware responses don't advance tabSequence, allowing slower tab-producing operations to still record their tabId.
Or push these changes by commenting:
@cursor push ecc256b8fd
Preview (ecc256b8fd)
diff --git a/apps/server/src/mcp/PreviewAutomationBroker.ts b/apps/server/src/mcp/PreviewAutomationBroker.ts
--- a/apps/server/src/mcp/PreviewAutomationBroker.ts
+++ b/apps/server/src/mcp/PreviewAutomationBroker.ts
@@ -556,6 +556,9 @@
) {
return current;
}
+ if (resultTabId === undefined) {
+ return current;
+ }
const assignments = new Map(current.assignments);
if (resultTabId === null) {
const { tabId: _tabId, ...withoutTabId } = assignment;
@@ -563,7 +566,7 @@
} else {
assignments.set(assignmentKey, {
...assignment,
- ...(resultTabId === undefined ? {} : { tabId: resultTabId }),
+ tabId: resultTabId,
tabSequence: requestSequence,
});
}You can send follow-ups to the cloud agent here.
- keep earlier tab decisions from being cleared by no-tab responses - share in-flight browser recording stop work across duplicate callers
- Keep the recording slot reserved until a cancelled start settles - Cover the race in browser recording tests - Update broker expectations for tab routing
- Replace webview config flow with native surface IPC - Add smoke test coverage for hidden preview automation and handoff - Update shared preview contracts and desktop/web state
github-actions
Bot
added
size:XXL
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Occlusion capture failure sticks native
- On captureSurfaceFrame failure, the ref is now reset to allow retries and setSurface is called with false to hide the native view, ensuring overlays remain interactive.
- ✅ Fixed: Tab close leaves recording stuck
- Added stopBrowserRecording(tabId) call in the HostedBrowserWebview unmount cleanup, ensuring the renderer-side recording state is cleared before the lease is released.
Or push these changes by commenting:
@cursor push 7ad5804beb
Preview (7ad5804beb)
diff --git a/apps/web/src/browser/HostedBrowserWebview.tsx b/apps/web/src/browser/HostedBrowserWebview.tsx
--- a/apps/web/src/browser/HostedBrowserWebview.tsx
+++ b/apps/web/src/browser/HostedBrowserWebview.tsx
@@ -15,6 +15,7 @@
import { reconcileLockedAspectRatio } from "./browserDeviceToolbarState";
import { BrowserDeviceToolbar } from "./BrowserDeviceToolbar";
import { BrowserViewportResizeHandles } from "./BrowserViewportResizeHandles";
+import { stopBrowserRecording } from "./browserRecording";
import { acquireDesktopTab, type AcquiredDesktopTab } from "./desktopTabLifetime";
import {
shouldPresentNativeSurface,
@@ -52,6 +53,7 @@
tabLeaseRef.current = lease;
return () => {
if (tabLeaseRef.current === lease) tabLeaseRef.current = null;
+ void stopBrowserRecording(tabId);
void previewBridge?.setSurface(tabId, { x: 0, y: 0, width: 1, height: 1 }, false, 1);
lease.release();
};
@@ -171,9 +173,14 @@
if (cancelled) return;
if (active && surfaceOccluded && !occlusionCaptureRequestedRef.current) {
occlusionCaptureRequestedRef.current = true;
- const frame = await bridge.captureSurfaceFrame(tabId);
+ const frame = await bridge.captureSurfaceFrame(tabId).catch(() => null);
if (cancelled) return;
- setOcclusionFrame(frame);
+ if (frame) {
+ setOcclusionFrame(frame);
+ } else {
+ occlusionCaptureRequestedRef.current = false;
+ await bridge.setSurface(tabId, bounds, false, layout.viewportScale);
+ }
return;
}
await bridge.setSurface(tabId, bounds, presentNativeSurface, layout.viewportScale);You can send follow-ups to the cloud agent here.
- Replace native surface IPC with webview registration - Add preview config IPC and update desktop/web callers - Drop obsolete native preview smoke test wiring
github-actions
Bot
added
size:L
Co-authored-by: codex <codex@users.noreply.github.com>
github-actions
Bot
added
size:XL
Co-authored-by: codex <codex@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Wait-startup hides saved artifact
- Changed the catch handler in stopBrowserRecording to return cleanupAfterStartup instead of re-throwing, so the original stop promise resolves with the artifact once startup settles rather than rejecting and leaving the artifact unreachable.
Or push these changes by commenting:
@cursor push e5a8c9f929
Preview (e5a8c9f929)
diff --git a/apps/web/src/browser/browserRecording.test.ts b/apps/web/src/browser/browserRecording.test.ts
--- a/apps/web/src/browser/browserRecording.test.ts
+++ b/apps/web/src/browser/browserRecording.test.ts
@@ -251,7 +251,7 @@
expect(startCallsBeforeFirstSettled).toBe(1);
});
- it("fails a stop that waits too long for startup without freeing the recording slot", async () => {
+ it("resolves stop with the artifact after startup eventually settles", async () => {
vi.useFakeTimers();
let finishStartingScreencast: (() => void) | undefined;
startScreencast.mockImplementationOnce(async () => {
@@ -272,13 +272,8 @@
await Promise.resolve();
expect(stopScreencast).toHaveBeenCalledOnce();
- const rejection = expect(stopPromise).rejects.toMatchObject({
- operation: "wait-startup",
- tabId: "recording-tab",
- });
await vi.advanceTimersByTimeAsync(BROWSER_RECORDING_STARTUP_SETTLE_TIMEOUT_MS);
- await rejection;
expect(save).not.toHaveBeenCalled();
await expect(startBrowserRecording("recording-tab")).rejects.toBeInstanceOf(
BrowserRecordingConflictError,
@@ -286,7 +281,8 @@
finishStartingScreencast?.();
await rejectedStart;
- await stopBrowserRecording("recording-tab");
+ const artifact = await stopPromise;
+ expect(artifact).toMatchObject({ path: "/tmp/recording-test.webm" });
expect(events.at(-1)).toBe("clear");
});
});
diff --git a/apps/web/src/browser/browserRecording.ts b/apps/web/src/browser/browserRecording.ts
--- a/apps/web/src/browser/browserRecording.ts
+++ b/apps/web/src/browser/browserRecording.ts
@@ -433,7 +433,7 @@
finalizeBrowserRecording(bridge, recording),
);
recording.lifecycle = { phase: "stopping", stopPromise: cleanupAfterStartup };
- void cleanupAfterStartup.catch(() => undefined);
+ return cleanupAfterStartup;
}
throw error;
});You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 5d15645. Configure here.
Co-authored-by: codex <codex@users.noreply.github.com>
juliusmarminge
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Behavior notes
Testing
vp test run apps/web/src/browser/browserRecording.test.ts apps/web/src/browser/desktopTabLifetime.test.ts apps/web/src/browser/browserSurfaceStore.test.ts apps/web/src/browser/browserViewportLayout.test.ts apps/web/src/browser/browserViewportActions.test.ts apps/web/src/browser/hostedBrowserWebviewStyle.test.ts apps/web/src/components/preview/previewAutomationTarget.test.ts apps/server/src/mcp/PreviewAutomationBroker.test.tsvp checkvp run typecheck