Refactor #1
Refactor #1
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds comprehensive accessibility API mappings, new utility extensions for element traversal and inspection, introduces a code generation tool for AX constants, and refactors observer notification handling to support Cancellable protocol conformance and publisher-based patterns. Updates Swift tools version from 5.3 to 5.11 and adds swift-argument-parser dependency. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Plugin as GenerateAXConstantsPlugin
participant Tool as AXConstantsGenerator
participant SDK as macOS SDK Headers
participant Output as Swift Source Files
User->>Plugin: swift package generate-ax-constants
Plugin->>Tool: Launch subprocess with output directory
Tool->>SDK: Discover SDK path & locate HIServices headers
Tool->>SDK: Read and parse header files
Tool->>Tool: Extract constants, groups, and documentation
Tool->>Output: Generate Swift enums/extensions with constants
Tool->>Plugin: Exit with status code
Plugin->>User: Report success or error
sequenceDiagram
participant Client
participant Observer as Accessibility.Observer
participant Token as Token (Cancellable)
participant Callback as Callback Handler
Client->>Observer: observe(notification, for element, callback)
Observer->>Observer: Register AX notification listener
Observer->>Token: Create with removeAction closure
Note over Token: Token conforms to Cancellable
Token-->>Client: Return Token
Note over Observer: AX notification fires
Observer->>Callback: Invoke callback with AXUIElement
Callback->>Callback: Handle notification
Client->>Token: cancel() or deinit
Token->>Observer: Execute removeAction
Observer->>Observer: Unregister listener
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@Package.swift`:
- Line 1: Replace the invalid toolchain declaration "//
swift-tools-version:5.11" in Package.swift with a supported version string —
e.g., "// swift-tools-version:5.10" or "// swift-tools-version:6.0" depending on
your minimum Xcode/toolchain requirement — so the Package.swift header uses a
real SwiftPM toolchain identifier.
In `@Sources/axdump/Commands/ActionCommand.swift`:
- Around line 148-217: The parser parseCustomAction currently uses
dropFirst(...) which leaves leading/trailing whitespace and can cause mismatches
in performCustomAction when comparing parsed.name; update parseCustomAction so
that after extracting each field (name, target, selector) you call
.trimmingCharacters(in: .whitespacesAndNewlines) to remove surrounding
whitespace/newlines; keep the rest of the parsing logic the same so
performCustomAction's comparison of parsed.name.lowercased() works reliably.
In `@Sources/axdump/Commands/FindCommand.swift`:
- Around line 211-257: In performCustomAction and parseCustomAction, trim
whitespace around the parsed "Name:" value before using it for comparisons or
printing: update parseCustomAction(_:) to return the name trimmed (use
trimmingCharacters(in: .whitespacesAndNewlines)), and in performCustomAction use
that trimmed, lowercased value when comparing to the requested name and when
listing available actions so matches no longer fail due to leading spaces.
In `@Sources/axdump/Commands/InspectCommand.swift`:
- Around line 63-115: When parsing the attributes string (attrString) into
attrNames before calling inspectAttributes, filter out any empty tokens produced
by trailing commas or consecutive commas so you never send an empty name (which
becomes "AX") to the inspector; change the attrNames construction in
InspectCommand.run() to trim tokens and drop those where the trimmed string is
empty, then pass the filtered array to inspectAttributes(of:names:printer:).
In `@Sources/axdump/Commands/QueryCommand.swift`:
- Around line 49-75: The CLI noColor flag (var noColor) is defined but not used
when creating ElementPrinter, so the flag has no effect; update the
ElementPrinter usage in run() to pass the noColor value (or add a parameter like
disableColor/colored: Bool to ElementPrinter initializer) and modify
ElementPrinter (constructor and internal printing logic) to respect that
parameter and disable ANSI/color output when noColor is true; ensure any calls
to ElementPrinter(fields:attributeFields, verbosity:) are updated to the new
initializer signature (e.g., ElementPrinter(fields:attributeFields,
verbosity:verbosity, noColor: noColor)) and tests/usage updated accordingly.
In `@Sources/axdump/Commands/ScreenshotCommand.swift`:
- Around line 96-123: The code uses getElementFrame(windowElement) to set
windowFrame and later calls drawBoundingBoxes which scales boxes using
windowFrame; when getElementFrame returns nil (windowFrame == .zero) that causes
division-by-zero in scaling, so update the ScreenshotCommand flow to detect a
missing frame: if boundingBox is non-empty and getElementFrame(windowElement)
returned nil, either skip drawing boxes or early-return with an error before
calling drawBoundingBoxes; modify the logic around windowFrame/boundingBox so
drawBoundingBoxes is only invoked when a valid frame exists (check
getElementFrame result instead of assuming .zero) and handle the alternative
path (log/throw or continue without boxes) accordingly.
In `@Sources/axdump/Commands/WatchCommand.swift`:
- Around line 17-23: The examples text includes an undefined flag "--click";
update the WatchCommand implementation so examples and flags match: either
remove "--click" from the examples string or add a matching flag definition in
the WatchCommand (e.g., declare a Bool flag/property named click or register a
Flag("click") in the WatchCommand argument/option parsing) and wire it into the
command logic so the flag is recognized; update the examples block (the
multiline string in WatchCommand) to reflect the actual flag name and behavior.
- Around line 44-115: The coordinate conversion is using NSScreen.main which can
be the focused screen, not the one under the cursor, causing wrong Y-flipping in
run(); replace the NSScreen.main lookup with code that finds the screen
containing the mouse (e.g., search NSScreen.screens for the screen whose frame
contains NSEvent.mouseLocation or use NSMouseInRect for boundary checks), then
use that screen's frame.height/origin to flip Y before calling
Accessibility.Element.systemWide.hitTest(x:y:); update the place where
screenHeight is computed and any dependent logic so hitTest uses coordinates
corrected for the actual screen under the cursor.
In `@Sources/axdump/Utilities/ElementPrinter.swift`:
- Around line 369-412: navigateToPath currently uses compactMap on path
components so non-integer parts like "1.a.2" are dropped; change parsing to
validate each component and throw a new NavigationError case for invalid
components instead of silently ignoring them. Add a NavigationError case such as
invalidComponent(component: String, step: Int) (with a suitable description) and
replace let indices = path.split(...).compactMap { Int($0) } with iterating over
path.split(separator: ".").enumerated(), attempting Int(String(component)) for
each component and throwing NavigationError.invalidComponent(component:
String(component), step: step) on failure, then proceed with the existing child
lookup logic in navigateToPath.
🧹 Nitpick comments (11)
Sources/AccessibilityControl/NSRunningApplication+Accessibility.swift (2)
1-3: Redundant import:CocoaincludesAppKit.Importing both
AppKitandCocoais redundant sinceCocoaalready includesAppKit. Consider removing theAppKitimport.♻️ Suggested fix
-import AppKit import WindowControl import Cocoa
6-12: Consider the underscore prefix naming convention.The leading underscore on
_accessibilityElementand_accessibilityWindowtypically indicates private/internal API in Swift. If these are intended for public use, consider removing the underscore. If they're experimental/internal, document this intent.Also,
try?on line 11 silently swallows errors fromwindow(), which may hide accessibility permission issues or invalid element states during debugging.Sources/AccessibilityControl/Observer.swift (1)
126-135: Potential observer lifetime issue in publisher convenience.The
Observerinstance created on line 131 is not directly retained - it's only kept alive through the closure captured in theToken. While the existing code comment (line 100-101) indicates this is intentional, consider documenting this retention behavior in thepublishermethod or adding a comment for maintainability.Sources/axdump/Utilities/TreePrinter.swift (1)
163-164: Consider extracting a truncation helper to reduce duplication.The truncation logic is repeated multiple times with slightly different lengths (40 for title, 30 for value/description/help). A small helper function would improve consistency and maintainability. Note that
ElementPrinteruses 50 characters for truncation—consider aligning these values for consistency across utilities.♻️ Suggested helper
private func truncate(_ text: String, maxLength: Int = 40) -> String { text.count > maxLength ? String(text.prefix(maxLength)) + "..." : text }Also applies to: 186-187, 194-195, 233-234
Sources/axdump/Commands/MenuCommand.swift (2)
155-162: Prefix matching may cause unintended matches.The matching logic uses
hasPrefixwhich could match "File" to "Files" or "Filter". Consider using exact match first, then falling back to prefix match only if exact match fails, or documenting this behavior.♻️ Suggested improvement for more precise matching
for child in children { let childTitle = (try? child.attribute(AXAttribute.title)()) ?? "" - if childTitle.lowercased() == name.lowercased() || - childTitle.lowercased().hasPrefix(name.lowercased()) { + // Exact match takes priority + if childTitle.lowercased() == name.lowercased() { found = child break } } + + // Fallback to prefix match if no exact match + if found == nil { + for child in children { + let childTitle = (try? child.attribute(AXAttribute.title)()) ?? "" + if childTitle.lowercased().hasPrefix(name.lowercased()) { + found = child + break + } + } + }
392-411: Simplified modifier handling may miss combined modifiers.The if-else chain doesn't handle combined modifiers (e.g.,
⌘⇧⌥). The comment acknowledges this complexity. If more accurate shortcut display is needed in the future, consider building the string additively.Sources/axdump/Utilities/ElementFilter.swift (1)
148-157: Consider documenting the AXChildren semantic difference.The AXChildren check uses
count > 0rather than checking attribute existence, which means "has children" semantically means "has at least one child." This is reasonable but differs from other fields where mere existence is checked. Consider a comment clarifying this intentional semantic difference.Sources/axdump/Commands/SetCommand.swift (2)
79-85: Child and path navigation can be combined—consider documenting order.When both
--childand--pathare provided, child navigation is applied first, then path navigation continues from that child. This is reasonable but the interaction might surprise users. Consider a note in the command discussion.
135-151: Type inference for AXValue may be surprising for boolean-like values.For
AXValue, the value "1" will be parsed asInt(1)rather thanBool(true). If a user intends to set a boolean value using "1", they should use "true" instead. This is reasonable but may warrant a note in the documentation about using "true"/"false" explicitly for boolean values.Sources/axdump/Commands/DumpCommand.swift (1)
179-192: Consider extracting shared descendant-matching logic.The
childHasMatchingDescendantmethod mirrorsTreePrinter.childPassesFilterOrHasMatchingDescendant. If filtering is used in more places, consider extracting this toElementFilterto avoid duplication.Sources/axdump/Commands/CompareCommand.swift (1)
261-283: Consider reusing shared frame/color helpers.
getElementFrameandparseColorduplicate logic fromSources/axdump/Commands/DumpCommand.swift; centralizing would reduce drift.
| @@ -1,15 +1,22 @@ | |||
| // swift-tools-version:5.3 | |||
| // swift-tools-version:5.11 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Swift 5.11 release date tools version availability
💡 Result:
- Swift 5.11 “release date” (stable): There was no general-availability Swift 5.11 release. The last Swift 5.x major release was Swift 5.10 (March 5, 2024), and the next major release was Swift 6 (September 17, 2024). [1][2]
- Where “Swift 5.11” existed: “5.11” showed up as a development-snapshot/toolchain identifier during the period after Swift 5.10 and before Swift 6 (e.g., Swift Forums discussions referencing “swift-DEVELOPMENT-SNAPSHOT … (which should map to 5.11)”). [3]
- Xcode / toolchain availability: Apple’s Xcode line moved from Swift 5.10-era toolchains to Swift 6.0 in Xcode 16 (i.e., not Swift 5.11). [2][4]
- SwiftPM
swift-tools-version: 5.11: If you’re asking about Swift Package Manager tools version 5.11, it’s not tied to a stable “Swift 5.11” compiler release, and using newer tools versions inside Xcode can be confusing because Xcode uses its own embedded SwiftPM, which may not match an installed custom toolchain. [3]
Sources:
[1] Swift.org — Swift 5.10 Released (Mar 5, 2024) (swift.org)
[2] Swift.org — Announcing Swift 6 (Sep 17, 2024) (swift.org)
[3] Swift Forums — discussion of 5.10/“5.11” development snapshots & Xcode SwiftPM behavior (Feb 2024) (forums.swift.org)
[4] Apple Developer — Xcode releases table showing Swift 6.0 with Xcode 16 (developer.apple.com)
Citations:
- 1: https://www.swift.org/blog/swift-5.10-released/?utm_source=openai
- 2: https://www.swift.org/blog/announcing-swift-6/?utm_source=openai
- 3: https://forums.swift.org/t/are-5-10-and-5-11-toolchain-builds-falling-back-to-5-9-when-building-a-package-from-xcode/70073?utm_source=openai
- 4: https://developer.apple.com/support/xcode/?utm_source=openai
Remove swift-tools-version:5.11—this version does not exist.
Swift 5.11 was never released. Swift 5.10 (March 2024) was the final Swift 5.x release; the next version is Swift 6.0 (September 2024). "5.11" appeared only as a development snapshot identifier and is not available in any production Xcode or SwiftPM toolchain. Change to swift-tools-version:5.10 or swift-tools-version:6.0 depending on your minimum Xcode requirement.
🤖 Prompt for AI Agents
In `@Package.swift` at line 1, Replace the invalid toolchain declaration "//
swift-tools-version:5.11" in Package.swift with a supported version string —
e.g., "// swift-tools-version:5.10" or "// swift-tools-version:6.0" depending on
your minimum Xcode/toolchain requirement — so the Package.swift header uses a
real SwiftPM toolchain identifier.
| private func performCustomAction(_ name: String, on element: Accessibility.Element) throws { | ||
| // Get all actions and find matching custom action | ||
| guard let actions = try? element.supportedActions() else { | ||
| print("Error: Could not read actions for element") | ||
| throw ExitCode.failure | ||
| } | ||
|
|
||
| // Find custom action matching the name | ||
| // Custom actions have format: "Name:...\nTarget:...\nSelector:..." | ||
| var matchingAction: Accessibility.Action? | ||
| for action in actions { | ||
| let actionName = action.name.value | ||
| if actionName.hasPrefix("Name:") { | ||
| // Parse custom action name | ||
| let parsed = parseCustomAction(actionName) | ||
| if parsed.name.lowercased() == name.lowercased() { | ||
| matchingAction = action | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| guard let action = matchingAction else { | ||
| print("Error: Custom action '\(name)' not found") | ||
| print() | ||
| print("Available custom actions:") | ||
| for action in actions { | ||
| let actionName = action.name.value | ||
| if actionName.hasPrefix("Name:") { | ||
| let parsed = parseCustomAction(actionName) | ||
| print(" - \(parsed.name)") | ||
| } | ||
| } | ||
| throw ExitCode.failure | ||
| } | ||
|
|
||
| // Perform the action | ||
| do { | ||
| try action() | ||
| print("Custom action performed: \(name)") | ||
|
|
||
| if verbose { | ||
| print() | ||
| print("Element state after action:") | ||
| printElementInfo(element) | ||
| } | ||
| } catch { | ||
| print("Error: Failed to perform custom action '\(name)': \(error)") | ||
| throw ExitCode.failure | ||
| } | ||
| } | ||
|
|
||
| private func parseCustomAction(_ raw: String) -> (name: String, target: String?, selector: String?) { | ||
| var name = "" | ||
| var target: String? | ||
| var selector: String? | ||
|
|
||
| for line in raw.split(separator: "\n", omittingEmptySubsequences: false) { | ||
| let lineStr = String(line) | ||
| if lineStr.hasPrefix("Name:") { | ||
| name = String(lineStr.dropFirst(5)) | ||
| } else if lineStr.hasPrefix("Target:") { | ||
| target = String(lineStr.dropFirst(7)) | ||
| } else if lineStr.hasPrefix("Selector:") { | ||
| selector = String(lineStr.dropFirst(9)) | ||
| } | ||
| } | ||
|
|
||
| return (name, target, selector) | ||
| } |
There was a problem hiding this comment.
Trim custom action fields to avoid whitespace mismatches
dropFirst leaves leading spaces (e.g., "Name: Heart" → " Heart"), so matching can fail. Trim whitespace for name/target/selector during parsing.
🧹 Suggested fix
- name = String(lineStr.dropFirst(5))
+ name = String(lineStr.dropFirst(5)).trimmingCharacters(in: .whitespaces)
@@
- target = String(lineStr.dropFirst(7))
+ target = String(lineStr.dropFirst(7)).trimmingCharacters(in: .whitespaces)
@@
- selector = String(lineStr.dropFirst(9))
+ selector = String(lineStr.dropFirst(9)).trimmingCharacters(in: .whitespaces)🤖 Prompt for AI Agents
In `@Sources/axdump/Commands/ActionCommand.swift` around lines 148 - 217, The
parser parseCustomAction currently uses dropFirst(...) which leaves
leading/trailing whitespace and can cause mismatches in performCustomAction when
comparing parsed.name; update parseCustomAction so that after extracting each
field (name, target, selector) you call .trimmingCharacters(in:
.whitespacesAndNewlines) to remove surrounding whitespace/newlines; keep the
rest of the parsing logic the same so performCustomAction's comparison of
parsed.name.lowercased() works reliably.
| private func performCustomAction(_ name: String, on element: Accessibility.Element) throws { | ||
| guard let actions = try? element.supportedActions() else { | ||
| print("→ Cannot read actions for element") | ||
| throw ExitCode.failure | ||
| } | ||
|
|
||
| // Find custom action matching the name | ||
| // Custom actions have format: "Name:...\nTarget:...\nSelector:..." | ||
| var matchingAction: Accessibility.Action? | ||
| for action in actions { | ||
| let actionName = action.name.value | ||
| if actionName.hasPrefix("Name:") { | ||
| let parsed = parseCustomAction(actionName) | ||
| if parsed.lowercased() == name.lowercased() { | ||
| matchingAction = action | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| guard let action = matchingAction else { | ||
| print("→ Custom action '\(name)' not found") | ||
| print() | ||
| print("Available custom actions:") | ||
| for action in actions { | ||
| let actionName = action.name.value | ||
| if actionName.hasPrefix("Name:") { | ||
| let parsed = parseCustomAction(actionName) | ||
| print(" - \(parsed)") | ||
| } | ||
| } | ||
| throw ExitCode.failure | ||
| } | ||
|
|
||
| try action() | ||
| print("→ Custom action: \(name)") | ||
| } | ||
|
|
||
| private func parseCustomAction(_ raw: String) -> String { | ||
| for line in raw.split(separator: "\n", omittingEmptySubsequences: false) { | ||
| let lineStr = String(line) | ||
| if lineStr.hasPrefix("Name:") { | ||
| return String(lineStr.dropFirst(5)) | ||
| } | ||
| } | ||
| return raw | ||
| } |
There was a problem hiding this comment.
Trim custom action names before matching
Name: lines commonly include a leading space; without trimming, matches can fail. Trim the parsed name (and keep it trimmed when listing).
🧹 Suggested fix
- return String(lineStr.dropFirst(5))
+ return String(lineStr.dropFirst(5)).trimmingCharacters(in: .whitespaces)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private func performCustomAction(_ name: String, on element: Accessibility.Element) throws { | |
| guard let actions = try? element.supportedActions() else { | |
| print("→ Cannot read actions for element") | |
| throw ExitCode.failure | |
| } | |
| // Find custom action matching the name | |
| // Custom actions have format: "Name:...\nTarget:...\nSelector:..." | |
| var matchingAction: Accessibility.Action? | |
| for action in actions { | |
| let actionName = action.name.value | |
| if actionName.hasPrefix("Name:") { | |
| let parsed = parseCustomAction(actionName) | |
| if parsed.lowercased() == name.lowercased() { | |
| matchingAction = action | |
| break | |
| } | |
| } | |
| } | |
| guard let action = matchingAction else { | |
| print("→ Custom action '\(name)' not found") | |
| print() | |
| print("Available custom actions:") | |
| for action in actions { | |
| let actionName = action.name.value | |
| if actionName.hasPrefix("Name:") { | |
| let parsed = parseCustomAction(actionName) | |
| print(" - \(parsed)") | |
| } | |
| } | |
| throw ExitCode.failure | |
| } | |
| try action() | |
| print("→ Custom action: \(name)") | |
| } | |
| private func parseCustomAction(_ raw: String) -> String { | |
| for line in raw.split(separator: "\n", omittingEmptySubsequences: false) { | |
| let lineStr = String(line) | |
| if lineStr.hasPrefix("Name:") { | |
| return String(lineStr.dropFirst(5)) | |
| } | |
| } | |
| return raw | |
| } | |
| private func performCustomAction(_ name: String, on element: Accessibility.Element) throws { | |
| guard let actions = try? element.supportedActions() else { | |
| print("→ Cannot read actions for element") | |
| throw ExitCode.failure | |
| } | |
| // Find custom action matching the name | |
| // Custom actions have format: "Name:...\nTarget:...\nSelector:..." | |
| var matchingAction: Accessibility.Action? | |
| for action in actions { | |
| let actionName = action.name.value | |
| if actionName.hasPrefix("Name:") { | |
| let parsed = parseCustomAction(actionName) | |
| if parsed.lowercased() == name.lowercased() { | |
| matchingAction = action | |
| break | |
| } | |
| } | |
| } | |
| guard let action = matchingAction else { | |
| print("→ Custom action '\(name)' not found") | |
| print() | |
| print("Available custom actions:") | |
| for action in actions { | |
| let actionName = action.name.value | |
| if actionName.hasPrefix("Name:") { | |
| let parsed = parseCustomAction(actionName) | |
| print(" - \(parsed)") | |
| } | |
| } | |
| throw ExitCode.failure | |
| } | |
| try action() | |
| print("→ Custom action: \(name)") | |
| } | |
| private func parseCustomAction(_ raw: String) -> String { | |
| for line in raw.split(separator: "\n", omittingEmptySubsequences: false) { | |
| let lineStr = String(line) | |
| if lineStr.hasPrefix("Name:") { | |
| return String(lineStr.dropFirst(5)).trimmingCharacters(in: .whitespaces) | |
| } | |
| } | |
| return raw | |
| } |
🤖 Prompt for AI Agents
In `@Sources/axdump/Commands/FindCommand.swift` around lines 211 - 257, In
performCustomAction and parseCustomAction, trim whitespace around the parsed
"Name:" value before using it for comparisons or printing: update
parseCustomAction(_:) to return the name trimmed (use trimmingCharacters(in:
.whitespacesAndNewlines)), and in performCustomAction use that trimmed,
lowercased value when comparing to the requested name and when listing available
actions so matches no longer fail due to leading spaces.
| func run() throws { | ||
| guard Accessibility.isTrusted(shouldPrompt: true) else { | ||
| print("Error: Accessibility permissions required") | ||
| throw ExitCode.failure | ||
| } | ||
|
|
||
| let appElement = Accessibility.Element(pid: pid) | ||
|
|
||
| // Determine starting element | ||
| var targetElement: Accessibility.Element = appElement | ||
|
|
||
| if focused { | ||
| guard let focusedElement: Accessibility.Element = try? appElement.attribute(.init("AXFocusedUIElement"))() else { | ||
| print("Error: Could not get focused element for PID \(pid)") | ||
| throw ExitCode.failure | ||
| } | ||
| targetElement = focusedElement | ||
| } else if window { | ||
| guard let focusedWindow: Accessibility.Element = try? appElement.attribute(.init("AXFocusedWindow"))() else { | ||
| print("Error: Could not get focused window for PID \(pid)") | ||
| throw ExitCode.failure | ||
| } | ||
| targetElement = focusedWindow | ||
| } | ||
|
|
||
| // Navigate to child if specified | ||
| if let childIndex = child { | ||
| targetElement = try navigateToChild(from: targetElement, index: childIndex) | ||
| } | ||
|
|
||
| // Navigate via path if specified | ||
| if let pathString = path { | ||
| targetElement = try navigateToPath(from: targetElement, path: pathString) | ||
| } | ||
|
|
||
| // Show element info | ||
| printElementHeader(targetElement) | ||
|
|
||
| let printer = ElementPrinter(maxLength: maxLength) | ||
|
|
||
| // Handle attribute inspection | ||
| if let attrString = attributes { | ||
| if attrString.lowercased() == "list" { | ||
| listAttributes(of: targetElement) | ||
| } else { | ||
| let attrNames = attrString.split(separator: ",").map { String($0).trimmingCharacters(in: .whitespaces) } | ||
| inspectAttributes(of: targetElement, names: attrNames, printer: printer) | ||
| } | ||
| } else { | ||
| // Default: show all attributes with full values | ||
| inspectAllAttributes(of: targetElement, printer: printer) | ||
| } | ||
| } |
There was a problem hiding this comment.
Filter empty attribute tokens to avoid querying "AX".
Trailing commas or empty segments can produce empty names, which then become "AX" and yield confusing output.
🧹 Suggested fix
- let attrNames = attrString.split(separator: ",").map { String($0).trimmingCharacters(in: .whitespaces) }
+ let attrNames = attrString
+ .split(separator: ",")
+ .map { String($0).trimmingCharacters(in: .whitespaces) }
+ .filter { !$0.isEmpty }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func run() throws { | |
| guard Accessibility.isTrusted(shouldPrompt: true) else { | |
| print("Error: Accessibility permissions required") | |
| throw ExitCode.failure | |
| } | |
| let appElement = Accessibility.Element(pid: pid) | |
| // Determine starting element | |
| var targetElement: Accessibility.Element = appElement | |
| if focused { | |
| guard let focusedElement: Accessibility.Element = try? appElement.attribute(.init("AXFocusedUIElement"))() else { | |
| print("Error: Could not get focused element for PID \(pid)") | |
| throw ExitCode.failure | |
| } | |
| targetElement = focusedElement | |
| } else if window { | |
| guard let focusedWindow: Accessibility.Element = try? appElement.attribute(.init("AXFocusedWindow"))() else { | |
| print("Error: Could not get focused window for PID \(pid)") | |
| throw ExitCode.failure | |
| } | |
| targetElement = focusedWindow | |
| } | |
| // Navigate to child if specified | |
| if let childIndex = child { | |
| targetElement = try navigateToChild(from: targetElement, index: childIndex) | |
| } | |
| // Navigate via path if specified | |
| if let pathString = path { | |
| targetElement = try navigateToPath(from: targetElement, path: pathString) | |
| } | |
| // Show element info | |
| printElementHeader(targetElement) | |
| let printer = ElementPrinter(maxLength: maxLength) | |
| // Handle attribute inspection | |
| if let attrString = attributes { | |
| if attrString.lowercased() == "list" { | |
| listAttributes(of: targetElement) | |
| } else { | |
| let attrNames = attrString.split(separator: ",").map { String($0).trimmingCharacters(in: .whitespaces) } | |
| inspectAttributes(of: targetElement, names: attrNames, printer: printer) | |
| } | |
| } else { | |
| // Default: show all attributes with full values | |
| inspectAllAttributes(of: targetElement, printer: printer) | |
| } | |
| } | |
| func run() throws { | |
| guard Accessibility.isTrusted(shouldPrompt: true) else { | |
| print("Error: Accessibility permissions required") | |
| throw ExitCode.failure | |
| } | |
| let appElement = Accessibility.Element(pid: pid) | |
| // Determine starting element | |
| var targetElement: Accessibility.Element = appElement | |
| if focused { | |
| guard let focusedElement: Accessibility.Element = try? appElement.attribute(.init("AXFocusedUIElement"))() else { | |
| print("Error: Could not get focused element for PID \(pid)") | |
| throw ExitCode.failure | |
| } | |
| targetElement = focusedElement | |
| } else if window { | |
| guard let focusedWindow: Accessibility.Element = try? appElement.attribute(.init("AXFocusedWindow"))() else { | |
| print("Error: Could not get focused window for PID \(pid)") | |
| throw ExitCode.failure | |
| } | |
| targetElement = focusedWindow | |
| } | |
| // Navigate to child if specified | |
| if let childIndex = child { | |
| targetElement = try navigateToChild(from: targetElement, index: childIndex) | |
| } | |
| // Navigate via path if specified | |
| if let pathString = path { | |
| targetElement = try navigateToPath(from: targetElement, path: pathString) | |
| } | |
| // Show element info | |
| printElementHeader(targetElement) | |
| let printer = ElementPrinter(maxLength: maxLength) | |
| // Handle attribute inspection | |
| if let attrString = attributes { | |
| if attrString.lowercased() == "list" { | |
| listAttributes(of: targetElement) | |
| } else { | |
| let attrNames = attrString | |
| .split(separator: ",") | |
| .map { String($0).trimmingCharacters(in: .whitespaces) } | |
| .filter { !$0.isEmpty } | |
| inspectAttributes(of: targetElement, names: attrNames, printer: printer) | |
| } | |
| } else { | |
| // Default: show all attributes with full values | |
| inspectAllAttributes(of: targetElement, printer: printer) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@Sources/axdump/Commands/InspectCommand.swift` around lines 63 - 115, When
parsing the attributes string (attrString) into attrNames before calling
inspectAttributes, filter out any empty tokens produced by trailing commas or
consecutive commas so you never send an empty name (which becomes "AX") to the
inspector; change the attrNames construction in InspectCommand.run() to trim
tokens and drop those where the trimmed string is empty, then pass the filtered
array to inspectAttributes(of:names:printer:).
| @Flag(name: .long, help: "Disable colored output") | ||
| var noColor: Bool = false | ||
|
|
||
| func run() throws { | ||
| guard Accessibility.isTrusted(shouldPrompt: true) else { | ||
| print("Error: Accessibility permissions required") | ||
| throw ExitCode.failure | ||
| } | ||
|
|
||
| let appElement = Accessibility.Element(pid: pid) | ||
|
|
||
| var targetElement: Accessibility.Element = appElement | ||
|
|
||
| if focused { | ||
| guard let focusedElement: Accessibility.Element = try? appElement.attribute(.init("AXFocusedUIElement"))() else { | ||
| print("Error: Could not get focused element for PID \(pid)") | ||
| throw ExitCode.failure | ||
| } | ||
| targetElement = focusedElement | ||
| } | ||
|
|
||
| if let pathString = path { | ||
| targetElement = try navigateToPath(from: targetElement, path: pathString) | ||
| } | ||
|
|
||
| let attributeFields = AttributeFields.parse(fields) | ||
| let printer = ElementPrinter(fields: attributeFields, verbosity: verbosity) |
There was a problem hiding this comment.
--no-color flag is currently ignored
Line 49 defines noColor, but Line 75 constructs ElementPrinter without using it, so the flag has no effect. Wire it through (and ensure the printer respects it) or drop the option to avoid misleading CLI behavior.
💡 Suggested wiring
- let printer = ElementPrinter(fields: attributeFields, verbosity: verbosity)
+ let printer = ElementPrinter(fields: attributeFields, verbosity: verbosity, useColor: !noColor)🤖 Prompt for AI Agents
In `@Sources/axdump/Commands/QueryCommand.swift` around lines 49 - 75, The CLI
noColor flag (var noColor) is defined but not used when creating ElementPrinter,
so the flag has no effect; update the ElementPrinter usage in run() to pass the
noColor value (or add a parameter like disableColor/colored: Bool to
ElementPrinter initializer) and modify ElementPrinter (constructor and internal
printing logic) to respect that parameter and disable ANSI/color output when
noColor is true; ensure any calls to ElementPrinter(fields:attributeFields,
verbosity:) are updated to the new initializer signature (e.g.,
ElementPrinter(fields:attributeFields, verbosity:verbosity, noColor: noColor))
and tests/usage updated accordingly.
| // Get window bounds for bounding box calculations | ||
| let windowFrame: CGRect = getElementFrame(windowElement) ?? .zero | ||
|
|
||
| // Capture the window | ||
| var imageOptions: CGWindowImageOption = [.boundsIgnoreFraming] | ||
| if shadow { | ||
| imageOptions = [] | ||
| } | ||
|
|
||
| guard let cgImage = CGWindowListCreateImage( | ||
| .null, | ||
| .optionIncludingWindow, | ||
| window.raw, | ||
| imageOptions | ||
| ) else { | ||
| print("Error: Failed to capture window image") | ||
| throw ExitCode.failure | ||
| } | ||
|
|
||
| // Draw bounding boxes if requested | ||
| let finalImage: CGImage | ||
| if !boundingBox.isEmpty { | ||
| finalImage = try drawBoundingBoxes( | ||
| on: cgImage, | ||
| windowElement: windowElement, | ||
| windowFrame: windowFrame, | ||
| paths: boundingBox | ||
| ) |
There was a problem hiding this comment.
Guard against missing window frames before scaling bounding boxes
When getElementFrame returns nil, windowFrame becomes .zero, and later scaling divides by zero. If bounding boxes are requested, bail out (or skip boxes) when the window frame is unavailable.
🛡️ Suggested fix
- let windowFrame: CGRect = getElementFrame(windowElement) ?? .zero
+ let windowFrame = getElementFrame(windowElement)
@@
- if !boundingBox.isEmpty {
- finalImage = try drawBoundingBoxes(
+ if !boundingBox.isEmpty {
+ guard let windowFrame, windowFrame.width > 0, windowFrame.height > 0 else {
+ print("Error: Could not determine window frame for bounding boxes")
+ throw ExitCode.failure
+ }
+ finalImage = try drawBoundingBoxes(
on: cgImage,
windowElement: windowElement,
windowFrame: windowFrame,
paths: boundingBox
)
} else {
finalImage = cgImage
}🤖 Prompt for AI Agents
In `@Sources/axdump/Commands/ScreenshotCommand.swift` around lines 96 - 123, The
code uses getElementFrame(windowElement) to set windowFrame and later calls
drawBoundingBoxes which scales boxes using windowFrame; when getElementFrame
returns nil (windowFrame == .zero) that causes division-by-zero in scaling, so
update the ScreenshotCommand flow to detect a missing frame: if boundingBox is
non-empty and getElementFrame(windowElement) returned nil, either skip drawing
boxes or early-return with an error before calling drawBoundingBoxes; modify the
logic around windowFrame/boundingBox so drawBoundingBoxes is only invoked when a
valid frame exists (check getElementFrame result instead of assuming .zero) and
handle the alternative path (log/throw or continue without boxes) accordingly.
| func run() throws { | ||
| guard Accessibility.isTrusted(shouldPrompt: true) else { | ||
| print("Error: Accessibility permissions required") | ||
| throw ExitCode.failure | ||
| } | ||
|
|
||
| let useColor = !noColor | ||
| print(Color.cyan.wrap("Watching accessibility elements under cursor...", enabled: useColor)) | ||
| print(Color.dim.wrap("Press Ctrl+C to stop\n", enabled: useColor)) | ||
|
|
||
| var lastElement: Accessibility.Element? | ||
| var lastInfo = "" | ||
|
|
||
| // Set up signal handler for clean exit | ||
| signal(SIGINT) { _ in | ||
| print("\n\nStopped watching.") | ||
| Darwin.exit(0) | ||
| } | ||
|
|
||
| while true { | ||
| let mouseLocation = NSEvent.mouseLocation | ||
|
|
||
| // Convert to screen coordinates (flip Y) | ||
| let screenHeight = NSScreen.main?.frame.height ?? 0 | ||
| let point = CGPoint(x: mouseLocation.x, y: screenHeight - mouseLocation.y) | ||
|
|
||
| // Hit test to find element at point | ||
| let systemWide = Accessibility.Element.systemWide | ||
| guard let element: Accessibility.Element = try? systemWide.hitTest(x: Float(point.x), y: Float(point.y)) else { | ||
| Thread.sleep(forTimeInterval: Double(interval) / 1000.0) | ||
| continue | ||
| } | ||
|
|
||
| // If filtering by PID, check it | ||
| if let filterPid = pid { | ||
| guard let elementPid = try? element.pid(), elementPid == filterPid else { | ||
| Thread.sleep(forTimeInterval: Double(interval) / 1000.0) | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| // Check if element changed | ||
| let currentInfo = formatElementInfo(element, useColor: useColor) | ||
| if currentInfo != lastInfo || element != lastElement { | ||
| lastElement = element | ||
| lastInfo = currentInfo | ||
|
|
||
| // Clear previous output and print new info | ||
| print("\u{001B}[2J\u{001B}[H", terminator: "") // Clear screen | ||
| print(Color.cyan.wrap("═══ Element Under Cursor ═══", enabled: useColor)) | ||
| print() | ||
| print(currentInfo) | ||
|
|
||
| if path { | ||
| printElementPath(element, useColor: useColor) | ||
| } | ||
|
|
||
| if actions { | ||
| printActions(element, useColor: useColor) | ||
| } | ||
|
|
||
| if full { | ||
| printFullAttributes(element, useColor: useColor) | ||
| } | ||
|
|
||
| print() | ||
| print(Color.dim.wrap("Mouse: (\(Int(point.x)), \(Int(point.y)))", enabled: useColor)) | ||
| print(Color.dim.wrap("Press Ctrl+C to stop", enabled: useColor)) | ||
| } | ||
|
|
||
| Thread.sleep(forTimeInterval: Double(interval) / 1000.0) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the relevant file to understand the context
cd Sources/axdump/Commands && wc -l WatchCommand.swiftRepository: beeper/BetterSwiftAX
Length of output: 137
🏁 Script executed:
# Read the file to understand the full context around lines 44-115
cat -n WatchCommand.swift | sed -n '40,120p'Repository: beeper/BetterSwiftAX
Length of output: 114
🌐 Web query:
NSScreen.main behavior multi-monitor macOS cursor coordinates
💡 Result:
-
NSScreen.main(“mainScreen”) is not the “primary” display. It returns the screen that macOS considers active for your app—i.e., the screen containing the window currently receiving keyboard events (key/main focus). It can differ from the screen with the menu bar and the (0,0) origin. [1], [3] -
The “primary” display is
NSScreen.screens[0]. Apple’s docs state the screen at index 0 is the one with the menu bar and whose origin is at (0,0) in the global AppKit screen coordinate space. [1] -
Global cursor coordinates (
NSEvent.mouseLocation) are in that global screen space. The point is in a desktop-wide coordinate system with origin at the bottom-left of the (0,0) screen; other displays can have negative X/Y depending on how the user arranged them in System Settings → Displays. This is why “cursor ↔ screen” math often breaks if you assumeNSScreen.main.framealways matches the mouse’s screen. [2], [6] -
To map the cursor to the correct monitor, don’t use
NSScreen.main. Find the screen whoseframecontains the mouse point: [4]let p = NSEvent.mouseLocation let screen = NSScreen.screens.first { NSMouseInRect(p, $0.frame, false) }
-
If you need “top-left origin” coordinates, note AppKit screen coordinates are bottom-left origin, so you must flip Y relative to the relevant screen or window height (and on multi-monitor setups you must do it per-screen, not with
NSScreen.mainblindly). [2], [4]
Sources: Apple NSScreen reference [1]; Stack Overflow discussions on mainScreen meaning and multi-display coordinates [3], [6]; cursor location coordinate caveats [2]; example of picking the screen under the mouse [4].
🌐 Web query:
macOS NSScreen coordinates Y-flip multi-display coordinate system
💡 Result:
AppKit / NSScreen (what Cocoa calls “screen coordinates”)
NSScreen.frame/NSWindow.frameare in a single global desktop space where:- The screen that has the menu bar is the reference screen whose bottom-left is (0,0).
- +Y goes up, +X goes right (non-flipped).
- Other displays’
screen.frame.origincan be negative or positive depending on how the user arranged displays in System Settings. (stackoverflow.com)
Core Graphics “screen space” used by some CG APIs (the common “Y-flip” surprise)
- Some Core Graphics APIs (notably window-list bounds via
kCGWindowBounds) report rectangles in screen space whose origin is the upper-left of the main display, i.e. +Y goes down. (entonos.com) - That’s why values look “Y-flipped” compared to
NSScreen.frame.
Practical conversion (CG window-bounds → AppKit global screen coords)
Let:
H = height of the menu-bar screen(oftenNSScreen.screens[0].frame.height)rectCGbe a CGRect from a CG API that uses top-left origin (likekCGWindowBounds)
Then convert to AppKit-style (bottom-left origin) with:
rectAppKit.origin.x = rectCG.origin.xrectAppKit.origin.y = H - rectCG.origin.y - rectCG.size.height(entonos.com)
If you need per-display Quartz geometry
- Map an
NSScreento aCGDirectDisplayIDviascreen.deviceDescription["NSScreenNumber"], then useCGDisplayBounds(displayID)to get that display’s bounds in Quartz Display Services coordinates. (stackoverflow.com)
Citations:
- 1: https://stackoverflow.com/questions/14579880/strange-nsscreen-coordinates?utm_source=openai
- 2: https://entonos.com/2021/05/20/which-cgrect-was-that/
- 3: https://entonos.com/2021/05/20/which-cgrect-was-that/
- 4: https://stackoverflow.com/questions/22671916/cocoa-getting-the-top-left-logical-point-of-multiple-screens?utm_source=openai
🏁 Script executed:
# Find the WatchCommand.swift file
fd -type f "WatchCommand.swift"Repository: beeper/BetterSwiftAX
Length of output: 234
🏁 Script executed:
# List the repository structure to understand the layout
git ls-files | head -30Repository: beeper/BetterSwiftAX
Length of output: 1257
🏁 Script executed:
# Search for any Swift files with "Watch" in the name
fd -type f -name "*Watch*"Repository: beeper/BetterSwiftAX
Length of output: 234
🏁 Script executed:
# Search for WatchCommand anywhere in the repo
fd "WatchCommand"Repository: beeper/BetterSwiftAX
Length of output: 46
🏁 Script executed:
# Search for any mention of WatchCommand in Swift files
rg -t swift "WatchCommand"Repository: beeper/BetterSwiftAX
Length of output: 46
🏁 Script executed:
# List all Swift files in the repository
find . -name "*.swift" -type fRepository: beeper/BetterSwiftAX
Length of output: 748
Use the screen containing the cursor for coordinate conversion, not NSScreen.main.
NSScreen.main returns the screen with keyboard focus, not the screen under the cursor. On multi-display setups, this causes incorrect Y-coordinate flipping for hit testing, resulting in elements being hit-tested at wrong screen locations.
🛠️ Suggested fix
let mouseLocation = NSEvent.mouseLocation
- // Convert to screen coordinates (flip Y)
- let screenHeight = NSScreen.main?.frame.height ?? 0
- let point = CGPoint(x: mouseLocation.x, y: screenHeight - mouseLocation.y)
+ // Convert to screen coordinates (flip Y) using the containing screen
+ let screenFrame = (NSScreen.screens.first { $0.frame.contains(mouseLocation) } ?? NSScreen.main)?.frame ?? .zero
+ let point = CGPoint(x: mouseLocation.x, y: screenFrame.maxY - mouseLocation.y)Consider using NSMouseInRect() for additional safety if comparing against screen boundaries.
🤖 Prompt for AI Agents
In `@Sources/axdump/Commands/WatchCommand.swift` around lines 44 - 115, The
coordinate conversion is using NSScreen.main which can be the focused screen,
not the one under the cursor, causing wrong Y-flipping in run(); replace the
NSScreen.main lookup with code that finds the screen containing the mouse (e.g.,
search NSScreen.screens for the screen whose frame contains
NSEvent.mouseLocation or use NSMouseInRect for boundary checks), then use that
screen's frame.height/origin to flip Y before calling
Accessibility.Element.systemWide.hitTest(x:y:); update the place where
screenHeight is computed and any dependent logic so hitTest uses coordinates
corrected for the actual screen under the cursor.
| /// Navigate to an element via dot-separated child indices | ||
| func navigateToPath(from element: Accessibility.Element, path: String) throws -> Accessibility.Element { | ||
| var current = element | ||
| let indices = path.split(separator: ".").compactMap { Int($0) } | ||
|
|
||
| for (step, index) in indices.enumerated() { | ||
| let childrenAttr: Accessibility.Attribute<[Accessibility.Element]> = current.attribute(.init("AXChildren")) | ||
| guard let children: [Accessibility.Element] = try? childrenAttr() else { | ||
| throw NavigationError.noChildren(step: step) | ||
| } | ||
| guard index >= 0 && index < children.count else { | ||
| throw NavigationError.indexOutOfRange(index: index, step: step, count: children.count) | ||
| } | ||
| current = children[index] | ||
| } | ||
|
|
||
| return current | ||
| } | ||
|
|
||
| /// Navigate to a single child by index | ||
| func navigateToChild(from element: Accessibility.Element, index: Int) throws -> Accessibility.Element { | ||
| let childrenAttr: Accessibility.Attribute<[Accessibility.Element]> = element.attribute(.init("AXChildren")) | ||
| guard let children: [Accessibility.Element] = try? childrenAttr() else { | ||
| throw NavigationError.noChildren(step: 0) | ||
| } | ||
| guard index >= 0 && index < children.count else { | ||
| throw NavigationError.indexOutOfRange(index: index, step: 0, count: children.count) | ||
| } | ||
| return children[index] | ||
| } | ||
|
|
||
| enum NavigationError: Error, CustomStringConvertible { | ||
| case noChildren(step: Int) | ||
| case indexOutOfRange(index: Int, step: Int, count: Int) | ||
|
|
||
| var description: String { | ||
| switch self { | ||
| case .noChildren(let step): | ||
| return "Element at step \(step) has no children" | ||
| case .indexOutOfRange(let index, let step, let count): | ||
| return "Child index \(index) at step \(step) out of range (0..<\(count))" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Invalid path components are silently ignored
Line 372 uses compactMap, so 1.a.2 becomes 1.2 without error, which can target the wrong element. Validate each component and throw a dedicated navigation error when parsing fails.
🛠️ Proposed fix
- let indices = path.split(separator: ".").compactMap { Int($0) }
+ let parts = path.split(separator: ".", omittingEmptySubsequences: false)
+ var indices: [Int] = []
+ for (step, part) in parts.enumerated() {
+ guard let index = Int(part) else {
+ throw NavigationError.invalidPathComponent(String(part), step: step)
+ }
+ indices.append(index)
+ } enum NavigationError: Error, CustomStringConvertible {
case noChildren(step: Int)
case indexOutOfRange(index: Int, step: Int, count: Int)
+ case invalidPathComponent(String, step: Int)
var description: String {
switch self {
case .noChildren(let step):
return "Element at step \(step) has no children"
case .indexOutOfRange(let index, let step, let count):
return "Child index \(index) at step \(step) out of range (0..<\(count))"
+ case .invalidPathComponent(let value, let step):
+ return "Invalid path component '\(value)' at step \(step)"
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Navigate to an element via dot-separated child indices | |
| func navigateToPath(from element: Accessibility.Element, path: String) throws -> Accessibility.Element { | |
| var current = element | |
| let indices = path.split(separator: ".").compactMap { Int($0) } | |
| for (step, index) in indices.enumerated() { | |
| let childrenAttr: Accessibility.Attribute<[Accessibility.Element]> = current.attribute(.init("AXChildren")) | |
| guard let children: [Accessibility.Element] = try? childrenAttr() else { | |
| throw NavigationError.noChildren(step: step) | |
| } | |
| guard index >= 0 && index < children.count else { | |
| throw NavigationError.indexOutOfRange(index: index, step: step, count: children.count) | |
| } | |
| current = children[index] | |
| } | |
| return current | |
| } | |
| /// Navigate to a single child by index | |
| func navigateToChild(from element: Accessibility.Element, index: Int) throws -> Accessibility.Element { | |
| let childrenAttr: Accessibility.Attribute<[Accessibility.Element]> = element.attribute(.init("AXChildren")) | |
| guard let children: [Accessibility.Element] = try? childrenAttr() else { | |
| throw NavigationError.noChildren(step: 0) | |
| } | |
| guard index >= 0 && index < children.count else { | |
| throw NavigationError.indexOutOfRange(index: index, step: 0, count: children.count) | |
| } | |
| return children[index] | |
| } | |
| enum NavigationError: Error, CustomStringConvertible { | |
| case noChildren(step: Int) | |
| case indexOutOfRange(index: Int, step: Int, count: Int) | |
| var description: String { | |
| switch self { | |
| case .noChildren(let step): | |
| return "Element at step \(step) has no children" | |
| case .indexOutOfRange(let index, let step, let count): | |
| return "Child index \(index) at step \(step) out of range (0..<\(count))" | |
| } | |
| } | |
| } | |
| /// Navigate to an element via dot-separated child indices | |
| func navigateToPath(from element: Accessibility.Element, path: String) throws -> Accessibility.Element { | |
| var current = element | |
| let parts = path.split(separator: ".", omittingEmptySubsequences: false) | |
| var indices: [Int] = [] | |
| for (step, part) in parts.enumerated() { | |
| guard let index = Int(part) else { | |
| throw NavigationError.invalidPathComponent(String(part), step: step) | |
| } | |
| indices.append(index) | |
| } | |
| for (step, index) in indices.enumerated() { | |
| let childrenAttr: Accessibility.Attribute<[Accessibility.Element]> = current.attribute(.init("AXChildren")) | |
| guard let children: [Accessibility.Element] = try? childrenAttr() else { | |
| throw NavigationError.noChildren(step: step) | |
| } | |
| guard index >= 0 && index < children.count else { | |
| throw NavigationError.indexOutOfRange(index: index, step: step, count: children.count) | |
| } | |
| current = children[index] | |
| } | |
| return current | |
| } | |
| /// Navigate to a single child by index | |
| func navigateToChild(from element: Accessibility.Element, index: Int) throws -> Accessibility.Element { | |
| let childrenAttr: Accessibility.Attribute<[Accessibility.Element]> = element.attribute(.init("AXChildren")) | |
| guard let children: [Accessibility.Element] = try? childrenAttr() else { | |
| throw NavigationError.noChildren(step: 0) | |
| } | |
| guard index >= 0 && index < children.count else { | |
| throw NavigationError.indexOutOfRange(index: index, step: 0, count: children.count) | |
| } | |
| return children[index] | |
| } | |
| enum NavigationError: Error, CustomStringConvertible { | |
| case noChildren(step: Int) | |
| case indexOutOfRange(index: Int, step: Int, count: Int) | |
| case invalidPathComponent(String, step: Int) | |
| var description: String { | |
| switch self { | |
| case .noChildren(let step): | |
| return "Element at step \(step) has no children" | |
| case .indexOutOfRange(let index, let step, let count): | |
| return "Child index \(index) at step \(step) out of range (0..<\(count))" | |
| case .invalidPathComponent(let value, let step): | |
| return "Invalid path component '\(value)' at step \(step)" | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@Sources/axdump/Utilities/ElementPrinter.swift` around lines 369 - 412,
navigateToPath currently uses compactMap on path components so non-integer parts
like "1.a.2" are dropped; change parsing to validate each component and throw a
new NavigationError case for invalid components instead of silently ignoring
them. Add a NavigationError case such as invalidComponent(component: String,
step: Int) (with a suitable description) and replace let indices =
path.split(...).compactMap { Int($0) } with iterating over path.split(separator:
".").enumerated(), attempting Int(String(component)) for each component and
throwing NavigationError.invalidComponent(component: String(component), step:
step) on failure, then proceed with the existing child lookup logic in
navigateToPath.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Package.swift`:
- Around line 14-15: The Package.swift currently lists .package(url:
"https://github.com/apple/swift-argument-parser.git", from: "1.2.0") but no
target references it; either remove that package entry from the dependencies
array or add the ArgumentParser product to the targets that actually use it
(e.g., add .product(name: "ArgumentParser", package: "swift-argument-parser") to
the dependencies array of the appropriate target(s) such as "CWindowControl",
"WindowControl", "CAccessibilityControl" or "AccessibilityControl"); update only
Package.swift so the declared package matches the target usage.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@Sources/AccessibilityControl/Element`+Utilities.swift:
- Around line 48-57: The recursiveSelectedChildren() sequence lacks cycle/depth
protection and can loop indefinitely if selectedChildren() forms cycles; update
recursiveSelectedChildren() (referencing the recursiveSelectedChildren function
and elt.selectedChildren()) to mirror recursiveChildren's safeguards by either
tracking visited elements (a Set of Accessibility.Element identifiers) and
skipping already-seen nodes, or by enforcing the existing maxTraversalComplexity
limit used by recursiveChildren and terminating the sequence when that threshold
is exceeded; ensure you preserve the AnySequence/sequence structure while
breaking traversal when a cycle is detected or the maxTraversalComplexity is
reached.
- Around line 15-17: The isInViewport computed property currently treats a
thrown frame() as non-null because (try? self.frame()) yields nil and nil !=
CGRect.null is true; change isInViewport to safely unwrap the result of try?
self.frame() and return false if frame() throws or returns nil, otherwise
compare the unwrapped CGRect to CGRect.null (i.e., use a guard/if let on try?
self.frame() and only then check != CGRect.null) so that exceptions result in
false; reference: isInViewport property and frame() method.
- Around line 65-76: The setFrame(_:) function is declared throws but uses try?
inside DispatchQueue.concurrentPerform so no errors are ever propagated; either
remove throws from setFrame or change the implementation to propagate errors:
replace the concurrentPerform block with sequential calls that use try (e.g.,
try position(assign:) followed by try size(assign:)) or capture and rethrow the
first error from both calls, ensuring position(assign:) and size(assign:) errors
are not swallowed; adjust the function signature accordingly if you remove
throwing behavior.
In `@Sources/AccessibilityControl/Element`+XMLDump.swift:
- Around line 237-242: The xmlEscaped computed property incorrectly maps the
double-quote character to the single-quote entity; update xmlEscaped so that it
replaces the double-quote character (") with the correct XML entity "
instead of ', keeping the other replacements for &, <, and > intact; locate
the xmlEscaped property in Element+XMLDump.swift and change the replacement for
"\"" from "'" to """.
- Around line 114-132: lastNonNilIndex is computed from Array(attributes.values)
(unsorted) but the loop iterates attributes.sorted(...), so omitTrailingSpace is
applied to the wrong item; fix by computing lastNonNilIndex from the same sorted
sequence used in the loop: build let sortedAttributes = attributes.sorted(by: {
$0.key < $1.key }), compute lastNonNilIndex = sortedAttributes.map({ $0.value
}).lastIndex(where: { $0 != nil }), then iterate over sortedAttributes
(enumerated) and call printAttribute(name:value:omitTrailingSpace:) using index
== lastNonNilIndex; references: lastNonNilIndex, attributes.sorted(by:),
printAttribute(to: &output,...), attributesPointingToElements,
Accessibility.Element.
- Around line 117-125: The force-cast in the attributesPointingToElements
population (attributesPointingToElements[name, default: []].append(contentsOf:
array.map { Accessibility.Element(raw: $0 as! AXUIElement) })) is unsafe because
only the first element is type-checked; replace the map + force-cast with a safe
filter/transform (e.g. use compactMap or a for loop) that validates each element
with CFGetTypeID(... as CFTypeRef) == AXUIElementGetTypeID() before creating
Accessibility.Element(raw: ...) so heterogeneous arrays won’t crash at runtime.
In `@Sources/AccessibilityControl/Names`+Standard.swift:
- Around line 46-48: Change the three properties selectedRows, selectedColumns,
and selectedCells to return AttributeName<[Accessibility.Element]> instead of
AttributeName<[AXUIElement]> so the public API uses the wrapper type; update
their declarations (the .init(kAXSelectedRowsAttribute),
.init(kAXSelectedColumnsAttribute), .init(kAXSelectedCellsAttribute) calls can
remain) and adjust any callers or type annotations that expect [AXUIElement] to
use [Accessibility.Element] (or map returned elements to Accessibility.Element)
to avoid leaking raw AXUIElement through the API.
🧹 Nitpick comments (2)
Sources/AccessibilityControl/Element+Utilities.swift (1)
23-46: BFS usesArray.removeFirst()which is O(n).For large trees, repeatedly removing from the front of an array degrades to O(n²). Consider using a
Dequeor an index-based approach. Not a blocker given the complexity cap, but worth noting.Sources/AccessibilityControl/Element+XMLDump.swift (1)
24-39:attributesLikelyToContainPIIis a computedstatic var, allocating a newSeton every call.Since this set is constant, use
static letinstead.Proposed fix
- static var attributesLikelyToContainPII: Set<String> { - [ + static let attributesLikelyToContainPII: Set<String> = [ kAXHelpAttribute, ... kAXDocumentAttribute, - ] - } + ]
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
Sources/AccessibilityControl/Accessibility+Role.swiftSources/AccessibilityControl/Accessibility+Subrole.swiftSources/AccessibilityControl/Element+Utilities.swiftSources/AccessibilityControl/Element+XMLDump.swiftSources/AccessibilityControl/Names+Standard.swiftSources/AccessibilityControl/Notification+Standard.swift
🧰 Additional context used
🧬 Code graph analysis (3)
Sources/AccessibilityControl/Element+Utilities.swift (2)
Sources/AccessibilityControl/Element.swift (1)
pid(28-32)Sources/AccessibilityControl/Attribute.swift (1)
count(114-121)
Sources/AccessibilityControl/Accessibility+Role.swift (1)
Sources/AccessibilityControl/Element.swift (1)
window(122-126)
Sources/AccessibilityControl/Names+Standard.swift (1)
Sources/AccessibilityControl/Observer.swift (1)
cancel(40-43)
🪛 SwiftLint (0.63.2)
Sources/AccessibilityControl/Notification+Standard.swift
[Warning] 3-3: Prefer not to use extension access modifiers
(no_extension_access_modifier)
Sources/AccessibilityControl/Element+Utilities.swift
[Warning] 6-6: Prefer not to use extension access modifiers
(no_extension_access_modifier)
[Warning] 86-86: Prefer not to use extension access modifiers
(no_extension_access_modifier)
[Warning] 66-66: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 23-23: Underscore(s) used as thousand separator(s) should be added after every 3 digits only
(number_separator)
Sources/AccessibilityControl/Names+Standard.swift
[Warning] 7-7: Prefer not to use extension access modifiers
(no_extension_access_modifier)
Sources/AccessibilityControl/Element+XMLDump.swift
[Warning] 70-70: Function should have complexity 12 or less; currently complexity is 24
(cyclomatic_complexity)
[Warning] 123-123: Force casts should be avoided
(force_cast)
[Warning] 70-70: Function body should span 80 lines or less excluding comments and whitespace: currently spans 94 lines
(function_body_length)
[Warning] 41-41: Optional should be implicitly initialized without nil
(implicit_optional_initialization)
[Warning] 206-206: Prefer not to use extension access modifiers
(no_extension_access_modifier)
[Warning] 236-236: Prefer not to use extension access modifiers
(no_extension_access_modifier)
[Warning] 144-144: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 190-190: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 126-126: Else branches should be avoided when the previous if-block exits the current scope
(superfluous_else)
🔇 Additional comments (7)
Sources/AccessibilityControl/Accessibility+Subrole.swift (1)
1-34: LGTM!Using a caseless enum as a namespace for AX subrole constants is idiomatic Swift. The mappings are clean and comprehensive.
Sources/AccessibilityControl/Accessibility+Role.swift (1)
1-56: LGTM!Consistent with the
Subrolepattern. The documentation link is a nice touch. Comprehensive coverage of standard AX roles.Sources/AccessibilityControl/Element+Utilities.swift (1)
1-9: Remaining segments look good.
isValid,isFrameValid,recursivelyFindChild,closeWindow, andfirstChild(withRole:)are clean and straightforward.Also applies to: 11-13, 59-63, 78-92
Sources/AccessibilityControl/Names+Standard.swift (2)
56-63:decrement,minValue, andmaxValueare gated behind#if DEBUG.
incrementis available in all builds (line 56), butdecrementis DEBUG-only. These are both standard AX actions/attributes. Was this intentional? If this is a safeguard to prevent accidental use, consider documenting the rationale; otherwise, move them outside the#if DEBUGblock.
1-55: Remaining attribute and action declarations look good.Comprehensive coverage of standard AX attributes, actions, and window-specific properties with appropriate mutability annotations (
AttributeNamevsMutableAttributeName).Also applies to: 65-79
Sources/AccessibilityControl/Notification+Standard.swift (1)
1-14: LGTM!Clean set of standard AX notification constants. Consistent use of
Self(...)initializer pattern.Sources/AccessibilityControl/Element+XMLDump.swift (1)
1-13: Remaining segments look reasonable.The
printAttributehelper,XMLDumperstruct layout, initializer, conveniencedumpXMLextension, andquoted/asCommentutilities are clean. Thedumpmethod is large (SwiftLint flags complexity 24 and 94 lines) but readable given its serialization nature — could be split in a follow-up.Also applies to: 15-23, 40-68, 206-232, 244-251
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| var isInViewport: Bool { | ||
| (try? self.frame()) != CGRect.null | ||
| } |
There was a problem hiding this comment.
isInViewport returns true when frame() throws.
try? self.frame() yields CGRect?. If frame() throws, the result is nil, and nil != CGRect.null evaluates to true — making the property incorrectly report the element as being in the viewport.
🐛 Proposed fix
var isInViewport: Bool {
- (try? self.frame()) != CGRect.null
+ guard let frame = try? self.frame() else { return false }
+ return frame != CGRect.null
}🤖 Prompt for AI Agents
In `@Sources/AccessibilityControl/Element`+Utilities.swift around lines 15 - 17,
The isInViewport computed property currently treats a thrown frame() as non-null
because (try? self.frame()) yields nil and nil != CGRect.null is true; change
isInViewport to safely unwrap the result of try? self.frame() and return false
if frame() throws or returns nil, otherwise compare the unwrapped CGRect to
CGRect.null (i.e., use a guard/if let on try? self.frame() and only then check
!= CGRect.null) so that exceptions result in false; reference: isInViewport
property and frame() method.
| func recursiveSelectedChildren() -> AnySequence<Accessibility.Element> { | ||
| AnySequence(sequence(state: [self]) { queue -> Accessibility.Element? in | ||
| guard !queue.isEmpty else { return nil } | ||
| let elt = queue.removeFirst() | ||
| if let selectedChildren = try? elt.selectedChildren() { | ||
| queue.append(contentsOf: selectedChildren) | ||
| } | ||
| return elt | ||
| }) | ||
| } |
There was a problem hiding this comment.
recursiveSelectedChildren has no cycle or depth protection.
Unlike recursiveChildren which has maxTraversalComplexity, this method has no safeguard. If selectedChildren relationships form a cycle, this will loop indefinitely.
🤖 Prompt for AI Agents
In `@Sources/AccessibilityControl/Element`+Utilities.swift around lines 48 - 57,
The recursiveSelectedChildren() sequence lacks cycle/depth protection and can
loop indefinitely if selectedChildren() forms cycles; update
recursiveSelectedChildren() (referencing the recursiveSelectedChildren function
and elt.selectedChildren()) to mirror recursiveChildren's safeguards by either
tracking visited elements (a Set of Accessibility.Element identifiers) and
skipping already-seen nodes, or by enforcing the existing maxTraversalComplexity
limit used by recursiveChildren and terminating the sequence when that threshold
is exceeded; ensure you preserve the AnySequence/sequence structure while
breaking traversal when a cycle is detected or the maxTraversalComplexity is
reached.
| func setFrame(_ frame: CGRect) throws { | ||
| DispatchQueue.concurrentPerform(iterations: 2) { i in | ||
| switch i { | ||
| case 0: | ||
| try? self.position(assign: frame.origin) | ||
| case 1: | ||
| try? self.size(assign: frame.size) | ||
| default: | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
setFrame is declared throws but can never throw.
Both position(assign:) and size(assign:) are called with try?, silently discarding any errors. Callers will incorrectly assume failures are propagated. Either remove throws or propagate the errors.
Option A: Remove `throws` (matches current behavior)
- func setFrame(_ frame: CGRect) throws {
+ func setFrame(_ frame: CGRect) {Option B: Propagate errors sequentially
func setFrame(_ frame: CGRect) throws {
- DispatchQueue.concurrentPerform(iterations: 2) { i in
- switch i {
- case 0:
- try? self.position(assign: frame.origin)
- case 1:
- try? self.size(assign: frame.size)
- default:
- break
- }
- }
+ try self.position(assign: frame.origin)
+ try self.size(assign: frame.size)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func setFrame(_ frame: CGRect) throws { | |
| DispatchQueue.concurrentPerform(iterations: 2) { i in | |
| switch i { | |
| case 0: | |
| try? self.position(assign: frame.origin) | |
| case 1: | |
| try? self.size(assign: frame.size) | |
| default: | |
| break | |
| } | |
| } | |
| } | |
| func setFrame(_ frame: CGRect) { | |
| DispatchQueue.concurrentPerform(iterations: 2) { i in | |
| switch i { | |
| case 0: | |
| try? self.position(assign: frame.origin) | |
| case 1: | |
| try? self.size(assign: frame.size) | |
| default: | |
| break | |
| } | |
| } | |
| } |
| func setFrame(_ frame: CGRect) throws { | |
| DispatchQueue.concurrentPerform(iterations: 2) { i in | |
| switch i { | |
| case 0: | |
| try? self.position(assign: frame.origin) | |
| case 1: | |
| try? self.size(assign: frame.size) | |
| default: | |
| break | |
| } | |
| } | |
| } | |
| func setFrame(_ frame: CGRect) throws { | |
| try self.position(assign: frame.origin) | |
| try self.size(assign: frame.size) | |
| } |
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 66-66: Magic numbers should be replaced by named constants
(no_magic_numbers)
🤖 Prompt for AI Agents
In `@Sources/AccessibilityControl/Element`+Utilities.swift around lines 65 - 76,
The setFrame(_:) function is declared throws but uses try? inside
DispatchQueue.concurrentPerform so no errors are ever propagated; either remove
throws from setFrame or change the implementation to propagate errors: replace
the concurrentPerform block with sequential calls that use try (e.g., try
position(assign:) followed by try size(assign:)) or capture and rethrow the
first error from both calls, ensuring position(assign:) and size(assign:) errors
are not swallowed; adjust the function signature accordingly if you remove
throwing behavior.
| let lastNonNilIndex = Array(attributes.values).lastIndex(where: { $0 != nil }) | ||
|
|
||
| for (index, (name, value)) in attributes.sorted(by: { $0.key < $1.key }).enumerated() { | ||
| if let array = value as? [Any] { | ||
| guard let first = array.first, CFGetTypeID(first as CFTypeRef) == AXUIElementGetTypeID() else { | ||
| continue | ||
| } | ||
| // attribute value is of an array of ui elements | ||
| attributesPointingToElements[name, default: []].append(contentsOf: array.map { | ||
| Accessibility.Element(raw: $0 as! AXUIElement) | ||
| }) | ||
| continue | ||
| } else if let element = Accessibility.Element(erased: value as CFTypeRef) { | ||
| // attribute value is of a singular ui element | ||
| attributesPointingToElements[name, default: []].append(element) | ||
| continue | ||
| } | ||
|
|
||
| printAttribute(to: &output, name: name, value: value, omitTrailingSpace: index == lastNonNilIndex) |
There was a problem hiding this comment.
lastNonNilIndex is computed on unsorted dictionary values but compared against sorted iteration index.
Array(attributes.values) preserves the arbitrary dictionary order, but line 116 iterates attributes.sorted(by: { $0.key < $1.key }). The index == lastNonNilIndex check on line 132 compares indices from two different orderings, so omitTrailingSpace will be applied to the wrong attribute.
🐛 Proposed fix — compute from the sorted sequence
- let lastNonNilIndex = Array(attributes.values).lastIndex(where: { $0 != nil })
-
- for (index, (name, value)) in attributes.sorted(by: { $0.key < $1.key }).enumerated() {
+ let sorted = attributes.sorted(by: { $0.key < $1.key })
+ let lastNonNilIndex = sorted.lastIndex(where: { $0.value != nil })
+
+ for (index, (name, value)) in sorted.enumerated() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let lastNonNilIndex = Array(attributes.values).lastIndex(where: { $0 != nil }) | |
| for (index, (name, value)) in attributes.sorted(by: { $0.key < $1.key }).enumerated() { | |
| if let array = value as? [Any] { | |
| guard let first = array.first, CFGetTypeID(first as CFTypeRef) == AXUIElementGetTypeID() else { | |
| continue | |
| } | |
| // attribute value is of an array of ui elements | |
| attributesPointingToElements[name, default: []].append(contentsOf: array.map { | |
| Accessibility.Element(raw: $0 as! AXUIElement) | |
| }) | |
| continue | |
| } else if let element = Accessibility.Element(erased: value as CFTypeRef) { | |
| // attribute value is of a singular ui element | |
| attributesPointingToElements[name, default: []].append(element) | |
| continue | |
| } | |
| printAttribute(to: &output, name: name, value: value, omitTrailingSpace: index == lastNonNilIndex) | |
| let sorted = attributes.sorted(by: { $0.key < $1.key }) | |
| let lastNonNilIndex = sorted.lastIndex(where: { $0.value != nil }) | |
| for (index, (name, value)) in sorted.enumerated() { | |
| if let array = value as? [Any] { | |
| guard let first = array.first, CFGetTypeID(first as CFTypeRef) == AXUIElementGetTypeID() else { | |
| continue | |
| } | |
| // attribute value is of an array of ui elements | |
| attributesPointingToElements[name, default: []].append(contentsOf: array.map { | |
| Accessibility.Element(raw: $0 as! AXUIElement) | |
| }) | |
| continue | |
| } else if let element = Accessibility.Element(erased: value as CFTypeRef) { | |
| // attribute value is of a singular ui element | |
| attributesPointingToElements[name, default: []].append(element) | |
| continue | |
| } | |
| printAttribute(to: &output, name: name, value: value, omitTrailingSpace: index == lastNonNilIndex) |
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 123-123: Force casts should be avoided
(force_cast)
[Warning] 126-126: Else branches should be avoided when the previous if-block exits the current scope
(superfluous_else)
🤖 Prompt for AI Agents
In `@Sources/AccessibilityControl/Element`+XMLDump.swift around lines 114 - 132,
lastNonNilIndex is computed from Array(attributes.values) (unsorted) but the
loop iterates attributes.sorted(...), so omitTrailingSpace is applied to the
wrong item; fix by computing lastNonNilIndex from the same sorted sequence used
in the loop: build let sortedAttributes = attributes.sorted(by: { $0.key <
$1.key }), compute lastNonNilIndex = sortedAttributes.map({ $0.value
}).lastIndex(where: { $0 != nil }), then iterate over sortedAttributes
(enumerated) and call printAttribute(name:value:omitTrailingSpace:) using index
== lastNonNilIndex; references: lastNonNilIndex, attributes.sorted(by:),
printAttribute(to: &output,...), attributesPointingToElements,
Accessibility.Element.
| if let array = value as? [Any] { | ||
| guard let first = array.first, CFGetTypeID(first as CFTypeRef) == AXUIElementGetTypeID() else { | ||
| continue | ||
| } | ||
| // attribute value is of an array of ui elements | ||
| attributesPointingToElements[name, default: []].append(contentsOf: array.map { | ||
| Accessibility.Element(raw: $0 as! AXUIElement) | ||
| }) | ||
| continue |
There was a problem hiding this comment.
Force cast $0 as! AXUIElement will crash if the array contains non-AXUIElement items.
The type check on line 118 only validates the first element. If the array is heterogeneous, subsequent elements will trigger a runtime crash.
Safer alternative using compactMap
- attributesPointingToElements[name, default: []].append(contentsOf: array.map {
- Accessibility.Element(raw: $0 as! AXUIElement)
- })
+ attributesPointingToElements[name, default: []].append(contentsOf: array.compactMap {
+ Accessibility.Element(erased: $0 as CFTypeRef)
+ })🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 123-123: Force casts should be avoided
(force_cast)
🤖 Prompt for AI Agents
In `@Sources/AccessibilityControl/Element`+XMLDump.swift around lines 117 - 125,
The force-cast in the attributesPointingToElements population
(attributesPointingToElements[name, default: []].append(contentsOf: array.map {
Accessibility.Element(raw: $0 as! AXUIElement) })) is unsafe because only the
first element is type-checked; replace the map + force-cast with a safe
filter/transform (e.g. use compactMap or a for loop) that validates each element
with CFGetTypeID(... as CFTypeRef) == AXUIElementGetTypeID() before creating
Accessibility.Element(raw: ...) so heterogeneous arrays won’t crash at runtime.
| var xmlEscaped: String { | ||
| replacingOccurrences(of: "&", with: "&") | ||
| .replacingOccurrences(of: "\"", with: "'") | ||
| .replacingOccurrences(of: "<", with: "<") | ||
| .replacingOccurrences(of: ">", with: ">") | ||
| } |
There was a problem hiding this comment.
XML escaping bug: " is escaped to ' instead of ".
Line 239 replaces " (double-quote) with ' (single-quote entity). The correct entity for double-quote is ". This will produce malformed XML when attribute values contain double-quotes.
🐛 Proposed fix
var xmlEscaped: String {
replacingOccurrences(of: "&", with: "&")
- .replacingOccurrences(of: "\"", with: "'")
+ .replacingOccurrences(of: "\"", with: """)
.replacingOccurrences(of: "<", with: "<")
.replacingOccurrences(of: ">", with: ">")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var xmlEscaped: String { | |
| replacingOccurrences(of: "&", with: "&") | |
| .replacingOccurrences(of: "\"", with: "'") | |
| .replacingOccurrences(of: "<", with: "<") | |
| .replacingOccurrences(of: ">", with: ">") | |
| } | |
| var xmlEscaped: String { | |
| replacingOccurrences(of: "&", with: "&") | |
| .replacingOccurrences(of: "\"", with: """) | |
| .replacingOccurrences(of: "<", with: "<") | |
| .replacingOccurrences(of: ">", with: ">") | |
| } |
🤖 Prompt for AI Agents
In `@Sources/AccessibilityControl/Element`+XMLDump.swift around lines 237 - 242,
The xmlEscaped computed property incorrectly maps the double-quote character to
the single-quote entity; update xmlEscaped so that it replaces the double-quote
character (") with the correct XML entity " instead of ', keeping the
other replacements for &, <, and > intact; locate the xmlEscaped property in
Element+XMLDump.swift and change the replacement for "\"" from "'" to
""".
| var selectedRows: AttributeName<[AXUIElement]> { .init(kAXSelectedRowsAttribute) } | ||
| var selectedColumns: AttributeName<[AXUIElement]> { .init(kAXSelectedColumnsAttribute) } | ||
| var selectedCells: AttributeName<[AXUIElement]> { .init(kAXSelectedCellsAttribute) } |
There was a problem hiding this comment.
Inconsistent return type: [AXUIElement] vs [Accessibility.Element].
selectedRows, selectedColumns, and selectedCells return AttributeName<[AXUIElement]>, while every other element-array attribute (e.g., rows, children, appWindows) returns AttributeName<[Accessibility.Element]>. This leaks raw AXUIElement through the public API, breaking the wrapper abstraction.
Proposed fix
- var selectedRows: AttributeName<[AXUIElement]> { .init(kAXSelectedRowsAttribute) }
- var selectedColumns: AttributeName<[AXUIElement]> { .init(kAXSelectedColumnsAttribute) }
- var selectedCells: AttributeName<[AXUIElement]> { .init(kAXSelectedCellsAttribute) }
+ var selectedRows: AttributeName<[Accessibility.Element]> { .init(kAXSelectedRowsAttribute) }
+ var selectedColumns: AttributeName<[Accessibility.Element]> { .init(kAXSelectedColumnsAttribute) }
+ var selectedCells: AttributeName<[Accessibility.Element]> { .init(kAXSelectedCellsAttribute) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var selectedRows: AttributeName<[AXUIElement]> { .init(kAXSelectedRowsAttribute) } | |
| var selectedColumns: AttributeName<[AXUIElement]> { .init(kAXSelectedColumnsAttribute) } | |
| var selectedCells: AttributeName<[AXUIElement]> { .init(kAXSelectedCellsAttribute) } | |
| var selectedRows: AttributeName<[Accessibility.Element]> { .init(kAXSelectedRowsAttribute) } | |
| var selectedColumns: AttributeName<[Accessibility.Element]> { .init(kAXSelectedColumnsAttribute) } | |
| var selectedCells: AttributeName<[Accessibility.Element]> { .init(kAXSelectedCellsAttribute) } |
🤖 Prompt for AI Agents
In `@Sources/AccessibilityControl/Names`+Standard.swift around lines 46 - 48,
Change the three properties selectedRows, selectedColumns, and selectedCells to
return AttributeName<[Accessibility.Element]> instead of
AttributeName<[AXUIElement]> so the public API uses the wrapper type; update
their declarations (the .init(kAXSelectedRowsAttribute),
.init(kAXSelectedColumnsAttribute), .init(kAXSelectedCellsAttribute) calls can
remain) and adjust any callers or type annotations that expect [AXUIElement] to
use [Accessibility.Element] (or map returned elements to Accessibility.Element)
to avoid leaking raw AXUIElement through the API.
- add GenerateAXConstants plugin and AXConstantsGenerator tool - include rich documentation for constants based on header files
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@Plugins/GenerateAXConstants/plugin.swift`:
- Around line 13-23: The current Process invocation (setting
process.executableURL, process.arguments, calling process.run()/waitUntilExit()
and checking process.terminationStatus) swallows generator stderr/stdout and
only logs via Diagnostics.error; change it to set process.standardOutput and
process.standardError to Pipe, run the process, read and decode the pipe data
after waitUntilExit(), and if process.terminationStatus != 0 then include the
decoded stderr/stdout in the Diagnostics.error message and throw an Error (e.g.,
make String conform to Error or use a small custom error) so the plugin reports
failure to SPM; keep the same Process symbols (process.executableURL,
process.arguments, process.run, process.waitUntilExit,
process.terminationStatus) when implementing this.
In `@Sources/AccessibilityControl/Accessibility`+ParameterizedAttributeKey.swift:
- Around line 38-40: The generated MARK comments contain raw type-signature
snippets (e.g., "// MARK: - (NSValue *) - (rectValue); param: (NSValue *) -
(rectValue)" and "// MARK: - AXTextMarkerRef; param: (NSValue *) -
(pointValue)"), which are noisy; update the generator to ignore candidate group
titles that include "param:" or C-style type patterns like "*)" or bare C type
names (e.g., "AXTextMarkerRef") and instead emit either a concise descriptive
heading or omit the MARK entirely; apply this change so symbols like
convertRelativeFrame / kAXConvertRelativeFrameParameterizedAttribute (and other
parameterized attribute constants) are grouped under meaningful section titles
rather than raw type annotations.
In `@Sources/AXConstantsGenerator/main.swift`:
- Around line 66-82: Read from the pipe before waiting for the process to exit
to avoid a potential deadlock; in findSDKPath() move the call to
pipe.fileHandleForReading.readDataToEndOfFile() to occur immediately after try
process.run() (and before process.waitUntilExit()), so the stdout is drained
while the child runs (keeping standardError as FileHandle.nullDevice as
currently set) and then call process.waitUntilExit() and proceed to
convert/trimming the data into path.
🧹 Nitpick comments (6)
Sources/AXConstantsGenerator/main.swift (2)
123-131: SwiftLint: extra spaces before commas for column alignment.The
commarule flags the alignment padding. If SwiftLint is enforced in CI, either fix the spacing or add a// swiftlint:disable:next commaper line (or a disable/enable block around the array).♻️ Proposed fix (remove alignment padding)
private let outputConfigs: [OutputConfig] = [ - .init(suffix: "ParameterizedAttribute", mode: .caselessEnum(name: "ParameterizedAttributeKey"), fileName: "Accessibility+ParameterizedAttributeKey.swift"), - .init(suffix: "Attribute", mode: .caselessEnum(name: "AttributeKey"), fileName: "Accessibility+AttributeKey.swift"), - .init(suffix: "Subrole", mode: .caselessEnum(name: "Subrole"), fileName: "Accessibility+Subrole.swift"), - .init(suffix: "Role", mode: .caselessEnum(name: "Role"), fileName: "Accessibility+Role.swift"), - .init(suffix: "Notification", mode: .extensionOnType(typeName: "Notification"), fileName: "Accessibility+Notification.swift"), - .init(suffix: "Action", mode: .extensionOnType(typeName: "Action.Name"), fileName: "Accessibility+Action.swift"), - .init(suffix: "Value", mode: .caselessEnum(name: "Value"), fileName: "Accessibility+Value.swift"), + .init(suffix: "ParameterizedAttribute", mode: .caselessEnum(name: "ParameterizedAttributeKey"), fileName: "Accessibility+ParameterizedAttributeKey.swift"), + .init(suffix: "Attribute", mode: .caselessEnum(name: "AttributeKey"), fileName: "Accessibility+AttributeKey.swift"), + .init(suffix: "Subrole", mode: .caselessEnum(name: "Subrole"), fileName: "Accessibility+Subrole.swift"), + .init(suffix: "Role", mode: .caselessEnum(name: "Role"), fileName: "Accessibility+Role.swift"), + .init(suffix: "Notification", mode: .extensionOnType(typeName: "Notification"), fileName: "Accessibility+Notification.swift"), + .init(suffix: "Action", mode: .extensionOnType(typeName: "Action.Name"), fileName: "Accessibility+Action.swift"), + .init(suffix: "Value", mode: .caselessEnum(name: "Value"), fileName: "Accessibility+Value.swift"), ]
240-364: Consider decomposingparseHeaderMetadata— cyclomatic complexity is 23 (SwiftLint limit: 12).This function handles block comments, line comments, quick-reference state,
#definematching, and group/comment aggregation all in one loop. Extracting focused helpers (e.g.,handleBlockCommentLine,handleDefine,handleLineComment) would lower complexity and improve testability. Same applies toextractDocumentation(lines 483–575, complexity 24).Not blocking for a code-gen tool, but worth tracking for maintainability.
Sources/AccessibilityControl/Accessibility+AttributeKey.swift (2)
166-181: SwiftLint: period spacing in doc comments (lines 167–172).SwiftLint flags double-spaces after periods in the
valueDescriptiondoc comment. These originate from the Apple header text, so it's understandable, but you may want to either suppress the rule for generated/extracted docs or fix the spacing to keep CI clean.
913-913: Consider a more descriptive MARK for the web/ARIA attributes section.
// MARK: - Attributesat Line 913 is ambiguous — the entire file is attributes. Since this section contains ARIA, DOM, math, and other web-centric keys (sourced fromAXWebConstants.h), something like// MARK: - Web & ARIA Attributeswould aid navigation.Sources/AccessibilityControl/Accessibility+Notification.swift (2)
5-5: SwiftLint: prefer per-member access modifiers over extension-levelpublic.The other constant files in this PR (Role, Subrole, AttributeKey) declare
public static leton each member. This file usespublic extensioninstead, which SwiftLint'sno_extension_access_modifierrule flags. For consistency with the rest of the PR, consider removingpublicfrom the extension and adding it to eachstatic let.
286-296: Same vague MARK nit:// MARK: - Notificationscould be// MARK: - Web Notifications.These appear to be web-specific notifications (from
AXWebConstants.h) — a more descriptive section header would help with code navigation, consistent with the feedback on the other files.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
Package.swiftPlugins/GenerateAXConstants/plugin.swiftSources/AXConstantsGenerator/main.swiftSources/AccessibilityControl/Accessibility+Action.swiftSources/AccessibilityControl/Accessibility+AttributeKey.swiftSources/AccessibilityControl/Accessibility+Notification.swiftSources/AccessibilityControl/Accessibility+ParameterizedAttributeKey.swiftSources/AccessibilityControl/Accessibility+Role.swiftSources/AccessibilityControl/Accessibility+Subrole.swiftSources/AccessibilityControl/Accessibility+Value.swiftSources/AccessibilityControl/Element+Hierarchy.swift
🧰 Additional context used
🧬 Code graph analysis (2)
Plugins/GenerateAXConstants/plugin.swift (1)
Sources/AXConstantsGenerator/main.swift (1)
run(15-61)
Sources/AccessibilityControl/Accessibility+AttributeKey.swift (1)
Sources/AccessibilityControl/Element.swift (1)
window(122-126)
🪛 SwiftLint (0.63.2)
Sources/AccessibilityControl/Accessibility+Action.swift
[Warning] 5-5: Prefer not to use extension access modifiers
(no_extension_access_modifier)
Sources/AXConstantsGenerator/main.swift
[Warning] 125-125: There should be no space before and one after any comma
(comma)
[Warning] 125-125: There should be no space before and one after any comma
(comma)
[Warning] 126-126: There should be no space before and one after any comma
(comma)
[Warning] 126-126: There should be no space before and one after any comma
(comma)
[Warning] 127-127: There should be no space before and one after any comma
(comma)
[Warning] 127-127: There should be no space before and one after any comma
(comma)
[Warning] 128-128: There should be no space before and one after any comma
(comma)
[Warning] 128-128: There should be no space before and one after any comma
(comma)
[Warning] 129-129: There should be no space before and one after any comma
(comma)
[Warning] 129-129: There should be no space before and one after any comma
(comma)
[Warning] 130-130: There should be no space before and one after any comma
(comma)
[Warning] 130-130: There should be no space before and one after any comma
(comma)
[Warning] 240-240: Function should have complexity 12 or less; currently complexity is 23
(cyclomatic_complexity)
[Warning] 483-483: Function should have complexity 12 or less; currently complexity is 24
(cyclomatic_complexity)
[Warning] 37-37: where clauses are preferred over a single if inside a for
(for_where)
[Warning] 196-196: where clauses are preferred over a single if inside a for
(for_where)
[Warning] 602-602: where clauses are preferred over a single if inside a for
(for_where)
[Warning] 782-782: where clauses are preferred over a single if inside a for
(for_where)
[Warning] 182-182: Force tries should be avoided
(force_try)
[Warning] 232-232: Force tries should be avoided
(force_try)
[Warning] 236-236: Force tries should be avoided
(force_try)
[Warning] 240-240: Function body should span 80 lines or less excluding comments and whitespace: currently spans 109 lines
(function_body_length)
[Warning] 483-483: Function body should span 80 lines or less excluding comments and whitespace: currently spans 81 lines
(function_body_length)
[Warning] 141-141: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 194-194: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 259-259: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 306-306: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 329-329: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 333-333: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 396-396: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 408-408: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 409-409: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 583-583: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 584-584: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 598-598: Magic numbers should be replaced by named constants
(no_magic_numbers)
Sources/AccessibilityControl/Element+Hierarchy.swift
[Warning] 124-124: Force casts should be avoided
(force_cast)
[Warning] 139-139: Force casts should be avoided
(force_cast)
[Warning] 245-245: Arguments should be either on the same line, or one per line
(multiline_arguments)
Sources/AccessibilityControl/Accessibility+AttributeKey.swift
[Warning] 167-167: Periods should not be followed by more than one space
(period_spacing)
[Warning] 168-168: Periods should not be followed by more than one space
(period_spacing)
[Warning] 170-170: Periods should not be followed by more than one space
(period_spacing)
[Warning] 171-171: Periods should not be followed by more than one space
(period_spacing)
[Warning] 172-172: Periods should not be followed by more than one space
(period_spacing)
Sources/AccessibilityControl/Accessibility+Notification.swift
[Warning] 5-5: Prefer not to use extension access modifiers
(no_extension_access_modifier)
🔇 Additional comments (10)
Sources/AccessibilityControl/Accessibility+Value.swift (1)
1-19: LGTM!Clean, consistent pattern with the other AX constant files. The caseless enum serves well as a namespace for the static constant mappings.
Sources/AXConstantsGenerator/main.swift (1)
159-174: LGTM!The
lowerCamelCasefunction correctly handles all documented cases — single leading uppercase, all-uppercase names, and multi-character acronym prefixes. The logic of preserving the last uppercase char of the prefix as the start of the remainder is a good approach.Sources/AccessibilityControl/Accessibility+Action.swift (1)
5-57: LGTM!Constants and documentation are clean. The
public extensionpattern here (vs.extension+publicmembers in caseless-enum files) follows from the generator's.extensionOnTypeoutput mode, so the difference is intentional.The SwiftLint
no_extension_access_modifierwarning is a style preference — suppressing it or switching to per-memberpublicis your call.Sources/AccessibilityControl/Accessibility+ParameterizedAttributeKey.swift (1)
1-157: The constant mappings themselves look correct.All static lets follow the established pattern and map to the appropriate
kAX...ParameterizedAttributeconstants.Package.swift (1)
32-50: New targets are correctly wired.The
AXConstantsGeneratorexecutable target properly depends onArgumentParser(resolving the previous false positive about an unused dependency), and theGenerateAXConstantsplugin correctly references the generator as a dependency with appropriatewriteToPackageDirectorypermissions.Sources/AccessibilityControl/Element+Hierarchy.swift (3)
233-251: LGTM — solid design for wrapping a private API.The
copyHierarchymethod properly handles the unavailable-symbol case, usestakeRetainedValue()correctly for a Copy-rule API, and forwardsfile/linefor clear diagnostics. TheAccessibility.check()wrapper keeps AXError handling consistent with the rest of the library.
26-38: Private API usage is well-isolated viadlsym.The lazy-static pattern with graceful
nilfallback is appropriate for optional private API access. TheunsafeBitCastfromdlsymis the standard Swift pattern for this.
121-126: The CFGetTypeID check is necessary and cannot be replaced with conditional casting.
AXUIElementis an opaque Core Foundation type. TheCFGetTypeIDverification is the authoritative way to check its type at the C level. Swift's conditional cast operator (as?) relies on Swift's type bridging system, which is inadequate for opaque CF types retrieved from weakly-typed collections likeNSDictionary. Removing theCFGetTypeIDcheck in favor ofas?would make the code less safe and potentially break it. The force cast is safe precisely because it's preceded by the CF-level type verification.Likely an incorrect or invalid review comment.
Sources/AccessibilityControl/Accessibility+Role.swift (1)
5-175: LGTM — uninhabited enum as a namespace is idiomatic Swift.Clean, consistent mapping of AX role constants. One minor nit: the MARK at Line 172 (
// MARK: - Roles) is vague compared to the well-labeled// MARK: - Standard Rolesabove. SinceimageMapcomes fromAXWebConstants.h, consider renaming it to// MARK: - Web Rolesfor clarity.Sources/AccessibilityControl/Accessibility+Subrole.swift (1)
5-117: LGTM — consistent pattern with proper keyword escaping.Good use of backticks for
`switch`on Line 45. Same minor nit as the other files:// MARK: - Subrolesat Line 59 is vague for the web-specific constants section; consider// MARK: - Web Subroles.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: generator.path.string) | ||
| process.arguments = [outputDir.string] | ||
|
|
||
| try process.run() | ||
| process.waitUntilExit() | ||
|
|
||
| guard process.terminationStatus == 0 else { | ||
| Diagnostics.error("AXConstantsGenerator exited with code \(process.terminationStatus)") | ||
| return | ||
| } |
There was a problem hiding this comment.
Generator failures are silently swallowed — capture stderr and throw on non-zero exit.
Diagnostics.error() logs but does not cause the plugin to report failure to SPM in all versions. Also, the generator's own stderr/stdout (which contains warnings and diagnostics) is not captured, so the user has no visibility into what went wrong.
🔧 Proposed fix
+ let outputPipe = Pipe()
+ let errorPipe = Pipe()
let process = Process()
process.executableURL = URL(fileURLWithPath: generator.path.string)
process.arguments = [outputDir.string]
+ process.standardOutput = outputPipe
+ process.standardError = errorPipe
try process.run()
process.waitUntilExit()
+ let stdoutData = outputPipe.fileHandleForReading.readDataToEndOfFile()
+ let stderrData = errorPipe.fileHandleForReading.readDataToEndOfFile()
+
+ if let output = String(data: stdoutData, encoding: .utf8), !output.isEmpty {
+ print(output)
+ }
+
guard process.terminationStatus == 0 else {
- Diagnostics.error("AXConstantsGenerator exited with code \(process.terminationStatus)")
- return
+ let stderr = String(data: stderrData, encoding: .utf8) ?? ""
+ throw "AXConstantsGenerator exited with code \(process.terminationStatus): \(stderr)"
}Note: You'll need a small String: Error conformance or use a custom error type for the throw.
🤖 Prompt for AI Agents
In `@Plugins/GenerateAXConstants/plugin.swift` around lines 13 - 23, The current
Process invocation (setting process.executableURL, process.arguments, calling
process.run()/waitUntilExit() and checking process.terminationStatus) swallows
generator stderr/stdout and only logs via Diagnostics.error; change it to set
process.standardOutput and process.standardError to Pipe, run the process, read
and decode the pipe data after waitUntilExit(), and if process.terminationStatus
!= 0 then include the decoded stderr/stdout in the Diagnostics.error message and
throw an Error (e.g., make String conform to Error or use a small custom error)
so the plugin reports failure to SPM; keep the same Process symbols
(process.executableURL, process.arguments, process.run, process.waitUntilExit,
process.terminationStatus) when implementing this.
| // MARK: - (NSValue *) - (rectValue); param: (NSValue *) - (rectValue) | ||
|
|
||
| public static let convertRelativeFrame = kAXConvertRelativeFrameParameterizedAttribute |
There was a problem hiding this comment.
Generator artifact: MARK section titles contain type signatures instead of descriptive headings.
Lines 38 and 134 produce MARK headings like // MARK: - (NSValue *) - (rectValue); param: (NSValue *) - (rectValue) and // MARK: - AXTextMarkerRef; param: (NSValue *) - (pointValue). These are inline type-annotation comments from the header, not meaningful section titles. Consider tightening the group-extraction heuristics in the generator (e.g., reject candidates containing param: or C type patterns like *)).
🤖 Prompt for AI Agents
In `@Sources/AccessibilityControl/Accessibility`+ParameterizedAttributeKey.swift
around lines 38 - 40, The generated MARK comments contain raw type-signature
snippets (e.g., "// MARK: - (NSValue *) - (rectValue); param: (NSValue *) -
(rectValue)" and "// MARK: - AXTextMarkerRef; param: (NSValue *) -
(pointValue)"), which are noisy; update the generator to ignore candidate group
titles that include "param:" or C-style type patterns like "*)" or bare C type
names (e.g., "AXTextMarkerRef") and instead emit either a concise descriptive
heading or omit the MARK entirely; apply this change so symbols like
convertRelativeFrame / kAXConvertRelativeFrameParameterizedAttribute (and other
parameterized attribute constants) are grouped under meaningful section titles
rather than raw type annotations.
| private func findSDKPath() throws -> String { | ||
| let process = Process() | ||
| let pipe = Pipe() | ||
| process.executableURL = URL(fileURLWithPath: "/usr/bin/xcrun") | ||
| process.arguments = ["--show-sdk-path"] | ||
| process.standardOutput = pipe | ||
| process.standardError = FileHandle.nullDevice | ||
| try process.run() | ||
| process.waitUntilExit() | ||
| let data = pipe.fileHandleForReading.readDataToEndOfFile() | ||
| guard let path = String(data: data, encoding: .utf8)? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !path.isEmpty else { | ||
| throw ValidationError("Could not determine SDK path. Ensure Xcode or Command Line Tools are installed.") | ||
| } | ||
| return path | ||
| } |
There was a problem hiding this comment.
Potential deadlock: read pipe data before waitUntilExit().
If a future change causes the subprocess to produce more output (or stderr is redirected back to the pipe), waitUntilExit() can block indefinitely because the pipe buffer is full while nothing is draining it. xcrun --show-sdk-path output is tiny so this works today, but the safe pattern is to read first.
🔧 Proposed fix
try process.run()
- process.waitUntilExit()
let data = pipe.fileHandleForReading.readDataToEndOfFile()
+ process.waitUntilExit()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private func findSDKPath() throws -> String { | |
| let process = Process() | |
| let pipe = Pipe() | |
| process.executableURL = URL(fileURLWithPath: "/usr/bin/xcrun") | |
| process.arguments = ["--show-sdk-path"] | |
| process.standardOutput = pipe | |
| process.standardError = FileHandle.nullDevice | |
| try process.run() | |
| process.waitUntilExit() | |
| let data = pipe.fileHandleForReading.readDataToEndOfFile() | |
| guard let path = String(data: data, encoding: .utf8)? | |
| .trimmingCharacters(in: .whitespacesAndNewlines), | |
| !path.isEmpty else { | |
| throw ValidationError("Could not determine SDK path. Ensure Xcode or Command Line Tools are installed.") | |
| } | |
| return path | |
| } | |
| private func findSDKPath() throws -> String { | |
| let process = Process() | |
| let pipe = Pipe() | |
| process.executableURL = URL(fileURLWithPath: "/usr/bin/xcrun") | |
| process.arguments = ["--show-sdk-path"] | |
| process.standardOutput = pipe | |
| process.standardError = FileHandle.nullDevice | |
| try process.run() | |
| let data = pipe.fileHandleForReading.readDataToEndOfFile() | |
| process.waitUntilExit() | |
| guard let path = String(data: data, encoding: .utf8)? | |
| .trimmingCharacters(in: .whitespacesAndNewlines), | |
| !path.isEmpty else { | |
| throw ValidationError("Could not determine SDK path. Ensure Xcode or Command Line Tools are installed.") | |
| } | |
| return path | |
| } |
🤖 Prompt for AI Agents
In `@Sources/AXConstantsGenerator/main.swift` around lines 66 - 82, Read from the
pipe before waiting for the process to exit to avoid a potential deadlock; in
findSDKPath() move the call to pipe.fileHandleForReading.readDataToEndOfFile()
to occur immediately after try process.run() (and before
process.waitUntilExit()), so the stdout is drained while the child runs (keeping
standardError as FileHandle.nullDevice as currently set) and then call
process.waitUntilExit() and proceed to convert/trimming the data into path.
1 participant
No description provided.