From e5a352b4187bd8707ec89fab245340d64af51bfd Mon Sep 17 00:00:00 2001 From: timmy Date: Mon, 10 Aug 2026 23:26:35 +0000 Subject: [PATCH] perf: isolate pull check refreshes (Closes #521) --- frontend/dashboard.js | 29 ++++++++++++++++-- frontend/pull-sheet.js | 8 +++++ frontend/review-sheet.js | 11 ++++++- src/gitea_proxy.py | 24 +++++++++++++++ src/main.py | 62 +++++++++++++++++++++++++++++++++++++++ tests/test_my_work.py | 41 ++++++++++++++++++++++---- tests/test_pull_api.py | 63 ++++++++++++++++++++++++++++++++++++++++ tests/test_review_api.py | 27 +++++++++++++++++ 8 files changed, 255 insertions(+), 10 deletions(-) diff --git a/frontend/dashboard.js b/frontend/dashboard.js index 09ae2e5..5ee1886 100644 --- a/frontend/dashboard.js +++ b/frontend/dashboard.js @@ -2333,6 +2333,22 @@ } } + function renderPullCheckStatus(status) { + qs('#pull-ci-state').textContent = 'CI ' + (status.ci_state || 'unknown'); + renderCheckSection('pull', status); + if (status.head_sha !== selectedPullDetail?.head_sha) { + qs('#pull-review-status').textContent = 'New commits detected. Reload review data before merging.'; + qs('#pull-merge-state').textContent = 'Review data is stale'; + qs('#merge-pull').disabled = true; + return; + } + selectedPullDetail = { ...selectedPullDetail, ...status }; + const eligibility = createPullSheet.mergeEligibility(selectedPullDetail, pullReviewState); + qs('#pull-merge-state').textContent = eligibility.reason; + qs('#merge-pull').disabled = !eligibility.allowed; + qs('#pull-review-status').textContent = 'Checks refreshed for the current head.'; + } + function focusNextUnreviewedPullFile() { if (!selectedPullDetail || !pullReviewState) return; const filename = pullController.nextUnreviewed(selectedPullDetail, pullReviewState); @@ -4280,11 +4296,18 @@ }); qs('#pull-review-retry').addEventListener('click', loadPullReview); qs('#refresh-pull-checks').addEventListener('click', async () => { + if (!selectedPull || !selectedPullDetail?.head_sha) return; + const item = selectedPull; const button = qs('#refresh-pull-checks'); button.disabled = true; qs('#pull-checks-summary').textContent = 'Refreshing…'; - try { await loadPullReview({ refresh: true }); } - finally { button.disabled = false; } + try { + const status = await pullController.loadChecks(item); + if (selectedPull !== item) return; + renderPullCheckStatus(status); + } catch (_error) { + if (selectedPull === item) qs('#pull-checks-summary').textContent = 'Refresh failed · retry'; + } finally { button.disabled = false; } }); qs('#next-unreviewed-pull-file').addEventListener('click', focusNextUnreviewedPullFile); qs('#load-older-pull-comments').addEventListener('click', async () => { @@ -4490,7 +4513,7 @@ button.disabled = true; qs('#review-checks-summary').textContent = 'Refreshing…'; try { - const detail = await reviewController.load(item); + const detail = await reviewController.loadChecks(item); if (selectedReview !== item) return; if (selectedReviewHead && detail.head_sha !== selectedReviewHead) { qs('#review-sheet-status').textContent = 'New commits detected. Reload the review before submitting feedback.'; diff --git a/frontend/pull-sheet.js b/frontend/pull-sheet.js index a17a75d..fc0394c 100644 --- a/frontend/pull-sheet.js +++ b/frontend/pull-sheet.js @@ -44,6 +44,7 @@ function renderFile(file, index, reviewed, escapeHtml) { function createPullSheet({ fetchJson, storage, createConversationPager = globalThis.createConversationPager, createOperationId = () => globalThis.crypto?.randomUUID?.() || String(Date.now()) + '-' + Math.random() }) { let commentRequest = null; let mergeRequest = null; + let checkRequest = null; const reviewRequests = new Map(); const reviewCache = new Map(); const pathFor = item => 'api/v1/repos/' + String(item.repository || '').split('/') @@ -82,6 +83,13 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT reviewRequests.set(key, request); return request; }, + loadChecks(item) { + if (checkRequest) return checkRequest; + checkRequest = fetchJson(pathFor(item) + '/checks', { + headers: { Accept: 'application/json' }, + }).finally(() => { checkRequest = null; }); + return checkRequest; + }, conversation(item, initialPage) { const pager = createConversationPager({ loadPage: page => fetchJson(pathFor(item) + '/comments?page=' + encodeURIComponent(page) + '&limit=20', { diff --git a/frontend/review-sheet.js b/frontend/review-sheet.js index 841bff5..63401a1 100644 --- a/frontend/review-sheet.js +++ b/frontend/review-sheet.js @@ -4,6 +4,7 @@ function createReviewController({ createOperationId = () => globalThis.crypto?.randomUUID?.() || String(Date.now()) + '-' + Math.random(), }) { let pendingSubmission = null; + let pendingChecks = null; function endpoint(item) { const [owner, repo] = String(item.repository || '').split('/'); @@ -18,6 +19,14 @@ function createReviewController({ return fetchJson(endpoint(item), { headers: { Accept: 'application/json' } }); } + function loadChecks(item) { + if (pendingChecks) return pendingChecks; + pendingChecks = fetchJson(endpoint(item) + '/checks', { + headers: { Accept: 'application/json' }, + }).finally(() => { pendingChecks = null; }); + return pendingChecks; + } + function submit(item, payload) { if (pendingSubmission) return pendingSubmission; const operationKey = 'stackchain.review-submit.v1:' + item.repository + '#' + item.number; @@ -44,7 +53,7 @@ function createReviewController({ return pendingSubmission; } - return { load, submit }; + return { load, loadChecks, submit }; } function createWrapPreference({ storage, mobile }) { diff --git a/src/gitea_proxy.py b/src/gitea_proxy.py index 359b6ee..5985236 100644 --- a/src/gitea_proxy.py +++ b/src/gitea_proxy.py @@ -1809,6 +1809,30 @@ async def pull_completion_review(repository: str, number: int) -> dict: } +async def pull_check_status(repository: str, number: int) -> dict: + """Load only mutable pull and CI state, without immutable review data.""" + base = f"repos/{repository}/pulls/{number}" + pull = await fetch(base) + if not isinstance(pull, dict): + raise ValueError("Gitea pull request response was not an object") + head_value = pull.get("head") + head: dict = head_value if isinstance(head_value, dict) else {} + sha_value = head.get("sha") + sha = sha_value if isinstance(sha_value, str) else "" + status = await fetch(f"repos/{repository}/commits/{sha}/status") + return { + "repository": repository, + "number": number, + "head_sha": sha, + "state": pull.get("state") if isinstance(pull.get("state"), str) else "", + "draft": pull.get("draft") is True, + "mergeable": pull.get("mergeable") is True, + "merged": pull.get("merged") is True, + "ci_state": status.get("state", "unknown") if isinstance(status, dict) else "unknown", + "checks": _normalize_commit_checks(status), + } + + async def is_pull_merged_at_head( repository: str, number: int, expected_head_sha: str ) -> bool: diff --git a/src/main.py b/src/main.py index ee8fc1b..e63c0e2 100644 --- a/src/main.py +++ b/src/main.py @@ -3718,6 +3718,37 @@ async def review_detail(owner: str, repo: str, number: int): ) +@app.get("/api/v1/repos/{owner}/{repo}/pulls/{number}/review/checks") +async def requested_review_checks( + owner: str, repo: str, number: int = PathParam(gt=0) +): + repository = f"{owner}/{repo}" + + async def load_checks(): + if not await is_requested_review(repository, number): + raise HTTPException(status_code=404, detail="Review request not found") + return await gitea_proxy.pull_check_status(repository, number) + + try: + return await asyncio.wait_for( + load_checks(), timeout=REVIEW_DETAIL_TIMEOUT_SECONDS + ) + except HTTPException: + raise + except TimeoutError: + return JSONResponse( + {"error": "Refreshing review checks timed out. Please retry."}, + status_code=503, + headers={"Retry-After": "1"}, + ) + except Exception: + return JSONResponse( + {"error": "Review checks are temporarily unavailable. Please retry."}, + status_code=503, + headers={"Retry-After": "1"}, + ) + + @app.get("/api/v1/repos/{owner}/{repo}/pulls/{number}/detail") async def assigned_pull_detail(owner: str, repo: str, number: int = PathParam(gt=0)): repository = f"{owner}/{repo}" @@ -3778,6 +3809,37 @@ async def assigned_pull_review_data( ) +@app.get("/api/v1/repos/{owner}/{repo}/pulls/{number}/checks") +async def assigned_pull_checks( + owner: str, repo: str, number: int = PathParam(gt=0) +): + repository = f"{owner}/{repo}" + + async def load_checks(): + if not await gitea_proxy.is_assigned_pull(repository, number): + raise HTTPException(status_code=404, detail="Assigned pull request not found") + return await gitea_proxy.pull_check_status(repository, number) + + try: + return await asyncio.wait_for( + load_checks(), timeout=REVIEW_DETAIL_TIMEOUT_SECONDS + ) + except HTTPException: + raise + except TimeoutError: + return JSONResponse( + {"error": "Refreshing pull request checks timed out. Please retry."}, + status_code=503, + headers={"Retry-After": "1"}, + ) + except Exception: + return JSONResponse( + {"error": "Pull request checks are temporarily unavailable. Please retry."}, + status_code=503, + headers={"Retry-After": "1"}, + ) + + @app.get("/api/v1/repos/{owner}/{repo}/pulls/{number}/comments") async def assigned_pull_conversation( owner: str, diff --git a/tests/test_my_work.py b/tests/test_my_work.py index 0160f0b..72f01d5 100644 --- a/tests/test_my_work.py +++ b/tests/test_my_work.py @@ -4055,7 +4055,7 @@ Promise.allSettled([first, concurrent]).then(async failed => {{ ] -def test_pull_sheet_refreshes_checks_without_clearing_head_scoped_progress(): +def test_pull_sheet_refreshes_status_only_without_clearing_review_progress(): script = f""" const createPullSheet = require({json.dumps(str(PULL_SHEET))}); const values = new Map(); @@ -4070,15 +4070,19 @@ const sheet = createPullSheet({{storage, fetchJson: url => {{ const item = {{repository:'stackchain/api', number:7}}; sheet.loadReview(item, 'abc123').then(first => {{ sheet.toggleReviewed(item, first, 'src/api.py'); - return sheet.loadReview(item, 'abc123', {{refresh:true}}).then(refreshed => {{ - process.stdout.write(JSON.stringify({{calls, refreshed, progress:sheet.reviewState(item, refreshed)}})); + return sheet.loadChecks(item).then(refreshed => {{ + const combined = {{...first, ...refreshed}}; + process.stdout.write(JSON.stringify({{calls, refreshed, progress:sheet.reviewState(item, combined)}})); }}); }}); """ result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True) output = json.loads(result.stdout) - assert len(output["calls"]) == 2 + assert output["calls"] == [ + "api/v1/repos/stackchain/api/pulls/7/review-data", + "api/v1/repos/stackchain/api/pulls/7/checks", + ] assert output["refreshed"]["ci_state"] == "success" assert output["progress"] == {"reviewed": ["src/api.py"], "total": 1, "complete": True} @@ -4483,6 +4487,28 @@ controller.load({{ repository: 'stackchain/api', number: 7 }}).then(detail => } +def test_review_controller_refreshes_checks_through_status_only_path(): + script = f""" +const createReviewController = require({json.dumps(str(REVIEW_SHEET))}); +const calls = []; +const controller = createReviewController({{ fetchJson: async (url, options) => {{ + calls.push({{url, accept:options.headers.Accept}}); + return {{head_sha:'abc123', ci_state:'success', checks:[]}}; +}} }}); +controller.loadChecks({{repository:'stackchain/api', number:7}}).then(status => + process.stdout.write(JSON.stringify({{calls, status}})) +); +""" + result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True) + output = json.loads(result.stdout) + + assert output["calls"] == [{ + "url": "api/v1/repos/stackchain/api/pulls/7/review/checks", + "accept": "application/json", + }] + assert output["status"]["ci_state"] == "success" + + def test_review_diff_rows_escape_content_and_toggle_accessibly(): script = f""" const reviewSheet = require({json.dumps(str(REVIEW_SHEET))}); @@ -4553,7 +4579,7 @@ process.stdout.write(JSON.stringify({{result, offline}})); @pytest.mark.anyio -async def test_mobile_pull_review_sheets_expose_actionable_refreshable_checks(): +async def test_mobile_pull_review_sheets_refresh_checks_without_reloading_diffs(): html = await dashboard() assert html.index('id="pull-checks"') < html.index('id="pull-files"') @@ -4561,7 +4587,10 @@ async def test_mobile_pull_review_sheets_expose_actionable_refreshable_checks(): assert 'id="refresh-pull-checks"' in html assert 'id="refresh-review-checks"' in html assert "createReviewController.renderChecks" in html - assert "{ refresh: true }" in html + assert "pullController.loadChecks(item)" in html + assert "reviewController.loadChecks(item)" in html + assert "New commits detected. Reload review data before merging." in html + assert "New commits detected. Reload the review before submitting feedback." in html assert "qs('#refresh-review-checks').disabled = offlineReview" in html assert ".ci-check-link" in html and "min-height:44px" in html assert ".ci-check-copy" in html and "overflow-wrap:anywhere" in html diff --git a/tests/test_pull_api.py b/tests/test_pull_api.py index ffa233e..d7a8bc0 100644 --- a/tests/test_pull_api.py +++ b/tests/test_pull_api.py @@ -125,6 +125,69 @@ async def test_assigned_pull_review_endpoint_loads_review_payload_on_demand(monk ] +@pytest.mark.anyio +async def test_gitea_pull_check_status_skips_files_reviews_and_diff(): + requests = [] + + async def handler(request): + requests.append((request.method, request.url.path)) + if request.url.path.endswith("/pulls/7"): + return httpx.Response(200, json={ + "state": "open", "draft": False, "mergeable": True, "merged": False, + "head": {"sha": "abc123"}, + }) + if request.url.path.endswith("/commits/abc123/status"): + return httpx.Response(200, json={ + "state": "success", + "statuses": [{"context": "tests", "status": "success", "description": "Passed"}], + }) + raise AssertionError(f"unexpected request: {request.method} {request.url.path}") + + gitea_proxy.start_client(transport=httpx.MockTransport(handler)) + try: + status = await gitea_proxy.pull_check_status("stackchain/api", 7) + finally: + await gitea_proxy.stop_client() + + assert requests == [ + ("GET", "/api/v1/repos/stackchain/api/pulls/7"), + ("GET", "/api/v1/repos/stackchain/api/commits/abc123/status"), + ] + assert status == { + "repository": "stackchain/api", "number": 7, "head_sha": "abc123", + "state": "open", "draft": False, "mergeable": True, "merged": False, + "ci_state": "success", + "checks": [{"name": "tests", "state": "success", "description": "Passed", "url": ""}], + } + + +@pytest.mark.anyio +async def test_assigned_pull_checks_endpoint_authorizes_and_returns_status_only(monkeypatch): + calls = [] + + async def assigned(repository, number): + calls.append(("assigned", repository, number)) + return True + + async def checks(repository, number): + calls.append(("checks", repository, number)) + return {"head_sha": "abc123", "ci_state": "pending", "checks": []} + + monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned) + monkeypatch.setattr(main.gitea_proxy, "pull_check_status", checks) + transport = httpx.ASGITransport(app=main.app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + response = await client.get("/api/v1/repos/stackchain/api/pulls/7/checks") + + assert response.status_code == 200 + assert response.headers["cache-control"] == "no-store" + assert response.json() == {"head_sha": "abc123", "ci_state": "pending", "checks": []} + assert calls == [ + ("assigned", "stackchain/api", 7), + ("checks", "stackchain/api", 7), + ] + + @pytest.mark.anyio async def test_gitea_assigned_pull_review_includes_bounded_diff_previews(): async def handler(request): diff --git a/tests/test_review_api.py b/tests/test_review_api.py index 67d0171..cccfc78 100644 --- a/tests/test_review_api.py +++ b/tests/test_review_api.py @@ -36,6 +36,33 @@ async def test_review_detail_endpoint_returns_normalized_no_store_payload(monkey assert response.json()["files"][0]["filename"] == "src/api.py" +@pytest.mark.anyio +async def test_requested_review_checks_endpoint_authorizes_and_returns_status_only(monkeypatch): + calls = [] + + async def requested(repository, number): + calls.append(("requested", repository, number)) + return True + + async def checks(repository, number): + calls.append(("checks", repository, number)) + return {"head_sha": "def456", "ci_state": "success", "checks": []} + + monkeypatch.setattr(main, "is_requested_review", requested) + monkeypatch.setattr(main.gitea_proxy, "pull_check_status", checks) + transport = httpx.ASGITransport(app=main.app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + response = await client.get("/api/v1/repos/stackchain/api/pulls/7/review/checks") + + assert response.status_code == 200 + assert response.headers["cache-control"] == "no-store" + assert response.json() == {"head_sha": "def456", "ci_state": "success", "checks": []} + assert calls == [ + ("requested", "stackchain/api", 7), + ("checks", "stackchain/api", 7), + ] + + @pytest.mark.anyio async def test_gitea_review_detail_returns_bounded_actionable_checks(monkeypatch): monkeypatch.setattr(gitea_proxy, "GITEA_URL", "https://forge.example/git") -- 2.43.0