Make assigned pull request sheets read-first with on-demand review #218
|
|
@ -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 & 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;
|
||||
|
|
|
|||
|
|
@ -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', {
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
}
|
||||
|
||||
|
||||
|
|
|
|||
31
src/main.py
31
src/main.py
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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 & 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))});
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user