fix(ios): carry the snapshot viewport across the runner boundary as a declared fact - #2908
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at e222012. The runner now carries the snapshot viewport across the boundary as a declared fact, which is what #2891 asked for, and the approach is sound. Does the design go far enough, though? The smaller fix looks like it's on the output side: making The Smoke Tests failure looks unrelated to this change: the failing occupancy test goes through I could not reproduce the author's live simulator A/B (same 22-node snapshot on main and head) or the listed mutations myself, since this pass had no device. The one live route I could reach only covers the reported-viewport tree tier; the query-sweep and private-AX tiers, where a Not blocking: the ADR 0004 line at docs/adr/0004-ios-snapshot-backend-strategy.md#L313 should name Before merge, please settle how an unknown viewport should travel on the runner wire: an absent |
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Reviewed at e222012. The declared viewport fact is the right direction. The remaining issue is that the migration stops just short of removing sentinel-box reachability in two adjacent places: the normalization pass still uses .null as the stand-in for no box, and derived viewport still does not encode the rotation rule it claims to have.
| // No viewport box means no app frame to be quarter-turned relative to either. `.null` is the box | ||
| // `SnapshotGeometry.isPositiveFinite` refuses, so this pass turns nothing — the outcome | ||
| // `CGRect.infinite` produced before the fact carried the absence. | ||
| let appFrame = viewport.rect ?? .null |
There was a problem hiding this comment.
The viewport fact is decoded straight back into a sentinel rectangle here. That works because isPositiveFinite happens to refuse .null, not because the type says so. The public space(appFrame:) and .deviceNative(appFrame:) APIs still accept any CGRect, including .infinite, and the updated coordinate-space test also switched to passing .null through that public surface, so the sentinel spelling moved rather than going away. Could space/isQuarterTurned take the viewport fact or a CGRect? so no caller has to name a box for no box?
| /// A box the capture inferred for itself out of its own root element instead of a screen read. It | ||
| /// clips and contains like a reported box, and it never anchors a rotation: the tier that produces | ||
| /// it reports no interface orientation beside it (#2612). | ||
| case derived(Box) |
There was a problem hiding this comment.
The derived case documents that it never anchors a rotation, but that invariant lives only in prose. interfaceOrientation is a separate SnapshotAcquisition field, and SnapshotGeometrySpace.space chooses .deviceNative from orientation plus box geometry without seeing whether the viewport is reported or derived, so derived + landscape is still constructible. Folding the orientation into the viewport fact, or otherwise making derived unable to produce a rotation-capable space, would remove the same unpaired state this PR is otherwise removing.
… declared fact
The runner spelled "viewport unknown" as CGRect.infinite, which
SnapshotGeometry.isGeometricallyActionable read as "everything is
actionable" before computing any center, while the host-AX path models
the same state as IosViewportEvidence and declines to publish the bit.
One state, two encodings, opposite directions, across a language
boundary.
SnapshotViewport now carries the three cases the host already uses --
reported, derived, and missing { reason } -- and refuses an unusable box
at declaration, so no caller has to re-check what it was handed. The
unknown-viewport policy is stated once, at the Swift decision site, and
fails CLOSED: no box means no supportable claim about where a tap lands.
Clipping stays a separate question from containment; a capture with no
box has no clip and no root clip for the cumulative invariant to
violate, which is an absent answer rather than an unbounded one.
The fixed 320x240 fold fixture cannot reach an edge-of-viewport center, a node rect neither language's null/empty check refuses, or an unknown viewport, so the predicate's two implementations disagreed on two of them: CGRect.infinite is built of finite Doubles and its center is (0,0), and a negative width passes an empty check while its center lands inside -- both actionable in Swift and not actionable in TypeScript. The private AX bridge's JSON frame parser can hand over either. contracts/fixtures/snapshot-actionability-policy.json is replayed by ActionabilityPolicyTests (Swift) and by scripts/ios-snapshot-differential .test.ts (TypeScript), which also asserts the host's own instrument: resolveViewportEvidence refuses a regular presentation without a positive finite viewport, which is why the TypeScript predicate has no unknown viewport case. Rows come in pairs sharing one center so a `false` that the guard, rather than containment, decides cannot go quiet.
…dicate `SnapshotGeometrySpace.isPlottable` and `SnapshotGeometry.isPositiveFinite` were the same five comparisons, written twice in one package, which is how #2891 started: one question with two encodings that can drift apart. `.null`, an inverted box, and an infinite box are refused identically by both, so the coordinate-space table needs no new rows.
`SnapshotViewport.reported(box:)` normalised an unusable box to `.missing(reason: .invalid)`, and the commit said no caller had to re-check what it was handed — while `.reported(rect:)` stayed a public case that accepted `CGRect.infinite` on the way past that claim. The associated value is now a box with an internal initialiser, so outside this package a viewport fact is only reachable through the two factories, and the case labels cannot be named around them. MSG2
The table replayed `{"infinite": true}` as `CGRect.infinite` in Swift and as a
rect with infinite components in TypeScript, so its infinity row never put the
same number in front of both sides. `CGRect.infinite` is made of finite Doubles:
TypeScript would call those components actionable, and only the platform that can
name the value can refuse it. The row is now Swift-only and carries `asymmetry`
naming that, every row declares both `swift` and `typescript`, and a shared
non-finite row replaces it with numbers both sides actually see.
Also states three claims the code had not been checked against: a published
`hittable: false` is retained rather than filtered out, the runner's viewport fact
decides only runner bits because the host re-derives its own from the capture's
root node, and `viewportFromRoot` — not only `resolveViewportEvidence` — is what
makes the TypeScript `viewport` argument total.
…ed viewport anchors a rotation A capture with no viewport box can still decide a disabled or degenerate node, but not containment, so the runner now omits hittable instead of declaring false, matching the host bridge's undefined bit. The interface orientation moves into SnapshotViewport.reported, so a derived or missing viewport cannot anchor a rotation, and SnapshotGeometrySpace takes the viewport fact rather than a sentinel CGRect.
…l lane contracts/fixtures/snapshot-actionability-policy.json is read by both sides of scripts/ios-snapshot-differential, and packages/kernel/src/rect.ts holds the TypeScript predicate that table replays, so a change to either must select that lane rather than rely on the generic golden-table rule reaching only the unit and Swift builds.
e222012 to
771322a
Compare
|
Addressed the 14:54Z comment and the 15:22Z review. The branch is rebased onto Blocking
Non-blocking (all taken)
Smoke Tests: Validation at
|
|
Reviewed at 771322a. The PR is ready to merge as far as the code goes. The .missing-viewport nil-source gap from the earlier pass (e222012) is fixed: SnapshotVisibilityFold.swift now falls back to the geometric predicate when the source bit is undecided, so a clipped-to-empty frame still publishes Not blocking: could you take a look, whenever convenient, at whether the fold should key off No device or test run was performed in this pass, so the author's 86 RunnerTests, the live Preferences snapshot at 771322a, and the differential-lane pass are unverified here. A Smoke Tests and Coverage haven't finished on 771322a, so nothing is failing yet. The diff touches the iOS runner snapshot_viewport hop in RunnerTests+SnapshotAcquisition.swift, which reads orientation inside safeSnapshotViewport — the same hop where the earlier MAIN_THREAD_WORK_ABANDONED failure occurred, so if Smoke Tests fail there again it needs its own look rather than being waved off as unrelated to this change. |
thymikee
added
the
ready-for-human
1 participant
Closes #2891. Related: #2881 (same
contracts/fixtures/mechanism, different payload).Summary
The runner spelled "viewport unknown" as
CGRect.infinite.SnapshotGeometry.isGeometricallyActionableread that sentinel as "everything is actionable" before it computed any center, while the host's
IosViewportEvidencedoes not publish the bit at all. The viewport now crosses the boundary as aSnapshotViewportfact with three cases:reported(box, interfaceOrientation)derived(box)missing(reason)Only factories can construct a box-bearing case. Only
reportedcarries an orientation, so aderivedor
missingviewport cannot anchor a rotation.SnapshotGeometrySpacetakes the fact, not a sentinelCGRect.With no box, a node whose actionability depends on containment has no
hittableon the wire, which ishow the host bridge leaves it. A disabled node or a degenerate frame is still a declared
false.Clipping stays separate from containment.
SnapshotPresentationInvariantreads "no box" as no root clipto violate, not as an unbounded clip.
contracts/fixtures/snapshot-actionability-policy.jsonpins the shared predicate for shapes that thefixed 320x240 fold fixture cannot reach. Every row declares both languages. A Swift-only row names its
asymmetry.
Validation
Tested SHA
771322a9e9, rebased on86d719dc33.pnpm test:ios-snapshot-differential: Swift 16 tests, node 5 tests, all pass.pnpm check:affected --run: exit 0.pnpm check:xctest-selection: pass.RunnerTestson a new iPhone 17 Pro / iOS 26.2 simulator:Executed 86 tests, with 0 failures.packages/platform-apple/src/runner/__tests__/snapshot-presentation.test.tssends a payload withhittableabsent throughpresentAppleRunnerSnapshot, and the bit stays absent.open com.apple.Preferences, thensnapshot --json. The tree backend was healthy: 73 nodes, 65hittable: true, 8false, none absent.e222012106: the author ran an A/B on thernav-reprosimulator against anorigin/mainrunner. Both runners returned the same 22-node snapshot.Not checked live: the CLI has no rotate command, and a
.missingviewport cannot be forced on a device. Unit tests cover both paths. A box shaped likeCGRect.infinitethat reaches the TypeScript predicate is still actionable there. The refusal belongs to the producer that parses an Apple frame.