feat: address requested review feedback (Closes #1338)

tests/e2e/test_mobile_address_review_feedback_release.py

@@ -0,0 +1,133 @@
+from pathlib import Path
+
+import pytest
+
+
+ROOT = Path(__file__).parents[2]
+
+
+@pytest.mark.parametrize("viewport", [(320, 568), (390, 844)])
+def test_mobile_author_addresses_every_review_comment_and_requests_updated_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": "current-head",
+        "reviewers": [{
+            "review_id": 42,
+            "login": "sam",
+            "status": "changes_requested",
+            "head_sha": "current-head",
+            "blocking": True,
+            "comments": [
+                {"id": 99, "path": "src/api.py", "body": "Handle the empty state.", "line": 12},
+                {"id": 100, "path": "frontend/app.js", "body": "Explain the retry.", "line": 8},
+            ],
+        }],
+        "files": [
+            {"filename": "src/api.py", "status": "modified", "diff_available": True,
+             "diff_lines": ["@@ -11 +11 @@", "+return empty"]},
+            {"filename": "frontend/app.js", "status": "modified", "diff_available": True,
+             "diff_lines": ["@@ -7 +7 @@", "+showRetry()"]},
+        ],
+    }
+
+    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 => {
+                globalThis.confirm = () => true;
+                globalThis.feedbackCalls = [];
+                globalThis.feedbackStored = new Map();
+                globalThis.feedbackStorage = {
+                    getItem:key => feedbackStored.has(key) ? feedbackStored.get(key) : null,
+                    setItem:(key, value) => feedbackStored.set(key, String(value)),
+                    removeItem:key => feedbackStored.delete(key),
+                };
+                globalThis.feedbackItem = {repository:'stackchain/api', number:7, key:'stackchain/api#7'};
+                globalThis.feedbackDetail = detail;
+                globalThis.feedbackController = createPullSheet({
+                    storage: feedbackStorage,
+                    createOperationId: () => 'feedback-operation',
+                    fetchJson: async (path, options = {}) => {
+                        feedbackCalls.push({path, options});
+                        if (path.endsWith('/comments')) return {id: 501, body:JSON.parse(options.body).body};
+                        if (path.endsWith('/request-review')) return {
+                            number:7, reviewer:'sam', head_sha:'current-head', requested_reviewers:['sam']
+                        };
+                        throw new Error('unexpected request');
+                    },
+                });
+                document.querySelector('#pull-sheet').classList.add('open');
+                document.querySelector('#pull-review').open = true;
+                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('');
+                createPullSheet.bindFeedbackControls(document, feedbackController,
+                    () => feedbackItem, () => feedbackDetail, () => 'timmy');
+                createPullSheet.review(detail, null, document);
+            }""",
+            detail,
+        )
+
+        page.locator(".pull-review-feedback summary").click()
+        page.locator('[data-address-review-feedback="42"]').click()
+        assert page.locator("#pull-feedback-progress").text_content() == "Comment 1 of 2"
+        assert page.locator("#finish-pull-feedback").is_disabled()
+        page.locator('[data-feedback-disposition="addressed"]').click()
+        page.locator("#next-pull-feedback").click()
+        page.locator("#pull-feedback-note").fill("Added a visible retry message.")
+        page.locator('[data-feedback-disposition="discussion"]').click()
+        assert not page.locator("#finish-pull-feedback").is_disabled()
+        page.locator("#finish-pull-feedback").click()
+        page.locator("#request-feedback-review").click()
+
+        page.wait_for_function("feedbackCalls.length === 2")
+        result = page.evaluate(
+            """() => ({
+                calls:feedbackCalls,
+                scrollWidth:document.documentElement.scrollWidth,
+                clientWidth:document.documentElement.clientWidth,
+                controlHeights:Array.from(document.querySelectorAll('#pull-feedback-pass button'))
+                    .filter(button => !button.hidden).map(button => button.getBoundingClientRect().height),
+                status:document.querySelector('#pull-feedback-status').textContent,
+                secondExpanded:document.querySelector('[data-pull-filename="frontend/app.js"] .pull-file-toggle')
+                    .getAttribute('aria-expanded'),
+                persisted:Array.from(feedbackStored.keys()).some(key => key.includes('feedback-pass')),
+            })"""
+        )
+        result["resetHidden"] = page.evaluate(
+            """() => {
+                createPullSheet.resetReviewRequestControls(document, feedbackDetail);
+                return document.querySelector('#pull-feedback-pass').hidden;
+            }"""
+        )
+        browser.close()
+
+    assert result["scrollWidth"] <= result["clientWidth"]
+    assert min(result["controlHeights"]) >= 44
+    assert result["secondExpanded"] == "true"
+    assert result["persisted"] is True
+    assert result["resetHidden"] is True
+    assert result["calls"][0]["path"].endswith("/comments")
+    assert result["calls"][0]["options"]["headers"]["Idempotency-Key"] == "feedback-operation"
+    summary = result["calls"][0]["options"]["body"]
+    assert "Handle the empty state" in summary
+    assert "Addressed" in summary
+    assert "Needs discussion" in summary
+    assert "Added a visible retry message" in summary
+    assert result["calls"][1]["path"].endswith("/request-review")
+    assert '"reviewer":"sam"' in result["calls"][1]["options"]["body"]
+    assert result["status"] == "Response posted; updated review requested from @sam."

tests/test_pull_api.py

@@ -225,14 +225,14 @@ def test_reviewer_statuses_distinguish_waiting_current_and_outdated_decisions():
 
     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},
+        {"review_id": 3, "login": "lee", "status": "commented", "head_sha": "new-head", "blocking": False},
+        {"review_id": 4, "login": "pat", "status": "approved", "head_sha": "new-head", "blocking": False},
+        {"review_id": 2, "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},
+        {"review_id": 1, "login": "casey", "status": "outdated", "head_sha": "old-head", "blocking": True},
     ]
 
 
@@ -280,13 +280,39 @@ def test_review_comments_are_bounded_and_require_a_file_and_body():
 
     assert len(normalized) == 20
     assert normalized[0] == {
-        "path": "src/api.py", "body": "Handle the empty state", "line": 4,
+        "id": 1, "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_review_feedback_preserves_stable_review_and_comment_identities():
+    pull = {"requested_reviewers": []}
+    reviews = [{
+        "id": 42,
+        "user": {"login": "sam"},
+        "state": "REQUEST_CHANGES",
+        "commit_id": "abc123",
+    }]
+
+    statuses = gitea_proxy._normalize_reviewer_statuses(pull, reviews, "abc123")
+    comments = gitea_proxy._normalize_review_comments([
+        {"id": 99, "path": "src/api.py", "body": "Handle the empty state", "new_position": 4},
+        {"id": -1, "path": "src/other.py", "body": "Invalid identity is omitted"},
+    ])
+
+    assert statuses == [{
+        "review_id": 42,
+        "login": "sam",
+        "status": "changes_requested",
+        "head_sha": "abc123",
+        "blocking": True,
+    }]
+    assert comments[0]["id"] == 99
+    assert "id" not in comments[1]
+
+
 def test_inline_feedback_is_loaded_only_for_each_reviewers_latest_decision():
     reviews = [
         {"id": 7, "user": {"login": "sam"}, "state": "REQUEST_CHANGES"},
@@ -416,7 +442,7 @@ async def test_gitea_assigned_pull_review_includes_bounded_diff_previews():
     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},
+        {"review_id": 8, "login": "sam", "status": "approved", "head_sha": "abc123", "blocking": False},
     ]
     assert "conversation" not in detail
 
@@ -441,7 +467,7 @@ async def test_assigned_pull_review_includes_latest_inline_feedback():
             ])
         if path.endswith("/pulls/7/reviews/8/comments"):
             return httpx.Response(200, json=[
-                {"path": "src/api.py", "body": "Return before parsing.", "new_position": 12},
+                {"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"}])
@@ -459,9 +485,9 @@ async def test_assigned_pull_review_includes_latest_inline_feedback():
 
     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,
+        "review_id": 8, "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}],
+        "comments": [{"id": 99, "path": "src/api.py", "body": "Return before parsing.", "line": 12}],
     }]