Support inbox-only filters, prevent 429s from Google getting in the w… - #113
Open
kevin-jones wants to merge 1 commit into
Open
Support inbox-only filters, prevent 429s from Google getting in the w…#113kevin-jones wants to merge 1 commit into
kevin-jones wants to merge 1 commit into
Conversation
…ay, ensure browser cache doesn't prevent new features working after container rebuild
Contributor
WalkthroughAdds a ChangesMail Scope Filtering
Asset Content-Based Cache Busting
Sequence Diagram(s)sequenceDiagram
participant Browser
participant FiltersJS as filters.js
participant DeleteJS as delete.js
participant APIActions as /api/delete-emails
participant GmailService as delete_emails_by_sender
Browser->>FiltersJS: user selects "Inbox" or "All" radio
Browser->>DeleteJS: startScan() called
DeleteJS->>FiltersJS: Filters.get()
FiltersJS-->>DeleteJS: { mail_scope: "inbox"|"all", ... }
DeleteJS->>DeleteJS: store GmailCleaner.deleteMailScope
Browser->>DeleteJS: delete sender clicked
DeleteJS->>APIActions: POST { sender, mail_scope }
APIActions->>GmailService: delete_emails_by_sender(sender, mail_scope)
GmailService->>GmailService: _get_mail_scope(), _build_scoped_query(), _get_scope_params()
GmailService-->>APIActions: { success, count }
APIActions-->>Browser: JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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 (1)
static/js/delete.js (1)
300-310:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle non-2xx bulk-delete start responses before polling.
If
/api/delete-emails-bulkreturns an error, polling still starts. That can leave the overlay in a misleading state even though no background task started.Suggested fix
- await fetch('/api/delete-emails-bulk', { + const response = await fetch('/api/delete-emails-bulk', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ senders: senderEmails, mail_scope: GmailCleaner.deleteMailScope || 'inbox' }) }); + + if (!response.ok) { + const errorData = await response.json().catch(() => ({})); + throw new Error(errorData.detail || `Request failed with status ${response.status}`); + } // Poll for progress this.pollDeleteProgress(checkboxes);🤖 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 `@static/js/delete.js` around lines 300 - 310, The fetch call to /api/delete-emails-bulk does not validate the response status before proceeding to call this.pollDeleteProgress. If the endpoint returns a non-2xx error status, polling will still start even though the bulk delete failed, leaving the UI in a misleading state. Add a check after the fetch call to verify the response.ok property (or check response.status), and only call this.pollDeleteProgress if the response indicates success. Handle the error case appropriately by either throwing an error, returning early, or displaying an error message to prevent the polling from starting.
🧹 Nitpick comments (3)
tests/unit/services/gmail/test_delete_service.py (1)
5-7: ⚡ Quick winAdd an autouse state-reset fixture for test isolation.
These tests mutate shared
app.core.state; an explicit reset fixture prevents order-dependent bleed as the suite grows.Suggested fixture
+import pytest from app.core import state from app.services.gmail import delete as delete_service + + +@pytest.fixture(autouse=True) +def reset_delete_state(): + state.reset_delete_scan() + state.reset_delete_bulk() + state.delete_scan_results = [] + yield + state.reset_delete_scan() + state.reset_delete_bulk() + state.delete_scan_results = []🤖 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 `@tests/unit/services/gmail/test_delete_service.py` around lines 5 - 7, The tests in this module mutate the shared app.core.state object without resetting it between test runs, which can cause test order dependencies and failures as the test suite grows. Add an autouse pytest fixture to this test file that resets app.core.state to a clean state before and after each test runs. This fixture should be defined at the module level and use the `@pytest.fixture` decorator with autouse=True parameter to ensure it executes automatically for every test in the file without needing explicit invocation.tests/unit/api/test_api_actions.py (1)
155-163: ⚡ Quick winAdd API tests for explicit
mail_scope="all"forwarding.Current assertions cover the default scope; adding explicit
"all"cases will lock down the non-default contract at the endpoint boundary.Suggested tests
class TestDeleteEmailsEndpoint: @@ `@patch`("app.api.actions.delete_emails_by_sender") def test_delete_emails_by_sender(self, mock_delete, client): @@ mock_delete.assert_called_once_with("newsletter@example.com", "inbox") + + `@patch`("app.api.actions.delete_emails_by_sender") + def test_delete_emails_by_sender_all_scope(self, mock_delete, client): + mock_delete.return_value = {"success": True, "deleted": 10} + response = client.post( + "/api/delete-emails", + json={"sender": "newsletter@example.com", "mail_scope": "all"}, + ) + assert response.status_code == 200 + mock_delete.assert_called_once_with("newsletter@example.com", "all") @@ class TestDeleteBulkEndpoint: @@ `@patch`("app.api.actions.delete_emails_bulk_background") def test_delete_bulk_with_valid_senders(self, mock_delete, client): @@ mock_delete.assert_called_once_with(senders, "inbox") + + `@patch`("app.api.actions.delete_emails_bulk_background") + def test_delete_bulk_with_all_scope(self, mock_delete, client): + senders = ["sender1@example.com", "sender2@example.com"] + response = client.post( + "/api/delete-emails-bulk", + json={"senders": senders, "mail_scope": "all"}, + ) + assert response.status_code == 200 + assert response.json() == {"status": "started"} + mock_delete.assert_called_once_with(senders, "all")Also applies to: 169-177
🤖 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 `@tests/unit/api/test_api_actions.py` around lines 155 - 163, Add new test cases to cover the explicit mail_scope="all" parameter for the delete-emails endpoint. The current test_delete_emails_by_sender test only validates the default scope behavior (inbox). Create additional test methods that send a POST request to /api/delete-emails with the JSON payload including both the sender and an explicit mail_scope parameter set to "all", then assert that the mocked delete_emails_by_sender function is called with the sender and "all" as arguments instead of the default "inbox" scope. This ensures the non-default mail_scope contract is properly validated at the endpoint boundary.app/models/schemas.py (1)
101-109: ⚡ Quick winCentralize
mail_scopenormalization in one helper.The same validation logic is copied in three models. Extracting one normalizer avoids drift and keeps behavior consistent when this contract changes.
Proposed refactor
+def _normalize_mail_scope(v: Optional[str]) -> str: + if v is None or v == "": + return "inbox" + value = v.strip().lower() + if value not in ("inbox", "all"): + raise ValueError('mail_scope must be either "inbox" or "all"') + return value + class FiltersModel(BaseModel): @@ def validate_mail_scope(cls, v) -> str: - if v is None or v == "": - return "inbox" - value = v.strip().lower() - if value not in ("inbox", "all"): - raise ValueError('mail_scope must be either "inbox" or "all"') - return value + return _normalize_mail_scope(v) @@ class DeleteEmailsRequest(BaseModel): @@ def validate_mail_scope(cls, v) -> str: - if v is None or v == "": - return "inbox" - value = v.strip().lower() - if value not in ("inbox", "all"): - raise ValueError('mail_scope must be either "inbox" or "all"') - return value + return _normalize_mail_scope(v) @@ class DeleteBulkRequest(BaseModel): @@ def validate_mail_scope(cls, v) -> str: - if v is None or v == "": - return "inbox" - value = v.strip().lower() - if value not in ("inbox", "all"): - raise ValueError('mail_scope must be either "inbox" or "all"') - return value + return _normalize_mail_scope(v)Also applies to: 160-168, 177-185
🤖 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 `@app/models/schemas.py` around lines 101 - 109, Extract the mail_scope validation logic from the validate_mail_scope method that appears in three separate models into a single centralized helper function. Create a reusable function (outside of the class definitions) that handles the normalization and validation logic for mail_scope values (checking for None or empty string, stripping and lowercasing, and validating against the allowed values "inbox" and "all"). Then replace each of the three copies of the validate_mail_scope validator method with a call to this centralized helper function to ensure consistent behavior and avoid duplication.
🤖 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 `@app/services/gmail/delete.py`:
- Around line 149-174: The retry_message_get function currently increments the
processed counter even when all retry attempts are exhausted and the message
fetch fails. This makes failed messages appear as successful in the aggregation
count, masking incomplete results. Modify the retry_message_get function to only
increment processed when process_message_response successfully completes, and
ensure that when all three retry attempts fail, the function returns early
without incrementing the counter or signals the failure state so callers can
detect that the message was not successfully processed.
In `@static/css/filters.css`:
- Around line 57-79: The filter-radio component is missing a visible keyboard
focus state. Add a CSS rule for the `.filter-radio input:focus + span` selector
that applies a visible focus style (such as an outline, box-shadow, or border)
to the span element. This will make the focused state clearly visible when users
navigate through the radios using the keyboard, improving accessibility and
usability.
---
Outside diff comments:
In `@static/js/delete.js`:
- Around line 300-310: The fetch call to /api/delete-emails-bulk does not
validate the response status before proceeding to call this.pollDeleteProgress.
If the endpoint returns a non-2xx error status, polling will still start even
though the bulk delete failed, leaving the UI in a misleading state. Add a check
after the fetch call to verify the response.ok property (or check
response.status), and only call this.pollDeleteProgress if the response
indicates success. Handle the error case appropriately by either throwing an
error, returning early, or displaying an error message to prevent the polling
from starting.
---
Nitpick comments:
In `@app/models/schemas.py`:
- Around line 101-109: Extract the mail_scope validation logic from the
validate_mail_scope method that appears in three separate models into a single
centralized helper function. Create a reusable function (outside of the class
definitions) that handles the normalization and validation logic for mail_scope
values (checking for None or empty string, stripping and lowercasing, and
validating against the allowed values "inbox" and "all"). Then replace each of
the three copies of the validate_mail_scope validator method with a call to this
centralized helper function to ensure consistent behavior and avoid duplication.
In `@tests/unit/api/test_api_actions.py`:
- Around line 155-163: Add new test cases to cover the explicit mail_scope="all"
parameter for the delete-emails endpoint. The current
test_delete_emails_by_sender test only validates the default scope behavior
(inbox). Create additional test methods that send a POST request to
/api/delete-emails with the JSON payload including both the sender and an
explicit mail_scope parameter set to "all", then assert that the mocked
delete_emails_by_sender function is called with the sender and "all" as
arguments instead of the default "inbox" scope. This ensures the non-default
mail_scope contract is properly validated at the endpoint boundary.
In `@tests/unit/services/gmail/test_delete_service.py`:
- Around line 5-7: The tests in this module mutate the shared app.core.state
object without resetting it between test runs, which can cause test order
dependencies and failures as the test suite grows. Add an autouse pytest fixture
to this test file that resets app.core.state to a clean state before and after
each test runs. This fixture should be defined at the module level and use the
`@pytest.fixture` decorator with autouse=True parameter to ensure it executes
automatically for every test in the file without needing explicit invocation.
🪄 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: CHILL
Plan: Pro
Run ID: b81e8e22-98fa-4564-b07a-9ae3da244fd3
📒 Files selected for processing (10)
app/api/actions.pyapp/main.pyapp/models/schemas.pyapp/services/gmail/delete.pystatic/css/filters.cssstatic/js/delete.jsstatic/js/filters.jstemplates/index.htmltests/unit/api/test_api_actions.pytests/unit/services/gmail/test_delete_service.py
Comment on lines
+149
to
+174
| def retry_message_get(msg_id: str) -> None: | ||
| nonlocal processed | ||
|
|
||
| for attempt in range(3): | ||
| try: | ||
| if attempt > 0: | ||
| time.sleep(0.5 * attempt) | ||
| response = ( | ||
| service.users() | ||
| .messages() | ||
| .get( | ||
| userId="me", | ||
| id=msg_id, | ||
| format="metadata", | ||
| metadataHeaders=["From", "Subject", "Date"], | ||
| ) | ||
| .execute() | ||
| ) | ||
| process_message_response(response) | ||
| processed += 1 | ||
| return | ||
| except Exception as e: | ||
| if attempt == 2: | ||
| logger.warning("Failed to fetch message %s: %s", msg_id, e) | ||
| processed += 1 | ||
|
|
Contributor
There was a problem hiding this comment.
Surface exhausted metadata retries; current flow still drops messages silently.
When retries fail, the message is skipped from aggregation and only logged, but status still looks fully successful. That can produce incomplete sender counts without user-visible signal.
Proposed fix
processed = 0
batch_size = 25
+ failed_metadata_ids: list[str] = []
@@
def retry_message_get(msg_id: str) -> None:
nonlocal processed
@@
except Exception as e:
if attempt == 2:
logger.warning("Failed to fetch message %s: %s", msg_id, e)
+ failed_metadata_ids.append(msg_id)
processed += 1
@@
state.delete_scan_status["progress"] = progress
state.delete_scan_status["message"] = f"Scanned {processed}/{total} emails"
@@
+ if failed_metadata_ids:
+ state.delete_scan_status["error"] = (
+ f"Skipped metadata for {len(failed_metadata_ids)} message(s) after retries"
+ )
+
# Sort by countAlso applies to: 208-214
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 170-170: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@app/services/gmail/delete.py` around lines 149 - 174, The retry_message_get
function currently increments the processed counter even when all retry attempts
are exhausted and the message fetch fails. This makes failed messages appear as
successful in the aggregation count, masking incomplete results. Modify the
retry_message_get function to only increment processed when
process_message_response successfully completes, and ensure that when all three
retry attempts fail, the function returns early without incrementing the counter
or signals the failure state so callers can detect that the message was not
successfully processed.
Comment on lines
+57
to
+79
| .filter-radio input { | ||
| position: absolute; | ||
| opacity: 0; | ||
| pointer-events: none; | ||
| } | ||
|
|
||
| .filter-radio span { | ||
| display: block; | ||
| min-width: 54px; | ||
| padding: 6px 12px; | ||
| text-align: center; | ||
| font-size: 13px; | ||
| line-height: 1.2; | ||
| } | ||
|
|
||
| .filter-radio + .filter-radio { | ||
| border-left: 1px solid var(--border-color); | ||
| } | ||
|
|
||
| .filter-radio input:checked + span { | ||
| background: var(--primary-color); | ||
| color: white; | ||
| } |
Contributor
There was a problem hiding this comment.
Add visible keyboard focus state for mail-scope radios.
Lines 57-79 style checked state, but there’s no visible focus style when tabbing through the radios. This makes keyboard interaction hard to use reliably.
Suggested fix
.filter-radio input:checked + span {
background: var(--primary-color);
color: white;
}
+
+.filter-radio input:focus-visible + span {
+ outline: 2px solid var(--primary-color);
+ outline-offset: -2px;
+}📝 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
| .filter-radio input { | |
| position: absolute; | |
| opacity: 0; | |
| pointer-events: none; | |
| } | |
| .filter-radio span { | |
| display: block; | |
| min-width: 54px; | |
| padding: 6px 12px; | |
| text-align: center; | |
| font-size: 13px; | |
| line-height: 1.2; | |
| } | |
| .filter-radio + .filter-radio { | |
| border-left: 1px solid var(--border-color); | |
| } | |
| .filter-radio input:checked + span { | |
| background: var(--primary-color); | |
| color: white; | |
| } | |
| .filter-radio input { | |
| position: absolute; | |
| opacity: 0; | |
| pointer-events: none; | |
| } | |
| .filter-radio span { | |
| display: block; | |
| min-width: 54px; | |
| padding: 6px 12px; | |
| text-align: center; | |
| font-size: 13px; | |
| line-height: 1.2; | |
| } | |
| .filter-radio + .filter-radio { | |
| border-left: 1px solid var(--border-color); | |
| } | |
| .filter-radio input:checked + span { | |
| background: var(--primary-color); | |
| color: white; | |
| } | |
| .filter-radio input:focus-visible + span { | |
| outline: 2px solid var(--primary-color); | |
| outline-offset: -2px; | |
| } |
🤖 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 `@static/css/filters.css` around lines 57 - 79, The filter-radio component is
missing a visible keyboard focus state. Add a CSS rule for the `.filter-radio
input:focus + span` selector that applies a visible focus style (such as an
outline, box-shadow, or border) to the span element. This will make the focused
state clearly visible when users navigate through the radios using the keyboard,
improving accessibility and usability.
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
When using gmail-cleaner's "Delete Emails" feature I realised that the tool was scanning ALL of my emails, not only those in my Gmail "inbox". This felt wrong to me as I was using the tool to Delete or Archive email and get them out of my inbox. But after Archive/Delete actions were completed, those emails could still appear in the scan, so it felt like I was seeing the same archived emails again and again.
I have added an inbox-only filter to make my experience better, but in doing so I found bugs with 429 errors from the Google API not being handled and the API being used more than necessary. When rebuilding the container, sometimes I had to hard-refresh to get new JS changes loaded - this again felt wrong and therefore I added asset hashing to auto-update broser caches.
Changes include:
Checklist
All scope is accepted and does not force INBOX filtering
Changes
mail_scopefield acrossFiltersModel,DeleteEmailsRequest, andDeleteBulkRequestschemas; defaults to inbox to limit scans to user's inbox onlydelete_emails_by_sender(),delete_emails_bulk(), anddelete_emails_bulk_background()functions to accept and usemail_scopeparameter throughout the delete scan and deletion flowsin:inboxfilter) and label filters (labelIds=["INBOX"]) for consistent filtering across operationsget_asset_hash()helper to compute content hashes from frontend assets instatic/andtemplates/, enabling automatic browser cache updates when container rebuilds occur without manual hard-refreshesGmailCleaner.DeleteandGmailCleaner.FiltersJavaScript to capture and passmail_scopein delete request payloads; added UI controls and state management for mail scope selection