Skip to content

W-555: read Arc's tab URL off the main thread so typing isn't blocked#175

Merged
wr merged 4 commits into
mainfrom
wells/w-555-arc-browser-search-bar-getting-blocked-by-mojito
Jul 22, 2026
Merged

W-555: read Arc's tab URL off the main thread so typing isn't blocked#175
wr merged 4 commits into
mainfrom
wells/w-555-arc-browser-search-bar-getting-blocked-by-mojito

Conversation

@wr

@wr wr commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Fixes the Arc "new-tab search bar gets blocked" bug (GitHub #173).

Root cause

Typing a word then a terminator (space/./,/!/?) in Arc's command bar stalled — the first keypress after the word did nothing, a second was needed.

The terminator triggers the ambient-emoticon check (<3→❤️). Engine.process handles it by calling AppContextDetector.current() for a fresh per-site exclusion check → BrowserURL.detect. Arc suppresses its web a11y tree, so every AX path returns nil and detect fell through to a synchronous NSAppleScript round-trip to Arc. That whole chain runs inside the CGEventTap callback (synchronous on the main run loop). A busy Arc replies slowly, the callback exceeds the tap timeout, macOS fires kCGEventTapDisabledByTimeout and drops the keystroke. Arc-only, because it's the sole browser on the AppleScript path.

Same failure class as the W-547 typing stall (slow cross-process IPC on the tap thread).

Fix

Never run the Apple Event on the main/tap thread. New BrowserURLCache resolves Arc's URL on a background serial queue and serves the last resolved value synchronously; detect() returns the cached URL and schedules a refresh for next time. Prefetches on app activation, and only serves a URL for the pid it was resolved from so a stale value can't bleed across an app switch. Mirrors FocusedElementCache's off-main-seed pattern. Bounded staleness (~one focus-change/keystroke) is fine for per-site exclusion matching, and it fixes all six BrowserURL.detect call sites, not just the emoticon path.

Test plan

  • Debug build succeeds; full suite green (266 tests, 20 suites).
  • Manual (needs live Arc): typing "how to cook" in a new Arc tab no longer stalls on space; a URL-excluded site still suppresses emoji/emoticon conversion.

Refs: W-555

Typing a word then a terminator (space/./,/!/?) in Arc's command bar
stalled: the first press after the word did nothing, a second was needed.

Cause: an ambient-emoticon terminator (`<3`→❤️ check) makes Engine.process
call AppContextDetector.current() for a fresh per-site exclusion check,
which calls BrowserURL.detect. Arc hides its web a11y tree, so every AX
path returns nil and detect fell through to a synchronous NSAppleScript
round-trip to Arc. That whole chain runs *inside the CGEventTap callback*
(synchronous on the main run loop). A busy Arc replies slowly, the
callback blows past the tap timeout, macOS fires tapDisabledByTimeout and
drops the keystroke. Only Arc hit this — it's the sole browser on the
AppleScript path. Same failure class as the W-547 typing stall.

Fix: never run the Apple Event on the main/tap thread. New BrowserURLCache
resolves Arc's URL on a background serial queue and serves the last value
synchronously; detect() returns the cached URL and schedules a refresh for
next time. Prefetches on app activation, and only serves a URL for the pid
it was resolved from so a stale value can't cross an app switch. Mirrors
FocusedElementCache's off-main-seed pattern. Staleness is bounded to about
one focus-change/keystroke, fine for per-site exclusion matching, and it
fixes all six BrowserURL.detect call sites, not just the emoticon one.

Verified: Debug build succeeds; full 266-test suite green.

Refs: W-555
@linear-code

linear-code Bot commented Jul 21, 2026

Copy link
Copy Markdown

W-555

@wr-claude-reviewer wr-claude-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean fix. The BrowserURLCache approach is exactly right: async serial queue for the Apple Event, synchronous cheap read on the hot path, pid-scoped cache to prevent stale URLs bleeding across app switches, and coalescing via the refreshing flag. The design faithfully mirrors FocusedElementCache and fixes all six BrowserURL.detect call sites in one go.

questionstatic let shared is lazy in Swift, so the activation-prefetch observer registered in init() doesn't fire until BrowserURLCache.shared is first touched (on the first BrowserURL.detect() call). CLAUDE.md notes that AppDelegate eagerly inits FocusedElementCache at launch for exactly this reason. A cold Mojito launch → switch to Arc (no prior : keystroke) will miss the prefetch and take two keystrokes to warm the cache. Intentional? If not, a _ = BrowserURLCache.shared in AppDelegate.applicationDidFinishLaunching would bring it into parity.

nitnonisolated on BrowserURL.normalizedURL is a no-op: BrowserURL is a plain enum with no actor isolation, so the keyword has no semantic effect here.

Cross-provider (Codex) review of the first cut found the async cache moved
NSAppleScript to a background GCD queue, but NSAppleScript is documented
main-thread-only — off-main it can hang or misbehave, which would also
wedge the single-flight `refreshing` flag forever. It also left the
synchronous AX tree walk on the tap thread for Arc even though that walk
always returns nil there.

Rework:
- detect(): for AppleScript-only browsers (Arc) branch straight to the
  cache before the AX walk, so no cross-process AX IPC runs on the tap
  thread for Arc at all.
- BrowserURLCache: run NSAppleScript on the MAIN thread (where it's
  supported) but via DispatchQueue.main.async so it lands on its own
  run-loop turn, never inside the CGEventTap callback. A 1s throttle keeps
  refreshes from recurring per keystroke; app-activation refreshes bypass
  it. The scheduled block always clears `refreshing`, so a failed/timed-out
  script can't wedge the cache (fixes the stuck-single-flight finding).

Hot-path read still does zero IPC — returns cached, schedules a refresh.
pid-guarded so a stale URL can't cross an app switch.

Verified: Debug build succeeds; full suite green (266 tests, 20 suites).

Refs: W-555

@wr-claude-reviewer wr-claude-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 2 follow-up — previous findings addressed.

The second commit ("address Codex review — main-thread AppleScript + skip Arc AX walk") correctly resolves both items from Round 1:

  • Nit fixednonisolated on normalizedURL is gone; the method is now fileprivate so BrowserURLCache (same file) can call it. That's the right access level for this.
  • Arc AX walk skipped — the BrowserURLCache check now gates before the AX walk, so Arc never enters the multi-node walk that always returns nil for it anyway. This is a meaningful tap-path improvement beyond just moving the AppleScript off-thread.

Lazy-init question (still open, still non-blocking): BrowserURLCache.shared isn't eagerly initialized in AppDelegate, so the activation-prefetch observer registered in init() won't fire until the first BrowserURL.detect() call touches shared. A cold Mojito launch → switch to Arc → immediate : keystroke will miss the prefetch and return nil for the URL (exclusions won't fire that one time). Consistent with the prior round: worth knowing, not a blocker. _ = BrowserURLCache.shared in AppDelegate.applicationDidFinishLaunching would bring it into parity with FocusedElementCache.

CI: only the SSOT pipeline is running (this review); no failing build/test checks.

Add a test seam to BrowserURLCache (inject resolver + clock, skip the
workspace observer) and a BrowserURLCacheTests suite that pins the property
whose violation caused the bug: the hot-path read does no blocking IPC and
defers the AppleScript to a later main-thread turn.

Covered deterministically (no live Arc / AppleScript / app switch):
- cold read returns nil and does NOT resolve synchronously — the resolver
  runs once, on a later turn, on the main thread (regression guard for
  AppleScript creeping back onto the tap thread)
- cached value served for the same pid; withheld across a pid change
- a burst of reads collapses to one resolve (single-flight)
- throttle skips a refresh within the interval, allows it after (injected clock)
- hot-path read returns in <50ms even when the resolver blocks 200ms

The true end-to-end "keys don't drop in Arc" is timing/Arc-dependent and
can't be asserted deterministically; these nail the cause instead.

Suite is green: 273 tests, 21 suites.

Refs: W-555

@wr-claude-reviewer wr-claude-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 3 follow-up — previous findings addressed; new commit adds tests only.

The third commit (077fe80) adds BrowserURLCacheTests.swift. No production code changed since Round 2. CI shows only the SSOT pipeline running — no failing build or test checks.

Test quality: the suite is well-designed. The ResolverSpy + injectable now clock lets the tests assert the exact concurrency contract without any live AppleScript or Arc process:

  • Cold cache returns nil synchronously and defers the resolver to a later turn (spy.calls == 0 before drain(); spy.calls == 1 after) — this is the crux of the W-555 fix and the right thing to pin.
  • ranOnMainThread assertion verifies the resolver (NSAppleScript) stays on the main thread.
  • Single-flight collapse, throttle window, and pid-guard are each independently covered.
  • hotPathReturnsPromptlyEvenIfResolverWouldBlock directly asserts the non-blocking property (elapsed < 50ms when resolver would block for 200ms).

Lazy-init (still open, third mention, still non-blocking): BrowserURLCache.shared is not eagerly initialized in AppDelegate, so the activation-prefetch observer doesn't register until the first detect() call. This was raised in Rounds 1 and 2; the test commit doesn't address it. A _ = BrowserURLCache.shared line in applicationDidFinishLaunching alongside the FocusedElementCache eager-init would bring it into parity — worth landing as a follow-up if the cold-launch miss is ever observed.

Nit (non-blocking): drain() uses a 30 ms Task.sleep to let the main queue drain the DispatchQueue.main.async refresh block. This is a standard pattern on Darwin and should be reliable, but if these tests ever become flaky on a loaded CI machine, bumping to 100 ms is a safe conservative fix.

Overall the PR is in good shape. Tests land the concurrency contract cleanly.

The prior commit added the test file but not the regenerated project (the
repo tracks project.pbxproj). run-tests.sh/the pre-push hook regenerate it
anyway, but keep the committed project in sync so a plain xcodebuild picks
up the suite.

Refs: W-555

@wr-claude-reviewer wr-claude-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 4 follow-up — no new changes since Round 3 beyond project regen.

The only commit since Round 3 (d8c097a) regenerates the Xcode project file to include BrowserURLCacheTests.swift in the test target. No production code changed.

All prior findings are resolved or were previously marked non-blocking:

  • Nit (nonisolated on normalizedURL) — fixed in Round 2, confirmed resolved in Round 3.
  • Lazy-init question (BrowserURLCache.shared not eagerly initialized in AppDelegate) — raised in all three prior rounds, non-blocking each time. Still technically open; a _ = BrowserURLCache.shared alongside the FocusedElementCache init in applicationDidFinishLaunching would close it whenever convenient.

CI shows only the SSOT pipeline running; no failing build or test checks.

Good to merge.

@wr
wr merged commit a03c6da into main Jul 22, 2026
10 checks passed
@wr
wr deleted the wells/w-555-arc-browser-search-bar-getting-blocked-by-mojito branch July 22, 2026 14:36
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.

1 participant