TF-4385 Fix spam banner show it once per day#4397
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR refactors spam-report handling and preference clearing across the mailbox/dashboard features. It changes dismissal timestamp types from DateTime to int (milliseconds), removes delete-last-time-dismissed functionality and its interactor/state, renames an exception to InvalidSpamDismissTimestampException and adds NoUnreadSpamEmailsException, increases the spam-banner interval from 12 to 24 hours, extracts banner rendering into DisplaySpamBannerExtension, updates controllers and interactors to use explicit spam-report state or the new last-time-dismissed interactor, and injects PreferencesSettingManager into CachingManager to clear preferences during cache cleanup. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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: 2
🧹 Nitpick comments (3)
lib/features/mailbox_dashboard/data/datasource/spam_report_datasource.dart (1)
11-13: Consider API symmetry for maintainability.The interface has an asymmetry:
storeLastTimeDismissedSpamReportedacceptsDateTimewhilegetLastTimeDismissedSpamReportedreturnsint(milliseconds). While this works for the current use case (interval calculations), consider whether both methods should use the same type for consistency.If the
intreturn type is intentional for simpler timestamp comparisons, this is acceptable but worth documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/mailbox_dashboard/data/datasource/spam_report_datasource.dart` around lines 11 - 13, The interface is asymmetric: storeLastTimeDismissedSpamReported(DateTime) accepts a DateTime while getLastTimeDismissedSpamReported() returns an int (milliseconds); update the API for symmetry by choosing one canonical type and making both methods use it (either change storeLastTimeDismissedSpamReported to accept an int milliseconds OR change getLastTimeDismissedSpamReported to return a DateTime), and if you intentionally keep the int for easier interval math, add a brief doc comment to getLastTimeDismissedSpamReported explaining it returns epoch milliseconds for comparison.lib/features/mailbox_dashboard/presentation/controller/spam_report_controller.dart (2)
135-153: Banner visibility may become stale when email is opened/closed.
_updateSpamBannerVisibility()readsdashboardController.isEmailOpened(line 145), which is a non-reactive getter. This method is only called during specific events (setSpamPresentationMailbox,_loadSpamReportConfigSuccess), but NOT whendashboardRoutechanges.If a user opens an email after the banner is displayed,
isSpamBannerVisiblewon't update to hide the banner because nothing triggers_updateSpamBannerVisibility()when the route changes.Consider listening to
dashboardRoutechanges or calling_updateSpamBannerVisibility()when the dashboard route transitions.♻️ Potential fix: add route change listener
In
onInit(), add a listener for route changes:`@override` void onInit() { super.onInit(); _appLifecycleListener ??= AppLifecycleListener( onResume: () { if (_spamReportLoaderStatus == LoaderStatus.loading) { return; } getSpamReportStateAction(); }, ); // Add route change listener final dashboardController = getBinding<MailboxDashBoardController>(); if (dashboardController != null) { ever(dashboardController.dashboardRoute, (_) => _updateSpamBannerVisibility()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/mailbox_dashboard/presentation/controller/spam_report_controller.dart` around lines 135 - 153, _updateSpamBannerVisibility reads the non-reactive getter dashboardController.isEmailOpened but is not invoked when the dashboard route changes, causing the banner to stay visible after opening an email; fix by subscribing to route changes in onInit: obtain the MailboxDashBoardController via getBinding<MailboxDashBoardController>() and register a listener (e.g., ever(dashboardController.dashboardRoute, (_) => _updateSpamBannerVisibility())) so _updateSpamBannerVisibility() runs on route transitions and hides/shows the spam banner correctly.
155-161: Clarify the timing of "last dismissed" timestamp storage.
_storeLastTimeDismissedSpamReportedAction()is called when the banner becomes visible (not when the user dismisses it). While this aligns with the PR objective ("show it once per day" - recording when the banner was shown), the namingLastTimeDismissedis misleading.Consider renaming to
_storeLastTimeSpamBannerShownAction()or adding a comment explaining that this records when the banner was displayed to enforce the once-per-day display policy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/mailbox_dashboard/presentation/controller/spam_report_controller.dart` around lines 155 - 161, The method name _storeLastTimeDismissedSpamReportedAction() is misleading because it is invoked when the spam banner becomes visible (not when dismissed); rename the method to _storeLastTimeSpamBannerShownAction() (and update all its references, e.g., the call in _handleGetSpamCachedSuccess and the method declaration) or alternatively add a clarifying comment at the method declaration explaining it records when the banner was shown to enforce the once-per-day display policy; ensure any tests or usages referencing the old name are updated accordingly.
🤖 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/features/mailbox_dashboard/presentation/extensions/display_spam_banner_extension.dart`:
- Around line 60-73: In _dismissSpamBanner the code clears the spam presentation
mailbox before reading it, so spamReportController.presentationSpamMailbox.value
will always be null and emitFailure is always triggered; fix by first reading
and storing spamReportController.presentationSpamMailbox.value into a local
(e.g. spamMailbox) before calling
spamReportController.setSpamPresentationMailbox(null), then proceed to check
spamMailbox == null and call markAsReadMailbox or emitFailure as originally
intended (update references to spamReportController.setSpamPresentationMailbox,
spamReportController.presentationSpamMailbox.value, emitFailure,
MarkAsEmailReadFailure, NotFoundSpamMailboxException, and markAsReadMailbox
accordingly).
- Around line 46-58: In _openSpamMailbox the code clears the cached value by
calling spamReportController.setSpamPresentationMailbox(null) before reading
spamReportController.presentationSpamMailbox.value, so the read is always null;
fix by first reading PresentationMailbox? current =
spamReportController.presentationSpamMailbox.value, then call
spamReportController.setSpamPresentationMailbox(null), then use current (fall
back to mapMailboxById[spamMailboxId] if current is null) and finally call
openMailboxAction(spamMailbox) as before.
---
Nitpick comments:
In `@lib/features/mailbox_dashboard/data/datasource/spam_report_datasource.dart`:
- Around line 11-13: The interface is asymmetric:
storeLastTimeDismissedSpamReported(DateTime) accepts a DateTime while
getLastTimeDismissedSpamReported() returns an int (milliseconds); update the API
for symmetry by choosing one canonical type and making both methods use it
(either change storeLastTimeDismissedSpamReported to accept an int milliseconds
OR change getLastTimeDismissedSpamReported to return a DateTime), and if you
intentionally keep the int for easier interval math, add a brief doc comment to
getLastTimeDismissedSpamReported explaining it returns epoch milliseconds for
comparison.
In
`@lib/features/mailbox_dashboard/presentation/controller/spam_report_controller.dart`:
- Around line 135-153: _updateSpamBannerVisibility reads the non-reactive getter
dashboardController.isEmailOpened but is not invoked when the dashboard route
changes, causing the banner to stay visible after opening an email; fix by
subscribing to route changes in onInit: obtain the MailboxDashBoardController
via getBinding<MailboxDashBoardController>() and register a listener (e.g.,
ever(dashboardController.dashboardRoute, (_) => _updateSpamBannerVisibility()))
so _updateSpamBannerVisibility() runs on route transitions and hides/shows the
spam banner correctly.
- Around line 155-161: The method name
_storeLastTimeDismissedSpamReportedAction() is misleading because it is invoked
when the spam banner becomes visible (not when dismissed); rename the method to
_storeLastTimeSpamBannerShownAction() (and update all its references, e.g., the
call in _handleGetSpamCachedSuccess and the method declaration) or alternatively
add a clarifying comment at the method declaration explaining it records when
the banner was shown to enforce the once-per-day display policy; ensure any
tests or usages referencing the old name are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3247576d-d1b9-41e8-b5d9-d59810eb515d
📒 Files selected for processing (25)
lib/features/caching/caching_manager.dartlib/features/mailbox/presentation/mailbox_controller.dartlib/features/mailbox_dashboard/data/datasource/spam_report_datasource.dartlib/features/mailbox_dashboard/data/datasource_impl/hive_spam_report_datasource_impl.dartlib/features/mailbox_dashboard/data/datasource_impl/local_spam_report_datasource_impl.dartlib/features/mailbox_dashboard/data/repository/spam_report_repository_impl.dartlib/features/mailbox_dashboard/domain/exceptions/spam_report_exception.dartlib/features/mailbox_dashboard/domain/repository/spam_report_repository.dartlib/features/mailbox_dashboard/domain/state/delete_last_time_dismissed_spam_reported_state.dartlib/features/mailbox_dashboard/domain/state/get_spam_mailbox_cached_state.dartlib/features/mailbox_dashboard/domain/state/store_spam_report_state.dartlib/features/mailbox_dashboard/domain/usecases/delete_last_time_dismissed_spam_reported_interactor.dartlib/features/mailbox_dashboard/domain/usecases/get_spam_mailbox_cached_interactor.dartlib/features/mailbox_dashboard/domain/usecases/store_last_time_dismissed_spam_reported_interactor.dartlib/features/mailbox_dashboard/domain/usecases/store_spam_report_state_interactor.dartlib/features/mailbox_dashboard/presentation/bindings/mailbox_dashboard_bindings.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/mailbox_dashboard/presentation/controller/spam_report_controller.dartlib/features/mailbox_dashboard/presentation/extensions/display_spam_banner_extension.dartlib/features/mailbox_dashboard/presentation/mailbox_dashboard_view_web.dartlib/features/manage_account/data/local/preferences_setting_manager.dartlib/features/search/mailbox/presentation/search_mailbox_controller.dartlib/features/thread/presentation/thread_view.dartlib/main/bindings/local/local_bindings.darttest/features/mailbox_dashboard/presentation/view/mailbox_dashboard_view_widget_test.dart
💤 Files with no reviewable changes (2)
- lib/features/mailbox_dashboard/domain/state/delete_last_time_dismissed_spam_reported_state.dart
- lib/features/mailbox_dashboard/domain/usecases/delete_last_time_dismissed_spam_reported_interactor.dart
lib/features/mailbox_dashboard/presentation/extensions/display_spam_banner_extension.dart
Show resolved
Hide resolved
lib/features/mailbox_dashboard/presentation/extensions/display_spam_banner_extension.dart
Show resolved
Hide resolved
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4397. |
|
A more optimal solution to this problem. Implement at #4408 |
1 participant
Issue
#4385
Resolved
Screen.Recording.2026-03-23.at.11.37.03.mov
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests