Conversation
📝 Walkthrough
Risks & best-practice notes:
WalkthroughAdded a lifecycle-aware accessor Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt (1)
965-992:⚠️ Potential issue | 🟠 Major
content.tabsaccessed from background thread inupdateTabs— same crash risk.Lines 978–989 run inside
editorActivityScope.launch { ... }, which usesDispatchers.Default. Accessingcontent.tabs.tabCount(line 978) and iterating tab children from a background thread has two problems:
content(the throwing property) will throwIllegalStateExceptionif the activity is destroyed — the same crash this PR fixes.- Accessing Android views from a non-main thread is not safe.
The
contentOrNullguard at line 992 only protects thewithContext(Dispatchers.Main)block but not the preceding background work that also touchescontent.Consider collecting the tab count on the main thread first, or restructuring so all view access is within the
withContext(Dispatchers.Main)block.
🤖 Fix all issues with AI agents
In
`@app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt`:
- Around line 703-712: The code is using the file index variable "index" as a
tab position when calling content.tabs.getTabAt(index); replace that lookup with
the correct tab position by calling getTabPositionForFileIndex(index) and using
its result to fetch the tab, while keeping the rest of the logic (updating
editorViewModel.areFilesModified and trimming the '*' prefix) intact; ensure you
handle a null/invalid position the same way the current code handles a missing
tab (return@withContext) and reference content.tabs.getTabAt(...) and
getTabPositionForFileIndex(index) when making the change.
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt (1)
949-963: Nit: inconsistent guard style — consider local capture for uniformity.Other methods in this PR (e.g.,
getEditorForFile,saveResultInternal,confirmProjectClose) capturecontentOrNullinto a localval content, while here you check-and-discard then use the throwingcontentproperty on line 959. Both are safe on the main thread, but using the local capture pattern uniformly would be more consistent and slightly more robust.Optional consistency improvement
fun onDocumentChange(event: DocumentChangeEvent) { - if (contentOrNull == null) return + val content = contentOrNull ?: return editorViewModel.areFilesModified = true val fileIndex = findIndexOfEditorByFile(event.file.toFile())
app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt
Show resolved
Hide resolved
jatezzz
force-pushed
the
fix/ADFA-2845-activity-destroyed-crash
branch
from
ca5dc9d to
edf9801
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt (2)
671-718:⚠️ Potential issue | 🔴 CriticalRemaining crash path:
getEditorAtIndexcalled from IO thread before thecontentOrNullguard.The
contentOrNullguard on line 704 only protects thewithContext(Dispatchers.Main)block. However, line 679 callsgetEditorAtIndex(index)while still on the IO thread (viasaveAllAsync→saveAllResult→saveResultInternal).
getEditorAtIndex(line 447) callsgetTabPositionForFileIndex(index)which accesses the throwingcontent.tabs.tabCountproperty. If the activity is destroyed during an IO-thread save, this will throw the sameIllegalStateExceptionthe PR intends to fix.Suggested fix
override fun getEditorAtIndex(index: Int): CodeEditorView? { + val content = contentOrNull ?: return null val tabPosition = getTabPositionForFileIndex(index) if (tabPosition < 0) return null - val child = _binding?.content?.editorContainer?.getChildAt(tabPosition) ?: return null + val child = content.editorContainer.getChildAt(tabPosition) ?: return null return if (child is CodeEditorView) child else null }And
getTabPositionForFileIndexalso needs guarding:private fun getTabPositionForFileIndex(fileIndex: Int): Int { if (fileIndex < 0) return -1 + val content = contentOrNull ?: return -1 var tabPos = 0 var fileCount = 0 while (tabPos < content.tabs.tabCount) {
967-1009:⚠️ Potential issue | 🔴 CriticalSame background-thread access issue in
updateTabs.Lines 980 and 984 access
content.tabs.tabCountandgetEditorAtIndex(index)from the backgroundeditorActivityScope(Dispatchers.Default) thread. If the activity is destroyed while this coroutine is running,contentwill throwIllegalStateException.The guard at line 994 only protects the
withContext(Dispatchers.Main)portion. The background portion needs its own guard:Suggested fix
private fun updateTabs() { editorActivityScope.launch { + val content = contentOrNull ?: return@launch val files = editorViewModel.getOpenedFiles() val dupliCount = mutableMapOf<String, Int>()
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt (1)
950-965: Consider using local capture for consistency.Line 952 checks
contentOrNull == nullbut subsequent lines (961, 962, 964) access the throwingcontentproperty directly. While this is safe on the main thread (no interleaving), it's inconsistent with the local-capture pattern used elsewhere (e.g., lines 704, 937, 994). Usingval content = contentOrNull ?: returnwould be more uniform and resilient to future refactoring.Suggested change
`@Subscribe`(threadMode = ThreadMode.MAIN) fun onDocumentChange(event: DocumentChangeEvent) { - if (contentOrNull == null) return + val content = contentOrNull ?: return editorViewModel.areFilesModified = true val fileIndex = findIndexOfEditorByFile(event.file.toFile())
Add contentOrNull check in async callbacks to handle activity destruction.
jatezzz
force-pushed
the
fix/ADFA-2845-activity-destroyed-crash
branch
from
edf9801 to
fc2c92f
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt (3)
349-358: 🛠️ Refactor suggestion | 🟠 MajorInconsistent
contentOrNullusage — prefer capturing into a local variable.Line 351 checks
contentOrNull == nullbut lines 352–358 accesscontent(the non-null property) directly. If the activity is destroyed between the check and the access, the crash can still occur. The same TOCTOU pattern appears at lines 632, 952, and 1318.Use the local-capture pattern consistently (as you already do at lines 704, 937, 994, 1074, 1293):
Proposed fix (line 351)
withContext(Dispatchers.Main) { - if (contentOrNull == null) return@withContext + val content = contentOrNull ?: return@withContext existingFiles.forEach { file -> openFile(File(file.filePath), file.selection) }Apply the same change at lines 632, 952, and 1318 for consistency and safety.
936-941:⚠️ Potential issue | 🟡 MinorFile index compared against tab count (includes plugin tabs).
Line 939 checks
index >= content.tabs.tabCount, butindexis a file index returned byfindIndexOfEditorByFile. With plugin tabs,tabCountcan exceed the file count, so this guard is too loose. It should compare againsteditorViewModel.getOpenedFileCount().Proposed fix
fun onFileRenamed(event: FileRenameEvent) { val content = contentOrNull ?: return val index = findIndexOfEditorByFile(event.file) - if (index < 0 || index >= content.tabs.tabCount) { + if (index < 0 || index >= editorViewModel.getOpenedFileCount()) { return }
1120-1124:⚠️ Potential issue | 🟡 Minor
createPluginTabreturnstruebeforerunOnUiThreadexecutes.The method returns
trueat line 1120 immediately, but the actual tab creation happens asynchronously insiderunOnUiThread(line 1073). If the tab creation fails (e.g.,contentOrNullis null), the caller still sees success. This is pre-existing, but worth noting since you're already modifying this method.
2 participants
Description
Introduced a
contentOrNullsafety property inBaseEditorActivityto prevent crashes when accessing the view binding after the Activity has been destroyed.Refactored
EditorHandlerActivityto use this property in various asynchronous callbacks (coroutines, EventBus subscribers, and UI thread runnables) to ensure the Activity state is valid before attempting to update the UI or access tabs.Details
Logic-related fix. Prevents the application from crashing with
java.lang.IllegalStateException: Activity destroyed; binding not accessiblewhen background tasks (like saving files or plugin loading) finish after the user has exited the editor.Before changes
Screen.Recording.2026-02-09.at.10.50.12.AM.mov
After changes
Screen.Recording.2026-02-09.at.11.45.15.AM.mov
Ticket
ADFA-2845
Observation
This fix addresses race conditions in
saveAll,onReadOpenedFilesCache,createPluginTab, and EventBus events likeonFileRenamedandonDocumentChange.