Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds mailbox move/delete operations, refactors read-status handling into a generalized mutation, introduces preview ActionsBar and related hooks, adds batch-processing and path-matching utilities with tests, extends MailService with trash support, and expands i18n keys across multiple locales. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
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: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/hooks/mail/useListActionContext.test.ts (1)
33-35:⚠️ Potential issue | 🟡 MinorUse
vi.restoreAllMocks()per testing convention.AGENTS.md mandates
vi.restoreAllMocks()inbeforeEach.vi.clearAllMocks()only resets call data, leaving any latervi.spyOn/mockImplementationoverrides leaking across tests.As per coding guidelines: "Reset mocks in beforeEach with vi.restoreAllMocks()."
beforeEach(() => { - vi.clearAllMocks(); + vi.restoreAllMocks(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/mail/useListActionContext.test.ts` around lines 33 - 35, Replace the vi.clearAllMocks() call in the beforeEach block with vi.restoreAllMocks() so mocks and spy implementations are fully reset between tests; find the beforeEach in useListActionContext.test.ts and change the invocation from vi.clearAllMocks() to vi.restoreAllMocks() to follow AGENTS.md convention and avoid mock leakage across tests.src/i18n/locales/it.json (1)
23-30:⚠️ Potential issue | 🟡 MinorStale
filter.starred/filter.unstarredkeys vs en.json.
en.jsonno longer shipsfilter.starred/filter.unstarred, but they remain here on lines 28–29 (and likewise infr.json). Either remove them from all locales or restore them toen.jsonso keys stay in sync acrossen/es/fr/it.As per coding guidelines: "Add i18n keys in all locales (en, es, fr, it)."
Proposed fix
"filter": { "all": "Tutti", "none": "Nessuno", "read": "Letto", - "unread": "Non letto", - "starred": "Speciali", - "unstarred": "Non speciali" + "unread": "Non letto" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/locales/it.json` around lines 23 - 30, Remove the stale locale keys filter.starred and filter.unstarred from this file (it.json) or restore them in en.json so all locales remain consistent; specifically, update the filter object (keys "starred" and "unstarred") in it.json to match the canonical set used in en.json and ensure the same change is applied across fr.json and es.json (or add the missing keys to en.json) so all four locale files (en.json, es.json, fr.json, it.json) contain the same i18n keys as required by the guidelines.src/i18n/locales/en.json (1)
23-28:⚠️ Potential issue | 🟡 MinorRemove
filter.starredandfilter.unstarredkeys fromit.jsonandfr.jsonto maintain locale parity.The filter object in
en.jsoncontains onlyall,none,read, andunread, butit.jsonandfr.jsonstill havestarredandunstarredkeys. No code references these removed keys, so synchronize all locale files by removing them from the Italian and French translations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/locales/en.json` around lines 23 - 28, Remove the obsolete keys filter.starred and filter.unstarred from the Italian and French locale objects so they match English's filter keys (all, none, read, unread); locate the filter object in the it.json and fr.json locale definitions and delete the starred and unstarred entries, ensuring JSON remains valid (commas adjusted) and no other keys are changed.
🤖 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/errors/mail/index.ts`:
- Line 37: The composed error message uses the raw action key (action?:
'markAsRead' | 'markAsFlagged' | 'markAsUnflagged' | 'moveToFolder') which
produces awkward phrasing like "Error while updating mail when moveToFolder";
change the message construction to map action to a human-friendly verb phrase
(e.g., map 'markAsRead' → 'marking as read', 'moveToFolder' → 'moving to
folder') and use that mapped phrase in the error/Sentry breadcrumb text; locate
where the error string is built in src/errors/mail/index.ts (the place that
reads the action variable) and replace the inline action interpolation with a
lookup from a small constant map to produce grammatically correct messages.
In `@src/features/mail/components/mail-preview/actions-bar/index.tsx`:
- Line 13: Replace the deep import of MenuItemsType with the public export
MenuItemType: remove the import "MenuItemsType" from "@internxt/ui/dist/..." and
import "MenuItemType" from the package's public entry, then replace usages of
MenuItemsType<T> with MenuItemType<T>[] (e.g., MenuItemType<unknown>[]). Update
the import statement and type annotations where MenuItemsType is referenced (in
this file's top-level import and any variable/prop typings) to use MenuItemType
and array syntax instead.
In `@src/features/mail/MailView.tsx`:
- Around line 57-64: The type mismatch comes from passing Promise<null> (from
updateReadStatus.unwrap(), moveToFolder.unwrap(), deleteEmails.unwrap()) into
usePreviewMailActions which expects Promise<void>; fix by discarding the
returned value at the call site: change the three callbacks to use the void
operator so they return void, e.g. replace updateReadStatus: (args) =>
updateReadStatus(args).unwrap() with updateReadStatus: (args) => void
updateReadStatus(args).unwrap(), and do the same for moveToFolder and
deleteEmails; alternatively you can widen usePreviewMailActions' param types to
accept Promise<void | null> (symbol: usePreviewMailActions) if you prefer a
signature change.
- Around line 36-37: The current code uses useGetMailMessageQuery({ emailId:
activeMailId! }, { skip: !activeMailId }) and then sets activeMail via a
redundant ternary; replace this pattern with RTK Query's skipToken: call
useGetMailMessageQuery(activeMailId ?? skipToken) (or use activeMailId ?
activeMailId : skipToken) so you can remove the non-null assertion on
activeMailId, and then simply use activeMailData directly (const activeMail =
activeMailData) without the ternary; update references to
useGetMailMessageQuery, activeMailData, activeMailId, and skipToken accordingly.
In `@src/hooks/mail/useListActionContext.test.ts`:
- Around line 51-92: The tests in useListActionContext.test.ts mix
Arrange/Act/Assert on the same block for several cases; update each test that
calls renderFor(...) and then immediately invokes getItems(...)[N].action to
insert explicit blank-line separation between Arrange (setup with renderFor and
ids/params), Act (calling the action: getItems(...)[].action or bulk[0].action),
and Assert (the expect(...) calls) so they follow the AAA pattern; specifically
adjust the tests that use renderFor, getItems(result.current.listActionContext),
getItems(result.current.bulkActionContext), and assertions against
params.selectAll/selectNone/selectRead/selectUnread/deleteEmails to have a blank
line between each phase.
In `@src/hooks/mail/usePreviewMailActions.test.ts`:
- Around line 25-27: In the test file's beforeEach block (the one calling
vi.clearAllMocks()), replace vi.clearAllMocks() with vi.restoreAllMocks() so
spies created by vi.spyOn(console, 'error').mockImplementation(...) are restored
between tests; locate the beforeEach in
src/hooks/mail/usePreviewMailActions.test.ts and update the call accordingly to
vi.restoreAllMocks().
In `@src/hooks/mail/usePreviewMailActions.ts`:
- Around line 76-86: The hook returns silent no-op handlers (onReply,
onReplyAll, onForward) which leave ActionsBar buttons clickable but doing
nothing; update usePreviewMailActions so those three symbols either (a) are
omitted/replace with undefined when a feature flag is disabled so the UI can
hide the buttons, or (b) replaced with a visible placeholder handler that logs a
TODO and calls the app toast/notification API (e.g. showToast or useToast) with
a “Coming soon” message so QA/users see feedback; implement one approach in the
hook and ensure ActionsBar checks for undefined handlers or uses the toast-path
when the placeholder handler is present.
- Around line 37-48: The setReadStatus callback currently calls
clearActiveMail() on success which collapses the open preview when toggling
read/unread; remove the clearActiveMail() invocation from setReadStatus and
instead ensure clearActiveMail() is only called in the handlers that actually
remove the message from the folder (e.g., onTrash and onMove) so toggling read
state preserves the preview; update usePreviewMailActions.test.ts to reflect
that setReadStatus does not clear the active mail, or if the current behavior is
intentional add an inline comment on setReadStatus explaining why the preview
should close on read/unread toggles.
- Around line 24-27: The helper logError currently swallows action failures by
only calling console.error; update logError to call
ErrorService.instance.notifyUser(...) or notificationsService.show(...) with a
translated message instead (use translate('errors.mail.markAsRead') /
'markAsUnread' / 'trash' / 'move' depending on the action) so failures surface
to the user and keep console.error for debugging; also add the corresponding
errors.mail.{markAsRead,markAsUnread,trash,move} keys to all locale files with
proper translations so the i18n lookup succeeds.
In `@src/i18n/locales/es.json`:
- Line 9: The translation key actions.trashEmail in the locales file currently
maps to "Mover a la papelera", which is misleading when used for permanent
deletion from the trash; add a new key actions.deletePermanently (or similar) in
src/i18n/locales/es.json with an appropriate Spanish label (e.g., "Eliminar
permanentemente") and update callers to choose between actions.trashEmail and
actions.deletePermanently based on the source folder (or gate selection in the
consumer); search for uses of actions.trashEmail in the codebase (SDK wrapper
and UI components) and replace or branch those usages so the correct key is used
when the source folder === "trash".
In `@src/i18n/locales/it.json`:
- Around line 173-180: composeMessageDialog.cc and composeMessageDialog.bcc in
the Italian locale diverge in casing from other locales; make the casing
consistent across locales by either (A) changing "CC"/"CCN" here back to
"Cc"/"Bcc" to match en.json/fr.json, or (B) normalize en.json and fr.json to
uppercase "CC"/"CCN"/"Cci" across all locales; update the keys
composeMessageDialog.cc and composeMessageDialog.bcc accordingly so every locale
uses the same shorthand style.
In `@src/services/sdk/mail/index.ts`:
- Around line 87-94: Rename the service method trashEmail to deleteEmail
(preserving the signature async deleteEmail(emailId: string): Promise<void>) and
update its JSDoc to: "Deletes an email. Behavior depends on the source folder:
emails in inbox/spam are moved to trash; emails already in trash are permanently
deleted." Replace all call sites that reference trashEmail — notably the RTK
Query mutation usage in src/store/api/mail/index.ts (around the deleteMails
mutation) and any tests that import or mock the service — to call deleteEmail
instead, and update test descriptions/expectations accordingly.
In `@src/services/sdk/mail/mail.service.test.ts`:
- Around line 219-228: Update the test description string to singular to match
the test scope: change the test title in
src/services/sdk/mail/mail.service.test.ts for the test that calls
MailService.instance.trashEmail (which sets up mockMailClient.deleteEmail) from
"When trashing emails fails, then an error should be thrown" to "When trashing
an email fails, then an error should be thrown" so the description accurately
reflects the single-email scenario being tested.
- Around line 207-217: Update the mock used in the "Trashing email" test so its
deleteEmail stub returns an explicit resolved value like the other successful
tests: when spying on SdkManager.instance.getMail and creating mockMailClient,
replace deleteEmail: vi.fn() with a promise-resolving stub (e.g.,
vi.fn().mockResolvedValue(<appropriate success value or undefined>)) so
MailService.instance.trashEmail receives the same explicit resolved response
shape as getEmail/setupMailAccount/updateEmail tests.
In `@src/store/api/mail/index.ts`:
- Around line 137-145: The cache is left stale because moveToFolder and
deleteMails only invalidate ListFolder for the destination but never the Mailbox
tag; update the success paths to call
dispatch(mailApi.util.invalidateTags([...])) with both { type: 'ListFolder', id:
targetMailbox } and { type: 'Mailbox', id: targetMailbox } in moveToFolder
(inside the try after await queryFulfilled), and in deleteMails when
sourceMailbox !== 'trash' add invalidations for the trash destination: { type:
'ListFolder', id: 'trash' } and { type: 'Mailbox', id: 'trash' } (keep existing
patchEmailList.undo()/patchMailbox.undo() in the catch).
- Around line 73-76: The error action always uses 'markAsRead' even when
updateReadStatus was called to mark unread; update the UpdateMailError action
union to include 'markAsUnread' and change the catch in updateReadStatus to pick
the label dynamically (e.g., action = isRead ? 'markAsRead' : 'markAsUnread')
and pass that action into new UpdateMailError(err.message, action,
err.requestId) so the error accurately reflects the attempted operation; modify
the UpdateMailError type/constructor usage and the call site in updateReadStatus
accordingly.
- Around line 68-78: The updateReadStatus mutation currently types the mailbox
param as string which forces an unsafe cast and allows invalid values; change
the mutation signature to use mailbox: FolderType (match
moveToFolder/deleteMails) in updateReadStatus and update the onQueryStarted
parameter destructuring accordingly, then remove the mailbox as FolderType cast
used inside onQueryStarted so the cache update uses the correctly typed mailbox
directly (references: updateReadStatus, onQueryStarted, FolderType).
In `@src/store/api/mail/mail.api.test.ts`:
- Around line 244-270: The test file duplicates helper functions
setupCachedStore and getCacheState across the "Move To Folder" and "Delete
Mails" suites; hoist a single shared implementation to file scope (near
mailboxQuery) and have both suites call that shared setup, ensuring the shared
setup uses getMockedMails(3) with the same isRead pattern and mocked mailbox
override (inboxMailbox with unreadEmails: 2) and reuses MailService spies
(listFolder, getMailboxesInfo) as in the existing setup; update both suites to
import/use the hoisted setupCachedStore and getCacheState helpers so the
duplicated blocks are removed.
In `@src/utils/batch-processes/index.test.ts`:
- Around line 5-7: The test setup currently calls vi.clearAllMocks() in
beforeEach which only resets call history; replace it with vi.restoreAllMocks()
so any mocked implementations (e.g., the test that patches global Promise.all
and uses spy.mockRestore()) are properly restored between tests; update the
beforeEach to call vi.restoreAllMocks() (or add vi.restoreAllMocks() alongside
vi.clearAllMocks()) to ensure mocks created by functions in this file are fully
reset before each test.
- Around line 63-87: Add an assertion before calling resolveFirst() to prove the
second batch hasn't started: after creating promise = batchProcess(items,
handler, 1) but before resolveFirst(), assert that callLog does not contain
'item-10' (e.g., expect(callLog).not.toContain('item-10')). This uses the
existing symbols batchProcess, handler, resolveFirst and callLog to ensure the
test verifies that item-10 is not logged until after resolveFirst() is invoked.
- Around line 40-61: The test currently couples to implementation by spying on
Promise.all and using the side-channel arrays `batches`/`current`; instead,
change the assertion to observe external behavior: after calling `await
batchProcess(items, handler, 3)` assert that the `Promise.all` spy (the `spy`
created with `vi.spyOn(Promise, 'all')`) was called the expected number of times
(e.g., Math.ceil(items.length / 3)) or assert on the `handler` mock
(`handler.mock.calls.length` and/or the sequence of calls) to derive batch
count; remove the side-channel pushes to `batches`/`current` and restore `spy`
as before.
In `@src/utils/batch-processes/index.ts`:
- Around line 1-7: Rename the constant BATCH_PROCESSES to DEFAULT_BATCH_SIZE and
update the default parameter on batchProcess to use that new name; also guard
against non-positive batchSize by coercing it to at least 1 (e.g. set batchSize
= Math.max(1, batchSize) or throw a validation error) at the start of the
batchProcess function so the loop cannot spin forever when a caller passes
batchSize <= 0.
In `@src/utils/current-path/index.test.ts`:
- Around line 4-15: The test currently reimplements react-router-dom.matchPath
via vi.mock which makes isCurrentPath pass by construction; update the tests to
either (a) stop mocking matchPath and import the real matchPath from
react-router-dom so isCurrentPath is verified against the real implementation,
or (b) keep the mock but change assertions to verify that matchPath (the vi.fn)
is called with the expected arguments and that isCurrentPath returns true for
any truthy matchPath return value and false only when matchPath returns null;
ensure you still call vi.restoreAllMocks() in beforeEach so mock call history is
cleared between tests but the factory mock remains as intended.
---
Outside diff comments:
In `@src/hooks/mail/useListActionContext.test.ts`:
- Around line 33-35: Replace the vi.clearAllMocks() call in the beforeEach block
with vi.restoreAllMocks() so mocks and spy implementations are fully reset
between tests; find the beforeEach in useListActionContext.test.ts and change
the invocation from vi.clearAllMocks() to vi.restoreAllMocks() to follow
AGENTS.md convention and avoid mock leakage across tests.
In `@src/i18n/locales/en.json`:
- Around line 23-28: Remove the obsolete keys filter.starred and
filter.unstarred from the Italian and French locale objects so they match
English's filter keys (all, none, read, unread); locate the filter object in the
it.json and fr.json locale definitions and delete the starred and unstarred
entries, ensuring JSON remains valid (commas adjusted) and no other keys are
changed.
In `@src/i18n/locales/it.json`:
- Around line 23-30: Remove the stale locale keys filter.starred and
filter.unstarred from this file (it.json) or restore them in en.json so all
locales remain consistent; specifically, update the filter object (keys
"starred" and "unstarred") in it.json to match the canonical set used in en.json
and ensure the same change is applied across fr.json and es.json (or add the
missing keys to en.json) so all four locale files (en.json, es.json, fr.json,
it.json) contain the same i18n keys as required by the guidelines.
🪄 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: 630c927f-83e8-41b8-9805-597dbd7cee22
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.locksrc/assets/icons/move-to.svgis excluded by!**/*.svg
📒 Files selected for processing (23)
.husky/pre-commitpackage.jsonsrc/errors/mail/index.tssrc/features/mail/MailView.tsxsrc/features/mail/components/mail-preview/actions-bar/index.tsxsrc/hooks/mail/useListActionContext.test.tssrc/hooks/mail/useListActionContext.tssrc/hooks/mail/useListFolderPaginated.tssrc/hooks/mail/useMailSelection.tssrc/hooks/mail/usePreviewMailActions.test.tssrc/hooks/mail/usePreviewMailActions.tssrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/it.jsonsrc/services/sdk/mail/index.tssrc/services/sdk/mail/mail.service.test.tssrc/store/api/mail/index.tssrc/store/api/mail/mail.api.test.tssrc/utils/batch-processes/index.test.tssrc/utils/batch-processes/index.tssrc/utils/current-path/index.test.tssrc/utils/current-path/index.ts
Deploying mail-web with
|
| Latest commit: |
e0c5bfa
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://62daeb28.mail-web-ea0.pages.dev |
| Branch Preview URL: | https://feature-mail-actions.mail-web-ea0.pages.dev |
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/i18n/locales/en.json (1)
23-28:⚠️ Potential issue | 🟡 MinorAlign
filtersection keys across all locales.The PR removes
filter.starredandfilter.unstarredfromen.json(lines 23–28), but these keys remain ines.json,fr.json, andit.json. Remove them consistently from all four locale files to maintain parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/locales/en.json` around lines 23 - 28, Remove the now-unused "filter.starred" and "filter.unstarred" keys from the remaining locale files to match the updated English locale; specifically delete the "starred" and "unstarred" entries under the "filter" object in es.json, fr.json, and it.json so all locales have the same "filter" keys ("all","none","read","unread") as in en.json.src/features/mail/MailView.tsx (2)
80-95:⚠️ Potential issue | 🟠 Major
onSelectEmailwon't catch network errors —.unwrap()is missing.
updateReadStatus(...)returns RTK Query's mutation result which resolves with{ data }or{ error }— it never rejects. Without.unwrap(), thetry/catchhere is dead code: a failing mutation won't triggercastError/console.error. This silently regresses error reporting on selection-driven mark-as-read.🐛 Proposed fix
try { - await updateReadStatus({ + await updateReadStatus({ emailId: id, mailbox: folder, isRead: true, - }); + }).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 80 - 95, The try/catch in onSelectEmail is ineffective because updateReadStatus returns an RTK Query mutation result that doesn't throw; call .unwrap() on the promise returned by updateReadStatus(...) so the mutation will reject on error and be caught by the existing try/catch. Update the call inside onSelectEmail (the updateReadStatus invocation that passes emailId, mailbox, isRead) to await updateReadStatus(...).unwrap(), leaving ErrorService.instance.castError and the console.error handling unchanged so network/RTK errors propagate into the catch block.
29-95: 🛠️ Refactor suggestion | 🟠 Major
MailViewcontinues to mix data/state/handlers — not "dumb" per AGENTS.md.
MailViewdeclares mutations, derives data, buildspreviewActions, and owns theonSelectEmailmark-as-read flow. AGENTS.md mandates components render only and delegate state/effects/data fetching/handlers touse<Feature>hooks undersrc/hooks/. Consider consolidating this orchestration (the three mutation hooks,activeMailIdstate,onSelectEmail, and thepreviewActionswiring) into a singleuseMailView(folder)hook so the component is purely presentational.As per coding guidelines: "Components must be dumb and render only. Extract all logic (state, effects, event handlers, data fetching) into
use<Feature>hooks."🤖 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 29 - 95, MailView is doing data fetching, mutations, state and handler orchestration (useGetMailMessageQuery, useUpdateReadStatusMutation, useMoveToFolderMutation, useDeleteMailsMutation, activeMailId state, onSelectEmail logic, previewActions wiring, and useListFolderPaginated/useMailSelection/useListActionContext usage) instead of being a presentational component; extract all of that into a new hook useMailView(folder) that encapsulates the activeMailId state, calls the three mutation hooks, list/query hooks, builds previewActions and exposes handlers (onSelectEmail, clearActiveMail, updateReadStatus/moveToFolder/deleteEmails wrappers) plus derived data (activeMail, listFolderEmails, selection and action contexts, folderName, from/to/cc/bcc, unreadByMailbox, hasMoreEmails, isLoadingListFolder, onLoadMore, toggleUnreadFilter, toggleSelectAll); then refactor MailView to import useMailView and only render UI using the values and handlers the hook returns.
♻️ Duplicate comments (1)
src/utils/batch-processes/index.test.ts (1)
5-8: 🧹 Nitpick | 🔵 TrivialRedundant
vi.clearAllMocks()call.
vi.restoreAllMocks()already clears mock call history in addition to restoring original implementations, so the precedingvi.clearAllMocks()is a no-op.♻️ Suggested simplification
beforeEach(() => { - vi.clearAllMocks(); vi.restoreAllMocks(); });As per coding guidelines: "Reset mocks in beforeEach with vi.restoreAllMocks()."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/batch-processes/index.test.ts` around lines 5 - 8, Remove the redundant vi.clearAllMocks() call in the test setup: in the beforeEach block that currently calls vi.clearAllMocks() and vi.restoreAllMocks(), delete the vi.clearAllMocks() line and rely solely on vi.restoreAllMocks() to reset mocks and restore implementations; keep the existing beforeEach and any surrounding test setup intact.
🤖 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/features/mail/components/mail-preview/actions-bar/index.tsx`:
- Around line 51-73: The dropdown trigger should be disabled or hidden when
optionsDisabled is true so users can't open a menu of all-disabled items; update
the component rendering that uses Dropdown to pass optionsDisabled into the
trigger (e.g., disable the trigger button or apply a disabled class via
Dropdown's classButton prop or wrap the trigger in a disabled <button>) or
conditionally render nothing when optionsDisabled is true, and remove the need
to rely only on per-item disabled callbacks in moveToItems (keep moveToItems
as-is but ensure the parent Dropdown/trigger respects optionsDisabled so the
menu cannot be opened when no mail is active).
- Around line 26-27: The prop types for onMove and onTrash are wrong: replace
Promise<null> with Promise<void> to match the actual hook/implementation
contract (usePreviewMailActions) and the async functions that return
Promise<void>; e.g. change onMove: (folder: FolderType) => Promise<null> | void
to onMove: (folder: FolderType) => Promise<void> | void and similarly change
onTrash: () => Promise<null> | void to onTrash: () => Promise<void> | void so
the component signature matches the hook and implementations.
- Around line 42-73: Create a new hook named useActionsBar that encapsulates the
current orchestration: move the useLocation/useCallback isActive logic, the
useMemo construction of moveToItems, and the toggle title/handler logic (derived
from isRead and onToggle) into the hook; have it accept the minimal inputs
(isRead, onMove, onToggle, optionsDisabled, translate) and return { moveToItems,
toggleTitle, toggleHandler } (and any other small helpers the component needs).
Update the ActionsBar component to remove useLocation, isActive, and the
moveToItems useMemo, instead calling useActionsBar(...) to receive moveToItems,
toggleTitle, and toggleHandler and use those values for rendering so the
component becomes purely presentational. Ensure exported symbol is named
useActionsBar and preserve existing MenuItemType/handler shapes (onMove,
optionsDisabled, translate) so behavior is unchanged.
In `@src/hooks/mail/usePreviewMailActions.test.ts`:
- Around line 30-33: Remove the redundant vi.clearAllMocks() call in the
beforeEach block and keep only vi.restoreAllMocks(); ensure the beforeEach uses
vi.restoreAllMocks() to reset mocks and original implementations, and if any
tests rely on module-level vi.mock factory return values (e.g., castError or
notifyUser) re-stub those (castError, notifyUser) inside beforeEach so they
return the expected values for each test since restoreAllMocks will reset their
implementations.
- Around line 35-196: Add assertions in each failure test to verify the
user-facing notification is triggered: in the tests that call
result.current.onMarkAsRead, onMarkAsUnread, onTrash and onMove (the four
"fails, then the error is logged" cases), after asserting
ErrorService.instance.castError and console.error, also assert that
ErrorService.instance.notifyUser was called with the translated key (use
translate('errors.mail.<key>') the hook passes). Locate the tests referencing
onMarkAsRead, onMarkAsUnread, onTrash and onMove and add
expect(ErrorService.instance.notifyUser).toHaveBeenCalledWith(translate('errors.mail.<key>'))
(and ensure any necessary spies/mocks for notifyUser are set/reset in the test
setup/teardown).
In `@src/hooks/mail/usePreviewMailActions.ts`:
- Around line 35-58: notifyError currently requires both an action string and an
i18n key even though the key is always `errors.mail.${action}`; change
notifyError signature to accept only a narrowed action type (e.g.
'markAsRead'|'markAsUnread'), derive the messageKey inside notifyError via
`errors.mail.${action}` before calling translate, and update all callers (like
setReadStatus) to pass only the action and error. Ensure you update the
useCallback dependencies for notifyError and setReadStatus (notifyError no
longer depends on translate parameter externally) and keep references to
ErrorService.instance.castError, ErrorService.instance.notifyUser, activeMailId,
folder, and updateReadStatus intact.
In `@src/i18n/locales/it.json`:
- Line 13: The locales contain an unused "actions.flag" key; either delete that
key from every locale file (en, es, fr, it) to avoid stale translations, or
implement the flag feature by adding a Flag button in the ActionsBar component,
wiring a new onFlag handler through the message actions/parent component (e.g.,
MessageItem or MessageActions), and consuming i18n.t("actions.flag") for its
label; ensure the ActionsBar props and any dispatched action or callback (e.g.,
handleFlag / onFlag) are added and tested.
In `@src/store/api/mail/mail.api.test.ts`:
- Around line 272-456: Tests for moveToFolder and deleteMails are missing
assertions that the destination caches are invalidated/refreshed on success;
update the tests that call mailApi.endpoints.moveToFolder.initiate and
mailApi.endpoints.deleteMails.initiate to also assert that
mailApi.endpoints.getListFolder.select(...) for the destination mailbox and
mailApi.endpoints.getMailboxesInfo.select() reflect invalidation/refresh (i.e.,
were refetched or their data changed) after successful
MailService.instance.updateEmailStatus / trashEmail resolves; reference the
existing helpers getCacheState, setupCachedStore, and the endpoint symbols
(mailApi.endpoints.moveToFolder, mailApi.endpoints.deleteMails,
mailApi.endpoints.getListFolder, mailApi.endpoints.getMailboxesInfo) when adding
these assertions so the tests fail if destination invalidation is not performed.
---
Outside diff comments:
In `@src/features/mail/MailView.tsx`:
- Around line 80-95: The try/catch in onSelectEmail is ineffective because
updateReadStatus returns an RTK Query mutation result that doesn't throw; call
.unwrap() on the promise returned by updateReadStatus(...) so the mutation will
reject on error and be caught by the existing try/catch. Update the call inside
onSelectEmail (the updateReadStatus invocation that passes emailId, mailbox,
isRead) to await updateReadStatus(...).unwrap(), leaving
ErrorService.instance.castError and the console.error handling unchanged so
network/RTK errors propagate into the catch block.
- Around line 29-95: MailView is doing data fetching, mutations, state and
handler orchestration (useGetMailMessageQuery, useUpdateReadStatusMutation,
useMoveToFolderMutation, useDeleteMailsMutation, activeMailId state,
onSelectEmail logic, previewActions wiring, and
useListFolderPaginated/useMailSelection/useListActionContext usage) instead of
being a presentational component; extract all of that into a new hook
useMailView(folder) that encapsulates the activeMailId state, calls the three
mutation hooks, list/query hooks, builds previewActions and exposes handlers
(onSelectEmail, clearActiveMail, updateReadStatus/moveToFolder/deleteEmails
wrappers) plus derived data (activeMail, listFolderEmails, selection and action
contexts, folderName, from/to/cc/bcc, unreadByMailbox, hasMoreEmails,
isLoadingListFolder, onLoadMore, toggleUnreadFilter, toggleSelectAll); then
refactor MailView to import useMailView and only render UI using the values and
handlers the hook returns.
In `@src/i18n/locales/en.json`:
- Around line 23-28: Remove the now-unused "filter.starred" and
"filter.unstarred" keys from the remaining locale files to match the updated
English locale; specifically delete the "starred" and "unstarred" entries under
the "filter" object in es.json, fr.json, and it.json so all locales have the
same "filter" keys ("all","none","read","unread") as in en.json.
---
Duplicate comments:
In `@src/utils/batch-processes/index.test.ts`:
- Around line 5-8: Remove the redundant vi.clearAllMocks() call in the test
setup: in the beforeEach block that currently calls vi.clearAllMocks() and
vi.restoreAllMocks(), delete the vi.clearAllMocks() line and rely solely on
vi.restoreAllMocks() to reset mocks and restore implementations; keep the
existing beforeEach and any surrounding test setup intact.
🪄 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: 15c20bab-126d-40d3-9d9f-e8b855d65dc8
📒 Files selected for processing (14)
src/features/mail/MailView.tsxsrc/features/mail/components/mail-preview/actions-bar/index.tsxsrc/hooks/mail/usePreviewMailActions.test.tssrc/hooks/mail/usePreviewMailActions.tssrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/it.jsonsrc/services/sdk/mail/index.tssrc/services/sdk/mail/mail.service.test.tssrc/store/api/mail/index.tssrc/store/api/mail/mail.api.test.tssrc/utils/batch-processes/index.test.tssrc/utils/current-path/index.test.ts
|
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
1 participant
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.
Adding mail individual actions like move email, trash email, mark as read.