Launch Playwright directly instead of via remote server by lewispb · Pull Request #55 · basecamp/upright

lewispb · 2026-04-09T09:42:35Z

Summary

Replace remote WebSocket server connection (connect_to_browser_server) with direct Chromium launch via Playwright.create
Eliminates need for separate Playwright Docker accessory — Playwright runs in the same container
Add Playwright trace recording support with links to trace.playwright.dev viewer
Auth steps now captured in video and trace recordings (single context lifecycle)
New playwright_cli_path config replaces playwright_server_url

Test plan

Run all Playwright probes locally with PLAYWRIGHT_CLI_PATH="npx playwright@1.59.0"
Verify video, trace, and log artifacts are captured
Verify trace artifact opens in trace.playwright.dev viewer
Run authenticator and probe tests

🤖 Generated with Claude Code

Copilot

Pull request overview

This PR switches Upright’s Playwright probe execution from connecting to a remote browser server to launching Chromium directly via Playwright.create, and adds support for Playwright trace artifacts with a Trace Viewer link.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Changes:

Replace remote server connection with direct Playwright CLI-based launch (playwright_cli_path config replaces playwright_server_url).
Add trace recording + attach .zip trace artifacts and render a Trace Viewer link in the artifact UI.
Refactor authentication handling into the Playwright lifecycle so auth happens within the same context (capturable by video/trace).

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
test/test_helper.rb	Removes `PlaywrightHelper` inclusion used for local skipping.
test/models/upright/probes/playwright_probe_test.rb	Removes local “skip if Playwright server isn’t running” guard.
test/models/upright/playwright/authenticator_test.rb	Updates authenticator tests to match refactored API/behavior.
test/lib/helpers/playwright_helper.rb	Deletes helper that probed a remote Playwright server port.
test/lib/helpers/mock_playwright_helper.rb	Extends mocks to support `context.tracing` and `page.video`.
lib/upright/version.rb	Bumps Upright version to `0.3.0`.
lib/upright/configuration.rb	Introduces `playwright_cli_path` and defaulting from `PLAYWRIGHT_CLI_PATH`.
lib/generators/upright/install/templates/upright.rb	Updates install template to document the new Playwright config.
Gemfile.lock	Updates gem version entry to `0.3.0`.
app/views/upright/artifacts/show.html.erb	Adds UI handling for trace `.zip` artifacts via Trace Viewer link.
app/models/upright/probes/playwright/base.rb	Attaches trace artifacts alongside video/log artifacts.
app/models/upright/playwright/authenticator/base.rb	Simplifies authenticator API around operating on an existing `page`.
app/models/upright/artifact.rb	Recognizes `.zip` artifacts for display/icon mapping.
app/models/concerns/upright/playwright/trace_recording.rb	New concern implementing trace start/stop + artifact attach.
app/models/concerns/upright/playwright/lifecycle.rb	Launches browser directly and moves auth+storage-state handling into lifecycle.
app/models/concerns/upright/playwright/form_authentication.rb	Removes older “authenticator returns authenticated_context” flow.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Replace the remote WebSocket server connection with direct Chromium launch via Playwright.create. This eliminates the need for a separate Playwright Docker accessory. - Replace playwright_server_url config with playwright_cli_path - Move authentication into the probe lifecycle so auth steps are captured in video and trace recordings - Add trace recording support with links to trace.playwright.dev - Simplify authenticator base class (no longer manages its own context) - Remove PlaywrightHelper (no external server to check) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

app/models/concerns/upright/playwright/form_authentication.rb:34

save_storage_state persists the full context.storage_state, but load_cached_storage_state only reapplies the cookies portion. Any other persisted state will be silently dropped, making the cache restore incomplete/inconsistent with what was saved. Consider restoring the full storage state (or persisting only what you can restore).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Vendor the Playwright trace viewer (v1.59.1, Apache 2.0) as static files served from the engine so traces can be viewed inline within the artifact modal instead of linking out to trace.playwright.dev. This avoids Chrome's Local Network Access restrictions and keeps trace data on the same origin. Also: fullscreen artifact modal, download link on filename, 🎭 icon for trace artifacts, and disable default OTLP exporter when no endpoint is configured. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Use full conditional in form_authentication - Replace LOCAL_PLAYWRIGHT with HEADLESS env var (default true) - Add recording_base_dir config instead of overloading video_storage_dir - Always record traces (remove test env guard) - Remove trace_viewer_url view concern from model layer - Revert session_valid? to original implementation - Indent private methods in authenticator base - Drop ENV OTEL_TRACES_EXPORTER override in tracing - Guard before_close callbacks against nil context - Wrap close in page_close callback block for correct before/after semantics - Add wait_for_load_state to MockPage - Delete vulnerable unused trace viewer files (snapshot.html, uiMode.html) - Fix install template comment to match actual default Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 26 out of 34 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Use rails_blob_url instead of manual base_url + path concatenation - Collapse before_close into page_close callbacks (simplify lifecycle) - Rescue stop_trace errors to keep teardown robust - Add service_name to test authenticator Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 27 out of 35 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Add PLAYWRIGHT_VERSION constant as single source of truth - Add package.json pinning npm playwright to match - Gemspec pins playwright-ruby-client to match and includes public/ - Add node to mise.toml - Add rake playwright:sync task to copy trace viewer from npm package, stripping vulnerable unused files (snapshot.html, uiMode.*) - Update bin/setup to install npm deps and Playwright browsers - Sync trace viewer from 1.55 to 1.58 - Update README with upgrade instructions and HEADLESS env var Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 32 out of 42 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T08:54:03Z

+  def ensure_authenticated(context, page)
+    @page = page
+    load_cached_storage_state(context)
+    page.goto(signin_redirect_url, timeout: 10.seconds.in_ms)
+
+    unless session_valid_on?(page)
+      authenticate_on(page)
+      @storage_state.save(context.storage_state)
+    end
+  end


ensure_authenticated introduces new authentication/cache behavior (load cached state into the context, navigate to the redirect URL, conditionally authenticate, then persist context.storage_state), but the updated test suite no longer exercises this flow (current tests only cover session_valid_on? and authenticate_on). Adding a focused test around ensure_authenticated (cached state applied, authenticate_on called only when needed, and state persisted) would help prevent regressions.

- Remove jacoblincool Docker service from GitHub Actions CI - Install Playwright directly via npm in CI - Fix smoke_test to use gem exec for rails new - Add trace viewer and trace artifact checks to smoke test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

The Ruby gem playwright-ruby-client ~> 1.58 resolved to 1.59.0 which expects different browser builds than npm playwright 1.58. Bump everything to 1.59 so browsers, driver, and trace viewer are in sync. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 33 out of 44 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T09:11:09Z

@@ -30,14 +40,9 @@ def session_valid?
    end
  end

-  def authenticated_context
-    if (cached_state = @storage_state.load)
-      context = create_context(cached_state)
-      return context if context_has_valid_session?(context)
-      context.close
-    end
-
-    perform_authentication
+  def session_valid_on?(page)
+    wait_for_network_idle(page)
+    !page.url.include?(signin_path)
  end


session_valid_on? (and ensure_authenticated) can raise Playwright::TimeoutError from wait_for_load_state or page.goto, which will abort authentication instead of treating it as an invalid session and re-authenticating (the previous implementation rescued timeouts and returned false). Consider rescuing Playwright::TimeoutError in session_valid_on? (and/or around the initial page.goto) and returning false so the flow falls back to authenticate_on.

CI bundle install fails in frozen mode when the CHECKSUMS section has an empty entry. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 33 out of 44 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T09:23:27Z


    def create_context(browser, **options)
-      authenticated_context(browser, options) || browser.new_context(userAgent: user_agent, **options)
+      browser.new_context(userAgent: user_agent, **options)


create_context no longer sets serviceWorkers: "block" (previously used to avoid service-worker caching/interference in synthetic probes). This changes runtime behavior and can introduce flakiness/non-determinism when target apps register service workers. Consider restoring the default to block service workers (while still allowing callers to override via **options).

Suggested change

browser.new_context(userAgent: user_agent, **options)

browser.new_context(serviceWorkers: "block", userAgent: user_agent, **options)

Copilot · 2026-04-10T09:23:28Z

+  test "session_valid_on? returns true when not on signin path" do
+    authenticator = TestAuthenticator.new
+    page = MockPage.new

-    assert_equal cached_state, context.state
+    assert authenticator.session_valid_on?(page)
  end

-  test "re-authenticates when cached session is invalid" do
-    cached_state = { "cookies" => [ { "name" => "session", "value" => "expired" } ] }
-    @storage_state.save(cached_state)
-    authenticator = TestAuthenticator.new(@browser, @service_name)
-    authenticator.session_should_be_valid = false
+  test "authenticate_on sets page and runs authenticate" do
+    authenticator = TestAuthenticator.new
+    page = MockPage.new

-    context = authenticator.authenticated_context
+    authenticator.authenticate_on(page)

-    assert_not_equal cached_state, context.state
+    assert authenticator.authenticated
  end


This test file no longer covers the new ensure_authenticated(context, page) flow (loading cached storage state into the context and persisting updated context.storage_state after re-auth). Adding focused tests for the cache-hit and cache-miss cases would help prevent regressions in authentication caching behavior.

Running Chromium as a non-root user in Docker crashes without --no-sandbox since the user namespace sandbox requires CAP_SYS_ADMIN. This was never an issue with the sidecar container which ran as root. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

rails_blob_url was generating URLs with the global "app" subdomain from default_url_options instead of the site-specific subdomain the user is browsing on, causing cross-origin failures in the trace viewer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Add a Dockerfile template to the install generator that embeds Playwright and Chromium in the app image with correct permissions for the non-root rails user (PLAYWRIGHT_BROWSERS_PATH=/ms-playwright). Remove the Playwright sidecar container from deploy.yml and docker-compose.yml templates since it is no longer needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 38 out of 49 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T11:25:44Z

+    page.goto(auth_check_url, timeout: 10.seconds.in_ms)
+
+    unless session_valid_on?(page)
+      authenticate_on(page)
+      @storage_state.save(context.storage_state)


ensure_authenticated no longer handles navigation/load timeouts. If page.goto or wait_for_network_idle raises Playwright::TimeoutError, authentication will fail hard instead of falling back to re-authentication. Consider rescuing Playwright::TimeoutError (and treating it as an invalid session) so transient slowness doesn’t break probes.

Copilot · 2026-04-10T11:25:45Z

+    def load_cached_storage_state(context)
+      if (cached_state = @storage_state.load)
+        cached_state.fetch("cookies", []).each do |cookie|
+          context.add_cookies([ cookie ])
+        end


load_cached_storage_state only rehydrates cookies from the saved storage_state. Playwright storage state can also include origins (localStorage/sessionStorage); previously this would have been restored when passing storageState to browser.new_context. If callers rely on origin storage, sessions may be treated as logged out. Consider restoring origins as well (or documenting/validating that only cookies are supported).

Bump version to 0.3.0

fa5ce5d

Copilot AI review requested due to automatic review settings April 9, 2026 09:42

Copilot started reviewing on behalf of lewispb April 9, 2026 09:43 View session

lewispb force-pushed the lewis/playwright-direct-launch branch 2 times, most recently from b42f1ba to 3adbf3f Compare April 9, 2026 09:48

Copilot AI reviewed Apr 9, 2026

View reviewed changes

lewispb force-pushed the lewis/playwright-direct-launch branch from 3adbf3f to 0b9abea Compare April 9, 2026 09:59

Copilot AI review requested due to automatic review settings April 9, 2026 10:02

lewispb force-pushed the lewis/playwright-direct-launch branch from 0b9abea to a74ddf9 Compare April 9, 2026 10:02

Copilot started reviewing on behalf of lewispb April 9, 2026 10:03 View session

lewispb force-pushed the lewis/playwright-direct-launch branch from a74ddf9 to e6dd3c2 Compare April 9, 2026 10:07

Copilot AI reviewed Apr 9, 2026

View reviewed changes

github-advanced-security AI found potential problems Apr 9, 2026

View reviewed changes

Comment thread public/trace-viewer/snapshot.html Fixed

Comment thread public/trace-viewer/snapshot.html Fixed