feat: load pull review feedback on demand (Closes #1342)

tests/e2e/test_mobile_pull_reviewer_feedback_release.py

@@ -90,3 +90,83 @@ def test_mobile_received_review_feedback_opens_matching_change(viewport):
     assert metrics["focused"] is True
     assert "Reviewed head old-head" in metrics["reviewedHead"]
     assert metrics["updatedReviewVisible"] is True
+
+
+@pytest.mark.parametrize("viewport", [(320, 568), (390, 844)])
+def test_mobile_pull_review_retries_inline_feedback_without_losing_core_review(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": "abc1234",
+        "reviewers": [{
+            "login": "sam", "review_id": 8, "status": "changes_requested",
+            "head_sha": "abc1234", "blocking": True,
+            "summary": "Please handle the empty state.",
+        }],
+        "files": [],
+    }
+
+    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 => {
+                window.lazyFeedbackDetail = detail;
+                window.lazyFeedbackItem = {repository:'stackchain/api', number:7, key:'stackchain/api#7'};
+                let attempts = 0;
+                const controller = createPullSheet({
+                    storage:null,
+                    fetchJson:() => {
+                        attempts += 1;
+                        if (attempts === 1) return Promise.reject(new Error('feedback unavailable'));
+                        return Promise.resolve({
+                            review_id:8, head_sha:'abc1234', reviewed_head_sha:'abc1234',
+                            comments:[{id:99, path:'src/api.py', body:'Return before parsing.', line:12}],
+                        });
+                    },
+                });
+                createPullSheet.bindFeedbackControls(
+                    document, controller, () => window.lazyFeedbackItem,
+                    () => window.lazyFeedbackDetail, () => 'timmy'
+                );
+                document.querySelector('#pull-sheet').classList.add('open');
+                document.querySelector('#pull-review').open = true;
+                const eligibility = createPullSheet.review(detail, null, document);
+                document.querySelector('#pull-merge-state').textContent = eligibility.reason;
+            }""",
+            detail,
+        )
+
+        page.locator(".pull-review-feedback summary").click()
+        load = page.get_by_role("button", name="Load inline feedback")
+        load.click()
+        retry = page.get_by_role("button", name="Retry feedback")
+        retry.wait_for()
+        retry.click()
+        page.get_by_text("Return before parsing.").wait_for()
+        metrics = page.evaluate(
+            """() => {
+                const details = document.querySelector('.pull-review-feedback');
+                const address = document.querySelector('[data-address-review-feedback]');
+                return {
+                    scrollWidth: document.documentElement.scrollWidth,
+                    clientWidth: document.documentElement.clientWidth,
+                    blocker: document.querySelector('#pull-merge-state').textContent,
+                    feedback: details.textContent,
+                    addressHeight: address.getBoundingClientRect().height,
+                };
+            }"""
+        )
+        browser.close()
+
+    assert metrics["scrollWidth"] <= metrics["clientWidth"]
+    assert "requested changes" in metrics["blocker"]
+    assert "Return before parsing." in metrics["feedback"]
+    assert metrics["addressHeight"] >= 44

tests/test_my_work.py

@@ -7867,6 +7867,38 @@ process.stdout.write(html);
     assert "Please <split>" not in html
 
 
+def test_pull_sheet_marks_inline_feedback_as_on_demand_and_retryable():
+    script = f"""
+const createPullSheet = require({json.dumps(str(PULL_SHEET))});
+const render = state => createPullSheet.renderReviewerStatuses([{{
+  login:'sam', review_id:8, status:'changes_requested', head_sha:'abc1234',
+  blocking:true, summary:'Please fix this.', feedback_state:state,
+}}], value => String(value));
+process.stdout.write(JSON.stringify({{idle:render(undefined), failed:render('error')}}));
+"""
+    result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True)
+    output = json.loads(result.stdout)
+
+    assert "Load inline feedback" in output["idle"]
+    assert 'data-load-review-feedback="8"' in output["idle"]
+    assert "Feedback unavailable" in output["failed"]
+    assert "Retry feedback" in output["failed"]
+    assert "Address feedback" not in output["idle"]
+
+
+def test_pull_sheet_binds_on_demand_feedback_with_visible_retry_state():
+    script = PULL_SHEET.read_text()
+
+    assert "event.target.closest?.('[data-load-review-feedback]')" in script
+    assert "await controller.loadFeedback(item, detail, reviewer" in script
+    assert "reviewer.feedback_state = 'loading'" in script
+    assert "reviewer.feedback_state = 'loaded'" in script
+    assert "reviewer.feedback_state = 'error'" in script
+    assert "result.head_sha !== detail.head_sha" in script
+    css = (Path(__file__).parents[1] / "frontend" / "dashboard.css").read_text()
+    assert "[data-load-review-feedback] { min-height:44px; width:100%;" in css
+
+
 def test_pull_sheet_persists_head_scoped_file_review_and_gates_merge():
     script = f"""
 const createPullSheet = require({json.dumps(str(PULL_SHEET))});
@@ -7942,6 +7974,37 @@ Promise.allSettled([first, concurrent]).then(async failed => {{
     ]
 
 
+def test_pull_sheet_feedback_is_single_flight_cached_by_head_and_review():
+    script = f"""
+const createPullSheet = require({json.dumps(str(PULL_SHEET))});
+const calls = [];
+let resolveFeedback;
+const sheet = createPullSheet({{storage:null, fetchJson:url => {{
+  calls.push(url);
+  return new Promise(resolve => {{ resolveFeedback = resolve; }});
+}}}});
+const item = {{repository:'stackchain/api', number:7}};
+const detail = {{head_sha:'abc1234'}};
+const reviewer = {{review_id:8, head_sha:'abc1234'}};
+const first = sheet.loadFeedback(item, detail, reviewer);
+const concurrent = sheet.loadFeedback(item, detail, reviewer);
+resolveFeedback({{review_id:8, head_sha:'abc1234', reviewed_head_sha:'abc1234', comments:[{{id:99}}]}});
+Promise.all([first, concurrent]).then(async results => {{
+  const cached = await sheet.loadFeedback(item, detail, reviewer);
+  process.stdout.write(JSON.stringify({{same:first === concurrent, results, cached, calls}}));
+}});
+"""
+    result = subprocess.run(["node", "-e", script], capture_output=True, text=True)
+
+    assert result.returncode == 0, result.stderr
+    output = json.loads(result.stdout)
+    assert output["same"] is True
+    assert output["cached"] == output["results"][0]
+    assert output["calls"] == [
+        "api/v1/repos/stackchain/api/pulls/7/reviews/8/feedback?expected_head_sha=abc1234"
+    ]
+
+
 def test_pull_sheet_refreshes_status_only_without_clearing_review_progress():
     script = f"""
 const createPullSheet = require({json.dumps(str(PULL_SHEET))});

tests/test_pull_api.py

@@ -211,6 +211,38 @@ async def test_assigned_pull_review_endpoint_loads_review_payload_on_demand(monk
     ]
 
 
+@pytest.mark.anyio
+async def test_assigned_pull_feedback_endpoint_authorizes_and_returns_one_review(monkeypatch):
+    calls = []
+
+    async def assigned(repository, number):
+        calls.append(("assigned", repository, number))
+        return True
+
+    async def feedback(repository, number, review_id, expected_head_sha):
+        calls.append(("feedback", repository, number, review_id, expected_head_sha))
+        return {
+            "review_id": review_id, "head_sha": expected_head_sha,
+            "reviewed_head_sha": expected_head_sha, "comments": [],
+        }
+
+    monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned)
+    monkeypatch.setattr(main.gitea_proxy, "pull_review_feedback", feedback, raising=False)
+    transport = httpx.ASGITransport(app=main.app)
+    async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
+        response = await client.get(
+            "/api/v1/repos/stackchain/api/pulls/7/reviews/8/feedback?expected_head_sha=abc1234"
+        )
+
+    assert response.status_code == 200
+    assert response.headers["cache-control"] == "no-store"
+    assert response.json()["review_id"] == 8
+    assert calls == [
+        ("assigned", "stackchain/api", 7),
+        ("feedback", "stackchain/api", 7, 8, "abc1234"),
+    ]
+
+
 def test_reviewer_statuses_distinguish_waiting_current_and_outdated_decisions():
     pull = {
         "requested_reviewers": [{"login": "casey"}],
@@ -448,7 +480,7 @@ async def test_gitea_assigned_pull_review_includes_bounded_diff_previews():
 
 
 @pytest.mark.anyio
-async def test_assigned_pull_review_includes_latest_inline_feedback():
+async def test_assigned_pull_review_does_not_block_on_inline_feedback():
     requests = []
 
     async def handler(request):
@@ -465,17 +497,13 @@ async def test_assigned_pull_review_includes_latest_inline_feedback():
                 {"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=[
-                {"id": 99, "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}")
+        raise AssertionError(f"optional feedback must not block core review: {request.method} {path}")
 
     gitea_proxy.start_client(transport=httpx.MockTransport(handler))
     try:
@@ -483,14 +511,50 @@ async def test_assigned_pull_review_includes_latest_inline_feedback():
     finally:
         await gitea_proxy.stop_client()
 
-    assert "/api/v1/repos/stackchain/api/pulls/7/reviews/7/comments" not in requests
+    assert not any(path.endswith("/comments") for path in requests)
     assert detail["reviewers"] == [{
         "review_id": 8, "login": "sam", "status": "changes_requested", "head_sha": "abc123", "blocking": True,
         "summary": "Please handle the empty state.",
-        "comments": [{"id": 99, "path": "src/api.py", "body": "Return before parsing.", "line": 12}],
     }]
 
 
+@pytest.mark.anyio
+async def test_pull_review_feedback_loads_one_current_bounded_review():
+    requests = []
+
+    async def handler(request):
+        path = request.url.path
+        requests.append(path)
+        if path.endswith("/pulls/7"):
+            return httpx.Response(200, json={"head": {"sha": "abc123"}})
+        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"},
+            ])
+        if path.endswith("/pulls/7/reviews/8/comments"):
+            return httpx.Response(200, json=[
+                {"id": 99, "path": "src/api.py", "body": "Return before parsing.", "new_position": 12},
+            ])
+        raise AssertionError(f"unexpected request: {request.method} {path}")
+
+    gitea_proxy.start_client(transport=httpx.MockTransport(handler))
+    try:
+        feedback = await gitea_proxy.pull_review_feedback(
+            "stackchain/api", 7, 8, "abc123"
+        )
+    finally:
+        await gitea_proxy.stop_client()
+
+    assert requests[-1].endswith("/pulls/7/reviews/8/comments")
+    assert feedback == {
+        "review_id": 8,
+        "head_sha": "abc123",
+        "reviewed_head_sha": "abc123",
+        "comments": [{"id": 99, "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 = []