Apply reviewer code suggestions from mobile #1383
|
|
@ -765,6 +765,15 @@ textarea { resize: vertical; min-height: 120px; }
|
|||
.review-feedback { display:grid; gap:8px; }
|
||||
.review-feedback label { display:grid; gap:6px; }
|
||||
.review-feedback select { min-height:44px; padding:8px; border-radius:8px; border:1px solid #1f3a5f; background:#0b1526; color:#e5e7eb; }
|
||||
.pull-feedback-code-actions, .pull-feedback-file-actions { display:grid; grid-template-columns:repeat(2,minmax(0,1fr)); gap:8px; }
|
||||
.pull-feedback-code-actions button, .pull-feedback-file-actions button { min-height:44px; }
|
||||
.pull-feedback-suggestion-preview { min-width:0; display:grid; gap:10px; margin:10px 0; padding:10px; border:1px solid #31577f; border-radius:10px; background:#07101e; }
|
||||
.pull-feedback-suggestion-preview[hidden] { display:none; }
|
||||
.pull-feedback-suggestion-preview h5 { margin:0; }
|
||||
.pull-feedback-suggestion-change { display:grid; gap:8px; min-width:0; }
|
||||
.pull-feedback-suggestion-change > div { min-width:0; }
|
||||
.pull-feedback-suggestion-change pre { box-sizing:border-box; max-width:100%; max-height:160px; white-space:pre-wrap; overflow-wrap:anywhere; word-break:break-word; }
|
||||
.pull-feedback-suggestion-preview input { box-sizing:border-box; width:100%; min-height:44px; }
|
||||
.review-handoff { position:sticky; bottom:0; z-index:3; display:grid; gap:8px; padding:10px 4px; padding-bottom:calc(10px + env(safe-area-inset-bottom)); background:rgba(11,21,38,.98); border-top:1px solid #2a496e; }
|
||||
.review-handoff button { min-height:44px; }
|
||||
.review-handoff-link { min-height:44px; display:flex; align-items:center; justify-content:center; border:1px solid #60a5fa; border-radius:8px; color:#bfdbfe; font-weight:700; text-decoration:none; }
|
||||
|
|
|
|||
|
|
@ -1741,7 +1741,23 @@
|
|||
<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>
|
||||
<button id="make-pull-feedback-fix" type="button" hidden>Make code change</button>
|
||||
<div class="pull-feedback-code-actions">
|
||||
<button id="review-pull-feedback-suggestion" type="button" hidden>Review suggested change</button>
|
||||
<button id="make-pull-feedback-fix" type="button" hidden>Edit full file</button>
|
||||
</div>
|
||||
<section class="pull-feedback-suggestion-preview" id="pull-feedback-suggestion-preview" aria-labelledby="pull-feedback-suggestion-heading" hidden>
|
||||
<h5 id="pull-feedback-suggestion-heading">Suggested change</h5>
|
||||
<div class="pull-feedback-suggestion-change">
|
||||
<div><span class="small">Current line</span><pre id="pull-feedback-suggestion-before"></pre></div>
|
||||
<div><span class="small">Suggested replacement</span><pre id="pull-feedback-suggestion-after"></pre></div>
|
||||
</div>
|
||||
<label for="pull-feedback-suggestion-message">Commit message</label>
|
||||
<input id="pull-feedback-suggestion-message" maxlength="120" value="fix: apply review suggestion">
|
||||
<div class="pull-feedback-file-actions">
|
||||
<button id="cancel-pull-feedback-suggestion" type="button" hidden>Cancel suggestion</button>
|
||||
<button id="apply-pull-feedback-suggestion" type="button" hidden>Commit suggested change</button>
|
||||
</div>
|
||||
</section>
|
||||
<section class="pull-feedback-file-editor" id="pull-feedback-file-editor" hidden>
|
||||
<h5>Edit <span id="pull-feedback-file-path"></span></h5>
|
||||
<label for="pull-feedback-file-content">File content</label>
|
||||
|
|
|
|||
|
|
@ -413,11 +413,29 @@ function bindFeedbackControls(doc, controller, getSelected, getDetail, getLogin)
|
|||
const save = () => {
|
||||
if (active) controller.saveFeedbackPass(active.item, active.detail, active.reviewer, active.login, active.state);
|
||||
};
|
||||
const clearSuggestion = () => {
|
||||
qs('#pull-feedback-suggestion-preview').hidden = true;
|
||||
qs('#cancel-pull-feedback-suggestion').hidden = true;
|
||||
qs('#apply-pull-feedback-suggestion').hidden = true;
|
||||
if (active) active.suggestion = null;
|
||||
};
|
||||
const suggestedFile = (file, suggestion) => {
|
||||
const line = Number(suggestion?.line);
|
||||
if (!Number.isInteger(line) || line < 1 || typeof suggestion?.content !== 'string') return null;
|
||||
const trailingNewline = file.content.endsWith('\n');
|
||||
const lines = file.content.split('\n');
|
||||
if (trailingNewline) lines.pop();
|
||||
if (line > lines.length) return null;
|
||||
const before = lines[line - 1];
|
||||
lines.splice(line - 1, 1, ...suggestion.content.split('\n'));
|
||||
return { before, after:suggestion.content, content:lines.join('\n') + (trailingNewline ? '\n' : '') };
|
||||
};
|
||||
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();
|
||||
if (active.suggestion && active.suggestion.commentId !== comment.id) clearSuggestion();
|
||||
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';
|
||||
|
|
@ -434,6 +452,14 @@ function bindFeedbackControls(doc, controller, getSelected, getDetail, getLogin)
|
|||
qs('#request-feedback-review').hidden = active.state.posted !== true;
|
||||
qs('#make-pull-feedback-fix').hidden = active.detail?.capabilities?.authored !== true ||
|
||||
active.detail?.state !== 'open' || active.detail?.merged === true || !comment.path;
|
||||
const canSuggest = active.detail?.capabilities?.authored === true && active.detail?.state === 'open' &&
|
||||
active.detail?.merged !== true && active.reviewer?.head_sha === active.detail?.head_sha &&
|
||||
comment.suggestion?.line === comment.line && typeof comment.suggestion?.content === 'string';
|
||||
qs('#review-pull-feedback-suggestion').hidden = !canSuggest;
|
||||
qs('#review-pull-feedback-suggestion').disabled = canSuggest && globalThis.navigator?.onLine === false;
|
||||
if (canSuggest && globalThis.navigator?.onLine === false) {
|
||||
qs('#pull-feedback-status').textContent = 'Reconnect to apply suggestion.';
|
||||
}
|
||||
const found = focusPullFile(doc, comment.path);
|
||||
qs('#pull-feedback-context').textContent = found ?
|
||||
'Matching change opened below.' : 'Matching change is unavailable in this preview.';
|
||||
|
|
@ -478,13 +504,79 @@ function bindFeedbackControls(doc, controller, getSelected, getDetail, getLogin)
|
|||
state:controller.loadFeedbackPass(item, detail, reviewer, login) };
|
||||
panel.hidden = false;
|
||||
qs('#pull-feedback-file-editor').hidden = true;
|
||||
qs('#pull-feedback-suggestion-preview').hidden = true;
|
||||
qs('#cancel-pull-feedback-fix').hidden = true;
|
||||
qs('#commit-pull-feedback-fix').hidden = true;
|
||||
active.file = null;
|
||||
active.suggestion = null;
|
||||
qs('#pull-feedback-status').textContent = active.state.posted ?
|
||||
'Response already posted. You can request an updated review.' : 'Progress saves on this device.';
|
||||
render();
|
||||
});
|
||||
qs('#review-pull-feedback-suggestion').addEventListener('click', async () => {
|
||||
const comment = current();
|
||||
if (!active || !comment?.suggestion || globalThis.navigator?.onLine === false) {
|
||||
qs('#pull-feedback-status').textContent = 'Reconnect to apply suggestion.';
|
||||
return;
|
||||
}
|
||||
const button = qs('#review-pull-feedback-suggestion');
|
||||
button.disabled = true;
|
||||
qs('#pull-feedback-status').textContent = 'Loading the current line for review…';
|
||||
try {
|
||||
const file = await controller.loadFeedbackFile(active.item, comment.path, active.detail.head_sha);
|
||||
if (!active || file.head_sha !== active.detail.head_sha || file.path !== comment.path) return;
|
||||
const change = suggestedFile(file, comment.suggestion);
|
||||
if (!change) throw new Error('The suggested line is no longer available.');
|
||||
active.suggestion = { file, content:change.content, commentId:comment.id };
|
||||
qs('#pull-feedback-suggestion-before').textContent = change.before;
|
||||
qs('#pull-feedback-suggestion-after').textContent = change.after;
|
||||
qs('#pull-feedback-suggestion-message').value = 'fix: apply review suggestion';
|
||||
qs('#pull-feedback-file-editor').hidden = true;
|
||||
qs('#pull-feedback-suggestion-preview').hidden = false;
|
||||
qs('#cancel-pull-feedback-suggestion').hidden = false;
|
||||
qs('#apply-pull-feedback-suggestion').hidden = false;
|
||||
qs('#pull-feedback-status').textContent = 'Review the bounded replacement before committing.';
|
||||
qs('#apply-pull-feedback-suggestion').focus();
|
||||
} catch (error) {
|
||||
qs('#pull-feedback-status').textContent = error.message + ' Use the full-file editor if needed.';
|
||||
button.focus();
|
||||
} finally {
|
||||
button.disabled = false;
|
||||
}
|
||||
});
|
||||
qs('#cancel-pull-feedback-suggestion').addEventListener('click', () => {
|
||||
clearSuggestion();
|
||||
qs('#review-pull-feedback-suggestion').focus();
|
||||
});
|
||||
qs('#apply-pull-feedback-suggestion').addEventListener('click', async () => {
|
||||
if (!active?.suggestion) return;
|
||||
const message = qs('#pull-feedback-suggestion-message').value.trim();
|
||||
if (!message) {
|
||||
qs('#pull-feedback-status').textContent = 'Enter a commit message before committing.';
|
||||
return;
|
||||
}
|
||||
if (!globalThis.confirm('Commit this suggested change to ' + active.item.key + '?')) return;
|
||||
const button = qs('#apply-pull-feedback-suggestion');
|
||||
button.disabled = true;
|
||||
const pending = active.suggestion;
|
||||
qs('#pull-feedback-status').textContent = 'Committing and verifying the suggested change…';
|
||||
try {
|
||||
const result = await controller.commitFeedbackFix(active.item, {
|
||||
path:pending.file.path, content:pending.content, message,
|
||||
expectedHeadSha:pending.file.head_sha, expectedBlobSha:pending.file.blob_sha,
|
||||
});
|
||||
active.detail.head_sha = result.head_sha;
|
||||
active.state.headSha = result.head_sha;
|
||||
clearSuggestion();
|
||||
save();
|
||||
qs('#pull-feedback-status').textContent = 'Suggested change committed. Mark the comment Addressed when satisfied.';
|
||||
render();
|
||||
} catch (error) {
|
||||
qs('#pull-feedback-status').textContent = error.message + ' Preview kept; reload if the branch changed, or retry.';
|
||||
button.disabled = false;
|
||||
button.focus();
|
||||
}
|
||||
});
|
||||
qs('#make-pull-feedback-fix').addEventListener('click', async () => {
|
||||
const comment = current();
|
||||
if (!active || !comment?.path || active.detail?.capabilities?.authored !== true) return;
|
||||
|
|
@ -495,6 +587,7 @@ function bindFeedbackControls(doc, controller, getSelected, getDetail, getLogin)
|
|||
const file = await controller.loadFeedbackFile(active.item, comment.path, active.detail.head_sha);
|
||||
if (!active || file.head_sha !== active.detail.head_sha || file.path !== comment.path) return;
|
||||
active.file = file;
|
||||
clearSuggestion();
|
||||
qs('#pull-feedback-file-path').textContent = file.path;
|
||||
qs('#pull-feedback-file-content').value = file.content;
|
||||
qs('#pull-feedback-commit-message').value = 'fix: address review feedback';
|
||||
|
|
|
|||
|
|
@ -3618,6 +3618,20 @@ async def pull_completion_detail(
|
|||
}
|
||||
|
||||
|
||||
def _single_line_suggestion(body: str, line: object) -> dict | None:
|
||||
if not isinstance(line, int) or line <= 0:
|
||||
return None
|
||||
blocks = re.findall(r"(?ms)^```suggestion[ \t]*\r?\n(.*?)\r?\n```[ \t]*$", body)
|
||||
if len(blocks) != 1:
|
||||
return None
|
||||
content = blocks[0].replace("\r\n", "\n")
|
||||
if not content or "\x00" in content or len(content.encode("utf-8")) > 4096:
|
||||
return None
|
||||
if len(content.splitlines()) > 20:
|
||||
return None
|
||||
return {"line": line, "content": content}
|
||||
|
||||
|
||||
def _normalize_review_comments(comments: object) -> list[dict]:
|
||||
normalized = []
|
||||
for comment in (comments if isinstance(comments, list) else [])[:100]:
|
||||
|
|
@ -3634,6 +3648,9 @@ def _normalize_review_comments(comments: object) -> list[dict]:
|
|||
position = comment.get("new_position") or comment.get("old_position")
|
||||
if isinstance(position, int) and position > 0:
|
||||
item["line"] = position
|
||||
suggestion = _single_line_suggestion(body, position)
|
||||
if suggestion is not None:
|
||||
item["suggestion"] = suggestion
|
||||
normalized.append(item)
|
||||
if len(normalized) == 20:
|
||||
break
|
||||
|
|
|
|||
103
tests/e2e/test_mobile_apply_review_suggestion_release.py
Normal file
103
tests/e2e/test_mobile_apply_review_suggestion_release.py
Normal file
|
|
@ -0,0 +1,103 @@
|
|||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
ROOT = Path(__file__).parents[2]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("viewport", [(320, 568), (390, 844)])
|
||||
def test_mobile_author_previews_and_applies_review_suggestion(viewport):
|
||||
playwright = pytest.importorskip("playwright.sync_api")
|
||||
html = (ROOT / "frontend" / "index.html").read_text()
|
||||
detail = {
|
||||
"state": "open", "merged": False, "head_sha": "abc1234",
|
||||
"capabilities": {"authored": True},
|
||||
"reviewers": [{
|
||||
"review_id": 42, "login": "sam", "status": "changes_requested",
|
||||
"head_sha": "abc1234", "blocking": True, "feedback_state": "loaded",
|
||||
"comments": [{
|
||||
"id": 99, "path": "src/api.py", "line": 2,
|
||||
"body": "Return the typed empty result.\n```suggestion\n return empty_result()\n```",
|
||||
"suggestion": {"line": 2, "content": " return empty_result()"},
|
||||
}, {
|
||||
"id": 100, "path": "src/api.py", "line": 3,
|
||||
"body": "Keep the result typed.",
|
||||
}],
|
||||
}],
|
||||
"files": [{"filename": "src/api.py", "status": "modified", "diff_available": True,
|
||||
"diff_lines": ["@@ -2 +2 @@", "+ return None"]}],
|
||||
}
|
||||
|
||||
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.suggestionCalls = [];
|
||||
globalThis.suggestionItem = {repository:'stackchain/api', number:7, key:'stackchain/api#7'};
|
||||
globalThis.suggestionDetail = detail;
|
||||
globalThis.suggestionController = createPullSheet({fetchJson: async (path, options = {}) => {
|
||||
suggestionCalls.push({path, options});
|
||||
if ((options.method || 'GET') === 'GET') return {
|
||||
path:'src/api.py', head_sha:'abc1234', blob_sha:'blob123',
|
||||
content:'def lookup():\\n return None\\n'
|
||||
};
|
||||
return {path:'src/api.py', previous_head_sha:'abc1234', head_sha:'def5678', blob_sha:'blob456'};
|
||||
}});
|
||||
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))
|
||||
).join('');
|
||||
createPullSheet.bindFeedbackControls(document, suggestionController,
|
||||
() => suggestionItem, () => suggestionDetail, () => 'alex');
|
||||
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-file-editor").is_hidden()
|
||||
assert page.locator("#review-pull-feedback-suggestion").is_visible()
|
||||
page.locator("#review-pull-feedback-suggestion").click()
|
||||
page.wait_for_function("suggestionCalls.length === 1")
|
||||
assert page.locator("#pull-feedback-suggestion-preview").is_visible()
|
||||
assert page.locator("#pull-feedback-suggestion-before").text_content() == " return None"
|
||||
assert page.locator("#pull-feedback-suggestion-after").text_content() == " return empty_result()"
|
||||
assert page.locator("#pull-feedback-file-editor").is_hidden()
|
||||
page.locator("#next-pull-feedback").click()
|
||||
assert page.locator("#pull-feedback-suggestion-preview").is_hidden()
|
||||
assert page.locator("#review-pull-feedback-suggestion").is_hidden()
|
||||
page.locator("#previous-pull-feedback").click()
|
||||
page.locator("#review-pull-feedback-suggestion").click()
|
||||
page.wait_for_function("suggestionCalls.length === 2")
|
||||
page.locator("#apply-pull-feedback-suggestion").click()
|
||||
page.wait_for_function("suggestionCalls.length === 3")
|
||||
result = page.evaluate("""() => ({
|
||||
calls:suggestionCalls, head:suggestionDetail.head_sha,
|
||||
previewHidden:document.querySelector('#pull-feedback-suggestion-preview').hidden,
|
||||
status:document.querySelector('#pull-feedback-status').textContent,
|
||||
scrollWidth:document.documentElement.scrollWidth,
|
||||
clientWidth:document.documentElement.clientWidth,
|
||||
heights:Array.from(document.querySelectorAll('#pull-feedback-pass button'))
|
||||
.filter(button => button.getClientRects().length > 0).map(button => button.getBoundingClientRect().height),
|
||||
})""")
|
||||
browser.close()
|
||||
|
||||
assert result["scrollWidth"] <= result["clientWidth"]
|
||||
assert min(result["heights"]) >= 44
|
||||
assert result["head"] == "def5678"
|
||||
assert result["previewHidden"] is True
|
||||
assert "suggested change committed" in result["status"].lower()
|
||||
body = result["calls"][2]["options"]["body"]
|
||||
assert '"expected_blob_sha":"blob123"' in body
|
||||
assert '"content":"def lookup():\\n return empty_result()\\n"' in body
|
||||
|
|
@ -875,6 +875,34 @@ def test_review_comments_are_bounded_and_require_a_file_and_body():
|
|||
assert all(item["path"] and item["body"] for item in normalized)
|
||||
|
||||
|
||||
def test_review_comments_expose_one_bounded_line_suggestion_only():
|
||||
comments = [
|
||||
{
|
||||
"id": 1,
|
||||
"path": "src/api.py",
|
||||
"new_position": 4,
|
||||
"body": "Handle the empty state:\n```suggestion\nreturn empty_result()\n```",
|
||||
},
|
||||
{
|
||||
"id": 2,
|
||||
"path": "src/api.py",
|
||||
"new_position": 7,
|
||||
"body": "Choose one:\n```suggestion\nfirst()\n```\n```suggestion\nsecond()\n```",
|
||||
},
|
||||
{
|
||||
"id": 3,
|
||||
"path": "src/api.py",
|
||||
"body": "No stable line:\n```suggestion\nunsafe()\n```",
|
||||
},
|
||||
]
|
||||
|
||||
normalized = gitea_proxy._normalize_review_comments(comments)
|
||||
|
||||
assert normalized[0]["suggestion"] == {"line": 4, "content": "return empty_result()"}
|
||||
assert "suggestion" not in normalized[1]
|
||||
assert "suggestion" not in normalized[2]
|
||||
|
||||
|
||||
def test_review_feedback_preserves_stable_review_and_comment_identities():
|
||||
pull = {"requested_reviewers": []}
|
||||
reviews = [{
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user