Delete unsubscribe emails - #102
Open
sdmonkey wants to merge 3 commits into
Open
Conversation
Contributor
WalkthroughAdds long-running job management and message-ID deletion: new API endpoints (/delete-by-ids, /job/start, /job/cancel, /job/status), Pydantic request models, service implementations (delete by IDs, job runner/cancel/status), job state tracking, scan result message_ids, and corresponding UI/JS and CSS for jobs and deletion flows. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser as Browser (jobs.js / scanner.js)
participant API as Server API (app.api.actions / app.api.status)
participant Jobs as Job Runner (app.services.gmail.jobs)
participant Gmail as Gmail API
User->>Browser: Start Job / Click Delete
Browser->>API: POST /job/start or POST /delete-by-ids
API->>API: validate request
alt start job
API->>Jobs: run_job(...) (background task)
Jobs->>Gmail: messages.list / messages.get / batchModify (per-page/batch)
Gmail-->>Jobs: responses
Jobs->>API: update AppState.job_status
Browser->>API: GET /job/status (poll)
API-->>Browser: job status
else delete-by-ids
API->>Gmail: batchModify(add TRASH label) per-1000 IDs
Gmail-->>API: response
API-->>Browser: deletion result
end
Browser->>API: POST /job/cancel (optional)
API->>Jobs: cancel_job()
Jobs-->>API: cancelled status
API-->>Browser: confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
static/css/components.css (1)
126-144:⚠️ Potential issue | 🟡 MinorRemove duplicate
.btn-dangerdeclarations.Lines 137-144 duplicate the
.btn-dangerstyles you just added at 126-135. The second block lacksborder-color, and due to CSS cascade, it will override your new styles. Consolidate into a single definition.🧹 Proposed fix to remove duplicate styles
.btn-danger { background: var(--danger-color); color: white; border-color: var(--danger-color); } .btn-danger:hover { background: var(--danger-hover); border-color: var(--danger-hover); } -.btn-danger { - background: var(--danger-color); - color: white; -} - -.btn-danger:hover { - background: var(--danger-hover); -} - .btn-download {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/css/components.css` around lines 126 - 144, There are duplicate CSS rules for the .btn-danger selector; remove the second set (the duplicate block that lacks border-color) and consolidate into a single .btn-danger and .btn-danger:hover definition that includes background, color, and border-color so the intended styles at the top (background: var(--danger-color); color: white; border-color: var(--danger-color); and hover variants using var(--danger-hover)) are preserved and not unintentionally overridden.
🧹 Nitpick comments (2)
app/services/gmail/delete.py (1)
289-294: Consider rate limiting for large batch operations.Other batch operations in this file include rate limiting (e.g., line 151-152:
time.sleep(0.3)every 5 batches). For very large message ID lists, you may hit API quotas without similar throttling.💡 Optional: Add rate limiting for large batches
for i in range(0, len(message_ids), batch_size): batch = message_ids[i : i + batch_size] service.users().messages().batchModify( userId="me", body={"ids": batch, "addLabelIds": ["TRASH"]} ).execute() deleted += len(batch) + + # Rate limiting for large operations + if (i // batch_size + 1) % 5 == 0: + time.sleep(0.3)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/gmail/delete.py` around lines 289 - 294, The batch delete loop over message_ids uses service.users().messages().batchModify but lacks throttling; add rate limiting similar to the other batch blocks by pausing (e.g., import time and call time.sleep(0.3)) every N iterations (e.g., every 5 batches) inside the loop that slices message_ids (the loop handling batch = message_ids[i : i + batch_size] and calling service.users().messages().batchModify) to avoid hitting API quotas and keep updating deleted as before.app/api/actions.py (1)
132-148: Service errors return HTTP 200 with a failure payload instead of raising exceptions.The
delete_emails_by_idsservice function (per the context snippet) catches all exceptions internally and returns{"success": False, ...}rather than raising. This means thetry/exceptblock here only catches unexpected failures (e.g., import errors, network issues before the call), while Gmail API errors will return HTTP 200 with a failure body.The frontend handles this correctly by checking
result.success, and this pattern matchesapi_delete_emailsat line 123. However, consider either:
- Checking
result["success"]and raisingHTTPException(500)on failure for consistent HTTP semantics, or- Adding a brief comment clarifying that service-level errors are handled via the response payload.
Option 1: Propagate service failures as HTTP errors
try: - return delete_emails_by_ids(request.message_ids) + result = delete_emails_by_ids(request.message_ids) + if not result.get("success"): + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail=result.get("message", "Failed to delete emails"), + ) + return result except Exception as e:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/actions.py` around lines 132 - 148, The endpoint api_delete_by_ids currently calls delete_emails_by_ids which returns a payload like {"success": False, ...} on service errors, so update api_delete_by_ids to capture the result (result = delete_emails_by_ids(request.message_ids)) and if result.get("success") is False raise HTTPException(status_code=500, detail=result.get("error", "Failed to delete emails")) so service-level failures propagate as HTTP errors consistent with api_delete_emails; alternatively, if you prefer not to change behavior, add a concise inline comment in api_delete_by_ids explaining that delete_emails_by_ids returns a failure payload and the frontend checks result["success"].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@static/js/scanner.js`:
- Line 504: The success toast is shown regardless of whether any deletions
succeeded; update the logic around GmailCleaner.UI.showSuccessToast so it checks
the deleted counter returned by the deletion routine (variable deleted) and only
calls GmailCleaner.UI.showSuccessToast(`Moved ${deleted} emails to Trash.`) when
deleted > 0; when deleted === 0, call the appropriate error/warning notifier
(e.g., GmailCleaner.UI.showErrorToast or showWarningToast) with a message like
"No emails were deleted" or omit any toast — modify the code path that currently
unconditionally invokes GmailCleaner.UI.showSuccessToast to branch on the
deleted value.
- Around line 359-373: The current branch logic for scope === 'scanned' falls
through to the delete-all endpoint when r.message_ids is missing; update the
handler around the scope check (the block using scope, r.message_ids and the
fetch calls to '/api/delete-by-ids' and '/api/delete-emails') to explicitly
validate that r.message_ids is a non-empty array before calling
'/api/delete-by-ids' — if message_ids is missing/empty, abort the delete
operation (e.g., show an error/confirmation and do not call
'/api/delete-emails') or handle it as a distinct case; ensure the else branch
only runs for an explicit "all"/sender scope, not as a fallback for missing IDs.
- Around line 464-478: The bulk-delete path incorrectly falls back to deleting
by sender when scope === 'scanned' but r.message_ids is empty; update the logic
around the fetch calls so that when scope === 'scanned' and r.message_ids is
falsy or empty you do not call the '/api/delete-emails' endpoint: instead skip
that item and emit a warning (e.g., console.warn or push to a warnings array)
that message_ids are missing for the scanned item; preserve the current behavior
for non-'scanned' items (use '/api/delete-emails' with sender r.domain) and for
scanned items with non-empty r.message_ids use '/api/delete-by-ids' as before.
Ensure you reference the existing variables r, scope, r.message_ids and the
fetch targets '/api/delete-by-ids' and '/api/delete-emails' when making the
change.
---
Outside diff comments:
In `@static/css/components.css`:
- Around line 126-144: There are duplicate CSS rules for the .btn-danger
selector; remove the second set (the duplicate block that lacks border-color)
and consolidate into a single .btn-danger and .btn-danger:hover definition that
includes background, color, and border-color so the intended styles at the top
(background: var(--danger-color); color: white; border-color:
var(--danger-color); and hover variants using var(--danger-hover)) are preserved
and not unintentionally overridden.
---
Nitpick comments:
In `@app/api/actions.py`:
- Around line 132-148: The endpoint api_delete_by_ids currently calls
delete_emails_by_ids which returns a payload like {"success": False, ...} on
service errors, so update api_delete_by_ids to capture the result (result =
delete_emails_by_ids(request.message_ids)) and if result.get("success") is False
raise HTTPException(status_code=500, detail=result.get("error", "Failed to
delete emails")) so service-level failures propagate as HTTP errors consistent
with api_delete_emails; alternatively, if you prefer not to change behavior, add
a concise inline comment in api_delete_by_ids explaining that
delete_emails_by_ids returns a failure payload and the frontend checks
result["success"].
In `@app/services/gmail/delete.py`:
- Around line 289-294: The batch delete loop over message_ids uses
service.users().messages().batchModify but lacks throttling; add rate limiting
similar to the other batch blocks by pausing (e.g., import time and call
time.sleep(0.3)) every N iterations (e.g., every 5 batches) inside the loop that
slices message_ids (the loop handling batch = message_ids[i : i + batch_size]
and calling service.users().messages().batchModify) to avoid hitting API quotas
and keep updating deleted as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c09cf4b-0388-4c6c-9163-17a6894c44e5
📒 Files selected for processing (10)
app/api/actions.pyapp/models/__init__.pyapp/models/schemas.pyapp/services/__init__.pyapp/services/gmail/__init__.pyapp/services/gmail/delete.pyapp/services/gmail/scan.pystatic/css/components.cssstatic/js/scanner.jstemplates/index.html
Comment on lines
+464
to
+478
| try { | ||
| let response; | ||
| if (scope === 'scanned' && r.message_ids && r.message_ids.length > 0) { | ||
| response = await fetch('/api/delete-by-ids', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ message_ids: r.message_ids }) | ||
| }); | ||
| } else { | ||
| response = await fetch('/api/delete-emails', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ sender: r.domain }) | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Same fallthrough issue in bulk delete path.
Identical logic flaw: if scope === 'scanned' but message_ids is empty, it silently deletes all emails from the sender instead.
Proposed fix: Skip items without message_ids and warn
try {
let response;
- if (scope === 'scanned' && r.message_ids && r.message_ids.length > 0) {
+ if (scope === 'scanned') {
+ if (!r.message_ids || r.message_ids.length === 0) {
+ // Skip this item - no message IDs available
+ if (btn) {
+ btn.classList.remove('btn-deleting');
+ btn.disabled = false;
+ btn.innerHTML = 'Delete';
+ }
+ continue;
+ }
response = await fetch('/api/delete-by-ids', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ message_ids: r.message_ids })
});
- } else {
+ } else if (scope === 'all') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@static/js/scanner.js` around lines 464 - 478, The bulk-delete path
incorrectly falls back to deleting by sender when scope === 'scanned' but
r.message_ids is empty; update the logic around the fetch calls so that when
scope === 'scanned' and r.message_ids is falsy or empty you do not call the
'/api/delete-emails' endpoint: instead skip that item and emit a warning (e.g.,
console.warn or push to a warnings array) that message_ids are missing for the
scanned item; preserve the current behavior for non-'scanned' items (use
'/api/delete-emails' with sender r.domain) and for scanned items with non-empty
r.message_ids use '/api/delete-by-ids' as before. Ensure you reference the
existing variables r, scope, r.message_ids and the fetch targets
'/api/delete-by-ids' and '/api/delete-emails' when making the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
static/js/jobs.js (1)
264-269: PreserveshowViewsignature in the monkey patchThe wrapper currently hardcodes
viewNameand drops return value. Forwarding args/return keeps compatibility ifshowViewevolves.Suggested refactor
- GmailCleaner.UI.showView = function(viewName) { - original(viewName); - if (viewName === 'job') { - GmailCleaner.Jobs.loadLabels(); - } - }; + GmailCleaner.UI.showView = function(...args) { + const result = original(...args); + if (args[0] === 'job') { + void GmailCleaner.Jobs.loadLabels(); + } + return result; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/js/jobs.js` around lines 264 - 269, The monkey-patch for GmailCleaner.UI.showView drops the original signature and return value; change the wrapper to forward all arguments and return the original result by calling original.apply(this, arguments) (or original.call with forwarded args) and store its return value, then trigger GmailCleaner.Jobs.loadLabels() only when the first argument equals 'job', and finally return the stored result so GmailCleaner.UI.showView preserves compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/actions.py`:
- Around line 277-294: The handler api_job_start should validate that when
request.action == "label" a label_id is provided before queuing run_job;
currently run_job will fail asynchronously but api_job_start returns
{"status":"started"} — change api_job_start to check request.action (or the
local variable action) and if it's "label" and request.label_id is falsy, raise
an HTTPException with a 400-level status and clear detail, otherwise proceed to
add the background task; reference the api_job_start function, the action and
label_id fields, and run_job to locate where to add this pre-flight validation.
- Around line 280-293: The race occurs because the check of
state.job_status.get("running") and the background_tasks.add_task(run_job, ...)
are not atomic; either set the running flag synchronously before queuing or use
a dedicated lock to claim the slot immediately: inside the handler, after the
guard check, set state.job_status["running"] = True (or acquire state.job_lock)
before calling background_tasks.add_task(...), and ensure run_job clears
state.job_status["running"] (or releases state.job_lock) in a finally block so
the slot is freed; reference state.job_status, run_job, and
background_tasks.add_task when applying this change.
In `@app/services/gmail/jobs.py`:
- Around line 285-296: The code accumulates every Gmail message ID into
unsubscribe_data["message_ids"] (see unsubscribe_data creation and usages in
find_subscriptions and the sections that append/copy message IDs), which grows
unbounded and can OOM; change this to track only a capped sample and an
aggregate count: introduce a MAX_SAVED_IDS constant, replace "message_ids" with
"sample_message_ids" and "message_count" (or equivalent) in the unsubscribe_data
default, when adding an ID push into sample_message_ids only if length <
MAX_SAVED_IDS and always increment message_count, and ensure when populating
state.scan_results you copy sample_message_ids and message_count instead of the
full ID list (apply the same capped-sample logic in the other places that
append/copy IDs referenced in the diff).
- Around line 120-128: The Gmail list helper base_list_params() and the mailbox
scan _run_find_subscriptions() don't set includeSpamTrash when mailbox is SPAM
or TRASH, so Gmail omits those messages; update both places (base_list_params
and the mailbox-scanning logic in _run_find_subscriptions) to detect when
mailbox equals the SPAM or TRASH label (e.g., "SPAM"/"TRASH" or the
constant/labelId used in your code) and set params["includeSpamTrash"] = True
before returning/issuing the users.messages.list call so Spam/Trash messages are
included when those mailboxes are selected.
In `@static/js/jobs.js`:
- Around line 165-167: The interval-based polling in startPolling uses
setInterval(() => this.pollStatus(), 500) which can fire overlapping async
pollStatus() calls; add an in-flight guard (e.g., a boolean this._pollInFlight)
to startPolling/pollStatus logic: in startPolling keep stopPolling and
setInterval but have the callback check and return if this._pollInFlight is
true, otherwise set it true, await this.pollStatus(), then set it false in
finally; also implement the same guard for the other polling block referenced
(the code around lines 177-184) and ensure stopPolling clears the interval and
resets the in-flight flag.
- Line 35: The Biome lint error comes from the arrow callback in
mailboxSel.querySelectorAll('.custom-label-opt').forEach(el => el.remove())
implicitly returning a value; change the callback to a block body so it does not
return anything (e.g. use forEach(el => { el.remove(); }) or replace with an
explicit loop) — update the statement that calls
querySelectorAll('.custom-label-opt').forEach to use a non-returning callback.
- Around line 123-132: The cancelJob function currently disables the
jobCancelBtn and never re-enables it if the cancel request fails; update
cancelJob to check the fetch response (use response.ok) and treat network/errors
as failures, and on failure restore jobCancelBtn.disabled = false and
jobCancelBtn.textContent = 'Cancel' (or original label) so the user can retry;
also consider showing a transient error message, but at minimum re-enable the
button and reset its label when fetch throws or response.ok is false.
---
Nitpick comments:
In `@static/js/jobs.js`:
- Around line 264-269: The monkey-patch for GmailCleaner.UI.showView drops the
original signature and return value; change the wrapper to forward all arguments
and return the original result by calling original.apply(this, arguments) (or
original.call with forwarded args) and store its return value, then trigger
GmailCleaner.Jobs.loadLabels() only when the first argument equals 'job', and
finally return the stored result so GmailCleaner.UI.showView preserves
compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0bf51214-de9e-423f-ac41-aaaf99a05947
📒 Files selected for processing (10)
app/api/actions.pyapp/api/status.pyapp/core/state.pyapp/models/__init__.pyapp/models/schemas.pyapp/services/__init__.pyapp/services/gmail/__init__.pyapp/services/gmail/jobs.pystatic/js/jobs.jstemplates/index.html
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/init.py
Comment on lines
+120
to
+128
| def base_list_params(include_spam_trash: bool = False) -> dict: | ||
| params: dict = {"userId": "me", "maxResults": 500} | ||
| if query: | ||
| params["q"] = query | ||
| if mailbox: | ||
| params["labelIds"] = [mailbox] | ||
| if include_spam_trash: | ||
| params["includeSpamTrash"] = True | ||
| return params |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the file to understand its structure
wc -l app/services/gmail/jobs.pyRepository: Gururagavendra/gmail-cleaner
Length of output: 102
🏁 Script executed:
# Read the file to see the full context, focusing on the mentioned lines
head -n 370 app/services/gmail/jobs.py | tail -n +110Repository: Gururagavendra/gmail-cleaner
Length of output: 9978
🏁 Script executed:
# Also search for where SPAM and TRASH constants are defined or used
rg -n "SPAM|TRASH" app/services/gmail/jobs.py | head -20Repository: Gururagavendra/gmail-cleaner
Length of output: 150
🏁 Script executed:
# Check if there's a UI component that exposes mailbox options
fd . --type f -name "*.tsx" -o -name "*.ts" -o -name "*.jsx" -o -name "*.js" | xargs rg -l "SPAM|TRASH" | head -10Repository: Gururagavendra/gmail-cleaner
Length of output: 418
🏁 Script executed:
# Look for where mailbox parameter comes from and what values it can be
rg -B 5 "mailbox" app/services/gmail/jobs.py | head -50Repository: Gururagavendra/gmail-cleaner
Length of output: 1212
🏁 Script executed:
# Check the UI to see what mailbox options are available
cat templates/index.html | grep -i "spam\|trash\|mailbox" -A 2 -B 2Repository: Gururagavendra/gmail-cleaner
Length of output: 2236
🏁 Script executed:
# Search for label mappings or constants
rg "SPAM|TRASH" app/ -t py | grep -v ".pyc"Repository: Gururagavendra/gmail-cleaner
Length of output: 596
🏁 Script executed:
# Find the exact lines 352-359 in the file
sed -n '352,359p' app/services/gmail/jobs.pyRepository: Gururagavendra/gmail-cleaner
Length of output: 332
🏁 Script executed:
# Let me check the full context of the main action loop to confirm which actions call base_list_params()
sed -n '120,250p' app/services/gmail/jobs.pyRepository: Gururagavendra/gmail-cleaner
Length of output: 5360
🏁 Script executed:
# Let's also verify the exact structure at 352-359 and surrounding code
sed -n '340,365p' app/services/gmail/jobs.pyRepository: Gururagavendra/gmail-cleaner
Length of output: 967
🏁 Script executed:
# Search for all calls to base_list_params to understand the issue scope
rg "base_list_params" app/services/gmail/jobs.py -B 2 -A 2Repository: Gururagavendra/gmail-cleaner
Length of output: 761
🏁 Script executed:
# Let me verify the Gmail API documentation claim about includeSpamTrash
web_search "Gmail API users.messages.list includeSpamTrash SPAM TRASH"Repository: Gururagavendra/gmail-cleaner
Length of output: 122
🌐 Web query:
Gmail API users.messages.list includeSpamTrash parameter SPAM TRASH labels
💡 Result:
In users.messages.list, includeSpamTrash controls whether the list operation is allowed to return messages that are in Gmail’s system labels SPAM and/or TRASH. By default it’s false, so those messages are excluded from results. [1]
- To list spam and/or trash, set
includeSpamTrash=true. [1] - You can also filter by
labelIds, and the system label IDs for those folders areSPAMandTRASH(used the same way as other label IDs). [1] - In practice, if you want only spam or only trash, do both:
includeSpamTrash=truelabelIds=['SPAM']orlabelIds=['TRASH'][1]
Example:
GET https://gmail.googleapis.com/gmail/v1/users/me/messages?includeSpamTrash=true&labelIds=SPAMSources: Gmail API reference for users.messages.list. [1]
[1] (developers.google.com)
Citations:
Enable includeSpamTrash whenever filtering by SPAM or TRASH mailboxes.
The UI exposes SPAM and TRASH as mailbox options, but the list builders don't set includeSpamTrash for those paths. Gmail's users.messages.list excludes Spam and Trash messages by default unless includeSpamTrash is true, so these operations silently return no results even though the mailbox filter is set.
This affects two code locations:
base_list_params()in the main action loop (delete, archive, label, mark_important)_run_find_subscriptions()when scanning a SPAM/TRASH mailbox
🐛 Proposed fixes
def base_list_params(include_spam_trash: bool = False) -> dict:
params: dict = {"userId": "me", "maxResults": 500}
if query:
params["q"] = query
if mailbox:
params["labelIds"] = [mailbox]
- if include_spam_trash:
+ if include_spam_trash or mailbox in {"SPAM", "TRASH"}:
params["includeSpamTrash"] = True
return params list_params: dict = {"userId": "me", "maxResults": 500}
if page_token:
list_params["pageToken"] = page_token
if query:
list_params["q"] = query
if mailbox:
list_params["labelIds"] = [mailbox]
+ if mailbox in {"SPAM", "TRASH"}:
+ list_params["includeSpamTrash"] = True📝 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
| def base_list_params(include_spam_trash: bool = False) -> dict: | |
| params: dict = {"userId": "me", "maxResults": 500} | |
| if query: | |
| params["q"] = query | |
| if mailbox: | |
| params["labelIds"] = [mailbox] | |
| if include_spam_trash: | |
| params["includeSpamTrash"] = True | |
| return params | |
| def base_list_params(include_spam_trash: bool = False) -> dict: | |
| params: dict = {"userId": "me", "maxResults": 500} | |
| if query: | |
| params["q"] = query | |
| if mailbox: | |
| params["labelIds"] = [mailbox] | |
| if include_spam_trash or mailbox in {"SPAM", "TRASH"}: | |
| params["includeSpamTrash"] = True | |
| return params |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/gmail/jobs.py` around lines 120 - 128, The Gmail list helper
base_list_params() and the mailbox scan _run_find_subscriptions() don't set
includeSpamTrash when mailbox is SPAM or TRASH, so Gmail omits those messages;
update both places (base_list_params and the mailbox-scanning logic in
_run_find_subscriptions) to detect when mailbox equals the SPAM or TRASH label
(e.g., "SPAM"/"TRASH" or the constant/labelId used in your code) and set
params["includeSpamTrash"] = True before returning/issuing the
users.messages.list call so Spam/Trash messages are included when those
mailboxes are selected.
Comment on lines
+285
to
+296
| unsubscribe_data: dict[str, dict] = defaultdict( | ||
| lambda: { | ||
| "link": None, | ||
| "count": 0, | ||
| "subjects": [], | ||
| "type": None, | ||
| "sender": "", | ||
| "email": "", | ||
| "first_date": None, | ||
| "last_date": None, | ||
| "message_ids": [], | ||
| } |
Contributor
There was a problem hiding this comment.
Don't accumulate every message ID for an unlimited scan.
find_subscriptions advertises a full-mailbox scan, but this path keeps every matching Gmail ID per domain and then copies those arrays into state.scan_results. For large senders, the result object now scales with total matching messages instead of sender count, which is an easy way to blow up memory and any response payload built from that state.
Also applies to: 327-328, 404-424
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/gmail/jobs.py` around lines 285 - 296, The code accumulates
every Gmail message ID into unsubscribe_data["message_ids"] (see
unsubscribe_data creation and usages in find_subscriptions and the sections that
append/copy message IDs), which grows unbounded and can OOM; change this to
track only a capped sample and an aggregate count: introduce a MAX_SAVED_IDS
constant, replace "message_ids" with "sample_message_ids" and "message_count"
(or equivalent) in the unsubscribe_data default, when adding an ID push into
sample_message_ids only if length < MAX_SAVED_IDS and always increment
message_count, and ensure when populating state.scan_results you copy
sample_message_ids and message_count instead of the full ID list (apply the same
capped-sample logic in the other places that append/copy IDs referenced in the
diff).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/services/gmail/jobs.py (1)
356-363:⚠️ Potential issue | 🟠 MajorSet
includeSpamTrashin the subscription scan path too.You fixed this in
base_list_params(), but_run_find_subscriptions()still buildsusers.messages.list()params withoutincludeSpamTrashforSPAM/TRASH. Gmail omits those messages by default, so scans from those mailboxes can come back empty even though the filter is selected.Does Gmail API users.messages.list require includeSpamTrash=true when labelIds contains SPAM or TRASH?🛠️ Proposed fix
if query: list_params["q"] = query if mailbox: list_params["labelIds"] = [mailbox] + if mailbox in {"SPAM", "TRASH"}: + list_params["includeSpamTrash"] = True result = _execute(service.users().messages().list(**list_params))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/gmail/jobs.py` around lines 356 - 363, The _run_find_subscriptions() function builds list_params for users.messages.list but doesn't set includeSpamTrash, so scans for SPAM/TRASH can miss messages; update _run_find_subscriptions() to add list_params["includeSpamTrash"] = True when mailbox (or labelIds) indicates SPAM or TRASH (match the same logic used in base_list_params()), ensuring users.messages.list calls include includeSpamTrash=True for those mailboxes.static/js/scanner.js (1)
467-468:⚠️ Potential issue | 🟠 MajorReset skipped buttons before continuing the scanned bulk-delete loop.
When
scope === 'scanned'andr.message_idsis empty, thiscontinueexits before any cleanup runs. That leaves the current button disabled as “Deleting...” even though nothing was deleted.🛠️ Proposed fix
if (scope === 'scanned') { - if (!r.message_ids || r.message_ids.length === 0) continue; + if (!r.message_ids || r.message_ids.length === 0) { + if (btn) { + btn.classList.remove('btn-deleting'); + btn.disabled = false; + btn.innerHTML = 'Delete'; + } + console.warn('Skipping scanned delete: missing message_ids for', r.domain); + continue; + } response = await fetch('/api/delete-by-ids', {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/js/scanner.js` around lines 467 - 468, When scope === 'scanned' and r.message_ids is empty the loop does an early continue without resetting the UI button state; locate the scanned bulk-delete branch around the "if (scope === 'scanned')" check in scanner.js and before the "continue" ensure you reset the current bulk action button (the button variable used when setting "Deleting..." or disabling it) back to its normal enabled state and label (or call the existing reset function used elsewhere), then continue; alternatively perform the existing cleanup/reset logic that's run after deletions (e.g., enable button, restore text, clear any spinner) and only then continue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@static/js/jobs.js`:
- Around line 231-240: The btn.onclick handler currently swallows fetch errors
and proceeds to render stale GmailCleaner.results; update the fetch logic in the
onclick async function (the handler assigned to btn.onclick) so that you capture
the error (catch (err)) and on failure or non-ok response you clear
GmailCleaner.results (set to empty array or null) instead of leaving stale data,
and only call GmailCleaner.Scanner.displayResults(...) and
GmailCleaner.UI.showView('unsubscribe') when the fetch succeeded and you have
fresh results; keep this.resetToForm() but ensure error cases do not render or
allow actions on stale results.
In `@static/js/scanner.js`:
- Line 347: The dialog is showing r.count but the delete API only deletes the
saved IDs; update the call site(s) that invoke showDeleteScopeDialog (currently
passing r.count) to pass the actual saved-ID count instead (e.g. use
r.saved_count or r.message_ids.length as provided by the response) so the UI
copy matches the deletable set; change both occurrences around the
showDeleteScopeDialog invocation (the line with "const scope = await
this.showDeleteScopeDialog(r.count, r.domain);" and the similar call at lines
~415-416) to use the saved ID count property.
---
Duplicate comments:
In `@app/services/gmail/jobs.py`:
- Around line 356-363: The _run_find_subscriptions() function builds list_params
for users.messages.list but doesn't set includeSpamTrash, so scans for
SPAM/TRASH can miss messages; update _run_find_subscriptions() to add
list_params["includeSpamTrash"] = True when mailbox (or labelIds) indicates SPAM
or TRASH (match the same logic used in base_list_params()), ensuring
users.messages.list calls include includeSpamTrash=True for those mailboxes.
In `@static/js/scanner.js`:
- Around line 467-468: When scope === 'scanned' and r.message_ids is empty the
loop does an early continue without resetting the UI button state; locate the
scanned bulk-delete branch around the "if (scope === 'scanned')" check in
scanner.js and before the "continue" ensure you reset the current bulk action
button (the button variable used when setting "Deleting..." or disabling it)
back to its normal enabled state and label (or call the existing reset function
used elsewhere), then continue; alternatively perform the existing cleanup/reset
logic that's run after deletions (e.g., enable button, restore text, clear any
spinner) and only then continue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83dce574-f784-4a7c-891c-c286a4bd926f
📒 Files selected for processing (4)
app/api/actions.pyapp/services/gmail/jobs.pystatic/js/jobs.jsstatic/js/scanner.js
Comment on lines
+231
to
+240
| btn.onclick = async () => { | ||
| try { | ||
| const res = await fetch('/api/results'); | ||
| if (res.ok) { | ||
| GmailCleaner.results = await res.json(); | ||
| } | ||
| } catch (_) {} | ||
| this.resetToForm(); | ||
| GmailCleaner.Scanner.displayResults(GmailCleaner.results); | ||
| GmailCleaner.UI.showView('unsubscribe'); |
Contributor
There was a problem hiding this comment.
Don’t open the unsubscribe view with stale results.
If /api/results fails here, the catch is swallowed and the code still renders whatever was already in GmailCleaner.results. After a previous scan/job, that can show stale senders and let users delete or unsubscribe the wrong conversations.
🛠️ Proposed fix
btn.onclick = async () => {
+ let results;
try {
const res = await fetch('/api/results');
- if (res.ok) {
- GmailCleaner.results = await res.json();
- }
- } catch (_) {}
+ if (!res.ok) throw new Error('results_load_failed');
+ results = await res.json();
+ } catch (_) {
+ alert('Failed to load scan results. Please try again.');
+ return;
+ }
+ GmailCleaner.results = results;
this.resetToForm();
- GmailCleaner.Scanner.displayResults(GmailCleaner.results);
+ GmailCleaner.Scanner.displayResults();
GmailCleaner.UI.showView('unsubscribe');
};📝 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
| btn.onclick = async () => { | |
| try { | |
| const res = await fetch('/api/results'); | |
| if (res.ok) { | |
| GmailCleaner.results = await res.json(); | |
| } | |
| } catch (_) {} | |
| this.resetToForm(); | |
| GmailCleaner.Scanner.displayResults(GmailCleaner.results); | |
| GmailCleaner.UI.showView('unsubscribe'); | |
| btn.onclick = async () => { | |
| let results; | |
| try { | |
| const res = await fetch('/api/results'); | |
| if (!res.ok) throw new Error('results_load_failed'); | |
| results = await res.json(); | |
| } catch (_) { | |
| alert('Failed to load scan results. Please try again.'); | |
| return; | |
| } | |
| GmailCleaner.results = results; | |
| this.resetToForm(); | |
| GmailCleaner.Scanner.displayResults(); | |
| GmailCleaner.UI.showView('unsubscribe'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@static/js/jobs.js` around lines 231 - 240, The btn.onclick handler currently
swallows fetch errors and proceeds to render stale GmailCleaner.results; update
the fetch logic in the onclick async function (the handler assigned to
btn.onclick) so that you capture the error (catch (err)) and on failure or
non-ok response you clear GmailCleaner.results (set to empty array or null)
instead of leaving stale data, and only call
GmailCleaner.Scanner.displayResults(...) and
GmailCleaner.UI.showView('unsubscribe') when the fetch succeeded and you have
fresh results; keep this.resetToForm() but ensure error cases do not render or
allow actions on stale results.
| const r = GmailCleaner.results[index]; | ||
| const btn = document.getElementById('del-' + index); | ||
|
|
||
| const scope = await this.showDeleteScopeDialog(r.count, r.domain); |
Contributor
There was a problem hiding this comment.
Use the saved ID count for the “Delete scanned” scope.
These dialogs advertise r.count, but /api/delete-by-ids can only trash the IDs in r.message_ids. In app/services/gmail/jobs.py Lines 29-31 and Lines 331-332, the job path caps saved IDs per sender, so high-volume senders can be offered a full scanned delete while only a subset is actually deletable.
🛠️ Proposed fix
- const scope = await this.showDeleteScopeDialog(r.count, r.domain);
+ const scannedCount = Array.isArray(r.message_ids) ? r.message_ids.length : 0;
+ const scope = await this.showDeleteScopeDialog(scannedCount, r.domain);
@@
- const scannedTotal = items.reduce((sum, { r }) => sum + (r.count || 0), 0);
+ const scannedTotal = items.reduce(
+ (sum, { r }) => sum + (Array.isArray(r.message_ids) ? r.message_ids.length : 0),
+ 0,
+ );Also applies to: 415-416
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@static/js/scanner.js` at line 347, The dialog is showing r.count but the
delete API only deletes the saved IDs; update the call site(s) that invoke
showDeleteScopeDialog (currently passing r.count) to pass the actual saved-ID
count instead (e.g. use r.saved_count or r.message_ids.length as provided by the
response) so the UI copy matches the deletable set; change both occurrences
around the showDeleteScopeDialog invocation (the line with "const scope = await
this.showDeleteScopeDialog(r.count, r.domain);" and the similar call at lines
~415-416) to use the saved ID count property.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a delete column to the unsubscribe email results. Allows user to delete the shown emails or all emails from that sender. This allows you to better manage historical subscription emails; leaving them or deleting them when you unsubscribe.
Checklist
Related Issues
NA