Move auth/session state to client and enable multi-user support - #92
Move auth/session state to client and enable multi-user support#92daddyparodz wants to merge 10 commits into
Conversation
|
Warning Rate limit exceeded@daddyparodz has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughReplaces global app state with per-session SessionState and SessionStore, injects SessionContext via FastAPI dependencies, threads session/token into services and background tasks, centralizes client-side session/token handling with GmailCleaner.apiFetch, updates UI/responsive behavior and tests, and adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant ClientJS as GmailCleaner (JS)
participant API as FastAPI
participant DI as get_session_context
participant Store as SessionStore
participant Auth as Auth Service
participant Gmail as Gmail Service
Browser->>ClientJS: user triggers action (scan / sign-in)
ClientJS->>API: HTTP request (adds X-Session-Id, X-Auth-Token)
API->>DI: resolve get_session_context(request)
DI->>Store: get_session_state(session_id)
Store-->>DI: SessionState
DI-->>API: SessionContext(session, token_json)
API->>Auth: check/start auth (session, token_json, host, scheme)
alt needs auth
API-->>ClientJS: 401 + pending_auth_url
ClientJS->>Browser: open auth popup (pollAuthUrl)
Browser->>API: auth callback / poll requests
API->>Auth: get_gmail_service(session, token_json, host, scheme)
Auth-->>API: token_json / success
API-->>ClientJS: token_json
ClientJS->>ClientJS: GmailCleaner.Session.setToken(token_json)
else authenticated
API->>Gmail: perform action (scan/download/delete) with session
Gmail-->>API: updates session.* (status/results)
API-->>ClientJS: 200 + data
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
static/js/delete.js (2)
121-124: Error path may poll indefinitely.When an error occurs in polling, the code retries without a maximum attempt limit. If the server is persistently unavailable, this could poll forever.
This is a minor issue since the user can navigate away, but consider adding a retry limit for robustness.
462-464: UseapiFetchwith blob download pattern for the CSV endpoint.The direct
window.location.hrefnavigation won't reliably carry the session context established byapiFetchcalls. While browsers send cookies, the current flow usesX-Session-Idheaders for session management. Without that header, the endpoint falls back to thegc_sessioncookie or the"default"session, causing session mismatch in multi-user scenarios.Consider using the blob download pattern already used in
scanner.js(lines 326–330), or ensureapiFetchhandles binary responses properly so you can fetch the CSV and trigger the download client-side.app/services/gmail/scan.py (1)
146-162: String comparison fallback for dates may give incorrect results.The fallback to string comparison (lines 148, 161) won't work correctly for RFC 2822 date strings since they don't sort lexicographically. However, since
parsedate_to_datetimehandles most valid dates, this fallback path should rarely execute.Consider logging when the fallback is used to track if this is actually a problem in practice.
🧹 Nitpick comments (11)
static/js/scanner.js (1)
47-56: Consider if token storage on every scan is necessary.The code checks
auth-statusand storestoken_jsonon every scan attempt (lines 49-51). If the user is already authenticated, this token should already be stored from the initial sign-in flow.This may be redundant and adds an extra API call overhead. Consider:
- Storing the token only during the initial auth flow
- Or checking if a token already exists before making the auth-status call
However, this pattern may be intentional for token refresh scenarios. If so, consider adding a comment explaining why the token is re-stored on each scan.
static/js/labels.js (1)
598-609: Consider consolidating auth initialization logic.Similar to
scanner.js, this module checks auth-status and storestoken_jsonon initialization (lines 601-605). The same token storage pattern appears in multiple files.Consider:
- Centralizing this initialization logic in a shared auth module
- Or checking if the token already exists before making the API call
This would reduce code duplication and unnecessary auth-status calls.
app/core/__init__.py (1)
4-6: Document the purpose of the "default" session fallback.The module-level
stateinitializes a "default" session at import time, which serves as a fallback when requests don't provide a session ID via headers or cookies. The per-request session architecture indeps.pyalready handles this properly—requests are resolved to specific sessions viaget_session_context(), or default to "default" if no session is specified.Consider adding a brief docstring explaining that the "default" session is the fallback for requests without explicit session context, clarifying when this is used versus per-request sessions.
static/js/ui.js (1)
17-45: Multiple handlers may close sidebar redundantly.The overlay listener (lines 19-22),
closeIfOutsideon document (lines 39-40), and mainContent listener (line 44) can all triggercloseSidebarForMobile()for the same user action. WhilecloseSidebarForMobileis idempotent (safe to call multiple times), it's inefficient to register overlapping handlers.Consider either:
- Relying solely on the overlay click handler, or
- Using
closeIfOutsidealone and removing the other handlersapp/services/gmail/archive.py (1)
82-86: Broad exception catch is acceptable for this background task.While static analysis flags
BLE001, catching all exceptions here ensures the status is always updated and the user receives feedback. For a background task, this is a reasonable tradeoff. If you want to be more defensive, you could log the exception type for debugging.🔎 Optional: Add logging for debugging
+import logging + +logger = logging.getLogger(__name__) + except Exception as e: + logger.exception("Archive operation failed") session.archive_status["error"] = f"{e!s}"static/js/auth.js (1)
99-106: Popup blocker could silently fail.If
window.openis blocked by a popup blocker, the user only sees a console warning. They won't know the auth URL couldn't be opened.🔎 Proposed improvement
if (status.pending_auth_url) { try { - window.open(status.pending_auth_url, '_blank', 'noopener'); + const authWindow = window.open(status.pending_auth_url, '_blank', 'noopener'); + if (!authWindow) { + console.warn('Popup blocked. Auth URL:', status.pending_auth_url); + GmailCleaner.UI.showInfoToast('Popup blocked. Check the console for the auth URL.'); + } } catch (error) { console.warn('Failed to open auth URL:', error); + GmailCleaner.UI.showInfoToast('Could not open auth window. Check console for URL.'); } return; }app/core/state.py (1)
202-218: Thread-safe session store, but no cleanup mechanism.The
SessionStorecorrectly uses a lock for thread-safe access. However, sessions are created but never removed, which could lead to memory growth over time in long-running deployments.Consider adding a TTL-based cleanup or explicit session removal on sign-out for production use.
Would you like me to propose a session cleanup mechanism with TTL expiration?
static/js/api.js (1)
38-46: Consider replacing deprecatedunescape()function.While
btoa(unescape(encodeURIComponent(str)))is a known pattern for UTF-8 to base64,unescape()is deprecated. A more modern approach usesTextEncoder:🔎 Modern alternative without deprecated functions
encodeToken(tokenJson) { if (!tokenJson) return null; try { - return btoa(unescape(encodeURIComponent(tokenJson))); + const bytes = new TextEncoder().encode(tokenJson); + const binString = Array.from(bytes, byte => String.fromCodePoint(byte)).join(''); + return btoa(binString); } catch (error) { console.warn('Failed to encode auth token:', error); return null; } }app/services/gmail/important.py (1)
88-91: Consider narrowing the exception type.Catching bare
Exceptionmasks the actual error source. The Gmail API raisesgoogleapiclient.errors.HttpErrorfor API failures. Narrowing would improve debuggability:🔎 More specific exception handling
+from googleapiclient.errors import HttpError + ... - except Exception as e: + except HttpError as e: session.important_status["error"] = f"{e!s}" session.important_status["done"] = True session.important_status["message"] = f"Error: {e!s}"That said, the current catch-all ensures the operation always completes gracefully, which may be the intended behavior for a background task.
app/services/gmail/download.py (1)
38-43: Implicit coupling to delete scan workflow.This reads from
session.delete_scan_results, creating a dependency on the delete scan having run first. The docstring explains this is intentional ("Uses message IDs stored during scan"), but consider making this explicit in the function signature or adding validation:+ if not session.delete_scan_results: + session.download_status["done"] = True + session.download_status["error"] = "No scan results available. Please run a delete scan first." + return + # Get message IDs from scan results (only emails we actually scanned) all_message_ids = []app/services/gmail/delete.py (1)
424-426: Clean result filtering after bulk delete.Uses list comprehension to remove deleted senders from cached results. The
not in senderscheck is O(n) per item; for very large sender lists, consider convertingsendersto a set first:+ sender_set = set(senders) session.delete_scan_results = [ - r for r in session.delete_scan_results if r.get("email") not in senders + r for r in session.delete_scan_results if r.get("email") not in sender_set ]For typical use (selecting tens of senders), this is a micro-optimization.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.gitignoreREADME.mdapp/api/actions.pyapp/api/deps.pyapp/api/status.pyapp/core/__init__.pyapp/core/state.pyapp/services/auth.pyapp/services/auth_handlers.pyapp/services/gmail/archive.pyapp/services/gmail/delete.pyapp/services/gmail/download.pyapp/services/gmail/important.pyapp/services/gmail/labels.pyapp/services/gmail/mark_read.pyapp/services/gmail/scan.pystatic/css/responsive.cssstatic/js/api.jsstatic/js/auth.jsstatic/js/delete.jsstatic/js/labels.jsstatic/js/main.jsstatic/js/markread.jsstatic/js/scanner.jsstatic/js/ui.jstemplates/index.html
🧰 Additional context used
🧬 Code graph analysis (11)
app/core/__init__.py (1)
app/core/state.py (1)
get_session_state(224-226)
static/js/auth.js (3)
static/js/delete.js (8)
response(76-83)response(100-100)response(229-233)response(315-315)response(455-455)status(101-101)status(316-316)status(456-456)static/js/labels.js (13)
response(15-15)response(34-38)response(55-57)response(73-77)response(86-90)response(99-99)response(404-408)response(426-426)response(511-515)response(533-533)status(100-100)status(427-427)status(534-534)static/js/markread.js (2)
response(13-13)status(68-68)
app/services/gmail/mark_read.py (2)
app/core/state.py (2)
SessionState(10-199)reset_mark_read(131-139)app/services/auth.py (1)
get_gmail_service(221-664)
app/services/gmail/important.py (2)
app/core/state.py (2)
SessionState(10-199)reset_important(189-199)app/services/auth.py (1)
get_gmail_service(221-664)
app/api/deps.py (1)
app/core/state.py (3)
SessionState(10-199)get_session_state(224-226)get(209-218)
app/services/gmail/archive.py (2)
app/core/state.py (2)
SessionState(10-199)reset_archive(177-187)app/services/auth.py (1)
get_gmail_service(221-664)
app/services/auth.py (1)
app/core/state.py (11)
SessionState(10-199)get_session_state(224-226)get(209-218)reset_scan(111-119)reset_delete_scan(121-129)reset_mark_read(131-139)reset_delete_bulk(141-151)reset_download(153-163)reset_label_operation(165-175)reset_archive(177-187)reset_important(189-199)
app/services/gmail/labels.py (2)
app/core/state.py (2)
SessionState(10-199)reset_label_operation(165-175)app/services/auth.py (1)
get_gmail_service(221-664)
app/api/actions.py (3)
app/api/deps.py (2)
SessionContext(17-22)get_session_context(39-51)app/core/state.py (1)
get(209-218)app/services/gmail/labels.py (2)
create_label(50-85)delete_label(88-106)
app/services/gmail/delete.py (2)
app/core/state.py (4)
SessionState(10-199)reset_delete_scan(121-129)get(209-218)reset_delete_bulk(141-151)app/services/auth.py (1)
get_gmail_service(221-664)
app/api/status.py (9)
app/api/deps.py (2)
SessionContext(17-22)get_session_context(39-51)app/core/state.py (1)
get(209-218)app/services/gmail/scan.py (2)
get_scan_status(233-235)get_scan_results(238-240)app/services/gmail/mark_read.py (2)
get_unread_count(14-32)get_mark_read_status(138-140)app/services/gmail/delete.py (2)
get_delete_scan_status(174-176)get_delete_scan_results(179-181)app/services/gmail/download.py (2)
get_download_status(185-194)get_download_csv(197-199)app/services/gmail/labels.py (2)
get_labels(11-47)get_label_operation_status(311-313)app/services/gmail/archive.py (1)
get_archive_status(88-90)app/services/gmail/important.py (1)
get_important_status(94-96)
🪛 markdownlint-cli2 (0.18.1)
README.md
150-150: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
154-154: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
🪛 Ruff (0.14.10)
app/services/gmail/mark_read.py
133-133: Do not catch blind exception: Exception
(BLE001)
app/services/gmail/important.py
88-88: Do not catch blind exception: Exception
(BLE001)
app/services/gmail/archive.py
82-82: Do not catch blind exception: Exception
(BLE001)
app/services/auth.py
66-66: Do not catch blind exception: Exception
(BLE001)
146-146: Consider moving this statement to an else block
(TRY300)
344-344: Possible binding to all interfaces
(S104)
381-384: Abstract raise to an inner function
(TRY301)
381-384: Avoid specifying long messages outside the exception class
(TRY003)
660-660: Do not catch blind exception: Exception
(BLE001)
app/services/gmail/labels.py
175-175: Do not catch blind exception: Exception
(BLE001)
178-178: Use explicit conversion flag
Replace with conversion flag
(RUF010)
254-254: Do not catch blind exception: Exception
(BLE001)
255-255: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/services/gmail/scan.py
228-228: Do not catch blind exception: Exception
(BLE001)
app/api/actions.py
52-52: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
80-80: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
91-91: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
120-120: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
136-136: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
151-151: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
173-173: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
186-186: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
200-200: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
215-215: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
237-237: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
263-263: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
289-289: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
305-305: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
app/services/gmail/delete.py
169-169: Do not catch blind exception: Exception
(BLE001)
420-420: Do not catch blind exception: Exception
(BLE001)
421-421: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/api/status.py
37-37: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
50-50: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
63-63: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
71-71: Consider moving this statement to an else block
(TRY300)
81-81: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
94-94: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
107-107: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
120-120: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
133-133: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
146-146: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
159-159: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
187-187: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
203-203: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
217-217: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
231-231: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
244-244: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
app/services/gmail/download.py
148-148: Use explicit conversion flag
Replace with conversion flag
(RUF010)
180-180: Do not catch blind exception: Exception
(BLE001)
182-182: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (65)
.gitignore (1)
3-3: Good addition to cover both virtual environment conventions.Adding
.venv/alongsidevenv/is a smart move—it accounts for developers using different tooling (poetry,pipenv, IDE defaults, etc.) that prefer the dot-prefixed directory, while keeping related ignores together.README.md (3)
148-153: Verify docker log command reliability across scenarios.The two docker log commands use different filter strategies:
- Line 148 filters by container name (
name=gmail-cleaner)- Line 152 filters by image ancestor (
ancestor=ghcr.io/...)If users customize their container name in
docker-compose.yml, the first command may fail. Consider documenting this or providing a more robust alternative (e.g., using the service name withdocker compose logs).Additionally, fix the markdown indentation issues flagged by markdownlint (lines 150, 154).
🔎 Recommended fix for indentation
- If the popup is blocked, check container logs for the OAuth URL: ```bash - docker logs $(docker ps -q --filter name=gmail-cleaner) +docker logs $(docker ps -q --filter name=gmail-cleaner)
- If you are using the published image:
+- If you are using the published image:- docker logs $(docker ps -q --filter ancestor=ghcr.io/gururagavendra/gmail-cleaner) +docker logs $(docker ps -q --filter ancestor=ghcr.io/gururagavendra/gmail-cleaner)
- Open the URL in your browser to continue
+- Open the URL in your browser to continue</details> --- `165-196`: **LGTM! Clear documentation of the new authentication model.** The restructured authentication section clearly distinguishes between: - Client-side session storage (default, per-browser) - Optional file persistence for non-web scenarios This aligns well with the PR's architectural shift to per-session state management and multi-user support. --- `272-304`: **LGTM! Comprehensive OAuth host configuration guidance.** The documentation clearly explains: - Default behavior (derives from request headers) - When to override (proxy issues) - Common mistakes to avoid This will help users troubleshoot OAuth redirect issues effectively. </blockquote></details> <details> <summary>static/css/responsive.css (1)</summary><blockquote> `26-175`: **LGTM! Mobile responsive improvements are well-structured.** The mobile styling updates properly handle: - Sidebar overlay with backdrop blur - Overflow prevention on mobile viewports - Touch-friendly sizing and spacing - Horizontal scroll handling for result items These changes align with the PR's mobile experience improvements. </blockquote></details> <details> <summary>static/js/scanner.js (1)</summary><blockquote> `47-84`: **LGTM! Consistent migration to GmailCleaner.apiFetch.** All API calls now properly use the centralized `GmailCleaner.apiFetch` wrapper, which handles session ID and token headers automatically. </blockquote></details> <details> <summary>static/js/markread.js (1)</summary><blockquote> `13-67`: **LGTM! Clean migration to centralized API wrapper.** All fetch calls properly replaced with `GmailCleaner.apiFetch`. The changes are consistent with the broader refactoring pattern. </blockquote></details> <details> <summary>templates/index.html (2)</summary><blockquote> `84-84`: **LGTM! Sidebar overlay properly integrated.** The overlay div with `onclick="toggleSidebar()"` handler correctly supports the mobile sidebar functionality described in the PR. --- `464-464`: **No duplicate api.js loading detected.** The search confirms only one instance of `api.js` is loaded at line 464. There is no duplicate script loading issue. > Likely an incorrect or invalid review comment. </blockquote></details> <details> <summary>static/js/main.js (1)</summary><blockquote> `7-14`: **LGTM! Safe initialization pattern prevents overwriting existing properties.** The new pattern (`window.GmailCleaner = window.GmailCleaner || {}; Object.assign(...)`) properly preserves any existing properties (like `apiFetch` and `Session` from `api.js`) while initializing the required state properties. This is good defensive programming and prevents module load order issues. </blockquote></details> <details> <summary>static/js/labels.js (1)</summary><blockquote> `15-99`: **LGTM! Comprehensive migration to centralized API wrapper.** All label management API calls now properly use `GmailCleaner.apiFetch`, ensuring consistent session/token handling across: - Label CRUD operations - Apply/remove label operations - Label operation status polling </blockquote></details> <details> <summary>static/js/ui.js (1)</summary><blockquote> `117-132`: **LGTM!** Good mobile detection using both viewport width and pointer type. The dual check (`max-width: 768px` OR `pointer: coarse`) ensures touch devices are handled regardless of screen size. </blockquote></details> <details> <summary>app/services/auth_handlers.py (2)</summary><blockquote> `30-44`: **LGTM!** Clean integration of per-session state. The `session` parameter is properly stored and used throughout the handler for OAuth state management. --- `76-83`: **Nested locking pattern is consistent.** The lock acquisition order (`callback_lock` → `oauth_state_lock`) is maintained consistently across all code paths, which prevents deadlocks. This is a safe pattern. </blockquote></details> <details> <summary>app/services/gmail/archive.py (2)</summary><blockquote> `13-31`: **LGTM!** Clean per-session state integration. Validation, reset, and error handling all properly target `session.archive_status`. --- `88-90`: **LGTM!** Returning `.copy()` is a good defensive pattern that prevents external code from mutating internal session state. </blockquote></details> <details> <summary>static/js/delete.js (1)</summary><blockquote> `44-56`: **LGTM!** Good pattern: checking auth status and storing the token via `GmailCleaner.Session.setToken` before proceeding with the scan. </blockquote></details> <details> <summary>static/js/auth.js (2)</summary><blockquote> `85-86`: **Good parallel polling approach.** Running `pollAuthUrl` and `pollStatus` concurrently is efficient. `pollAuthUrl` stops once it opens the URL, while `pollStatus` continues until authentication completes. --- `160-174`: **LGTM!** Clean sign-out flow: clears server-side session, clears client token, resets UI state, and re-checks auth status. </blockquote></details> <details> <summary>app/services/gmail/scan.py (2)</summary><blockquote> `25-43`: **LGTM!** Clean per-session state integration with proper input validation and early returns for error cases. --- `98-166`: **Callback design is sound.** The `process_message` callback correctly uses closure variables (`processed`, `unsubscribe_data`) rather than accessing session state. Session updates happen synchronously after `batch.execute()` returns, which is the correct pattern. </blockquote></details> <details> <summary>app/core/state.py (2)</summary><blockquote> `10-76`: **LGTM!** Well-structured per-session state container with isolated OAuth locks and consistent status dict patterns. Each session gets its own `oauth_state_lock`, ensuring proper multi-user isolation. --- `224-226`: **LGTM!** Clean public API for session state retrieval. </blockquote></details> <details> <summary>app/services/gmail/mark_read.py (3)</summary><blockquote> `14-32`: **LGTM!** Clean per-session integration for `get_unread_count`. Error handling returns structured response with optional error field. --- `35-136`: **LGTM!** Solid pagination and batching implementation with proper per-session state updates. The `count=0` for "mark all" is a nice API design. --- `133-136`: **Broad exception catch is acceptable here.** Same reasoning as other background tasks: ensures status is always updated for user feedback. </blockquote></details> <details> <summary>app/api/deps.py (3)</summary><blockquote> `16-22`: **LGTM! Clean dataclass definition.** Using `slots=True` reduces memory overhead and attribute access time - good choice for a frequently instantiated request context object. --- `25-36`: **Solid defensive decoding with proper error handling.** Catches both base64 decode errors (`ValueError`, `binascii.Error`) and UTF-8 decode errors separately. Returns `None` gracefully on failure, which is the right approach for header-supplied tokens that may be malformed. --- `45-50`: **Potential race condition when mutating shared SessionState.** Multiple concurrent requests with the same `session_id` but different tokens could race on `session.allow_token_file` and `session.token_json`. The last request wins, potentially causing token mismatches for in-flight operations. Consider: 1. Making these assignments atomic or guarded by a lock, or 2. Not mutating session state here—instead, let downstream code read from `ctx.token_json` directly. For now, this may be acceptable if single-user-per-session is the expected pattern, but worth noting for future scaling. </blockquote></details> <details> <summary>static/js/api.js (2)</summary><blockquote> `11-22`: **Good session ID generation with graceful fallback.** The `crypto.randomUUID()` check with fallback to timestamp + random ensures compatibility across browsers while still producing reasonably unique IDs. --- `49-66`: **Clean API wrapper implementation.** The `apiFetch` function properly: - Ensures session ID exists before each request - Conditionally adds auth token header only when present - Merges caller-provided options without clobbering headers This centralizes session management nicely for all API calls. </blockquote></details> <details> <summary>app/services/gmail/important.py (2)</summary><blockquote> `13-15`: **Clean migration to per-session state.** The function signature now accepts `SessionState` directly, enabling proper isolation between concurrent user sessions. The keyword-only `important` parameter keeps the API explicit. --- `94-96`: **Good defensive copy on status return.** Returning `.copy()` prevents callers from accidentally mutating the session's internal state dictionary. </blockquote></details> <details> <summary>app/services/gmail/download.py (3)</summary><blockquote> `16-17`: **Session-based download operation - clean signature.** --- `173-178`: **Consider memory implications for large exports.** Storing the entire CSV string in `session.download_status["csv_data"]` could consume significant memory for large mailboxes (50,000 char limit per body × many emails). For very large exports, consider streaming to a temp file or implementing pagination. For typical use cases, this is likely acceptable. --- `185-194`: **Good API design - status without payload.** Excluding `csv_data` from the status response keeps polling lightweight. The separate `get_download_csv` endpoint retrieves the payload only when needed. </blockquote></details> <details> <summary>app/services/gmail/labels.py (4)</summary><blockquote> `11-15`: **Consistent session-based API across label operations.** All public functions now accept `SessionState`, maintaining consistency with the broader per-session architecture. --- `109-134`: **Well-designed parameterized helper.** The `_apply_label_operation_background` function consolidates add/remove label logic with message templates. This avoids code duplication while keeping the public API clean. The keyword-only parameters (`*`) enforce explicit argument naming, improving readability at call sites. --- `165-180`: **Good optimization: fetch label name once.** Fetching the label name upfront for remove operations avoids repeated API calls during iteration. The early-exit error handling is appropriate. --- `311-313`: **Consistent status accessor pattern.** Returns a defensive copy, matching the pattern used across other status functions. </blockquote></details> <details> <summary>app/api/status.py (3)</summary><blockquote> `36-46`: **Standard FastAPI dependency injection.** Using `Depends(get_session_context)` in the function signature is the canonical FastAPI pattern. The static analysis warning (B008) is a false positive in this context—FastAPI specifically expects this pattern for dependency injection. --- `62-77`: **One-time token handoff pattern.** The `pending_token_json` is read and immediately cleared, ensuring the token is only returned once to the client. This prevents token replay on subsequent status checks. Good security practice. --- `158-183`: **Well-structured CSV download endpoint.** - Checks for data availability before proceeding - Uses UTC timestamp for consistent filenames across timezones - Re-raises `HTTPException` to avoid masking 404s - Proper Content-Disposition header for download </blockquote></details> <details> <summary>app/services/auth.py (5)</summary><blockquote> `56-68`: **Clean credential loading helper.** Proper two-stage validation: first parse JSON, then construct Credentials. Logs warnings without exposing sensitive data. Returns `None` on any failure, letting callers handle the fallback. --- `124-156`: **Per-session credential refresh with dual storage.** Refresh updates both `session.token_json` (for subsequent API calls) and `session.pending_token_json` (for client pickup). Token file write is conditional on `allow_token_file`, supporting the hybrid storage model. On refresh failure, both session token and file are cleared—clean slate for re-auth. --- `373-376`: **Always uses manual OAuth flow.** Setting `manual_flow = True` and `open_browser = False` ensures consistent behavior across Docker, headless, and local environments. The client is responsible for opening the auth URL, which is stored in `session.pending_auth_url`. This is a good architectural decision for multi-user support. --- `667-689`: **Thorough session cleanup on sign-out.** Clears all credentials, user info, and resets every operation status. The token file is only removed if `allow_token_file` is true, preserving the conditional storage model. --- `692-717`: **Token precedence in login check.** The order is: `token_json` param → `pending_token_json` → `session.token_json`. This allows: 1. Client-provided token (highest priority) 2. Freshly obtained token (from OAuth callback) 3. Previously stored session token The refresh path (lines 708-717) updates session state on successful refresh. </blockquote></details> <details> <summary>app/services/gmail/delete.py (4)</summary><blockquote> `20-22`: **Session-aware scan function.** Accepts `SessionState` directly, enabling per-user isolation. Optional `filters` parameter keeps the API flexible. --- `194-207`: **Good input validation with regex patterns.** Validates sender format against both email and domain patterns before proceeding. This prevents injection of malformed queries into the Gmail API. --- `328-330`: **Efficient two-phase bulk delete.** Phase 1 collects all message IDs (0-40% progress), Phase 2 batch-deletes in 1000-message chunks (40-100% progress). This minimizes API calls compared to per-sender deletion. The progress reporting is user-friendly with percentage-based updates. Also applies to: 398-419 --- `444-446`: **Consistent status accessor pattern.** Returns a defensive copy, matching the pattern used throughout the codebase. </blockquote></details> <details> <summary>app/api/actions.py (13)</summary><blockquote> `9-11`: **LGTM!** Import additions support the new per-request session context pattern and OAuth host extraction. --- `48-59`: **LGTM!** Session context integration is correct. The endpoint properly injects the per-request context and passes it to the background scan task. --- `76-87`: **OAuth flow integration looks correct.** The endpoint properly extracts host/scheme from the request and stores them in the session for OAuth callback URL construction. However, this depends on the `_get_request_host_and_scheme` helper, which has a critical HTTPS bug (see previous comment). --- `90-100`: **LGTM!** Session context is properly integrated and error handling is maintained. --- `116-129`: **LGTM!** Session context correctly passed to the mark-as-read background task. --- `132-145`: **LGTM!** Session context correctly passed to the delete-scan background task. --- `148-166`: **LGTM!** Session context integration is correct, and the added validation for empty sender prevents unnecessary service calls. --- `169-179`: **LGTM!** Session context correctly passed to the bulk delete background task. --- `182-191`: **LGTM!** Session context correctly passed to the download background task. --- `197-210`: **LGTM!** Session context correctly passed to the label creation service. --- `213-230`: **LGTM!** Session context integration is correct, and the validation for empty label ID prevents unnecessary service calls. --- `233-282`: **LGTM!** Both label endpoints properly integrate session context and include thorough input validation. Background task invocations are correct. --- `285-321`: **LGTM!** Both endpoints properly integrate session context with appropriate validation. The use of `partial` in `api_mark_important` correctly binds the session and important flag for the background task. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
hey @daddyparodz thank you so much for this contribution to the project, it means an lot, |
|
@daddyparodz please fix the tests, when u have time |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/unit/api/test_api_actions.py (1)
199-202: LGTM!Good edge case coverage for the empty list scenario. The test correctly validates that even with an empty senders list, the session-aware API contract is maintained.
Optional: Consider adding session isolation tests.
Since this PR introduces multi-user support with per-session state, consider adding integration tests (if not already present elsewhere) that verify concurrent requests with different sessions don't interfere with each other—particularly for background operations like bulk delete that track progress in session state.
tests/unit/services/auth/test_oauth_flow_complete.py (1)
91-105: Extract duplicate helper function.The
exists_side_effecthelper is duplicated across multiple tests with identical or near-identical implementations. Consider extracting it as a module-level fixture or shared helper to reduce duplication and improve maintainability.🔎 Example refactor
At module level:
def _mock_credentials_exists_side_effect(path): """ Simulates os.path.exists behavior for tests by indicating presence only for credentials files. Parameters: path (str | os.PathLike): Path or filename to check. Returns: True if `path` contains "credentials.json", False otherwise (including when it contains "token.json"). """ if "token.json" in str(path): return False if "credentials.json" in str(path): return True return FalseThen in tests:
-def exists_side_effect(path): - """...""" - if "token.json" in str(path): - return False - if "credentials.json" in str(path): - return True - return False - -mock_exists.side_effect = exists_side_effect +mock_exists.side_effect = _mock_credentials_exists_side_effectAlso applies to: 147-152, 199-213, 259-273
tests/unit/services/auth/test_token_management_complete.py (2)
62-62: Consider clarifying the test mode distinction.The tests now show a mixed pattern: some pass
session=SessionState()while others (lines 93, 175, 404, 418) don't pass a session parameter. This appears intentional to test both the new per-session mode and the legacy file-based default mode, which is good coverage given the PR's backward compatibility goals.However, the distinction could be clearer. Consider:
- Add comments to test methods indicating which mode they're testing (e.g., "Tests session-based auth" vs "Tests file-based fallback").
- Consider a fixture for consistent SessionState creation:
@pytest.fixture def fresh_session(): """Provides a fresh SessionState for per-session auth tests.""" return SessionState()Then use it as:
auth.get_gmail_service(session=fresh_session)This would make the intent more explicit and easier to maintain.
Also applies to: 145-145, 224-224, 250-250, 275-275, 296-296, 316-316, 368-368
62-62: Optional: Consider testing session state reuse.Each call creates a fresh
SessionState()with no initial state, which means these tests exercise the file-based fallback path even when passing a session parameter. This is fine for the current test scenarios (error handling, validation), but you may want to add tests that:
- Pre-populate
session.token_jsonwith credential data- Reuse the same session instance across multiple calls
- Verify that tokens are stored in and retrieved from
session.token_jsonThis would provide fuller coverage of the pure session-based flow (without file fallback).
Also applies to: 145-145, 224-224, 368-368
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/unit/api/test_api_actions.pytests/unit/services/auth/test_oauth_flow_complete.pytests/unit/services/auth/test_token_management_complete.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/services/auth/test_token_management_complete.py (2)
app/core/state.py (1)
SessionState(10-199)app/services/auth.py (2)
get_gmail_service(221-664)needs_auth_setup(71-108)
tests/unit/api/test_api_actions.py (1)
app/core/state.py (1)
SessionState(10-199)
🪛 Ruff (0.14.10)
tests/unit/services/auth/test_oauth_flow_complete.py
85-85: Possible hardcoded password assigned to: "token_file"
(S105)
175-175: Possible binding to all interfaces
(S104)
🔇 Additional comments (7)
tests/unit/api/test_api_actions.py (3)
9-10: LGTM!The import is necessary for validating that API functions receive
SessionStateas their first parameter, aligning with the per-session state architecture.
165-168: LGTM!The test correctly validates that
delete_emails_by_sendernow receivesSessionStateas its first parameter, followed by the sender identifier. This runtime shape validation ensures the session-aware API contract is maintained.
181-184: LGTM!The test validates that
delete_emails_bulk_backgroundreceivesSessionStateas its first parameter and the senders list as the second. The validation pattern is consistent with the other session-aware API tests.tests/unit/services/auth/test_oauth_flow_complete.py (3)
13-54: LGTM: Clean test helper for synchronous thread execution.The
ImmediateThreadclass correctly mimics threading.Thread's interface while executing targets synchronously for deterministic testing. The dual initialization pattern (positional/keyword) is well-handled.
295-442: Good error handling test coverage.The error scenario tests appropriately verify OAuth flow behavior for invalid authorization codes, timeouts, and error state management. The tests correctly assert that service is None and error messages are returned.
120-120: The session parameter is optional with a sensible default, so the inconsistent usage is intentional.The
get_gmail_service()function hassession: SessionState | None = Noneas its signature, and on line 232 it applies a fallback:session = session or get_session_state("default"). The tests without an explicit session parameter (lines 120, 341, 389, 437) verify the default behavior, while those withsession=SessionState()(lines 168, 229, 287) test with specific session state. This is correct and requires no changes.tests/unit/services/auth/test_token_management_complete.py (1)
11-11: LGTM: Import aligns with per-session architecture.The SessionState import is necessary for the new per-session authentication model and is correctly placed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/services/auth/test_oauth_flow_complete.py (1)
13-54: Addjoin()method to ImmediateThread for full compatibility.The
ImmediateThreadclass correctly simulates immediate execution for testing, but it's missing thejoin()method thatthreading.Threadprovides. If the code under test callsthread.join(), the test will fail with anAttributeError.🔎 Proposed addition of join() method
def start(self): """ Execute the stored target callable immediately in the current thread. If no target was provided, this method does nothing. """ if self._target: self._target(*self._args, **self._kwargs) + + def join(self, timeout=None): + """ + No-op join since execution is immediate and synchronous. + + Parameters: + timeout: Ignored, kept for signature compatibility. + """ + pass
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/services/auth/test_oauth_flow_complete.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/services/auth/test_oauth_flow_complete.py (1)
app/core/state.py (1)
SessionState(10-199)
🪛 Ruff (0.14.10)
tests/unit/services/auth/test_oauth_flow_complete.py
85-85: Possible hardcoded password assigned to: "token_file"
(S105)
175-175: Possible binding to all interfaces
(S104)
🔇 Additional comments (3)
tests/unit/services/auth/test_oauth_flow_complete.py (3)
137-175: Static analysis hint is a false positive.The Ruff warning about "Possible binding to all interfaces" on line 175 is a false positive. This line is a test assertion verifying that the code correctly selects
0.0.0.0as the bind address in web auth mode, not an actual security concern.
196-196: Past duplicate line issue is resolved.The previous review flagged a duplicate
oauth_external_port = Noneassignment on line 197, which has been fixed. The setting is now assigned only once.
85-85: Static analysis hint is a false positive.The Ruff warning about "Possible hardcoded password" for
"token.json"is a false positive. This is a filename configuration, not an actual password or credential.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/services/auth/test_oauth_flow_complete.py (2)
13-57: Consider simplifying the test helper.The
ImmediateThreadclass is well-implemented for synchronous test execution. Two minor observations:
- The
timeoutparameter injoin()is unused (flagged by static analysis). While it's there for API compatibility withthreading.Thread, you could acknowledge this with a brief comment or suppress the warning if needed.- The docstrings are quite verbose for a test utility. Consider condensing them to focus on the key behavior: "Simulates threading.Thread by executing targets synchronously for deterministic testing."
</comment_end>
94-108: Consider extracting the repeatedexists_side_effecthelper.The
exists_side_effectfunction is duplicated across multiple tests with identical or nearly identical logic. Some versions include verbose docstrings while others don't, creating inconsistency.Consider extracting this to a shared test fixture or module-level helper function to reduce duplication and maintain a single source of truth.
</comment_end>
Also applies to: 150-155, 201-215, 261-275, 327-332, 376-381, 423-428
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/services/auth/test_oauth_flow_complete.py
🧰 Additional context used
🪛 Ruff (0.14.10)
tests/unit/services/auth/test_oauth_flow_complete.py
55-55: Unused method argument: timeout
(ARG002)
88-88: Possible hardcoded password assigned to: "token_file"
(S105)
178-178: Possible binding to all interfaces
(S104)
🔇 Additional comments (1)
tests/unit/services/auth/test_oauth_flow_complete.py (1)
123-123: No action needed – session parameter usage is correct and consistent.The
sessionparameter is optional (defaults toNonethen usesget_session_state("default")), and tests correctly exercise both paths:
- Tests verifying server binding behavior (lines 171, 231, 289) pass
SessionState()to ensure clean, isolated session state for those specific assertions- Tests verifying error handling (lines 123, 343, 391, 439) don't need explicit session state since they test error paths independent of session specifics
This pattern aligns with the per-session architecture and requires no changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/services/auth/test_oauth_flow_complete.py (1)
72-74: Simplify the join() method.The
join()method can be streamlined:def join(self, timeout=None): - _ = timeout - return None + passThe no-op assignment and explicit
return Noneare unnecessary since the method body can simply bepass.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/services/auth/test_oauth_flow_complete.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/services/auth/test_oauth_flow_complete.py (3)
app/core/state.py (2)
SessionState(10-199)get(209-218)app/services/auth.py (1)
get_gmail_service(221-664)tests/conftest.py (1)
mock_exists(50-54)
🪛 Ruff (0.14.10)
tests/unit/services/auth/test_oauth_flow_complete.py
106-106: Possible hardcoded password assigned to: "token_file"
(S105)
173-173: Possible binding to all interfaces
(S104)
🔇 Additional comments (5)
tests/unit/services/auth/test_oauth_flow_complete.py (5)
9-9: LGTM! SessionState import aligns with per-session architecture.The import of
SessionStateis consistent with the PR's goal of enabling multi-user support through per-session state management. Tests correctly use this to simulate individual user sessions during OAuth flows.
13-27: LGTM! Test helper correctly simulates file presence.The
exists_side_effectfunction appropriately simulates the presence of credentials files and absence of token files for OAuth flow testing. The logic is clear and the docstring is helpful.
156-173: LGTM! Test correctly verifies web auth mode binding.The test properly uses
ImmediateThreadto execute OAuth flow synchronously andSessionState()to simulate per-session behavior. The assertion verifies that web auth mode binds to0.0.0.0as expected. The static analysis hint about binding to all interfaces is a false positive—this is the intended behavior being tested.
194-217: LGTM! Desktop mode binding test is correct.The test appropriately verifies that desktop mode binds to
localhostinstead of0.0.0.0. The use ofImmediateThreadandSessionState()is consistent with the updated architecture.
238-257: LGTM! Custom OAuth host test is correct.The test properly verifies that when a custom
oauth_hostis configured, the redirect URI reflects that host. The assertion on line 257 correctly checks the constructed redirect URI.
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v6 | ||
| if: ${{ env.ACT != 'true' }} |
There was a problem hiding this comment.
https://github.com/nektos/act
very cool tool that allows to run GH actions locally inside docker containers
I used it to skip certain steps in the workflow when running locally with act
|
thank you for the pr and sorry for the delay started reviewing it |
2 participants
Description
Checklist
Related Issues
N/A, I just wanted to help because I really like this project and I want to be part of it :)