Merge pull request 'Load pull review feedback on demand without blocking mobile review' (#1343) from timmy/1342-lazy-review-feedback into main
This commit is contained in:
commit
1a2297e943
|
|
@ -1275,7 +1275,7 @@ textarea { resize: vertical; min-height: 120px; }
|
||||||
.pull-review-feedback-file li { margin:6px 0; }
|
.pull-review-feedback-file li { margin:6px 0; }
|
||||||
.pull-review-feedback-file small { display:block; color:#93c5fd; }
|
.pull-review-feedback-file small { display:block; color:#93c5fd; }
|
||||||
.pull-review-feedback-file button { min-height:44px; width:100%; margin-top:4px; }
|
.pull-review-feedback-file button { min-height:44px; width:100%; margin-top:4px; }
|
||||||
.address-review-feedback { min-height:44px; width:100%; margin-top:8px; }
|
.address-review-feedback, [data-load-review-feedback] { min-height:44px; width:100%; margin-top:8px; }
|
||||||
.pull-feedback-pass { margin-top:10px; padding:10px; border:1px solid #3b82f6; border-radius:10px; overflow-wrap:anywhere; }
|
.pull-feedback-pass { margin-top:10px; padding:10px; border:1px solid #3b82f6; border-radius:10px; overflow-wrap:anywhere; }
|
||||||
.pull-feedback-pass-heading { display:flex; align-items:start; justify-content:space-between; gap:8px; }
|
.pull-feedback-pass-heading { display:flex; align-items:start; justify-content:space-between; gap:8px; }
|
||||||
.pull-feedback-pass-heading h4 { margin:2px 0 8px; }
|
.pull-feedback-pass-heading h4 { margin:2px 0 8px; }
|
||||||
|
|
|
||||||
|
|
@ -44,16 +44,26 @@ function renderReviewerFeedback(reviewer, escapeHtml) {
|
||||||
'</ul><button type="button" data-review-feedback-file="' + escapeHtml(path) +
|
'</ul><button type="button" data-review-feedback-file="' + escapeHtml(path) +
|
||||||
'">View in changes</button></section>'
|
'">View in changes</button></section>'
|
||||||
).join('');
|
).join('');
|
||||||
if (!summary && !files) return '';
|
const loadable = Number.isInteger(reviewer?.review_id) && reviewer.review_id > 0 &&
|
||||||
|
['commented', 'changes_requested', 'outdated'].includes(reviewer?.status);
|
||||||
|
const feedbackState = Array.isArray(reviewer?.comments) ? 'loaded' : reviewer?.feedback_state;
|
||||||
|
const feedbackAction = !loadable || feedbackState === 'loaded' ? '' :
|
||||||
|
feedbackState === 'loading' ? '<p class="small" role="status">Loading inline feedback…</p>' :
|
||||||
|
'<p class="small" role="status">' + (feedbackState === 'error' ? 'Feedback unavailable. ' : '') +
|
||||||
|
'<button type="button" data-load-review-feedback="' + Number(reviewer.review_id) + '">' +
|
||||||
|
(feedbackState === 'error' ? 'Retry feedback' : 'Load inline feedback') + '</button></p>';
|
||||||
|
const emptyLoaded = loadable && feedbackState === 'loaded' && !files ?
|
||||||
|
'<p class="small">No inline comments on this review.</p>' : '';
|
||||||
|
if (!summary && !files && !feedbackAction && !emptyLoaded) return '';
|
||||||
const reviewedHead = reviewer?.status === 'outdated' && reviewer?.head_sha ?
|
const reviewedHead = reviewer?.status === 'outdated' && reviewer?.head_sha ?
|
||||||
'<p class="small">Reviewed head ' + escapeHtml(reviewer.head_sha.slice(0, 8)) + '</p>' : '';
|
'<p class="small">Reviewed head ' + escapeHtml(reviewer.head_sha.slice(0, 8)) + '</p>' : '';
|
||||||
const addressable = Number.isInteger(reviewer?.review_id) && reviewer.review_id > 0 &&
|
const addressable = Number.isInteger(reviewer?.review_id) && reviewer.review_id > 0 &&
|
||||||
['changes_requested', 'outdated'].includes(reviewer?.status) &&
|
['changes_requested', 'outdated'].includes(reviewer?.status) && feedbackState === 'loaded' &&
|
||||||
(reviewer.comments || []).some(comment => Number.isInteger(comment?.id) && comment.id > 0);
|
(reviewer.comments || []).some(comment => Number.isInteger(comment?.id) && comment.id > 0);
|
||||||
const address = addressable ? '<button type="button" class="address-review-feedback" data-address-review-feedback="' +
|
const address = addressable ? '<button type="button" class="address-review-feedback" data-address-review-feedback="' +
|
||||||
Number(reviewer.review_id) + '">Address feedback</button>' : '';
|
Number(reviewer.review_id) + '">Address feedback</button>' : '';
|
||||||
return '<details class="pull-review-feedback"><summary>Review feedback</summary>' +
|
return '<details class="pull-review-feedback"><summary>Review feedback</summary>' +
|
||||||
reviewedHead + summary + files + address + '</details>';
|
reviewedHead + summary + feedbackAction + emptyLoaded + files + address + '</details>';
|
||||||
}
|
}
|
||||||
|
|
||||||
function renderReviewerStatuses(reviewers, escapeHtml) {
|
function renderReviewerStatuses(reviewers, escapeHtml) {
|
||||||
|
|
@ -70,7 +80,9 @@ function renderReviewerStatuses(reviewers, escapeHtml) {
|
||||||
'<button type="button" class="cancel-review-request" data-cancel-review-request="' +
|
'<button type="button" class="cancel-review-request" data-cancel-review-request="' +
|
||||||
escapeHtml(reviewer.login) + '">Cancel request</button>' : '';
|
escapeHtml(reviewer.login) + '">Cancel request</button>' : '';
|
||||||
return '<article class="pull-reviewer-status" data-reviewer-status="' +
|
return '<article class="pull-reviewer-status" data-reviewer-status="' +
|
||||||
escapeHtml(reviewer?.status || 'unknown') + '"><div class="pull-reviewer-status-heading"><strong>@' +
|
escapeHtml(reviewer?.status || 'unknown') + '" data-review-id="' +
|
||||||
|
(Number.isInteger(reviewer?.review_id) ? Number(reviewer.review_id) : '') +
|
||||||
|
'"><div class="pull-reviewer-status-heading"><strong>@' +
|
||||||
escapeHtml(reviewer?.login || 'unknown') + '</strong><span>' + escapeHtml(status) + '</span></div>' +
|
escapeHtml(reviewer?.login || 'unknown') + '</strong><span>' + escapeHtml(status) + '</span></div>' +
|
||||||
renderReviewerFeedback(reviewer, escapeHtml) + cancel + '</article>';
|
renderReviewerFeedback(reviewer, escapeHtml) + cancel + '</article>';
|
||||||
}).join('');
|
}).join('');
|
||||||
|
|
@ -79,7 +91,13 @@ function renderReviewerStatuses(reviewers, escapeHtml) {
|
||||||
function renderReviewerPanel(doc, detail, escapeHtml = value => String(value)
|
function renderReviewerPanel(doc, detail, escapeHtml = value => String(value)
|
||||||
.replaceAll('&', '&').replaceAll('<', '<').replaceAll('>', '>')) {
|
.replaceAll('&', '&').replaceAll('<', '<').replaceAll('>', '>')) {
|
||||||
const reviewers = Array.isArray(detail?.reviewers) ? detail.reviewers : [];
|
const reviewers = Array.isArray(detail?.reviewers) ? detail.reviewers : [];
|
||||||
|
const openFeedback = new Set(Array.from(doc.querySelectorAll('.pull-review-feedback[open]'))
|
||||||
|
.map(panel => panel.closest('.pull-reviewer-status')?.dataset.reviewId).filter(Boolean));
|
||||||
doc.querySelector('#pull-reviewer-statuses').innerHTML = renderReviewerStatuses(reviewers, escapeHtml);
|
doc.querySelector('#pull-reviewer-statuses').innerHTML = renderReviewerStatuses(reviewers, escapeHtml);
|
||||||
|
openFeedback.forEach(reviewId => {
|
||||||
|
const panel = doc.querySelector('.pull-reviewer-status[data-review-id="' + reviewId + '"] .pull-review-feedback');
|
||||||
|
if (panel) panel.open = true;
|
||||||
|
});
|
||||||
doc.querySelectorAll('[data-review-feedback-file]').forEach(button => {
|
doc.querySelectorAll('[data-review-feedback-file]').forEach(button => {
|
||||||
button.addEventListener('click', () => focusPullFile(doc, button.dataset.reviewFeedbackFile));
|
button.addEventListener('click', () => focusPullFile(doc, button.dataset.reviewFeedbackFile));
|
||||||
});
|
});
|
||||||
|
|
@ -336,7 +354,33 @@ function bindFeedbackControls(doc, controller, getSelected, getDetail, getLogin)
|
||||||
'Matching change opened below.' : 'Matching change is unavailable in this preview.';
|
'Matching change opened below.' : 'Matching change is unavailable in this preview.';
|
||||||
panel.scrollIntoView({ block:'center', behavior:'smooth' });
|
panel.scrollIntoView({ block:'center', behavior:'smooth' });
|
||||||
};
|
};
|
||||||
qs('#pull-reviewer-statuses').addEventListener('click', event => {
|
qs('#pull-reviewer-statuses').addEventListener('click', async event => {
|
||||||
|
const loadButton = event.target.closest?.('[data-load-review-feedback]');
|
||||||
|
if (loadButton && !loadButton.disabled) {
|
||||||
|
const detail = getDetail?.();
|
||||||
|
const item = getSelected?.();
|
||||||
|
const reviewId = Number(loadButton.dataset.loadReviewFeedback);
|
||||||
|
const reviewer = (detail?.reviewers || []).find(candidate => candidate.review_id === reviewId);
|
||||||
|
if (!detail || !item || !reviewer) return;
|
||||||
|
const refresh = reviewer.feedback_state === 'error';
|
||||||
|
reviewer.feedback_state = 'loading';
|
||||||
|
renderReviewerPanel(doc, detail);
|
||||||
|
try {
|
||||||
|
const result = await controller.loadFeedback(item, detail, reviewer, { refresh });
|
||||||
|
if (getDetail?.() !== detail || getSelected?.() !== item ||
|
||||||
|
result.head_sha !== detail.head_sha || Number(result.review_id) !== reviewId) return;
|
||||||
|
reviewer.comments = Array.isArray(result.comments) ? result.comments : [];
|
||||||
|
reviewer.feedback_state = 'loaded';
|
||||||
|
renderReviewerPanel(doc, detail);
|
||||||
|
doc.querySelector('[data-address-review-feedback="' + reviewId + '"]')?.focus();
|
||||||
|
} catch (error) {
|
||||||
|
if (getDetail?.() !== detail || getSelected?.() !== item) return;
|
||||||
|
reviewer.feedback_state = 'error';
|
||||||
|
renderReviewerPanel(doc, detail);
|
||||||
|
doc.querySelector('[data-load-review-feedback="' + reviewId + '"]')?.focus();
|
||||||
|
}
|
||||||
|
return;
|
||||||
|
}
|
||||||
const button = event.target.closest?.('[data-address-review-feedback]');
|
const button = event.target.closest?.('[data-address-review-feedback]');
|
||||||
if (!button) return;
|
if (!button) return;
|
||||||
const detail = getDetail?.();
|
const detail = getDetail?.();
|
||||||
|
|
@ -570,6 +614,8 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT
|
||||||
let feedbackRequest = null;
|
let feedbackRequest = null;
|
||||||
const reviewRequests = new Map();
|
const reviewRequests = new Map();
|
||||||
const reviewCache = new Map();
|
const reviewCache = new Map();
|
||||||
|
const feedbackRequests = new Map();
|
||||||
|
const feedbackCache = new Map();
|
||||||
const pathFor = item => 'api/v1/repos/' + String(item.repository || '').split('/')
|
const pathFor = item => 'api/v1/repos/' + String(item.repository || '').split('/')
|
||||||
.map(encodeURIComponent).join('/') + '/pulls/' + encodeURIComponent(item.number);
|
.map(encodeURIComponent).join('/') + '/pulls/' + encodeURIComponent(item.number);
|
||||||
const draftKey = item => 'stackchain.pull-comment.v1:' + item.repository + '#' + item.number;
|
const draftKey = item => 'stackchain.pull-comment.v1:' + item.repository + '#' + item.number;
|
||||||
|
|
@ -609,6 +655,25 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT
|
||||||
reviewRequests.set(key, request);
|
reviewRequests.set(key, request);
|
||||||
return request;
|
return request;
|
||||||
},
|
},
|
||||||
|
loadFeedback(item, detail, reviewer, { refresh = false } = {}) {
|
||||||
|
const reviewId = Number(reviewer?.review_id);
|
||||||
|
const headSha = detail?.head_sha || '';
|
||||||
|
const key = item.repository + '#' + item.number + ':' + headSha + ':' + reviewId;
|
||||||
|
if (!refresh && feedbackCache.has(key)) return Promise.resolve(feedbackCache.get(key));
|
||||||
|
if (feedbackRequests.has(key)) return feedbackRequests.get(key);
|
||||||
|
const request = fetchJson(pathFor(item) + '/reviews/' + encodeURIComponent(reviewId) +
|
||||||
|
'/feedback?expected_head_sha=' + encodeURIComponent(headSha), {
|
||||||
|
headers: { Accept: 'application/json' },
|
||||||
|
}).then(result => {
|
||||||
|
if (result?.head_sha !== headSha || Number(result?.review_id) !== reviewId) {
|
||||||
|
throw new Error('Review feedback changed. Reload review data before retrying.');
|
||||||
|
}
|
||||||
|
feedbackCache.set(key, result);
|
||||||
|
return result;
|
||||||
|
}).finally(() => feedbackRequests.delete(key));
|
||||||
|
feedbackRequests.set(key, request);
|
||||||
|
return request;
|
||||||
|
},
|
||||||
loadChecks(item) {
|
loadChecks(item) {
|
||||||
if (checkRequest) return checkRequest;
|
if (checkRequest) return checkRequest;
|
||||||
checkRequest = fetchJson(pathFor(item) + '/checks', {
|
checkRequest = fetchJson(pathFor(item) + '/checks', {
|
||||||
|
|
|
||||||
|
|
@ -2938,20 +2938,6 @@ async def pull_completion_review(repository: str, number: int) -> dict:
|
||||||
diff, diff_truncated = diff_result
|
diff, diff_truncated = diff_result
|
||||||
previews = _diff_previews(diff, diff_truncated)
|
previews = _diff_previews(diff, diff_truncated)
|
||||||
reviewers = _normalize_reviewer_statuses(pull, reviews, sha)
|
reviewers = _normalize_reviewer_statuses(pull, reviews, sha)
|
||||||
feedback_ids = _latest_feedback_review_ids(reviews)
|
|
||||||
if feedback_ids:
|
|
||||||
feedback_payloads = await asyncio.gather(*(
|
|
||||||
fetch(f"{base}/reviews/{review_id}/comments")
|
|
||||||
for review_id in feedback_ids.values()
|
|
||||||
))
|
|
||||||
feedback = {
|
|
||||||
login: _normalize_review_comments(payload)
|
|
||||||
for login, payload in zip(feedback_ids, feedback_payloads)
|
|
||||||
}
|
|
||||||
for reviewer in reviewers:
|
|
||||||
comments = feedback.get(reviewer["login"].casefold(), [])
|
|
||||||
if comments:
|
|
||||||
reviewer["comments"] = comments
|
|
||||||
return {
|
return {
|
||||||
"repository": repository,
|
"repository": repository,
|
||||||
"number": number,
|
"number": number,
|
||||||
|
|
@ -2985,6 +2971,40 @@ async def pull_completion_review(repository: str, number: int) -> dict:
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
async def pull_review_feedback(
|
||||||
|
repository: str, number: int, review_id: int, expected_head_sha: str
|
||||||
|
) -> dict:
|
||||||
|
"""Load optional inline feedback only for a review on the confirmed pull head."""
|
||||||
|
base = f"repos/{repository}/pulls/{number}"
|
||||||
|
pull, reviews = await asyncio.gather(
|
||||||
|
fetch(base),
|
||||||
|
fetch(f"{base}/reviews?limit=100"),
|
||||||
|
)
|
||||||
|
head = pull.get("head") if isinstance(pull, dict) else None
|
||||||
|
head_sha = head.get("sha") if isinstance(head, dict) else None
|
||||||
|
if head_sha != expected_head_sha:
|
||||||
|
raise StaleReviewError("Pull request head changed")
|
||||||
|
review_items = reviews if isinstance(reviews, list) else []
|
||||||
|
latest_ids = set(_latest_feedback_review_ids(review_items).values())
|
||||||
|
review = next(
|
||||||
|
(
|
||||||
|
item for item in review_items
|
||||||
|
if isinstance(item, dict) and item.get("id") == review_id
|
||||||
|
),
|
||||||
|
None,
|
||||||
|
)
|
||||||
|
if review is None or review_id not in latest_ids:
|
||||||
|
raise StaleReviewError("Review feedback is no longer current")
|
||||||
|
comments = await fetch(f"{base}/reviews/{review_id}/comments")
|
||||||
|
reviewed_head = review.get("commit_id")
|
||||||
|
return {
|
||||||
|
"review_id": review_id,
|
||||||
|
"head_sha": head_sha,
|
||||||
|
"reviewed_head_sha": reviewed_head if isinstance(reviewed_head, str) else "",
|
||||||
|
"comments": _normalize_review_comments(comments),
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
async def release_receipt_status(repository: str, commit_sha: str) -> dict:
|
async def release_receipt_status(repository: str, commit_sha: str) -> dict:
|
||||||
"""Return CI and release evidence for one exact merge commit."""
|
"""Return CI and release evidence for one exact merge commit."""
|
||||||
status, releases = await asyncio.gather(
|
status, releases = await asyncio.gather(
|
||||||
|
|
|
||||||
44
src/main.py
44
src/main.py
|
|
@ -6640,6 +6640,50 @@ async def assigned_pull_review_data(
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@app.get("/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews/{review_id}/feedback")
|
||||||
|
async def assigned_pull_review_feedback(
|
||||||
|
owner: str,
|
||||||
|
repo: str,
|
||||||
|
number: int = PathParam(gt=0),
|
||||||
|
review_id: int = PathParam(gt=0),
|
||||||
|
expected_head_sha: str = Query(min_length=7, max_length=64, pattern=r"^[A-Fa-f0-9]+$"),
|
||||||
|
):
|
||||||
|
repository = f"{owner}/{repo}"
|
||||||
|
|
||||||
|
async def load_feedback():
|
||||||
|
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_review_feedback(
|
||||||
|
repository, number, review_id, expected_head_sha
|
||||||
|
)
|
||||||
|
|
||||||
|
try:
|
||||||
|
result = await asyncio.wait_for(
|
||||||
|
load_feedback(), timeout=REVIEW_DETAIL_TIMEOUT_SECONDS
|
||||||
|
)
|
||||||
|
except HTTPException:
|
||||||
|
raise
|
||||||
|
except gitea_proxy.StaleReviewError:
|
||||||
|
return JSONResponse(
|
||||||
|
{"error": "The pull request or review changed. Reload review data before retrying."},
|
||||||
|
status_code=409,
|
||||||
|
headers={"Cache-Control": "no-store"},
|
||||||
|
)
|
||||||
|
except TimeoutError:
|
||||||
|
return JSONResponse(
|
||||||
|
{"error": "Loading review feedback timed out. Please retry."},
|
||||||
|
status_code=503,
|
||||||
|
headers={"Cache-Control": "no-store", "Retry-After": "1"},
|
||||||
|
)
|
||||||
|
except Exception:
|
||||||
|
return JSONResponse(
|
||||||
|
{"error": "Review feedback is temporarily unavailable. Please retry."},
|
||||||
|
status_code=503,
|
||||||
|
headers={"Cache-Control": "no-store", "Retry-After": "1"},
|
||||||
|
)
|
||||||
|
return JSONResponse(result, headers={"Cache-Control": "no-store"})
|
||||||
|
|
||||||
|
|
||||||
@app.get("/api/v1/repos/{owner}/{repo}/pulls/{number}/checks")
|
@app.get("/api/v1/repos/{owner}/{repo}/pulls/{number}/checks")
|
||||||
async def assigned_pull_checks(
|
async def assigned_pull_checks(
|
||||||
owner: str, repo: str, number: int = PathParam(gt=0)
|
owner: str, repo: str, number: int = PathParam(gt=0)
|
||||||
|
|
|
||||||
|
|
@ -90,3 +90,83 @@ def test_mobile_received_review_feedback_opens_matching_change(viewport):
|
||||||
assert metrics["focused"] is True
|
assert metrics["focused"] is True
|
||||||
assert "Reviewed head old-head" in metrics["reviewedHead"]
|
assert "Reviewed head old-head" in metrics["reviewedHead"]
|
||||||
assert metrics["updatedReviewVisible"] is True
|
assert metrics["updatedReviewVisible"] is True
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("viewport", [(320, 568), (390, 844)])
|
||||||
|
def test_mobile_pull_review_retries_inline_feedback_without_losing_core_review(viewport):
|
||||||
|
playwright = pytest.importorskip("playwright.sync_api")
|
||||||
|
html = (ROOT / "frontend" / "index.html").read_text()
|
||||||
|
detail = {
|
||||||
|
"state": "open", "draft": False, "mergeable": True, "merged": False,
|
||||||
|
"ci_state": "success", "head_sha": "abc1234",
|
||||||
|
"reviewers": [{
|
||||||
|
"login": "sam", "review_id": 8, "status": "changes_requested",
|
||||||
|
"head_sha": "abc1234", "blocking": True,
|
||||||
|
"summary": "Please handle the empty state.",
|
||||||
|
}],
|
||||||
|
"files": [],
|
||||||
|
}
|
||||||
|
|
||||||
|
with playwright.sync_playwright() as runtime:
|
||||||
|
try:
|
||||||
|
browser = runtime.chromium.launch(headless=True)
|
||||||
|
except Exception as error:
|
||||||
|
pytest.skip(f"Chromium is not installed: {error}")
|
||||||
|
page = browser.new_page(viewport={"width": viewport[0], "height": viewport[1]})
|
||||||
|
page.set_content(html, wait_until="domcontentloaded")
|
||||||
|
page.add_style_tag(path=ROOT / "frontend" / "dashboard.css")
|
||||||
|
page.add_script_tag(path=ROOT / "frontend" / "pull-sheet.js")
|
||||||
|
page.evaluate(
|
||||||
|
"""detail => {
|
||||||
|
window.lazyFeedbackDetail = detail;
|
||||||
|
window.lazyFeedbackItem = {repository:'stackchain/api', number:7, key:'stackchain/api#7'};
|
||||||
|
let attempts = 0;
|
||||||
|
const controller = createPullSheet({
|
||||||
|
storage:null,
|
||||||
|
fetchJson:() => {
|
||||||
|
attempts += 1;
|
||||||
|
if (attempts === 1) return Promise.reject(new Error('feedback unavailable'));
|
||||||
|
return Promise.resolve({
|
||||||
|
review_id:8, head_sha:'abc1234', reviewed_head_sha:'abc1234',
|
||||||
|
comments:[{id:99, path:'src/api.py', body:'Return before parsing.', line:12}],
|
||||||
|
});
|
||||||
|
},
|
||||||
|
});
|
||||||
|
createPullSheet.bindFeedbackControls(
|
||||||
|
document, controller, () => window.lazyFeedbackItem,
|
||||||
|
() => window.lazyFeedbackDetail, () => 'timmy'
|
||||||
|
);
|
||||||
|
document.querySelector('#pull-sheet').classList.add('open');
|
||||||
|
document.querySelector('#pull-review').open = true;
|
||||||
|
const eligibility = createPullSheet.review(detail, null, document);
|
||||||
|
document.querySelector('#pull-merge-state').textContent = eligibility.reason;
|
||||||
|
}""",
|
||||||
|
detail,
|
||||||
|
)
|
||||||
|
|
||||||
|
page.locator(".pull-review-feedback summary").click()
|
||||||
|
load = page.get_by_role("button", name="Load inline feedback")
|
||||||
|
load.click()
|
||||||
|
retry = page.get_by_role("button", name="Retry feedback")
|
||||||
|
retry.wait_for()
|
||||||
|
retry.click()
|
||||||
|
page.get_by_text("Return before parsing.").wait_for()
|
||||||
|
metrics = page.evaluate(
|
||||||
|
"""() => {
|
||||||
|
const details = document.querySelector('.pull-review-feedback');
|
||||||
|
const address = document.querySelector('[data-address-review-feedback]');
|
||||||
|
return {
|
||||||
|
scrollWidth: document.documentElement.scrollWidth,
|
||||||
|
clientWidth: document.documentElement.clientWidth,
|
||||||
|
blocker: document.querySelector('#pull-merge-state').textContent,
|
||||||
|
feedback: details.textContent,
|
||||||
|
addressHeight: address.getBoundingClientRect().height,
|
||||||
|
};
|
||||||
|
}"""
|
||||||
|
)
|
||||||
|
browser.close()
|
||||||
|
|
||||||
|
assert metrics["scrollWidth"] <= metrics["clientWidth"]
|
||||||
|
assert "requested changes" in metrics["blocker"]
|
||||||
|
assert "Return before parsing." in metrics["feedback"]
|
||||||
|
assert metrics["addressHeight"] >= 44
|
||||||
|
|
|
||||||
|
|
@ -7867,6 +7867,38 @@ process.stdout.write(html);
|
||||||
assert "Please <split>" not in html
|
assert "Please <split>" not in html
|
||||||
|
|
||||||
|
|
||||||
|
def test_pull_sheet_marks_inline_feedback_as_on_demand_and_retryable():
|
||||||
|
script = f"""
|
||||||
|
const createPullSheet = require({json.dumps(str(PULL_SHEET))});
|
||||||
|
const render = state => createPullSheet.renderReviewerStatuses([{{
|
||||||
|
login:'sam', review_id:8, status:'changes_requested', head_sha:'abc1234',
|
||||||
|
blocking:true, summary:'Please fix this.', feedback_state:state,
|
||||||
|
}}], value => String(value));
|
||||||
|
process.stdout.write(JSON.stringify({{idle:render(undefined), failed:render('error')}}));
|
||||||
|
"""
|
||||||
|
result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True)
|
||||||
|
output = json.loads(result.stdout)
|
||||||
|
|
||||||
|
assert "Load inline feedback" in output["idle"]
|
||||||
|
assert 'data-load-review-feedback="8"' in output["idle"]
|
||||||
|
assert "Feedback unavailable" in output["failed"]
|
||||||
|
assert "Retry feedback" in output["failed"]
|
||||||
|
assert "Address feedback" not in output["idle"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_pull_sheet_binds_on_demand_feedback_with_visible_retry_state():
|
||||||
|
script = PULL_SHEET.read_text()
|
||||||
|
|
||||||
|
assert "event.target.closest?.('[data-load-review-feedback]')" in script
|
||||||
|
assert "await controller.loadFeedback(item, detail, reviewer" in script
|
||||||
|
assert "reviewer.feedback_state = 'loading'" in script
|
||||||
|
assert "reviewer.feedback_state = 'loaded'" in script
|
||||||
|
assert "reviewer.feedback_state = 'error'" in script
|
||||||
|
assert "result.head_sha !== detail.head_sha" in script
|
||||||
|
css = (Path(__file__).parents[1] / "frontend" / "dashboard.css").read_text()
|
||||||
|
assert "[data-load-review-feedback] { min-height:44px; width:100%;" in css
|
||||||
|
|
||||||
|
|
||||||
def test_pull_sheet_persists_head_scoped_file_review_and_gates_merge():
|
def test_pull_sheet_persists_head_scoped_file_review_and_gates_merge():
|
||||||
script = f"""
|
script = f"""
|
||||||
const createPullSheet = require({json.dumps(str(PULL_SHEET))});
|
const createPullSheet = require({json.dumps(str(PULL_SHEET))});
|
||||||
|
|
@ -7942,6 +7974,37 @@ Promise.allSettled([first, concurrent]).then(async failed => {{
|
||||||
]
|
]
|
||||||
|
|
||||||
|
|
||||||
|
def test_pull_sheet_feedback_is_single_flight_cached_by_head_and_review():
|
||||||
|
script = f"""
|
||||||
|
const createPullSheet = require({json.dumps(str(PULL_SHEET))});
|
||||||
|
const calls = [];
|
||||||
|
let resolveFeedback;
|
||||||
|
const sheet = createPullSheet({{storage:null, fetchJson:url => {{
|
||||||
|
calls.push(url);
|
||||||
|
return new Promise(resolve => {{ resolveFeedback = resolve; }});
|
||||||
|
}}}});
|
||||||
|
const item = {{repository:'stackchain/api', number:7}};
|
||||||
|
const detail = {{head_sha:'abc1234'}};
|
||||||
|
const reviewer = {{review_id:8, head_sha:'abc1234'}};
|
||||||
|
const first = sheet.loadFeedback(item, detail, reviewer);
|
||||||
|
const concurrent = sheet.loadFeedback(item, detail, reviewer);
|
||||||
|
resolveFeedback({{review_id:8, head_sha:'abc1234', reviewed_head_sha:'abc1234', comments:[{{id:99}}]}});
|
||||||
|
Promise.all([first, concurrent]).then(async results => {{
|
||||||
|
const cached = await sheet.loadFeedback(item, detail, reviewer);
|
||||||
|
process.stdout.write(JSON.stringify({{same:first === concurrent, results, cached, calls}}));
|
||||||
|
}});
|
||||||
|
"""
|
||||||
|
result = subprocess.run(["node", "-e", script], capture_output=True, text=True)
|
||||||
|
|
||||||
|
assert result.returncode == 0, result.stderr
|
||||||
|
output = json.loads(result.stdout)
|
||||||
|
assert output["same"] is True
|
||||||
|
assert output["cached"] == output["results"][0]
|
||||||
|
assert output["calls"] == [
|
||||||
|
"api/v1/repos/stackchain/api/pulls/7/reviews/8/feedback?expected_head_sha=abc1234"
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
def test_pull_sheet_refreshes_status_only_without_clearing_review_progress():
|
def test_pull_sheet_refreshes_status_only_without_clearing_review_progress():
|
||||||
script = f"""
|
script = f"""
|
||||||
const createPullSheet = require({json.dumps(str(PULL_SHEET))});
|
const createPullSheet = require({json.dumps(str(PULL_SHEET))});
|
||||||
|
|
|
||||||
|
|
@ -211,6 +211,38 @@ async def test_assigned_pull_review_endpoint_loads_review_payload_on_demand(monk
|
||||||
]
|
]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.anyio
|
||||||
|
async def test_assigned_pull_feedback_endpoint_authorizes_and_returns_one_review(monkeypatch):
|
||||||
|
calls = []
|
||||||
|
|
||||||
|
async def assigned(repository, number):
|
||||||
|
calls.append(("assigned", repository, number))
|
||||||
|
return True
|
||||||
|
|
||||||
|
async def feedback(repository, number, review_id, expected_head_sha):
|
||||||
|
calls.append(("feedback", repository, number, review_id, expected_head_sha))
|
||||||
|
return {
|
||||||
|
"review_id": review_id, "head_sha": expected_head_sha,
|
||||||
|
"reviewed_head_sha": expected_head_sha, "comments": [],
|
||||||
|
}
|
||||||
|
|
||||||
|
monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned)
|
||||||
|
monkeypatch.setattr(main.gitea_proxy, "pull_review_feedback", feedback, 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/reviews/8/feedback?expected_head_sha=abc1234"
|
||||||
|
)
|
||||||
|
|
||||||
|
assert response.status_code == 200
|
||||||
|
assert response.headers["cache-control"] == "no-store"
|
||||||
|
assert response.json()["review_id"] == 8
|
||||||
|
assert calls == [
|
||||||
|
("assigned", "stackchain/api", 7),
|
||||||
|
("feedback", "stackchain/api", 7, 8, "abc1234"),
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
def test_reviewer_statuses_distinguish_waiting_current_and_outdated_decisions():
|
def test_reviewer_statuses_distinguish_waiting_current_and_outdated_decisions():
|
||||||
pull = {
|
pull = {
|
||||||
"requested_reviewers": [{"login": "casey"}],
|
"requested_reviewers": [{"login": "casey"}],
|
||||||
|
|
@ -448,7 +480,7 @@ async def test_gitea_assigned_pull_review_includes_bounded_diff_previews():
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.anyio
|
@pytest.mark.anyio
|
||||||
async def test_assigned_pull_review_includes_latest_inline_feedback():
|
async def test_assigned_pull_review_does_not_block_on_inline_feedback():
|
||||||
requests = []
|
requests = []
|
||||||
|
|
||||||
async def handler(request):
|
async def handler(request):
|
||||||
|
|
@ -465,17 +497,13 @@ async def test_assigned_pull_review_includes_latest_inline_feedback():
|
||||||
{"id": 8, "user": {"login": "sam"}, "state": "REQUEST_CHANGES",
|
{"id": 8, "user": {"login": "sam"}, "state": "REQUEST_CHANGES",
|
||||||
"commit_id": "abc123", "body": "Please handle the empty state."},
|
"commit_id": "abc123", "body": "Please handle the empty state."},
|
||||||
])
|
])
|
||||||
if path.endswith("/pulls/7/reviews/8/comments"):
|
|
||||||
return httpx.Response(200, json=[
|
|
||||||
{"id": 99, "path": "src/api.py", "body": "Return before parsing.", "new_position": 12},
|
|
||||||
])
|
|
||||||
if path.endswith("/pulls/7/files"):
|
if path.endswith("/pulls/7/files"):
|
||||||
return httpx.Response(200, json=[{"filename": "src/api.py", "status": "modified"}])
|
return httpx.Response(200, json=[{"filename": "src/api.py", "status": "modified"}])
|
||||||
if path.endswith("/commits/abc123/status"):
|
if path.endswith("/commits/abc123/status"):
|
||||||
return httpx.Response(200, json={"state": "success"})
|
return httpx.Response(200, json={"state": "success"})
|
||||||
if path.endswith("/pulls/7.diff"):
|
if path.endswith("/pulls/7.diff"):
|
||||||
return httpx.Response(200, text="")
|
return httpx.Response(200, text="")
|
||||||
raise AssertionError(f"unexpected request: {request.method} {path}")
|
raise AssertionError(f"optional feedback must not block core review: {request.method} {path}")
|
||||||
|
|
||||||
gitea_proxy.start_client(transport=httpx.MockTransport(handler))
|
gitea_proxy.start_client(transport=httpx.MockTransport(handler))
|
||||||
try:
|
try:
|
||||||
|
|
@ -483,14 +511,50 @@ async def test_assigned_pull_review_includes_latest_inline_feedback():
|
||||||
finally:
|
finally:
|
||||||
await gitea_proxy.stop_client()
|
await gitea_proxy.stop_client()
|
||||||
|
|
||||||
assert "/api/v1/repos/stackchain/api/pulls/7/reviews/7/comments" not in requests
|
assert not any(path.endswith("/comments") for path in requests)
|
||||||
assert detail["reviewers"] == [{
|
assert detail["reviewers"] == [{
|
||||||
"review_id": 8, "login": "sam", "status": "changes_requested", "head_sha": "abc123", "blocking": True,
|
"review_id": 8, "login": "sam", "status": "changes_requested", "head_sha": "abc123", "blocking": True,
|
||||||
"summary": "Please handle the empty state.",
|
"summary": "Please handle the empty state.",
|
||||||
"comments": [{"id": 99, "path": "src/api.py", "body": "Return before parsing.", "line": 12}],
|
|
||||||
}]
|
}]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.anyio
|
||||||
|
async def test_pull_review_feedback_loads_one_current_bounded_review():
|
||||||
|
requests = []
|
||||||
|
|
||||||
|
async def handler(request):
|
||||||
|
path = request.url.path
|
||||||
|
requests.append(path)
|
||||||
|
if path.endswith("/pulls/7"):
|
||||||
|
return httpx.Response(200, json={"head": {"sha": "abc123"}})
|
||||||
|
if path.endswith("/pulls/7/reviews"):
|
||||||
|
return httpx.Response(200, json=[
|
||||||
|
{"id": 7, "user": {"login": "sam"}, "state": "COMMENT", "commit_id": "old-head"},
|
||||||
|
{"id": 8, "user": {"login": "sam"}, "state": "REQUEST_CHANGES", "commit_id": "abc123"},
|
||||||
|
])
|
||||||
|
if path.endswith("/pulls/7/reviews/8/comments"):
|
||||||
|
return httpx.Response(200, json=[
|
||||||
|
{"id": 99, "path": "src/api.py", "body": "Return before parsing.", "new_position": 12},
|
||||||
|
])
|
||||||
|
raise AssertionError(f"unexpected request: {request.method} {path}")
|
||||||
|
|
||||||
|
gitea_proxy.start_client(transport=httpx.MockTransport(handler))
|
||||||
|
try:
|
||||||
|
feedback = await gitea_proxy.pull_review_feedback(
|
||||||
|
"stackchain/api", 7, 8, "abc123"
|
||||||
|
)
|
||||||
|
finally:
|
||||||
|
await gitea_proxy.stop_client()
|
||||||
|
|
||||||
|
assert requests[-1].endswith("/pulls/7/reviews/8/comments")
|
||||||
|
assert feedback == {
|
||||||
|
"review_id": 8,
|
||||||
|
"head_sha": "abc123",
|
||||||
|
"reviewed_head_sha": "abc123",
|
||||||
|
"comments": [{"id": 99, "path": "src/api.py", "body": "Return before parsing.", "line": 12}],
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.anyio
|
@pytest.mark.anyio
|
||||||
async def test_assigned_pull_conversation_endpoint_reuses_issue_thread_with_pull_authorization(monkeypatch):
|
async def test_assigned_pull_conversation_endpoint_reuses_issue_thread_with_pull_authorization(monkeypatch):
|
||||||
calls = []
|
calls = []
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue
Block a user