Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions apple/snapshot-presentation/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,14 @@ let package = Package(
],
targets: [
.target(name: "AgentDeviceSnapshotPresentation"),
.target(name: "RecordingSupport", dependencies: ["AgentDeviceSnapshotPresentation"]),
.executableTarget(
name: "SnapshotPresentationConformance",
dependencies: ["AgentDeviceSnapshotPresentation"]
dependencies: ["AgentDeviceSnapshotPresentation", "RecordingSupport"]
),
.testTarget(
name: "AgentDeviceSnapshotPresentationTests",
dependencies: ["AgentDeviceSnapshotPresentation"]
dependencies: ["AgentDeviceSnapshotPresentation", "RecordingSupport"]
),
]
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import CoreGraphics
import Foundation

/// A rect as a `contracts/fixtures/` table spells it. JSON has no infinity, so the two unusable
/// boxes are named: `{"infinite": true}` is `CGRect.infinite`, and `{"nonFinite": true}` is a
/// box whose components are infinite.
package struct FixtureRect: Decodable {
private enum CodingKeys: String, CodingKey {
case x, y, width, height, infinite, nonFinite
}

package let cgRect: CGRect

package init(from decoder: Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)
if try container.decodeIfPresent(Bool.self, forKey: .infinite) == true {
self.cgRect = .infinite
} else if try container.decodeIfPresent(Bool.self, forKey: .nonFinite) == true {
self.cgRect = CGRect(x: -.infinity, y: -.infinity, width: .infinity, height: .infinity)
} else {
self.cgRect = CGRect(
x: try container.decode(Double.self, forKey: .x),
y: try container.decode(Double.self, forKey: .y),
width: try container.decode(Double.self, forKey: .width),
height: try container.decode(Double.self, forKey: .height)
)
}
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import Foundation
import AgentDeviceSnapshotPresentation

package struct RawAlertRecording: Decodable {
package struct Node: Decodable {
package let index: Int
package let parentIndex: Int?
package let element: String
package let type: String
package let identifier: String
package let label: String
package let rect: FixtureRect
}

let format: Int
package let plain: [Node]
package let honorModal: [Node]

package static func load(_ file: URL) throws -> RawAlertRecording {
func invalid(_ detail: String) -> NSError {
NSError(domain: "alert-recording", code: 1, userInfo: [NSLocalizedDescriptionKey: "\(file.path): \(detail)"])
}
let recording: Self
do {
recording = try JSONDecoder().decode(Self.self, from: Data(contentsOf: file))
} catch let error as DecodingError {
let context: DecodingError.Context
switch error {
case let .keyNotFound(_, value), let .valueNotFound(_, value),
let .typeMismatch(_, value), let .dataCorrupted(value): context = value
@unknown default: throw error
}
let path = context.codingPath.map { $0.intValue.map { "node \($0)" } ?? $0.stringValue }.joined(separator: ".")
throw invalid("\(path): \(context.debugDescription)")
}
guard recording.format == 2 else { throw invalid("format \(recording.format) is not 2. Capture it again.") }
for (name, nodes) in [("plain", recording.plain), ("honorModal", recording.honorModal)] {
guard !nodes.isEmpty else { throw invalid("\(name): empty tree") }
for (position, node) in nodes.enumerated() {
guard node.index == position, node.parentIndex.map({ $0 >= 0 && $0 < position }) ?? (position == 0) else {
throw invalid("\(name) node \(position): invalid preorder index or parent")
}
}
}
return recording
}

package static func tree(_ nodes: [Node], at root: Int = 0) -> AlertSurfaceNode {
var children: [Int: [Int]] = [:]
for node in nodes {
if let parent = node.parentIndex { children[parent, default: []].append(node.index) }
}
func build(_ index: Int) -> AlertSurfaceNode {
let node = nodes[index]
return AlertSurfaceNode(
type: node.type,
identifier: node.identifier,
label: node.label,
frame: node.rect.cgRect,
children: (children[index] ?? []).map(build)
)
}
return build(root)
}

package static func modalScopeNodes(_ nodes: [Node]) -> [ModalScopeNode] {
nodes.map { node in
ModalScopeNode(
element: node.element,
parentElement: node.parentIndex.map { nodes[$0].element },
frame: node.rect.cgRect
)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import AgentDeviceSnapshotPresentation
import CoreGraphics
import Foundation

import RecordingSupport

func printAlertSurface(_ arguments: [String]) throws {
guard let file = arguments.first, arguments.count <= 2,
arguments.count == 1 || ["honorModal", "plain"].contains(arguments[1])
else {
throw NSError(domain: "print:alert-surface", code: 1, userInfo: [
NSLocalizedDescriptionKey: "Usage: pnpm print:alert-surface <recording.json> [honorModal|plain]"
])
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nit · "print:alert-surface" appears twice as an NSError domain and once as script-name text

NSError(domain: "print:alert-surface", code: 1, ...) is written out on both error sites (the usage error and the format error). A pnpm script name is not an error domain, and copying the literal plus the code: 1 on each site means a third error added later will most likely drift. Lift one usageError(_ message: String) -> NSError helper (or a shared domain constant) and call it from both throws.

Suggested change
}
private func corpusError(_ message: String) -> NSError {
NSError(domain: "print:alert-surface", code: 1, userInfo: [NSLocalizedDescriptionKey: message])
}
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.

let recording = try RawAlertRecording.load(URL(fileURLWithPath: file))
let plain = RawAlertRecording.modalScopeNodes(recording.plain)
let honorModal = RawAlertRecording.modalScopeNodes(recording.honorModal)
let reading = ModalSurface.read(
plain: plain, honorModal: honorModal, confirmingHonorModal: honorModal, truncated: false
)
let answer: String
var markers: [Int: String] = [:]
switch reading {
case let .modal(surfaces):
answer = "modal"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor · Reading line prints a module-qualified reflection description for unstable reasons and drops the node index

answer = "unknown(\(reason))" interpolates ModalSurfaceUnknownReason, which conforms only to Equatable (ModalSurface.swift:16-28). Payload-free cases print as the bare case name, so duplicatedChild, rootMismatch, unorderedRead and noSurvivingSurface match the unknown(<reason>) form README.md:46 and README.md:64-70 tell the contributor to read. But .unstable(ModalScopeInstability) has an associated value, so Swift's interpolation uses the mirror-based description and the line reads e.g. unknown(unstable(AgentDeviceSnapshotPresentation.ModalScopeInstability.movedElement)) instead of the unknown(unstable(movedElement)) the removed TS printer emitted (see its pairInstability, deleted in this PR).

Suggested change
answer = "modal"
case let .unknown(reason):
switch reason {
case let .unstable(instability): answer = "unknown(unstable(\(instability.rawValue)))"
default: answer = "unknown(\(reason))"
}
}

Suggested change

Print the reason in the stable form the README documents: switch over ModalSurfaceUnknownReason and write "unknown(\(name))" with unstable rendered as its ModalScopeInstability raw value (instability.rawValue), e.g. unknown(unstable(movedElement)). Either keep the node index/element id in the duplicatedChild line or state in the README how to find the offending node without it.

Details

The node index is also gone. The TS printer returned unknown(unstable(movedElement)): node 42 and unknown(duplicatedChild): node 7 kept 1 of 3 children with element id 99943:…, which the contributor used to locate the offending node in the printed tree and in the recording JSON. The new line names neither the node nor the element, while README.md:64-68 now only says to "Inspect repeated AX element ids under the same parent in the recording" — with no pointer to which parent. On a 248-node recording that is a search with no starting point.

Also raised here

Also raised at this location by senior-developer: unknown(...) printout drops the node index / element id the README tells contributors to search for — The old TS printer emitted the failing node (and, for duplicatedChild, the raw AX element id) in the first line: unknown(unstable(reparentedElement)): node 42 and unknown(duplicatedChild): node 5 kept 1 of 2 children with element id 99943:4829729792.1219. PrintAlertSurface.swift line 86 interpolates a ModalSurfaceUnknownReason with String(describing:), so the tool now prints only unknown(duplicatedChild) / unknown(unstable(reparentedElement)) / unknown(rootMismatch) — no node, no element id, no kept/listed counts. The README still routes the contributor through that missing data: it says "a node lists one element more than once … Inspect repeated AX element ids under the same parent in the recording" and warns not to write {"reading": "unknown", "reason": "duplicatedChild"} until those copies are confirmed. Without the offending node/element the contributor has to diff the two trees by hand in a tree with dozens of nodes, which is the work the command exists to save. …

persona: javascript, 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.

for surface in surfaces {
markers[surface.host] = " [surface roots: \(surface.roots.map(String.init).joined(separator: ", "))]"
}
case .noExclusion:
answer = "noExclusion"
case let .unknown(.unstable(instability)):
answer = "unknown(unstable(\(instability.rawValue)))"
case let .unknown(reason):
answer = "unknown(\(reason))"
Comment thread
thymikee marked this conversation as resolved.
Resolved
}
print("ModalSurface reads (the rule's answer, not ground truth): \(answer)")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nit · Tree choice reads arguments.last, so a recording named plain switches the default tree

let printPlain = arguments.last == "plain" picks the tree from the last argument, but the guard above only validates arguments[1] when there are two arguments. With a single argument, arguments.last is the file path, so pnpm print:alert-surface plain (a recording named plain in the working directory) silently prints the plain tree; the deleted script destructured [file, treeName = 'honorModal'], so a one-argument call always printed honorModal. Test the validated second argument instead of the last argument.

Suggested change
print("ModalSurface reads (the rule's answer, not ground truth): \(answer)")
let printPlain = arguments.count == 2 && arguments[1] == "plain"
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.

let printPlain = arguments.count == 2 && arguments[1] == "plain"
let nodes = printPlain ? recording.plain : recording.honorModal
var depths: [Int] = []
let encoder = JSONEncoder()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nit · Label is quoted by encoding a top-level String with JSONEncoder, which throws unless .fragmentsAllowed is set

let label = String(decoding: try encoder.encode(node.label), as: UTF8.self) encodes a bare String at the top level of a JSONEncoder. encoder.outputFormatting sets only .withoutEscapingSlashes; it does not set .fragmentsAllowed. Foundation's writer rejects a top-level JSON fragment ("Top-level String encoded as string JSON fragment. Consider adding 'fragmentsAllowed' to the encoder used to encode this value.") unless that option is present, so on a toolchain whose JSONEncoder still enforces it, the very first printed node throws, the error goes to stderr from main.swift and the command exits 1 for every recording — the print command prints nothing but the reading line. The removed TS predecessor used JSON.stringify(node.label), which has no such constraint, so this is a new failure mode on a line the diff adds. I could not run a Swift toolchain in this environment to settle the behaviour for the deployment toolchain (recent Foundation builds relaxed it), hence the lowered confidence; note that no test exercises printAlertSurface, so swift test (50 passed) would not catch it. The try on that line is also only there to satisfy this encode call — no other work in the print loop can throw — which is a hint the encoding was not meant to be fallible.

Suggested change
let encoder = JSONEncoder()
let encoder = JSONEncoder()
encoder.outputFormatting = [.withoutEscapingSlashes, .fragmentsAllowed]

Suggested change

Quote the label without a JSON encoder, or set the option the encode needs: either use encoder.outputFormatting = [.withoutEscapingSlashes, .fragmentsAllowed] (and keep try), or replace the encode with "\\\(node.label)\\" built from a small escaping helper, so the print path has no throwable step.

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.

encoder.outputFormatting = [.withoutEscapingSlashes]
for node in nodes {
let depth = node.parentIndex.map { depths[$0] + 1 } ?? 0
depths.append(depth)
let label = String(decoding: try encoder.encode(node.label), as: UTF8.self)
let index = String(repeating: " ", count: max(0, 4 - String(node.index).count)) + String(node.index)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nit · Hand-rolled padStart with String(repeating:) and count

let index = String(repeating: " ", count: max(0, 4 - String(node.index).count)) + String(node.index) re-implements right-padding by hand, and String(node.index) is computed twice on the line. The max(0, ...) guard is only there because String(repeating:count:) traps on a negative count, which a reader has to know to see why it is safe. String(format: "%4d", node.index) says the same thing in one call.

Suggested change
let index = String(repeating: " ", count: max(0, 4 - String(node.index).count)) + String(node.index)
let index = String(format: "%4d", node.index)
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.

let marker = printPlain ? "" : markers[node.index] ?? ""
print("\(index) \(String(repeating: " ", count: depth))\(node.type) \(label)\(marker)")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -144,12 +144,21 @@ private func reason(for failure: SnapshotPresentationFailure) -> String {
}
}

private let input = try JSONDecoder().decode(
BatchInput.self,
from: FileHandle.standardInput.readDataToEndOfFile()
)
private let output = try JSONEncoder().encode(
BatchOutput(cases: input.cases.map(present))
)
FileHandle.standardOutput.write(output)
FileHandle.standardOutput.write(Data([0x0a]))
if CommandLine.arguments.dropFirst().first == "print-alert-surface" {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nit · print-alert-surface literal is written in three places across two languages

The mode word appears as the dispatch comparison in main.swift and in the print:alert-surface usage string inside PrintAlertSurface.swift, plus the npm script. The usage text is also a pnpm invocation returned from a function that is reached through the raw swift run subcommand, so someone following it gets the pnpm wrapper's failure, not the tool's, when the package is not built. Keep one constant for the subcommand and phrase the usage line for the executable this file is compiled into.

Suggested change

Export one printAlertSurfaceCommand constant from PrintAlertSurface.swift, compare against it in main.swift, and make the usage line show the executable plus the subcommand.

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.

do {
try printAlertSurface(Array(CommandLine.arguments.dropFirst(2)))
} catch {
FileHandle.standardError.write(Data("\(error.localizedDescription)\n".utf8))
exit(1)
}
} else {
let input = try JSONDecoder().decode(
BatchInput.self,
from: FileHandle.standardInput.readDataToEndOfFile()
)
let output = try JSONEncoder().encode(
BatchOutput(cases: input.cases.map(present))
)
FileHandle.standardOutput.write(output)
FileHandle.standardOutput.write(Data([0x0a]))
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import RecordingSupport
import AgentDeviceSnapshotPresentation
import CoreGraphics
import Foundation
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import RecordingSupport
import AgentDeviceSnapshotPresentation
import CoreGraphics
import Foundation
Expand Down
Original file line number Diff line number Diff line change
@@ -1,32 +1,5 @@
import CoreGraphics
import Foundation

/// A rect as a `contracts/fixtures/` table spells it. JSON has no infinity, so the two unusable
/// boxes are named: `{"infinite": true}` is `CGRect.infinite`, and `{"nonFinite": true}` is a
/// box whose components are infinite.
struct FixtureRect: Decodable {
private enum CodingKeys: String, CodingKey {
case x, y, width, height, infinite, nonFinite
}

let cgRect: CGRect

init(from decoder: Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)
if try container.decodeIfPresent(Bool.self, forKey: .infinite) == true {
self.cgRect = .infinite
} else if try container.decodeIfPresent(Bool.self, forKey: .nonFinite) == true {
self.cgRect = CGRect(x: -.infinity, y: -.infinity, width: .infinity, height: .infinity)
} else {
self.cgRect = CGRect(
x: try container.decode(Double.self, forKey: .x),
y: try container.decode(Double.self, forKey: .y),
width: try container.decode(Double.self, forKey: .width),
height: try container.decode(Double.self, forKey: .height)
)
}
}
}
import RecordingSupport

func contractsFixtureURL(_ name: String, from filePath: String = #filePath) -> URL {
URL(fileURLWithPath: filePath)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import Foundation
import XCTest

final class PrintAlertSurfaceTests: XCTestCase {
func testPrintModeDispatchAndCorpusOutput() throws {
let package = URL(fileURLWithPath: #filePath)
.deletingLastPathComponent().deletingLastPathComponent().deletingLastPathComponent()
let process = Process()
process.executableURL = package.appendingPathComponent(".build/debug/snapshot-presentation-conformance")
process.arguments = [
"print-alert-surface",
package.appendingPathComponent("Tests/AlertSurfaceCorpus/iphone-duo-folded-safari-open-this-page-in-app.json").path,
]
let output = Pipe()
process.standardOutput = output
try process.run()
let text = String(decoding: output.fileHandleForReading.readDataToEndOfFile(), as: UTF8.self)
process.waitUntilExit()
XCTAssertEqual(process.terminationStatus, 0)
XCTAssertTrue(text.hasPrefix("ModalSurface reads (the rule's answer, not ground truth): modal\n"))
XCTAssertTrue(text.contains("[surface roots:"))
XCTAssertTrue(text.contains("Application"))
}
}
Original file line number Diff line number Diff line change
@@ -1,24 +1,14 @@
import CoreGraphics
import Foundation
@testable import AgentDeviceSnapshotPresentation
import RecordingSupport

/// Two trees of one screen recorded by `pnpm capture:alert-surface`, and what the rules must find in
/// them. Trees are never edited by hand. `expect` names, for each alert source, the one surface that
/// source must find in the honor-modal tree; a source it does not name must find nothing.
/// `expectModalScope` is what `ModalSurface` must read from the pair.
struct RecordedAlertSurface: Decodable {
static let format = 2

struct Node: Decodable {
let index: Int
let parentIndex: Int?
let element: String
let type: String
let identifier: String
let label: String
let rect: FixtureRect
}

struct RecordedAlertSurface {
typealias Node = RawAlertRecording.Node
struct Expectation: Decodable {
let title: String
let buttons: [String]
Expand All @@ -41,57 +31,30 @@ struct RecordedAlertSurface: Decodable {
let excludedActions: [String]?
}

let format: Int
let expect: [String: Expectation]
let expectModalScope: ModalScopeExpectation
let plain: [Node]
let honorModal: [Node]

static func load(_ file: URL) throws -> RecordedAlertSurface {
let recorded = try JSONDecoder().decode(RecordedAlertSurface.self, from: Data(contentsOf: file))
guard recorded.format == format, isPreorder(recorded.plain), isPreorder(recorded.honorModal) else {
throw CocoaError(.coderReadCorrupt)
}
return recorded
private struct Expectations: Decodable {
let expect: [String: Expectation]
let expectModalScope: ModalScopeExpectation
}

// Each node's index is its position, and its parent comes before it.
private static func isPreorder(_ nodes: [Node]) -> Bool {
!nodes.isEmpty && nodes.indices.allSatisfy { position in
let node = nodes[position]
guard node.index == position else { return false }
guard let parent = node.parentIndex else { return position == 0 }
return parent >= 0 && parent < position
}
private let recording: RawAlertRecording
private let expectations: Expectations
var expect: [String: Expectation] { expectations.expect }
var expectModalScope: ModalScopeExpectation { expectations.expectModalScope }
var plain: [Node] { recording.plain }
var honorModal: [Node] { recording.honorModal }

static func load(_ file: URL) throws -> RecordedAlertSurface {
RecordedAlertSurface(
recording: try RawAlertRecording.load(file),
expectations: try JSONDecoder().decode(Expectations.self, from: Data(contentsOf: file))
)
}

static func tree(_ nodes: [Node], at root: Int = 0) -> AlertSurfaceNode {
var children: [Int: [Int]] = [:]
for node in nodes {
if let parent = node.parentIndex { children[parent, default: []].append(node.index) }
}
func build(_ index: Int) -> AlertSurfaceNode {
let node = nodes[index]
return AlertSurfaceNode(
type: node.type,
identifier: node.identifier,
label: node.label,
frame: node.rect.cgRect,
children: (children[index] ?? []).map(build)
)
}
return build(root)
RawAlertRecording.tree(nodes, at: root)
}
static let modalScopeNodes = RawAlertRecording.modalScopeNodes

static func modalScopeNodes(_ nodes: [Node]) -> [ModalScopeNode] {
nodes.map { node in
ModalScopeNode(
element: node.element,
parentElement: node.parentIndex.map { nodes[$0].element },
frame: node.rect.cgRect
)
}
}
}

func recordedAlertSurfaces() throws -> [(name: String, recorded: RecordedAlertSurface)] {
Expand Down
Loading
Loading