[PB-1974]: feature/drafts#64
Conversation
|
|
||
| const onSent = useCallback(async () => { | ||
| clearDialog(); | ||
| }, [item, draftId, mode, deleteEmails, clearDialog]); |
There was a problem hiding this comment.
all of these dependencies are unused
|
|
||
| const onSent = useCallback(async () => { | ||
| clearDialog(); | ||
| }, [item, draftId, mode, deleteEmails, clearDialog]); |
There was a problem hiding this comment.
also, deleteEmails seems unused aside from this
| export type PersistedAttachmentInput = { | ||
| blobId: string; | ||
| name: string; | ||
| size: number; | ||
| type: string; | ||
| }; | ||
|
|
There was a problem hiding this comment.
this type is defined twice, also in useAttachments.ts
Deploying mail-web with
|
| Latest commit: |
b495fca
|
| Status: | 🚫 Build failed. |
|
Warning Review limit reached
More reviews will be available in 7 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds complete draft email support including new ChangesDraft Email Compose Feature
Attachment Component Refactoring
Icon Service for File Extensions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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.
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/compose-message/index.tsx (1)
70-176: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract draft lifecycle orchestration into a dedicated hook.
This component now owns draft hydration, persisted attachment hydration, session-key recovery, autosave wiring, and close/send flow orchestration. That violates the repo’s “dumb component” rule and makes this UI path harder to test and evolve. Please move this logic into a
useComposeDraftLifecycle-style hook and keep TSX render-focused.As per coding guidelines, "Components must be dumb: move state/effects/data fetching/event handlers into
use<Name>hooks; ensure logic is testable via hooks, not TSX components."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/compose-message/index.tsx` around lines 70 - 176, Extract the draft lifecycle management logic from the component into a new custom hook called useComposeDraftLifecycle. This hook should encapsulate all the draft-related useEffect hooks (the ones managing sessionKeyHydratedRef, hydratedForwardAttachmentsRef, hydratedPersistedAttachmentsRef), the session-key recovery logic, the clearDialog callback that manages ref resets, and the onClose/onSent flow orchestration. Move all these effects, refs, and callbacks into the hook so the component TSX only focuses on rendering and minimal state management, returning the necessary callbacks and state from the hook for the component to use.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/compose-message/hooks/useAttachments.ts`:
- Line 9: The import statement for PersistedAttachmentInput is using a relative
path './useInitialComposeState' instead of the repository path alias. Replace
the relative import path with the `@/`* alias format that maps to src/*, so the
import should reference the full path from src/ using the `@/` prefix. This
ensures consistency with the repository's coding guidelines for internal module
imports.
In `@src/components/compose-message/hooks/useComposeSend.ts`:
- Around line 236-237: Remove the draftId field from the payload object passed
to the sendEmail function in the useComposeSend hook, as draftId is not part of
the SendEmailRequestDto type defined in the SDK. The field should be removed
from the object that gets passed to the sendEmail call before the unwrap()
method is invoked. If draft tracking is required, implement that functionality
separately outside of the send request payload.
In `@src/components/compose-message/hooks/useDraftMessage.ts`:
- Around line 6-7: The import statements for Recipient and AttachmentTask in the
useDraftMessage.ts hook are using relative paths instead of the repository's
path alias convention. Replace the relative import paths (../types and
./useAttachments) with the corresponding `@/`* path aliases that map to the src
directory, following the repository's coding guidelines for internal module
imports. Update both the import from '../types' and the import from
'./useAttachments' to use their absolute alias paths from the src root.
- Around line 65-72: The buildPayload function returns an encryption field that
is not defined in the DraftEmailRequest type from the SDK, creating a type
mismatch that could cause runtime validation failures. Remove the encryption
field from the return object in buildPayload, or verify with SDK maintainers
that encryption is a supported field and update the type definitions
accordingly. Additionally, update the relative import statement for Recipient
type from '../types' to use the path alias '`@/components/compose-message/types`'
instead.
In `@src/components/compose-message/index.tsx`:
- Around line 117-128: The sessionKeyHydratedRef.current flag is being set to
true before the decryption promise completes, which means if the
decryptAttachmentsSessionKey call fails, the flag remains true and subsequent
retries never happen. Move the sessionKeyHydratedRef.current = true assignment
into the .then() success callback after setAttachmentsSessionKey is called, or
alternatively reset the flag to false in the .catch() error handler to allow
future retry attempts when the effect dependencies change again.
- Around line 153-156: The onClose function in the compose-message component
silently catches errors from saveDraft() and still clears the dialog, causing
potential silent draft loss. Replace the catch block with proper error handling
that notifies the user using ErrorService.instance.notifyUser() or
notificationsService.show() with a translated error message following the
pattern translate('errors.<domain>.<key>'), and ensure clearDialog() is only
called when saveDraft() succeeds. Keep the dialog open if the save operation
fails to prevent losing unsaved changes.
In `@src/hooks/mail/useOpenDraftInCompose.ts`:
- Around line 10-13: The draft lookup in the async callback is falling back to
thread[0] when the draft ID is not found, which can mask when a draft is deleted
or moved. Modify the line that assigns the draft variable (currently using
`thread.find((m) => m.id === draftId) ?? thread[0]`) by either removing the
fallback operator and thread[0] reference entirely to rely solely on the
subsequent !draft guard check, or alternatively keep the fallback but add a
console warning or logger call when the fallback is used to indicate that the
intended draft was not found and a fallback message is being opened instead.
In `@src/services/sdk/mail/index.ts`:
- Line 16: The import of UpdateDraftResponse on line 16 does not exist in the
current SDK version 1.17.7, causing a compilation error. Either upgrade the
`@internxt/sdk` dependency to a version that exports UpdateDraftResponse, or
remove this import and refactor the updateDraft() and getDraft() methods to use
return types and SDK method calls that are actually available in version 1.17.7.
Verify that all SDK method calls (this.client.updateDraft(),
this.client.getDraft()) correspond to actual methods exported by the installed
SDK version.
In `@src/store/api/mail/index.ts`:
- Around line 326-327: The sendEmail cache invalidation is too broad and
invalidates all ListFolder caches when only the sent and drafts folders should
be refreshed. Modify the invalidatesTags function in the sendEmail endpoint to
return a more targeted array that includes ListFolder invalidations only for the
specific folder IDs 'sent' (to display the newly sent email) and 'drafts' (to
remove the sent draft), rather than invalidating the entire ListFolder type
without an ID, which forces unnecessary refetches of all folders.
- Line 34: The import statement references `UpdateDraftResponse` which does not
exist in the SDK (v1.17.7), and the corresponding `updateDraft()` method call
does not exist on the MailApi either. Replace the `UpdateDraftResponse` import
with `EmailCreatedResponse` which is the actual type exported by the SDK, and
update the method call from `updateDraft()` to `saveDraft()` throughout the file
(including in src/services/sdk/mail/index.ts at line 160 where the SDK client
method is called). This aligns the code with the actual SDK API that exports
saveDraft() returning EmailCreatedResponse.
---
Outside diff comments:
In `@src/components/compose-message/index.tsx`:
- Around line 70-176: Extract the draft lifecycle management logic from the
component into a new custom hook called useComposeDraftLifecycle. This hook
should encapsulate all the draft-related useEffect hooks (the ones managing
sessionKeyHydratedRef, hydratedForwardAttachmentsRef,
hydratedPersistedAttachmentsRef), the session-key recovery logic, the
clearDialog callback that manages ref resets, and the onClose/onSent flow
orchestration. Move all these effects, refs, and callbacks into the hook so the
component TSX only focuses on rendering and minimal state management, returning
the necessary callbacks and state from the hook for the component to use.
🪄 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: d558dbc3-3e31-47aa-8c2e-9b337fedb42a
📒 Files selected for processing (14)
src/components/compose-message/hooks/useAttachments.test.tssrc/components/compose-message/hooks/useAttachments.tssrc/components/compose-message/hooks/useComposeSend.test.tssrc/components/compose-message/hooks/useComposeSend.tssrc/components/compose-message/hooks/useDraftMessage.test.tssrc/components/compose-message/hooks/useDraftMessage.tssrc/components/compose-message/hooks/useInitialComposeState.test.tssrc/components/compose-message/hooks/useInitialComposeState.tssrc/components/compose-message/index.tsxsrc/errors/mail/index.tssrc/features/mail/MailView.tsxsrc/hooks/mail/useOpenDraftInCompose.tssrc/services/sdk/mail/index.tssrc/store/api/mail/index.ts
| draftId, | ||
| }).unwrap(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -e ts -e tsx "useComposeSend" --type fRepository: internxt/mail-web
Length of output: 176
🏁 Script executed:
rg "SendEmailRequest" --type ts -A 5 -B 2Repository: internxt/mail-web
Length of output: 3591
🏁 Script executed:
cat -n src/components/compose-message/hooks/useComposeSend.ts | sed -n '230,245p'Repository: internxt/mail-web
Length of output: 654
🏁 Script executed:
fd -path "*/node_modules/@internxt/sdk*" "types*" --type f 2>/dev/null | head -20Repository: internxt/mail-web
Length of output: 43
🏁 Script executed:
rg "type SendEmailRequest|interface SendEmailRequest" --type ts -A 15Repository: internxt/mail-web
Length of output: 43
🏁 Script executed:
find . -path "*/node_modules/@internxt/sdk/dist/mail*" -name "*.d.ts" 2>/dev/null | head -10Repository: internxt/mail-web
Length of output: 212
🏁 Script executed:
cat ./node_modules/@internxt/sdk/dist/mail/types.d.ts | grep -A 25 "SendEmailRequest"Repository: internxt/mail-web
Length of output: 2133
🏁 Script executed:
cat ./node_modules/@internxt/sdk/dist/mail/schema.d.ts | grep -A 30 "SendEmailRequestDto"Repository: internxt/mail-web
Length of output: 2251
Remove draftId from the sendEmail payload.
The draftId field is not part of the SendEmailRequest type (defined as SendEmailRequestDto in the SDK). Passing it will cause a TypeScript compilation error. If draft tracking is needed, this should be handled separately outside the send request payload.
🧰 Tools
🪛 GitHub Actions: Mail Web CI / 0_ci (24.x).txt
[error] 236-236: TypeScript error TS2353: Object literal may only specify known properties, and 'draftId' does not exist in type '{ to: { name?: string | undefined; email: string; }[]; cc?: { name?: string | undefined; email: string; }[] | undefined; bcc?: { name?: string | undefined; email: string; }[] | undefined; subject: string; ... 5 more ...; inReplyToEmailId?: string | undefined; }'.
🪛 GitHub Actions: Mail Web CI / ci (24.x)
[error] 236-236: TypeScript (TS2353) build failed. Object literal may only specify known properties; 'draftId' does not exist in the expected type for the object passed in this location.
🪛 GitHub Check: ci (24.x)
[failure] 236-236:
Object literal may only specify known properties, and 'draftId' does not exist in type '{ to: { name?: string | undefined; email: string; }[]; cc?: { name?: string | undefined; email: string; }[] | undefined; bcc?: { name?: string | undefined; email: string; }[] | undefined; subject: string; ... 5 more ...; inReplyToEmailId?: string | undefined; }'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/compose-message/hooks/useComposeSend.ts` around lines 236 -
237, Remove the draftId field from the payload object passed to the sendEmail
function in the useComposeSend hook, as draftId is not part of the
SendEmailRequestDto type defined in the SDK. The field should be removed from
the object that gets passed to the sendEmail call before the unwrap() method is
invoked. If draft tracking is required, implement that functionality separately
outside of the send request payload.
Source: Linters/SAST tools
| return { | ||
| to: toRecipients.length ? toRecipients.map(toEmailAddress) : undefined, | ||
| cc: ccRecipients.length ? ccRecipients.map(toEmailAddress) : undefined, | ||
| bcc: bccRecipients.length ? bccRecipients.map(toEmailAddress) : undefined, | ||
| subject: subject || undefined, | ||
| encryption, | ||
| attachments: doneAttachments.length ? doneAttachments : undefined, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/compose-message/hooks/useDraftMessage.tsRepository: internxt/mail-web
Length of output: 4895
🏁 Script executed:
find . -type f -name "*.d.ts" -o -name "*.ts" | xargs grep -l "DraftEmailRequest" | head -20Repository: internxt/mail-web
Length of output: 333
🏁 Script executed:
rg "DraftEmailRequest" --type ts --type tsx -A 5 -B 2Repository: internxt/mail-web
Length of output: 89
🏁 Script executed:
rg "DraftEmailRequest" -A 8 -B 2Repository: internxt/mail-web
Length of output: 6873
🏁 Script executed:
cat node_modules/@internxt/sdk/dist/mail/types.d.ts | grep -A 15 "DraftEmailRequest"Repository: internxt/mail-web
Length of output: 1174
🏁 Script executed:
cat node_modules/@internxt/sdk/dist/mail/schema.d.ts | grep -A 25 "DraftEmailRequestDto"Repository: internxt/mail-web
Length of output: 1799
🏁 Script executed:
npm run lint 2>&1 | grep -i "encryption\|draft" | head -30Repository: internxt/mail-web
Length of output: 118
🏁 Script executed:
npx tsc --noEmit 2>&1 | grep -i "encryption\|useDraftMessage" | head -50Repository: internxt/mail-web
Length of output: 43
🏁 Script executed:
cat node_modules/@internxt/sdk/dist/mail/types.d.ts | head -100Repository: internxt/mail-web
Length of output: 3754
🏁 Script executed:
head -10 src/components/compose-message/types.tsRepository: internxt/mail-web
Length of output: 160
🏁 Script executed:
head -10 src/components/compose-message/hooks/useAttachments.tsRepository: internxt/mail-web
Length of output: 626
🏁 Script executed:
ls -la src/components/compose-message/Repository: internxt/mail-web
Length of output: 587
🏁 Script executed:
rg "`@internxt/sdk`" node_modules/@internxt/sdk/package.json -A 3Repository: internxt/mail-web
Length of output: 224
🏁 Script executed:
cat src/tsconfig.json 2>/dev/null || cat tsconfig.json | grep -A 20 "compilerOptions"Repository: internxt/mail-web
Length of output: 43
🏁 Script executed:
cat src/components/compose-message/types/index.ts 2>/dev/null || cat src/components/compose-message/types.ts 2>/dev/null || find src/components/compose-message -name "*.ts" -o -name "*.tsx" | xargs ls -laRepository: internxt/mail-web
Length of output: 436
🏁 Script executed:
grep -n "import.*\.\." src/components/compose-message/hooks/useDraftMessage.tsRepository: internxt/mail-web
Length of output: 106
🏁 Script executed:
cat node_modules/@internxt/sdk/dist/mail/schema.d.ts | grep -B 5 -A 30 "DraftEmailRequestDto:"Repository: internxt/mail-web
Length of output: 1303
🏁 Script executed:
cat tsconfig.json | jq '.compilerOptions | {skipLibCheck, strict, noImplicitAny}' 2>/dev/null || grep -A 30 "compilerOptions" tsconfig.jsonRepository: internxt/mail-web
Length of output: 131
🏁 Script executed:
cat src/components/compose-message/hooks/useDraftMessage.ts | sed -n '49,73p'Repository: internxt/mail-web
Length of output: 1274
🏁 Script executed:
rg "buildEncryptionBlock" -A 5 -B 2 --max-count=3Repository: internxt/mail-web
Length of output: 8961
Payload includes encryption field not present in SDK DraftEmailRequest type.
The buildPayload function returns an object with an encryption field (line 70), but DraftEmailRequest from @internxt/sdk only defines: to, cc, bcc, subject, textBody, htmlBody, and attachments. TypeScript currently does not catch this as an error, but the payload schema is misaligned with the SDK contract and will likely cause runtime validation failures on the server.
Fix: Either remove encryption from the draft payload (if drafts should not store encryption metadata), or verify with the SDK maintainers that encryption is a supported field and update the type definitions accordingly.
Minor: Use path alias for relative import on line 6.
Replace import type { Recipient } from '../types' with import type { Recipient } from '@/components/compose-message/types'.
🧰 Tools
🪛 GitHub Check: ci (24.x)
[failure] 70-70:
Object literal may only specify known properties, and 'encryption' does not exist in type '{ to?: { name?: string | undefined; email: string; }[] | undefined; cc?: { name?: string | undefined; email: string; }[] | undefined; bcc?: { name?: string | undefined; email: string; }[] | undefined; subject?: string | undefined; textBody?: string | undefined; htmlBody?: string | undefined; attachments?: { ...; }[]...'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/compose-message/hooks/useDraftMessage.ts` around lines 65 -
72, The buildPayload function returns an encryption field that is not defined in
the DraftEmailRequest type from the SDK, creating a type mismatch that could
cause runtime validation failures. Remove the encryption field from the return
object in buildPayload, or verify with SDK maintainers that encryption is a
supported field and update the type definitions accordingly. Additionally,
update the relative import statement for Recipient type from '../types' to use
the path alias '`@/components/compose-message/types`' instead.
Source: Linters/SAST tools
[PB-6519]: feat/attachments design
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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/compose-message/hooks/useDraftMessage.ts (1)
102-113:⚠️ Potential issue | 🟠 MajorAutosave does not trigger on editor body changes.
The
editorin the dependency array is a stable TipTapEditorinstance whose reference never changes. When users type in the message body, no dependency changes occur, so the autosave effect won't re-run until recipients, subject, or attachments change.TipTap's
editor.on('transaction')event is available and already used elsewhere in the codebase (e.g.,useEditorBar.ts). Subscribe to this event to track body changes and trigger autosave on edit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/compose-message/hooks/useDraftMessage.ts` around lines 102 - 113, The autosave effect does not trigger when editor body changes because the `editor` reference in the dependency array is stable and never changes when content is edited. Within the useEffect that sets up the autosave timer, subscribe to the TipTap editor's `editor.on('transaction')` event to detect when the editor content changes, and trigger the autosave timer reset on each transaction. Remove `editor` from the dependency array and instead rely on the transaction event subscription to drive autosave behavior, similar to how it is implemented in `useEditorBar.ts`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/attachment-chip/index.tsx`:
- Around line 41-55: The button elements in this component are missing the
explicit type attribute, which causes them to default to type="submit" when used
within a form context, potentially triggering unintended form submissions. Add
type="button" to all button elements in this component, specifically the retry
upload button with onClick={onRetry} and the download attachment button with
onClick={onDownload}, as well as any other buttons referenced in the "Also
applies to" section (lines 69-72), to explicitly prevent form submission
behavior.
- Around line 42-43: The aria-label attributes in the attachment-chip component
contain hardcoded English strings like "Retry upload", "Download attachment",
and "Remove attachment" instead of using i18n translations. Replace each
hardcoded aria-label string with a call to translate() using appropriate i18n
keys. Create new i18n key entries in the locale files (src/i18n/locales/en.json,
es.json, fr.json, it.json) for each aria-label string, then update the
aria-label attributes to reference these keys through the translate function.
- Around line 68-72: The remove button in the attachment-chip component is only
visible on hover due to the className containing `hidden group-hover:flex`,
making it inaccessible to keyboard users. Modify the className of the button
element to be visible not only on group-hover but also on focus-visible or
focus-within states so that keyboard navigation can reach and interact with the
remove control. This ensures the button is discoverable for both pointer and
keyboard users while maintaining the hover behavior for mouse users.
In `@src/services/icons/file-extension.service.ts`:
- Around line 1-31: Create a test file named file-extension.service.test.ts in
the same directory as file-extension.service.ts to provide test coverage for the
service logic. The test file should follow the AAA (Arrange-Act-Assert) pattern
and cover multiple scenarios: testing computeExtensionsLists with no filter
argument (happy path), testing computeExtensionsLists with a filter applied,
testing computeExtensionsList function directly with various filter conditions,
testing edge cases such as empty filters and filtering that excludes all
extensions, and verifying that the FileExtensionGroup enum is iterated correctly
and all groups are processed. Include assertions to verify that the computed
extension lists contain the correct extensions and that filtering works as
expected.
- Around line 27-31: The fileExtensionService is currently exported as a plain
object instead of following the singleton pattern required per AGENTS.md.
Convert the service to a class named FileExtensionService that contains the
computeExtensionsLists method, add a static readonly instance property that
instantiates the class, and change the export to export the FileExtensionService
class instead of the fileExtensionService object. Update all callers to use
FileExtensionService.instance.computeExtensionsLists() instead of
fileExtensionService.computeExtensionsLists().
- Around line 3-18: Add JSDoc documentation to the public computeExtensionsLists
function to comply with AGENTS.md guidelines. Create a JSDoc block above the
function that describes what the function does (computing extension lists for
different file extension groups), explains the filter parameter (its type as an
object mapping FileExtensionGroup to string arrays, and its purpose/default
value), and documents the return type and what the returned Record contains.
Ensure the documentation clarifies the purpose of the function and how callers
should use it.
- Around line 8-14: The numeric enum FileExtensionGroup creates dual key mapping
which causes TypeScript type errors when indexing with string values after
filtering. Replace Object.values() with Object.keys() to iterate directly over
the enum keys, then cast each key using as keyof typeof FileExtensionGroup
before indexing into the enum. This allows you to safely extract the enum value
and use it for subsequent indexing operations in the extensionsLists and filter
assignments, maintaining proper type safety.
In `@src/services/icons/icon.service.ts`:
- Line 48: Fix the TypeScript enum indexing error at the assignment `groupId =
FileExtensionGroup[key]` by applying a type cast or refactoring approach (use
the same solution as applied in file-extension.service.ts for the
FileExtensionGroup reverse-key issue). Additionally, add a clarifying comment
above the line `const XmlFile = CodeFile;` explaining the design choice for why
XmlFile is aliased to CodeFile rather than having its own icon definition.
- Around line 1-62: Create a new test file icon.service.test.ts that tests the
icon selection logic in the getItemIcon function. The tests should follow the
AAA (Arrange, Act, Assert) pattern and cover: all FileExtensionGroup entries by
verifying that each extension group returns the correct icon component from
iconsByFileExtensionGroup, folder handling by confirming that isFolder true
returns LightFolder regardless of extension, edge cases including undefined
extension (should return DefaultFile), non-matching extensions (should return
DefaultFile), and case-insensitive extension matching (uppercase and mixed case
should match). Additionally, verify consistency with the file-extension.service
by testing that the extension matching logic works correctly with the
extensionsList computed from fileExtensionService.computeExtensionsLists().
- Around line 58-62: The iconService is currently exported as a plain object
instead of following the required singleton pattern. Refactor by creating an
IconService class with the getItemIcon method as an instance method, add a
static readonly instance property that instantiates the class, and update the
default export to export the IconService class (or the instance). This ensures
consistency with the singleton pattern required by AGENTS.md and allows callers
to use either IconService.instance.getItemIcon or import the instance directly
depending on how other parts of the codebase are configured.
- Around line 40-56: Add JSDoc documentation to the getItemIcon function to
clarify that the itemExtension parameter receives MIME type subparts (e.g.,
'svg+xml' from 'image/svg+xml', 'plain' from 'text/plain') rather than
traditional file extensions. Document that not all MIME type subparts map
cleanly to entries in the extensionsList (such as 'plain' not matching text
extensions or 'svg+xml' not matching 'svg'), and explicitly note that when no
matching extension group is found, the function falls back to
FileExtensionGroup.Default and returns the corresponding icon.
- Around line 44-52: The extension-matching logic in the loop over
Object.entries(extensionsList) uses .every() with side effects and inverted
logic (return !matched), which is non-idiomatic. Replace this with .find()
instead, which will return the first matching entry where
list.includes(itemExtension.toLowerCase()) is true. Once a match is found via
.find(), extract the key from that entry and set groupId =
FileExtensionGroup[key]. This approach is clearer and more semantically correct
for finding a single matching extension group rather than checking all elements.
In `@src/services/icons/types/index.ts`:
- Line 117: The extension key 'ogg' is mapped in both audioExtensions and
videoExtensions object mappings, which causes non-deterministic icon selection
since Object.entries iteration order is not guaranteed. Remove the duplicate
'ogg' key from one of the mappings (either audioExtensions or videoExtensions)
and rename it to be unique (e.g., 'ogg_audio' or 'ogg_video') to disambiguate
which format the extension maps to, ensuring consistent and predictable icon
selection based on the file type context.
---
Outside diff comments:
In `@src/components/compose-message/hooks/useDraftMessage.ts`:
- Around line 102-113: The autosave effect does not trigger when editor body
changes because the `editor` reference in the dependency array is stable and
never changes when content is edited. Within the useEffect that sets up the
autosave timer, subscribe to the TipTap editor's `editor.on('transaction')`
event to detect when the editor content changes, and trigger the autosave timer
reset on each transaction. Remove `editor` from the dependency array and instead
rely on the transaction event subscription to drive autosave behavior, similar
to how it is implemented in `useEditorBar.ts`.
🪄 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: da4e26ba-a839-4745-84d8-406c500fdb10
⛔ Files ignored due to path filters (20)
src/assets/icons/file-types/audio.svgis excluded by!**/*.svgsrc/assets/icons/file-types/code.svgis excluded by!**/*.svgsrc/assets/icons/file-types/csv.svgis excluded by!**/*.svgsrc/assets/icons/file-types/default.svgis excluded by!**/*.svgsrc/assets/icons/file-types/drive.svgis excluded by!**/*.svgsrc/assets/icons/file-types/excel.svgis excluded by!**/*.svgsrc/assets/icons/file-types/figma.svgis excluded by!**/*.svgsrc/assets/icons/file-types/folder.svgis excluded by!**/*.svgsrc/assets/icons/file-types/illustrator.svgis excluded by!**/*.svgsrc/assets/icons/file-types/image.svgis excluded by!**/*.svgsrc/assets/icons/file-types/indesign.svgis excluded by!**/*.svgsrc/assets/icons/file-types/pdf.svgis excluded by!**/*.svgsrc/assets/icons/file-types/photoshop.svgis excluded by!**/*.svgsrc/assets/icons/file-types/powerpoint.svgis excluded by!**/*.svgsrc/assets/icons/file-types/ppt.svgis excluded by!**/*.svgsrc/assets/icons/file-types/sketch.svgis excluded by!**/*.svgsrc/assets/icons/file-types/txt.svgis excluded by!**/*.svgsrc/assets/icons/file-types/video.svgis excluded by!**/*.svgsrc/assets/icons/file-types/word.svgis excluded by!**/*.svgsrc/assets/icons/file-types/zip.svgis excluded by!**/*.svg
📒 Files selected for processing (8)
src/components/attachment-chip/index.tsxsrc/components/compose-message/components/AttachmentList.tsxsrc/components/compose-message/hooks/useDraftMessage.tssrc/components/compose-message/index.tsxsrc/features/mail/components/mail-preview/preview/index.tsxsrc/services/icons/file-extension.service.tssrc/services/icons/icon.service.tssrc/services/icons/types/index.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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/store/api/mail/index.ts (1)
343-354:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid refetching drafts list on every autosave update.
updateDraftcurrently invalidatesListFolder:draftson each call. With debounced autosave, this can cause repeated background refetch churn and unnecessary load.Suggested change
updateDraft: builder.mutation<EmailResponse, { draftId: string; payload: DraftEmailRequest }>({ @@ - invalidatesTags: [{ type: 'ListFolder', id: 'drafts' }], + // Autosave path: avoid forcing a drafts refetch on every keystroke-triggered update. + invalidatesTags: [], }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/api/mail/index.ts` around lines 343 - 354, The updateDraft mutation is invalidating the ListFolder:drafts tag on every call, which causes unnecessary refetch churn when autosave is debounced and triggers multiple updates. Remove the invalidatesTags array from the updateDraft mutation definition to prevent automatic invalidation of the drafts list on each draft update, since updating an individual draft does not require refreshing the entire drafts folder list.src/hooks/mail/useOpenDraftInCompose.ts (1)
5-21:⚠️ Potential issue | 🟠 MajorAdd co-located tests for this logic hook.
This hook contains async logic and side effects but lacks a paired
useOpenDraftInCompose.test.ts. Add test coverage following AAA pattern, including success path (thread found, dialog opened) and missing-draft path (returns early), withvi.restoreAllMocks()inbeforeEach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/mail/useOpenDraftInCompose.ts` around lines 5 - 21, Create a new test file `useOpenDraftInCompose.test.ts` co-located with the hook to add test coverage for the async logic and side effects. Add tests following the AAA pattern that cover the success path (thread fetched, draft found, and openDialog called with correct parameters including ActionDialog.ComposeMessage and draft data) and the missing-draft path (when thread is fetched but draft is not found, function returns early without calling openDialog). Mock the dependencies useLazyGetThreadQuery and useActionDialog hooks, and include vi.restoreAllMocks() in a beforeEach hook to clean up mocks between test cases.Source: Coding guidelines
src/components/compose-message/index.tsx (1)
117-124: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMove draft orchestration out of the TSX component into a hook.
This block adds state/effects/event handlers directly in the component, which increases coupling and makes the dialog non-dumb. Extract the draft lifecycle (hydrate persisted attachments, recover session key, save/close/discard handlers) into a dedicated hook and pass only props/callbacks to JSX.
♻️ Suggested direction
- const hydratedPersistedAttachmentsRef = useRef(false); - useEffect(() => { ... }, [mode, item.persistedAttachments, addPersistedAttachments]); - useEffect(() => { ...decryptAttachmentsSessionKey... }, [mode, composeDialogData]); - const onClose = useCallback(async () => { ... }, [...]); - const onDiscardDraft = useCallback(async () => { ... }, [...]); + const { + onClose, + onDiscardDraft, + draftSavedAt, + isDiscarding, + } = useComposeDraftLifecycle({ + mode, + item, + composeDialogData, + attachments, + recipients: { toRecipients, ccRecipients, bccRecipients }, + subject: subjectValue, + editor, + closeDialog, + attachmentsSessionKey, + setAttachmentsSessionKey, + });As per coding guidelines,
src/{features,components}/**/*.tsxcomponents must be dumb and render-only, with logic extracted to hooks.Also applies to: 126-143, 158-189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/compose-message/index.tsx` around lines 117 - 124, Extract the draft lifecycle management logic from the compose-message component into a dedicated custom hook. Move the useEffect block that handles persisted attachments hydration (lines 117-124), along with the session key recovery logic (lines 126-143) and the save/close/discard event handlers (lines 158-189) into a new hook (e.g., useDraftLifecycle or useDraftOrchestration). This hook should manage all state updates and side effects internally and return only the necessary callbacks and values needed by the component JSX. Replace the direct state declarations and useEffect calls in the TSX with a single call to this new hook, keeping the component focused solely on rendering UI based on the props and callbacks provided by the hook.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/compose-message/index.tsx`:
- Around line 301-303: The discard draft button (with onDiscardDraft handler) at
line 301 is only disabled when isDiscarding is true, but it should also be
disabled while a send operation is in progress to prevent concurrent mutations
on the same draft. Add an additional condition to the disabled prop of the
Button with variant="destructive" to disable it when the send operation is
active (likely controlled by a state variable like isSending based on the
similar pattern at line 329). This ensures the discard action cannot be
triggered while the draft is being sent.
- Line 297: The className attribute in the flex container div contains an
invalid Tailwind utility class token with a space: "items center". Replace this
with the correct hyphenated class name "items-center" to properly enable
vertical centering in the flexbox layout. The fix involves changing the
className string from "flex flex-row justify-between items center" to include
the correct "items-center" token.
---
Outside diff comments:
In `@src/components/compose-message/index.tsx`:
- Around line 117-124: Extract the draft lifecycle management logic from the
compose-message component into a dedicated custom hook. Move the useEffect block
that handles persisted attachments hydration (lines 117-124), along with the
session key recovery logic (lines 126-143) and the save/close/discard event
handlers (lines 158-189) into a new hook (e.g., useDraftLifecycle or
useDraftOrchestration). This hook should manage all state updates and side
effects internally and return only the necessary callbacks and values needed by
the component JSX. Replace the direct state declarations and useEffect calls in
the TSX with a single call to this new hook, keeping the component focused
solely on rendering UI based on the props and callbacks provided by the hook.
In `@src/hooks/mail/useOpenDraftInCompose.ts`:
- Around line 5-21: Create a new test file `useOpenDraftInCompose.test.ts`
co-located with the hook to add test coverage for the async logic and side
effects. Add tests following the AAA pattern that cover the success path (thread
fetched, draft found, and openDialog called with correct parameters including
ActionDialog.ComposeMessage and draft data) and the missing-draft path (when
thread is fetched but draft is not found, function returns early without calling
openDialog). Mock the dependencies useLazyGetThreadQuery and useActionDialog
hooks, and include vi.restoreAllMocks() in a beforeEach hook to clean up mocks
between test cases.
In `@src/store/api/mail/index.ts`:
- Around line 343-354: The updateDraft mutation is invalidating the
ListFolder:drafts tag on every call, which causes unnecessary refetch churn when
autosave is debounced and triggers multiple updates. Remove the invalidatesTags
array from the updateDraft mutation definition to prevent automatic invalidation
of the drafts list on each draft update, since updating an individual draft does
not require refreshing the entire drafts folder list.
🪄 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: d88575a4-870b-410c-8d3e-283279687f9e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
package.jsonsrc/components/compose-message/hooks/useDraftMessage.test.tssrc/components/compose-message/hooks/useDraftMessage.tssrc/components/compose-message/hooks/useInitialComposeState.tssrc/components/compose-message/index.tsxsrc/hooks/mail/useOpenDraftInCompose.tssrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/it.jsonsrc/services/sdk/mail/index.tssrc/store/api/mail/index.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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/compose-message/index.tsx (1)
126-142: 🗄️ Data Integrity & Integration | 🟠 MajorAdd keys to effect dependencies or use a reactive hook to ensure session-key recovery retries when keys become available.
At line 130, if
getCurrentKeys()returns null when the effect first runs, the effect exits and never retries. Since the effect depends only on[mode, composeDialogData], if keys load intoMailKeysServicelater, the effect won't re-run. This leaves draft attachments with an incorrectattachmentsSessionKey, mismatched from the persisted encryption context.Either depend the effect on a reactive key source (e.g., extract to a hook or observe MailKeysService state), or add a guard at the dialog level to prevent opening before keys are decrypted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/compose-message/index.tsx` around lines 126 - 142, The useEffect hook that calls MailEncryptionService.instance.decryptAttachmentsSessionKey exits early if MailKeysService.instance.getCurrentKeys() returns null on the first run, and never retries since the dependency array only includes mode and composeDialogData. To fix this, either extract a reactive hook or mechanism to observe MailKeysService key availability and add that as a dependency to the useEffect so it re-runs when keys become available, or add a guard at the dialog/compose level to prevent opening the compose dialog before keys are loaded. This ensures sessionKeyHydratedRef and attachmentsSessionKey are properly set even if keys load after the component mounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/mail/useOpenDraftInCompose.ts`:
- Around line 16-17: When the draft is not found in the fetched thread (in the
`if (!draft) return;` block), the function returns without notifying the user.
Before the return statement, add a user feedback notification using the same
translated open-failure message that is used for error cases in this hook. This
ensures the user is informed when their requested draft cannot be opened rather
than experiencing silent failure.
---
Outside diff comments:
In `@src/components/compose-message/index.tsx`:
- Around line 126-142: The useEffect hook that calls
MailEncryptionService.instance.decryptAttachmentsSessionKey exits early if
MailKeysService.instance.getCurrentKeys() returns null on the first run, and
never retries since the dependency array only includes mode and
composeDialogData. To fix this, either extract a reactive hook or mechanism to
observe MailKeysService key availability and add that as a dependency to the
useEffect so it re-runs when keys become available, or add a guard at the
dialog/compose level to prevent opening the compose dialog before keys are
loaded. This ensures sessionKeyHydratedRef and attachmentsSessionKey are
properly set even if keys load after the component mounts.
🪄 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: 0ff69f26-a590-466c-99d0-209b8ab8befe
📒 Files selected for processing (13)
src/components/compose-message/components/AttachmentList.tsxsrc/components/compose-message/hooks/useDraftMessage.test.tssrc/components/compose-message/hooks/useDraftMessage.tssrc/components/compose-message/index.tsxsrc/hooks/mail/useOpenDraftInCompose.tssrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/it.jsonsrc/services/icons/file-extension.service.test.tssrc/services/icons/file-extension.service.tssrc/services/icons/icon.service.test.tssrc/services/icons/icon.service.ts
|
2 participants
Implementing Drafted Emails. Adding the design + logic to Save a Draft, Update a Draft and Send a Draft and the flow after Sending a draft - removing the Draft and force-fetching the folders so we can get the "Sent" email.