Conversation
hal-eisen-adfa
commented
Apr 25, 2026
hal-eisen-adfa
commented
- WebServer socket tagging -- No more UntaggedSocketViolation from our WebServer (the ones showing are from OkHttp and JDI, outside our control)
- InstallationViewModel.checkStorageAndNotify() -- Now runs via withContext(Dispatchers.IO) inside a viewModelScope.launch block, no main-thread disk reads
- PermissionsFragment.startIdeSetup() -- Storage check now inside a coroutine scope, no synchronous main-thread call before the async block
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 6 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR refactors the IDE setup flow to improve coroutine handling and state management. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/itsaky/androidide/fragments/onboarding/PermissionsFragment.kt`:
- Around line 242-245: The call to viewModel.isSetupComplete() is performing
disk I/O on the main thread; change the call site in PermissionsFragment (inside
the viewLifecycleScope.launch) to perform the check on IO by using
withContext(Dispatchers.IO) { viewModel.isSetupComplete() } (or make
viewModel.isSetupComplete()/checkToolsIsInstalled() suspend and call it from
IO), then act on the result on the main thread (calling (activity as?
OnboardingActivity)?.tryNavigateToMainIfSetupIsCompleted() only after the IO
call returns). Also search other call sites of isSetupComplete(),
isPolicyRespected, and onUserIllegallyRequestedNextPage and plan to convert them
similarly if they run on the main thread.
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 138-141: The TrafficStats tag is being set only during
ServerSocket creation (TrafficStats.setThreadStatsTag(0xC0DE)) then cleared
before the accept loop, so accepted client sockets remain untagged and I/O will
still trigger UntaggedSocketViolation; also an exception during ServerSocket()
or bind() can leak the tag. Fix by ensuring the tag is active for the lifetime
of accept/IO: either keep the tag set across the accept loop, or (preferably)
clear it after setup in a finally block and explicitly tag/untag each accepted
client socket using TrafficStats.tagSocket(clientSocket) and
TrafficStats.untagSocket(clientSocket) around the per-client I/O, and wrap the
ServerSocket creation/bind in try...finally to always call
TrafficStats.clearThreadStatsTag() if you choose to set the thread tag for
setup.
In `@app/src/main/java/com/itsaky/androidide/viewmodel/InstallationViewModel.kt`:
- Around line 63-71: checkToolsIsInstalled() performs a filesystem stat on the
main dispatcher, causing StrictMode disk-read violations; wrap the call in
withContext(Dispatchers.IO) inside the existing viewModelScope.launch, e.g. call
val installed = withContext(Dispatchers.IO) { checkToolsIsInstalled() } and then
branch on installed to call _state.update { InstallationComplete } (or switch
back to Main if you prefer to keep state mutation on the main thread); apply the
same pattern where PermissionsFragment.startIdeSetup calls
viewModel.isSetupComplete() after checkStorageAndNotify to avoid main-thread
disk reads.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e830d0ad-d53e-47f0-a97d-f9bd81b27238
📒 Files selected for processing (3)
app/src/main/java/com/itsaky/androidide/fragments/onboarding/PermissionsFragment.ktapp/src/main/java/com/itsaky/androidide/localWebServer/WebServer.ktapp/src/main/java/com/itsaky/androidide/viewmodel/InstallationViewModel.kt
| if (viewModel.isSetupComplete()) { | ||
| (activity as? OnboardingActivity)?.tryNavigateToMainIfSetupIsCompleted() | ||
| return@launch | ||
| } |
There was a problem hiding this comment.
isSetupComplete() still performs disk I/O on the main thread.
viewLifecycleScope.launch defaults to Dispatchers.Main.immediate, so viewModel.isSetupComplete() here runs on the main thread. Internally it calls checkToolsIsInstalled(), which invokes Environment.ANDROID_HOME.exists() — a filesystem call that StrictMode's DiskReadViolation will catch just like the storage check this PR is fixing. Consider running this check on Dispatchers.IO (or making isSetupComplete()/checkToolsIsInstalled() suspend) so the same violation pattern doesn't reappear two lines below the one you just fixed.
♻️ Sketch of one approach
- if (viewModel.isSetupComplete()) {
+ val setupComplete = withContext(Dispatchers.IO) { viewModel.isSetupComplete() }
+ if (setupComplete) {
(activity as? OnboardingActivity)?.tryNavigateToMainIfSetupIsCompleted()
return@launch
}Note: isSetupComplete() is also called from isPolicyRespected (line 348) and onUserIllegallyRequestedNextPage (line 353) on the main thread; those call sites would also benefit from the same treatment, though they're outside this PR's scope.
📝 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.
| if (viewModel.isSetupComplete()) { | |
| (activity as? OnboardingActivity)?.tryNavigateToMainIfSetupIsCompleted() | |
| return@launch | |
| } | |
| val setupComplete = withContext(Dispatchers.IO) { viewModel.isSetupComplete() } | |
| if (setupComplete) { | |
| (activity as? OnboardingActivity)?.tryNavigateToMainIfSetupIsCompleted() | |
| return@launch | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/itsaky/androidide/fragments/onboarding/PermissionsFragment.kt`
around lines 242 - 245, The call to viewModel.isSetupComplete() is performing
disk I/O on the main thread; change the call site in PermissionsFragment (inside
the viewLifecycleScope.launch) to perform the check on IO by using
withContext(Dispatchers.IO) { viewModel.isSetupComplete() } (or make
viewModel.isSetupComplete()/checkToolsIsInstalled() suspend and call it from
IO), then act on the result on the main thread (calling (activity as?
OnboardingActivity)?.tryNavigateToMainIfSetupIsCompleted() only after the IO
call returns). Also search other call sites of isSetupComplete(),
isPolicyRespected, and onUserIllegallyRequestedNextPage and plan to convert them
similarly if they run on the main thread.
| TrafficStats.setThreadStatsTag(0xC0DE) | ||
| serverSocket = ServerSocket().apply { reuseAddress = true } | ||
| serverSocket.bind(InetSocketAddress(config.bindName, config.port)) | ||
| TrafficStats.clearThreadStatsTag() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Android TrafficStats setThreadStatsTag: are sockets returned by ServerSocket.accept() tagged with the current thread's stats tag at the time of accept, or only sockets created via new Socket() while the tag is set? Does clearing the thread tag before calling accept() leave accepted client sockets untagged and still trigger StrictMode UntaggedSocketViolation on read/write?
💡 Result:
Sockets returned by ServerSocket.accept are automatically tagged with the current thread's stats tag (set via TrafficStats.setThreadStatsTag) at the time of accept. This happens because the underlying libcore.io.BlockGuardOs.accept method explicitly calls tagSocket on the newly accepted FileDescriptor if it is an inet socket, using the thread's current tag. Sockets created via new Socket are similarly tagged automatically during their creation in BlockGuardOs.socket. Clearing the thread tag (via TrafficStats.clearThreadStatsTag) before calling accept will cause the accepted sockets to be untagged (tag == -1), which triggers StrictMode UntaggedSocketViolation on subsequent read/write operations, as the tagSocket call in accept uses the current (cleared/untagged) thread tag, and StrictMode checks for this during tagging.
Citations:
- 1: https://developer.android.com/reference/android/net/TrafficStats
- 2: https://android.googlesource.com/platform/prebuilts/fullsdk/sources/+/refs/heads/androidx-media-release/android-34/android/net/TrafficStats.java
- 3: https://android.googlesource.com/platform/libcore/+/master/luni/src/main/java/libcore/io/BlockGuardOs.java
- 4: https://stackoverflow.com/questions/47723973/strictmode-java-lang-throwable-untagged-socket-detected
- 5: Untagged socket detected on Android NanoHttpd/nanohttpd#515
Tagging window defeats the fix for client sockets, and lacks exception safety.
Two actionable issues:
-
Coverage.
TrafficStats.setThreadStatsTag()tags sockets at the moment they are bound, connected, or accepted on the thread with the tag active. Here, you clear the tag at line 141—immediately before the accept loop (lines 144–217). EverySocketreturned byserverSocket.accept()will be created with the tag already cleared, inheriting an untagged state (tag == -1). These client sockets are where the bulk of I/O happens (reads/writes on their InputStreams/OutputStreams), soUntaggedSocketViolationwill still fire on those operations. The listening socket is tagged, but that is not where the violation typically occurs.
Fix: Either (a) keep the tag set throughout the entire accept loop, (b) re-apply the tag before eachaccept(), or (c) explicitly tag each accepted socket withTrafficStats.tagSocket(clientSocket)and untag it when done. -
Exception safety. If
ServerSocket()orbind()throws, line 141 never executes and the thread retains tag0xC0DE, leaking it into any subsequent socket operations. Wrap the setup intry { ... } finally { TrafficStats.clearThreadStatsTag() }.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt` around
lines 138 - 141, The TrafficStats tag is being set only during ServerSocket
creation (TrafficStats.setThreadStatsTag(0xC0DE)) then cleared before the accept
loop, so accepted client sockets remain untagged and I/O will still trigger
UntaggedSocketViolation; also an exception during ServerSocket() or bind() can
leak the tag. Fix by ensuring the tag is active for the lifetime of accept/IO:
either keep the tag set across the accept loop, or (preferably) clear it after
setup in a finally block and explicitly tag/untag each accepted client socket
using TrafficStats.tagSocket(clientSocket) and
TrafficStats.untagSocket(clientSocket) around the per-client I/O, and wrap the
ServerSocket creation/bind in try...finally to always call
TrafficStats.clearThreadStatsTag() if you choose to set the thread tag for
setup.
| return | ||
| } | ||
| if (viewModel.isSetupCompleteAsync()) { | ||
| (activity as? OnboardingActivity)?.tryNavigateToMainIfSetupIsCompleted() |
There was a problem hiding this comment.
I'd be a bit cautious here and wrap the startActivity part in tryNavigatetoToMainIfSetupIsCompleted in a withContext(Dispatchers.Main.immediate) { ... } here.
But as CodeRabbit suggested, tryNavigatetoToMainIfSetupIsCompleted may still perform disk IO when it calls isSetupComplete.
A better alternative would be to just do this :
if (isSetupCompleteAsync()) {
(activity as? OnboardingActivity)?.apply {
startActivity(Intent(this, MainActivity::class.java))
finish()
}
}Or maybe OnboardingActivity can have a helper function to just do the navigation which can then be shared at both call sites.
2 participants
