diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 30d2154..ad147fa 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -57,7 +57,7 @@ jobs: pip install -r requirements-e2e.txt python3 -m playwright install --with-deps chromium - name: Exercise packaged mobile work journeys - run: python3 -m pytest tests/e2e/test_mobile_offline_issue_release.py tests/e2e/test_mobile_search_preview_navigation.py tests/e2e/test_mobile_search_week_plan.py tests/e2e/test_mobile_find_work_release.py tests/e2e/test_mobile_home_bootstrap_release.py tests/e2e/test_mobile_sign_out_release.py tests/e2e/test_mobile_today_handoff_release.py tests/e2e/test_mobile_today_wrap_up_release.py tests/e2e/test_mobile_today_summary_release.py tests/e2e/test_mobile_tomorrow_conflict_release.py tests/e2e/test_mobile_week_ahead_release.py tests/e2e/test_mobile_today_week_reschedule_release.py tests/e2e/test_mobile_wrap_up_handoff_release.py tests/e2e/test_mobile_following_release.py tests/e2e/test_mobile_detail_watch_release.py tests/e2e/test_mobile_pull_reviewer_status_release.py -q + run: python3 -m pytest tests/e2e/test_mobile_offline_issue_release.py tests/e2e/test_mobile_search_preview_navigation.py tests/e2e/test_mobile_search_week_plan.py tests/e2e/test_mobile_find_work_release.py tests/e2e/test_mobile_home_bootstrap_release.py tests/e2e/test_mobile_sign_out_release.py tests/e2e/test_mobile_today_handoff_release.py tests/e2e/test_mobile_today_wrap_up_release.py tests/e2e/test_mobile_today_summary_release.py tests/e2e/test_mobile_tomorrow_conflict_release.py tests/e2e/test_mobile_week_ahead_release.py tests/e2e/test_mobile_today_week_reschedule_release.py tests/e2e/test_mobile_wrap_up_handoff_release.py tests/e2e/test_mobile_following_release.py tests/e2e/test_mobile_detail_watch_release.py tests/e2e/test_mobile_pull_reviewer_status_release.py tests/e2e/test_mobile_pull_reviewer_feedback_release.py -q release-candidate: runs-on: ubuntu-latest diff --git a/frontend/dashboard.css b/frontend/dashboard.css index 1b1e812..43226ed 100644 --- a/frontend/dashboard.css +++ b/frontend/dashboard.css @@ -1261,9 +1261,19 @@ textarea { resize: vertical; min-height: 120px; } .pull-review-tools button { min-height:44px; } .pull-reviewer-summary { margin:10px 0; padding:10px; border:1px solid #203a5c; border-radius:10px; } .pull-reviewer-summary h3 { margin:0 0 6px; font-size:14px; } -.pull-reviewer-status { display:flex; justify-content:space-between; gap:10px; padding:8px 0; border-top:1px solid #203a5c; overflow-wrap:anywhere; } +.pull-reviewer-status { padding:8px 0; border-top:1px solid #203a5c; overflow-wrap:anywhere; } .pull-reviewer-status:first-child { border-top:0; } -.pull-reviewer-status span { text-align:right; } +.pull-reviewer-status-heading { display:flex; justify-content:space-between; gap:10px; } +.pull-reviewer-status-heading span { text-align:right; } +.pull-review-feedback { margin-top:8px; border:1px solid #2a496e; border-radius:8px; padding:0 8px 8px; } +.pull-review-feedback > summary { min-height:44px; display:flex; align-items:center; cursor:pointer; } +.pull-review-feedback-summary { margin:4px 0 10px; white-space:pre-wrap; } +.pull-review-feedback-file { margin-top:8px; padding-top:8px; border-top:1px solid #203a5c; } +.pull-review-feedback-file h4 { margin:0 0 4px; overflow-wrap:anywhere; } +.pull-review-feedback-file ul { margin:0; padding-left:20px; } +.pull-review-feedback-file li { margin:6px 0; } +.pull-review-feedback-file small { display:block; color:#93c5fd; } +.pull-review-feedback-file button { min-height:44px; width:100%; margin-top:4px; } #request-updated-pull-review { min-height:44px; width:100%; margin-top:8px; } .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; } diff --git a/frontend/pull-sheet.js b/frontend/pull-sheet.js index 1a73f7d..db22224 100644 --- a/frontend/pull-sheet.js +++ b/frontend/pull-sheet.js @@ -28,6 +28,29 @@ function mergeEligibility(detail, reviewState) { return { allowed: true, reason: 'Ready to merge' }; } +function renderReviewerFeedback(reviewer, escapeHtml) { + const groups = new Map(); + (Array.isArray(reviewer?.comments) ? reviewer.comments : []).forEach(comment => { + if (!comment?.path || !comment?.body) return; + if (!groups.has(comment.path)) groups.set(comment.path, []); + groups.get(comment.path).push(comment); + }); + const summary = reviewer?.summary ? '

' + + escapeHtml(reviewer.summary) + '

' : ''; + const files = Array.from(groups.entries()).map(([path, comments]) => + '

' + escapeHtml(path) + '

' + ).join(''); + if (!summary && !files) return ''; + const reviewedHead = reviewer?.status === 'outdated' && reviewer?.head_sha ? + '

Reviewed head ' + escapeHtml(reviewer.head_sha.slice(0, 8)) + '

' : ''; + return '
Review feedback' + + reviewedHead + summary + files + '
'; +} + function renderReviewerStatuses(reviewers, escapeHtml) { const labels = { waiting: 'Waiting for review', @@ -38,9 +61,10 @@ function renderReviewerStatuses(reviewers, escapeHtml) { }; return (Array.isArray(reviewers) ? reviewers : []).map(reviewer => { const status = labels[reviewer?.status] || 'Status unavailable'; - return '
@' + - escapeHtml(reviewer?.login || 'unknown') + '' + escapeHtml(status) + '
'; + return '
@' + + escapeHtml(reviewer?.login || 'unknown') + '' + escapeHtml(status) + '
' + + renderReviewerFeedback(reviewer, escapeHtml) + '
'; }).join(''); } @@ -48,6 +72,9 @@ function renderReviewerPanel(doc, detail, escapeHtml = value => String(value) .replaceAll('&', '&').replaceAll('<', '<').replaceAll('>', '>')) { const reviewers = Array.isArray(detail?.reviewers) ? detail.reviewers : []; doc.querySelector('#pull-reviewer-statuses').innerHTML = renderReviewerStatuses(reviewers, escapeHtml); + doc.querySelectorAll('[data-review-feedback-file]').forEach(button => { + button.addEventListener('click', () => focusPullFile(doc, button.dataset.reviewFeedbackFile)); + }); doc.querySelector('#pull-reviewer-status').textContent = reviewers.length ? reviewers.length + ' reviewer status' + (reviewers.length === 1 ? '' : 'es') + ' for this pull request.' : 'No review has been requested; existing merge policy is unchanged.'; @@ -83,18 +110,22 @@ function renderFile(file, index, reviewed, escapeHtml) { '" aria-pressed="' + String(reviewed) + '">' + (reviewed ? 'Reviewed' : 'Mark reviewed') + ''; } -function focusNextUnreviewed(doc, detail, state, controller) { - if (!detail || !state) return; - const filename = controller.nextUnreviewed(detail, state); +function focusPullFile(doc, filename) { const article = Array.from(doc.querySelectorAll('#pull-files .pull-file')) .find(file => file.dataset.pullFilename === filename); const toggle = article?.querySelector('.pull-file-toggle'); const panel = toggle && doc.getElementById(toggle.getAttribute('aria-controls')); - if (!toggle || !panel) return; + if (!toggle || !panel) return false; toggle.setAttribute('aria-expanded', 'true'); panel.hidden = false; toggle.scrollIntoView({ block: 'center', behavior: 'smooth' }); toggle.focus(); + return true; +} + +function focusNextUnreviewed(doc, detail, state, controller) { + if (!detail || !state) return; + focusPullFile(doc, controller.nextUnreviewed(detail, state)); } function removeFromSnapshot(data, item) { diff --git a/src/gitea_proxy.py b/src/gitea_proxy.py index d39001b..69da4bc 100644 --- a/src/gitea_proxy.py +++ b/src/gitea_proxy.py @@ -2771,6 +2771,49 @@ async def pull_completion_detail(repository: str, number: int) -> dict: } +def _normalize_review_comments(comments: object) -> list[dict]: + normalized = [] + for comment in (comments if isinstance(comments, list) else [])[:100]: + if not isinstance(comment, dict): + continue + path = comment.get("path") + body = comment.get("body") + if not isinstance(path, str) or not path.strip() or not isinstance(body, str) or not body.strip(): + continue + item = {"path": path.strip()[:300], "body": body.strip()[:500]} + position = comment.get("new_position") or comment.get("old_position") + if isinstance(position, int) and position > 0: + item["line"] = position + normalized.append(item) + if len(normalized) == 20: + break + return normalized + + +def _latest_feedback_review_ids(reviews: object) -> dict[str, int]: + latest: dict[str, tuple[int, str]] = {} + for review in (reviews if isinstance(reviews, list) else [])[:100]: + if not isinstance(review, dict): + continue + user = review.get("user") if isinstance(review.get("user"), dict) else {} + login = user.get("login") + review_id = review.get("id") + state = review.get("state") + if ( + not isinstance(login, str) or not login or not isinstance(review_id, int) + or review_id <= 0 or not isinstance(state, str) + ): + continue + key = login.casefold() + if key not in latest or review_id >= latest[key][0]: + latest[key] = (review_id, state) + feedback = [ + (key, review_id) for key, (review_id, state) in latest.items() + if state in {"REQUEST_CHANGES", "COMMENT"} + ] + return dict(feedback[:10]) + + def _normalize_reviewer_statuses(pull: dict, reviews: object, head_sha: str) -> list[dict]: """Return one bounded, current decision per reviewer without exposing raw review data.""" latest: dict[str, dict] = {} @@ -2792,8 +2835,10 @@ def _normalize_reviewer_statuses(pull: dict, reviews: object, head_sha: str) -> previous = latest.get(login.casefold()) review_id = review.get("id") if isinstance(review.get("id"), int) else 0 if previous is None or review_id >= previous["id"]: + body = review.get("body") latest[login.casefold()] = { "id": review_id, "login": login, "state": state, "commit_id": commit_id, + "summary": body.strip()[:500] if isinstance(body, str) and body.strip() else "", } statuses: dict[str, dict] = {} @@ -2809,6 +2854,8 @@ def _normalize_reviewer_statuses(pull: dict, reviews: object, head_sha: str) -> "head_sha": review["commit_id"], "blocking": status in {"changes_requested", "outdated"}, } + if review["summary"]: + statuses[key]["summary"] = review["summary"] requested = pull.get("requested_reviewers") if isinstance(pull, dict) else [] for reviewer in (requested if isinstance(requested, list) else [])[:25]: @@ -2839,6 +2886,21 @@ 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, @@ -2849,7 +2911,7 @@ async def pull_completion_review(repository: str, number: int) -> dict: "merged": pull.get("merged") is True, "ci_state": status.get("state", "unknown") if isinstance(status, dict) else "unknown", "checks": _normalize_commit_checks(status), - "reviewers": _normalize_reviewer_statuses(pull, reviews, sha), + "reviewers": reviewers, "files": [ { "filename": item.get("filename", ""), diff --git a/tests/e2e/test_mobile_pull_reviewer_feedback_release.py b/tests/e2e/test_mobile_pull_reviewer_feedback_release.py new file mode 100644 index 0000000..87667bf --- /dev/null +++ b/tests/e2e/test_mobile_pull_reviewer_feedback_release.py @@ -0,0 +1,92 @@ +from pathlib import Path + +import pytest + + +ROOT = Path(__file__).parents[2] + + +@pytest.mark.parametrize("viewport", [(320, 568), (390, 844)]) +def test_mobile_received_review_feedback_opens_matching_change(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": "current-head", + "reviewers": [{ + "login": "sam", + "status": "outdated", + "head_sha": "old-head-123456", + "blocking": True, + "summary": "Please handle the narrow empty state before merging.", + "comments": [{ + "path": "src/a/very/long/mobile/path/review_target.py", + "body": "Return before parsing when the payload is empty.", + "line": 12, + }], + }], + "files": [{ + "filename": "src/a/very/long/mobile/path/review_target.py", + "status": "modified", + "additions": 2, + "deletions": 1, + "diff_available": True, + "diff_lines": ["@@ -10,2 +10,3 @@", "-parse(payload)", "+if not payload: return", "+parse(payload)"], + }], + } + + 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 => { + document.querySelector('#pull-sheet').classList.add('open'); + document.querySelector('#pull-review').open = true; + createPullSheet.review(detail, null, document); + document.querySelector('#pull-files').innerHTML = detail.files.map((file, index) => + createPullSheet.renderFile(file, index, false, value => String(value) + .replaceAll('&', '&').replaceAll('<', '<').replaceAll('>', '>')) + ).join(''); + }""", + detail, + ) + + page.locator(".pull-review-feedback summary").click() + jump = page.locator('[data-review-feedback-file]') + jump.click() + metrics = page.evaluate( + """() => { + const jump = document.querySelector('[data-review-feedback-file]'); + const toggle = document.querySelector('.pull-file-toggle'); + const panel = document.getElementById(toggle.getAttribute('aria-controls')); + return { + scrollWidth: document.documentElement.scrollWidth, + clientWidth: document.documentElement.clientWidth, + jumpHeight: jump.getBoundingClientRect().height, + expanded: toggle.getAttribute('aria-expanded'), + panelHidden: panel.hidden, + focused: document.activeElement === toggle, + reviewedHead: document.querySelector('.pull-review-feedback').textContent, + updatedReviewVisible: !document.querySelector('#request-updated-pull-review').hidden, + }; + }""" + ) + browser.close() + + assert metrics["scrollWidth"] <= metrics["clientWidth"] + assert metrics["jumpHeight"] >= 44 + assert metrics["expanded"] == "true" + assert metrics["panelHidden"] is False + assert metrics["focused"] is True + assert "Reviewed head old-head" in metrics["reviewedHead"] + assert metrics["updatedReviewVisible"] is True diff --git a/tests/test_ci_workflow.py b/tests/test_ci_workflow.py index 5e62ece..8455d5c 100644 --- a/tests/test_ci_workflow.py +++ b/tests/test_ci_workflow.py @@ -74,7 +74,8 @@ def test_release_promotion_waits_for_packaged_mobile_journeys(): "tests/e2e/test_mobile_wrap_up_handoff_release.py " "tests/e2e/test_mobile_following_release.py " "tests/e2e/test_mobile_detail_watch_release.py " - "tests/e2e/test_mobile_pull_reviewer_status_release.py -q" + "tests/e2e/test_mobile_pull_reviewer_status_release.py " + "tests/e2e/test_mobile_pull_reviewer_feedback_release.py -q" ) in browser assert "needs: [lint, build-release, browser-journey]" in release diff --git a/tests/test_my_work.py b/tests/test_my_work.py index 63c3af1..adda748 100644 --- a/tests/test_my_work.py +++ b/tests/test_my_work.py @@ -7772,6 +7772,32 @@ process.stdout.write(JSON.stringify({{ assert "Approved current head" in output["html"] +def test_pull_sheet_renders_bounded_feedback_grouped_by_file(): + script = f""" +const createPullSheet = require({json.dumps(str(PULL_SHEET))}); +const html = createPullSheet.renderReviewerStatuses([{{ + login:'sam', status:'changes_requested', head_sha:'abc123', blocking:true, + summary:'Please this helper.', + comments:[ + {{path:'src/api.py', body:'Handle .', line:12}}, + {{path:'src/api.py', body:'Keep the return explicit.'}}, + {{path:'frontend/app.js', body:'Show the error.'}}, + ], +}}], value => String(value).replaceAll('&', '&').replaceAll('<', '<').replaceAll('>', '>')); +process.stdout.write(html); +""" + html = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True).stdout + + assert "Review feedback" in html + assert "Please <split> this helper." in html + assert html.count('data-review-feedback-file="src/api.py"') == 1 + assert 'data-review-feedback-file="frontend/app.js"' in html + assert "Handle <empty>." in html + assert "Line 12" in html + assert "View in changes" in html + assert "Please " not in html + + def test_pull_sheet_persists_head_scoped_file_review_and_gates_merge(): 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 960cd7d..f71ef84 100644 --- a/tests/test_pull_api.py +++ b/tests/test_pull_api.py @@ -236,6 +236,68 @@ def test_reviewer_statuses_distinguish_waiting_current_and_outdated_decisions(): ] +def test_reviewer_status_exposes_only_bounded_nonempty_feedback_summary(): + pull = {"requested_reviewers": []} + reviews = [ + { + "id": 8, + "user": {"login": "sam"}, + "state": "REQUEST_CHANGES", + "commit_id": "abc123", + "body": "Please split this helper. " + ("x" * 900), + }, + { + "id": 9, + "user": {"login": "lee"}, + "state": "COMMENT", + "commit_id": "abc123", + "body": " ", + }, + ] + + statuses = gitea_proxy._normalize_reviewer_statuses(pull, reviews, "abc123") + + sam = next(item for item in statuses if item["login"] == "sam") + lee = next(item for item in statuses if item["login"] == "lee") + assert sam["summary"].startswith("Please split this helper.") + assert len(sam["summary"]) == 500 + assert "summary" not in lee + + +def test_review_comments_are_bounded_and_require_a_file_and_body(): + comments = [ + {"id": 1, "path": "src/api.py", "body": "Handle the empty state", "new_position": 4}, + {"id": 2, "path": "src/api.py", "body": "x" * 900, "old_position": 7}, + {"id": 3, "path": "", "body": "missing file"}, + {"id": 4, "path": "src/ignored.py", "body": " "}, + "malformed", + ] + [ + {"id": index, "path": f"src/{index}.py", "body": "bounded"} + for index in range(5, 40) + ] + + normalized = gitea_proxy._normalize_review_comments(comments) + + assert len(normalized) == 20 + assert normalized[0] == { + "path": "src/api.py", "body": "Handle the empty state", "line": 4, + } + assert normalized[1]["line"] == 7 + assert len(normalized[1]["body"]) == 500 + assert all(item["path"] and item["body"] for item in normalized) + + +def test_inline_feedback_is_loaded_only_for_each_reviewers_latest_decision(): + reviews = [ + {"id": 7, "user": {"login": "sam"}, "state": "REQUEST_CHANGES"}, + {"id": 8, "user": {"login": "sam"}, "state": "APPROVED"}, + {"id": 9, "user": {"login": "lee"}, "state": "COMMENT"}, + {"id": 10, "user": {"login": "pat"}, "state": "PENDING"}, + ] + + assert gitea_proxy._latest_feedback_review_ids(reviews) == {"lee": 9} + + @pytest.mark.anyio async def test_gitea_pull_check_status_refreshes_reviewer_decisions_without_files_or_diff(): requests = [] @@ -359,6 +421,50 @@ async def test_gitea_assigned_pull_review_includes_bounded_diff_previews(): assert "conversation" not in detail +@pytest.mark.anyio +async def test_assigned_pull_review_includes_latest_inline_feedback(): + requests = [] + + async def handler(request): + path = request.url.path + requests.append(path) + if path.endswith("/pulls/7"): + return httpx.Response(200, json={ + "state": "open", "mergeable": True, "head": {"sha": "abc123"}, + "requested_reviewers": [], + }) + 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", "body": "Please handle the empty state."}, + ]) + if path.endswith("/pulls/7/reviews/8/comments"): + return httpx.Response(200, json=[ + {"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}") + + gitea_proxy.start_client(transport=httpx.MockTransport(handler)) + try: + detail = await gitea_proxy.pull_completion_review("stackchain/api", 7) + finally: + await gitea_proxy.stop_client() + + assert "/api/v1/repos/stackchain/api/pulls/7/reviews/7/comments" not in requests + assert detail["reviewers"] == [{ + "login": "sam", "status": "changes_requested", "head_sha": "abc123", "blocking": True, + "summary": "Please handle the empty state.", + "comments": [{"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 = []