@' +
+ escapeHtml(reviewer?.status || 'unknown') + '" data-review-id="' +
+ (Number.isInteger(reviewer?.review_id) ? Number(reviewer.review_id) : '') +
+ '">" 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():
script = f"""
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():
script = f"""
const createPullSheet = require({json.dumps(str(PULL_SHEET))});
diff --git a/tests/test_pull_api.py b/tests/test_pull_api.py
index de93e01..f9060aa 100644
--- a/tests/test_pull_api.py
+++ b/tests/test_pull_api.py
@@ -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():
pull = {
"requested_reviewers": [{"login": "casey"}],
@@ -448,7 +480,7 @@ async def test_gitea_assigned_pull_review_includes_bounded_diff_previews():
@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 = []
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",
"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"):
return httpx.Response(200, json=[{"filename": "src/api.py", "status": "modified"}])
if path.endswith("/commits/abc123/status"):
return httpx.Response(200, json={"state": "success"})
if path.endswith("/pulls/7.diff"):
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))
try:
@@ -483,14 +511,50 @@ async def test_assigned_pull_review_includes_latest_inline_feedback():
finally:
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"] == [{
"review_id": 8, "login": "sam", "status": "changes_requested", "head_sha": "abc123", "blocking": True,
"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
async def test_assigned_pull_conversation_endpoint_reuses_issue_thread_with_pull_authorization(monkeypatch):
calls = []
@' +
escapeHtml(reviewer?.login || 'unknown') + '' + escapeHtml(status) + '
' +
renderReviewerFeedback(reviewer, escapeHtml) + cancel + '';
}).join('');
@@ -79,7 +91,13 @@ function renderReviewerStatuses(reviewers, escapeHtml) {
function renderReviewerPanel(doc, detail, escapeHtml = value => String(value)
.replaceAll('&', '&').replaceAll('<', '<').replaceAll('>', '>')) {
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);
+ 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 => {
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.';
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]');
if (!button) return;
const detail = getDetail?.();
@@ -570,6 +614,8 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT
let feedbackRequest = null;
const reviewRequests = new Map();
const reviewCache = new Map();
+ const feedbackRequests = new Map();
+ const feedbackCache = 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;
@@ -609,6 +655,25 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT
reviewRequests.set(key, 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) {
if (checkRequest) return checkRequest;
checkRequest = fetchJson(pathFor(item) + '/checks', {
diff --git a/src/gitea_proxy.py b/src/gitea_proxy.py
index 04249fe..deda010 100644
--- a/src/gitea_proxy.py
+++ b/src/gitea_proxy.py
@@ -2938,20 +2938,6 @@ async def pull_completion_review(repository: str, number: int) -> dict:
diff, diff_truncated = diff_result
previews = _diff_previews(diff, diff_truncated)
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 {
"repository": repository,
"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:
"""Return CI and release evidence for one exact merge commit."""
status, releases = await asyncio.gather(
diff --git a/src/main.py b/src/main.py
index e2c9d0b..3c2fa06 100644
--- a/src/main.py
+++ b/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")
async def assigned_pull_checks(
owner: str, repo: str, number: int = PathParam(gt=0)
diff --git a/tests/e2e/test_mobile_pull_reviewer_feedback_release.py b/tests/e2e/test_mobile_pull_reviewer_feedback_release.py
index 87667bf..ed27dc2 100644
--- a/tests/e2e/test_mobile_pull_reviewer_feedback_release.py
+++ b/tests/e2e/test_mobile_pull_reviewer_feedback_release.py
@@ -90,3 +90,83 @@ def test_mobile_received_review_feedback_opens_matching_change(viewport):
assert metrics["focused"] is True
assert "Reviewed head old-head" in metrics["reviewedHead"]
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
diff --git a/tests/test_my_work.py b/tests/test_my_work.py
index 80e5390..ef46665 100644
--- a/tests/test_my_work.py
+++ b/tests/test_my_work.py
@@ -7867,6 +7867,38 @@ process.stdout.write(html);
assert "Please