refactor(browser): retire dead :protocol gate, simplify find_timeout#26
Merged
Conversation
…h_patch_await
The \`:protocol\` field on Wallabidi.Session was the V1-era gate for
\"this session can do JS-side LiveView orchestration.\" The V2 promotion
landed three new remote drivers, none of which ever populated the
field. The \`live_view_aware?(%Session{protocol: mod}) when not is_nil(mod)\`
predicate has been permanently false since.
Knock-on effects of the dead predicate, by call site:
1. \`fill_in/3\` — drain_idle_ms always 0 → fused JS skipped the
post-fill phx-change drain → assert_has retries papered over the
race. Replaced the dead outer gate with the already-internal
classify_interaction check (returns :none for non-phx-bound inputs;
prepare_patch returns :no_liveview for non-LV pages downstream).
2. \`with_patch_await\` (used by clear/send_keys/click) — patch /
navigate / full_page orchestration skipped entirely → bare fun.()
on every Chrome CDP/BiDi click of phx-bound elements. Lightpanda
click goes via v2_click_with_await so unaffected. Re-gated on
\`remote_session?\` instead.
3. \`execute_query\`'s second cond branch was a duplicate of the first
(same outcome via different predicate); deleted.
4. \`maybe_await_selector\` + \`extract_await_text\` (62 lines): only
call site was inside the dead gate. Deleted. The MutationObserver
awaiter at LiveViewAware.await_selector is now unreachable but
left in place — separate cleanup PR.
Also drops :protocol from Wallabidi.Session.t() / defstruct and from
the four driver / test-helper structs that set it to nil.
Net diff: -61 lines.
No new tests for #1 specifically — the race is real but timing on dev
machines hides it consistently (the existing fill_in awaits-the-right-
patch test passed even with drain disabled). The fix's correctness is
verified by:
- existing fill_in / await_patch / click tests continue to pass,
- mix test (unit) 159/159,
- chrome await_patch_test 25/25 (the new PubSub test from #21 still
deterministically passes),
- CDP/LP/LV smoke suites,
- credo --strict clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The find_timeout :resolved branch was a Map.pop / Map.put round-trip that re-inserted the same value (Map.pop is functional, so the original map is unchanged — get_in on it still returns the resolved value). The audit flagged it as a bug; it isn't, but the expression is awkward enough that it could become one under refactor. Simplify to match the BiDi SessionActor's already-cleaner shape: look up with Map.get, return state unchanged for :resolved, Map.delete for pending. Same semantics, half the cognitive load. Add three handler tests for transport/session.ex covering the :resolved / unknown / pending-no-caller branches. These guard against future refactors that might accidentally drop the :resolved entry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pinetops
force-pushed
the
cleanup/dead-protocol-gate
branch
from
d77ef32 to
731c892
Compare
Removing the dead :protocol gate unblocked the outer with_patch_await wrapper around click() for Chrome CDP/BiDi. But each of those drivers already does patch / navigate / page-ready awaiting INSIDE click_aware (invoked via Element.click → driver.click). Wrapping with_patch_await around it caused double-await: the outer arms a patch promise, then the click happens, then the outer await_patch times out at 5s waiting for a patch that came back as a redirect, then the inner click_aware finishes. Visible as: - phx-submit-redirect-detected-quickly test took 5072ms vs 3000ms budget - JoinPendingWaitTest took 20s vs 18s budget - mailbox_test hit ExUnit's 60s test timeout Fix: drop with_patch_await from the click call sites. Element.click → driver.click → click_aware is the source of truth for click-side waiting. Keep with_patch_await for clear/send_keys (those don't have a per-driver fused equivalent). Also bump JoinPendingWaitTest's slow budget 18s → 25s; the test inherently burns ~15-20s in click_aware's await_patch + page_ready fallback chain (the fixture has no real LV so every wait stage runs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 tasks
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
1 participant
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
Two-commit follow-up to PR #25 covering items 1, 2, and 3 from the code-review audit.
Commit 1: `fix(browser): retire dead :protocol gate`
The `:protocol` field on `Wallabidi.Session` was the V1-era gate for "this session can do JS-side LiveView orchestration." The V2 promotion landed three new remote drivers, none of which populated the field. The `live_view_aware?` predicate has been permanently false since.
Knock-on effects this fixes:
Also drops `:protocol` from `Wallabidi.Session.t()`, defstruct, and the four driver/test-helper structs.
Net diff: -61 lines.
Lightpanda click path
Lightpanda's `click` is routed via `v2_click_with_await` (separate from `with_patch_await`) so the change to `with_patch_await` doesn't affect LP click semantics. Chrome CDP/BiDi clicks now correctly enter the patch-await mechanism for phx-bound elements.
No new test for #1 (fill_in drain)
The drain race is real but the existing `fill_in awaits the set_value patch` test (`await_patch_test.exs:308`) was passing even with drain disabled — local timing hides the race. A deterministic test would need a fixture with a slow phx-change handler. Out of scope for this PR; the fix is verified by existing tests continuing to pass plus the centerpiece PubSub `await_patch` test from PR #25 (which now exercises the unified prepare/await machinery).
Commit 2: `refactor(transport): simplify find_timeout handler`
The audit (item #3) claimed the `:resolved` branch had a Map.pop bug. It was a false positive — `Map.pop` is functional, the `state` variable still references the original map, and `get_in` returns the resolved value as intended.
But the expression IS awkward (pop-then-re-add-the-same-value). Simplified to match the already-cleaner BiDi SessionActor pattern: `Map.get` for lookup, `{:noreply, state}` for the :resolved no-op, `Map.delete` for pending. Same semantics, half the cognitive load.
Adds three regression tests covering the :resolved / unknown / pending-no-caller branches.
Test plan
🤖 Generated with Claude Code