fix(posting): eliminate N+1 API calls in _post_full_review#22
fix(posting): eliminate N+1 API calls in _post_full_review#22Alm0stSurely wants to merge 1 commit intonexiouscaliver:mainfrom
Conversation
## Problem _post_full_review() re-fetched MR diff refs for each finding, resulting in N+1 API calls for a review with N findings. ## Solution - Extract _post_inline_thread_with_refs() that accepts pre-fetched refs - Fetch diff refs once in _post_full_review() before the loop - Pass refs to inline thread posting function - _post_inline_thread() maintains backward compatibility ## Impact For a review with 15 findings: ~16 API calls → 2 API calls Fixes nexiouscaliver#6
There was a problem hiding this comment.
Pull request overview
Reduces redundant GitLab API calls by eliminating an N+1 pattern when posting inline review threads for a merge request.
Changes:
- Introduces
_post_inline_thread_with_refs()to post inline threads using pre-fetched MR diff refs. - Updates
_post_full_review()to fetch diff refs once and reuse them for all findings. - Updates/extends posting tests and adds
test_diff_refs_fetched_onceto verify the N+1 fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| plugins/omniforge/tools/omniforge_mcp_server.py | Adds a “with pre-fetched refs” posting path and reuses diff refs across all inline threads in full review posting. |
| plugins/omniforge/tests/test_posting.py | Adjusts mocks for new call ordering and adds a test asserting diff refs are fetched once. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Fetch diff refs once for all inline threads (avoids N+1 API calls) | ||
| diff_refs = await _get_mr_diff_refs(mr_id, repo_root) | ||
| if not diff_refs or not diff_refs.get("success"): | ||
| return { | ||
| "success": False, | ||
| "mr_id": mr_id, | ||
| "summary_posted": False, | ||
| "threads_posted": 0, | ||
| "threads_total": len(findings), | ||
| "errors": [diff_refs.get("error", f"Could not fetch diff refs for MR !{mr_id}.") if diff_refs else f"Could not fetch diff refs for MR !{mr_id}."], | ||
| } | ||
|
|
There was a problem hiding this comment.
In _post_full_review, diff refs are fetched before attempting to post the summary and before determining whether there are any valid findings. This changes behavior: if _get_mr_diff_refs fails (e.g., JSON parse error), the function returns early and the summary is never posted; it also introduces an extra API call for empty/all-invalid findings.
Consider posting the summary first, then fetching diff refs only if there is at least one valid finding to post (or lazily fetching on the first valid finding). This avoids unnecessary network I/O and preserves the prior ability to post a summary even when diff-ref fetching fails.
| mock_run.return_value = _make_result(0, SAMPLE_DIFF_REFS_JSON) | ||
| result = asyncio.run(_post_full_review("136", "Summary", [], repo)) | ||
|
|
||
| assert result["success"] is True | ||
| assert result["summary_posted"] is True | ||
| assert result["threads_posted"] == 0 | ||
| assert result["threads_total"] == 0 |
There was a problem hiding this comment.
test_empty_findings_list now stubs run_exec to always return SAMPLE_DIFF_REFS_JSON, which makes the test pass even if _post_full_review incorrectly calls glab mr view for an empty findings list. If the intended behavior is to avoid unnecessary diff-ref fetches when findings is empty, update this test to use a side effect that distinguishes mr note vs mr view, and assert that no mr view -F json call occurs.
| mock_run.return_value = _make_result(0, SAMPLE_DIFF_REFS_JSON) | |
| result = asyncio.run(_post_full_review("136", "Summary", [], repo)) | |
| assert result["success"] is True | |
| assert result["summary_posted"] is True | |
| assert result["threads_posted"] == 0 | |
| assert result["threads_total"] == 0 | |
| view_called = False | |
| def side_effect(args, cwd=None, timeout=60): | |
| nonlocal view_called | |
| cmd = " ".join(args) | |
| if "mr" in cmd and "view" in cmd and "-F" in cmd: | |
| view_called = True | |
| return _make_result(1, stderr="mr view should not be called for empty findings") | |
| if "mr" in cmd and "note" in cmd: | |
| return _make_result(0) | |
| return _make_result(0) | |
| mock_run.side_effect = side_effect | |
| result = asyncio.run(_post_full_review("136", "Summary", [], repo)) | |
| assert result["success"] is True | |
| assert result["summary_posted"] is True | |
| assert result["threads_posted"] == 0 | |
| assert result["threads_total"] == 0 | |
| assert view_called is False |
| call_count = 0 | ||
| view_call_count = 0 | ||
| def side_effect(args, cwd=None, timeout=60): | ||
| nonlocal call_count, view_call_count | ||
| call_count += 1 |
There was a problem hiding this comment.
In test_diff_refs_fetched_once, call_count is incremented but never used in any assertion. Removing it (or asserting the expected total call count) will keep the test focused and avoid unused-variable noise.
| call_count = 0 | |
| view_call_count = 0 | |
| def side_effect(args, cwd=None, timeout=60): | |
| nonlocal call_count, view_call_count | |
| call_count += 1 | |
| view_call_count = 0 | |
| def side_effect(args, cwd=None, timeout=60): | |
| nonlocal view_call_count |
|
Could you please resolve the issues surfaced by the copilot and we will be good to merge, Thanks btw for showing intrest here ;) |
Problem
_post_full_review()iterates over findings and calls_post_inline_thread()for each. Each call independently fetches MR diff refs via_get_mr_diff_refs().For a review with 15 findings, this means 15 redundant API calls to fetch the same SHA refs.
Analysis
The N+1 pattern occurs because:
Solution
_post_inline_thread_with_refs()that accepts pre-fetched diff refs_post_full_review()before the loop_post_inline_thread()maintains backward compatibilityBenchmarks
Testing
test_diff_refs_fetched_onceto verify N+1 fixFixes #6