feat: add 'Delete All Spam' button on /mail/spam route (#886)#888
feat: add 'Delete All Spam' button on /mail/spam route (#886)#888giteshsarvaiya wants to merge 3 commits intoMail-0:stagingfrom
Conversation
|
@giteshsarvaiya is attempting to deploy a commit to the Zero Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe updates introduce spam management functions to the mail application, enabling detection and deletion of all spam emails via new asynchronous actions and UI elements. Database migrations set default user settings and update the migration snapshot and journal. Docker-related npm scripts are revised to use the modern Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MailUI
participant Actions
participant Driver
User->>MailUI: Navigate to Spam folder
MailUI->>Actions: checkSpamEmails()
Actions->>Driver: list('SPAM')
Driver-->>Actions: Return spam emails
Actions-->>MailUI: { hasEmails, count }
MailUI->>User: Show "Delete All Spam" button if hasEmails
User->>MailUI: Click "Delete All Spam"
MailUI->>Actions: deleteAllSpamEmails()
Actions->>Driver: list('SPAM')
Driver-->>Actions: Return spam emails
Actions->>Driver: modifyLabels(emailIds, add=['TRASH'])
Driver-->>Actions: Success
Actions-->>MailUI: { success, message }
MailUI->>User: Show toast notification
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/mail/actions/mail.ts (3)
193-209: Use optional chaining for cleaner null checking.The function follows the existing error handling pattern but could use optional chaining for cleaner code.
- return { - hasEmails: !!(spamEmails && spamEmails.threads && spamEmails.threads.length > 0), - count: spamEmails?.threads?.length || 0 - }; + return { + hasEmails: !!spamEmails?.threads?.length, + count: spamEmails?.threads?.length || 0 + };🧰 Tools
🪛 Biome (1.9.4)
[error] 202-202: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
211-236: Consider reusing existing bulkDeleteThread function.The
deleteAllSpamEmailsfunction duplicates logic from the existingbulkDeleteThreadfunction. Consider reusing it for better maintainability.export const deleteAllSpamEmails = async () => { try { const driver = await getActiveDriver(); // Get all emails from spam folder const spamEmails = await driver.list("spam"); console.log(spamEmails); if (!spamEmails || spamEmails.threads.length === 0) { return { success: true, message: 'No spam emails to delete' }; } // Extract email IDs const emailIds = spamEmails.threads.map((thread) => thread.id); - // Use bulkDeleteThread to move them to trash - await driver.modifyLabels(emailIds, { addLabels: ['TRASH'], removeLabels: [] }); + // Use existing bulkDeleteThread function + await bulkDeleteThread({ ids: emailIds }); return { success: true, message: `Successfully deleted ${emailIds.length} email(s) from spam folder` }; } catch (error) { console.error('Error deleting all spam emails:', error); throw error; } };
216-218: Remove debug logging in production code.The
console.log(spamEmails)statement is likely for debugging and should be removed before production.// Get all emails from spam folder const spamEmails = await driver.list("spam"); -console.log(spamEmails); if (!spamEmails || spamEmails.threads.length === 0) {packages/db/migrations/meta/0025_snapshot.json (1)
90-111: Add indexes on foreign-key and frequently filtered columns
Every table that declares a foreign key (e.g.,mail0_account.user_id,mail0_connection.user_id,mail0_note.user_id,mail0_writing_style_matrix.connectionId, etc.) currently has an emptyindexesblock. Without explicit indexes on these columns, join and lookup performance will degrade under load. Please add single-column indexes for each foreign-key column and any other columns frequently used inWHEREclauses (e.g.,thread_id,connection_id).Also applies to: 189-191, 263-265, 343-345, 418-420, 502-503, 570-571, 614-615, 672-673, 743-744, 781-782
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/mail/actions/mail.ts(1 hunks)apps/mail/components/mail/mail.tsx(8 hunks)package.json(1 hunks)packages/db/migrations/0025_salty_the_fallen.sql(1 hunks)packages/db/migrations/meta/0025_snapshot.json(1 hunks)packages/db/migrations/meta/_journal.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/mail/actions/mail.ts (2)
apps/mail/app/api/driver/google.ts (1)
driver(183-1109)apps/mail/actions/utils.ts (1)
getActiveDriver(33-63)
🪛 Biome (1.9.4)
apps/mail/actions/mail.ts
[error] 202-202: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
package.json (1)
16-18: Use Docker Compose V2 CLI commands
The scripts have been correctly updated from the legacydocker-composeto the moderndocker composesyntax.packages/db/migrations/meta/_journal.json (1)
179-186: Clean migration journal entry added.The new migration entry for "0025_salty_the_fallen" is properly structured and follows the existing pattern in the journal.
packages/db/migrations/0025_salty_the_fallen.sql (1)
1-1: Well-structured default settings configuration.The migration properly sets default user preferences as a JSONB value for the "settings" column, ensuring new users have consistent initial configuration.
apps/mail/components/mail/mail.tsx (5)
82-82: Good state initialization for spam emails detection.The state variable for tracking spam emails is correctly initialized as false by default.
139-157: Well-structured promise handling with appropriate user feedback.The
onDeleteAllSpamfunction properly handles the promise lifecycle with toast notifications for success/error states and triggers the necessary data refreshes.
184-198: Properly implemented effect hook for spam email detection.The effect correctly checks for spam emails when in the spam folder and handles errors appropriately.
260-272: Well-implemented conditional UI for the Delete All Spam button.The button is only rendered when in the spam folder and when spam emails are present, with appropriate styling that matches the rest of the application.
732-735: Simplified CategorySelect component initialization.The component now properly uses useQueryState with a default value of 'Important'.
| "public.mail0_writing_style_matrix": { | ||
| "name": "mail0_writing_style_matrix", | ||
| "schema": "", | ||
| "columns": { | ||
| "connectionId": { | ||
| "name": "connectionId", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true | ||
| }, | ||
| "numMessages": { | ||
| "name": "numMessages", | ||
| "type": "integer", | ||
| "primaryKey": false, | ||
| "notNull": true | ||
| }, | ||
| "style": { | ||
| "name": "style", | ||
| "type": "jsonb", | ||
| "primaryKey": false, | ||
| "notNull": true | ||
| }, | ||
| "updatedAt": { | ||
| "name": "updatedAt", | ||
| "type": "timestamp", | ||
| "primaryKey": false, | ||
| "notNull": true, | ||
| "default": "now()" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent column naming: use snake_case
The mail0_writing_style_matrix table defines columns connectionId and updatedAt in camelCase, while all other tables and columns use snake_case. For consistency and to avoid confusion (especially in SQL queries), these should be renamed to connection_id and updated_at.
|
I think you provided the wrong demo link... Please double check. |
hey needle, thanks for pointing out, i had pasted a wrong link there, now the link has been updated |
| const driver = await getActiveDriver(); | ||
|
|
||
| // Get all emails from spam folder | ||
| const spamEmails = await driver.list('spam'); |
There was a problem hiding this comment.
this deletes the default count of list which is 20 threads in the spam folder, which is the number of threads on the user's page, it doesn't delete all
There was a problem hiding this comment.
oh yess, I see that the maxResult is set to 20 by default and can go upto 500(as per the gmail api provider). so yes, tRPC would be a better approach.
my next commit will have tRPC
There was a problem hiding this comment.
what if we introduced pagination(to fetch all spams with maxResult 500) and implemented it with the same logic as of previous commit, i.e. without tRPC ? is there a specific reason we chose tRPC over server action ?
|
there was already trpc setup, why did you reimplement a 2nd TRPC client?? Edit: looks like your PR is based on |
|
hey @BlankParticle, yes, there is no tRPC in the main branch yet. edit: thanks for the clarification |
4 participants
Description
This PR (address #886):
Type of Change
Please delete options that are not relevant.
Areas Affected
Please check all that apply:
Testing Done
Describe the tests you've done:
Security Considerations
For changes involving data or authentication:
Checklist
Screenshots/Recordings
Loom [Link]
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
UI Improvements
Chores
docker composecommand.Database