Merge pull request 'Review assigned pull diffs before mobile merge' (#188) from timmy/187-review-assigned-pull-diffs into main
All checks were successful
CI / lint (push) Successful in 13s
Release / release-candidate (push) Successful in 3s
CI / build-frontend (push) Successful in 4s

This commit is contained in:
rockachopa 2026-08-07 10:24:08 +00:00
commit c0eabbf865
5 changed files with 246 additions and 10 deletions

View File

@ -178,6 +178,17 @@ textarea { resize: vertical; min-height: 120px; }
.pull-sheet-header button, .pull-sheet-actions button, .pull-sheet-actions a, .pull-comment-composer button { min-height:44px; }
.pull-sheet-content { overflow-wrap:anywhere; white-space:pre-wrap; }
.pull-file, .pull-comment-card { margin:8px 0; padding:10px; border:1px solid #203a5c; border-radius:10px; }
.pull-file-toggle, .pull-review-file { min-height:44px; width:100%; }
.pull-file-toggle { display:flex; justify-content:space-between; align-items:center; gap:8px; text-align:left; }
.pull-review-file { margin-top:8px; }
.pull-diff { overflow-x:auto; margin:8px 0; padding:10px; background:#07111f; border-radius:8px; font-size:12px; }
.pull-diff-line { display:block; width:max-content; min-width:100%; }
.pull-diff-line.added { color:#86efac; background:#123422; }
.pull-diff-line.removed { color:#fca5a5; background:#3b161b; }
.pull-diff-line.hunk, .pull-diff-note { color:#93c5fd; }
.pull-diff-empty { margin:8px 0; padding:10px; border:1px dashed #4e6b8a; border-radius:8px; }
.pull-review-tools { display:flex; align-items:center; justify-content:space-between; gap:8px; margin:8px 0; }
.pull-review-tools button { min-height:44px; }
.pull-comment-composer textarea { width:100%; min-height:110px; resize:vertical; }
.pull-sheet-actions { position:sticky; bottom:0; display:grid; grid-template-columns:1fr 1fr; gap:8px; padding:10px 0; padding-bottom:calc(10px + env(safe-area-inset-bottom)); background:#0b1526; }
.pull-sheet-actions a { display:grid; place-items:center; border:1px solid #60a5fa; border-radius:10px; font-weight:700; }
@ -449,7 +460,9 @@ textarea { resize: vertical; min-height: 120px; }
<button class="pull-retry" id="retry-pull-load" type="button" hidden>Retry loading pull request</button>
<div class="row"><span class="pill" id="pull-ci-state">CI unknown</span><span class="pill" id="pull-merge-state">Checking merge status</span></div>
<p class="pull-sheet-content" id="pull-sheet-body"></p>
<h2>Changed files</h2><div id="pull-files"></div>
<h2>Changed files</h2>
<div class="pull-review-tools"><span id="pull-review-progress" class="small" aria-live="polite">Review progress unavailable.</span><button id="next-unreviewed-pull-file" type="button" disabled>Next unreviewed</button></div>
<div id="pull-files"></div>
<h2>Recent discussion</h2><div id="pull-comments"></div>
<section class="pull-comment-composer" aria-labelledby="pull-comment-title">
<h2 id="pull-comment-title">Add comment</h2>
@ -580,6 +593,7 @@ textarea { resize: vertical; min-height: 120px; }
let selectedPull = null;
let pullTrigger = null;
let selectedPullDetail = null;
let pullReviewState = null;
let creatingIssue = false;
let findingWork = false;
let availablePagination = { page: 1, total: 0, has_more: false };
@ -1085,17 +1099,67 @@ textarea { resize: vertical; min-height: 120px; }
if (issueTrigger?.isConnected) issueTrigger.focus();
}
function renderPullReview(detail, focusFilename = null) {
pullReviewState = pullController.reviewState(selectedPull, detail);
qs('#pull-review-progress').textContent = pullReviewState.total ?
pullReviewState.reviewed.length + ' of ' + pullReviewState.total + ' files reviewed' : 'No changed files to review';
qs('#next-unreviewed-pull-file').disabled = pullReviewState.complete;
const eligibility = createPullSheet.mergeEligibility(detail, pullReviewState);
qs('#pull-merge-state').textContent = eligibility.reason;
qs('#merge-pull').disabled = !eligibility.allowed;
qs('#pull-files').innerHTML = (detail.files || []).length ? detail.files.map((file, index) =>
createPullSheet.renderFile(file, index, pullReviewState.reviewed.includes(file.filename), escapeHtml)
).join('') : '<div class="muted">No changed files reported.</div>';
qs('#pull-files').querySelectorAll('.pull-file-toggle').forEach(button => {
button.addEventListener('click', () => {
const panel = document.getElementById(button.getAttribute('aria-controls'));
const expanded = button.getAttribute('aria-expanded') === 'true';
button.setAttribute('aria-expanded', String(!expanded));
if (panel) panel.hidden = expanded;
});
});
qs('#pull-files').querySelectorAll('.pull-review-file').forEach(button => {
button.addEventListener('click', () => {
const filename = button.dataset.pullReviewFile;
pullReviewState = pullController.toggleReviewed(selectedPull, detail, filename);
renderPullReview(detail, filename);
});
});
if (focusFilename) {
Array.from(qs('#pull-files').querySelectorAll('.pull-review-file'))
.find(button => button.dataset.pullReviewFile === focusFilename)?.focus();
}
}
function focusNextUnreviewedPullFile() {
if (!selectedPullDetail || !pullReviewState) return;
const filename = pullController.nextUnreviewed(selectedPullDetail, pullReviewState);
const article = Array.from(qs('#pull-files').querySelectorAll('.pull-file'))
.find(file => file.dataset.pullFilename === filename);
const toggle = article?.querySelector('.pull-file-toggle');
const panel = toggle && document.getElementById(toggle.getAttribute('aria-controls'));
if (toggle && panel) {
toggle.setAttribute('aria-expanded', 'true');
panel.hidden = false;
toggle.scrollIntoView({ block: 'center', behavior: 'smooth' });
toggle.focus();
}
}
async function openPullSheet(item, trigger) {
if (!item) return;
selectedPull = item;
pullTrigger = trigger;
selectedPullDetail = null;
pullReviewState = null;
qs('#pull-sheet').classList.add('open');
qs('#pull-sheet-key').textContent = item.key || '';
qs('#pull-sheet-title').textContent = item.title || 'Assigned pull request';
qs('#pull-sheet-status').textContent = 'Loading pull request…';
qs('#pull-sheet-body').textContent = '';
qs('#pull-files').textContent = '';
qs('#pull-review-progress').textContent = 'Loading review progress…';
qs('#next-unreviewed-pull-file').disabled = true;
qs('#pull-comments').textContent = '';
qs('#pull-comment').value = pullController.loadDraft(item);
qs('#pull-comment-status').textContent = '';
@ -1109,16 +1173,10 @@ textarea { resize: vertical; min-height: 120px; }
const detail = await pullController.load(item);
if (selectedPull !== item) return;
selectedPullDetail = detail;
const eligibility = createPullSheet.mergeEligibility(detail);
qs('#pull-sheet-title').textContent = detail.title || 'Assigned pull request';
qs('#pull-sheet-body').textContent = detail.body || 'No description provided.';
qs('#pull-ci-state').textContent = 'CI ' + (detail.ci_state || 'unknown');
qs('#pull-merge-state').textContent = eligibility.reason;
qs('#merge-pull').disabled = !eligibility.allowed;
qs('#pull-files').innerHTML = (detail.files || []).length ? detail.files.map(file =>
'<div class="pull-file"><strong>' + escapeHtml(file.filename) + '</strong><div class="small">' +
escapeHtml(file.status || 'changed') + ' · +' + Number(file.additions || 0) + ' / ' + Number(file.deletions || 0) + '</div></div>'
).join('') : '<div class="muted">No changed files reported.</div>';
renderPullReview(detail);
qs('#pull-comments').innerHTML = (detail.comments || []).length ? detail.comments.map(comment =>
'<div class="pull-comment-card">' + renderIssueComment(comment) + '</div>'
).join('') : '<div class="muted">No comments yet.</div>';
@ -1136,6 +1194,7 @@ textarea { resize: vertical; min-height: 120px; }
qs('#pull-sheet').classList.remove('open');
selectedPull = null;
selectedPullDetail = null;
pullReviewState = null;
if (pullTrigger?.isConnected) pullTrigger.focus();
}
@ -1767,6 +1826,7 @@ textarea { resize: vertical; min-height: 120px; }
qs('#retry-pull-load').addEventListener('click', () => {
if (selectedPull) openPullSheet(selectedPull, pullTrigger);
});
qs('#next-unreviewed-pull-file').addEventListener('click', focusNextUnreviewedPullFile);
qs('#pull-comment').addEventListener('input', event => {
if (selectedPull) pullController.saveDraft(selectedPull, event.target.value);
});

View File

@ -1,4 +1,4 @@
function mergeEligibility(detail) {
function mergeEligibility(detail, reviewState) {
if (!detail || detail.state !== 'open' || detail.merged) {
return { allowed: false, reason: 'Pull request is not open' };
}
@ -8,9 +8,35 @@ function mergeEligibility(detail) {
return { allowed: false, reason: 'CI must succeed before merging' };
}
if (!detail.head_sha) return { allowed: false, reason: 'Current head is unavailable' };
if (reviewState && !reviewState.complete) {
return { allowed: false, reason: 'Review every changed file before merging' };
}
return { allowed: true, reason: 'Ready to merge' };
}
function renderFile(file, index, reviewed, escapeHtml) {
const filename = escapeHtml(file.filename || 'Unknown file');
const panelId = 'pull-diff-' + index;
const lines = (file.diff_lines || []).map(line => {
const text = String(line);
const kind = text.startsWith('@@') ? 'hunk' : text.startsWith('+') ? 'added' :
text.startsWith('-') ? 'removed' : 'context';
return '<span class="pull-diff-line ' + kind + '">' + escapeHtml(text) + '</span>';
}).join('');
const preview = file.diff_available
? '<pre class="pull-diff" id="' + panelId + '" hidden>' + lines +
(file.diff_truncated ? '<span class="pull-diff-note">Preview truncated · open in Gitea for the full diff.</span>' : '') + '</pre>'
: '<div class="pull-diff-empty" id="' + panelId + '" hidden>' +
(file.diff_binary ? 'Binary file · preview unavailable.' : 'Diff preview unavailable.') +
(file.diff_truncated ? ' The response was truncated.' : '') + '</div>';
return '<article class="pull-file" data-pull-filename="' + filename + '">' +
'<button type="button" class="pull-file-toggle" aria-expanded="false" aria-controls="' + panelId + '">' +
'<strong>' + filename + '</strong><span class="small">' + escapeHtml(file.status || 'changed') +
' · +' + Number(file.additions || 0) + ' / ' + Number(file.deletions || 0) + '</span></button>' + preview +
'<button type="button" class="pull-review-file" data-pull-review-file="' + filename +
'" aria-pressed="' + String(reviewed) + '">' + (reviewed ? 'Reviewed' : 'Mark reviewed') + '</button></article>';
}
function createPullSheet({ fetchJson, storage, createOperationId = () => globalThis.crypto?.randomUUID?.() || String(Date.now()) + '-' + Math.random() }) {
let commentRequest = null;
let mergeRequest = null;
@ -18,11 +44,37 @@ function createPullSheet({ fetchJson, storage, createOperationId = () => globalT
.map(encodeURIComponent).join('/') + '/pulls/' + encodeURIComponent(item.number);
const draftKey = item => 'stackchain.pull-comment.v1:' + item.repository + '#' + item.number;
const operationKey = item => draftKey(item) + ':operation';
const reviewKey = (item, detail) => 'stackchain.pull-review.v1:' + item.repository + '#' + item.number + ':' + detail.head_sha;
const fileNames = detail => (detail?.files || []).map(file => file.filename).filter(Boolean);
function reviewState(item, detail) {
const files = fileNames(detail);
let saved = [];
try {
const value = JSON.parse(storage?.getItem(reviewKey(item, detail)) || '[]');
if (Array.isArray(value)) saved = value;
} catch (_error) { /* Corrupt progress safely starts over. */ }
const reviewed = files.filter(filename => saved.includes(filename));
return { reviewed, total: files.length, complete: reviewed.length === files.length };
}
return {
load(item) {
return fetchJson(pathFor(item) + '/detail', { headers: { Accept: 'application/json' } });
},
reviewState,
toggleReviewed(item, detail, filename) {
const state = reviewState(item, detail);
const reviewed = new Set(state.reviewed);
if (reviewed.has(filename)) reviewed.delete(filename);
else if (fileNames(detail).includes(filename)) reviewed.add(filename);
try { storage?.setItem(reviewKey(item, detail), JSON.stringify(Array.from(reviewed))); }
catch (_error) { /* In-memory controls still work for this render. */ }
return reviewState(item, detail);
},
nextUnreviewed(detail, state) {
return fileNames(detail).find(filename => !state.reviewed.includes(filename)) || null;
},
loadDraft(item) {
try { return storage?.getItem(draftKey(item)) || ''; }
catch (_error) { return ''; }
@ -71,4 +123,5 @@ function createPullSheet({ fetchJson, storage, createOperationId = () => globalT
}
createPullSheet.mergeEligibility = mergeEligibility;
createPullSheet.renderFile = renderFile;
if (typeof module !== 'undefined' && module.exports) module.exports = createPullSheet;

View File

@ -842,11 +842,16 @@ async def pull_completion_detail(repository: str, number: int) -> dict:
raise ValueError("Gitea pull request response was not an object")
head = pull.get("head") if isinstance(pull.get("head"), dict) else {}
sha = head.get("sha") if isinstance(head.get("sha"), str) else ""
files, status, comments = await asyncio.gather(
files, status, comments, diff_result = await asyncio.gather(
fetch(f"{base}/files"),
fetch(f"repos/{repository}/commits/{sha}/status"),
fetch(f"repos/{repository}/issues/{number}/comments?limit=20&page=1"),
fetch_text(
f"repos/{repository}/pulls/{number}.diff", REVIEW_DIFF_MAX_BYTES
),
)
diff, diff_truncated = diff_result
previews = _diff_previews(diff, diff_truncated)
user = pull.get("user") if isinstance(pull.get("user"), dict) else {}
return {
"repository": repository,
@ -867,6 +872,15 @@ async def pull_completion_detail(repository: str, number: int) -> dict:
"status": item.get("status") or "changed",
"additions": item.get("additions") or 0,
"deletions": item.get("deletions") or 0,
**previews.get(
item["filename"],
{
"diff_lines": [],
"diff_available": False,
"diff_binary": False,
"diff_truncated": diff_truncated,
},
),
}
for item in (files if isinstance(files, list) else [])[:100]
if isinstance(item, dict) and isinstance(item.get("filename"), str)

View File

@ -1220,6 +1220,65 @@ process.stdout.write(JSON.stringify(states.map(createPullSheet.mergeEligibility)
assert output[3]["allowed"] is False and "conflict" in output[3]["reason"].lower()
def test_pull_sheet_persists_head_scoped_file_review_and_gates_merge():
script = f"""
const createPullSheet = require({json.dumps(str(PULL_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 item = {{repository:'stackchain/api', number:7}};
const detail = {{state:'open', draft:false, mergeable:true, merged:false, ci_state:'success', head_sha:'abc', files:[
{{filename:'src/api.py'}}, {{filename:'frontend/app.js'}}
]}};
const first = createPullSheet({{storage, fetchJson:()=>Promise.resolve()}});
const before = first.reviewState(item, detail);
const afterOne = first.toggleReviewed(item, detail, 'src/api.py');
const restored = createPullSheet({{storage, fetchJson:()=>Promise.resolve()}}).reviewState(item, detail);
const complete = first.toggleReviewed(item, detail, 'frontend/app.js');
const changedHead = first.reviewState(item, {{...detail, head_sha:'def'}});
process.stdout.write(JSON.stringify({{
before, afterOne, restored, complete, changedHead,
blocked:createPullSheet.mergeEligibility(detail, afterOne),
allowed:createPullSheet.mergeEligibility(detail, complete),
next:first.nextUnreviewed(detail, afterOne),
}}));
"""
result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True)
output = json.loads(result.stdout)
assert output["before"] == {"reviewed": [], "total": 2, "complete": False}
assert output["afterOne"]["reviewed"] == ["src/api.py"]
assert output["restored"] == output["afterOne"]
assert output["complete"]["complete"] is True
assert output["changedHead"]["reviewed"] == []
assert output["blocked"] == {"allowed": False, "reason": "Review every changed file before merging"}
assert output["allowed"] == {"allowed": True, "reason": "Ready to merge"}
assert output["next"] == "frontend/app.js"
def test_pull_sheet_renders_mobile_diff_fallbacks_and_review_controls():
script = f"""
const createPullSheet = require({json.dumps(str(PULL_SHEET))});
const escapeHtml = value => String(value).replaceAll('&', '&amp;').replaceAll('<', '&lt;');
const text = createPullSheet.renderFile({{
filename:'src/<unsafe>.py', status:'modified', additions:1, deletions:1,
diff_available:true, diff_lines:['@@ -1 +1 @@', '-old', '+new'], diff_truncated:true
}}, 0, false, escapeHtml);
const binary = createPullSheet.renderFile({{
filename:'static/logo.png', diff_available:false, diff_binary:true, diff_truncated:false
}}, 1, true, escapeHtml);
process.stdout.write(JSON.stringify({{text, binary}}));
"""
result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True)
output = json.loads(result.stdout)
assert 'aria-controls="pull-diff-0"' in output["text"]
assert "&lt;unsafe>" in output["text"] and "+new" in output["text"]
assert "Preview truncated" in output["text"]
assert 'data-pull-review-file="src/&lt;unsafe>.py"' in output["text"]
assert "Binary file · preview unavailable" in output["binary"]
assert 'aria-pressed="true"' in output["binary"] and "Reviewed" in output["binary"]
@pytest.mark.anyio
async def test_assigned_pulls_open_accessible_mobile_completion_sheet():
html = await dashboard()
@ -1228,10 +1287,17 @@ async def test_assigned_pulls_open_accessible_mobile_completion_sheet():
assert 'class="my-work-card-main pull-trigger"' in html
assert 'id="pull-sheet-status"' in html
assert 'id="pull-files"' in html and 'id="pull-comments"' in html
assert 'id="pull-review-progress"' in html and 'aria-live="polite"' in html
assert 'id="next-unreviewed-pull-file"' in html
assert 'id="pull-comment"' in html and 'maxlength="10000"' in html
assert 'id="merge-pull"' in html and 'id="open-pull-gitea"' in html
assert '<script src="static/pull-sheet.js"></script>' in html
assert "pullController.load(item)" in html
assert "createPullSheet.renderFile" in html
assert "pullController.toggleReviewed" in html
assert "pullController.nextUnreviewed" in html
assert ".pull-diff { overflow-x:auto;" in html
assert ".pull-file-toggle, .pull-review-file { min-height:44px;" in html
assert "window.confirm('Merge ' + selectedPull.key" in html
assert "expected_head_sha" in html
assert "item.kind === 'pull'" in html and "pull-trigger" in html

View File

@ -53,6 +53,49 @@ async def test_assigned_pull_detail_rejects_unassigned_pull(monkeypatch):
assert response.status_code == 404
@pytest.mark.anyio
async def test_gitea_assigned_pull_detail_includes_bounded_diff_previews():
async def handler(request):
path = request.url.path
if path.endswith("/pulls/7"):
return httpx.Response(200, json={
"number": 7,
"title": "Review this patch",
"state": "open",
"mergeable": True,
"head": {"sha": "abc123"},
})
if path.endswith("/pulls/7/files"):
return httpx.Response(200, json=[
{"filename": "src/api.py", "status": "modified", "additions": 1, "deletions": 1},
{"filename": "static/logo.png", "status": "modified"},
])
if path.endswith("/commits/abc123/status"):
return httpx.Response(200, json={"state": "success"})
if path.endswith("/issues/7/comments"):
return httpx.Response(200, json=[])
if path.endswith("/pulls/7.diff"):
return httpx.Response(200, text=(
"diff --git a/src/api.py b/src/api.py\n"
"--- a/src/api.py\n+++ b/src/api.py\n"
"@@ -1 +1 @@\n-old\n+new\n"
"diff --git a/static/logo.png b/static/logo.png\n"
"Binary files a/static/logo.png and b/static/logo.png differ\n"
))
raise AssertionError(f"unexpected request: {request.method} {path}")
gitea_proxy.start_client(transport=httpx.MockTransport(handler))
try:
detail = await gitea_proxy.pull_completion_detail("stackchain/api", 7)
finally:
await gitea_proxy.stop_client()
assert detail["files"][0]["diff_available"] is True
assert "+new" in detail["files"][0]["diff_lines"]
assert detail["files"][1]["diff_binary"] is True
assert detail["files"][1]["diff_available"] is False
@pytest.mark.anyio
async def test_assigned_pull_comment_posts_only_after_assignment_check(monkeypatch):
calls = []