From d776a2b0033fe17b883b397c0ead37598cb314ec Mon Sep 17 00:00:00 2001 From: timmy Date: Mon, 24 Aug 2026 08:01:25 +0000 Subject: [PATCH 1/2] feat: cancel pending pull review requests (Closes #1340) --- frontend/dashboard.css | 1 + frontend/pull-sheet.js | 49 ++++++++++++++++++++++- src/gitea_proxy.py | 47 ++++++++++++++++++++++ src/main.py | 28 +++++++++++++ tests/test_my_work.py | 69 ++++++++++++++++++++++++++++++++ tests/test_pull_api.py | 91 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 284 insertions(+), 1 deletion(-) diff --git a/frontend/dashboard.css b/frontend/dashboard.css index f563690..74e6d5f 100644 --- a/frontend/dashboard.css +++ b/frontend/dashboard.css @@ -1265,6 +1265,7 @@ textarea { resize: vertical; min-height: 120px; } .pull-reviewer-status:first-child { border-top:0; } .pull-reviewer-status-heading { display:flex; justify-content:space-between; gap:10px; } .pull-reviewer-status-heading span { text-align:right; } +.cancel-review-request { min-height:44px; width:100%; margin-top:8px; } .pull-review-feedback { margin-top:8px; border:1px solid #2a496e; border-radius:8px; padding:0 8px 8px; } .pull-review-feedback > summary { min-height:44px; display:flex; align-items:center; cursor:pointer; } .pull-review-feedback-summary { margin:4px 0 10px; white-space:pre-wrap; } diff --git a/frontend/pull-sheet.js b/frontend/pull-sheet.js index 6eae4ac..675c8d2 100644 --- a/frontend/pull-sheet.js +++ b/frontend/pull-sheet.js @@ -66,10 +66,13 @@ function renderReviewerStatuses(reviewers, escapeHtml) { }; return (Array.isArray(reviewers) ? reviewers : []).map(reviewer => { const status = labels[reviewer?.status] || 'Status unavailable'; + const cancel = reviewer?.status === 'waiting' && reviewer?.login ? + '' : ''; return '
@' + escapeHtml(reviewer?.login || 'unknown') + '' + escapeHtml(status) + '
' + - renderReviewerFeedback(reviewer, escapeHtml) + '
'; + renderReviewerFeedback(reviewer, escapeHtml) + cancel + ''; }).join(''); } @@ -223,6 +226,37 @@ function bindReviewRequestControls(doc, controller, getSelected, getDetail) { qs('#pull-merge-state').textContent = 'Waiting for @' + result.reviewer + '’s review'; qs('#merge-pull').disabled = true; }; + qs('#pull-reviewer-statuses').addEventListener('click', async event => { + const button = event.target.closest?.('[data-cancel-review-request]'); + if (!button || button.disabled) return; + const selected = getSelected?.(); + const detail = getDetail?.(); + const reviewer = button.dataset.cancelReviewRequest; + if (!selected || !detail?.head_sha || !reviewer || + !globalThis.confirm('Cancel @' + reviewer + '’s review request for ' + selected.key + '?')) return; + button.disabled = true; + qs('#pull-reviewer-status').textContent = 'Cancelling @' + reviewer + '’s review request…'; + try { + const result = await controller.cancelReview(selected, reviewer, detail.head_sha); + if (result?.head_sha !== detail.head_sha || result?.requested_reviewers?.includes(reviewer)) { + throw new Error('Cancellation was not confirmed.'); + } + detail.reviewers = (detail.reviewers || []).filter(candidate => + candidate.login !== reviewer || candidate.status !== 'waiting'); + renderReviewerPanel(doc, detail); + const eligibility = mergeEligibility(detail, controller.reviewState(selected, detail)); + qs('#pull-merge-state').textContent = eligibility.reason; + qs('#merge-pull').disabled = !eligibility.allowed; + qs('#pull-review-request').open = true; + qs('#pull-review-request-status').textContent = 'Review request cancelled. Choose a replacement reviewer.'; + load.disabled = false; + load.focus(); + } catch (error) { + qs('#pull-reviewer-status').textContent = error.message + ' Refresh review data before retrying.'; + button.disabled = false; + button.focus(); + } + }); load.addEventListener('click', async () => { const selected = getSelected(); if (!selected) return; @@ -531,6 +565,7 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT let candidateRequest = null; let reviewCandidateRequest = null; let reviewRequestMutation = null; + let reviewCancelMutation = null; let editRequest = null; let feedbackRequest = null; const reviewRequests = new Map(); @@ -607,6 +642,18 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT }).finally(() => { reviewRequestMutation = null; }); return reviewRequestMutation; }, + cancelReview(item, reviewer, expectedHeadSha) { + if (reviewCancelMutation) return reviewCancelMutation; + reviewCancelMutation = fetchJson(pathFor(item) + '/request-review', { + method: 'DELETE', + headers: { Accept: 'application/json', 'Content-Type': 'application/json' }, + body: JSON.stringify({ reviewer, expected_head_sha: expectedHeadSha }), + }).then(result => { + this.onReviewCancelled?.(result); + return result; + }).finally(() => { reviewCancelMutation = null; }); + return reviewCancelMutation; + }, loadEditDraft(item) { try { const value = JSON.parse(storage?.getItem(editDraftKey(item)) || 'null'); diff --git a/src/gitea_proxy.py b/src/gitea_proxy.py index b4d24a3..04249fe 100644 --- a/src/gitea_proxy.py +++ b/src/gitea_proxy.py @@ -2028,6 +2028,53 @@ async def request_assigned_pull_review( } +async def cancel_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 {} + requested = [ + item["login"] for item in (pull.get("requested_reviewers") or []) + if isinstance(item, dict) and isinstance(item.get("login"), str) + ] + 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 + or reviewer not in requested + ): + raise IssueNotAvailableError("Pending review request changed") + response = await _get_client().request( + "DELETE", + 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() + remaining = [ + 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 [] + confirmed_head = confirmed.get("head") if isinstance(confirmed, dict) and isinstance(confirmed.get("head"), dict) else {} + if reviewer in remaining or confirmed_head.get("sha") != expected_head_sha: + raise ValueError("Gitea did not confirm reviewer cancellation") + return { + "repository": repository, + "number": number, + "head_sha": expected_head_sha, + "requested_reviewers": remaining, + "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 05ab136..e2c9d0b 100644 --- a/src/main.py +++ b/src/main.py @@ -6773,6 +6773,34 @@ async def request_assigned_pull_review( return JSONResponse(result) +@app.delete("/api/v1/repos/{owner}/{repo}/pulls/{number}/request-review") +async def cancel_assigned_pull_review( + request: PullReviewRequest, + owner: str, + repo: str, + number: int = PathParam(gt=0), +) -> JSONResponse: + try: + result = await asyncio.wait_for( + gitea_proxy.cancel_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 cancelling the request."}, + status_code=409, + ) + except Exception: + return JSONResponse( + {"error": "The cancellation could not be confirmed. Refresh review data before retrying."}, + 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 adda748..80e5390 100644 --- a/tests/test_my_work.py +++ b/tests/test_my_work.py @@ -7662,6 +7662,45 @@ const item = {{repository:'stackchain/api',number:7}}; } +def test_pull_sheet_single_flights_pending_review_cancellation(): + 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, body:JSON.parse(options.body)}}); + return new Promise(resolve => {{ finish = resolve; }}); + }} +}}); +const notified = []; +controller.onReviewCancelled = result => notified.push(result); +const item = {{repository:'stackchain/api',number:7}}; +(async () => {{ + const first = controller.cancelReview(item, 'sam', 'abc1234'); + const duplicate = controller.cancelReview(item, 'sam', 'abc1234'); + finish({{reviewer:'sam',head_sha:'abc1234',requested_reviewers:['casey']}}); + const results = await Promise.all([first, duplicate]); + process.stdout.write(JSON.stringify({{calls,results,same:first===duplicate,notified}})); +}})(); +""" + 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/request-review", + "method": "DELETE", + "body": {"reviewer": "sam", "expected_head_sha": "abc1234"}, + }], + "results": [{"reviewer": "sam", "head_sha": "abc1234", "requested_reviewers": ["casey"]}] * 2, + "same": True, + "notified": [{"reviewer": "sam", "head_sha": "abc1234", "requested_reviewers": ["casey"]}], + } + + def test_pull_sheet_exposes_touch_safe_review_request_flow(): root = PULL_SHEET.parents[1] html = (root / "frontend" / "index.html").read_text() @@ -7772,6 +7811,36 @@ process.stdout.write(JSON.stringify({{ assert "Approved current head" in output["html"] +def test_pull_sheet_offers_cancel_only_for_pending_review_requests(): + script = f""" +const createPullSheet = require({json.dumps(str(PULL_SHEET))}); +const html = createPullSheet.renderReviewerStatuses([ + {{login:'sam',status:'waiting'}}, + {{login:'casey',status:'approved'}}, + {{login:'lee',status:'commented'}}, +], value => String(value)); +process.stdout.write(html); +""" + html = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True).stdout + + assert html.count('data-cancel-review-request=') == 1 + assert 'data-cancel-review-request="sam"' in html + assert "Cancel request" in html + assert 'data-cancel-review-request="casey"' not in html + assert 'data-cancel-review-request="lee"' not in html + + +def test_pull_sheet_pending_review_cancellation_is_touch_safe_and_opens_replacement_picker(): + root = PULL_SHEET.parents[1] + css = (root / "frontend" / "dashboard.css").read_text() + script = PULL_SHEET.read_text() + + assert ".cancel-review-request { min-height:44px;" in css + assert "controller.cancelReview(selected, reviewer, detail.head_sha)" in script + assert "Choose a replacement reviewer." in script + assert "qs('#pull-review-request').open = true" in script + + def test_pull_sheet_renders_bounded_feedback_grouped_by_file(): 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 86d4667..de93e01 100644 --- a/tests/test_pull_api.py +++ b/tests/test_pull_api.py @@ -970,6 +970,30 @@ async def test_assigned_pull_review_request_endpoints_list_and_submit(monkeypatc ] +@pytest.mark.anyio +async def test_assigned_pull_cancel_review_endpoint_forwards_head_scoped_request(monkeypatch): + calls = [] + + async def cancel(repository, number, reviewer, expected_head_sha): + calls.append((repository, number, reviewer, expected_head_sha)) + return { + "repository": repository, "number": number, "reviewer": reviewer, + "head_sha": expected_head_sha, "requested_reviewers": ["casey"], + } + + monkeypatch.setattr(main.gitea_proxy, "cancel_assigned_pull_review", cancel, raising=False) + transport = httpx.ASGITransport(app=main.app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + response = await client.request( + "DELETE", "/api/v1/repos/stackchain/api/pulls/7/request-review", + json={"reviewer": "sam", "expected_head_sha": "abc1234"}, + ) + + assert response.status_code == 200 + assert response.json()["requested_reviewers"] == ["casey"] + assert calls == [("stackchain/api", 7, "sam", "abc1234")] + + @pytest.mark.anyio async def test_gitea_pull_handoff_preserves_coassignees_and_confirms_ownership_exit(): requests = [] @@ -1090,6 +1114,73 @@ async def test_gitea_pull_review_request_filters_candidates_and_confirms_request assert mutation[2] == b'{"reviewers":["casey"]}' +@pytest.mark.anyio +async def test_gitea_cancel_pending_pull_review_confirms_authoritative_removal(): + requests = [] + cancelled = False + + async def handler(request): + nonlocal cancelled + 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("/pulls/7"): + return httpx.Response(200, json={ + "number": 7, "state": "open", "merged": False, + "head": {"sha": "abc123"}, "assignees": [{"login": "timmy"}], + "requested_reviewers": [{"login": "casey"}] if cancelled else [{"login": "sam"}, {"login": "casey"}], + }) + if request.method == "DELETE" and request.url.path.endswith("/requested_reviewers"): + assert request.content == b'{"reviewers":["sam"]}' + cancelled = True + return httpx.Response(204) + raise AssertionError(f"unexpected request: {request.method} {request.url.path}") + + gitea_proxy.start_client(transport=httpx.MockTransport(handler)) + try: + result = await gitea_proxy.cancel_assigned_pull_review( + "stackchain/api", 7, "sam", "abc123" + ) + finally: + await gitea_proxy.stop_client() + + assert result == { + "repository": "stackchain/api", "number": 7, "head_sha": "abc123", + "requested_reviewers": ["casey"], "reviewer": "sam", + } + assert [method for method, _path, _body in requests] == ["GET", "GET", "DELETE", "GET"] + + +@pytest.mark.anyio +async def test_gitea_cancel_pending_pull_review_rejects_head_drift_before_mutation(): + requests = [] + + async def handler(request): + requests.append(request.method) + if request.url.path.endswith("/user"): + return httpx.Response(200, json={"login": "timmy"}) + if request.url.path.endswith("/pulls/7"): + return httpx.Response(200, json={ + "number": 7, "state": "open", "merged": False, + "head": {"sha": "new-head"}, "assignees": [{"login": "timmy"}], + "requested_reviewers": [{"login": "sam"}], + }) + if request.method == "DELETE": + return httpx.Response(204) + raise AssertionError(request.url.path) + + gitea_proxy.start_client(transport=httpx.MockTransport(handler)) + try: + with pytest.raises(gitea_proxy.IssueNotAvailableError): + await gitea_proxy.cancel_assigned_pull_review( + "stackchain/api", 7, "sam", "old-head" + ) + finally: + await gitea_proxy.stop_client() + + assert requests == ["GET", "GET"] + + @pytest.mark.anyio async def test_gitea_pull_release_removes_current_login_case_insensitively(): async def handler(request): -- 2.43.0 From 1f9f0f5c22c59b3e6efb3ec73f28d460194b35c3 Mon Sep 17 00:00:00 2001 From: timmy Date: Mon, 24 Aug 2026 08:16:21 +0000 Subject: [PATCH 2/2] test: gate mobile review cancellation journey --- .gitea/workflows/ci.yml | 2 +- ...bile_cancel_pull_review_request_release.py | 88 +++++++++++++++++++ tests/test_ci_workflow.py | 8 ++ 3 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 tests/e2e/test_mobile_cancel_pull_review_request_release.py diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index ad147fa..e97ccb0 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -57,7 +57,7 @@ jobs: pip install -r requirements-e2e.txt python3 -m playwright install --with-deps chromium - name: Exercise packaged mobile work journeys - run: python3 -m pytest tests/e2e/test_mobile_offline_issue_release.py tests/e2e/test_mobile_search_preview_navigation.py tests/e2e/test_mobile_search_week_plan.py tests/e2e/test_mobile_find_work_release.py tests/e2e/test_mobile_home_bootstrap_release.py tests/e2e/test_mobile_sign_out_release.py tests/e2e/test_mobile_today_handoff_release.py tests/e2e/test_mobile_today_wrap_up_release.py tests/e2e/test_mobile_today_summary_release.py tests/e2e/test_mobile_tomorrow_conflict_release.py tests/e2e/test_mobile_week_ahead_release.py tests/e2e/test_mobile_today_week_reschedule_release.py tests/e2e/test_mobile_wrap_up_handoff_release.py tests/e2e/test_mobile_following_release.py tests/e2e/test_mobile_detail_watch_release.py tests/e2e/test_mobile_pull_reviewer_status_release.py tests/e2e/test_mobile_pull_reviewer_feedback_release.py -q + run: python3 -m pytest tests/e2e/test_mobile_offline_issue_release.py tests/e2e/test_mobile_search_preview_navigation.py tests/e2e/test_mobile_search_week_plan.py tests/e2e/test_mobile_find_work_release.py tests/e2e/test_mobile_home_bootstrap_release.py tests/e2e/test_mobile_sign_out_release.py tests/e2e/test_mobile_today_handoff_release.py tests/e2e/test_mobile_today_wrap_up_release.py tests/e2e/test_mobile_today_summary_release.py tests/e2e/test_mobile_tomorrow_conflict_release.py tests/e2e/test_mobile_week_ahead_release.py tests/e2e/test_mobile_today_week_reschedule_release.py tests/e2e/test_mobile_wrap_up_handoff_release.py tests/e2e/test_mobile_following_release.py tests/e2e/test_mobile_detail_watch_release.py tests/e2e/test_mobile_pull_reviewer_status_release.py tests/e2e/test_mobile_pull_reviewer_feedback_release.py -q tests/e2e/test_mobile_address_review_feedback_release.py tests/e2e/test_mobile_cancel_pull_review_request_release.py release-candidate: runs-on: ubuntu-latest diff --git a/tests/e2e/test_mobile_cancel_pull_review_request_release.py b/tests/e2e/test_mobile_cancel_pull_review_request_release.py new file mode 100644 index 0000000..ff75c40 --- /dev/null +++ b/tests/e2e/test_mobile_cancel_pull_review_request_release.py @@ -0,0 +1,88 @@ +import os +from pathlib import Path + +import pytest + + +ROOT = Path(__file__).parents[2] + +if os.environ.get("STACKCHAIN_RUN_RELEASE_E2E") != "1": + pytest.skip("release browser journey is opt-in", allow_module_level=True) + + +@pytest.mark.parametrize("viewport", [(320, 568), (390, 844)]) +def test_mobile_author_cancels_pending_review_and_opens_replacement_picker(viewport): + playwright = pytest.importorskip("playwright.sync_api") + html = (ROOT / "frontend" / "index.html").read_text() + + with playwright.sync_playwright() as runtime: + try: + browser = runtime.chromium.launch(headless=True) + except Exception as error: + pytest.skip(f"Chromium is not installed: {error}") + page = browser.new_page(viewport={"width": viewport[0], "height": viewport[1]}) + page.set_content(html, wait_until="domcontentloaded") + page.add_style_tag(path=ROOT / "frontend" / "dashboard.css") + page.add_script_tag(path=ROOT / "frontend" / "pull-sheet.js") + page.evaluate( + """() => { + globalThis.confirm = () => true; + globalThis.cancelCalls = []; + globalThis.cancelItem = { + repository:'stackchain/api', number:7, key:'stackchain/api#7' + }; + globalThis.cancelDetail = { + state:'open', draft:false, mergeable:true, merged:false, + ci_state:'success', head_sha:'abc1234', files:[], + reviewers:[{ + login:'sam', status:'waiting', head_sha:'abc1234', blocking:true + }], + }; + globalThis.cancelController = createPullSheet({ + storage:null, + fetchJson:async (path, options={}) => { + cancelCalls.push({path, options}); + return { + repository:'stackchain/api', number:7, reviewer:'sam', + head_sha:'abc1234', requested_reviewers:[], + }; + }, + }); + createPullSheet.bindReviewRequestControls( + document, cancelController, () => cancelItem, () => cancelDetail + ); + createPullSheet.review(cancelDetail, null, document); + document.querySelector('#pull-sheet').classList.add('open'); + document.querySelector('#pull-review').open = true; + }""" + ) + + cancel = page.locator('[data-cancel-review-request="sam"]') + bounds = cancel.bounding_box() + assert bounds and bounds["height"] >= 44 + cancel.click() + page.wait_for_function("cancelCalls.length === 1") + page.wait_for_function( + "document.querySelector('#pull-review-request-status').textContent.includes('Choose a replacement reviewer')" + ) + result = page.evaluate( + """() => ({ + calls:cancelCalls, + waitingRows:document.querySelectorAll('[data-reviewer-status="waiting"]').length, + pickerOpen:document.querySelector('#pull-review-request').open, + focused:document.activeElement?.id, + scrollWidth:document.documentElement.scrollWidth, + clientWidth:document.documentElement.clientWidth, + })""" + ) + browser.close() + + assert result["scrollWidth"] <= result["clientWidth"] + assert result["waitingRows"] == 0 + assert result["pickerOpen"] is True + assert result["focused"] == "load-pull-reviewers" + assert len(result["calls"]) == 1 + assert result["calls"][0]["options"]["method"] == "DELETE" + assert result["calls"][0]["options"]["body"] == ( + '{"reviewer":"sam","expected_head_sha":"abc1234"}' + ) diff --git a/tests/test_ci_workflow.py b/tests/test_ci_workflow.py index 8455d5c..ec6c6b7 100644 --- a/tests/test_ci_workflow.py +++ b/tests/test_ci_workflow.py @@ -85,3 +85,11 @@ def test_browser_job_runs_packaged_today_week_reschedule_journey(): browser = text[text.index(" browser-journey:") : text.index(" release-candidate:")] assert "tests/e2e/test_mobile_today_week_reschedule_release.py" in browser + + +def test_browser_job_gates_latest_pull_review_mobile_journeys(): + text = WORKFLOW.read_text() + browser = text[text.index(" browser-journey:") : text.index(" release-candidate:")] + + assert "tests/e2e/test_mobile_address_review_feedback_release.py" in browser + assert "tests/e2e/test_mobile_cancel_pull_review_request_release.py" in browser -- 2.43.0