Wrap mobile review diffs without horizontal panning #446

Merged
timmy merged 1 commits from timmy/445-wrap-mobile-review-diffs into main 2026-08-10 02:39:39 +00:00
6 changed files with 133 additions and 14 deletions

View File

@ -94,7 +94,12 @@ Approve, and Request changes reviews, and assigned-PR merge require repository
write permission. Native Comment, Approve, and Request changes reviews support write permission. Native Comment, Approve, and Request changes reviews support
head-scoped draft comments anchored to changed lines; the dashboard validates each head-scoped draft comments anchored to changed lines; the dashboard validates each
comment path and submits the summary, decision, and inline comments in one review comment path and submits the summary, decision, and inline comments in one review
request. The dashboard rechecks the current pull-request head, CI success, draft request. On phone-width review sheets, changed lines wrap inside a stable old/new
line-number gutter by default so long source lines remain readable and commentable
without horizontal panning. **Lines wrapped** toggles back to whitespace-preserving
horizontal inspection, and an explicit choice persists on the device across files,
review-sheet reopen, and reload. Desktop review diffs remain horizontally scrollable
until wrapping is explicitly enabled. The dashboard rechecks the current pull-request head, CI success, draft
state, and mergeability immediately before every merge. state, and mergeability immediately before every merge.
Serve the dashboard only to trusted users on its own origin; cross-origin API Serve the dashboard only to trusted users on its own origin; cross-origin API
access is intentionally disabled. Authentication defaults to fail-closed access is intentionally disabled. Authentication defaults to fail-closed

View File

@ -187,9 +187,18 @@ textarea { resize: vertical; min-height: 120px; }
.review-copy-fallback { min-height:140px; } .review-copy-fallback { min-height:140px; }
.review-progress-actions { position:sticky; bottom:0; z-index:2; display:flex; align-items:center; justify-content:space-between; gap:10px; margin:8px -4px 0; padding:10px 4px; background:rgba(11,21,38,.96); border-top:1px solid #2a496e; } .review-progress-actions { position:sticky; bottom:0; z-index:2; display:flex; align-items:center; justify-content:space-between; gap:10px; margin:8px -4px 0; padding:10px 4px; background:rgba(11,21,38,.96); border-top:1px solid #2a496e; }
.review-progress-actions button { min-height:44px; max-width:100%; } .review-progress-actions button { min-height:44px; max-width:100%; }
.review-display-tools { display:flex; align-items:center; gap:10px; margin-bottom:8px; }
.review-display-tools button { min-height:44px; flex:0 0 auto; }
.review-display-tools button[aria-pressed="true"] { color:#bfdbfe; background:#17365a; outline:1px solid #60a5fa; }
.review-diff { overflow-x:auto; max-width:100%; margin-top:8px; white-space:pre; } .review-diff { overflow-x:auto; max-width:100%; margin-top:8px; white-space:pre; }
.review-diff-line { display:block; min-width:max-content; } .review-diff-line { display:grid; grid-template-columns:max-content minmax(max-content,1fr); align-items:stretch; min-width:max-content; }
.review-inline-target { min-height:44px; width:100%; padding:8px; border:0; border-radius:0; text-align:left; font:inherit; white-space:pre; } .review-line-numbers { position:sticky; left:0; z-index:1; display:grid; grid-template-columns:4ch 4ch; align-self:stretch; background:#0b1526; color:#7694b6; user-select:none; }
.review-line-number { padding:8px 4px; text-align:right; border-right:1px solid #244363; }
.review-line-code { min-width:0; padding:8px; white-space:pre; overflow-wrap:anywhere; }
.review-inline-target { min-height:44px; width:100%; padding:0; border:0; border-radius:0; text-align:left; font:inherit; white-space:normal; }
.wrap-lines .review-diff { overflow-x:hidden; white-space:normal; }
.wrap-lines .review-diff-line { grid-template-columns:max-content minmax(0,1fr); min-width:0; }
.wrap-lines .review-line-code { white-space:pre-wrap; overflow-wrap:anywhere; }
.review-inline-target.has-draft { box-shadow:inset 4px 0 #fbbf24; } .review-inline-target.has-draft { box-shadow:inset 4px 0 #fbbf24; }
.review-inline-composer { position:sticky; bottom:0; z-index:4; display:grid; gap:8px; padding:10px; padding-bottom:calc(10px + env(safe-area-inset-bottom)); border:1px solid #60a5fa; border-radius:10px; background:#0b1526; } .review-inline-composer { position:sticky; bottom:0; z-index:4; display:grid; gap:8px; padding:10px; padding-bottom:calc(10px + env(safe-area-inset-bottom)); border:1px solid #60a5fa; border-radius:10px; background:#0b1526; }
.review-inline-composer[hidden] { display:none; } .review-inline-composer[hidden] { display:none; }

View File

@ -237,6 +237,10 @@
return payload; return payload;
} }
const reviewController = createReviewController({ fetchJson: fetchReviewJson, storage: localStorage }); const reviewController = createReviewController({ fetchJson: fetchReviewJson, storage: localStorage });
const wrapPreference = createReviewController.createWrapPreference({
storage: localStorage,
mobile: window.matchMedia('(max-width: 600px)').matches,
});
const issueController = createIssueSheet({ fetchJson: fetchReviewJson, storage: localStorage }); const issueController = createIssueSheet({ fetchJson: fetchReviewJson, storage: localStorage });
const planningLoader = createIssueSheet.createPlanningLoader({ const planningLoader = createIssueSheet.createPlanningLoader({
loadLabels: item => issueController.loadLabels(item), loadLabels: item => issueController.loadLabels(item),
@ -1765,6 +1769,14 @@
); );
} }
function applyReviewWrap(snapshot = wrapPreference.snapshot()) {
const reviewFilesElement = qs('#review-files');
const button = qs('#review-wrap-lines');
reviewFilesElement.classList.toggle('wrap-lines', snapshot.wrapped);
button.setAttribute('aria-pressed', String(snapshot.wrapped));
button.textContent = snapshot.wrapped ? 'Lines wrapped' : 'Wrap lines';
}
function showReviewProgress(snapshot) { function showReviewProgress(snapshot) {
const complete = snapshot.total > 0 && snapshot.reviewedCount === snapshot.total; const complete = snapshot.total > 0 && snapshot.reviewedCount === snapshot.total;
qs('#review-progress').textContent = complete ? qs('#review-progress').textContent = complete ?
@ -2582,6 +2594,7 @@
qs('#retry-review-load').hidden = true; qs('#retry-review-load').hidden = true;
qs('#review-sheet-body').textContent = ''; qs('#review-sheet-body').textContent = '';
qs('#review-files').textContent = ''; qs('#review-files').textContent = '';
applyReviewWrap();
qs('#review-progress').textContent = '0 of 0 files reviewed'; qs('#review-progress').textContent = '0 of 0 files reviewed';
qs('#next-unreviewed-review').disabled = true; qs('#next-unreviewed-review').disabled = true;
qs('#review-history').textContent = ''; qs('#review-history').textContent = '';
@ -3910,6 +3923,10 @@
qs('#next-unreviewed-review').addEventListener('click', () => { qs('#next-unreviewed-review').addEventListener('click', () => {
if (progress) openNextUnreviewed(progress.snapshot()); if (progress) openNextUnreviewed(progress.snapshot());
}); });
qs('#review-wrap-lines').addEventListener('click', () => {
const next = !wrapPreference.snapshot().wrapped;
applyReviewWrap(wrapPreference.setWrapped(next));
});
qs('#review-summary').addEventListener('input', event => draft?.setSummary(event.target.value)); qs('#review-summary').addEventListener('input', event => draft?.setSummary(event.target.value));
qs('#review-decision').addEventListener('change', event => draft?.setDecision(event.target.value)); qs('#review-decision').addEventListener('change', event => draft?.setDecision(event.target.value));
qs('#save-inline-comment').addEventListener('click', () => { qs('#save-inline-comment').addEventListener('click', () => {

View File

@ -539,6 +539,10 @@
<div class="review-sheet-body markdown-content" id="review-sheet-body"></div> <div class="review-sheet-body markdown-content" id="review-sheet-body"></div>
<div class="row"><span class="pill" id="review-ci-state">CI unknown</span><button class="share-work-route" type="button">Share</button><a id="open-review-gitea" href="#" target="_blank" rel="noopener noreferrer">Open in Gitea</a></div> <div class="row"><span class="pill" id="review-ci-state">CI unknown</span><button class="share-work-route" type="button">Share</button><a id="open-review-gitea" href="#" target="_blank" rel="noopener noreferrer">Open in Gitea</a></div>
<h2>Changed files</h2> <h2>Changed files</h2>
<div class="review-display-tools">
<button id="review-wrap-lines" type="button" aria-pressed="false" aria-controls="review-files">Wrap lines</button>
<span class="small">Keep long diffs inside the review sheet.</span>
</div>
<div id="review-files" class="muted"></div> <div id="review-files" class="muted"></div>
<section class="review-inline-composer" id="review-inline-composer" aria-labelledby="review-inline-title" hidden> <section class="review-inline-composer" id="review-inline-composer" aria-labelledby="review-inline-title" hidden>
<strong id="review-inline-title">Comment on changed line</strong> <strong id="review-inline-title">Comment on changed line</strong>

View File

@ -47,6 +47,28 @@ function createReviewController({
return { load, submit }; return { load, submit };
} }
function createWrapPreference({ storage, mobile }) {
const key = 'stackchain.review-wrap.v1';
let explicit = false;
let wrapped = Boolean(mobile);
try {
const saved = storage?.getItem(key);
if (saved === 'true' || saved === 'false') {
explicit = true;
wrapped = saved === 'true';
}
} catch (_error) { /* keep the viewport default */ }
function snapshot() { return { wrapped, explicit }; }
function setWrapped(value) {
wrapped = Boolean(value);
explicit = true;
try { storage?.setItem(key, String(wrapped)); } catch (_error) { /* keep in memory */ }
return snapshot();
}
return { snapshot, setWrapped, storageKey: key };
}
function diffLineClass(line) { function diffLineClass(line) {
if (line.startsWith('@@')) return 'hunk'; if (line.startsWith('@@')) return 'hunk';
if (line.startsWith('+')) return 'added'; if (line.startsWith('+')) return 'added';
@ -70,16 +92,19 @@ function parseDiffLines(lines) {
return { text, kind: diffLineClass(text), commentable: false }; return { text, kind: diffLineClass(text), commentable: false };
} }
if (text.startsWith('-')) { if (text.startsWith('-')) {
const row = { text, kind: 'removed', commentable: true, old_position: oldLine }; const row = { text, kind: 'removed', commentable: true, old_line: oldLine, old_position: oldLine };
oldLine += 1; oldLine += 1;
return row; return row;
} }
if (text.startsWith('+')) { if (text.startsWith('+')) {
const row = { text, kind: 'added', commentable: true, new_position: newLine }; const row = { text, kind: 'added', commentable: true, new_line: newLine, new_position: newLine };
newLine += 1; newLine += 1;
return row; return row;
} }
const row = { text, kind: 'context', commentable: true, new_position: newLine }; const row = {
text, kind: 'context', commentable: true,
old_line: oldLine, new_line: newLine, new_position: newLine,
};
oldLine += 1; oldLine += 1;
newLine += 1; newLine += 1;
return row; return row;
@ -91,8 +116,14 @@ function renderDiffFile(file, index, escapeHtml) {
let preview; let preview;
if (file.diff_available) { if (file.diff_available) {
const lines = parseDiffLines(file.diff_lines).map(row => { const lines = parseDiffLines(file.diff_lines).map(row => {
const oldNumber = row.old_line || row.old_position || '';
const newNumber = row.new_line || row.new_position || '';
const content = '<span class="review-line-numbers" aria-hidden="true">' +
'<span class="review-line-number old">' + oldNumber + '</span>' +
'<span class="review-line-number new">' + newNumber + '</span></span>' +
'<span class="review-line-code">' + escapeHtml(row.text) + '</span>';
if (!row.commentable) { if (!row.commentable) {
return '<span class="review-diff-line ' + row.kind + '">' + escapeHtml(row.text) + '</span>'; return '<span class="review-diff-line ' + row.kind + '">' + content + '</span>';
} }
const position = row.old_position const position = row.old_position
? ' data-old-position="' + row.old_position + '"' ? ' data-old-position="' + row.old_position + '"'
@ -100,7 +131,7 @@ function renderDiffFile(file, index, escapeHtml) {
return '<button type="button" class="review-diff-line review-inline-target ' + row.kind + return '<button type="button" class="review-diff-line review-inline-target ' + row.kind +
'" data-review-filename="' + escapeHtml(file.filename || '') + '"' + position + '" data-review-filename="' + escapeHtml(file.filename || '') + '"' + position +
' aria-label="Comment on ' + escapeHtml(file.filename || 'changed file') + ' line ' + ' aria-label="Comment on ' + escapeHtml(file.filename || 'changed file') + ' line ' +
(row.old_position || row.new_position) + '">' + escapeHtml(row.text) + '</button>'; (row.old_position || row.new_position) + '">' + content + '</button>';
}).join(''); }).join('');
preview = '<pre class="review-diff" id="' + panelId + '" hidden>' + lines + preview = '<pre class="review-diff" id="' + panelId + '" hidden>' + lines +
(file.diff_truncated ? '<span class="review-diff-note">Preview truncated · open in Gitea for the full diff.</span>' : '') + (file.diff_truncated ? '<span class="review-diff-note">Preview truncated · open in Gitea for the full diff.</span>' : '') +
@ -325,6 +356,7 @@ function prepareMergeContinuation({ storage, item, headSha, reviewed, decision }
} }
createReviewController.renderDiffFile = renderDiffFile; createReviewController.renderDiffFile = renderDiffFile;
createReviewController.createWrapPreference = createWrapPreference;
createReviewController.parseDiffLines = parseDiffLines; createReviewController.parseDiffLines = parseDiffLines;
createReviewController.toggleDiff = toggleDiff; createReviewController.toggleDiff = toggleDiff;
createReviewController.createProgress = createProgress; createReviewController.createProgress = createProgress;

View File

@ -3950,12 +3950,45 @@ process.stdout.write(JSON.stringify({{ html, expanded: button.attrs['aria-expand
assert 'class="review-diff-line review-inline-target removed"' in output["html"] assert 'class="review-diff-line review-inline-target removed"' in output["html"]
assert 'data-old-position="1"' in output["html"] assert 'data-old-position="1"' in output["html"]
assert 'data-new-position="1"' in output["html"] assert 'data-new-position="1"' in output["html"]
assert '<span class="review-line-numbers" aria-hidden="true"><span class="review-line-number old">1</span><span class="review-line-number new"></span></span>' in output["html"]
assert '<span class="review-line-numbers" aria-hidden="true"><span class="review-line-number old"></span><span class="review-line-number new">1</span></span>' in output["html"]
assert '<span class="review-line-code">-old &lt;token&gt;</span>' in output["html"]
assert '<span class="review-line-code">+new &amp; safe</span>' in output["html"]
assert 'aria-label="Comment on src/&lt;api&gt;.py line 1"' in output["html"] assert 'aria-label="Comment on src/&lt;api&gt;.py line 1"' in output["html"]
assert 'Mark reviewed' in output["html"] assert 'Mark reviewed' in output["html"]
assert output["expanded"] == "true" assert output["expanded"] == "true"
assert output["hidden"] is False assert output["hidden"] is False
def test_review_wrap_preference_defaults_to_phone_layout_and_persists_explicit_choice():
script = f"""
const reviewSheet = require({json.dumps(str(REVIEW_SHEET))});
const values = new Map();
const storage = {{
getItem: key => values.has(key) ? values.get(key) : null,
setItem: (key, value) => values.set(key, value),
}};
const phone = reviewSheet.createWrapPreference({{storage, mobile: true}});
const initial = phone.snapshot();
const disabled = phone.setWrapped(false);
const restoredPhone = reviewSheet.createWrapPreference({{storage, mobile: true}}).snapshot();
const restoredDesktop = reviewSheet.createWrapPreference({{storage, mobile: false}}).snapshot();
process.stdout.write(JSON.stringify({{initial, disabled, restoredPhone, restoredDesktop,
stored: values.get('stackchain.review-wrap.v1')}}));
"""
result = subprocess.run(
["node", "-e", script], check=True, capture_output=True, text=True
)
assert json.loads(result.stdout) == {
"initial": {"wrapped": True, "explicit": False},
"disabled": {"wrapped": False, "explicit": True},
"restoredPhone": {"wrapped": False, "explicit": True},
"restoredDesktop": {"wrapped": False, "explicit": True},
"stored": "false",
}
def test_review_diff_parser_maps_multi_hunk_lines_to_old_or_new_positions(): def test_review_diff_parser_maps_multi_hunk_lines_to_old_or_new_positions():
script = f""" script = f"""
const reviewSheet = require({json.dumps(str(REVIEW_SHEET))}); const reviewSheet = require({json.dumps(str(REVIEW_SHEET))});
@ -3973,14 +4006,14 @@ process.stdout.write(JSON.stringify(rows));
assert json.loads(result.stdout) == [ assert json.loads(result.stdout) == [
{"text": "@@ -10,3 +20,4 @@ function run()", "kind": "hunk", "commentable": False}, {"text": "@@ -10,3 +20,4 @@ function run()", "kind": "hunk", "commentable": False},
{"text": " context", "kind": "context", "commentable": True, "new_position": 20}, {"text": " context", "kind": "context", "commentable": True, "old_line": 10, "new_line": 20, "new_position": 20},
{"text": "-removed", "kind": "removed", "commentable": True, "old_position": 11}, {"text": "-removed", "kind": "removed", "commentable": True, "old_line": 11, "old_position": 11},
{"text": "+added", "kind": "added", "commentable": True, "new_position": 21}, {"text": "+added", "kind": "added", "commentable": True, "new_line": 21, "new_position": 21},
{"text": "+second", "kind": "added", "commentable": True, "new_position": 22}, {"text": "+second", "kind": "added", "commentable": True, "new_line": 22, "new_position": 22},
{"text": "\\ No newline at end of file", "kind": "note", "commentable": False}, {"text": "\\ No newline at end of file", "kind": "note", "commentable": False},
{"text": "@@ -40 +51 @@", "kind": "hunk", "commentable": False}, {"text": "@@ -40 +51 @@", "kind": "hunk", "commentable": False},
{"text": "-old tail", "kind": "removed", "commentable": True, "old_position": 40}, {"text": "-old tail", "kind": "removed", "commentable": True, "old_line": 40, "old_position": 40},
{"text": "+new tail", "kind": "added", "commentable": True, "new_position": 51}, {"text": "+new tail", "kind": "added", "commentable": True, "new_line": 51, "new_position": 51},
] ]
@ -4207,6 +4240,25 @@ async def test_review_requests_open_an_accessible_mobile_detail_sheet():
assert '.review-diff' in html and 'overflow-x:auto' in html assert '.review-diff' in html and 'overflow-x:auto' in html
@pytest.mark.anyio
async def test_mobile_review_wraps_long_lines_with_gutters_and_persisted_toggle():
html = await dashboard()
assert 'id="review-wrap-lines"' in html
assert 'aria-pressed="false"' in html
assert 'aria-controls="review-files"' in html
assert "createReviewController.createWrapPreference" in html
assert "matchMedia('(max-width: 600px)').matches" in html
assert "reviewFilesElement.classList.toggle('wrap-lines', snapshot.wrapped)" in html
assert "wrapPreference.setWrapped" in html
assert '.review-display-tools button' in html and 'min-height:44px' in html
assert '.review-line-numbers' in html and 'grid-template-columns:4ch 4ch' in html
assert '.review-line-code' in html and 'overflow-wrap:anywhere' in html
assert '.wrap-lines .review-diff' in html and 'overflow-x:hidden' in html
assert '.wrap-lines .review-diff-line' in html and 'min-width:0' in html
assert '.wrap-lines .review-line-code' in html and 'white-space:pre-wrap' in html
@pytest.mark.anyio @pytest.mark.anyio
async def test_review_attention_drafts_require_revision_instead_of_resending_stale_payload(): async def test_review_attention_drafts_require_revision_instead_of_resending_stale_payload():
html = await dashboard() html = await dashboard()