feat(capture): surface browser dialogs for user-driven capture (closes #46)#52
Merged
Merged
Conversation
…ting (#46) Related to #46 - stop relying on Playwright's default auto-dismiss behavior for interactive headed runs - watch for browser dialogs, surface them to the user, and record the resolved outcome in _solentlabs - add opened_at timestamps so repeated dialogs are captured as distinct events for HAR analysis - add test coverage for dialog capture and run the full test suite to check for regressions - document dialog behavior and backfill the missing popup coverage in the capture docs
…nups Three follow-ups on top of ccpk1's dialog work (b12881a) to make it defensible per CLAUDE.md principles: 1. Polling loop -> page.expose_function (principle #10: no shortcuts). The original implementation maintained a window-scoped outcome queue and polled it from a Python dialog handler with `while True: ...; time.sleep(0.1)`. That's a workaround for not using Playwright's first-class JS->Python bridge. Replaced with a two-event model: `page.on("dialog")` creates the open record; the exposed `__harCaptureDialogResolved` binding (called by the JS init script after the user clicks) updates it with the action. No polling, no deadlock surface. Match-by-(type, message) so nested or concurrent dialogs can't mis-correlate. 2. sys.stderr.write -> _LOGGER.info (principle #11: quality gates). Both call sites converted to match the module's 26 existing _LOGGER calls. `sys` import removed. 3. Revert ~250 lines of unrelated fixture reformatting. The original PR reformatted multiple unrelated test_browser.json sections from compact one-line JSON to multi-line. Restored the project's compact convention; kept only the 7 substantive has_dialogs field additions + the new with_dialogs case. Cumulative diff vs. main now 311+/18- (was 544+/77-). Full suite: 2002 passed, 18 deselected. Co-Authored-By: ccpk1 <64691424+ccpk1@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
Promote the dialog support entry into the 0.9.0 ### Added section now that PR #52 ships in 0.9.0 (not deferred to a later release). Expanded the entry to cite #46 and to note the page.expose_function architectural choice for future readers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kwschulz
added a commit
that referenced
this pull request
May 13, 2026
Bundles the v0.9.1 release content: the Authorization-scheme preservation feature (already cherry-picked), the custom_patterns coverage audit (already cherry-picked), the dialog integration tests (already on this branch from PR #52 follow-up), plus the doc-system audit and lint hardening work this commit adds. Doc-system router restructure - CLAUDE.md slimmed to disciplines + Where Things Live routing table - New docs/CODE_REVIEW.md houses code-quality and test-file standards - docs/ARCHITECTURE.md gains Code Organization section (SoC, DRY, no-CLI-dep, additive-only) - docs/RELEASE.md gains Version Numbering (pre-1.0 bump policy + CHANGELOG-section tiebreaker) and Branching and Merging sections - New ADR-11 in docs/ARCHITECTURE_DECISIONS.md records the rationale Cross-reference migration - 10 code/test/script comments that cited numbered CLAUDE.md principles now point at the authoritative doc and section instead - scripts/release.py rule-3 reference corrected (DRY was rule 2) - Historical CHANGELOG entries untouched Lint adoption - New .markdownlint.jsonc / .json / -cli2.jsonc — every customization verified by isolation-test, only MD013 customized (line-length enforced at 120 with table/code exemptions documented in config) - markdownlint-cli2 v0.22.1 added as pre-commit hook - mdformat hook now passes --wrap 120 and includes mdformat-frontmatter plugin (required so .github/ISSUE_TEMPLATE/*.md frontmatter stops being collapsed to horizontal-rule underscores) - Full doc tree reflowed to 120-char lines (~2700 mechanical diff lines) - Surfaced and fixed: 3 bare-URL violations, 2 trailing-colon headings, 1 missing-H1 in PR template, 1 detect-secrets pragma separated from its target line by the reflow Release-flow fixes - .github/workflows/release.yml awk regex extracted empty release notes for every prior release (single-range pattern where both endpoints matched the same line). Replaced with flag-toggle pattern; verified against the 0.9.1 CHANGELOG content - .github/ISSUE_TEMPLATE/bug_report.md and feature_request.md restored to proper YAML frontmatter format Verification - 2064 tests passing, coverage 94.34% - All pre-commit hooks green - markdownlint 0 errors across 24 doc files - Fresh-context subagent smoke test on three trick questions (version bump, PR consolidation, coverage threshold) — all answered correctly from docs alone Version bump - pyproject.toml + src/har_capture/__init__.py: 0.9.0 -> 0.9.1 - CHANGELOG.md dated 2026-05-13 with comparison link 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
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.
Summary
Closes #46. Builds on the substance from #48 (by @ccpk1) with three follow-up fixes that bring the implementation in line with CLAUDE.md principles. PR #48 is being closed in favor of this one — full credit preserved via
Co-Authored-Byon the fix commit.For interactive headed capture (
headless=False, timeout=None), nativealert/confirm/promptdialogs now display to the user instead of being silently auto-dismissed by Playwright. The HAR records_solentlabs.dialogsentries with type, message, default value, opened-at timestamp, the user's action (accept/dismiss), andresolved_by="browser_ui".Without this change, cable-modem actions like Reboot / Reset / Apply (which fire
window.confirm()) couldn't be captured — Playwright would auto-dismiss the dialog and the form's network request never fired.Two commits
b12881afeat(capture): surface browser dialogs (by @ccpk1) — the substance: dialog observation,_solentlabs.dialogsaudit trail, headed-only gating, README/ARCHITECTURE/CAPTURE_SPEC/CHANGELOG updated, 5 new tests.9c07826fix(capture): replace dialog polling with page.expose_function + cleanups — three follow-ups per CLAUDE.md principles:page.expose_function(principle chore: release v0.2.4 #10 no shortcuts). The originalwhile True: ...; time.sleep(0.1)polling of a window-scoped outcome queue replaced with the first-class Playwright JS→Python bridge. No polling, no deadlock surface. Match-by-(type, message) so nested or concurrent dialogs can't mis-correlate.sys.stderr.write→_LOGGER.info(principle Sanitize tokens in URL query strings #11 quality gates). Matches the module's 26 existing_LOGGERcalls.sysimport removed.tests/fixtures/test_browser.json. Kept only the 7 substantivehas_dialogsfield additions + the newwith_dialogscase.Cumulative diff vs
main: 311+/18− (was 544+/77− in #48).Test plan
test_dialog_handler_records_browser_ui_outcome_in_hardrives both the open-event handler and the exposed-function resolution callback through mocks, asserting the final HAR records the correctactionandresolved_by.Stacking
This PR is on top of #45 (the v0.9.0 release branch). Merge order: #45 first, then this. Or rebase this onto
mainafter #45 lands — both clean.🤖 Generated with Claude Code