fix(ios): make both rect guards refuse the sentinel and let the fold answer clipped carriers - #2945
Merged
Merged
Conversation
…ehind `isPositiveFiniteRect` compared four components, and `CGRect.infinite` is built of four finite Doubles whose center is (0, 0): x=y=-8.988465674311579e+307, w=h=1.7976931348623157e+308. The bridge's `rectDictionary` refuses only a non-finite component, so that box crossed the wire intact, `viewportFromRoot` called it `.reported`, and every node center on the screen landed inside it -- the host path publishing `hittable: true` for a whole tree whose viewport read had failed. The Swift twin already refused it with `!rect.isInfinite`; the two declarations of one rule disagreed on exactly the input that started #2891. Refused now, in both twins: a non-finite component, a box whose finite components overflow its own right or bottom edge, and the sentinel itself. The extent check is a separate hole, not the sentinel's fix -- the sentinel's own extents are finite, which is why a value refusal stays and is pinned as non-vacuous. It lives in the shared guard rather than in one producer's parser because every TypeScript producer (the simulator bridge, the runner wire, a remote provider's tree) feeds this one function. The old `isPlottable` accepted both newly-refused classes and nothing else changed: for every other box the five comparisons are the ones it had.
The sentinel row declared `typescript: false` on the claim that `frameFromGuest` would refuse the frame before the predicate saw it. Its components are finite and non-negative, so that refusal never happens, and the row's `asymmetry` note said the shared rule contained a magic value it did not have. The row now runs in both languages against the guard that actually refuses the box. Adds the overflow class the component checks cannot reach, and pins three things that were previously only argued in prose: a refused box never becomes a declared viewport on either side; the wire carries no `hittable` key at all when the viewport is unknown, while a disabled node and a root stay declared `false`; and a table row neither language runs is now rejected instead of accepted. Non-vacuity, verified by mutation: dropping `!rect.isInfinite` reddens the Swift rows and the declaration test; dropping the value refusal from the TypeScript guard reddens the kernel test and the bridge reader's viewport case; flipping a row to `swift: false, typescript: false` reddens both lanes.
`Box`'s memberwise initializer was internal, so any code in the package could still write `Box(positiveFinite: .infinite)` and wrap it in `.reported`: the guarantee held for callers outside the module and nowhere else, while the comment on the type claimed the factories were the only constructors. The check moves into the initializer, which is now failable, so an unchecked box is not a thing a caller can name and the factories simply translate a refusal into `.missing(reason: .invalid)`. This is a compile-shape guarantee, so it has no runtime test of its own; the declaration test added alongside it covers the behavior both callers observe.
A third review pass at `f620148f60` refuted the reason this had been left alone: `shouldInclude` exempts `Application` and `Window` from the visibility test, so a carrier a scroll anchor clips to nothing is RETAINED, and it reaches the `hittable` decision with the source bit undecided. There `flatMap` over the source bit publishes absence for a node whose own frame is degenerate, which is exactly the corner where ADR 0004 promises "disabled or degenerate nodes stay declared `false`" and where the `nil` case is documented as "only containment is left to decide". The source bit is now consulted only for a declared `false`; anything undecided is re-decided on the clipped frame, so `nil` means containment is the open question and nothing else. With a box the source bit is never `nil`, so the reported-viewport path is untouched. Also two comments that overclaimed: the wire test does not prove what the Swift encoder could regress into, and the TypeScript predicate does not validate the viewport it is handed -- it cannot, because it answers in `boolean`, so the requirement on callers is now stated where a caller reads it.
Both were added without compiling the runner target, which is not reachable from the differential lane: `SnapshotRect` takes Doubles and the new fold test spelled its y offset in Ints, and the fold test reached for `RawAXNode.replacing`, which is internal to the presentation module and invisible to the runner tests. The fold cases now state their source bit where the node is built, through a defaulted parameter on the test's own builder, which is also what `normalized()` leaves behind when the viewport read failed. Verified with the runner test target built with `AGENT_DEVICE_XCUITEST_INCLUDE_UNIT_TESTS=1` and run on the `rnav-repro` simulator: 9 selected cases, 0 failures, including both new ones and `testRegularFoldKeepsWindowCarriersButNeverHittableOutsideClip`, which is the reported-viewport neighbour of the new fold policy.
…mise Four claims around the previous commits were wider than the code. `rect.ts` justified trusting its `viewport` argument by saying an unknown viewport is not representable in `SnapshotViewport`, which has a `.missing` case for exactly that. The reason the Swift twin needs no such request is that it takes that fact and returns `Bool?`, so it can answer "no idea"; a `boolean`-returning rule cannot, which is why the request exists at all. The wire test's comment promised that a failed viewport read publishes no `hittable` at all, over its own assertions, which require a declared `false` from the root and from the disabled node. The bit that goes absent is the one containment would have decided. The overflow fixture row said the box has finite extents and then handed it to a guard that refuses it because `x + width` is infinity. Its components are finite; its right edge is not. The ADR paragraph on the two guards said the guard being replaced had accepted both newly-refused classes. Nothing was replaced here: the Swift twin already refused the sentinel by identity, and this branch adds the extent check there and both refusals on the TypeScript side. The narrowing now names which twin lost what, which is also the answer to whether any input changed classification. Differential lane green after the row rename: Swift 17 tests, node 6 tests, 0 failures. Runner test target rebuilt with unit tests and rerun on the iPhone 17 / iOS 26.2 simulator: 9 selected cases, 0 failures.
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apple/snapshot-presentation/Sources/AgentDeviceSnapshotPresentation/SnapshotVisibilityFold.swift">
<violation number="1" location="apple/snapshot-presentation/Sources/AgentDeviceSnapshotPresentation/SnapshotVisibilityFold.swift:247">
P3: This comment says a carrier clipped to nothing is `nil` today, but the helper now returns `false`; update the comment to match the implemented policy.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Comment on lines
+247
to
+248
| /// open question. A `visibilityExempt` carrier clipped to nothing by a scroll anchor is that | ||
| /// `nil` today, and its own frame already answers the question. |
There was a problem hiding this comment.
P3: This comment says a carrier clipped to nothing is nil today, but the helper now returns false; update the comment to match the implemented policy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/snapshot-presentation/Sources/AgentDeviceSnapshotPresentation/SnapshotVisibilityFold.swift, line 247:
<comment>This comment says a carrier clipped to nothing is `nil` today, but the helper now returns `false`; update the comment to match the implemented policy.</comment>
<file context>
@@ -244,4 +241,23 @@ public enum SnapshotVisibilityFold {
+ /// The fold's share of the `hittable` policy (#2891). A declared `false` is kept; anything the
+ /// source left undecided is re-decided on the clipped frame, which refuses a disabled or
+ /// degenerate node with no viewport to consult and answers `nil` only while containment is the
+ /// open question. A `visibilityExempt` carrier clipped to nothing by a scroll anchor is that
+ /// `nil` today, and its own frame already answers the question.
+ private static func clippedHittability(
</file context>
Suggested change
| /// open question. A `visibilityExempt` carrier clipped to nothing by a scroll anchor is that | |
| /// `nil` today, and its own frame already answers the question. | |
| /// open question. A `visibilityExempt` carrier clipped to nothing by a scroll anchor is | |
| /// `false` today, not `nil`, and its own frame already answers the question. |
Member
Author
|
This is ready for human review at 1e1fa90. Code looks correct: both rect guards now refuse the sentinel, and the fold's answer for a clipped carrier is the one the callers need. All 19 checks are green, and I saw no conflicts. Nothing is blocking. The live A/B on Preferences, the XCTest runner run, and the gate outputs are the author's own reports; I did not re-run them. I also did not check whether SnapshotBridgeRuntime.m serializes CGRectInfinite with full precision — if it prints shortened digits, the value won't match the TS identity check exactly, and the finite checks would only catch it when the rounding overflows to Infinity. A missing viewport can't be forced on a device, so the changed fold route (nil source plus a clipped carrier) has only unit coverage, which the author already states. Not blocking: |
thymikee
added
the
ready-for-human
|
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.
Follow-up to #2908, which merged before this review round finished. Carries the four findings against #2891; nothing merged is reverted.
Summary
Both guards refuse the box a failed read leaves behind. The TypeScript
isPositiveFiniteRectacceptedCGRect.infinite: its four components are finite, and so are both extents, so a check that reads only components or only extents waves it through. It also accepted finite components whose extents overflow. The SwiftisPositiveFinitealready refused the sentinel by identity, but it too accepted the overflowing shape. Both now refuse non-finite components, non-finite extents, andCG_RECT_INFINITEby value, so the sentinel is unusable on both sides of the boundary instead of only where an Apple frame is parsed. Every other box is classified the same as before; the narrowing is stated in the ADR rather than in prose.A box is checked where it is made.
Box(checked:)is the in-module path from aCGRectto a declared fact, and the viewport factories turn refusal into.missing(reason: .invalid). There is no longer a way to hold aBoxthat the shared guard would refuse.The fold answers a carrier its own clip already decided.
ApplicationandWindoware exempt from visibility, so a retained carrier can reach the fold with a degenerate clipped frame and no source bit. The mergedflatMappublishednilthere, which reads as "containment is undecided" about a node the policy already refuses.clippedHittabilitykeeps the source's ownfalsefirst, answers from the clipped frame second, and withholds only when containment is the open question.The golden table says what it runs. The sentinel row now executes in both languages, a row with finite components and a non-finite extent pins the overflow refusal in both, a nonfinite row is labelled as pinning the guard rather than a producer, and a row declaring
falsein both languages is rejected by both readers instead of being silently skipped.SnapshotBridgeRuntime.mis deliberately unchanged. Refusing to serialize a box there would turn "the source reported an unusable frame" into "the source reported no frame", which is a different claim about the device, and that file has no test seam to notice the difference.Validation
Tested SHA
1e1fa90d84, on top oforigin/mainatfa1082dd14.pnpm test:ios-snapshot-differential: Swift 17 tests, node 6, 0 failures. The sentinel row now runs in both languages.pnpm check:xctest-selection: 321 declaredRunnerTestsmethods, 0 reachable by no lane.check:layeringand the eager-closure budget pass, andpresentIosRunnerSnapshotkeeps its zero fold call count.AGENT_DEVICE_XCUITEST_INCLUDE_UNIT_TESTS=1on an iPhone 17 / iOS 26.2 simulator:Executed 9 tests, with 0 failures, including both new cases andtestRegularFoldKeepsWindowCarriersButNeverHittableOutsideClip, the reported-viewport neighbour of the new fold policy.ActionabilityPolicyTests, dropping the TypeScript one failsrect.test.tsandtree.test.ts, and afalse/falserow fails both table readers.open com.apple.Preferences, this head against anfa1082dd14runner on separate derived paths:snapshot --jsonandsnapshot -i --jsonreturn the same ordered payload — 58 nodes (52true, 6false, none absent) and 18 (16, 2, none absent). OnlysnapshotDiagnosticstimings andrefsGenerationdiffer.ios-lifecycle.test.tspasses at this head and at cleanfa1082dd14; the earlier failure did not reproduce.Not checked live: a
.missingviewport cannot be forced on a device, so the absent-bit and fold paths are unit-covered only.