refactor: share Maestro target matching primitives#622
Conversation
thymikee
left a comment
There was a problem hiding this comment.
Review — share snapshot parent-index traversal
The extraction is clean and the split is right: pulling the pure parent-index traversal (buildSnapshotNodeByIndex / findSnapshotAncestor / isDescendantOfSnapshotNode) into shared helpers while keeping the Maestro-specific ranking (regex/fuzzy text, RN overlay blocking, foreground-duplicate preference, tab-slot inference, rect inheritance, tap promotion) compat-local is exactly the boundary that keeps us Maestro-compatible while letting the generic selector path reuse the mechanics. The new childOf non-contiguous-index test is a good guard.
Two things worth confirming:
-
findNearestAncestorchanged resolution semantics, not just location. The old implementation walked parents via array position (nodes[current.parentIndex]) and de-duped byref. The new sharedfindSnapshotAncestorresolves by index map first (nodeByIndex.get(parentIndex) ?? nodes[parentIndex]) and de-dupes byindex. For contiguous snapshots (index === position) this is identical, but for non-contiguous indexes it's a genuine behavior change — arguably a bug fix, but it now affects everyfindNearestHittableAncestorcaller, not just Maestro. Please confirm that's intended and that the non-Maestro callers are happy with index-based resolution (thechildOftest only covers the Maestro path). Bonus: the consolidation also adds cycle protection (visited) to the Maestro traversal, which previously had none — nice. -
result !== nullcontinuation contract.findSnapshotAncestornow stops as soon asresolve(...)returns anything!== null, so aresolvethat returns a falsy-but-non-null value (0,false,'') would terminate early. Current callers only return a node ornull, so this is fine, but a one-line doc comment on the generic stating "returnnullto keep walking" would prevent a future footgun.
Mostly mechanical — no objections.
Generated by Claude Code
|
Summary
Extracts the stable snapshot parent-index traversal primitives from Maestro runtime target matching into shared snapshot helpers, then reuses them from selector visibility and Maestro child/ancestor matching.
Maestro-specific ranking stays compat-local: regex/fuzzy text ranking, React Native overlay blocking, foreground duplicate preference, tab/breadcrumb slot inference, rect inheritance, and tap ancestor promotion are unchanged because they encode Maestro compatibility policy rather than generic selector semantics.
Touched files: 5. Scope stayed within Maestro target matching plus shared selector/snapshot helpers and regression coverage.
Validation
Focused Maestro runtime target and selector predicate tests passed, including the childOf guard for non-contiguous node indexes. Added shared snapshot-processing coverage for non-contiguous parent indexes and null-continues ancestor traversal.
pnpm typecheck,pnpm format, andpnpm check:unitpassed.