feat: show requested-change feedback (Closes #1336)

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 = []