perf: isolate pull check refreshes (Closes #521)

2026-08-10 23:26:35 +00:00 · 2026-08-10 23:26:35 +00:00 · e5a352b418
commit e5a352b418
parent 8907263760
8 changed files with 255 additions and 10 deletions
--- 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.';
--- 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', {
--- 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 }) {
--- 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:
--- 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,
--- 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
--- 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):
--- 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")