Repository navigation
Search focus bug-fix - #129
Merged
Merged
Conversation
MikeMcQuaid
approved these changes
Aug 20, 2026
⌘F published a `Binding<Bool>` wired to `.searchable(isPresented:)` and set it to `true`. Two things were wrong with that. `isPresented` is not focus. A macOS toolbar search field stays presented after the cursor leaves it, so once the field had been shown the binding was already `true` — and writing `true` over `true` is not a state change, so SwiftUI did nothing with it. ⌘F went dead until something else happened to reset the flag. And on Installed/Upgrades the view hosting `.searchable` observed nothing: `InstalledUpgradesContainer.body` read only `mode`, handing the search state to `.searchable` purely as `$installed.isSearchFieldPresented`. A Binding's getter is lazy, so building one registers no Observation dependency — a model-side change had no reliable path to invalidate the view owning the field. Discover escaped this because its body reads a dozen model properties and re-renders constantly, which is why only those two tabs misbehaved. Publish the ⌘F contract as `FocusSearchFieldAction`, a callable that fires every time, and have both panes drive a real `@FocusState` through `.searchFocused`. Presentation becomes view-owned `@State`; the models keep a flag mirroring the actual cursor position. The action marks the models before moving the cursor because the package list auto-focuses off `shouldFocusList`, and a list re-inserted in the gap would pull the cursor straight back out. The `onChange(of:query)` keeps a filtered list's field on screen and is also what establishes the missing Observation dependency. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The three view models called this `isSearchFieldPresented` back when it was bound straight to `.searchable(isPresented:)`. It now mirrors `.searchFocused`, so the old name describes the wrong thing — and misleadingly so, since presentation was never a valid signal for the question `shouldFocusList` asks. A macOS toolbar field stays presented after the cursor leaves it, so the list was declining to claim focus long after the search field had stopped holding it. Pure rename plus the doc comments and test names that referred to presentation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The suite had no test for ⌘F at all. `BrewUISearchField.activate()` presses the shortcut only when the field is absent and then clicks the field regardless, so every search test passed with the shortcut completely dead — which is how the regression this covers reached a release build. Add keyboard-only helpers alongside it and seven cases across Installed, Upgrades and Discover. Each asserts that typed characters arrived *in the field* rather than that the field looks focused: the shortcut's job is to move the cursor, and only text landing proves it moved. The re-focus cases are the regression proper — they move the cursor out while leaving the field populated, which is the state the old presentation binding could not distinguish from "the user is still typing", then press ⌘F again. `activate()` keeps its click, since its callers' subject is the search and not the shortcut; its doc comment now says so instead of claiming to test ⌘F. Compiles and lints; not run — the sandbox cannot launch the XCUITest runner (LaunchServices -10810). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two Upgrades cases failed on CI with an empty field value. `assertValue` read `element.value` the instant after typing, which races the app's next render, the flake `BrewUIElement` exists to prevent. Wait on the value like the other element helpers wait on existence. Both cases also filtered Upgrades down to zero matches, which swaps the list for the empty state while the keystrokes are still landing. The subject is focus, not filtering, so they now type a query that keeps a row on screen and settle the list before pressing ⌘F. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
graeme
force-pushed
the
search-focus-bug
branch
from
August 22, 2026 08:00
d7f7e99 to
41f6287
Compare
⌘F used to resign the list's focus in the same update that asked a not-yet-presented search field to take over. Neither held the keyboard and no further event re-fired, so the shortcut looked dead until pressed again — the "press it four or five times" report. Two independent `@FocusState`s made that gap representable. A single target cannot, and unlike the `@FocusState`s it is a plain value, so the orderings that only showed up as a UI-test flake on CI are now deterministic unit tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both tabs share one container, so they now share one `@FocusState` threaded down into the packages views. The per-view focus state and its `.task(id: shouldFocusList)` auto-claim are gone: the container decides who gets the keyboard, and a ⌘F that has to present the field first leaves the list holding it until the field can accept the handover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same wiring as Installed and Upgrades. The trending-loaded-and-not-searching gate that used to live in `shouldFocusList` moves to the view as `canFocusList`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`isSearchFieldFocused` and `shouldFocusList` on the Installed, Upgrades and Discover view models no longer drive anything. Their tests went with them: they asserted the mirrored flag rather than who actually holds the keyboard, so they passed just as happily while ⌘F was broken. Doctor keeps its own `shouldFocusList` — it has no search field to compete with. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
graeme
force-pushed
the
search-focus-bug
branch
from
August 23, 2026 00:49
10be8ea to
923f1f5
Compare
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
2 participants
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.
PR: Fix Cmd+F not focusing the search field on Installed and Upgrades
Summary
Cmd+F took several presses before the search field accepted the cursor on the
Installed and Upgrades tabs, and past a certain point stopped working entirely.
Two independent defects were responsible. Both are fixed, and the shortcut now
has test coverage on all three searchable tabs.
Changes
Shortcut behaviour (e985945)
SearchCommandspublishesFocusSearchFieldAction, a callable, in place of aBinding<Bool>. Writingtrueover an already-true presentation binding isnot a state change, so every press after the first was discarded.
@FocusStatevia.searchFocused. Presentation becomes view-owned@State.InstalledUpgradesContainer.bodyread no observable state, so model-sidechanges had no reliable way to invalidate the view hosting
.searchable. Itnow reads the active query. Discover was unaffected because its body reads
enough model state to re-render constantly, which is why only two tabs broke.
Naming (4f7ac84)
isSearchFieldPresentedbecomesisSearchFieldFocusedin the three viewmodels.
shouldFocusListnow keys off the cursor. Presentation was never avalid signal: a macOS toolbar field stays presented after the cursor leaves.
Comments (d61b195) and tests (e8939ca)
the cursor leaves the field.
BrewUISearchField.activate()clicks the field,which is why a dead shortcut kept the suite green; its doc no longer claims
to cover Cmd+F.
Why this split
Every commit builds and passes tests standalone, so e985945 is a safe bisect or
revert point independent of the rename that follows it.
Testing
scripts/test, 779 tests passingswift build,Brewscheme,Brew-UIbuild-for-testingXCUITest runner (LaunchServices -10810). Needs a
scripts/test-uirun.PR checklist
building the package, app and UI test target, the full unit suite, and all
three linters. Runtime behaviour is NOT verified: the UI tests could not
be executed, and the original report was release-build only.
Follow-ups
hasFocus, because which element AX reports as focused for a SwiftUI toolbarsearch field could not be confirmed here. Worth tightening if it holds up.