Address requested review feedback from mobile pull detail #1339
|
|
@ -1274,6 +1274,17 @@ textarea { resize: vertical; min-height: 120px; }
|
|||
.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; }
|
||||
.address-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-heading { display:flex; align-items:start; justify-content:space-between; gap:8px; }
|
||||
.pull-feedback-pass-heading h4 { margin:2px 0 8px; }
|
||||
.pull-feedback-pass p { white-space:pre-wrap; }
|
||||
.pull-feedback-pass textarea { width:100%; min-height:88px; box-sizing:border-box; }
|
||||
.pull-feedback-dispositions { display:grid; grid-template-columns:1fr; gap:6px; margin:10px 0; padding:8px; }
|
||||
.pull-feedback-dispositions button[aria-pressed="true"] { border-color:#60a5fa; background:#173b63; }
|
||||
.pull-feedback-navigation { display:grid; grid-template-columns:1fr 1fr; gap:8px; margin-top:8px; }
|
||||
.pull-feedback-pass button { min-height:44px; }
|
||||
#finish-pull-feedback, #request-feedback-review { width:100%; margin-top:8px; }
|
||||
#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; }
|
||||
|
|
|
|||
|
|
@ -1726,6 +1726,30 @@
|
|||
<div id="pull-reviewer-status" class="small" aria-live="polite">Reviewer status not loaded.</div>
|
||||
<div id="pull-reviewer-statuses"></div>
|
||||
<button id="request-updated-pull-review" type="button" hidden>Request updated review</button>
|
||||
<section class="pull-feedback-pass" id="pull-feedback-pass" aria-labelledby="pull-feedback-heading" hidden>
|
||||
<div class="pull-feedback-pass-heading">
|
||||
<div><span class="small" id="pull-feedback-progress"></span><h4 id="pull-feedback-heading">Address review feedback</h4></div>
|
||||
<button id="close-pull-feedback" type="button">Close</button>
|
||||
</div>
|
||||
<p class="small" id="pull-feedback-location"></p>
|
||||
<p id="pull-feedback-body"></p>
|
||||
<div class="small" id="pull-feedback-context" aria-live="polite"></div>
|
||||
<fieldset class="pull-feedback-dispositions">
|
||||
<legend>Disposition</legend>
|
||||
<button type="button" data-feedback-disposition="addressed">Addressed</button>
|
||||
<button type="button" data-feedback-disposition="discussion">Needs discussion</button>
|
||||
<button type="button" data-feedback-disposition="skipped">Skip for now</button>
|
||||
</fieldset>
|
||||
<label for="pull-feedback-note">Response note (optional)</label>
|
||||
<textarea id="pull-feedback-note" maxlength="1000" placeholder="What changed or needs discussion?"></textarea>
|
||||
<div class="pull-feedback-navigation">
|
||||
<button id="previous-pull-feedback" type="button">Previous</button>
|
||||
<button id="next-pull-feedback" type="button">Next</button>
|
||||
</div>
|
||||
<button id="finish-pull-feedback" type="button" disabled>Post feedback response</button>
|
||||
<button id="request-feedback-review" type="button" hidden>Request updated review from this reviewer</button>
|
||||
<div id="pull-feedback-status" class="small" role="status" aria-live="assertive"></div>
|
||||
</section>
|
||||
</section>
|
||||
<details class="ci-checks" id="pull-checks">
|
||||
<summary><span>Checks</span><span class="small" id="pull-checks-summary">Not loaded</span></summary>
|
||||
|
|
|
|||
|
|
@ -47,8 +47,13 @@ function renderReviewerFeedback(reviewer, escapeHtml) {
|
|||
if (!summary && !files) return '';
|
||||
const reviewedHead = reviewer?.status === 'outdated' && reviewer?.head_sha ?
|
||||
'<p class="small">Reviewed head ' + escapeHtml(reviewer.head_sha.slice(0, 8)) + '</p>' : '';
|
||||
const addressable = Number.isInteger(reviewer?.review_id) && reviewer.review_id > 0 &&
|
||||
['changes_requested', 'outdated'].includes(reviewer?.status) &&
|
||||
(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="' +
|
||||
Number(reviewer.review_id) + '">Address feedback</button>' : '';
|
||||
return '<details class="pull-review-feedback"><summary>Review feedback</summary>' +
|
||||
reviewedHead + summary + files + '</details>';
|
||||
reviewedHead + summary + files + address + '</details>';
|
||||
}
|
||||
|
||||
function renderReviewerStatuses(reviewers, escapeHtml) {
|
||||
|
|
@ -190,6 +195,8 @@ function resetReviewRequestControls(doc, detail) {
|
|||
qs('#pull-reviewer-status').textContent = 'Reviewer status not loaded.';
|
||||
qs('#pull-reviewer-statuses').textContent = '';
|
||||
qs('#request-updated-pull-review').hidden = true;
|
||||
qs('#pull-feedback-pass').hidden = true;
|
||||
qs('#pull-feedback-status').textContent = '';
|
||||
}
|
||||
|
||||
function bindReviewRequestControls(doc, controller, getSelected, getDetail) {
|
||||
|
|
@ -259,6 +266,123 @@ function bindReviewRequestControls(doc, controller, getSelected, getDetail) {
|
|||
});
|
||||
}
|
||||
|
||||
function bindFeedbackControls(doc, controller, getSelected, getDetail, getLogin) {
|
||||
const qs = selector => doc.querySelector(selector);
|
||||
const panel = qs('#pull-feedback-pass');
|
||||
if (panel.dataset.feedbackBound === 'true') return;
|
||||
panel.dataset.feedbackBound = 'true';
|
||||
let active = null;
|
||||
const comments = () => (active?.reviewer?.comments || [])
|
||||
.filter(comment => Number.isInteger(comment?.id) && comment.id > 0);
|
||||
const current = () => comments()[active?.state?.index || 0];
|
||||
const save = () => {
|
||||
if (active) controller.saveFeedbackPass(active.item, active.detail, active.reviewer, active.login, active.state);
|
||||
};
|
||||
const render = () => {
|
||||
if (!active) return;
|
||||
const items = comments();
|
||||
active.state.index = Math.max(0, Math.min(Number(active.state.index) || 0, items.length - 1));
|
||||
const comment = current();
|
||||
const response = active.state.responses[String(comment.id)] || {};
|
||||
qs('#pull-feedback-progress').textContent = 'Comment ' + (active.state.index + 1) + ' of ' + items.length;
|
||||
qs('#pull-feedback-heading').textContent = 'Address @' + active.reviewer.login + '’s feedback';
|
||||
qs('#pull-feedback-location').textContent = comment.path + (comment.line ? ' · line ' + Number(comment.line) : '');
|
||||
qs('#pull-feedback-body').textContent = comment.body;
|
||||
qs('#pull-feedback-note').value = response.note || '';
|
||||
panel.querySelectorAll('[data-feedback-disposition]').forEach(button => {
|
||||
button.setAttribute('aria-pressed', String(button.dataset.feedbackDisposition === response.disposition));
|
||||
});
|
||||
qs('#previous-pull-feedback').disabled = active.state.index === 0;
|
||||
qs('#next-pull-feedback').disabled = active.state.index >= items.length - 1;
|
||||
qs('#finish-pull-feedback').disabled = active.state.posted === true ||
|
||||
items.some(item => !active.state.responses[String(item.id)]?.disposition);
|
||||
qs('#request-feedback-review').hidden = active.state.posted !== true;
|
||||
const found = focusPullFile(doc, comment.path);
|
||||
qs('#pull-feedback-context').textContent = found ?
|
||||
'Matching change opened below.' : 'Matching change is unavailable in this preview.';
|
||||
panel.scrollIntoView({ block:'center', behavior:'smooth' });
|
||||
};
|
||||
qs('#pull-reviewer-statuses').addEventListener('click', event => {
|
||||
const button = event.target.closest?.('[data-address-review-feedback]');
|
||||
if (!button) return;
|
||||
const detail = getDetail?.();
|
||||
const item = getSelected?.();
|
||||
const reviewId = Number(button.dataset.addressReviewFeedback);
|
||||
const reviewer = (detail?.reviewers || []).find(candidate => candidate.review_id === reviewId);
|
||||
if (!detail || !item || !reviewer) return;
|
||||
const login = getLogin?.() || '';
|
||||
active = { item, detail, reviewer, login,
|
||||
state:controller.loadFeedbackPass(item, detail, reviewer, login) };
|
||||
panel.hidden = false;
|
||||
qs('#pull-feedback-status').textContent = active.state.posted ?
|
||||
'Response already posted. You can request an updated review.' : 'Progress saves on this device.';
|
||||
render();
|
||||
});
|
||||
panel.querySelector('.pull-feedback-dispositions').addEventListener('click', event => {
|
||||
const disposition = event.target.dataset?.feedbackDisposition;
|
||||
const comment = current();
|
||||
if (!active || !comment || !['addressed', 'discussion', 'skipped'].includes(disposition)) return;
|
||||
const key = String(comment.id);
|
||||
active.state.responses[key] = { ...(active.state.responses[key] || {}), disposition };
|
||||
save();
|
||||
render();
|
||||
});
|
||||
qs('#pull-feedback-note').addEventListener('input', event => {
|
||||
const comment = current();
|
||||
if (!active || !comment) return;
|
||||
const key = String(comment.id);
|
||||
active.state.responses[key] = { ...(active.state.responses[key] || {}), note:event.target.value.slice(0, 1000) };
|
||||
save();
|
||||
});
|
||||
qs('#previous-pull-feedback').addEventListener('click', () => {
|
||||
if (!active) return;
|
||||
active.state.index -= 1; save(); render();
|
||||
});
|
||||
qs('#next-pull-feedback').addEventListener('click', () => {
|
||||
if (!active) return;
|
||||
active.state.index += 1; save(); render();
|
||||
});
|
||||
qs('#close-pull-feedback').addEventListener('click', () => {
|
||||
panel.hidden = true;
|
||||
doc.querySelector('[data-address-review-feedback="' + active?.reviewer?.review_id + '"]')?.focus();
|
||||
});
|
||||
qs('#finish-pull-feedback').addEventListener('click', async () => {
|
||||
if (!active || qs('#finish-pull-feedback').disabled) return;
|
||||
const latest = getDetail?.();
|
||||
if (!latest || latest.head_sha !== active.state.headSha) {
|
||||
qs('#pull-feedback-status').textContent = 'The pull request changed. Refresh review data before posting.';
|
||||
return;
|
||||
}
|
||||
if (!globalThis.confirm('Post one feedback response to ' + active.item.key + '?')) return;
|
||||
qs('#finish-pull-feedback').disabled = true;
|
||||
qs('#pull-feedback-status').textContent = 'Posting feedback response…';
|
||||
try {
|
||||
await controller.postFeedbackResponse(active.item, active.detail, active.reviewer, active.login, active.state);
|
||||
active.state.posted = true;
|
||||
save();
|
||||
qs('#pull-feedback-status').textContent = 'Response posted. Request an updated review when ready.';
|
||||
render();
|
||||
} catch (error) {
|
||||
qs('#pull-feedback-status').textContent = error.message + ' Progress kept; retry without duplicating the response.';
|
||||
qs('#finish-pull-feedback').disabled = false;
|
||||
}
|
||||
});
|
||||
qs('#request-feedback-review').addEventListener('click', async () => {
|
||||
if (!active?.state?.posted) return;
|
||||
const button = qs('#request-feedback-review');
|
||||
button.disabled = true;
|
||||
qs('#pull-feedback-status').textContent = 'Requesting updated review…';
|
||||
try {
|
||||
await controller.requestReview(active.item, active.reviewer.login, active.detail.head_sha);
|
||||
qs('#pull-feedback-status').textContent = 'Response posted; updated review requested from @' + active.reviewer.login + '.';
|
||||
button.hidden = true;
|
||||
} catch (error) {
|
||||
qs('#pull-feedback-status').textContent = 'Response posted; review not requested. ' + error.message;
|
||||
button.disabled = false;
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
function bindContextEditor(doc, controller, getSelected, getDetail, getLogin) {
|
||||
const qs = selector => doc.querySelector(selector);
|
||||
const form = qs('#pull-edit-form');
|
||||
|
|
@ -333,6 +457,7 @@ function bindContextEditor(doc, controller, getSelected, getDetail, getLogin) {
|
|||
|
||||
function bindOwnershipControls(doc, controller, getSelected, finish, getDetail, getLogin) {
|
||||
bindReviewRequestControls(doc, controller, getSelected, getDetail);
|
||||
bindFeedbackControls(doc, controller, getSelected, getDetail, getLogin);
|
||||
controller.edit = bindContextEditor(doc, controller, getSelected, getDetail, getLogin);
|
||||
const qs = selector => doc.querySelector(selector);
|
||||
const load = qs('#load-pull-handoff');
|
||||
|
|
@ -407,6 +532,7 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT
|
|||
let reviewCandidateRequest = null;
|
||||
let reviewRequestMutation = null;
|
||||
let editRequest = null;
|
||||
let feedbackRequest = null;
|
||||
const reviewRequests = new Map();
|
||||
const reviewCache = new Map();
|
||||
const pathFor = item => 'api/v1/repos/' + String(item.repository || '').split('/')
|
||||
|
|
@ -415,6 +541,8 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT
|
|||
const operationKey = item => draftKey(item) + ':operation';
|
||||
const editDraftKey = item => 'stackchain.pull-edit.v1:' + item.repository + '#' + item.number;
|
||||
const reviewKey = (item, detail) => 'stackchain.pull-review.v1:' + item.repository + '#' + item.number + ':' + detail.head_sha;
|
||||
const feedbackKey = (item, detail, reviewer, login) => 'stackchain.feedback-pass.v1:' +
|
||||
item.repository + '#' + item.number + ':' + reviewer.review_id + ':' + reviewer.head_sha + ':' + detail.head_sha + ':' + login;
|
||||
const fileNames = detail => (detail?.files || []).map(file => file.filename).filter(Boolean);
|
||||
|
||||
function reviewState(item, detail) {
|
||||
|
|
@ -490,6 +618,50 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT
|
|||
try { storage?.setItem(editDraftKey(item), JSON.stringify(draft)); }
|
||||
catch (_error) { /* The edit fields remain the fallback. */ }
|
||||
},
|
||||
loadFeedbackPass(item, detail, reviewer, login) {
|
||||
const fresh = { headSha:detail.head_sha, index:0, responses:{}, posted:false };
|
||||
try {
|
||||
const saved = JSON.parse(storage?.getItem(feedbackKey(item, detail, reviewer, login)) || 'null');
|
||||
return saved && saved.headSha === detail.head_sha && saved.responses && typeof saved.responses === 'object'
|
||||
? { ...fresh, ...saved } : fresh;
|
||||
} catch (_error) { return fresh; }
|
||||
},
|
||||
saveFeedbackPass(item, detail, reviewer, login, state) {
|
||||
try { storage?.setItem(feedbackKey(item, detail, reviewer, login), JSON.stringify(state)); }
|
||||
catch (_error) { /* The visible pass remains usable without storage. */ }
|
||||
},
|
||||
feedbackSummary(reviewer, state) {
|
||||
const labels = { addressed:'Addressed', discussion:'Needs discussion', skipped:'Skip for now' };
|
||||
const lines = (reviewer.comments || []).filter(comment => Number.isInteger(comment?.id) && comment.id > 0)
|
||||
.map(comment => {
|
||||
const response = state.responses[String(comment.id)] || {};
|
||||
const path = String(comment.path || '').replaceAll('`', "'").replaceAll('\n', ' ').slice(0, 300);
|
||||
const body = String(comment.body || '').replaceAll('\n', ' ').slice(0, 180);
|
||||
const note = String(response.note || '').replaceAll('\n', ' ').slice(0, 1000);
|
||||
return '- `' + path + '` · comment #' + comment.id + ' · **' + (labels[response.disposition] || 'Unresolved') +
|
||||
'** — ' + body + (note ? '\n - Response: ' + note : '');
|
||||
});
|
||||
return ('Addressed review feedback from @' + reviewer.login + ' (review #' + reviewer.review_id + ').\n\n' +
|
||||
lines.join('\n')).slice(0, 10000);
|
||||
},
|
||||
postFeedbackResponse(item, detail, reviewer, login, state) {
|
||||
if (feedbackRequest) return feedbackRequest;
|
||||
const key = feedbackKey(item, detail, reviewer, login);
|
||||
let operationId;
|
||||
try {
|
||||
operationId = storage?.getItem(key + ':operation') || String(createOperationId()).slice(0, 128);
|
||||
storage?.setItem(key + ':operation', operationId);
|
||||
} catch (_error) { operationId = String(createOperationId()).slice(0, 128); }
|
||||
feedbackRequest = fetchJson(pathFor(item) + '/comments', {
|
||||
method:'POST',
|
||||
headers:{ Accept:'application/json', 'Content-Type':'application/json', 'Idempotency-Key':operationId },
|
||||
body:JSON.stringify({ body:this.feedbackSummary(reviewer, state) }),
|
||||
}).then(result => {
|
||||
try { storage?.removeItem(key + ':operation'); } catch (_error) { /* Confirmed upstream. */ }
|
||||
return result;
|
||||
}).finally(() => { feedbackRequest = null; });
|
||||
return feedbackRequest;
|
||||
},
|
||||
updateContent(item, draft) {
|
||||
if (editRequest) return editRequest;
|
||||
this.saveEditDraft(item, draft);
|
||||
|
|
@ -616,4 +788,5 @@ createPullSheet.bindOwnershipControls = bindOwnershipControls;
|
|||
createPullSheet.bindContextEditor = bindContextEditor;
|
||||
createPullSheet.resetReviewRequestControls = resetReviewRequestControls;
|
||||
createPullSheet.bindReviewRequestControls = bindReviewRequestControls;
|
||||
createPullSheet.bindFeedbackControls = bindFeedbackControls;
|
||||
if (typeof module !== 'undefined' && module.exports) module.exports = createPullSheet;
|
||||
|
|
|
|||
|
|
@ -2781,6 +2781,9 @@ def _normalize_review_comments(comments: object) -> list[dict]:
|
|||
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]}
|
||||
comment_id = comment.get("id")
|
||||
if isinstance(comment_id, int) and comment_id > 0:
|
||||
item["id"] = comment_id
|
||||
position = comment.get("new_position") or comment.get("old_position")
|
||||
if isinstance(position, int) and position > 0:
|
||||
item["line"] = position
|
||||
|
|
@ -2849,6 +2852,7 @@ def _normalize_reviewer_statuses(pull: dict, reviews: object, head_sha: str) ->
|
|||
current = review["commit_id"] == head_sha
|
||||
status = state_names[review["state"]] if current else "outdated"
|
||||
statuses[key] = {
|
||||
"review_id": review["id"],
|
||||
"login": review["login"],
|
||||
"status": status,
|
||||
"head_sha": review["commit_id"],
|
||||
|
|
|
|||
133
tests/e2e/test_mobile_address_review_feedback_release.py
Normal file
133
tests/e2e/test_mobile_address_review_feedback_release.py
Normal file
|
|
@ -0,0 +1,133 @@
|
|||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
ROOT = Path(__file__).parents[2]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("viewport", [(320, 568), (390, 844)])
|
||||
def test_mobile_author_addresses_every_review_comment_and_requests_updated_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": "current-head",
|
||||
"reviewers": [{
|
||||
"review_id": 42,
|
||||
"login": "sam",
|
||||
"status": "changes_requested",
|
||||
"head_sha": "current-head",
|
||||
"blocking": True,
|
||||
"comments": [
|
||||
{"id": 99, "path": "src/api.py", "body": "Handle the empty state.", "line": 12},
|
||||
{"id": 100, "path": "frontend/app.js", "body": "Explain the retry.", "line": 8},
|
||||
],
|
||||
}],
|
||||
"files": [
|
||||
{"filename": "src/api.py", "status": "modified", "diff_available": True,
|
||||
"diff_lines": ["@@ -11 +11 @@", "+return empty"]},
|
||||
{"filename": "frontend/app.js", "status": "modified", "diff_available": True,
|
||||
"diff_lines": ["@@ -7 +7 @@", "+showRetry()"]},
|
||||
],
|
||||
}
|
||||
|
||||
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 => {
|
||||
globalThis.confirm = () => true;
|
||||
globalThis.feedbackCalls = [];
|
||||
globalThis.feedbackStored = new Map();
|
||||
globalThis.feedbackStorage = {
|
||||
getItem:key => feedbackStored.has(key) ? feedbackStored.get(key) : null,
|
||||
setItem:(key, value) => feedbackStored.set(key, String(value)),
|
||||
removeItem:key => feedbackStored.delete(key),
|
||||
};
|
||||
globalThis.feedbackItem = {repository:'stackchain/api', number:7, key:'stackchain/api#7'};
|
||||
globalThis.feedbackDetail = detail;
|
||||
globalThis.feedbackController = createPullSheet({
|
||||
storage: feedbackStorage,
|
||||
createOperationId: () => 'feedback-operation',
|
||||
fetchJson: async (path, options = {}) => {
|
||||
feedbackCalls.push({path, options});
|
||||
if (path.endsWith('/comments')) return {id: 501, body:JSON.parse(options.body).body};
|
||||
if (path.endsWith('/request-review')) return {
|
||||
number:7, reviewer:'sam', head_sha:'current-head', requested_reviewers:['sam']
|
||||
};
|
||||
throw new Error('unexpected request');
|
||||
},
|
||||
});
|
||||
document.querySelector('#pull-sheet').classList.add('open');
|
||||
document.querySelector('#pull-review').open = true;
|
||||
document.querySelector('#pull-files').innerHTML = detail.files.map((file, index) =>
|
||||
createPullSheet.renderFile(file, index, false, value => String(value)
|
||||
.replaceAll('&', '&').replaceAll('<', '<').replaceAll('>', '>'))
|
||||
).join('');
|
||||
createPullSheet.bindFeedbackControls(document, feedbackController,
|
||||
() => feedbackItem, () => feedbackDetail, () => 'timmy');
|
||||
createPullSheet.review(detail, null, document);
|
||||
}""",
|
||||
detail,
|
||||
)
|
||||
|
||||
page.locator(".pull-review-feedback summary").click()
|
||||
page.locator('[data-address-review-feedback="42"]').click()
|
||||
assert page.locator("#pull-feedback-progress").text_content() == "Comment 1 of 2"
|
||||
assert page.locator("#finish-pull-feedback").is_disabled()
|
||||
page.locator('[data-feedback-disposition="addressed"]').click()
|
||||
page.locator("#next-pull-feedback").click()
|
||||
page.locator("#pull-feedback-note").fill("Added a visible retry message.")
|
||||
page.locator('[data-feedback-disposition="discussion"]').click()
|
||||
assert not page.locator("#finish-pull-feedback").is_disabled()
|
||||
page.locator("#finish-pull-feedback").click()
|
||||
page.locator("#request-feedback-review").click()
|
||||
|
||||
page.wait_for_function("feedbackCalls.length === 2")
|
||||
result = page.evaluate(
|
||||
"""() => ({
|
||||
calls:feedbackCalls,
|
||||
scrollWidth:document.documentElement.scrollWidth,
|
||||
clientWidth:document.documentElement.clientWidth,
|
||||
controlHeights:Array.from(document.querySelectorAll('#pull-feedback-pass button'))
|
||||
.filter(button => !button.hidden).map(button => button.getBoundingClientRect().height),
|
||||
status:document.querySelector('#pull-feedback-status').textContent,
|
||||
secondExpanded:document.querySelector('[data-pull-filename="frontend/app.js"] .pull-file-toggle')
|
||||
.getAttribute('aria-expanded'),
|
||||
persisted:Array.from(feedbackStored.keys()).some(key => key.includes('feedback-pass')),
|
||||
})"""
|
||||
)
|
||||
result["resetHidden"] = page.evaluate(
|
||||
"""() => {
|
||||
createPullSheet.resetReviewRequestControls(document, feedbackDetail);
|
||||
return document.querySelector('#pull-feedback-pass').hidden;
|
||||
}"""
|
||||
)
|
||||
browser.close()
|
||||
|
||||
assert result["scrollWidth"] <= result["clientWidth"]
|
||||
assert min(result["controlHeights"]) >= 44
|
||||
assert result["secondExpanded"] == "true"
|
||||
assert result["persisted"] is True
|
||||
assert result["resetHidden"] is True
|
||||
assert result["calls"][0]["path"].endswith("/comments")
|
||||
assert result["calls"][0]["options"]["headers"]["Idempotency-Key"] == "feedback-operation"
|
||||
summary = result["calls"][0]["options"]["body"]
|
||||
assert "Handle the empty state" in summary
|
||||
assert "Addressed" in summary
|
||||
assert "Needs discussion" in summary
|
||||
assert "Added a visible retry message" in summary
|
||||
assert result["calls"][1]["path"].endswith("/request-review")
|
||||
assert '"reviewer":"sam"' in result["calls"][1]["options"]["body"]
|
||||
assert result["status"] == "Response posted; updated review requested from @sam."
|
||||
|
|
@ -225,14 +225,14 @@ def test_reviewer_statuses_distinguish_waiting_current_and_outdated_decisions():
|
|||
|
||||
assert gitea_proxy._normalize_reviewer_statuses(pull, reviews, "new-head") == [
|
||||
{"login": "casey", "status": "waiting", "head_sha": "new-head", "blocking": True},
|
||||
{"login": "lee", "status": "commented", "head_sha": "new-head", "blocking": False},
|
||||
{"login": "pat", "status": "approved", "head_sha": "new-head", "blocking": False},
|
||||
{"login": "sam", "status": "changes_requested", "head_sha": "new-head", "blocking": True},
|
||||
{"review_id": 3, "login": "lee", "status": "commented", "head_sha": "new-head", "blocking": False},
|
||||
{"review_id": 4, "login": "pat", "status": "approved", "head_sha": "new-head", "blocking": False},
|
||||
{"review_id": 2, "login": "sam", "status": "changes_requested", "head_sha": "new-head", "blocking": True},
|
||||
]
|
||||
|
||||
pull["requested_reviewers"] = []
|
||||
assert gitea_proxy._normalize_reviewer_statuses(pull, reviews[:1], "new-head") == [
|
||||
{"login": "casey", "status": "outdated", "head_sha": "old-head", "blocking": True},
|
||||
{"review_id": 1, "login": "casey", "status": "outdated", "head_sha": "old-head", "blocking": True},
|
||||
]
|
||||
|
||||
|
||||
|
|
@ -280,13 +280,39 @@ def test_review_comments_are_bounded_and_require_a_file_and_body():
|
|||
|
||||
assert len(normalized) == 20
|
||||
assert normalized[0] == {
|
||||
"path": "src/api.py", "body": "Handle the empty state", "line": 4,
|
||||
"id": 1, "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_review_feedback_preserves_stable_review_and_comment_identities():
|
||||
pull = {"requested_reviewers": []}
|
||||
reviews = [{
|
||||
"id": 42,
|
||||
"user": {"login": "sam"},
|
||||
"state": "REQUEST_CHANGES",
|
||||
"commit_id": "abc123",
|
||||
}]
|
||||
|
||||
statuses = gitea_proxy._normalize_reviewer_statuses(pull, reviews, "abc123")
|
||||
comments = gitea_proxy._normalize_review_comments([
|
||||
{"id": 99, "path": "src/api.py", "body": "Handle the empty state", "new_position": 4},
|
||||
{"id": -1, "path": "src/other.py", "body": "Invalid identity is omitted"},
|
||||
])
|
||||
|
||||
assert statuses == [{
|
||||
"review_id": 42,
|
||||
"login": "sam",
|
||||
"status": "changes_requested",
|
||||
"head_sha": "abc123",
|
||||
"blocking": True,
|
||||
}]
|
||||
assert comments[0]["id"] == 99
|
||||
assert "id" not in comments[1]
|
||||
|
||||
|
||||
def test_inline_feedback_is_loaded_only_for_each_reviewers_latest_decision():
|
||||
reviews = [
|
||||
{"id": 7, "user": {"login": "sam"}, "state": "REQUEST_CHANGES"},
|
||||
|
|
@ -416,7 +442,7 @@ async def test_gitea_assigned_pull_review_includes_bounded_diff_previews():
|
|||
assert detail["files"][1]["diff_available"] is False
|
||||
assert detail["reviewers"] == [
|
||||
{"login": "casey", "status": "waiting", "head_sha": "abc123", "blocking": True},
|
||||
{"login": "sam", "status": "approved", "head_sha": "abc123", "blocking": False},
|
||||
{"review_id": 8, "login": "sam", "status": "approved", "head_sha": "abc123", "blocking": False},
|
||||
]
|
||||
assert "conversation" not in detail
|
||||
|
||||
|
|
@ -441,7 +467,7 @@ async def test_assigned_pull_review_includes_latest_inline_feedback():
|
|||
])
|
||||
if path.endswith("/pulls/7/reviews/8/comments"):
|
||||
return httpx.Response(200, json=[
|
||||
{"path": "src/api.py", "body": "Return before parsing.", "new_position": 12},
|
||||
{"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"}])
|
||||
|
|
@ -459,9 +485,9 @@ async def test_assigned_pull_review_includes_latest_inline_feedback():
|
|||
|
||||
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,
|
||||
"review_id": 8, "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}],
|
||||
"comments": [{"id": 99, "path": "src/api.py", "body": "Return before parsing.", "line": 12}],
|
||||
}]
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user