Conversation
|
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe cycle progress endpoint now calculates backlog, unstarted, started, cancelled, completed, and total issue counts in one aggregate query. Contract tests cover returned counts, issue exclusions, query count, and snapshot responses. ChangesCycle progress count aggregation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to No cycle-progress count regression was established, and no issue remains that should block merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CycleProgressEndpoint resolved its six work-item counts with six sequential .count() calls, each re-running the same issues/cycle_issues/states join over the cycle's entire work-item set and differing only by state__group. Collapse them into one .aggregate() using Count(filter=Q(...)), which compiles to Postgres COUNT(*) FILTER (WHERE ...). Six round trips and six scans become one. The filter predicates, the Issue.issue_objects manager semantics and the response payload are unchanged. Add contract tests covering the per-group counts, the exclusion rules (soft-deleted cycle links, archived/draft/triage work items, items outside the cycle), the progress_snapshot branch, and a regression guard asserting the counts stay a single query.
This branch has not been deployed
2 participants
Description
CycleProgressEndpoint(GET …/cycles/<cycle_id>/progress/) resolved its six work-item counts with six sequential.count()queries. This collapses them into one.aggregate().No behaviour change — same numbers, same response payload, one round trip instead of six.
The problem
The six blocks in
plane/app/views/cycle/base.pywere byte-identical except for a single predicate:Each one re-executes the same
issues ⋈ cycle_issues ⋈ statesjoin across the cycle's entire work-item set, discards the rows, and returns one integer. Six times per request.Three reasons this is worth fixing rather than leaving alone:
Cycle.progress_snapshotdefaults to{}(plane/db/models/cycle.py), which is falsy — so every active cycle takes the live-count branch. The snapshot branch only serves cycles already frozen.Issue.Metadeclares no indexes andstate__groupis unindexed, so each of the six is a scan over the cycle's work items. That cost is paid six times for data one pass already has in hand.CycleAnalyticsEndpoint, ~30 lines below, usesCount(..., filter=Q(...)), and the estimate-points aggregate directly above already uses a single.aggregate()withCase/When. The counts were the odd ones out.The change
Count(filter=Q(...))compiles to PostgresCOUNT(*) FILTER (WHERE …), so all six numbers come out of one pass over one join:Why this is safe
A refactor of how the numbers are fetched, not which rows are counted:
.filter()verbatim;state__groupmoves into eachCount'sfilter=.Issue.issue_objects, so the archived / draft / triage / soft-deleted exclusions are untouched.Count("id")counts joined rows exactly as the original.count()did. I deliberately did not adddistinct=True— that would be a behaviour change, not an optimisation.Response(...)block has zero diff.Net: −42 / +15 in
base.py.Type of Change
N/A — no user-facing change. The response payload is byte-identical; only the query count differs.
Test Scenarios
New file:
plane/tests/contract/app/test_cycle_progress_app.py. This endpoint previously had no test coverage at either tier.test_counts_are_reported_per_state_grouptotal_issuesis their sumtest_out_of_scope_work_items_are_excludedCycleIssuelink, work item in no cycle, and archived / draft / triage items all excluded — one case per predicate the aggregate must keep honouringtest_counts_are_resolved_in_a_single_queryCOUNTquery touchingcycle_issues. Scoped that way so unrelated query-count changes elsewhere in the request don't make it brittletest_snapshot_is_preferred_over_live_countsprogress_snapshotbranch still short-circuits and issues no count queryruff checkandruff format --checkclean on both files.References
No existing issue — found while reading the cycle views. Happy to open one first if you'd prefer that for perf work.
Scope kept deliberately narrow: one endpoint, one concern. The same pattern exists elsewhere (
WorkspaceUserProfileStatsEndpointandanalytic/advance.pyeach run 5–6 sequential.count()calls over a shared base queryset) — glad to follow up if this lands well, but I'd rather not bundle them.Summary by CodeRabbit