Repository navigation
refactor(ios): drop the unused modal-scope input and document corpus recording - #4147
Conversation
…recording SnapshotPresentedSurface.classify loses its modalScope parameter, which its one caller always passed as nil, along with the modalScopeExpected column and its tests. ModalSurface counts each parent's children once and notes the rootMismatch label and the duplicate-occurrence rule. The corpus README now lists prerequisites, a worked expect and expectModalScope example, and pnpm print:alert-surface, which prints a recording's honor-modal tree with indexes, types and labels. The test-app README gains a full fresh-simulator recipe for the SpringBoard prompt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
print:alert-surface now checks preorder like the Swift loader and pairs each
honor-modal node with one plain occurrence, the way ModalSurface walks the
pair. It marks a node whose duplicated child was kept only in part as
duplicatedChild instead of merging occurrences. A recording can now state
that reading as {"reading": "unknown", "reason": "duplicatedChild"}, and the
ModalSurface comment says what the walk does when every occurrence is
removed.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The classify change at d59a304 looks right, but two open threads below (the README Not blocking, and you can take or leave these: print-alert-surface-tree.ts:25 counts children by The open inline threads on the README Two Smoke Tests runs and Coverage were still pending when I looked, and Repo Guards & Compatibility and the other Smoke Tests run have passed. The diff touches no packages/ or src/ code, so a Coverage or Smoke failure that does not name the Swift changes or the new script is likely unrelated. There are no conflicts. Once the pending runs finish green, this is ready to merge. |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Apex SentinelVerdict: Since the last review: 6 new · 0 still open · 6 no longer raised 6 finding(s): 2 minor, 4 nit. Verdict: approve. reached max rounds (1). 🟡 Minor (2)
🔵 Nit (4)
Reply under an inline comment with |
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 6 new findings on 07dd8c8. The verdict and the full list are in the summary comment.
|
|
||
| // Pairs each honor-modal node with one plain occurrence, the way ModalSurface walks the pair. An | ||
| // element listed more than once under one parent pairs in order when every occurrence is kept. | ||
| // When only some are kept, ModalSurface reads `unknown(duplicatedChild)`, so this stops there. |
There was a problem hiding this comment.
🟡 Minor · print-alert-surface-tree.ts reports nothing for a root mismatch or an empty honor-modal read, so those recordings look like noExclusion
The marker walk starts pairing only when the two roots carry the same element (line 63), and ModalSurface.classify-style pairing returns .unknown(.rootMismatch) in exactly that case (and for an empty honor-modal read, per the note this PR adds at ModalSurface.swift:61-62). When the roots differ, pairedPlain stays empty, every node hits continue at line 66, markers stays empty and the script prints a clean tree with no marker and no message. The corpus README this PR adds then instructs the opposite reading ("If no node is marked, write {"reading": "noExclusion"}"), so the contributor records noExclusion for a screen whose expected reading is unknown/rootMismatch, and ModalSurfaceTests fails on a fresh recording with no hint why. Fail or print an explicit [rootMismatch] marker instead of printing a marker-free tree. scripts/print-alert-surface-tree.ts:63.
| // When only some are kept, ModalSurface reads `unknown(duplicatedChild)`, so this stops there. | |
| const markers = new Map<number, string>(); | |
| if (treeName === 'honorModal') { | |
| const plainChildren = childrenOf(plain); | |
| const keptChildren = childrenOf(nodes); | |
| const pairedPlain = new Map<number, number>(); | |
| if (nodes[0].element !== plain[0].element) { | |
| fail(`${file}: the roots differ (${plain[0].element} vs ${nodes[0].element}), so the pair reads unknown(rootMismatch).`); | |
| } | |
| pairedPlain.set(0, 0); |
Suggested change
When nodes[0].element !== plain[0].element or the honor-modal tree is empty, fail the command (or add a root-level marker) saying the pair reads unknown(rootMismatch), so a marker-free tree only ever means noExclusion.
Details
Also raised here
Also raised at this location by swift-ios: Printer reports a tree as unmarked when the two reads have different roots, so the README's rule yields a wrong noExclusion — scripts/print-alert-surface-tree.ts:66 seeds the pairing only when nodes[0].element === plain[0].element. When the honor-modal read starts at a different element, pairedPlain stays empty, so the loop at line 67 continues on every node and the printer prints the whole tree with no markers and no warning — yet ModalSurface.read answers .unknown(.rootMismatch) for exactly that pair (ModalSurface.swift:63). The new README instructs the contributor to translate "no node is marked" into {"reading": "noExclusion"} (README.md:48), which is the opposite of what the rule reads. The author's own testAnHonorModalReadFromAnotherRootIsUnknown (ModalSurfaceTests.swift:179-182) covers that reading, so the pair is reachable in the rule the printer exists to serve. A pair whose two roots differ prints identically to a screen with no modal. Print the reason when the roots differ instead of staying silent.
persona: maintainability, swift-ios
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| ); | ||
| continue; | ||
| } | ||
| const removed = [...plainChildren[plainNode]]; |
There was a problem hiding this comment.
🟡 Minor · Tree printer marks only partial exclusions, so the README's marker-only procedure mislabels noSurvivingSurface and root-mismatch recordings
The new printer marks a node only when it removed children AND kept at least one: if (removed.length > 0 && keptChildren[node.index].length > 0) (scripts/print-alert-surface-tree.ts:91). A node whose children were all removed is printed with no marker, and the script never reports the other readings ModalSurface.read can return — noSurvivingSurface (removal with no kept sibling), rootMismatch (the script pairs only when nodes[0].element === plain[0].element, line 66, and prints an unmarked tree otherwise), and the unstable(...) reasons.
Suggested change
Either mark the other unknown readings in the printer output (root mismatch, kept-nothing / noSurvivingSurface), or extend the README step 5 with the case where a node loses all of its children and the case where the honor-modal root differs, so a contributor does not write noExclusion for a pair ModalSurface reads as unknown.
Details
The rewritten README decides the expectation from the markers alone: "If no node is marked, write {\"reading\": \"noExclusion\"}" (apple/snapshot-presentation/Tests/AlertSurfaceCorpus/README.md, step 5). So for a recording whose honor-modal read empties a container without keeping a sibling — the shape the existing unit test testRemovedContentWithoutAKeptSiblingIsUnknown pins as .unknown(.noSurvivingSurface) — the printer prints no marker and the documented procedure tells the contributor to write noExclusion, which then fails swift test with no explanation of why. The same happens for a recording whose honor-modal read starts at a different root, which ModalSurface.read answers .unknown(.rootMismatch).
The printer already prints the reason for one unknown (duplicatedChild); it should do the same for the other readings it can infer (root mismatch, no kept sibling), or the README should say what to write when a node loses all its children instead of falling back to "no node is marked".
persona: javascript
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| // write its `expectModalScope` by hand. See apple/snapshot-presentation/Tests/AlertSurfaceCorpus/README.md. | ||
| import fs from 'node:fs'; | ||
|
|
||
| type RecordedNode = { |
There was a problem hiding this comment.
🔵 Nit · New printer script is outside the typechecked program, so its RecordedNode shape drifts from the decoder silently
scripts/print-alert-surface-tree.ts is added, but tsconfig.json include lists only src, test and a few named scripts (scripts/help-conformance-command-validator.ts, scripts/clean-daemon.ts, scripts/coverage-*/**, the vitest reporters), so pnpm typecheck (tsc -b tsconfig.typecheck.json) never compiles it — it runs only through node --experimental-strip-types. Its RecordedNode type (index, parentIndex?, element, type, label) is a hand-maintained mirror of RecordedAlertSurface.Node in apple/snapshot-presentation/Tests/AgentDeviceSnapshotPresentationTests/RecordedAlertSurface.swift, and the corpus node shape already carries identifier and rect fields the TS type omits. Nothing fails today because the script only reads index, parentIndex, element, type, label, and the as Record<string, unknown> / tree as RecordedNode[] casts (lines 22, 40) assert the shape instead of checking it, so a future field rename in the capture format reaches the script as undefined with no typecheck error and no runtime validation.
Suggested change
Add scripts/print-alert-surface-tree.ts (and scripts/capture-alert-surface.ts, which has the same gap) to the tsconfig include list so the corpus-node mirror is at least typechecked.
Details
Also raised here
Also raised at this location by swift-ios: New script's typecheck coverage and duplicated preorder rule vs the corpus loader — scripts/print-alert-surface-tree.ts re-implements the corpus preorder rule that RecordedAlertSurface.isPreorder already owns (RecordedAlertSurface.swift:59-66), and unlike the neighbouring entries in tsconfig.json:28-32 (scripts/help-conformance-command-validator.ts, scripts/clean-daemon.ts, scripts/vitest-coverage-run-stamp-reporter.ts) it is not listed there, so its types are not checked by tsc — the tree.forEach((node: RecordedNode, position) => …) cast at line 30/40 is exactly the unverified assertion a checked build would look at (recording[name] is unknown[], so node.element is unchecked). Nothing fails today on the seven recordings, which all pass the printer.
persona: javascript, swift-ios
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| if (!file || (treeName !== 'honorModal' && treeName !== 'plain')) { | ||
| fail('Usage: pnpm print:alert-surface <recording.json> [honorModal|plain]'); | ||
| } | ||
| const recording = JSON.parse(fs.readFileSync(file, 'utf8')) as Record<string, unknown>; |
There was a problem hiding this comment.
🔵 Nit · Printer's preorder check accepts Number.isInteger floats the corpus reader rejects, and prints an ENOENT stack for a mistyped path
Two smaller gaps on the new script. (1) preorderTree accepts a parent written as 1.0 because Number.isInteger(1.0) is true, while it then indexes children[node.parentIndex] at line 46 with that raw value; the Swift reader it claims to mirror (RecordedAlertSurface.isPreorder, RecordedAlertSurface.swift:59-66) decodes Int, so the same recording fails RecordedAlertSurface.load after the printer calls it well-formed. The comment on line 26 says "The same check as RecordedAlertSurface.load", which is then not quite true. (2) A mistyped recording path bypasses fail() and dumps a Node stack trace (Error: ENOENT: no such file or directory, open 'does-not-exist.json') instead of the script's own message, because fs.readFileSync on line 22 is not guarded. Both are contributor-facing only, on a helper script.
| const recording = JSON.parse(fs.readFileSync(file, 'utf8')) as Record<string, unknown>; | |
| let raw: string; | |
| try { | |
| raw = fs.readFileSync(file, 'utf8'); | |
| } catch (error) { | |
| fail(`${file}: ${error instanceof Error ? error.message : String(error)}`); | |
| } | |
| const recording = JSON.parse(raw) as Record<string, unknown>; |
Suggested change
Wrap the read in the script's own failure path, and require an integer-valued parentIndex the way the Swift loader does (e.g. Number.isInteger(parent) && parent >= 0 && parent < position && String(parent) === String(node.parentIndex) or a plain typeof parent === 'number' && Number.isInteger(parent) plus rejecting a non-array/NaN index).
Details
persona: swift-ios
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| return tree as RecordedNode[]; | ||
| } | ||
|
|
||
| function childrenOf(tree: RecordedNode[]): number[][] { |
There was a problem hiding this comment.
🔵 Nit · print-alert-surface-tree.ts rebuilds the tree from parentIndex while claiming to pair "the way ModalSurface walks the pair"
The new printer derives both trees' children with childrenOf, which keys on node.parentIndex (lines 43-48), and pairs nodes with that structure (lines 62-97). The Swift rule it documents instead rebuilds each read's tree from parentElement: ModalSurface.children(of:) (ModalSurface.swift:122-140) assigns every node to its nearest listed ancestor whose element equals the node's parent element, and the README tells contributors to read host/roots off the markers this script prints (AlertSurfaceCorpus README step 3 and 5, "the innermost marked node of the branch"). Those two walks agree on the seven recorded corpus files (I compared both walks on all of them: 0 differing nodes), so the agreement is coincidental, not enforced: it holds only while no node's nearest matching-element ancestor differs from the node parentIndex names — e.g. a node whose parent element equals its own element, or an element listed under an ancestor chain that repeats a parent element. The corpus already contains elements listed twice under one parent, so a recapture can break the invariant. When it does, the printer marks a different node than ModalSurface reads as host: the contributor writes the wrong host/roots, ModalSurfaceTests fails, and the README's advice is "print the tree again", which prints the same misleading marker — the marker and the rule disagree with no hint which one is wrong. The script's own comment (line 58: "the way ModalSurface walks the pair") and its preorder comment (line 26: "The same check as RecordedAlertSurface.load") claim an equivalence neither type nor test enforces. Two concrete divergences in the preorder comment alone: the recording's root node carries no parentIndex key and the corpus records parentIndex: null style nodes only by absence, so parent === undefined and Swift's decoded Int? nil coincide only because the capture never writes an explicit null; and a parentIndex: null on node 0 would pass Swift's check but fail the printer's Number.isInteger(parent) as "out of preorder".
| function childrenOf(tree: RecordedNode[]): number[][] { | |
| function childrenOf(tree: RecordedNode[]): number[][] { | |
| // Mirrors ModalSurface.children(of:): a node belongs to its nearest listed ancestor whose element | |
| // equals its parent element, not to the node its parentIndex names. Keep both walks in step. | |
| const children = tree.map((): number[] => []); | |
| const ancestors: number[] = []; | |
| for (const node of tree) { | |
| const parentElement = | |
| node.parentIndex === undefined || node.parentIndex === null | |
| ? undefined | |
| : tree[node.parentIndex].element; | |
| if (parentElement !== undefined) { | |
| while (ancestors.length > 0 && tree[ancestors[ancestors.length - 1]].element !== parentElement) | |
| ancestors.pop(); | |
| if (ancestors.length === 0) fail(`${file}: "${node.index}" names no listed parent element.`); | |
| children[ancestors[ancestors.length - 1]].push(node.index); | |
| } else if (node.index !== 0) { | |
| fail(`${file}: node ${node.index} is out of preorder. Capture it again.`); | |
| } | |
| ancestors.push(node.index); | |
| } | |
| return children; | |
| } | |
Suggested change
Either port the Swift nearest-ancestor-by-parentElement walk into the printer (as in the patch) or have the printer assert at startup that parentIndex-based and parentElement-based children agree, and fail with a message naming the node where they diverge. Do not claim in the comment and in the README that the printer pairs "the way ModalSurface walks the pair" while it reads a different parent field.
Details
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| const listedCount = listed.get(element) ?? 0; | ||
| return listedCount > 1 && count < listedCount; | ||
| }); | ||
| if (ambiguous) { |
There was a problem hiding this comment.
🔵 Nit · Printer's duplicatedChild marker prints the raw AX element id, which the README's example presents as an element name
The README tells the contributor to look for [duplicatedChild: kept 1 of 2 occurrences of …] (AlertSurfaceCorpus/README.md step 3). The script fills that slot with element (print-alert-surface-tree.ts:80), and in real recordings element is an opaque accessibility id — for example 99943:4829729792.1219 in iphone-duo-folded-springboard-open-in-app.json — not a type or label, so the marker the contributor is told to recognise never resembles the documented one and names no node they can see on screen. The tree lines print type and label, so the same node is identifiable there by two other fields the marker ignores.
| if (ambiguous) { | |
| if (ambiguous) { | |
| const [element, count] = ambiguous; | |
| const name = nodes[node.index].type || nodes[node.index].label || element; | |
| markers.set( | |
| node.index, | |
| `[duplicatedChild: kept ${count} of ${listed.get(element)} occurrences of ${name} (${element})]`, | |
| ); | |
| continue; | |
| } | |
Suggested change
Print the node's type and label alongside the raw element id in the marker, or show the README a real marker line so the documented example matches the command output.
Details
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
print:alert-surface now ports ModalScopePair.pair and ModalSurface.read step for step. It marks kept children only when the rule reads a modal surface. Otherwise it prints the rule's reason on the first line, such as `unknown: unstable(reparentedElement) at node 3`, and marks no node. The seven corpus recordings print unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
print:alert-surface now prints `reading: <x>` first for every recording: modal, noExclusion, or unknown(<reason>) with the failing node. It ports children(of:) with its ancestor walk and CGRect equality, so the reading matches ModalSurface.read in every case. It rejects a recording the corpus loader rejects, such as a non-integer index, and reports a missing file in one line. The corpus README procedure now starts from the reading line, names the raw AX element id in duplicatedChild, and asks contributors to check the reading against the screenshot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
print:alert-surface rejects a recording whose format is not 2, and labels its first line as ModalSurface's answer, not ground truth. The corpus README now says, before the steps, that expect and expectModalScope come from the screenshot. The printer only helps find node indexes. Step 5 derives the surface from the prompt on the screen, then compares it with the rule's reading. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I reviewed c7a395e. The open thread on the missing Smoke Tests is still running and is the only check not passing. This head changes no Item (1) is the blocking one; the others you can take or leave: (1) The README says the printer stops on a recording the replay would not load, and the script says it makes the loader's checks, but recordedTree() skips two of them. It accepts The two open inline threads on the missing A question on scope: does the printer need to port the rule into TypeScript? The delta replaced a ~25-line marker counter with ~150 lines that re-implement pair, read and I did not run |
There was a problem hiding this comment.
Apex Sentinel: 6 new findings on 4b6ccdf. The verdict and the full list are in the summary comment.
| if (!Array.isArray(tree) || (tree.length === 0 && name === 'plain')) { | ||
| fail(`${file}: "${name}" is not a tree`); | ||
| } | ||
| tree.forEach((node, position) => { |
There was a problem hiding this comment.
🆕 🟡 Minor · print:alert-surface accepts non-integer node indexes the Swift replay rejects
recordedTree validates node.index === position without checking that the index is an integer, while the loader it claims to mirror (RecordedAlertSurface.isPreorder) decodes index as Int, so any non-integer index makes the recording fail to load there. JSON.stringify/JSON.parse keeps a fractional literal distinct (1.5 !== 1, and 1.0 is stored as the double 1, not 1.5), so a recording whose node 1 carries "index": 1.5 passes the printer and fails RecordedAlertSurface.load with a corrupt-decode error.
| tree.forEach((node, position) => { | |
| tree.forEach((node, position) => { | |
| if (node.parentIndex === null) delete node.parentIndex; | |
| const parent = node.parentIndex; | |
| const isValid = | |
| Number.isInteger(node.index) && | |
| node.index === position && | |
| (parent === undefined | |
| ? position === 0 | |
| : Number.isInteger(parent) && parent >= 0 && parent < position) && | |
| ['element', 'type', 'identifier', 'label'].every((key) => typeof node[key] === 'string') && | |
| isRect(node.rect); |
Suggested change
Require Number.isInteger(node.index) in recordedTree, so the printer rejects exactly the recordings RecordedAlertSurface.load rejects.
Details
Result: the printer's promise in the comment on lines 44-45 ("The checks RecordedAlertSurface.load makes") and in the README ("If the recording does not load in the replay, the command stops with one line that names the problem") is false for this case. The contributor gets a printed reading and node indexes for a recording that the corpus replay can never load, and the real failure only appears later as CocoaError(.coderReadCorrupt) with no node named.
The same gap applies to parentIndex: only Number.isInteger(parent) is checked (line 58), never that node.index is an integer.
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| if (found === undefined) return undefined; | ||
| nested += found; | ||
| } | ||
| if (removed.length === 0 || kept.length === 0) return nested; |
There was a problem hiding this comment.
🆕 🟡 Minor · Tree printer marks nodes the rule does not treat as hosts when modals nest
surfaces() writes a [kept N of M children] marker on every node that removed some and kept some children, including a node that has a nested surface under it (lines 179-184). Swift ModalSurface.read records a host only when the kept children hold no surface of their own (guard nested.isEmpty, !removed.isEmpty, !kept.isEmpty, ModalSurface.swift:93) — the inner surface replaces the outer one.
Suggested change
Mark a node only when it is a host in the rule's sense (no nested surface under it), matching guard nested.isEmpty, !removed.isEmpty, !kept.isEmpty, or state in the README and the printer output that outer marked nodes are not hosts and the innermost mark is the host.
Details
I ran the printer's own logic on a nested pair (window keeps a sheet and loses its content; the sheet keeps an alert and loses its sheetContent). It printed markers on BOTH the window and the sheet, while the rule answers ModalSurface(host: 3, roots: [4]) — the sheet only. The README tells the contributor that the host is "the node where the honor-modal tree removed the rest of the screen", and the PR's own worked example quietly relies on picking "the innermost marked node", but neither the README nor the printer says that several nodes can be marked, or that the outer ones are not hosts. A contributor who reads host off the first/outer marker writes an expectModalScope the replay then rejects for a recording whose tree is fine.
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| } | ||
|
|
||
| // CGRect equality compares rects after it makes their sizes positive. | ||
| function frameKey({ infinite, nonFinite, x, y, width, height }: Rect): string { |
There was a problem hiding this comment.
🆕 🔵 Nit · frameKey collapses infinite and nonFinite rects differently than the Swift CGRect equality it ports
The printer ports ModalScopePair.pair and claims (lines 84-86) that pairInstability returns the reading the replay checks. For the frames it does not, in one case. FixtureRect decodes {"infinite":true} to CGRect.infinite (= x:-inf, y:-inf, w:inf, h:inf) and {"nonFinite":true} to the identical rect. So in Swift, a plain node flagged infinite and its honor-modal counterpart flagged nonFinite compare equal via match.frame == node.frame, and the pair stays .paired. In the TS port, frameKey returns 'infinite' for one and 'nonFinite' for the other (lines 78-79), so pairInstability prints unknown(unstable(movedElement)) where the replay reads a normal result. Reachable only when the AX server flags the same node's rect with different flags across the two reads of one screen, so the impact is a misleading label in the printed reading, not a wrong test result; the comment claiming parity should then hold only with that caveat, or the two flags should collapse to one key like Swift does.
| function frameKey({ infinite, nonFinite, x, y, width, height }: Rect): string { | |
| function frameKey({ infinite, nonFinite, x, y, width, height }: Rect): string { | |
| if (infinite || nonFinite) return 'nonFinite'; | |
| const origin = [width < 0 ? x + width : x, height < 0 ? y + height : y]; | |
| return [...origin, Math.abs(width), Math.abs(height)].join(); | |
| } |
Details
persona: javascript
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| function frameKey({ infinite, nonFinite, x, y, width, height }: Rect): string { | ||
| if (infinite) return 'infinite'; | ||
| if (nonFinite) return 'nonFinite'; | ||
| const origin = [width < 0 ? x + width : x, height < 0 ? y + height : y]; |
There was a problem hiding this comment.
🆕 🔵 Nit · frameKey builds an origin array it only spreads again
origin holds two of the four numbers that join() then receives, so a reader has to follow the spread across two statements to see the key is origin, width, height. Join the four values in one call.
| const origin = [width < 0 ? x + width : x, height < 0 ? y + height : y]; | |
| return [width < 0 ? x + width : x, height < 0 ? y + height : y, Math.abs(width), Math.abs(height)].join(); |
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| const counts = new Map<string, number>(); | ||
| for (const position of positions) { | ||
| counts.set(tree[position].element, (counts.get(tree[position].element) ?? 0) + 1); | ||
| } |
There was a problem hiding this comment.
🆕 🔵 Nit · surfaces() re-asserts its two child tables on every line
readModalSurface returns early when plainChildren or keptChildren is undefined, yet the nested surfaces still writes plainChildren! and keptChildren! five times, so each use has to be checked against that guard by hand. Bind them once after the guard (const plainChildren = tables.plainChildren; const keptChildren = tables.keptChildren;) and drop the assertions.
| } | |
| const plainChildren = plainTables ?? fail(`${file}: plain children are not readable`); | |
| const keptChildren = keptTables ?? fail(`${file}: honor-modal children are not readable`); |
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| const listedCount = listed.get(element) ?? 0; | ||
| const keptCount = keptListed.get(element) ?? 0; | ||
| if (listedCount >= 2 && keptCount < listedCount) { | ||
| failure = `unknown(duplicatedChild): node ${honorModalNode} kept ${keptCount} of ${listedCount} children with element id ${element}`; |
There was a problem hiding this comment.
🆕 🔵 Nit · Pairing instability messages can name the wrong node index when the printed tree is plain
pairInstability and surfaces build their failure strings with node.index from whichever tree the failing node came from — the honor-modal tree for movedElement/unmatchedElement/reparentedElement, and the honor-modal parent for duplicatedChild (lines 103, 106, 163, 168). Line 197 then optionally prints the plain tree instead. Run as pnpm print:alert-surface <recording> plain, the first line says e.g. unknown(unstable(movedElement)): node 12 while the listed node 12 is the plain tree's node 12, an unrelated node; the plain counterpart is the removed match the walk already has at line 166. The README instructs contributors to search the recording for the reported id (which is correct for duplicatedChild), so a numeric index that points at the wrong tree of the same file is exactly the confusion the tool should avoid.
Suggested change
Either suppress the : node N suffix when treeName is 'plain', or print the node's element id (which is shared by both trees) alongside the honor-modal index.
Details
persona: javascript
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
|
The code looks good at 4b6ccdf. The format check and the screenshot-first rule from the earlier review are now in place, and the two open inline threads on them (#4147 (comment) and #4147 (comment)) are fixed at this head, so the author can resolve them. Not blocking, and you can take or leave these: Of the other open threads, the P2 notes on the marker for removed children (#4147 (comment)) and the instability index in Smoke Tests and Coverage are still running, so there is no result to attribute yet. This delta changes only the README and the printer script, and neither job runs that script. I did not run |
thymikee
added
the
ready-for-human
Summary
SnapshotPresentedSurface.classifydrops itsmodalScopeinput. Its only caller always passednil. ThemodalScopeExpectedcolumn, its fixture text, and its Swift test go too. The table rows and the TS rule stay as they are.ModalSurface(follow-up to fix(ios): reject ambiguous modal-surface pairings #4128): a note that an empty honor-modal read reportsrootMismatchonly as a label, the duplicate-occurrence rule in the walk comment, and each parent's children counted once. No behavior change.expect+expectModalScopeexample, the closingswift teststep, and a screenshot-first rule: expectations come from what the screen shows, never from the rule's output. An ambiguous duplicate may be expected asunknown(duplicatedChild).pnpm print:alert-surface: prints a recording's honor-modal tree with indexes and kept-children markers, plusModalSurface's reading labelled as the rule's answer, not ground truth. It rejects malformed or non-format-2 recordings.Tree printer on a real recording
Verification
swift test --package-path apple/snapshot-presentation: 50 passedgeometry-policy-presented-surface.test.ts: passedbuild:xcuitest:iosandbuild:xcuitest:macos: succeededpnpm check:affected --run --head 4b6ccdf7cc: passed🤖 Generated with Claude Code