Keep mobile review handoff working when popups are blocked #144

Merged
timmy merged 1 commits from timmy/143-popup-safe-review-handoff into main 2026-08-06 22:26:13 +00:00
3 changed files with 99 additions and 34 deletions

View File

@ -87,6 +87,8 @@ textarea { resize: vertical; min-height: 120px; }
.review-feedback select { min-height:44px; padding:8px; border-radius:8px; border:1px solid #1f3a5f; background:#0b1526; color:#e5e7eb; } .review-feedback select { min-height:44px; padding:8px; border-radius:8px; border:1px solid #1f3a5f; background:#0b1526; color:#e5e7eb; }
.review-handoff { position:sticky; bottom:0; z-index:3; display:grid; gap:8px; padding:10px 4px; background:rgba(11,21,38,.98); border-top:1px solid #2a496e; } .review-handoff { position:sticky; bottom:0; z-index:3; display:grid; gap:8px; padding:10px 4px; background:rgba(11,21,38,.98); border-top:1px solid #2a496e; }
.review-handoff button { min-height:44px; } .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; }
.review-handoff-link[hidden] { display:none; }
.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%; }
@ -264,6 +266,7 @@ textarea { resize: vertical; min-height: 120px; }
<button id="copy-review-feedback">Copy feedback &amp; continue in Gitea</button> <button id="copy-review-feedback">Copy feedback &amp; continue in Gitea</button>
<span class="small" id="review-handoff-status" aria-live="polite"></span> <span class="small" id="review-handoff-status" aria-live="polite"></span>
<textarea class="review-copy-fallback" id="review-copy-fallback" readonly hidden aria-label="Feedback to copy manually"></textarea> <textarea class="review-copy-fallback" id="review-copy-fallback" readonly hidden aria-label="Feedback to copy manually"></textarea>
<a class="review-handoff-link" id="review-handoff-link" target="_blank" rel="noopener noreferrer" hidden>Continue to Gitea</a>
</div> </div>
</section> </section>
<h2>Review history</h2> <h2>Review history</h2>
@ -323,6 +326,7 @@ textarea { resize: vertical; min-height: 120px; }
let reviewFiles = []; let reviewFiles = [];
let bulkConfirmationPending = false; let bulkConfirmationPending = false;
let bulkMarkPending = false; let bulkMarkPending = false;
let reviewHandoffPending = false;
async function fetchReviewJson(url, options) { async function fetchReviewJson(url, options) {
const response = await fetch(url, options); const response = await fetch(url, options);
@ -533,6 +537,8 @@ textarea { resize: vertical; min-height: 120px; }
qs('#review-summary').value = ''; qs('#review-summary').value = '';
qs('#review-copy-fallback').hidden = true; qs('#review-copy-fallback').hidden = true;
qs('#review-copy-fallback').value = ''; qs('#review-copy-fallback').value = '';
qs('#review-handoff-link').hidden = true;
qs('#review-handoff-link').href = item.url;
qs('#review-handoff-status').textContent = ''; qs('#review-handoff-status').textContent = '';
draft = null; draft = null;
reviewFiles = []; reviewFiles = [];
@ -735,14 +741,24 @@ textarea { resize: vertical; min-height: 120px; }
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('#copy-review-feedback').addEventListener('click', async () => { qs('#copy-review-feedback').addEventListener('click', async () => {
if (!draft || !selectedReview) return; if (!draft || !selectedReview || reviewHandoffPending) return;
reviewHandoffPending = true;
qs('#copy-review-feedback').disabled = true;
const fallback = qs('#review-copy-fallback'); const fallback = qs('#review-copy-fallback');
const directLink = qs('#review-handoff-link');
fallback.hidden = true; fallback.hidden = true;
const copied = await createReviewController.copyAndContinue({ directLink.hidden = true;
let handoffWindow = null;
try {
const result = await createReviewController.copyAndContinue({
text: createReviewController.formatFeedback(draft.snapshot(), reviewFiles), text: createReviewController.formatFeedback(draft.snapshot(), reviewFiles),
url: selectedReview.url, url: selectedReview.url,
copy: text => navigator.clipboard.writeText(text), copy: text => navigator.clipboard.writeText(text),
open: url => window.open(url, '_blank', 'noopener,noreferrer'), open: () => {
handoffWindow = window.open('about:blank', '_blank');
if (handoffWindow) handoffWindow.opener = null;
return handoffWindow;
},
fallback: text => { fallback: text => {
fallback.value = text; fallback.value = text;
fallback.hidden = false; fallback.hidden = false;
@ -750,9 +766,18 @@ textarea { resize: vertical; min-height: 120px; }
fallback.select(); fallback.select();
}, },
}); });
qs('#review-handoff-status').textContent = copied ? if (result.opened) {
'Feedback copied. Opening Gitea…' : qs('#review-handoff-status').textContent = 'Feedback copied. Gitea opened in a new tab.';
'Clipboard unavailable. Copy the selected feedback, then use Open in Gitea.'; } else {
directLink.hidden = false;
qs('#review-handoff-status').textContent = result.copied ?
'Feedback copied. Your browser blocked the new tab; continue with the link below.' :
'Clipboard unavailable. Copy the selected feedback, then continue to Gitea.';
}
} finally {
reviewHandoffPending = false;
qs('#copy-review-feedback').disabled = false;
}
}); });
const contextPoller = createContextPoller({ const contextPoller = createContextPoller({

View File

@ -166,14 +166,20 @@ function formatFeedback(draft, files) {
} }
async function copyAndContinue({ text, url, copy, open, fallback }) { async function copyAndContinue({ text, url, copy, open, fallback }) {
const destination = open();
try { try {
await copy(text); await copy(text);
} catch (error) { } catch (error) {
destination?.close?.();
fallback(text); fallback(text);
return false; return { copied: false, opened: false };
} }
open(url); if (!destination) {
return true; fallback(text);
return { copied: true, opened: false };
}
destination.location.href = url;
return { copied: true, opened: true };
} }
createReviewController.renderDiffFile = renderDiffFile; createReviewController.renderDiffFile = renderDiffFile;

View File

@ -534,25 +534,29 @@ process.stdout.write(markdown);
) )
def test_review_handoff_opens_gitea_only_after_copy_and_preserves_failed_copy(): def test_review_handoff_reserves_window_before_copy_and_reports_blocked_popup():
script = f""" script = f"""
const reviewSheet = require({json.dumps(str(REVIEW_SHEET))}); const reviewSheet = require({json.dumps(str(REVIEW_SHEET))});
const events = []; const events = [];
let finishCopy;
const reserved = {{ location: {{ href: '' }}, close: () => events.push('close') }};
const success = reviewSheet.copyAndContinue({{ const success = reviewSheet.copyAndContinue({{
text: 'feedback', url: 'https://forge.example/pulls/7', text: 'feedback', url: 'https://forge.example/pulls/7',
copy: async text => events.push('copy:' + text), copy: text => new Promise(resolve => {{ events.push('copy:' + text); finishCopy = resolve; }}),
open: url => events.push('open:' + url), open: () => {{ events.push('reserve'); return reserved; }},
fallback: text => events.push('fallback:' + text), fallback: text => events.push('fallback:' + text),
}}); }});
const failedEvents = []; const beforeCopySettles = events.slice();
const failure = reviewSheet.copyAndContinue({{ finishCopy();
text: 'keep me', url: 'https://forge.example/pulls/8', const blockedEvents = [];
copy: async () => {{ throw new Error('denied'); }}, const blocked = reviewSheet.copyAndContinue({{
open: url => failedEvents.push('open:' + url), text: 'copied', url: 'https://forge.example/pulls/8',
fallback: text => failedEvents.push('fallback:' + text), copy: async () => blockedEvents.push('copy'),
open: () => {{ blockedEvents.push('reserve'); return null; }},
fallback: text => blockedEvents.push('fallback:' + text),
}}); }});
Promise.all([success, failure]).then(results => process.stdout.write(JSON.stringify({{ Promise.all([success, blocked]).then(results => process.stdout.write(JSON.stringify({{
events, failedEvents, results beforeCopySettles, events, blockedEvents, destination: reserved.location.href, results
}}))); }})));
""" """
result = subprocess.run( result = subprocess.run(
@ -560,9 +564,35 @@ Promise.all([success, failure]).then(results => process.stdout.write(JSON.string
) )
assert json.loads(result.stdout) == { assert json.loads(result.stdout) == {
"events": ["copy:feedback", "open:https://forge.example/pulls/7"], "beforeCopySettles": ["reserve", "copy:feedback"],
"failedEvents": ["fallback:keep me"], "events": ["reserve", "copy:feedback"],
"results": [True, False], "blockedEvents": ["reserve", "copy", "fallback:copied"],
"destination": "https://forge.example/pulls/7",
"results": [
{"copied": True, "opened": True},
{"copied": True, "opened": False},
],
}
def test_review_handoff_closes_reserved_window_when_copy_fails():
script = f"""
const reviewSheet = require({json.dumps(str(REVIEW_SHEET))});
const events = [];
reviewSheet.copyAndContinue({{
text: 'keep me', url: 'https://forge.example/pulls/8',
copy: async () => {{ throw new Error('denied'); }},
open: () => ({{ close: () => events.push('close') }}),
fallback: text => events.push('fallback:' + text),
}}).then(result => process.stdout.write(JSON.stringify({{ events, result }})));
"""
result = subprocess.run(
["node", "-e", script], check=True, capture_output=True, text=True
)
assert json.loads(result.stdout) == {
"events": ["close", "fallback:keep me"],
"result": {"copied": False, "opened": False},
} }
@ -611,6 +641,9 @@ async def test_mobile_review_sheet_captures_and_safely_hands_off_feedback():
assert 'id="review-summary"' in html assert 'id="review-summary"' in html
assert 'id="copy-review-feedback"' in html assert 'id="copy-review-feedback"' in html
assert 'id="review-copy-fallback"' in html assert 'id="review-copy-fallback"' in html
assert 'id="review-handoff-link"' in html
assert '.review-handoff-link' in html and 'min-height:44px' in html
assert '.review-handoff-link[hidden]' in html and 'display:none' in html
assert '.review-note' in html and 'min-height:88px' in html assert '.review-note' in html and 'min-height:88px' in html
assert '.review-handoff' in html and 'position:sticky' in html assert '.review-handoff' in html and 'position:sticky' in html
assert "createReviewController.createDraft" in html assert "createReviewController.createDraft" in html
@ -620,7 +653,8 @@ async def test_mobile_review_sheet_captures_and_safely_hands_off_feedback():
assert "createReviewController.formatFeedback" in html assert "createReviewController.formatFeedback" in html
assert "createReviewController.copyAndContinue" in html assert "createReviewController.copyAndContinue" in html
assert "navigator.clipboard.writeText" in html assert "navigator.clipboard.writeText" in html
assert "window.open(url, '_blank', 'noopener,noreferrer')" in html assert "window.open('about:blank', '_blank')" in html
assert "handoffWindow.opener = null" in html
@pytest.mark.anyio @pytest.mark.anyio