Merge pull request 'Complete review requests without leaving mobile Updates triage' (#776) from timmy/775-update-review-handoff into main

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); }

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,

frontend/index.html

@@ -814,6 +814,7 @@
     <div class="update-decision-bar" aria-label="Update decisions">
       <button id="keep-update-unread" type="button" aria-label="Keep unread and open next update">Keep unread</button>
       <button id="mark-update-read-next" type="button" aria-label="Mark read and open next update">Mark read</button>
+      <button id="review-update-now" type="button" hidden>Review now</button>
       <button id="focus-update-reply" type="button">Reply</button>
       <button id="toggle-update-more" type="button" aria-expanded="false">More</button>
     </div>
@@ -1088,6 +1089,7 @@
 <script src="static/mobile-work-entry.js"></script>
 <script src="static/mobile-queue-launcher.js"></script>
 <script src="static/update-triage-session.js"></script>
+<script src="static/update-review-handoff.js"></script>
 <script src="static/update-read-position.js"></script>
 <script src="static/update-triage-launcher.js"></script>
 <script src="static/update-triage-gesture.js"></script>

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',

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,
+  };
+});

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",

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",

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 '<script src="static/update-review-handoff.js"></script>' 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