diff --git a/README.md b/README.md index 4678a9e..e5bae97 100644 --- a/README.md +++ b/README.md @@ -21,8 +21,11 @@ submit pull-request reviews. Pull-request replies and mobile My Work issue and P comments use Gitea's issue-comment API; mobile issue capture requires issue creation and assignment permission. Closing an assigned issue, native Comment, Approve, and Request changes reviews, and assigned-PR merge require repository -write permission. The dashboard rechecks the current pull-request head, CI success, -draft state, and mergeability immediately before every merge. +write permission. Native Comment, Approve, and Request changes reviews support +head-scoped draft comments anchored to changed lines; the dashboard validates each +comment path and submits the summary, decision, and inline comments in one review +request. The dashboard rechecks the current pull-request head, CI success, draft +state, and mergeability immediately before every merge. Serve the dashboard only to trusted users on its own origin; cross-origin API access is intentionally disabled. Then start the API and bundled frontend: diff --git a/frontend/index.html b/frontend/index.html index f65304b..fb589ae 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -97,6 +97,13 @@ textarea { resize: vertical; min-height: 120px; } .review-progress-actions button { min-height:44px; max-width:100%; } .review-diff { overflow-x:auto; max-width:100%; margin-top:8px; white-space:pre; } .review-diff-line { display:block; min-width:max-content; } +.review-inline-target { min-height:44px; width:100%; padding:8px; border:0; border-radius:0; text-align:left; font:inherit; white-space:pre; } +.review-inline-target.has-draft { box-shadow:inset 4px 0 #fbbf24; } +.review-inline-composer { position:sticky; bottom:0; z-index:4; display:grid; gap:8px; padding:10px; padding-bottom:calc(10px + env(safe-area-inset-bottom)); border:1px solid #60a5fa; border-radius:10px; background:#0b1526; } +.review-inline-composer[hidden] { display:none; } +.review-inline-composer textarea { min-height:96px; width:100%; } +.review-inline-actions { display:grid; grid-template-columns:1fr 1fr; gap:8px; } +.review-inline-actions button { min-height:44px; } .review-diff-line.hunk { color:#93c5fd; } .review-diff-line.added { color:#86efac; background:rgba(34,197,94,.09); } .review-diff-line.removed { color:#fca5a5; background:rgba(239,68,68,.09); } @@ -426,6 +433,16 @@ textarea { resize: vertical; min-height: 120px; }
' + lines +
(file.diff_truncated ? 'Preview truncated · open in Gitea for the full diff.' : '') +
'';
@@ -115,17 +155,27 @@ function createProgress({ storage, repository, number, headSha, files }) {
function createDraft({ storage, repository, number, headSha, files }) {
const filenames = (files || []).map(file => file && file.filename).filter(Boolean);
const key = 'stackchain.review-draft.v1:' + repository + '#' + number + '@' + headSha;
- let draft = { notes: {}, summary: '', decision: 'comment' };
+ let draft = { notes: {}, comments: [], summary: '', decision: 'comment' };
try {
const saved = JSON.parse(storage.getItem(key) || '{}');
if (saved && typeof saved === 'object') {
draft.notes = Object.fromEntries(filenames
.filter(filename => typeof saved.notes?.[filename] === 'string' && saved.notes[filename])
.map(filename => [filename, saved.notes[filename]]));
- if (typeof saved.summary === 'string') draft.summary = saved.summary;
- if (['comment', 'approve', 'request_changes'].includes(saved.decision)) {
- draft.decision = saved.decision;
+ if (Array.isArray(saved.comments)) {
+ draft.comments = saved.comments.filter(comment =>
+ comment && filenames.includes(comment.path) && typeof comment.body === 'string' &&
+ Boolean(comment.body.trim()) &&
+ ((Number.isInteger(comment.new_position) && !comment.old_position) ||
+ (Number.isInteger(comment.old_position) && !comment.new_position))
+ ).map(comment => ({
+ path: comment.path, body: comment.body,
+ ...(comment.new_position ? { new_position: comment.new_position } : {}),
+ ...(comment.old_position ? { old_position: comment.old_position } : {}),
+ }));
}
+ if (typeof saved.summary === 'string') draft.summary = saved.summary;
+ if (['comment', 'approve', 'request_changes'].includes(saved.decision)) draft.decision = saved.decision;
}
} catch (error) { /* start with an empty in-memory draft */ }
@@ -134,7 +184,10 @@ function createDraft({ storage, repository, number, headSha, files }) {
}
function snapshot() {
- return { notes: { ...draft.notes }, summary: draft.summary, decision: draft.decision };
+ return {
+ notes: { ...draft.notes }, comments: draft.comments.map(comment => ({ ...comment })),
+ summary: draft.summary, decision: draft.decision,
+ };
}
function setNote(filename, note) {
@@ -145,6 +198,35 @@ function createDraft({ storage, repository, number, headSha, files }) {
return snapshot();
}
+ function inlineKey(comment) {
+ return comment.path + ':' + (comment.new_position ? 'new:' + comment.new_position : 'old:' + comment.old_position);
+ }
+
+ function setInlineComment(anchor, body) {
+ if (!anchor || !filenames.includes(anchor.path)) return snapshot();
+ const position = Number.isInteger(anchor.new_position) && anchor.new_position > 0
+ ? { new_position: anchor.new_position }
+ : Number.isInteger(anchor.old_position) && anchor.old_position > 0
+ ? { old_position: anchor.old_position } : null;
+ const text = String(body || '').trim();
+ if (!position || !text) return snapshot();
+ const comment = { path: anchor.path, body: text, ...position };
+ const key = inlineKey(comment);
+ const index = draft.comments.findIndex(item => inlineKey(item) === key);
+ if (index >= 0) draft.comments[index] = comment;
+ else draft.comments.push(comment);
+ persist();
+ return snapshot();
+ }
+
+ function removeInlineComment(anchor) {
+ if (!anchor) return snapshot();
+ const key = inlineKey(anchor);
+ draft.comments = draft.comments.filter(comment => inlineKey(comment) !== key);
+ persist();
+ return snapshot();
+ }
+
function setSummary(summary) {
draft.summary = String(summary || '');
persist();
@@ -160,12 +242,12 @@ function createDraft({ storage, repository, number, headSha, files }) {
}
function clear() {
- draft = { notes: {}, summary: '', decision: 'comment' };
+ draft = { notes: {}, comments: [], summary: '', decision: 'comment' };
try { storage.removeItem(key); } catch (error) { /* cleared in memory */ }
return snapshot();
}
- return { snapshot, setNote, setSummary, setDecision, clear };
+ return { snapshot, setNote, setInlineComment, removeInlineComment, setSummary, setDecision, clear };
}
function formatFeedback(draft, files) {
@@ -208,6 +290,7 @@ async function copyAndContinue({ text, url, copy, open, fallback }) {
}
createReviewController.renderDiffFile = renderDiffFile;
+createReviewController.parseDiffLines = parseDiffLines;
createReviewController.toggleDiff = toggleDiff;
createReviewController.createProgress = createProgress;
createReviewController.createDraft = createDraft;
diff --git a/src/gitea_proxy.py b/src/gitea_proxy.py
index 6ac6560..bb93159 100644
--- a/src/gitea_proxy.py
+++ b/src/gitea_proxy.py
@@ -18,6 +18,10 @@ class StaleReviewError(ValueError):
"""Raised before mutation when a pull request head changed during review."""
+class InvalidReviewCommentError(ValueError):
+ """Raised before mutation when an inline comment cannot target this change."""
+
+
class StalePullError(ValueError):
"""Raised before merge when an assigned pull request head changed."""
@@ -694,6 +698,7 @@ async def submit_pull_review(
expected_head_sha: str,
decision: str,
body: str,
+ comments: list[dict] | None = None,
) -> dict:
base = f"repos/{repository}/pulls/{number}"
pull = await fetch(base)
@@ -701,6 +706,15 @@ async def submit_pull_review(
current_sha = head.get("sha") if isinstance(head, dict) else None
if current_sha != expected_head_sha:
raise StaleReviewError("Pull request changed while it was being reviewed")
+ if comments:
+ files = await fetch(f"{base}/files")
+ changed_paths = {
+ item.get("filename")
+ for item in (files if isinstance(files, list) else [])
+ if isinstance(item, dict) and isinstance(item.get("filename"), str)
+ }
+ if any(comment.get("path") not in changed_paths for comment in comments):
+ raise InvalidReviewCommentError("Inline comment path is not in this pull request")
response = await _get_client().post(
f"/api/v1/{base}/reviews",
headers=_auth(),
@@ -712,6 +726,7 @@ async def submit_pull_review(
"request_changes": "REQUEST_CHANGES",
}[decision],
"commit_id": current_sha,
+ **({"comments": comments} if comments else {}),
},
)
response.raise_for_status()
diff --git a/src/main.py b/src/main.py
index 1de0786..853ff4e 100644
--- a/src/main.py
+++ b/src/main.py
@@ -7,7 +7,7 @@ from pathlib import Path
from fastapi import FastAPI, HTTPException, Path as PathParam, Query
from fastapi.responses import JSONResponse
from fastapi.staticfiles import StaticFiles
-from pydantic import BaseModel, Field, PositiveInt, field_validator
+from pydantic import BaseModel, Field, PositiveInt, field_validator, model_validator
from src import gitea_proxy
from src.gitea_proxy import (
@@ -125,10 +125,32 @@ class IssueCreation(BaseModel):
return value.strip()
+class PullReviewComment(BaseModel):
+ path: str = Field(min_length=1, max_length=1_000)
+ body: str = Field(min_length=1, max_length=10_000)
+ new_position: PositiveInt | None = None
+ old_position: PositiveInt | None = None
+
+ @field_validator("path", "body")
+ @classmethod
+ def strip_comment_text(cls, value: str) -> str:
+ value = value.strip()
+ if not value:
+ raise ValueError("inline comment values must not be blank")
+ return value
+
+ @model_validator(mode="after")
+ def require_one_position(self):
+ if (self.new_position is None) == (self.old_position is None):
+ raise ValueError("inline comments require exactly one old or new position")
+ return self
+
+
class PullReviewSubmission(BaseModel):
decision: str
body: str = Field(max_length=10_000)
expected_head_sha: str
+ comments: list[PullReviewComment] = Field(default_factory=list, max_length=50)
@field_validator("decision")
@classmethod
@@ -987,13 +1009,19 @@ async def submit_review(
async def submit_requested_review():
if not await is_requested_review(repository, number):
raise HTTPException(status_code=404, detail="Review request not found")
- return await gitea_proxy.submit_pull_review(
+ args = (
repository,
number,
submission.expected_head_sha,
submission.decision,
submission.body,
)
+ if submission.comments:
+ return await gitea_proxy.submit_pull_review(
+ *args,
+ [comment.model_dump() for comment in submission.comments],
+ )
+ return await gitea_proxy.submit_pull_review(*args)
try:
result = await asyncio.wait_for(
@@ -1006,6 +1034,11 @@ async def submit_review(
{"error": "New commits were pushed. Refresh the review before submitting."},
status_code=409,
)
+ except gitea_proxy.InvalidReviewCommentError:
+ return JSONResponse(
+ {"error": "An inline comment no longer matches this pull request. Refresh the review."},
+ status_code=422,
+ )
except Exception:
return JSONResponse(
{"error": "The review could not be submitted. Your draft is safe; please retry."},
diff --git a/tests/test_my_work.py b/tests/test_my_work.py
index b324518..76d9919 100644
--- a/tests/test_my_work.py
+++ b/tests/test_my_work.py
@@ -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
diff --git a/tests/test_review_api.py b/tests/test_review_api.py
index fd005f2..b6e198d 100644
--- a/tests/test_review_api.py
+++ b/tests/test_review_api.py
@@ -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