-
Notifications
You must be signed in to change notification settings - Fork 17
Feat: Logs recording indicator #418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Implements a floating widget that indicates when logging is enabled. This widget navigates the user to the logs screen on tap. The indicator is only visible when logging is enabled and not on the logs screen.
WalkthroughAdds a global LogsRecordingIndicator widget rendered across routes (except Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Routing
participant Indicator as LogsRecordingIndicator
participant Settings as Settings Provider
participant Drawer as Drawer Provider
participant Logs as Logs Screen
App->>Indicator: instantiate on route render
Indicator->>Settings: query isLoggingEnabled
alt logging disabled
Indicator->>App: render nothing
else logging enabled
Indicator->>Drawer: query drawer/bottom-nav state
Drawer-->>Indicator: state
Indicator->>Indicator: compute position & start animation
Indicator->>App: render pulsing indicator (overlay)
App->>Indicator: user taps indicator
Indicator->>Logs: navigate to /logs
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/features/logs/widgets/logs_recording_indicator.dart`:
- Around line 60-66: The indicator is only being moved when the drawer opens
instead of being hidden and the non-bottom-nav base offset is 10px instead of
16px; update the widget that uses isDrawerOpen (from drawerProvider) and
currentRoute/hasBottomNavBar so that if isDrawerOpen is true the
LogsRecordingIndicator does not render at all (e.g., return an empty widget)
rather than just changing bottomOffset, and change the fallback base offset from
10.0 to 16.0 so bottomOffset is computed as isDrawerOpen ? (hide) :
(hasBottomNavBar ? 90.0 : 16.0); apply this change around the bottomOffset
calculation and the parent build method that renders the indicator (refer to
isDrawerOpen, drawerProvider, currentRoute, hasBottomNavBar, bottomOffset, and
the LogsRecordingIndicator build logic).
🧹 Nitpick comments (1)
lib/features/logs/widgets/logs_recording_indicator.dart (1)
25-35: Pause the pulse animation when the indicator is hidden.
The controller repeats even when the widget returnsSizedBox.shrink(), so the ticker keeps running while logging is disabled or on/logs. Consider starting/stopping the animation based on logging state to avoid unnecessary ticks.
| final isDrawerOpen = ref.watch(drawerProvider); | ||
| final hasBottomNavBar = currentRoute == '/' || | ||
| currentRoute == '/order_book' || | ||
| currentRoute == '/chat_list'; | ||
|
|
||
| final bottomOffset = isDrawerOpen ? 10.0 : (hasBottomNavBar ? 90.0 : 10.0); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant section
if [ -f "lib/features/logs/widgets/logs_recording_indicator.dart" ]; then
echo "=== File exists ==="
wc -l "lib/features/logs/widgets/logs_recording_indicator.dart"
echo -e "\n=== Lines 50-75 ==="
sed -n '50,75p' "lib/features/logs/widgets/logs_recording_indicator.dart"
else
echo "File not found"
fiRepository: MostroP2P/mobile
Length of output: 988
🏁 Script executed:
# Search for drawerProvider and other offset references in the codebase
echo "=== Searching for drawerProvider ==="
rg "drawerProvider" -A 2 -B 2
echo -e "\n=== Searching for offset patterns in logs_recording_indicator ==="
rg "bottomOffset" lib/features/logs/widgets/logs_recording_indicator.dart -B 3 -A 3
echo -e "\n=== Searching for spec or PR objectives in comments ==="
rg "spec|objective|16px|drawer.*hide" lib/features/logs/widgets/logs_recording_indicator.dart -iRepository: MostroP2P/mobile
Length of output: 3786
🏁 Script executed:
# Check the full implementation of the widget to understand the context
if [ -f "lib/features/logs/widgets/logs_recording_indicator.dart" ]; then
echo "=== Full file content ==="
cat -n "lib/features/logs/widgets/logs_recording_indicator.dart"
fiRepository: MostroP2P/mobile
Length of output: 4667
Hide the indicator when the drawer is open and adjust the base offset to 16px.
The indicator should be hidden (not just offset) when the drawer opens. Additionally, the base offset for non-bottom-nav screens should be 16px instead of 10px to match the specification.
🔧 Suggested adjustment
final isDrawerOpen = ref.watch(drawerProvider);
+ if (isDrawerOpen) {
+ return const SizedBox.shrink();
+ }
final hasBottomNavBar = currentRoute == '/' ||
currentRoute == '/order_book' ||
currentRoute == '/chat_list';
- final bottomOffset = isDrawerOpen ? 10.0 : (hasBottomNavBar ? 90.0 : 10.0);
+ final bottomOffset = hasBottomNavBar ? 90.0 : 16.0;📝 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 isDrawerOpen = ref.watch(drawerProvider); | |
| final hasBottomNavBar = currentRoute == '/' || | |
| currentRoute == '/order_book' || | |
| currentRoute == '/chat_list'; | |
| final bottomOffset = isDrawerOpen ? 10.0 : (hasBottomNavBar ? 90.0 : 10.0); | |
| final isDrawerOpen = ref.watch(drawerProvider); | |
| if (isDrawerOpen) { | |
| return const SizedBox.shrink(); | |
| } | |
| final hasBottomNavBar = currentRoute == '/' || | |
| currentRoute == '/order_book' || | |
| currentRoute == '/chat_list'; | |
| final bottomOffset = hasBottomNavBar ? 90.0 : 16.0; |
🤖 Prompt for AI Agents
In `@lib/features/logs/widgets/logs_recording_indicator.dart` around lines 60 -
66, The indicator is only being moved when the drawer opens instead of being
hidden and the non-bottom-nav base offset is 10px instead of 16px; update the
widget that uses isDrawerOpen (from drawerProvider) and
currentRoute/hasBottomNavBar so that if isDrawerOpen is true the
LogsRecordingIndicator does not render at all (e.g., return an empty widget)
rather than just changing bottomOffset, and change the fallback base offset from
10.0 to 16.0 so bottomOffset is computed as isDrawerOpen ? (hide) :
(hasBottomNavBar ? 90.0 : 16.0); apply this change around the bottomOffset
calculation and the parent build method that renders the indicator (refer to
isDrawerOpen, drawerProvider, currentRoute, hasBottomNavBar, bottomOffset, and
the LogsRecordingIndicator build logic).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep logs indicator when drawer is open. However, I going to change the fallback offset to 16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BraCR10, sounds good! I understand you prefer to keep the indicator visible when the drawer is open. Changing the fallback offset to 16px will align it with the spec for non-bottom-nav screens. 👍
🧠 Learnings used
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.974Z
Learning: In the Mostro mobile app's RelaySelector widget (lib/features/relays/widgets/relay_selector.dart), watching relaysProvider.notifier correctly triggers rebuilds because the relaysProvider itself depends on settingsProvider (line 8 in relays_provider.dart). When blacklist changes via toggleMostroRelayBlacklist(), the settingsProvider updates, causing relaysProvider to rebuild, which then notifies widgets watching the notifier. The UI correctly reflects active/inactive states in real-time through this dependency chain.
Updates the bottom offset of the logs recording indicator to ensure consistent spacing across different UI elements.
Phase 6: Recording Indicator Widget
This PR adds a floating recording indicator that shows when logging is active.
What's Included
Recording Indicator:
Note: This implementation is fully open to feedback. If you think the position, color, size, or animation should be different, please let me know and I'll adjust it accordingly.
Testing
Navigate to: Settings → Dev Tools → Logs Report
See
docs/LOGGING_IMPLEMENTATION.mdfor details.Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.