feat(cu): mirror the driven window instead of competing for the screen - #1882
Conversation
fcf738f to
7a332f4
Compare
|
Repaired the lifecycle wiring this PR was split away from, plus two defects found reading the same code. What was inert The controller was built as a local inside assembleDesktopTools and never returned, so nothing in the app could reach it. complete, clearForSession, destroyAll and setStopHandler had zero production callers. The mirror was never torn down — not when a turn ended, not when a session was stopped or deleted, not when the main window closed, not on quit — so it stayed on screen with its 20Hz hover poll running until the process exited. The thirty second linger and the pip:completed effect were unreachable, and the stop button in the mirror could not stop anything. The assembly also declares a mainWindow dependency that nothing in the repo supplied, so production always took the empty-deps branch. No parent window, so the alwaysOnTop fallback applied and the mirror floated above every other application, which is the exact fault the doc comment says child-windowing removed. No cursorPoint, so hover never fired, so setIgnoreMouseEvents(false) never ran and the controls were never drawn or clickable, with drag and resize dead behind the same guard. And bounds fell back to the primary display, so on two monitors it landed on the wrong one. Every existing PiP test injects those dependencies. That is why the suite was green with none of it wired, and it is why the new tests assert the call sites rather than constructing a controller. What now wires it MainWindowController gains windowBounds, browserWindow and onWindowGeometryChanged. boot.ts passes the controller into assembleDesktopTools, destroys the mirror in onMainWindowClose alongside the cursor overlay, and forwards it to registerSessionsIpc, createSessionStreamer and wireAppLifecycle. app-lifecycle destroys it on window-all-closed and in the before-quit cleanup. sessions-ipc clears it on delete, stop and archive — the same three paths that clear the cursor — and points its stop control at the session stop. sessions:stop became a named stopSession for that, so the mirror's button and the in-app button are one function rather than two that can drift. The session streamer calls complete when a turn ends, so the mirror lingers and retires instead of outliving its run and showing that run's last frame while the next turn drives a different application. Third finding, confirmed and fixed A load failure produced a permanently invisible, undying window with no log. defaultCreateWindow gates on did-finish-load and ensure puts showInactive and the hover watch inside onReady, so a failed loadFile — dist/overlay/pip.html missing because the overlay build was not re-run is enough — emits did-fail-load instead, ready stays false forever, frames pile up in the queue, and pip-feed's bare catch swallows what comes through. One leaked window per session, indistinguishable from a mirror that is working. The Electron adapter now reports the failure once, with the asset path and the Chromium error code, and the controller treats it as gone and destroys it. No fallback, no silent recovery: the window does not leak and the failure is in the log. Fourth finding, confirmed and fixed restPoint built its anchors from the app window's rect and then clamped them with the work area of the display the mirror was currently on. With the app on an external display and the mirror on the built-in one, that computes a corner in one coordinate space and clamps it into another, parking the mirror against an arbitrary edge. Both restPoint and the drag release now clamp against the host's own display. Tests Nine new assertions in computer-use-pip-wiring.test.ts plus two in the existing behaviour file. boot.ts, app-lifecycle.ts and sessions-ipc-main.ts cannot be imported outside Electron, so their hunks are asserted against the source text the way this repo's other cross-file conventions are; the session-streamer one is a real call. Each was negative-controlled: the fix removed, the build re-run, the guarding test observed red, the fix restored and observed green. All nine wiring cases and both controller cases behaved that way. Verified locally: 71 of 71 PiP tests pass, the desktop main suite goes from 77 failures to 69 with this change and none of the remainder touch these files (they are node:sqlite and unbuildable-renderer-package artifacts of this machine). biome check clean on every touched file, check-console clean with pip-electron.ts added to the allow-list for the one load-failure diagnostic. Not verifiable here: apps/desktop cannot be built on this machine, so the mirror was not exercised against a real BrowserWindow — the did-fail-load subscription and the child-window parenting are argued from the Electron API, not observed. Note on the boot vs main call site: #1880 split boot out of main.ts, and the wiring belongs in boot.ts on current origin/main. The version of these hunks on the long-lived branch predates that split and lives in main.ts there; they were hand-applied, not checked out, and the diff against origin/main removes nothing upstream added. |
|
Second-round review fixes. The cross-PR merge hazard first, because it is the one that cannot be caught by reading either branch on its own. This PR and #1889 both rewrite the same expression, the Both PRs now do. The new assertion here reads the production expression out of tool-assembly.ts and requires Now this branch's own findings. The real-Electron smoke failed six of its thirteen checks as committed, and this half of it had never completed as written. It pressed at page (20, 20) to start the throw, and Chasing that turned up the defect it was hiding, which is the substantive fix here. Main sends the anchor down over a new The last smoke failure was the fixture rather than the product. Hover is decided from the main-side pointer, so the controls stay click-through until that pointer is on the mirror; the script only sent page events for the hide click, and the grip drag happened to leave its pointer one point past the tile's bottom edge. It moves both now and asserts the controls are on before it clicks. The run is thirteen of thirteen plus the two new checks. Two more from reading the same files. pip-feed's Every new assertion was negative-controlled by actually reverting the code, rebuilding and watching it go red, then restoring it and watching it go green. Full desktop suite is clean; the only failures I saw anywhere were shell-env and Rive process-kill timeouts under a load average of 34, and both pass in isolation. |
4eab3dc to
8a2a23e
Compare
Computer Use drives an application without bringing it to the front, which is the whole point and also the problem: the work is invisible. A person watching sees a cursor move over a window they cannot see, and the only way to check what happened is to raise the target — which is exactly what the feature promises not to do. The mirror shows the driven window in a small panel beside the app, the way Codex does. Reverse-engineered from ChatGPT.app's sky.node and recorded in docs/codex-pip-reverse-engineering.md, because two of the behaviours are not obvious and both were got wrong first: - It is a child window at level 0, not a floating panel. A panel that moves itself chases the app window and trails behind every drag; made a child it is carried by the app's own moves in the same transaction, and it stops covering other applications. - When the target is occluded it stays on top rather than hiding. Hiding is the intuitive choice and it is backwards: occlusion is the moment the mirror is the only way to see anything. It can be thrown and it can be resized, both with the constants Codex uses. The drag reads the pointer from `screen.getCursorScreenPoint()` rather than the renderer's `screenX`, because the window moves in screen points and nothing should convert between two spaces that can disagree. The overlay hook composes: `withComputerUsePip` decorates whatever hook it is given, so this adds a mirror without changing what the cursor overlay does.
The picture-in-picture controller was constructed as a local inside `assembleDesktopTools` and never returned, so nothing in the app could reach it. `complete`, `clearForSession`, `destroyAll` and `setStopHandler` had zero production callers: the mirror was never torn down when a turn ended, a session stopped, a session was deleted, the window closed or the app quit, and it sat on screen with its 20Hz hover poll running until the process exited. The thirty second linger and the completion effect were unreachable code, and the stop button in the mirror could not stop anything. The assembly also declares a `mainWindow` dependency that nothing supplied, so production ran the empty-deps branch: no parent window, therefore the `alwaysOnTop` fallback and a mirror floating above every other application — the exact fault child-windowing was added to remove; no `cursorPoint`, therefore no hover, therefore `setIgnoreMouseEvents(false)` never ran and the controls were never drawn or clickable, with drag and resize dead behind the same guard; and bounds falling back to the primary display, so on two monitors it landed on the wrong one. Every existing test injects those dependencies, which is why the suite was green with none of it wired. The new tests assert the call sites instead, because that is where the defect was. `MainWindowController` grows the three capabilities the assembly asks for. boot.ts passes the controller in, destroys the mirror with the window it is a child of, and forwards it to the session IPC, the session streamer and the lifecycle wiring. A turn ending now retires the mirror rather than leaving it showing the previous run's last frame while the next turn drives a different application. `sessions:stop` becomes a named function so the mirror's own stop control can run it, rather than a second path that could drift. Two more, found reading the same code: A failed load produced a permanently invisible, undying window with no log. `did-finish-load` gates showing the window and starting the hover watch; a failed `loadFile` emits `did-fail-load` instead, so `ready` stayed false forever, frames piled up in the queue, and pip-feed's bare catch swallowed anything that came back. One leaked window per session, indistinguishable from a mirror that is working — a missing dist/overlay/pip.html is enough to produce it. The adapter now reports the failure once, with the asset path and the Chromium error code, and the controller treats it as gone. `restPoint` built its anchors from the app window's rect and clamped them with the work area of the display the mirror was on. With the app on an external display and the mirror on the built-in one it computed a corner in one coordinate space and clamped it into another, parking the mirror against an arbitrary edge. Both it and the drag release now clamp against the host's own display.
The mirror's own real-Electron smoke failed six of its thirteen checks as committed, and had never completed as written. It pressed at page (20, 20) to start the throw; `#resize` is a 20x20 chip at `left: 8px; top: 8px`, so the press landed on the grip, which stops propagation, and no drag ever began. The moves that followed ran as a resize instead, growing the tile to the 400 clamp with the alignment still bottom-right, and the five checks after it failed on the cascade. It now grabs the middle of the tile, clear of both the grip and the controls. Chasing that turned up the defect it was hiding. `pipResizeEdge` picks the gesture's sign from the alignment, because the anchored corner is the one held still while the tile grows — so the handle has to be on the corner that moves. Nothing ever sent the alignment to the renderer (`grep alignment src/overlay/` returned nothing; the only channels pushed were controls, frame, cursor and completed), so the grip stayed pinned to the tile's top-left in CSS. That is correct for exactly one of the four anchors. Throw the mirror to the top-left and the grip sat on the pinned corner: dragging it down grew the tile from the opposite edge while the handle itself could not move, and the pointer left the grip on the first pixel. The comment above the rule said the grip "sits in the corner opposite the anchor", which is what the CSS did not implement. Main now sends the anchor down over `pip:layout` and the page moves the grip to the opposite corner, turning the bracket and the resize cursor with it. The controls move off the top-right on the one anchor whose grip lands there. The smoke asserts the grip's position directly rather than only that the tile grew: with the CSS reverted it reports the grip at (18, 18) of a tile anchored top-left while the growth check still passes, because a scripted pointer can follow a handle a real one would have come off. The last smoke failure was the fixture, not the product. Hover is decided from the main-side pointer, so the controls are click-through until that pointer is on the mirror; the script only sent page events for the hide click, and the grip drag happened to leave its pointer one point past the tile's bottom edge. It moves both now, and asserts the controls are on before it clicks. Thirteen of thirteen, plus the two new checks. Two more, from reading the same files: pip-feed's `resolvedScreenPoint ?? presentationScreenPoint` had no test at all, and it is the line that keeps the cursor drawn during accessibility actions — which is every action Maka dispatches by default. Reverting it to the resolved point alone left all 71 tests passing. Covered now, in both directions. pip-window re-declared PIP_DEFAULT_EDGE/MIN/MAX that nothing in the file read, so changing the clamp there would have silently disagreed with the live copies in pip-motion. PIP_MARGIN moves to pip-motion, which breaks a real value-level import cycle between pip-window and pip-electron that worked only because each side touched the other's bindings from inside a function body. Four unused imports go with them, and so do PIP_FLICK_MIN_SPEED and PipDragTracker.hasMoved — no production reader, and the only assertion on the first compared the literal to itself. Why the flick threshold does not apply to a single-host mirror is now written down where it was. Finally, the wiring test asserts that the overlay hook the host is given is still wrapped by `withComputerUsePip`. That expression is a merge conflict by construction: every feature that wants to watch actions go past wraps the one before it, so resolving the conflict by taking one side compiles, type-checks, and leaves both branches' suites green while one branch's feed is disconnected. Measured: replacing it with the bare hook leaves tsc at exit 0 and all 71 mirror tests passing against a window that can never receive a frame, because every one of them builds the wrapper itself.
The codebase disagreed with itself about what a run is. `pip-window.ts` said one thing in prose — "hiding it is a statement about this run, not a setting" — and another in code, where `hiddenSessionId` was cleared only by `clearForSession` and `destroyAll`. Turn end calls `complete`, which cleared nothing. `session-stream.ts` calls `complete` at every turn end under the comment "The run finished". So a person who started a Computer Use run, hid the mirror during turn one and then sent a follow-up got turn two driving another application for minutes with nothing on screen and no way to ask for the mirror back. Stopping, archiving or deleting the session were the only things that cleared the flag, and all three end the run they wanted to keep watching. A run is a turn. That is what `complete` already marks, and it is the boundary the two clears either side of that call already use — `computerUseOverlay .clearForSession` and `computerUseTools.clearSession` both reset at turn end. The hide flag was the only piece of Computer Use presence scoped to the whole conversation. It is also the same defect this branch already fixed for the window itself: before `complete` existed the mirror outlived its turn and went on showing one run's last frame while the next drove a different app. Between a control the user has to press twice and one they cannot undo for minutes, the recoverable one wins. Re-hiding is a click; not being able to un-hide costs the entire point of the mirror. The clear happens before `complete`'s `!win` guard, because a dismissed mirror has no window — anything after that check would never run for the one case that needs it. Also runs the clock over the retirement timer. Its body had never executed in a test: the linger was only ever asserted as "not destroyed synchronously", which is equally true of a timer that never fires, a body that does nothing, and a zero-length linger. The first two leave the mirror up forever, the third takes it away at the moment a person looks over, and setting `PIP_COMPLETED_LINGER_MS` to 0 left all forty-one tests green.
8a2a23e to
438ead1
Compare
|
Rebased onto current main (34e7ddb), and settled what a run is for the mirror's hide control. Rebase. One conflict, in sessions-ipc-main.ts: main replaced the three emitSessionsChanged calls after a stop with the stoppedTurnBroadcasts loop, while this branch extracted that body into a named stopSession so the mirror's stop control could be pointed at it. The resolution keeps both. A second rebase onto the newer main was clean. CI. The e2e reds were inherited, not introduced. e2e_shard 1/2 was the plan-reminders edit-dialog focus assertion, which was failing on main at the same commit and reproduced on a rerun rather than being flaky; shard 2/2 was a Mermaid zoom pixel-offset assertion that passed on rerun; alignment_audit failed inside install-electron with a fetch error. All three are unrelated to this branch, and the first two are fixed upstream, so this is now rebased past them. The storybook check is green. Hide was scoped to the session, and the code disagreed with itself about it. pip-window.ts said in prose that hiding it is a statement about this run and not a setting, while hiddenSessionId was cleared only by clearForSession and destroyAll. Turn end calls complete, which cleared nothing. session-stream.ts calls complete at every turn end under the comment "The run finished". So someone who started a Computer Use run, hid the mirror during turn one and then sent a follow-up got turn two driving another application for minutes with nothing on screen and no way to ask for the mirror back — stopping, archiving and deleting were the only things that cleared the flag, and all three end the run they wanted to keep watching. A run is a turn, and complete now clears the dismissal. Three reasons. Both prose comments already said run and only the identifier said session. The two clears either side of that same call — computerUseOverlay.clearForSession and computerUseTools.clearSession — already reset at turn end, so the hide flag was the only piece of Computer Use presence scoped to the whole conversation. And it is the same defect this branch already fixed for the window itself: the wiring test's own comment records that before complete existed the mirror outlived its turn and went on showing one run's last frame while the next drove a different app. The asymmetry decides the close call. Re-hiding costs one click; not being able to un-hide costs the entire point of the mirror for however long the turn runs. Between a control you have to press twice and one you cannot undo, the recoverable one wins. The clear happens before complete's !win guard, because a dismissed mirror has no window — anything after that check would never run for the one case that needs it. Negative-controlled by reverting just that line: AssertionError [ERR_ASSERTION]: the next turn is a fresh decision The existing test at computer-use-pip.test.ts did not actually pin the old behaviour, for what it is worth: it hides on s1 and presents s2, which is a different session and stays visible either way. Its second assertion, that the mirror stays dismissed for the rest of the run, is unchanged and still holds — no complete happens in between. Also ran the clock over the retirement timer. Its body had never executed in a test: the linger was only ever asserted as not-destroyed-synchronously, which is equally true of a timer that never fires, a body that does nothing, and a zero-length linger. Setting PIP_COMPLETED_LINGER_MS to 0 left all forty-one tests green. The new test ticks to 29999 and then to 30000, so both sides of the duration are pinned; with the linger at 0 it fails with: AssertionError [ERR_ASSERTION]: still there a moment before the linger is up A second test covers the cancel path — a frame arriving mid-linger keeps the mirror and does not rebuild the window. Local runs on the final rebase: desktop 1461/1462 with the one failure being a login-shell process-spawn test that passes in isolation under lower load, and all four pip suites 80/80. Merge-order note. This PR and #1889 both extract the same ipcMain.handle('sessions:stop') body into an async function stopSession. A union of the two produces two declarations of it in the same scope and an unclosed ipcMain.handle, which does not parse; that hunk needs a hand-written resolution keeping one function whose body carries both branches' per-session clears, one setStopHandler per surface, and one handler. The overlay: expression in tool-assembly.ts also conflicts, and the correct resolution nests all three wrappers rather than choosing one. All three wrong resolutions still fail loudly after this rebase: taking this branch's side reddens the presence wiring test, taking presence's side reddens this branch's wiring test, and the naive union fails tsc with TS1117. |
1 participant
The problem the feature creates
Computer Use drives an application without bringing it to the front. That is the whole point, and it is also the problem:
What this adds
A small panel beside the app showing the driven window, the way Codex does. Reverse-engineered from ChatGPT.app's
sky.nodeand written down indocs/codex-pip-reverse-engineering.md, because two of the behaviours are not obvious and both were got wrong on the first attempt:The first matters because a panel that moves itself chases the app window and trails behind every drag; made a child it is carried along in the same transaction, and it stops covering other applications. The second is backwards from intuition: occlusion is the moment the mirror is the only way to see anything.
It can be thrown and resized, with the constants Codex uses. The drag reads the pointer from
screen.getCursorScreenPoint()rather than the renderer'sscreenX— the window moves in screen points, so the pointer is read in screen points and nothing converts between two spaces that can disagree.Shape
All new files except three lines of wiring.
withComputerUsePipdecorates whatever overlay hook it is given, so this adds a mirror without changing what the cursor overlay does:mainWindowbecomes an optional dep onassembleDesktopTools; without it the mirror is simply not a child of anything and the rest still works.Verification
1295 lines of the diff are the three test files. The interaction smoke script drives a real panel — throw, resize, occlusion — against a real Electron window.
Local desktop typecheck cannot run on this machine (upstream's
@astryxdesign/core@0.2.0does not resolve here), so CI is the check for the desktop build. The PiP files import onlyelectron,node:*and each other, and the wiring typechecks clean against current main.