TW-4484 Display X-TWP-Message warnings when opening a mail#4639
Open
chibenwa wants to merge 9 commits into
Open
TW-4484 Display X-TWP-Message warnings when opening a mail#4639chibenwa wants to merge 9 commits into
chibenwa wants to merge 9 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds support for backend-provided Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Member
Author
|
From what I understand this work massively use GetIT Is this reasonnable to try to migrate this to use Riverpod? (in a separate commit than other fies...) Cc @hoangdat ? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
model/test/extensions/list_email_header_extension_test.dart (1)
77-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider a test for duplicate identical
X-TWP-Messageheaders.The multi-warning case uses distinct values, so it never exercises whether two identical warning headers retain separate positional indexes. Given dismissal correctness depends on stable counts/indexes (see the
twpWarningsgetter), a test with two identical header values would document the expected behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/test/extensions/list_email_header_extension_test.dart` around lines 77 - 90, Add a test that covers two identical X-TWP-Message headers so the twpWarnings getter is verified with duplicate values, not just distinct ones. Use the existing list_email_header_extension_test.dart setup and the EmailHeader, EmailProperty.headerTwpMessageKey, and twpWarnings symbols to create two identical warning entries, then assert they are both preserved with separate positional indexes. Keep the current multi-warning test, but extend coverage to document stable ordering/counts when duplicates are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/features/email/data/network/email_api.dart`:
- Around line 868-870: The dismiss update is being added twice because
`setEmailMethod` is invoked once and then added again when assigning
`setEmailInvocation`, which can send duplicate `SetEmail` calls. In the
`dismiss` flow in `email_api.dart`, keep only the single
`requestBuilder.invocation(setEmailMethod)` call whose result is used, and
remove the redundant invocation so the request contains only one `SetEmail`
method call; check the surrounding `JmapRequestBuilder` usage to ensure
`setEmailMethod` is only appended once.
In `@lib/features/email/presentation/email_view.dart`:
- Around line 359-373: The dismiss state for TwpWarningBanner is based on a
non-reactive connectivity check, so it can go stale inside the Obx rebuild.
Update the email_view.dart Obx block and the
SingleEmailController/isNetworkConnectionAvailable path to use a reactive
network status instead of calling
NetworkConnectionController.isNetworkConnectionAvailable() through a plain bool
getter. Either expose an RxBool from NetworkConnectionController or read the
underlying observable directly in the Obx so the banner’s isDismissable value
stays in sync with connectivity changes.
In `@lib/features/email/presentation/widgets/twp_warning_banner.dart`:
- Around line 60-69: The dismiss control in TwpWarningBanner is too small
because the InkWell only wraps the 20px close Icon, making it hard to tap on
touch devices. Update the dismiss action area to use a larger hit target while
keeping the same onDismissAction(warning.index) behavior, ideally by expanding
the clickable region around the Tooltip/Icon pair. Use the existing
isDismissable branch and the surrounding Padding/InkWell structure to adjust the
tappable area without changing the visual icon size.
In `@lib/l10n/intl_en.arb`:
- Around line 2582-2599: The warning strings were added only to the generated
English ARB copy, but the source-of-truth catalog is intl_messages.arb, so move
these keys and their metadata there and keep intl_en.arb in sync from it. While
updating the source entries for twpWarningSuspiciousSender, twpWarningVirus, and
twpWarningVirusRemoved, fix the copy issues in the messages (for example,
correct “immitating” and smooth out the virus wording) so downstream locale
generation picks up the cleaned text.
In `@model/lib/extensions/list_email_header_extension.dart`:
- Around line 38-51: The dismissal index for TWP warnings is currently derived
from iterating over a Set in twpWarnings, which makes the persisted
twp-warning-dismissed-N key unstable. Update
ListEmailHeaderExtension.twpWarnings to avoid positional indexing from Set order
by switching to a deterministic List-based flow with an explicit stable
sort/filter, or by generating the dismissal key from a stable warning identifier
instead of the loop index. Keep the parsing logic in TwpWarning.parse intact,
but ensure the index/value mapping cannot change across fetches or
deduplication.
---
Nitpick comments:
In `@model/test/extensions/list_email_header_extension_test.dart`:
- Around line 77-90: Add a test that covers two identical X-TWP-Message headers
so the twpWarnings getter is verified with duplicate values, not just distinct
ones. Use the existing list_email_header_extension_test.dart setup and the
EmailHeader, EmailProperty.headerTwpMessageKey, and twpWarnings symbols to
create two identical warning entries, then assert they are both preserved with
separate positional indexes. Keep the current multi-warning test, but extend
coverage to document stable ordering/counts when duplicates are present.
🪄 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: 25ce2399-0842-437e-901d-859b6a06c3b9
📒 Files selected for processing (28)
lib/features/email/data/datasource/email_datasource.dartlib/features/email/data/datasource_impl/email_datasource_impl.dartlib/features/email/data/datasource_impl/email_hive_cache_datasource_impl.dartlib/features/email/data/datasource_impl/email_local_storage_datasource_impl.dartlib/features/email/data/datasource_impl/email_session_storage_datasource_impl.dartlib/features/email/data/network/email_api.dartlib/features/email/data/repository/email_repository_impl.dartlib/features/email/domain/repository/email_repository.dartlib/features/email/domain/state/dismiss_twp_warning_state.dartlib/features/email/domain/usecases/dismiss_twp_warning_interactor.dartlib/features/email/presentation/bindings/email_bindings.dartlib/features/email/presentation/bindings/email_interactor_bindings.dartlib/features/email/presentation/controller/single_email_controller.dartlib/features/email/presentation/email_view.dartlib/features/email/presentation/model/twp_warning_code.dartlib/features/email/presentation/widgets/twp_warning_banner.dartlib/l10n/intl_en.arblib/main/localizations/app_localizations.dartmodel/lib/email/email_property.dartmodel/lib/email/presentation_email.dartmodel/lib/email/twp_warning/twp_warning.dartmodel/lib/email/twp_warning/twp_warning_level.dartmodel/lib/extensions/email_id_extensions.dartmodel/lib/extensions/keyword_identifier_extension.dartmodel/lib/extensions/list_email_header_extension.dartmodel/lib/model.dartmodel/test/extensions/list_email_header_extension_test.darttest/features/email/presentation/controller/single_email_controller_test.dart
Member
Author
|
Addressed the review feedback in fa05eb7: Fixed
Explained instead of changed
|
Member
Author
🤖 Session stats69 turns | in=3456 out=27029 cache_read=3107532 cache_write=102523 | cost=n/a |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/l10n/intl_messages.arb`:
- Around line 2691-2708: `AppLocalizations` is still using the old
`Intl.message(...)` text for the warning strings, so the updated ARB copy will
not be visible to English users. Update the localization source that defines
these keys in `AppLocalizations` (the message entries for
`twpWarningSuspiciousSender`, `twpWarningVirus`, and `twpWarningVirusRemoved`)
or regenerate the generated localization file so it matches the corrected ARB
strings.
🪄 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: c81086df-9f07-4854-adc9-695bc6574bda
📒 Files selected for processing (6)
lib/features/email/data/network/email_api.dartlib/features/email/presentation/widgets/twp_warning_banner.dartlib/l10n/intl_en.arblib/l10n/intl_messages.arbmodel/lib/extensions/list_email_header_extension.dartmodel/test/extensions/list_email_header_extension_test.dart
💤 Files with no reviewable changes (1)
- lib/features/email/data/network/email_api.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/l10n/intl_en.arb
- model/lib/extensions/list_email_header_extension.dart
- model/test/extensions/list_email_header_extension_test.dart
- lib/features/email/presentation/widgets/twp_warning_banner.dart
Member
Author
|
The only new activity is the CodeScene delta-analysis bot. It flags that No code change made:
No human review comments require changes. |
Member
Author
🤖 Session stats13 turns | in=2455 out=4571 cache_read=195300 cache_write=14177 | cost=n/a |
dab246
reviewed
Jun 29, 2026
dab246
reviewed
Jun 29, 2026
dab246
reviewed
Jun 29, 2026
dab246
reviewed
Jun 29, 2026
dab246
reviewed
Jun 29, 2026
dab246
reviewed
Jun 29, 2026
Member
Author
|
Thanks for the review @dab246 ! I let you manage this from this point onward. If you want to see how it can self solve your remarks, if meaningful I let you set the |
Member
You don't need to label it anymore, let me take care of the PR from here. |
Render backend positioned warnings (X-TWP-Message header) as a colored banner between the header section and the body of the read view. - Parse `level:<info|warn|error> code:<code> <fallback text>` headers into TwpWarning models, keeping their positional index. - Map info/warn/error to blue/yellow/red banners; resolve the message from a frontend code registry (suspicious-sender, virus, virus-removed) falling back to the server provided text for unknown codes. - Dismiss persists a per-position `twp-warning-dismissed-<index>` keyword via SetEmail; dismissal is disabled while offline and applied optimistically. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… sync TWP warning strings to source ARB - Remove duplicate SetEmailMethod invocation in dismissTwpWarning - Enlarge dismiss button tap target in TwpWarningBanner - Add TWP warning strings to source intl_messages.arb and fix copy (imitating, virus wording) - Document twpWarnings index ordering/dedup contract and add a duplicate-header test
…eded for Riverpod flows Route urgent exceptions (re-login/reconnect) for flows that bypass BaseController.consumeState; bind a lifetime handler in MainBindings instead of MailboxDashBoardController; reuse the shared helper in EmptyFolderProviderListenerDelegate. See ADR-0093.
…le banner Replace the dismiss state/handlers in SingleEmailController with an autoDispose TwpWarningDismiss notifier + TwpWarningBannerList Consumer (keeps markTwpWarningDismissedLocally for keyword sync). Extract TwpWarningBannerStyle, center-align/compact the banner, use AppColor + TMailButtonWidget. Reconcile l10n ARBs to app_localizations. Add notifier/parser/keyword/interactor/resolver/banner unit tests.
Composite email robot (assertion + twpWarning sub-robots, web injection), TwpWarningEmailProvisioner (JMAP create with individualHeaders X-TWP-Message) + ProvisioningTwpWarningEmail value object, and the dismiss scenario/test tagged android/ios/web.
dab246
force-pushed
the
feature/4484-display-mail-warnings
branch
from
fa05eb7 to
cb988af
Compare
Member
Demo E2E
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
test/features/email/presentation/providers/twp_warning_dismiss_notifier_test.dart (1)
37-51: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the shared stubs echo the invoked warning position.
stubSuccess()andstubFailure()always emit the file-levelindex, so the second dismissal on Lines 216-217 still passes even if the notifier forwards the wrong position to the interactor. Returning the invocation args here would make the multi-index tests actually lock the per-position contract.Suggested test helper update
void stubSuccess() { - when(mockInteractor.execute(any, any, any, any)).thenAnswer( - (_) => Stream.value( - Right<Failure, Success>(DismissTwpWarningSuccess(emailId, index)), - ), - ); + when(mockInteractor.execute(any, any, any, any)).thenAnswer((invocation) { + final invokedEmailId = invocation.positionalArguments[2] as EmailId; + final invokedIndex = invocation.positionalArguments[3] as int; + return Stream.value( + Right<Failure, Success>( + DismissTwpWarningSuccess(invokedEmailId, invokedIndex), + ), + ); + }); } void stubFailure() { - when(mockInteractor.execute(any, any, any, any)).thenAnswer( - (_) => Stream.value( - Left<Failure, Success>(DismissTwpWarningFailure(index: index)), - ), - ); + when(mockInteractor.execute(any, any, any, any)).thenAnswer((invocation) { + final invokedIndex = invocation.positionalArguments[3] as int; + return Stream.value( + Left<Failure, Success>(DismissTwpWarningFailure(index: invokedIndex)), + ); + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/features/email/presentation/providers/twp_warning_dismiss_notifier_test.dart` around lines 37 - 51, The shared test stubs in `stubSuccess()` and `stubFailure()` are hardcoding the outer `index`, so they don’t verify that `TwpWarningDismissNotifier` passes the correct warning position into `mockInteractor.execute`. Update these helpers to derive the emitted `DismissTwpWarningSuccess` and `DismissTwpWarningFailure` from the invocation arguments instead of the file-level value, and keep the existing multi-index assertions in `twp_warning_dismiss_notifier_test.dart` aligned with that per-call behavior.integration_test/robots/abstract/abstract_email_twp_warning_robot.dart (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccept a warning index in this robot contract.
Dismissal is keyed by warning position, but this API can only express “dismiss banner 0”. That blocks integration coverage for non-first warnings and can hide index-specific regressions.
♻️ Proposed contract update
abstract class AbstractEmailTwpWarningRobot { - Future<void> tapDismissWarning(); + Future<void> tapDismissWarning({int index = 0}); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration_test/robots/abstract/abstract_email_twp_warning_robot.dart` around lines 3 - 4, The AbstractEmailTwpWarningRobot contract only allows dismissing a single implicit warning, but dismissal needs to be keyed by warning position to cover non-first warnings. Update the tapDismissWarning method signature in AbstractEmailTwpWarningRobot to accept a warning index, and then propagate that parameter through every implementation and caller so each warning can be dismissed by its specific position.integration_test/scenarios/email_detailed/dismiss_twp_warning_banner_scenario.dart (1)
35-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReopen the mail before the final assertion.
Right now this only proves the optimistic local hide. If the
SetEmaildismissal write regresses, the test still passes because the banner disappears from current state before the round-trip is proven. Reopening or reloading the message here would exercise the persistence contract.🧪 Suggested scenario hardening
await emailRobot.twpWarning.tapDismissWarning(); + await emailRobot.onTapBackButton(); + await threadRobot.openEmailWithSubject(subject); + await $.pumpAndSettle(); await emailRobot.assertion.expectTwpWarningBannerNotVisible();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration_test/scenarios/email_detailed/dismiss_twp_warning_banner_scenario.dart` around lines 35 - 36, The dismissal check in dismiss_twp_warning_banner_scenario.dart only verifies the banner is hidden in the current view, so it can miss a broken persistence write. Update the scenario around tapDismissWarning() and expectTwpWarningBannerNotVisible() to reopen or reload the same email before the final assertion, using the existing emailRobot flow, so the test validates that the dismissal survives a round-trip rather than just a local UI hide.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/features/email/presentation/widgets/twp_warning_banner_list.dart`:
- Around line 97-111: The switch in TwpWarningBannerList’s result handling is
missing terminators for the TwpWarningDismissResult.dismissed and
TwpWarningDismissResult.failed cases, which makes the Dart switch invalid.
Update the switch that handles result so each case is explicitly terminated,
either by adding breaks or by using another terminating statement, while keeping
the existing behavior in controller.markTwpWarningDismissedLocally and the
appToastProvider.showToastErrorMessage path intact.
In
`@lib/features/mailbox_dashboard/presentation/delegates/empty_folder_provider_listener_delegate.dart`:
- Around line 181-184: The _handleUrgentException helper currently ignores the
boolean returned by handleUrgentExceptionIfNeeded, so urgent auth/network
failures can still show the local toast in the EmptyFolderFailure path. Update
_handleUrgentException in EmptyFolderProviderListenerDelegate to return that
boolean result, then adjust the caller handling EmptyFolderFailure to skip its
generic failure toast when the helper reports the exception was already handled.
In `@test/features/email/presentation/widgets/twp_warning_banner_test.dart`:
- Around line 61-69: The test in the warning banner widget only asserts that the
raw fallback text is not shown, so it can still pass when the localized message
is missing or incorrect. Update the `renders the localized message for a known
code` test in `twp_warning_banner_test.dart` to explicitly assert the expected
localized text produced by `TwpWarningBanner` for the `virus` code, using the
same widget setup and checking that the resolved string is present.
---
Nitpick comments:
In `@integration_test/robots/abstract/abstract_email_twp_warning_robot.dart`:
- Around line 3-4: The AbstractEmailTwpWarningRobot contract only allows
dismissing a single implicit warning, but dismissal needs to be keyed by warning
position to cover non-first warnings. Update the tapDismissWarning method
signature in AbstractEmailTwpWarningRobot to accept a warning index, and then
propagate that parameter through every implementation and caller so each warning
can be dismissed by its specific position.
In
`@integration_test/scenarios/email_detailed/dismiss_twp_warning_banner_scenario.dart`:
- Around line 35-36: The dismissal check in
dismiss_twp_warning_banner_scenario.dart only verifies the banner is hidden in
the current view, so it can miss a broken persistence write. Update the scenario
around tapDismissWarning() and expectTwpWarningBannerNotVisible() to reopen or
reload the same email before the final assertion, using the existing emailRobot
flow, so the test validates that the dismissal survives a round-trip rather than
just a local UI hide.
In
`@test/features/email/presentation/providers/twp_warning_dismiss_notifier_test.dart`:
- Around line 37-51: The shared test stubs in `stubSuccess()` and
`stubFailure()` are hardcoding the outer `index`, so they don’t verify that
`TwpWarningDismissNotifier` passes the correct warning position into
`mockInteractor.execute`. Update these helpers to derive the emitted
`DismissTwpWarningSuccess` and `DismissTwpWarningFailure` from the invocation
arguments instead of the file-level value, and keep the existing multi-index
assertions in `twp_warning_dismiss_notifier_test.dart` aligned with that
per-call behavior.
🪄 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: e0b065dc-237c-402f-820d-fa27bbf66b9e
📒 Files selected for processing (58)
docs/adr/0093-urgent-exception-handling-for-riverpod-flows.mdintegration_test/models/provisioning_twp_warning_email.dartintegration_test/provisioning/twp_warning_email_provisioner.dartintegration_test/robots/abstract/abstract_email_assertion_robot.dartintegration_test/robots/abstract/abstract_email_robot.dartintegration_test/robots/abstract/abstract_email_twp_warning_robot.dartintegration_test/robots/email_assertion_robot.dartintegration_test/robots/email_robot.dartintegration_test/robots/email_twp_warning_robot.dartintegration_test/robots/mobile/mobile_email_robot.dartintegration_test/robots/web/web_email_assertion_robot.dartintegration_test/robots/web/web_email_robot.dartintegration_test/scenarios/email_detailed/dismiss_twp_warning_banner_scenario.dartintegration_test/tests/email_detailed/dismiss_twp_warning_banner_test.dartlib/features/base/handle_urgent_exception.dartlib/features/base/model/ui_keys.dartlib/features/base/urgent_exception_handler_service.dartlib/features/email/data/datasource/email_datasource.dartlib/features/email/data/datasource_impl/email_datasource_impl.dartlib/features/email/data/datasource_impl/email_hive_cache_datasource_impl.dartlib/features/email/data/datasource_impl/email_local_storage_datasource_impl.dartlib/features/email/data/datasource_impl/email_session_storage_datasource_impl.dartlib/features/email/data/network/email_api.dartlib/features/email/data/repository/email_repository_impl.dartlib/features/email/domain/repository/email_repository.dartlib/features/email/domain/state/dismiss_twp_warning_state.dartlib/features/email/domain/usecases/dismiss_twp_warning_interactor.dartlib/features/email/presentation/bindings/email_interactor_bindings.dartlib/features/email/presentation/controller/single_email_controller.dartlib/features/email/presentation/email_view.dartlib/features/email/presentation/model/twp_warning_code.dartlib/features/email/presentation/providers/twp_warning_dismiss_notifier.dartlib/features/email/presentation/styles/twp_warning_banner_style.dartlib/features/email/presentation/widgets/twp_warning_banner.dartlib/features/email/presentation/widgets/twp_warning_banner_list.dartlib/features/mailbox_dashboard/presentation/bindings/mailbox_dashboard_bindings.dartlib/features/mailbox_dashboard/presentation/delegates/empty_folder_provider_listener_delegate.dartlib/l10n/intl_en.arblib/l10n/intl_messages.arblib/main/bindings/main_bindings.dartlib/main/localizations/app_localizations.dartmodel/lib/email/email_property.dartmodel/lib/email/presentation_email.dartmodel/lib/email/twp_warning/twp_warning.dartmodel/lib/email/twp_warning/twp_warning_level.dartmodel/lib/extensions/email_id_extensions.dartmodel/lib/extensions/keyword_identifier_extension.dartmodel/lib/extensions/list_email_header_extension.dartmodel/lib/model.dartmodel/test/email/twp_warning/twp_warning_test.dartmodel/test/extensions/list_email_header_extension_test.dartmodel/test/extensions/twp_warning_dismiss_keyword_test.darttest/features/base/handle_urgent_exception_test.darttest/features/base/urgent_exception_handler_service_test.darttest/features/email/domain/usecases/dismiss_twp_warning_interactor_test.darttest/features/email/presentation/model/twp_warning_code_resolver_test.darttest/features/email/presentation/providers/twp_warning_dismiss_notifier_test.darttest/features/email/presentation/widgets/twp_warning_banner_test.dart
💤 Files with no reviewable changes (1)
- lib/features/mailbox_dashboard/presentation/bindings/mailbox_dashboard_bindings.dart
✅ Files skipped from review due to trivial changes (8)
- integration_test/tests/email_detailed/dismiss_twp_warning_banner_test.dart
- integration_test/robots/email_assertion_robot.dart
- integration_test/models/provisioning_twp_warning_email.dart
- model/lib/email/email_property.dart
- docs/adr/0093-urgent-exception-handling-for-riverpod-flows.md
- lib/l10n/intl_messages.arb
- lib/l10n/intl_en.arb
- lib/main/localizations/app_localizations.dart
🚧 Files skipped from review as they are similar to previous changes (20)
- lib/features/email/data/datasource_impl/email_session_storage_datasource_impl.dart
- lib/features/email/data/repository/email_repository_impl.dart
- lib/features/email/data/datasource/email_datasource.dart
- lib/features/email/data/datasource_impl/email_hive_cache_datasource_impl.dart
- lib/features/email/domain/usecases/dismiss_twp_warning_interactor.dart
- model/lib/extensions/keyword_identifier_extension.dart
- lib/features/email/data/datasource_impl/email_local_storage_datasource_impl.dart
- lib/features/email/domain/state/dismiss_twp_warning_state.dart
- lib/features/email/data/network/email_api.dart
- model/test/extensions/list_email_header_extension_test.dart
- model/lib/email/twp_warning/twp_warning_level.dart
- model/lib/extensions/list_email_header_extension.dart
- model/lib/email/twp_warning/twp_warning.dart
- lib/features/email/presentation/model/twp_warning_code.dart
- lib/features/email/domain/repository/email_repository.dart
- model/lib/model.dart
- lib/features/email/presentation/bindings/email_interactor_bindings.dart
- lib/features/email/data/datasource_impl/email_datasource_impl.dart
- model/lib/extensions/email_id_extensions.dart
- model/lib/email/presentation_email.dart
Member
|
Docker image published for this PR: linagora/tmail-web-pr:4639 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: dab246 <tdvu@linagora.com>
There was a problem hiding this comment.
Our agent can fix these. Install it.
Gates Passed
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Member
|
Docker image published for this PR: linagora/tmail-web-pr:4639 |
Member
|
Member
|
please help me splits this PR into smaller on value oriented |
Member
Author
|
Thanks @dab246 and @hoangdat for the review. No automated commit is being pushed for this round, on purpose. The new feedback is large-scope, maintainer-driven work rather than isolated fixes:
Given that the PR is going to be restructured/split and the Riverpod migration is a substantial refactor being driven by the maintainers, pushing sweeping (or isolated) changes onto this branch now would only conflict with that work — so I am deferring to you both. For the record, the only remaining small quick-win from the review is extracting a Generated automatically |
Member
Author
🤖 Session stats8 turns | 99s | in=3024 out=5545 cache_read=144134 cache_write=28237 | cost=0.5091 USD |
Member
Author
|
What's thestatus of this work ? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4484
Why
Allow the backend to position warning banners on a message (suspicious sender, virus, virus removed, …) so that they are surfaced when the user opens the mail.
What
Renders backend positioned warnings carried by
X-TWP-Messageheaders as a colored banner between the header section and the body of the read view.Header contract (confirmed in the issue):
info/warn/errormap to blue / yellow / red.code, otherwise the server provided fallback text.Implementation
model):TwpWarning+TwpWarningLevelwith a tolerant parser;ListEmailHeaderExtension.twpWarningsextracts and indexes the warnings (the header set is already fetched viaEmailProperty.headers);PresentationEmail.isTwpWarningDismissed(index); positionaltwp-warning-dismissed-<index>keyword helpers.TwpWarningBannerwidget + aTwpWarningCodeResolverregistry (suspicious-sender,virus,virus-removed) — easy to extend with new codes. Inserted inEmailViewright after the header block.SetEmailflow (DismissTwpWarningInteractor->EmailRepository->EmailApi), mirroring the existing unsubscribe/markAsStar flows. Applied optimistically and disabled while offline.Tests
ListEmailHeaderExtension.twpWarningsparser tests (single/multiple warnings, positional indexes, missing/unknown tokens defaulting toinfo).🤖 Generated with Claude Code
Summary by CodeRabbit