Skip to content

fix: close the remaining link-loss path in the picker batching - #1

Merged
jnahian merged 1 commit into
alimuzzaman:codex/fix-concurrent-link-pickerfrom
jnahian:fix/picker-batch-review
Sep 7, 2026
Merged

jnahian merged 1 commit into
alimuzzaman:codex/fix-concurrent-link-pickerfrom
jnahian:fix/picker-batch-review

Conversation

@jnahian

@jnahian jnahian commented Sep 7, 2026

Copy link
Copy Markdown

Follow-up to jnahian#14 — review fixes, targeted at that PR's branch so they land as part of it. Merging this updates jnahian#14.

Thanks for this; the batching design is sound and the seam tests are genuinely good. Review turned up one path that can still lose links, plus a set of smaller things.

The blocking one

finishOpening presents the links that arrived during a launch, and then the browser it just opened — launched with activates = true — takes key a beat later. windowDidResignKey read that as click-away and discarded a batch the user had never seen: not opened, not copied, not in the fallback. The comment at the pick call site asserts this can't happen, but nothing established it.

A batch surfaced by finishOpening is now marked unseen and ignores focus loss until the user proves they've looked at it — a key press or a click on the panel. Hover deliberately doesn't count: the panel opens at the cursor, so mouse drift would clear the flag in the same moment the browser takes key. A click can't come from a browser activating, so accepting it keeps click-away dismissal working the way 0.8.2 fixed it; without it an unseen panel would need a click and an Esc to clear. Pickers the user opened themselves behave exactly as before.

PickerPanelControllerTests covers all three states, and the unseen-survives test fails if the guard is removed.

The rest

  • The no-browsers degrade path issued one NSWorkspace request per link — the same race the batching removes everywhere else. It goes through a single Dispatcher.openInBrowser(urls:) now, via a new AppState.openInFallback(urls:).
  • Arriving links appended below the fold of the link list and were never revealed; the list scrolls to the newest arrival.
  • The browser list was force-scrolled back to the selection on every arrival. That's gone. Worth flagging: it does not make a hand-scroll survive an arrival — SwiftUI restores its own remembered offset when the content changes. I wrote a test for that, watched it fail with the fix already applied, and dropped it rather than ship a test asserting something untrue. Separate problem, not addressed here.
  • Row heights were hardcoded at 17/34pt and feed the panel's fittingSize, so Larger Text clipped the list. They're @ScaledMetric now, identical at the default text size (the layout test's 34n - 2 still holds).
  • resizePanel ran twice per arrival, and read fittingSize in the same turn it set maximumHeight — a panel moved between screens could be measured against the constraint it just left.
  • installPanelForTesting behind #if DEBUG; the never-read state field removed (the closures already capture what they need); Environment.live is a static let rather than rebuilding six closures per Dispatcher.
  • JunctionAppTests declares JunctionMacKit explicitly instead of resolving it transitively, and DispatcherBatchTests moves to its own JunctionMacKitTests target.
  • The layout tests' fixed 50ms settle() is now bounded polling — 50ms is not enough on a loaded runner and would flake.
  • Dropped docs/picker-multiple-links.png; nothing in the repo references it, and the PR description links it from a raw URL anyway.
  • Extended the ## Unreleased bullet to cover the new user-visible behavior.

Verification

  • swift test: 113 pass on macOS (109 → 113)
  • swift test in swift:6.0 under Docker, matching the Linux CI leg: 87 pass
  • swift build -c release: clean, so the #if DEBUG seam holds in a shipping build
  • cd web && npm test: passes, so the changelog edit parses

Not verified: the universal bundle + relocated-resource CLI smoke test, and real browser-tab delivery. Those need Actions authorized on the fork.

One I left alone deliberately: choicesProvider() now running once per session rather than once per link. It's a behavior change from main, but freezing the browser list while the picker is open seems right — rows shifting under the cursor mid-selection is worse than a slightly stale list. Happy to add the refresh back if you disagree.

Review of #14 surfaced a path that still loses links. finishOpening
presents the links that arrived during a launch, then the browser we just
opened — launched with activates = true — takes key a beat later.
windowDidResignKey read that as click-away and discarded a batch the user
had never seen: not opened, not copied, not in the fallback.

A batch surfaced by finishOpening is now marked unseen and ignores focus
loss until the user proves they have looked at it, by pressing a key or
clicking the panel. Hover deliberately does not count: the panel opens at
the cursor, so mouse drift would clear the flag in the same moment the
browser takes key. A click cannot come from a browser activating, so
accepting it keeps click-away dismissal working as it has since 0.8.2 —
without it, an unseen panel would need a click *and* an Esc to clear.
Pickers the user opened themselves are unaffected.

Also from the review:

- The no-browsers degrade path issued one NSWorkspace request per link,
  which is the race the batching was meant to remove. It now goes through
  a single Dispatcher.openInBrowser(urls:) like every other path.
- Arriving links appended below the fold of the link list and were never
  revealed. The list now scrolls to the newest arrival.
- The browser list was force-scrolled back to the selection on every
  arrival; that redundant scroll is gone. Note this does not make a manual
  scroll survive an arrival — SwiftUI restores its own remembered offset
  when the content changes, which this does not address.
- Row heights were hardcoded at 17/34pt and drive the panel's fittingSize,
  so Larger Text clipped the list. They scale with the text size now.
- resizePanel ran twice per arrival and read fittingSize in the same turn
  it set maximumHeight, so a panel moved between screens could be measured
  against the constraint it just left.
- installPanelForTesting is behind #if DEBUG, the dead AppState field is
  gone, Environment.live is a static let, JunctionAppTests declares
  JunctionMacKit explicitly, and the Dispatcher tests move to their own
  target rather than resolving the module transitively.
- Drops docs/picker-multiple-links.png, which nothing in the repo
  references, and replaces the layout tests' fixed 50ms settle with
  bounded polling so a loaded runner doesn't flake.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MQGKvo7zQ2D5sV31uAxfnM
@jnahian
jnahian merged commit 0810472 into alimuzzaman:codex/fix-concurrent-link-picker Sep 7, 2026
@jnahian

jnahian commented Sep 7, 2026

Copy link
Copy Markdown
Author

Applied these directly to codex/fix-concurrent-link-picker instead (commit 0810472), so they're now part of jnahian#14 rather than a separate review round. Closing this — the full rationale for each change is in the commit message and in the description above, and the dismissal-semantics change is the one worth a second opinion if you have one.

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