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
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
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.
Serve the dashboard only to trusted users on its own origin; cross-origin API
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-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-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-line { display:block; 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-diff-line { display:grid; grid-template-columns:max-content minmax(max-content,1fr); align-items:stretch; min-width:max-content; }
.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-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; }

View File

@ -237,6 +237,10 @@
return payload;
}
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 planningLoader = createIssueSheet.createPlanningLoader({
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) {
const complete = snapshot.total > 0 && snapshot.reviewedCount === snapshot.total;
qs('#review-progress').textContent = complete ?
@ -2582,6 +2594,7 @@
qs('#retry-review-load').hidden = true;
qs('#review-sheet-body').textContent = '';
qs('#review-files').textContent = '';
applyReviewWrap();
qs('#review-progress').textContent = '0 of 0 files reviewed';
qs('#next-unreviewed-review').disabled = true;
qs('#review-history').textContent = '';
@ -3910,6 +3923,10 @@
qs('#next-unreviewed-review').addEventListener('click', () => {
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-decision').addEventListener('change', event => draft?.setDecision(event.target.value));
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="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>
<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>
<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>

View File

@ -47,6 +47,28 @@ function createReviewController({
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) {
if (line.startsWith('@@')) return 'hunk';
if (line.startsWith('+')) return 'added';
@ -70,16 +92,19 @@ function parseDiffLines(lines) {
return { text, kind: diffLineClass(text), commentable: false };
}
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;
return row;
}
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;
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;
newLine += 1;
return row;
@ -91,8 +116,14 @@ function renderDiffFile(file, index, escapeHtml) {
let preview;
if (file.diff_available) {
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) {
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
? ' 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 +
'" data-review-filename="' + escapeHtml(file.filename || '') + '"' + position +
' 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('');
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>' : '') +
@ -325,6 +356,7 @@ function prepareMergeContinuation({ storage, item, headSha, reviewed, decision }
}
createReviewController.renderDiffFile = renderDiffFile;
createReviewController.createWrapPreference = createWrapPreference;
createReviewController.parseDiffLines = parseDiffLines;
createReviewController.toggleDiff = toggleDiff;
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 'data-old-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 'Mark reviewed' in output["html"]
assert output["expanded"] == "true"
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():
script = f"""
const reviewSheet = require({json.dumps(str(REVIEW_SHEET))});
@ -3973,14 +4006,14 @@ process.stdout.write(JSON.stringify(rows));
assert json.loads(result.stdout) == [
{"text": "@@ -10,3 +20,4 @@ function run()", "kind": "hunk", "commentable": False},
{"text": " context", "kind": "context", "commentable": True, "new_position": 20},
{"text": "-removed", "kind": "removed", "commentable": True, "old_position": 11},
{"text": "+added", "kind": "added", "commentable": True, "new_position": 21},
{"text": "+second", "kind": "added", "commentable": True, "new_position": 22},
{"text": " context", "kind": "context", "commentable": True, "old_line": 10, "new_line": 20, "new_position": 20},
{"text": "-removed", "kind": "removed", "commentable": True, "old_line": 11, "old_position": 11},
{"text": "+added", "kind": "added", "commentable": True, "new_line": 21, "new_position": 21},
{"text": "+second", "kind": "added", "commentable": True, "new_line": 22, "new_position": 22},
{"text": "\\ No newline at end of file", "kind": "note", "commentable": False},
{"text": "@@ -40 +51 @@", "kind": "hunk", "commentable": False},
{"text": "-old tail", "kind": "removed", "commentable": True, "old_position": 40},
{"text": "+new tail", "kind": "added", "commentable": True, "new_position": 51},
{"text": "-old tail", "kind": "removed", "commentable": True, "old_line": 40, "old_position": 40},
{"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
@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
async def test_review_attention_drafts_require_revision_instead_of_resending_stale_payload():
html = await dashboard()