fix(costs): eliminate N+1 in /tasks endpoint (#750)

[frankbria]

Closes #750

Problem

The costs /tasks endpoint was O(N) in queries against limit (capped at 1000):

• Router (costs_v2.py): _query_top_tasks called tasks.get() per row — each opening a new SQLite connection — for title lookup.
• Repository (token_repository.py): get_top_tasks_by_cost ran a dominant-agent subquery per task.

At limit=1000 (the badge-map case) that's ~1000 connection opens + ~2000 queries for a single dashboard request.

Fix

• tasks.get_titles(workspace, ids) — batch title resolution in one WHERE id IN (...) query; _query_top_tasks now calls it once instead of looping tasks.get().
• get_top_tasks_by_cost — dominant agent per task is picked with a single windowed CTE (ROW_NUMBER() OVER (PARTITION BY task_id ORDER BY COUNT(*) DESC, agent_id)), replacing the per-task subquery.

Query count is now O(1) regardless of limit — meeting the acceptance criteria (batched title lookup, dominant-agent folded into one windowed query).

Behavior

• Response shape unchanged. Missing tasks still fall back to the placeholder title (now via key-presence, preserving a genuinely-empty title).
• Tie-break on dominant agent is now deterministic (agent_id ascending) where it was previously "arbitrary" — a strict improvement.

Tests

• New: dominant-agent selection (most-used agent per task), per-task independence, and tasks.get_titles batch/omit-missing/empty-input cases.
• All 23 tests in tests/ui/test_costs_v2.py pass; ruff clean.

Known limitations

• The IN (...) batch relies on SQLite's 32766 bound-variable ceiling (3.32+); limit caps at 1000 so no chunking needed (noted in a code comment).

Summary by CodeRabbit

• New Features

  • Costs views now resolve task titles in bulk, improving response speed for task lists.
  • Task cost results now consistently show the most-used agent per task within the selected period.

• Bug Fixes

  • Missing task titles now fall back gracefully instead of breaking the response.
  • Results no longer rely on repeated per-item lookups, reducing slowdowns on larger task sets.

• Tests

  • Added coverage for batch title resolution and dominant-agent selection across multiple tasks.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 51 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3b9c9be6-fc38-4d01-8986-58a09998ae5b

📥 Commits

Reviewing files that changed from the base of the PR and between 3fa5574 and c2b66a9.

📒 Files selected for processing (1)

• codeframe/ui/routers/costs_v2.py

Walkthrough

Added a batch get_titles function for resolving multiple task titles in one query, replacing per-task lookups in the costs router. Refactored get_top_tasks_by_cost to use a single CTE-based windowed query with ROW_NUMBER() for dominant-agent resolution, eliminating N+1 database calls. Added corresponding tests.

Changes

N+1 Query Elimination for Costs Tasks Endpoint

Layer / File(s)	Summary
Batch task title lookup codeframe/core/tasks.py, codeframe/ui/routers/costs_v2.py, tests/ui/test_costs_v2.py	Adds get_titles to resolve multiple task titles via one IN (...) query; router batches task IDs and calls it once, falling back to placeholders for missing IDs; tests validate batch resolution and empty-input handling.
Windowed dominant-agent query codeframe/platform_store/repositories/token_repository.py, tests/ui/test_costs_v2.py	Replaces per-task agent subquery with a single CTE using ROW_NUMBER() to rank agents per task and left-join the top agent; row processing reads agent_id directly from query output; endpoint tests verify dominant-agent selection per task.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Endpoint as /api/v2/costs/tasks
  participant Router as costs_v2._query_top_tasks
  participant Repo as token_repository.get_top_tasks_by_cost
  participant Tasks as tasks.get_titles
  participant DB as SQLite

  Endpoint->>Repo: request top tasks by cost
  Repo->>DB: single CTE query with ROW_NUMBER() per task
  DB-->>Repo: rows with dominant agent_id
  Repo-->>Router: top tasks with agent_id
  Router->>Tasks: get_titles(workspace, all task_ids)
  Tasks->>DB: SELECT title WHERE id IN (...)
  DB-->>Tasks: matching titles
  Tasks-->>Router: {id: title} map
  Router-->>Endpoint: task entries with titles and dominant agents

Loading

Poem

A hop, a query, one and done,
No more N+1 under the sun! 🐇
Titles fetched in a single bound,
Dominant agents neatly found,
CTEs whir like carrot stew—
This bunny's database dreams come true! 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely reflects the main change: removing the N+1 pattern in the costs /tasks endpoint.
Linked Issues check	✅ Passed	The changes batch title lookup and fold dominant-agent selection into a single CTE/window query, matching #750's acceptance criteria.
Out of Scope Changes check	✅ Passed	The diff stays focused on the /tasks costs path and its tests, with no unrelated feature or behavior changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/750-costs-tasks-n-plus-1

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Review

This PR replaces the N+1 title lookup (tasks.get() per row, each opening its own SQLite connection) with a single batched tasks.get_titles() call, and replaces the N+1 dominant-agent subquery in get_top_tasks_by_cost with a single windowed CTE using ROW_NUMBER(). Both changes correctly preserve the response shape and existing top-N-by-cost selection logic, and the new tests (dominant agent, per-task independence, batch/omit-missing/empty-input for get_titles) look sound. No prior human/bot review is present on this PR yet (only CodeRabbit's in-progress placeholder).

Findings

1. codeframe/ui/routers/costs_v2.py:230-235 — batch title lookup failure now blanks titles for every row, not just the affected one.
   Previously each row's tasks.get() call was wrapped individually, so a failure for one task only degraded that row's title to the placeholder. Now tasks_module.get_titles(...) is a single call wrapped in one try/except; if it raises for any reason (locked DB, transient SQLite error), titles = {} and every row in the response falls back to "Unknown task (...)", even for tasks that would otherwise have resolved fine. This is a real fault-isolation regression versus the pre-PR behavior, and it's not covered by the new tests (TestCostsTasksDominantAgent / TestGetTitlesBatch don't exercise the lookup-failure path). Worth a comment either way — if this is an accepted tradeoff for the perf win, that's reasonable, but it should be a conscious choice rather than a side effect.

2. codeframe/platform_store/repositories/token_repository.py:420-433 — agent_ranked CTE aggregates over every task in the window, not just the top-limit tasks selected by per_task.
   The old N+1 subquery ran once per task in per_task (bounded by limit, indexed WHERE task_id = ? lookup each time). The new agent_ranked CTE has no correlation to per_task — it does GROUP BY task_id, agent_id with a window function over all task_ids in the timestamp range before the final join discards everything except the limit tasks that survive. For a workspace with many distinct tasks touched in the window but a small limit (e.g. limit=10 against thousands of tasks), this trades N indexed point-lookups for one unbounded aggregate scan — the opposite direction of the stated goal ("O(1) queries regardless of limit" is true, but the total rows scanned is no longer bounded by limit). Restricting agent_ranked to WHERE task_id IN (SELECT task_id FROM per_task) would keep it a single query while bounding the work to the same set per_task already selected.

3. codeframe/core/tasks.py:211 — stray "ponytail" in the comment.
   # ponytail: limit is capped at 1000 upstream, ... — looks like an editing artifact (should probably just be "Note:"). Minor, but worth cleaning up before merge since it reads as noise in an otherwise clear docstring/comment.

Nothing else stood out — the SQL parameter binding order for the two CTEs is correct, the IN (...) batching is safely parameterized (no injection risk), and the dominant-agent tie-break change (arbitrary → agent_id ASC) is a genuine, documented improvement.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

codeframe/platform_store/repositories/token_repository.py (1)

406-433: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Consider adding an index on token_usage(task_id, timestamp).

Both per_task and agent_ranked filter/group on task_id/timestamp over the full table for every call. Without a supporting index, this remains a full table scan even though the query count is now O(1); an index would materially help as token_usage grows.

🤖 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 `@codeframe/platform_store/repositories/token_repository.py` around lines 406 -
433, The query in token_repository’s per_task and agent_ranked CTEs still scans
token_usage by task_id and timestamp on every call, so add a supporting index on
token_usage(task_id, timestamp) in the repository’s schema/migration setup.
Update the token_repository data model or migration logic where token_usage
indexes are defined so these filters and groupings can use the index efficiently
without changing the query behavior.

🤖 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 `@codeframe/ui/routers/costs_v2.py`:
- Around line 226-243: The batch title lookup failure in the costs_v2 flow is
logged at debug even though it has the same fallback behavior as the
sqlite3.Error branch. Update the exception handler around
tasks_module.get_titles in the title-resolution logic to log at warning instead
of debug, keeping exc_info=True and the same contextual message so operators can
detect systemic lookup failures consistently.

---

Nitpick comments:
In `@codeframe/platform_store/repositories/token_repository.py`:
- Around line 406-433: The query in token_repository’s per_task and agent_ranked
CTEs still scans token_usage by task_id and timestamp on every call, so add a
supporting index on token_usage(task_id, timestamp) in the repository’s
schema/migration setup. Update the token_repository data model or migration
logic where token_usage indexes are defined so these filters and groupings can
use the index efficiently without changing the query behavior.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b1714524-acd6-4a6d-9e64-c733163b5a37

📥 Commits

Reviewing files that changed from the base of the PR and between f395a3c and 3fa5574.

📒 Files selected for processing (4)

• codeframe/core/tasks.py
• codeframe/platform_store/repositories/token_repository.py
• codeframe/ui/routers/costs_v2.py
• tests/ui/test_costs_v2.py

[frankbria]

Demo — acceptance criteria verified with outcome evidence

Instrumented sqlite3.connect to count connection opens and SQL statements executed while serving GET /api/v2/costs/tasks, across growing task counts and limits:

scenario	connections	statements	rows	dominant-agent correct
limit=10, n_tasks=20	2	7	10	✅
limit=10, n_tasks=200	2	7	10	✅
limit=1000, n_tasks=200	2	7	200	✅

Mapping to acceptance criteria:

• Title lookup batched via WHERE id IN (...) → connections stay at 2 regardless of row count (was ~1 + N: one per title lookup).
• Dominant-agent folded into one windowed query → statements stay at 7 regardless of rows (was ~2 + N: one subquery per task); the dominant agent (codex, 3 calls vs 1) is correct for all 200 tasks.
• O(1) queries regardless of limit → conns=2 / stmts=7 are identical whether the endpoint returns 10 or 200 rows.

Pre-fix these numbers grew linearly with the number of tasks; post-fix they are flat.

[frankbria]

Applied — bumped the batch title-lookup failure log to warning in c2b66a9 for parity with the sqlite3.Error path. Thanks @coderabbitai.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@frankbria Thanks for the update — confirmed the log level bump to warning in c2b66a9 for parity with the sqlite3.Error path. Looks good.

٩(◕‿◕)۶

[claude]

Follow-up review

Only one commit landed since my last review (c2b66a9): the batch title-lookup failure log level bumped from debug to warning, addressing CodeRabbit's actionable comment. My three findings from the previous review are still present in the code as of the latest diff — flagging status on each rather than repeating the full analysis:

1. costs_v2.py — fault-isolation regression on title lookup (still open). _query_top_tasks still wraps the single tasks_module.get_titles(...) call in one try/except; a failure there still blanks every row's title to the placeholder, versus the old per-row isolation. The warning-level log (this PR's only new commit) makes the failure visible to operators, which helps, but doesn't restore per-row isolation. If that's an accepted tradeoff for the O(1)-query win (reasonable, given get_titles failing at all likely means the DB itself is having a bad time), it'd still be worth a one-line comment saying so explicitly, since it's not obvious from the code that this is intentional rather than overlooked.

2. token_repository.py:406-433 — agent_ranked CTE still unbounded by limit (still open). It aggregates GROUP BY task_id, agent_id with a window function over every task_id touched in the timestamp window, then the final LEFT JOIN/ORDER BY/(implicit) result discards everything outside the per_task-selected rows. So the query count is O(1) as claimed, but the scan volume for agent_ranked is no longer bounded by limit the way the old per-task subquery was (each old subquery was an indexed WHERE task_id = ? lookup, bounded by the limit tasks in per_task). For a workspace with many distinct tasks in the window and a small limit (e.g. limit=10 against thousands of tasks), this is trading N cheap point-lookups for one full aggregate scan. Restricting with WHERE task_id IN (SELECT task_id FROM per_task) on agent_ranked would keep the single-query shape while re-bounding the scan to the same task set per_task already picked. Demo numbers in the PR (n_tasks=20/200, limit=10/1000) are too small to show this — it'd only show up with many more distinct tasks than limit.

3. codeframe/core/tasks.py:211 — stray "ponytail" in the comment (still open, cosmetic). # ponytail: limit is capped at 1000 upstream, ... — reads as a leftover autocomplete artifact rather than intentional wording (probably meant "Note:"). Trivial, but worth a quick fix before merge since it's the kind of thing that looks odd in git blame later.

Also noting CodeRabbit's nitpick (an index on token_usage(task_id, timestamp)) is still unaddressed — reasonable to leave for a follow-up since it's non-blocking and the CTEs' correctness isn't affected either way.

Nothing new to add beyond these three points plus the index nitpick — the core approach (batched IN (...) title lookup, windowed ROW_NUMBER() for dominant agent) is sound and the new tests for it are good coverage for the happy path.