Uh oh!
There was an error while loading. Please reload this page.
ADFA-934: Build output timestamp - #1586
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough
WalkthroughBuild output now supports persisted line-number, timestamp, and delta visibility controls. Streamed lines receive timing prefixes. Filtering and editor updates use the active display settings and generation guards. The editor bottom sheet exposes the new view-options popup. ChangesBuild output display contract
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt (1)
183-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit test coverage for the new timing/formatting logic.
formatOutputcontains non-trivial timing math (total/step deltas, blank-line/trailing-newline preservation) but no accompanying test file was included in this PR. As per coding guidelines, new/changed non-UI logic should target at least 50% line and branch coverage.Based on learnings and 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." Want me to draft a JUnit test class for
formatOutput/resetBuildTimers?🤖 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/handlers/EditorBuildEventListener.kt` around lines 183 - 197, Add unit tests for the non-UI formatting logic in formatOutput and timer behavior in resetBuildTimers. Cover total and step timing prefixes, multi-line output, blank lines, inputs with and without trailing newlines, and timer reset behavior, targeting at least 50% line and branch coverage.Source: Coding guidelines
app/src/main/java/com/itsaky/androidide/fragments/output/LogFilterBarController.kt (1)
69-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider wiring long-press tooltip help for the new toggle chips.
The three new chips (line numbers, timestamps, deltas) are new interactive elements with no evident long-press help wiring (e.g. via the
TooltipTag-based mechanism used elsewhere, such aseditor?.tag = TooltipTag.PROJECT_BUILD_OUTPUTinBuildOutputFragment.kt).Based on coding guidelines: "New interactive elements and screens must provide long-press contextual help through idetooltips and the three-tier tooltip system." Please confirm whether an existing mechanism (not shown in this diff) already covers these chips, or whether tooltip tagging should be added here.
🤖 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/LogFilterBarController.kt` around lines 69 - 92, Verify whether chipLineNumbers, chipTimestamps, and chipDeltas already receive long-press help through an existing tooltip mechanism; if not, add the appropriate TooltipTag-based three-tier tooltip wiring for each chip in the controller’s setup flow, using distinct contextual tags and preserving the existing visibility, checked-state, and listener behavior.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/BuildOutputFragment.kt`:
- Around line 88-99: Separate the generation used for session-file persistence
from editorContentGeneration. Add a persistence generation that is incremented
only by clearOutput(), capture and propagate both generations through
processLogs and flushToEditor, and use the persistence generation to gate
buildOutputViewModel.append(text) while retaining editorContentGeneration for
visible appendBatch calls. Ensure filter and display-toggle re-renders cannot
discard drained output.
In
`@app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt`:
- Around line 79-104: Update prepareBuild and onProgressEvent to capture the
result of checkActivity(...) in a local activity instance and return when it is
null, then use that captured instance throughout each method instead of
re-reading the throwing activity accessor. Preserve the existing build-status,
timer, output, and progress behavior while eliminating the
time-of-check/time-of-use race.
- Around line 45-46: Update build elapsed-time tracking in
EditorBuildEventListener to use SystemClock.elapsedRealtime() for
buildStartTimeMs, lastOutputTimeMs, and all totalDeltaMs/stepDeltaMs
calculations, including the referenced later sections. Preserve
System.currentTimeMillis()/Instant only for displayed wall-clock timestamps,
ensuring duration values remain monotonic and non-negative.
In `@app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt`:
- Around line 202-227: Update PREFIX_TIME_FORMAT used by formatLinePrefix to
apply an explicit ASCII DecimalStyle, ensuring the [HH:mm:ss.SSS] timestamp
always emits ASCII digits regardless of the device locale. Do not rely solely on
Locale.US; preserve the existing timestamp pattern and formatting behavior.
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/fragments/output/LogFilterBarController.kt`:
- Around line 69-92: Verify whether chipLineNumbers, chipTimestamps, and
chipDeltas already receive long-press help through an existing tooltip
mechanism; if not, add the appropriate TooltipTag-based three-tier tooltip
wiring for each chip in the controller’s setup flow, using distinct contextual
tags and preserving the existing visibility, checked-state, and listener
behavior.
In
`@app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt`:
- Around line 183-197: Add unit tests for the non-UI formatting logic in
formatOutput and timer behavior in resetBuildTimers. Cover total and step timing
prefixes, multi-line output, blank lines, inputs with and without trailing
newlines, and timer reset behavior, targeting at least 50% line and branch
coverage.
🪄 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 Plus
Run ID: d78f0844-2eb6-4697-a35e-eed2f14c5974
📒 Files selected for processing (7)
app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.ktapp/src/main/java/com/itsaky/androidide/fragments/output/LogFilterBarController.ktapp/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.ktapp/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.ktapp/src/main/res/layout/layout_log_filter_bar.xmlapp/src/test/java/com/itsaky/androidide/viewmodel/BuildOutputFilterTest.ktresources/src/main/res/values/strings.xml
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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 (1)
app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt (1)
249-251: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the complete trailing line separator before splitting.
lineSequence()treats CRLF as one separator, butfilterLines()only strips a final\n. When content ends with\r\n, no separator remains to trigger the existing blank-line drop, sofilterLines()can append an extra blank line for that line ending. Strip\r\nbefore stripping isolated\nor\r, and add a CRLF regression test.🤖 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 249 - 251, Update the body preparation near `filterLines()` to remove one complete trailing CRLF separator before handling isolated trailing `\n` or `\r`, preserving the existing blank-line behavior for newline-terminated content. Add a regression test covering CRLF-terminated input and verifying that `filterLines()` does not append an extra blank line.
🤖 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.
Outside diff comments:
In `@app/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.kt`:
- Around line 249-251: Update the body preparation near `filterLines()` to
remove one complete trailing CRLF separator before handling isolated trailing
`\n` or `\r`, preserving the existing blank-line behavior for newline-terminated
content. Add a regression test covering CRLF-terminated input and verifying that
`filterLines()` does not append an extra blank line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6bbaad46-d8b5-4303-a44e-e821f4a61c9d
📒 Files selected for processing (6)
app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.ktapp/src/main/java/com/itsaky/androidide/fragments/output/LogFilterBarController.ktapp/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.ktapp/src/main/java/com/itsaky/androidide/viewmodel/BuildOutputViewModel.ktapp/src/test/java/com/itsaky/androidide/viewmodel/BuildOutputFilterTest.ktresources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (5)
- resources/src/main/res/values/strings.xml
- app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt
- app/src/main/java/com/itsaky/androidide/fragments/output/LogFilterBarController.kt
- app/src/test/java/com/itsaky/androidide/viewmodel/BuildOutputFilterTest.kt
- app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/res/drawable/ic_tune.xml`:
- Around line 1-11: Move the drawable resource represented by ic_tune.xml from
the app module into the resources module’s drawable directory, then remove the
original app-module copy. Preserve the vector contents and resource name so
existing consumers continue resolving it through the resources module.
🪄 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: 579ef83a-41df-4837-860e-dfbd8739fcfe
📒 Files selected for processing (8)
app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.ktapp/src/main/java/com/itsaky/androidide/fragments/output/LogFilterBarController.ktapp/src/main/java/com/itsaky/androidide/fragments/output/ViewOptionsOutputFragment.ktapp/src/main/java/com/itsaky/androidide/ui/EditorBottomSheet.ktapp/src/main/res/drawable/ic_tune.xmlapp/src/main/res/layout/layout_editor_bottom_sheet.xmlidetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.ktresources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/src/main/res/values/strings.xml
- app/src/main/java/com/itsaky/androidide/fragments/output/BuildOutputFragment.kt
Uh oh!
There was an error while loading. Please reload this page.
d79d066 to
a24eb9bCompare
Add time stamps, time deltas, and line numbers to the Build Output