[PB-6431]: feat/implement RecipientKeysService for managing public keys and add lookup functionality in MailService#54
Merged
Conversation
…lic keys and add lookup functionality in MailService
Deploying mail-web with
|
| Latest commit: |
b6ddff7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7c64af0f.mail-web-ea0.pages.dev |
| Branch Preview URL: | https://feat-fetch-recipient-public.mail-web-ea0.pages.dev |
…sageDialog with subject handling
…nce encryption handling
…ooks to validate email decryption logic
jzunigax2
changed the title
jzunigax2
force-pushed
the
feat/fetch-recipient-public-keys
branch
from
2f4efb7 to
90badcb
Compare
…lic keys and add lookup functionality in MailService
jzunigax2
force-pushed
the
feat/fetch-recipient-public-keys
branch
from
90badcb to
1df2887
Compare
xabg2
reviewed
May 28, 2026
Comment on lines
+36
to
+46
| (async () => { | ||
| try { | ||
| const envelope = parseEncryptionBlock(mail.textBody as string); | ||
| const text = await decryptEnvelope(envelope, senderKeys); | ||
| if (!cancelled) { | ||
| setCached({ mailId: mail.id, ok: true, text }); | ||
| } | ||
| } catch { | ||
| if (!cancelled) setCached({ mailId: mail.id, ok: false }); | ||
| } | ||
| })(); |
Contributor
There was a problem hiding this comment.
Extract that to a function with a well described name.
| pending.forEach((s) => attempted.current.add(s.id)); | ||
|
|
||
| let cancelled = false; | ||
| (async () => { |
| import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'; | ||
| import { RecipientKeysService } from '.'; | ||
|
|
||
| describe('RecipientKeysService', () => { |
Contributor
There was a problem hiding this comment.
Remember how we write the tests: When..., then....
| return unique; | ||
| } | ||
|
|
||
| export function classifyRecipients( |
| try { | ||
| resolved[summary.id] = await decryptSummaryPreview(summary.encryption!, keypair); | ||
| } catch { | ||
| // Not decryptable |
Contributor
There was a problem hiding this comment.
Would be better to add a console.error so we can debug it better (for example, a user record a Jam and send it to us).
…seDecryptedPreviews hooks for improved readability and error handling
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughImplements end-to-end mail encryption: envelope build/parse/decrypt, recipient key caching, decryption hooks for bodies/previews, SDK + RTK endpoints for domains/keys/sending, compose send hook and UI wiring, utilities/errors/i18n, and tests. ChangesEnd-to-End Mail Encryption
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/components/compose-message/hooks/useComposeSend.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/components/compose-message/hooks/useComposeSend.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/services/mail-encryption/index.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 |
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: 20
🤖 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 60-79: Extract the compose send/encryption orchestration into a
new useComposeSend hook: move data fetching calls (useGetActiveDomainsQuery,
useGetMailAccountKeysQuery), the lazy lookup trigger
(useLazyLookupRecipientKeysQuery), the send mutation (useSendEmailMutation), the
allRecipients useMemo and the encryptionState useMemo (which calls
classifyRecipients) plus the send/encrypt orchestration logic currently in this
component (also present around lines 85-153) into that hook; expose from
useComposeSend the derived values (activeDomains, senderKeys, encryptionState,
isSending), the triggerLookup/sendEmail functions, and any handlers needed by
the component so the .tsx only renders and wires callbacks to these returned
values/handlers. Ensure you update references in the component to consume the
hook and remove the moved imports/logic.
- Around line 70-79: The current useMemo for encryptionState silently downgrades
to 'cleartext' when activeDomains or sender keys are unavailable, which can
allow sending unencrypted; change the logic in the encryptionState computation
(the useMemo that uses classifyRecipients with allRecipients.map and
activeDomains) to return 'none' (or a distinct blocked state) whenever
activeDomains or required sender keys are missing instead of falling back to
'cleartext', and ensure the send handler (e.g., handleSend/onSend) checks
encryptionState and blocks the send with a translated warning UI if the state is
not 'encrypted'; apply the same change to the other branch mentioned (lines
106-132) so classifyRecipients is only used when prerequisites are present and
no silent downgrade happens.
In `@src/hooks/mail/useDecryptedMail.test.tsx`:
- Around line 5-6: Update the test's internal imports to use the repo path alias
instead of relative paths: replace the current imports of useDecryptedMail and
useMailKeys with their aliased forms (e.g., import { useDecryptedMail } from
'`@/hooks/mail/useDecryptedMail`' and import { useMailKeys } from
'`@/hooks/mail/useMailKeys`') so they follow the project's "`@/`*" → "src/*" import
convention.
- Line 35: Replace the vi.clearAllMocks() call in the beforeEach of the
useDecryptedMail.test.tsx test suite with vi.restoreAllMocks() so mock
implementations are properly reset; locate the beforeEach block that currently
calls vi.clearAllMocks() (or any test setup referencing vi.clearAllMocks) and
change it to vi.restoreAllMocks(), ensuring the test harness uses restored mock
behavior for functions used by the useDecryptedMail tests.
In `@src/hooks/mail/useDecryptedMail.ts`:
- Line 4: Replace the relative import of the internal hook with the project
path-alias form: change the import that currently references './useMailKeys' to
use the '`@/`...' alias (i.e. import useMailKeys from '`@/hooks/mail/useMailKeys`')
so the useMailKeys hook import follows the repo convention; update the import
statement in useDecryptedMail.ts where useMailKeys is referenced.
- Around line 42-87: The current single-value cache (cached / setCached) causes
reuse to fail across different mail IDs; change cached to a map keyed by mail.id
(e.g., Record<string, CachedResult> or Map<string, CachedResult>) and update the
useEffect that calls decryptMailBody(mail, senderKeys) to store the result under
cached[mail.id] (or map.set(mail.id, result)); then update the useMemo lookup
(currently using cached && cached.mailId === mail.id ? cached : null) to read
the cached entry for the current mail id and return that entry’s text/ok into
the same state shape; ensure cleanup logic and dependency array remain correct
for mail, isEncrypted, canDecrypt and senderKeys.
In `@src/hooks/mail/useDecryptedPreviews.test.tsx`:
- Around line 5-6: The test imports useDecryptedPreviews and useMailKeys via
relative paths; update their import statements to use the project path alias by
replacing "./useDecryptedPreviews" with "`@/hooks/mail/useDecryptedPreviews`" and
"./useMailKeys" with "`@/hooks/mail/useMailKeys`" (locate the imports at the top
of src/hooks/mail/useDecryptedPreviews.test.tsx where useDecryptedPreviews and
useMailKeys are referenced).
- Line 24: Replace the call to vi.clearAllMocks() in the test's beforeEach with
vi.restoreAllMocks() so the test suite follows the repo standard of restoring
mocked implementations; locate the beforeEach that currently calls
vi.clearAllMocks() in src/hooks/mail/useDecryptedPreviews.test.tsx and swap it
to vi.restoreAllMocks() (no other changes needed).
In `@src/hooks/mail/useDecryptedPreviews.ts`:
- Line 4: The import in useDecryptedPreviews.ts currently uses a relative path
for the internal hook; update the import of useMailKeys to use the project path
alias (e.g., '`@/hooks/mail/useMailKeys`') so it follows the repository convention
for internal modules and aligns with the alias mapping from src/* to `@/`*.
In `@src/i18n/locales/en.json`:
- Around line 195-199: Move the three compose-related translation keys
(noRecipients, sendFailed, keyLookupFailed) from their current location into the
errors.mail namespace (i.e., errors.mail.noRecipients, errors.mail.sendFailed,
errors.mail.keyLookupFailed) in the en.json file, and update all callers that
currently reference modals.composeMessageDialog.errors.* to use
translate('errors.mail.<key>') instead (search for usages in
composeMessageDialog code paths and any send/lookup error handlers that call
translate or pass translation keys).
In `@src/services/mail-encryption/index.test.ts`:
- Line 11: Replace the relative internal import "from '.'" in the test with the
configured path-alias import using the `@/`* mapping (e.g., change to import from
'`@/services/mail-encryption`'); update the import statements in
src/services/mail-encryption/index.test.ts so all internal module imports use
the @ alias instead of relative '.' to comply with the coding guideline.
- Around line 15-161: Add a top-level beforeEach that calls vi.restoreAllMocks()
to ensure Vitest mocks are restored between tests; insert before the existing
describe blocks in this test file (so add beforeEach(() =>
vi.restoreAllMocks()); near the top of
src/services/mail-encryption/index.test.ts) to prevent cross-test mock leakage
affecting functions like buildEncryptionBlock, decryptEnvelope,
decryptSummaryPreview, isEncryptedEmailBody and parseEncryptionBlock.
In `@src/services/mail-encryption/index.ts`:
- Around line 49-51: Replace the generic Error throws around the recipients
check with the appropriate typed mail-domain error exported from
src/errors/mail/index.ts: import the specific error class used for
validation/failure (e.g., MailValidationError or the domain-specific class
exported there) and throw new ThatError('At least one recipient is required to
build the encryption block') instead of new Error(...); do the same for the
other throw at the second location (lines ~112-113) so both checks use the mail
error types allowing callers to catch domain-specific errors.
- Around line 45-121: The module currently exports top-level functions
(buildEncryptionBlock, isEncryptedEmailBody, parseEncryptionBlock,
decryptEnvelope, decryptSummaryPreview and internal trialDecrypt) which violates
the singleton service pattern; refactor by wrapping these functions into a class
(e.g., class MailEncryptionService) as instance methods, make trialDecrypt a
private method, and export a singleton via "export const instance = new
MailEncryptionService()" or "export class MailEncryptionService { public static
readonly instance = new MailEncryptionService(); }" so callers use
MailEncryptionService.instance.buildEncryptionBlock(...) (or instance.*) instead
of importing standalone functions; ensure existing internal helper symbols
(encryptEmail, encryptEmailWithKey, encryptKeysHybrid, decryptKeysHybrid,
decryptEmail, secureShuffle, base64ToUint8Array) remain referenced from the new
class methods.
In `@src/services/recipient-keys/index.test.ts`:
- Around line 5-9: Add a call to vi.restoreAllMocks() at the start of the
beforeEach block so mocks are reset before
RecipientKeysService.instance.clear(), vi.useFakeTimers(), and
vi.setSystemTime(...) run; this ensures any spies/stubs are restored between
tests and keeps tests fully isolated.
- Line 2: Update the internal import in the test to use the project path alias
instead of a relative path: replace the current import of RecipientKeysService
from '.' with the alias-based import '`@/services/recipient-keys`' so the test
imports RecipientKeysService via the configured `@/`* → src/* alias.
In `@src/services/recipient-keys/index.ts`:
- Around line 20-28: get(address: string, ttlMs: number = DEFAULT_TTL_MS):
CachedKey | null currently returns the exact object stored in this.cache (via
this.normalize), allowing external mutation of cache internals; change get to
return an immutable or copied shape instead (e.g., return a shallow copy of the
CachedKey or an Object.freeze/readonly view) after performing the ttl/fetch
checks so callers cannot mutate entry.fetchedAt or other fields; keep the
existing ttl logic and use this.normalize(address) once to locate/delete the
entry and then return the safe copy.
In `@src/store/api/mail/index.ts`:
- Around line 250-258: serializeQueryArgs and queryFn are out of sync:
serializeQueryArgs lowercases/sorts/joined addresses for the cache but queryFn
still forwards the original raw addresses to
MailService.instance.lookupRecipientKeys, allowing whitespace/duplicate variants
to bypass cache and hit the backend. Update both serializeQueryArgs and queryFn
to use the same normalized array: split the serialized string (or start from
queryArgs.addresses array), trim each address, lowercase, remove empty entries
and duplicates, sort, then use the joined string for the cache key in
serializeQueryArgs and pass the normalized array (not the raw input) into
MailService.instance.lookupRecipientKeys inside queryFn so lookup uses the same
deduped/normalized addresses.
In `@src/store/api/mail/mail.api.test.ts`:
- Around line 593-620: Add tests to cover normalization/duplicate and empty-list
boundaries for the lookupRecipientKeys endpoint: write a test that dispatches
mailApi.endpoints.lookupRecipientKeys.initiate with addresses that include
whitespace and case variants (e.g. ' Alice@inxt.me ', 'alice@INXT.me') and
duplicates, mock MailService.instance.lookupRecipientKeys to verify it's called
once with the normalized, unique addresses, assert result.data contains expected
recipients and that RecipientKeysService.instance cache was written for each
normalized address; add another test that dispatches with an empty addresses
array, assert it returns an empty array (result.data === []) and that
MailService.instance.lookupRecipientKeys was not called (or appropriately
handled) and no cache entries were written; reuse createTestStore,
MailService.instance, RecipientKeysService.instance and assert errors still map
to FetchRecipientKeysError where applicable.
In `@src/utils/domain/index.test.ts`:
- Around line 6-69: Split each test into explicit Arrange / Act / Assert
sections separated by a blank line: for tests covering getDomain,
isInternxtDomain, uniqueEmailAddresses, and classifyRecipients, add an "Arrange"
block to set up inputs (e.g., input addresses and internxtDomains), an "Act"
block to call the function under test (e.g., const result = getDomain(...),
isInternxtDomain(...), uniqueEmailAddresses(...), classifyRecipients(...)), then
an "Assert" block with the expect(...) assertions; ensure there is a blank line
between Arrange/Act and Act/Assert in every test to follow the AAA pattern.
🪄 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: 30b2f9c5-44eb-4ab3-a310-97cb105e4916
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
package.jsonsrc/components/compose-message/index.tsxsrc/errors/mail/index.tssrc/features/mail/MailView.tsxsrc/features/mail/components/mail-preview/index.tsxsrc/features/mail/components/tray/search/components/list/index.tsxsrc/hooks/mail/useDecryptedMail.test.tsxsrc/hooks/mail/useDecryptedMail.tssrc/hooks/mail/useDecryptedPreviews.test.tsxsrc/hooks/mail/useDecryptedPreviews.tssrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/it.jsonsrc/services/mail-encryption/index.test.tssrc/services/mail-encryption/index.tssrc/services/recipient-keys/index.test.tssrc/services/recipient-keys/index.tssrc/services/sdk/mail/index.tssrc/services/sdk/mail/mail.service.test.tssrc/store/api/base.tssrc/store/api/mail/index.tssrc/store/api/mail/mail.api.test.tssrc/utils/domain/index.test.tssrc/utils/domain/index.tssrc/utils/format-emails/index.test.tssrc/utils/format-emails/index.ts
Comment on lines
+5
to
+6
| import { useDecryptedMail } from './useDecryptedMail'; | ||
| import { useMailKeys } from './useMailKeys'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Switch internal test imports to @/... aliases
Use path aliases for these internal imports to keep consistency with repo rules.
As per coding guidelines, "Use the path alias @/* → src/* when importing internal modules."
🤖 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/useDecryptedMail.test.tsx` around lines 5 - 6, Update the
test's internal imports to use the repo path alias instead of relative paths:
replace the current imports of useDecryptedMail and useMailKeys with their
aliased forms (e.g., import { useDecryptedMail } from
'`@/hooks/mail/useDecryptedMail`' and import { useMailKeys } from
'`@/hooks/mail/useMailKeys`') so they follow the project's "`@/`*" → "src/*" import
convention.
| import { useEffect, useMemo, useState } from 'react'; | ||
| import type { EmailResponse } from '@internxt/sdk/dist/mail/types'; | ||
| import type { HybridKeyPair } from 'internxt-crypto'; | ||
| import { useMailKeys } from './useMailKeys'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use path alias for internal hook import
Please replace the relative import with @/... to match the repo import convention.
As per coding guidelines, "Use the path alias @/* → src/* when importing internal modules."
🤖 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/useDecryptedMail.ts` at line 4, Replace the relative import of
the internal hook with the project path-alias form: change the import that
currently references './useMailKeys' to use the '`@/`...' alias (i.e. import
useMailKeys from '`@/hooks/mail/useMailKeys`') so the useMailKeys hook import
follows the repo convention; update the import statement in useDecryptedMail.ts
where useMailKeys is referenced.
…onService for improved modularity and error handling in ComposeMessageDialog and related hooks
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
🤖 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/store/api/mail/index.ts`:
- Around line 269-272: The code only caches recipients with a truthy publicKey,
so lookups that return { publicKey: null } are never recorded and keep
re-querying; update the loop that handles
MailService.instance.lookupRecipientKeys(...) so it writes every recipient into
RecipientKeysService.instance via RecipientKeysService.instance.set(r.address,
r.publicKey) even when r.publicKey is null (i.e., cache negative results) so the
RecipientKeysService’s 5-minute TTL will apply to misses as well.
🪄 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: 95159f92-73f7-4c14-81ef-49af9f538195
📒 Files selected for processing (9)
src/components/compose-message/index.tsxsrc/hooks/mail/useDecryptedMail.test.tsxsrc/hooks/mail/useDecryptedMail.tssrc/hooks/mail/useDecryptedPreviews.test.tsxsrc/hooks/mail/useDecryptedPreviews.tssrc/services/mail-encryption/index.test.tssrc/services/mail-encryption/index.tssrc/store/api/mail/index.tssrc/store/api/mail/mail.api.test.ts
…d hook for streamlined email sending and improved encryption handling
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: 4
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/mail.api.test.ts (1)
64-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
vi.restoreAllMocks()in suite setup.Line 65 currently calls
vi.clearAllMocks(). Replace it withvi.restoreAllMocks()to fully reset mocked implementations between tests.As per coding guidelines, "Reset mocks in beforeEach with vi.restoreAllMocks()."
🤖 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/mail.api.test.ts` around lines 64 - 66, The test suite's setup uses vi.clearAllMocks() in the beforeEach block; replace that call with vi.restoreAllMocks() so mocked implementations are fully reset between tests—update the beforeEach to call vi.restoreAllMocks() instead of vi.clearAllMocks() to comply with the reset-mocks guideline.src/services/recipient-keys/index.ts (1)
16-35: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd JSDoc to public service methods.
set,get,has, andclearare public API surface but currently undocumented.As per coding guidelines, "Use JSDoc on service public methods to document their purpose."
🤖 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/services/recipient-keys/index.ts` around lines 16 - 35, Add JSDoc comments to the public methods set, get, has, and clear in the recipient-keys service to document their purpose, parameters, return values and behavior (including that address is normalized, publicKey may be null, get/has accept an optional ttlMs defaulting to DEFAULT_TTL_MS and that get returns a defensive copy or null, and clear removes all cached entries). For each method include a short description, `@param` tags for address/publicKey/ttlMs, `@returns` for get/has, and mention side effects (cache.set, cache.delete, cache.clear) so callers understand TTL expiration and null semantics.src/services/mail-encryption/index.ts (1)
84-127: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd JSDoc for all remaining public service methods.
isEncryptedEmailBody,parseEncryptionBlock,decryptEnvelope, anddecryptSummaryPrevieware public but undocumented; this file currently documents only one public method.As per coding guidelines: "Use JSDoc on service public methods to document their purpose."
🤖 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/services/mail-encryption/index.ts` around lines 84 - 127, Add JSDoc comments for the public methods isEncryptedEmailBody, parseEncryptionBlock, decryptEnvelope, and decryptSummaryPreview: for isEncryptedEmailBody document that it detects the ENCRYPTED_EMAIL_PREFIX in a text body and returns boolean; for parseEncryptionBlock document that it extracts and base64-decodes the payload, parses JSON and returns an EncryptionBlock; for decryptEnvelope and decryptSummaryPreview document that they attempt trial decryption using trialDecrypt with the provided HybridKeyPair and return Promise<string> containing the decrypted text, and note that failures result in an EnvelopeDecryptionError from trialDecrypt; include `@param` and `@returns` for each and `@throws` where applicable.
🤖 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/useComposeSend.ts`:
- Around line 101-102: When triggerLookup({ addresses: uniqueAddresses
}).unwrap() can throw, catch that specific failure and map it to the user-facing
error key errors.mail.keyLookupFailed via the translate function (e.g.,
translate('errors.mail.keyLookupFailed')) instead of letting it fall through to
the generic sendFailed path; update the lookup block around triggerLookup/usable
in useComposeSend (and the similar lookup handling later in the same file) to
throw or return an error object that uses
translate('errors.mail.keyLookupFailed') so the UI displays the correct i18n
message.
- Around line 60-63: The encryptionState computation currently returns 'none'
when activeDomains is undefined causing a silent cleartext fallback; update the
logic in useComposeSend (encryptionState, which calls classifyRecipients) to
treat an unresolved activeDomains as a transient "unknown" state (or
throw/return an explicit error state) instead of 'none', and ensure the send
path checks for that state and prevents cleartext sends by surfacing a
user-facing i18n error via the existing i18n utilities; update the code paths
that read encryptionState (including the send handler around where cleartext is
selected) to block sending until activeDomains is resolved and add
behavior-focused tests for useComposeSend to cover unresolved activeDomains and
the i18n error presentation.
- Around line 40-53: Add a Vitest behavior test file for the useComposeSend hook
that follows AAA (arrange-act-assert) and covers: 1) no recipients — assert
sendEmail is not called and a warning toast is shown, 2) encrypted path with
missing sender keys — mock useGetMailAccountKeysQuery to return no keys and
assert keyLookupFailed behavior (triggerLookup result), 3) encrypted path with
incomplete key lookup — mock useLazyLookupRecipientKeysQuery to return
partial/missing keys and assert keyLookupFailed, 4) send mutation failure — mock
useSendEmailMutation to reject and assert a sendFailed toast and that onSent is
not called, and 5) happy paths for encrypted and cleartext — mock successful
lookup and useSendEmailMutation resolve and assert sendEmail called with
expected payload and onSent invoked; in each test mock
useGetActiveDomainsQuery/useTranslationContext as needed and verify toast
messages and sendEmail calls appropriately by spying/mocking the functions
(useComposeSend, triggerLookup from useLazyLookupRecipientKeysQuery, sendEmail
from useSendEmailMutation, and translate/onSent).
In `@src/components/compose-message/index.tsx`:
- Line 11: The import of useComposeSend in index.tsx uses a relative path;
update it to use the project alias by replacing the './hooks/useComposeSend'
import with the '`@/components/compose-message/hooks/useComposeSend`' (or the
correct alias path that maps to
src/components/compose-message/hooks/useComposeSend) so the symbol
useComposeSend is imported via the `@/` alias for consistency with repository
import rules.
---
Outside diff comments:
In `@src/services/mail-encryption/index.ts`:
- Around line 84-127: Add JSDoc comments for the public methods
isEncryptedEmailBody, parseEncryptionBlock, decryptEnvelope, and
decryptSummaryPreview: for isEncryptedEmailBody document that it detects the
ENCRYPTED_EMAIL_PREFIX in a text body and returns boolean; for
parseEncryptionBlock document that it extracts and base64-decodes the payload,
parses JSON and returns an EncryptionBlock; for decryptEnvelope and
decryptSummaryPreview document that they attempt trial decryption using
trialDecrypt with the provided HybridKeyPair and return Promise<string>
containing the decrypted text, and note that failures result in an
EnvelopeDecryptionError from trialDecrypt; include `@param` and `@returns` for each
and `@throws` where applicable.
In `@src/services/recipient-keys/index.ts`:
- Around line 16-35: Add JSDoc comments to the public methods set, get, has, and
clear in the recipient-keys service to document their purpose, parameters,
return values and behavior (including that address is normalized, publicKey may
be null, get/has accept an optional ttlMs defaulting to DEFAULT_TTL_MS and that
get returns a defensive copy or null, and clear removes all cached entries). For
each method include a short description, `@param` tags for
address/publicKey/ttlMs, `@returns` for get/has, and mention side effects
(cache.set, cache.delete, cache.clear) so callers understand TTL expiration and
null semantics.
In `@src/store/api/mail/mail.api.test.ts`:
- Around line 64-66: The test suite's setup uses vi.clearAllMocks() in the
beforeEach block; replace that call with vi.restoreAllMocks() so mocked
implementations are fully reset between tests—update the beforeEach to call
vi.restoreAllMocks() instead of vi.clearAllMocks() to comply with the
reset-mocks guideline.
🪄 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: 954bdb66-4efc-4b58-87d1-500f8fc7dfc8
📒 Files selected for processing (15)
src/components/compose-message/hooks/useComposeSend.tssrc/components/compose-message/index.tsxsrc/errors/mail/index.tssrc/hooks/mail/useDecryptedMail.test.tsxsrc/hooks/mail/useDecryptedPreviews.test.tsxsrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/it.jsonsrc/services/mail-encryption/index.test.tssrc/services/mail-encryption/index.tssrc/services/recipient-keys/index.test.tssrc/services/recipient-keys/index.tssrc/store/api/mail/index.tssrc/store/api/mail/mail.api.test.ts
| import { ActionDialog, useActionDialog } from '@/context/dialog-manager'; | ||
| import { useTranslationContext } from '@/i18n'; | ||
| import useComposeMessage from './hooks/useComposeMessage'; | ||
| import useComposeSend from './hooks/useComposeSend'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use the @/ alias for this internal import.
Please switch this relative import to the repository alias for consistency with import rules.
Proposed change
-import useComposeSend from './hooks/useComposeSend';
+import useComposeSend from '`@/components/compose-message/hooks/useComposeSend`';As per coding guidelines: "Use the path alias @/* → src/* when importing internal modules."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import useComposeSend from './hooks/useComposeSend'; | |
| import useComposeSend from '`@/components/compose-message/hooks/useComposeSend`'; |
🤖 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` at line 11, The import of
useComposeSend in index.tsx uses a relative path; update it to use the project
alias by replacing the './hooks/useComposeSend' import with the
'`@/components/compose-message/hooks/useComposeSend`' (or the correct alias path
that maps to src/components/compose-message/hooks/useComposeSend) so the symbol
useComposeSend is imported via the `@/` alias for consistency with repository
import rules.
…g logic and error handling
|
✅ Actions performedReview triggered.
|
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 (2)
src/services/recipient-keys/index.ts (1)
5-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a co-located test file for this service.
This is logic-bearing service code, but the PR does not include a co-located
src/services/recipient-keys/index.test.tscovering normalization, TTL eviction, andhas()/clear()behavior.As per coding guidelines, "Test every
.tsfile that contains logic ... Co-locate test files: foo.ts → foo.test.ts."🤖 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/services/recipient-keys/index.ts` around lines 5 - 41, Add a co-located test file src/services/recipient-keys/index.test.ts that exercises RecipientKeysService behavior: test normalize by calling set() with mixed-case/whitespace addresses and asserting stored key is retrievable via get() using different casings; test TTL eviction by setting an entry, advancing time (or mocking Date.now) beyond DEFAULT_TTL_MS and asserting get() returns null and that the internal cache no longer has the key; test has() returns true/false consistent with get() and ttl; and test clear() empties the cache. Use the public API (RecipientKeysService.instance, set, get, has, clear) and verify returned CachedKey is a defensive copy (mutating get() result does not affect stored cache).src/services/mail-encryption/index.ts (1)
40-139:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd co-located tests for the encryption service before merge.
This service is core logic and currently lacks a co-located
src/services/mail-encryption/index.test.tsin the PR. Please add behavior tests for: empty recipients error path, prefix/parse behavior, successful decrypt path, and failed trial-decrypt path.As per coding guidelines, "Test every
.tsfile that contains logic ... Co-locate test files: foo.ts → foo.test.ts."🤖 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/services/mail-encryption/index.ts` around lines 40 - 139, Add a co-located test file next to index.ts that imports MailEncryptionService and tests: (1) buildEncryptionBlock throws BuildEncryptionBlockError when recipients is empty; (2) isEncryptedEmailBody and parseEncryptionBlock correctly detect/payload-decode the ENCRYPTED_EMAIL_PREFIX; (3) a full happy path where buildEncryptionBlock produces an EncryptionBlock and decryptEnvelope/decryptSummaryPreview with the correct recipient HybridKeyPair returns the original body and preview; and (4) a negative path where a non-recipient keypair causes trialDecrypt to throw EnvelopeDecryptionError; stub or control the underlying encrypt/decrypt helpers (encryptEmail, encryptEmailWithKey, encryptKeysHybrid, decryptKeysHybrid, decryptEmail) as needed to make tests deterministic and assert expected exceptions and plaintext results.
🤖 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/useComposeSend.test.ts`:
- Around line 5-8: The test imports useComposeSend, MailEncryptionService,
notificationsService and Recipient via relative paths; update those imports to
use the project alias pattern (`@/`*) instead of relative paths so internal
modules follow repo policy—replace the relative import of useComposeSend, the
MailEncryptionService import, notificationsService import, and the Recipient
type import with their equivalent '`@/`...' paths (e.g. start with '`@/`' and mirror
the src/* layout) so the test uses alias-based internal imports.
In `@src/components/compose-message/hooks/useComposeSend.ts`:
- Line 14: Replace the relative import of the Recipient type with the project
path alias; update the import in useComposeSend.ts that currently imports
"Recipient" from '../types' to use the '`@/`...' alias pointing at the same module
(e.g., import Recipient from '`@/components/compose-message/types`' or the correct
aliased path to that types file) so the file continues to reference the same
symbol Recipient but via the `@/`* alias.
---
Outside diff comments:
In `@src/services/mail-encryption/index.ts`:
- Around line 40-139: Add a co-located test file next to index.ts that imports
MailEncryptionService and tests: (1) buildEncryptionBlock throws
BuildEncryptionBlockError when recipients is empty; (2) isEncryptedEmailBody and
parseEncryptionBlock correctly detect/payload-decode the ENCRYPTED_EMAIL_PREFIX;
(3) a full happy path where buildEncryptionBlock produces an EncryptionBlock and
decryptEnvelope/decryptSummaryPreview with the correct recipient HybridKeyPair
returns the original body and preview; and (4) a negative path where a
non-recipient keypair causes trialDecrypt to throw EnvelopeDecryptionError; stub
or control the underlying encrypt/decrypt helpers (encryptEmail,
encryptEmailWithKey, encryptKeysHybrid, decryptKeysHybrid, decryptEmail) as
needed to make tests deterministic and assert expected exceptions and plaintext
results.
In `@src/services/recipient-keys/index.ts`:
- Around line 5-41: Add a co-located test file
src/services/recipient-keys/index.test.ts that exercises RecipientKeysService
behavior: test normalize by calling set() with mixed-case/whitespace addresses
and asserting stored key is retrievable via get() using different casings; test
TTL eviction by setting an entry, advancing time (or mocking Date.now) beyond
DEFAULT_TTL_MS and asserting get() returns null and that the internal cache no
longer has the key; test has() returns true/false consistent with get() and ttl;
and test clear() empties the cache. Use the public API
(RecipientKeysService.instance, set, get, has, clear) and verify returned
CachedKey is a defensive copy (mutating get() result does not affect stored
cache).
🪄 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: 48a9d4c9-873b-4568-b527-066b2215259f
📒 Files selected for processing (8)
src/components/compose-message/hooks/useComposeSend.test.tssrc/components/compose-message/hooks/useComposeSend.tssrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/it.jsonsrc/services/mail-encryption/index.tssrc/services/recipient-keys/index.ts
Comment on lines
+5
to
+8
| import useComposeSend from './useComposeSend'; | ||
| import { MailEncryptionService } from '@/services/mail-encryption'; | ||
| import notificationsService from '@/services/notifications'; | ||
| import type { Recipient } from '../types'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use project path aliases for internal imports in tests.
Switch the relative imports to @/* aliases to match repository import policy.
Proposed patch
-import useComposeSend from './useComposeSend';
+import useComposeSend from '`@/components/compose-message/hooks/useComposeSend`';
import { MailEncryptionService } from '`@/services/mail-encryption`';
import notificationsService from '`@/services/notifications`';
-import type { Recipient } from '../types';
+import type { Recipient } from '`@/components/compose-message/types`';As per coding guidelines, "Use the path alias @/* → src/* when importing internal modules."
🤖 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.test.ts` around lines 5 -
8, The test imports useComposeSend, MailEncryptionService, notificationsService
and Recipient via relative paths; update those imports to use the project alias
pattern (`@/`*) instead of relative paths so internal modules follow repo
policy—replace the relative import of useComposeSend, the MailEncryptionService
import, notificationsService import, and the Recipient type import with their
equivalent '`@/`...' paths (e.g. start with '`@/`' and mirror the src/* layout) so
the test uses alias-based internal imports.
| import { MailEncryptionService, type RecipientPublicKey } from '@/services/mail-encryption'; | ||
| import notificationsService, { ToastType } from '@/services/notifications'; | ||
| import { useTranslationContext } from '@/i18n'; | ||
| import type { Recipient } from '../types'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace relative internal import with @/* alias.
Use the project alias for Recipient import to keep module imports consistent.
Proposed patch
-import type { Recipient } from '../types';
+import type { Recipient } from '`@/components/compose-message/types`';As per coding guidelines, "Use the path alias @/* → src/* when importing internal modules."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import type { Recipient } from '../types'; | |
| import type { Recipient } from '`@/components/compose-message/types`'; |
🤖 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` at line 14, Replace
the relative import of the Recipient type with the project path alias; update
the import in useComposeSend.ts that currently imports "Recipient" from
'../types' to use the '`@/`...' alias pointing at the same module (e.g., import
Recipient from '`@/components/compose-message/types`' or the correct aliased path
to that types file) so the file continues to reference the same symbol Recipient
but via the `@/`* alias.
xabg2
approved these changes
May 29, 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
2 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.
Adds the client plumbing to fetch recipients' public encryption keys ahead of an encrypted send. And mail encryption functionality
RecipientKeysService— in-memory cache of address → public key with a 5-minute TTL and case/whitespace-normalized addresses.get/hasevict expired entries;set/clearmanage the cache.MailService.lookupRecipientKeys(addresses)— wraps the SDK endpoint; returns{ address, publicKey | null }per address, wherenullsignals an external/unknown recipient the caller should treat as cleartext-only.services/mail-encryption): encrypts body + preview under one symmetric key, wrapped per-recipient. Wrapped keys ship as a de-identified, order-randomized array carrying no addresses, so the recipient set (Bcc included) stays hidden; recipients find their entry by trial decryption. Subject stays cleartext so the backend can index/search it.services/recipient-keys,store/api/mail): resolves recipients' public keys before an encrypted send; falls back to a cleartext send if any recipient has no key.compose-message): wires lookup + encryption into the send flow.useDecryptedMaildecrypts the opened message;useDecryptedPreviewsdecrypts list/search row previews (encrypted rows show a blank snippet until decrypted, never ciphertext).SendEmailError,FetchRecipientKeysError,FetchActiveDomainsError) and i18n keys across all locales.