refactor(ios): move the coordinate-space rule into the presentation package

[thymikee]

Summary

Moves the rotation table and the coordinate-space rule (RunnerInterfaceOrientation, CoordinateSpaceRotation, SnapshotGeometrySpace) out of the XCTest runner bundle into AgentDeviceSnapshotPresentation as SnapshotCoordinateSpace.swift, public, written in that package's comment-free style. isSurfaceHost takes isWindow/parentIsWindow booleans instead of an XCUIElement.ElementType so the file has no XCTest dependency; the two runner walkers pass isWindowElement(...). The six rotation/space tests and the golden-table replay move into the package suite (CoordinateSpaceTests), so swift test proves the table on any macOS host without a simulator.

No behaviour change. ADR 0004 gains a pointer to the #2659 spike verdict (the bridge cannot read interface orientation; measured) and names the package as the rule's home.

Package size delta vs main measured with npm pack --dry-run --ignore-scripts: +25 bytes.

Two earlier pieces (a failure-scope field and an unplaced-geometry disclosure) were tried and cut; the reasoning is in the review thread.

Follow-ups filed from the same review: #2659 (spike, closed), #2660, #2661, #2662.

Validation

Head 321aef2. pnpm typecheck, pnpm format:check clean; touched TS test files are main's (74 pass). swift test --package-path apple/snapshot-presentation 22/22. Runner built with AGENT_DEVICE_XCUITEST_INCLUDE_UNIT_TESTS=1 (arm64); testPrivateAXAcquisitionPublishesATurnedSurfaceHostInAppOrientationSpace, testScrollViewportDispatchKeepsTheUnclippedFrameAsItsCoordinateRotationBasis, testStampedPayloadTruncationTracksCompletenessNotRecoveryProvenance pass on a throwaway iPhone 17 Pro (iOS 26.2). Live runner-route captures (Safari, keyboard up): landscape band x 75 y 238 w 724 h 162, key q at x 77 y 243 w 72 h 35, 31 keys; portrait band y 583 w 402 h 233, 33 keys — both in app space, no coordinate-space warning.

[github-actions]

Size Report

Metric	Base	Current	Diff
Installed (including dependencies)	4.60 MB	4.60 MB	-27 B
Package (unpacked)	4.60 MB	4.60 MB	-27 B
Package (download)	1.37 MB	1.37 MB	+180 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	26.0 ms	25.9 ms	-0.1 ms
CLI --help	72.5 ms	72.8 ms	+0.3 ms

[thymikee]

Reviewed 6cfe6a5. The code looks right, but one gap stops me from calling it ready: nothing live shows that the moved geometry code still works on a device.

unplacedSurfaceHostCount now runs on every runner capture, on every tier, and both walker call sites now go through the moved package table (isWindowElement -> isSurfaceHost(isWindow:)), see https://github.com/callstack/agent-device/blob/6cfe6a5/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SnapshotCapturePlan.swift#L512-L514. The unit tests use hand-built trees only. If the count or the new wiring is wrong on a real tree, every iOS runner snapshot could carry a false "window(s) on this screen" warning, or the landscape keyboard band could fall back to the #2612 behavior. The PR body says no code path of the landscape-keyboard scenario changed, but both walker call sites did. Please run a live snapshot through the runner route on 6cfe6a5: (a) a landscape screen with the system keyboard up, with the band published in app space (near y≈198 on iPhone 17 Pro) and no unresolvedCoordinateSpaceWindows in the --debug snapshot quality; (b) the same screen in portrait, with no warning. Attach the snapshot output and the snapshotQuality line.

Non-blocking, take or leave: preparation.ts:123 has no test for its scope: 'capture' (switch it back to snapshotSourceError and all tests still pass, because the SnapshotSourceError constructor defaults to 'generation'), and unplacedSurfaceHostCount counts a window and its direct child twice when both report the turned box, so "windows" in the field name can overcount.

The package grows 8.6 kB unpacked, over the 3 kB mark. Most of that looks like the comments of the moved Swift file: runner sources go through comment stripping in scripts/package-apple-runner-source.mjs, but snapshot-presentation sources are copied as they are. Would it be enough to strip comments for the presentation package too, so the move is size-neutral? And would removing the SnapshotSourceError scope default, so every constructor names its side of the rule, be a smaller way to get the guarantee the PR body describes? If you rejected either, what would have to change first?

One question I could not settle from the code: does the counter's parentIndex lookup (and its TypeScript twin) hold on every tier, meaning node indices always equal array positions?

One Smoke Tests job was still running at review time. It is not independent of this change, because iOS smoke builds the runner that now compiles SnapshotCoordinateSpace.swift from the package, so a red result there needs a look. The next step before merge is the live runner-route landscape and portrait snapshots above.

[thymikee]

Live validation done on iPhone 17 Pro (iOS 26.2 Simulator), runner route (snapshotQuality.backend: tree; the bridge was circuit-disabled by Safari's web view, so every capture below was XCTest-served — the route you asked about). Head 5f93e58.

(a) Landscape, keyboard up. Safari address field focused after orientation landscape-left, app frame 874×402:

• keyboard plane Keyboard at x 75 y 238 w 724 h 162 — a wide bottom band in app space, not a tall left-edge strip (unnormalized would be ~w 162 × h 724 at x 0). 31 real keys, e.g. q at x 77 y 243 w 72 h 35. Same shape as the fix(ios): publish captured keyboard geometry in the app's orientation space #2653 fixture band (75,198,724,202) / key q (77,203,72,45); Safari's keyboard sits a little lower and shorter than the test-app form, the normalization is identical.
• snapshotQuality: {"state":"healthy","backend":"tree", …} — no unresolvedCoordinateSpaceWindows. Orientation was readable, the walk turned everything back, nothing disclosed.

(b) Portrait, keyboard up. App frame 402×874: keyboard band at y 583 w 402 h 233, 33 key nodes, no unresolvedCoordinateSpaceWindows, and the only warning is the unrelated circuit-disabled (bridge off, same as landscape). So no false "window(s) on this screen" warning and no #2612 fallback in either orientation.

So the moved geometry code and both walker call sites are correct on a real tree.

---

Took the two non-blocking points, and the answers to the two questions:

Scope default. Agreed — removed it. SnapshotSourceError now takes scope as a required argument (moved next to code, since a required param can't follow the optional message/cause); snapshotSourceError passes 'generation', snapshotSourceCaptureError passes 'capture'. A failure can't be built without naming its side of the rule now. errors.test.ts pins both factories. (preparation.ts's 'capture' was already covered behaviourally — the "bridge still being prepared" route test asserts acquire is called twice, which fails if preparing opened the circuit — but the direct unit test makes it explicit.)

Overcount. Fixed. unplacedSurfaceHostCount now folds a turned surface's window and the surface directly under it into one host, so the count is a surface count even in the pathological both-turned case; the real keyboard (window reports the app box, child reports the turned box) was already 1. Package test covers the both-turned pair.

parentIndex == array position? Yes, on every tier, by construction. Each builder appends nodes in traversal order with index: nodes.count and sets a child's parentIndex to the parent's index, so nodes[parentIndex] is that parent (recursive RunnerTests+Snapshot.swift:227/261, raw :418/430, query-sweep :521, private-AX RunnerTests+PrivateAXPresentation.swift). unplacedSurfaceHostCount runs on that assembled array and only reads nodes[parentIndex], and I added a comment stating the invariant it relies on.

Package size / comment stripping. The presentation package is already comment-stripped in dist: scripts/package-apple-runner-source.mjs runs snapshot-presentation sources through the same processDirectory → stripSwiftComments path as apple/runner (the packaging guard reports "26 files contain 34 stripped blocks", both dirs). So the 8.6 kB is not comments — it's the disclosure code itself: the kernel unresolvedCoordinateSpaceWindows field, the capture-kit reader and warning, SnapshotGeometrySpace.unplacedSurfaceHostCount, and the TS bundle deltas, plus the net Swift code from unplacedSurfaceHostCount/isWindowType. Stripping won't recover it; the only lever that would is dropping the disclosure, which is the thing you asked for in the first review. I'd keep it.

Smoke Tests / iOS runner build. Green on the pushed head, and I rebuilt the runner locally with AGENT_DEVICE_XCUITEST_INCLUDE_UNIT_TESTS=1 (SnapshotCoordinateSpace.swift compiles from the package) — testStampedPayloadDisclosesUnplacedSurfaceHosts and testPrivateAXAcquisitionPublishesATurnedSurfaceHostInAppOrientationSpace pass. swift test --package-path apple/snapshot-presentation 22/22.

One heads-up unrelated to the review: #2669 and #2670 (the #2661 and #2660 workers) overlap this PR. I left merge-order notes on both — this lands first, they rebase to the behaviour layer. Details on those PRs.

[thymikee]

Reviewed 5f93e58. The code looks right, and nothing blocks it.

The runner-route landscape and portrait snapshots you posted for 5f93e58 close the gap from my last review: the keyboard band is in app space, and neither orientation shows unresolvedCoordinateSpaceWindows. Those runs cover the count=0 path. The new both-turned fold in unplacedSurfaceHostCount is covered by the package test, which fails on the old loop. Your package-size answer holds too: scripts/package-apple-runner-source.mjs already strips comments from the presentation sources, so the growth is the disclosure code itself.

All 18 checks pass on 5f93e58, including the iOS smoke job that builds the runner with the moved SnapshotCoordinateSpace.swift.

Non-blocking, fine for a follow-up: the TypeScript twin countUnresolvedCoordinateSpaceWindows does not have the new fold, so a turned window with a turned child reports windows: 2 where Swift reports 1 (the refusal itself agrees), and no test pins scope: 'capture' at preparation.ts:123; asserting failureScope on the failure that preparation.test.ts:55 already gets back would do it.

[thymikee]

Correction, and a cut. Head 321aef2.

I was wrong about the package size, and your hypothesis was right. I claimed the presentation package was comment-stripped in dist. It is not: packageSnapshotPresentationSource calls processDirectory with validateSwift: false, which copies .swift files verbatim (dist equals source byte for byte). It was harmless until now because every existing file in that package is written comment-free — running stripSwiftComments over all ten Sources/ files saves 0 bytes on nine of them and 6,829 bytes on mine. I had moved a prose-heavy file into a comment-free package.

Measured with npm pack --dry-run --ignore-scripts on both refs, the earlier +9,330 bytes was: 6,829 comments in the moved file, ~1,550 disclosure plumbing (quality-warnings.js +747, RunnerTests+SnapshotCapturePlan.swift +425, sdk-contracts.d.ts +352), ~180 scope field, 67 the two call sites. The move itself was near zero.

What this head keeps: only the move. The file is rewritten in the package's style (no prose; rationale stays in ADR 0004 and the fixture description), same code, public, tests in the package suite. Delta against main is now +25 bytes (-5408 old runner copy, +5314 package copy, the rest is bundler chunk reshuffle).

Cut, and why:

• Scope field. A required constructor argument that guards what the route tests already guard, for two fixed codes. Same guarantee, more surface. Back to opensGenerationCircuit.
• Disclosure. I could not show it fires on its one target path. That path (private-AX tier, penalized XCTest channel) anchors on the bridge's root frame with orientation unknown; the count guards isPlottable(viewport), and that frame is .infinite in the common case, so the count is zero by construction there — and nobody has measured which space that root frame reports in when it is finite. Never observed live. Noted on refactor(ios-runner): acquire first, present after — one geometry normalization pass instead of per-walker threading #2661, where the one-pass normalization makes it a measured one-liner if it is ever wanted.

Validation on 321aef2: typecheck, format, lint clean; the reverted TS tests are main's (74 pass); swift test 22/22 with the trimmed CoordinateSpaceTests; runner rebuilt with unit tests and testPrivateAXAcquisitionPublishesATurnedSurfaceHostInAppOrientationSpace, testScrollViewportDispatchKeepsTheUnclippedFrameAsItsCoordinateRotationBasis, testStampedPayloadTruncationTracksCompletenessNotRecoveryProvenance pass on a throwaway iPhone 17 Pro (iOS 26.2). The live landscape/portrait captures from the previous comment still stand: both walker call sites are unchanged by this cut.

[thymikee]

Reviewed 321aef2. The code looks clean. Since 5f93e58 the change only drops the failure-scope field and the unplaced-geometry disclosure, and the TS side of those files is back to main. The moved isSurfaceHost gets the same answer as before, because both walkers pass isWindowElement(...) for the old element-type check. The golden table only changes its description path.

Your live landscape and portrait captures still cover this head, because the delta does not change the walker call sites or the rotation logic. I did not rerun swift test, the runner tests or the captures.

Smoke Tests is still running on 321aef2. It builds the runner, which now uses the moved file, so it needs to finish green before merge. There are no conflicts.

Not blocking: SnapshotCoordinateSpace.swift#L4-L6 still has a 3-line header comment, and the package ships its sources as they are, so you can delete it (ADR 0004 and the fixture already hold that rationale) or leave it.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-19 06:07 UTC