fix(rn-fast-runner): honest hittable computation in iOS snapshots (#395)#519
Merged
Conversation
Investigation showed the refusal half of #395 was fixed by #396 (@@ref double-prefix; the 'no longer hittable' wording comes from the stale-ref path, not a hittability gate). The remaining defect: the iOS runner's computedSnapshotHittable occlusion loop counts trailing transparent full-screen containers as occluders, marking every node hittable=false on all screens (85/85 on the wizard modal, 226/226 on the Tasks screen, device-verified). Approved design drops the occlusion term (enabled ∧ non-empty frame ∧ center-in-viewport) and decouples shouldInclude filtering from the emitted flag. Refs #395 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…395) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rebased fix/395-hittable-computation onto post-#498 main: canonical runner source is packages/rn-fast-runner (host copies are generated via build:host-runtimes), TS moved to packages/rn-dev-agent-core, changeset package is rn-dev-agent-core, and Task 4 gains the PR #517 hittable-first screen-rect consumer compatibility check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er check Codex-pair review flagged the CGRect.contains-delegated edge test as environment-dependent; the predicate now specifies [min, max) bounds directly and both edge tests assert deterministically. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…predicate (#395) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…uple filtering (#395) hittable now means "enabled and its center is on-screen" (half-open viewport bounds). The later-node occlusion loop counted transparent full-screen RN containers as occluders and marked every node false on real screens; real modal occlusion is unrepresentable anyway since RN modals live in their own UIWindow. Snapshot filtering is decoupled from hittable (its de-facto behavior under the always-false flag), keeping snapshot sizes stable. Host runner copies regenerated via build:host-runtimes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mment-budget note Snapshot filtering decoupled from hittable can only shrink node counts (trailing contentless overlay wrappers the old algorithm marked hittable are no longer included), never grow them — plan + changeset now say so precisely. Plan comment budget updated to include the reviewer-requested half-open-bounds note. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Android scoped out explicitly (two internally-inconsistent hittable sources — snapshot visible-to-user vs find isEnabled — filed as #520) and the rollout question settled: the version gate is env-passed at spawn so a runner-version-constant bump cannot detect artifact staleness; per-version cache dirs + release-process artifact evidence cover rollout instead. Refs #395, #520 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- spec §4: original Android instruction now defers to its own out-of-scope amendment (verify-only; Android changes are a separate follow-up) - spec §5: mark the non-functional 'bump runner version constant' mitigation as must-not-attempt, pointing to the settled amendment - plan Step 6: note getCachedScreenRect requires the device-wrapper path, not direct runIOS — drop or reroute the screenRect assertion accordingly Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…scoping - spec §5: new amendment making 'corepack yarn build:host-runtimes' a required rollout step (marketplace installs copy the generated host runner copies, not packages/rn-fast-runner) — codex-pair HIGH - spec out-of-scope: no Android change on this branch, unconditionally - plan Step 6: retitled/rescoped to the client-mapping + ranking path it actually exercises; ref-map/screenRect coverage deferred to the wrapper path + existing story-06 TS unit tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… 1-3) Honest hittable data (#395) activated three latent consumer assumptions calibrated to the old constant-false flag: - screen-rect union (#517 path): a center-on-screen straddling card inflated the estimate past the physical viewport (off-screen taps, scrollintoview false-pass). Application/Window node extents now cap the union on iOS; Android (no window-typed nodes) keeps the uncapped union per the #517 CI-Android constraint. - settle-hash: hittable encoded an UNquantized center-vs-edge bit that defeated the 4px bounds quantization (settle timeouts, phantom hierarchyChanged suppressing Story 05 re-taps). Dropped from the node hash — it is derived from enabled + rect + viewport, both hashed. - device_find ranking: the +1000 hittable bonus outranked real controls with on-screen body text. Type priority is now primary; hittable only breaks same-type ties. 2988/2988 unit tests. Refs #395, #519 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
finding 4) A pre-#395 runner artifact passes every staleness gate: the wire protocol is unchanged, no command verb was added, and runnerVersion is env-passed at launch — so the version-skew check reports whatever plugin spawned it. The artifact keeps silently emitting hittable=false for every node. HONEST_HITTABLE is now compiled into the /health capabilities, making absence-on-a-healthy-probe the one artifact-truthful staleness signal. The client queues a warn-once advisory through the existing pendingFastRunnerArtifactNote channel (surfaced as meta.note by the open and dispatch paths) — advisory only, never a forced rebuild, honoring the no-silent-multi-minute-xcodebuild contract. Host-runtime mirrors regenerated. 2993/2993 unit tests. Refs #395, #519 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
copyCleanDir copied the entire runner package including build/DerivedData, then stripped it post-copy. DerivedData's CompilationCache is a hardlink/APFS-clone-dedup'd store: 127M on disk materialized as 16G+ when cpSync expanded every link, filling the disk (ENOSPC) before the strip ran. Exclude build output via cpSync's filter so it is never copied. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- changeset: note HONEST_HITTABLE capability + stale-artifact advisory - reword dead-control language in snapshot predicates (finding 6) - align iOS fallback behavior with the capability gate (finding 7) - drop dead occlusion logic left from the old heuristic (finding 8) - refresh stale comments referencing the removed heuristic (finding 9) Mirrors regenerated via build:host-runtimes; 2993/2993 unit tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
1 participant
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.
What & why
Closes #395. The iOS runner's per-node
hittableflag now means "enabled and its center is on-screen" (plausibly tappable, half-open viewport bounds) instead of the old occlusion heuristic.The old
computedSnapshotHittablewalked later snapshot nodes and marked a node non-hittable if any later node's frame contained its center. On real RN screens this counted trailing transparent full-screen containers (gesture-handler roots, portal hosts, collapsed bottom-sheet wrappers) as occluders and drove nearly every node tohittable=false— poisoningdevice_findcandidate ranking,device_batch's dead-control annotation, and starving the hittable-first screen-rect union (#517) into its all-nodes fallback. Front-most is unrepresentable fromXCUIElementSnapshotdata anyway: RN modals get their ownUIWindow, so occluded content is absent from the XCUI tree entirely.Approach
Two pure, unit-testable predicates land in a new
SnapshotPredicates.swiftin theRnFastRunnerUITeststarget:computeSnapshotHittable(enabled:frame:viewport:)—enabled ∧ non-empty frame ∧ center-in-viewport, with an explicit half-open[min, max)bounds check (edge policy is deterministic, not delegated toCGRect.contains).shouldIncludeSnapshotNode(...)— snapshot filtering, with nohittableparameter. Under the old always-false flag, filtering was de-facto content/type-based; making that explicit keeps snapshot sizes stable whilehittablegains its new meaning.RnFastRunnerTests+Snapshot.swiftis rewired to call both, and the dead occlusion machinery is deleted (computedSnapshotHittable,isOccludingType,flattenedSnapshots,laterSnapshots, plus theflatSnapshots/snapshotRangescontext fields). The collapsed-tab fallback's realXCUIElement.isHittableis untouched (out of scope). No wire-shape change (RUNNER_PROTOCOL_VERSIONstays 1); host runner copies regenerated viabuild:host-runtimes.No TypeScript changes — consumers start receiving meaningful data for free.
Testing
enabled∧onscreen→true,disabled→false, empty/null frame→false, off-screen center (wizard pane at x=423 on a 402pt viewport)→false, half-open edge policy, and filtering fully independent ofhittable. The full bundle compiling after the deletions proves no dangling reference survived.dist/churn (no TS sources on this branch). The fix(story-06): post-merge review fixes — hittable-first screen rect + 4 more (#387) #517 hittable-first screen-rect union was written for honest hittable semantics — this PR activates its primary branch on iOS; thestory-06-screenrect-system-windowstests stay green.CommandDispatcher.kt:168-178already mapshittablefrom UIAutomator'svisible-to-userattribute (native visibility, no occlusion heuristic) — consistent with the new iOS semantics.Device evidence
Checkout-built (new-code) runner, real device (iPhone 17 sim), expo-dev-client launcher — a real screen containing 20 stacked full-screen (402×874)
Othercontainers, the exact pattern the old heuristic penalized: 80/80 nodeshittable, disabled→false invariant holds, no uniform-false pathology.Docs
Spec:
docs/superpowers/specs/2026-07-09-395-hittable-computation-design.md· Plan:docs/superpowers/plans/2026-07-09-395-hittable-computation.md🤖 Generated with Claude Code