Repository navigation
feat(automation): unify browser previews on the shared page engine - #1020
Conversation
114ac44 to
dbd964b
Compare
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Independent read-only review of dbd964b, using git diff feat/automation-input...HEAD (not dev). One actionable P2 finding below; no P0/P1 findings identified.
Reviewed the new BrowserPage integration, private Electron/Tauri begin/send/check/end routing, exact target/document fences, lease/queue cleanup, protocol budgets and debugger/emulation ownership. Tauri review was static; I did not run its GUI.
Validation: 56 targeted server/Electron/preload tests passed, and the existing isolated hidden-window Electron preview integration passed with Electron 39. Additional hidden real-guest checks reproduced the keyboard finding below. No installed application or shared daemon was used; no window was shown/raised, no repository files or branches were changed, and review scratch profiles were removed.
| // CDP Input.insertText is silently ignored by an embedded/background view. | ||
| () => command === "Input.insertText" && typeof (params as { text?: unknown }).text === "string" | ||
| ? lease.registration.guest.insertText((params as { text: string }).text).then(() => ({})) | ||
| : lease.registration.guest.debugger.sendCommand(command, params), Math.min(deadline, lease.expires), |
There was a problem hiding this comment.
[P2] Deliver keyboard input to a newly opened guest without requiring a prior pointer click
Only Input.insertText gets the embedded-guest workaround here; Input.dispatchKeyEvent still returns a successful CDP reply even when no keyboard event reaches the guest. Using the production BrowserPage/controller and this PR's isolated hidden Electron fixture, I opened an input with a keydown/keyup recorder, snapshotted its ref, typed Pilot, then invoked press with that same ref and key: "a". The result reported focused: 'input "Field"', but the value remained Pilot and the event recorder was empty. Control+A followed by Backspace likewise did nothing. In contrast, adding click on the field before press made a insert normally and produced both key events.
Thus DOM focus from type / press(ref) is not sufficient to establish the guest input focus that the native keyboard path needs. An agent cannot reliably type then submit with Enter, navigate with Tab, or use shortcuts in a freshly opened preview, and receives no failure indication. Please establish keyboard delivery for the exact owned guest without showing/raising the application (or explicitly refuse if unavailable), and extend automation-preview.test.ts to assert actual keyboard events/default effects after type, with no prerequisite pointer click.
…guest Map the guarded channel's key events to Electron's native sendInputEvent, including text-producing character events and modifier/key-name translation. CDP reported successful key dispatch but delivered no event until a pointer click had focused a new guest. Keep exact lease checks and do not show or raise a native window. Regression: the hidden real-guest integration now types, presses a printable key, selects all and deletes, moves focus with Tab and submits once with Enter before any pointer click. All 23 controller tests and the real Electron integration pass; ES2022 Electron typecheck is clean. Addresses the gatekeeper P2 finding on PR #1020.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Round-two independent read-only review of f420488, against feat/automation-input (the full PR3 delta, retaining the unchanged channel/ownership review from round one).
The previous P2 is resolved. The production hidden-guest integration now verifies typing followed by character input, Ctrl+A/Backspace, Tab focus movement and exactly one Enter submission, without a prerequisite pointer click. No remaining P0-P2 findings identified. One remaining P3 key-translation edge case is noted below.
Validation: 57 targeted server/Electron/preload tests plus the real hidden Electron integration passed (58 total); git diff --check passed. Additional isolated hidden-guest checks exercised arrow aliases, Space, punctuation, named navigation/function keys, combined modifiers, fresh press(ref) without prior typing, and prevention of character insertion/Enter submission by keydown.preventDefault(). Tauri's unchanged private adapter remains statically reviewed, not GUI-tested. No installed application/shared daemon or visible/foreground window was used; repository files/branches were unchanged and scratch profiles were removed.
| async function sendGuestKey(guest: WebContents, params: Record<string, unknown>): Promise<Record<string, unknown>> { | ||
| const aliases: Record<string, string> = { ArrowDown: "Down", ArrowUp: "Up", ArrowLeft: "Left", ArrowRight: "Right", " ": "Space" } | ||
| if (typeof params.key !== "string" || !["keyDown", "rawKeyDown", "keyUp"].includes(String(params.type))) throw new Error("Invalid browser key event") | ||
| const mask = typeof params.modifiers === "number" ? params.modifiers : 0 |
There was a problem hiding this comment.
[P3] Explicitly handle the shared engine's ContextMenu key before native dispatch
page/keys.ts accepts ContextMenu and supplies virtual key 93, but this adapter discards that numeric key code and passes the unrecognized string ContextMenu to Electron's sendInputEvent. Electron 39's native key-name lookup does not support that name. In the real isolated hidden guest, press({ref, key: "ContextMenu"}) returned success while both keydown and keyup had event.key === "" and event.code === ""; the requested context-menu key was never delivered. Other tested named keys/aliases worked.
Please provide an actual supported equivalent, or explicitly reject this unsupported key before sending unknown-key events (with a supported alternative such as Shift+F10), and add a regression. The shared parser should not advertise a successful key action that this native adapter silently changes into an unknown key.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Round-three independent read-only review of 89c4239, using git diff feat/automation-input...HEAD for the full PR3 delta, not dev.
Zero remaining actionable P0-P3 findings identified in PR3. Both previous findings are resolved: native guest keyboard delivery works without a preceding pointer click, and unsupported ContextMenu is explicitly rejected before native event emission with the Shift+F10 alternative. The hidden real-guest integration verifies the rejection and successfully continues with subsequent select/click/wait operations.
Rechecked the BrowserPage/private-host integration and retained the earlier ownership, authentication, target/document-generation, expiry, queue, protocol-budget and debugger/emulation review. Parent PR2 implementation and its separately assigned timeout-outcome/cleanup item are outside this review; this comment does not settle that parent review.
Validation on this exact SHA: 58 targeted server/Electron/preload tests and the isolated hidden Electron integration passed (59 total); git diff --check passed. Tauri remains statically reviewed, not GUI-tested. No installed application or shared daemon was used, no window was shown/raised, and no repository files, branches or merge state were changed.
…guest Map the guarded channel's key events to Electron's native sendInputEvent, including text-producing character events and modifier/key-name translation. CDP reported successful key dispatch but delivered no event until a pointer click had focused a new guest. Keep exact lease checks and do not show or raise a native window. Regression: the hidden real-guest integration now types, presses a printable key, selects all and deletes, moves focus with Tab and submits once with Enter before any pointer click. All 23 controller tests and the real Electron integration pass; ES2022 Electron typecheck is clean. Addresses the gatekeeper P2 finding on PR #1020.
89c4239 to
86c8d30
Compare
|
All PR3 findings are fixed; round-three independent review reported zero actionable P0–P3 findings at 89c4239. Rebased afterward onto PR2's latest fixes: the PR3 delta is identical (stable patch-id 48eafc0aca0bb87c26fbe12c6450ec6117aa6481 before and after). The real hidden-window Electron integration still passes. PR3 remains open, not merged; its base will become dev after PR2 merges. |
…guest Map the guarded channel's key events to Electron's native sendInputEvent, including text-producing character events and modifier/key-name translation. CDP reported successful key dispatch but delivered no event until a pointer click had focused a new guest. Keep exact lease checks and do not show or raise a native window. Regression: the hidden real-guest integration now types, presses a printable key, selects all and deletes, moves focus with Tab and submits once with Enter before any pointer click. All 23 controller tests and the real Electron integration pass; ES2022 Electron typecheck is clean. Addresses the gatekeeper P2 finding on PR #1020.
86c8d30 to
2b173c2
Compare
…ed engine Use one server-side page engine for the app and session previews. BrowserPage serializes session operations, binds refs to the exact native guest/document generation, fences cancelled and late reads and always releases its exact operation lease. Tool schemas share action validation and expose scoped snapshots, selections, keys, hover, drag, scroll and waits without arbitrary evaluation. Add server and route coverage for ownership, generation changes, cross-session refs and cancellation, plus an isolated hidden-window Electron integration test for both production ends. Native channel adapters are added in the following commit; this branch is not yet ready to merge.
…mand Exercise one-shot native mutation failure, exact lease cleanup, bounded pending operations and shutdown fencing. Make the hidden Electron regression clean up even if launch fails, and retain import.meta identity when bundling the production controller.
…hannels Implement the private begin/send/check/end protocol on Electron and Windows Tauri. Bind each bounded 30-second lease to the exact visible registration, document and operation; revalidate before native dispatch and after IO, reject stale or ambiguous targets and release only owned debugger attachments. Keep mobile emulation, history, storage, navigation and screenshots native, while deleting duplicate snapshot/ref/click/type implementations. Add native channel regressions for expiry, deadlines, concurrent demand, target replacement, external detach, response/parameter bounds and emulation ownership. The real isolated hidden-window Electron regression drives the server engine through its actual native adapter; use Electron guest insertText to avoid silently ignored CDP insertion without raising a window. Validation: 22 controller unit tests pass (plus host security checks from implementation), 20 Rust browser-controller tests and cargo check --tests pass. Shared server suite passes 2,403 tests with seven existing skips. Isolated OpenCode 2.0.31 automation and native bundle regression pass. Real Electron end-to-end preview test passes using an isolated cached Electron 39 executable, no installed app or shared daemon changes.
…guest Map the guarded channel's key events to Electron's native sendInputEvent, including text-producing character events and modifier/key-name translation. CDP reported successful key dispatch but delivered no event until a pointer click had focused a new guest. Keep exact lease checks and do not show or raise a native window. Regression: the hidden real-guest integration now types, presses a printable key, selects all and deletes, moves focus with Tab and submits once with Enter before any pointer click. All 23 controller tests and the real Electron integration pass; ES2022 Electron typecheck is clean. Addresses the gatekeeper P2 finding on PR #1020.
Electron's native key-name lookup does not recognize ContextMenu and silently generated empty key/code events. Refuse it before native dispatch with the supported Shift+F10 alternative. Add unit and real hidden-guest regressions; all 24 controller tests and the integration pass.
2b173c2 to
e167331
Compare
1 participant
Scope
Third automation step after merged #1018. Browser previews now use the same server-side outline, refs, actions and validation as the app window. Removes duplicated snapshot/click/type/ref implementations from Electron and Tauri.
Native authority
Hosts retain storage, history, emulation, navigation, screenshots and exact guest ownership. A bounded 30-second begin/send/check/end lease fences every allowlisted protocol command and response against visibility, registration and document changes. Models and renderer pages cannot choose protocol methods or scripts; guests gain no app capabilities. Cleanup preserves independently owned emulation overrides.
Electron uses native WebContents.insertText/sendInputEvent for embedded guest text and keys, without showing/raising windows; unsupported ContextMenu explicitly points to Shift+F10.
Review and validation
Three independent gatekeeper rounds: zero actionable P0–P3 findings in the final review. Rebases changed ancestry only; identical stable patch-id 48eafc0aca0bb87c26fbe12c6450ec6117aa6481.
No installed application or shared daemon changes.
PR #1018 merged as b2d5b72. This PR is ready on dev and deliberately remains OPEN. Refs #1008.