feat: add inline mobile review comments (#165)
This commit is contained in:
1 parent
4b7405aa65
commit
58376f6d45
7 files changed
+436
-16
No files matched your search
+82
-1
@@ -1073,11 +1073,43 @@ process.stdout.write(JSON.stringify({{ html, expanded: button.attrs['aria-expand
|
||||
assert 'Preview truncated' in output["html"]
|
||||
assert 'class="review-mark"' in output["html"]
|
||||
assert 'data-review-filename="src/<api>.py"' in output["html"]
|
||||
assert 'class="review-diff-line review-inline-target removed"' in output["html"]
|
||||
assert 'data-old-position="1"' in output["html"]
|
||||
assert 'data-new-position="1"' in output["html"]
|
||||
assert 'aria-label="Comment on src/<api>.py line 1"' in output["html"]
|
||||
assert 'Mark reviewed' in output["html"]
|
||||
assert output["expanded"] == "true"
|
||||
assert output["hidden"] is False
|
||||
|
||||
|
||||
def test_review_diff_parser_maps_multi_hunk_lines_to_old_or_new_positions():
|
||||
script = f"""
|
||||
const reviewSheet = require({json.dumps(str(REVIEW_SHEET))});
|
||||
const rows = reviewSheet.parseDiffLines([
|
||||
'@@ -10,3 +20,4 @@ function run()',
|
||||
' context', '-removed', '+added', '+second',
|
||||
String.raw`\\ No newline at end of file`,
|
||||
'@@ -40 +51 @@', '-old tail', '+new tail'
|
||||
]);
|
||||
process.stdout.write(JSON.stringify(rows));
|
||||
"""
|
||||
result = subprocess.run(
|
||||
["node", "-e", script], check=True, capture_output=True, text=True
|
||||
)
|
||||
|
||||
assert json.loads(result.stdout) == [
|
||||
{"text": "@@ -10,3 +20,4 @@ function run()", "kind": "hunk", "commentable": False},
|
||||
{"text": " context", "kind": "context", "commentable": True, "new_position": 20},
|
||||
{"text": "-removed", "kind": "removed", "commentable": True, "old_position": 11},
|
||||
{"text": "+added", "kind": "added", "commentable": True, "new_position": 21},
|
||||
{"text": "+second", "kind": "added", "commentable": True, "new_position": 22},
|
||||
{"text": "\\ No newline at end of file", "kind": "note", "commentable": False},
|
||||
{"text": "@@ -40 +51 @@", "kind": "hunk", "commentable": False},
|
||||
{"text": "-old tail", "kind": "removed", "commentable": True, "old_position": 40},
|
||||
{"text": "+new tail", "kind": "added", "commentable": True, "new_position": 51},
|
||||
]
|
||||
|
||||
|
||||
def test_review_progress_is_explicit_and_restores_for_the_same_head_sha():
|
||||
script = f"""
|
||||
const reviewSheet = require({json.dumps(str(REVIEW_SHEET))});
|
||||
@@ -1159,11 +1191,42 @@ process.stdout.write(JSON.stringify(restored));
|
||||
|
||||
assert json.loads(result.stdout) == {
|
||||
"notes": {"src/a.py": "Handle the empty state."},
|
||||
"comments": [],
|
||||
"summary": "One blocker remains.",
|
||||
"decision": "request_changes",
|
||||
}
|
||||
|
||||
|
||||
def test_review_draft_persists_edits_and_removes_inline_comments_for_same_head():
|
||||
script = f"""
|
||||
const reviewSheet = require({json.dumps(str(REVIEW_SHEET))});
|
||||
const values = new Map();
|
||||
const storage = {{getItem:k => values.get(k) || null, setItem:(k,v) => values.set(k,v), removeItem:k => values.delete(k)}};
|
||||
const options = {{storage, repository:'stackchain/api', number:7, headSha:'abc123', files:[{{filename:'src/a.py'}}]}};
|
||||
const first = reviewSheet.createDraft(options);
|
||||
first.setInlineComment({{path:'src/a.py', new_position:42}}, 'Handle empty values.');
|
||||
first.setInlineComment({{path:'src/a.py', new_position:42}}, 'Handle null and empty values.');
|
||||
first.setInlineComment({{path:'src/a.py', old_position:9}}, 'Why remove this guard?');
|
||||
const restored = reviewSheet.createDraft(options);
|
||||
const beforeRemove = restored.snapshot().comments;
|
||||
const afterRemove = restored.removeInlineComment({{path:'src/a.py', old_position:9}}).comments;
|
||||
process.stdout.write(JSON.stringify({{beforeRemove, afterRemove}}));
|
||||
"""
|
||||
result = subprocess.run(
|
||||
["node", "-e", script], check=True, capture_output=True, text=True
|
||||
)
|
||||
|
||||
assert json.loads(result.stdout) == {
|
||||
"beforeRemove": [
|
||||
{"path": "src/a.py", "body": "Handle null and empty values.", "new_position": 42},
|
||||
{"path": "src/a.py", "body": "Why remove this guard?", "old_position": 9},
|
||||
],
|
||||
"afterRemove": [
|
||||
{"path": "src/a.py", "body": "Handle null and empty values.", "new_position": 42},
|
||||
],
|
||||
}
|
||||
|
||||
|
||||
def test_review_feedback_formats_non_empty_file_notes_in_changed_file_order():
|
||||
script = f"""
|
||||
const reviewSheet = require({json.dumps(str(REVIEW_SHEET))});
|
||||
@@ -1313,6 +1376,22 @@ async def test_mobile_review_sheet_captures_and_safely_hands_off_feedback():
|
||||
assert "handoffWindow.opener = null" in html
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_mobile_review_sheet_edits_inline_drafts_and_submits_them_with_review():
|
||||
html = await dashboard()
|
||||
|
||||
assert 'id="review-inline-composer"' in html
|
||||
assert 'id="review-inline-body"' in html and 'maxlength="10000"' in html
|
||||
assert 'id="save-inline-comment"' in html
|
||||
assert 'id="delete-inline-comment"' in html
|
||||
assert '.review-inline-target' in html and 'min-height:44px' in html
|
||||
assert '.review-inline-composer' in html and 'position:sticky' in html
|
||||
assert "document.querySelectorAll('.review-inline-target')" in html
|
||||
assert "draft.setInlineComment" in html
|
||||
assert "draft.removeInlineComment" in html
|
||||
assert "comments: snapshot.comments" in html
|
||||
|
||||
|
||||
def test_review_controller_submits_once_while_request_is_in_flight():
|
||||
script = f"""
|
||||
const createReviewController = require({json.dumps(str(REVIEW_SHEET))});
|
||||
@@ -1389,7 +1468,9 @@ process.stdout.write(JSON.stringify({{removed, draft:draft.snapshot(), progress:
|
||||
"stackchain.review-draft.v1:stackchain/api#7@abc123",
|
||||
"stackchain.review-progress.v1:stackchain/api#7@abc123",
|
||||
]
|
||||
assert payload["draft"] == {"notes": {}, "summary": "", "decision": "comment"}
|
||||
assert payload["draft"] == {
|
||||
"notes": {}, "comments": [], "summary": "", "decision": "comment"
|
||||
}
|
||||
assert payload["progress"]["reviewedCount"] == 0
|
||||
|
||||
|
||||
|
||||
@@ -111,6 +111,41 @@ async def test_review_submission_posts_validated_decision_for_requested_current_
|
||||
]
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_review_submission_forwards_valid_inline_comments(monkeypatch):
|
||||
calls = []
|
||||
|
||||
async def requested(repository, number):
|
||||
return True
|
||||
|
||||
async def submit(repository, number, expected_head_sha, decision, body, comments):
|
||||
calls.append((repository, number, expected_head_sha, decision, body, comments))
|
||||
return {"id": 92, "state": "REQUEST_CHANGES", "url": "https://forge.example/reviews/92"}
|
||||
|
||||
monkeypatch.setattr(main, "is_requested_review", requested)
|
||||
monkeypatch.setattr(main.gitea_proxy, "submit_pull_review", submit)
|
||||
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/review",
|
||||
json={
|
||||
"decision": "request_changes",
|
||||
"body": "One inline blocker.",
|
||||
"expected_head_sha": "abc123",
|
||||
"comments": [{
|
||||
"path": "src/api.py", "body": "Handle the empty value.",
|
||||
"new_position": 42,
|
||||
}],
|
||||
},
|
||||
)
|
||||
|
||||
assert response.status_code == 201
|
||||
assert calls == [(
|
||||
"stackchain/api", 7, "abc123", "request_changes", "One inline blocker.",
|
||||
[{"path": "src/api.py", "body": "Handle the empty value.", "new_position": 42, "old_position": None}],
|
||||
)]
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_gitea_review_submission_checks_head_then_maps_decision_upstream():
|
||||
requests = []
|
||||
@@ -148,6 +183,38 @@ async def test_gitea_review_submission_checks_head_then_maps_decision_upstream()
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_gitea_review_submission_sends_inline_comments_in_single_review_request():
|
||||
requests = []
|
||||
|
||||
async def handler(request):
|
||||
requests.append(request)
|
||||
if request.url.path.endswith("/files"):
|
||||
return httpx.Response(200, json=[{"filename": "src/api.py"}])
|
||||
if request.method == "GET":
|
||||
return httpx.Response(200, json={"head": {"sha": "abc123"}})
|
||||
return httpx.Response(200, json={"id": 93, "state": "COMMENT"})
|
||||
|
||||
gitea_proxy.start_client(transport=httpx.MockTransport(handler))
|
||||
try:
|
||||
await gitea_proxy.submit_pull_review(
|
||||
"stackchain/api", 7, "abc123", "comment", "Summary",
|
||||
[{"path": "src/api.py", "body": "Handle empty values.", "new_position": 42, "old_position": None}],
|
||||
)
|
||||
finally:
|
||||
await gitea_proxy.stop_client()
|
||||
|
||||
assert [(request.method, request.url.path) for request in requests] == [
|
||||
("GET", "/api/v1/repos/stackchain/api/pulls/7"),
|
||||
("GET", "/api/v1/repos/stackchain/api/pulls/7/files"),
|
||||
("POST", "/api/v1/repos/stackchain/api/pulls/7/reviews"),
|
||||
]
|
||||
assert requests[-1].content == (
|
||||
b'{"body":"Summary","event":"COMMENT","commit_id":"abc123","comments":'
|
||||
b'[{"path":"src/api.py","body":"Handle empty values.","new_position":42,"old_position":null}]}'
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_gitea_review_submission_rejects_changed_head_without_posting():
|
||||
methods = []
|
||||
@@ -168,6 +235,32 @@ async def test_gitea_review_submission_rejects_changed_head_without_posting():
|
||||
assert methods == ["GET"]
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_gitea_review_submission_rejects_inline_comment_for_unchanged_path_without_posting():
|
||||
requests = []
|
||||
|
||||
async def handler(request):
|
||||
requests.append((request.method, request.url.path))
|
||||
if request.url.path.endswith("/files"):
|
||||
return httpx.Response(200, json=[{"filename": "src/api.py"}])
|
||||
return httpx.Response(200, json={"head": {"sha": "abc123"}})
|
||||
|
||||
gitea_proxy.start_client(transport=httpx.MockTransport(handler))
|
||||
try:
|
||||
with pytest.raises(gitea_proxy.InvalidReviewCommentError):
|
||||
await gitea_proxy.submit_pull_review(
|
||||
"stackchain/api", 7, "abc123", "comment", "Review note.",
|
||||
[{"path": "src/unknown.py", "body": "Not in this change", "new_position": 4}],
|
||||
)
|
||||
finally:
|
||||
await gitea_proxy.stop_client()
|
||||
|
||||
assert requests == [
|
||||
("GET", "/api/v1/repos/stackchain/api/pulls/7"),
|
||||
("GET", "/api/v1/repos/stackchain/api/pulls/7/files"),
|
||||
]
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_review_submission_returns_conflict_when_pull_head_changed(monkeypatch):
|
||||
async def requested(repository, number):
|
||||
@@ -195,6 +288,32 @@ async def test_review_submission_returns_conflict_when_pull_head_changed(monkeyp
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_review_submission_returns_validation_error_when_inline_path_is_stale(monkeypatch):
|
||||
async def requested(repository, number):
|
||||
return True
|
||||
|
||||
async def submit(*args):
|
||||
raise gitea_proxy.InvalidReviewCommentError("unknown path")
|
||||
|
||||
monkeypatch.setattr(main, "is_requested_review", requested)
|
||||
monkeypatch.setattr(main.gitea_proxy, "submit_pull_review", submit)
|
||||
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/review",
|
||||
json={
|
||||
"decision": "comment", "body": "Note", "expected_head_sha": "abc123",
|
||||
"comments": [{"path": "gone.py", "body": "Stale", "new_position": 1}],
|
||||
},
|
||||
)
|
||||
|
||||
assert response.status_code == 422
|
||||
assert response.json() == {
|
||||
"error": "An inline comment no longer matches this pull request. Refresh the review."
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_review_submission_rejects_unsupported_decision_before_upstream(monkeypatch):
|
||||
called = False
|
||||
|
||||
Reference in new issue
Block a user