From 437da72f3de554564689c40a7f670a9f3469a1d8 Mon Sep 17 00:00:00 2001 From: timmy Date: Mon, 17 Aug 2026 10:02:00 +0000 Subject: [PATCH] fix: keep delivery recovery single-flight (Closes #1016) --- frontend/dashboard.js | 7 ++ frontend/mobile-delivery-recovery.js | 44 +++++----- .../e2e/test_mobile_home_bootstrap_release.py | 60 ++++++++++++++ tests/test_mobile_delivery_recovery.py | 80 +++++++++++++++++++ 4 files changed, 172 insertions(+), 19 deletions(-) diff --git a/frontend/dashboard.js b/frontend/dashboard.js index 0ddecf8..4905a8b 100644 --- a/frontend/dashboard.js +++ b/frontend/dashboard.js @@ -90,10 +90,17 @@ qs('#my-work').scrollIntoView({block:'start'}); qs('#my-work').focus(); } + async function recoverMobileDelivery(item) { + if (!item?.outbox_id || !activeFlushLogin) throw new Error('Sign in again to retry.'); + const authored = item.kind === 'authored-outbox'; + const result = await (authored ? authoredOutbox : issueOutbox).retry(item.outbox_id, activeFlushLogin); + (authored ? applyAuthoredOutboxResult : applyOutboxResult)(result); + } let mobileQueueCounts = {}; const mobileDeliveryRecovery = createMobileDeliveryRecovery({ getItems: () => draftInbox.partition(lastDrafts).deliveries, getIndex: item => lastDrafts.indexOf(item), + activate: recoverMobileDelivery, beforeOpen: () => selectMobileQueue('draft'), onComplete: () => { if (!mobileStartDay.completePhase('delivery')) showMobileQueueCompletion('Delivery', true); diff --git a/frontend/mobile-delivery-recovery.js b/frontend/mobile-delivery-recovery.js index 059d1e9..4a29466 100644 --- a/frontend/mobile-delivery-recovery.js +++ b/frontend/mobile-delivery-recovery.js @@ -49,6 +49,7 @@ }; let currentId = ''; let active = false; + let inFlight = null; function currentItem(items) { return items.find(item => identity(item) === currentId) || items[0]; @@ -107,36 +108,41 @@ return 'opened'; } - async function defaultActivate(item) { + async function attend(item) { if (typeof document === 'undefined') return false; const index = options.getIndex ? options.getIndex(item) : -1; - const selector = item.status === 'authorization' ? '.draft-authorize' : - item.checklist_conflict ? '.draft-review-checklist' : item.quarantined ? '.draft-copy' : - item.status === 'attention' ? '.draft-resume' : '.draft-send'; + const selector = item.quarantined ? '.draft-copy' : + item.checklist_conflict ? '.draft-review-checklist' : '.draft-resume'; const action = document.querySelector('#my-work-list ' + selector + '[data-draft-index="' + index + '"]'); - if (!action) { - if (elements.status) elements.status.textContent = 'This delivery changed. Reopen recovery to load its current action.'; - return false; - } + if (!action) throw new Error('This delivery changed. Reopen recovery to load its current action.'); action.click(); - if (selector === '.draft-resume' || selector === '.draft-copy') elements.dialog.close(); - await Promise.resolve(); return true; } - async function activate() { + function activate() { + if (inFlight) return inFlight; const items = ordered(); const item = currentItem(items); - const perform = options.activate || defaultActivate; - if (!item) return render(); + const perform = state(item) === 'attention' ? attend : options.activate; + if (!item) return Promise.resolve(render()); if (elements?.action) elements.action.disabled = true; if (elements?.status) elements.status.textContent = 'Working…'; - try { - await perform(item, state(item)); - } finally { - if (elements?.action) elements.action.disabled = false; - } - return render(); + inFlight = (async () => { + let failure = null; + try { + await perform(item, state(item)); + } catch (error) { + failure = String(error?.message || 'Delivery could not be completed.').trim().slice(0, 160); + } finally { + if (elements?.action) elements.action.disabled = false; + } + const result = render(); + if (failure && elements?.status) { + elements.status.textContent = failure + ' Try again when the connection is ready.'; + } + return result; + })().finally(() => { inFlight = null; }); + return inFlight; } function start() { diff --git a/tests/e2e/test_mobile_home_bootstrap_release.py b/tests/e2e/test_mobile_home_bootstrap_release.py index ab94839..5c2fb62 100644 --- a/tests/e2e/test_mobile_home_bootstrap_release.py +++ b/tests/e2e/test_mobile_home_bootstrap_release.py @@ -140,3 +140,63 @@ def test_release_artifact_bootstraps_mobile_home_and_returns_from_insights( fake.shutdown() fake.server_close() fake_thread.join(timeout=5) + + +def test_release_artifact_keeps_mobile_delivery_recovery_single_flight(tmp_path: Path): + archives = sorted((ROOT / "dist").glob("stackchain-dashboard-*.tar.gz")) + assert len(archives) == 1, "browser job must download exactly one assembled release archive" + + fake = FakeGiteaServer(("127.0.0.1", 0)) + fake_thread = threading.Thread(target=fake.serve_forever, daemon=True) + fake_thread.start() + fake_url = f"http://127.0.0.1:{fake.server_port}" + browser_errors: list[str] = [] + + try: + with release_server(archives[0], tmp_path, fake_url) as origin, sync_playwright() as playwright: + browser = playwright.chromium.launch(args=["--ignore-certificate-errors"]) + page = browser.new_page(viewport={"width": 390, "height": 844}) + page.on("pageerror", lambda error: browser_errors.append(error.stack or str(error))) + page.goto(origin + "/", wait_until="networkidle") + page.locator('input[name="device_label"]').fill("Delivery recovery release phone") + page.locator('input[name="access_token"]').fill(ACCESS_TOKEN) + page.locator("#submit-sign-in").click() + page.wait_for_url(origin + "/", wait_until="networkidle") + + page.evaluate( + """ + () => { + window.__deliveryAttempts = 0; + window.__deliverySettled = new Promise(resolve => { window.__finishDelivery = resolve; }); + window.__releaseRecovery = createMobileDeliveryRecovery({ + getItems: () => [{outbox_id:'release-retry', delivery_state:'waiting', title:'Post release note'}], + activate: async () => { + window.__deliveryAttempts += 1; + await window.__deliverySettled; + }, + }); + window.__releaseRecovery.open(); + window.__firstRecovery = window.__releaseRecovery.activate(); + window.__secondRecovery = window.__releaseRecovery.activate(); + } + """ + ) + action = page.locator("#mobile-delivery-recovery-action") + expect(page.locator("#mobile-delivery-recovery")).to_be_visible() + expect(action).to_be_disabled() + expect(page.locator("#mobile-delivery-recovery-status")).to_have_text("Working…") + assert page.evaluate("window.__deliveryAttempts") == 1 + bounds = action.bounding_box() + assert bounds and bounds["height"] >= 44 + assert page.evaluate("document.documentElement.scrollWidth <= window.innerWidth") + + page.evaluate("window.__finishDelivery()") + page.evaluate("Promise.all([window.__firstRecovery, window.__secondRecovery])") + expect(action).to_be_enabled() + assert page.evaluate("window.__deliveryAttempts") == 1 + assert browser_errors == [] + browser.close() + finally: + fake.shutdown() + fake.server_close() + fake_thread.join(timeout=5) diff --git a/tests/test_mobile_delivery_recovery.py b/tests/test_mobile_delivery_recovery.py index 6f8db96..056b479 100644 --- a/tests/test_mobile_delivery_recovery.py +++ b/tests/test_mobile_delivery_recovery.py @@ -97,6 +97,83 @@ const createRecovery = require({json.dumps(str(RECOVERY))}); } +def test_recovery_keeps_one_attempt_locked_until_delivery_settles(): + script = f""" +const createRecovery = require({json.dumps(str(RECOVERY))}); +(async () => {{ + let finish; + let attempts = 0; + const settled = new Promise(resolve => {{finish = resolve}}); + const element = () => ({{textContent:'', disabled:false}}); + const elements = {{ + dialog: {{open:false, showModal(){{this.open=true}}, close(){{this.open=false}}}}, + title:element(), destination:element(), reason:element(), progress:element(), + action:element(), status:element(), + }}; + const recovery = createRecovery({{ + getItems: () => [{{outbox_id:'retry', delivery_state:'waiting', title:'Post comment'}}], + activate: async () => {{attempts += 1; await settled; return true}}, + elements, + }}); + recovery.open(); + const first = recovery.activate(); + const second = recovery.activate(); + await Promise.resolve(); + const pending = {{attempts, disabled:elements.action.disabled, status:elements.status.textContent}}; + finish(); + await Promise.all([first, second]); + process.stdout.write(JSON.stringify({{ + pending, attempts, disabled:elements.action.disabled, + }})); +}})(); +""" + + assert run_node(script) == { + "pending": {"attempts": 1, "disabled": True, "status": "Working…"}, + "attempts": 1, + "disabled": False, + } + + +def test_recovery_retains_failed_delivery_with_actionable_status(): + script = f""" +const createRecovery = require({json.dumps(str(RECOVERY))}); +(async () => {{ + const element = () => ({{textContent:'', disabled:false}}); + const elements = {{ + dialog: {{open:false, showModal(){{this.open=true}}, close(){{this.open=false}}}}, + title:element(), destination:element(), reason:element(), progress:element(), + action:element(), status:element(), + }}; + const recovery = createRecovery({{ + getItems: () => [{{outbox_id:'retry', delivery_state:'waiting', title:'Post comment'}}], + activate: async () => {{throw new Error('Network unavailable')}}, + elements, + }}); + recovery.open(); + const result = await recovery.activate(); + process.stdout.write(JSON.stringify({{ + result, disabled:elements.action.disabled, status:elements.status.textContent, + }})); +}})(); +""" + + assert run_node(script) == { + "result": { + "count": 1, + "current": { + "id": "retry", + "state": "waiting", + "primary": "Retry now", + "position": 1, + "total": 1, + }, + }, + "disabled": False, + "status": "Network unavailable Try again when the connection is ready.", + } + + def test_recovery_renders_delivery_context_and_hands_off_when_cleared(): script = f""" const createRecovery = require({json.dumps(str(RECOVERY))}); @@ -157,6 +234,9 @@ async def test_dashboard_packages_phone_safe_guided_delivery_recovery(): assert 'id="mobile-delivery-recovery-action"' in html assert '' in html assert "const mobileDeliveryRecovery = createMobileDeliveryRecovery({" in html + assert "activate: recoverMobileDelivery," in html + assert "await (authored ? authoredOutbox : issueOutbox).retry(item.outbox_id, activeFlushLogin)" in html + assert "(authored ? applyAuthoredOutboxResult : applyOutboxResult)(result)" in html assert "openDelivery: () => mobileDeliveryRecovery.open()" in html assert "mobileStartDay.completePhase('delivery')" in html assert "BASE + 'static/mobile-delivery-recovery.js'" in service_worker