diff --git a/frontend/dashboard.css b/frontend/dashboard.css index 4aa8675..428de66 100644 --- a/frontend/dashboard.css +++ b/frontend/dashboard.css @@ -232,6 +232,18 @@ textarea { resize: vertical; min-height: 120px; } .review-diff-note, .review-diff-empty { display:block; padding:8px; color:#fcd34d; white-space:normal; } .review-action { min-height:44px; } .review-retry { min-height:44px; margin-top:10px; } +.ci-checks { max-width:100%; margin:10px 0; border:1px solid #2a496e; border-radius:10px; overflow:hidden; } +.ci-checks > summary { min-height:44px; display:flex; align-items:center; justify-content:space-between; gap:8px; padding:0 10px; cursor:pointer; } +.ci-checks > summary .small { min-width:0; overflow-wrap:anywhere; text-align:right; } +.ci-check-refresh { width:100%; min-height:44px; margin-bottom:6px; } +.ci-check-list { display:grid; gap:6px; min-width:0; padding:0 8px 8px; } +.ci-check { min-width:0; display:grid; grid-template-columns:minmax(0,1fr) auto; align-items:center; gap:8px; padding:8px; border:1px solid #203a5c; border-radius:8px; } +.ci-check-failure, .ci-check-error { border-color:#dc2626; background:rgba(127,29,29,.22); } +.ci-check-pending { border-color:#d97706; background:rgba(120,53,15,.2); } +.ci-check-success { border-color:#15803d; } +.ci-check-copy { min-width:0; display:grid; gap:3px; overflow-wrap:anywhere; } +.ci-check-copy strong, .ci-check-copy span { min-width:0; overflow-wrap:anywhere; } +.ci-check-link { min-height:44px; display:flex; align-items:center; justify-content:center; padding:0 10px; border:1px solid #60a5fa; border-radius:8px; white-space:nowrap; } .update-sheet { position:fixed; inset:0; z-index:55; display:none; justify-content:flex-end; background:rgba(5,12,21,.72); backdrop-filter:blur(4px); } .update-sheet.open { display:flex; } .update-sheet-panel { width:min(560px,100%); height:100%; overflow:auto; padding:18px; background:#0b1526; border-left:1px solid #2a496e; } diff --git a/frontend/dashboard.js b/frontend/dashboard.js index c5d1ab4..09ae2e5 100644 --- a/frontend/dashboard.js +++ b/frontend/dashboard.js @@ -2293,7 +2293,15 @@ if (issueTrigger?.isConnected) issueTrigger.focus(); } + function renderCheckSection(prefix, detail, offline = false) { + const rendered = createReviewController.renderChecks(detail?.checks, escapeHtml, { offline }); + qs('#' + prefix + '-checks-summary').textContent = rendered.summary; + qs('#' + prefix + '-check-list').innerHTML = rendered.html || '
No individual checks were reported.
'; + qs('#' + prefix + '-checks').open = rendered.expanded; + } + function renderPullReview(detail, focusFilename = null) { + renderCheckSection('pull', detail); 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'; @@ -2340,7 +2348,7 @@ } } - async function loadPullReview() { + async function loadPullReview({ refresh = false } = {}) { if (!selectedPull || !selectedPullDetail?.head_sha) return; const item = selectedPull; const readDetail = selectedPullDetail; @@ -2348,7 +2356,7 @@ qs('#pull-review-retry').hidden = true; qs('#merge-pull').disabled = true; try { - const review = await pullController.loadReview(item, readDetail.head_sha); + const review = await pullController.loadReview(item, readDetail.head_sha, { refresh }); if (selectedPull !== item) return; selectedPullDetail = { ...readDetail, ...review }; qs('#pull-ci-state').textContent = 'CI ' + (review.ci_state || 'unknown'); @@ -2396,6 +2404,9 @@ qs('#pull-comment').value = pullController.loadDraft(item); qs('#pull-comment-status').textContent = ''; qs('#pull-ci-state').textContent = 'CI unknown'; + qs('#pull-checks-summary').textContent = 'Not loaded'; + qs('#pull-check-list').textContent = ''; + qs('#pull-checks').open = false; qs('#pull-merge-state').textContent = 'Review data not loaded'; qs('#merge-pull').disabled = true; qs('#merge-pull').textContent = workSession.active() ? 'Merge & next' : 'Merge'; @@ -2946,6 +2957,11 @@ qs('#review-handoff-status').textContent = ''; qs('#review-submit-status').textContent = ''; qs('#continue-review-to-merge').hidden = true; + qs('#review-ci-state').textContent = 'CI unknown'; + qs('#review-checks-summary').textContent = cachedDetail ? 'Last known · loading' : 'Loading'; + qs('#review-check-list').textContent = ''; + qs('#review-checks').open = false; + qs('#refresh-review-checks').disabled = offlineReview; qs('#submit-review').disabled = true; qs('#submit-review').textContent = offlineReview && reviewingActiveTodayItem() ? 'Queue review & next' : (offlineReview ? 'Queue review for reconnect' : 'Submit review'); @@ -2959,6 +2975,7 @@ if (selectedReview !== item) return; qs('#review-sheet-body').innerHTML = renderMarkdown(detail.body || 'No description provided.'); qs('#review-ci-state').textContent = 'CI ' + (detail.ci_state || 'unknown'); + renderCheckSection('review', detail, offlineReview); qs('#review-files').innerHTML = (detail.files || []).length ? detail.files.map((file, index) => createReviewController.renderDiffFile(file, index, escapeHtml) ).join('') : '
No changed files reported.
'; @@ -4262,6 +4279,13 @@ if (event.currentTarget.open) loadPullReview(); }); qs('#pull-review-retry').addEventListener('click', loadPullReview); + qs('#refresh-pull-checks').addEventListener('click', async () => { + const button = qs('#refresh-pull-checks'); + button.disabled = true; + qs('#pull-checks-summary').textContent = 'Refreshing…'; + try { await loadPullReview({ refresh: true }); } + finally { button.disabled = false; } + }); qs('#next-unreviewed-pull-file').addEventListener('click', focusNextUnreviewedPullFile); qs('#load-older-pull-comments').addEventListener('click', async () => { if (!pullConversation) return; @@ -4459,6 +4483,25 @@ qs('#retry-review-load').addEventListener('click', () => { if (selectedReview) openReviewSheet(selectedReview, reviewTrigger); }); + qs('#refresh-review-checks').addEventListener('click', async () => { + if (!selectedReview || offlineReview) return; + const item = selectedReview; + const button = qs('#refresh-review-checks'); + button.disabled = true; + qs('#review-checks-summary').textContent = 'Refreshing…'; + try { + const detail = await reviewController.load(item); + if (selectedReview !== item) return; + if (selectedReviewHead && detail.head_sha !== selectedReviewHead) { + qs('#review-sheet-status').textContent = 'New commits detected. Reload the review before submitting feedback.'; + qs('#submit-review').disabled = true; + } + qs('#review-ci-state').textContent = 'CI ' + (detail.ci_state || 'unknown'); + renderCheckSection('review', detail); + } catch (error) { + if (selectedReview === item) qs('#review-checks-summary').textContent = 'Refresh failed · retry'; + } finally { button.disabled = false; } + }); qs('#next-unreviewed-review').addEventListener('click', () => { if (progress) openNextUnreviewed(progress.snapshot()); }); diff --git a/frontend/index.html b/frontend/index.html index 4163646..e8f14b5 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -578,6 +578,11 @@
Expand to load changed files and merge readiness.
CI unknownReview data not loaded
+
+ ChecksNot loaded + +
+
Review progress unavailable.
@@ -609,6 +614,11 @@
CI unknownOpen in Gitea
+
+ ChecksNot loaded + +
+

Changed files

diff --git a/frontend/pull-sheet.js b/frontend/pull-sheet.js index 4be1f6d..a17a75d 100644 --- a/frontend/pull-sheet.js +++ b/frontend/pull-sheet.js @@ -5,7 +5,11 @@ function mergeEligibility(detail, reviewState) { if (detail.draft) return { allowed: false, reason: 'Draft pull requests cannot be merged' }; if (!detail.mergeable) return { allowed: false, reason: 'Resolve conflicts before merging' }; if (detail.ci_state !== 'success') { - return { allowed: false, reason: 'CI must succeed before merging' }; + const blockers = (detail.checks || []).filter(check => + ['failure', 'error', 'pending'].includes(check.state) + ).map(check => check.name).filter(Boolean).slice(0, 3); + return { allowed: false, reason: blockers.length ? + 'CI blocked by ' + blockers.join(', ') : 'CI must succeed before merging' }; } if (!detail.head_sha) return { allowed: false, reason: 'Current head is unavailable' }; if (reviewState && !reviewState.complete) { @@ -64,9 +68,9 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT load(item) { return fetchJson(pathFor(item) + '/detail', { headers: { Accept: 'application/json' } }); }, - loadReview(item, headSha) { + loadReview(item, headSha, { refresh = false } = {}) { const key = item.repository + '#' + item.number + ':' + headSha; - if (reviewCache.has(key)) return Promise.resolve(reviewCache.get(key)); + if (!refresh && reviewCache.has(key)) return Promise.resolve(reviewCache.get(key)); if (reviewRequests.has(key)) return reviewRequests.get(key); const request = fetchJson(pathFor(item) + '/review-data', { headers: { Accept: 'application/json' }, diff --git a/frontend/review-sheet.js b/frontend/review-sheet.js index de972b6..841bff5 100644 --- a/frontend/review-sheet.js +++ b/frontend/review-sheet.js @@ -355,6 +355,34 @@ function prepareMergeContinuation({ storage, item, headSha, reviewed, decision } return true; } +function renderChecks(checks, escapeHtml, { offline = false } = {}) { + const rank = { error: 0, failure: 0, pending: 1, warning: 2, unknown: 2, success: 3 }; + const items = (Array.isArray(checks) ? checks : []).filter(check => + check && typeof check.name === 'string' && check.name + ).map((check, index) => ({ ...check, index })).sort((left, right) => + (rank[left.state] ?? 2) - (rank[right.state] ?? 2) || left.index - right.index + ); + const failed = items.filter(check => ['failure', 'error'].includes(check.state)).length; + const pending = items.filter(check => check.state === 'pending').length; + const passed = items.filter(check => check.state === 'success').length; + const other = items.length - failed - pending - passed; + const parts = [ + failed ? failed + ' failed' : '', pending ? pending + ' pending' : '', + passed ? passed + ' passed' : '', other ? other + ' other' : '', + ].filter(Boolean); + const summary = (offline ? 'Last known · ' : '') + (parts.join(' · ') || 'No checks reported'); + const html = items.map(check => { + const url = typeof check.url === 'string' ? check.url : ''; + const link = url ? 'Open job' : ''; + return '
' + + '
' + escapeHtml(check.name) + '' + + '' + escapeHtml(check.state || 'unknown') + + (check.description ? ' · ' + escapeHtml(check.description) : '') + '
' + link + '
'; + }).join(''); + return { summary, html, expanded: failed > 0 }; +} + createReviewController.renderDiffFile = renderDiffFile; createReviewController.createWrapPreference = createWrapPreference; createReviewController.parseDiffLines = parseDiffLines; @@ -364,6 +392,7 @@ createReviewController.createDraft = createDraft; createReviewController.formatFeedback = formatFeedback; createReviewController.copyAndContinue = copyAndContinue; createReviewController.prepareMergeContinuation = prepareMergeContinuation; +createReviewController.renderChecks = renderChecks; if (typeof module !== 'undefined' && module.exports) { module.exports = createReviewController; diff --git a/src/gitea_proxy.py b/src/gitea_proxy.py index c77433d..359b6ee 100644 --- a/src/gitea_proxy.py +++ b/src/gitea_proxy.py @@ -4,7 +4,7 @@ import re import shlex from contextlib import asynccontextmanager from typing import Any -from urllib.parse import urlsplit +from urllib.parse import urljoin, urlsplit import httpx @@ -600,6 +600,51 @@ def _safe_web_url(value: Any) -> str: return value if parsed.scheme in {"http", "https"} and parsed.netloc else "" +def _safe_gitea_web_url(value: Any) -> str: + if not isinstance(value, str) or not value.strip(): + return "" + resolved = urljoin(f"{GITEA_URL}/", value.strip()) + parsed = urlsplit(resolved) + configured = urlsplit(GITEA_URL) + base_path = configured.path.rstrip("/") + if ( + parsed.scheme not in {"http", "https"} + or parsed.scheme != configured.scheme + or parsed.netloc != configured.netloc + or (base_path and parsed.path != base_path and not parsed.path.startswith(f"{base_path}/")) + ): + return "" + return resolved + + +def _normalize_commit_checks(status: Any) -> list[dict]: + entries = status.get("statuses") if isinstance(status, dict) else None + if not isinstance(entries, list): + return [] + rank = {"error": 0, "failure": 0, "pending": 1, "warning": 2, "success": 3} + checks = [] + for index, entry in enumerate(entries): + if not isinstance(entry, dict): + continue + name = entry.get("context") + if not isinstance(name, str) or not name.strip(): + continue + state = entry.get("status", entry.get("state", "unknown")) + state = state.lower() if isinstance(state, str) else "unknown" + if state not in rank: + state = "unknown" + description = entry.get("description") + checks.append({ + "name": name.strip()[:120], + "state": state, + "description": description.strip()[:240] if isinstance(description, str) else "", + "url": _safe_gitea_web_url(entry.get("target_url")), + "_index": index, + }) + checks.sort(key=lambda check: (rank.get(check["state"], 2), check["_index"])) + return [{key: value for key, value in check.items() if key != "_index"} for check in checks[:20]] + + def _normalize_notifications(threads: Any) -> list[dict]: if not isinstance(threads, list): raise ValueError("Gitea notification response was not a list") @@ -1741,6 +1786,7 @@ async def pull_completion_review(repository: str, number: int) -> dict: "mergeable": pull.get("mergeable") is True, "merged": pull.get("merged") is True, "ci_state": status.get("state", "unknown") if isinstance(status, dict) else "unknown", + "checks": _normalize_commit_checks(status), "files": [ { "filename": item.get("filename", ""), @@ -1873,6 +1919,7 @@ async def pull_review_detail(repository: str, number: int) -> dict: "author": user.get("login", ""), "head_sha": sha, "ci_state": status.get("state", "unknown") if isinstance(status, dict) else "unknown", + "checks": _normalize_commit_checks(status), "files": normalized_files, "reviews": normalized_reviews, } diff --git a/tests/test_my_work.py b/tests/test_my_work.py index 075e22d..0160f0b 100644 --- a/tests/test_my_work.py +++ b/tests/test_my_work.py @@ -3965,7 +3965,9 @@ const createPullSheet = require({json.dumps(str(PULL_SHEET))}); const states = [ {{state:'open', draft:false, mergeable:true, merged:false, ci_state:'success', head_sha:'abc'}}, {{state:'open', draft:true, mergeable:true, merged:false, ci_state:'success', head_sha:'abc'}}, - {{state:'open', draft:false, mergeable:true, merged:false, ci_state:'failure', head_sha:'abc'}}, + {{state:'open', draft:false, mergeable:true, merged:false, ci_state:'failure', head_sha:'abc', checks:[ + {{name:'lint', state:'failure'}}, {{name:'release', state:'pending'}}, {{name:'build', state:'success'}} + ]}}, {{state:'open', draft:false, mergeable:false, merged:false, ci_state:'success', head_sha:'abc'}}, ]; process.stdout.write(JSON.stringify(states.map(createPullSheet.mergeEligibility))); @@ -3974,7 +3976,7 @@ process.stdout.write(JSON.stringify(states.map(createPullSheet.mergeEligibility) output = json.loads(result.stdout) assert output[0] == {"allowed": True, "reason": "Ready to merge"} assert output[1]["allowed"] is False and "draft" in output[1]["reason"].lower() - assert output[2]["allowed"] is False and "CI" in output[2]["reason"] + assert output[2] == {"allowed": False, "reason": "CI blocked by lint, release"} assert output[3]["allowed"] is False and "conflict" in output[3]["reason"].lower() @@ -4053,6 +4055,34 @@ Promise.allSettled([first, concurrent]).then(async failed => {{ ] +def test_pull_sheet_refreshes_checks_without_clearing_head_scoped_progress(): + 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)}}; +const calls = []; +const sheet = createPullSheet({{storage, fetchJson: url => {{ + calls.push(url); + return Promise.resolve({{head_sha:'abc123', ci_state:calls.length === 1 ? 'pending' : 'success', + checks:[{{name:'lint', state:calls.length === 1 ? 'pending' : 'success'}}], + files:[{{filename:'src/api.py'}}]}}); +}}}}); +const item = {{repository:'stackchain/api', number:7}}; +sheet.loadReview(item, 'abc123').then(first => {{ + sheet.toggleReviewed(item, first, 'src/api.py'); + return sheet.loadReview(item, 'abc123', {{refresh:true}}).then(refreshed => {{ + process.stdout.write(JSON.stringify({{calls, refreshed, progress:sheet.reviewState(item, refreshed)}})); + }}); +}}); +""" + result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True) + output = json.loads(result.stdout) + + assert len(output["calls"]) == 2 + assert output["refreshed"]["ci_state"] == "success" + assert output["progress"] == {"reviewed": ["src/api.py"], "total": 1, "complete": True} + + @pytest.mark.anyio async def test_mobile_pull_sheet_puts_reading_before_collapsed_review_controls(): html = await dashboard() @@ -4493,6 +4523,50 @@ process.stdout.write(JSON.stringify({{ html, expanded: button.attrs['aria-expand assert output["hidden"] is False +def test_review_checks_render_blockers_first_with_safe_touch_links(): + script = f""" +const reviewSheet = require({json.dumps(str(REVIEW_SHEET))}); +const escapeHtml = value => String(value) + .replaceAll('&', '&').replaceAll('<', '<').replaceAll('>', '>').replaceAll('"', '"'); +const result = reviewSheet.renderChecks([ + {{name:'build', state:'success', description:'Passed'}}, + {{name:'lint ', state:'failure', description:'Open & fix', url:'https://forge.example/jobs/4'}}, + {{name:'release', state:'pending', description:'Waiting'}}, +], escapeHtml, {{offline:false}}); +const offline = reviewSheet.renderChecks([ + {{name:'lint', state:'success', description:'Passed'}}, +], escapeHtml, {{offline:true}}); +process.stdout.write(JSON.stringify({{result, offline}})); +""" + result = subprocess.run( + ["node", "-e", script], check=True, capture_output=True, text=True + ) + output = json.loads(result.stdout) + + assert output["result"]["summary"] == "1 failed · 1 pending · 1 passed" + assert output["result"]["expanded"] is True + assert output["result"]["html"].index("lint <mobile>") < output["result"]["html"].index("build") + assert 'href="https://forge.example/jobs/4"' in output["result"]["html"] + assert 'target="_blank" rel="noopener noreferrer"' in output["result"]["html"] + assert "Open job" in output["result"]["html"] + assert output["offline"]["summary"].startswith("Last known · ") + + +@pytest.mark.anyio +async def test_mobile_pull_review_sheets_expose_actionable_refreshable_checks(): + html = await dashboard() + + assert html.index('id="pull-checks"') < html.index('id="pull-files"') + assert html.index('id="review-checks"') < html.index('id="review-files"') + assert 'id="refresh-pull-checks"' in html + assert 'id="refresh-review-checks"' in html + assert "createReviewController.renderChecks" in html + assert "{ refresh: true }" in html + assert "qs('#refresh-review-checks').disabled = offlineReview" in html + assert ".ci-check-link" in html and "min-height:44px" in html + assert ".ci-check-copy" in html and "overflow-wrap:anywhere" in html + + def test_review_wrap_preference_defaults_to_phone_layout_and_persists_explicit_choice(): script = f""" const reviewSheet = require({json.dumps(str(REVIEW_SHEET))}); diff --git a/tests/test_review_api.py b/tests/test_review_api.py index b6e198d..67d0171 100644 --- a/tests/test_review_api.py +++ b/tests/test_review_api.py @@ -36,6 +36,57 @@ async def test_review_detail_endpoint_returns_normalized_no_store_payload(monkey assert response.json()["files"][0]["filename"] == "src/api.py" +@pytest.mark.anyio +async def test_gitea_review_detail_returns_bounded_actionable_checks(monkeypatch): + monkeypatch.setattr(gitea_proxy, "GITEA_URL", "https://forge.example/git") + + async def handler(request): + path = request.url.path + if path.endswith("/pulls/7"): + return httpx.Response(200, json={ + "title": "Review API", "head": {"sha": "abc123"}, + "user": {"login": "alex"}, + }) + if path.endswith("/pulls/7/files") or path.endswith("/pulls/7/reviews"): + return httpx.Response(200, json=[]) + if path.endswith("/commits/abc123/status"): + return httpx.Response(200, json={ + "state": "failure", + "statuses": [ + { + "context": "lint/", "status": "failure", + "description": "Formatting failed " + "x" * 300, + "target_url": "https://forge.example/git/stackchain/api/actions/runs/9/jobs/4", + }, + { + "context": "external", "status": "pending", + "description": "Waiting", "target_url": "https://evil.example/job/1", + }, + {"context": "build-release", "status": "success", "description": "Passed"}, + ] + [{"context": f"extra-{index}", "status": "success"} for index in range(30)], + }) + if path.endswith("/pulls/7.diff"): + return httpx.Response(200, text="") + raise AssertionError(f"unexpected request: {request.method} {path}") + + gitea_proxy.start_client(transport=httpx.MockTransport(handler)) + try: + detail = await gitea_proxy.pull_review_detail("stackchain/api", 7) + finally: + await gitea_proxy.stop_client() + + assert len(detail["checks"]) == 20 + assert detail["checks"][:3] == [ + { + "name": "lint/", "state": "failure", + "description": "Formatting failed " + "x" * 222, + "url": "https://forge.example/git/stackchain/api/actions/runs/9/jobs/4", + }, + {"name": "external", "state": "pending", "description": "Waiting", "url": ""}, + {"name": "build-release", "state": "success", "description": "Passed", "url": ""}, + ] + + @pytest.mark.anyio async def test_review_detail_rejects_pulls_not_requested_from_service_user(monkeypatch): async def requested(repository, number):