From 765e982a945d816493f14f0b682e3ffa4beac0bd Mon Sep 17 00:00:00 2001 From: JuanMa Date: Sun, 9 Aug 2026 00:08:08 +0200 Subject: [PATCH 1/3] Say what a ticket switch is doing while it does it (#173) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A switch is a worktree scan and a full checkout — seconds on a real wordpress-develop, during which the window said nothing. Every other long operation in this app streams, so this was the one place where "it is taking a while" looked exactly like "it has hung", and the natural responses are clicking again or force-quitting. Force-quitting part-way through a checkout is what leaves the half-swapped worktree the mid-switch marker exists to recover from. The panel now names each stage as it happens. The park stages matter most and are entirely synthetic: statusMatrix reports nothing while it runs and is about a third of a switch, and it covers the stretch where the contributor's edits are not committed anywhere yet — "Saving your work on #59234" is the sentence that stops someone quitting there. Measured before designing: a checkout of 1500 files calls back ~4400 times in 143ms, so the events are throttled to one per stage change plus one per 100ms. The rule that earns its keep is flushing whatever was suppressed when the stage turns over, so a line can never stop at 87% and jump to done — a frozen line reads as a hang, which is the failure this exists to prevent. Progress is additive: the handlers still await and return exactly as before. Restructuring into an invoke-then-done pair would have meant subscribing after the invoke answered, and the first event lands about 4ms in, so the start of every switch would have been lost. The subscription lives in App rather than SiteRow because every row stays mounted — one per row would open a listener per registered site. The trunk update feeds its own log instead of this channel: one operation with two progress surfaces is how the two end up disagreeing. Co-Authored-By: Claude Fable 5 --- src/main.js | 83 +++++++++- src/preload.js | 11 ++ src/renderer/index.jsx | 39 ++++- src/switch-progress.cjs | 183 +++++++++++++++++++++ src/ticket-branches.js | 79 ++++++--- test/ipc-wiring.test.cjs | 138 ++++++++++++++++ test/preload-listeners.test.cjs | 65 ++++++++ test/switch-progress.test.cjs | 189 ++++++++++++++++++++++ test/ticket-branches.integration.test.cjs | 97 +++++++++++ 9 files changed, 855 insertions(+), 29 deletions(-) create mode 100644 src/switch-progress.cjs create mode 100644 test/switch-progress.test.cjs diff --git a/src/main.js b/src/main.js index 41679b7..4b9d8be 100644 --- a/src/main.js +++ b/src/main.js @@ -45,7 +45,14 @@ const { switchToBranch, deleteTicketBranch } = require('./ticket-branches'); +const { createProgressThrottle, describeSwitchProgress } = require('./switch-progress.cjs'); const { getStore } = require('./settings-store'); + +// One name for the send-only progress channel (#173), shared with preload.js +// through the tests rather than by import — the renderer bundle and the main +// process do not share a module graph, and a rename that only lands on one side +// unsubscribes the panel silently. +const SWITCH_PROGRESS_CHANNEL = 'switch:progress'; const { parseTicketRef } = require('./renderer/trac-ticket.cjs'); const { parseHandle } = require('./wporg-handle.cjs'); const { parseEventName, buildProvenanceHeader, handoffFilename } = require('./patch-provenance.cjs'); @@ -701,6 +708,48 @@ async function withSwitchMarker(sitePath, run) { } } +/** + * Switch progress as terminal lines, for the trunk update (#173). + * + * `intervalMs: Infinity` suppresses everything except a change of stage and the + * final flush, which turns a switch into about five lines instead of fifteen — + * the same module the panel uses, in the mode an append-only log wants. + * + * @param {Function} sendLog Writes one chunk to the update's log stream. + * @return {{emit: Function, flush: Function}} Pass `emit` as `onProgress`. + */ +function updateSwitchLogger(sendLog) { + return createProgressThrottle({ + intervalMs: Infinity, + onEmit: (payload) => sendLog(` ${describeSwitchProgress(payload)}\n`) + }); +} + +/** + * Streams a switch's progress to the window that asked for it (#173). + * + * Additive on purpose: the handler still awaits and returns its result exactly + * as before, and this only sends alongside. Restructuring into an + * invoke-then-done pair would have meant the renderer subscribing after the + * invoke answers — and the first checkout event lands about 4ms in, so the + * beginning of every switch would be lost. + * + * Sends are wrapped because the window can be gone by the time a multi-second + * checkout finishes, and a closed window must not turn a completed switch into + * a failure. + * + * @param {Object} event The IPC event, for its sender. + * @param {string} sitePath Which site the progress belongs to. + * @return {{emit: Function, flush: Function}} Pass `emit` as `onProgress`. + */ +function switchProgressReporter(event, sitePath) { + return createProgressThrottle({ + onEmit: (payload) => { + try { event.sender.send(SWITCH_PROGRESS_CHANNEL, { sitePath, ...payload }); } catch {} + } + }); +} + /** * Where the state that describes the *work* lives: per branch once a ticket is * being worked on, at site level on trunk and for sites that predate #108. One @@ -834,9 +883,17 @@ ipcMain.handle('git:update-trunk', async (event, sitePath) => { ticketBefore = branchBefore === TRUNK ? null : ticketIdFromRef(branchBefore); if (branchBefore !== TRUNK) { sendLog(`Parking your work on ${branchBefore} before updating…\n`); + // The same progress the ticket panel shows (#173), but into the + // terminal this flow already streams to rather than onto the + // switch channel: one operation with two progress surfaces is + // how the two end up disagreeing. `Infinity` keeps it to one + // line per stage, since this log is append-only. + const parkLog = updateSwitchLogger(sendLog); await withSwitchMarker(sitePath, () => switchToBranch(sitePath, TRUNK, { - baseOid: branchMetaBefore && branchMetaBefore.baseOid + baseOid: branchMetaBefore && branchMetaBefore.baseOid, + onProgress: parkLog.emit })); + parkLog.flush(); await mergeSiteMeta(sitePath, { currentBranch: TRUNK }); } @@ -868,7 +925,9 @@ ipcMain.handle('git:update-trunk', async (event, sitePath) => { // the app never silently rebases anyone. if (ticketBefore !== null) { sendLog(`\nReturning to your work on ${branchBefore}…\n`); - await withSwitchMarker(sitePath, () => switchToBranch(sitePath, branchBefore, {})); + const returnLog = updateSwitchLogger(sendLog); + await withSwitchMarker(sitePath, () => switchToBranch(sitePath, branchBefore, { onProgress: returnLog.emit })); + returnLog.flush(); await mergeSiteMeta(sitePath, { currentBranch: branchBefore, tracTicket: ticketBefore }); } sendDone({ ok: true, ...result, branch: branchBefore }); @@ -1383,7 +1442,7 @@ async function withRegisteredSite(sitePath, run) { // The site-level `tracTicket` is kept in step with the active branch so the // handlers that read it (`git:list-ticket-patches`, `trac:list-attachments`, // `site:status`) need no change. -ipcMain.handle('sites:set-ticket', async (_e, sitePath, ref) => withRegisteredSite(sitePath, async () => { +ipcMain.handle('sites:set-ticket', async (event, sitePath, ref) => withRegisteredSite(sitePath, async () => { // Empty means unlink — the panel's Unlink button and a cleared field both // land here, and neither is an error. The branch and its work stay; going // back to trunk is not the same as throwing a ticket away. @@ -1391,7 +1450,9 @@ ipcMain.handle('sites:set-ticket', async (_e, sitePath, ref) => withRegisteredSi if (!raw) { const { ref: current, meta } = await activeBranch(sitePath, { migrate: true }); if (current !== TRUNK) { - await withSwitchMarker(sitePath, () => switchToBranch(sitePath, TRUNK, { baseOid: meta && meta.baseOid })); + const progress = switchProgressReporter(event, sitePath); + await withSwitchMarker(sitePath, () => switchToBranch(sitePath, TRUNK, { baseOid: meta && meta.baseOid, onProgress: progress.emit })); + progress.flush(); } await mergeSiteMeta(sitePath, { tracTicket: null, currentBranch: TRUNK }); return { ok: true, ticket: null, branch: TRUNK }; @@ -1412,14 +1473,18 @@ ipcMain.handle('sites:set-ticket', async (_e, sitePath, ref) => withRegisteredSi const known = await listTicketBranches(sitePath); let baseOid; if (known.includes(branchRef)) { - await withSwitchMarker(sitePath, () => switchToBranch(sitePath, branchRef, { baseOid: meta && meta.baseOid })); + const progress = switchProgressReporter(event, sitePath); + await withSwitchMarker(sitePath, () => switchToBranch(sitePath, branchRef, { baseOid: meta && meta.baseOid, onProgress: progress.emit })); + progress.flush(); baseOid = ((await readSiteMeta(sitePath)).branches || {})[branchRef]?.baseOid || null; } else { // Starting a ticket from another ticket parks that one first; from trunk // the loose edits ride along into the new branch (that is deliberate — // "I started editing, then realised which ticket this is"). if (current !== TRUNK) { - await withSwitchMarker(sitePath, () => switchToBranch(sitePath, TRUNK, { baseOid: meta && meta.baseOid })); + const progress = switchProgressReporter(event, sitePath); + await withSwitchMarker(sitePath, () => switchToBranch(sitePath, TRUNK, { baseOid: meta && meta.baseOid, onProgress: progress.emit })); + progress.flush(); } ({ baseOid } = await startTicketBranch(sitePath, parsed.id)); } @@ -1449,11 +1514,13 @@ ipcMain.handle('branches:list', async (_e, sitePath) => withRegisteredSite(siteP return { ok: true, current, branches }; })); -ipcMain.handle('branches:switch', async (_e, sitePath, targetRef) => withRegisteredSite(sitePath, async () => { +ipcMain.handle('branches:switch', async (event, sitePath, targetRef) => withRegisteredSite(sitePath, async () => { const blocked = await midSwitchBlock(sitePath); if (blocked) return blocked; const { ref: current, meta } = await activeBranch(sitePath, { migrate: true }); - const result = await withSwitchMarker(sitePath, () => switchToBranch(sitePath, targetRef, { baseOid: meta && meta.baseOid })); + const progress = switchProgressReporter(event, sitePath); + const result = await withSwitchMarker(sitePath, () => switchToBranch(sitePath, targetRef, { baseOid: meta && meta.baseOid, onProgress: progress.emit })); + progress.flush(); const ticketId = ticketIdFromRef(targetRef); if (targetRef !== TRUNK) { await mergeBranchMeta(sitePath, targetRef, { lastUsedAt: new Date().toISOString() }); diff --git a/src/preload.js b/src/preload.js index b9ce768..5976ec7 100644 --- a/src/preload.js +++ b/src/preload.js @@ -83,6 +83,17 @@ contextBridge.exposeInMainWorld('api', { switchBranch: (sitePath, ref) => ipcRenderer.invoke('branches:switch', sitePath, ref) , deleteBranch: (sitePath, ref) => ipcRenderer.invoke('branches:delete', sitePath, ref) +, + // A long-lived subscription rather than the per-run pair the installs use + // (#173): a switch's first progress event lands a few milliseconds in, well + // before its `invoke` answers, and a listener attached afterwards would miss + // the start of every switch. Payloads carry `sitePath`, so one subscription + // serves every site. + subscribeSwitchProgress: (handler) => { + const h = (_e, payload) => handler && handler(payload); + ipcRenderer.on('switch:progress', h); + return () => ipcRenderer.removeListener('switch:progress', h); + } , subscribeSetupProgress: (handler) => { const h = (_e, payload) => handler && handler(payload); diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index 4cb54a4..efcfd59 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -24,6 +24,7 @@ import { pickLatest } from '../latest-patch.cjs'; import { parsePrRef } from '../patch-sources.cjs'; import { ticketUrl, attachUrl } from './trac-ticket.cjs'; import { ticketBranchRows } from './ticket-branch-list.cjs'; +import { describeSwitchProgress } from '../switch-progress.cjs'; import { highlightDiff, hasDiffLines } from './diff-highlight.cjs'; const TERMINAL_ALLOWED_SCRIPTS = ['build', 'build:dev', 'dev', 'test', 'watch', 'grunt']; @@ -227,6 +228,10 @@ function App() { const [createSubmitting, setCreateSubmitting] = useState(false); const [setupLogsBySite, setSetupLogsBySite] = useState({}); const setupLogAliasRef = useRef({}); + // Where a ticket switch has got to, per site (#173). Held here rather than in + // SiteRow because every row stays mounted — subscribing per row would open one + // listener per registered site and wake all of them for each other's events. + const [switchProgressBySite, setSwitchProgressBySite] = useState({}); const appendSetupLog = useCallback((siteTarget, message) => { const key = siteTarget ? String(siteTarget) : ''; @@ -331,6 +336,21 @@ function App() { return () => { if (unsubProg) unsubProg(); if (unsubStat) unsubStat(); }; }, [addPendingSite, appendSetupLog, clearPendingSites]); + // Dropped when a switch begins, so a failed switch's last sentence is not the + // next one's first frame. + const clearSwitchProgress = useCallback((sitePath) => { + setSwitchProgressBySite((prev) => (prev[sitePath] ? { ...prev, [sitePath]: null } : prev)); + }, []); + + // One subscription for every site; the payload says which one (#173). + useEffect(() => { + const unsub = window.api.subscribeSwitchProgress((p) => { + if (!p || !p.sitePath) return; + setSwitchProgressBySite((prev) => ({ ...prev, [p.sitePath]: p.stage === 'done' ? null : p })); + }); + return () => { if (unsub) unsub(); }; + }, []); + const chooseAndSetup = useCallback(() => { setCreateSiteName(''); setCreateSiteDir(''); @@ -803,6 +823,8 @@ function App() { wporg={wporg} isPending={pendingSites.includes(s)} setupLogs={setupLogsBySite[s] || ''} + switchProgress={switchProgressBySite[s] || null} + onClearSwitchProgress={clearSwitchProgress} isActive={activeSite === s} /> @@ -940,7 +962,7 @@ function DestinationCard({ title, cost, after, children }) { ); } -function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSiteMetaPatch, onForget, onDelete, onRename, editor, wporg, isPending = false, setupLogs = '', isActive = false }) { +function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSiteMetaPatch, onForget, onDelete, onRename, editor, wporg, isPending = false, setupLogs = '', isActive = false, switchProgress = null, onClearSwitchProgress = null }) { // Kept in a ref so loadStatus's dependency list stays [sitePath] — a // recreated callback prop must not retrigger the status-loading effect. const metaPatchRef = useRef(onSiteMetaPatch); @@ -1263,6 +1285,8 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit setTicketSaving(true); setTicketError(''); setBlockedByTrunkWork(null); + // The previous switch's last sentence must not be this one's first frame. + if (onClearSwitchProgress) onClearSwitchProgress(sitePath); try { const res = await window.api.setSiteTicket(sitePath, ref); if (!res?.ok) { @@ -1287,7 +1311,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit } finally { setTicketSaving(false); } - }, [sitePath, loadBranches, loadStatus]); + }, [sitePath, loadBranches, loadStatus, onClearSwitchProgress]); const linkTicket = useCallback(() => saveTicket(ticketInput), [saveTicket, ticketInput]); const unlinkTicket = useCallback(() => saveTicket(''), [saveTicket]); @@ -1879,6 +1903,15 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // operations as well as on each other — the same trio every destructive // control in this panel guards on. const branchRows = ticketBranchRows({ branches: ticketBranches.branches, current: ticketBranches.current, tracTicket, now: Date.now() }); + // What the switch is doing, while it does it (#173). Gated on the busy flag + // rather than merely cleared by it: the last sends can land after the invoke + // has already answered, which would flash a sentence under an idle panel. + const switchProgressLine = ticketSaving && switchProgress ? ( +
+ + {describeSwitchProgress(switchProgress)} +
+ ) : null; const ticketActionsBlocked = ticketSaving || deletingBranch !== null || updateState !== 'idle' || installing || building; const renderBranchRows = (linked) => (
@@ -3021,6 +3054,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit {ticketError ? (
{ticketError}
) : null} + {switchProgressLine} {blockedByTrunkWork ? (
@@ -3405,6 +3439,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit style={{ justifyContent:'center' }} >Link ticket {ticketError ?
{ticketError}
: null} + {switchProgressLine} )} diff --git a/src/switch-progress.cjs b/src/switch-progress.cjs new file mode 100644 index 0000000..a4b238e --- /dev/null +++ b/src/switch-progress.cjs @@ -0,0 +1,183 @@ +'use strict'; + +/** + * What a ticket switch is doing while it does it (issue #173). + * + * Switching tickets is a worktree scan and a full checkout — seconds of silence + * on a real `wordpress-develop`, during which the window looks hung. The + * natural responses are clicking again or force-quitting, and force-quitting + * part-way through a checkout leaves the half-swapped worktree that + * `withSwitchMarker` exists to recover from. This module is the vocabulary for + * saying what is happening instead. + * + * Two jobs, kept together because they are two ends of one contract: the + * throttle that decides which events are worth sending, and the sentence the + * panel shows for one. Pure and dependency-free, so `node --test` drives it + * directly while both the main process and the renderer bundle require it. + */ + +// A checkout of ~1500 files calls back about 4400 times in 143ms. At 100ms +// between frames a switch produces a couple of dozen sends — enough for the +// line to move, few enough that the IPC channel stays a channel. +const DEFAULT_INTERVAL_MS = 100; + +// isomorphic-git's own phase strings, which belong to it and not to us. Pinned +// here so a version bump breaks one lookup rather than leaking a foreign +// vocabulary into the UI. +const CHECKOUT_PHASES = { + 'Analyzing workdir': 'analyze', + 'Updating workdir': 'apply' +}; + +/** + * Coalesces a flood of progress events down to what is worth sending. + * + * A stage change always goes out immediately — the stage *is* the sentence on + * screen, and making it wait for the interval is what produces a line that + * describes the previous thing. Within a stage, events are held to one per + * interval. + * + * The rule that matters most is the one that keeps a line from stopping at 87% + * and jumping to done: whatever was suppressed last is flushed when the stage + * changes, and again by `flush()` at the end. A progress line that freezes is + * read as a hang, which is the exact failure this is meant to prevent. + * + * `emit` is deliberately synchronous and returns nothing: isomorphic-git awaits + * whatever `onProgress` returns, so a promise here would add a microtask + * between every one of those 4400 events. + * + * @param {Object} options + * @param {Function} options.onEmit Called with each payload that survives. + * @param {number} [options.intervalMs] Minimum gap within a stage. `Infinity` + * reduces a switch to one event per + * stage, for an append-only log. + * @param {Function} [options.now] Clock, injected so tests need no timers. + * @return {{emit: Function, flush: Function}} The emitter and its final flush. + */ +function createProgressThrottle({ onEmit, intervalMs = DEFAULT_INTERVAL_MS, now = Date.now } = {}) { + let lastStage = null; + let lastAt = -Infinity; + let pending = null; + + const send = (payload) => { + pending = null; + lastStage = payload.stage; + lastAt = now(); + if (onEmit) onEmit(payload); + }; + + return { + emit(payload) { + if (!payload) return; + if (payload.stage !== lastStage) { + if (pending) send(pending); + send(payload); + return; + } + if (now() - lastAt >= intervalMs) { + send(payload); + return; + } + pending = payload; + }, + flush() { + if (pending) send(pending); + } + }; +} + +/** + * One of isomorphic-git's checkout progress events, in this app's vocabulary. + * + * `Analyzing workdir` reports a running count with no total — there is no + * honest percentage for that half, and the sentence for it says so rather than + * inventing one. + * + * @param {{phase: string, loaded: number, total: number}} event + * @return {{stage: string, loaded: number, total: ?number}} Our shape. + */ +function mapCheckoutPhase(event = {}) { + return { + stage: CHECKOUT_PHASES[event.phase] || 'apply', + loaded: event.loaded, + total: event.total + }; +} + +/** + * A ticket number from a branch ref, or null for trunk and anything else. + * + * @param {?string} ref + */ +function ticketOf(ref) { + const match = /^ticket\/(\d+)$/.exec(String(ref || '')); + return match ? match[1] : null; +} + +/** + * What the panel says for one progress event. + * + * The parking sentences name the ticket being left, which is the point of the + * whole feature: they are what stops someone force-quitting during the seconds + * when their edits are not committed anywhere yet. Branch refs never reach the + * screen — a contributor knows `#59234`, not `ticket/59234`. + * + * @param {Object} progress + * @param {string} progress.stage + * @param {number} [progress.loaded] + * @param {number} [progress.total] + * @param {?string} [progress.from] Branch being left. + * @param {?string} [progress.to] Branch being entered. + * @return {string} A sentence, never empty, for any stage including a new one. + */ +function describeSwitchProgress({ stage, loaded, total, from, to } = {}) { + const saving = () => { + const leaving = ticketOf(from); + return leaving ? `your work on #${leaving}` : 'your work'; + }; + const entering = () => { + const id = ticketOf(to); + return id ? ` for #${id}` : ''; + }; + + switch (stage) { + case 'scan': + return `Saving ${saving()}…`; + case 'stage': + return `Saving ${saving()}… ${withCount(loaded, total)}`; + case 'commit': + return `Saving ${saving()}…`; + case 'analyze': + return 'Checking which files change…'; + case 'apply': + return `Swapping files${entering()}… ${withCount(loaded, total)}`; + case 'done': + return ticketOf(to) ? `Ready to work on #${ticketOf(to)}` : 'Ready'; + default: + // A stage this version does not know — a newer isomorphic-git, or a + // caller ahead of this module. Saying something true and vague beats + // rendering nothing where a sentence was. + return 'Working…'; + } +} + +/** + * The trailing "42%" or "1,200 files", or nothing when neither is knowable. + * + * @param {?number} loaded + * @param {?number} total + */ +function withCount(loaded, total) { + if (Number.isFinite(total) && total > 0 && Number.isFinite(loaded)) { + return `${Math.min(100, Math.round((loaded / total) * 100))}%`; + } + if (Number.isFinite(loaded)) return `${loaded.toLocaleString()} files`; + return ''; +} + +module.exports = { + DEFAULT_INTERVAL_MS, + createProgressThrottle, + mapCheckoutPhase, + describeSwitchProgress +}; diff --git a/src/ticket-branches.js b/src/ticket-branches.js index cd066f9..7af76e5 100644 --- a/src/ticket-branches.js +++ b/src/ticket-branches.js @@ -29,6 +29,7 @@ const fs = require('fs'); const git = require('isomorphic-git'); const { ensureAutocrlf } = require('./trunk-update.js'); +const { mapCheckoutPhase } = require('./switch-progress.cjs'); /** The pristine snapshot branch. Never committed to, never deleted. */ const TRUNK = 'trunk'; @@ -100,24 +101,28 @@ async function listTicketBranches(dir) { * `statusMatrix` excludes gitignored paths by default, so `node_modules` and * `build/` never enter the index no matter how large they have grown. * - * @param {string} dir - * @param {Array} [matrix] a statusMatrix the caller already computed + * @param {string} dir + * @param {Array} [matrix] a statusMatrix the caller already computed + * @param {Function} [onProgress] told how far the staging has got (#173) */ -async function stageWorktree(dir, matrix = null) { +async function stageWorktree(dir, matrix = null, onProgress = null) { // The caller has usually just scanned the worktree to decide whether there // was anything to park. On wordpress-develop that scan hashes thousands of // files on the main process's event loop, so it is passed in and reused // rather than repeated. if (!matrix) matrix = await git.statusMatrix({ fs, dir }); + // The one stage of a park with a real total, and it comes free: the rows + // worth staging are known before any of them is written. + const pending = matrix.filter(([, head, workdir, stage]) => !(head === workdir && workdir === stage)); let staged = 0; - for (const [filepath, head, workdir, stage] of matrix) { - if (head === workdir && workdir === stage) continue; + for (const [filepath, , workdir] of pending) { if (workdir === 0) { // Gone from disk: drop it from the index, and from the next commit. try { await git.remove({ fs, dir, filepath }); staged += 1; } catch {} } else { try { await git.add({ fs, dir, filepath }); staged += 1; } catch {} } + if (onProgress) onProgress({ stage: 'stage', loaded: staged, total: pending.length }); } return staged; } @@ -157,12 +162,13 @@ async function scanWorktree(dir, ref = 'HEAD') { * work that starts there is carried into a branch by `startTicketBranch`, not * committed where every other branch's diff base lives. * - * @param {string} dir - * @param {Object} root0 - * @param {string} [root0.baseOid] branch point; resolved from `trunk` when absent - * @param {Object} [root0.author] + * @param {string} dir + * @param {Object} root0 + * @param {string} [root0.baseOid] branch point; resolved from `trunk` when absent + * @param {Object} [root0.author] + * @param {Function} [root0.onProgress] told which stage of the park is running (#173) */ -async function parkCurrentWork(dir, { baseOid, author = WIP_AUTHOR } = {}) { +async function parkCurrentWork(dir, { baseOid, author = WIP_AUTHOR, onProgress = null } = {}) { await ensureAutocrlf(dir); const branch = await currentBranchName(dir); if (!branch || branch === TRUNK) { @@ -171,6 +177,11 @@ async function parkCurrentWork(dir, { baseOid, author = WIP_AUTHOR } = {}) { throw error; } + // Announced before the scan rather than after it: `statusMatrix` reports + // nothing while it runs and is about a third of a switch, so this is the + // stretch that would otherwise be silent — and the stretch during which the + // contributor's edits are not committed anywhere yet. + if (onProgress) onProgress({ stage: 'scan', from: branch }); const { matrix, changed } = await scanWorktree(dir); if (!changed) return { parked: false, branch, oid: null }; @@ -178,7 +189,8 @@ async function parkCurrentWork(dir, { baseOid, author = WIP_AUTHOR } = {}) { // wiped by hand, a site adopted from disk) still parks against something // sane instead of throwing. const parent = baseOid || await git.resolveRef({ fs, dir, ref: TRUNK }); - await stageWorktree(dir, matrix); + await stageWorktree(dir, matrix, onProgress); + if (onProgress) onProgress({ stage: 'commit', from: branch }); const oid = await git.commit({ fs, dir, message: WIP_MESSAGE, author, parent: [parent] }); return { parked: true, branch, oid }; } @@ -220,13 +232,23 @@ async function startTicketBranch(dir, ticketId) { * honest options — start a ticket for the work, or discard it. Silently * destroying edits is the one outcome this feature must never produce. * - * @param {string} dir - * @param {string} ref - * @param {Object} [root0] - * @param {string} [root0.baseOid] branch point of the branch being left - * @param {Object} [root0.author] + * Progress (#173) is reported through `onProgress` in this module's own + * vocabulary — `scan`, `stage`, `commit`, then the checkout's own phases mapped + * by switch-progress.cjs. The scans are announced before they start, because + * `statusMatrix` says nothing while it runs and is roughly a third of a switch. + * + * `deleteTicketBranch` checks out too and is deliberately left silent: it runs + * under a different busy flag in the panel, so covering it would mean a second + * progress surface for a rarely-used destructive action. + * + * @param {string} dir + * @param {string} ref + * @param {Object} [root0] + * @param {string} [root0.baseOid] branch point of the branch being left + * @param {Object} [root0.author] + * @param {Function} [root0.onProgress] told which stage is running (#173) */ -async function switchToBranch(dir, ref, { baseOid, author = WIP_AUTHOR } = {}) { +async function switchToBranch(dir, ref, { baseOid, author = WIP_AUTHOR, onProgress = null } = {}) { await ensureAutocrlf(dir); const from = await currentBranchName(dir); if (from === ref) return { switched: false, from, to: ref, parked: false }; @@ -238,15 +260,23 @@ async function switchToBranch(dir, ref, { baseOid, author = WIP_AUTHOR } = {}) { throw error; } + // Every payload carries where the switch is going, so the panel can name the + // destination without tracking it separately; the park stages add where it + // came from, which is the ticket whose work is being saved. + const report = onProgress ? (p) => onProgress({ to: ref, ...p }) : null; + let parked = false; if (from === TRUNK) { + // A full scan that usually ends in "nothing to do" and occasionally in a + // refusal — silent either way without this. + if (report) report({ stage: 'scan', from }); if (await hasChangesAgainst(dir)) { const error = new Error('Uncommitted work on trunk would be lost by switching'); error.code = 'dirty-trunk'; throw error; } } else if (from) { - ({ parked } = await parkCurrentWork(dir, { baseOid, author })); + ({ parked } = await parkCurrentWork(dir, { baseOid, author, onProgress: report })); } // Tagged with the stage it died in, the same contract updateToLatestTrunk @@ -260,7 +290,17 @@ async function switchToBranch(dir, ref, { baseOid, author = WIP_AUTHOR } = {}) { // parking rewrites rather than appends, put the real work out of reach. The // caller has to record that and refuse to park until it is reconciled. try { - await git.checkout({ fs, dir, ref, force: true }); + // No `nonBlocking`/`batchSize`: measured, the progress events already + // arrive spread across the whole checkout without them, and yielding to + // the event loop between batches would only widen the window in which the + // worktree is half-swapped — the state described above. + await git.checkout({ + fs, + dir, + ref, + force: true, + ...(report ? { onProgress: (p) => report(mapCheckoutPhase(p)) } : {}) + }); } catch (e) { if (e && typeof e === 'object') { e.stage = 'checkout'; @@ -269,6 +309,7 @@ async function switchToBranch(dir, ref, { baseOid, author = WIP_AUTHOR } = {}) { } throw e; } + if (report) report({ stage: 'done', from }); return { switched: true, from, to: ref, parked }; } diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index 212962a..0d9e715 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -1744,6 +1744,144 @@ test('branches:list reports the branches on disk with their stored context', asy assert.equal(result.branches[1].baseOid, null, 'a branch the registry has never seen still lists'); }); +// The same wait, for the trunk update's own :done channel. +async function updateDone(event, updateId, cap = 50) { + for (let i = 0; i < cap; i++) { + const hit = event.sent.find((m) => m.channel === 'git:update-trunk:done' && m.payload.updateId === updateId); + if (hit) return hit.payload; + await new Promise((r) => setImmediate(r)); + } + throw new Error('git:update-trunk never reported done'); +} + +// --- switch progress -> src/switch-progress.cjs (#173) --------------------- + +// The channel name, in one place. It is a send-only channel, so the +// classification guard below cannot see it and a rename would silently +// unsubscribe the panel instead of failing anything. +const SWITCH_PROGRESS_CHANNEL = 'switch:progress'; + +// A switch is a worktree scan and a full checkout — seconds during which the +// window said nothing and looked hung. The handler still returns its result the +// way it always did; the progress rides alongside, so nothing about the call +// shape changed. +test('sites:set-ticket streams switch progress for the site it is switching (issue #173)', async () => { + const switchToBranch = spy(async (dir, ref, options) => { + options.onProgress({ stage: 'scan', from: 'ticket/59234', to: ref }); + options.onProgress({ stage: 'done', from: 'ticket/59234', to: ref }); + return { switched: true, parked: true }; + }); + const currentBranchName = spy(async () => 'ticket/59234'); + const listTicketBranches = spy(async () => ['ticket/61002']); + const settings = fakeSettingsStore({ + sites: ['/sites/wp'], + siteMeta: { '/sites/wp': { branches: { 'ticket/59234': { baseOid: 'abc' } }, currentBranch: 'ticket/59234' } } + }); + const main = loadMain({ + stubs: { ...silentLogging(), ...settings.stubs, './ticket-branches': { switchToBranch, currentBranchName, listTicketBranches } } + }); + + const event = createIpcEvent(); + const result = await main.invokeWith('sites:set-ticket', event, '/sites/wp', '61002'); + + assert.equal(result.ok, true, 'progress is additive — the answer is unchanged'); + const progress = event.sent.filter((m) => m.channel === SWITCH_PROGRESS_CHANNEL); + assert.deepEqual(progress.map((m) => m.payload.stage), ['scan', 'done']); + assert.equal( + progress.every((m) => m.payload.sitePath === '/sites/wp'), + true, + 'the renderer keeps one subscription for every site, so each frame has to say which one it is' + ); +}); + +// A multi-second checkout easily outlives the window that asked for it. The +// switch itself completed; reporting that it failed because nobody was left to +// tell would be a lie with a mid-switch marker attached. +test('a window closed mid-switch does not turn a finished switch into a failure (issue #173)', async () => { + const switchToBranch = spy(async (dir, ref, options) => { + options.onProgress({ stage: 'scan', to: ref }); + return { switched: true, parked: false }; + }); + const currentBranchName = spy(async () => 'ticket/59234'); + const settings = fakeSettingsStore({ + sites: ['/sites/wp'], + siteMeta: { '/sites/wp': { branches: { 'ticket/59234': { baseOid: 'abc' } }, currentBranch: 'ticket/59234' } } + }); + const main = loadMain({ + stubs: { ...silentLogging(), ...settings.stubs, './ticket-branches': { switchToBranch, currentBranchName } } + }); + + const event = createIpcEvent(); + event.sender.send = () => { throw new Error('Object has been destroyed'); }; + + const result = await main.invokeWith('sites:set-ticket', event, '/sites/wp', ''); + + assert.equal(result.ok, true); + assert.equal(result.ticket, null); +}); + +// No renderer calls this door yet. It streams anyway: the day the switcher is +// wired to it, silence would arrive with nothing failing — the guard below +// cannot see a send-only channel. +test('branches:switch streams the same progress as sites:set-ticket (issue #173)', async () => { + const switchToBranch = spy(async (dir, ref, options) => { + options.onProgress({ stage: 'apply', loaded: 1, total: 2, to: ref }); + return { switched: true, parked: false }; + }); + const currentBranchName = spy(async () => 'ticket/59234'); + const settings = fakeSettingsStore({ + sites: ['/sites/wp'], + siteMeta: { '/sites/wp': { branches: { 'ticket/59234': { baseOid: 'abc' } }, currentBranch: 'ticket/59234' } } + }); + const main = loadMain({ + stubs: { ...silentLogging(), ...settings.stubs, './ticket-branches': { switchToBranch, currentBranchName } } + }); + + const event = createIpcEvent(); + await main.invokeWith('branches:switch', event, '/sites/wp', 'ticket/61002'); + + const progress = event.sent.filter((m) => m.channel === SWITCH_PROGRESS_CHANNEL); + assert.deepEqual(progress.map((m) => m.payload.stage), ['apply']); + assert.equal(progress[0].payload.sitePath, '/sites/wp'); +}); + +// The trunk update parks and returns, and it already owns a log stream the +// renderer is watching. Sending its switch stages to the panel's channel as +// well would give one operation two progress surfaces, which is how the two end +// up disagreeing. +test('the trunk update reports its switches in its own log, not on the switch channel (issue #173)', async () => { + const switchToBranch = spy(async (dir, ref, options) => { + if (options && options.onProgress) options.onProgress({ stage: 'scan', from: 'ticket/59234', to: ref }); + return { switched: true, parked: true }; + }); + const currentBranchName = spy(async () => 'ticket/59234'); + const updateToLatestTrunk = spy(async () => { throw new Error('stop after the park'); }); + const settings = fakeSettingsStore({ + sites: ['/sites/wp'], + siteMeta: { '/sites/wp': { branches: { 'ticket/59234': { baseOid: 'abc' } }, currentBranch: 'ticket/59234' } } + }); + const main = loadMain({ + stubs: { + ...silentLogging(), + ...settings.stubs, + './ticket-branches': { switchToBranch, currentBranchName }, + './trunk-update': { updateToLatestTrunk, ensureAutocrlf: async () => {}, readTrunkInfo: async () => ({}) } + } + }); + + const event = createIpcEvent(); + const { updateId } = await main.invokeWith('git:update-trunk', event, '/sites/wp'); + await updateDone(event, updateId); + + const logs = event.sent.filter((m) => m.channel === 'git:update-trunk:log').map((m) => m.payload.data).join(''); + assert.match(logs, /Saving your work on #59234/, 'the stage reaches the terminal it belongs to'); + assert.deepEqual( + event.sent.filter((m) => m.channel === SWITCH_PROGRESS_CHANNEL), + [], + 'and not the panel channel, which would be a second surface for one operation' + ); +}); + test('branches:switch delegates to ticket-branches and records the new active branch', async () => { const switchToBranch = spy(async () => ({ switched: true, from: 'ticket/59234', to: 'ticket/61002', parked: true })); const currentBranchName = spy(async () => 'ticket/59234'); diff --git a/test/preload-listeners.test.cjs b/test/preload-listeners.test.cjs index 9cd2319..ce52c7d 100644 --- a/test/preload-listeners.test.cjs +++ b/test/preload-listeners.test.cjs @@ -287,3 +287,68 @@ test('runNpmScript returns the run id to the caller', async () => { // attached so early logs/URL are captured"), which is what the fix would look // like here. Left as a note rather than a test: asserting the current ordering // would pin the gap shut, and closing it is not #149. + +// --- the subscribe* family (#173) ------------------------------------------ +// +// The other half of the bridge, and until now untested. These are long-lived +// subscriptions rather than the per-run pairs above, which is exactly why the +// switch progress uses one: a switch emits its first event milliseconds in, +// well before its invoke answers, so the ordering gap noted at the end of this +// file would have swallowed the start of every switch. +// +// The leak shape here is different too. `App` subscribes once for every site +// rather than per run, so an unsubscribe that removed by channel instead of by +// handler would silence a site that is still open. +const SUBSCRIPTIONS = [ + { name: 'subscribeSwitchProgress', channel: 'switch:progress' }, + { name: 'subscribeSetupProgress', channel: 'download:progress' }, + { name: 'subscribeSetupStatus', channel: 'download:status' } +]; + +for (const sub of SUBSCRIPTIONS) { + test(`${sub.name}: subscribing listens once and unsubscribing stops (issue #173)`, () => { + const { api, ipcRenderer } = loadPreload(); + + const unsubscribe = api[sub.name](() => {}); + assert.equal(ipcRenderer.listenerCount(sub.channel), 1); + + unsubscribe(); + assert.equal(ipcRenderer.listenerCount(sub.channel), 0); + }); + + test(`${sub.name}: the handler gets the payload, never the Electron event (issue #173)`, () => { + const { api, ipcRenderer } = loadPreload(); + const seen = []; + + api[sub.name]((payload) => seen.push(payload)); + ipcRenderer.emit(sub.channel, { sitePath: '/sites/wp', stage: 'scan' }); + + assert.deepEqual(seen, [{ sitePath: '/sites/wp', stage: 'scan' }]); + }); + + test(`${sub.name}: one unsubscribe leaves other subscribers alone (issue #173)`, () => { + const { api, ipcRenderer } = loadPreload(); + const mine = []; + const theirs = []; + + const unsubscribeMine = api[sub.name]((p) => mine.push(p)); + api[sub.name]((p) => theirs.push(p)); + unsubscribeMine(); + + ipcRenderer.emit(sub.channel, { stage: 'done' }); + assert.equal(ipcRenderer.listenerCount(sub.channel), 1); + assert.deepEqual(mine, []); + assert.deepEqual(theirs, [{ stage: 'done' }]); + }); + + test(`${sub.name}: unsubscribing twice, and no handler at all, are both safe (issue #173)`, () => { + const { api, ipcRenderer } = loadPreload(); + + const unsubscribe = api[sub.name](); + ipcRenderer.emit(sub.channel, { stage: 'scan' }); + unsubscribe(); + unsubscribe(); + + assert.equal(ipcRenderer.listenerCount(sub.channel), 0); + }); +} diff --git a/test/switch-progress.test.cjs b/test/switch-progress.test.cjs new file mode 100644 index 0000000..bc4ba40 --- /dev/null +++ b/test/switch-progress.test.cjs @@ -0,0 +1,189 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert'); +const { + createProgressThrottle, + mapCheckoutPhase, + describeSwitchProgress +} = require('../src/switch-progress.cjs'); + +// A clock the tests move by hand: throttling asserted without timers, so none +// of this can go flaky on a loaded CI runner. +function harness(options = {}) { + let clock = 1000; + const emitted = []; + const throttle = createProgressThrottle({ + onEmit: (p) => emitted.push(p), + now: () => clock, + ...options + }); + return { emitted, throttle, advance: (ms) => { clock += ms; } }; +} + +// --- createProgressThrottle ------------------------------------------------ + +// A `git.checkout` of wordpress-develop calls onProgress thousands of times in +// well under a second. Sending each one would flood IPC with frames nobody can +// read, so within a stage they are coalesced. +test('throttle: repeats inside the interval are coalesced (issue #173)', () => { + const { emitted, throttle, advance } = harness({ intervalMs: 100 }); + + throttle.emit({ stage: 'apply', loaded: 1, total: 500 }); + throttle.emit({ stage: 'apply', loaded: 2, total: 500 }); + advance(30); + throttle.emit({ stage: 'apply', loaded: 3, total: 500 }); + + assert.deepStrictEqual(emitted.map((p) => p.loaded), [1]); +}); + +test('throttle: once the interval has passed the next event goes out (issue #173)', () => { + const { emitted, throttle, advance } = harness({ intervalMs: 100 }); + + throttle.emit({ stage: 'apply', loaded: 1, total: 500 }); + advance(100); + throttle.emit({ stage: 'apply', loaded: 2, total: 500 }); + + assert.deepStrictEqual(emitted.map((p) => p.loaded), [1, 2]); +}); + +// A stage is what the sentence on screen is made of, so it can never wait for +// the interval — the whole point is that the panel keeps saying what is +// happening. +test('throttle: a new stage is emitted immediately, interval or not (issue #173)', () => { + const { emitted, throttle } = harness({ intervalMs: 100 }); + + throttle.emit({ stage: 'scan', loaded: 1 }); + throttle.emit({ stage: 'commit', loaded: 1 }); + + assert.deepStrictEqual(emitted.map((p) => p.stage), ['scan', 'commit']); +}); + +// The 87% rule: a progress line that stops partway and jumps to done is worse +// than no line at all, because it reads as a hang at exactly the moment the +// user is deciding whether to force-quit. The last suppressed event of a stage +// has to arrive before the next stage starts. +test('throttle: the last suppressed event of a stage survives into the next one (issue #173)', () => { + const { emitted, throttle } = harness({ intervalMs: 100 }); + + throttle.emit({ stage: 'apply', loaded: 1, total: 500 }); + throttle.emit({ stage: 'apply', loaded: 250, total: 500 }); + throttle.emit({ stage: 'apply', loaded: 500, total: 500 }); + throttle.emit({ stage: 'done' }); + + assert.deepStrictEqual( + emitted.map((p) => `${p.stage}:${p.loaded ?? ''}`), + ['apply:1', 'apply:500', 'done:'], + 'the 500/500 frame is what makes the line reach the end' + ); +}); + +test('throttle: flush emits what is pending, and only once (issue #173)', () => { + const { emitted, throttle } = harness({ intervalMs: 100 }); + + throttle.emit({ stage: 'apply', loaded: 1, total: 500 }); + throttle.emit({ stage: 'apply', loaded: 2, total: 500 }); + throttle.flush(); + throttle.flush(); + + assert.deepStrictEqual(emitted.map((p) => p.loaded), [1, 2]); +}); + +test('throttle: flush with nothing pending emits nothing (issue #173)', () => { + const { emitted, throttle } = harness({ intervalMs: 100 }); + + throttle.flush(); + + assert.deepStrictEqual(emitted, []); +}); + +// isomorphic-git awaits whatever onProgress returns. A thenable would put a +// microtask between every one of ~4400 checkout events, so the callback is +// synchronous by contract, not by accident. +test('throttle: emit is synchronous and returns nothing to await (issue #173)', () => { + const { throttle } = harness(); + + const result = throttle.emit({ stage: 'scan', loaded: 1 }); + + assert.strictEqual(result, undefined); + assert.strictEqual(typeof (result && result.then), 'undefined'); +}); + +// The trunk update writes into an append-only terminal, where one line per +// stage is informative and fifteen is spam. Same module, no second throttle. +test('throttle: an infinite interval reduces a switch to one line per stage (issue #173)', () => { + const { emitted, throttle } = harness({ intervalMs: Infinity }); + + throttle.emit({ stage: 'scan', loaded: 1 }); + throttle.emit({ stage: 'scan', loaded: 2 }); + throttle.emit({ stage: 'apply', loaded: 1, total: 9 }); + throttle.emit({ stage: 'apply', loaded: 5, total: 9 }); + throttle.flush(); + + assert.deepStrictEqual( + emitted.map((p) => `${p.stage}:${p.loaded}`), + ['scan:1', 'scan:2', 'apply:1', 'apply:5'], + 'stage changes and the flush get through; the middle of a stage does not' + ); +}); + +// --- mapCheckoutPhase ------------------------------------------------------ + +test('mapCheckoutPhase: the two phases checkout actually emits (issue #173)', () => { + assert.deepStrictEqual( + mapCheckoutPhase({ phase: 'Analyzing workdir', loaded: 12 }), + { stage: 'analyze', loaded: 12, total: undefined } + ); + assert.deepStrictEqual( + mapCheckoutPhase({ phase: 'Updating workdir', loaded: 3, total: 40 }), + { stage: 'apply', loaded: 3, total: 40 } + ); +}); + +// isomorphic-git owns these strings, not us. A version bump that renames or +// adds one must degrade to a generic stage rather than an undefined that +// renders as "undefined" in front of a contributor. +test('mapCheckoutPhase: an unknown phase still produces a usable stage (issue #173)', () => { + const mapped = mapCheckoutPhase({ phase: 'Reticulating splines', loaded: 1, total: 2 }); + + assert.strictEqual(typeof mapped.stage, 'string'); + assert.ok(mapped.stage.length > 0); + assert.strictEqual(typeof describeSwitchProgress(mapped), 'string'); +}); + +// --- describeSwitchProgress ------------------------------------------------ + +// The sentence this whole issue exists for: someone who force-quits here loses +// work that is not committed anywhere yet, so the panel has to say that this is +// what it is doing, and for which ticket. +test('describeSwitchProgress: parking names the ticket being left (issue #173)', () => { + const line = describeSwitchProgress({ stage: 'scan', from: 'ticket/59234', to: 'ticket/61002' }); + + assert.match(line, /59234/); + assert.doesNotMatch(line, /ticket\//, 'a branch ref is our word for it, not the contributor\'s'); +}); + +test('describeSwitchProgress: leaving trunk talks about the work, not a branch name (issue #173)', () => { + const line = describeSwitchProgress({ stage: 'scan', from: 'trunk', to: 'ticket/61002' }); + + assert.strictEqual(typeof line, 'string'); + assert.doesNotMatch(line, /trunk/); +}); + +// `Analyzing workdir` reports `loaded` with no `total`, so there is no honest +// percentage for that half of the checkout — and a made-up one is worse than +// none. +test('describeSwitchProgress: a percentage only when there is a total (issue #173)', () => { + assert.doesNotMatch(describeSwitchProgress({ stage: 'analyze', loaded: 900 }), /%/); + assert.doesNotMatch(describeSwitchProgress({ stage: 'apply', loaded: 5, total: 0 }), /%/); + assert.match(describeSwitchProgress({ stage: 'apply', loaded: 25, total: 100 }), /25%/); +}); + +test('describeSwitchProgress: every stage says something, including one we do not know (issue #173)', () => { + for (const stage of ['scan', 'stage', 'commit', 'analyze', 'apply', 'done', 'something-new']) { + const line = describeSwitchProgress({ stage, loaded: 1, total: 2, from: 'trunk', to: 'ticket/1' }); + assert.strictEqual(typeof line, 'string', stage); + assert.ok(line.length > 0, stage); + assert.doesNotMatch(line, /undefined|NaN/, stage); + } +}); diff --git a/test/ticket-branches.integration.test.cjs b/test/ticket-branches.integration.test.cjs index 45f56ab..0392ea3 100644 --- a/test/ticket-branches.integration.test.cjs +++ b/test/ticket-branches.integration.test.cjs @@ -234,3 +234,100 @@ test('switching to a branch that does not exist refuses (issue #108)', async (t) (e) => e.code === 'no-such-branch' ); }); + +// --- progress while the worktree is swapped (issue #173) ------------------- + +// A switch is a worktree scan and a full checkout: seconds of silence on a real +// wordpress-develop, during which the window is indistinguishable from hung. +// The stages below are what the panel turns into a sentence, so their order and +// their presence is the contract — particularly the park stages, which cover +// the stretch where the contributor's edits are not committed anywhere yet. +test('switchToBranch reports every stage of a park and a checkout (issue #173)', async (t) => { + const { dir, baseOid } = await makeSite(t); + await startTicketBranch(dir, 59234); + await switchToBranch(dir, TRUNK, { baseOid }); + await startTicketBranch(dir, 61002); + fs.writeFileSync(path.join(dir, 'wp-login.php'), ' seen.push(p) }); + + const stages = seen.map((p) => p.stage); + assert.ok(stages.includes('scan'), stages.join(',')); + assert.ok(stages.includes('stage'), stages.join(',')); + assert.ok(stages.includes('commit'), stages.join(',')); + assert.equal(stages.indexOf('scan') < stages.indexOf('stage'), true, 'the scan comes before what it feeds'); + assert.equal(stages.indexOf('stage') < stages.indexOf('commit'), true); + assert.equal(stages[stages.length - 1], 'done', 'the line has to reach the end'); + // Where it is going, on every payload, so the panel need not track it. + assert.equal(seen.every((p) => p.to === ticketBranchRef(59234)), true); + // And where the work being saved came from. + assert.equal(seen.find((p) => p.stage === 'commit').from, ticketBranchRef(61002)); + // The staging stage is the one with an honest total. + const staging = seen.filter((p) => p.stage === 'stage'); + assert.ok(staging.length > 0); + assert.equal(staging.every((p) => Number.isFinite(p.total) && p.total > 0), true); +}); + +// Nothing to park is the common case — switching away from a ticket you only +// read. The scan still runs and still costs, so it is still announced; the +// commit never happens and must not be claimed. +test('a clean branch reports the scan but never claims to commit (issue #173)', async (t) => { + const { dir, baseOid } = await makeSite(t); + await startTicketBranch(dir, 59234); + await switchToBranch(dir, TRUNK, { baseOid }); + await startTicketBranch(dir, 61002); + + const seen = []; + await switchToBranch(dir, ticketBranchRef(59234), { baseOid, onProgress: (p) => seen.push(p) }); + + const stages = seen.map((p) => p.stage); + assert.equal(stages[0], 'scan'); + assert.equal(stages.includes('commit'), false, 'nothing was committed, so nothing may say so'); + assert.equal(stages[stages.length - 1], 'done'); +}); + +// Leaving trunk runs a full scan that usually ends in "nothing to do". Silent +// before this, and it is the same cost as any other scan. +test('leaving a clean trunk still reports its scan (issue #173)', async (t) => { + const { dir, baseOid } = await makeSite(t); + await startTicketBranch(dir, 59234); + await switchToBranch(dir, TRUNK, { baseOid }); + + const seen = []; + await switchToBranch(dir, ticketBranchRef(59234), { baseOid, onProgress: (p) => seen.push(p) }); + + assert.equal(seen[0].stage, 'scan'); + assert.equal(seen[0].from, TRUNK); + assert.equal(seen[seen.length - 1].stage, 'done'); +}); + +// A refused switch changed nothing, so it must not report a checkout it never +// ran, and must not say it is done. +test('a refused dirty-trunk switch reports the scan and stops there (issue #173)', async (t) => { + const { dir, baseOid } = await makeSite(t); + await startTicketBranch(dir, 59234); + await switchToBranch(dir, TRUNK, { baseOid }); + fs.writeFileSync(path.join(dir, 'wp-login.php'), ' switchToBranch(dir, ticketBranchRef(59234), { baseOid, onProgress: (p) => seen.push(p) }), + (e) => e.code === 'dirty-trunk' + ); + + assert.deepEqual(seen.map((p) => p.stage), ['scan']); +}); + +// Progress is an addition, not a requirement: every existing caller passes no +// callback and must keep working. +test('a switch without a progress callback still works (issue #173)', async (t) => { + const { dir, baseOid } = await makeSite(t); + await startTicketBranch(dir, 59234); + fs.writeFileSync(path.join(dir, 'wp-login.php'), ' Date: Sun, 9 Aug 2026 00:13:52 +0200 Subject: [PATCH 2/3] Keep the ticket named through the longest stage, and flush what a failure held (self-review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the pre-PR review pass. The one that mattered: the staging stage carried no `from`, so the sentence dropped to "Saving your work… 41%" for the bulk of the park — losing the ticket number for exactly the stretch where naming it is the point. The scan and commit either side of it read correctly, which is how it looked right in every screenshot. Verified by rendering a real payload, and now asserted on the sentence rather than on the fields. The flushes were bare statements after an await, so a switch that died mid-checkout dropped whatever the throttle was holding — the case where the last frame is worth most, and in the trunk update the last line of the stage that failed. They are in a finally now, with a test that fails without it. And a refused switch left its last frame in the map, so the discard-and-switch that follows a dirty-trunk refusal ran under a spinner describing the attempt that never happened. Co-Authored-By: Claude Fable 5 --- src/main.js | 60 ++++++++++++++++------- src/renderer/index.jsx | 5 +- src/ticket-branches.js | 5 +- test/ipc-wiring.test.cjs | 33 +++++++++++++ test/ticket-branches.integration.test.cjs | 11 +++++ 5 files changed, 94 insertions(+), 20 deletions(-) diff --git a/src/main.js b/src/main.js index 4b9d8be..08dea02 100644 --- a/src/main.js +++ b/src/main.js @@ -711,9 +711,10 @@ async function withSwitchMarker(sitePath, run) { /** * Switch progress as terminal lines, for the trunk update (#173). * - * `intervalMs: Infinity` suppresses everything except a change of stage and the - * final flush, which turns a switch into about five lines instead of fifteen — - * the same module the panel uses, in the mode an append-only log wants. + * `intervalMs: Infinity` suppresses everything except the first frame of a + * stage and the last one held when it turns over, which is a handful of lines per switch instead of + * thousands — the same module the panel drives live, in the mode an + * append-only log wants. * * @param {Function} sendLog Writes one chunk to the update's log stream. * @return {{emit: Function, flush: Function}} Pass `emit` as `onProgress`. @@ -889,11 +890,16 @@ ipcMain.handle('git:update-trunk', async (event, sitePath) => { // how the two end up disagreeing. `Infinity` keeps it to one // line per stage, since this log is append-only. const parkLog = updateSwitchLogger(sendLog); - await withSwitchMarker(sitePath, () => switchToBranch(sitePath, TRUNK, { - baseOid: branchMetaBefore && branchMetaBefore.baseOid, - onProgress: parkLog.emit - })); - parkLog.flush(); + try { + await withSwitchMarker(sitePath, () => switchToBranch(sitePath, TRUNK, { + baseOid: branchMetaBefore && branchMetaBefore.baseOid, + onProgress: parkLog.emit + })); + } finally { + // The last line of the stage a park died in is the one line + // a contributor reading this log actually wants. + parkLog.flush(); + } await mergeSiteMeta(sitePath, { currentBranch: TRUNK }); } @@ -926,8 +932,11 @@ ipcMain.handle('git:update-trunk', async (event, sitePath) => { if (ticketBefore !== null) { sendLog(`\nReturning to your work on ${branchBefore}…\n`); const returnLog = updateSwitchLogger(sendLog); - await withSwitchMarker(sitePath, () => switchToBranch(sitePath, branchBefore, { onProgress: returnLog.emit })); - returnLog.flush(); + try { + await withSwitchMarker(sitePath, () => switchToBranch(sitePath, branchBefore, { onProgress: returnLog.emit })); + } finally { + returnLog.flush(); + } await mergeSiteMeta(sitePath, { currentBranch: branchBefore, tracTicket: ticketBefore }); } sendDone({ ok: true, ...result, branch: branchBefore }); @@ -1451,8 +1460,13 @@ ipcMain.handle('sites:set-ticket', async (event, sitePath, ref) => withRegistere const { ref: current, meta } = await activeBranch(sitePath, { migrate: true }); if (current !== TRUNK) { const progress = switchProgressReporter(event, sitePath); - await withSwitchMarker(sitePath, () => switchToBranch(sitePath, TRUNK, { baseOid: meta && meta.baseOid, onProgress: progress.emit })); - progress.flush(); + try { + await withSwitchMarker(sitePath, () => switchToBranch(sitePath, TRUNK, { baseOid: meta && meta.baseOid, onProgress: progress.emit })); + } finally { + // In a finally because a switch that dies mid-checkout is exactly + // when the last frame it reached is worth having. + progress.flush(); + } } await mergeSiteMeta(sitePath, { tracTicket: null, currentBranch: TRUNK }); return { ok: true, ticket: null, branch: TRUNK }; @@ -1474,8 +1488,11 @@ ipcMain.handle('sites:set-ticket', async (event, sitePath, ref) => withRegistere let baseOid; if (known.includes(branchRef)) { const progress = switchProgressReporter(event, sitePath); - await withSwitchMarker(sitePath, () => switchToBranch(sitePath, branchRef, { baseOid: meta && meta.baseOid, onProgress: progress.emit })); - progress.flush(); + try { + await withSwitchMarker(sitePath, () => switchToBranch(sitePath, branchRef, { baseOid: meta && meta.baseOid, onProgress: progress.emit })); + } finally { + progress.flush(); + } baseOid = ((await readSiteMeta(sitePath)).branches || {})[branchRef]?.baseOid || null; } else { // Starting a ticket from another ticket parks that one first; from trunk @@ -1483,8 +1500,11 @@ ipcMain.handle('sites:set-ticket', async (event, sitePath, ref) => withRegistere // "I started editing, then realised which ticket this is"). if (current !== TRUNK) { const progress = switchProgressReporter(event, sitePath); - await withSwitchMarker(sitePath, () => switchToBranch(sitePath, TRUNK, { baseOid: meta && meta.baseOid, onProgress: progress.emit })); - progress.flush(); + try { + await withSwitchMarker(sitePath, () => switchToBranch(sitePath, TRUNK, { baseOid: meta && meta.baseOid, onProgress: progress.emit })); + } finally { + progress.flush(); + } } ({ baseOid } = await startTicketBranch(sitePath, parsed.id)); } @@ -1519,8 +1539,12 @@ ipcMain.handle('branches:switch', async (event, sitePath, targetRef) => withRegi if (blocked) return blocked; const { ref: current, meta } = await activeBranch(sitePath, { migrate: true }); const progress = switchProgressReporter(event, sitePath); - const result = await withSwitchMarker(sitePath, () => switchToBranch(sitePath, targetRef, { baseOid: meta && meta.baseOid, onProgress: progress.emit })); - progress.flush(); + let result; + try { + result = await withSwitchMarker(sitePath, () => switchToBranch(sitePath, targetRef, { baseOid: meta && meta.baseOid, onProgress: progress.emit })); + } finally { + progress.flush(); + } const ticketId = ticketIdFromRef(targetRef); if (targetRef !== TRUNK) { await mergeBranchMeta(sitePath, targetRef, { lastUsedAt: new Date().toISOString() }); diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index efcfd59..ce379a9 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -1337,6 +1337,9 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit const discardTrunkWorkAndSwitch = useCallback(async (ref) => { setTicketSaving(true); setTicketError(''); + // The refused attempt left its last frame behind — without this, the + // discard runs under a spinner describing a switch that never happened. + if (onClearSwitchProgress) onClearSwitchProgress(sitePath); try { const res = await window.api.discardChanges(sitePath); if (!res?.ok) { @@ -1354,7 +1357,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // Outside the guard above: saveTicket owns the busy flag itself, and the // discard has already succeeded — a failure here is about the switch. await saveTicket(ref); - }, [sitePath, saveTicket]); + }, [sitePath, saveTicket, onClearSwitchProgress]); // "Delete this ticket's work" (#108) — destroys the branch, which is why it // sits behind a confirm while switching does not. diff --git a/src/ticket-branches.js b/src/ticket-branches.js index 7af76e5..7d01ccf 100644 --- a/src/ticket-branches.js +++ b/src/ticket-branches.js @@ -189,7 +189,10 @@ async function parkCurrentWork(dir, { baseOid, author = WIP_AUTHOR, onProgress = // wiped by hand, a site adopted from disk) still parks against something // sane instead of throwing. const parent = baseOid || await git.resolveRef({ fs, dir, ref: TRUNK }); - await stageWorktree(dir, matrix, onProgress); + // `from` added here rather than inside the loop: staging is the longest + // stretch of a park, and without it the sentence loses the ticket number for + // exactly the seconds it most needs to name it. + await stageWorktree(dir, matrix, onProgress && ((p) => onProgress({ from: branch, ...p }))); if (onProgress) onProgress({ stage: 'commit', from: branch }); const oid = await git.commit({ fs, dir, message: WIP_MESSAGE, author, parent: [parent] }); return { parked: true, branch, oid }; diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index 0d9e715..0808edd 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -1820,6 +1820,39 @@ test('a window closed mid-switch does not turn a finished switch into a failure assert.equal(result.ticket, null); }); +// A switch that dies mid-checkout is exactly when the last frame it reached is +// worth having — and it is the case where nothing calls flush unless the flush +// is in a finally. +test('a switch that fails still delivers the last frame it reached (issue #173)', async () => { + const switchToBranch = spy(async (dir, ref, options) => { + options.onProgress({ stage: 'apply', loaded: 1, total: 900, to: ref }); + // Suppressed by the throttle: same stage, well inside the interval. + options.onProgress({ stage: 'apply', loaded: 400, total: 900, to: ref }); + const e = new Error('EPERM'); + e.stage = 'checkout'; + throw e; + }); + const currentBranchName = spy(async () => 'ticket/59234'); + const settings = fakeSettingsStore({ + sites: ['/sites/wp'], + siteMeta: { '/sites/wp': { branches: { 'ticket/59234': { baseOid: 'abc' } }, currentBranch: 'ticket/59234' } } + }); + const main = loadMain({ + stubs: { ...silentLogging(), ...settings.stubs, './ticket-branches': { switchToBranch, currentBranchName } } + }); + + const event = createIpcEvent(); + const result = await main.invokeWith('branches:switch', event, '/sites/wp', 'ticket/61002'); + + assert.equal(result.ok, false, 'the failure is still a failure'); + const progress = event.sent.filter((m) => m.channel === SWITCH_PROGRESS_CHANNEL); + assert.deepEqual( + progress.map((m) => m.payload.loaded), + [1, 400], + 'the frame held back by the throttle has to arrive, or the line freezes part-way' + ); +}); + // No renderer calls this door yet. It streams anyway: the day the switcher is // wired to it, silence would arrive with nothing failing — the guard below // cannot see a send-only channel. diff --git a/test/ticket-branches.integration.test.cjs b/test/ticket-branches.integration.test.cjs index 0392ea3..ce577c1 100644 --- a/test/ticket-branches.integration.test.cjs +++ b/test/ticket-branches.integration.test.cjs @@ -25,6 +25,7 @@ const { switchToBranch, deleteTicketBranch } = require('../src/ticket-branches.js'); +const { describeSwitchProgress } = require('../src/switch-progress.cjs'); const AUTHOR = { name: 'test', email: 'test@example.com' }; @@ -267,6 +268,16 @@ test('switchToBranch reports every stage of a park and a checkout (issue #173)', const staging = seen.filter((p) => p.stage === 'stage'); assert.ok(staging.length > 0); assert.equal(staging.every((p) => Number.isFinite(p.total) && p.total > 0), true); + // And it is the longest stretch of the park, so it has to keep naming the + // ticket — a sentence that drops to "Saving your work…" for most of the wait + // is the one that fails to stop someone force-quitting. + assert.equal(staging.every((p) => p.from === ticketBranchRef(61002)), true); + assert.equal( + seen.every((p) => describeSwitchProgress(p).length > 0), + true, + 'every payload has to render as something' + ); + assert.match(describeSwitchProgress(staging[0]), /#61002/); }); // Nothing to park is the common case — switching away from a ticket you only From 196dc2d68e40ce1ca6906a9dc3d0de1a34063ab4 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Sun, 9 Aug 2026 15:19:03 +0200 Subject: [PATCH 3/3] Wait for the trunk update's result the same way as the rest Same helper, same reason: a park reads the worktree before it does anything, so how many event-loop turns the result takes is a property of the filesystem, not of the code. Co-Authored-By: Claude Fable 5 --- test/ipc-wiring.test.cjs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index 1331ade..d8bdc8c 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -1757,14 +1757,10 @@ test('branches:list reports the branches on disk with their stored context', asy assert.equal(result.branches[1].baseOid, null, 'a branch the registry has never seen still lists'); }); -// The same wait, for the trunk update's own :done channel. -async function updateDone(event, updateId, cap = 50) { - for (let i = 0; i < cap; i++) { - const hit = event.sent.find((m) => m.channel === 'git:update-trunk:done' && m.payload.updateId === updateId); - if (hit) return hit.payload; - await new Promise((r) => setImmediate(r)); - } - throw new Error('git:update-trunk never reported done'); +// The same wait, for the trunk update's own :done channel — and on the clock +// for the same reason, since a park reads the worktree before anything else. +async function updateDone(event, updateId) { + return waitForDone(event, 'git:update-trunk:done', 'updateId', updateId); } // --- switch progress -> src/switch-progress.cjs (#173) ---------------------