Conversation
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR changes spam-report timestamp handling from Possibly related PRs
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/features/mailbox_dashboard/data/datasource_impl/hive_spam_report_datasource_impl.dart (2)
20-23:⚠️ Potential issue | 🟡 MinorReturn type mismatch with abstract contract.
The method
deleteLastTimeDismissedSpamReported()returnsFuture<bool>here, but the abstractSpamReportDataSourcedeclaresFuture<void>. While Dart covariance allows this to compile, it creates an inconsistency with the contract.Proposed fix
`@override` -Future<bool> deleteLastTimeDismissedSpamReported() { +Future<void> deleteLastTimeDismissedSpamReported() { throw UnimplementedError(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/mailbox_dashboard/data/datasource_impl/hive_spam_report_datasource_impl.dart` around lines 20 - 23, The implementation of deleteLastTimeDismissedSpamReported() currently returns Future<bool> which mismatches the abstract SpamReportDataSource signature (Future<void>); update the method signature in hive_spam_report_datasource_impl.dart to return Future<void> and adjust the method body to not return a boolean (e.g., perform the deletion and complete/throw on error) so it conforms to SpamReportDataSource.deleteLastTimeDismissedSpamReported.
47-50:⚠️ Potential issue | 🟡 MinorReturn type mismatch with abstract contract.
Same issue:
storeLastTimeDismissedSpamReportedreturnsFuture<bool>but the abstract declaresFuture<void>.Proposed fix
`@override` -Future<bool> storeLastTimeDismissedSpamReported(DateTime lastTimeDismissedSpamReported) { +Future<void> storeLastTimeDismissedSpamReported(DateTime lastTimeDismissedSpamReported) { throw UnimplementedError(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/mailbox_dashboard/data/datasource_impl/hive_spam_report_datasource_impl.dart` around lines 47 - 50, The implementation of storeLastTimeDismissedSpamReported in HiveSpamReportDatasourceImpl has the wrong return type (Future<bool>) and must match the abstract contract (Future<void>); update the method signature to Future<void> storeLastTimeDismissedSpamReported(DateTime lastTimeDismissedSpamReported) and implement the body to persist the timestamp to Hive (or the appropriate storage used elsewhere in this class) without returning a value—allow any async operations to complete or throw errors as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@lib/features/mailbox_dashboard/data/datasource_impl/hive_spam_report_datasource_impl.dart`:
- Around line 20-23: The implementation of deleteLastTimeDismissedSpamReported()
currently returns Future<bool> which mismatches the abstract
SpamReportDataSource signature (Future<void>); update the method signature in
hive_spam_report_datasource_impl.dart to return Future<void> and adjust the
method body to not return a boolean (e.g., perform the deletion and
complete/throw on error) so it conforms to
SpamReportDataSource.deleteLastTimeDismissedSpamReported.
- Around line 47-50: The implementation of storeLastTimeDismissedSpamReported in
HiveSpamReportDatasourceImpl has the wrong return type (Future<bool>) and must
match the abstract contract (Future<void>); update the method signature to
Future<void> storeLastTimeDismissedSpamReported(DateTime
lastTimeDismissedSpamReported) and implement the body to persist the timestamp
to Hive (or the appropriate storage used elsewhere in this class) without
returning a value—allow any async operations to complete or throw errors as
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c12bfab3-18d6-4bc7-b949-58a8d1a499e0
📒 Files selected for processing (11)
lib/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/get_spam_mailbox_cached_state.dartlib/features/mailbox_dashboard/domain/usecases/get_spam_mailbox_cached_interactor.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/mailbox_dashboard/presentation/controller/spam_report_controller.dart
💤 Files with no reviewable changes (2)
- lib/features/mailbox_dashboard/domain/state/get_spam_mailbox_cached_state.dart
- lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4408. |
| final isIntervalElapsed = | ||
| elapsed.inHours > spamReportBannerDisplayIntervalInHours; |
There was a problem hiding this comment.
| final isIntervalElapsed = | |
| elapsed.inHours > spamReportBannerDisplayIntervalInHours; | |
| final isIntervalElapsed = | |
| elapsed.inHours >= spamReportBannerDisplayIntervalInHours; |
|
|
||
| final lastTime = DateTime.fromMillisecondsSinceEpoch(lastTimeDismissedMs); | ||
| final now = DateTime.now(); | ||
| final elapsed = now.difference(lastTime); |
There was a problem hiding this comment.
because elapsed can be negative (user change timezone, ...), so we should check also
if (elapsed.isNegative) return true;
|
|
||
| void _validateSpamMailboxChanged(GetSpamMailboxCachedFailure failure) { | ||
| if (failure.exception is NoUnreadSpamEmailsException) { | ||
| final currentSpamMailbox = presentationSpamMailbox.value; |
There was a problem hiding this comment.
IMO in this case of NoUnreadSpamEmailsException, presentationSpamMailbox also null?
There was a problem hiding this comment.
It might have a null value, but we don't care if it's null.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/features/mailbox_dashboard/presentation/controller/spam_report_controller_test.dart (1)
129-130: Avoid fixed 100ms sleeps in these async tests.These waits make the suite slower and can still flake under CI scheduling. Prefer a small helper that waits for the mocked stream/interaction to finish so the assertions run deterministically without the repeated timing magic number.
Also applies to: 142-143, 155-156, 170-170, 190-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/mailbox_dashboard/presentation/controller/spam_report_controller_test.dart` around lines 129 - 130, Replace the fixed Future.delayed(const Duration(milliseconds: 100)) sleeps with a deterministic wait helper: implement a small async helper (e.g., waitForStreamEvent or waitUntilIdle) that awaits the mocked stream or the specific interaction to complete using a Completer or by polling a condition (e.g., expecting mockStream.toList()/firstWhere, or checking a flag set by the mock), then use that helper in place of Future.delayed in the tests (references: Future.delayed occurrences in spam_report_controller_test.dart) so assertions run only after the actual async work completes and the tests stop relying on a magic timing number.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@test/features/mailbox_dashboard/domain/usecases/get_spam_mailbox_cached_interactor_test.dart`:
- Around line 88-139: The tests only check 1h and 25h which are valid for both
the old 12h and new 24h cooldown; update the GetSpamMailboxCachedInteractor
tests to assert the new 24-hour threshold by adding at least one mid-window case
(e.g., msAgo(13) or msAgo(23)) and an exact boundary case msAgo(24), and
optionally a small future timestamp to cover clock-skew; modify the groups that
call interactor.execute(accountId, userName) and the mock
spamReportRepository.getLastTimeDismissedSpamReportedMilliseconds() to include
these new msAgo values and expectations (use same
Right(GetSpamMailboxCachedLoading()), then assert either success via
Right(GetSpamMailboxCachedSuccess(spamMailbox)) or the appropriate exception
like SpamDismissCooldownActiveException or NoUnreadSpamEmailsException).
---
Nitpick comments:
In
`@test/features/mailbox_dashboard/presentation/controller/spam_report_controller_test.dart`:
- Around line 129-130: Replace the fixed Future.delayed(const
Duration(milliseconds: 100)) sleeps with a deterministic wait helper: implement
a small async helper (e.g., waitForStreamEvent or waitUntilIdle) that awaits the
mocked stream or the specific interaction to complete using a Completer or by
polling a condition (e.g., expecting mockStream.toList()/firstWhere, or checking
a flag set by the mock), then use that helper in place of Future.delayed in the
tests (references: Future.delayed occurrences in
spam_report_controller_test.dart) so assertions run only after the actual async
work completes and the tests stop relying on a magic timing number.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1af6eba5-3206-4c62-ae98-d8591b66f90a
📒 Files selected for processing (3)
lib/features/mailbox_dashboard/domain/usecases/get_spam_mailbox_cached_interactor.darttest/features/mailbox_dashboard/domain/usecases/get_spam_mailbox_cached_interactor_test.darttest/features/mailbox_dashboard/presentation/controller/spam_report_controller_test.dart
test/features/mailbox_dashboard/domain/usecases/get_spam_mailbox_cached_interactor_test.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/features/mailbox_dashboard/domain/usecases/get_spam_mailbox_cached_interactor_test.dart (1)
118-171: Consider adding clock-skew tests for completeness.The interactor has logic for handling future timestamps (negative elapsed time):
if (elapsed.isNegative) { if (elapsed.abs() < const Duration(days: 1)) return false; return true; }This handles cases where device clock was adjusted forward then back. No tests currently cover these paths. Consider adding:
- Future timestamp < 1 day → should not show banner (cooldown active).
- Future timestamp >= 1 day → should show banner (likely clock reset).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/mailbox_dashboard/domain/usecases/get_spam_mailbox_cached_interactor_test.dart` around lines 118 - 171, Add two tests in get_spam_mailbox_cached_interactor_test.dart mirroring the existing "cooldown expired" group that cover future timestamps: call spamReportRepository.getLastTimeDismissedSpamReportedMilliseconds() with msAgo(-12) (future < 1 day) and assert interactor.execute(accountId, userName) emits loading then leftWithException<NoUnreadSpamEmailsException>() (cooldown active), and call it with msAgo(-25) (future >= 1 day) and assert it emits loading then Right(GetSpamMailboxCachedSuccess(spamMailbox)) (cooldown expired). Use the same helper msAgo, the same spamMailbox setups, and the same expectations structure so tests reference interactor.execute, spamReportRepository.getLastTimeDismissedSpamReportedMilliseconds, and GetSpamMailboxCachedSuccess/NoUnreadSpamEmailsException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@test/features/mailbox_dashboard/domain/usecases/get_spam_mailbox_cached_interactor_test.dart`:
- Around line 118-171: Add two tests in
get_spam_mailbox_cached_interactor_test.dart mirroring the existing "cooldown
expired" group that cover future timestamps: call
spamReportRepository.getLastTimeDismissedSpamReportedMilliseconds() with
msAgo(-12) (future < 1 day) and assert interactor.execute(accountId, userName)
emits loading then leftWithException<NoUnreadSpamEmailsException>() (cooldown
active), and call it with msAgo(-25) (future >= 1 day) and assert it emits
loading then Right(GetSpamMailboxCachedSuccess(spamMailbox)) (cooldown expired).
Use the same helper msAgo, the same spamMailbox setups, and the same
expectations structure so tests reference interactor.execute,
spamReportRepository.getLastTimeDismissedSpamReportedMilliseconds, and
GetSpamMailboxCachedSuccess/NoUnreadSpamEmailsException.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32344017-c052-424e-a98b-89e7973d0233
📒 Files selected for processing (2)
test/features/mailbox_dashboard/domain/usecases/get_spam_mailbox_cached_interactor_test.darttest/features/mailbox_dashboard/presentation/controller/spam_report_controller_test.dart
✅ Files skipped from review due to trivial changes (1)
- test/features/mailbox_dashboard/presentation/controller/spam_report_controller_test.dart
Fixed |
2 participants
Issue
#4385
Resolved
Screen.Recording.2026-03-25.at.15.23.26.mov
Summary by CodeRabbit