fix: bugfix and refactor Windows start script - #121
Conversation
Review pass over scripts/start-windows.ps1: Bugs - Add `dart pub get` in backend/ before `dart_frog build` (fresh checkouts failed with unresolved deps). - Replace fixed `Start-Sleep -Seconds 3` with port polling (Wait-ForPort) and exit if the backend never starts listening. - Pass `-s <device>` to `adb reverse` (was ambiguous/failing with multiple devices attached). - Scope and restore `$env:PORT` so it no longer leaks into the session. Refactor - Dedupe device detection into a single Get-ConnectedAndroidDevice helper. - Hoist magic values into a param() block (-Port, -Avd) + SDK path vars. - Use Push-Location/Pop-Location with try/finally around the backend build. - Stay PowerShell 5.1 compatible (avoid Start-Process -Environment). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 5 minutes and 13 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the start-windows.ps1 script by mirroring the Android startup script's capabilities, including dynamic port management, automatic emulator booting, and port forwarding. The review feedback highlights three key areas for improvement: narrowing down the process-killing logic to only target listening ports, supporting custom Android SDK locations via environment variables, and replacing the slow Test-NetConnection cmdlet with a faster .NET TCP client to ensure reliable port polling.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| $portPids = (Get-NetTCPConnection -LocalPort $Port -ErrorAction SilentlyContinue).OwningProcess | | ||
| Sort-Object -Unique | ||
| foreach ($pidToKill in $portPids) { | ||
| Stop-Process -Id $pidToKill -Force -ErrorAction SilentlyContinue | ||
| Write-Host "Killed process $pidToKill on port $Port" | ||
| } |
There was a problem hiding this comment.
Calling Get-NetTCPConnection without specifying -State Listen will return all TCP connections, including active outbound connections where the local ephemeral port happens to match $Port. This could lead to accidentally killing unrelated processes (like web browsers or IDEs). Additionally, wrapping the result in @(...) and checking if it's not null prevents errors when no processes are listening.
$connections = Get-NetTCPConnection -LocalPort $Port -State Listen -ErrorAction SilentlyContinue
if ($connections) {
$portPids = @($connections.OwningProcess) | Sort-Object -Unique
foreach ($pidToKill in $portPids) {
Stop-Process -Id $pidToKill -Force -ErrorAction SilentlyContinue
Write-Host "Killed process $pidToKill on port $Port"
}
}
| Set-Location $projectRoot | ||
|
|
||
| # Android SDK tool locations. | ||
| $sdkRoot = Join-Path $env:LOCALAPPDATA "Android\Sdk" |
There was a problem hiding this comment.
To support developers who have installed the Android SDK in a custom location, it is highly recommended to check the ANDROID_HOME and ANDROID_SDK_ROOT environment variables before falling back to the default %LOCALAPPDATA% path.
$sdkRoot = if ($env:ANDROID_HOME -and (Test-Path $env:ANDROID_HOME)) {
$env:ANDROID_HOME
} elseif ($env:ANDROID_SDK_ROOT -and (Test-Path $env:ANDROID_SDK_ROOT)) {
$env:ANDROID_SDK_ROOT
} else {
Join-Path $env:LOCALAPPDATA "Android\Sdk"
}
| function Wait-ForPort { | ||
| param([int]$PortNumber, [int]$TimeoutSeconds = 30) | ||
| $deadline = (Get-Date).AddSeconds($TimeoutSeconds) | ||
| while ((Get-Date) -lt $deadline) { | ||
| if (Test-NetConnection -ComputerName "localhost" -Port $PortNumber -WarningAction SilentlyContinue -InformationLevel Quiet) { | ||
| return $true | ||
| } | ||
| Start-Sleep -Milliseconds 500 | ||
| } | ||
| return $false | ||
| } |
There was a problem hiding this comment.
Test-NetConnection is notoriously slow when a port is closed because it performs additional diagnostics (like ICMP pings and DNS resolution) and has a long built-in timeout. This can cause the 30-second timeout to be reached with very few polling attempts. Using .NET's System.Net.Sockets.TcpClient is instantaneous and much more reliable for polling.
function Wait-ForPort {
param([int]$PortNumber, [int]$TimeoutSeconds = 30)
$deadline = (Get-Date).AddSeconds($TimeoutSeconds)
while ((Get-Date) -lt $deadline) {
$client = New-Object System.Net.Sockets.TcpClient
try {
$client.Connect("localhost", $PortNumber)
return $true
}
catch {}
finally {
if ($client) { $client.Close() }
}
Start-Sleep -Milliseconds 500
}
return $false
}
1 participant
Summary
Review + bugfix + refactor pass over
scripts/start-windows.ps1(added in the recent Windows-support commits). The script now matches the behavior ofscripts/start-android.shmore closely and is robust on a fresh Windows checkout.Bugs fixed
dart pub getinbackend/beforedart_frog build; backend deps were never resolved before, so a clean checkout failed.Start-Sleep -Seconds 3withWait-ForPortpolling, and the script now exits with an error if the backend never starts listening (instead of launching the app against a dead backend).adb reversewith multiple devices — now targets the specific device via-s <device>; previously ambiguous/failing when an emulator and a phone were both attached.$env:PORTsession leak — now scoped to the child process and restored afterward.Refactor / cleanup
Get-ConnectedAndroidDevicehelper.param()block:-Port(default 8080) and-Avd(defaultPixel_10), plus SDK path variables, with a doc comment and usage example.Push-Location/Pop-Locationwithtry/finallyso a build failure can't strand the shell inbackend/.Compatibility
Deliberately avoided
Start-Process -Environment(PowerShell 7.4+ only) since Windows ships PowerShell 5.1 by default — the script stays 5.1-compatible.Testing
pwshis not available in the dev environment, so the script was reviewed by hand rather than executed. Recommend a manual run on a Windows machine before merge.Follow-up (not in this PR)
There is no Windows port of
scripts/regenerate-build.sh(Prisma client + backend codegen). This script, likestart-android.sh, assumes that generated code already exists. Aregenerate-build.ps1could be added separately.🤖 Generated with Claude Code