lib/features/settings/settings_notifier.dart (1)

208-213: Guard sessionExpirationHours against invalid values before persisting.

This setter accepts any integer. Invalid values (especially negatives) can break retention semantics in downstream cutoff calculations. Consider restricting to known presets plus 0 (“never”).

Proposed hardening

   Future<void> updateSessionExpirationHours(int newValue) async {
+    const allowedValues = <int>{0, 168, 720, 2160, 4320, 8760};
+    if (!allowedValues.contains(newValue)) {
+      logger.w('Ignoring invalid session expiration value: $newValue');
+      return;
+    }
+
     final oldValue = state.sessionExpirationHours;
+    if (oldValue == newValue) return;
+
     state = state.copyWith(sessionExpirationHours: newValue);
     await _saveToPrefs();
     logger.i('Session expiration changed: $oldValue → $newValue hours');
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/features/settings/settings_notifier.dart` around lines 208 - 213, The
updateSessionExpirationHours setter currently accepts any int; validate the
incoming newValue in updateSessionExpirationHours before mutating state and
calling _saveToPrefs: ensure newValue is one of the allowed presets (e.g., 0 for
“never” and the defined retention options) and reject or clamp negatives/unknown
values (log/warn and return early or set to a safe default). Use
state.sessionExpirationHours and the same copyWith/state update pattern only
after validation, and keep the existing logger.i call to record the effective
change.

lib/shared/notifiers/session_notifier.dart (1)

90-93: Avoid running periodic no-op cleanup when retention is set to “Never”.

_cleanup() now exits early for _isForever, but the periodic timer still triggers. Consider skipping scheduling in that mode and re-scheduling when settings change.

♻️ Suggested refactor

 void _scheduleCleanup() {
   _cleanupTimer?.cancel();
+  if (_isForever) return;
   _cleanupTimer = Timer.periodic(
     const Duration(minutes: Config.cleanupIntervalMinutes),
     (timer) => _cleanup(),
   );
 }

 void updateSettings(Settings settings) {
   _settings = settings.copyWith();
+  _scheduleCleanup();
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shared/notifiers/session_notifier.dart` around lines 90 - 93, The
periodic cleanup timer is still scheduled even when _cleanup() immediately
returns for _isForever; update the scheduling logic to skip creating the timer
when _isForever is true and to cancel any existing timer when settings flip to
forever. Locate the timer creation/management code (e.g., _startCleanupTimer,
_schedulePeriodicCleanup or the constructor where a Timer/Timer.periodic is
created) and add a guard that only schedules the periodic Timer when
!_isForever; also ensure you cancel and null out the timer (e.g.,
_cleanupTimer?.cancel(); _cleanupTimer = null) when _isForever becomes true and
reschedule the timer when settings change back to finite retention.

lib/shared/widgets/trade_history_selector.dart (1)

28-29: Avoid hardcoding 720 for dropdown fallback.

At Line 29, the literal 720 can drift from Config.sessionExpirationHours. Use a config-derived default (and guard that it exists in _optionKeys) to keep UI behavior aligned with runtime config.

♻️ Suggested refactor

-    final displayValue =
-        _optionKeys.containsKey(effectiveValue) ? effectiveValue : 720;
+    final defaultValue = _optionKeys.containsKey(Config.sessionExpirationHours)
+        ? Config.sessionExpirationHours
+        : _optionKeys.keys.first;
+    final displayValue =
+        _optionKeys.containsKey(effectiveValue) ? effectiveValue : defaultValue;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shared/widgets/trade_history_selector.dart` around lines 28 - 29, The
dropdown fallback currently hardcodes 720 in the assignment to displayValue
which can drift from Config.sessionExpirationHours; update the logic that
computes displayValue (the variable named displayValue in
trade_history_selector.dart) to derive the default from
Config.sessionExpirationHours and only use it if that value exists in
_optionKeys (otherwise fall back to a safe existing key from _optionKeys), i.e.
replace the literal 720 with a config-derived default and a membership guard
against _optionKeys so the shown value always matches available options.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

lib/shared/notifiers/session_notifier.dart (2)

129-134: ⚠️ Potential issue | 🟡 Minor

Same null-safety concern in _cleanup().

Lines 131 and 132 force-unwrap session.orderId! without null guards, consistent with the unresolved concern in init().

🛡️ Proposed fix

   for (final session in expiredSessions) {
+    if (session.orderId == null) continue;
     if (session.startTime.isBefore(cutoff)) {
       await _storage.deleteSession(session.orderId!);
       _sessions.remove(session.orderId!);
       await _cleanupSessionData(session);
     }
   }

,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shared/notifiers/session_notifier.dart` around lines 129 - 134, The loop
in _cleanup() force-unwraps session.orderId (used in _storage.deleteSession and
_sessions.remove and passed to _cleanupSessionData) which can be null; guard
against this by capturing session.orderId into a local (e.g., orderId), check
for null and skip that session (or handle it explicitly) before calling
_storage.deleteSession(orderId), _sessions.remove(orderId), and
_cleanupSessionData(session), mirroring the null-safety fix you applied in
init().

---

85-100: ⚠️ Potential issue | 🟡 Minor

Null-safety guards still missing in init().

Lines 87, 94, 96, and 97 force-unwrap session.orderId! without first checking for null. While _cleanupSessionData now guards against null, the init() loops do not, which can cause runtime exceptions if a malformed session exists in storage.

🛡️ Proposed defensive approach

   if (_isForever) {
     for (final session in allSessions) {
+      if (session.orderId == null) continue;
       _sessions[session.orderId!] = session;
     }
   } else {
     final cutoff = DateTime.now()
         .subtract(Duration(hours: _expirationHours));
     for (final session in allSessions) {
+      if (session.orderId == null) continue;
       if (session.startTime.isAfter(cutoff)) {
         _sessions[session.orderId!] = session;
       } else {
         await _storage.deleteSession(session.orderId!);
         _sessions.remove(session.orderId!);
         await _cleanupSessionData(session);
       }
     }
   }

,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shared/notifiers/session_notifier.dart` around lines 85 - 100, The init()
loops currently force-unwrap session.orderId! (in the _isForever branch and the
else branch) which can throw for malformed sessions; update both loops in init()
to null-check session.orderId before using it (skip entries where orderId is
null), only assign to _sessions[orderId] and call
_storage.deleteSession(orderId) / _cleanupSessionData(session) when orderId !=
null, and ensure you do not assume orderId downstream when interacting with the
_sessions map or calling _storage.deleteSession and _cleanupSessionData.

lib/shared/widgets/trade_history_selector.dart (1)

8-83: Well-structured widget with proper value resolution and localization.

The fallback logic correctly handles null preferences, invalid stored values, and uses S.of(context)! for all user-facing strings. One minor optimization:

The Icon widget (lines 56-60) has all constant parameters and could be marked const:

♻️ Suggested improvement

-                  Icon(
-                    Icons.schedule,
-                    color: AppTheme.mostroGreen,
-                    size: 20,
-                  ),
+                  const Icon(
+                    Icons.schedule,
+                    color: AppTheme.mostroGreen,
+                    size: 20,
+                  ),

,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shared/widgets/trade_history_selector.dart` around lines 8 - 83, Summary:
Make the Icon in TradeHistorySelector build constant to improve performance.
Change the non-const Icon(...) instance inside the DropdownMenuItem builder (the
Icon using Icons.schedule, AppTheme.mostroGreen, size: 20) to a const Icon so
the widget becomes compile-time constant; ensure the surrounding Row children
that include const SizedBox and Text remain valid (you may need to adjust const
placement on the Text if localization prevents const). Update the Icon
instantiation in the _optionKeys.map builder accordingly.

lib/shared/notifiers/session_notifier.dart (1)

63-66: Type cast bypasses the repository abstraction.

The cast as NotificationsStorage couples this code to the concrete implementation rather than the NotificationsRepository interface. If the provider's backing implementation changes, this will fail at runtime.

Consider adding deleteWhere to the NotificationsRepository interface, or exposing a dedicated method like deleteByOrderId(String orderId) that encapsulates this logic.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shared/notifiers/session_notifier.dart` around lines 63 - 66, The code in
session_notifier.dart reads notificationsRepositoryProvider and force-casts it
to NotificationsStorage (notificationsStore = ref.read(... ) as
NotificationsStorage), which breaks the NotificationsRepository abstraction and
can fail if implementation changes; update the NotificationsRepository interface
to include the needed deletion operation (e.g., add deleteWhere or
deleteByOrderId(String orderId)) and replace the cast with the interface type
(NotificationsRepository notificationsRepo) then call the new repository method
(e.g., notificationsRepo.deleteByOrderId(orderId) or
notificationsRepo.deleteWhere(predicate)) instead of relying on the concrete
NotificationsStorage methods.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

lib/shared/notifiers/session_notifier.dart (2)

131-140: ⚠️ Potential issue | 🟡 Minor

Same null-safety concern in _cleanup().

Line 133 and 134 force-unwrap session.orderId! without a null check. Apply the same guard pattern here for consistency with the init() fix.

🛡️ Proposed fix

   for (final session in expiredSessions) {
+    if (session.orderId == null) continue;
     if (session.startTime.isBefore(cutoff)) {
       await _storage.deleteSession(session.orderId!);
       _sessions.remove(session.orderId!);
       try {
         await _cleanupSessionData(session);
       } catch (e) {
         logger.e('Failed to cleanup data for session ${session.orderId}: $e');
       }
     }
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shared/notifiers/session_notifier.dart` around lines 131 - 140, In
_cleanup() avoid force-unwrapping session.orderId; mirror the guard used in
init(): check if session.orderId is null before calling _storage.deleteSession,
_sessions.remove and _cleanupSessionData (e.g., if (session.orderId == null)
continue or log and continue), and only use the non-null orderId afterwards to
prevent null-safety violations when invoking _storage.deleteSession,
_sessions.remove, and _cleanupSessionData for that session.

---

83-102: ⚠️ Potential issue | 🟡 Minor

Apply null-safety guard for session.orderId in init() loops.

Lines 85, 92, 94, and 95 force-unwrap session.orderId! without first checking for null. If a session has a null orderId, this will throw a runtime exception before the guarded _cleanupSessionData is reached. Add a null check at the start of each loop iteration.

🛡️ Proposed defensive approach

 Future<void> init() async {
   final allSessions = await _storage.getAllSessions();
   if (_isForever) {
     for (final session in allSessions) {
+      if (session.orderId == null) continue;
       _sessions[session.orderId!] = session;
     }
   } else {
     final cutoff = DateTime.now()
         .subtract(Duration(hours: _expirationHours));
     for (final session in allSessions) {
+      if (session.orderId == null) continue;
       if (session.startTime.isAfter(cutoff)) {
         _sessions[session.orderId!] = session;
       } else {
         await _storage.deleteSession(session.orderId!);
         _sessions.remove(session.orderId!);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shared/notifiers/session_notifier.dart` around lines 83 - 102, The init()
loop uses session.orderId! in multiple places and can throw if orderId is null;
add a null-safety guard at the start of each iteration (inside both branches
that iterate allSessions) by reading final id = session.orderId and skipping the
iteration if id == null (continue), then use id instead of session.orderId! when
assigning to _sessions, calling _storage.deleteSession(id), and in the
logger/_cleanupSessionData calls; this ensures _cleanupSessionData and the
delete path only run for sessions with a non-null orderId.

lib/shared/notifiers/session_notifier.dart (1)

56-79: Consider per-operation error handling for resilience.

While the callers wrap _cleanupSessionData in try/catch, a failure in any individual operation (e.g., eventStore.deleteWhere) will prevent subsequent cleanup steps from executing. For best-effort cleanup, wrapping each storage operation individually would allow partial success.

That said, the current approach does log failures and continue processing other sessions, which may be acceptable if cleanup failures are rare.

♻️ Optional: per-operation try/catch for maximum resilience

 Future<void> _cleanupSessionData(Session session) async {
   final orderId = session.orderId;
   if (orderId == null) {
     logger.w('Skipping cleanup for session with null orderId');
     return;
   }
   final eventStore = ref.read(eventStorageProvider);
   final mostroStore = ref.read(mostroStorageProvider);
   final notificationsRepo = ref.read(notificationsRepositoryProvider);

-  await eventStore.deleteWhere(Filter.equals('order_id', orderId));
+  try {
+    await eventStore.deleteWhere(Filter.equals('order_id', orderId));
+  } catch (e) {
+    logger.e('Failed to delete events for order $orderId: $e');
+  }
   if (session.disputeId != null) {
-    await eventStore.deleteWhere(
-        Filter.equals('dispute_id', session.disputeId));
+    try {
+      await eventStore.deleteWhere(
+          Filter.equals('dispute_id', session.disputeId));
+    } catch (e) {
+      logger.e('Failed to delete dispute events for ${session.disputeId}: $e');
+    }
   }
-  await mostroStore.deleteAllMessagesByOrderId(orderId);
-  await notificationsRepo.deleteByOrderId(orderId);
+  try {
+    await mostroStore.deleteAllMessagesByOrderId(orderId);
+  } catch (e) {
+    logger.e('Failed to delete mostro messages for order $orderId: $e');
+  }
+  try {
+    await notificationsRepo.deleteByOrderId(orderId);
+  } catch (e) {
+    logger.e('Failed to delete notifications for order $orderId: $e');
+  }

   final prefs = await SharedPreferences.getInstance();
-  await prefs.remove('chat_last_read_$orderId');
+  try {
+    await prefs.remove('chat_last_read_$orderId');
+  } catch (e) {
+    logger.e('Failed to remove chat_last_read pref for order $orderId: $e');
+  }
   if (session.disputeId != null) {
-    await prefs.remove('dispute_last_read_${session.disputeId}');
+    try {
+      await prefs.remove('dispute_last_read_${session.disputeId}');
+    } catch (e) {
+      logger.e('Failed to remove dispute_last_read pref: $e');
+    }
   }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shared/notifiers/session_notifier.dart` around lines 56 - 79, The
_cleanupSessionData function performs multiple storage/removal operations
atomically so a failure in one (e.g., eventStore.deleteWhere,
mostroStore.deleteAllMessagesByOrderId, notificationsRepo.deleteByOrderId,
prefs.remove) aborts the rest; change it to perform each operation in its own
try/catch so failures are logged per-operation but do not prevent subsequent
cleanup steps, i.e., wrap each call (including both eventStore.deleteWhere
calls, mostroStore.deleteAllMessagesByOrderId,
notificationsRepo.deleteByOrderId, and each prefs.remove) in small try/catch
blocks that log the error via logger.e/w and continue.

lib/shared/notifiers/session_notifier.dart (1)

14-14: Use the model barrel for the new Order dependency.

This adds another direct model-file import in a codepath that should be coming through the barrel instead. Please pull Order from package:mostro_mobile/data/models.dart here.

♻️ Minimal import change

-import 'package:mostro_mobile/data/models/order.dart';
+import 'package:mostro_mobile/data/models.dart' show Order;

As per coding guidelines **/{lib,test}/**/*.dart: Import models through data/models.dart instead of importing individual model files.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shared/notifiers/session_notifier.dart` at line 14, The file
session_notifier.dart currently imports the Order model directly; replace the
direct import of package:mostro_mobile/data/models/order.dart with the model
barrel package:mostro_mobile/data/models.dart so Order is resolved via the
shared models barrel (keep other imports intact and ensure no name collisions
with existing symbols in SessionNotifier).