From b51542363cb98aae0a55df734d941a3b8352bf4d Mon Sep 17 00:00:00 2001 From: timmy Date: Thu, 6 Aug 2026 22:25:22 +0000 Subject: [PATCH] fix: keep mobile review handoff popup-safe (#143) --- frontend/index.html | 57 +++++++++++++++++++++++++---------- frontend/review-sheet.js | 12 ++++++-- tests/test_my_work.py | 64 ++++++++++++++++++++++++++++++---------- 3 files changed, 99 insertions(+), 34 deletions(-) diff --git a/frontend/index.html b/frontend/index.html index ccab759..e392e87 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -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-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-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-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%; } @@ -264,6 +266,7 @@ textarea { resize: vertical; min-height: 120px; } +

Review history

@@ -323,6 +326,7 @@ textarea { resize: vertical; min-height: 120px; } let reviewFiles = []; let bulkConfirmationPending = false; let bulkMarkPending = false; + let reviewHandoffPending = false; async function fetchReviewJson(url, options) { const response = await fetch(url, options); @@ -533,6 +537,8 @@ textarea { resize: vertical; min-height: 120px; } qs('#review-summary').value = ''; qs('#review-copy-fallback').hidden = true; qs('#review-copy-fallback').value = ''; + qs('#review-handoff-link').hidden = true; + qs('#review-handoff-link').href = item.url; qs('#review-handoff-status').textContent = ''; draft = null; reviewFiles = []; @@ -735,24 +741,43 @@ textarea { resize: vertical; min-height: 120px; } qs('#review-summary').addEventListener('input', event => draft?.setSummary(event.target.value)); qs('#review-decision').addEventListener('change', event => draft?.setDecision(event.target.value)); 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 directLink = qs('#review-handoff-link'); fallback.hidden = true; - const copied = await createReviewController.copyAndContinue({ - text: createReviewController.formatFeedback(draft.snapshot(), reviewFiles), - url: selectedReview.url, - copy: text => navigator.clipboard.writeText(text), - open: url => window.open(url, '_blank', 'noopener,noreferrer'), - fallback: text => { - fallback.value = text; - fallback.hidden = false; - fallback.focus(); - fallback.select(); - }, - }); - qs('#review-handoff-status').textContent = copied ? - 'Feedback copied. Opening Gitea…' : - 'Clipboard unavailable. Copy the selected feedback, then use Open in Gitea.'; + directLink.hidden = true; + let handoffWindow = null; + try { + const result = await createReviewController.copyAndContinue({ + text: createReviewController.formatFeedback(draft.snapshot(), reviewFiles), + url: selectedReview.url, + copy: text => navigator.clipboard.writeText(text), + open: () => { + handoffWindow = window.open('about:blank', '_blank'); + if (handoffWindow) handoffWindow.opener = null; + return handoffWindow; + }, + fallback: text => { + fallback.value = text; + fallback.hidden = false; + fallback.focus(); + fallback.select(); + }, + }); + if (result.opened) { + qs('#review-handoff-status').textContent = 'Feedback copied. Gitea opened in a new tab.'; + } 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({ diff --git a/frontend/review-sheet.js b/frontend/review-sheet.js index a090251..ed56a65 100644 --- a/frontend/review-sheet.js +++ b/frontend/review-sheet.js @@ -166,14 +166,20 @@ function formatFeedback(draft, files) { } async function copyAndContinue({ text, url, copy, open, fallback }) { + const destination = open(); try { await copy(text); } catch (error) { + destination?.close?.(); fallback(text); - return false; + return { copied: false, opened: false }; } - open(url); - return true; + if (!destination) { + fallback(text); + return { copied: true, opened: false }; + } + destination.location.href = url; + return { copied: true, opened: true }; } createReviewController.renderDiffFile = renderDiffFile; diff --git a/tests/test_my_work.py b/tests/test_my_work.py index f77c9c4..73996d4 100644 --- a/tests/test_my_work.py +++ b/tests/test_my_work.py @@ -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""" const reviewSheet = require({json.dumps(str(REVIEW_SHEET))}); const events = []; +let finishCopy; +const reserved = {{ location: {{ href: '' }}, close: () => events.push('close') }}; const success = reviewSheet.copyAndContinue({{ text: 'feedback', url: 'https://forge.example/pulls/7', - copy: async text => events.push('copy:' + text), - open: url => events.push('open:' + url), + copy: text => new Promise(resolve => {{ events.push('copy:' + text); finishCopy = resolve; }}), + open: () => {{ events.push('reserve'); return reserved; }}, fallback: text => events.push('fallback:' + text), }}); -const failedEvents = []; -const failure = reviewSheet.copyAndContinue({{ - text: 'keep me', url: 'https://forge.example/pulls/8', - copy: async () => {{ throw new Error('denied'); }}, - open: url => failedEvents.push('open:' + url), - fallback: text => failedEvents.push('fallback:' + text), +const beforeCopySettles = events.slice(); +finishCopy(); +const blockedEvents = []; +const blocked = reviewSheet.copyAndContinue({{ + text: 'copied', url: 'https://forge.example/pulls/8', + 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({{ - events, failedEvents, results +Promise.all([success, blocked]).then(results => process.stdout.write(JSON.stringify({{ + beforeCopySettles, events, blockedEvents, destination: reserved.location.href, results }}))); """ result = subprocess.run( @@ -560,9 +564,35 @@ Promise.all([success, failure]).then(results => process.stdout.write(JSON.string ) assert json.loads(result.stdout) == { - "events": ["copy:feedback", "open:https://forge.example/pulls/7"], - "failedEvents": ["fallback:keep me"], - "results": [True, False], + "beforeCopySettles": ["reserve", "copy:feedback"], + "events": ["reserve", "copy:feedback"], + "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="copy-review-feedback"' 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-handoff' in html and 'position:sticky' 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.copyAndContinue" 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