feat: add device events and file access#5
Open
mozharovsky wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds device attach/detach event streaming, high-level AFC filesystem operations and metadata types, HouseArrest app-container vending, CLI ChangesDevice Attachment Events, File Access, and Monitoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
53-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove “device events” from the planned-services list.
This sentence now conflicts with the Features section and CLI docs where
watchis already available.Suggested doc tweak
-and planned services such as pairing creation, device events, syslog, crash +and planned services such as pairing creation, syslog, crash reports, debugserver, developer image mounting, backup, and restore.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 53 - 54, Remove the phrase "device events" from the planned-services list in the README so it no longer conflicts with the existing Features/CLI docs; locate the sentence containing "and planned services such as pairing creation, device events, syslog, crash reports, debugserver, developer image mounting, backup, and restore." and delete only the "device events" item (and adjust punctuation/commas accordingly) so the list remains grammatically correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Docs/Roadmap.md`:
- Line 45: Update the roadmap wording that currently says "watching devices and
moving files" to a broader, more accurate phrase such as "watching devices and
managing files" to reflect the full file command suite
(list/info/pull/push/mkdir/rm/mv); replace the existing phrase wherever it
appears in the Docs/Roadmap.md content to ensure the scope is correctly
communicated.
In `@README.md`:
- Line 118: The "files" subcommand description is too narrow; update the README
entry for the "files" subcommand to list all supported operations (e.g., browse,
copy, upload, metadata, mkdir, remove, move) and give a short phrase like
"Browse, copy, upload, inspect metadata, create directories, remove and move
files via AFC or HouseArrest" so users see the full capability; edit the line
that currently reads "files Browse and copy files through AFC
or HouseArrest." to the expanded description for the "files" subcommand.
In `@Sources/RorkDevice/HouseArrest/HouseArrestClient.swift`:
- Around line 26-37: The openApplicationContainer call vends the underlying
connection to AFC and must make the HouseArrestClient single-use; update
openApplicationContainer (and HouseArrestClient) to record that the connection
has been vended (e.g. set a Bool flag like isVended or nil out the connection)
immediately after validating the response and before returning
AFCClient(connection: connection), ensure subsequent calls to
openApplicationContainer check that flag/connection and throw a clear error
instead of attempting to write a plist into an AFC stream, and make the
flag/connection mutation thread-safe if HouseArrestClient can be used
concurrently.
In `@Sources/RorkDevice/USBMux/USBMuxClient.swift`:
- Around line 123-129: The catch block for read failures currently closes the
connection and always finishes the AsyncStream with an error (via
continuation.finish(throwing: error)), which causes a normal daemon socket close
to be reported as a failure; update the catch to detect the "remote/daemon
closed the listen socket" condition (instead of Task.isCancelled) and call
continuation.finish() for that case: in the catch associated with the read loop
where connection.close() is invoked, check the caught error for the specific
EOF/connection-closed error that usbmux emits and call continuation.finish() for
that error path, otherwise preserve the existing behavior (finish(throwing:
error) or finish() when Task.isCancelled). Ensure you reference the existing
symbols connection.close(), Task.isCancelled, continuation.finish() and
continuation.finish(throwing: error) when making the change.
- Around line 305-308: validateUSBMuxResult currently treats responses with a
missing or non-numeric "Number" field as success; update validateUSBMuxResult(_
response: [String: Any], operation: String) to require that response["Number"]
exists and is an NSNumber and throw a RorkDeviceError.transport when it's
missing or not an NSNumber (e.g. "usbmux \(operation) missing or invalid Number
in response") so that callers like deviceEvents() and connect(...) fail fast on
malformed control replies.
In `@Sources/RorkDeviceCLI/RorkDeviceCommand.swift`:
- Around line 89-105: The CLI currently ignores the --container flag when
bundleIdentifier is nil which can lead to operating on the wrong AFC root;
update RorkDeviceCommand to validate flags (e.g., in afcClient() or a
command-validate step) and reject/throw a user-facing error if container is true
while bundleIdentifier is nil, referencing the bundleIdentifier property and
container Flag so callers must supply --bundle-identifier when using
--container; ensure the error is clear and prevents proceeding (do not silently
fall back to session.openAFC()).
In `@Tests/RorkDeviceTests/AFCClientTests.swift`:
- Around line 173-176: The test currently decodes file bytes with
String(decoding:as:) which tolerates invalid UTF‑8; change the assertion to use
strict UTF‑8 decoding by creating a String via String(data:encoding: .utf8) from
the result of client.contentsOfFile(at:) and fail the test if that returns nil
(e.g., XCTAssertNotNil or guard+XCTFail), then compare the unwrapped string to
"hello world"; keep the existing check of connection.sent mapped with
afcOperation unchanged.
---
Outside diff comments:
In `@README.md`:
- Around line 53-54: Remove the phrase "device events" from the planned-services
list in the README so it no longer conflicts with the existing Features/CLI
docs; locate the sentence containing "and planned services such as pairing
creation, device events, syslog, crash reports, debugserver, developer image
mounting, backup, and restore." and delete only the "device events" item (and
adjust punctuation/commas accordingly) so the list remains grammatically
correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 716272a3-26f3-456c-809f-cbaf3baf72e1
📒 Files selected for processing (15)
Docs/Roadmap.mdREADME.mdSources/RorkDevice/AFC/AFCClient.swiftSources/RorkDevice/HouseArrest/HouseArrestClient.swiftSources/RorkDevice/Public/Device.swiftSources/RorkDevice/Public/DeviceSession.swiftSources/RorkDevice/Public/RorkDevice.swiftSources/RorkDevice/USBMux/USBMuxClient.swiftSources/RorkDeviceCLI/RorkDeviceCommand.swiftTests/RorkDeviceCLITests/RorkDeviceCLITests.swiftTests/RorkDeviceTests/AFCClientTests.swiftTests/RorkDeviceTests/DeviceClientIntegrationTests.swiftTests/RorkDeviceTests/FakeUSBMuxDaemon.swiftTests/RorkDeviceTests/HouseArrestClientTests.swiftTests/RorkDeviceTests/USBMuxClientIntegrationTests.swift
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tests/RorkDeviceTests/USBMuxClientIntegrationTests.swift (1)
86-88: ⚡ Quick winDocument the intent of the empty catch block.
The empty catch block appears intentional (to suppress cancellation errors during the test), but adding a brief comment would make this clearer to future maintainers.
📝 Suggested documentation
let task = Task { do { for try await _ in client.deviceEvents() {} - } catch {} + } catch { + // Suppress cancellation errors; we're testing cleanup, not error handling + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/RorkDeviceTests/USBMuxClientIntegrationTests.swift` around lines 86 - 88, Add a brief comment inside the empty catch block that explains why exceptions from client.deviceEvents() are intentionally suppressed (e.g., to ignore CancellationError during test shutdown/teardown). Locate the catch immediately following the for try await _ in client.deviceEvents() loop and add a one-line comment such as "// Intentionally ignore cancellation/errors from deviceEvents() during test teardown" so future maintainers understand the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Tests/RorkDeviceTests/USBMuxClientIntegrationTests.swift`:
- Around line 86-88: Add a brief comment inside the empty catch block that
explains why exceptions from client.deviceEvents() are intentionally suppressed
(e.g., to ignore CancellationError during test shutdown/teardown). Locate the
catch immediately following the for try await _ in client.deviceEvents() loop
and add a one-line comment such as "// Intentionally ignore cancellation/errors
from deviceEvents() during test teardown" so future maintainers understand the
intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f41e9e71-873a-41cb-8854-ccf78a8c55c4
📒 Files selected for processing (7)
Sources/RorkDevice/AFC/AFCClient.swiftSources/RorkDevice/HouseArrest/HouseArrestClient.swiftSources/RorkDevice/Public/DeviceSession.swiftSources/RorkDevice/USBMux/USBMuxClient.swiftTests/RorkDeviceTests/DeviceClientIntegrationTests.swiftTests/RorkDeviceTests/FakeUSBMuxDaemon.swiftTests/RorkDeviceTests/USBMuxClientIntegrationTests.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- Tests/RorkDeviceTests/DeviceClientIntegrationTests.swift
- Sources/RorkDevice/USBMux/USBMuxClient.swift
- Sources/RorkDevice/HouseArrest/HouseArrestClient.swift
- Sources/RorkDevice/AFC/AFCClient.swift
- Tests/RorkDeviceTests/FakeUSBMuxDaemon.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
USBMuxClientandDeviceClientAFCClientinstancesrorkdevice watchandrorkdevice files ...CLI commandsValidation
swift testgit diff --checkswift run rorkdevice --helpswift run rorkdevice files list --helpSummary by CodeRabbit
New Features
watchcommand plusfilessuite (list, info, pull, push, mkdir, rm, mv) and file-access optionsDocumentation
Tests