refactor(core): decompose Maestro screenshot relay out of api.js#2286
Open
Sriram567 wants to merge 8 commits into
Open
refactor(core): decompose Maestro screenshot relay out of api.js#2286Sriram567 wants to merge 8 commits into
Sriram567 wants to merge 8 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.
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).🤖 Generated with Claude Code