fix: Bound live build output memory - #1642
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
📝 Walkthrough
WalkthroughThe build output pipeline now uses a bounded, session-aware buffer. Editor rendering keeps only a bounded tail, rejects stale updates, and refreshes filtered content when needed. Tests cover buffering, omissions, clearing, session generations, tail refreshes, and filtering. ChangesBuild output pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BuildOutputFragment
participant BuildOutputBuffer
participant BuildOutputViewModel
participant Editor
BuildOutputFragment->>BuildOutputBuffer: queue session-tagged build output
BuildOutputBuffer-->>BuildOutputFragment: provide bounded output batch
BuildOutputFragment->>BuildOutputViewModel: append batch if session is current
BuildOutputViewModel-->>BuildOutputFragment: provide bounded editor window
BuildOutputFragment->>Editor: apply filtered content or append visible text
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 7
🧹 Nitpick comments (5)
app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.kt (2)
79-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the threading invariant for these counters.
@Volatilegives visibility but not atomicity. Every update in this file uses+=, which is a read-modify-write. The code is correct only because all writes happen on the main thread (lines 138-139, 326-327, 345-346, 359-360, 388-389, 457, 487-488, 501-502, 513-514), while line 445 performs a read from a background dispatcher.That invariant is load-bearing and not visible at the declaration. Add a short comment. If a future change writes from a background dispatcher, switch to
AtomicInteger.As per coding guidelines: "Use short comments only for non-obvious reasons, workarounds, constraints, or subtle invariants."
📝 Proposed comment
+ // Written only on the main thread; `+=` is not atomic. Read from background dispatchers, + // hence `@Volatile`. `@Volatile` private var visibleEditorChars = 0 + `@Volatile` private var editorSourceChars = 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.kt` around lines 79 - 83, Add a short comment above visibleEditorChars and editorSourceChars documenting that all writes occur on the main thread, while background work only reads them, so their volatile read-modify-write updates remain safe. Note that any future background-thread writes must use AtomicInteger.Source: Coding guidelines
350-367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
launchplusjoin()is redundant and the comment is misleading.This block already runs on the main thread inside the
withContext(Dispatchers.Main)at line 335. It launches a child coroutine on the same dispatcher and then callsjob.join()at line 366, so the caller waits for it. The append is not deferred. The comment at line 351 states the opposite.The child uses
viewLifecycleOwner.lifecycleScope, but the parent at line 101 uses the same scope, so the cancellation behavior is identical. CallawaitLayoutinline.Note the equivalent branch in
flushToEditorat lines 507-519 launches without joining, so it is genuinely deferred. Align the two paths or document why they differ.♻️ Proposed simplification
} else { - // Timeout: defer append until layout is ready so content is not lost - val job = - viewLifecycleOwner.lifecycleScope.launch(Dispatchers.Main) { - editor.run { - awaitLayout(onForceVisible = { updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) }) - editorContentMutex.withLock { - if (isRestoreCurrent()) { - appendBatch(content) - visibleEditorChars += content.length - editorSourceChars = window.length - updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) - } - } - } - } - job.join() + // Layout timed out; keep waiting so the restored content is not lost. + editor.awaitLayout(onForceVisible = { updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) }) + editorContentMutex.withLock { + if (isRestoreCurrent()) { + editor.appendBatch(content) + visibleEditorChars += content.length + editorSourceChars = window.length + updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.kt` around lines 350 - 367, In the timeout branch containing `awaitLayout`, remove the child `viewLifecycleOwner.lifecycleScope.launch(Dispatchers.Main)` and its `job.join()`, and call `awaitLayout` plus the mutex-protected append inline within the existing main-thread context. Update the misleading timeout comment to describe the actual behavior, while preserving the `isRestoreCurrent()` guard and state updates.app/src/test/java/com/itsaky/androidide/fragments/output/BuildOutputBufferTest.kt (2)
132-179: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThese two tests re-implement the production algorithm instead of calling it.
Both tests copy the append-or-refresh decision loop from
BuildOutputFragment.flushToEditorinto the test body. They assert that the local copy behaves as expected. They do not executeBuildOutputFragment. If the fragment logic changes, these tests still pass. The only production code they cover iswouldExceedEditorWindowandfilterLines.The refresh branch in
BuildOutputFragmentis the core of this memory fix and currently has no direct test.Extract the decision into a pure function on
BuildOutputViewModel(for examplenextEditorWindow(visible, sourceChars, batch)), call it from bothflushToEditorand these tests. Then the tests bind to production behavior.Both tests also live in
BuildOutputBufferTestbut exerciseBuildOutputViewModel. Move them to aBuildOutputViewModelTestclass.As per coding guidelines: "Use unit tests for non-UI logic, cover error and edge paths, and target at least 50% line and branch coverage for new or changed non-UI code."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/itsaky/androidide/fragments/output/BuildOutputBufferTest.kt` around lines 132 - 179, Extract the append-or-refresh logic from BuildOutputFragment.flushToEditor into a pure BuildOutputViewModel function such as nextEditorWindow(visible, sourceChars, batch), preserving bounded-tail refresh behavior and source-character tracking. Update flushToEditor and both tests to call this production function directly, then move the tests from BuildOutputBufferTest into BuildOutputViewModelTest.Source: Coding guidelines
105-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen this test so it verifies the
retainedCharsreset.With
maxPendingChars = 4, the input"kept"normalizes to 5 characters.BuildOutputBuffer.offerrejects it and records an omission marker, so nothing is ever retained. The test then passes without exercising theretainedChars = 0reset inclear().Use a limit that retains
"kept". Thenclear()must reset the accounting for"new"to be retained. Also assertpendingCharsafter the clear.💚 Proposed fixture change
- val buffer = BuildOutputBuffer(maxPendingChars = 4, maxBatchChars = 64) + val buffer = BuildOutputBuffer(maxPendingChars = 8, maxBatchChars = 64) buffer.offer("kept") buffer.offer("dropped") buffer.clear() + assertEquals(0, buffer.pendingChars) buffer.offer("new") assertEquals("new\n", buffer.takeBatch().text)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/itsaky/androidide/fragments/output/BuildOutputBufferTest.kt` around lines 105 - 116, Update the `clear resets pending output and overflow accounting` test to use a `maxPendingChars` value that retains the normalized `"kept"` input, ensuring `clear()` exercises the retained character accounting reset. After `clear()` and offering `"new"`, assert `pendingChars` reflects only the new content and retain the existing batch text assertion.app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt (1)
89-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
isCurrentSessioncontract in the KDoc.The new parameter changes the contract of
append. Callers must know two things: the callback runs onDispatchers.IOwhile the file lock is held, and afalseresult discards the text silently without an error.The placement of the check inside
lock.withLockis correct. It closes the race againstclear(), which takes the same lock.As per coding guidelines: "Public classes, functions, and non-obvious logic must have KDoc or Javadoc documenting contracts, rationale, threading, nullability, side effects, or units."
📝 Proposed KDoc update
/** * Appends text to the session file. File I/O is performed on a background dispatcher; call from * any thread. Prefer calling before switching to Main so disk write does not block the UI. + * + * `@param` isCurrentSession Evaluated on [Dispatchers.IO] while the session-file lock is held. + * Return `false` to discard [text] without writing, for example after a new build cleared the + * session. The call then completes silently. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt` around lines 89 - 100, Update the KDoc for BuildOutputViewModel.append to document that isCurrentSession executes on Dispatchers.IO while lock.withLock is held, and that returning false silently discards the text without writing or reporting an error. Preserve the existing lock placement and threading guidance.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputBuffer.kt`:
- Line 140: Update BuildOutputBuffer.omissionMarker to use singular wording when
lineCount is 1 and plural wording otherwise, preserving the existing marker
format; update the corresponding BuildOutputBufferTest expectation. Move the
marker text into strings.xml as a plurals resource and retrieve the correctly
pluralized value through the existing Android resource access pattern.
In
`@app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.kt`:
- Around line 461-479: Move the `getWindowForEditor()` I/O and filtering in the
`refreshEditorWindow` branch outside `editorContentMutex`, then re-acquire the
mutex only to apply the computed `refreshedWindow` through the existing
generation check. Keep `renderFiltered` and `flushToEditor` able to proceed
while the window is read and processed, preserving the current filtering and
editor-update behavior.
- Around line 481-483: In the flushToEditor main-thread block, move
updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) to
after the editorGen != editorContentGeneration guard. Ensure stale flushes
return before changing the empty state, while current-generation flushes retain
the existing update.
- Around line 309-348: Gate live batch consumption on restoration completion so
restoreWindowFromViewModel finishes before processLogs consumes queued output.
Add or complete the restoreComplete synchronization signal, ensure
restoreWindowFromViewModel signals it in a finally block on every exit path, and
make processLogs await that signal before processing batches. Preserve
isRestoreCurrent() as the duplication guard rather than allowing a concurrent
batch to invalidate and discard the restored window.
- Around line 418-424: Update processLogs to handle failures independently for
each batch: wrap flushToEditor in per-iteration exception handling, rethrow
CancellationException, and catch only the non-cancellation exception types that
flushToEditor can raise. Log the failure through the fragment’s existing SLF4J
logger, or add one if absent, then continue processing subsequent batches.
In `@app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt`:
- Around line 114-121: Decouple the editor display window from the in-memory
append cache in BuildOutputViewModel: update getWindowForEditor/readTailFromFile
at lines 114-121 to decode only the required tail, and update the
cachedContentSnapshot append logic at lines 282-283 to use a separate, smaller
cache limit. Preserve tail ordering and the existing empty-content behavior.
In
`@app/src/test/java/com/itsaky/androidide/fragments/output/BuildOutputBufferTest.kt`:
- Around line 22-24: Update BuildOutputBufferTest to use the configured JUnit
Jupiter and Google Truth APIs: replace the JUnit 4 assertion and test imports
with com.google.common.truth.Truth.assertThat and org.junit.jupiter.api.Test,
and adjust assertions to the Truth style.
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.kt`:
- Around line 79-83: Add a short comment above visibleEditorChars and
editorSourceChars documenting that all writes occur on the main thread, while
background work only reads them, so their volatile read-modify-write updates
remain safe. Note that any future background-thread writes must use
AtomicInteger.
- Around line 350-367: In the timeout branch containing `awaitLayout`, remove
the child `viewLifecycleOwner.lifecycleScope.launch(Dispatchers.Main)` and its
`job.join()`, and call `awaitLayout` plus the mutex-protected append inline
within the existing main-thread context. Update the misleading timeout comment
to describe the actual behavior, while preserving the `isRestoreCurrent()` guard
and state updates.
In `@app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt`:
- Around line 89-100: Update the KDoc for BuildOutputViewModel.append to
document that isCurrentSession executes on Dispatchers.IO while lock.withLock is
held, and that returning false silently discards the text without writing or
reporting an error. Preserve the existing lock placement and threading guidance.
In
`@app/src/test/java/com/itsaky/androidide/fragments/output/BuildOutputBufferTest.kt`:
- Around line 132-179: Extract the append-or-refresh logic from
BuildOutputFragment.flushToEditor into a pure BuildOutputViewModel function such
as nextEditorWindow(visible, sourceChars, batch), preserving bounded-tail
refresh behavior and source-character tracking. Update flushToEditor and both
tests to call this production function directly, then move the tests from
BuildOutputBufferTest into BuildOutputViewModelTest.
- Around line 105-116: Update the `clear resets pending output and overflow
accounting` test to use a `maxPendingChars` value that retains the normalized
`"kept"` input, ensuring `clear()` exercises the retained character accounting
reset. After `clear()` and offering `"new"`, assert `pendingChars` reflects only
the new content and retain the existing batch text assertion.
🪄 Autofix
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 Plus
Run ID: 7c825b74-9ff6-49ff-8006-ed7410a8eab3
📒 Files selected for processing (4)
app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputBuffer.ktapp/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.ktapp/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.ktapp/src/test/java/com/itsaky/androidide/fragments/output/BuildOutputBufferTest.kt
| return Batch(batch.toString(), sessionGeneration) | ||
| } | ||
|
|
||
| private fun omissionMarker(lineCount: Long): String = "[$lineCount build output lines omitted]\n" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the singular form of the omission marker.
The marker always uses "lines". For lineCount == 1 the output reads "[1 build output lines omitted]". Users see this text in the build output pane. The test at BuildOutputBufferTest.kt line 100 currently asserts the incorrect form, so update that expectation too.
The string is also hardcoded here instead of a string resource. If the build output pane is localized elsewhere, move this marker to strings.xml with a plurals entry.
🔤 Proposed fix for the singular case
- private fun omissionMarker(lineCount: Long): String = "[$lineCount build output lines omitted]\n"
+ private fun omissionMarker(lineCount: Long): String {
+ val noun = if (lineCount == 1L) "line" else "lines"
+ return "[$lineCount build output $noun omitted]\n"
+ }📝 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.
| private fun omissionMarker(lineCount: Long): String = "[$lineCount build output lines omitted]\n" | |
| private fun omissionMarker(lineCount: Long): String { | |
| val noun = if (lineCount == 1L) "line" else "lines" | |
| return "[$lineCount build output $noun omitted]\n" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputBuffer.kt`
at line 140, Update BuildOutputBuffer.omissionMarker to use singular wording
when lineCount is 1 and plural wording otherwise, preserving the existing marker
format; update the corresponding BuildOutputBufferTest expectation. Move the
marker text into strings.xml as a plurals resource and retrieve the correctly
pluralized value through the existing Android resource access pattern.
| val generationAtRestore = editorContentGeneration | ||
| val visibleCharsAtRestore = visibleEditorChars | ||
| val sourceCharsAtRestore = editorSourceChars | ||
| fun isRestoreCurrent() = | ||
| editorContentGeneration == generationAtRestore && | ||
| visibleEditorChars == visibleCharsAtRestore && | ||
| editorSourceChars == sourceCharsAtRestore | ||
| val isSourceEmpty = window.isBlank() | ||
| val isFilteredEmpty = content.isBlank() | ||
|
|
||
| if (content.isEmpty()) return | ||
| withContext(Dispatchers.Main) { | ||
| val editor = this@BuildOutputFragment.editor ?: return@withContext | ||
| val layoutCompleted = | ||
| withTimeoutOrNull(LAYOUT_TIMEOUT_MS) { | ||
| editor.awaitLayout(onForceVisible = { updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) }) | ||
| withContext(Dispatchers.Main) { | ||
| updateEmptyState(isSourceEmpty = isSourceEmpty, isFilterActive = isFilterActive) | ||
| noMatchTracker.prime(isFilteredEmpty) | ||
| if (!isSourceEmpty && isFilteredEmpty) { | ||
| editorContentMutex.withLock { | ||
| if (isRestoreCurrent()) { | ||
| editor?.setText("") | ||
| visibleEditorChars = 0 | ||
| editorSourceChars = window.length | ||
| onContentReplaced() | ||
| } | ||
| } | ||
| } | ||
| if (layoutCompleted != null) { | ||
| editor.appendBatch(content) | ||
| updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) | ||
| } else { | ||
| // Timeout: defer append until layout is ready so content is not lost | ||
| val generationAtRestore = editorContentGeneration | ||
| val job = | ||
| viewLifecycleOwner.lifecycleScope.launch(Dispatchers.Main) { | ||
| editor.run { | ||
| awaitLayout(onForceVisible = { updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) }) | ||
| editorContentMutex.withLock { | ||
| if (editorContentGeneration == generationAtRestore) { | ||
| appendBatch(content) | ||
| updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) | ||
| } | ||
|
|
||
| if (content.isEmpty()) return@withContext | ||
| withContext(Dispatchers.Main) { | ||
| val editor = this@BuildOutputFragment.editor ?: return@withContext | ||
| val layoutCompleted = | ||
| withTimeoutOrNull(LAYOUT_TIMEOUT_MS) { | ||
| editor.awaitLayout(onForceVisible = { updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) }) | ||
| } | ||
| if (layoutCompleted != null) { | ||
| editorContentMutex.withLock { | ||
| if (isRestoreCurrent()) { | ||
| editor.appendBatch(content) | ||
| visibleEditorChars += content.length | ||
| editorSourceChars = window.length | ||
| updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
A live batch during restore discards the whole restored window.
isRestoreCurrent() compares editorContentGeneration, visibleEditorChars, and editorSourceChars. flushToEditor increments the two counters on every successful append. restoreWindowFromViewModel and processLogs are launched concurrently at lines 101-102.
During an active build the sequence is:
- Line 310 captures the counters.
awaitLayoutat line 338 waits, up toLAYOUT_TIMEOUT_MS(2000 ms).- A batch arrives and
flushToEditorappends it, changing the counters. isRestoreCurrent()at line 343 returns false, so line 344 never runs.
The restored history is then dropped. The editor shows only output that arrived after the view was created. The state recovers only when the user changes the filter, because renderFiltered reloads the window from disk.
The guard correctly prevents duplication, but the remedy should be ordering, not dropping. Complete the restore before processLogs starts consuming, or re-read the window and replace the editor content instead of skipping the append.
🔧 Sketch: gate batch consumption on restore completion
+ // Live batches must not interleave with the initial window restore, otherwise the
+ // restore is discarded or the content is duplicated.
+ private val restoreComplete = CompletableDeferred<Unit>()
+
viewLifecycleOwner.lifecycleScope.launch {
launch { restoreWindowFromViewModel() }
- launch(Dispatchers.Default) { processLogs() }
+ launch(Dispatchers.Default) {
+ restoreComplete.await()
+ processLogs()
+ }Complete restoreComplete in a finally block at the end of restoreWindowFromViewModel.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.kt`
around lines 309 - 348, Gate live batch consumption on restoration completion so
restoreWindowFromViewModel finishes before processLogs consumes queued output.
Add or complete the restoreComplete synchronization signal, ensure
restoreWindowFromViewModel signals it in a finally block on every exit path, and
make processLogs await that signal before processing batches. Preserve
isRestoreCurrent() as the duplication guard rather than allowing a concurrent
batch to invalidate and discard the restored window.
| private suspend fun processLogs() { | ||
| while (true) { | ||
| val batch = outputBuffer.takeBatch() | ||
| val editorGenAtDrain = editorContentGeneration | ||
| flushToEditor(batch.text, batch.sessionGeneration, editorGenAtDrain) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Protect the batch loop so one failed batch does not stop build output.
processLogs runs an unbounded loop with no error handling. flushToEditor calls filterLines, fitEditorWindow, and editor mutations. If any of these throws, the loop exits permanently. Build output then stops for the rest of the session, and the exception propagates to the global crash handler.
Catch per iteration, log the failure, and continue. Rethrow CancellationException so view destruction still cancels the loop.
As per coding guidelines: "Catch recoverable I/O, parsing, IPC, git, and plugin failures locally; convert them into explicit error states, never allow unexpected exceptions to reach the global GlitchTip crash handler" and "In coroutine code, catch exceptions inside the launching coroutine, rethrow CancellationException, and avoid broad catches where narrower types suffice."
🛡️ Proposed fix
private suspend fun processLogs() {
while (true) {
val batch = outputBuffer.takeBatch()
val editorGenAtDrain = editorContentGeneration
- flushToEditor(batch.text, batch.sessionGeneration, editorGenAtDrain)
+ try {
+ flushToEditor(batch.text, batch.sessionGeneration, editorGenAtDrain)
+ } catch (e: CancellationException) {
+ throw e
+ } catch (e: IOException) {
+ log.error("Failed to flush a build output batch to the editor", e)
+ }
}
}Widen the caught type only to the exceptions flushToEditor can actually raise. Add an SLF4J log field if the fragment does not already have one.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.kt`
around lines 418 - 424, Update processLogs to handle failures independently for
each batch: wrap flushToEditor in per-iteration exception handling, rethrow
CancellationException, and catch only the non-cancellation exception types that
flushToEditor can raise. Log the failure through the fragment’s existing SLF4J
logger, or add one if absent, then continue processing subsequent batches.
Source: Coding guidelines
| val refreshedWindow = | ||
| if (refreshEditorWindow) { | ||
| val window = withContext(Dispatchers.IO) { buildOutputViewModel.getWindowForEditor() } | ||
| withContext(Dispatchers.Default) { | ||
| Pair( | ||
| BuildOutputViewModel.fitEditorWindow( | ||
| BuildOutputViewModel.filterLines( | ||
| window, | ||
| buildOutputViewModel.filterText.value, | ||
| buildOutputViewModel.showTimestamps.value, | ||
| buildOutputViewModel.showDeltas.value, | ||
| ), | ||
| ), | ||
| window.length, | ||
| ) | ||
| } | ||
| } else { | ||
| null | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
The window refresh holds editorContentMutex across file I/O and filtering.
editorContentMutex is acquired at line 438 and held here across getWindowForEditor() on Dispatchers.IO (line 463) and a filter pass over up to 512 Ki characters on Dispatchers.Default (lines 464-476). withContext does not release the mutex.
While the lock is held, renderFiltered (line 127) and the next flushToEditor both block. Build output keeps arriving and fills outputBuffer. Once the 256 KiB pending budget is exhausted, lines are dropped into omission markers.
This refresh runs every time the source window fills, which is repeatedly during a large assembleDebug. The result is dropped output during exactly the scenario in issue #1367.
Compute the refreshed window outside the lock, then re-acquire it to apply the result under the existing generation check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.kt`
around lines 461 - 479, Move the `getWindowForEditor()` I/O and filtering in the
`refreshEditorWindow` branch outside `editorContentMutex`, then re-acquire the
mutex only to apply the computed `refreshedWindow` through the existing
generation check. Keep `renderFiltered` and `flushToEditor` able to proceed
while the window is read and processed, preserving the current filtering and
editor-update behavior.
| withContext(Dispatchers.Main) { | ||
| updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) | ||
| if (visibleText.isEmpty()) { | ||
| if (editorGen != editorContentGeneration) return@withContext |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Move updateEmptyState after the generation check.
Line 482 sets isSourceEmpty = false before line 483 tests editorGen != editorContentGeneration. A stale flush therefore still changes the empty state.
The sequence is:
flushToEditorsuspends at thewithContext(Dispatchers.Main)on line 481.clearOutputruns on the main thread, bumpseditorContentGenerationat line 387, and sets the empty state toisSourceEmpty = trueat line 395.- This block resumes, line 482 sets
isSourceEmpty = false, and line 483 then returns.
The empty state now reports content for a cleared session. The editor is blank but the empty view is hidden.
🐛 Proposed fix
withContext(Dispatchers.Main) {
- updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive)
if (editorGen != editorContentGeneration) return@withContext
+ updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive)
if (refreshedWindow != null) {📝 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.
| withContext(Dispatchers.Main) { | |
| updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) | |
| if (visibleText.isEmpty()) { | |
| if (editorGen != editorContentGeneration) return@withContext | |
| withContext(Dispatchers.Main) { | |
| if (editorGen != editorContentGeneration) return@withContext | |
| updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.kt`
around lines 481 - 483, In the flushToEditor main-thread block, move
updateEmptyState(isSourceEmpty = false, isFilterActive = isFilterActive) to
after the editorGen != editorContentGeneration guard. Ensure stale flushes
return before changing the empty state, while current-generation flushes retain
the existing update.
| /** | ||
| * Returns the last [WINDOW_SIZE_CHARS] characters from the session file for the editor to | ||
| * Returns the last [EDITOR_WINDOW_MAX_CHARS] characters from the session file for the editor to | ||
| * display (e.g. initial view or after rotation). Returns empty string if no content. | ||
| */ | ||
| fun getWindowForEditor(): String = | ||
| lock.withLock { | ||
| readTailFromFile(sessionFile, WINDOW_SIZE_CHARS) | ||
| readTailFromFile(sessionFile, EDITOR_WINDOW_MAX_CHARS) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Recover the previous window constant and list every consumer of the new one.
git show HEAD~1:app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt 2>/dev/null \
| rg -n 'WINDOW_SIZE_CHARS|CACHE_SNAPSHOT_MAX_CHARS' || echo "previous revision unavailable"
rg -n 'EDITOR_WINDOW_MAX_CHARS|CACHE_SNAPSHOT_MAX_CHARS|readTailFromFile|setCachedSnapshot' --type=ktRepository: appdevforall/CodeOnTheGo
Length of output: 835
🏁 Script executed:
#!/bin/bash
set -e
echo "== tracked Kotlin mentions of constants/functions =="
git ls-files '*.kt' | \
xargs -r rg -n 'EDITOR_WINDOW_MAX_CHARS|CACHE_SNAPSHOT_MAX_CHARS|WINDOW_SIZE_CHARS|readTailFromFile|cachedContentSnapshot|setCachedSnapshot|flushToEditor'
echo
echo "== relevant ViewModel sections =="
sed -n '1,150p' app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt
sed -n '250,315p' app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt
echo
echo "== relevant prior revision sections =="
sed -n '1,140p' HEAD~1:app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt || true
sed -n '250,305p' HEAD~1:app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt || true
echo
echo "== programmatic size-shape check from source text =="
python3 - <<'PY'
from pathlib import Path
import re
p=Path('app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt')
s=p.read_text()
for token in ['WINDOW_SIZE_CHARS','EDITOR_WINDOW_MAX_CHARS','CACHE_SNAPSHOT_MAX_CHARS']:
for m in re.finditer(fr'\b{token}\b|\bWINDOW_SIZE_CHARS\b|\sconst val\s+WINDOW_SIZE_CHARS', s):
idx=m.start()
line=s[:idx].count('\n')+1
snippet='\n'.join(s[:idx].count('\n')+1 for _ in range(1))
print(f"{p}:{line}:{s.splitlines()[line-1]}")
for name in ['readTailFromFile','CACHE_SNAPSHOT_MAX_CHARS','cachedContentSnapshot']:
print(f"\n--- {name} ---")
for i,line in enumerate(s.splitlines(),1):
if name in line or 'readTailFromFile' in line:
print(f"{i}: {line}")
PYRepository: appdevforall/CodeOnTheGo
Length of output: 4277
🏁 Script executed:
#!/bin/bash
set -e
echo "== tracked Kotlin mentions of constants/functions =="
git ls-files '*.kt' | \
xargs -r rg -n 'EDITOR_WINDOW_MAX_CHARS|CACHE_SNAPSHOT_MAX_CHARS|WINDOW_SIZE_CHARS|readTailFromFile|cachedContentSnapshot|setCachedSnapshot|flushToEditor'
echo
echo "== relevant ViewModel sections =="
sed -n '1,150p' app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt
sed -n '250,315p' app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt
echo
echo "== relevant prior revision sections =="
sed -n '1,140p' HEAD~1:app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt || true
sed -n '250,305p' HEAD~1:app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt || true
echo
echo "== programmatic size-shape check from source text =="
python3 - <<'PY'
from pathlib import Path
import re
p=Path('app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt')
s=p.read_text()
for token in ['WINDOW_SIZE_CHARS','EDITOR_WINDOW_MAX_CHARS','CACHE_SNAPSHOT_MAX_CHARS']:
for m in re.finditer(fr'\b{token}\b|\bWINDOW_SIZE_CHARS\b|\sconst val\s+WINDOW_SIZE_CHARS', s):
idx=m.start()
line=s[:idx].count('\n')+1
print(f"FOUND:{p}:{line}:{token}:{s.splitlines()[line-1]}")
PYRepository: appdevforall/CodeOnTheGo
Length of output: 4277
Size the editor window and snapshot cache independently. EDITOR_WINDOW_MAX_CHARS is 512 Ki characters now, and readTailFromFile() decodes a full 4x byte range before taking the tail. The cache limit is tied to that same value, so each large append rebuilds a 512 Ki-character snapshot with (cachedContentSnapshot + text).takeLast(...). Decode only the needed tail or cap the in-memory append cache separately.
📍 Affects 1 file
app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt#L114-L121(this comment)app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt#L282-L283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt`
around lines 114 - 121, Decouple the editor display window from the in-memory
append cache in BuildOutputViewModel: update getWindowForEditor/readTailFromFile
at lines 114-121 to decode only the required tail, and update the
cachedContentSnapshot append logic at lines 282-283 to use a separate, smaller
cache limit. Preserve tail ordering and the existing empty-content behavior.
| import org.junit.Assert.assertEquals | ||
| import org.junit.Assert.assertTrue | ||
| import org.junit.Test |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify JUnit Jupiter and Truth availability for the app module tests.
fd -t f 'libs.versions.toml' gradle
rg -n 'junit|jupiter|truth|mockk' gradle/libs.versions.toml
fd -t f 'build.gradle.kts' app --max-depth 1 --exec rg -n 'testImplementation|testRuntimeOnly|useJUnitPlatform'Repository: appdevforall/CodeOnTheGo
Length of output: 1063
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== app/build.gradle.kts relevant config =="
fd -t f 'build.gradle.kts' app --max-depth 1 --exec sh -c 'echo "--- $1"; sed -n "1,220p" "$1" | grep -nE "android|useJUnitPlatform|testImplementation|testRuntimeOnly|libs\.tests|libs\.tests-junit|libs\.google-truth|libs\.truth|jupiter|gradle-kotlin-dsl|repositories|Maven|Gradle" || true' sh {}
echo
echo "== BuildOutputBufferTest.kt =="
fd -t f 'BuildOutputBufferTest.kt' . --exec sh -c 'echo "--- $1"; wc -l "$1"; sed -n "1,160p" "$1"' sh {}
echo
echo "== Gradle config search for test engine =="
rg -n "useJUnitPlatform|junit-jupiter|tests-junit-jupiter|jupiter|gradle-kotlin-dsl|repository" -S --glob '*.kts' --glob '*.gradle' --glob 'libs.versions.toml' .Repository: appdevforall/CodeOnTheGo
Length of output: 6883
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== root build.gradle.kts test-related declarations =="
if [ -f build.gradle.kts ]; then
sed -n '1,80p' build.gradle.kts
rg -n "useJUnitPlatform|testImplementation|testRuntimeOnly|libs\.tests|tests-junit-jupiter|google-truth|androidx-test|ext\.jvmTest|junit" -S build.gradle.kts
fi
echo
echo "== app/build.gradle.kts test-related declarations =="
sed -n '70,140p' app/build.gradle.kts
sed -n '140,235p' app/build.gradle.kts
rg -n "testImplementation|testRuntimeOnly|androidTestImplementation|useJUnitPlatform|libs\.tests|junit-jupiter|google-truth|androidx-test" -S app/build.gradle.ktsRepository: appdevforall/CodeOnTheGo
Length of output: 3310
Use JUnit Jupiter and Truth for this new test class.
gradle/libs.versions.toml defines tests-junit-jupiter and tests-google-truth, so this new test should use import org.junit.jupiter.api.Test and com.google.common.truth.Truth.assertThat instead of the JUnit 4 imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/test/java/com/itsaky/androidide/fragments/output/BuildOutputBufferTest.kt`
around lines 22 - 24, Update BuildOutputBufferTest to use the configured JUnit
Jupiter and Google Truth APIs: replace the JUnit 4 assertion and test imports
with com.google.common.truth.Truth.assertThat and org.junit.jupiter.api.Test,
and adjust assertions to the Truth style.
Source: Coding guidelines
1 participant
Large-project builds can make the editor UI stutter, freeze, and eventually restart with an
OutOfMemoryError; the reporter reproduced it with Xed-Editor while the default Compose template remained stable. The latest stack trace anchors the allocation failure inBuildOutputFragment.processLogs, where an unlimited channel is drained into an unboundedStringBuilder. AlthoughBuildOutputViewModelnow stores output in a file and limits restored/cached content to a 512 KiB tail, the live editor still appends every processed line for the duration of a build. The fix must bound both pending batches and the live editor document without changing the build-service-to-fragment call path.Summary
Introduce a small production-consumed
BuildOutputBufferthat accepts the fragment's incoming strings, emits size-limited batches in order, and enforces a fixed pending-output budget; when a producer burst exceeds that budget, coalesce the dropped count into one explicit omission marker rather than retaining an unlimited backlog. UpdateBuildOutputFragmentto consume those bounded batches, keep the existing session-generation checks, and replace the live editor content with the filtered tail fromBuildOutputViewModelwhenever appending would exceed the editor window instead of allowing the Sora document to grow for the whole build. Move the editor-window limit into an internalBuildOutputViewModelcontract used by both file-tail reads and the fragment so restore and live-stream behavior cannot diverge.Test plan
Fixes #1367