From 173af9f698c39ff7ad6d6f2cadb720f3c7246392 Mon Sep 17 00:00:00 2001 From: timmy Date: Thu, 6 Aug 2026 18:25:10 +0000 Subject: [PATCH] feat: make mobile review loading retryable (#127) --- frontend/index.html | 11 +++++- src/gitea_proxy.py | 21 +++++++----- src/main.py | 24 +++++++++---- tests/test_gitea_work_search.py | 61 +++++++++++++++++++++++++++++---- tests/test_my_work.py | 12 +++++++ tests/test_review_api.py | 27 +++++++++++++++ 6 files changed, 133 insertions(+), 23 deletions(-) diff --git a/frontend/index.html b/frontend/index.html index 1f64ff4..03ad5ae 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -86,6 +86,7 @@ textarea { resize: vertical; min-height: 120px; } .review-diff-line.removed { color:#fca5a5; background:rgba(239,68,68,.09); } .review-diff-note, .review-diff-empty { display:block; padding:8px; color:#fcd34d; white-space:normal; } .review-action { min-height:44px; } +.review-retry { min-height:44px; margin-top:10px; } @media (max-width: 600px) { header { align-items:flex-start; } .my-work { margin:0; } @@ -222,6 +223,7 @@ textarea { resize: vertical; min-height: 120px; }
Choose a review request.
+

CI unknownOpen in Gitea

Changed files

@@ -415,6 +417,7 @@ textarea { resize: vertical; min-height: 120px; } qs('#review-sheet-key').textContent = item.key; qs('#review-sheet-title').textContent = item.title; qs('#review-sheet-status').textContent = 'Loading review details…'; + qs('#retry-review-load').hidden = true; qs('#review-sheet-body').textContent = ''; qs('#review-files').textContent = ''; qs('#review-progress').textContent = '0 of 0 files reviewed'; @@ -459,7 +462,10 @@ textarea { resize: vertical; min-height: 120px; } ).join('') : '
No prior reviews.
'; qs('#review-sheet-status').textContent = 'Ready to review · by ' + (detail.author || 'unknown author'); } catch (error) { - qs('#review-sheet-status').textContent = error.message + ' Use Open in Gitea or close and retry.'; + if (selectedReview !== item) return; + qs('#review-sheet-status').textContent = error.message + ' Retry here or use Open in Gitea.'; + qs('#retry-review-load').hidden = false; + qs('#retry-review-load').focus(); } } @@ -577,6 +583,9 @@ textarea { resize: vertical; min-height: 120px; } document.addEventListener('keydown', (e) => { if ((e.metaKey||e.ctrlKey) && e.key==='k') { e.preventDefault(); qs('#cmd-palette').classList.toggle('open'); if(qs('#cmd-palette').classList.contains('open')){ qs('#cmd-input').focus(); renderCommands(''); } } }); qs('#close-whiteboard').addEventListener('click', () => closeModal('whiteboard-modal')); qs('#close-review-sheet').addEventListener('click', closeReviewSheet); + qs('#retry-review-load').addEventListener('click', () => { + if (selectedReview) openReviewSheet(selectedReview, reviewTrigger); + }); qs('#next-unreviewed-review').addEventListener('click', () => { if (progress) openNextUnreviewed(progress.snapshot()); }); diff --git a/src/gitea_proxy.py b/src/gitea_proxy.py index 575b7e0..805c78b 100644 --- a/src/gitea_proxy.py +++ b/src/gitea_proxy.py @@ -1,3 +1,4 @@ +import asyncio import os import shlex from typing import Any @@ -119,14 +120,15 @@ async def pull_requests() -> list[dict]: async def is_requested_review(repository: str, number: int) -> bool: - pulls = await pull_requests() + pulls = await fetch( + "repos/issues/search?state=open&review_requested=true&type=pulls&limit=50" + ) return any( isinstance(pull, dict) and pull.get("number") == number - and "review_requested" in (pull.get("work_reasons") or []) and isinstance(pull.get("repository"), dict) and pull["repository"].get("full_name") == repository - for pull in pulls + for pull in (pulls or []) ) @@ -135,16 +137,19 @@ async def pull_review_detail(repository: str, number: int) -> dict: pull = await fetch(base) if not isinstance(pull, dict): raise ValueError("Gitea pull request response was not an object") - files = await fetch(f"{base}/files") 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") - reviews = await fetch(f"{base}/reviews") - diff, diff_truncated = await fetch_text( - f"repos/{repository}/pulls/{number}.diff", REVIEW_DIFF_MAX_BYTES + files, status, reviews, diff_result = await asyncio.gather( + fetch(f"{base}/files"), + fetch(f"repos/{repository}/commits/{sha}/status"), + fetch(f"{base}/reviews"), + fetch_text( + f"repos/{repository}/pulls/{number}.diff", REVIEW_DIFF_MAX_BYTES + ), ) + diff, diff_truncated = diff_result previews = _diff_previews(diff, diff_truncated) user_value = pull.get("user") user: dict = user_value if isinstance(user_value, dict) else {} diff --git a/src/main.py b/src/main.py index ad3a97d..ea6150f 100644 --- a/src/main.py +++ b/src/main.py @@ -24,6 +24,7 @@ app = FastAPI(title="Stackchain Dashboard") CONTEXT_TIMEOUT_SECONDS = 5.0 EVENT_STREAM_TIMEOUT_SECONDS = 5.0 READINESS_TIMEOUT_SECONDS = 5.0 +REVIEW_DETAIL_TIMEOUT_SECONDS = 5.0 FRONTEND_DIR = Path(__file__).resolve().parent.parent / "frontend" @@ -262,19 +263,28 @@ async def event_stream(): @app.get("/api/v1/repos/{owner}/{repo}/pulls/{number}/review") async def review_detail(owner: str, repo: str, number: int): - try: + async def load_requested_review(): repository = f"{owner}/{repo}" - if not await asyncio.wait_for( - is_requested_review(repository, number), - timeout=CONTEXT_TIMEOUT_SECONDS, - ): + if not await is_requested_review(repository, number): raise HTTPException(status_code=404, detail="Review request not found") + return await pull_review_detail(repository, number) + + try: return await asyncio.wait_for( - pull_review_detail(repository, number), - timeout=CONTEXT_TIMEOUT_SECONDS, + load_requested_review(), timeout=REVIEW_DETAIL_TIMEOUT_SECONDS ) except HTTPException: raise + except TimeoutError: + return JSONResponse( + {"error": "Pull request review details timed out. Please retry."}, + status_code=503, + headers={ + "Retry-After": str( + max(1, math.ceil(REVIEW_DETAIL_TIMEOUT_SECONDS)) + ) + }, + ) except Exception: return JSONResponse( {"error": "Pull request review details are temporarily unavailable"}, diff --git a/tests/test_gitea_work_search.py b/tests/test_gitea_work_search.py index f700dd2..979ec24 100644 --- a/tests/test_gitea_work_search.py +++ b/tests/test_gitea_work_search.py @@ -1,6 +1,8 @@ -import pytest +import asyncio import json +import pytest + from src import gitea_proxy from src import main @@ -54,26 +56,32 @@ async def test_pull_requests_merge_assignment_and_review_responsibilities(monkey @pytest.mark.anyio -async def test_requested_review_guard_matches_repository_number_and_reason(monkeypatch): - async def pulls(): +async def test_requested_review_guard_uses_only_dedicated_review_search(monkeypatch): + requested_paths = [] + + async def fake_fetch(path): + requested_paths.append(path) return [ { "number": 7, "repository": {"full_name": "stackchain/api"}, - "work_reasons": ["review_requested"], }, { "number": 8, "repository": {"full_name": "stackchain/api"}, - "work_reasons": ["assigned_to_me"], }, ] - monkeypatch.setattr(gitea_proxy, "pull_requests", pulls) + monkeypatch.setattr(gitea_proxy, "fetch", fake_fetch) assert await gitea_proxy.is_requested_review("stackchain/api", 7) is True - assert await gitea_proxy.is_requested_review("stackchain/api", 8) is False + assert await gitea_proxy.is_requested_review("stackchain/api", 9) is False assert await gitea_proxy.is_requested_review("stackchain/private", 7) is False + assert requested_paths == [ + "repos/issues/search?state=open&review_requested=true&type=pulls&limit=50", + "repos/issues/search?state=open&review_requested=true&type=pulls&limit=50", + "repos/issues/search?state=open&review_requested=true&type=pulls&limit=50", + ] @pytest.mark.anyio @@ -183,6 +191,45 @@ async def test_pull_review_detail_combines_pr_files_status_and_reviews(monkeypat assert len(detail["reviews"]) == 1 +@pytest.mark.anyio +async def test_pull_review_detail_fetches_independent_head_resources_concurrently(monkeypatch): + started = {name: asyncio.Event() for name in ("files", "status", "reviews", "diff")} + release = asyncio.Event() + + async def wait_for_release(name, result): + started[name].set() + await release.wait() + return result + + async def fake_fetch(path): + if path.endswith("/pulls/7"): + return {"head": {"sha": "abc123"}, "user": {"login": "alex"}} + if path.endswith("/files"): + return await wait_for_release("files", []) + if path.endswith("/reviews"): + return await wait_for_release("reviews", []) + return await wait_for_release("status", {"state": "success"}) + + async def fake_fetch_text(path, max_bytes): + return await wait_for_release("diff", ("", False)) + + monkeypatch.setattr(gitea_proxy, "fetch", fake_fetch) + monkeypatch.setattr(gitea_proxy, "fetch_text", fake_fetch_text) + + detail_task = asyncio.create_task( + gitea_proxy.pull_review_detail("stackchain/api", 7) + ) + try: + await asyncio.wait_for( + asyncio.gather(*(event.wait() for event in started.values())), timeout=1 + ) + finally: + release.set() + + detail = await detail_task + assert detail["head_sha"] == "abc123" + + @pytest.mark.anyio async def test_pull_review_detail_attaches_bounded_per_file_diff_previews(monkeypatch): async def fake_fetch(path): diff --git a/tests/test_my_work.py b/tests/test_my_work.py index 7833597..4415ee1 100644 --- a/tests/test_my_work.py +++ b/tests/test_my_work.py @@ -293,3 +293,15 @@ async def test_review_sheet_loads_details_and_preserves_safe_gitea_handoff(): assert "review-history" in html assert "open-review-gitea" in html assert "reviewController.submit" not in html + + +@pytest.mark.anyio +async def test_mobile_review_failure_offers_an_in_place_retry_for_the_same_item(): + html = await dashboard() + + assert 'id="retry-review-load"' in html + assert '.review-retry' in html and 'min-height:44px' in html + assert "qs('#retry-review-load').hidden = false" in html + assert "qs('#retry-review-load').hidden = true" in html + assert "openReviewSheet(selectedReview, reviewTrigger)" in html + assert "qs('#retry-review-load').focus()" in html diff --git a/tests/test_review_api.py b/tests/test_review_api.py index fda4ed8..8fc14d3 100644 --- a/tests/test_review_api.py +++ b/tests/test_review_api.py @@ -1,3 +1,5 @@ +import asyncio + import httpx import pytest @@ -48,6 +50,31 @@ async def test_review_detail_rejects_pulls_not_requested_from_service_user(monke assert response.headers["cache-control"] == "no-store" +@pytest.mark.anyio +async def test_review_detail_has_one_retryable_deadline_and_cancels_pending_work(monkeypatch): + cancelled = asyncio.Event() + + async def requested(repository, number): + try: + await asyncio.Event().wait() + finally: + cancelled.set() + + monkeypatch.setattr(main, "is_requested_review", requested) + monkeypatch.setattr(main, "REVIEW_DETAIL_TIMEOUT_SECONDS", 0.01) + 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") + + assert response.status_code == 503 + assert response.headers["cache-control"] == "no-store" + assert response.headers["retry-after"] == "1" + assert response.json() == { + "error": "Pull request review details timed out. Please retry." + } + assert cancelled.is_set() + + @pytest.mark.anyio async def test_public_review_endpoint_does_not_expose_service_token_mutations(): transport = httpx.ASGITransport(app=main.app)