Merge pull request 'Show requested-change feedback in mobile pull detail' (#1337) from timmy/1336-review-feedback into main

.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 -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
 
   release-candidate:
     runs-on: ubuntu-latest

frontend/dashboard.css

@@ -1261,9 +1261,19 @@ textarea { resize: vertical; min-height: 120px; }
 .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 { 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; }
+.pull-reviewer-status-heading { display:flex; justify-content:space-between; gap:10px; }
+.pull-reviewer-status-heading span { text-align:right; }
+.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; }
+.pull-review-feedback-file { margin-top:8px; padding-top:8px; border-top:1px solid #203a5c; }
+.pull-review-feedback-file h4 { margin:0 0 4px; overflow-wrap:anywhere; }
+.pull-review-feedback-file ul { margin:0; padding-left:20px; }
+.pull-review-feedback-file li { margin:6px 0; }
+.pull-review-feedback-file small { display:block; color:#93c5fd; }
+.pull-review-feedback-file button { min-height:44px; width:100%; margin-top:4px; }
 #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; }

frontend/pull-sheet.js

@@ -28,6 +28,29 @@ function mergeEligibility(detail, reviewState) {
   return { allowed: true, reason: 'Ready to merge' };
 }
 
+function renderReviewerFeedback(reviewer, escapeHtml) {
+  const groups = new Map();
+  (Array.isArray(reviewer?.comments) ? reviewer.comments : []).forEach(comment => {
+    if (!comment?.path || !comment?.body) return;
+    if (!groups.has(comment.path)) groups.set(comment.path, []);
+    groups.get(comment.path).push(comment);
+  });
+  const summary = reviewer?.summary ? '<p class="pull-review-feedback-summary">' +
+    escapeHtml(reviewer.summary) + '</p>' : '';
+  const files = Array.from(groups.entries()).map(([path, comments]) =>
+    '<section class="pull-review-feedback-file"><h4>' + escapeHtml(path) + '</h4><ul>' +
+    comments.map(comment => '<li>' + escapeHtml(comment.body) +
+      (comment.line ? '<small>Line ' + Number(comment.line) + '</small>' : '') + '</li>').join('') +
+    '</ul><button type="button" data-review-feedback-file="' + escapeHtml(path) +
+    '">View in changes</button></section>'
+  ).join('');
+  if (!summary && !files) return '';
+  const reviewedHead = reviewer?.status === 'outdated' && reviewer?.head_sha ?
+    '<p class="small">Reviewed head ' + escapeHtml(reviewer.head_sha.slice(0, 8)) + '</p>' : '';
+  return '<details class="pull-review-feedback"><summary>Review feedback</summary>' +
+    reviewedHead + summary + files + '</details>';
+}
+
 function renderReviewerStatuses(reviewers, escapeHtml) {
   const labels = {
     waiting: 'Waiting for review',
@@ -38,9 +61,10 @@ function renderReviewerStatuses(reviewers, escapeHtml) {
   };
   return (Array.isArray(reviewers) ? reviewers : []).map(reviewer => {
     const status = labels[reviewer?.status] || 'Status unavailable';
-    return '<div class="pull-reviewer-status" data-reviewer-status="' +
-      escapeHtml(reviewer?.status || 'unknown') + '"><strong>@' +
-      escapeHtml(reviewer?.login || 'unknown') + '</strong><span>' + escapeHtml(status) + '</span></div>';
+    return '<article class="pull-reviewer-status" data-reviewer-status="' +
+      escapeHtml(reviewer?.status || 'unknown') + '"><div class="pull-reviewer-status-heading"><strong>@' +
+      escapeHtml(reviewer?.login || 'unknown') + '</strong><span>' + escapeHtml(status) + '</span></div>' +
+      renderReviewerFeedback(reviewer, escapeHtml) + '</article>';
   }).join('');
 }
 
@@ -48,6 +72,9 @@ function renderReviewerPanel(doc, detail, escapeHtml = value => String(value)
   .replaceAll('&', '&amp;').replaceAll('<', '&lt;').replaceAll('>', '&gt;')) {
   const reviewers = Array.isArray(detail?.reviewers) ? detail.reviewers : [];
   doc.querySelector('#pull-reviewer-statuses').innerHTML = renderReviewerStatuses(reviewers, escapeHtml);
+  doc.querySelectorAll('[data-review-feedback-file]').forEach(button => {
+    button.addEventListener('click', () => focusPullFile(doc, button.dataset.reviewFeedbackFile));
+  });
   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.';
@@ -83,18 +110,22 @@ function renderFile(file, index, reviewed, escapeHtml) {
     '" aria-pressed="' + String(reviewed) + '">' + (reviewed ? 'Reviewed' : 'Mark reviewed') + '</button></article>';
 }
 
-function focusNextUnreviewed(doc, detail, state, controller) {
-  if (!detail || !state) return;
-  const filename = controller.nextUnreviewed(detail, state);
+function focusPullFile(doc, filename) {
   const article = Array.from(doc.querySelectorAll('#pull-files .pull-file'))
     .find(file => file.dataset.pullFilename === filename);
   const toggle = article?.querySelector('.pull-file-toggle');
   const panel = toggle && doc.getElementById(toggle.getAttribute('aria-controls'));
-  if (!toggle || !panel) return;
+  if (!toggle || !panel) return false;
   toggle.setAttribute('aria-expanded', 'true');
   panel.hidden = false;
   toggle.scrollIntoView({ block: 'center', behavior: 'smooth' });
   toggle.focus();
+  return true;
+}
+
+function focusNextUnreviewed(doc, detail, state, controller) {
+  if (!detail || !state) return;
+  focusPullFile(doc, controller.nextUnreviewed(detail, state));
 }
 
 function removeFromSnapshot(data, item) {

src/gitea_proxy.py

@@ -2771,6 +2771,49 @@ async def pull_completion_detail(repository: str, number: int) -> dict:
     }
 
 
+def _normalize_review_comments(comments: object) -> list[dict]:
+    normalized = []
+    for comment in (comments if isinstance(comments, list) else [])[:100]:
+        if not isinstance(comment, dict):
+            continue
+        path = comment.get("path")
+        body = comment.get("body")
+        if not isinstance(path, str) or not path.strip() or not isinstance(body, str) or not body.strip():
+            continue
+        item = {"path": path.strip()[:300], "body": body.strip()[:500]}
+        position = comment.get("new_position") or comment.get("old_position")
+        if isinstance(position, int) and position > 0:
+            item["line"] = position
+        normalized.append(item)
+        if len(normalized) == 20:
+            break
+    return normalized
+
+
+def _latest_feedback_review_ids(reviews: object) -> dict[str, int]:
+    latest: dict[str, tuple[int, str]] = {}
+    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")
+        review_id = review.get("id")
+        state = review.get("state")
+        if (
+            not isinstance(login, str) or not login or not isinstance(review_id, int)
+            or review_id <= 0 or not isinstance(state, str)
+        ):
+            continue
+        key = login.casefold()
+        if key not in latest or review_id >= latest[key][0]:
+            latest[key] = (review_id, state)
+    feedback = [
+        (key, review_id) for key, (review_id, state) in latest.items()
+        if state in {"REQUEST_CHANGES", "COMMENT"}
+    ]
+    return dict(feedback[:10])
+
+
 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] = {}
@@ -2792,8 +2835,10 @@ def _normalize_reviewer_statuses(pull: dict, reviews: object, head_sha: str) ->
         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"]:
+            body = review.get("body")
             latest[login.casefold()] = {
                 "id": review_id, "login": login, "state": state, "commit_id": commit_id,
+                "summary": body.strip()[:500] if isinstance(body, str) and body.strip() else "",
             }
 
     statuses: dict[str, dict] = {}
@@ -2809,6 +2854,8 @@ def _normalize_reviewer_statuses(pull: dict, reviews: object, head_sha: str) ->
             "head_sha": review["commit_id"],
             "blocking": status in {"changes_requested", "outdated"},
         }
+        if review["summary"]:
+            statuses[key]["summary"] = review["summary"]
 
     requested = pull.get("requested_reviewers") if isinstance(pull, dict) else []
     for reviewer in (requested if isinstance(requested, list) else [])[:25]:
@@ -2839,6 +2886,21 @@ async def pull_completion_review(repository: str, number: int) -> dict:
     )
     diff, diff_truncated = diff_result
     previews = _diff_previews(diff, diff_truncated)
+    reviewers = _normalize_reviewer_statuses(pull, reviews, sha)
+    feedback_ids = _latest_feedback_review_ids(reviews)
+    if feedback_ids:
+        feedback_payloads = await asyncio.gather(*(
+            fetch(f"{base}/reviews/{review_id}/comments")
+            for review_id in feedback_ids.values()
+        ))
+        feedback = {
+            login: _normalize_review_comments(payload)
+            for login, payload in zip(feedback_ids, feedback_payloads)
+        }
+        for reviewer in reviewers:
+            comments = feedback.get(reviewer["login"].casefold(), [])
+            if comments:
+                reviewer["comments"] = comments
     return {
         "repository": repository,
         "number": number,
@@ -2849,7 +2911,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),
+        "reviewers": reviewers,
         "files": [
             {
                 "filename": item.get("filename", ""),

tests/e2e/test_mobile_pull_reviewer_feedback_release.py

@@ -0,0 +1,92 @@
+from pathlib import Path
+
+import pytest
+
+
+ROOT = Path(__file__).parents[2]
+
+
+@pytest.mark.parametrize("viewport", [(320, 568), (390, 844)])
+def test_mobile_received_review_feedback_opens_matching_change(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": "sam",
+            "status": "outdated",
+            "head_sha": "old-head-123456",
+            "blocking": True,
+            "summary": "Please handle the narrow empty state before merging.",
+            "comments": [{
+                "path": "src/a/very/long/mobile/path/review_target.py",
+                "body": "Return before parsing when the payload is empty.",
+                "line": 12,
+            }],
+        }],
+        "files": [{
+            "filename": "src/a/very/long/mobile/path/review_target.py",
+            "status": "modified",
+            "additions": 2,
+            "deletions": 1,
+            "diff_available": True,
+            "diff_lines": ["@@ -10,2 +10,3 @@", "-parse(payload)", "+if not payload: return", "+parse(payload)"],
+        }],
+    }
+
+    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);
+                document.querySelector('#pull-files').innerHTML = detail.files.map((file, index) =>
+                    createPullSheet.renderFile(file, index, false, value => String(value)
+                        .replaceAll('&', '&amp;').replaceAll('<', '&lt;').replaceAll('>', '&gt;'))
+                ).join('');
+            }""",
+            detail,
+        )
+
+        page.locator(".pull-review-feedback summary").click()
+        jump = page.locator('[data-review-feedback-file]')
+        jump.click()
+        metrics = page.evaluate(
+            """() => {
+                const jump = document.querySelector('[data-review-feedback-file]');
+                const toggle = document.querySelector('.pull-file-toggle');
+                const panel = document.getElementById(toggle.getAttribute('aria-controls'));
+                return {
+                    scrollWidth: document.documentElement.scrollWidth,
+                    clientWidth: document.documentElement.clientWidth,
+                    jumpHeight: jump.getBoundingClientRect().height,
+                    expanded: toggle.getAttribute('aria-expanded'),
+                    panelHidden: panel.hidden,
+                    focused: document.activeElement === toggle,
+                    reviewedHead: document.querySelector('.pull-review-feedback').textContent,
+                    updatedReviewVisible: !document.querySelector('#request-updated-pull-review').hidden,
+                };
+            }"""
+        )
+        browser.close()
+
+    assert metrics["scrollWidth"] <= metrics["clientWidth"]
+    assert metrics["jumpHeight"] >= 44
+    assert metrics["expanded"] == "true"
+    assert metrics["panelHidden"] is False
+    assert metrics["focused"] is True
+    assert "Reviewed head old-head" in metrics["reviewedHead"]
+    assert metrics["updatedReviewVisible"] is True

tests/test_ci_workflow.py

@@ -74,7 +74,8 @@ def test_release_promotion_waits_for_packaged_mobile_journeys():
         "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"
+        "tests/e2e/test_mobile_pull_reviewer_status_release.py "
+        "tests/e2e/test_mobile_pull_reviewer_feedback_release.py -q"
     ) in browser
     assert "needs: [lint, build-release, browser-journey]" in release
 

tests/test_my_work.py

@@ -7772,6 +7772,32 @@ process.stdout.write(JSON.stringify({{
     assert "Approved current head" in output["html"]
 
 
+def test_pull_sheet_renders_bounded_feedback_grouped_by_file():
+    script = f"""
+const createPullSheet = require({json.dumps(str(PULL_SHEET))});
+const html = createPullSheet.renderReviewerStatuses([{{
+  login:'sam', status:'changes_requested', head_sha:'abc123', blocking:true,
+  summary:'Please <split> this helper.',
+  comments:[
+    {{path:'src/api.py', body:'Handle <empty>.', line:12}},
+    {{path:'src/api.py', body:'Keep the return explicit.'}},
+    {{path:'frontend/app.js', body:'Show the error.'}},
+  ],
+}}], value => String(value).replaceAll('&', '&amp;').replaceAll('<', '&lt;').replaceAll('>', '&gt;'));
+process.stdout.write(html);
+"""
+    html = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True).stdout
+
+    assert "Review feedback" in html
+    assert "Please &lt;split&gt; this helper." in html
+    assert html.count('data-review-feedback-file="src/api.py"') == 1
+    assert 'data-review-feedback-file="frontend/app.js"' in html
+    assert "Handle &lt;empty&gt;." in html
+    assert "Line 12" in html
+    assert "View in changes" in html
+    assert "Please <split>" not in html
+
+
 def test_pull_sheet_persists_head_scoped_file_review_and_gates_merge():
     script = f"""
 const createPullSheet = require({json.dumps(str(PULL_SHEET))});

tests/test_pull_api.py

@@ -236,6 +236,68 @@ def test_reviewer_statuses_distinguish_waiting_current_and_outdated_decisions():
     ]
 
 
+def test_reviewer_status_exposes_only_bounded_nonempty_feedback_summary():
+    pull = {"requested_reviewers": []}
+    reviews = [
+        {
+            "id": 8,
+            "user": {"login": "sam"},
+            "state": "REQUEST_CHANGES",
+            "commit_id": "abc123",
+            "body": "Please split this helper. " + ("x" * 900),
+        },
+        {
+            "id": 9,
+            "user": {"login": "lee"},
+            "state": "COMMENT",
+            "commit_id": "abc123",
+            "body": "   ",
+        },
+    ]
+
+    statuses = gitea_proxy._normalize_reviewer_statuses(pull, reviews, "abc123")
+
+    sam = next(item for item in statuses if item["login"] == "sam")
+    lee = next(item for item in statuses if item["login"] == "lee")
+    assert sam["summary"].startswith("Please split this helper.")
+    assert len(sam["summary"]) == 500
+    assert "summary" not in lee
+
+
+def test_review_comments_are_bounded_and_require_a_file_and_body():
+    comments = [
+        {"id": 1, "path": "src/api.py", "body": "Handle the empty state", "new_position": 4},
+        {"id": 2, "path": "src/api.py", "body": "x" * 900, "old_position": 7},
+        {"id": 3, "path": "", "body": "missing file"},
+        {"id": 4, "path": "src/ignored.py", "body": "   "},
+        "malformed",
+    ] + [
+        {"id": index, "path": f"src/{index}.py", "body": "bounded"}
+        for index in range(5, 40)
+    ]
+
+    normalized = gitea_proxy._normalize_review_comments(comments)
+
+    assert len(normalized) == 20
+    assert normalized[0] == {
+        "path": "src/api.py", "body": "Handle the empty state", "line": 4,
+    }
+    assert normalized[1]["line"] == 7
+    assert len(normalized[1]["body"]) == 500
+    assert all(item["path"] and item["body"] for item in normalized)
+
+
+def test_inline_feedback_is_loaded_only_for_each_reviewers_latest_decision():
+    reviews = [
+        {"id": 7, "user": {"login": "sam"}, "state": "REQUEST_CHANGES"},
+        {"id": 8, "user": {"login": "sam"}, "state": "APPROVED"},
+        {"id": 9, "user": {"login": "lee"}, "state": "COMMENT"},
+        {"id": 10, "user": {"login": "pat"}, "state": "PENDING"},
+    ]
+
+    assert gitea_proxy._latest_feedback_review_ids(reviews) == {"lee": 9}
+
+
 @pytest.mark.anyio
 async def test_gitea_pull_check_status_refreshes_reviewer_decisions_without_files_or_diff():
     requests = []
@@ -359,6 +421,50 @@ async def test_gitea_assigned_pull_review_includes_bounded_diff_previews():
     assert "conversation" not in detail
 
 
+@pytest.mark.anyio
+async def test_assigned_pull_review_includes_latest_inline_feedback():
+    requests = []
+
+    async def handler(request):
+        path = request.url.path
+        requests.append(path)
+        if path.endswith("/pulls/7"):
+            return httpx.Response(200, json={
+                "state": "open", "mergeable": True, "head": {"sha": "abc123"},
+                "requested_reviewers": [],
+            })
+        if path.endswith("/pulls/7/reviews"):
+            return httpx.Response(200, json=[
+                {"id": 7, "user": {"login": "sam"}, "state": "COMMENT", "commit_id": "old-head"},
+                {"id": 8, "user": {"login": "sam"}, "state": "REQUEST_CHANGES",
+                 "commit_id": "abc123", "body": "Please handle the empty state."},
+            ])
+        if path.endswith("/pulls/7/reviews/8/comments"):
+            return httpx.Response(200, json=[
+                {"path": "src/api.py", "body": "Return before parsing.", "new_position": 12},
+            ])
+        if path.endswith("/pulls/7/files"):
+            return httpx.Response(200, json=[{"filename": "src/api.py", "status": "modified"}])
+        if path.endswith("/commits/abc123/status"):
+            return httpx.Response(200, json={"state": "success"})
+        if path.endswith("/pulls/7.diff"):
+            return httpx.Response(200, text="")
+        raise AssertionError(f"unexpected request: {request.method} {path}")
+
+    gitea_proxy.start_client(transport=httpx.MockTransport(handler))
+    try:
+        detail = await gitea_proxy.pull_completion_review("stackchain/api", 7)
+    finally:
+        await gitea_proxy.stop_client()
+
+    assert "/api/v1/repos/stackchain/api/pulls/7/reviews/7/comments" not in requests
+    assert detail["reviewers"] == [{
+        "login": "sam", "status": "changes_requested", "head_sha": "abc123", "blocking": True,
+        "summary": "Please handle the empty state.",
+        "comments": [{"path": "src/api.py", "body": "Return before parsing.", "line": 12}],
+    }]
+
+
 @pytest.mark.anyio
 async def test_assigned_pull_conversation_endpoint_reuses_issue_thread_with_pull_authorization(monkeypatch):
     calls = []