feat: implement SimplePrintLogger for better debugging#176
feat: implement SimplePrintLogger for better debugging#176conventoangelo merged 5 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
⌛ 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. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a SimplePrintLogger utility and replaces ad-hoc prints with structured logging across multiple services; expands HotKeyService setupHotKeys signature to include per-hotkey enable flags and a master switch; KanataService connection flow gains reconnection scheduling and explicit reconnect enabling. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant KanataService
participant Socket
participant ReconnectScheduler
participant Logger
App->>KanataService: connect()
KanataService->>Logger: _log.info('connecting')
KanataService->>Socket: open connection
alt connection success
Socket-->>KanataService: onConnected
KanataService->>Logger: _log.info('connected')
KanataService->>ReconnectScheduler: cancel pending reconnects
else connection failure / socket error
Socket-->>KanataService: onError / onDisconnected
KanataService->>Logger: _log.error/_log.warning(...)
KanataService->>ReconnectScheduler: _scheduleReconnect()
ReconnectScheduler->>KanataService: trigger reconnect after delay
KanataService->>Logger: _log.debug('reconnecting')
KanataService->>Socket: reopen connection
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
lib/services/startup_service.dart (1)
6-8: Minor: Missing blank line between doc comments.The doc comment for the logger instance and the doc comment for the method run together without a blank line separator, which is slightly inconsistent with other files in this PR.
♻️ Add blank line for consistency
/// Logger instance for this service final _log = SimplePrintLogger('StartupService'); + /// Enables or disables the application from launching at system startup🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/startup_service.dart` around lines 6 - 8, The two doc comments run together for the logger and the following member; insert a single blank line between the doc comment above the field "_log" (type SimplePrintLogger in class StartupService) and the next doc comment so the logger doc and the "Enables or disables the application..." doc are separated for consistency with other files.lib/services/hotkey_service.dart (1)
2-2: Potentially unused import.The
flutter/foundation.dartimport appears to be unused in this file. ThekDebugModecheck is handled internally bySimplePrintLogger.♻️ Remove unused import
import 'package:hotkey_manager/hotkey_manager.dart'; -import 'package:flutter/foundation.dart'; import '../utils/logger.dart';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/hotkey_service.dart` at line 2, Remove the unused import of flutter/foundation.dart: delete the import line and ensure no direct references to kDebugMode remain in HotkeyService; rely on SimplePrintLogger's internal kDebugMode handling, then run the analyzer/format to confirm no unused-import warnings.lib/services/config_service.dart (1)
120-122: Consider reducing log level for missing customFont.When
customFontis not defined in the config file, this is likely an expected condition (user hasn't configured a custom font yet) rather than a warning-worthy situation. Usingwarninglevel here may create noise in debug logs.♻️ Use debug level or remove the log
if (config.customFont == null) { - _log.warning( - 'Cannot get custom font: customFont is not defined in the config file'); + _log.debug('customFont not defined in config file'); return null; }Alternatively, simply remove the log since returning
nullis a valid and expected response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/config_service.dart` around lines 120 - 122, This warning-level log for a missing customFont is noisy; replace the _log.warning call with a lower-severity debug/fine log (e.g., _log.fine(...)) or remove the log entirely since returning null is an expected outcome when customFont is absent; update the call to _log.fine('customFont not defined in config') (or delete the log line) in the method that returns null for customFont (the location calling _log.warning and returning null) so the behavior remains the same but without a warning-level entry.lib/utils/logger.dart (1)
26-27: Consider consistent formatting for error details.The
Error:andStackTrace:lines are printed without timestamps or logger names, which may make it harder to correlate them with the main log message when multiple services are logging concurrently.♻️ Optional: Add context to error/stackTrace lines
void warning(String message, {Object? error, StackTrace? stackTrace}) { if (kDebugMode) { print('[${_timestamp()}] [$name] ⚠️ $message'); - if (error != null) print('Error: $error'); - if (stackTrace != null) print('StackTrace: $stackTrace'); + if (error != null) print('[${_timestamp()}] [$name] Error: $error'); + if (stackTrace != null) print('[${_timestamp()}] [$name] StackTrace: $stackTrace'); } } void error(String message, {Object? error, StackTrace? stackTrace}) { if (kDebugMode) { print('[${_timestamp()}] [$name] ❌ $message'); - if (error != null) print('Error: $error'); - if (stackTrace != null) print('StackTrace: $stackTrace'); + if (error != null) print('[${_timestamp()}] [$name] Error: $error'); + if (stackTrace != null) print('[${_timestamp()}] [$name] StackTrace: $stackTrace'); } }Also applies to: 34-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils/logger.dart` around lines 26 - 27, The error/stackTrace prints currently output raw lines "Error: $error" and "StackTrace: $stackTrace" which lack the timestamp/logger context; change these branches (the null-checks that print those lines) to emit the same formatted log prefix as the main message (include timestamp, logger name and level) instead of plain prints—e.g., reuse the existing timestamp/label formatter used elsewhere in this file and prepend it to the "Error:" and "StackTrace:" text so they can be correlated with the primary log entry.
🤖 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/hooks.dart`:
- Around line 68-69: The critical-error logging currently uses _log.error
(SimplePrintLogger) right before calling exit(1), which is gated by kDebugMode
and will be invisible in release builds; update those places (the _log.error +
exit(1) occurrences and the other instances referenced) to either call
print(...) immediately before exit(1) or use a non-debug-gated logger so the
message is always emitted in production; locate the calls by searching for
_log.error followed by exit(1) in hooks.dart and replace or supplement them with
an unconditional print or a production-capable logger call.
---
Nitpick comments:
In `@lib/services/config_service.dart`:
- Around line 120-122: This warning-level log for a missing customFont is noisy;
replace the _log.warning call with a lower-severity debug/fine log (e.g.,
_log.fine(...)) or remove the log entirely since returning null is an expected
outcome when customFont is absent; update the call to _log.fine('customFont not
defined in config') (or delete the log line) in the method that returns null for
customFont (the location calling _log.warning and returning null) so the
behavior remains the same but without a warning-level entry.
In `@lib/services/hotkey_service.dart`:
- Line 2: Remove the unused import of flutter/foundation.dart: delete the import
line and ensure no direct references to kDebugMode remain in HotkeyService; rely
on SimplePrintLogger's internal kDebugMode handling, then run the
analyzer/format to confirm no unused-import warnings.
In `@lib/services/startup_service.dart`:
- Around line 6-8: The two doc comments run together for the logger and the
following member; insert a single blank line between the doc comment above the
field "_log" (type SimplePrintLogger in class StartupService) and the next doc
comment so the logger doc and the "Enables or disables the application..." doc
are separated for consistency with other files.
In `@lib/utils/logger.dart`:
- Around line 26-27: The error/stackTrace prints currently output raw lines
"Error: $error" and "StackTrace: $stackTrace" which lack the timestamp/logger
context; change these branches (the null-checks that print those lines) to emit
the same formatted log prefix as the main message (include timestamp, logger
name and level) instead of plain prints—e.g., reuse the existing timestamp/label
formatter used elsewhere in this file and prepend it to the "Error:" and
"StackTrace:" text so they can be correlated with the primary log entry.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lib/app.dartlib/services/config_service.dartlib/services/configuration_loader.dartlib/services/hotkey_service.dartlib/services/kanata_service.dartlib/services/key_event_service.dartlib/services/method_call_handler.dartlib/services/startup_service.dartlib/utils/hooks.dartlib/utils/logger.dart
| _log.error('Failed to register session notification'); | ||
| exit(1); |
There was a problem hiding this comment.
Critical errors logged only in debug mode.
These _log.error calls are followed by exit(1), indicating critical failures that terminate the application. However, SimplePrintLogger only outputs in kDebugMode, meaning these error messages won't appear in release builds before the app exits.
Consider using print() directly (or a logger that isn't gated by kDebugMode) for truly critical errors that cause immediate termination, to aid in diagnosing production issues.
💡 Example: Use print for critical errors
if (result == 0) {
- _log.error('Failed to register session notification');
+ // Use print directly for critical errors that cause exit
+ print('[Hooks] ❌ Failed to register session notification');
exit(1);
}Also applies to: 100-102, 117-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/utils/hooks.dart` around lines 68 - 69, The critical-error logging
currently uses _log.error (SimplePrintLogger) right before calling exit(1),
which is gated by kDebugMode and will be invisible in release builds; update
those places (the _log.error + exit(1) occurrences and the other instances
referenced) to either call print(...) immediately before exit(1) or use a
non-debug-gated logger so the message is always emitted in production; locate
the calls by searching for _log.error followed by exit(1) in hooks.dart and
replace or supplement them with an unconditional print or a production-capable
logger call.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
2 participants
Summary by CodeRabbit
New Features
Chores