refactor(core): decompose Maestro screenshot relay + derive exact system-bar insets#2286
Open
Sriram567 wants to merge 13 commits into
Open
refactor(core): decompose Maestro screenshot relay + derive exact system-bar insets#2286Sriram567 wants to merge 13 commits into
Sriram567 wants to merge 13 commits into
Conversation
…son-upload.js Move handleComparisonUpload out of api.js into its own module, mirroring the handleSyncJob extraction pattern. Promote the shared encodeURLSearchParams helper to utils.js (alongside the other URL helpers) so both api.js and the new module can use it without a circular import. No behavior change.
…ro-screenshot.js Move the ~440-line route handler body, plus the parsePngDimensions and manualScreenshotWalk helpers, verbatim out of api.js into a dedicated maestro-screenshot.js module exporting handleMaestroScreenshot(req, res, percy). api.js now declares the route as a one-line delegate, mirroring the handleComparisonUpload/handleSyncJob pattern. All istanbul-ignore annotations travel verbatim with their statements. Move the file-level semgrep path-traversal suppression from api.js to maestro-screenshot.js (the new owner of the path.join sinks). No behavior change. api.js drops from 994 to 383 lines.
…creenshot-file.js Carve the file-location concern (platform glob pattern, manualScreenshotWalk fallback, multi-match mtime selection, realpath + session-root prefix check) out of handleMaestroScreenshot into locateScreenshot(). The handler now passes the shape-validated filePath and gets back a canonicalized absolute path or a 404. Move the semgrep path-traversal suppression to the new module (it now owns the path.join sinks). istanbul-ignore annotations travel verbatim. No behavior change.
…egions.js Move validateRegionInputs (shape checks) and resolveRegions (element/coordinate bbox resolution → comparison-payload fragments) out of handleMaestroScreenshot into a dedicated module. The hierarchy dump and warn-once flag stay request-scoped (call-local inside resolveRegions), preserving the per-request memoization invariant. Three independent output fields, algorithm pass-through, and percy.grpcClientCache threading are unchanged. istanbul-ignore annotations travel verbatim. No behavior change. maestro-screenshot.js is now a 182-line orchestrator.
…import Add test/unit/maestro-screenshot.test.js (parsePngDimensions) and test/unit/maestro-regions.test.js (validateRegionInputs + resolveRegions coordinate paths) — direct module-level coverage for the now-pure extracted functions, mirroring the maestro-hierarchy.test.js convention. The existing api.test.js HTTP-level suite remains the behavior contract (incl. the non-mocked sync-drain canary and the locateScreenshot filePath/glob/traversal specs). Remove the now-unused ServerError import from api.js (all its throws moved into the extracted modules).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Put platform/sessionId/percy on their own lines in the multiline resolveRegions() call objects (eslint object-property-newline).
…p join
The api.js .semgrepignore entry was covering two sinks: the maestro-screenshot
path joins (now moved to maestro-screenshot-file.js) AND createStaticServer's
path.posix.join('/', baseUrl, …) sitemap-URL construction. Removing api.js
re-exposed the latter to the path-join-resolve-traversal rule (baseUrl is
trusted server config; filenames are locally globbed *.html). Re-add api.js
with a createStaticServer-focused rationale.
…sign semgrep (express-data-exfiltration) flagged Object.assign(payload, …) where the merged object derives from req.body as a mass-assignment risk. The original inline code set payload.regions / ignoredElementsData / consideredElementsData explicitly; restore that — resolveRegions returns only those three keys, so assigning them by name is behavior-identical, coverage-neutral, and avoids the dynamic merge of request-derived data.
Add deriveDeviceInsets(options) — relay-side, transport-reusing derivation of
exact device system-bar insets (pixels) for the Maestro comparison tile:
- iOS: fetch /viewHierarchy, read the statusBars element frame (elementType 26,
XCUIElementTypeStatusBar) in points and the AUT root frame for an empirical
points->pixels scale (PNG height / root point height, integer-snapped +
sanity-bounded to guard the Plus-class nativeScale!=scale gap). navBarHeight
is always 0 (static home indicator, fleet-consistent). The status-bar frame
is a sibling of the AUT and is discarded by dump()'s flatten, so this does a
dedicated raw-response parse.
- Android: distinct 'dumpsys window' read via the existing resolveSerial +
execAdb path (ANDROID_SERIAL or adb-devices probe; always -s <serial>),
parsing mStableInsets=Rect(L, T - R, B) -> {status:T, nav:B}.
Returns null on any failure (transport, parse, no device, implausible scale) —
the relay falls back to the SDK default. Additive only; dump() and the resolver
cascade are untouched. 25 fixture-driven unit specs (stubbed httpRequest/execAdb).
…per-session cache handleMaestroScreenshot now derives exact device system-bar insets (once per session, cached on percy.maestroInsetCache) and uses them for the tile's statusBarHeight/navBarHeight, authoritative over the SDK's static defaults: - statusBarHeight = derived ?? (req.body.statusBarHeight || 0) - navBarHeight = iOS ? 0 : (derived ?? (req.body.navBarHeight || 0)) Any derivation failure (null) falls back to the SDK value; iOS bottom is always 0. The cache (plain per-Percy Map, keyed by sessionId) is constructed in the Percy constructor and cleared in stop(), mirroring grpcClientCache. Tests: relay override/fallback/iOS-0/derive-and-cache specs (seeding the cache as the deterministic test seam — no real adb/HTTP in the unit env) + a stop()-clears-cache spec.
…l-body spec Collapse compound defensive conditions in the iOS/Android inset parsers to single, fully-testable checks (relying on the scale sanity bounds, JSON.parse's catch, and findAxAutRoot's own guard rather than redundant pre-checks), and mark the genuinely-unreachable frame/optional-chain guards with surgical istanbul-ignores + rationale (mirroring flattenIosAxElement). Add an iOS spec for a JSON-literal-null body. Behavior unchanged.
Sriram567
changed the title
Compose the makeIosBody / no-status-bar fixture nodes from single-line object literals instead of multi-line literals with multiple properties per line (eslint object-property-newline / object-curly-newline).
- Drop the Promise.race circuit-breaker in deriveIosInsets (the timeout-arrow never fires under test → uncovered line+function); a single httpRequest with the transport's own timeoutMs is sufficient for best-effort derivation. - Cover deriveDeviceInsets' defensive catch with a throwing-getEnv spec instead of an istanbul-ignore. - Replace the relay debug log's derived/fallback ternary with JSON.stringify (branchless + more informative) — the 'derived' arm needed a real device. Closes the maestro-hierarchy.js (1230,1443) + maestro-screenshot.js (133) coverage gaps; all specs still green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR #2217 (cross-platform Maestro view-hierarchy resolver + screenshot
filePathrelay) shipped its resolver cleanly inmaestro-hierarchy.js, but landed ~639 lines of relay glue directly inpackages/core/src/api.js— a ~440-line/percy/maestro-screenshotroute closure plus thehandleComparisonUploadmultipart handler and theparsePngDimensions/manualScreenshotWalkhelpers.api.jsis meant to be a route table that delegates to domain modules (thehandleSyncJob-in-snapshot.jspattern); this restores that boundary.Behavior-preserving refactor — no wire/runtime change. Decomposes the relay into four cohesive modules and reduces each route to a one-line delegate:
comparison-upload.js/percy/comparison/uploadmultipart handlermaestro-screenshot.js/percy/maestro-screenshotorchestrator + request validation +parsePngDimensions+ payload build + sync/async uploadmaestro-screenshot-file.jsmanualScreenshotWalk, mtime pick) + realpath/session-root security checkmaestro-regions.jsencodeURLSearchParamsmoved toutils.js(next to the other URL helpers) so the modules share it without a circular import.api.js: 994 → 382 lines.Commits (one cohesive unit each)
comparison-upload.jsmaestro-screenshot-file.js(file location + security)maestro-regions.js(region validation + resolution)ServerErrorimportInvariants preserved
cachedDumpstays request-scoped (call-local insideresolveRegions) — no cross-request hierarchy leak.parsePngDimensionsfill-not-override semantics; strictsync === true; three independent region output fields;algorithmpass-through (no relay-side enum validation);percy.grpcClientCachethreading./* istanbul ignore */annotation travels attached to its exact statement..semgrepignorepath-traversal suppression moved fromapi.jstomaestro-screenshot-file.js(the new owner of thepath.joinsinks); theapi.jsentry is dropped (it predated this code and has no remaining tainted joins).Testing
api.test.js(the HTTP-level behavior contract, incl. the non-mocked sync-drain canary and thelocateScreenshotfilePath/glob/traversal specs) passes unchanged after every unit — the single remaining failure is the pre-existing IPv4/IPv6ECONNREFUSED→AggregateErrorflake (api.test.js:655), unrelated to this change.test/unit/specs (parsePngDimensions,validateRegionInputs,resolveRegionscoordinate paths) pass.api.test.jsdrives the same route into the relocated code) with ignores intact, plus direct unit coverage of the pure functions. The 100% NYC gate and semgrep are validated authoritatively in CI (the full local suite has ~27 unrelated environmental flakes — browser-launch timeouts, port-5338 collisions, system-proxy detection — that make a local full-coverage run unreliable on this machine).Post-Deploy Monitoring & Validation
nyc --check-coverage(100% branches/lines/funcs/statements), and the semgreppath-join-resolve-traversaljob (confirms the.semgrepignoreretarget tomaestro-screenshot-file.js).Also bundled: exact device system-bar insets (commits
393fc5f9,317fe30b,32114908)The Maestro comparison tile's
statusBarHeight/navBarHeightare now derived per-device, relay-side (CLI) instead of taking the SDK's static defaults — authoritative over the SDK value, cached per session, with graceful fallback to the default on any failure. Works identically on BrowserStack App Automate and self-hosted Maestro (no SDK or host-injection change). Plan:docs/plans/2026-06-19-001-feat-maestro-exact-system-bar-insets-plan.md; brainstorm + cross-SDK rationale:docs/brainstorms/2026-06-19-...; ticket PER-8982./viewHierarchystatusBarselement frame (elementType 26) × empirical PNG→points scale. iPhone 14 → 141px (corrects the prior unscaled47).percy-xcui-swift/percy-appium-python. Replaces the SDK's80outlier.adb dumpsys windowmStableInsetstopmStableInsetsbottom (gesture vs 3-button both surface here)Design
deriveDeviceInsets()inmaestro-hierarchy.js— additive; the resolver cascade,dump(), and the drift envelope are untouched. Reuses the existing transports (/viewHierarchyHTTP,resolveSerial+execAdb).PNG pixel height ÷ AUT root point height, integer-snapped + sanity-bounded (guards the Plus-classnativeScale ≠ scalegap); implausible scale → fallback.null→ SDK default. Never blocks a snapshot.percy.maestroInsetCache, keyed bysessionId) mirrorsgrpcClientCache: constructed in thePercyconstructor, cleared instop().Tests
-s <serial>targeting,adb devicesprobe, no-device / spawn-error / non-zero-exit / unparseable); relay override/fallback/iOS-0/derive-and-cache;stop()clears the cache.Post-Deploy Monitoring & Validation (insets)
[percy] maestro device insets (<platform>): derived|fallbackdebug line.fallbackon devices where derivation should work, or a new wave of false diffs attributable to bar heights → derivation is mis-parsing; the fallback keeps snapshots flowing while investigated. Confirmdumpsys mStableInsetsparse and the iOSelementType 26frame against the affected device.🤖 Generated with Claude Code