From bd96c3caa3788f5e8fd57771cacb2004d0bdbfb5 Mon Sep 17 00:00:00 2001 From: timmy Date: Mon, 24 Aug 2026 04:09:05 +0000 Subject: [PATCH 1/2] feat: show pull reviewer decisions (Closes #1334) --- frontend/dashboard.css | 6 ++++ frontend/dashboard.js | 9 ++--- frontend/index.html | 6 ++++ frontend/pull-sheet.js | 68 ++++++++++++++++++++++++++++++++++-- src/gitea_proxy.py | 65 +++++++++++++++++++++++++++++++++-- tests/test_my_work.py | 53 +++++++++++++++++++++++++++- tests/test_pull_api.py | 78 +++++++++++++++++++++++++++++++++++++++++- 7 files changed, 275 insertions(+), 10 deletions(-) diff --git a/frontend/dashboard.css b/frontend/dashboard.css index aae4bdc..1b1e812 100644 --- a/frontend/dashboard.css +++ b/frontend/dashboard.css @@ -1259,6 +1259,12 @@ textarea { resize: vertical; min-height: 120px; } .pull-diff-empty { margin:8px 0; padding:10px; border:1px dashed #4e6b8a; border-radius:8px; } .pull-review-tools { display:flex; align-items:center; justify-content:space-between; gap:8px; margin:8px 0; } .pull-review-tools button { min-height:44px; } +.pull-reviewer-summary { margin:10px 0; padding:10px; border:1px solid #203a5c; border-radius:10px; } +.pull-reviewer-summary h3 { margin:0 0 6px; font-size:14px; } +.pull-reviewer-status { display:flex; justify-content:space-between; gap:10px; padding:8px 0; border-top:1px solid #203a5c; overflow-wrap:anywhere; } +.pull-reviewer-status:first-child { border-top:0; } +.pull-reviewer-status span { text-align:right; } +#request-updated-pull-review { min-height:44px; width:100%; margin-top:8px; } .pull-review { margin-top:14px; overflow:hidden; border:1px solid #2a496e; border-radius:10px; padding:0 10px 10px; } .pull-review summary { min-height:44px; display:flex; align-items:center; cursor:pointer; } .pull-review summary h2 { margin:0; font-size:16px; } diff --git a/frontend/dashboard.js b/frontend/dashboard.js index e1caffc..f8ee785 100644 --- a/frontend/dashboard.js +++ b/frontend/dashboard.js @@ -1288,6 +1288,7 @@ paintMyWork(lastContextSnapshot); return false; }, ()=>selectedPullDetail, ()=>confirmedOwnerLogin); + } attachReleaseReceipt(); if (!reviewController) reviewController = createReviewController({ fetchJson: fetchReviewJson, storage: localStorage }); @@ -4536,7 +4537,7 @@ qs('#pull-review-progress').textContent = pullReviewState.total ? pullReviewState.reviewed.length + ' of ' + pullReviewState.total + ' files reviewed' : 'No changed files to review'; qs('#next-unreviewed-pull-file').disabled = pullReviewState.complete; - const eligibility = createPullSheet.mergeEligibility(detail, pullReviewState); + const eligibility = createPullSheet.review(detail, pullReviewState, document); qs('#pull-merge-state').textContent = eligibility.reason; qs('#merge-pull').disabled = !eligibility.allowed; qs('#pull-files').innerHTML = (detail.files || []).length ? detail.files.map((file, index) => @@ -4573,9 +4574,7 @@ return; } selectedPullDetail = { ...selectedPullDetail, ...status }; - const eligibility = createPullSheet.mergeEligibility(selectedPullDetail, pullReviewState); - qs('#pull-merge-state').textContent = eligibility.reason; - qs('#merge-pull').disabled = !eligibility.allowed; + renderPullReview(selectedPullDetail); qs('#pull-review-status').textContent = 'Checks refreshed for the current head.'; } @@ -4648,6 +4647,7 @@ qs('#pull-checks-summary').textContent = 'Not loaded'; qs('#pull-check-list').textContent = ''; qs('#pull-checks').open = false; + qs('#pull-merge-state').textContent = 'Review data not loaded'; qs('#merge-pull').disabled = true; qs('#merge-pull').textContent = workSession.active() ? 'Merge & next' : 'Merge'; @@ -7245,6 +7245,7 @@ if (event.currentTarget.open) loadPullReview(); }); qs('#pull-review-retry').addEventListener('click', loadPullReview); + qs('#refresh-pull-checks').addEventListener('click', async () => { if (!selectedPull || !selectedPullDetail?.head_sha) return; const item = selectedPull; diff --git a/frontend/index.html b/frontend/index.html index 5b4d0db..c502015 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -1721,6 +1721,12 @@
Expand to load changed files and merge readiness.
CI unknownReview data not loaded
+
+

Reviewer status

+
Reviewer status not loaded.
+
+ +
ChecksNot loaded diff --git a/frontend/pull-sheet.js b/frontend/pull-sheet.js index 3b58d60..1a73f7d 100644 --- a/frontend/pull-sheet.js +++ b/frontend/pull-sheet.js @@ -12,12 +12,54 @@ function mergeEligibility(detail, reviewState) { 'CI blocked by ' + blockers.join(', ') : 'CI must succeed before merging' }; } if (!detail.head_sha) return { allowed: false, reason: 'Current head is unavailable' }; + const reviewer = (detail.reviewers || []).find(item => item?.blocking); + if (reviewer?.status === 'waiting') { + return { allowed: false, reason: 'Waiting for @' + reviewer.login + '’s review' }; + } + if (reviewer?.status === 'changes_requested') { + return { allowed: false, reason: '@' + reviewer.login + ' requested changes' }; + } + if (reviewer?.status === 'outdated') { + return { allowed: false, reason: '@' + reviewer.login + '’s review is outdated after new commits' }; + } if (reviewState && !reviewState.complete) { return { allowed: false, reason: 'Review every changed file before merging' }; } return { allowed: true, reason: 'Ready to merge' }; } +function renderReviewerStatuses(reviewers, escapeHtml) { + const labels = { + waiting: 'Waiting for review', + approved: 'Approved current head', + changes_requested: 'Changes requested', + commented: 'Commented', + outdated: 'Outdated after new commits', + }; + return (Array.isArray(reviewers) ? reviewers : []).map(reviewer => { + const status = labels[reviewer?.status] || 'Status unavailable'; + return '
@' + + escapeHtml(reviewer?.login || 'unknown') + '' + escapeHtml(status) + '
'; + }).join(''); +} + +function renderReviewerPanel(doc, detail, escapeHtml = value => String(value) + .replaceAll('&', '&').replaceAll('<', '<').replaceAll('>', '>')) { + const reviewers = Array.isArray(detail?.reviewers) ? detail.reviewers : []; + doc.querySelector('#pull-reviewer-statuses').innerHTML = renderReviewerStatuses(reviewers, escapeHtml); + doc.querySelector('#pull-reviewer-status').textContent = reviewers.length ? + reviewers.length + ' reviewer status' + (reviewers.length === 1 ? '' : 'es') + ' for this pull request.' : + 'No review has been requested; existing merge policy is unchanged.'; + doc.querySelector('#request-updated-pull-review').hidden = + !reviewers.some(reviewer => reviewer.status === 'outdated'); +} + +function reviewEligibility(detail, reviewState, doc) { + renderReviewerPanel(doc, detail); + return mergeEligibility(detail, reviewState); +} + function renderFile(file, index, reviewed, escapeHtml) { const filename = escapeHtml(file.filename || 'Unknown file'); const panelId = 'pull-diff-' + index; @@ -114,9 +156,12 @@ function resetReviewRequestControls(doc, detail) { 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.'; + qs('#pull-reviewer-status').textContent = 'Reviewer status not loaded.'; + qs('#pull-reviewer-statuses').textContent = ''; + qs('#request-updated-pull-review').hidden = true; } -function bindReviewRequestControls(doc, controller, getSelected) { +function bindReviewRequestControls(doc, controller, getSelected, getDetail) { const qs = selector => doc.querySelector(selector); const load = qs('#load-pull-reviewers'); if (load.dataset.reviewRequestBound === 'true') return; @@ -126,6 +171,20 @@ function bindReviewRequestControls(doc, controller, getSelected) { controller.edit?.setDetail(detail, Boolean(detail?.saved_at)); }; resetReviewRequestControls(doc, null); + qs('#request-updated-pull-review').addEventListener('click', () => { + qs('#pull-review-request').open = true; + qs('#pull-review-request').scrollIntoView({ block:'center', behavior:'smooth' }); + load.focus(); + }); + controller.onReviewRequested = result => { + const detail = getDetail?.(); + if (!detail || result.head_sha !== detail.head_sha) return; + detail.reviewers = (detail.reviewers || []).filter(reviewer => reviewer.login !== result.reviewer); + detail.reviewers.push({login:result.reviewer, status:'waiting', head_sha:result.head_sha, blocking:true}); + renderReviewerPanel(doc, detail); + qs('#pull-merge-state').textContent = 'Waiting for @' + result.reviewer + '’s review'; + qs('#merge-pull').disabled = true; + }; load.addEventListener('click', async () => { const selected = getSelected(); if (!selected) return; @@ -242,7 +301,7 @@ function bindContextEditor(doc, controller, getSelected, getDetail, getLogin) { } function bindOwnershipControls(doc, controller, getSelected, finish, getDetail, getLogin) { - bindReviewRequestControls(doc, controller, getSelected); + bindReviewRequestControls(doc, controller, getSelected, getDetail); controller.edit = bindContextEditor(doc, controller, getSelected, getDetail, getLogin); const qs = selector => doc.querySelector(selector); const load = qs('#load-pull-handoff'); @@ -383,6 +442,9 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT method: 'POST', headers: { Accept: 'application/json', 'Content-Type': 'application/json' }, body: JSON.stringify({ reviewer, expected_head_sha: expectedHeadSha }), + }).then(result => { + this.onReviewRequested?.(result); + return result; }).finally(() => { reviewRequestMutation = null; }); return reviewRequestMutation; }, @@ -509,6 +571,8 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT } createPullSheet.mergeEligibility = mergeEligibility; +createPullSheet.review = reviewEligibility; +createPullSheet.renderReviewerStatuses = renderReviewerStatuses; createPullSheet.renderFile = renderFile; createPullSheet.focusNextUnreviewed = focusNextUnreviewed; createPullSheet.removeFromSnapshot = removeFromSnapshot; diff --git a/src/gitea_proxy.py b/src/gitea_proxy.py index e6f9158..d39001b 100644 --- a/src/gitea_proxy.py +++ b/src/gitea_proxy.py @@ -2771,6 +2771,55 @@ async def pull_completion_detail(repository: str, number: int) -> dict: } +def _normalize_reviewer_statuses(pull: dict, reviews: object, head_sha: str) -> list[dict]: + """Return one bounded, current decision per reviewer without exposing raw review data.""" + latest: dict[str, dict] = {} + for review in (reviews if isinstance(reviews, list) else [])[:100]: + if not isinstance(review, dict): + continue + user = review.get("user") if isinstance(review.get("user"), dict) else {} + login = user.get("login") + state = review.get("state") + commit_id = review.get("commit_id") + if ( + not isinstance(login, str) + or not login + or state not in {"APPROVED", "REQUEST_CHANGES", "COMMENT"} + or not isinstance(commit_id, str) + or not commit_id + ): + continue + previous = latest.get(login.casefold()) + review_id = review.get("id") if isinstance(review.get("id"), int) else 0 + if previous is None or review_id >= previous["id"]: + latest[login.casefold()] = { + "id": review_id, "login": login, "state": state, "commit_id": commit_id, + } + + statuses: dict[str, dict] = {} + state_names = { + "APPROVED": "approved", "REQUEST_CHANGES": "changes_requested", "COMMENT": "commented", + } + for key, review in latest.items(): + current = review["commit_id"] == head_sha + status = state_names[review["state"]] if current else "outdated" + statuses[key] = { + "login": review["login"], + "status": status, + "head_sha": review["commit_id"], + "blocking": status in {"changes_requested", "outdated"}, + } + + requested = pull.get("requested_reviewers") if isinstance(pull, dict) else [] + for reviewer in (requested if isinstance(requested, list) else [])[:25]: + login = reviewer.get("login") if isinstance(reviewer, dict) else None + if isinstance(login, str) and login: + statuses[login.casefold()] = { + "login": login, "status": "waiting", "head_sha": head_sha, "blocking": True, + } + return sorted(statuses.values(), key=lambda item: item["login"].casefold()) + + async def pull_completion_review(repository: str, number: int) -> dict: base = f"repos/{repository}/pulls/{number}" pull = await fetch(base) @@ -2780,12 +2829,13 @@ async def pull_completion_review(repository: str, number: int) -> dict: head: dict = head_value if isinstance(head_value, dict) else {} sha_value = head.get("sha") sha = sha_value if isinstance(sha_value, str) else "" - files, status, diff_result = await asyncio.gather( + files, status, diff_result, reviews = await asyncio.gather( fetch(f"{base}/files"), fetch(f"repos/{repository}/commits/{sha}/status"), fetch_text( f"repos/{repository}/pulls/{number}.diff", REVIEW_DIFF_MAX_BYTES ), + fetch(f"{base}/reviews?limit=100"), ) diff, diff_truncated = diff_result previews = _diff_previews(diff, diff_truncated) @@ -2799,6 +2849,7 @@ async def pull_completion_review(repository: str, number: int) -> dict: "merged": pull.get("merged") is True, "ci_state": status.get("state", "unknown") if isinstance(status, dict) else "unknown", "checks": _normalize_commit_checks(status), + "reviewers": _normalize_reviewer_statuses(pull, reviews, sha), "files": [ { "filename": item.get("filename", ""), @@ -2873,7 +2924,10 @@ async def pull_check_status(repository: str, number: int) -> dict: 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") + status, reviews = await asyncio.gather( + fetch(f"repos/{repository}/commits/{sha}/status"), + fetch(f"{base}/reviews?limit=100"), + ) return { "repository": repository, "number": number, @@ -2884,6 +2938,7 @@ async def pull_check_status(repository: str, number: int) -> dict: "merged": pull.get("merged") is True, "ci_state": status.get("state", "unknown") if isinstance(status, dict) else "unknown", "checks": _normalize_commit_checks(status), + "reviewers": _normalize_reviewer_statuses(pull, reviews, sha), } @@ -2924,6 +2979,12 @@ async def merge_assigned_pull( or ci_state != "success" ): raise PullNotMergeableError("Pull request is not currently safe to merge") + reviews = await fetch(f"{base}/reviews?limit=100") + if any( + reviewer["blocking"] + for reviewer in _normalize_reviewer_statuses(pull, reviews, current_sha) + ): + raise PullNotMergeableError("Pull request review is not currently resolved") response = await _get_client().post( f"/api/v1/{base}/merge", headers=_auth(), diff --git a/tests/test_my_work.py b/tests/test_my_work.py index ce4fab7..63c3af1 100644 --- a/tests/test_my_work.py +++ b/tests/test_my_work.py @@ -7629,6 +7629,8 @@ const controller = createPullSheet({{ return new Promise(resolve => {{ finish = resolve; }}); }} }}); +const notified = []; +controller.onReviewRequested = result => notified.push(result); const item = {{repository:'stackchain/api',number:7}}; (async () => {{ const candidates = await controller.loadReviewCandidates(item); @@ -7636,7 +7638,7 @@ const item = {{repository:'stackchain/api',number:7}}; 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}})); + process.stdout.write(JSON.stringify({{calls,candidates,results,same:first===duplicate,notified}})); }})(); """ output = json.loads(subprocess.run( @@ -7656,6 +7658,7 @@ const item = {{repository:'stackchain/api',number:7}}; {"reviewer": "casey", "requested_reviewers": ["sam", "casey"]}, ], "same": True, + "notified": [{"reviewer": "casey", "requested_reviewers": ["sam", "casey"]}], } @@ -7675,6 +7678,22 @@ def test_pull_sheet_exposes_touch_safe_review_request_flow(): assert "pullController.setReviewDetail(detail)" in dashboard +def test_pull_sheet_exposes_mobile_reviewer_status_and_updated_review_action(): + 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-reviewer-statuses"' in html + assert 'id="request-updated-pull-review"' in html + assert 'id="pull-reviewer-status" class="small" aria-live="polite"' in html + assert ".pull-reviewer-status" in css + assert "overflow-wrap:anywhere" in css + assert "#request-updated-pull-review { min-height:44px;" in css + assert "createPullSheet.review(detail, pullReviewState, document)" in dashboard + assert "reviewers.some(reviewer => reviewer.status === 'outdated')" in PULL_SHEET.read_text() + + def test_pull_sheet_surfaces_pending_merge_confirmation_guidance(): script = f""" const createPullSheet = require({json.dumps(str(PULL_SHEET))}); @@ -7721,6 +7740,38 @@ process.stdout.write(JSON.stringify(states.map(createPullSheet.mergeEligibility) assert output[3]["allowed"] is False and "conflict" in output[3]["reason"].lower() +def test_pull_sheet_renders_reviewer_decisions_and_blocks_unresolved_requests(): + script = f""" +const createPullSheet = require({json.dumps(str(PULL_SHEET))}); +const base = {{state:'open', draft:false, mergeable:true, merged:false, ci_state:'success', head_sha:'new'}}; +const states = [ + [{{login:'casey', status:'waiting', head_sha:'new', blocking:true}}], + [{{login:'sam', status:'changes_requested', head_sha:'new', blocking:true}}], + [{{login:'lee', status:'outdated', head_sha:'old', blocking:true}}], + [{{login:'pat', status:'approved', head_sha:'new', blocking:false}}], +]; +process.stdout.write(JSON.stringify({{ + eligibility:states.map(reviewers => createPullSheet.mergeEligibility({{...base, reviewers}})), + html:createPullSheet.renderReviewerStatuses(states.flat(), value => String(value) + .replaceAll('&', '&').replaceAll('<', '<').replaceAll('>', '>')), +}})); +""" + output = json.loads(subprocess.run( + ["node", "-e", script], check=True, capture_output=True, text=True + ).stdout) + + assert output["eligibility"] == [ + {"allowed": False, "reason": "Waiting for @casey’s review"}, + {"allowed": False, "reason": "@sam requested changes"}, + {"allowed": False, "reason": "@lee’s review is outdated after new commits"}, + {"allowed": True, "reason": "Ready to merge"}, + ] + assert "@casey" in output["html"] and "Waiting for review" in output["html"] + assert "Changes requested" in output["html"] + assert "Outdated after new commits" in output["html"] + assert "Approved current head" in output["html"] + + def test_pull_sheet_persists_head_scoped_file_review_and_gates_merge(): 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 5bcd70b..960cd7d 100644 --- a/tests/test_pull_api.py +++ b/tests/test_pull_api.py @@ -211,8 +211,33 @@ async def test_assigned_pull_review_endpoint_loads_review_payload_on_demand(monk ] +def test_reviewer_statuses_distinguish_waiting_current_and_outdated_decisions(): + pull = { + "requested_reviewers": [{"login": "casey"}], + "head": {"sha": "new-head"}, + } + reviews = [ + {"id": 1, "user": {"login": "casey"}, "state": "APPROVED", "commit_id": "old-head"}, + {"id": 2, "user": {"login": "sam"}, "state": "REQUEST_CHANGES", "commit_id": "new-head"}, + {"id": 3, "user": {"login": "lee"}, "state": "COMMENT", "commit_id": "new-head"}, + {"id": 4, "user": {"login": "pat"}, "state": "APPROVED", "commit_id": "new-head"}, + ] + + assert gitea_proxy._normalize_reviewer_statuses(pull, reviews, "new-head") == [ + {"login": "casey", "status": "waiting", "head_sha": "new-head", "blocking": True}, + {"login": "lee", "status": "commented", "head_sha": "new-head", "blocking": False}, + {"login": "pat", "status": "approved", "head_sha": "new-head", "blocking": False}, + {"login": "sam", "status": "changes_requested", "head_sha": "new-head", "blocking": True}, + ] + + pull["requested_reviewers"] = [] + assert gitea_proxy._normalize_reviewer_statuses(pull, reviews[:1], "new-head") == [ + {"login": "casey", "status": "outdated", "head_sha": "old-head", "blocking": True}, + ] + + @pytest.mark.anyio -async def test_gitea_pull_check_status_skips_files_reviews_and_diff(): +async def test_gitea_pull_check_status_refreshes_reviewer_decisions_without_files_or_diff(): requests = [] async def handler(request): @@ -221,7 +246,10 @@ async def test_gitea_pull_check_status_skips_files_reviews_and_diff(): return httpx.Response(200, json={ "state": "open", "draft": False, "mergeable": True, "merged": False, "head": {"sha": "abc123"}, + "requested_reviewers": [{"login": "casey"}], }) + if request.url.path.endswith("/pulls/7/reviews"): + return httpx.Response(200, json=[]) if request.url.path.endswith("/commits/abc123/status"): return httpx.Response(200, json={ "state": "success", @@ -238,12 +266,16 @@ async def test_gitea_pull_check_status_skips_files_reviews_and_diff(): assert requests == [ ("GET", "/api/v1/repos/stackchain/api/pulls/7"), ("GET", "/api/v1/repos/stackchain/api/commits/abc123/status"), + ("GET", "/api/v1/repos/stackchain/api/pulls/7/reviews"), ] 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": ""}], + "reviewers": [ + {"login": "casey", "status": "waiting", "head_sha": "abc123", "blocking": True}, + ], } @@ -285,7 +317,12 @@ async def test_gitea_assigned_pull_review_includes_bounded_diff_previews(): "state": "open", "mergeable": True, "head": {"sha": "abc123"}, + "requested_reviewers": [{"login": "casey"}], }) + if path.endswith("/pulls/7/reviews"): + return httpx.Response(200, json=[ + {"id": 8, "user": {"login": "sam"}, "state": "APPROVED", "commit_id": "abc123"}, + ]) if path.endswith("/pulls/7/files"): return httpx.Response(200, json=[ {"filename": "src/api.py", "status": "modified", "additions": 1, "deletions": 1}, @@ -315,6 +352,10 @@ async def test_gitea_assigned_pull_review_includes_bounded_diff_previews(): assert "+new" in detail["files"][0]["diff_lines"] assert detail["files"][1]["diff_binary"] is True assert detail["files"][1]["diff_available"] is False + assert detail["reviewers"] == [ + {"login": "casey", "status": "waiting", "head_sha": "abc123", "blocking": True}, + {"login": "sam", "status": "approved", "head_sha": "abc123", "blocking": False}, + ] assert "conversation" not in detail @@ -678,6 +719,39 @@ async def test_gitea_merge_rejects_failed_ci_without_upstream_mutation(): ] +@pytest.mark.anyio +@pytest.mark.parametrize("requested,reviews", [ + ([{"login": "casey"}], []), + ([], [{"id": 9, "user": {"login": "casey"}, "state": "REQUEST_CHANGES", "commit_id": "abc123"}]), +]) +async def test_gitea_merge_rejects_unresolved_reviewer_status_without_mutation(requested, reviews): + requests = [] + + async def handler(request): + requests.append((request.method, request.url.path)) + if request.url.path.endswith("/pulls/7"): + return httpx.Response(200, json={ + "number": 7, "state": "open", "draft": False, "mergeable": True, + "merged": False, "head": {"sha": "abc123"}, + "requested_reviewers": requested, + }) + if request.url.path.endswith("/commits/abc123/status"): + return httpx.Response(200, json={"state": "success"}) + if request.url.path.endswith("/pulls/7/reviews"): + return httpx.Response(200, json=reviews) + raise AssertionError(f"unexpected mutation: {request.method} {request.url.path}") + + gitea_proxy.start_client(transport=httpx.MockTransport(handler)) + try: + with pytest.raises(gitea_proxy.PullNotMergeableError): + await gitea_proxy.merge_assigned_pull("stackchain/api", 7, "abc123") + finally: + await gitea_proxy.stop_client() + + assert requests[-1] == ("GET", "/api/v1/repos/stackchain/api/pulls/7/reviews") + assert all(method == "GET" for method, _path in requests) + + @pytest.mark.anyio async def test_assigned_pull_ownership_endpoints_list_handoff_and_release(monkeypatch): calls = [] @@ -920,6 +994,8 @@ async def test_gitea_merge_returns_the_exact_merge_commit_for_release_tracking() }) if request.url.path.endswith("/commits/abc123/status"): return httpx.Response(200, json={"state": "success"}) + if request.url.path.endswith("/pulls/7/reviews"): + return httpx.Response(200, json=[]) if request.url.path.endswith("/pulls/7/merge"): return httpx.Response(200, json={"merged": True, "sha": "merge456"}) raise AssertionError(request.url.path) From 3bc1d2f602a4efbb272d2458cf076c62d32fdc24 Mon Sep 17 00:00:00 2001 From: timmy Date: Mon, 24 Aug 2026 04:28:24 +0000 Subject: [PATCH 2/2] test: exercise mobile reviewer status journey --- .gitea/workflows/ci.yml | 2 +- ...est_mobile_pull_reviewer_status_release.py | 87 +++++++++++++++++++ tests/test_ci_workflow.py | 3 +- 3 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 tests/e2e/test_mobile_pull_reviewer_status_release.py diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 128df5d..30d2154 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 -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 -q release-candidate: runs-on: ubuntu-latest diff --git a/tests/e2e/test_mobile_pull_reviewer_status_release.py b/tests/e2e/test_mobile_pull_reviewer_status_release.py new file mode 100644 index 0000000..5f8a4c4 --- /dev/null +++ b/tests/e2e/test_mobile_pull_reviewer_status_release.py @@ -0,0 +1,87 @@ +from pathlib import Path + +import pytest + + +ROOT = Path(__file__).parents[2] + + +@pytest.mark.parametrize("viewport", [(320, 568), (390, 844)]) +def test_mobile_pull_reviewer_status_wraps_and_keeps_updated_review_reachable(viewport): + playwright = pytest.importorskip("playwright.sync_api") + html = (ROOT / "frontend" / "index.html").read_text() + detail = { + "state": "open", + "draft": False, + "mergeable": True, + "merged": False, + "ci_state": "success", + "head_sha": "current-head", + "reviewers": [ + { + "login": "casey-with-a-very-long-mobile-login", + "status": "waiting", + "head_sha": "current-head", + "blocking": True, + }, + { + "login": "sam", + "status": "changes_requested", + "head_sha": "current-head", + "blocking": True, + }, + { + "login": "lee", + "status": "outdated", + "head_sha": "old-head", + "blocking": True, + }, + { + "login": "pat", + "status": "approved", + "head_sha": "current-head", + "blocking": False, + }, + ], + } + + 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( + """detail => { + document.querySelector('#pull-sheet').classList.add('open'); + document.querySelector('#pull-review').open = true; + createPullSheet.review(detail, null, document); + }""", + detail, + ) + + metrics = page.evaluate( + """() => ({ + scrollWidth: document.documentElement.scrollWidth, + clientWidth: document.documentElement.clientWidth, + updatedReviewHeight: document.querySelector('#request-updated-pull-review') + .getBoundingClientRect().height, + updatedReviewVisible: !document.querySelector('#request-updated-pull-review').hidden, + statuses: Array.from(document.querySelectorAll('.pull-reviewer-status span')) + .map(node => node.textContent), + })""" + ) + browser.close() + + assert metrics["scrollWidth"] <= metrics["clientWidth"] + assert metrics["updatedReviewHeight"] >= 44 + assert metrics["updatedReviewVisible"] is True + assert metrics["statuses"] == [ + "Waiting for review", + "Changes requested", + "Outdated after new commits", + "Approved current head", + ] diff --git a/tests/test_ci_workflow.py b/tests/test_ci_workflow.py index a582f8d..5e62ece 100644 --- a/tests/test_ci_workflow.py +++ b/tests/test_ci_workflow.py @@ -73,7 +73,8 @@ def test_release_promotion_waits_for_packaged_mobile_journeys(): "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 -q" + "tests/e2e/test_mobile_detail_watch_release.py " + "tests/e2e/test_mobile_pull_reviewer_status_release.py -q" ) in browser assert "needs: [lint, build-release, browser-journey]" in release