Conversation
|
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:
WalkthroughAdds an immutable MailboxCollection model and migrates mailbox tree generation and update flows to use it. Tree builder methods now accept and return MailboxCollection. BaseMailboxController exposes 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
🤖 Fix all issues with AI agents
In
`@lib/features/mailbox/presentation/extensions/handle_action_required_tab_extension.dart`:
- Around line 52-64: The helper _addToAllMailboxes mutates the input list by
calling currentAllMailboxes.add(folder); change it to be non-mutating: when the
id is not present, return a new list containing the existing entries plus folder
(e.g., copy currentAllMailboxes and append folder) instead of modifying
currentAllMailboxes in place so callers' original MailboxCollection.allMailboxes
is not altered.
In
`@lib/features/mailbox/presentation/extensions/handle_favorite_tab_extension.dart`:
- Around line 55-67: The function _addFavoriteFolderToAllMailboxes currently
mutates the input list allMailboxes by calling .add(favoriteFolder); change it
to return a new list instead to preserve immutability (e.g., create a copy of
allMailboxes and append favoriteFolder or return a new list combining
allMailboxes and favoriteFolder) while keeping the existing early-exit when the
id already exists; update only inside _addFavoriteFolderToAllMailboxes and
preserve parameter names favoriteFolder and allMailboxes.
🧹 Nitpick comments (1)
lib/features/search/mailbox/presentation/search_mailbox_controller.dart (1)
177-183: Consider passing callback torefreshTreefor consistency.The
buildTreecall (lines 170-173) passesonUpdateMailboxCollectionCallback: updateMailboxCollection, but therefreshTreecall here doesn't pass the callback. InMailboxController, the equivalentrefreshTreecall (line 629-632) does pass the callback. This inconsistency might cause virtual folders not to be added after a refresh in the search mailbox context.♻️ Proposed fix
} else if (success is RefreshChangesAllMailboxSuccess) { currentMailboxState = success.currentMailboxState; - await refreshTree(success.mailboxList); + await refreshTree( + success.mailboxList, + onUpdateMailboxCollectionCallback: updateMailboxCollection, + ); if (currentContext != null) { syncAllMailboxWithDisplayName(currentContext!); }
lib/features/mailbox/presentation/extensions/handle_action_required_tab_extension.dart
Show resolved
Hide resolved
lib/features/mailbox/presentation/extensions/handle_favorite_tab_extension.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/mailbox/presentation/extensions/handle_favorite_tab_extension.dart (1)
36-52: Avoid mutating the existing default tree list.
currentDefaultFoldersaliasesdefaultTree.root.childrenItems. The subsequent insert mutates the original list, undermining the new immutable flow and potentially causing side effects elsewhere. Clone the list before inserting.🔧 Suggested fix
- List<MailboxNode> currentDefaultFolders = - defaultMailboxNode.childrenItems ?? []; + final currentDefaultFolders = + List<MailboxNode>.from(defaultMailboxNode.childrenItems ?? []);
🤖 Fix all issues with AI agents
In
`@lib/features/mailbox/presentation/extensions/handle_action_required_tab_extension.dart`:
- Around line 40-49: The _addToDefaultMailboxTree function unconditionally
inserts a virtual Action Required node which can create duplicates; before
calling children.insertAfterStarredOrInbox(MailboxNode(folder)) check the
existing children (root.childrenItems) for an existing MailboxNode representing
the same virtual folder (e.g., compare mailbox id or a distinguishing flag on
PresentationMailbox) and skip insertion (return currentDefaultTree or
root.copyWith(children: children)) if a match is found; use the same identifiers
present in this file (function _addToDefaultMailboxTree, PresentationMailbox,
MailboxTree, MailboxNode, insertAfterStarredOrInbox, root.childrenItems) so the
guard prevents inserting a duplicate node.
In `@lib/features/search/mailbox/presentation/search_mailbox_controller.dart`:
- Around line 224-235: The failure branch is bypassing virtual-folder gating by
calling addActionRequiredFolder directly; instead, construct the
MailboxCollection (using MailboxCollection with
allMailboxes/defaultTree/personalTree/teamTree) and pass it into
updateMailboxCollection so updateMailboxCollection → autoCreateVirtualFolder
applies feature-flag checks (isAINeedsActionEnabled) and also ensures Favorites
get created; replace the direct addActionRequiredFolder call with a call to
updateMailboxCollection(mailboxCollection: MailboxCollection(...),
isRefreshTrigger: false) to unify behavior with the success path.
lib/features/mailbox/presentation/extensions/handle_action_required_tab_extension.dart
Show resolved
Hide resolved
lib/features/search/mailbox/presentation/search_mailbox_controller.dart
Outdated
Show resolved
Hide resolved
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4290. |
There was a problem hiding this comment.
how we apply the same pattern with other, return a MailboxCollection instead of mutate directly?
Signed-off-by: dab246 <tdvu@linagora.com>
dab246
force-pushed
the
bugfix/tf-4284-refreshing-causes-virtual-mailboxes-to-be-hidden
branch
from
5c22259 to
e26bf41
Compare
![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.
Signed-off-by: dab246 <tdvu@linagora.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lib/features/mailbox/presentation/extensions/handle_favorite_tab_extension.dart
Show resolved
Hide resolved
|
how about to add these tests to group('MailboxCollection allMailboxes field', () {
final inbox = PresentationMailbox(
MailboxId(Id('inbox')),
name: MailboxName('Inbox'),
role: Role('inbox'),
);
final sent = PresentationMailbox(
MailboxId(Id('sent')),
name: MailboxName('Sent'),
role: Role('sent'),
);
test(
'generateMailboxTreeInUI: returned allMailboxes matches input list',
() async {
final input = [inbox, sent];
final result = await TreeBuilder().generateMailboxTreeInUI(
allMailboxes: input,
currentCollection: MailboxCollection.empty(),
);
expect(result.allMailboxes.length, equals(input.length));
expect(
result.allMailboxes.map((m) => m.id).toList(),
containsAll(input.map((m) => m.id)),
);
});
test(
'generateMailboxTreeInUIAfterRefreshChanges: returned allMailboxes matches input list',
() async {
final input = [inbox, sent];
final result =
await TreeBuilder().generateMailboxTreeInUIAfterRefreshChanges(
allMailboxes: input,
currentCollection: MailboxCollection.empty(),
);
expect(result.allMailboxes.length, equals(input.length));
expect(
result.allMailboxes.map((m) => m.id).toList(),
containsAll(input.map((m) => m.id)),
);
});
test(
'generateMailboxTreeInUIAfterRefreshChanges: allMailboxes in result excludes virtual folders from current collection',
() async {
final serverMailboxes = [inbox, sent];
final virtualFavorite = PresentationMailbox.favoriteFolder;
final previousCollection = MailboxCollection(
allMailboxes: [inbox, virtualFavorite],
defaultTree: MailboxTree(MailboxNode.root()),
personalTree: MailboxTree(MailboxNode.root()),
teamMailboxTree: MailboxTree(MailboxNode.root()),
);
final result =
await TreeBuilder().generateMailboxTreeInUIAfterRefreshChanges(
allMailboxes: serverMailboxes,
currentCollection: previousCollection,
);
expect(
result.allMailboxes.every((m) => !m.isVirtualFolder),
isTrue,
reason:
'Tree builder must not inject virtual folders — they are added by the callback after buildTree/refreshTree',
);
expect(result.allMailboxes.length, equals(serverMailboxes.length));
});
}); |
… hidden Signed-off-by: dab246 <tdvu@linagora.com>
There was a problem hiding this comment.
(4 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| mailbox_controller.dart | 6.75 → 6.87 | Complex Method |
| mailbox_tree_builder.dart | 9.54 → 10.00 | Bumpy Road Ahead, Excess Number of Function Arguments |
| mailbox_tree_builder_test.dart | 9.10 → 9.39 | Code Duplication |
| rule_filter_creator_controller_test.dart | 8.74 → 8.76 | Large Method |
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.
Test case 3 has been applied, along with some additional test cases. Test cases 1 and 2 have been skipped because: Test 1 is technically correct but weak — it only checks no items are lost/duplicated, while the meaningful behavior (selected mailbox → deactivated) is already covered by Test 4. It adds overlap without value. Test 2 is a tautology — it asserts More valuable tests are those ensuring no stale data (virtual folders, deleted mailboxes) leaks after refresh, which are already covered elsewhere. |
3 participants
Issue
#4284
Root cause
Because each refresh involves rebuilding the tree before adding the virtual folders (Favorite/Action Required), the mailbox list view is re-rendered.
Solution
Add virtual folders when creating the mailbox tree. Only then trigger the rendering to the UI.
Resolved
Screen.Recording.2026-01-29.at.17.49.38.mov
Summary by CodeRabbit
New Features
Refactor
Tests