Skip to content

fix(link-hints): a pick decided when the labels come survives a click during their draw (follow-up of #32537) - #34042

Merged
lawrencecchen merged 43 commits into
feat-cmux-nextfrom
fix/p0-lh-pick-during-draw
Nov 3, 2026
Merged

lawrencecchen merged 43 commits into
feat-cmux-nextfrom
fix/p0-lh-pick-during-draw

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Follow-up of #32537 (P0 browser-click-then-type), review 6603845634 finding 1. It fixes a regression from #32537's commit e98112e.

When a full link-hint label is typed before the labels come, the pick is decided when they come and acts after their draw. Before this PR, the pick ran in the controller's work task. A click (the app's mouse-down observer calls cancel()) or a new f during the draw, which takes a few ms, cancelled that task. The pick then never clicked, and the labels the draw had put on the page stayed until a scroll or resize.

Now a decided pick runs in its own task (picking) that cancel() does not reach. The session ended when the pick was decided, so nothing in that session is left to cancel.

Testing

Fleet, package-test-lane.sh suite, 5 runs each, proof-step.py. A step counts only with "Test run with N tests", N > 0.

Observed / Not run

  • Observed: the runs above.
  • Not run: a real-key journey round. This path needs a full label typed before a page answers, then a click within a few ms of the labels' draw; the controller test covers it.
  • Still open from the same review (not this PR): finding 2 (the bound can end a newer session in a very small window) and finding 3 (a nested start from a delivered key during start's own cancel).

Changelog

Fixed: A link hint you type in full before the labels show now always clicks, also when you click the page while the labels appear

Proof

No capture: key routing; the proof is the tests.

…me key gets the keys from its cmux window (browser-click-then-type, red; ChildPageFocus takes a page-window lookup, unused yet)
…become key takes the keys from its cmux window, so typed keys reach the page (browser-click-then-type)
…om the cmux window to the page window, also for a delayed grant (browser-click-then-type, review of #32537; red). The ChildPageFocus hand-over is taken back: every grant path goes through CEFTab.grantFocus
…e cmux window to the page window, in CEFTab.grantFocus that every grant path uses (browser-click-then-type, review of #32537)
…h the keyboard passes its letters to the field (browser-click-then-type VERDICT app, STATUS 7518; red, the editing answer is a stub)
… a link hint session started by a stale unhandled-key report then ends and gives the letters it took to the field, in order (browser-click-then-type VERDICT app)
…eys typed during the page's answer are never held, reordered or doubled (review of #32537; red at 1972dd8, which holds and re-sends them)
…h key as typed); ask the page whether a field has the keyboard in a session-only script, so window hints keep the plain target list (review of #32537)
…ile red at 1972dd8: switch must be exhaustive in WindowHintController)
… text field once each in order; case 2, hint letters typed before the labels show pick and never reach the page (lead review of #32537; red: the rework sent them to the page, LinkHintHeldKeys is a stub)
…nt keys checked when the labels show (never sent to the page), or, when the page says a text field has the keyboard, sent to it once each in order (lead review of #32537)
… never taken again, so the keys that reach the page equal the keys typed (v5 of #32537 looped without end; red, LinkHintReplays is a stub)
…never held again, so link hints can no longer loop it (v5 of #32537 sent the same keys without end)
…n started) is never sent back; the keys the page receives equal the keys typed (v6 of #32537 doubled the first letters; red: the typed time is not read)
…to the page; the session takes it but never sends it back, so the page gets each key once (v6 of #32537 doubled the first letters)
… mutating; the macro's closure made it immutable: compile red at 706883c)
…ed after it reaches the page once; a key Chromium sent back is never the session's (browser-click-then-type, lead on STATUS 7738; red: route(typedAt:) is a stub)
…: it goes on to the page on its normal path, once, so a key still queued at the start is not lost and a key Chromium sent back is not doubled (browser-click-then-type, lead on STATUS 7738)
…reaches the page once in order (a field) or is a hint key; a key that is no hint key, Escape, an early end and a key-up keep their order; a key is the session's unless the app already dispatched it (browser-click-then-type, review of #32537; red: the new types are stubs)
… answer and then reaches the field once in order or is a hint key; a non-hint key, Escape, a click, a lost focus, a failed or slow page call never lose a key; a key-up waits behind its key-down; a key the app already dispatched is the page's (browser-click-then-type, review of #32537)
…es, and two identical keys in one refresh are two keys (lead on STATUS 7898; red: the answer needs targets; the two-keys test is a guard, green before and after)
…cts its targets only when no text field has the keyboard, so a heavy page's target walk never delays a field's keys; each page call's time goes to the input journal (browser-click-then-type, lead on STATUS 7898)
…ng delimiter sets the indentation; d17dbcb did not compile)
…g of event time and key code; a synthetic key by its object), so a plain key in a terminal makes 0 allocations again (KeyRouterBench: 66 per key at c62a2e4; browser-click-then-type)
…omium page window, so typing anywhere else does no link-hint work at all (lead on STATUS 8037)
…tood in for): a field gets every key once in order, a slow target collect never sends hint letters to the page, a click or no answer gives the held keys to the page, and a key Chromium sent back is the page's when Chromium loaded after the first key (review 6594559342; red: the bound covers the target collect and the page window class is cached as missing)
…remembered as missing before Chromium loaded), the answer's bound covers only the focus check (a heavy page's target collect never sends hint letters to the page), and a bound that passes after the tab closed still gives the held keys to the page (review 6594559342)
…: seeded random keys, clicks, focus changes, page answers and the bound against a reference model, checked after every event (no key lost or doubled, no hint letter to the page, order kept, one session); the controller's label scripts and answer bound get test seams (red: a label typed in full before the labels came keeps the session live while they draw)
…e session when the page answers, and the pick waits only for the labels' draw, so a key typed while they draw is the page's, never taken (it was lost, and the pick with it); found by LinkHintControllerFuzzTests
… sent back to the page no longer unmarks the rest of that send, so a key after two link-hint chords in one send-back goes to the page, never into the newest session (found by LinkHintControllerFuzzTests at e98112e: seed 4642252087, sequence 10)
…k or a new start during their draw: it removes the labels and acts (review 6603845634 finding 1, a regression of e98112e; red)
…eaches, so a click or a new start during the labels' draw no longer drops it and leaves the labels on the page (review 6603845634 finding 1)
…ick-during-draw

# Conflicts:
#	Packages/macOS/CmuxNext/Sources/CmuxNextBrowser/LinkHints/LinkHintController.swift
#	Packages/macOS/CmuxNext/Tests/CmuxNextBrowserTests/CEFDeferredFocusTests.swift
#	Packages/macOS/CmuxNext/Tests/CmuxNextBrowserTests/LinkHintControllerTests.swift
@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review 6a79970: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr34042 -f ref=fix/p0-lh-pick-during-draw -f platform=macos -f backend_mode=local

red: cmux-next checks (god files, concurrency, crash safety, l10n); no finished base run on 83c8336 to compare; do not merge until green; see lane-rules

Shipped in:

@lawrencecchen lawrencecchen added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 3, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent subagent): SHIP at 5e3cb1b

I found no correctness defect. I read the diff against origin/feat-cmux-next (2 files only), all of LinkHintController.swift, removeScript and drawScript, CEFTab.evaluate, CEFRuntime.evaluate and its waiters, CEFTab.sendClick, the callers (the AppServices mouse-down observer and the BrowserHandlers openInSplit), the test harness, REVIEW.md §1–2 and skills/cmux-review/SKILL.md. I compiled nothing and ran no tests.

What I checked

  1. A click during the draw (LinkHintController.swift:196-202). The click calls cancel(). session is already nil, so it returns early and does not touch picking. When the draw answers, pick() runs removeScript, which returns true, and then clicks the target. The pick was decided when the last label letter was typed, before the click, so acting on it is consistent. The person-typed path through apply(.pick) already behaved this way: pick() never checks Task.isCancelled, and the evaluate continuation (CEFReplyWaiters.reply) does not respond to cancellation. Before the fix, cancelling work therefore did not stop that path's script. The PR makes both paths the same. Before e98112e, the design was "click cancels the pick and removes the labels". This PR chooses "the pick survives". Both remove the stale labels. I accept the choice.

  2. A new start during the draw. start calls cancel(), which is a no-op for the ended session. The old pick sends its remove script right after its own draw answers. The new session's draw is sent only after its focus and targets round trips. With in-order evaluation per browser, the old remove runs before the new draw, so it cannot remove the new labels.

    • The native sendClick does not go through NSApp. It therefore does not hit mouseDownObserver and does not cancel the new session.
    • If the old pick's click navigates the page, the new session's labels go stale. The existing scroll and narrow-false handling covers that.
  3. Two picks in flight. picking is a write-only holder. Assigning a new task does not cancel the previous one, so both picks act. The person typed two full labels, so that is intended. In the worst ordering, the second remove returns false and that pick does nothing. Nothing clicks at a stale position.

  4. A closed tab or window. The task holds tab strongly, as work did before.

    • evaluate throws .closed when isClosed, and failEvaluations(of:) fails any call in flight. The 5 s timeout bounds the rest. script then returns nil, and the == .bool(true) guard in pick() (line 251) stops the click.
    • sendClick also guards isClosed.
    • When the pane is gone, openInSplit makes splitBrowser throw and shows a notice on the weak chrome, or does nothing.
  5. removeScript == .bool(true) as the guard against stale positions. This is correct. A scroll, a resize or a navigation removes the overlay, the script returns false, and nothing is clicked.

  6. work = nil in apply(.pick) (line 244). This is correct. apply(.pick) is now reached only from interceptKeyDown, because collected handles .pick before it creates work. The draw task it cancels is the one before the pick. The remove is sent after that draw, so it removes the drawn labels.

  7. Swift 6. Task {} in the @MainActor class inherits main-actor isolation. The captures (script, current, target, tab) are the same ones the previous work task in collected captured, and the author's compile-check passed. The defer { deliver(resolved.page) } still runs on the new early return (line 202), so the keys after the label still reach the page.

  8. The test (LinkHintControllerTests.swift:204-229). It discriminates.

    • Without the fix: cancel() cancels work, the draw answers, and guard !Task.isCancelled drops the pick. The asked list stays ["draw"] and the expectation fails, as the author's red run reported.
    • With the fix: picking survives and remove is asked.
    • Because the stub returns .bool(false) for remove, the test proves "the pick ran and removed the labels", not "the target was clicked". That is enough for this fix.

Nits (not blocking)

  • In the pick branch of collected, work still points at the finished start task. A later cancel() cancels a completed task, which is harmless.
  • The test covers the click case. The new-start case uses the same cancel() path, so it is covered by the same code but has no test of its own.
  • The PR body could state the design choice from point 1 ("the pick survives the click", not "the click cancels the pick").

I posted nothing on GitHub.

Files:

  • /Users/lawrence/fun/cmux/.claude/worktrees/lh-pick-draw/Packages/macOS/CmuxNext/Sources/CmuxNextBrowser/LinkHints/LinkHintController.swift
  • /Users/lawrence/fun/cmux/.claude/worktrees/lh-pick-draw/Packages/macOS/CmuxNext/Tests/CmuxNextBrowserTests/LinkHintControllerTests.swift

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent subagent): SHIP at 52d8656

Head 52d8656 has the same change that was reviewed at 5e3cb1b. The merge changed nothing in the PR's paths.

  • Same change: The PR diff at 52d8656 (git diff origin/feat-cmux-next...52d8656651fe) and the reviewed diff (git diff 463b73873bc1 5e3cb1bf882e, from the old merge base) are byte-identical, and their stable patch-ids are the same. Both touch only LinkHints/LinkHintController.swift (+14/-4) and LinkHintControllerTests.swift (+27).
  • Merge: 52d8656 has two parents: 5e3cb1b and e3bb5b2 (fix(acpmux): forward result-line option through stable task start #34053, the cmux-tui fix). git show --remerge-diff 52d8656651fe is empty, so the merge resolved no conflicts in any file.
  • Changes from feat between the two merge bases (463b738 to e3bb5b2: 18 commits, 150 files): none touch LinkHint*, AppServices, KeyRouter, BrowserHandlers, CEFTab or other CEF files. The diff has no added or removed line with LinkHint, mouseDownObserver, sendClick or evaluate(. The Swift changes are in Cloud start/asks, Feed, CrashReporting, Settings schema and Sidebar favicons, and none of them is in a link-hint or click path.
  • Newer feat: feat has moved again since the merge, to 2e9aa60 (11 more commits). None of those commits touch these paths either.
  • Not run: I compiled nothing and ran no tests. This is a read-only diff and history check, and I posted nothing.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 3, 2026
…h-pick-during-draw

# Conflicts:
#	Packages/macOS/CmuxNext/Tests/CmuxNextBrowserTests/LinkHintControllerTests.swift
@lawrencecchen lawrencecchen removed the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 3, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent subagent): SHIP at 6a79970

The merge resolved the test-file conflict correctly, and the auto-merge of the controller joins the two features without problems. I did not compile or run anything; this review only reads the code.

1. PR diff now (git diff origin/feat-cmux-next...6a79970ab8da)

  • The diff is the same as the one reviewed at 52d8656: LinkHintController.swift +14/-4 and the test +27.
  • Since the merge base e3bb5b2, the PR side changed no other line in LinkHints/ or CmuxNextBrowserTests/.

2. Merge resolution (--remerge-diff)

3. Interaction between the two features

  • Session 1's pick runs while session 2 starts. Session 2's start → cancel() finds session == nil and only cancels work, so the pick goes on.
    • Session 1's draw call is sent before session 2's focus check. Session 1's remove is sent when its draw answers. Session 2's draw comes only after its focus and targets answers.
    • If the page runs its scripts in the order it gets them, session 1's remove therefore runs before session 2's draw.
    • Only if the page ran them out of order could session 1's remove clear session 2's labels. The base cancel() already has the same race between its fire-and-forget remove and the next draw, so this PR does not add it. Not blocking.
  • The same applies to the defer { deliver(resolved.page) } path (:213). A delivered key can start session 2 in the same turn. The picking task was already made and is not cancelled, and the order of the page calls is the same as above.
  • A second pick replaces picking. Assigning a new task to picking does not cancel the old one. Both picks run in the order they were decided, and nothing waits on picking. No harm.
  • The new test still works with fix(link-hints): an ended session never acts on a newer one (follow-up of #32537, review findings 2 and 3) #34137's ids. The flow is: focus says false, then the no-field path collects the targets with the matching id, then the pick goes into picking. The cancel() in the test then changes only work. The test is still red without the fix and green with it, for the same reason as at 52d8656.

No findings that block.

@blacksmith-sh

blacksmith-sh Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Found 12 test failures on Blacksmith runners:

Failures

Test View Logs
/tmp/tmp6eoetx5o/summary-missing.json (live):/summary-missing.json (live): View Logs
/tmp/tmpsm3603ae/summary-fail.json (live):/summary-fail.json (live): View Logs
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows/
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows
View Logs
[new_workspaces] the window draws no frames/[new_workspaces] the window draws no frames View Logs
[tabs] debug.tab_switch timed 0 of 40 switches/
[tabs] debug.tab_switch timed 0 of 40 switches
View Logs
[tabs] the window draws no frames/[tabs] the window draws no frames View Logs
nightly still handles the legacy tunnel extension/
nightly still handles the legacy tunnel extension
View Logs
test_skipped_steps_read_no_unkeyed_decide_output (main.Workflow.test_skipped_steps_
read_no_unkeyed_decide_output) (step='- name: Build nightly app (Release)', output='not
ary_probe')/
test_skipped_steps_read_no_unkeyed_decide_output (main.Workflow.test_skipped_steps_
read_no_unkeyed_decide_output) (step='- name: Build nightly app (Release)', output='not
ary_probe')
View Logs
test_skipped_steps_read_no_unkeyed_decide_output (main.Workflow.test_skipped_steps_
read_no_unkeyed_decide_output) (step='- name: Build nightly app (Release)', output='not
ary_probe')/
test_skipped_steps_read_no_unkeyed_decide_output (main.Workflow.test_skipped_steps_
read_no_unkeyed_decide_output) (step='- name: Build nightly app (Release)', output='not
ary_probe')
View Logs
test_skipped_steps_read_no_unkeyed_decide_output (main.Workflow.test_skipped_steps_
read_no_unkeyed_decide_output) (step='- name: Build nightly app (Release)', output='pro
file_channel')/
test_skipped_steps_read_no_unkeyed_decide_output (main.Workflow.test_skipped_steps_
read_no_unkeyed_decide_output) (step='- name: Build nightly app (Release)', output='pro
file_channel')
View Logs

...and 2 more test failures. View all on Blacksmith: run 41702723268, run 41702723426

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 3, 2026
@lawrencecchen
lawrencecchen merged commit d05f040 into feat-cmux-next Nov 3, 2026
208 of 224 checks passed
@lawrencecchen
lawrencecchen deleted the fix/p0-lh-pick-during-draw branch November 3, 2026 20:07

This branch was previously deployed

1 inactive deployment
ci-routing-agent — 6a79970a Deployed Nov 3, 2026 by lawrencecchen via wait for same-key tree publisher #107384
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant