Merge pull request 'Request teammate review from mobile pull detail' (#1331) from timmy/1330-request-teammate-review into main

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, .pull-ownership, .pull-review-request { 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 > div, .pull-ownership > div, .pull-review-request > 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 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; }

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.');

frontend/index.html

@@ -1720,6 +1720,16 @@
       <div id="pull-files"></div>
       <button id="merge-pull" type="button" disabled>Merge</button>
     </details>
+    <details class="pull-review-request" id="pull-review-request">
+      <summary>Request review</summary>
+      <div>
+        <button id="load-pull-reviewers" type="button">Choose reviewer</button>
+        <label for="pull-review-recipient" class="small">Eligible repository reviewer</label>
+        <select id="pull-review-recipient" disabled><option value="">Select a teammate</option></select>
+        <button id="confirm-pull-review" type="button" disabled>Request review</button>
+        <div id="pull-review-request-status" class="small" aria-live="assertive">Load teammates to request a review.</div>
+      </div>
+    </details>
     <details class="pull-ownership" id="pull-ownership">
       <summary>Ownership</summary>
       <div>

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 = '<option value="">Select a teammate</option>';
+  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;

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:

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)

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))});

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):