feat: apply review suggestions across files
CI / lint (pull_request) Successful in 3m32s
CI / build-release (pull_request) Successful in 7s
CI / browser-journey (pull_request) Successful in 6m13s
CI / release-candidate (pull_request) Has been skipped

Closes #1386
This commit is contained in:
timmy committed 2026-08-25 08:02:16 +00:00
1 parent 7bb22e3525
commit 1c02dcb217
6 files changed
+238 -53

No files matched your search

@@ -7,7 +7,7 @@ ROOT = Path(__file__).parents[2]
@pytest.mark.parametrize("viewport", [(320, 568), (390, 844)])
def test_mobile_author_stages_and_commits_two_same_file_suggestions_atomically(viewport):
def test_mobile_author_stages_and_commits_cross_file_suggestions_atomically(viewport):
playwright = pytest.importorskip("playwright.sync_api")
html = (ROOT / "frontend" / "index.html").read_text()
detail = {
@@ -19,14 +19,15 @@ def test_mobile_author_stages_and_commits_two_same_file_suggestions_atomically(v
"comments": [
{"id": 99, "path": "src/api.py", "line": 2, "body": "Use the typed empty result.",
"suggestion": {"line": 2, "content": " return empty_result()"}},
{"id": 100, "path": "src/api.py", "line": 3, "body": "Keep the result typed.",
"suggestion": {"line": 3, "content": " return typed_result()"}},
{"id": 100, "path": "tests/test_api.py", "line": 2, "body": "Prove the typed result.",
"suggestion": {"line": 2, "content": " assert lookup() == empty_result()"}},
],
}],
"files": [
{"filename": "src/api.py", "status": "modified", "diff_available": True,
"diff_lines": ["@@ -2 +2 @@", "+ return None"]},
{"filename": "tests/test_api.py", "status": "modified", "diff_available": True,
"diff_lines": ["@@ -2 +2 @@", "+ assert lookup() is None"]},
],
}
@@ -48,10 +49,13 @@ def test_mobile_author_stages_and_commits_two_same_file_suggestions_atomically(v
globalThis.batchController = createPullSheet({fetchJson: async (path, options = {}) => {
batchCalls.push({path, options});
if ((options.method || 'GET') === 'GET') {
return {path:'src/api.py', head_sha:'abc1234', blob_sha:'blob1',
content:'def lookup():\\n return None\\n return result\\n'};
if (batchCalls.length === 1) return {path:'src/api.py', head_sha:'abc1234', blob_sha:'blob1',
content:'def lookup():\\n return None\\n'};
return {path:'tests/test_api.py', head_sha:'abc1234', blob_sha:'blob2',
content:'def test_lookup():\\n assert lookup() is None\\n'};
}
return {path:'src/api.py', suggestion_count:2, previous_head_sha:'abc1234', head_sha:'def5678'};
return {paths:['src/api.py','tests/test_api.py'], suggestion_count:2,
previous_head_sha:'abc1234', head_sha:'def5678'};
}});
document.querySelector('#pull-sheet').classList.add('open');
document.querySelector('#pull-review').open = true;
@@ -78,7 +82,7 @@ def test_mobile_author_stages_and_commits_two_same_file_suggestions_atomically(v
assert page.locator("#review-pull-feedback-batch").text_content() == "Review fixes (2)"
page.locator("#review-pull-feedback-batch").click()
assert page.locator("#pull-feedback-batch-review").is_visible()
assert page.locator("#pull-feedback-batch-items li").count() == 1
assert page.locator("#pull-feedback-batch-items li").count() == 2
page.locator("#commit-pull-feedback-batch").click()
page.wait_for_function("batchCalls.length === 3")
result = page.evaluate("""() => ({
@@ -102,7 +106,8 @@ def test_mobile_author_stages_and_commits_two_same_file_suggestions_atomically(v
assert request["options"]["method"] == "POST"
body = request["options"]["body"]
assert '"expected_head_sha":"abc1234"' in body
assert '"file":{"path":"src/api.py"' in body
assert '"files":[{"path":"src/api.py"' in body
assert '"path":"tests/test_api.py"' in body
assert '"suggestion_count":2' in body
assert 'return empty_result()' in body
assert 'return typed_result()' in body
assert 'assert lookup() == empty_result()' in body
+92 -16
View File
@@ -109,7 +109,7 @@ async def test_gitea_commits_feedback_fix_and_verifies_advanced_pull_head():
@pytest.mark.anyio
async def test_gitea_commits_multiple_same_file_suggestions_as_one_commit():
async def test_gitea_commits_cross_file_suggestions_as_one_atomic_commit():
requests = []
pull = {
"number": 7, "state": "open", "merged": False,
@@ -117,6 +117,8 @@ async def test_gitea_commits_multiple_same_file_suggestions_as_one_commit():
"head": {"sha": "abc1234", "ref": "alex/review-fix", "repo": {"full_name": "stackchain/api"}},
"base": {"repo": {"full_name": "stackchain/api"}},
}
originals = {"src/api.py": ("return empty\n", "blob1"), "tests/test_api.py": ("assert empty\n", "blob2")}
updated = {"src/api.py": ("return handled\n", "blob3"), "tests/test_api.py": ("assert handled\n", "blob4")}
async def handler(request):
requests.append((request.method, request.url.path))
@@ -124,16 +126,23 @@ async def test_gitea_commits_multiple_same_file_suggestions_as_one_commit():
return httpx.Response(200, json={"login": "alex"})
if request.url.path == "/api/v1/repos/stackchain/api/pulls/7":
return httpx.Response(200, json=pull)
if request.url.path == "/api/v1/repos/stackchain/api/contents/src/api.py" and request.method == "GET":
updated = request.url.params["ref"] == "def5678"
return httpx.Response(200, json=_content_payload(
"first fixed\nsecond fixed\n" if updated else "first\nsecond\n",
"blob2" if updated else "blob1",
))
if request.url.path == "/api/v1/repos/stackchain/api/contents/src/api.py" and request.method == "PUT":
prefix = "/api/v1/repos/stackchain/api/contents/"
if request.url.path.startswith(prefix) and request.method == "GET":
path = request.url.path.removeprefix(prefix)
content, sha = (updated if request.url.params["ref"] == "def5678" else originals)[path]
return httpx.Response(200, json=_content_payload(content, sha))
if request.url.path == "/api/v1/repos/stackchain/api/contents" and request.method == "POST":
body = json.loads(request.content)
assert body["branch"] == "alex/review-fix"
assert base64.b64decode(body["content"]).decode() == "first fixed\nsecond fixed\n"
assert body == {
"branch": "alex/review-fix",
"message": "fix: apply review suggestions",
"files": [
{"operation": "update", "path": "src/api.py", "sha": "blob1",
"content": base64.b64encode(b"return handled\n").decode()},
{"operation": "update", "path": "tests/test_api.py", "sha": "blob2",
"content": base64.b64encode(b"assert handled\n").decode()},
],
}
pull["head"]["sha"] = "def5678"
return httpx.Response(200, json={"commit": {"sha": "def5678"}})
raise AssertionError(f"unexpected request: {request.method} {request.url}")
@@ -142,17 +151,23 @@ async def test_gitea_commits_multiple_same_file_suggestions_as_one_commit():
try:
result = await gitea_proxy.commit_authored_pull_feedback_batch(
"stackchain/api", 7,
{"path": "src/api.py", "content": "first fixed\nsecond fixed\n", "expected_blob_sha": "blob1"},
[
{"path": "src/api.py", "content": "return handled\n", "expected_blob_sha": "blob1"},
{"path": "tests/test_api.py", "content": "assert handled\n", "expected_blob_sha": "blob2"},
],
2, "fix: apply review suggestions", "abc1234",
)
finally:
await gitea_proxy.stop_client()
assert result["previous_head_sha"] == "abc1234"
assert result["head_sha"] == "def5678"
assert result["path"] == "src/api.py"
assert result["suggestion_count"] == 2
assert len([request for request in requests if request[0] == "PUT"]) == 1
assert result == {
"repository": "stackchain/api", "number": 7,
"paths": ["src/api.py", "tests/test_api.py"],
"previous_head_sha": "abc1234", "head_sha": "def5678",
"suggestion_count": 2, "message": "fix: apply review suggestions",
}
assert len([request for request in requests if request == ("POST", "/api/v1/repos/stackchain/api/contents")]) == 1
assert not [request for request in requests if request[0] == "PUT"]
@pytest.mark.anyio
@@ -176,6 +191,67 @@ async def test_feedback_batch_api_requires_at_least_two_suggestions(monkeypatch)
assert called is False
@pytest.mark.anyio
async def test_feedback_batch_api_accepts_unique_cross_file_manifest(monkeypatch):
calls = []
async def commit(*args):
calls.append(args)
return {
"repository": "stackchain/api", "number": 7,
"paths": ["src/api.py", "tests/test_api.py"],
"previous_head_sha": "abc1234", "head_sha": "def5678",
"suggestion_count": 2, "message": "fix: suggestions",
}
monkeypatch.setattr(main.gitea_proxy, "commit_authored_pull_feedback_batch", commit, raising=False)
transport = httpx.ASGITransport(app=main.app)
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
response = await client.post(
"/api/v1/repos/stackchain/api/pulls/7/feedback-batch",
json={
"message": "fix: suggestions", "expected_head_sha": "abc1234", "suggestion_count": 2,
"files": [
{"path": "src/api.py", "content": "after\n", "expected_blob_sha": "blob1"},
{"path": "tests/test_api.py", "content": "assert after\n", "expected_blob_sha": "blob2"},
],
},
)
assert response.status_code == 200
assert response.json()["paths"] == ["src/api.py", "tests/test_api.py"]
assert calls[0][2] == [
{"path": "src/api.py", "content": "after\n", "expected_blob_sha": "blob1"},
{"path": "tests/test_api.py", "content": "assert after\n", "expected_blob_sha": "blob2"},
]
@pytest.mark.anyio
async def test_feedback_batch_api_rejects_duplicate_file_paths(monkeypatch):
called = False
async def commit(*args):
nonlocal called
called = True
monkeypatch.setattr(main.gitea_proxy, "commit_authored_pull_feedback_batch", commit, raising=False)
transport = httpx.ASGITransport(app=main.app)
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
response = await client.post(
"/api/v1/repos/stackchain/api/pulls/7/feedback-batch",
json={
"message": "fix: suggestions", "expected_head_sha": "abc1234", "suggestion_count": 2,
"files": [
{"path": "src/api.py", "content": "after\n", "expected_blob_sha": "blob1"},
{"path": "src/api.py", "content": "other\n", "expected_blob_sha": "blob2"},
],
},
)
assert response.status_code == 422
assert called is False
@pytest.mark.anyio
async def test_feedback_file_api_loads_and_commits_an_authored_pull_fix(monkeypatch):
calls = []