feat(screenshot): add --crop-on to crop captures to a selector frame - #2276
Conversation
… scenario exemption
|
Size Report
npm unpacked components
Startup median (7 runs, lower is better):
Top changed chunks:
Top changed packed files
|
|
BLOCKED at exact head
No separate runtime correctness finding: the admitted screenshot/snapshot route, typed failure handling, CLI/Node/script surfaces, and live iOS/Android pixel-identity evidence are otherwise sound, and every other exact-head check is green. |
…ant coverage Address review comments at 570da2c: - Split the 328-line screenshot-crop.ts leaf: the target acceptance matrix, classifier, and pre-device argument policy move to screenshot-crop-target.ts, so both implementation modules meet the 300-line target. - Reuse kernel isPositiveFiniteRect/rectArea in the rect-projection module instead of redefining them locally. - Drop the crop-on CLI forwarding case (redundant with screenshot-options flag-mapping coverage + the generic dispatcher) and the transport-based warnings case, replacing the latter with a focused screenshot-result unit test. This also returns the two legacy aggregate test files to their merge-base length for the test-file size ratchet.
|
Both items addressed at new head 1. Test-file size ratchet — the two legacy aggregate files are back to their merge-base length, with the new cases relocated to the owning topology:
2. Readiness size — net production trimmed from ≈740 to +671 (39 files, +1836 −147). Concretely:
Fallow complexity on the split |
…sifier under the complexity budget classifyAppleCropTarget inlined the macOS surface decision, pushing its cyclomatic complexity to the fallow threshold. Move it back out to a small helper so the target classifier stays within budget.
…sh png-crop - Hoist isMeaningfulSignal into @agent-device/contracts/snapshot (next to normalizeType/isMeaningfulLabel) so the ref overlay and the crop rect-projection share one copy instead of each carrying an identical private predicate. Behavior is unchanged. - png-crop: isCropBox was a no-op 'box is Rect' predicate (input already Rect) — make it a plain boolean, and tighten the doc to the contract.
thymikee
added
the
ready-for-human
|
Re-review of Both prior blockers are resolved. The legacy aggregate tests are back at merge-base size; warning normalization now has a focused The production route remains coherent and behavior-preserving: policy before device work → admitted screenshot+snapshot binding → platform PNG write → request-scoped full-tree snapshot → selector resolution/projection/intersection → worker crop → scale/result metadata; it does not mutate the session snapshot. The pre-refactor live iOS/Android pixel-identity evidence remains applicable because the review commits only move/simplify ownership and exact-head geometry/runtime tests pass. Size scrutiny is satisfied: +673 net production/+915 tests is itemized, smaller alternatives are addressed, and the refactor removed roughly 70 production lines. CI size is +8.7 kB unpacked/+2.6 kB gzip for the new functionality. All exact-head checks are green except iOS Smoke, which is still in progress; per review policy this clean head can be marked |
…rojection seams - ScreenshotCropOutcome.cropped was a constant true that no caller read; the crop either returns (success) or throws, so the outcome reduces to the partialIntersection observation. - resolveScreenshotRectSpace and resolveSnapshotBounds were the only projection exports without coverage: pin the accepted-backend map, the unaccepted-backend typed refusal, and the viewport-root / union / empty bounds branches.
1 participant
Summary
Add
screenshot --crop-on <selector>: capture the screen, then crop the saved PNG to the frame the selector resolves to on the same screen. Runs daemon-side after the platform write and before--scale, against a fresh full-tree snapshot (nosetSessionSnapshot). Accepted on iOS simulators and Android emulators; every other target is refused before device work. Cannot combine with--overlay-refsor--fullscreen.Success carries post-crop
width/heightand awarningsentry when the frame is clipped to the image. Failures use stabledetails.reasoncodes:CROP_TARGET_NOT_FOUND(+findhint),CROP_TARGET_AMBIGUOUS(+candidates),CROP_SELECTOR_INVALID,CROP_FRAME_MISMATCH,CROP_TARGET_NOT_ACCEPTED.Validation
Head
62f39143ce(rebased onto33084c7748; the review commits7edcd2adec→c5be390a85→643c9b8a80→62f39143ceare behavior-preserving —pnpm check:affected --rungreen, same seam/geometry tests):pnpm check:affected --run— all runnable checks passed.Live pixel-identity (crop region vs independent raw capture, mean-abs-diff, collected at the pre-refactor head
570da2c417; the refactor is behavior-preserving):simctl0.0000 at both densities; one-row-shifted control 8.44.android-helper@ package version): 436×71 @1:1; vs rawscreencap0.0000; control 42.83.NOT_FOUND,AMBIGUOUS(matches:11),SELECTOR_INVALID,FRAME_MISMATCH.EMPTY_INTERSECTIONis unit-tested (off-screen rows are absent from the live tree). macOS cell stays rejected (terminal lacks Screen Recording).Size review
41 files, +1899 −155: net +670 production / +971 tests (was ≈740 net / 2043 gross at the pre-review head; the review pass trimmed ~70 production lines by splitting the crop module, reusing kernel rect helpers, hoisting the duplicated meaningful-signal predicate into contracts, and dropping two redundant test cases).
Retained production growth, itemized:
screenshot-crop.ts(+144): the crop leaf — capture, sparse/empty refusal, selector resolution with typed ambiguity/missing reasons, project→intersect→crop, and the single warning-composition owner.screenshot-crop-target.ts(+148): the target acceptance matrix (one cell per capture frame), the device→cell classifier, and the pre-device argument policy. Split from the leaf so both modules meet the 300-line target (the review's size item).capture-kit/snapshot-rect-projection.ts(+138): space decision (device-pixels vs viewport-points), no-clamp projection, crop intersection, and the shared viewport-bounds resolver consumed by both the overlay and the crop. Reuses kernelisPositiveFiniteRect/rectAreaand the sharedisMeaningfulSignal(no local redefinitions).capture-kit/png-crop.ts(+54): in-place PNG region crop on the worker.screenshot-runtime.ts(+111 −15): the integration delta on the existing module — read flag, answer policy pre-device, admit the plan, run crop after the write/before scale, append the warning.contracts/screenshot.ts(+66 −2): the machine-readable flag + 10-reason typed taxonomy, flag keys,--crop-ondefinition, options↔flags projection, script string-flag reader.client/screenshot-result.ts(+55 −21): default-level normalization (incl. response-level warnings) + field picker, shared by client and CLI.screenshot-runtime-binding.ts(+26 −10, plan admits the crop's snapshot op),platform-runtime-operations.ts(+18 −7),contracts/snapshot-text.ts(+8, the hoistedisMeaningfulSignalshared by overlay + crop),selector-pipeline-policy.ts(+8),capture-kit/package.json(+8, subpaths),screenshot-overlay.ts(+7 −74, refactored to consume the shared projection and the shared predicate — net −67),cli/commands/screenshot.ts(+6),resolution-policy.ts(+5),commands/capture/screenshot.ts(+4),script-utils.ts(+4),agent-device-client.ts(+2 −12, inline reconstruction moved into the shared normalizer), contract facades (+2 each),response-views.ts(+1),snapshot-types.ts(+1).Why a materially smaller design was rejected:
setSessionSnapshot) is what keeps the authorized-ref frame a following interaction resolves untouched; reusing the session snapshot would corrupt that invariant.Tests (+915): geometry fixtures, the matrix one-to-one classifier/policy, the crop orchestration seam across 8 outcome paths, runtime ordering/warning wiring, the contract flag round-trip, CLI warning rendering, and the focused
screenshot-resultnormalization. Two redundant cases were dropped (the crop-on CLI forwarding case — covered by the flag-mapping contract tests plus the generic dispatcher — and the transport-based warnings case, now a focused unit test).