Skip to content

refactor(capture-kit): resolve recording helper scripts from swift-cache - #2743

Merged
thymikee merged 1 commit into
mainfrom
refactor/capture-kit-shared-capture-primitives
Sep 22, 2026
Merged

thymikee merged 1 commit into
mainfrom
refactor/capture-kit-shared-capture-primitives

Conversation

@thymikee

@thymikee thymikee commented Sep 21, 2026 •

Copy link
Copy Markdown
Member

Why

Two recording features now need the same answer: where is apple/runner/AgentDeviceRunner/RecordingScripts/<script> from a checkout, from a built package, or from a working directory? recording/overlay.ts owned that answer privately.

What

  • buildRecordingScriptPathCandidates() and resolveRecordingScriptPath() move to recording/swift-cache.ts, next to the compile seam that already consumes a resolved script.
  • The caller names the project root it trusts, so the lookup adds no host-metadata import to whatever closure imports swift-cache.
  • overlay.ts keeps its memoised paths and its host-kit/version import; behaviour is unchanged, including the searched order and the searchedPaths on the refusal.
  • The packaged-dist candidate test follows the function to swift-cache.test.ts.

First layer of the recording contact sheet stack: the decoder in the next layer compiles a second helper through this seam.

@thymikee
thymikee added this pull request to stack #2748 September 21, 2026 21:00
@thymikee thymikee changed the title refactor/capture kit shared capture primitives refactor(capture-kit): share glyph drawing and recording script lookup Sep 21, 2026
@github-actions

github-actions Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.74 MB 4.74 MB -6 B
Package (unpacked) 4.74 MB 4.74 MB -6 B
Package (download) 1.42 MB 1.42 MB -11 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 29.8 ms 29.0 ms -0.9 ms
CLI --help 85.2 ms 83.2 ms -2.0 ms

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed 6c1c562. Two things need a change before this layer is ready.

screenshot-overlay-draw.ts now imports setPngPixel for fillRect (L79), but drawHorizontalLine and drawVerticalLine still call a local setPixel with the same code (L108-L120). The goal of this PR is one shared primitive, and two copies can drift on the next bounds fix. Can both functions call setPngPixel, and setPixel be deleted?

Moving resolveRecordingScriptPath into swift-cache.ts (L3) adds static imports of fileURLToPath and findProjectRoot there. video.ts and screenshot-overlay.ts import swift-cache.ts, so their eager closures now pull in host-kit/version, although video.ts never calls the new function. That is why the eager-closure budget check in Coverage fails (video.ts 27 modules vs 23, screenshot-overlay.ts 33 vs 32). Could the findProjectRoot wrapper live in a module that video.ts does not import, with only the pure buildRecordingScriptPathCandidates in swift-cache.ts?

Not blocking: png-pixels.fixtures.ts could call setPngPixel instead of inlining the RGBA write, the new png-resize.ts export has no consumer yet, and the scale loops in drawPngGlyphText are over the complexity threshold.

CI: Coverage and Compatibility & Provenance fail on code this PR adds or moves, so they are related. iOS Smoke fails at "automation-longpress did not become visible after scrolling"; main also fails the live iOS fixture job and this diff is not on that route, so it looks unrelated. No conflicts.

@thymikee
thymikee force-pushed the refactor/capture-kit-shared-capture-primitives branch from 6c1c562 to 73fec06 Compare September 22, 2026 06:44
@thymikee
thymikee removed this pull request from stack #2748 September 22, 2026 06:45
@thymikee
thymikee added this pull request to stack #2753 September 22, 2026 06:45
@thymikee thymikee changed the title refactor(capture-kit): share glyph drawing and recording script lookup refactor(capture-kit): resolve recording helper scripts from swift-cache Sep 22, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Both fixed, and the shape of this layer changed because of the second point.

One pixel primitive. setPixel is gone; every painter in the module — border lines, badge fill, glyph blocks — writes through setPngPixel. That work now lives in #2746, which is where the primitive gains its second consumer.

The eager-closure growth. You were right that findProjectRoot in swift-cache.ts was the cause. Rather than split the wrapper into a third module, resolveRecordingScriptPath(scriptName, projectRoot, …) now takes the root from its caller: overlay.ts already imported findProjectRoot at the merge base, so its closure is byte-identical, and video.ts never sees it. Eager-closure is green locally (video.ts back to its merge-base count, screenshot-overlay.ts unchanged).

The glyph-table extraction is withdrawn. screenshot-overlay.ts's eager-closure entry has a no-growth budget, and a new module in its closure failed it before any second consumer existed — the same objection fallow raised as an unused export. Glyphs stay where they are and #2746 widens them in place.

Non-blocking items taken: the png-resize.ts export moves to #2746 where the renderer calls it; drawPngGlyphText's scale loops move into #2746, where the per-glyph block is a drawGlyph helper and health is clean; and the pixel fixtures now write through setPngPixel (#2746), since the primitive only exists from that layer up.

CI: Coverage and Compatibility & Provenance both came from this diff and are fixed here. iOS Smoke also fails on origin/main at the same live-fixture step, so it is not this route.

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed 73fec06. Both earlier findings are resolved: this PR no longer touches screenshot-overlay-draw.ts, and resolveRecordingScriptPath takes the project root from its caller, so swift-cache.ts adds no host-metadata import and the video.ts and screenshot-overlay.ts eager closures do not grow. The moved lookup keeps its candidate order, and its test moved with it. All checks pass and there are no conflicts.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 22, 2026
@thymikee
thymikee force-pushed the refactor/capture-kit-shared-capture-primitives branch from 73fec06 to 6c8934a Compare September 22, 2026 07:47
@thymikee
thymikee merged commit d6ba105 into main Sep 22, 2026
18 checks passed
@thymikee
thymikee deleted the refactor/capture-kit-shared-capture-primitives branch September 22, 2026 11:08
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-22 11:08 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