Make assigned pull request sheets read-first with on-demand review #218

Merged
timmy merged 1 commits from timmy/217-read-first-pull-sheet into main 2026-08-07 18:01:16 +00:00
6 changed files with 251 additions and 26 deletions

View File

@ -231,6 +231,10 @@ textarea { resize: vertical; min-height: 120px; }
.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-review { margin-top:14px; overflow:hidden; border:1px solid #2a496e; border-radius:10px; padding:0 10px 10px; }
.pull-review summary { min-height:44px; display:flex; align-items:center; cursor:pointer; }
.pull-review summary h2 { margin:0; font-size:16px; }
.pull-review > #merge-pull { min-height:44px; width:100%; margin-top:10px; }
.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; }
@ -579,11 +583,7 @@ textarea { resize: vertical; min-height: 120px; }
</div>
<div id="pull-sheet-status" class="small" aria-live="polite">Choose a pull request.</div>
<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 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>Full conversation</h2><div id="pull-comments"></div>
<button class="conversation-more" id="load-older-pull-comments" type="button" hidden>Load older messages</button>
<div id="pull-conversation-status" class="small" aria-live="assertive"></div>
@ -593,9 +593,17 @@ textarea { resize: vertical; min-height: 120px; }
<button id="send-pull-comment" type="button">Post comment</button>
<div id="pull-comment-status" class="small" aria-live="assertive"></div>
</section>
<details class="pull-review" id="pull-review">
<summary><h2>Review &amp; merge</h2></summary>
<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>
<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>
</details>
<div class="pull-sheet-actions">
<button class="share-work-route" type="button">Share</button>
<button id="merge-pull" type="button" disabled>Merge</button>
<a id="open-pull-gitea" href="#" target="_blank" rel="noopener noreferrer">Open in Gitea</a>
</div>
<nav class="work-session-nav" aria-label="Work session" hidden>
@ -1555,6 +1563,30 @@ textarea { resize: vertical; min-height: 120px; }
}
}
async function loadPullReview() {
if (!selectedPull || !selectedPullDetail?.head_sha) return;
const item = selectedPull;
const readDetail = selectedPullDetail;
qs('#pull-review-status').textContent = 'Loading changed files and merge readiness…';
qs('#pull-review-retry').hidden = true;
qs('#merge-pull').disabled = true;
try {
const review = await pullController.loadReview(item, readDetail.head_sha);
if (selectedPull !== item) return;
selectedPullDetail = { ...readDetail, ...review };
qs('#pull-ci-state').textContent = 'CI ' + (review.ci_state || 'unknown');
renderPullReview(selectedPullDetail);
qs('#pull-review-status').textContent = review.head_sha === readDetail.head_sha ?
'Review data ready for the current head.' : 'New commits detected. Review progress restarted for the latest head.';
} catch (error) {
if (selectedPull !== item) return;
qs('#pull-review-status').textContent = error.message + ' Reading and replies remain available; retry here.';
qs('#pull-review-retry').hidden = false;
qs('#pull-merge-state').textContent = 'Review data unavailable';
qs('#merge-pull').disabled = true;
}
}
async function openPullSheet(item, trigger) {
if (!item) return;
selectedPull = item;
@ -1568,7 +1600,10 @@ textarea { resize: vertical; min-height: 120px; }
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('#pull-review').open = false;
qs('#pull-review-status').textContent = 'Expand to load changed files and merge readiness.';
qs('#pull-review-retry').hidden = true;
qs('#pull-review-progress').textContent = 'Review progress unavailable.';
qs('#next-unreviewed-pull-file').disabled = true;
qs('#pull-comments').textContent = '';
qs('#load-older-pull-comments').hidden = true;
@ -1576,7 +1611,7 @@ textarea { resize: vertical; min-height: 120px; }
qs('#pull-comment').value = pullController.loadDraft(item);
qs('#pull-comment-status').textContent = '';
qs('#pull-ci-state').textContent = 'CI unknown';
qs('#pull-merge-state').textContent = 'Checking merge status';
qs('#pull-merge-state').textContent = 'Review data not loaded';
qs('#merge-pull').disabled = true;
qs('#retry-pull-load').hidden = true;
qs('#open-pull-gitea').href = item.url || '#';
@ -1588,8 +1623,6 @@ textarea { resize: vertical; min-height: 120px; }
pullConversation = pullController.conversation(item, detail.conversation);
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');
renderPullReview(detail);
renderPullConversation(pullConversation.snapshot());
qs('#open-pull-gitea').href = detail.url || item.url || '#';
qs('#pull-sheet-status').textContent = 'Pull request ready · by ' + (detail.author || 'unknown author');
@ -2609,6 +2642,10 @@ textarea { resize: vertical; min-height: 120px; }
qs('#retry-pull-load').addEventListener('click', () => {
if (selectedPull) openPullSheet(selectedPull, pullTrigger);
});
qs('#pull-review').addEventListener('toggle', event => {
if (event.currentTarget.open) loadPullReview();
});
qs('#pull-review-retry').addEventListener('click', loadPullReview);
qs('#next-unreviewed-pull-file').addEventListener('click', focusNextUnreviewedPullFile);
qs('#load-older-pull-comments').addEventListener('click', async () => {
if (!pullConversation) return;

View File

@ -40,6 +40,8 @@ function renderFile(file, index, reviewed, escapeHtml) {
function createPullSheet({ fetchJson, storage, createConversationPager = globalThis.createConversationPager, createOperationId = () => globalThis.crypto?.randomUUID?.() || String(Date.now()) + '-' + Math.random() }) {
let commentRequest = null;
let mergeRequest = null;
const reviewRequests = new Map();
const reviewCache = new Map();
const pathFor = item => 'api/v1/repos/' + String(item.repository || '').split('/')
.map(encodeURIComponent).join('/') + '/pulls/' + encodeURIComponent(item.number);
const draftKey = item => 'stackchain.pull-comment.v1:' + item.repository + '#' + item.number;
@ -62,6 +64,20 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT
load(item) {
return fetchJson(pathFor(item) + '/detail', { headers: { Accept: 'application/json' } });
},
loadReview(item, headSha) {
const key = item.repository + '#' + item.number + ':' + headSha;
if (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' },
}).then(detail => {
const confirmedKey = item.repository + '#' + item.number + ':' + detail.head_sha;
reviewCache.set(confirmedKey, detail);
return detail;
}).finally(() => reviewRequests.delete(key));
reviewRequests.set(key, request);
return request;
},
conversation(item, initialPage) {
const pager = createConversationPager({
loadPage: page => fetchJson(pathFor(item) + '/comments?page=' + encodeURIComponent(page) + '&limit=20', {

View File

@ -1222,18 +1222,9 @@ async def pull_completion_detail(repository: str, number: int) -> dict:
pull = await fetch(base)
if not isinstance(pull, dict):
raise ValueError("Gitea pull request response was not an object")
conversation = await issue_conversation_page(repository, number)
head = pull.get("head") if isinstance(pull.get("head"), dict) else {}
sha = head.get("sha") if isinstance(head.get("sha"), str) else ""
files, status, conversation, diff_result = await asyncio.gather(
fetch(f"{base}/files"),
fetch(f"repos/{repository}/commits/{sha}/status"),
issue_conversation_page(repository, number),
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,
@ -1244,6 +1235,33 @@ async def pull_completion_detail(repository: str, number: int) -> dict:
"author": user.get("login") if isinstance(user.get("login"), str) else "",
"head_sha": sha,
"state": pull.get("state") if isinstance(pull.get("state"), str) else "",
"conversation": conversation,
}
async def pull_completion_review(repository: str, number: int) -> dict:
base = f"repos/{repository}/pulls/{number}"
pull = await fetch(base)
if not isinstance(pull, dict):
raise ValueError("Gitea pull request response was not an object")
head_value = pull.get("head")
head: dict = head_value if isinstance(head_value, dict) else {}
sha_value = head.get("sha")
sha = sha_value if isinstance(sha_value, str) else ""
files, status, diff_result = await asyncio.gather(
fetch(f"{base}/files"),
fetch(f"repos/{repository}/commits/{sha}/status"),
fetch_text(
f"repos/{repository}/pulls/{number}.diff", REVIEW_DIFF_MAX_BYTES
),
)
diff, diff_truncated = diff_result
previews = _diff_previews(diff, diff_truncated)
return {
"repository": repository,
"number": number,
"head_sha": sha,
"state": pull.get("state") if isinstance(pull.get("state"), str) else "",
"draft": pull.get("draft") is True,
"mergeable": pull.get("mergeable") is True,
"merged": pull.get("merged") is True,
@ -1267,8 +1285,6 @@ async def pull_completion_detail(repository: str, number: int) -> dict:
for item in (files if isinstance(files, list) else [])[:100]
if isinstance(item, dict) and isinstance(item.get("filename"), str)
],
"comments": conversation["comments"],
"conversation": conversation,
}

View File

@ -1642,6 +1642,37 @@ async def assigned_pull_detail(owner: str, repo: str, number: int = PathParam(gt
)
@app.get("/api/v1/repos/{owner}/{repo}/pulls/{number}/review-data")
async def assigned_pull_review_data(
owner: str, repo: str, number: int = PathParam(gt=0)
):
repository = f"{owner}/{repo}"
async def load_assigned_pull_review():
if not await gitea_proxy.is_assigned_pull(repository, number):
raise HTTPException(status_code=404, detail="Assigned pull request not found")
return await gitea_proxy.pull_completion_review(repository, number)
try:
return await asyncio.wait_for(
load_assigned_pull_review(), timeout=REVIEW_DETAIL_TIMEOUT_SECONDS
)
except HTTPException:
raise
except TimeoutError:
return JSONResponse(
{"error": "Loading review and merge data timed out. Please retry."},
status_code=503,
headers={"Retry-After": "1"},
)
except Exception:
return JSONResponse(
{"error": "Review and merge data is temporarily unavailable. Please retry."},
status_code=503,
headers={"Retry-After": "1"},
)
@app.get("/api/v1/repos/{owner}/{repo}/pulls/{number}/comments")
async def assigned_pull_conversation(
owner: str,

View File

@ -2023,6 +2023,63 @@ process.stdout.write(JSON.stringify({{
assert output["next"] == "frontend/app.js"
def test_pull_sheet_lazy_review_is_single_flight_cached_by_head_and_retryable():
script = f"""
const createPullSheet = require({json.dumps(str(PULL_SHEET))});
const calls = [];
let rejectFirst;
const fetchJson = url => {{
calls.push(url);
if (calls.length === 1) return new Promise((_resolve, reject) => {{ rejectFirst = reject; }});
return Promise.resolve({{head_sha:'abc123', files:[]}});
}};
const sheet = createPullSheet({{fetchJson, storage:null}});
const item = {{repository:'stackchain/api', number:7}};
const first = sheet.loadReview(item, 'abc123');
const concurrent = sheet.loadReview(item, 'abc123');
rejectFirst(new Error('diff unavailable'));
Promise.allSettled([first, concurrent]).then(async failed => {{
const retried = await sheet.loadReview(item, 'abc123');
const cached = await sheet.loadReview(item, 'abc123');
process.stdout.write(JSON.stringify({{
same:first === concurrent,
failed:failed.map(result => result.status),
retried,
cached,
calls,
}}));
}});
"""
result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True)
output = json.loads(result.stdout)
assert output["same"] is True
assert output["failed"] == ["rejected", "rejected"]
assert output["retried"]["head_sha"] == "abc123"
assert output["cached"] == output["retried"]
assert output["calls"] == [
"api/v1/repos/stackchain/api/pulls/7/review-data",
"api/v1/repos/stackchain/api/pulls/7/review-data",
]
@pytest.mark.anyio
async def test_mobile_pull_sheet_puts_reading_before_collapsed_review_controls():
html = await dashboard()
body = html.index('id="pull-sheet-body"')
conversation = html.index('<h2>Full conversation</h2>', body)
composer = html.index('id="pull-comment-title"', conversation)
review = html.index('id="pull-review"', composer)
files = html.index('id="pull-files"', review)
assert '<summary><h2>Review &amp; merge</h2></summary>' in html
assert '<details class="pull-review" id="pull-review">' in html
assert 'id="pull-review-retry"' in html
assert 'id="pull-review-status"' in html
assert body < conversation < composer < review < files
def test_pull_sheet_renders_mobile_diff_fallbacks_and_review_controls():
script = f"""
const createPullSheet = require({json.dumps(str(PULL_SHEET))});

View File

@ -54,7 +54,77 @@ async def test_assigned_pull_detail_rejects_unassigned_pull(monkeypatch):
@pytest.mark.anyio
async def test_gitea_assigned_pull_detail_includes_bounded_diff_previews():
async def test_gitea_assigned_pull_detail_loads_reading_without_review_resources():
requests = []
async def handler(request):
requests.append((request.method, request.url.path))
if request.url.path.endswith("/pulls/7"):
return httpx.Response(200, json={
"number": 7,
"title": "Read this first",
"body": "The discussion should not wait for the diff.",
"state": "open",
"mergeable": True,
"head": {"sha": "abc123"},
"user": {"login": "alex"},
})
if request.url.path.endswith("/issues/7/comments"):
return httpx.Response(200, json=[{
"id": 9, "body": "Question", "user": {"login": "sam"},
"created_at": "2026-08-07T18:00:00Z",
}], headers={"X-Total-Count": "1"})
raise AssertionError(f"review resource requested during read load: {request.url.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 requests == [
("GET", "/api/v1/repos/stackchain/api/pulls/7"),
("GET", "/api/v1/repos/stackchain/api/issues/7/comments"),
]
assert detail["head_sha"] == "abc123"
assert detail["conversation"]["comments"][0]["body"] == "Question"
assert "files" not in detail
assert "ci_state" not in detail
@pytest.mark.anyio
async def test_assigned_pull_review_endpoint_loads_review_payload_on_demand(monkeypatch):
calls = []
async def assigned(repository, number):
calls.append(("assigned", repository, number))
return True
async def review(repository, number):
calls.append(("review", repository, number))
return {
"repository": repository, "number": number, "head_sha": "abc123",
"state": "open", "draft": False, "mergeable": True, "merged": False,
"ci_state": "success", "files": [{"filename": "src/api.py"}],
}
monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned)
monkeypatch.setattr(main.gitea_proxy, "pull_completion_review", review, raising=False)
transport = httpx.ASGITransport(app=main.app)
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
response = await client.get("/api/v1/repos/stackchain/api/pulls/7/review-data")
assert response.status_code == 200
assert response.headers["cache-control"] == "no-store"
assert response.json()["files"] == [{"filename": "src/api.py"}]
assert calls == [
("assigned", "stackchain/api", 7),
("review", "stackchain/api", 7),
]
@pytest.mark.anyio
async def test_gitea_assigned_pull_review_includes_bounded_diff_previews():
async def handler(request):
path = request.url.path
if path.endswith("/pulls/7"):
@ -86,7 +156,7 @@ async def test_gitea_assigned_pull_detail_includes_bounded_diff_previews():
gitea_proxy.start_client(transport=httpx.MockTransport(handler))
try:
detail = await gitea_proxy.pull_completion_detail("stackchain/api", 7)
detail = await gitea_proxy.pull_completion_review("stackchain/api", 7)
finally:
await gitea_proxy.stop_client()
@ -94,9 +164,7 @@ async def test_gitea_assigned_pull_detail_includes_bounded_diff_previews():
assert "+new" in detail["files"][0]["diff_lines"]
assert detail["files"][1]["diff_binary"] is True
assert detail["files"][1]["diff_available"] is False
assert detail["conversation"] == {
"comments": [], "page": 1, "older_page": None, "total": 0
}
assert "conversation" not in detail
@pytest.mark.anyio