feat: Add support for progress stream to serverpod_logging#118
Conversation
|
Warning Review limit reached
More reviews will be available in 28 minutes and 19 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 (5)
📝 WalkthroughWalkthroughThis PR extends the logging system with stream-based progress tracking, Zone-integrated writer context, and scope mutation capabilities. Scope instances now support copyWith for label updates, the LogWriter interface adds updateScope for in-flight modifications, and the progress API gains a progressStream variant that consumes events with per-step label updates via the new Log.currentWriter getter wired into Zone values. ChangesZone-based progress stream with scope mutation and UI updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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: 5
🤖 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 127-128: The new abstract method updateScope(LogScope scope) is
source-breaking for existing LogWriter implementations; change it to a
non-required overridable no-op by giving updateScope a default body (e.g.,
return a completed Future) in the class that declares it so implementations of
LogWriter/LogScope remain compatible while still allowing overrides.
In `@packages/serverpod_logging/lib/src/log.dart`:
- Around line 224-225: The current code unconditionally maps the stream value
into a scope label via toMessage fallback, causing spurious labels (e.g.,
"null", "true", "Instance of ...") for progress(); change this so updateScope is
only called when an explicit message mapper is provided or the caller opted into
labeling: guard the call to
currentWriter?.updateScope(currentScope.copyWith(label: message)) behind a check
for toMessage != null (or an internal opt-out flag if present), and avoid using
event.toString() as a default; update the logic in the
progress()/stream-handling path that computes message, referencing toMessage,
event, currentWriter, updateScope, and currentScope.copyWith.
- Around line 152-163: The current progress() eagerly invokes runner() by
building Stream.fromFuture(runner()) before progressStream() opens its scope;
change progress() so the runner is invoked only when the returned stream is
listened to — e.g., create a lazy stream (using an async* generator that awaits
runner() or a StreamController/onListen that calls runner()) and pass that lazy
Stream to progressStream(label, lazyStream, ...), preserving metadata and
isSuccess and keeping the runner type as Future<T> Function().
In `@packages/serverpod_logging/test/test_utils/io_helper.dart`:
- Around line 7-9: The function signature for collectOutput (returning
Future<({MockStdout stdout, MockStdout stderr, MockStdin stdin})>) is
incorrectly wrapped/indented and fails dart format; run `dart format` on the
file or reflow the declaration so the generic Future<> and the parameter list
align with Dart style (adjust spacing/line breaks around FutureOr, the
tuple-like return type and the parameters) to match the formatter’s expected
layout for collectOutput, MockStdout, and MockStdin declarations.
In `@packages/serverpod_logging/test/test_utils/mock_stdin.dart`:
- Around line 245-247: Reformat the multiline method signature for transform to
match Dart formatting rules: ensure the generic and parameters are arranged on a
single properly indented line or split across lines as dart format expects (the
method header starting with "Stream<S> transform<S>(" and its parameter "final
StreamTransformer<List<int>, S> streamTransformer)") and keep the body throwing
UnimplementedError unchanged; run `dart format --set-exit-if-changed` to verify.
🪄 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: ee964966-da63-46b5-9bda-15ccd8a261a7
📒 Files selected for processing (8)
packages/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/test/progress_test.dartpackages/serverpod_logging/test/test_utils/io_helper.dartpackages/serverpod_logging/test/test_utils/mock_stdin.dartpackages/serverpod_logging/test/test_utils/mock_stdout.dart
| /// Updates a scoped operation. | ||
| Future<void> updateScope(LogScope scope); |
There was a problem hiding this comment.
Give updateScope() a default no-op body.
Adding a new abstract member here is a source-breaking change for any downstream LogWriter implementation. Unless this package is intentionally taking a major-version API break, keep the new hook overridable but non-required.
Suggested change
/// Updates a scoped operation.
- Future<void> updateScope(LogScope scope);
+ Future<void> updateScope(LogScope scope) async {}📝 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.
| /// Updates a scoped operation. | |
| Future<void> updateScope(LogScope scope); | |
| /// Updates a scoped operation. | |
| Future<void> updateScope(LogScope scope) async {} |
🤖 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 127 - 128,
The new abstract method updateScope(LogScope scope) is source-breaking for
existing LogWriter implementations; change it to a non-required overridable
no-op by giving updateScope a default body (e.g., return a completed Future) in
the class that declares it so implementations of LogWriter/LogScope remain
compatible while still allowing overrides.
| Future<T> progress<T>( | ||
| String label, | ||
| FutureOr<T> Function() runner, { | ||
| Future<T> Function() runner, { | ||
| Map<String, Object?>? metadata, | ||
| bool Function(T result)? isSuccess, | ||
| }) async { | ||
| return await progressStream( | ||
| label, | ||
| Stream.fromFuture(runner()), | ||
| isSuccess: isSuccess, | ||
| metadata: metadata, | ||
| ); |
There was a problem hiding this comment.
Make progress() lazy again.
Line 160 invokes runner() while building Stream.fromFuture(...), before progressStream() opens the scope or installs the Zone. That means log calls before the runner’s first await escape the new scope, synchronous throws bypass the normal close-with-failure path, and the callback type got narrowed unnecessarily to Future<T> Function().
Suggested change
Future<T> progress<T>(
String label,
- Future<T> Function() runner, {
+ FutureOr<T> Function() runner, {
Map<String, Object?>? metadata,
bool Function(T result)? isSuccess,
}) async {
return await progressStream(
label,
- Stream.fromFuture(runner()),
+ Stream<void>.value(null).asyncMap((_) => runner()),
isSuccess: isSuccess,
metadata: metadata,
);
}Based on learnings, Log.call() in this package intentionally defers work until execution time to avoid breaking scope propagation across boundaries.
📝 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.
| Future<T> progress<T>( | |
| String label, | |
| FutureOr<T> Function() runner, { | |
| Future<T> Function() runner, { | |
| Map<String, Object?>? metadata, | |
| bool Function(T result)? isSuccess, | |
| }) async { | |
| return await progressStream( | |
| label, | |
| Stream.fromFuture(runner()), | |
| isSuccess: isSuccess, | |
| metadata: metadata, | |
| ); | |
| Future<T> progress<T>( | |
| String label, | |
| FutureOr<T> Function() runner, { | |
| Map<String, Object?>? metadata, | |
| bool Function(T result)? isSuccess, | |
| }) async { | |
| return await progressStream( | |
| label, | |
| Stream<void>.value(null).asyncMap((_) => runner()), | |
| isSuccess: isSuccess, | |
| 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 152 - 163, The
current progress() eagerly invokes runner() by building
Stream.fromFuture(runner()) before progressStream() opens its scope; change
progress() so the runner is invoked only when the returned stream is listened to
— e.g., create a lazy stream (using an async* generator that awaits runner() or
a StreamController/onListen that calls runner()) and pass that lazy Stream to
progressStream(label, lazyStream, ...), preserving metadata and isSuccess and
keeping the runner type as Future<T> Function().
| final message = toMessage?.call(event) ?? event.toString(); | ||
| await currentWriter?.updateScope(currentScope.copyWith(label: message)); |
There was a problem hiding this comment.
Don’t turn the stream value into a progress label by default.
This fallback makes progress() emit an extra update like null..., true..., or Instance of ... right before close, because its delegated stream only carries the runner result. That regresses both interactive spinner output and non-interactive stdout. Only update the scope label when an explicit message mapper is provided, or give progress() an internal opt-out.
Suggested change
- final message = toMessage?.call(event) ?? event.toString();
- await currentWriter?.updateScope(currentScope.copyWith(label: message));
+ final message = toMessage?.call(event);
+ if (message != null && message != currentScope.label) {
+ await currentWriter?.updateScope(currentScope.copyWith(label: message));
+ }📝 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.
| final message = toMessage?.call(event) ?? event.toString(); | |
| await currentWriter?.updateScope(currentScope.copyWith(label: message)); | |
| final message = toMessage?.call(event); | |
| if (message != null && message != currentScope.label) { | |
| await currentWriter?.updateScope(currentScope.copyWith(label: message)); | |
| } |
🤖 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 224 - 225, The
current code unconditionally maps the stream value into a scope label via
toMessage fallback, causing spurious labels (e.g., "null", "true", "Instance of
...") for progress(); change this so updateScope is only called when an explicit
message mapper is provided or the caller opted into labeling: guard the call to
currentWriter?.updateScope(currentScope.copyWith(label: message)) behind a check
for toMessage != null (or an internal opt-out flag if present), and avoid using
event.toString() as a default; update the logic in the
progress()/stream-handling path that computes message, referencing toMessage,
event, currentWriter, updateScope, and currentScope.copyWith.
1 participant
Loggains aprogressStreammethod to mirror theLoggerinterface fromcli_tools.Summary by CodeRabbit
Release Notes
New Features
Changes
Futurereturn types (breaking change)Tests