From 1f950f93dd6d9249323c3484aa6d1add05e0dc10 Mon Sep 17 00:00:00 2001 From: timmy Date: Mon, 24 Aug 2026 02:11:24 +0000 Subject: [PATCH] feat: request teammate pull reviews (Closes #1330) --- frontend/dashboard.css | 7 ++-- frontend/dashboard.js | 1 + frontend/index.html | 10 +++++ frontend/pull-sheet.js | 83 ++++++++++++++++++++++++++++++++++++++ src/gitea_proxy.py | 66 +++++++++++++++++++++++++++++++ src/main.py | 60 ++++++++++++++++++++++++++++ tests/test_my_work.py | 59 +++++++++++++++++++++++++++ tests/test_pull_api.py | 90 ++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 373 insertions(+), 3 deletions(-) diff --git a/frontend/dashboard.css b/frontend/dashboard.css index 644be9a..aae4bdc 100644 --- a/frontend/dashboard.css +++ b/frontend/dashboard.css @@ -958,11 +958,12 @@ textarea { resize: vertical; min-height: 120px; } .issue-sheet-actions #watch-issue-detail, .pull-sheet-actions #watch-pull-detail { min-width:0; min-height:44px; } .detail-watch-status:not(:empty) { margin-top:8px; overflow-wrap:anywhere; } .issue-sheet-actions a { border:1px solid #60a5fa; border-radius:10px; font-weight:700; } -.issue-handoff, .pull-ownership { margin-top:14px; padding:12px; border:1px solid #2a496e; border-radius:12px; } -.issue-handoff > div, .pull-ownership > div { display:grid; gap:8px; margin-top:10px; } -.issue-handoff select, .pull-ownership select { width:100%; max-width:100%; padding:8px; border:1px solid #1f3a5f; border-radius:8px; background:#0b1526; color:var(--text); } +.issue-handoff, .pull-ownership, .pull-review-request { margin-top:14px; padding:12px; border:1px solid #2a496e; border-radius:12px; } +.issue-handoff > div, .pull-ownership > div, .pull-review-request > div { display:grid; gap:8px; margin-top:10px; } +.issue-handoff select, .pull-ownership select, .pull-review-request select { width:100%; max-width:100%; padding:8px; border:1px solid #1f3a5f; border-radius:8px; background:#0b1526; color:var(--text); } .issue-handoff select, .issue-handoff button { min-height:44px; } .pull-ownership select, .pull-ownership button { min-height:44px; } +.pull-review-request select, .pull-review-request button { min-height:44px; } .issue-retry { min-height:44px; width:100%; margin-top:10px; } .new-issue { min-height:44px; } .find-work-action { min-height:44px; } diff --git a/frontend/dashboard.js b/frontend/dashboard.js index ddd33f4..206ca17 100644 --- a/frontend/dashboard.js +++ b/frontend/dashboard.js @@ -4659,6 +4659,7 @@ const detail = offlineDetail || await pullController.load(item); if (selectedPull !== item) return; selectedPullDetail = detail; + pullController.setReviewDetail(detail); pullConversation = pullController.conversation(item, detail.conversation); qs('#pull-sheet-title').textContent = detail.title || 'Assigned pull request'; qs('#pull-sheet-body').innerHTML = renderMarkdown(detail.body || 'No description provided.'); diff --git a/frontend/index.html b/frontend/index.html index a281168..6bd4051 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -1720,6 +1720,16 @@
+
+ Request review +
+ + + + +
Load teammates to request a review.
+
+
Ownership
diff --git a/frontend/pull-sheet.js b/frontend/pull-sheet.js index d42f75d..8d44ca3 100644 --- a/frontend/pull-sheet.js +++ b/frontend/pull-sheet.js @@ -101,7 +101,70 @@ function resetOwnershipControls(doc, item, checkpointed) { qs('#pull-handoff-status').textContent = 'Load teammates to transfer ownership.'; } +function resetReviewRequestControls(doc, detail) { + const qs = selector => doc.querySelector(selector); + qs('#pull-review-request').open = false; + qs('#pull-review-request').dataset.headSha = detail?.head_sha || ''; + qs('#pull-review-recipient').innerHTML = ''; + qs('#pull-review-recipient').disabled = true; + qs('#confirm-pull-review').disabled = true; + qs('#load-pull-reviewers').disabled = !detail?.head_sha; + qs('#pull-review-request-status').textContent = detail?.head_sha ? + 'Load teammates to request a review.' : 'Load the current pull request before requesting review.'; +} + +function bindReviewRequestControls(doc, controller, getSelected) { + const qs = selector => doc.querySelector(selector); + const load = qs('#load-pull-reviewers'); + if (load.dataset.reviewRequestBound === 'true') return; + load.dataset.reviewRequestBound = 'true'; + controller.setReviewDetail = detail => resetReviewRequestControls(doc, detail); + resetReviewRequestControls(doc, null); + load.addEventListener('click', async () => { + const selected = getSelected(); + if (!selected) return; + const select = qs('#pull-review-recipient'); + load.disabled = true; + qs('#pull-review-request-status').textContent = 'Loading eligible reviewers…'; + try { + const count = renderHandoffCandidates(select, await controller.loadReviewCandidates(selected), doc); + select.disabled = !count; + qs('#confirm-pull-review').disabled = true; + qs('#pull-review-request-status').textContent = count ? + 'Choose a teammate to review the current head.' : 'No eligible reviewers were found.'; + if (count) select.focus(); + else load.disabled = false; + } catch (error) { + qs('#pull-review-request-status').textContent = error.message + ' Retry loading reviewers.'; + load.disabled = false; + load.focus(); + } + }); + qs('#pull-review-recipient').addEventListener('change', event => { + qs('#confirm-pull-review').disabled = !event.target.value; + }); + qs('#confirm-pull-review').addEventListener('click', async () => { + const selected = getSelected(); + const detail = { head_sha: qs('#pull-review-request').dataset.headSha }; + const reviewer = qs('#pull-review-recipient').value; + if (!selected || !detail?.head_sha || !reviewer || + !globalThis.confirm('Request review of ' + selected.key + ' at ' + detail.head_sha.slice(0, 8) + ' from @' + reviewer + '?')) return; + const button = qs('#confirm-pull-review'); + button.disabled = true; + qs('#pull-review-request-status').textContent = 'Requesting review…'; + try { + await controller.requestReview(selected, reviewer, detail.head_sha); + qs('#pull-review-request-status').textContent = 'Review requested from @' + reviewer + '.'; + } catch (error) { + qs('#pull-review-request-status').textContent = error.message + ' Selection kept; retry the request.'; + button.disabled = false; + button.focus(); + } + }); +} + function bindOwnershipControls(doc, controller, getSelected, finish) { + bindReviewRequestControls(doc, controller, getSelected); const qs = selector => doc.querySelector(selector); const load = qs('#load-pull-handoff'); if (load.dataset.ownershipBound === 'true') return; @@ -172,6 +235,8 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT let checkRequest = null; let ownershipRequest = null; let candidateRequest = null; + let reviewCandidateRequest = null; + let reviewRequestMutation = null; const reviewRequests = new Map(); const reviewCache = new Map(); const pathFor = item => 'api/v1/repos/' + String(item.repository || '').split('/') @@ -224,6 +289,22 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT }).finally(() => { candidateRequest = null; }); return candidateRequest; }, + loadReviewCandidates(item) { + if (reviewCandidateRequest) return reviewCandidateRequest; + reviewCandidateRequest = fetchJson(pathFor(item) + '/review-candidates', { + headers: { Accept: 'application/json' }, + }).finally(() => { reviewCandidateRequest = null; }); + return reviewCandidateRequest; + }, + requestReview(item, reviewer, expectedHeadSha) { + if (reviewRequestMutation) return reviewRequestMutation; + reviewRequestMutation = fetchJson(pathFor(item) + '/request-review', { + method: 'POST', + headers: { Accept: 'application/json', 'Content-Type': 'application/json' }, + body: JSON.stringify({ reviewer, expected_head_sha: expectedHeadSha }), + }).finally(() => { reviewRequestMutation = null; }); + return reviewRequestMutation; + }, handoff(item, recipient) { if (ownershipRequest) return ownershipRequest; ownershipRequest = fetchJson(pathFor(item) + '/handoff', { @@ -323,4 +404,6 @@ createPullSheet.renderHandoffCandidates = renderHandoffCandidates; createPullSheet.ownershipExitMessage = ownershipExitMessage; createPullSheet.resetOwnershipControls = resetOwnershipControls; createPullSheet.bindOwnershipControls = bindOwnershipControls; +createPullSheet.resetReviewRequestControls = resetReviewRequestControls; +createPullSheet.bindReviewRequestControls = bindReviewRequestControls; if (typeof module !== 'undefined' && module.exports) module.exports = createPullSheet; diff --git a/src/gitea_proxy.py b/src/gitea_proxy.py index 1b5ec6e..98fa3aa 100644 --- a/src/gitea_proxy.py +++ b/src/gitea_proxy.py @@ -1962,6 +1962,72 @@ async def pull_handoff_candidates(repository: str) -> list[dict]: ][:25] +async def pull_review_candidates(repository: str, number: int) -> list[dict]: + pull, candidates = await asyncio.gather( + fetch(f"repos/{repository}/pulls/{number}"), + pull_handoff_candidates(repository), + ) + if not isinstance(pull, dict): + raise ValueError("Gitea pull request response was not an object") + excluded = { + item["login"].casefold() + for item in (pull.get("requested_reviewers") or []) + if isinstance(item, dict) and isinstance(item.get("login"), str) + } + author = pull.get("user") if isinstance(pull.get("user"), dict) else {} + if isinstance(author.get("login"), str): + excluded.add(author["login"].casefold()) + return [ + item for item in candidates + if item["login"].casefold() not in excluded + ][:25] + + +async def request_assigned_pull_review( + repository: str, number: int, reviewer: str, expected_head_sha: str +) -> dict: + login, pull = await _current_login_and_target( + f"repos/{repository}/pulls/{number}" + ) + head = pull.get("head") if isinstance(pull.get("head"), dict) else {} + if ( + pull.get("state") != "open" + or pull.get("merged") is True + or not _login_in_users(login, pull.get("assignees")) + or head.get("sha") != expected_head_sha + ): + raise IssueNotAvailableError("Pull request is no longer eligible for review request") + eligible = { + item["login"] for item in await pull_review_candidates(repository, number) + } + if reviewer not in eligible: + raise IssueNotAvailableError("Reviewer is not eligible") + response = await _get_client().post( + f"/api/v1/repos/{repository}/pulls/{number}/requested_reviewers", + headers=_auth(), + json={"reviewers": [reviewer]}, + ) + response.raise_for_status() + confirmed_response = await _get_client().get( + f"/api/v1/repos/{repository}/pulls/{number}", headers=_auth() + ) + confirmed_response.raise_for_status() + confirmed = confirmed_response.json() + requested = [ + item["login"] for item in (confirmed.get("requested_reviewers") or []) + if isinstance(item, dict) and isinstance(item.get("login"), str) + ] if isinstance(confirmed, dict) else [] + if reviewer not in requested: + raise ValueError("Gitea did not confirm the requested reviewer") + return { + "repository": repository, + "number": number, + "head_sha": expected_head_sha, + "requested_reviewers": requested, + "reviewer": reviewer, + } + + async def _change_assigned_pull_owners( repository: str, number: int, recipient: str | None ) -> dict: diff --git a/src/main.py b/src/main.py index 889babe..5459955 100644 --- a/src/main.py +++ b/src/main.py @@ -1108,6 +1108,15 @@ class IssueHandoff(BaseModel): ) +class PullReviewRequest(BaseModel): + reviewer: str = Field( + min_length=1, max_length=255, pattern=r"^[A-Za-z0-9_.-]+$" + ) + expected_head_sha: str = Field( + min_length=7, max_length=64, pattern=r"^[A-Fa-f0-9]+$" + ) + + class IssueReassignment(IssueHandoff): expected_assignees: list[str] = Field(min_length=1, max_length=10) @@ -6662,6 +6671,57 @@ async def handoff_assigned_pull( return JSONResponse(result) +@app.get("/api/v1/repos/{owner}/{repo}/pulls/{number}/review-candidates") +async def pull_review_candidates( + owner: str, repo: str, number: int = PathParam(gt=0) +) -> JSONResponse: + repository = f"{owner}/{repo}" + try: + if not await gitea_proxy.is_assigned_pull(repository, number): + raise HTTPException(status_code=404, detail="Assigned pull request not found") + result = await asyncio.wait_for( + gitea_proxy.pull_review_candidates(repository, number), + timeout=ISSUE_ACTION_TIMEOUT_SECONDS, + ) + except HTTPException: + raise + except Exception: + return JSONResponse( + {"error": "Reviewers could not be loaded. Please retry."}, + status_code=503, + headers={"Retry-After": "1"}, + ) + return JSONResponse(result, headers={"Cache-Control": "no-store"}) + + +@app.post("/api/v1/repos/{owner}/{repo}/pulls/{number}/request-review") +async def request_assigned_pull_review( + request: PullReviewRequest, + owner: str, + repo: str, + number: int = PathParam(gt=0), +) -> JSONResponse: + try: + result = await asyncio.wait_for( + gitea_proxy.request_assigned_pull_review( + f"{owner}/{repo}", number, request.reviewer, request.expected_head_sha + ), + timeout=ISSUE_ACTION_TIMEOUT_SECONDS, + ) + except gitea_proxy.IssueNotAvailableError: + return JSONResponse( + {"error": "The pull request or reviewer changed. Reload before requesting review."}, + status_code=409, + ) + except Exception: + return JSONResponse( + {"error": "The review request could not be confirmed. Please retry."}, + status_code=503, + headers={"Retry-After": "1"}, + ) + return JSONResponse(result) + + @app.patch("/api/v1/repos/{owner}/{repo}/pulls/{number}/release") async def release_assigned_pull( owner: str, repo: str, number: int = PathParam(gt=0) diff --git a/tests/test_my_work.py b/tests/test_my_work.py index fd80520..154a980 100644 --- a/tests/test_my_work.py +++ b/tests/test_my_work.py @@ -7539,6 +7539,65 @@ const item = {{repository:'stackchain/api',number:7}}; assert output["released"] == {"assignees": []} +def test_pull_sheet_loads_reviewers_and_single_flights_review_request(): + script = f""" +const createPullSheet = require({json.dumps(str(PULL_SHEET))}); +const calls = []; +let finish; +const controller = createPullSheet({{ + storage: null, + fetchJson: (url, options={{}}) => {{ + calls.push({{url, method:options.method || 'GET', body:options.body ? JSON.parse(options.body) : null}}); + if (url.endsWith('/review-candidates')) return Promise.resolve([{{login:'casey',name:'Casey'}}]); + return new Promise(resolve => {{ finish = resolve; }}); + }} +}}); +const item = {{repository:'stackchain/api',number:7}}; +(async () => {{ + const candidates = await controller.loadReviewCandidates(item); + const first = controller.requestReview(item, 'casey', 'abc1234'); + const duplicate = controller.requestReview(item, 'casey', 'abc1234'); + finish({{reviewer:'casey',requested_reviewers:['sam','casey']}}); + const results = await Promise.all([first, duplicate]); + process.stdout.write(JSON.stringify({{calls,candidates,results,same:first===duplicate}})); +}})(); +""" + output = json.loads(subprocess.run( + ["node", "-e", script], check=True, capture_output=True, text=True + ).stdout) + + assert output == { + "calls": [ + {"url": "api/v1/repos/stackchain/api/pulls/7/review-candidates", "method": "GET", "body": None}, + {"url": "api/v1/repos/stackchain/api/pulls/7/request-review", "method": "POST", "body": { + "reviewer": "casey", "expected_head_sha": "abc1234" + }}, + ], + "candidates": [{"login": "casey", "name": "Casey"}], + "results": [ + {"reviewer": "casey", "requested_reviewers": ["sam", "casey"]}, + {"reviewer": "casey", "requested_reviewers": ["sam", "casey"]}, + ], + "same": True, + } + + +def test_pull_sheet_exposes_touch_safe_review_request_flow(): + root = PULL_SHEET.parents[1] + html = (root / "frontend" / "index.html").read_text() + css = (root / "frontend" / "dashboard.css").read_text() + dashboard = (root / "frontend" / "dashboard.js").read_text() + + assert 'id="pull-review-request"' in html + assert 'id="load-pull-reviewers"' in html + assert 'id="pull-review-recipient"' in html + assert 'id="confirm-pull-review"' in html + assert 'id="pull-review-request-status" class="small" aria-live="assertive"' in html + assert '.pull-review-request select, .pull-review-request button { min-height:44px;' in css + assert "createPullSheet.bindOwnershipControls(document, pullController" in dashboard + assert "pullController.setReviewDetail(detail)" in dashboard + + def test_pull_sheet_surfaces_pending_merge_confirmation_guidance(): script = f""" const createPullSheet = require({json.dumps(str(PULL_SHEET))}); diff --git a/tests/test_pull_api.py b/tests/test_pull_api.py index b597484..3c108a2 100644 --- a/tests/test_pull_api.py +++ b/tests/test_pull_api.py @@ -640,6 +640,45 @@ async def test_assigned_pull_ownership_endpoints_list_handoff_and_release(monkey ] +@pytest.mark.anyio +async def test_assigned_pull_review_request_endpoints_list_and_submit(monkeypatch): + calls = [] + + async def assigned(repository, number): + calls.append(("assigned", repository, number)) + return True + + async def candidates(repository, number): + calls.append(("candidates", repository, number)) + return [{"login": "casey", "name": "Casey"}] + + async def request_review(repository, number, reviewer, expected_head_sha): + calls.append(("request", repository, number, reviewer, expected_head_sha)) + return {"number": number, "reviewer": reviewer, "requested_reviewers": [reviewer]} + + monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned) + monkeypatch.setattr(main.gitea_proxy, "pull_review_candidates", candidates, raising=False) + monkeypatch.setattr(main.gitea_proxy, "request_assigned_pull_review", request_review, raising=False) + transport = httpx.ASGITransport(app=main.app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + listed = await client.get("/api/v1/repos/stackchain/api/pulls/7/review-candidates") + requested = await client.post( + "/api/v1/repos/stackchain/api/pulls/7/request-review", + json={"reviewer": "casey", "expected_head_sha": "abc1234"}, + ) + + assert listed.status_code == 200 + assert listed.headers["cache-control"] == "no-store" + assert listed.json() == [{"login": "casey", "name": "Casey"}] + assert requested.status_code == 200 + assert requested.json()["requested_reviewers"] == ["casey"] + assert calls == [ + ("assigned", "stackchain/api", 7), + ("candidates", "stackchain/api", 7), + ("request", "stackchain/api", 7, "casey", "abc1234"), + ] + + @pytest.mark.anyio async def test_gitea_pull_handoff_preserves_coassignees_and_confirms_ownership_exit(): requests = [] @@ -709,6 +748,57 @@ async def test_pull_handoff_candidates_are_bounded_and_exclude_invalid_or_curren ] +@pytest.mark.anyio +async def test_gitea_pull_review_request_filters_candidates_and_confirms_requested_reviewer(): + requests = [] + review_requested = False + + async def handler(request): + nonlocal review_requested + requests.append((request.method, request.url.path, request.content)) + if request.url.path.endswith("/user"): + return httpx.Response(200, json={"login": "timmy"}) + if request.url.path.endswith("/assignees"): + return httpx.Response(200, json=[ + {"login": "timmy", "full_name": "Timmy"}, + {"login": "alex", "full_name": "Alexander"}, + {"login": "sam", "full_name": "Sam"}, + {"login": "casey", "full_name": "Casey"}, + ]) + if request.url.path.endswith("/pulls/7"): + requested = [{"login": "sam"}] + if review_requested: + requested.append({"login": "casey"}) + return httpx.Response(200, json={ + "number": 7, "state": "open", "merged": False, + "head": {"sha": "abc123"}, "user": {"login": "alex"}, + "assignees": [{"login": "timmy"}], + "requested_reviewers": requested, + }) + if request.method == "POST" and request.url.path.endswith("/requested_reviewers"): + review_requested = True + return httpx.Response(201, json={}) + raise AssertionError(f"unexpected request: {request.method} {request.url.path}") + + gitea_proxy.start_client(transport=httpx.MockTransport(handler)) + try: + candidates = await gitea_proxy.pull_review_candidates("stackchain/api", 7) + result = await gitea_proxy.request_assigned_pull_review( + "stackchain/api", 7, "casey", "abc123" + ) + finally: + await gitea_proxy.stop_client() + + assert candidates == [{"login": "casey", "name": "Casey"}] + assert result == { + "repository": "stackchain/api", "number": 7, "head_sha": "abc123", + "requested_reviewers": ["sam", "casey"], "reviewer": "casey", + } + mutation = next(item for item in requests if item[0] == "POST") + assert mutation[1] == "/api/v1/repos/stackchain/api/pulls/7/requested_reviewers" + assert mutation[2] == b'{"reviewers":["casey"]}' + + @pytest.mark.anyio async def test_gitea_pull_release_removes_current_login_case_insensitively(): async def handler(request):