RCF-1256 In the auto logged out, while in the offline mode the warning pop up time start after 5 minutes #619
Conversation
…time start after 5 minutes Signed-off-by: Rakshithasai123 <[email protected]>
WalkthroughAdds periodic network checking at app startup, a Timer-based auto-check in ConnectivityProvider with proper disposal, and connectivity-aware guards/listeners in InactivityTracker to prevent or cancel inactivity timers and show warnings when offline. Changes
Sequence Diagram(s)sequenceDiagram
participant App as BuildApp (main.dart)
participant Connectivity as ConnectivityProvider
participant Timer as Timer
participant Network as Network Check
participant Inactivity as InactivityTracker
App->>Connectivity: startAutoNetworkCheck()
activate Connectivity
Note over Connectivity: create Timer.periodic(3s)
Connectivity->>Timer: schedule periodic callback
deactivate Connectivity
loop Every 3s
Timer->>Network: checkNetworkConnection()
Network-->>Connectivity: connection status
Connectivity->>Inactivity: notify listeners (online/offline)
alt offline
Inactivity->>Inactivity: cancel/start guards for inactivity timer
Inactivity->>Inactivity: schedule microtask to show warning dialog (if mounted)
else online
Inactivity->>Inactivity: allow/start inactivity timer if needed
end
end
Note over Connectivity: on dispose
Connectivity->>Timer: cancel()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
Signed-off-by: Rakshithasai123 <[email protected]>
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/main.dart(1 hunks)lib/provider/connectivity_provider.dart(2 hunks)lib/utils/inactivity_tracker.dart(3 hunks)
🔇 Additional comments (3)
lib/provider/connectivity_provider.dart (1)
35-39: Good disposal pattern.The timer is properly cancelled in
dispose()to prevent memory leaks.lib/main.dart (1)
117-118: Network monitoring initialization looks good.The connectivity provider is correctly accessed with
listen: falseand network checks are started during app initialization.lib/utils/inactivity_tracker.dart (1)
110-120: Review comment premise is incorrect; design intentionally shows warning when offline.The existing code intentionally triggers
_showWarningDialog()when the user goes offline (lines 49–54), making it part of the offline notification system. The_showWarningDialog()method itself properly handles login and sync state checks. While there is an inconsistency (the lifecycle-based call at line 197 doesn't verify current connectivity state before showing the dialog), adding a connectivity check inside the method would contradict the current design where offline status triggers the warning. If the intent is to prevent warnings when offline, that requires design reconsideration across all call sites, not just within the method.Likely an incorrect or invalid review comment.
| _autoCheckTimer = Timer.periodic(const Duration(seconds: 3), (timer) { | ||
| checkNetworkConnection(); | ||
| }); |
There was a problem hiding this comment.
Consider increasing the polling interval to reduce resource consumption.
A 3-second network check interval may be excessive and could impact battery life and performance, especially on mobile devices. Consider increasing it to 10-15 seconds or using connectivity change listeners instead of polling.
Additionally, checkNetworkConnection() is async but not awaited in the timer callback. If a network check takes longer than 3 seconds, multiple concurrent calls could stack up, potentially overwhelming the network service.
Apply this diff to increase the interval and await the network check:
- _autoCheckTimer = Timer.periodic(const Duration(seconds: 3), (timer) {
- checkNetworkConnection();
+ _autoCheckTimer = Timer.periodic(const Duration(seconds: 10), (timer) async {
+ await checkNetworkConnection();
});📝 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.
| _autoCheckTimer = Timer.periodic(const Duration(seconds: 3), (timer) { | |
| checkNetworkConnection(); | |
| }); | |
| _autoCheckTimer = Timer.periodic(const Duration(seconds: 10), (timer) async { | |
| await checkNetworkConnection(); | |
| }); |
🤖 Prompt for AI Agents
In lib/provider/connectivity_provider.dart around lines 30 to 32, the timer
currently polls every 3 seconds and calls the async checkNetworkConnection()
without awaiting it; increase the polling interval to a more conservative value
(e.g., 15 seconds) and await the async network check inside the timer callback
to prevent overlapping calls (or replace polling with connectivity change
listeners later); update the Timer.periodic to use Duration(seconds: 15) and
make the callback async so it awaits checkNetworkConnection() to avoid
concurrent executions.
| final connectivity = Provider.of<ConnectivityProvider>(context, listen: false); | ||
|
|
||
| // 🔥 When network status changes, offline should trigger popup | ||
| connectivity.addListener(() { | ||
| if (!connectivity.isConnected) { | ||
| Future.microtask(() { | ||
| if (mounted) _showWarningDialog(); | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Memory leak: Connectivity listener is never removed.
The connectivity listener is added in initState() but never removed in dispose(). This will cause a memory leak as the listener will continue to exist even after the widget is disposed.
Apply this diff to remove the listener in dispose():
+ ConnectivityProvider? _connectivityProvider;
+
@override
void initState() {
super.initState();
globalProvider = Provider.of<GlobalProvider>(context, listen: false);
WidgetsBinding.instance.addObserver(this);
- final connectivity = Provider.of<ConnectivityProvider>(context, listen: false);
+ _connectivityProvider = Provider.of<ConnectivityProvider>(context, listen: false);
// 🔥 When network status changes, offline should trigger popup
- connectivity.addListener(() {
- if (!connectivity.isConnected) {
+ _connectivityProvider!.addListener(_onConnectivityChanged);
+
+ _startInactivityTimer();
+ }
+
+ void _onConnectivityChanged() {
+ if (_connectivityProvider != null && !_connectivityProvider!.isConnected) {
Future.microtask(() {
if (mounted) _showWarningDialog();
});
- }
- });
-
- _startInactivityTimer();
+ }
}
@override
void dispose() {
+ _connectivityProvider?.removeListener(_onConnectivityChanged);
_countdown.dispose();
_cancelAllTimers();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/utils/inactivity_tracker.dart around lines 46 to 55, the connectivity
listener added in initState is never removed; store the ConnectivityProvider
instance and the listener callback as fields on the State, add the listener
using that named callback, and call
connectivity.removeListener(theStoredCallback) in dispose() to unregister it
before calling super.dispose(); ensure the stored callback and provider are
non-null when removing to avoid runtime errors.
| void _startInactivityTimer() { | ||
| final connectivity = Provider.of<ConnectivityProvider>(context, listen: false); | ||
| if (!connectivity.isConnected) { | ||
| Future.microtask(() { | ||
| if (mounted) _showWarningDialog(); | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (!widget.isUserLoggedIn) return; | ||
|
|
||
| if (!connectivity.isConnected) { | ||
| Future.microtask(() { | ||
| if (mounted) _showWarningDialog(); | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Duplicate connectivity checks and unclear business logic.
There are two identical connectivity check blocks in _startInactivityTimer() (lines 79-85 and 89-94), with only the user login check (line 87) between them. This is clearly unintentional duplication.
Additionally, the PR title states "warning pop up time start after 5 minutes" when offline, but this implementation shows the warning dialog immediately when offline. This contradicts the expected behavior.
Apply this diff to remove the duplicate and clarify the intent:
void _startInactivityTimer() {
final connectivity = Provider.of<ConnectivityProvider>(context, listen: false);
+ if (!widget.isUserLoggedIn) return;
+
if (!connectivity.isConnected) {
- Future.microtask(() {
- if (mounted) _showWarningDialog();
- });
- return;
- }
-
- if (!widget.isUserLoggedIn) return;
-
- if (!connectivity.isConnected) {
- Future.microtask(() {
- if (mounted) _showWarningDialog();
- });
+ // TODO: Should this show warning immediately or wait 5 minutes as per PR title?
+ // Current implementation shows immediately
return;
}
_inactivityTimer = Timer(widget.timeout, _showWarningDialog);Please clarify the expected behavior: should the warning appear immediately when going offline, or should there be a 5-minute grace period as suggested by the PR title "[RCF-1256] In the auto logged out, while in the offline mode the warning pop up time start after 5 minutes"?
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/utils/inactivity_tracker.dart around lines 78 to 94, remove the duplicate
connectivity check block (the second identical if (!connectivity.isConnected) {
... } block) so the connectivity verification appears only once; then implement
the intended offline behavior: either 1) show the warning dialog immediately
when offline (leave the current immediate Future.microtask call in place) or 2)
start a 5‑minute delayed timer when offline and only show the warning after that
delay (use a Timer(Duration(minutes:5), ...) and cancel it if connectivity is
restored or the user logs out). Update the logic to check widget.isUserLoggedIn
before scheduling the offline timer, ensure the timer is canceled on dispose and
on connectivity change, and ask the reviewer to confirm whether the expected
behavior is immediate warning or a 5‑minute grace period.
ase-101
changed the title
1 participant
RCF-1256
Summary by CodeRabbit
New Features
Bug Fixes