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 two ADRs. ADR-0071 enables 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0071-collapse-threads-in-email-query.md`:
- Around line 39-40: The ADR is inconsistent about the authoritative source for
thread mode — pick the existing thread-enabled setting as the single source of
truth and update the document so all references use that setting; remove or
reword the statement about passing thread-enabled status from Session (or state
that Session reads the canonical setting rather than owning it), and ensure the
toggle handler description mentions clearing the email cache and rebuilding via
getAllEmail() under the existing thread-enabled setting; update any mentions of
"thread-enabled from Session" to reference the canonical thread-enabled setting
(and Session only reads it) so wording is consistent across Decision and Steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72a1eabf-9953-4224-b3f0-9c4ef76bf032
📒 Files selected for processing (1)
docs/adr/0071-collapse-threads-in-email-query.md
|
Ok with me. |
|
Added |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md`:
- Around line 48-50: The markdown fenced code blocks containing content like
"ThreadId → List<EmailId>" (and the other blocks flagged around the same areas)
are missing language identifiers; update each triple-backtick fence to include
an appropriate language tag (e.g., ```text for plain text, ```mermaid for
diagrams, or ```dart for code examples) so markdownlint MD040 is satisfied,
ensuring you change the opening fences for the blocks that contain the ThreadId
→ List<EmailId> fragment and the other two flagged blocks to include the correct
language identifier.
- Around line 258-265: The MoveThreadInteractor.execute currently only accepts
List<ThreadId> threadIds which conflicts with the ADR requiring support for
mixed selection; update MoveThreadInteractor.execute signature to accept both
threadIds and messageIds (or emailIds) like the MarkAsThreadReadInteractor
pattern, then implement the same merge/dedup workflow used in
MarkAsThreadReadInteractor: merge incoming thread and message ID lists into a
unified set, deduplicate related thread IDs (resolve messages to their parent
threads), and proceed with the existing move logic using the merged/deduped
thread list; locate symbols MoveThreadInteractor, execute, and the parameter
threadIds to apply the changes and mirror the merge/dedup helper logic used by
MarkAsThreadReadInteractor.
- Around line 11-12: Update ADR-0072 to require and document an explicit
two-level precondition before enabling thread-aware behavior: (1) the server
Session must advertise support for collapseThreads and (2) the user Settings
toggle must be enabled; reference the concepts "collapseThreads", "Session",
"Settings", "EmailList", "ThreadId" and "EmailId" in the text and add a clear
conditional sentence or checklist in the "thread-aware flow" section that both
checks must pass before treating EmailList items as ThreadId rather than
EmailId; ensure examples and any flow diagrams or pseudocode in the doc reflect
both checks so implementers cannot enable collapseThreads based solely on ADR
assumption.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eef60d07-a6f0-47cf-97ed-9fa5a236fa94
📒 Files selected for processing (1)
docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md
docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md
Outdated
Show resolved
Hide resolved
docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md (3)
370-374: Add integration complexity to negative consequences.Given that existing email interactors return
Stream<Either<Failure, Success>>and require complex request objects (likeMoveToMailboxRequest), the integration effort is higher than the simplified examples suggest.📝 Suggested addition
### Negative * Increased number of interactors * Requires careful cache invalidation * Still involves multiple API calls (mitigated via cache + parallelism) +* Stream-based integration adds complexity (async iteration, result collection, state management) +* Complex request object construction (MoveToMailboxRequest, emailIdsByMailboxId) requires additional context gathering🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md` around lines 370 - 374, Update the "Negative consequences" section to explicitly call out increased integration complexity: mention that existing email interactors return Stream<Either<Failure, Success>> and that many operations require complex request objects (e.g., MoveToMailboxRequest), so integrating the proposed change will need more wiring, error-stream handling, and request mapping than the simplified examples imply; add a bullet noting this higher effort and suggest ensuring cache invalidation and API call orchestration account for these interactor shapes.
118-118: Consider adding cache size limits or TTL for production use.The in-memory cache has no bounds or eviction policy. For apps with many threads or long sessions, this could accumulate memory. Consider adding a maximum size (LRU eviction) or time-to-live.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md` at line 118, The in-memory Map named _cache (Map<ThreadId, List<EmailId>>) is unbounded and needs eviction—add a size limit and/or TTL to prevent memory growth; implement an LRU or TTL-backed wrapper around _cache (e.g., replace with or wrap using an LRU map or a map that stores timestamps and evicts on access), expose configurable maxEntries and ttl values, and update accesses in methods that read/write _cache to perform eviction checks so production usage cannot grow unbounded.
346-357: Specify where cache invalidation should be hooked.The cache invalidation triggers are correct, but the ADR should guide implementers on where to place the
clearCache()calls (e.g., in repository callbacks, after mailbox sync completion, in state change observers, etc.).💡 Suggested addition
Add a subsection under "Cache Invalidation":
### Implementation Points Hook `expansionService.clearCache()` in: - `MailboxRepository` after successful sync completion - Email interactor wrappers after successful state mutations (read/unread, star, move, delete) - Settings controller when `collapseThreads` toggle changes - Thread detail update callbacks when thread membership changes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md` around lines 346 - 357, The ADR lists cache invalidation triggers but doesn't say where to call expansionService.clearCache(); add an "Implementation Points" subsection under "Cache Invalidation" that instructs implementers to call expansionService.clearCache() from the concrete places: after successful mailbox sync in MailboxRepository, in email interactor wrappers immediately after successful state mutations (mark read/unread, star, move, delete), in the Settings controller when collapseThreads is toggled, and in thread detail update callbacks when thread membership or content changes; mention these specific symbols (expansionService.clearCache(), MailboxRepository, Settings controller, email interactor wrappers, thread detail update callbacks) so integrators know exactly where to hook the invalidation calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md`:
- Around line 186-198: The ThreadActionResult class references undefined types
Id and SetError (alongside EmailId and ThreadId) — update the ADR snippet by
either adding the appropriate import statements for the JMAP types at the top of
the code block or replace those identifiers with fully‑qualified names used
elsewhere in the codebase; specifically, ensure the file includes imports for Id
and SetError (or uses the full module/path names) so ThreadActionResult, its
fields success, actionErrors, and expansionErrors compile and the dependency is
explicit.
- Around line 296-307: The ADR example uses the wrong signature and return
handling for MoveMultipleEmailToMailboxInteractor: replace the shown call to
moveInteractor.execute(session:..., accountId:..., emailIds:...,
destinationMailboxId:...) with construction of a MoveToMailboxRequest (populate
currentMailboxes from expanded threads, set moveAction and emailActionType) and
pass the required Map<EmailId, bool> emailIdsWithReadStatus, then call
moveInteractor.execute(session, accountId, moveRequest, emailIdsWithReadStatus)
which returns a Stream<Either<Failure, Success>>; update the example to consume
the stream (e.g., await first / listen) and handle the concrete success/failure
union types MoveMultipleEmailToMailboxAllSuccess and
MoveMultipleEmailToMailboxHasSomeEmailFailure to derive the ThreadActionResult
instead of accessing non-existent .emailIdsSuccess/.mapErrors fields.
- Around line 241-251: The ADR example calls emailInteractor.execute(...) with
the wrong signature and awaits a Stream result; update the example to call
MarkAsMultipleEmailReadInteractor.execute (or emailInteractor.execute) with the
required ReadActions readAction and Map<MailboxId, List<EmailId>>
emailIdsByMailboxId parameters and handle the Stream<Either<Failure, Success>>
return correctly (e.g., use an await for or stream.listen to aggregate successes
and mapErrors from each Either), then build the ThreadActionResult using the
aggregated emailIdsSuccess and mapErrors alongside expansion.errors;
alternatively mark the snippet explicitly as simplified pseudocode and state the
real signature to avoid compilation confusion.
---
Nitpick comments:
In `@docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md`:
- Around line 370-374: Update the "Negative consequences" section to explicitly
call out increased integration complexity: mention that existing email
interactors return Stream<Either<Failure, Success>> and that many operations
require complex request objects (e.g., MoveToMailboxRequest), so integrating the
proposed change will need more wiring, error-stream handling, and request
mapping than the simplified examples imply; add a bullet noting this higher
effort and suggest ensuring cache invalidation and API call orchestration
account for these interactor shapes.
- Line 118: The in-memory Map named _cache (Map<ThreadId, List<EmailId>>) is
unbounded and needs eviction—add a size limit and/or TTL to prevent memory
growth; implement an LRU or TTL-backed wrapper around _cache (e.g., replace with
or wrap using an LRU map or a map that stores timestamps and evicts on access),
expose configurable maxEntries and ttl values, and update accesses in methods
that read/write _cache to perform eviction checks so production usage cannot
grow unbounded.
- Around line 346-357: The ADR lists cache invalidation triggers but doesn't say
where to call expansionService.clearCache(); add an "Implementation Points"
subsection under "Cache Invalidation" that instructs implementers to call
expansionService.clearCache() from the concrete places: after successful mailbox
sync in MailboxRepository, in email interactor wrappers immediately after
successful state mutations (mark read/unread, star, move, delete), in the
Settings controller when collapseThreads is toggled, and in thread detail update
callbacks when thread membership or content changes; mention these specific
symbols (expansionService.clearCache(), MailboxRepository, Settings controller,
email interactor wrappers, thread detail update callbacks) so integrators know
exactly where to hook the invalidation calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c132941-54e3-4413-8ffb-3b7fe9ac89b7
📒 Files selected for processing (1)
docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md
docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md
Outdated
Show resolved
Hide resolved
docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md
Outdated
Show resolved
Hide resolved
…pseThreads in EmailList
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md (1)
118-118: Consider adding cache size limits or TTL to prevent unbounded memory growth.The
_cacheMap has no size limit or expiration policy. In a long-running mobile app, if users interact with many threads over time, this cache could accumulate entries indefinitely, leading to memory bloat.Consider either:
- Implementing an LRU eviction policy (e.g., keeping only the N most recent threads)
- Adding a time-to-live (TTL) mechanism
- Documenting in the Consequences section that cache growth is unbounded and relies on periodic
clearCache()calls💡 Example: LRU cache pattern
import 'package:collection/collection.dart'; final LruMap<ThreadId, List<EmailId>> _cache = LruMap(maximumSize: 100);Or document the limitation:
### Negative * Increased number of interactors * Requires careful cache invalidation +* Cache is unbounded; relies on timely clearCache() calls to prevent memory growth * Still involves multiple API calls (mitigated via cache + parallelism)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md` at line 118, The _cache Map currently has no bounds or expiration (symbol: _cache) which can cause unbounded memory growth; update the implementation to either replace the Map with a bounded LRU structure (e.g., use an LruMap or similar with a maximumSize) or add a TTL/expiry mechanism that evicts stale ThreadId entries, and ensure any existing clearCache() behavior is preserved; alternatively, if you choose not to change code, explicitly document in the ADR Consequences that the cache is unbounded and relies on periodic clearCache() calls to avoid memory bloat.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md`:
- Around line 363-371: Replace vague implementation points with concrete class
and method locations: call expansionService.clearCache() in
MailboxRepository.afterSync() (after successful sync), in EmailController
methods that perform state mutations such as
EmailController.markRead()/markUnread(), EmailController.toggleStar(),
EmailController.moveEmail(), EmailController.deleteEmail() immediately after
successful mutation, in SettingsController.onCollapseThreadsToggle() after the
toggle persists, and in
ThreadController.onMembershipChanged()/ThreadController.refreshAllEmail() when
thread membership or composition changes; follow the existing pattern used by
ThreadRepositoryImpl.clearAllData() / cachingManager.clearAllData() for error
handling and logging.
- Around line 207-261: The MarkAsThreadReadInteractor violates the architecture
and has a logic bug: instead of calling EmailRepository.markAsRead from execute,
inject and delegate to the higher-level MarkAsMultipleEmailReadInteractor
(replace the EmailRepository dependency with a dependency on
MarkAsMultipleEmailReadInteractor and call that interactor from execute) so you
reuse existing business logic, and when invoking the mark-as-read interactor
pass the merged/deduplicated allEmailIds (not the original emailIds) produced by
expansionService.expandThreads; ensure you adapt to the mark-as-multiple
interactor's return/stream shape (await/await-for/.first as appropriate) and
propagate its success/mapErrors into the returned ThreadActionResult along with
expansion.errors.
- Around line 265-314: The MoveThreadInteractor example incorrectly calls
EmailRepository and assumes a Future result; update it to delegate to
MoveMultipleEmailToMailboxInteractor (injected instead of EmailRepository) and
call its execute(...) which returns a Stream<Either<Failure, Success>>; build
the required Map<EmailId, bool> emailIdsWithReadStatus from the expanded
allEmailIds (e.g., Map.fromIterable(allEmailIds, value: (_) => false)) and
subscribe to the Stream, folding results to handle success states like
MoveMultipleEmailToMailboxAllSuccess and failure states like
MoveMultipleEmailToMailboxHasSomeEmailFailure, then map those states into
ThreadActionResult and expansion errors before returning/emitting the final
response.
---
Nitpick comments:
In `@docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md`:
- Line 118: The _cache Map currently has no bounds or expiration (symbol:
_cache) which can cause unbounded memory growth; update the implementation to
either replace the Map with a bounded LRU structure (e.g., use an LruMap or
similar with a maximumSize) or add a TTL/expiry mechanism that evicts stale
ThreadId entries, and ensure any existing clearCache() behavior is preserved;
alternatively, if you choose not to change code, explicitly document in the ADR
Consequences that the cache is unbounded and relies on periodic clearCache()
calls to avoid memory bloat.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 373dee2e-a19d-48bc-8c1d-67f414364fb2
📒 Files selected for processing (1)
docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md
…r collapseThreads in EmailList
docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md
Outdated
Show resolved
Hide resolved
docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md
Outdated
Show resolved
Hide resolved
docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md
Outdated
Show resolved
Hide resolved
docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md
Outdated
Show resolved
Hide resolved
…ions for collapseThreads in EmailList
…ulk actions for collapseThreads in EmailList
|
it is ok to me |
…aware bulk actions for collapseThreads in EmailList
| class ThreadExpansionService { | ||
| final ThreadDetailRepository threadDetailRepository; | ||
|
|
||
| final Map<ThreadId, List<EmailId>> _cache = {}; |
There was a problem hiding this comment.
no "max size" for this cache? No TTL ? No eviction strategy?
There was a problem hiding this comment.
No. it will be update whenever email changes happen, user switchs folder.
| return; | ||
| } | ||
|
|
||
| final threadDetails = await threadDetailRepository.getThreadById( |
There was a problem hiding this comment.
Since we only need to have ids at the end, do we really need to use getThreadById? It seems overkill (lot of mapping and maybe other stuff). Can't we have a simple jmap thread/get id request (https://datatracker.ietf.org/doc/html/rfc8621#section-3.1) ? Or is it the same? (naive question)
There was a problem hiding this comment.
getThreadById is a function used to retrieve a list of email IDs (from Thread/get) along with some metadata (such as keywords, mailboxId) (from Email/get). Since emails in a thread can be in different mailboxes, we need these properties to support processing those actions.
| Cache must be cleared when: | ||
|
|
||
| * Mailbox sync occurs | ||
| * Email state changes (read, star, move, delete) |
There was a problem hiding this comment.
If the cache is Map<ThreadId, List<EmailId>>, why do we need to invalide the cache when we have a read or star change? For move & delete I understand.
There was a problem hiding this comment.
This descriptive name might not be accurate (I'll delete it), but the point is that we should clear the cache every time we open the mailbox and refresh for changes.
|
|
||
| * Mailbox sync occurs | ||
| * Email state changes (read, star, move, delete) | ||
| * Thread content changes |
There was a problem hiding this comment.
what is "thread content"?<List<MailId>>?
There was a problem hiding this comment.
These are actions related to threads, such as when a thread changes, or when an email is added or removed from the thread.
| → deduplicate | ||
| → call existing Email Interactor | ||
| → return (success + actionErrors + expansionErrors) | ||
| ``` |
There was a problem hiding this comment.
ThreadId → List<EmailId>, In the list, can we have a mailId that is not in the same mailbox than the other ones?
If yes, then for mark as read it can be ok to apply the action to all the thread, but for the "move" action for exemple, I don't know. Should ThreadExpansionService.expandThreads() accept an optional mailbox filter ?
There was a problem hiding this comment.
What you said is correct; the emails in the thread can be in different mailboxes, but we've already handled retrieving some email metadata (like mailbox ID) when calling getThreadById. Therefore, performing the previous actions becomes easier.
There was a problem hiding this comment.
Great to now have access to the mailboxid.
So if I’ve in my thread id 1:
- mail 1 -> mailbox 1
- Mail 2 -> mailbox 2
do we give to the user the ability to move the thread? If yes what will be the behavior in this case? We’ll move mail 1 and 2 to the new selected mailbox id?
There was a problem hiding this comment.
Yes, we allow it to be done because we already have enough email data (Id, mailboxId) after performing getThreadById. We will convert List<EmailInThreadDetailsInfo> to Map<MailboxId, List<EmailId>> , then simply pass it to the moveToMailbox function in EmailAPI to handle it automatically. Basically, the moveToMailbox function creates PatchObjects:
{
emailId1: {
mailboxIds/mailboxId1: null,
mailboxIds/newMailboxId: true,
},
emailId2: {
mailboxIds/mailboxId2: null,
mailboxIds/newMailboxId: true,
}
}
There was a problem hiding this comment.
So for a user who chooses the thread view, we display all the emails in the thread, even if they’re not in the same mailbox. Then, on that thread page, we allow the user to move the entire thread to another mailbox.
So that would potentially change the mailbox of an email that the user had specifically placed in a particular mailbox?
A more concrete example:
I’m in a conversation with a client, so my thread is growing. For one of the emails, I move it into a specific mailbox called “client files,” because it contains an important attachment, then the thread continues.
A few days later, I come back, select the thread, and move it to “finalized client.”
Does that mean I won’t find the email with the attachment anymore in my “client files” mailbox? Or do we keep both mailboxes, but then it’s not really a move anymore?
Not sure if my question is clear.
There was a problem hiding this comment.
If we follow the current logic, when you move your current thread to the finalized client folder, all emails from that thread (regardless of which folder they belong to) will also be moved to the finalized client folder.
There was a problem hiding this comment.
I think my code caused you to misunderstand, so I've updated it :))
{
emailId1: {
mailboxIds/mailboxId1: null,
mailboxIds/newMailboxId: true,
},
emailId2: {
mailboxIds/mailboxId2: null,
mailboxIds/newMailboxId: true,
}
}
There was a problem hiding this comment.
OK thanks for the clarification @dab246 . I'm still not convince from a user point of view, but in the meantime I don't know how, as a user, I'll be able to have mails from different mailboxs in the same thread anyway.
As long as we do not display emails from the Trash in threads, that works for me. (and for Draft, I don't know ^^')
There was a problem hiding this comment.
What you said makes sense. Perhaps we need to adjust some of the old logic and compare it with other mail client approaches to create a reasonable UX here.
There was a problem hiding this comment.
(and for Draft, I don't know ^^')
That's interesting!
Do we have detailed spec for product team on this (handling of system mailbox in threads?)
Cc @poupotte
…thread-aware bulk actions for collapseThreads in EmailList
|
This ADR will be implemented through the following tickets:
|
4 participants
ADR to implement
collapseThreadsinEmail/Querycc @chibenwa @Crash-- @poupotte
Summary by CodeRabbit