mirror of
https://github.com/callstack/agent-device.git
synced 2026-09-14 20:06:34 +08:00
Michał Pierzchała
13b3d4fc88
* fix(replay): demote non-unique ids from writer identity/selector chain (#1269) Android list-row GET replays bind the wrong row because the recorder uses the non-unique framework resource id `android:id/title` (matchCount 11 on Settings root) as primary identity; positional drift then makes the identity verifier correctly refuse with `identity-mismatch`. Demote an id from identity whenever it matches more than one node in the record-time tree (capture-time uniqueness, not an `android:id/*` namespace check — a reused RN FlatList testID hits the same class on iOS). Applied in both places a recorded id feeds identity: - `computeTargetEvidence` (session-target-evidence.ts): the `target-v1` identity tuple falls back to role+label when the id's own capture-time match count exceeds one, reusing the existing `filterIdentitySet` domain machinery (an empty ancestry degrades it to a plain id scan). - `buildSelectorChainForNode` (selectors/build.ts): the recorded selector chain omits a non-unique id rather than leading with it. Every writer call site (get/press/fill recording, plus the divergence-suggestion path) now passes the record-time tree so the check has something to count against; omitting it preserves prior behavior for isolated-node callers (tests). Resolver-side `resolveSelectorChain` and live press/fill resolution are untouched per ADR 0012 (disclosed-not-changed disambiguation) — this is writer/replay-scoped only. Amends ADR 0012 decision 3: an id may serve as identity (and lead the selector chain) only when it uniquely denotes the target in the record-time tree. Adds fixtures: an Android duplicated-`android:id/title` list (the measured repro) and an iOS/RN duplicated-testID FlatList shape, both demoted and still verifying via the now-selective label; a regression case confirming an already-unique id is unaffected. Out of scope: the Android list-*press* class (matchCount 12, label-less `role="linearlayout"` container with no id at all to demote) needs a separate design decision — deriving identity from the labeled descendant. Tracked as a follow-up, not attempted here. * fix(replay): unify the id-uniqueness predicate across both writer sites (#1269 review) Address the maintainer review on #1272: 1. ONE shared uniqueness predicate. The two demotion sites were counting id matches with DIFFERENT semantics — `demoteNonUniqueId` via `filterIdentitySet` (NFC + 256-byte cap, and a broken-parent-walk exclusion), `selectableId` via a raw `normalizeSelectorText` scan (trim, no NFC/cap, no exclusion) — so the identity tuple and the selector chain could disagree and half-demote (id gone from one, kept in the other). Extract `idMatchCountInTree(nodes, id)` in target-identity-node.ts, counting over the canonical `readNodeLocalIdentity` id the replay verifier keys on, with no ancestry/parent-walk exclusion. Both `demoteNonUniqueId` and `selectableId` now call it. Corrects the inaccurate "vacuously-true / plain id scan" comment. Cross-invariant test (build.test.ts): for the same node+tree, evidence.id === undefined iff the built chain has no id= clause — across demoted, unique, and a non-NFC (decomposed vs precomposed) edge case. Verified it fails under the old raw-scan and passes under the unified predicate. 2. End-to-end reorder proof (session-replay-target-classification.test.ts): record against a tree whose rows share android:id/title, then classify against a DIFFERENT replay tree where the shared-id rows reorder — the demoted role+label identity rebinds the correct row (verified, matchCount 1) while `id="android:id/title"` resolves ambiguously (null). This pins the FDR 1.0 -> 0 mechanism, not just record-time demotion. 3. Removed the conflated "20/20 clean" live-number comment from the unit test; it now states the mechanism (role+label selectivity) instead. Behavior for the already-clean unique-id path is unchanged: for ordinary ascii ids the canonical count equals the old raw count. The only outcomes that change are the edge cases the old split mishandled (non-NFC, broken parent walk) — where demotion is the correct result. The kept clause still emits the chain's own normalizeSelectorText id string, so unique ids lead the chain exactly as before. * fix(replay): thread record-time tree through the extracted suggestion helper Rebase-conflict resolution against origin/main. #1217 (typed direct Maestro engine) extracted `buildReplayDivergenceSuggestionForNode` out of `resolveSuggestionCandidate` and added a second caller in `session-replay-maestro-failure.ts`. My #1269 change had added `nodes` to the `buildSelectorChainForNode` call that #1217 moved into the extracted helper, so after rebase the helper referenced an out-of-scope `nodes`. Thread the record-time tree as a required `nodes` param on the helper and pass it from BOTH callers (each already has it in scope). This keeps the non-unique-id demotion applied wherever a divergence/repair suggestion chain is built — now including the typed-Maestro suggestion path — with no behavior change for the already-unique-id case.