fix: enhance logging with comprehensive levels and revision tracking#178
Merged
conventoangelo merged 6 commits intomainfrom Feb 28, 2026
Merged
fix: enhance logging with comprehensive levels and revision tracking#178conventoangelo merged 6 commits intomainfrom
conventoangelo merged 6 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdded a revision counter and revision Stream to LogCapture, switched internal log buffers to ListQueue with trimmed duplicate checks, and replaced timer-based polling in DebugViewer with revision-driven cache updates and a dedicated log list widget. Changes
Sequence Diagram(s)sequenceDiagram
participant LogCapture
participant DebugViewer
participant UI as UI/Cache
Note over LogCapture: New log created or received
LogCapture->>LogCapture: enqueue into _logs/_receivedLogs\n_trim to limit\ninvalidate cache\n_increment _revision\nemit on revisionStream
DebugViewer->>LogCapture: subscription receives revision event
alt revision changed
DebugViewer->>DebugViewer: read combined logs -> _cachedLogs\nschedule post-frame auto-scroll if enabled
DebugViewer->>UI: setState -> render _LogListView with _cachedLogs
else revision unchanged
DebugViewer->>UI: no rebuild
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/utils/logger.dart (1)
167-186:⚠️ Potential issue | 🟡 MinorDuplicate detection no longer checks
level, which may allow false matches.The duplicate check compares
timestamp,loggerName, andmessage, but notlevel. If two log entries have the same timestamp, logger, and message but different levels (e.g., an INFO and a WARNING), they would be considered duplicates and one would be dropped.Was the removal of the level comparison intentional? If logs with the same message but different severity can legitimately occur, this could cause data loss.
🛡️ Proposed fix to include level in duplicate check
final isDuplicate = recentLogsToCheck.any((log) => log.timestamp == parsedTimestamp && log.loggerName == loggerName && + log.level.value == levelValue && log.message == message) || recentReceivedToCheck.any((log) => log.timestamp == parsedTimestamp && log.loggerName == loggerName && + log.level.value == levelValue && log.message == message);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils/logger.dart` around lines 167 - 186, The duplicate-detection logic (variables recentLogsToCheck, recentReceivedToCheck and the isDuplicate computation) currently compares timestamp, loggerName and message but omits the log level; update the isDuplicate checks to also compare the log's level (e.g., include log.level == level) for both recentLogsToCheck.any(...) and recentReceivedToCheck.any(...) so entries with identical timestamp/loggerName/message but different severity are not treated as duplicates.
🧹 Nitpick comments (4)
lib/widgets/debug_viewer.dart (3)
20-21: Consider initializing_lastRevisionfrom current revision.
_lastRevisionis initialized to-1, but this means the first timer tick will always trigger an update even if logs haven't changed sinceinitState. Since_cachedLogsis also initialized ininitState(line 26), you could set_lastRevision = _logCapture.revisionthere as well to avoid the redundant first update.♻️ Proposed initialization alignment
- int _lastRevision = -1; + int _lastRevision = 0; List<LogEntry> _cachedLogs = []; `@override` void initState() { super.initState(); _cachedLogs = _logCapture.logs; + _lastRevision = _logCapture.revision;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/widgets/debug_viewer.dart` around lines 20 - 21, The _lastRevision currently starts at -1 causing the first timer tick to always call an update; in initState where _cachedLogs is set, also initialize _lastRevision from _logCapture.revision so it reflects the current logs and prevents an unnecessary initial refresh. Locate initState and update the initialization of _lastRevision (alongside _cachedLogs) to assign _logCapture.revision, ensuring the periodic timer logic compares against that value instead of -1.
29-50: Potential issue:addPostFrameCallbackmay queue multiple scroll animations.If logs arrive rapidly and multiple timer ticks fire before post-frame callbacks execute, you could queue multiple
animateTocalls. While Flutter handles this gracefully (animations are interrupted), it's slightly wasteful. Consider using a flag to debounce or skip if a scroll is already pending.Also, the nested
hasClientscheck on line 40 is good defensive coding after the async callback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/widgets/debug_viewer.dart` around lines 29 - 50, The periodic Timer in _refreshTimer can schedule multiple WidgetsBinding.instance.addPostFrameCallback scrolls when logs arrive quickly; modify the logic in the Timer callback to debounce pending scroll work by introducing a boolean flag (e.g., _scrollPending) that is set before calling addPostFrameCallback and cleared inside the callback, and only schedule a new addPostFrameCallback/animateTo when _autoScroll is true, _scrollController.hasClients is true, and _scrollPending is false; ensure the existing nested hasClients checks around _scrollController.animateTo remain to avoid races.
73-77: Cache is cleared but_lastRevisionis not reset.After clearing logs,
_logCapture.clear()increments_revisionin the logger. The next timer tick will seecurrentRevision != _lastRevision, causing anothersetStateand re-assignment of_cachedLogs(which would be empty anyway). This is benign but slightly redundant. If you want to avoid the extra setState, update_lastRevisionhere.♻️ Proposed fix to sync revision on clear
void _clearLogs() { _logCapture.clear(); _cachedLogs = []; + _lastRevision = _logCapture.revision; setState(() {}); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/widgets/debug_viewer.dart` around lines 73 - 77, The clear routine (_clearLogs) resets _logCapture and _cachedLogs but doesn't update _lastRevision, so the next timer tick will detect a revision change and call setState again; after calling _logCapture.clear() set _lastRevision to the logger's current revision (e.g. _logCapture.revision or whatever getter exposes the internal _revision) to keep the cache in sync and avoid the redundant setState; update the assignment in _clearLogs near where _cachedLogs is cleared and before returning.lib/utils/logger.dart (1)
79-100: Performance issue: O(n)removeAt(0)on List is inefficient for trimming.In Dart,
List.removeAt(0)is O(n) because it shifts all remaining elements. With up to 1000 logs and potentially many removals, this can become expensive. Consider using a more efficient data structure likeListQueue(double-ended queue) which has O(1) removal from the front.♻️ Alternative approach using ListQueue
import 'dart:collection'; // Change field declarations: final ListQueue<LogEntry> _logs = ListQueue(); final ListQueue<LogEntry> _receivedLogs = ListQueue(); // Then removeAt(0) becomes removeFirst(): _logs.removeFirst(); _receivedLogs.removeFirst();Note: This would require updating the
logsgetter to handle theListQueuetype when combining logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils/logger.dart` around lines 79 - 100, _trimLogsToLimit currently uses repeated List.removeAt(0) on _logs and _receivedLogs which is O(n); change the backing collections to ListQueue<LogEntry> for both _logs and _receivedLogs (import dart:collection), replace removeAt(0) calls with removeFirst(), and update any code that reads/combines logs (e.g., the public logs getter) to handle ListQueue (convert to List or iterate) so trimming becomes O(1) per removal while preserving the existing timestamp comparison logic and use of _maxLogs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/utils/logger.dart`:
- Around line 167-186: The duplicate-detection logic (variables
recentLogsToCheck, recentReceivedToCheck and the isDuplicate computation)
currently compares timestamp, loggerName and message but omits the log level;
update the isDuplicate checks to also compare the log's level (e.g., include
log.level == level) for both recentLogsToCheck.any(...) and
recentReceivedToCheck.any(...) so entries with identical
timestamp/loggerName/message but different severity are not treated as
duplicates.
---
Nitpick comments:
In `@lib/utils/logger.dart`:
- Around line 79-100: _trimLogsToLimit currently uses repeated List.removeAt(0)
on _logs and _receivedLogs which is O(n); change the backing collections to
ListQueue<LogEntry> for both _logs and _receivedLogs (import dart:collection),
replace removeAt(0) calls with removeFirst(), and update any code that
reads/combines logs (e.g., the public logs getter) to handle ListQueue (convert
to List or iterate) so trimming becomes O(1) per removal while preserving the
existing timestamp comparison logic and use of _maxLogs.
In `@lib/widgets/debug_viewer.dart`:
- Around line 20-21: The _lastRevision currently starts at -1 causing the first
timer tick to always call an update; in initState where _cachedLogs is set, also
initialize _lastRevision from _logCapture.revision so it reflects the current
logs and prevents an unnecessary initial refresh. Locate initState and update
the initialization of _lastRevision (alongside _cachedLogs) to assign
_logCapture.revision, ensuring the periodic timer logic compares against that
value instead of -1.
- Around line 29-50: The periodic Timer in _refreshTimer can schedule multiple
WidgetsBinding.instance.addPostFrameCallback scrolls when logs arrive quickly;
modify the logic in the Timer callback to debounce pending scroll work by
introducing a boolean flag (e.g., _scrollPending) that is set before calling
addPostFrameCallback and cleared inside the callback, and only schedule a new
addPostFrameCallback/animateTo when _autoScroll is true,
_scrollController.hasClients is true, and _scrollPending is false; ensure the
existing nested hasClients checks around _scrollController.animateTo remain to
avoid races.
- Around line 73-77: The clear routine (_clearLogs) resets _logCapture and
_cachedLogs but doesn't update _lastRevision, so the next timer tick will detect
a revision change and call setState again; after calling _logCapture.clear() set
_lastRevision to the logger's current revision (e.g. _logCapture.revision or
whatever getter exposes the internal _revision) to keep the cache in sync and
avoid the redundant setState; update the assignment in _clearLogs near where
_cachedLogs is cleared and before returning.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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)
lib/utils/logger.dart (1)
103-117:⚠️ Potential issue | 🟠 MajorPlease add focused tests for the new revision/retention paths before merge.
These are core consistency paths (
revision, dedup, trim, clear), and current patch coverage for this file is 0%. That leaves high regression risk for log viewer sync behavior.I can generate a compact test set covering: revision increments, dual-buffer trim ordering, dedup window behavior, and clear invalidation—want me to draft it?
Also applies to: 147-208, 230-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils/logger.dart` around lines 103 - 117, Add focused unit tests exercising the revision/retention paths: call the logger path that uses _handleLogRecord to assert LogEntry fields and that _revision increments and _isCacheValid flips false on new entries; create tests that push more than the retention limit to trigger _trimLogsToLimit and assert trimming order/dual-buffer behavior on _logs; add tests for the deduplication window (the dedup path invoked by _handleLogRecord/add entry) to confirm duplicate suppression within the window and acceptance after it; and test clear() or the clear path to ensure it empties buffers, invalidates cache, and updates revision. Reference LogEntry, _handleLogRecord, _trimLogsToLimit, and clear (and the dedup/dedup-window logic) when locating code under test.
🧹 Nitpick comments (2)
lib/widgets/debug_viewer.dart (1)
30-54: Consider moving refresh from polling to event-driven invalidation.The revision check prevents unnecessary rebuilds, but
Timer.periodicwakes every 100ms even when logs are unchanged. Exposing logs changes as aStreamorChangeNotifierfromLogCapturewould eliminate idle timer wakeups while maintaining the same update behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/widgets/debug_viewer.dart` around lines 30 - 54, Replace the polling Timer logic (_refreshTimer and its periodic callback that checks _logCapture.revision vs _lastRevision and updates _cachedLogs) with an event-driven subscription from LogCapture (e.g., a Stream or ChangeNotifier callback) that notifies on log revisions; in initState subscribe to that event (store as _logSubscription) and in the handler update _lastRevision/_cachedLogs, call setState(), and perform the same auto-scroll logic using _autoScroll, _scrollController and _scrollPending; remove the Timer and ensure you cancel/dispose the subscription in dispose() to avoid leaks.lib/utils/logger.dart (1)
170-176: Reduce allocation churn in duplicate detection hot path.
toList().sublist(...)allocates a full copy every timeaddReceivedLogruns. This is avoidable and can regress throughput under high log traffic.♻️ Proposed refactor
- final recentLogsToCheck = _logs.length > 50 - ? _logs.toList().sublist(_logs.length - 50) - : _logs.toList(); - final recentReceivedToCheck = _receivedLogs.length > 50 - ? _receivedLogs.toList().sublist(_receivedLogs.length - 50) - : _receivedLogs.toList(); + Iterable<LogEntry> recentTail(ListQueue<LogEntry> queue) { + return queue.length > 50 ? queue.skip(queue.length - 50) : queue; + } - final isDuplicate = recentLogsToCheck.any((log) => + final isDuplicate = recentTail(_logs).any((log) => log.timestamp == parsedTimestamp && log.loggerName == loggerName && log.level == level && log.message == message) || - recentReceivedToCheck.any((log) => + recentTail(_receivedLogs).any((log) => log.timestamp == parsedTimestamp && log.loggerName == loggerName && log.level == level && log.message == message);Also applies to: 177-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils/logger.dart` around lines 170 - 176, The duplicate-detection hot path is creating full copies via _logs.toList().sublist(...) and _receivedLogs.toList().sublist(...); change those to use List.getRange (or Iterable.getRange) so you only create a view of the tail instead of allocating the entire list each call. In the addReceivedLog flow replace recentLogsToCheck and recentReceivedToCheck assignments with ranges from _logs.getRange(startIndex, _logs.length) and _receivedLogs.getRange(startIndex, _receivedLogs.length) (or equivalent Iterable slicing) and iterate that range for duplicate checks to avoid the extra toList() copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/utils/logger.dart`:
- Around line 103-117: Add focused unit tests exercising the revision/retention
paths: call the logger path that uses _handleLogRecord to assert LogEntry fields
and that _revision increments and _isCacheValid flips false on new entries;
create tests that push more than the retention limit to trigger _trimLogsToLimit
and assert trimming order/dual-buffer behavior on _logs; add tests for the
deduplication window (the dedup path invoked by _handleLogRecord/add entry) to
confirm duplicate suppression within the window and acceptance after it; and
test clear() or the clear path to ensure it empties buffers, invalidates cache,
and updates revision. Reference LogEntry, _handleLogRecord, _trimLogsToLimit,
and clear (and the dedup/dedup-window logic) when locating code under test.
---
Nitpick comments:
In `@lib/utils/logger.dart`:
- Around line 170-176: The duplicate-detection hot path is creating full copies
via _logs.toList().sublist(...) and _receivedLogs.toList().sublist(...); change
those to use List.getRange (or Iterable.getRange) so you only create a view of
the tail instead of allocating the entire list each call. In the addReceivedLog
flow replace recentLogsToCheck and recentReceivedToCheck assignments with ranges
from _logs.getRange(startIndex, _logs.length) and
_receivedLogs.getRange(startIndex, _receivedLogs.length) (or equivalent Iterable
slicing) and iterate that range for duplicate checks to avoid the extra toList()
copy.
In `@lib/widgets/debug_viewer.dart`:
- Around line 30-54: Replace the polling Timer logic (_refreshTimer and its
periodic callback that checks _logCapture.revision vs _lastRevision and updates
_cachedLogs) with an event-driven subscription from LogCapture (e.g., a Stream
or ChangeNotifier callback) that notifies on log revisions; in initState
subscribe to that event (store as _logSubscription) and in the handler update
_lastRevision/_cachedLogs, call setState(), and perform the same auto-scroll
logic using _autoScroll, _scrollController and _scrollPending; remove the Timer
and ensure you cancel/dispose the subscription in dispose() to avoid leaks.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/widgets/debug_viewer.dart (1)
63-74:⚠️ Potential issue | 🟡 MinorAwait clipboard operation before showing success message.
Clipboard.setData()returns aFutureand can throwPlatformExceptionif the platform call fails. Currently, the method fire-and-forgets the clipboard write on line 66, then immediately shows a success snackbar regardless of the outcome. This gives false positive feedback if the clipboard write actually fails.Suggested fix
- void _copyAllLogsToClipboard() { + Future<void> _copyAllLogsToClipboard() async { final logs = _cachedLogs.map((log) => log.formattedMessage).join('\n'); - Clipboard.setData(ClipboardData(text: logs)); - - ScaffoldMessenger.of(context).showSnackBar( - const SnackBar( - content: Text('Logs copied to clipboard'), - duration: Duration(seconds: 2), - ), - ); + try { + await Clipboard.setData(ClipboardData(text: logs)); + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar( + content: Text('Logs copied to clipboard'), + duration: Duration(seconds: 2), + ), + ); + } on PlatformException { + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar( + content: Text('Failed to copy logs'), + duration: Duration(seconds: 2), + ), + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/widgets/debug_viewer.dart` around lines 63 - 74, The _copyAllLogsToClipboard method currently fires Clipboard.setData without awaiting or handling errors; change it to await Clipboard.setData(ClipboardData(text: logs)) inside a try-catch around that call, and only call ScaffoldMessenger.of(context).showSnackBar with the success SnackBar after the await succeeds; in the catch block, show a failure SnackBar (or log the PlatformException) so the user is notified if the clipboard write fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/utils/logger.dart`:
- Around line 88-108: _trimLogsToLimit assumes _receivedLogs is time-ordered but
incoming remote logs are appended unsorted; to fix, ensure any code that adds to
_receivedLogs (the append at the received-log path around the append at lines
~213-214) inserts logs in timestamp order or uses a timestamp-ordered insert
routine, or sort _receivedLogs by timestamp before _trimLogsToLimit runs; update
the insertion logic that currently does _receivedLogs.add(...) to perform a
binary-search insert by log.timestamp (or call a stable sort on _receivedLogs
after batch insert) so _trimLogsToLimit can safely compare _logs.first.timestamp
and _receivedLogs.first.timestamp.
In `@test/utils/logger_test.dart`:
- Around line 361-394: Make the test "logs getter combines and sorts logs from
both buffers" async, await the two Future.delayed(...) calls so timestamps are
actually separated, and then assert ordering rather than just presence: use
logger.info(...) and logCapture.addReceivedLog(...) as before, read
logCapture.logs and verify the sequence/order of messages (e.g., indexOf or
compare adjacent timestamps) to ensure logs are sorted by timestamp. Ensure you
update the test signature to be async and await the delays before capturing
midTimestamp and before the second logger.info call.
---
Outside diff comments:
In `@lib/widgets/debug_viewer.dart`:
- Around line 63-74: The _copyAllLogsToClipboard method currently fires
Clipboard.setData without awaiting or handling errors; change it to await
Clipboard.setData(ClipboardData(text: logs)) inside a try-catch around that
call, and only call ScaffoldMessenger.of(context).showSnackBar with the success
SnackBar after the await succeeds; in the catch block, show a failure SnackBar
(or log the PlatformException) so the user is notified if the clipboard write
fails.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Performance
New Features
UI Updates
Tests