Repository navigation
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
vouch:trusted
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be6c9cbe18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const onOwnerDestroyed = () => { | ||
| discardAdoptedPopupSync(runtimeTabId); | ||
| runFork(closeTab(runtimeTabId).pipe(Effect.ignore)); |
There was a problem hiding this comment.
When the desktop renderer reloads or its render process crashes, this handler closes only the desktop manager tab; it does not remove the popup session previously created on the server by ElectronBrowserHost. Because server sessions survive renderer reconnects, the new renderer lists that session with an empty native-popup reservation store and mounts a regular HostedBrowserWebview at the last URL, losing window.opener, POST state, and in-memory page contents—the exact recreation this cleanup is intended to prevent. Propagate a logical-session close or retain a tombstone that prevents this stale session from being remounted.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
be6c9cb to
c3f0d2e
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3f0d2ecd7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (message.kind === "pointer") { | ||
| cursor.style.display = "block"; |
There was a problem hiding this comment.
When an automation click in an adopted popup triggers a same-document navigation, such as a hash change or history.pushState, Manager reports did-navigate-in-page and usePreviewBridge clears the regular renderer cursor, but this guest overlay has no corresponding hide action and only ever sets the cursor to display: block. Because same-document navigation does not reload the popup's document or preload, the agent cursor remains visible indefinitely at its old position; add a native cursor-clear message on the same navigation path.
Useful? React with 👍 / 👎.
| input: { | ||
| threadId: threadRef.threadId, | ||
| requestedTabId: request.popupId, | ||
| activation: popupActivation(request), | ||
| ...(seedUrl === undefined ? {} : { url: seedUrl }), |
There was a problem hiding this comment.
When a newer Desktop client connects to a server predating requestedTabId, the older preview.open implementation can return its own generated snapshot.tabId, but this flow continues treating request.popupId as the logical session ID. The returned ID is persisted as a second recovery while ownedPopupIdsRef contains only the requested ID, so the recovery effect can classify the actual adopted session as unowned and immediately close it; cleanup also sends preview.close for the nonexistent requested ID. Use the returned snapshot.tabId consistently, or capability-gate native adoption, so popups do not fail during remote client/server version skew.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
| adopted.ownerWebContents.on("did-start-navigation", onOwnerNavigation); | ||
| adopted.view.webContents.once("destroyed", onPopupDestroyed); |
There was a problem hiding this comment.
When the adopted popup's own renderer exits because of a site crash or OOM, Electron emits render-process-gone without destroying the WebContents. This setup listens for that event only on the owning app renderer and registers merely destroyed on the popup, so the native view remains attached on Chromium's crashed-page surface while its logical session and desktop tab continue to appear live. Either recover the guest as regular hosted webviews do or close the popup session when its renderer is gone.
Useful? React with 👍 / 👎.
c3f0d2e to
c0cd37f
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0cd37fd49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| x: Math.round(lastRect.x + layout.viewportX), | ||
| y: Math.round(lastRect.y + layout.viewportY), | ||
| width: Math.max(1, Math.round(layout.viewportWidth)), | ||
| height: Math.max(1, Math.round(layout.viewportHeight)), |
There was a problem hiding this comment.
When a popup-created tab is shown in ThreadPreviewMiniPlayer, these bounds cover the entire BrowserSurfaceSlot, including the controls and resize handle rendered inside that rectangle at ThreadPreviewMiniPlayer.tsx:308-391. Because a WebContentsView paints above the renderer regardless of CSS z-index, those controls cannot receive hover or pointer events, leaving users unable to move, resize, close, reopen, or pop out the floating preview; reserve space for the controls or hide/reposition the native view while they are used.
AGENTS.md reference: AGENTS.md:L73-L73
Useful? React with 👍 / 👎.
| ...popupWebPreferences, | ||
| }, | ||
| }); | ||
| view.webContents.setWindowOpenHandler(() => ({ action: "deny" })); |
There was a problem hiding this comment.
If the newly created popup calls window.open during its initial load, before the server round trip and adoptPopup reach attachListeners, this temporary handler unconditionally denies the child request without forwarding or reporting it. Popup chains used by payment or authentication flows can therefore lose their next browsing context solely because adoption has not completed yet; install the normal popup handler immediately or queue these requests until the parent is adopted.
Useful? React with 👍 / 👎.
| }) { | ||
| const { threadRef, tabId, runtimeTabId, initialUrl, viewport, zoomFactor } = props; | ||
| const config = usePreviewWebviewConfig(threadRef.environmentId); | ||
| const nativePopup = useNativePreviewPopupStore((state) => state.tabIds.has(tabId)); |
There was a problem hiding this comment.
When another Desktop renderer is connected to the same server and thread, this renderer-local Zustand/sessionStorage set does not contain the popup ID, so that renderer mounts the fallback <webview> at the server's seed URL instead of recognizing the session as an owner-only native popup. This can replay an authentication or payment URL with a different cookie jar, display a misleading reconstructed tab, and let the second renderer close the owner's live popup. Unlike the reload recovery discussed previously, the fresh evidence is that reservation metadata never crosses renderer or device boundaries; propagate the native-popup designation through shared state or suppress reconstruction on non-owners.
AGENTS.md reference: AGENTS.md:L78-L78
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c91d74beb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (cancelled || isDisposed()) | ||
| throw new Error("The desktop popup owner was closed during adoption."); | ||
| adoptedRuntimeTabId = runtimeTabId(snapshot); | ||
| await desktop.adoptPopup(request.popupId, adoptedRuntimeTabId); |
There was a problem hiding this comment.
When a popup finishes loading before the server openSession() round trip completes—especially for about:blank/document.write popups or remote servers—adoptPopup emits its current desktop state at Manager.ts:2029, but the popup's HostedBrowserWebview and its usePreviewBridge subscription are not guaranteed to mount until the subsequent reconcile call. With no later navigation event, that one emission is lost, leaving the logical session Idle or Loading indefinitely and the address, title, controls, and server-visible status stale; establish the state listener before adoption or return/buffer the adopted state for reconciliation.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Pathway already opens ordinary new-tab links in its browser panel, but scripted popups still used separate windows and URL-only opens could lose form submissions or background activation. This change keeps popup pages in browser tabs in the same thread, preserving their Chromium contents, session, opener relationship, and original navigation.
The desktop adopts Electron's supplied guest rather than recreating it. Deferred browser-initiated opens receive their initial navigation once, with referrer and POST data. Safe preferences are applied before guest creation, and the blank-popup check now accepts Electron 41.5's actual preference shape. The implementation follows the pinned Electron guest-window lifecycle.
Validation: 268 tests passed across 17 focused files; desktop, web, server, and contracts typechecks passed; targeted lint, formatting, and diff checks passed. All CI jobs that ran passed on the latest commit; automated review is running again. Automated review findings addressed in code: reload recovery, older-server capability checks before opening a logical popup session, native cursor clearing after in-page navigation, and closing popup sessions when their renderer crashes. Convex performance fallback: no dedicated skill was installed; inspection found no changed Convex functions, queries, subscriptions, or frame payloads.
Native verification: 15/15 assertions passed on
c0cd37fd4using Electron 41.5, the actual PreviewManager and preload, a real source webview, and genuine Chromium popup contents. This covers target-blank adoption, retained source state, document.write and live opener access, POST body preservation, named-window reuse, page-driven close, background disposition/focus, and fitted CSS viewport dimensions without changing the opener's zoom. The final run also verifies that same-document navigation clears native cursor state, and a forced guest renderer crash emits popup closure and removes the native Manager tab. Server-session cleanup is covered by focused tests. Final runtime results.These screenshots show the isolated native integration fixture. They do not claim full Pathway tab-strip UI verification. Foreground focus, app-menu overlap, and complete shell interaction remain covered by focused code tests rather than an integrated shell pass. Native cursor feedback currently omits the provider badge because native pointer events do not contain provider metadata.
The cursor pair comes from the final native rerun, after compositor presentation settled. Sequential screenshots are provided; no full-shell recording was captured.
Codex task:
codex://threads/01a07e57-6fe5-71b3-b311-327109b28842. GitHub displays this as copyable text; no approved HTTPS deep-link bridge is configured.Model and harness: GPT-6 in Codex, with GPT-5.6 Sol for native evidence and automated review follow-up.