Skip to content

Memoize Playwright page target IDs - #355

Closed
ehfeng wants to merge 2 commits into
codex/cus-580-skip-page-resolutionfrom
codex/cus-580-memoize-page-target-ids
Closed

Memoize Playwright page target IDs#355
ehfeng wants to merge 2 commits into
codex/cus-580-skip-page-resolutionfrom
codex/cus-580-memoize-page-target-ids

Conversation

@ehfeng

@ehfeng ehfeng commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • cache successful Playwright page-to-CDP target ID lookups in a WeakMap
  • coalesce concurrent discovery and prevent pre-reset work from repopulating the cache
  • refresh cached IDs on the resolver retry and reset them on browser disconnect
  • build a per-attempt pageByTargetId index for the tab-to-page join
  • add focused Node tests for reuse, refresh, failures, concurrency, and reset behavior

Scope

This PR intentionally leaves browser-level CDP session consolidation unchanged. It isolates target-ID memoization and stale-cache recovery from that separate lifecycle refactor.

Stack

Validation

  • make test-runtime
  • go test ./e2e -run ^$
  • go vet ./...
  • go test -race ./cmd/api/api ./lib/oapi
  • Playwright daemon bundle build

Note

Medium Risk
Changes which Playwright page the daemon binds for user code (foreground tab resolution) and adds caching that must stay correct across CDP reconnects and target replacement.

Overview
Adds PageTargetIdCache to memoize CDP targetId lookups per Playwright Page (WeakMap keys, shared in-flight discovery, optional refresh, and reset on browser disconnect/reconnect). Active-tab resolution now builds a pageByTargetId index from the cache instead of opening a CDP session per page on every join, and pageForTabTarget matches related targets via that map.

resolveActivePage uses one browser CDP session per attempt, refreshes cached IDs on retries, bumps attempts to 3 with a 150ms delay between passes (for cross-origin tab target swaps), and logs unmatched active tab IDs on fallback.

CI/build: make test-runtime runs Node tests for the cache; server unit workflow adds Node 22 and runs it; headful/headless Dockerfiles bundle the new module into the Playwright daemon esbuild step.

Reviewed by Cursor Bugbot for commit 16d6b3a. Bugbot is set up for automated code reviews on this repo. Configure here.

@ehfeng
ehfeng force-pushed the codex/cus-580-memoize-page-target-ids branch from 1ceb4ab to 5782b01 Compare August 24, 2026 23:05
@ehfeng
ehfeng marked this pull request as ready for review August 24, 2026 23:06
@ehfeng
ehfeng requested a review from rgarcia August 24, 2026 23:12
@rgarcia

rgarcia commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

superseded by #356 — same memoization work rebased directly onto #347 (the resolve_active_page middle PR was dropped from the stack, see #352), plus browser-level CDP session consolidation. GitHub's stack metadata blocked retargeting this PR's base, so it was recreated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants