feat: Adds serverpod logging package#115
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 46 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a new serverpod_logging package: core logging types and writers, an async-safe Log orchestration with Zone-scoped progress(), interactive spinner and text writers, a test writer, global singletons, package metadata/config, and unit tests for progress behavior. ChangesServerpod Logging Package
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
🧹 Nitpick comments (3)
packages/serverpod_logging/lib/src/spinner_log_writer.dart (1)
113-123: ⚡ Quick winOverride
close()to cancel the periodic timer.
_timeris only stopped whenscopeStackempties viacloseScope. If the writer is disposed while scopes remain open (e.g., abnormal shutdown or aprogressthat never closes), theTimer.periodickeeps firing and can keep the event loop alive.LogWriter.close()exists for exactly this resource-release purpose but isn't overridden here.♻️ Proposed cleanup hook
void _stopTimer() { _timer?.cancel(); _timer = null; } + + `@override` + Future<void> close() async { + _stopTimer(); + await super.close(); + }🤖 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 `@packages/serverpod_logging/lib/src/spinner_log_writer.dart` around lines 113 - 123, The class starts a periodic Timer via _startTimer and only cancels it in _stopTimer when scopeStack is emptied via closeScope; override the LogWriter.close() method to ensure any remaining timer is cancelled on writer disposal by calling _stopTimer() (and any other cleanup like draining scopeStack if needed) so the periodic Timer (_timer) cannot keep firing after the writer is closed; update the class to implement close() that calls _stopTimer() and then calls super.close() or existing disposal logic.packages/serverpod_logging/lib/src/log.dart (1)
70-132: 💤 Low valueConsider adding a
fatal()convenience method for completeness.
LogLevel.fatalexists but has no corresponding convenience method inLogConvenience. If fatal-level logging is expected to be used, consider adding:void fatal( String message, { Object? error, StackTrace? stackTrace, Map<String, Object?>? metadata, }) => this( LogLevel.fatal, () => LogEntry( time: DateTime.now(), level: LogLevel.fatal, message: message, scope: currentScope, error: error, stackTrace: stackTrace, metadata: metadata, ), );🤖 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 `@packages/serverpod_logging/lib/src/log.dart` around lines 70 - 132, Add a fatal() convenience method to the LogConvenience extension that mirrors the error() method but uses LogLevel.fatal; implement it on the extension (LogConvenience) so it calls this(LogLevel.fatal, () => LogEntry(...)) with time: DateTime.now(), level: LogLevel.fatal, message, scope: currentScope and optional error, stackTrace, and metadata parameters, and also ensure isDebugEnabled remains unchanged.packages/serverpod_logging/lib/src/log_types.dart (1)
52-57: 💤 Low valueMultiple root scopes share the same
id: 'root'.If
LogScope.root()is called multiple times (e.g., in tests or parallel processes), all root scopes will have identicalidvalues, which may complicate log correlation or debugging. Consider whether this is intentional or if a unique identifier should be generated.🤖 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 `@packages/serverpod_logging/lib/src/log_types.dart` around lines 52 - 57, The LogScope.root factory is assigning a constant id 'root' causing multiple root scopes to share the same identifier; change LogScope.root to generate a unique id (e.g., using a UUID, a monotonic counter, or incorporate DateTime.now().microsecondsSinceEpoch) instead of the literal 'root' so each root scope (created via LogScope.root) has a distinct id while keeping label and startTime as-is; update any related tests or consumers of LogScope.id to accept the generated unique id.
🤖 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 `@packages/serverpod_logging/lib/src/log_types.dart`:
- Around line 143-177: The iteration methods (log, openScope, closeScope, close)
currently iterate directly over the mutable _writers list while add/remove can
mutate it, causing ConcurrentModificationError; fix by taking a snapshot (e.g.,
final writers = List.of(_writers)) at the start of each of LogChain.log,
LogChain.openScope, LogChain.closeScope, and LogChain.close and iterate over
that snapshot so concurrent add/remove calls won’t modify the iterated
collection; ensure add and remove keep their current behavior and no other logic
changes are required.
In `@packages/serverpod_logging/lib/src/log.dart`:
- Around line 51-58: The problem is that call(LogLevel level, LogEntryFactory
factory) defers invoking the factory until the chained future executes, so
scope-dependent values (e.g., currentScope used by LogConvenience.info()) are
captured too late; fix by eagerly evaluating and capturing scope-related data
before queuing: invoke the factory (or at least capture currentScope and any
other mutable context) immediately inside call() and store the resulting
LogEntry (or a closure with captured values) to be passed to _writer.log inside
the _latest.then(...) block, keeping the existing _latest chaining and error
handling with _writer.log(await factory()) replaced by
_writer.log(capturedEntry) or similar so scope is resolved at call time.
In `@packages/serverpod_logging/test/log_progress_test.dart`:
- Around line 5-95: The tests should be reorganized to nest action groups under
each "Given" group: for each existing group block (the three groups around
Log.progress), create an inner group('when ...') that contains the current test
cases, move the test descriptions to start with "then ..." and keep the existing
setUp() and assertions intact; specifically update the tests that call
log.progress (including usages of log.progress<T>, TestLogWriter, and checks of
writer.closedScopes.success/error) so they are inside their respective when
groups, preserving behavior and assertions but renaming the test descriptions to
match "then ..." convention.
---
Nitpick comments:
In `@packages/serverpod_logging/lib/src/log_types.dart`:
- Around line 52-57: The LogScope.root factory is assigning a constant id 'root'
causing multiple root scopes to share the same identifier; change LogScope.root
to generate a unique id (e.g., using a UUID, a monotonic counter, or incorporate
DateTime.now().microsecondsSinceEpoch) instead of the literal 'root' so each
root scope (created via LogScope.root) has a distinct id while keeping label and
startTime as-is; update any related tests or consumers of LogScope.id to accept
the generated unique id.
In `@packages/serverpod_logging/lib/src/log.dart`:
- Around line 70-132: Add a fatal() convenience method to the LogConvenience
extension that mirrors the error() method but uses LogLevel.fatal; implement it
on the extension (LogConvenience) so it calls this(LogLevel.fatal, () =>
LogEntry(...)) with time: DateTime.now(), level: LogLevel.fatal, message, scope:
currentScope and optional error, stackTrace, and metadata parameters, and also
ensure isDebugEnabled remains unchanged.
In `@packages/serverpod_logging/lib/src/spinner_log_writer.dart`:
- Around line 113-123: The class starts a periodic Timer via _startTimer and
only cancels it in _stopTimer when scopeStack is emptied via closeScope;
override the LogWriter.close() method to ensure any remaining timer is cancelled
on writer disposal by calling _stopTimer() (and any other cleanup like draining
scopeStack if needed) so the periodic Timer (_timer) cannot keep firing after
the writer is closed; update the class to implement close() that calls
_stopTimer() and then calls super.close() or existing disposal logic.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 203e04fc-e698-4487-8233-c3cf401339e2
📒 Files selected for processing (13)
packages/serverpod_logging/.gitignorepackages/serverpod_logging/CHANGELOG.mdpackages/serverpod_logging/README.mdpackages/serverpod_logging/analysis_options.yamlpackages/serverpod_logging/lib/serverpod_logging.dartpackages/serverpod_logging/lib/src/global_log.dartpackages/serverpod_logging/lib/src/log.dartpackages/serverpod_logging/lib/src/log_types.dartpackages/serverpod_logging/lib/src/spinner_log_writer.dartpackages/serverpod_logging/lib/src/test_log_writer.dartpackages/serverpod_logging/lib/src/text_log_writer.dartpackages/serverpod_logging/pubspec.yamlpackages/serverpod_logging/test/log_progress_test.dart
nielsenko
approved these changes
Jun 4, 2026
Contributor
nielsenko
left a comment
nielsenko
left a comment
There was a problem hiding this comment.
Approved. Replied on @CodeRabbit comments
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/serverpod_logging/lib/src/log_types.dart (1)
139-139:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
addmethod discards the new list — writers are never actually added.The arrow expression creates
[..._writers, writer]but never assigns it to_writers. Since the return type isvoid, the result is discarded and the method has no effect.Based on learnings, the copy-on-write pattern requires assignment:
_writers = [..._writers, writer].🐛 Proposed fix
- void add(LogWriter writer) => [..._writers, writer]; + void add(LogWriter writer) => _writers = [..._writers, writer];🤖 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 `@packages/serverpod_logging/lib/src/log_types.dart` at line 139, The add method currently constructs a new list with [..._writers, writer] but discards it, so _writers is never updated; modify the add(LogWriter writer) implementation to assign the new list back to the backing field (i.e., set _writers = [..._writers, writer]) so the writer is actually added, preserving the copy-on-write behavior used by the class.
🤖 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.
Duplicate comments:
In `@packages/serverpod_logging/lib/src/log_types.dart`:
- Line 139: The add method currently constructs a new list with [..._writers,
writer] but discards it, so _writers is never updated; modify the add(LogWriter
writer) implementation to assign the new list back to the backing field (i.e.,
set _writers = [..._writers, writer]) so the writer is actually added,
preserving the copy-on-write behavior used by the class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9bcd0b1f-8b38-4b43-9a0a-3fe8fcff4ffe
📒 Files selected for processing (3)
.github/workflows/ci.ymlpackages/serverpod_logging/lib/src/log.dartpackages/serverpod_logging/lib/src/log_types.dart
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/serverpod_logging/lib/src/log.dart
This was referenced Jun 5, 2026
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.
This PR adds a
serverpod_loggingpackage that contains tools for bridging logging between Serverpod framework and CLI tooling.Summary by CodeRabbit
New Features
Documentation
Tests
Chores