Feature/experiment#16
Merged
Merged
Conversation
- Introduces DeviceManager interface for generic platform operations - Migrates iOS logic to IOSManager and Android logic to AndroidManager - Standardizes executeDeviceAction looping over managers - Refactors app.go, media.go, and dashboard.go to use manager lookups
- Fix shell injection in create.go and logs.go - Fix config directory creation to not fallback to /tmp - Fix copy heuristic for device detection - Fix silent error swallowing across device teardowns and generations - Merge shutdown command into stop command as alias - Extract hardcoded strings into constants - Update CI configuration to run all tests - Ignore coverage output files
There was a problem hiding this comment.
Pull request overview
This PR refactors device operations into a unified DeviceManager abstraction (iOS + Android), updates command implementations to use the new manager layer, and improves safety by removing shell-based log filtering. It also updates config path APIs to return errors and adjusts tests/CI accordingly.
Changes:
- Introduces
DeviceManagerinterface with newIOSManagerandAndroidManagerimplementations and routes device commands through them. - Reworks log streaming to avoid
sh -c | grep(shell injection surface) by handling filtering inside the TUI log viewer. - Updates config helpers (
GetConfigDir/GetConfigPath) to return(string, error)and updates tests and CLI commands to handle errors; expands CI test scope togo test -race ./....
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration_test.go | Updates integration tests for new config helper error-returning signatures. |
| tests/config_test.go | Updates unit tests to handle (string, error) config helpers. |
| cmd/ui.go | Normalizes device/platform display strings and state formatting. |
| cmd/root.go | Removes the standalone shutdown command registration (now handled via aliases). |
| cmd/media.go | Switches active-device detection to the unified FindRunningDevice. |
| cmd/manager.go | Adds the new manager registry and unified FindRunningDevice implementation. |
| cmd/logs.go | Removes shell-based grep piping and passes filter into the TUI viewer. |
| cmd/log_viewer.go | Adds viewer-side filtering support for streamed logs. |
| cmd/list_benchmark_test.go | Adds benchmarks for simulator/emulator listing and runtime formatting. |
| cmd/ios_manager.go | Implements iOS device operations behind DeviceManager. |
| cmd/android_manager.go | Implements Android device operations behind DeviceManager. |
| cmd/device.go | Refactors stop/restart/delete/erase/clone/start to use the manager interface; folds shutdown into stop aliases. |
| cmd/dashboard.go | Updates dashboard stop behavior to use managers. |
| cmd/create.go | Removes sh -c usage for AVD creation and improves error handling in the wizard. |
| cmd/copy.go | Adjusts argument parsing heuristics for optional device argument. |
| cmd/constants.go | Adds NameIOS/NameAndroid constants for display normalization. |
| cmd/config.go | Updates config directory/path helpers to return errors and threads that through config load/save. |
| cmd/config_cmd.go | Updates config path command to handle config-path errors. |
| cmd/completion.go | Adds error handling for shell completion generation. |
| cmd/app.go | Switches install/uninstall device resolution to the unified FindRunningDevice. |
| .gitignore | Updates ignore rules for coverage artifacts. |
| .github/workflows/ci.yml | Expands race-test scope from ./tests/ to ./.... |
Comments suppressed due to low confidence (1)
cmd/logs.go:108
- This
if filter != "" { ... }block is empty. Since filtering is always handled byrunLogViewer, remove the conditional to avoid empty-branch lints and simplify the control flow.
}
stdout, err := logCmd.StdoutPipe()
if err != nil {
return err
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Test Configuration:
Checklist:
Screenshots (if applicable):
Please add screenshots to help explain your changes.
Additional Notes:
Add any other notes about the pull request here.