Merge pull request 'Add draft-safe inline comments to mobile pull request reviews' (#166) from timmy/165-mobile-inline-review-comments into main
This commit is contained in:
commit
5fe0b28747
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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; }
|
|||
<div class="row"><span class="pill" id="review-ci-state">CI unknown</span><a id="open-review-gitea" href="#" target="_blank" rel="noopener noreferrer">Open in Gitea</a></div>
|
||||
<h2>Changed files</h2>
|
||||
<div id="review-files" class="muted"></div>
|
||||
<section class="review-inline-composer" id="review-inline-composer" aria-labelledby="review-inline-title" hidden>
|
||||
<strong id="review-inline-title">Comment on changed line</strong>
|
||||
<label class="small" id="review-inline-location" for="review-inline-body"></label>
|
||||
<textarea id="review-inline-body" maxlength="10000" placeholder="Write a line-specific comment"></textarea>
|
||||
<div class="review-inline-actions">
|
||||
<button id="save-inline-comment" type="button">Save draft comment</button>
|
||||
<button id="delete-inline-comment" type="button">Delete draft</button>
|
||||
<button id="cancel-inline-comment" type="button">Cancel</button>
|
||||
</div>
|
||||
</section>
|
||||
<div class="review-progress-actions">
|
||||
<span class="small" id="review-progress" aria-live="polite">0 of 0 files reviewed</span>
|
||||
<button id="next-unreviewed-review" disabled>Next unreviewed</button>
|
||||
|
|
@ -520,6 +537,7 @@ textarea { resize: vertical; min-height: 120px; }
|
|||
let draft = null;
|
||||
let reviewFiles = [];
|
||||
let selectedReviewHead = '';
|
||||
let activeInlineTarget = null;
|
||||
let bulkConfirmationPending = false;
|
||||
let bulkMarkPending = false;
|
||||
let reviewHandoffPending = false;
|
||||
|
|
@ -1019,6 +1037,38 @@ textarea { resize: vertical; min-height: 120px; }
|
|||
qs('#new-issue').focus();
|
||||
}
|
||||
|
||||
function inlineAnchor(target) {
|
||||
return {
|
||||
path: target.dataset.reviewFilename,
|
||||
...(target.dataset.newPosition ? { new_position: Number(target.dataset.newPosition) } : {}),
|
||||
...(target.dataset.oldPosition ? { old_position: Number(target.dataset.oldPosition) } : {}),
|
||||
};
|
||||
}
|
||||
|
||||
function sameInlineAnchor(comment, anchor) {
|
||||
return comment.path === anchor.path && comment.new_position === anchor.new_position &&
|
||||
comment.old_position === anchor.old_position;
|
||||
}
|
||||
|
||||
function closeInlineComposer() {
|
||||
qs('#review-inline-composer').hidden = true;
|
||||
qs('#review-inline-body').value = '';
|
||||
activeInlineTarget = null;
|
||||
}
|
||||
|
||||
function openInlineComposer(target) {
|
||||
if (!draft) return;
|
||||
activeInlineTarget = target;
|
||||
const anchor = inlineAnchor(target);
|
||||
const existing = draft.snapshot().comments.find(comment => sameInlineAnchor(comment, anchor));
|
||||
qs('#review-inline-location').textContent = anchor.path + ' · line ' +
|
||||
(anchor.new_position || anchor.old_position) + (anchor.new_position ? ' (new)' : ' (old)');
|
||||
qs('#review-inline-body').value = existing?.body || '';
|
||||
qs('#delete-inline-comment').disabled = !existing;
|
||||
qs('#review-inline-composer').hidden = false;
|
||||
qs('#review-inline-body').focus();
|
||||
}
|
||||
|
||||
async function openReviewSheet(item, trigger) {
|
||||
selectedReview = item;
|
||||
reviewTrigger = trigger;
|
||||
|
|
@ -1042,6 +1092,7 @@ textarea { resize: vertical; min-height: 120px; }
|
|||
qs('#review-handoff-status').textContent = '';
|
||||
qs('#review-submit-status').textContent = '';
|
||||
qs('#submit-review').disabled = true;
|
||||
closeInlineComposer();
|
||||
draft = null;
|
||||
reviewFiles = [];
|
||||
selectedReviewHead = '';
|
||||
|
|
@ -1083,6 +1134,13 @@ textarea { resize: vertical; min-height: 120px; }
|
|||
note.value = draftSnapshot.notes[note.dataset.reviewFilename] || '';
|
||||
note.addEventListener('input', () => draft.setNote(note.dataset.reviewFilename, note.value));
|
||||
});
|
||||
document.querySelectorAll('.review-inline-target').forEach(target => {
|
||||
const anchor = inlineAnchor(target);
|
||||
target.classList.toggle('has-draft', draftSnapshot.comments.some(comment =>
|
||||
sameInlineAnchor(comment, anchor)
|
||||
));
|
||||
target.addEventListener('click', () => openInlineComposer(target));
|
||||
});
|
||||
document.querySelectorAll('.review-mark').forEach(button => {
|
||||
button.addEventListener('click', () => {
|
||||
const snapshot = progress.markReviewed(button.dataset.reviewFilename);
|
||||
|
|
@ -1114,6 +1172,7 @@ textarea { resize: vertical; min-height: 120px; }
|
|||
draft = null;
|
||||
reviewFiles = [];
|
||||
selectedReviewHead = '';
|
||||
closeInlineComposer();
|
||||
if (reviewTrigger?.isConnected) reviewTrigger.focus();
|
||||
}
|
||||
|
||||
|
|
@ -1481,6 +1540,32 @@ textarea { resize: vertical; min-height: 120px; }
|
|||
});
|
||||
qs('#review-summary').addEventListener('input', event => draft?.setSummary(event.target.value));
|
||||
qs('#review-decision').addEventListener('change', event => draft?.setDecision(event.target.value));
|
||||
qs('#save-inline-comment').addEventListener('click', () => {
|
||||
if (!draft || !activeInlineTarget) return;
|
||||
const body = qs('#review-inline-body').value.trim();
|
||||
if (!body) {
|
||||
qs('#review-inline-body').focus();
|
||||
return;
|
||||
}
|
||||
draft.setInlineComment(inlineAnchor(activeInlineTarget), body);
|
||||
activeInlineTarget.classList.add('has-draft');
|
||||
const target = activeInlineTarget;
|
||||
closeInlineComposer();
|
||||
target.focus();
|
||||
});
|
||||
qs('#delete-inline-comment').addEventListener('click', () => {
|
||||
if (!draft || !activeInlineTarget) return;
|
||||
draft.removeInlineComment(inlineAnchor(activeInlineTarget));
|
||||
activeInlineTarget.classList.remove('has-draft');
|
||||
const target = activeInlineTarget;
|
||||
closeInlineComposer();
|
||||
target.focus();
|
||||
});
|
||||
qs('#cancel-inline-comment').addEventListener('click', () => {
|
||||
const target = activeInlineTarget;
|
||||
closeInlineComposer();
|
||||
target?.focus();
|
||||
});
|
||||
qs('#submit-review').addEventListener('click', async () => {
|
||||
if (!draft || !selectedReview || !selectedReviewHead) return;
|
||||
const snapshot = draft.snapshot();
|
||||
|
|
@ -1496,6 +1581,7 @@ textarea { resize: vertical; min-height: 120px; }
|
|||
decision: snapshot.decision,
|
||||
body: createReviewController.formatFeedback(snapshot, reviewFiles),
|
||||
expected_head_sha: selectedReviewHead,
|
||||
comments: snapshot.comments,
|
||||
});
|
||||
draft.clear();
|
||||
progress?.clear();
|
||||
|
|
|
|||
|
|
@ -34,14 +34,54 @@ function diffLineClass(line) {
|
|||
return 'context';
|
||||
}
|
||||
|
||||
function parseDiffLines(lines) {
|
||||
let oldLine = null;
|
||||
let newLine = null;
|
||||
return (lines || []).map(value => {
|
||||
const text = String(value);
|
||||
const hunk = text.match(/^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/);
|
||||
if (hunk) {
|
||||
oldLine = Number(hunk[1]);
|
||||
newLine = Number(hunk[2]);
|
||||
return { text, kind: 'hunk', commentable: false };
|
||||
}
|
||||
if (text.startsWith('\\')) return { text, kind: 'note', commentable: false };
|
||||
if (oldLine === null || newLine === null) {
|
||||
return { text, kind: diffLineClass(text), commentable: false };
|
||||
}
|
||||
if (text.startsWith('-')) {
|
||||
const row = { text, kind: 'removed', commentable: true, old_position: oldLine };
|
||||
oldLine += 1;
|
||||
return row;
|
||||
}
|
||||
if (text.startsWith('+')) {
|
||||
const row = { text, kind: 'added', commentable: true, new_position: newLine };
|
||||
newLine += 1;
|
||||
return row;
|
||||
}
|
||||
const row = { text, kind: 'context', commentable: true, new_position: newLine };
|
||||
oldLine += 1;
|
||||
newLine += 1;
|
||||
return row;
|
||||
});
|
||||
}
|
||||
|
||||
function renderDiffFile(file, index, escapeHtml) {
|
||||
const panelId = 'review-diff-' + index;
|
||||
let preview;
|
||||
if (file.diff_available) {
|
||||
const lines = (file.diff_lines || []).map(line =>
|
||||
'<span class="review-diff-line ' + diffLineClass(String(line)) + '">' +
|
||||
escapeHtml(String(line)) + '</span>'
|
||||
).join('');
|
||||
const lines = parseDiffLines(file.diff_lines).map(row => {
|
||||
if (!row.commentable) {
|
||||
return '<span class="review-diff-line ' + row.kind + '">' + escapeHtml(row.text) + '</span>';
|
||||
}
|
||||
const position = row.old_position
|
||||
? ' data-old-position="' + row.old_position + '"'
|
||||
: ' data-new-position="' + row.new_position + '"';
|
||||
return '<button type="button" class="review-diff-line review-inline-target ' + row.kind +
|
||||
'" data-review-filename="' + escapeHtml(file.filename || '') + '"' + position +
|
||||
' aria-label="Comment on ' + escapeHtml(file.filename || 'changed file') + ' line ' +
|
||||
(row.old_position || row.new_position) + '">' + escapeHtml(row.text) + '</button>';
|
||||
}).join('');
|
||||
preview = '<pre class="review-diff" id="' + panelId + '" hidden>' + lines +
|
||||
(file.diff_truncated ? '<span class="review-diff-note">Preview truncated · open in Gitea for the full diff.</span>' : '') +
|
||||
'</pre>';
|
||||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
37
src/main.py
37
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."},
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user