fix: optimize "simctl list" performance by eliminating it #115
Conversation
|
Warning Rate limit exceeded@gmegidish has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughThe PR shifts two device discovery mechanisms from live command-line queries to filesystem-based configuration parsing: AVD discovery now reads Changes
Sequence DiagramsequenceDiagram
autonumber
participant client as Caller
participant common as GetAllControllableDevices()
participant avd as getAVDDetails()
participant fs as Filesystem (.ini)
participant sim as getSimulators()
participant plist as device.plist
client->>common: Request all devices (includeOffline)
common->>common: Start total timer
rect rgb(200, 220, 240)
note over common,avd: Android Device Discovery
common->>common: Start Android phase timer
common->>avd: Get offline AVD details
avd->>fs: Glob *.ini files in ~/.android/avd/
fs-->>avd: List of .ini files
loop For each .ini file
avd->>fs: Read .ini (path, display name, target)
fs-->>avd: Config data
avd->>fs: Read config.ini from avd directory
fs-->>avd: API level, device info
end
avd->>avd: Convert API level to version
avd-->>common: AVDInfo list
common->>common: Record Android duration & count
end
rect rgb(240, 220, 200)
note over common,sim: iOS Simulator Discovery
common->>common: Start simulator phase timer
common->>sim: Get simulators via plist
sim->>fs: Iterate CoreSimulator Devices directory
loop For each device directory
sim->>fs: Read device.plist
fs-->>sim: Plist data
sim->>plist: Unmarshal to devicePlist struct
sim->>sim: Map state (3=Booted, other=Shutdown)
end
sim-->>common: Simulator list
common->>common: Record simulator duration & count
end
common->>common: Compute total duration
common->>common: Log per-category metrics & counts
common-->>client: All devices with timing metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
devices/avd.go (1)
109-112: Consider using AVD name as fallback when displayName is empty.If
avd.ini.displaynameis not set in config.ini (which can happen for older or manually created AVDs), the entire AVD is skipped. This could cause valid AVDs to not appear in the offline device list.displayName := configData.Section("").Key("avd.ini.displayname").String() if displayName == "" { - continue + // fallback to avd name if displayname is not set + displayName = strings.TrimSuffix(filepath.Base(iniFile), ".ini") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
devices/avd.go(2 hunks)devices/common.go(3 hunks)devices/simulator.go(4 hunks)go.mod(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
devices/common.go (3)
utils/logger.go (1)
Verbose(19-23)devices/ios.go (1)
ListIOSDevices(89-107)devices/simulator.go (1)
GetSimulators(124-175)
⏰ 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 (7)
devices/common.go (1)
62-139: Well-structured timing instrumentation for performance monitoring.The timing implementation correctly captures per-phase durations and device counts without affecting the core device discovery logic. The verbose logging at the end provides useful diagnostics for identifying performance bottlenecks.
devices/simulator.go (4)
36-42: LGTM - devicePlist struct correctly maps plist fields.The struct properly uses plist tags to map the device.plist fields.
123-134: Filesystem-based discovery is a good performance optimization.Reading directly from the filesystem avoids spawning
xcrun simctl listprocesses. The error handling correctly surfaces directory access issues while the function gracefully handles missing or inaccessible device directories.
143-154: Silent error handling for individual devices is appropriate.Skipping devices without
device.plistor with invalid plist data is the right approach - these are likely incomplete/corrupted simulator instances that shouldn't block discovery of valid simulators.
156-162: I'll help you verify this review comment about incomplete state mapping in the simulator. Let me start by examining the relevant code sections.
<function_calls>
find . -name "simulator.go" -type f | head -20
</function_calls>
<function_calls>
wc -l devices/simulator.go 2>/dev/null || echo "File not found, searching..."
</function_calls>
<function_calls>
cat -n devices/simulator.go | sed -n '150,170p'
</function_calls>
<function_calls>
cat -n devices/simulator.go | sed -n '410,420p'
</function_calls>
<function_calls>
rg -n "State\s*=|state\s*=" devices/simulator.go -C 2
</function_calls>devices/avd.go (2)
20-47: Comprehensive API level mapping for offline AVD version display.The mapping covers API levels 21-36 (Android 5.0 to 16.0), which is a good range. The fallback to return the API level as-is handles future/unknown API levels gracefully.
71-129: Filesystem-based AVD discovery is a solid performance improvement.Reading INI files directly avoids the overhead of spawning
avdmanagerand parsing its textual output. The implementation correctly handles the two-level INI structure (.ini→config.ini) and gracefully skips problematic entries with verbose logging.
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.