feat: support installing wda from local directory #123
Conversation
WalkthroughReworked the simulator's WebDriverAgent installation to choose architecture-specific filenames/URLs at runtime, prefer a local WDA when MOBILECLI_WDA_PATH is set, and otherwise download the appropriate ZIP from GitHub; removed the exported DownloadWebDriverAgent method and added internal helpers and conditional cleanup. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as InstallWebDriverAgent
participant FS as Local Filesystem
participant Env as MOBILECLI_WDA_PATH
participant Downloader as GitHub
participant Unzipper as Unzip Logic
Caller->>Env: check MOBILECLI_WDA_PATH
alt local path set
Caller->>FS: stat path / verify filename
alt file matches expected filename
FS-->>Caller: local file path
Caller->>Unzipper: unzip local file
Unzipper-->>Caller: success / error
else file missing or wrong name
Caller->>Downloader: build arch-specific URL
Downloader-->>Caller: download ZIP
Caller->>Unzipper: unzip downloaded ZIP
Unzipper-->>Caller: success / error
Caller->>FS: conditional cleanup downloaded ZIP
end
else no local path
Caller->>Downloader: build arch-specific URL
Downloader-->>Caller: download ZIP
Caller->>Unzipper: unzip downloaded ZIP
Unzipper-->>Caller: success / error
Caller->>FS: cleanup downloaded ZIP
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
devices/simulator.go (2)
301-304: Extract hardcoded version to a named constant.The version
v9.15.1is embedded in the URL string. Extracting it to a constant improves maintainability and makes version updates more visible.+const webDriverAgentVersion = "v9.15.1" + func getWebdriverAgentDownloadUrl(arch string) string { filename := getWebdriverAgentFilename(arch) - return "https://github.com/appium/WebDriverAgent/releases/download/v9.15.1/" + filename + return "https://github.com/appium/WebDriverAgent/releases/download/" + webDriverAgentVersion + "/" + filename }
346-356: Use streaming copy instead of loading entire file into memory.
os.ReadFileloads the entire WDA zip (~20-50MB) into memory before writing. Usingio.Copywith file streams is more memory-efficient.- input, err := os.ReadFile(localPath) + srcFile, err := os.Open(localPath) if err != nil { _ = os.Remove(tmpFile.Name()) return "", fmt.Errorf("failed to read local WebDriverAgent: %w", err) } + defer srcFile.Close() - err = os.WriteFile(tmpFile.Name(), input, 0644) + dstFile, err := os.Create(tmpFile.Name()) + if err != nil { + return "", fmt.Errorf("failed to create temp file: %w", err) + } + defer dstFile.Close() + + _, err = io.Copy(dstFile, srcFile) if err != nil { _ = os.Remove(tmpFile.Name()) return "", fmt.Errorf("failed to copy local WebDriverAgent: %w", err) }This requires adding
"io"to the imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
devices/simulator.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
devices/simulator.go (1)
utils/logger.go (1)
Verbose(19-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (3)
devices/simulator.go (3)
294-299: LGTM!The architecture mapping correctly translates Go's
amd64tox86_64for the WDA binary naming convention, witharm64as the default for Apple Silicon.
306-329: LGTM!The GitHub download flow correctly handles temp file creation, cleanup on failure, and logs download progress. The silent fallback when file stat fails for logging purposes is acceptable.
367-375: LGTM!The integration with the new
downloadWebDriverAgentmethod is correct and maintains the existing error handling and cleanup logic.
devices/simulator.go
Outdated
|
|
||
| tmpFile, err := os.CreateTemp("", "wda-*.zip") | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create temp file: %v", err) | ||
| } | ||
| _ = tmpFile.Close() | ||
|
|
||
| input, err := os.ReadFile(localPath) | ||
| if err != nil { | ||
| _ = os.Remove(tmpFile.Name()) | ||
| return "", fmt.Errorf("failed to read local WebDriverAgent: %w", err) | ||
| } | ||
|
|
||
| err = os.WriteFile(tmpFile.Name(), input, 0644) | ||
| if err != nil { | ||
| _ = os.Remove(tmpFile.Name()) | ||
| return "", fmt.Errorf("failed to copy local WebDriverAgent: %w", err) | ||
| } | ||
|
|
||
| return tmpFile.Name(), nil | ||
| } | ||
|
|
||
| utils.Verbose("Local WebDriverAgent not found at: %s", localPath) | ||
| } | ||
|
|
||
| return s.downloadWebDriverAgentFromGitHub() | ||
| } | ||
|
|
||
| func (s SimulatorDevice) InstallWebDriverAgent(onProgress func(string)) error { | ||
| if onProgress != nil { | ||
| onProgress("Downloading WebDriverAgent") | ||
| } | ||
|
|
||
| file, err := s.DownloadWebDriverAgent() | ||
| file, err := s.downloadWebDriverAgent() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any references to the public DownloadWebDriverAgent method
rg -n --type=go '\bDownloadWebDriverAgent\b' -g '!devices/simulator.go'Repository: mobile-next/mobilecli
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search in all files including tests
rg -n '\bDownloadWebDriverAgent\b'Repository: mobile-next/mobilecli
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Read the simulator.go file around the specified lines to see context
wc -l devices/simulator.goRepository: mobile-next/mobilecli
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Search for all function/method definitions in simulator.go
rg -n '^\s*func.*DownloadWebDriver|^\s*func.*download' devices/simulator.goRepository: mobile-next/mobilecli
Length of output: 218
🏁 Script executed:
#!/bin/bash
# Search for all exported (capitalized) methods in simulator.go
rg -n '^\s*func.*\(s.*Simulator.*\)\s+[A-Z]' devices/simulator.goRepository: mobile-next/mobilecli
Length of output: 2289
🏁 Script executed:
#!/bin/bash
# Check git log for changes to DownloadWebDriverAgent
git log --oneline --all -S 'DownloadWebDriverAgent' -- devices/simulator.go | head -20Repository: mobile-next/mobilecli
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Check the initial commit to see if DownloadWebDriverAgent existed then
git show 88cf9fe:devices/simulator.go | rg -n 'DownloadWebDriverAgent'Repository: mobile-next/mobilecli
Length of output: 181
🏁 Script executed:
#!/bin/bash
# Check current branch and see what changed
git diff 88cf9fe HEAD -- devices/simulator.go | head -100Repository: mobile-next/mobilecli
Length of output: 3027
🏁 Script executed:
#!/bin/bash
# Get the specific diff around where DownloadWebDriverAgent was removed
git diff 88cf9fe HEAD -- devices/simulator.go | grep -A 20 -B 5 'DownloadWebDriverAgent' || echo "Method not in diff"Repository: mobile-next/mobilecli
Length of output: 1844
🏁 Script executed:
#!/bin/bash
# Check if there's a CHANGELOG or deprecation notice
find . -name "CHANGELOG*" -o -name "MIGRATION*" -o -name "BREAKING*" | head -10Repository: mobile-next/mobilecli
Length of output: 79
🏁 Script executed:
#!/bin/bash
# Check CHANGELOG for mentions of DownloadWebDriverAgent or WebDriverAgent changes
rg -n 'DownloadWebDriverAgent|Breaking|WebDriverAgent' CHANGELOG.md | head -20Repository: mobile-next/mobilecli
Length of output: 47
Based on my verification, I can now provide the rewritten review comment:
Public API method DownloadWebDriverAgent was removed without deprecation notice or changelog entry.
The exported method DownloadWebDriverAgent that existed in the initial commit has been removed and replaced with a private downloadWebDriverAgent helper. This is a breaking change for external consumers of this package. External code that calls SimulatorDevice.DownloadWebDriverAgent() will fail after this change. Consider either: maintaining backward compatibility by keeping a deprecated public method, or documenting this as a breaking change in the CHANGELOG and migration guide.
🤖 Prompt for AI Agents
In devices/simulator.go around lines 331 to 365, the refactor removed the
exported SimulatorDevice.DownloadWebDriverAgent() public API and replaced it
with a private downloadWebDriverAgent(), which is a breaking change for external
consumers; restore backward compatibility by reintroducing a public
DownloadWebDriverAgent method on SimulatorDevice that simply calls the new
private downloadWebDriverAgent and returns its results, mark the public method
as deprecated in its Go doc (pointing to the new behavior) so callers see the
migration path, and add an entry to the CHANGELOG under "Unreleased" mentioning
the deprecation (or, alternatively, if you intend to break the API, add a clear
breaking-change note to the CHANGELOG and migration guide instead of restoring
the public method).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
devices/simulator.go (2)
301-304: Consider making the WDA version configurable.The WebDriverAgent version is hardcoded to v9.15.1. Consider making this configurable via an environment variable (e.g.,
MOBILECLI_WDA_VERSION) or a package-level constant to allow users to specify different versions and make updates easier.Example implementation:
+const defaultWdaVersion = "v9.15.1" + +func getWdaVersion() string { + if version := os.Getenv("MOBILECLI_WDA_VERSION"); version != "" { + return version + } + return defaultWdaVersion +} + func getWebdriverAgentDownloadUrl(arch string) string { filename := getWebdriverAgentFilename(arch) - return "https://github.com/appium/WebDriverAgent/releases/download/v9.15.1/" + filename + return "https://github.com/appium/WebDriverAgent/releases/download/" + getWdaVersion() + "/" + filename }
336-348: Document the MOBILECLI_WDA_PATH environment variable.The new
MOBILECLI_WDA_PATHenvironment variable enables local WDA installation but isn't documented in the code. Consider adding a comment above theInstallWebDriverAgentmethod or in a package-level doc comment explaining:
- The purpose of
MOBILECLI_WDA_PATH- Expected directory structure (should contain architecture-specific ZIP files)
- Fallback behavior when not set or file not found
Example:
// InstallWebDriverAgent installs the WebDriverAgent app on the simulator. // If MOBILECLI_WDA_PATH is set, it will look for the architecture-specific // WDA bundle in that directory (e.g., WebDriverAgentRunner-Build-Sim-arm64.zip). // If not found locally, it downloads the appropriate bundle from GitHub releases. // The onProgress callback is called to report installation progress. func (s SimulatorDevice) InstallWebDriverAgent(onProgress func(string)) error {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
devices/simulator.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
devices/simulator.go (2)
utils/logger.go (1)
Verbose(19-23)utils/zipfile.go (1)
Unzip(12-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
devices/simulator.go (4)
11-11: LGTM!The
runtimeimport is correctly added to support architecture detection viaruntime.GOARCH.
306-329: LGTM!The download logic is well-structured with proper error handling and cleanup. The verbose logging provides good visibility into the download process.
Note: This method's success depends on
getWebdriverAgentDownloadUrlreturning a valid URL, which relies on proper architecture validation (see comment on lines 294-299).
331-366: Local-first installation pattern is well-implemented.The refactored logic correctly prioritizes a local WDA path and falls back to GitHub download, with appropriate conditional cleanup. The verbose logging provides good visibility into the installation flow.
However, the logic relies on
getWebdriverAgentFilenamereturning valid filenames. If an unsupported architecture is detected, this could construct invalid paths. Ensure architecture validation is added as suggested in the comment on lines 294-299.
370-370: Good improvement to error wrapping.Changing from
%vto%wenables proper error chain unwrapping, which is a Go best practice.
| filename := getWebdriverAgentFilename(arch) | ||
| return "https://github.com/appium/WebDriverAgent/releases/download/v9.15.1/" + filename | ||
| } | ||
|
|
||
| func (s SimulatorDevice) downloadWebDriverAgentFromGitHub() (string, error) { | ||
| url := getWebdriverAgentDownloadUrl(runtime.GOARCH) |
There was a problem hiding this comment.
Add validation for unsupported architectures.
The function defaults to arm64 for any architecture that isn't "amd64", which may cause issues if the code runs on unsupported architectures (e.g., "386", "arm", "mips", etc.). Consider adding explicit validation and returning an error for unsupported architectures.
Apply this diff to add validation:
func getWebdriverAgentFilename(arch string) string {
- if arch == "amd64" {
+ switch arch {
+ case "amd64":
return "WebDriverAgentRunner-Build-Sim-x86_64.zip"
+ case "arm64":
+ return "WebDriverAgentRunner-Build-Sim-arm64.zip"
+ default:
+ // Unsupported architecture
+ return ""
}
- return "WebDriverAgentRunner-Build-Sim-arm64.zip"
}Then update the callers to check for empty strings and return appropriate errors.
📝 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 getWebdriverAgentFilename(arch string) string { | |
| if arch == "amd64" { | |
| return "WebDriverAgentRunner-Build-Sim-x86_64.zip" | |
| } | |
| return "WebDriverAgentRunner-Build-Sim-arm64.zip" | |
| } | |
| func getWebdriverAgentFilename(arch string) string { | |
| switch arch { | |
| case "amd64": | |
| return "WebDriverAgentRunner-Build-Sim-x86_64.zip" | |
| case "arm64": | |
| return "WebDriverAgentRunner-Build-Sim-arm64.zip" | |
| default: | |
| // Unsupported architecture | |
| return "" | |
| } | |
| } |
🤖 Prompt for AI Agents
In devices/simulator.go around lines 294-299, getWebdriverAgentFilename
currently defaults to the arm64 filename for any non-"amd64" arch; change it to
validate supported architectures explicitly (e.g., accept "amd64" and "arm64"
only), return an empty string for unsupported values, and do not silently assume
arm64. Update every caller to check the returned string and return or propagate
a clear error when it is empty (or accept an error type) so unsupported
architectures are handled upstream instead of producing incorrect filenames.
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.