feat(tasks): wire up multi-account task sync - #55
Open
corrin wants to merge 4 commits into
Open
Conversation
POST /api/tasks/sync was a stub returning "Sync not yet implemented."
and the complete sync_provider_tasks() service was never called, so
real provider tasks never reached the combined list.
- Add sync_all_task_accounts() orchestrator: loops a user's task-enabled
ExternalAccounts, maps DB provider -> task provider (reusing
PROVIDER_TO_TASK), fetches via TaskManager, upserts via the existing
content-hash sync, and is resilient (one bad account does not abort the
batch). Returns {accounts, upserted, failed}.
- Implement POST /api/tasks/sync to run it (manual "Sync now"; the
frontend store already calls this and refetches).
- Add task_sync_loop background job (asyncio, 15-min interval) mirroring
token_refresh_loop, started/cancelled in the app lifespan; plus
ExternalAccount.get_user_ids_with_task_accounts() to enumerate users.
- Fix a latent runtime-breaking import in google_tasks.py (move the
stubs-only googleapiclient._apis import under TYPE_CHECKING), exposed
now that TaskManager is imported at runtime.
- Tests: unit coverage for provider mapping / failure isolation / empty
case, an integration test driving the endpoint end-to-end with
idempotency, and an e2e tasks/sync test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
It asserted task cards appear, which cannot pass until a task provider is actually connected (no Todoist token; calendar accounts are calendar-only). The sync endpoint is genuinely covered by the integration test (tests/integration/test_tasks.py::TestSync). A real e2e test belongs with the Todoist OAuth work, when there is data to run against. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds provider task sync across users and accounts, wires a periodic background worker into app startup, introduces a task-enable action for external accounts and the settings UI control for it, and expands unit, integration, and end-to-end test coverage. ChangesTask Sync and Task Enable Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (2)
backend/underway/models/external_account.py (1)
136-140: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMirror the actual syncable-account filters here.
sync_all_users()uses this query to decide which users get a background sync pass, butsync_all_task_accounts()later excludesneeds_reauthand missing-credential accounts viaget_task_accounts_for_user(). Users whose only task accounts are stale still get scanned every 15 minutes for a guaranteed no-op. Reusing the same predicates here keeps the fan-out set aligned with accounts that can really sync.♻️ Proposed change
`@classmethod` async def get_user_ids_with_task_accounts(cls, session: AsyncSession) -> list[uuid.UUID]: """Return distinct user ids that have at least one task-enabled account.""" - stmt = select(cls.user_id).where(cls.use_for_tasks.is_(True)).distinct() + stmt = ( + select(cls.user_id) + .where( + cls.use_for_tasks.is_(True), + cls.needs_reauth.is_(False), + or_( + and_(cls.provider == "todoist", cls.api_key.is_not(None)), + and_(cls.provider.in_(["google", "o365"]), cls.token.is_not(None)), + ), + ) + .distinct() + ) result = await session.execute(stmt) return list(result.scalars().all())🤖 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 `@backend/underway/models/external_account.py` around lines 136 - 140, `ExternalAccount.get_user_ids_with_task_accounts()` is selecting users too broadly because it only checks `use_for_tasks`, while `sync_all_task_accounts()` later filters out accounts that cannot actually sync. Update this query to mirror the same syncable-account predicates used by `get_task_accounts_for_user()` so `sync_all_users()` only fans out to users with at least one реально syncable task account. Keep the distinct user-id return shape unchanged.backend/tests/unit/test_task_sync.py (1)
141-172: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the persisted provider mapping, not just the
TaskManagercall mapping.These tests prove
get_tasks()receives"google_tasks"/"outlook", but the only DB assertion still exercises"todoist". A regression that writesTask.provider="google"or"o365"would pass here even thoughsync_all_task_accounts()is supposed to persist the mapped provider key.Possible follow-up test
+ async def test_persists_mapped_provider_name(self, db_session: AsyncSession) -> None: + user = await _create_user(db_session) + await _create_task_account(db_session, user, "google", "g@example.com") + pt = _make_provider_task(title="Imported") + + manager = _FakeTaskManager(tasks_by_email={"g@example.com": [pt]}) + summary = await sync_all_task_accounts(db_session, user.id, manager) + + assert summary == {"accounts": 1, "upserted": 1, "failed": 0} + result = await db_session.execute(select(Task).where(Task.user_id == user.id)) + task = result.scalar_one() + assert task.provider == "google_tasks"🤖 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 `@backend/tests/unit/test_task_sync.py` around lines 141 - 172, The current test only verifies the TaskManager provider mapping, but it does not assert that sync_all_task_accounts persists the mapped provider key into Task.provider for non-todoist accounts. Update test_syncs_provider_tasks_into_db in TestSyncAllTaskAccounts to create a google or o365 account, sync a provider task through _FakeTaskManager, and assert the saved Task.provider matches the mapped key used by sync_all_task_accounts rather than the original DB provider value.
🤖 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 `@backend/underway/services/task_sync.py`:
- Around line 53-60: The per-account write path in sync_provider_tasks is not
isolated, so a database failure can leave the shared AsyncSession in a bad state
and affect later accounts. Wrap the provider-specific fetch/write block in
task_sync inside session.begin_nested() so each account runs in a savepoint, and
keep the existing exception logging/failed counter handling around that nested
transaction.
---
Nitpick comments:
In `@backend/tests/unit/test_task_sync.py`:
- Around line 141-172: The current test only verifies the TaskManager provider
mapping, but it does not assert that sync_all_task_accounts persists the mapped
provider key into Task.provider for non-todoist accounts. Update
test_syncs_provider_tasks_into_db in TestSyncAllTaskAccounts to create a google
or o365 account, sync a provider task through _FakeTaskManager, and assert the
saved Task.provider matches the mapped key used by sync_all_task_accounts rather
than the original DB provider value.
In `@backend/underway/models/external_account.py`:
- Around line 136-140: `ExternalAccount.get_user_ids_with_task_accounts()` is
selecting users too broadly because it only checks `use_for_tasks`, while
`sync_all_task_accounts()` later filters out accounts that cannot actually sync.
Update this query to mirror the same syncable-account predicates used by
`get_task_accounts_for_user()` so `sync_all_users()` only fans out to users with
at least one реально syncable task account. Keep the distinct user-id return
shape unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 12f614e8-f89f-45a3-9129-31fadec1cccd
📒 Files selected for processing (8)
backend/tests/integration/test_tasks.pybackend/tests/unit/test_task_sync.pybackend/underway/app.pybackend/underway/models/external_account.pybackend/underway/providers/google_tasks.pybackend/underway/providers/task_sync_loop.pybackend/underway/services/task_sync.pybackend/underway/viewsets/tasks.py
Comment on lines
+53
to
+60
| try: | ||
| provider_tasks = await task_manager.get_tasks(session, user_id, account.external_email, task_provider) | ||
| upserted += await sync_provider_tasks( | ||
| session, user_id, account.external_email, task_provider, provider_tasks | ||
| ) | ||
| except Exception: | ||
| logger.exception("Sync failed for account %s (%s)", account.external_email, task_provider) | ||
| failed += 1 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files backend/underway/services/task_sync.py backend/underway/providers/task_sync_loop.py backend/underway/viewsets/tasks.py backend/underway/services | sed 's#^`#FILE` #'
echo '--- task_sync.py outline ---'
ast-grep outline backend/underway/services/task_sync.py --view expanded || true
echo '--- sync_provider_tasks references ---'
rg -n "sync_provider_tasks|begin_nested|AsyncSession|session.begin" backend/underway -S
echo '--- task_sync.py lines ---'
nl -ba backend/underway/services/task_sync.py | sed -n '1,220p'Repository: corrin/underway
Length of output: 11122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [
Path("backend/underway/services/task_sync.py"),
Path("backend/underway/providers/task_sync_loop.py"),
Path("backend/underway/auth/dependencies.py"),
Path("backend/underway/models/base.py"),
]:
print(f"\n===== {path} =====")
text = path.read_text()
for i, line in enumerate(text.splitlines(), 1):
if path.name == "task_sync.py" or path.name in {"task_sync_loop.py", "dependencies.py", "base.py"}:
print(f"{i:4d}: {line}")
PYRepository: corrin/underway
Length of output: 14081
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [
Path("backend/underway/services/task_sync.py"),
Path("backend/underway/providers/task_sync_loop.py"),
Path("backend/underway/auth/dependencies.py"),
Path("backend/underway/models/base.py"),
]:
print(f"\n===== {path} =====")
text = path.read_text()
for i, line in enumerate(text.splitlines(), 1):
print(f"{i:4d}: {line}")
PYRepository: corrin/underway
Length of output: 14081
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
paths = [Path("tests"), Path("backend/underway")]
for base in paths:
if base.exists():
print(f"===== {base} =====")
for p in sorted(base.rglob("*task*sync*")):
if p.is_file():
print(p)
PY
echo '--- test references ---'
rg -n "sync_all_task_accounts|sync_provider_tasks|begin_nested|rollback|savepoint|failed" tests backend/underway -SRepository: corrin/underway
Length of output: 4315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for base in [Path("tests"), Path("backend/underway")]:
if base.exists():
print(f"===== {base} =====")
for p in sorted(base.rglob("*task*sync*")):
if p.is_file():
print(p)
PY
echo '--- test references ---'
rg -n "sync_all_task_accounts|sync_provider_tasks|begin_nested|rollback|savepoint|failed" tests backend/underway -SRepository: corrin/underway
Length of output: 4315
Wrap the per-account write path in a savepoint. In backend/underway/services/task_sync.py:53-60, a DB exception inside sync_provider_tasks() can poison the shared AsyncSession for later accounts and still leave partial work from that account pending for the user-level commit. Use session.begin_nested() around the write path so one failed account stays isolated.
🤖 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 `@backend/underway/services/task_sync.py` around lines 53 - 60, The per-account
write path in sync_provider_tasks is not isolated, so a database failure can
leave the shared AsyncSession in a bad state and affect later accounts. Wrap the
provider-specific fetch/write block in task_sync inside session.begin_nested()
so each account runs in a savepoint, and keep the existing exception
logging/failed counter handling around that nested transaction.
Make a real OAuth task provider possible so sync has something to pull
(the Todoist api-key path is being removed). The Google OAuth already
requests the tasks scope (GOOGLE_CALENDAR_SCOPES), but nothing could mark
a Google/O365 account use_for_tasks, so get_task_accounts_for_user never
returned them.
- Add POST /api/external-accounts/{id}/use-for-tasks (detail action on the
read-only viewset) that sets use_for_tasks + is_primary_tasks, scoped to
the current user. Integration tests: enable, cross-user 404, bad-id 400.
- Settings UI: a "Use for tasks" button on google/o365 account cards that
calls it and refetches.
GoogleTaskProvider read path already works once the token carries the tasks
scope and the account is task-enabled.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Recreates the deleted task-sync e2e test as a REAL one (not a placeholder): it creates a sentinel task in a live Google Tasks account, clicks Sync in the UI, and asserts the task appears in the combined list; then edits it upstream and re-syncs (assert reflected), and deletes it upstream and re-syncs (assert reconciled away). Drives both UI and API (via ngrok). Fails loudly if sync does not perform correctly. Sentinel tasks are cleaned up on teardown. - conftest: google_tasks fixture (reads the connected, task-enabled Google account from the app DB; skips with guidance if none) + GoogleTasksSandbox. - scripts/run-e2e.sh: runs the suite with -p no:asyncio -p no:playwright (both plugins inject an event loop that breaks sync Playwright) and documents the close-Chrome / connect-Google prerequisites. Not yet executed green — needs the full stack up and a Google account connected for tasks (Settings: re-auth Google, then Use for tasks). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@backend/tests/e2e/conftest.py`:
- Around line 113-129: The Google task-account lookup is too broad because
`_load_google_tasks_account()` selects the first eligible `ExternalAccount`
across the entire database, which can target the wrong user during e2e runs.
Update this helper, and the call sites around the later sync assertions, to
scope the query to the authenticated e2e user’s account identity from the active
page/JWT fixture so only that user’s Google account is mutated and verified.
Keep the filtering tied to the existing `ExternalAccount` lookup and the user
context already available in the test setup.
In `@backend/underway/viewsets/external_accounts.py`:
- Around line 48-59: Reject unsupported providers before setting task-related
state in the external account flow. In the branch that updates
`account.use_for_tasks` and calls `ExternalAccount.set_as_primary`, add a
provider guard so only supported providers such as Google and O365 can be marked
as tasks-enabled/primary; otherwise raise an HTTP error and exit before
persisting anything. Use the existing `account`,
`ExternalAccount.set_as_primary`, and `use_for_tasks` logic in
`external_accounts.py` to place the check right before the assignment and flush.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ae71e26-c918-4c37-8cb7-dd5e4a2834c9
📒 Files selected for processing (6)
backend/scripts/run-e2e.shbackend/tests/e2e/conftest.pybackend/tests/e2e/test_tasks.pybackend/tests/integration/test_external_accounts.pybackend/underway/viewsets/external_accounts.pyfrontend/src/views/SettingsView.vue
✅ Files skipped from review due to trivial changes (1)
- backend/scripts/run-e2e.sh
Comment on lines
+113
to
+129
| async def _load_google_tasks_account() -> tuple[str, str | None, str | None] | None: | ||
| """Read a connected, task-enabled Google account from the app DB, or None.""" | ||
| settings = get_settings() | ||
| engine = create_async_engine(settings.database_url) | ||
| try: | ||
| async with async_sessionmaker(engine, expire_on_commit=False)() as session: | ||
| result = await session.execute( | ||
| select(ExternalAccount).where( | ||
| ExternalAccount.provider == "google", | ||
| ExternalAccount.use_for_tasks.is_(True), | ||
| ExternalAccount.needs_reauth.is_(False), | ||
| ) | ||
| ) | ||
| account = result.scalars().first() | ||
| if account is None: | ||
| return None | ||
| return account.external_email, account.token, account.refresh_token |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scope the Google sandbox account to the authenticated e2e user.
Line 126 picks the first eligible Google task account in the whole database, but the page/JWT under test can belong to a different user. As soon as multiple users have task-enabled Google accounts, this fixture starts mutating one account and asserting sync results against another.
Also applies to: 171-180
🤖 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 `@backend/tests/e2e/conftest.py` around lines 113 - 129, The Google
task-account lookup is too broad because `_load_google_tasks_account()` selects
the first eligible `ExternalAccount` across the entire database, which can
target the wrong user during e2e runs. Update this helper, and the call sites
around the later sync assertions, to scope the query to the authenticated e2e
user’s account identity from the active page/JWT fixture so only that user’s
Google account is mutated and verified. Keep the filtering tied to the existing
`ExternalAccount` lookup and the user context already available in the test
setup.
Comment on lines
+48
to
+59
| if account is None: | ||
| raise HTTPException(status_code=404, detail="Account not found.") | ||
| else: | ||
| account.use_for_tasks = True | ||
| await ExternalAccount.set_as_primary( | ||
| session, | ||
| external_email=account.external_email, | ||
| provider=account.provider, | ||
| user_id=user.id, | ||
| account_type="tasks", | ||
| ) | ||
| await session.flush() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject unsupported providers before persisting use_for_tasks.
The UI only offers this action for Google/O365, but Line 51 currently marks any owned external account as task-enabled and primary. That stores an invalid task-source state for providers the sync flow does not support.
Suggested fix
account = result.scalar_one_or_none()
if account is None:
raise HTTPException(status_code=404, detail="Account not found.")
- else:
- account.use_for_tasks = True
- await ExternalAccount.set_as_primary(
- session,
- external_email=account.external_email,
- provider=account.provider,
- user_id=user.id,
- account_type="tasks",
- )
- await session.flush()
+ if account.provider not in {"google", "o365"}:
+ raise HTTPException(
+ status_code=400,
+ detail="Tasks are only supported for Google and Microsoft 365 accounts.",
+ )
+
+ account.use_for_tasks = True
+ await ExternalAccount.set_as_primary(
+ session,
+ external_email=account.external_email,
+ provider=account.provider,
+ user_id=user.id,
+ account_type="tasks",
+ )
+ await session.flush()📝 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
| if account is None: | |
| raise HTTPException(status_code=404, detail="Account not found.") | |
| else: | |
| account.use_for_tasks = True | |
| await ExternalAccount.set_as_primary( | |
| session, | |
| external_email=account.external_email, | |
| provider=account.provider, | |
| user_id=user.id, | |
| account_type="tasks", | |
| ) | |
| await session.flush() | |
| if account is None: | |
| raise HTTPException(status_code=404, detail="Account not found.") | |
| if account.provider not in {"google", "o365"}: | |
| raise HTTPException( | |
| status_code=400, | |
| detail="Tasks are only supported for Google and Microsoft 365 accounts.", | |
| ) | |
| account.use_for_tasks = True | |
| await ExternalAccount.set_as_primary( | |
| session, | |
| external_email=account.external_email, | |
| provider=account.provider, | |
| user_id=user.id, | |
| account_type="tasks", | |
| ) | |
| await session.flush() |
🤖 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 `@backend/underway/viewsets/external_accounts.py` around lines 48 - 59, Reject
unsupported providers before setting task-related state in the external account
flow. In the branch that updates `account.use_for_tasks` and calls
`ExternalAccount.set_as_primary`, add a provider guard so only supported
providers such as Google and O365 can be marked as tasks-enabled/primary;
otherwise raise an HTTP error and exit before persisting anything. Use the
existing `account`, `ExternalAccount.set_as_primary`, and `use_for_tasks` logic
in `external_accounts.py` to place the check right before the assignment and
flush.
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.
What
Connects the one missing wire in the "combined task list from multiple accounts" feature. Everything around it already existed — providers, the multi-account
ExternalAccountmodel, the combinedGET /api/taskslist, the frontend Kanban board, and a complete content-hash sync service — butsync_provider_tasks()was never called andPOST /api/tasks/syncreturned"Sync not yet implemented.". So real Todoist/Google/Outlook tasks never reached the list.Changes
sync_all_task_accounts()(services/task_sync.py) — loops a user's task-enabled accounts, maps DB provider → task provider (reusing the existingPROVIDER_TO_TASK), fetches viaTaskManager, upserts via the existing content-hash sync. Resilient: one failing account is logged/counted, never aborts the batch. Returns{accounts, upserted, failed}.POST /api/tasks/syncnow runs it (manual "Sync now"; the frontend store already calls it and refetches).task_sync_loop(providers/task_sync_loop.py) — background asyncio job, 15-min interval, mirroring the existingtoken_refresh_loop; started/cancelled in the app lifespan. PlusExternalAccount.get_user_ids_with_task_accounts().google_tasks.pyimported the stubs-onlygoogleapiclient._apisat runtime — harmless until now because nothing importedTaskManagerat runtime. Moved underTYPE_CHECKING(same pattern ascalendar/google.py). No suppressions.Architecture note
In-process asyncio loop, not Celery — matches the only background pattern in the repo (
token_refresh_loop), no broker dependency, single-instance deploy.sync_all_task_accountsis runtime-agnostic, so a future move to a queue would change only the caller. Known limit: >1 app instance would duplicate the loop's work.Verification
mypy --strict+ruffclean.POST /api/tasks/sync→{"status":"ok","accounts":0,"upserted":0,"failed":0}(real orchestrator running, not the old stub).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
/api/tasks/syncwith real upsert results.Bug Fixes
Tests