Make mobile pull review loading bounded, concurrent, and retryable #128
|
|
@ -86,6 +86,7 @@ textarea { resize: vertical; min-height: 120px; }
|
||||||
.review-diff-line.removed { color:#fca5a5; background:rgba(239,68,68,.09); }
|
.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-diff-note, .review-diff-empty { display:block; padding:8px; color:#fcd34d; white-space:normal; }
|
||||||
.review-action { min-height:44px; }
|
.review-action { min-height:44px; }
|
||||||
|
.review-retry { min-height:44px; margin-top:10px; }
|
||||||
@media (max-width: 600px) {
|
@media (max-width: 600px) {
|
||||||
header { align-items:flex-start; }
|
header { align-items:flex-start; }
|
||||||
.my-work { margin:0; }
|
.my-work { margin:0; }
|
||||||
|
|
@ -222,6 +223,7 @@ textarea { resize: vertical; min-height: 120px; }
|
||||||
<button class="review-action" id="close-review-sheet">Close</button>
|
<button class="review-action" id="close-review-sheet">Close</button>
|
||||||
</div>
|
</div>
|
||||||
<div id="review-sheet-status" class="small" aria-live="polite">Choose a review request.</div>
|
<div id="review-sheet-status" class="small" aria-live="polite">Choose a review request.</div>
|
||||||
|
<button class="review-retry" id="retry-review-load" hidden>Retry loading review</button>
|
||||||
<p class="review-sheet-body" id="review-sheet-body"></p>
|
<p class="review-sheet-body" id="review-sheet-body"></p>
|
||||||
<div class="row"><span class="pill" id="review-ci-state">CI unknown</span><a id="open-review-gitea" href="#" target="_blank" rel="noopener noreferrer">Open in Gitea</a></div>
|
<div class="row"><span class="pill" id="review-ci-state">CI unknown</span><a id="open-review-gitea" href="#" target="_blank" rel="noopener noreferrer">Open in Gitea</a></div>
|
||||||
<h2>Changed files</h2>
|
<h2>Changed files</h2>
|
||||||
|
|
@ -415,6 +417,7 @@ textarea { resize: vertical; min-height: 120px; }
|
||||||
qs('#review-sheet-key').textContent = item.key;
|
qs('#review-sheet-key').textContent = item.key;
|
||||||
qs('#review-sheet-title').textContent = item.title;
|
qs('#review-sheet-title').textContent = item.title;
|
||||||
qs('#review-sheet-status').textContent = 'Loading review details…';
|
qs('#review-sheet-status').textContent = 'Loading review details…';
|
||||||
|
qs('#retry-review-load').hidden = true;
|
||||||
qs('#review-sheet-body').textContent = '';
|
qs('#review-sheet-body').textContent = '';
|
||||||
qs('#review-files').textContent = '';
|
qs('#review-files').textContent = '';
|
||||||
qs('#review-progress').textContent = '0 of 0 files reviewed';
|
qs('#review-progress').textContent = '0 of 0 files reviewed';
|
||||||
|
|
@ -459,7 +462,10 @@ textarea { resize: vertical; min-height: 120px; }
|
||||||
).join('') : '<div>No prior reviews.</div>';
|
).join('') : '<div>No prior reviews.</div>';
|
||||||
qs('#review-sheet-status').textContent = 'Ready to review · by ' + (detail.author || 'unknown author');
|
qs('#review-sheet-status').textContent = 'Ready to review · by ' + (detail.author || 'unknown author');
|
||||||
} catch (error) {
|
} 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(''); } } });
|
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-whiteboard').addEventListener('click', () => closeModal('whiteboard-modal'));
|
||||||
qs('#close-review-sheet').addEventListener('click', closeReviewSheet);
|
qs('#close-review-sheet').addEventListener('click', closeReviewSheet);
|
||||||
|
qs('#retry-review-load').addEventListener('click', () => {
|
||||||
|
if (selectedReview) openReviewSheet(selectedReview, reviewTrigger);
|
||||||
|
});
|
||||||
qs('#next-unreviewed-review').addEventListener('click', () => {
|
qs('#next-unreviewed-review').addEventListener('click', () => {
|
||||||
if (progress) openNextUnreviewed(progress.snapshot());
|
if (progress) openNextUnreviewed(progress.snapshot());
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -1,3 +1,4 @@
|
||||||
|
import asyncio
|
||||||
import os
|
import os
|
||||||
import shlex
|
import shlex
|
||||||
from typing import Any
|
from typing import Any
|
||||||
|
|
@ -119,14 +120,15 @@ async def pull_requests() -> list[dict]:
|
||||||
|
|
||||||
|
|
||||||
async def is_requested_review(repository: str, number: int) -> bool:
|
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(
|
return any(
|
||||||
isinstance(pull, dict)
|
isinstance(pull, dict)
|
||||||
and pull.get("number") == number
|
and pull.get("number") == number
|
||||||
and "review_requested" in (pull.get("work_reasons") or [])
|
|
||||||
and isinstance(pull.get("repository"), dict)
|
and isinstance(pull.get("repository"), dict)
|
||||||
and pull["repository"].get("full_name") == repository
|
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)
|
pull = await fetch(base)
|
||||||
if not isinstance(pull, dict):
|
if not isinstance(pull, dict):
|
||||||
raise ValueError("Gitea pull request response was not an object")
|
raise ValueError("Gitea pull request response was not an object")
|
||||||
files = await fetch(f"{base}/files")
|
|
||||||
head_value = pull.get("head")
|
head_value = pull.get("head")
|
||||||
head: dict = head_value if isinstance(head_value, dict) else {}
|
head: dict = head_value if isinstance(head_value, dict) else {}
|
||||||
sha_value = head.get("sha")
|
sha_value = head.get("sha")
|
||||||
sha = sha_value if isinstance(sha_value, str) else ""
|
sha = sha_value if isinstance(sha_value, str) else ""
|
||||||
status = await fetch(f"repos/{repository}/commits/{sha}/status")
|
files, status, reviews, diff_result = await asyncio.gather(
|
||||||
reviews = await fetch(f"{base}/reviews")
|
fetch(f"{base}/files"),
|
||||||
diff, diff_truncated = await fetch_text(
|
fetch(f"repos/{repository}/commits/{sha}/status"),
|
||||||
f"repos/{repository}/pulls/{number}.diff", REVIEW_DIFF_MAX_BYTES
|
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)
|
previews = _diff_previews(diff, diff_truncated)
|
||||||
user_value = pull.get("user")
|
user_value = pull.get("user")
|
||||||
user: dict = user_value if isinstance(user_value, dict) else {}
|
user: dict = user_value if isinstance(user_value, dict) else {}
|
||||||
|
|
|
||||||
24
src/main.py
24
src/main.py
|
|
@ -24,6 +24,7 @@ app = FastAPI(title="Stackchain Dashboard")
|
||||||
CONTEXT_TIMEOUT_SECONDS = 5.0
|
CONTEXT_TIMEOUT_SECONDS = 5.0
|
||||||
EVENT_STREAM_TIMEOUT_SECONDS = 5.0
|
EVENT_STREAM_TIMEOUT_SECONDS = 5.0
|
||||||
READINESS_TIMEOUT_SECONDS = 5.0
|
READINESS_TIMEOUT_SECONDS = 5.0
|
||||||
|
REVIEW_DETAIL_TIMEOUT_SECONDS = 5.0
|
||||||
FRONTEND_DIR = Path(__file__).resolve().parent.parent / "frontend"
|
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")
|
@app.get("/api/v1/repos/{owner}/{repo}/pulls/{number}/review")
|
||||||
async def review_detail(owner: str, repo: str, number: int):
|
async def review_detail(owner: str, repo: str, number: int):
|
||||||
try:
|
async def load_requested_review():
|
||||||
repository = f"{owner}/{repo}"
|
repository = f"{owner}/{repo}"
|
||||||
if not await asyncio.wait_for(
|
if not await is_requested_review(repository, number):
|
||||||
is_requested_review(repository, number),
|
|
||||||
timeout=CONTEXT_TIMEOUT_SECONDS,
|
|
||||||
):
|
|
||||||
raise HTTPException(status_code=404, detail="Review request not found")
|
raise HTTPException(status_code=404, detail="Review request not found")
|
||||||
|
return await pull_review_detail(repository, number)
|
||||||
|
|
||||||
|
try:
|
||||||
return await asyncio.wait_for(
|
return await asyncio.wait_for(
|
||||||
pull_review_detail(repository, number),
|
load_requested_review(), timeout=REVIEW_DETAIL_TIMEOUT_SECONDS
|
||||||
timeout=CONTEXT_TIMEOUT_SECONDS,
|
|
||||||
)
|
)
|
||||||
except HTTPException:
|
except HTTPException:
|
||||||
raise
|
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:
|
except Exception:
|
||||||
return JSONResponse(
|
return JSONResponse(
|
||||||
{"error": "Pull request review details are temporarily unavailable"},
|
{"error": "Pull request review details are temporarily unavailable"},
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,8 @@
|
||||||
import pytest
|
import asyncio
|
||||||
import json
|
import json
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
from src import gitea_proxy
|
from src import gitea_proxy
|
||||||
from src import main
|
from src import main
|
||||||
|
|
||||||
|
|
@ -54,26 +56,32 @@ async def test_pull_requests_merge_assignment_and_review_responsibilities(monkey
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.anyio
|
@pytest.mark.anyio
|
||||||
async def test_requested_review_guard_matches_repository_number_and_reason(monkeypatch):
|
async def test_requested_review_guard_uses_only_dedicated_review_search(monkeypatch):
|
||||||
async def pulls():
|
requested_paths = []
|
||||||
|
|
||||||
|
async def fake_fetch(path):
|
||||||
|
requested_paths.append(path)
|
||||||
return [
|
return [
|
||||||
{
|
{
|
||||||
"number": 7,
|
"number": 7,
|
||||||
"repository": {"full_name": "stackchain/api"},
|
"repository": {"full_name": "stackchain/api"},
|
||||||
"work_reasons": ["review_requested"],
|
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
"number": 8,
|
"number": 8,
|
||||||
"repository": {"full_name": "stackchain/api"},
|
"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", 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 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
|
@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
|
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
|
@pytest.mark.anyio
|
||||||
async def test_pull_review_detail_attaches_bounded_per_file_diff_previews(monkeypatch):
|
async def test_pull_review_detail_attaches_bounded_per_file_diff_previews(monkeypatch):
|
||||||
async def fake_fetch(path):
|
async def fake_fetch(path):
|
||||||
|
|
|
||||||
|
|
@ -293,3 +293,15 @@ async def test_review_sheet_loads_details_and_preserves_safe_gitea_handoff():
|
||||||
assert "review-history" in html
|
assert "review-history" in html
|
||||||
assert "open-review-gitea" in html
|
assert "open-review-gitea" in html
|
||||||
assert "reviewController.submit" not 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
|
||||||
|
|
|
||||||
|
|
@ -1,3 +1,5 @@
|
||||||
|
import asyncio
|
||||||
|
|
||||||
import httpx
|
import httpx
|
||||||
import pytest
|
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"
|
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
|
@pytest.mark.anyio
|
||||||
async def test_public_review_endpoint_does_not_expose_service_token_mutations():
|
async def test_public_review_endpoint_does_not_expose_service_token_mutations():
|
||||||
transport = httpx.ASGITransport(app=main.app)
|
transport = httpx.ASGITransport(app=main.app)
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue
Block a user