refactor(webkit): extract target session manager (#706 3/5)#735
Merged
shaun0927 merged 12 commits intoMay 8, 2026
Conversation
Behavior-preserving; same target/active-target semantics; per-target enabled-domain dedup preserved. TargetSessionManager owns knownTargets, activeTargetId, enabledDomainsPerTarget and Target.* event subscriptions. WebKitClient delegates all target state operations to the manager via the TargetCommandSender adapter interface (no circular deps). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors target management in WebKitClient by delegating target lifecycle, active target tracking, and domain management to a new TargetSessionManager class. The refactoring includes the addition of comprehensive unit tests for the new manager. I have no feedback to provide.
Behavior-preserving extraction of high-level browser commands from client.ts into three new modules: - src/webkit/dom-input-scripts.ts: DOM script builders (tap, longPress, swipe, setValue, typeChar, focus, selectOption). Uses document.createTouch() / document.createTouchList() — never new Touch(). Uses prototype-walk value setter to avoid cross-realm TypeError. - src/webkit/evaluate.ts: evaluateValue<T> helper implementing the three-step WebKit eval pattern (Runtime.evaluate → Runtime.awaitPromise as separate command → Runtime.callFunctionOn for object serialization). - src/webkit/browser-commands.ts: BrowserCommands class taking a BrowserCommandSender adapter. Implements navigate, screenshot (Page.snapshotRect only — never captureScreenshot), click, longPress, swipe, type, scroll, press, dismissKeyboard, selectOption, readPage, querySelector/All, inspect, waitFor, getCookies/setCookies/clearCookies. WebKitClient becomes a thin facade: same public API, delegates every browser command to BrowserCommands. Same protocol calls, screenshot path, touch APIs, and per-target enabled-domain dedup preserved. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced May 7, 2026
- screenshot: skip viewport eval roundtrip when caller provides clip - screenshot: surface underlying error in fallback message for diagnostics - press: use DigitN/KeyN code prefix per UI Events spec, omit for symbols - getCookies: exact domain match (with optional leading-dot tolerance) instead of permissive substring includes() - clearCookies: strip leading dot from cookie domain before building URL for Page.deleteCookie to avoid invalid `https://.example.com/` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex review noted that strict-equality filtering dropped subdomain cookies:
getCookies("example.com") would no longer return cookies scoped to
www.example.com. The previous includes() check was too permissive
(it matched another-example.com), and exact equality was too strict.
Match per RFC 6265 cookie scoping: a filter of `example.com` matches
`example.com`, `.example.com`, any subdomain (`www.example.com`), and any
parent domain whose scope still applies — but not `another-example.com`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- press: alphabetic-key fallback now sets keyCode to the uppercase ASCII value (65-90) per KeyboardEvent semantics on keydown/keyup, instead of emitting lowercase ASCII (97-122) for lowercase input - getCookies: drop subdomain match — cookies scoped to a subdomain of the filter host are not sent to that host per RFC 6265, so they should not be returned by a parent-host filter; also guard against empty cookie domains in the fallback path - clearCookies: skip cookies whose domain is empty (document.cookie fallback path) so we never construct `https:///` URLs for the Page.deleteCookie protocol command Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex flagged a P1 regression: when Page.getCookies is unsupported, getCookies() returns cookies with domain=''. The previous fix skipped those entries to avoid invalid `https:///` URLs but then silently returned, making clearCookies a no-op on proxies that lack the protocol command. Track whether any cookie was actually processed via the protocol path. If we collected cookies but none had a usable domain, fall through to the JS-based document.cookie clearing block instead of returning early. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5/5) Extract EventBridge class and typed payload interfaces (ConsoleMessage, RequestInfo, ResponseInfo, ErrorInfo, TargetCreatedPayload, TargetDestroyedPayload) from client.ts into src/webkit/events.ts. WebKitClient is now a pure facade: constructor wiring only, all branches delegation-only, zero business logic remaining. Behavior-preserving: same event names, same payload shapes, same emission timing. Typed interfaces exported from events.ts and re-exported via index.ts. New tests/unit/events.test.ts: 14 assertions covering transport forwarding, target lifecycle re-emission, and all five typed convenience listeners. client.ts: 550 → 496 lines (this PR); 1316 → 496 across the 5-PR series. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- attach(): make idempotent so repeat calls do not double-wrap transport.emit or stack duplicate target lifecycle listeners - targetSessionEmitter param: use the imported EventEmitter type for consistency with the rest of the file (drops the global NodeJS namespace) - on*() convenience listeners: attach the listener BEFORE awaiting enableDomain via a shared wireDomainListener() helper. Closes the race window where an event fires between the resolver send and the promise microtask. On enableDomain failure, detach the listener and surface the error so callers are not left silently subscribed to a domain that never woke up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The host parameter is typed `EventBridgeHost & EventEmitter` and EventBridgeHost.emit is part of the interface, so the EventEmitter prototype call is unnecessary indirection. Use the typed method directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Stacked on PR #734 (which is stacked on PR #732). Will retarget to develop after upstream merges.
Implements #706 step 3 of 5. Behavior-preserving. Follow-ups: browser-commands, events+facade.
Does not Close — multi-PR refactor.