Skip to content

Fix CI: add missing CaretGeometrySelector type referenced by tests#322

Merged
FuJacob merged 1 commit into
mainfrom
fix/caret-geometry-selector
May 27, 2026
Merged

Fix CI: add missing CaretGeometrySelector type referenced by tests#322
FuJacob merged 1 commit into
mainfrom
fix/caret-geometry-selector

Conversation

@FuJacob
Copy link
Copy Markdown
Owner

@FuJacob FuJacob commented May 27, 2026

Summary

The Tests workflow on main was red because FocusSnapshotResolverSelectionTests (added in #321) referenced a CaretGeometrySelector type that was never committed — cannot find 'CaretGeometrySelector' in scope. This adds that type as a pure helper in Support/ and wires FocusSnapshotResolver through it, removing the now-duplicated private selectCaretGeometry/SelectedCaretGeometry.

The extracted policy also reflects the behavior the tests encode: primary .derived caret geometry is now trusted over a deep .exact result and skips the deep AX-tree walk entirely (consistent with shouldSearchDeep returning false for .derived).

Validation

swiftlint lint --quiet Cotabby/Support/CaretGeometrySelector.swift Cotabby/Services/Focus/FocusSnapshotResolver.swift
# exit 0

xcodebuild test -project Cotabby.xcodeproj -scheme Cotabby -destination 'platform=macOS' \
  -only-testing:CotabbyTests/FocusSnapshotResolverSelectionTests CODE_SIGNING_ALLOWED=NO
# ** TEST SUCCEEDED **  5 tests, 0 failures

xcodebuild -project Cotabby.xcodeproj -scheme Cotabby -destination 'platform=macOS' build CODE_SIGNING_ALLOWED=NO
# ** BUILD SUCCEEDED **

xcodegen generate re-run; the only project drift is the new CaretGeometrySelector.swift source reference.

Linked issues

Refs #321

Risk / rollout notes

  • Behavior change: primary .derived caret geometry now wins over deep .exact and no longer triggers the deep AX walk. This trusts the focused input's own rect for .derived-quality geometry rather than escaping to a descendant leaf. Matches the trust policy asserted by the new tests.
  • Cotabby.xcodeproj/project.pbxproj regenerated to register the new source file.

Greptile Summary

This PR fixes a broken CI build by adding the missing CaretGeometrySelector type that the tests added in #321 referenced but was never committed. As a side effect it refactors FocusSnapshotResolver's private selectCaretGeometry/SelectedCaretGeometry pair into the new public CaretGeometrySelector enum, and changes the caret-geometry trust policy so that a primary .derived result is now trusted and the deep AX-tree walk is skipped for it.

  • CaretGeometrySelector.swift (new): pure, testable enum containing shouldSearchDeep and select; .derived primary geometry is now treated as trustworthy, reversing the old precedence where deep .exact could override primary .derived.
  • FocusSnapshotResolver.swift: removes the old private SelectedCaretGeometry struct and selectCaretGeometry helper, delegates both the search-deep decision and the geometry-selection precedence to CaretGeometrySelector.
  • project.pbxproj: registers the new source file in the Xcode build system.

Confidence Score: 3/5

The refactor is structurally clean but ships an intentional trust-policy change whose correctness for real Chrome sessions is not validated by the new unit tests.

The new CaretGeometrySelector class doc explicitly states that Chrome's AXTextArea labels a multi-line union rect .derived and it is "unusable for precise caret placement," then says the deep walk can recover a real .exact from the leaf. Yet shouldSearchDeep now returns false for .derived and select ships the primary .derived over any deep .exact. If Chrome still exposes that union rect as .derived, caret placement in web editors breaks silently — no integration-level test covers this path and the unit tests use synthetic coordinates that cannot distinguish a precise caret rect from a broad union rect.

CaretGeometrySelector.swift — its own header describes the Chrome scenario that the implementation no longer handles; whether that is stale documentation or a latent regression needs confirmation against a real Chromium editor.

Important Files Changed

Filename Overview
Cotabby/Support/CaretGeometrySelector.swift New pure type extracting caret-geometry trust policy; class doc describes the Chrome .derived-is-unusable scenario but the implementation no longer guards against it — shouldSearchDeep returns false for .derived and select trusts it over a deep .exact.
Cotabby/Services/Focus/FocusSnapshotResolver.swift Private selectCaretGeometry/SelectedCaretGeometry removed and delegated to CaretGeometrySelector; the call sites and surrounding throttle logic look correct, but inherit the behavior change in the selector.
Cotabby.xcodeproj/project.pbxproj Registers CaretGeometrySelector.swift in both the file-reference and build-file sections; looks correct and complete.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[resolveSnapshot called] --> B[Build primary candidate\ncaretRect + caretQuality]
    B --> C{CaretGeometrySelector\n.shouldSearchDeep?}
    C -- primaryRect nil\nor quality .estimated/nil --> D[Run deepWalkThrottle\nresolveDeepGeometrySource BFS\nup to ~200 AX nodes]
    C -- quality .exact or .derived --> E[deepResult = nil\nskip BFS entirely]
    D --> F[CaretGeometrySelector.select]
    E --> F
    F --> G{primary .exact?}
    G -- yes --> H[Ship primary .exact]
    G -- no --> I{primary .derived?}
    I -- yes --> J[Ship primary .derived\ndeepResult ignored]
    I -- no --> K{deepResult present?}
    K -- yes --> L[Ship deep result\nany quality]
    K -- no --> M{primary rect present?}
    M -- yes --> N[Ship primary fallback\n.estimated quality]
    M -- no --> O[Return nil →\nunsupported snapshot]
Loading

Fix All in Codex Fix All in Claude Code

Reviews (1): Last reviewed commit: "Fix CI: add CaretGeometrySelector type r..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

FocusSnapshotResolverSelectionTests referenced a CaretGeometrySelector
type that was never committed in #321, breaking the Tests workflow on
main. Extract the caret-geometry trust policy out of FocusSnapshotResolver
into a pure CaretGeometrySelector in Support/, exposing shouldSearchDeep
and select. Wire the resolver through it.

Per the tests, primary .derived geometry is now trusted over a deep .exact
result and skips the deep AX walk entirely, matching shouldSearchDeep.
@FuJacob FuJacob merged commit 072fb67 into main May 27, 2026
4 checks passed
@FuJacob FuJacob deleted the fix/caret-geometry-selector branch May 27, 2026 11:14
Comment on lines +12 to +15
/// The regression these protect against is trusting a descendant rect over the focused input's own
/// usable rect (or vice versa). Chrome's AXTextArea answers BoundsForRange with a multi-line union
/// rect — labelled `.derived` but unusable for precise caret placement — while the leaf AXStaticText
/// holding the active line carries a real `.exact` rect that the deep walk can recover.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Class doc contradicts the implementation it describes

Lines 13–15 say Chrome's AXTextArea labels its multi-line union rect .derived, it is "unusable for precise caret placement," and "the deep walk can recover" a real .exact rect from the leaf AXStaticText. But shouldSearchDeep now returns false for .derived, and select prefers primary .derived over a deep .exact (see testPrimaryDerivedWinsOverDeepExact). The protection described in the header is exactly the behavior this PR removes. A Chrome user with a .derived primary union rect will get that unusable rect shipped; the deep walk that recovered the leaf's .exact no longer fires.

Either the doc is stale (Chrome's .derived geometry is now actually usable) and the comment should be updated or removed, or the code regresses Chrome caret placement for editors that still expose a wide union rect under .derived.

Fix in Codex Fix in Claude Code

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