feat(email): Block sender frontend + backend improvements#2156
feat(email): Block sender frontend + backend improvements#2156evanhutnik merged 6 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR implements sender-blocking end-to-end: adds a Block Sender UI action (soup menu item, toolbar tool, hotkey, and Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/app/component/next-soup/actions/make-block-sender-action.ts`:
- Around line 10-15: The execute function currently loops entities and calls
blockSenderWithToast for every entity, causing duplicate attempts when multiple
entities share the same senderEmail; change execute to first collect unique
senderEmail values (e.g., using a Set while preserving order) from the entities
array (only for entity.type === 'email' and entity.senderEmail present) and then
iterate over that deduplicated list to await blockSenderWithToast for each
unique address; reference execute, entities, senderEmail, and
blockSenderWithToast when making the change.
In `@js/app/packages/block-email/component/EmailContext.tsx`:
- Around line 363-366: The current sender selection uses currentUserEmail()
without guarding for undefined, allowing your own address to be considered
blockable; update the logic around userEmail and senderEmail (the userEmail =
currentUserEmail() call and the thread.messages.find(...) that sets senderEmail)
to first guard for a missing currentUserEmail()—either return/skip selection
when currentUserEmail() is falsy or set userEmail to null and change the find
predicate to only compare toLowerCase() when userEmail is present (e.g.,
short-circuit the comparison with userEmail) so you never classify the user's
own address as blockable when currentUserEmail() is unavailable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d484e273-ffa5-48fe-b25d-1d5d0e10b20e
⛔ Files ignored due to path filters (3)
js/app/packages/service-clients/service-email/generated/client.tsis excluded by!**/generated/**js/app/packages/service-clients/service-email/generated/schemas/index.tsis excluded by!**/generated/**js/app/packages/service-clients/service-email/generated/schemas/unblockSenderRequest.tsis excluded by!**/generated/**
📒 Files selected for processing (19)
js/app/packages/app/component/next-soup/actions/index.tsjs/app/packages/app/component/next-soup/actions/make-block-sender-action.tsjs/app/packages/app/component/next-soup/soup-view/soup-entity-actions-menu.tsxjs/app/packages/block-email/component/Email.tsxjs/app/packages/block-email/component/EmailContext.tsxjs/app/packages/block-email/component/TopBar.tsxjs/app/packages/block-email/util/emailHotkeys.tsjs/app/packages/core/hotkey/tokens.tsjs/app/packages/core/util/safeFetch.tsjs/app/packages/queries/email/thread.tsjs/app/packages/service-clients/service-email/client.tsjs/app/packages/service-clients/service-email/openapi.jsonrust/cloud-storage/email_service/src/api/email/contacts/block_sender.rsrust/cloud-storage/email_service/src/api/email/contacts/list_blocked.rsrust/cloud-storage/email_service/src/api/email/contacts/mod.rsrust/cloud-storage/email_service/src/api/email/contacts/unblock_sender.rsrust/cloud-storage/email_service/src/api/swagger.rsrust/cloud-storage/gmail_client/src/filters.rsrust/cloud-storage/models_email/src/gmail/error.rs
| const execute = async (entities: EntityData[]) => { | ||
| for (const entity of entities) { | ||
| if (entity.type !== 'email' || !entity.senderEmail) continue; | ||
| await blockSenderWithToast(entity.senderEmail); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Deduplicate sender emails to avoid redundant block attempts.
When multiple selected entities share the same senderEmail, the loop will attempt to block the same address multiple times, resulting in duplicate toasts and potential 409 conflict errors from the API.
🔧 Proposed fix
const execute = async (entities: EntityData[]) => {
+ const seen = new Set<string>();
for (const entity of entities) {
if (entity.type !== 'email' || !entity.senderEmail) continue;
+ if (seen.has(entity.senderEmail)) continue;
+ seen.add(entity.senderEmail);
await blockSenderWithToast(entity.senderEmail);
}
};📝 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.
| const execute = async (entities: EntityData[]) => { | |
| for (const entity of entities) { | |
| if (entity.type !== 'email' || !entity.senderEmail) continue; | |
| await blockSenderWithToast(entity.senderEmail); | |
| } | |
| }; | |
| const execute = async (entities: EntityData[]) => { | |
| const seen = new Set<string>(); | |
| for (const entity of entities) { | |
| if (entity.type !== 'email' || !entity.senderEmail) continue; | |
| if (seen.has(entity.senderEmail)) continue; | |
| seen.add(entity.senderEmail); | |
| await blockSenderWithToast(entity.senderEmail); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/next-soup/actions/make-block-sender-action.ts`
around lines 10 - 15, The execute function currently loops entities and calls
blockSenderWithToast for every entity, causing duplicate attempts when multiple
entities share the same senderEmail; change execute to first collect unique
senderEmail values (e.g., using a Set while preserving order) from the entities
array (only for entity.type === 'email' and entity.senderEmail present) and then
iterate over that deduplicated list to await blockSenderWithToast for each
unique address; reference execute, entities, senderEmail, and
blockSenderWithToast when making the change.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
js/app/packages/block-email/component/EmailContext.tsx (1)
372-372: 🧹 Nitpick | 🔵 TrivialPrefix with
voidto mark the intentional fire-and-forget call.
blockSenderWithToastis an async function returning aPromise. While errors are handled internally via toasts, the floating promise should be explicitly marked withvoidto communicate intent and satisfy linters that flag unhandled promises.Suggested fix
- blockSenderWithToast(senderEmail); + void blockSenderWithToast(senderEmail);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-email/component/EmailContext.tsx` at line 372, The call to the async function blockSenderWithToast should be explicitly marked as fire-and-forget by prefixing it with void to avoid a floating promise; update the invocation of blockSenderWithToast(senderEmail) inside the EmailContext component (where the toast-handled async call is made) to use void blockSenderWithToast(senderEmail) so linters and readers know the promise is intentionally not awaited.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@js/app/packages/block-email/component/EmailContext.tsx`:
- Line 372: The call to the async function blockSenderWithToast should be
explicitly marked as fire-and-forget by prefixing it with void to avoid a
floating promise; update the invocation of blockSenderWithToast(senderEmail)
inside the EmailContext component (where the toast-handled async call is made)
to use void blockSenderWithToast(senderEmail) so linters and readers know the
promise is intentionally not awaited.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a80eb7c8-9823-4efd-9134-ec51852eb7d2
📒 Files selected for processing (1)
js/app/packages/block-email/component/EmailContext.tsx
1 participant
Summary
gmail.settings.basic)DELETE /contacts/block/{email_address}toPOST /contacts/unblockwith JSON body to avoid URL encoding issues with special chars in email addressesGmailError::Forbiddenvariant to the Gmail client error enumFORBIDDENerror code tosafeFetch, shows a permissions toast prompting re-authentication on 403blockSenderWithToast()utility in queries layer handles block API call, success/error toasts with undo support -- used by both thread view and soup context menuDetails
Block sender flow
senderEmailfrom entity)POST /email/contacts/blockcreates a Gmail filter that sends the sender's emails to trashPOST /email/contacts/unblockto remove the filterPermission handling
gmail.settings.basicOAuth scope, Gmail returns 403