Skip to content

refactor(ios): extract snapshot presenter - #2206

Merged
thymikee merged 3 commits into
mainfrom
codex/2191/swift-presenter
Sep 1, 2026
Merged

refactor(ios): extract snapshot presenter#2206
thymikee merged 3 commits into
mainfrom
codex/2191/swift-presenter

Conversation

@thymikee

@thymikee thymikee commented Sep 1, 2026

Copy link
Copy Markdown
Member

Summary

Layer A of #2191 (“refactor(ios): build and prove the converged snapshot engine”), stacked on merged #2203 and current main (af2c65d).

  • Extracts the reusable pure Swift geometric presenter into apple/snapshot-presentation/; both the runner target and standalone conformance executable consume the same source.
  • Production callers use AgentDeviceSnapshotPresentation directly; only the narrow SnapshotPresentationResult -> SnapshotBackendCapture wire conversion remains in the runner.
  • Runner command routing, fallback selection, public CLI/RPC surface, producer selection, and wire behavior are unchanged.

Scope and design rationale: 38 touched files, +1,348/-1,012 vs current main. The change is move-dominated: shared package source, runner import/adapter/test rewiring, and packaging tests/harness relocation. This is the smallest boundary that lets production and conformance consume one presenter implementation; retaining it in the runner or duplicating it in the harness would leave two implementations. The distributed runner manifest filters development-only tests/conformance sources. Bundle evidence: JS unchanged, Apple runner source -37.8 kB, other package files +33.9 kB, net npm unpacked -3.9 kB / tarball -2.5 kB.

Validation

Exact reviewed head: 4ef845fe1fec3d0cd4ed9a17cfcad703a35bea45

  • CI=true pnpm install --frozen-lockfile
  • pnpm build
  • swift test --package-path apple/snapshot-presentation (1 test)
  • pnpm check:xctest-selection: 217 declared, 189 host-reached, 76 iOS-PR-selected, 215 nightly-reached, 0 lane gaps.
  • pnpm build:xcuitest:ios: TEST BUILD SUCCEEDED with Xcode 27 Beta.6 / iPhone 17 Pro iOS Simulator.
  • Exact-head iOS Smoke Tests passed targeted presenter/depth/fold/raw XCTest, CLI preflight, Settings replay, fixture-backed iOS Simulator E2E (15/15), gesture replay, and artifact upload.
  • Recorded fixture evidence includes smoke:automation-input, smoke:form-input, and smoke:regular-visible-depth-frontier; the latter proves regular depth 1 reparenting/presentation of visible-depth-projected-child while raw traversal remains bounded. smoke:capture-close passed.
  • Exact-head CI static/package/native checks are green: lint/format, typecheck/package, integration, coverage, Repo Guards, Compatibility & Provenance, Android, Linux, macOS, bundle size, test-app cache, and CodeQL.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.55 MB 2.55 MB 0 B
JS gzip 857.4 kB 857.4 kB 0 B
npm tarball 981.9 kB 979.4 kB -2.5 kB
npm unpacked 3.39 MB 3.38 MB -3.9 kB

npm unpacked components

Component Base Current Diff
JS / dist source 2.71 MB 2.71 MB 0 B
Apple runner source/project 581.2 kB 543.4 kB -37.8 kB
macOS helper source 54.8 kB 54.8 kB 0 B
Android helper artifacts 0 B 0 B 0 B
Other package files 45.7 kB 79.7 kB +33.9 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 30.7 ms 30.7 ms +0.0 ms
CLI --help 83.1 ms 78.4 ms -4.7 ms

Top changed chunks: no changes in the largest emitted chunks.

Top changed packed files

Packed file Base Current Diff
dist/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SnapshotPresentation.swift 14.1 kB 0 B -14.1 kB
dist/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SnapshotVisibilityFold.swift 11.7 kB 0 B -11.7 kB
dist/apple/snapshot-presentation/Sources/AgentDeviceSnapshotPresentation/SnapshotVisibilityFold.swift 0 B 7.6 kB +7.6 kB
dist/apple/snapshot-presentation/Sources/AgentDeviceSnapshotPresentation/SnapshotModels.swift 0 B 6.5 kB +6.5 kB
dist/apple/snapshot-presentation/Sources/AgentDeviceSnapshotPresentation/SnapshotPresentationProjection.swift 0 B 5.0 kB +5.0 kB
dist/apple/snapshot-presentation/Sources/AgentDeviceSnapshotPresentation/SnapshotPresentation.swift 0 B 4.7 kB +4.7 kB
dist/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SnapshotPresentationInvariant.swift 4.9 kB 558 B -4.3 kB
dist/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SnapshotPresentationModels.swift 3.3 kB 0 B -3.3 kB
dist/apple/snapshot-presentation/Sources/AgentDeviceSnapshotPresentation/SnapshotVisibilityFoldProjection.swift 0 B 3.1 kB +3.1 kB
dist/apple/snapshot-presentation/Sources/AgentDeviceSnapshotPresentation/SnapshotPresentationInvariant.swift 0 B 2.7 kB +2.7 kB

@thymikee

thymikee commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Blocked at exact head d6e61ba92060367361873e5d50febb42a5450036:

  1. The stack is stale. This commit is still parented to refactor(ios): define snapshot acquisition and presentation contracts #2203’s superseded d4623a1...; refactor(ios): define snapshot acquisition and presentation contracts #2203 is now green at bce60b5.... Restack onto the exact prerequisite head before revalidation. That should remove the inherited eager-closure Coverage failures, but it will not fix this PR’s own iOS failure.
  2. iOS Smoke fails at compile time (exit 65): RunnerTests+SelectorMatchPolicyTests.swift:117-118 accesses PresentedNode properties without importing AgentDeviceSnapshotPresentation. The runner-local aliases do not establish the module boundary for consumers. Fix the actual consumers/import ownership and prove the exact-head iOS lane; this is not an emulator flake.
  3. RunnerTests+SnapshotPresentation.swift recreates almost the entire old presenter as forwarding methods, while RunnerTests+SnapshotPresentationModels.swift preserves twelve old local names as typealiases. That is a compatibility shim around the new owner. Migrate callers to consume the shared presenter directly and keep only one distinctly named, narrow runner adapter at the real SnapshotPresentationResult -> SnapshotBackendCapture wire boundary. Add structural proof that the old local presenter/model facade cannot regrow.
  4. CI reports npm unpacked +4.5 kB (>3 kB scrutiny threshold), with Other package files +38.2 kB. package-apple-runner-source.mjs copies the entire Swift package with validation disabled, so checkout-only Sources/SnapshotPresentationConformance/main.swift and Tests/.../ConformanceTests.swift ship even though the runner needs only the library. Keep the standalone harness in the repository but remove development-only targets/sources from the distributed runner package (or identify a real runtime consumer), then itemize justified growth and why a smaller distribution was rejected.

The moved presentation behavior otherwise appears faithful and does not change producer selection/routing. Re-review after the ownership/package fixes, exact-base restack, focused presenter/XCTest proof, and exact-head CI/device evidence.

@thymikee
thymikee force-pushed the codex/2191/swift-presenter branch from d6e61ba to 0b0d5c0 Compare September 1, 2026 07:42
@thymikee

thymikee commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 0b0d5c0420b8a67e6f93a37354d56b40fc0fab39.

The earlier architecture/package blockers are resolved correctly: the stack is now based on current #2203 (bce60b56fb5c5c8d57247e0946fa1fd540e292a1); the runner-local presenter facade and model aliases are gone; the production route directly uses AgentDeviceSnapshotPresentation with only the distinctly named wire conversion left; anti-regrowth coverage rejects recreating the facade/aliases; and the distributed Swift manifest excludes development tests/conformance targets. Bundle evidence is now healthy at -3.9 kB unpacked / -2.5 kB tarball.

Still BLOCKED on exact-head owner-action CI:

  1. iOS and macOS Smoke both fail Swift compilation at RunnerTests+SnapshotPresentationTests.swift:188: presentRegular(...).nodes is non-optional [PresentedNode], but the assertion still uses presented?.compactMap. Remove the stale optional chain and rerun both native lanes. No device behavior evidence ran because compilation stopped first.
  2. Compatibility & Provenance fails Fallow at scripts/package-apple-runner-source.mjs:62: packageSnapshotPresentationSource is 6 cyclomatic / 5 cognitive / CRAP 42. Extract the manifest/copy concern into a smaller owning helper; do not suppress or baseline the new complexity.

No additional architecture/code findings beyond those two CI blockers. Android/Linux, lint/typecheck/package, integration, coverage, repo guards, and bundle size pass. Please update the head and let the exact-head lanes rerun.

@thymikee

thymikee commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 09f247cc9305ae54f6dbf9a60b78961f8afee7e9.

The two prior owner-action blockers are fixed at root:

  • the stale optional chain in RunnerTests+SnapshotPresentationTests.swift is removed;
  • packageSnapshotPresentationSource now delegates output preparation, manifest validation, and manifest copying to focused helpers, with no Fallow suppression/baseline. Compatibility & Provenance is green.

The remediation delta is narrow, remains exactly stacked on current #2203 (bce60b56fb5c5c8d57247e0946fa1fd540e292a1), and does not reintroduce the runner-local presenter facade or model aliases. I found no new code/architecture findings. Exact-head Compatibility, Coverage, Repo Guards, Typecheck & Package, Integration, lint, bundle size, Analyze, and three Smoke lanes are green; iOS Smoke is still in progress.

Code review is clean, but this is not merge-ready yet. The PR body is still only gh-stack boilerplate. Please add the required ## Summary and ## Validation, including linked issue/slice context, touched-file count and move-dominated scope rationale, the exact reviewed head, completed native checks, and real iOS Simulator evidence for the changed device-facing snapshot route (or state the precise blocker/command and residual risk). Then let the remaining iOS Smoke lane finish on this exact head.

No ready-for-human label until that readiness evidence is recorded.

Base automatically changed from codex/refactor/ios-snapshot-contracts to main September 1, 2026 09:33
@thymikee
thymikee force-pushed the codex/2191/swift-presenter branch 3 times, most recently from 09f247c to 6f58524 Compare September 1, 2026 10:20
@thymikee

thymikee commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

BLOCKED at exact head 6f585240e672c34aec1ad567fbe9236031e88434.

The prior facade/package/CI findings remain resolved: the runner uses the shared presenter directly, the narrow wire conversion remains, package tests/manifests keep development targets out of distribution, and the previous compile/Fallow fixes are intact. No new code finding.

Readiness evidence is still absent. The body remains gh-stack boilerplate and must record the #2191 slice context, move-dominated/touched-file rationale, exact-head validation, and real iOS Simulator evidence for the changed snapshot route (or the precise blocker and residual risk). Current exact-head CI is also incomplete, with lanes queued/in progress. Do not label or merge until that evidence is recorded and the exact-head required checks finish.

@thymikee
thymikee force-pushed the codex/2191/swift-presenter branch from 6f58524 to 4ef845f Compare September 1, 2026 11:10
@thymikee

thymikee commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 6f585240e672c34aec1ad567fbe9236031e88434: no new code or architecture findings. The earlier facade/package/compile/Fallow blockers remain resolved, and the body now records #2191 Layer A, the move-dominated ownership rationale, exact head, and native validation. All exact-head checks are green. The iOS lane built the shared presenter into the runner, booted a Simulator, passed the 15-test fixture-backed E2E, and completed gesture replay (with two timeout retries recorded before the passing attempt), so the changed device path has practical evidence. READY for human code review.

Minor body cleanup: Coverage and iOS Smoke are no longer in progress; both passed.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 1, 2026
@thymikee

thymikee commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed new exact head 4ef845fe1fec3d0cd4ed9a17cfcad703a35bea45: the branch was rebuilt on current main; the snapshot-presenter/runner/package-script delta is unchanged, and the prior clean code/architecture verdict still holds. No new finding.

This is READY for human code review, but not merge-ready. The body is stale at 6f585240…, replacement exact-head CI is queued/in progress, and successful Simulator evidence belongs to the prior head. Update the exact-head/body/check record and attach the replacement iOS Simulator outcome before claiming merge readiness. The existing ready-for-human label remains appropriate for the clean code.

@thymikee

thymikee commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 4ef845fe1fec3d0cd4ed9a17cfcad703a35bea45: replacement readiness evidence is complete. The body is current, explains the move-dominated single-owner design and package-size result, and records exact-head validation. All required checks are green; iOS Smoke built the shared presenter into the runner, booted a Simulator, ran targeted presenter regressions, passed 15/15 fixture E2E and gesture replay, and uploaded artifacts. No new finding. READY for human review and merge-ready.

@thymikee

thymikee commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Addressed in the PR body at exact head 4ef845fe1fec3d0cd4ed9a17cfcad703a35bea45: it now records the #2191 Slice A context, the 38-file move-dominated scope and smallest-boundary rationale, exact-head local validation, and the recorded iOS Simulator evidence for the changed snapshot route. The exact-head required checks are green. No readiness label was added.

@thymikee

thymikee commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Reconciled unchanged exact head 4ef845fe1fec3d0cd4ed9a17cfcad703a35bea45: code, stack, body, label, and all exact-head checks remain unchanged and clean, including iOS Simulator Smoke. The prior READY/merge-ready verdict stands.

@thymikee
thymikee merged commit 868f8f9 into main Sep 1, 2026
18 checks passed
@thymikee
thymikee deleted the codex/2191/swift-presenter branch September 1, 2026 13:59
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-01 13:59 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant