[feat] add maxLogcatEntries new config param#247
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (1)
📝 WalkthroughWalkthroughThis PR adds configurable logcat buffer retention through a new ChangesConfigurable logcat buffer capacity
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Claude finished @Manabu-GT's task in 2m 12s —— View job Claude PR Review
SummaryNice feature — exposing That said, there is one regression worth fixing before merge, plus a few smaller suggestions. Detailed comments inline. Findings🔴 Blocker — capacity validation lost (inline) 🟡 Minor — 🟡 Minor — LogcatDataSource test value (inline) 🟢 Nit — annotation consistency (inline) What's working well
Security & performance
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a39f1bc8e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /** | ||
| * Maximum number of entries retained in the in-memory buffer and the count | ||
| * requested from logcat on next subscription. The currently-running subprocess | ||
| * keeps its original `-T N` arg until [WhileSubscribed][SharingStarted.WhileSubscribed] | ||
| * restarts the producer (panel reopen). | ||
| */ | ||
| var maxEntries: Int | ||
| get() = entries.capacity | ||
| set(value) { | ||
| entries.capacity = value | ||
| } |
There was a problem hiding this comment.
Thread-safety of maxEntries property
Reads/writes here delegate to entries.capacity, which is @Synchronized — so each individual get/set is atomic. Good.
However, the producerSignal flow at line 97 and captureLogcatOnce() at line 172 read entries.capacity without locking around the surrounding operation. That means:
setLogcatMaxEntries(N)called concurrently fromDebugOverlay.config = ...(any thread, sinceconfigsetter has no thread restriction) can change the value between the read and theexec()call. Not actually a problem here — whatever value is read is valid.
The KDoc accurately documents the "subprocess keeps its original -T N until panel reopen" behavior, which is the right call (interrupting an active stream just to apply a new cap would be disruptive). 👍
Minor nit: Consider adding @IntRange(from = 1) on the public maxEntries setter as well, to mirror the constraint advertised on Config.maxLogcatEntries. This is a lint hint only, but it's consistent.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/EvictingQueue.kt`:
- Around line 19-37: The capacity invariant must be enforced at runtime: in the
EvictingQueue<T> constructor validate initialCapacity >= 1 and throw an
IllegalArgumentException if not, and in the public var capacity setter validate
the new value >= 1 before assigning/evicting (throw IllegalArgumentException for
invalid values); keep the `@Synchronized` semantics and the existing eviction loop
(queue.removeFirst()) but only run it after the validated assignment in the
capacity setter so add()/removeFirst() cannot be driven by a zero/negative
capacity.
In
`@debugoverlay-core/src/test/kotlin/com/ms/square/debugoverlay/internal/data/EvictingQueueTest.kt`:
- Around line 129-176: Add tests asserting that EvictingQueue's constructor and
the capacity setter reject non-positive values: call the constructor
EvictingQueue<T>(0) and EvictingQueue<T>(-1) expecting IllegalArgumentException
(or the project's preferred exception), and set queue.capacity = 0 and
queue.capacity = -1 expecting the same; reference the EvictingQueue class, its
constructor, and the capacity property/ setter in your tests to ensure both
initialization and runtime changes validate capacity > 0.
🪄 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: bf68df9e-6b77-4559-a984-c195ca738321
📒 Files selected for processing (7)
debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/DebugOverlay.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/DebugOverlayDataRepository.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/EvictingQueue.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/source/LogcatDataSource.ktdebugoverlay-core/src/test/kotlin/com/ms/square/debugoverlay/DebugOverlayTest.ktdebugoverlay-core/src/test/kotlin/com/ms/square/debugoverlay/internal/data/EvictingQueueTest.ktdebugoverlay-core/src/test/kotlin/com/ms/square/debugoverlay/internal/data/source/LogcatDataSourceTest.kt
Qodana for JVMIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at qodana-support@jetbrains.com
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/source/LogcatDataSource.kt (2)
99-99: ⚖️ Poor tradeoffStatic analysis: Runtime.exec() command construction.
The static analysis tool flags these
Runtime.exec()calls because the command is built via string interpolation, which could be vulnerable to injection. However, in this caseentries.capacityis internally controlled and validated by@IntRange(from = 1), making actual injection impossible.Best practice would be to use
ProcessBuilderwith an explicit argument list to avoid shell interpretation entirely:ProcessBuilder("logcat", "-v", "threadtime,printable,epoch", "-T", entries.capacity.toString()) .start()This would make the intent clearer and eliminate the static analysis warning, but given the current code is functionally safe, this can be deferred.
Also applies to: 174-174
🤖 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 `@debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/source/LogcatDataSource.kt` at line 99, Replace the Runtime.getRuntime().exec(...) string-interpolated calls in LogcatDataSource (the assignment that creates "process") with a ProcessBuilder that passes each logcat argument as a separate element (e.g. "logcat", "-v", "threadtime,printable,epoch", "-T", entries.capacity.toString()) and then call start(); update both occurrences (the one assigning val process at the earlier creation and the second occurrence around line 174) so the command is constructed without shell interpolation to satisfy static analysis and avoid injection risks.
42-42: ⚡ Quick winAdd
@IntRange(from = 1)annotation to the constructor parameter for consistency.The
maxEntriesproperty already uses@IntRange(from = 1)on both getter and setter (lines 65, 67). Apply the same annotation to theinitialMaxEntriesconstructor parameter to provide consistent IDE validation and documentation hints to callers.Suggested fix
- initialMaxEntries: Int, + `@IntRange`(from = 1) initialMaxEntries: Int,🤖 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 `@debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/source/LogcatDataSource.kt` at line 42, Add the annotation `@IntRange`(from = 1) to the LogcatDataSource constructor parameter initialMaxEntries to match the existing annotations on the maxEntries getter/setter; ensure the import for IntRange (androidx.annotation.IntRange) is present so the IDE and lint pick up the validation.
🤖 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.
Nitpick comments:
In
`@debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/source/LogcatDataSource.kt`:
- Line 99: Replace the Runtime.getRuntime().exec(...) string-interpolated calls
in LogcatDataSource (the assignment that creates "process") with a
ProcessBuilder that passes each logcat argument as a separate element (e.g.
"logcat", "-v", "threadtime,printable,epoch", "-T", entries.capacity.toString())
and then call start(); update both occurrences (the one assigning val process at
the earlier creation and the second occurrence around line 174) so the command
is constructed without shell interpolation to satisfy static analysis and avoid
injection risks.
- Line 42: Add the annotation `@IntRange`(from = 1) to the LogcatDataSource
constructor parameter initialMaxEntries to match the existing annotations on the
maxEntries getter/setter; ensure the import for IntRange
(androidx.annotation.IntRange) is present so the IDE and lint pick up the
validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b0a6178-bc6e-47ab-9f2f-87a697847a12
📒 Files selected for processing (3)
debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/DebugOverlay.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/source/LogcatDataSource.ktdebugoverlay-core/src/test/kotlin/com/ms/square/debugoverlay/DebugOverlayTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/DebugOverlay.kt
1 participant
Summary by CodeRabbit
New Features
maxLogcatEntriesconfiguration setting to control the Logcat buffer size; defaults to 300 entries.Tests