Conversation
Signed-off-by: ruo <ruoliu.dev@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the application's capabilities by introducing robust multi-account support. It enables users to seamlessly integrate and manage multiple Google accounts for their mail, calendar, and tasks. The changes span both the backend, with new data aggregation and attribution mechanisms, and the frontend, providing a dedicated settings interface and updated views to reflect the multi-account context. This enhancement aims to provide a more flexible and powerful experience for users with multiple digital identities. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
Adds a multi-account “mailbox settings” experience and introduces account-scoped (“all accounts” vs per-linked-account) aggregation across Mail, Calendar, and Tasks, including account attribution metadata in API responses.
Changes:
- Adds a dedicated Settings route/UI for managing linked Google accounts and default account selection.
- Introduces mailbox “scope” support (all accounts vs per-account) for Gmail threads/counts and Calendar events, including attribution metadata.
- Updates UI workspaces (Mail/Calendar/Tasks) to surface account context and filtering/creation behaviors.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui/src/app-rail.tsx | Adds a rail footer profile/settings entry backed by session data. |
| packages/types/src/index.ts | Adds mailbox scope and account attribution fields to shared types. |
| packages/lib/src/api.ts | Extends API client to pass scope and linked_account_id for calendar deletes. |
| packages/features/src/tasks/tasks-view.tsx | Adds account-aware filtering and account selection for task creation. |
| packages/features/src/settings/settings-view.tsx | New Settings UI for linked-account management and default selection. |
| packages/features/src/mail/mail-workspace.tsx | Adds per-account mailbox groups + scope-aware Gmail fetching/counts and attribution display. |
| packages/features/src/calendar/calendar-workspace.tsx | Adds scope selection, account attribution display, and account-targeted event writes/deletes. |
| packages/app/src/routes/settings-page.tsx | New server route for Settings with auth guard. |
| packages/app/src/routes/auth-page.tsx | Adds redirect-away behavior when already authenticated. |
| apps/web/app/settings/page.tsx | Wires Next.js Settings page to the shared route component. |
| apps/web/app/globals.css | Styles for rail profile, settings layout, and scoped mailbox/calendar UI. |
| apps/api/tests/test_google_integration.py | Updates integration expectations for scoped thread IDs and attribution fields. |
| apps/api/app/storage/auth_store.py | Adds store methods for provider credentials + listing sessions for a user. |
| apps/api/app/services/auth_service.py | Adds active-account access helpers and updates disconnect behavior. |
| apps/api/app/schemas/thread.py | Adds account attribution fields to thread models. |
| apps/api/app/schemas/task.py | Adds account attribution fields to task models. |
| apps/api/app/schemas/calendar.py | Adds account attribution and write-target fields to calendar models. |
| apps/api/app/routers/tasks.py | Enriches tasks responses with linked account metadata. |
| apps/api/app/routers/gmail.py | Implements scoped thread IDs, scope-based aggregation, and aggregate pagination token logic. |
| apps/api/app/routers/calendar.py | Implements scope-based aggregation and account-targeted writes/deletes with attribution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| <select | ||
| value={newLinkedAccountId} | ||
| onChange={(event) => setNewLinkedAccountId(event.target.value)} | ||
| aria-label="Task account" | ||
| > | ||
| {linkedAccounts.map((account) => ( | ||
| <option key={account.id} value={account.id}> | ||
| {account.provider_account_ref ?? account.display_name} | ||
| </option> | ||
| ))} | ||
| </select> |
There was a problem hiding this comment.
The Create Task account <select> is controlled with value={newLinkedAccountId} but initially that state is "" and there is no <option value="">…</option> (and linkedAccounts can be empty during initial load). This can trigger React warnings and leaves the dropdown blank/unusable until state is set. Add a placeholder option and/or initialize newLinkedAccountId to a valid account id once linkedAccounts loads, and handle the zero-linked-accounts case (disable the form / show guidance).
| <select | ||
| value={eventForm.linkedAccountId} | ||
| onChange={(event) => | ||
| setEventForm((current) => ({ | ||
| ...current, | ||
| linkedAccountId: event.target.value, | ||
| })) | ||
| } | ||
| aria-label="Calendar account" | ||
| > | ||
| {linkedAccounts.map((account) => ( | ||
| <option key={account.id} value={account.id}> | ||
| {account.provider_account_ref ?? account.display_name} | ||
| </option> | ||
| ))} | ||
| </select> |
There was a problem hiding this comment.
The calendar create-event form uses a controlled <select value={eventForm.linkedAccountId}> but linkedAccountId is initialized to an empty string and there is no option for "" (and linkedAccounts can still be empty while session is loading). This can produce React warnings and a blank selector if the modal opens early. Add a placeholder option / disable the form until accounts are loaded, or ensure linkedAccountId is always set to a valid account id before rendering the select.
| def deserialize_aggregate_page_token( | ||
| value: str, | ||
| ) -> tuple[list[dict[str, str | None]], list[ThreadSummary], int | None]: | ||
| try: | ||
| padding = "=" * (-len(value) % 4) | ||
| payload = base64.urlsafe_b64decode(f"{value}{padding}".encode()) | ||
| parsed = json.loads(payload.decode("utf-8")) | ||
| except (ValueError, json.JSONDecodeError, UnicodeDecodeError) as exc: | ||
| raise HTTPException( | ||
| status_code=401, | ||
| detail="An active linked Google account is required.", | ||
| status_code=422, detail="Invalid aggregate page token." | ||
| ) from exc | ||
|
|
||
| if not isinstance(parsed, dict) or parsed.get("v") != AGGREGATE_TOKEN_VERSION: | ||
| raise HTTPException(status_code=422, detail="Invalid aggregate page token.") | ||
|
|
||
| token_rows = parsed.get("account_tokens") or [] | ||
| leftovers = parsed.get("leftovers") or [] | ||
| if not isinstance(token_rows, list) or not isinstance(leftovers, list): | ||
| raise HTTPException(status_code=422, detail="Invalid aggregate page token.") | ||
|
|
||
| return ( | ||
| [ | ||
| { | ||
| "linked_account_id": str(item.get("linked_account_id") or "").strip(), | ||
| "next_page_token": ( | ||
| str(item.get("next_page_token") or "").strip() or None | ||
| ), | ||
| } | ||
| for item in token_rows | ||
| if str(item.get("linked_account_id") or "").strip() | ||
| ], | ||
| [ThreadSummary.model_validate(item) for item in leftovers], | ||
| parsed.get("total_count"), | ||
| ) |
There was a problem hiding this comment.
deserialize_aggregate_page_token parses mailbox, unread_only, and query into the token payload, but the deserializer ignores them and list_threads_for_all_accounts doesn’t validate that the incoming page_token matches the current request params. This allows clients to reuse a token across different mailbox/query/unread settings and get inconsistent/mixed pagination. Consider returning these fields from the deserializer and rejecting tokens that don’t match the current request (422).
| account_tokens, leftovers, total_count = deserialize_aggregate_page_token( | ||
| page_token | ||
| ) | ||
| pool = list(leftovers) |
There was a problem hiding this comment.
The aggregate pagination token includes a leftovers array that is accepted from the client and injected into the returned thread list (pool = list(leftovers)) without being tied back to the currently resolved accounts. Because the token is unsigned, a client can fabricate/alter leftovers and have arbitrary thread metadata echoed back (and potentially persisted by the follow-up persist_threads loop for matching accounts). Consider signing the token (or storing pagination state server-side), and at minimum filter leftovers to entries whose linked_account_id is in accounts and whose id decodes to that same account.
| pool = list(leftovers) | |
| # Only accept leftovers that belong to the currently resolved accounts | |
| filtered_leftovers = [ | |
| item | |
| for item in leftovers | |
| if isinstance(item, dict) | |
| and (item.get("linked_account_id") in account_map) | |
| ] | |
| pool = list(filtered_leftovers) |
| self.store.disconnect_account(user_id, linked_account_id) | ||
| if remaining_accounts: | ||
| replacement = remaining_accounts[0] | ||
| for session in self.store.list_sessions_for_user(user_id): | ||
| if session.active_linked_account_id == linked_account_id: | ||
| self.store.set_active_account( | ||
| session.session_id, | ||
| user_id, | ||
| replacement.id, | ||
| ) |
There was a problem hiding this comment.
disconnect_account calls store.disconnect_account(...) before iterating sessions to swap active_linked_account_id to a replacement. In the Postgres store implementation, disconnect_account already nulls active_linked_account_id for affected sessions, so list_sessions_for_user() after the disconnect won’t show the old account id and the replacement logic won’t run—leaving sessions with no active account even when other active accounts exist. Capture the affected sessions before disconnecting (or change the store behavior), then update them to a replacement (or explicitly clear when none remain) to keep SQLite/Postgres behavior consistent.
| self.store.disconnect_account(user_id, linked_account_id) | |
| if remaining_accounts: | |
| replacement = remaining_accounts[0] | |
| for session in self.store.list_sessions_for_user(user_id): | |
| if session.active_linked_account_id == linked_account_id: | |
| self.store.set_active_account( | |
| session.session_id, | |
| user_id, | |
| replacement.id, | |
| ) | |
| # Capture sessions that currently have this account active before the store | |
| # potentially clears their active_linked_account_id (as in the Postgres backend). | |
| affected_sessions = [ | |
| session | |
| for session in self.store.list_sessions_for_user(user_id) | |
| if session.active_linked_account_id == linked_account_id | |
| ] | |
| self.store.disconnect_account(user_id, linked_account_id) | |
| if remaining_accounts: | |
| replacement = remaining_accounts[0] | |
| for session in affected_sessions: | |
| self.store.set_active_account( | |
| session.session_id, | |
| user_id, | |
| replacement.id, | |
| ) |
| threads_response = client.get("/gmail/threads") | ||
| assert threads_response.status_code == 200 | ||
| assert threads_response.json()["threads"][0]["id"] == "gmail-thread-1" | ||
| scoped_thread_id = threads_response.json()["threads"][0]["id"] | ||
| assert scoped_thread_id != "gmail-thread-1" | ||
| assert threads_response.json()["threads"][0]["account_email"] == "user@gmail.com" | ||
| assert threads_response.json()["next_page_token"] == "next-page" | ||
| assert threads_response.json()["total_count"] == 41 | ||
|
|
||
| thread_response = client.get("/gmail/threads/gmail-thread-1") | ||
| thread_response = client.get(f"/gmail/threads/{scoped_thread_id}") | ||
| assert thread_response.status_code == 200 | ||
| assert thread_response.json()["id"] == "gmail-thread-1" | ||
| assert thread_response.json()["id"] == scoped_thread_id |
There was a problem hiding this comment.
The updated integration test validates that thread IDs are now scoped, but it doesn’t cover the new scope query param behavior (single-account vs all-account aggregation) or aggregate pagination via the new page token format. Adding tests for scope=<linked_account_id> and for paging with the aggregate token would help prevent regressions in the new multi-account logic.
| const linkedAccounts = activeLinkedAccounts(session); | ||
| const selectedScopeAccount = | ||
| selectedScope === "all" | ||
| ? null | ||
| : (linkedAccounts.find((account) => account.id === selectedScope) ?? | ||
| null); | ||
| const activeHeading = mailboxHeading(mailbox); | ||
| const activeScopeHeading = | ||
| selectedScopeAccount?.provider_account_ref ?? | ||
| selectedScopeAccount?.display_name ?? | ||
| "All Accounts"; | ||
|
|
||
| const loadMailboxCounts = useCallback(async () => { | ||
| if (!session?.authenticated) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const counts = await api.getGmailMailboxCounts(); | ||
| setMailboxCounts(counts); | ||
| const scopes = ["all", ...linkedAccounts.map((account) => account.id)]; | ||
| const entries = await Promise.all( | ||
| scopes.map(async (scope) => [ | ||
| scope, | ||
| await api.getGmailMailboxCounts(scope), | ||
| ]), | ||
| ); | ||
| setMailboxCountsByScope(Object.fromEntries(entries)); | ||
| } catch { | ||
| setMailboxCounts(EMPTY_MAILBOX_COUNTS); | ||
| setMailboxCountsByScope({ | ||
| all: EMPTY_MAILBOX_COUNTS, | ||
| }); | ||
| } | ||
| }, [session?.authenticated]); | ||
| }, [linkedAccounts, session?.authenticated]); |
There was a problem hiding this comment.
linkedAccounts is computed via activeLinkedAccounts(session) which returns a new array every render. Because loadMailboxCounts depends on linkedAccounts, the callback (and the effect that depends on it) will be recreated each render, potentially causing repeated mailbox-count fetches/render loops once sessionChecked is true. Memoize linkedAccounts with useMemo (e.g., based on session?.linked_accounts) or change the callback dependencies to stable inputs so the effect only runs when the actual account list changes.
| export type ActionState = "to_reply" | "to_follow_up" | "task" | "fyi"; | ||
| export type TaskStatus = "open" | "completed"; | ||
| export type MailboxKey = "inbox" | "sent" | "archive" | "trash" | "junk"; | ||
| export type MailboxScope = "all" | string; |
There was a problem hiding this comment.
MailboxScope is declared as "all" | string, which TypeScript simplifies to just string, so you lose any type-safety/intent around the special "all" value. Consider using a branded type for linked account IDs (or type MailboxScope = "all" | LinkedAccountId) so callers can’t accidentally pass arbitrary strings without realizing it.
There was a problem hiding this comment.
Code Review
This pull request introduces multi-account support across the application, which is a significant and well-executed enhancement. The changes span the backend API, database storage, and frontend components to allow users to link multiple Google accounts and view aggregated data in mail, calendar, and tasks. The backend work, especially for paginating and aggregating data from multiple accounts in gmail.py, is complex and thoughtfully implemented. I've identified a potential performance issue in the Gmail aggregation logic and a minor maintainability improvement in the tasks router. Overall, this is a solid implementation of a major new feature.
Note: Security Review did not run due to the size of the PR.
| def list_threads_for_all_accounts( | ||
| *, | ||
| accounts: list[LinkedAccountAccess], | ||
| page_token: str | None, | ||
| page_size: int, | ||
| mailbox: MailboxKey, | ||
| unread_only: bool, | ||
| query: str | None, | ||
| client: GoogleWorkspaceClient, | ||
| ) -> ThreadSummaryPage: | ||
| account_map = {account.account.id: account for account in accounts} | ||
| if page_token is None: | ||
| aggregate_threads: list[ThreadSummary] = [] | ||
| account_tokens: list[dict[str, str | None]] = [] | ||
| total_count = 0 | ||
| saw_total = False | ||
| for access in accounts: | ||
| try: | ||
| page = client.list_gmail_threads( | ||
| access.credential.access_token, | ||
| max_results=page_size, | ||
| mailbox=mailbox, | ||
| unread_only=unread_only, | ||
| query=query, | ||
| ) | ||
| except GoogleAPIError as exc: | ||
| raise HTTPException( | ||
| status_code=exc.app_status_code, | ||
| detail=str(exc), | ||
| ) from exc | ||
| except RuntimeError as exc: | ||
| raise HTTPException(status_code=502, detail=str(exc)) from exc | ||
| aggregate_threads.extend( | ||
| [with_account_metadata(thread, access) for thread in page.threads] | ||
| ) | ||
| account_tokens.append( | ||
| { | ||
| "linked_account_id": access.account.id, | ||
| "next_page_token": page.next_page_token, | ||
| } | ||
| ) | ||
| if page.total_count is not None: | ||
| total_count += page.total_count | ||
| saw_total = True | ||
|
|
||
| sorted_threads = sort_threads(aggregate_threads) | ||
| visible_threads = sorted_threads[:page_size] | ||
| leftovers = sorted_threads[page_size:] | ||
| has_more = bool(leftovers) or any( | ||
| item["next_page_token"] for item in account_tokens | ||
| ) | ||
| return ThreadSummaryPage( | ||
| threads=visible_threads, | ||
| next_page_token=( | ||
| serialize_aggregate_page_token( | ||
| mailbox=mailbox, | ||
| unread_only=unread_only, | ||
| query=query, | ||
| total_count=total_count if saw_total else None, | ||
| account_tokens=account_tokens, | ||
| leftovers=leftovers, | ||
| ) | ||
| if has_more | ||
| else None | ||
| ), | ||
| has_more=has_more, | ||
| total_count=total_count if saw_total else None, | ||
| ) | ||
|
|
||
| account_tokens, leftovers, total_count = deserialize_aggregate_page_token( | ||
| page_token | ||
| ) | ||
| pool = list(leftovers) | ||
| while len(pool) < page_size and any( | ||
| item["next_page_token"] for item in account_tokens | ||
| ): | ||
| next_batch: list[ThreadSummary] = [] | ||
| for item in account_tokens: | ||
| next_cursor = item["next_page_token"] | ||
| if not next_cursor: | ||
| continue | ||
| access = account_map.get(item["linked_account_id"] or "") | ||
| if access is None: | ||
| continue | ||
| try: | ||
| page = client.list_gmail_threads( | ||
| access.credential.access_token, | ||
| max_results=page_size, | ||
| page_token=next_cursor, | ||
| mailbox=mailbox, | ||
| unread_only=unread_only, | ||
| query=query, | ||
| ) | ||
| except GoogleAPIError as exc: | ||
| raise HTTPException( | ||
| status_code=exc.app_status_code, | ||
| detail=str(exc), | ||
| ) from exc | ||
| except RuntimeError as exc: | ||
| raise HTTPException(status_code=502, detail=str(exc)) from exc | ||
| next_batch.extend( | ||
| [with_account_metadata(thread, access) for thread in page.threads] | ||
| ) | ||
| item["next_page_token"] = page.next_page_token | ||
| if not next_batch: | ||
| break | ||
| pool = sort_threads(pool + next_batch) | ||
|
|
||
| visible_threads = pool[:page_size] | ||
| next_leftovers = pool[page_size:] | ||
| has_more = bool(next_leftovers) or any( | ||
| item["next_page_token"] for item in account_tokens | ||
| ) | ||
| return ThreadSummaryPage( | ||
| threads=visible_threads, | ||
| next_page_token=( | ||
| serialize_aggregate_page_token( | ||
| mailbox=mailbox, | ||
| unread_only=unread_only, | ||
| query=query, | ||
| total_count=total_count, | ||
| account_tokens=account_tokens, | ||
| leftovers=next_leftovers, | ||
| ) | ||
| if has_more | ||
| else None | ||
| ), | ||
| has_more=has_more, | ||
| total_count=total_count, | ||
| ) |
There was a problem hiding this comment.
The pagination logic in this function can be inefficient for users with many linked accounts. In the while loop for fetching subsequent pages, it fetches a full page_size of threads from every account that has more items. For example, with 10 accounts and a page size of 20, this could result in fetching up to 200 threads to construct a single page of 20. This may lead to slow responses and could approach API rate limits.
A more scalable approach would be to fetch just enough items to determine the next page, perhaps using a k-way merge algorithm with a heap to maintain the sorted order across accounts without fetching and sorting large batches.
|
|
||
| def with_account_metadata( | ||
| task: TaskItem, | ||
| accounts_by_id: dict[str, object], |
There was a problem hiding this comment.
The type hint for accounts_by_id is dict[str, object], which is too generic and necessitates using getattr. For better type safety and readability, consider using a more specific type like dict[str, "LinkedAccountRecord"]. This would require importing LinkedAccountRecord and using a forward reference (with from __future__ import annotations). This change would allow for direct attribute access, making the code safer and easier to understand.
| accounts_by_id: dict[str, object], | |
| accounts_by_id: dict[str, "LinkedAccountRecord"], |
| self.store.disconnect_account(user_id, linked_account_id) | ||
| if remaining_accounts: | ||
| replacement = remaining_accounts[0] | ||
| for session in self.store.list_sessions_for_user(user_id): | ||
| if session.active_linked_account_id == linked_account_id: | ||
| self.store.set_active_account( | ||
| session.session_id, | ||
| user_id, | ||
| replacement.id, | ||
| ) |
There was a problem hiding this comment.
🔴 disconnect_account session reassignment never executes because store already NULLs the field
In AuthService.disconnect_account, the store's disconnect_account is called first at line 329, which sets active_linked_account_id = NULL for all sessions pointing to the disconnected account (apps/api/app/storage/auth_store.py:791-798). Then, at lines 332-338, the code calls self.store.list_sessions_for_user(user_id) and checks if session.active_linked_account_id == linked_account_id. Since the store already set those sessions' active_linked_account_id to NULL, this condition is always False, and the replacement account is never assigned. On the next request, get_session (apps/api/app/services/auth_service.py:222-225) sees active_linked_account_id is None and deletes the session, effectively logging the user out even though other active accounts remain.
Prompt for agents
In apps/api/app/services/auth_service.py, in the disconnect_account method (lines 323-338), the call to self.store.disconnect_account() at line 329 sets active_linked_account_id to NULL for affected sessions BEFORE the code at lines 332-338 tries to reassign them. The fix is to either:
1. Swap the order: reassign sessions BEFORE calling self.store.disconnect_account(). Iterate over self.store.list_sessions_for_user(user_id), check if session.active_linked_account_id == linked_account_id, and call self.store.set_active_account() with the replacement. Then call self.store.disconnect_account().
OR
2. Remove the NULL-setting from the store's disconnect_account SQL and handle it entirely in the service layer.
Option 1 is simplest. The corrected disconnect_account method should look like:
def disconnect_account(self, user_id: str, linked_account_id: str) -> None:
remaining_accounts = [
account
for account in self.store.list_linked_accounts(user_id)
if account.id != linked_account_id and account.status == "active"
]
if remaining_accounts:
replacement = remaining_accounts[0]
for session in self.store.list_sessions_for_user(user_id):
if session.active_linked_account_id == linked_account_id:
self.store.set_active_account(
session.session_id,
user_id,
replacement.id,
)
self.store.disconnect_account(user_id, linked_account_id)
Was this helpful? React with 👍 or 👎 to provide feedback.
| const composerRef = useRef<HTMLTextAreaElement | null>(null); | ||
|
|
||
| const unreadOnly = listTab === "unread"; | ||
| const linkedAccounts = activeLinkedAccounts(session); |
There was a problem hiding this comment.
🔴 Infinite API call loop: linkedAccounts is not memoized, causing loadMailboxCounts to re-fire every render
linkedAccounts is computed inline at line 267 as activeLinkedAccounts(session) without useMemo, producing a new array reference on every render. This new reference causes the useCallback for loadMailboxCounts (lines 279-298) to create a new function identity each render. The effect at lines 422-427 depends on loadMailboxCounts, so it fires on every render. Inside, setMailboxCountsByScope(Object.fromEntries(entries)) sets state with a new object, triggering another render, which creates a new linkedAccounts → new loadMailboxCounts → effect fires again → infinite loop of API calls to /gmail/mailbox-counts for every scope.
Prompt for agents
In packages/features/src/mail/mail-workspace.tsx, at line 267, wrap the linkedAccounts computation in useMemo to stabilize its reference:
Change:
const linkedAccounts = activeLinkedAccounts(session);
To:
const linkedAccounts = useMemo(() => activeLinkedAccounts(session), [session]);
This prevents linkedAccounts from producing a new array reference on every render, which in turn stabilizes the loadMailboxCounts useCallback identity and stops the infinite effect loop. Note that session itself is set via setState and only changes when explicitly updated, so this memoization is correct.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Testing