Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConsolidates RTK Query into a shared base API, adds a mail API with optimistic mark-as-read, introduces mail-specific error classes, adds mail message fetching and date formatting utilities, updates UI to use live mail/store data, and adjusts tests, fixtures, store wiring, and config. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App.tsx`:
- Around line 21-24: The initializeUser async wrapper calls
dispatch(initializeUserThunk()).unwrap() and
dispatch(refreshAvatarThunk()).unwrap() which can throw, and it's invoked inside
useEffect without handling rejections; update the useEffect invocation (or the
initializeUser function) to catch and handle errors from initializeUser — e.g.,
wrap the await calls in try/catch inside initializeUser or append .catch(...) to
the call in useEffect, and use process/UI-safe handling (logging via
console.error or dispatching an error action / setting local error state) so
unhandled promise rejections are avoided.
In `@src/components/user-chip/index.tsx`:
- Line 17: The userName derivation uses name.split(' ')[0] which can return an
empty token for leading or repeated spaces; update both occurrences (the
userName assignment and the similar logic at the other location) to normalize
whitespace first — e.g., use name.trim().split(/\s+/)[0] (or equivalent
normalization) before falling back to email.split('@')[0] so the first non-empty
name token is used.
In `@src/errors/mail/index.ts`:
- Around line 1-11: The constructors redundantly reassign the requestId property
created by the public parameter declaration; remove the explicit this.requestId
= requestId; line from FetchMailboxesInfoError (and the same redundant
assignment in the other three error classes) so the public requestId?: string
parameter alone defines and assigns the property.
In `@src/features/mail/components/tray/index.tsx`:
- Around line 12-14: The Tray component's props include hasMoreItems and
loadMore but MailView.tsx doesn't pass them; add a clear TODO comment next to
the props declaration (hasMoreItems?: boolean; loadMore?: () => void;)
explaining these are reserved for future pagination support and should be wired
up by MailView.tsx when implemented, or alternatively remove those two props
from the Tray props interface if you prefer not to keep unused API surface;
reference the Tray props and the parent MailView.tsx when making the change.
In `@src/features/mail/MailView.tsx`:
- Around line 52-58: TrayList is missing pagination props — pass its
hasMoreItems and loadMore props using the query response and a loader function:
use listFolder?.hasMore (or the appropriate pagination flag on listFolder) for
hasMoreItems, and provide a loadMore handler that calls the same query/fetch
function used to populate listFolder with a next position/cursor (e.g., current
listFolder.emails.length or listFolder.nextCursor) to request the next page;
wire these to TrayList as hasMoreItems={...} and loadMore={...} so infinite
scroll/pagination works with the existing listFolder and isLoadingListFolder
state.
- Around line 38-47: The onSelectEmail handler awaits markAsRead but doesn't
handle failures; wrap the await call in a try/catch inside onSelectEmail so
errors from markAsRead are caught, log the error (or send to monitoring) and
surface user feedback (e.g., trigger a toast/notification or set an error state)
so the user knows the action failed—keep setActiveMailId as-is, perform the
optimistic markAsRead call, and on catch revert or rely on the mutation rollback
while showing the error message to the user.
In `@src/store/queries/mail/mail.query.test.ts`:
- Line 1: Update the top-of-file comment that currently reads
"store/queries/__tests__/storage.query.test.ts" to the correct test filename
"store/queries/__tests__/mail.query.test.ts" so the comment matches this file
(mail.query.test.ts); locate the header comment in the test file (the initial
comment line) and replace the incorrect filename string accordingly to avoid the
copy-paste mismatch.
In `@src/store/queries/mail/mail.query.ts`:
- Line 35: The current providesTags use global strings ('ListFolder') which
broadens invalidation; change getListFolder's providesTags to return
argument-scoped tags (e.g., provide [{ type: 'ListFolder', id:
JSON.stringify(args) } or use folderId/page as the id]) and change
getMailMessage's providesTags to include the specific emailId (e.g., { type:
'MailMessage', id: emailId }); then update any mutating endpoints such as
markAsRead to invalidate or update only those specific tags (invalidate [{ type:
'MailMessage', id: emailId }] and/or the corresponding ListFolder id) so cache
reconciliation is precise.
- Around line 59-80: The optimistic mailbox counter is decremented
unconditionally; change onQueryStarted so the mailbox counter (patchMailboxes
via mailQuery.util.updateQueryData('getMailboxesInfo', ...)) only decrements
when the email actually transitioned from unread to read by detecting that
change in the same patch that updates the email (use the result of the email
lookup in the patchEmailList operation or a local flag set when mail &&
!mail.isRead). Apply the counter decrement only if that flag is true, and on
catch() do not call patchMailboxes.undo(); instead dispatch a refetch of the
mailbox info (e.g., dispatch(mailQuery.util.invalidateTags or trigger a refetch
of 'getMailboxesInfo')) so overlapping optimistic updates cannot restore stale
scalar counts.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9c3c5fc-21db-45a2-8f08-dbd473a237d2
📒 Files selected for processing (18)
src/App.tsxsrc/components/user-chip/index.tsxsrc/constants.tssrc/errors/index.tssrc/errors/mail/index.tssrc/features/mail/MailView.tsxsrc/features/mail/components/tray/index.tsxsrc/hooks/navigation/useSidenavNavigation.tsxsrc/services/date/date.service.test.tssrc/services/date/index.tssrc/services/sdk/mail/index.tssrc/services/sdk/mail/mail.service.test.tssrc/store/index.tssrc/store/queries/mail/mail.query.test.tssrc/store/queries/mail/mail.query.tssrc/test-utils/createTestStore.tssrc/test-utils/fixtures.tsvitest.config.ts
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/features/mail/MailView.tsx (1)
44-52:⚠️ Potential issue | 🟡 MinorCall
.unwrap()here or thiscatchstays dead.RTK Query mutation triggers resolve to
{ data | error }; they only reject on.unwrap(). As written, failedmarkAsReadcalls skip thiscatchand nothing is logged/cast.💡 Minimal fix
try { await markAsRead({ emailId: id, query, - }); + }).unwrap(); } catch (error) { const err = ErrorService.instance.castError(error); console.error(`Error while marking as read the email ${id}: `, err); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/mail/MailView.tsx` around lines 44 - 52, The try/catch around the RTK Query mutation markAsRead is ineffective because RTK returns a {data|error} object unless you call .unwrap(); update the call in MailView.tsx to await markAsRead({ emailId: id, query }).unwrap() so that failures throw and the existing catch (which uses ErrorService.instance.castError and console.error) will run; keep the same error handling logic in the catch block.src/store/queries/mail/mail.query.ts (1)
74-85:⚠️ Potential issue | 🟠 MajorRefetch mailbox counts on failure instead of
undo()ing the counter patch.
patchMailboxes.undo()restores an old scalar snapshot. If twomarkAsReadmutations overlap and an earlier request fails after a later one succeeds, this rollback can erase the later success and overstateunreadEmails.💡 Safer rollback
try { await queryFulfilled; } catch { patchEmailList.undo(); - patchMailboxes.undo(); + dispatch(mailQuery.util.invalidateTags(['Mailbox'])); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/queries/mail/mail.query.ts` around lines 74 - 85, The rollback currently calls patchMailboxes.undo(), which can revert a newer successful update when overlapping markAsRead mutations occur; instead, on catch stop undoing the scalar snapshot and trigger a fresh refetch of mailbox counts: keep patchEmailList.undo() as-is, remove patchMailboxes.undo(), and dispatch mailQuery.endpoints.getMailboxesInfo.initiate(undefined, { forceRefetch: true }) (using the same dispatch visible in this scope) so the mailbox counts (getMailboxesInfo) are re-fetched when queryFulfilled fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/user-chip/index.tsx`:
- Line 56: The tooltip fullName currently uses the raw expression name || email
which allows whitespace-only names to win; change the logic used when rendering
<UserCheap ... fullName={...} /> so the name is normalized (trimmed) and treated
as empty if it contains only whitespace—e.g., compute a normalizedFullName =
(name || '').trim() || email and pass normalizedFullName into the fullName prop
of UserCheap to ensure the email is used as the fallback.
In `@src/features/mail/MailView.tsx`:
- Around line 1-30: The MailView component keeps activeMailId across mailbox
changes causing the previous message to remain visible; add a useEffect in
MailView that listens for changes to the folder prop and calls
setActiveMailId(undefined) to clear the selection on mailbox switch, and ensure
useEffect is imported from React (so reference useEffect, setActiveMailId,
activeMailId, and the MailView component).
In `@src/store/queries/mail/mail.query.ts`:
- Around line 18-20: The current catch blocks in
src/store/queries/mail/mail.query.ts return custom Error instances (e.g., new
FetchMailboxesInfoError(...)) into RTK Query, which is non-serializable; change
those return values to plain serializable objects like { error: { message:
err.message, requestId: err.requestId } } instead of returning Error objects.
Locate the catch blocks that create FetchMailboxesInfoError,
FetchMailboxMessagesError, FetchMessageError (and the other similar
error-returning branches at the commented ranges) and replace the returned Error
instances with plain objects containing at minimum message and optional
requestId so consumers can reconstruct Error instances where needed.
---
Duplicate comments:
In `@src/features/mail/MailView.tsx`:
- Around line 44-52: The try/catch around the RTK Query mutation markAsRead is
ineffective because RTK returns a {data|error} object unless you call .unwrap();
update the call in MailView.tsx to await markAsRead({ emailId: id, query
}).unwrap() so that failures throw and the existing catch (which uses
ErrorService.instance.castError and console.error) will run; keep the same error
handling logic in the catch block.
In `@src/store/queries/mail/mail.query.ts`:
- Around line 74-85: The rollback currently calls patchMailboxes.undo(), which
can revert a newer successful update when overlapping markAsRead mutations
occur; instead, on catch stop undoing the scalar snapshot and trigger a fresh
refetch of mailbox counts: keep patchEmailList.undo() as-is, remove
patchMailboxes.undo(), and dispatch
mailQuery.endpoints.getMailboxesInfo.initiate(undefined, { forceRefetch: true })
(using the same dispatch visible in this scope) so the mailbox counts
(getMailboxesInfo) are re-fetched when queryFulfilled fails.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7cc457de-60d0-4a1c-9ce8-3c67f1d7d2f8
📒 Files selected for processing (7)
src/App.tsxsrc/components/user-chip/index.tsxsrc/errors/mail/index.tssrc/features/mail/MailView.tsxsrc/services/sdk/mail/index.tssrc/store/queries/mail/mail.query.test.tssrc/store/queries/mail/mail.query.ts
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/features/mail/MailView.tsx (3)
1-1:⚠️ Potential issue | 🟠 MajorClear the selected message when
folderchanges.
activeMailIdis folder-agnostic state here, so switching folders can keep rendering the previous folder’s message until the user clicks another item.💡 Minimal fix
-import { useState } from 'react'; +import { useEffect, useState } from 'react'; ... const [activeMailId, setActiveMailId] = useState<string | undefined>(undefined); + + useEffect(() => { + setActiveMailId(undefined); + }, [folder]);Also applies to: 18-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/mail/MailView.tsx` at line 1, The component-level state activeMailId in MailView is folder-agnostic and must be reset when folder changes; add an effect that watches the folder prop/variable and calls setActiveMailId(null) (or undefined) to clear the selection whenever folder updates so the previous folder’s message is not rendered; locate the activeMailId state and its setter (setActiveMailId) in MailView and implement a useEffect(() => { setActiveMailId(null); }, [folder]) to fix it.
39-52:⚠️ Potential issue | 🟡 MinorUse
.unwrap()here or thiscatchnever runs.RTK Query mutation triggers resolve to
{ data | error }unless you call.unwrap(). A failedmarkAsReadwill still roll back the optimistic patch, but thistry/catchwill not log or surface the failure as written.In Redux Toolkit RTK Query 2.11.2, when a mutation trigger is awaited directly (for example `await markAsRead(args)`), does it reject on request failure, or is `.unwrap()` required for `try/catch` to run?🔧 Minimal fix
try { await markAsRead({ emailId: id, query, - }); + }).unwrap(); } catch (error) { const err = ErrorService.instance.castError(error); console.error(`Error while marking as read the email ${id}: `, err); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/mail/MailView.tsx` around lines 39 - 52, The try/catch in onSelectEmail won't catch RTK Query mutation failures because markAsRead returns a fulfilled result object unless unwrapped; update the call in onSelectEmail to await markAsRead({ emailId: id, query }).unwrap() so that failed requests reject and trigger the existing catch block (ensure you import/keep ErrorService handling as-is and keep the optimistic rollback behavior intact).
19-28:⚠️ Potential issue | 🟠 MajorThe folder query is still pinned to page 0.
positionnever moves off0, andTrayListalready exposeshasMoreItems/loadMore(src/features/mail/components/tray/index.tsx:7-24) but does not receive them here. Any folder with more thanDEFAULT_FOLDER_LIMITmessages is therefore truncated to the first batch.Also applies to: 58-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/mail/MailView.tsx` around lines 19 - 28, The query always sets position: 0 so pagination never advances; update MailView to track the folder position (e.g., use state like position or page keyed by folder) and pass that dynamic position into the query object used by useGetListFolderQuery (instead of the hardcoded 0), and forward TrayList's hasMoreItems and loadMore props into MailView so loadMore increments the stored position (or updates the cursor) and triggers a refetch; reference symbols: query, position, useGetListFolderQuery, DEFAULT_FOLDER_LIMIT, TrayList, hasMoreItems, loadMore.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/store/api/mail/mail.api.test.ts`:
- Around line 145-164: Add a test that covers the zero-unread boundary so
optimistic decrement cannot underflow: create a variant of the existing
optimistic-store fixture (use setupOptimisticStore or mutate the returned
store/cache) so the mailboxState for the 'inbox' starts with unreadEmails: 0 and
an email with isRead: false, then spy MailService.instance.updateEmailStatus to
resolve and dispatch mailApi.endpoints.markAsRead.initiate({ emailId, query:
mailboxQuery }) and assert unreadEmails stays 0 (not -1) and the email becomes
read; also add the failing case (mockRejectedValue) and assert unreadEmails
remains 0 after rollback. Reference setupOptimisticStore,
mailApi.endpoints.markAsRead.initiate, MailService.instance.updateEmailStatus
and the mailboxState.unreadEmails field.
- Around line 21-23: Replace vi.clearAllMocks() in the test suite setup with
vi.restoreAllMocks() so mocked implementations (not just call history) are reset
between tests; this prevents leftover mockResolvedValue/mockRejectedValue on the
singleton MailService.instance from affecting later tests. Also add a boundary
unit test for the optimistic-update behavior of the markAsRead flow (the
functions used in tests around markAsRead and unreadEmails) that starts with
unreadEmails = 1, invokes the optimistic mark-as-read action, and asserts the
count becomes 0 (and never negative) both on success and on rollback scenarios
to cover the zero-underflow case.
---
Duplicate comments:
In `@src/features/mail/MailView.tsx`:
- Line 1: The component-level state activeMailId in MailView is folder-agnostic
and must be reset when folder changes; add an effect that watches the folder
prop/variable and calls setActiveMailId(null) (or undefined) to clear the
selection whenever folder updates so the previous folder’s message is not
rendered; locate the activeMailId state and its setter (setActiveMailId) in
MailView and implement a useEffect(() => { setActiveMailId(null); }, [folder])
to fix it.
- Around line 39-52: The try/catch in onSelectEmail won't catch RTK Query
mutation failures because markAsRead returns a fulfilled result object unless
unwrapped; update the call in onSelectEmail to await markAsRead({ emailId: id,
query }).unwrap() so that failed requests reject and trigger the existing catch
block (ensure you import/keep ErrorService handling as-is and keep the
optimistic rollback behavior intact).
- Around line 19-28: The query always sets position: 0 so pagination never
advances; update MailView to track the folder position (e.g., use state like
position or page keyed by folder) and pass that dynamic position into the query
object used by useGetListFolderQuery (instead of the hardcoded 0), and forward
TrayList's hasMoreItems and loadMore props into MailView so loadMore increments
the stored position (or updates the cursor) and triggers a refetch; reference
symbols: query, position, useGetListFolderQuery, DEFAULT_FOLDER_LIMIT, TrayList,
hasMoreItems, loadMore.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 769ededb-7228-4d4d-ac90-34f20a3dce5c
📒 Files selected for processing (10)
src/components/Sidenav/index.tsxsrc/features/mail/MailView.tsxsrc/hooks/navigation/useSidenavNavigation.tsxsrc/store/api/base.tssrc/store/api/mail/index.tssrc/store/api/mail/mail.api.test.tssrc/store/api/storage/index.tssrc/store/api/storage/storage.api.test.tssrc/store/index.tssrc/test-utils/createTestStore.ts
larryrider
reviewed
Mar 30, 2026
larryrider
reviewed
Mar 30, 2026
| textBody: faker.lorem.paragraphs(2), | ||
| htmlBody: `<p>${faker.lorem.paragraphs(2)}</p>`, | ||
| textBody: faker.lorem.paragraphs(2) as unknown as null, | ||
| htmlBody: `<p>${faker.lorem.paragraphs(2)}</p>` as unknown as null, |
Contributor
Author
There was a problem hiding this comment.
I'm not sure why the type is incorrect in the SDK. It defines htmlBody as Record<string, never> (empty object) | null, which prevents us from assigning a string to this field. However, the backend actually returns a string, so I had to temporarily set it to null to satisfy the type constraints. I'll investigate the SDK type generation to understand where this mismatch is coming from.
Contributor
Author
There was a problem hiding this comment.
Ok, I already fixed it in the backend (and in a new SDK version). I will fix this in another PR @larryrider
Contributor
Author
There was a problem hiding this comment.
Yep, I fixed the SDK (by fixing the server types). But I will update the SDK version in another PR as I have to update some types/params.
larryrider
approved these changes
Mar 30, 2026
|
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
3 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.
Implements the RTK Query layer for the mail feature, including all endpoints needed to list, read and interact with emails.
getMailboxesInfo— fetches all mailboxes and their unread countsgetListFolder— fetches paginated email list for a given foldergetMailMessage— fetches full email content by IDmarkAsRead— marks an email as read with optimistic updates: instantly sets isRead: true in the list cache and decrements unreadEmails in the mailbox cache, rolling back both on API failure