refactor(logging): route raw print through unified_logger#4521
Merged
Conversation
untreu2
requested review from
NotThatKindOfDrLiz and
rabble
and removed request for
rabble
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
NotThatKindOfDrLiz
requested changes
May 18, 2026
Member
NotThatKindOfDrLiz
left a comment
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Requesting changes for four issues that should be fixed in this PR before merge:
- models now depends on unified_logger even though unified_logger already depends on models, which creates a package cycle and pulls a Flutter dependent package into a shared data package.
- The VideoEvent parsing path now routes 55 debug statements through Log.debug. In this logger implementation that still allocates LogEntry objects and writes them into the always-on in-memory capture buffer in release, so this materially changes the runtime cost of a hot feed path.
- The new raw-logging guard script is not enforced anywhere in CI, so the PR currently claims a regression guard that does not actually run.
- The cache fallback logging path in InfiniteVideoFeed drops the stack trace even though the sibling error paths in the same file still preserve it.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…nified_logger Closes #3346. ## What changed Migrated all production app and package code away from raw print(), debugPrint(), and dart:developer log() to Log.debug/info/warning/error via unified_logger. This ensures every log message is: - subject to category filtering and production verbosity controls - captured in the 50k-entry memory ring buffer (used for bug reports) - consistently named and categorised ## Files touched **mobile/lib/** (21 files, ~38 call sites): - Deleted double-logging debug scaffolding in video_loading_metrics.dart (print() alongside existing UnifiedLogger.info() calls, plus the METRICS_TEST_OUTPUT test probe string) - Migrated main.dart, native_proofmode_service, video_event_service, nip07_service, analytics_service, api_service, bandwidth_tracker_service, video_format_preference, connection_status_service, crash_reporting_service, error_analytics_tracker, startup_coordinator, feature_flag_service, video_event_extensions, subtitle_providers, pooled_fullscreen_video_feed_screen, new_message_search_bloc, sounds_tab, branded_loading_indicator, classic_viners_slider **mobile/packages/** (17 packages, ~180 call sites): - Added unified_logger dep to: keycast_flutter, models, pooled_video_player, media_cache, infinite_video_feed, videos_repository, notification_repository, people_lists_repository, reposts_repository, likes_repository, hashtag_repository, image_metadata_stripper, nostr_key_manager, nostr_app_bridge_repository, divine_camera, hls_auth_web_player, tv_static_effect, sound_service - nostr_sdk and nostr_client intentionally excluded (publishable SDK packages; cannot depend on app-internal unified_logger) ## Lint mobile/analysis_options.yaml: avoid_print: false -> true ## Regression guard Added mobile/scripts/check_raw_logging.sh — exits non-zero if any production .dart file outside the documented exceptions imports dart:developer, calls debugPrint(), or calls print() directly.
Edit during logging migration accidentally closed the class early, stranding all static helper methods outside the class body and causing a parse error.
- Remove unused cacheStackTrace catch variable in infinite_video_feed - Remove invalid level: 800 param from Log.debug in audio_playback_service (Log.debug has no level: param; 800 is the debug level it already implies) - Sort unified_logger imports alphabetically to satisfy directives_ordering lint in likes_repository, reposts_repository, people_lists_repository_impl, platform_secure_storage, secure_key_storage
CI runs 'flutter analyze lib test integration_test' so the avoid_print rule we enabled was flagging legitimate print() calls in test fixtures and benchmark helpers. Add test/analysis_options.yaml that inherits all other rules but sets avoid_print: false — test output is expected there.
…ale test Fixes unnecessary_raw_strings and prefer_interpolation_to_compose_strings lint warnings introduced when avoid_print was enabled and test/ gained its own analysis_options.yaml that applies the full rule set.
Four issues raised in code review: **models ⇄ unified_logger cycle (breaking)** Remove unified_logger from models/pubspec.yaml. unified_logger already depends on models, so the reverse dep created a package cycle and dragged a Flutter-dependent package into a shared leaf data package. pending_upload.dart reverts to developer.log (level: 1000) which is correct for a package that cannot take the unified_logger dep. **55 Log.debug calls on hot parse path (perf)** Delete all Log.debug calls from video_event.dart. These were parse-time debug probes that existed before this PR as developer.log calls. Migrating them 1:1 to Log.debug was wrong: UnifiedLogger._log always allocates a LogEntry and writes it into the always-on 50k in-memory ring buffer even in release, turning debug noise into steady allocation churn on the feed scroll path. The debug probes are deleted entirely rather than migrated. **cacheStackTrace dropped in InfiniteVideoFeed (correctness)** Restore catch (cacheError, cacheStackTrace) and pass stackTrace to Log.error. The stack trace was accidentally dropped when removing the unused_catch_stack warning; it belongs here — cache-corruption failures are exactly where a stack trace is useful for debugging. **Regression guard not enforced in CI (guard gap)** Wire check_raw_logging.sh into .github/workflows/mobile_ci.yaml in the generated-files job alongside check_native_transport_security.sh. Also fix the print() grep to use a Perl-compatible negative lookbehind so comment lines (// print(...) and /// print(...)) are not flagged as violations, and add models/ to the documented exception list.
…gger linux_video_player_backend.dart was added to origin/main after this branch was cut and contained two developer.log calls that the CI guard caught on the runner. Add unified_logger dep and migrate both calls to Log.warning.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… in VineBottomSheetActionMenu - models: remove unused dart:developer import, dead debug loop (tagName), and unused catch clause (e) - divine_ui: wrap ListTile in Material(color: transparent) to prevent ListTile+ColoredBox assertion from Flutter framework
…patibility The CI pins Flutter 3.41.9 (Dart 3.7.x) whose formatter places the trailing semicolon of enum bodies on its own line when the enum is split across lines. Dart 3.12.0 (local) consolidates it onto the last value line. Restore the Dart 3.7.x layout so the CI format check passes.
This comment has been minimized.
This comment has been minimized.
Member
|
Pushed a cleanup follow-up for the dead code left behind in mobile/packages/models/lib/src/video_event.dart after the raw logging removal. This removes the empty if/else blocks and stale debug comment without changing behavior.\n\nFocused verification run locally:\n- bash mobile/scripts/check_raw_logging.sh\n- flutter test test/src/video_event_parsing_test.dart from mobile/packages/models\n- flutter test test/unit/models/video_event_multiple_imeta_test.dart from mobile\n\nThe repo pre-push gate also passed analyze, generated-file verification, and the changed-file test suite before the push completed. I’m watching the PR checks now and will only call it ready once they come back green. |
Mobile PR PreviewPreview refreshed for Last refresh:
|
NotThatKindOfDrLiz
approved these changes
May 18, 2026
Member
NotThatKindOfDrLiz
left a comment
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Re-reviewed after the cleanup follow-up. The dead code left behind in video_event.dart is removed, the stack trace preservation fix is intact, and the raw-logging guard is wired and passing. CI is green across Mobile CI, preview build, and the package matrix.
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.
What changed
Migrated all production app and package code away from raw print(), debugPrint(), and dart:developer log() to Log.debug/info/warning/error via unified_logger. This ensures every log message is:
Files touched
mobile/lib/ (21 files, ~38 call sites):
mobile/packages/ (17 packages, ~180 call sites):
Lint
mobile/analysis_options.yaml: avoid_print: false -> true
Regression guard
Added mobile/scripts/check_raw_logging.sh — exits non-zero if any production .dart file outside the documented exceptions imports dart:developer, calls debugPrint(), or calls print() directly.
Related Issue: Closes #3346
Type of Change