From 378291d3c8a8abb5dd81f2ad17bc99437e8735aa Mon Sep 17 00:00:00 2001 From: timmy Date: Thu, 13 Aug 2026 21:59:35 +0000 Subject: [PATCH] feat: review pull requests from update triage (Closes #775) --- frontend/dashboard.css | 2 +- frontend/dashboard.js | 60 +++++++++++++++++- frontend/index.html | 2 + frontend/service-worker.js | 1 + frontend/update-review-handoff.js | 62 +++++++++++++++++++ src/frontend_bundle.py | 2 +- tests/test_service_worker.py | 1 + tests/test_update_review_handoff.py | 96 +++++++++++++++++++++++++++++ 8 files changed, 223 insertions(+), 3 deletions(-) create mode 100644 frontend/update-review-handoff.js create mode 100644 tests/test_update_review_handoff.py diff --git a/frontend/dashboard.css b/frontend/dashboard.css index e6ca520..e5baa40 100644 --- a/frontend/dashboard.css +++ b/frontend/dashboard.css @@ -408,7 +408,7 @@ textarea { resize: vertical; min-height: 120px; } .update-sheet-actions a { border:1px solid #60a5fa; border-radius:10px; font-weight:700; } .update-more-actions { margin-top:14px; } .update-more-actions > summary { min-height:44px; display:flex; align-items:center; justify-content:center; cursor:pointer; border:1px solid #60a5fa; border-radius:10px; font-weight:700; } -.update-decision-bar { display:grid; grid-template-columns:repeat(4,minmax(0,1fr)); gap:6px; margin-top:14px; padding:8px; background:rgba(11,21,38,.98); border-top:1px solid #2a496e; } +.update-decision-bar { display:grid; grid-template-columns:repeat(auto-fit,minmax(76px,1fr)); gap:6px; margin-top:14px; padding:8px; background:rgba(11,21,38,.98); border-top:1px solid #2a496e; } .update-decision-bar button, .update-decision-bar summary { min-height:44px; min-width:0; padding-inline:6px; } .update-retry { min-height:44px; width:100%; margin-top:10px; } .issue-sheet { position:fixed; inset:0; z-index:56; display:none; justify-content:flex-end; background:rgba(5,12,21,.72); backdrop-filter:blur(4px); } diff --git a/frontend/dashboard.js b/frontend/dashboard.js index 615490f..2d526d7 100644 --- a/frontend/dashboard.js +++ b/frontend/dashboard.js @@ -193,6 +193,7 @@ let draft = null; let reviewFiles = []; let selectedReviewHead = ''; + let reviewHandoffSubmitted = false; let activeInlineTarget = null; let bulkConfirmationPending = false; let bulkMarkPending = false; @@ -943,7 +944,7 @@ const updateDecision = createUpdateDecisionTransaction({ controls: [ qs('#keep-update-unread'), qs('#mark-update-read-next'), - qs('#focus-update-reply'), qs('#toggle-update-more'), + qs('#review-update-now'), qs('#focus-update-reply'), qs('#toggle-update-more'), ], status: qs('#update-gesture-status'), }); @@ -985,6 +986,7 @@ qs('#update-ownership-action').hidden = true; qs('#update-ownership-start').hidden = true; qs('#create-update-follow-up').hidden = true; + qs('#review-update-now').hidden = true; qs('#retry-update-load').hidden = true; setOfflineUpdateControls(false); qs('#keep-update-unread').focus(); @@ -999,6 +1001,7 @@ qs('#acknowledge-update-next').hidden = !detail.acknowledge_supported; qs('#mute-update-next').hidden = !detail.mute_supported; qs('#create-update-follow-up').hidden = !['Issue', 'Pull'].includes(detail.subject_type); + qs('#review-update-now').hidden = !updateReviewHandoff.eligible(detail, selectedUpdate); if (offlineWorkMode) { setOfflineUpdateControls(true); } else { @@ -1056,6 +1059,36 @@ showUpdateTriageOutcome(outcome); }, }); + const updateReviewHandoff = createUpdateReviewHandoff({ + openReview: item => { + qs('#update-sheet').classList.remove('open'); + reviewHandoffSubmitted = false; + openReviewSheet(item, qs('#review-update-now'), + offlineWorkMode ? offlineWorkStore.loadDetail(confirmedOwnerLogin, item) : null); + }, + restoreUpdate: () => { + qs('#review-sheet').classList.remove('open'); + selectedReview = null; + qs('#update-sheet').classList.add('open'); + qs('#review-update-now').focus(); + }, + admitRead: notificationId => offlineWorkMode ? + notificationReadOutbox.enqueueDurably(notificationId) : markNotificationRead(notificationId), + advance: source => { + mobileComposerViewport.close(qs('#review-sheet .review-sheet-panel')); + qs('#review-sheet').classList.remove('open'); + selectedReview = null; + offlineReview = false; + progress = null; + draft = null; + reviewFiles = []; + selectedReviewHead = ''; + notificationReader.acceptReadAndNext(lastMyWork, source); + if (updateTriage.active()) updateTriage.acceptCompleted(); + reviewHandoffSubmitted = false; + }, + }); + qs('#review-update-now').addEventListener('click', () => updateReviewHandoff.begin(selectedUpdate)); qs('#review-kept-updates').addEventListener('click', () => { qs('#mobile-queue-sheet').close(); updateTriage.reviewKept(keptUpdateIdentities); @@ -4034,6 +4067,10 @@ } function closeReviewSheet(navigate = true) { + if (updateReviewHandoff.active()) { + updateReviewHandoff.cancel(); + return; + } if (navigate && createWorkRoute.parse(window.location.hash)) { workRoute.close(); return; @@ -5767,6 +5804,17 @@ )) return; const button = qs('#submit-review'); button.disabled = true; + if (updateReviewHandoff.active() && reviewHandoffSubmitted) { + try { + await updateReviewHandoff.complete(); + } catch (error) { + qs('#review-submit-status').textContent = 'Review submitted, but the update is still unread. ' + + error.message + ' Retry to continue.'; + button.disabled = false; + button.focus(); + } + return; + } if (offlineReview) { const queuedTodayReview = reviewingActiveTodayItem(); qs('#review-submit-status').textContent = 'Queueing review safely…'; @@ -5784,6 +5832,11 @@ draftFingerprint: localStorage.getItem(draft.storageKey) || '', progressFingerprint: localStorage.getItem(progress?.storageKey) || '', }); + if (updateReviewHandoff.active()) { + reviewHandoffSubmitted = true; + await updateReviewHandoff.complete(); + return; + } if (queuedTodayReview) { const advanced = completeTodayItem(selectedReview, { successMessage: 'Review queued. Next Today item opened.', @@ -5812,6 +5865,11 @@ expected_head_sha: selectedReviewHead, comments: snapshot.comments, }); + if (updateReviewHandoff.active()) { + reviewHandoffSubmitted = true; + await updateReviewHandoff.complete(); + return; + } const canContinueToMerge = createReviewController.prepareMergeContinuation({ storage: localStorage, item, diff --git a/frontend/index.html b/frontend/index.html index c9cca02..47f8a1b 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -814,6 +814,7 @@
+
@@ -1088,6 +1089,7 @@ + diff --git a/frontend/service-worker.js b/frontend/service-worker.js index c15c851..a0aedfb 100644 --- a/frontend/service-worker.js +++ b/frontend/service-worker.js @@ -77,6 +77,7 @@ const SHELL = [ BASE + 'static/mobile-work-entry.js', BASE + 'static/mobile-queue-launcher.js', BASE + 'static/update-triage-session.js', + BASE + 'static/update-review-handoff.js', BASE + 'static/update-read-position.js', BASE + 'static/update-triage-launcher.js', BASE + 'static/update-triage-gesture.js', diff --git a/frontend/update-review-handoff.js b/frontend/update-review-handoff.js new file mode 100644 index 0000000..0dbbb56 --- /dev/null +++ b/frontend/update-review-handoff.js @@ -0,0 +1,62 @@ +(function (root, factory) { + if (typeof module === 'object' && module.exports) module.exports = factory; + else root.createUpdateReviewHandoff = factory; +})(typeof self !== 'undefined' ? self : this, function createUpdateReviewHandoff(options) { + let source = null; + let completing = null; + + function eligible(detail, item = detail) { + const merged = { ...(item || {}), ...(detail || {}) }; + return Boolean( + merged.is_review && merged.update_reason === 'Review requested' && + merged.subject_type === 'Pull' && /^[^/]+\/[^/]+$/.test(String(merged.repository || '')) && + Number.isInteger(Number(merged.number)) && Number(merged.number) > 0 && + Number.isInteger(Number(merged.notification_id)) + ); + } + + function reviewItem(item) { + return { + ...item, + kind: 'review', + is_review: true, + key: item.key || item.repository + '#' + item.number, + }; + } + + function begin(item) { + if (!eligible(item) || source) return false; + source = item; + options.openReview(reviewItem(item)); + return true; + } + + function cancel() { + if (!source || completing) return false; + const item = source; + source = null; + options.restoreUpdate?.(item); + return true; + } + + function complete() { + if (!source) return Promise.resolve(false); + if (completing) return completing; + const item = source; + completing = Promise.resolve(options.admitRead(item.notification_id)).then(() => { + options.advance(item); + source = null; + return true; + }).finally(() => { completing = null; }); + return completing; + } + + return { + eligible, + begin, + cancel, + complete, + active: () => Boolean(source), + source: () => source, + }; +}); diff --git a/src/frontend_bundle.py b/src/frontend_bundle.py index 4a28fbf..2545d5d 100644 --- a/src/frontend_bundle.py +++ b/src/frontend_bundle.py @@ -28,7 +28,7 @@ FEATURE_SOURCES = { "device-setup": ("static/install-app.js", "static/mobile-device-setup.js"), "security-center": ("static/security-center.js",), "today-timer": ( - "static/my-work.js", "static/protect-today.js", "static/mobile-task-dock.js", "static/mobile-queue-launcher.js", "static/update-triage-session.js", "static/update-triage-launcher.js", "static/update-triage-gesture.js", "static/notification-undo.js", "static/today-timer.js", "static/today-recap.js", + "static/my-work.js", "static/protect-today.js", "static/mobile-task-dock.js", "static/mobile-queue-launcher.js", "static/update-triage-session.js", "static/update-review-handoff.js", "static/update-triage-launcher.js", "static/update-triage-gesture.js", "static/notification-undo.js", "static/today-timer.js", "static/today-recap.js", "static/today-rollover.js", "static/later-work.js", "static/drafts.js", "static/unfiled-captures.js", "static/draft-filing-session.js", "static/draft-capacity-dialog.js", "static/work-selection.js", "static/today-work.js", "static/pick-work.js", "static/batch-find-work.js", diff --git a/tests/test_service_worker.py b/tests/test_service_worker.py index 39aa748..19871c7 100644 --- a/tests/test_service_worker.py +++ b/tests/test_service_worker.py @@ -847,6 +847,7 @@ def test_install_precaches_complete_subpath_scoped_app_shell(): "/dashboard/static/mobile-work-entry.js", "/dashboard/static/mobile-queue-launcher.js", "/dashboard/static/update-triage-session.js", + "/dashboard/static/update-review-handoff.js", "/dashboard/static/update-read-position.js", "/dashboard/static/update-triage-launcher.js", "/dashboard/static/update-triage-gesture.js", diff --git a/tests/test_update_review_handoff.py b/tests/test_update_review_handoff.py new file mode 100644 index 0000000..7ce769d --- /dev/null +++ b/tests/test_update_review_handoff.py @@ -0,0 +1,96 @@ +import json +import subprocess +from pathlib import Path + +import pytest + +from tests.dashboard_bundle import dashboard + + +HANDOFF = Path(__file__).resolve().parents[1] / "frontend" / "update-review-handoff.js" + + +def run_handoff(script): + source = f"const createHandoff = require({json.dumps(str(HANDOFF))});\n" + script + result = subprocess.run(["node", "-e", source], capture_output=True, text=True) + assert result.returncode == 0, result.stderr + return json.loads(result.stdout) + + +def test_review_handoff_only_admits_review_requested_pull_updates(): + result = run_handoff(""" +const handoff = createHandoff({openReview:()=>{}, admitRead:async()=>{}, advance:()=>{}}); +const cases = [ + {is_review:true, update_reason:'Review requested', subject_type:'Pull', repository:'acme/app', number:7, notification_id:41}, + {is_review:true, update_reason:'Mentioned', subject_type:'Pull', repository:'acme/app', number:7, notification_id:42}, + {is_review:true, update_reason:'Review requested', subject_type:'Issue', repository:'acme/app', number:7, notification_id:43}, + {is_review:false, update_reason:'Review requested', subject_type:'Pull', repository:'acme/app', number:7, notification_id:44}, +]; +process.stdout.write(JSON.stringify(cases.map(item => handoff.eligible(item)))); +""") + + assert result == [True, False, False, False] + + +def test_review_handoff_preserves_update_on_cancel_and_advances_after_read_admission(): + result = run_handoff(""" +(async () => { + const events = []; + const update = {is_review:true, update_reason:'Review requested', subject_type:'Pull', repository:'acme/app', number:7, notification_id:41, title:'Ship it'}; + const handoff = createHandoff({ + openReview:item=>events.push(['open', item.repository, item.number]), + restoreUpdate:item=>events.push(['restore', item.notification_id]), + admitRead:async id=>events.push(['read', id]), + advance:()=>events.push(['advance']), + }); + handoff.begin(update); + handoff.cancel(); + handoff.begin(update); + const completed = await handoff.complete(); + process.stdout.write(JSON.stringify({events, completed, active:handoff.active()})); +})().catch(error => { console.error(error); process.exit(1); }); +""") + + assert result == { + "events": [ + ["open", "acme/app", 7], + ["restore", 41], + ["open", "acme/app", 7], + ["read", 41], + ["advance"], + ], + "completed": True, + "active": False, + } + + +def test_review_handoff_keeps_current_update_when_read_admission_fails(): + result = run_handoff(""" +(async () => { + const events = []; + const update = {is_review:true, update_reason:'Review requested', subject_type:'Pull', repository:'acme/app', number:7, notification_id:41}; + const handoff = createHandoff({ + openReview:()=>events.push('open'), + admitRead:async()=>{ events.push('read'); throw new Error('queue unavailable'); }, + advance:()=>events.push('advance'), + }); + handoff.begin(update); + let message = ''; + try { await handoff.complete(); } catch (error) { message = error.message; } + process.stdout.write(JSON.stringify({events, message, active:handoff.active()})); +})().catch(error => { console.error(error); process.exit(1); }); +""") + + assert result == {"events": ["open", "read"], "message": "queue unavailable", "active": True} + + +@pytest.mark.anyio +async def test_dashboard_wires_review_requested_update_into_existing_review_workspace(): + html = await dashboard() + + assert '' in html + assert 'id="review-update-now"' in html + assert "updateReviewHandoff.begin(selectedUpdate)" in html + assert "await updateReviewHandoff.complete()" in html + assert "updateReviewHandoff.cancel()" in html + assert "updateReviewHandoff.eligible(detail" in html -- 2.43.0