diff --git a/frontend/index.html b/frontend/index.html index 2677bf3..166279c 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -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; }
CI unknownChecking merge status

-

Changed files

+

Changed files

+
Review progress unavailable.
+

Recent discussion

Add comment

@@ -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('') : '
No changed files reported.
'; + 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 => - '
' + escapeHtml(file.filename) + '
' + - escapeHtml(file.status || 'changed') + ' · +' + Number(file.additions || 0) + ' / −' + Number(file.deletions || 0) + '
' - ).join('') : '
No changed files reported.
'; + renderPullReview(detail); qs('#pull-comments').innerHTML = (detail.comments || []).length ? detail.comments.map(comment => '
' + renderIssueComment(comment) + '
' ).join('') : '
No comments yet.
'; @@ -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); }); diff --git a/frontend/pull-sheet.js b/frontend/pull-sheet.js index 85e7aca..78baf33 100644 --- a/frontend/pull-sheet.js +++ b/frontend/pull-sheet.js @@ -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 '' + escapeHtml(text) + ''; + }).join(''); + const preview = file.diff_available + ? '' + : ''; + return '
' + + '' + preview + + '
'; +} + 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; diff --git a/src/gitea_proxy.py b/src/gitea_proxy.py index 513c316..688f392 100644 --- a/src/gitea_proxy.py +++ b/src/gitea_proxy.py @@ -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) diff --git a/tests/test_my_work.py b/tests/test_my_work.py index 031dc2b..9dfeb2c 100644 --- a/tests/test_my_work.py +++ b/tests/test_my_work.py @@ -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('&', '&').replaceAll('<', '<'); +const text = createPullSheet.renderFile({{ + filename:'src/.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 "<unsafe>" in output["text"] and "+new" in output["text"] + assert "Preview truncated" in output["text"] + assert 'data-pull-review-file="src/<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 '' 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 diff --git a/tests/test_pull_api.py b/tests/test_pull_api.py index cb7a1dd..4a6a3cd 100644 --- a/tests/test_pull_api.py +++ b/tests/test_pull_api.py @@ -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 = []