Show actionable CI checks in mobile pull reviews #518

Merged
timmy merged 1 commits from timmy/517-actionable-ci-checks into main 2026-08-10 22:33:52 +00:00
8 changed files with 278 additions and 8 deletions

View File

@ -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; }

View File

@ -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 || '<div class="muted">No individual checks were reported.</div>';
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('') : '<div>No changed files reported.</div>';
@ -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());
});

View File

@ -578,6 +578,11 @@
<div id="pull-review-status" class="small" aria-live="polite">Expand to load changed files and merge readiness.</div>
<button class="pull-retry" id="pull-review-retry" type="button" hidden>Retry review data</button>
<div class="row"><span class="pill" id="pull-ci-state">CI unknown</span><span class="pill" id="pull-merge-state">Review data not loaded</span></div>
<details class="ci-checks" id="pull-checks">
<summary><span>Checks</span><span class="small" id="pull-checks-summary">Not loaded</span></summary>
<button class="ci-check-refresh" id="refresh-pull-checks" type="button">Refresh checks</button>
<div class="ci-check-list" id="pull-check-list"></div>
</details>
<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>
<button id="merge-pull" type="button" disabled>Merge</button>
@ -609,6 +614,11 @@
<button class="review-retry" id="retry-review-load" hidden>Retry loading review</button>
<div class="review-sheet-body markdown-content" id="review-sheet-body"></div>
<div class="row"><span class="pill" id="review-ci-state">CI unknown</span><button class="share-work-route" type="button">Share</button><a id="open-review-gitea" href="#" target="_blank" rel="noopener noreferrer">Open in Gitea</a></div>
<details class="ci-checks" id="review-checks">
<summary><span>Checks</span><span class="small" id="review-checks-summary">Not loaded</span></summary>
<button class="ci-check-refresh" id="refresh-review-checks" type="button">Refresh checks</button>
<div class="ci-check-list" id="review-check-list"></div>
</details>
<h2>Changed files</h2>
<div class="review-display-tools">
<button id="review-wrap-lines" type="button" aria-pressed="false" aria-controls="review-files">Wrap lines</button>

View File

@ -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' },

View File

@ -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 ? '<a class="ci-check-link" href="' + escapeHtml(url) +
'" target="_blank" rel="noopener noreferrer">Open job</a>' : '';
return '<article class="ci-check ci-check-' + escapeHtml(check.state || 'unknown') + '">' +
'<div class="ci-check-copy"><strong>' + escapeHtml(check.name) + '</strong>' +
'<span class="small">' + escapeHtml(check.state || 'unknown') +
(check.description ? ' · ' + escapeHtml(check.description) : '') + '</span></div>' + link + '</article>';
}).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;

View File

@ -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,
}

View File

@ -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('&', '&amp;').replaceAll('<', '&lt;').replaceAll('>', '&gt;').replaceAll('"', '&quot;');
const result = reviewSheet.renderChecks([
{{name:'build', state:'success', description:'Passed'}},
{{name:'lint <mobile>', 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 &lt;mobile&gt;") < 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))});

View File

@ -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/<unsafe>", "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/<unsafe>", "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):