From 863775351f79c2ed21e7edcb18187af977142dd0 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Wed, 12 Aug 2026 11:37:06 +0200 Subject: [PATCH 1/7] Bump the Trac scrape generation before the auto-read fires, not after MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two useEffects were both keyed off tracTicket: one auto-triggered a scrape right after a ticket was freshly linked, the other bumped scrapeGenRef to mark later scrapes as stale. React runs same-component passive effects in declaration order, so the auto-scrape fired first and captured the previous ticket's generation — its finally guard then never matched, leaving the "Reading ticket..." spinner stuck with no way to recover short of switching tasks again. Merges the two effects and bumps the generation synchronously at the top, before anything below can trigger a scrape. Fixes #299 Co-Authored-By: Claude Sonnet 5 --- src/renderer/index.jsx | 24 ++++++++------- test/trac-ticket-scrape-ordering.test.cjs | 36 +++++++++++++++++++++++ 2 files changed, 49 insertions(+), 11 deletions(-) create mode 100644 test/trac-ticket-scrape-ordering.test.cjs diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index d51d1d6..aa3aa48 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -2996,12 +2996,20 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // an already-linked site would open a Trac window nobody asked for (#292). const autoReadTicketRef = useRef(null); const tracScrapeRef = useRef(null); + // A Trac scrape can run up to 90s. Bump a generation on every ticket change so + // a scrape that resolves after the ticket has moved on is dropped, rather than + // shown under the wrong ticket or clearing a newer request's loading flag. + const scrapeGenRef = useRef(0); useEffect(() => { + // Bumped first, synchronously, before anything below can trigger a scrape + // (#299): loadTracAttachments reads this ref at call time, so the auto-read + // a few lines down must never run against last ticket's generation. + scrapeGenRef.current += 1; if (!tracTicket) { setTicketPatches(null); // Attachments are per-ticket and loaded on demand; a stale list from the // previous ticket must not linger, and a scrape dropped by the generation - // bump below must not leave a stuck spinner. + // bump above must not leave a stuck spinner. setTracAttachments(null); setTracAttachmentsLoading(false); loadedTicketRef.current = null; @@ -3010,10 +3018,10 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit if (!isActive || loadedTicketRef.current === tracTicket) return; // A new ticket on the active site: drop any attachments the previous one // loaded (and clear its loading flag, so a scrape dropped by the generation - // bump cannot leave a stuck spinner with no button to recover), then fetch - // its PRs. Marked loaded before the fetch resolves, on purpose: a failed - // initial fetch is not retried on every re-activation (which could keep - // spending a rate-limited quota) — Refresh is the retry. + // bump above cannot leave a stuck spinner with no button to recover), then + // fetch its PRs. Marked loaded before the fetch resolves, on purpose: a + // failed initial fetch is not retried on every re-activation (which could + // keep spending a rate-limited quota) — Refresh is the retry. setTracAttachments(null); setTracAttachmentsLoading(false); loadedTicketRef.current = tracTicket; @@ -3031,12 +3039,6 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit } }, [tracTicket, isActive, loadTicketPatches]); - // A Trac scrape can run up to 90s. Bump a generation on every ticket change so - // a scrape that resolves after the ticket has moved on is dropped, rather than - // shown under the wrong ticket or clearing a newer request's loading flag. - const scrapeGenRef = useRef(0); - useEffect(() => { scrapeGenRef.current += 1; }, [tracTicket]); - // Fetches a PR's diff and drops into the same preview the file picker uses, // so applying a PR and applying a downloaded patch are one path from here on. const previewPr = async (pr) => { diff --git a/test/trac-ticket-scrape-ordering.test.cjs b/test/trac-ticket-scrape-ordering.test.cjs new file mode 100644 index 0000000..248800e --- /dev/null +++ b/test/trac-ticket-scrape-ordering.test.cjs @@ -0,0 +1,36 @@ +'use strict'; +const test = require('node:test'); +const assert = require('node:assert'); +const fs = require('node:fs'); +const path = require('node:path'); + +// #299: the auto-read effect and the scrapeGenRef bump were two separate +// useEffects, both keyed off tracTicket. React runs same-component passive +// effects in declaration order, so the auto-scrape fired before the +// generation bump — loadTracAttachments captured a stale gen, and its +// finally guard (`gen === scrapeGenRef.current`) never matched, leaving the +// "Reading ticket..." spinner stuck. The fix bumps the generation +// synchronously, ahead of the auto-triggered scrape, in the same effect. +test('scrape ordering: scrapeGenRef bumps before the auto-triggered scrape fires (issue #299)', () => { + const source = fs.readFileSync(path.join(__dirname, '..', 'src', 'renderer', 'index.jsx'), 'utf8'); + + const genBumpIdx = source.indexOf('scrapeGenRef.current += 1'); + const scrapeCallIdx = source.indexOf('tracScrapeRef.current()'); + + assert.ok(genBumpIdx !== -1, 'expected to find the scrapeGenRef bump in index.jsx'); + assert.ok(scrapeCallIdx !== -1, 'expected to find the auto-triggered tracScrapeRef.current() call in index.jsx'); + assert.ok( + genBumpIdx < scrapeCallIdx, + 'scrapeGenRef must bump before the auto-scrape fires, or loadTracAttachments captures a stale generation and its loading flag never clears (#299)' + ); + + // A second useEffect bumping the generation, declared after the + // auto-scrape effect, is exactly the ordering bug: same-component + // passive effects run in declaration order, so a later effect cannot + // beat an earlier one's synchronous body. + assert.strictEqual( + source.split('scrapeGenRef.current += 1').length - 1, + 1, + 'expected exactly one scrapeGenRef bump; a separate effect duplicating it reintroduces the ordering race (#299)' + ); +}); From 0f5669f5f010cbd379d47bcc0f4dfab1899e0eb8 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Wed, 12 Aug 2026 12:13:58 +0200 Subject: [PATCH 2/7] Measure the patch conflict warning from the ticket's base, not from HEAD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-apply warning ("You have your own edits to ...") asked `collectDirtyFiles`, which is HEAD-relative. Under the ticket-as-branch model a ticket's work lives in its parked WIP commit, so HEAD is the ticket's last saved state — and a ticket that has been left and resumed has a worktree matching it exactly. The warning went silent on precisely the tree it exists to protect, and spoke up about files edited back to what the base holds. `git:preview-patch` now asks `collectUnsubmittedFiles` — the same base-relative walk the patch itself, the PR, the card's note and `git:discard-to-base` are measured with. On trunk `patchBaseOid` answers null and the walk falls back to HEAD, so trunk sites keep their answer. Fixes #301 Co-Authored-By: Claude Opus 5 (1M context) --- src/main.js | 19 +++++++--- test/ipc-wiring.test.cjs | 81 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 89 insertions(+), 11 deletions(-) diff --git a/src/main.js b/src/main.js index 2cb85ce..86641ef 100644 --- a/src/main.js +++ b/src/main.js @@ -1134,9 +1134,10 @@ async function collectUnsubmittedFiles(sitePath) { // Two questions, deliberately two channels (#239). This one asks "are there // edits not written down yet" — the narrow reading the checkout guards need: -// the trunk-update dirty dialog and the patch-apply collision scan protect -// exactly the files a force checkout would overwrite, and parked work is not -// among them. The card's note asks `git:unsubmitted-work` instead. +// the trunk-update dirty dialog protects exactly the files a force checkout +// would overwrite, and parked work is not among them. Everything that asks +// what this ticket has done — the card's note, and the patch-apply collision +// scan (#301) — asks `git:unsubmitted-work`'s question instead. ipcMain.handle('git:worktree-dirty', async (_e, sitePath) => { try { const files = await collectDirtyFiles(sitePath); @@ -1433,18 +1434,26 @@ const REVERTABLE_PATCH_LIMIT = 512 * 1024; // Reading a patch without touching the checkout, so the contributor sees which // files it would change — and which of their own edits it collides with — // before deciding. +// +// Measured from the ticket's base, not from HEAD (#301). Under the +// ticket-as-branch model a ticket's work lives in its parked WIP commit, so a +// ticket that has been left and resumed has a worktree matching HEAD exactly: +// HEAD-relative, the warning goes silent on precisely the tree it exists to +// protect, and speaks up about files edited back to what the base holds. This +// is the same measurement the patch itself is taken with, so what the warning +// calls "your own edits" is what the patch modal would show. ipcMain.handle('git:preview-patch', async (_e, sitePath, patchText) => { try { const parsed = parsePatchFiles(patchText); if (!parsed.ok) return { ok: false, error: parsed.error }; let dirtyPaths; try { - dirtyPaths = await collectDirtyFiles(sitePath); + dirtyPaths = await collectUnsubmittedFiles(sitePath); } catch (e) { // Failing open here would promise "no collisions" precisely when the // app could not look — surface the failure instead. logError('git:preview-patch', String(e && e.stack ? e.stack : e)); - return { ok: false, error: 'Could not check your working tree for conflicts, so the preview was not shown.' }; + return { ok: false, error: 'Could not check your work for conflicts, so the preview was not shown.' }; } const plan = planApply({ files: parsed.files, dirtyPaths }); return { ok: true, ...plan, files: parsed.files.map((f) => ({ kind: f.kind, path: f.path })) }; diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index f8e8b98..dbdfc31 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -465,24 +465,31 @@ test('git:discard-changes resets through trunk-update and clears the applied-pat // A site the way the ticket-as-branch model leaves it: the contributor's work // parked in the branch's single WIP commit, the worktree clean. `git status` // says nothing; the patch says +1 line. The note has to side with the patch. -async function parkedTicketRepo(t) { +// `workFile` is where the parked edit lands. The default keeps this repo as the +// note's tests have always had it; the patch-preview tests below ask for the +// `src/` layout instead, because a patch's paths are read through +// `mapToSrcLayout` and a collision is a string match on the result. +async function parkedTicketRepo(t, { workFile = 'wp-login.php' } = {}) { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ipc-wiring-parked-')); t.after(() => fs.rmSync(dir, { recursive: true, force: true })); await git.init({ fs, dir, defaultBranch: 'trunk' }); const author = { name: 'test', email: 'test@example.com' }; - fs.writeFileSync(path.join(dir, 'wp-login.php'), ' { assert.deepEqual(result, { ok: false, error: 'unreadable' }); }); +// --- what the collision warning is measured against (#301) ---------------- + +// A patch against `src/wp-login.php`, the file the parked repo's ticket work is +// in. Written the way the app's own reader expects to read it back. +const LOGIN_DIFF = `diff --git a/src/wp-login.php b/src/wp-login.php +index 384bf9e..dbfa038 100644 +--- a/src/wp-login.php ++++ b/src/wp-login.php +@@ -1,2 +1,2 @@ + { + const { dir, baseOid } = await parkedTicketRepo(t, { workFile: 'src/wp-login.php' }); + const main = parkedTicketMain(dir, baseOid); + + const narrow = await main.invoke('git:worktree-dirty', dir); + assert.equal(narrow.dirty, false, 'nothing is uncommitted — the guards keep their answer'); + + const preview = await main.invoke('git:preview-patch', dir, LOGIN_DIFF); + assert.equal(preview.ok, true); + assert.deepEqual(preview.conflicts, ['src/wp-login.php']); +}); + +// The other direction: a file edited back to what the base holds carries none +// of the contributor's work, whatever HEAD says about it. Announcing it sends +// someone looking for changes they never made. +test('git:preview-patch does not warn about a file that matches the base (#301)', async (t) => { + const { dir, baseOid, workFile, baseText } = await parkedTicketRepo(t, { workFile: 'src/wp-login.php' }); + // Differs from HEAD (the parked commit), identical to the branch point. + fs.writeFileSync(path.join(dir, workFile), baseText); + const main = parkedTicketMain(dir, baseOid); + + const narrow = await main.invoke('git:worktree-dirty', dir); + assert.equal(narrow.dirty, true, 'HEAD-relative this file reads as edited'); + + const preview = await main.invoke('git:preview-patch', dir, LOGIN_DIFF); + assert.equal(preview.ok, true); + assert.deepEqual(preview.conflicts, []); +}); + +// And a patch that touches nothing the ticket has worked on still warns about +// nothing — the wider baseline widens what counts as the contributor's work, +// it does not warn about every file in the patch. +test('git:preview-patch stays quiet about files the ticket never touched (#301)', async (t) => { + const { dir, baseOid } = await parkedTicketRepo(t, { workFile: 'src/wp-login.php' }); + const main = parkedTicketMain(dir, baseOid); + + const preview = await main.invoke('git:preview-patch', dir, LOGIN_DIFF.replace(/wp-login\.php/g, 'wp-signup.php')); + assert.equal(preview.ok, true); + assert.deepEqual(preview.paths, ['src/wp-signup.php']); + assert.deepEqual(preview.conflicts, []); +}); + // git:apply-patch reads the store for its guard before delegating, which is the // seam fakeSettingsStore stands in for — so it is a wired handler, not a hole. // It streams, so its result comes back on the :done channel, not the return. From 7ec330c5edaecde557d8186e1857155d94919de9 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Wed, 12 Aug 2026 13:42:07 +0200 Subject: [PATCH 3/7] Name the contributor's own work when a patch fails in a file they edited MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed apply narrated an open pull request from its state alone: written against an older trunk, and here is a button to ask its author for a rebase. That was never checked against the checkout it had just tried to apply to, so a change that would not fit because the contributor's own work sits in the same lines — the ordinary shape of a resumed ticket — sent a newcomer to a stranger to ask for work that would not help. The app applies two-way, without the pull request's base, so it cannot prove which side any single failed region belongs to. It does not need to: it already knows which files this ticket has work in, and #302 makes that measurement base-relative. The preview's collision list is now threaded into describeApplyFailure, and the framing branches on it — own work in the way, a pull request behind trunk, or both said together, since picking one would be guessing again. Closed, merged and already-in-trunk pull requests are untouched, and so are loose patches, which have no author to misblame. Fixes #303. Co-Authored-By: Claude Fable 5 --- src/renderer/apply-conflict.cjs | 102 ++++++++++++++++++--- src/renderer/index.jsx | 8 +- test/apply-conflict.test.cjs | 155 ++++++++++++++++++++++++++++++++ 3 files changed, 252 insertions(+), 13 deletions(-) diff --git a/src/renderer/apply-conflict.cjs b/src/renderer/apply-conflict.cjs index 05d56d3..b62d647 100644 --- a/src/renderer/apply-conflict.cjs +++ b/src/renderer/apply-conflict.cjs @@ -106,6 +106,33 @@ function otherPatchCount({ label, prs = [], attachments = [] } = {}) { + attachments.filter((att) => att.filename !== failedLabel).length; } +// How many paths a sentence will name before it starts counting instead. The +// panel already lists every failing file underneath, so the headline's job is +// the attribution, not the inventory — and a pull request failing in a dozen +// files would otherwise open the notice with a paragraph of paths. Same reason +// `REGION_DETAIL_LIMIT` exists in patch-apply.js. +const PATH_NAME_LIMIT = 3; + +/** + * The files on one side of the failure, named for a sentence. + * + * Deduplicated because a concatenated patch can fail the same file twice — the + * same reason `describeApplyFailure` consumes conflicts one by one instead of + * keying them by path. + * + * @param {Array} rows Conflicts. + * @return {{count: number, text: string}} + */ +function namePaths(rows) { + const paths = [...new Set(rows.map((c) => c.path))]; + if (paths.length <= PATH_NAME_LIMIT) return { count: paths.length, text: paths.join(', ') }; + const rest = paths.length - PATH_NAME_LIMIT; + return { + count: paths.length, + text: `${paths.slice(0, PATH_NAME_LIMIT).join(', ')} and ${rest} more file${rest === 1 ? '' : 's'}` + }; +} + /** * The pull-request framing: whose problem this is, and how big. * @@ -127,11 +154,26 @@ function otherPatchCount({ label, prs = [], attachments = [] } = {}) { * merge would settle — close enough to size the problem, not a claim GitHub * will show the identical number. * - * @param {Array} conflicts - * @param {?string} prState 'open' | 'merged' | 'closed' | null when unknown. + * That same missing base is why `ownWorkPaths` is here (#303). Matching two-way + * cannot prove which side a single failed region belongs to, so the framing used + * to answer from the pull request's state alone: open meant stale, always. On a + * ticket someone has come back to, the ordinary reason a change will not fit is + * the contributor's own work sitting in the same file — and sending them to ask + * a stranger for a rebase that would not help is worse than saying nothing. + * + * The app cannot prove the region, but it knows the file: `ownWorkPaths` is the + * preview's own collision list, the files this ticket has work in measured from + * its base (#301). A file on that list gets the collision named for what it is + * and the ways out that are the contributor's own; a file off it keeps the + * stale-pull-request framing. When both are present the notice says both, since + * choosing one would be guessing again. + * + * @param {Array} conflicts + * @param {?string} prState 'open' | 'merged' | 'closed' | null when unknown. + * @param {string[]} [ownWorkPaths] Files this ticket has its own work in. * @return {{headline: string, advice: string, prButton: ?string}} */ -function prFraming(conflicts, prState) { +function prFraming(conflicts, prState, ownWorkPaths = []) { const failed = conflicts.reduce((sum, c) => sum + c.regions.length, 0); const total = conflicts.reduce((sum, c) => sum + c.total, 0); const allApplied = conflicts.every((c) => c.regions.every((r) => r.status === 'already-applied')); @@ -151,7 +193,10 @@ function prFraming(conflicts, prState) { }; } - const files = conflicts.length; + // Distinct files, not conflict rows: a concatenated patch can fail the same + // file twice, and the sentences below go on to name the files, so counting + // the rows would have the count disagreeing with the list beside it. + const files = new Set(conflicts.map((c) => c.path)).size; const scale = `${failed} of its ${total} change${total === 1 ? '' : 's'}, in ${files} file${files === 1 ? '' : 's'},`; // A closed pull request has no author coming back to it: asking for a @@ -165,9 +210,40 @@ function prFraming(conflicts, prState) { }; } + const own = new Set(ownWorkPaths); + const mine = namePaths(conflicts.filter((c) => own.has(c.path))); + const theirs = namePaths(conflicts.filter((c) => !own.has(c.path))); + const REBASE_IS_THEIRS = 'Bringing it up to date is its author\'s work — a rebase, or merging trunk in. Leaving a comment on the pull request to let them know is a real contribution in itself.'; + // A ticket's changes are cheap to redo and expensive to untangle, so keeping + // a copy and starting clean is a recommended way forward here, not a defeat. + // What must never happen is work going quietly; going on purpose, with the + // copy already saved, is a good outcome. + const YOUR_WORK_WAY_OUT = 'Save a patch of your work first to keep a copy. Then apply this on a ticket that does not have that work, or discard it here once the copy is saved.'; + + // Every failing file is one this ticket has work in, so nothing here is the + // pull request being behind trunk, and there is no rebase to ask anyone for. + // The pull request is still worth opening — reading it is how the + // contributor decides whether to start a clean ticket for it — so the button + // stays and only the ask goes. + if (!theirs.count) { + return { + headline: `This pull request does not fit your checkout: ${scale} would need rework. Your own work is in the way, in ${mine.text}.`, + advice: `This is your work meeting the pull request, not a pull request that has gone stale, so asking its author for a rebase would not help. ${YOUR_WORK_WAY_OUT}`, + prButton: 'Open the pull request' + }; + } + + if (mine.count) { + return { + headline: `This pull request does not fit your checkout: ${scale} would need rework. Your own work is in the way in ${mine.text}. The rest is the pull request being behind trunk: ${theirs.text}.`, + advice: `${REBASE_IS_THEIRS} Your own files are yours to sort out. ${YOUR_WORK_WAY_OUT}`, + prButton: 'Ask its author for a rebase' + }; + } + return { headline: `This pull request was written against an older trunk and no longer fits it: ${scale} would need rework.`, - advice: 'Bringing it up to date is its author\'s work — a rebase, or merging trunk in. Leaving a comment on the pull request to let them know is a real contribution in itself.', + advice: REBASE_IS_THEIRS, prButton: 'Ask its author for a rebase' }; } @@ -190,14 +266,16 @@ function prFraming(conflicts, prState) { * folder, a file to add that already exists) as well as the conflict that could * not be broken down. * - * @param {Object} result The apply payload from main. - * @param {Object} [options] - * @param {number} [options.otherPatchCount] Other patches on this ticket. - * @param {?string} [options.prUrl] The failing patch's pull request. - * @param {?string} [options.prState] Its state, when known. + * @param {Object} result The apply payload from main. + * @param {Object} [options] + * @param {number} [options.otherPatchCount] Other patches on this ticket. + * @param {?string} [options.prUrl] The failing patch's pull request. + * @param {?string} [options.prState] Its state, when known. + * @param {string[]} [options.ownWorkPaths] Files this ticket has work in, from + * the preview's collision list (#303). * @return {?Object} */ -function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, prUrl = null, prState = null } = {}) { +function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, prUrl = null, prState = null, ownWorkPaths = [] } = {}) { if (!result || result.ok) return null; const failures = Array.isArray(result.failures) ? result.failures : []; @@ -237,7 +315,7 @@ function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, pr let prButton = fromPr ? 'Open the pull request' : null; if (conflicts.length) { const framing = fromPr - ? prFraming(conflicts, prState) + ? prFraming(conflicts, prState, ownWorkPaths) : { headline: headlineFor(conflicts), advice: '', prButton: null }; headline = framing.headline; advice = framing.advice; diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index aa3aa48..f96da9c 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -3186,7 +3186,13 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit attachments: patchAttachments }), prUrl: preview?.prUrl || null, - prState: preview?.prState || null + prState: preview?.prState || null, + // The preview's own collision list: the files this ticket has work + // in, measured from its base (#301). Without it an open pull request + // is always narrated as stale, so a failure caused by the + // contributor's own edits sends them to ask a stranger for a rebase + // that would not help (#303). + ownWorkPaths: preview?.conflicts || [] })); finishApply(); return; diff --git a/test/apply-conflict.test.cjs b/test/apply-conflict.test.cjs index eb1b823..8b51f3a 100644 --- a/test/apply-conflict.test.cjs +++ b/test/apply-conflict.test.cjs @@ -350,6 +350,161 @@ test('describeApplyFailure: a pull request already in trunk asks for nothing (is assert.equal(result.advice, ''); }); +// --- whose work is actually in the way (#303) ---------------------------- +// +// The framing above answers from the pull request's state alone, so an open one +// is always "stale, ask for a rebase". On a resumed ticket the usual reason a +// change will not fit is the contributor's own work in the same file, and that +// button asks a stranger for work that would not help. The app cannot prove +// which side a single region belongs to — it matches without the PR's base — but +// it knows which files this ticket has work in, and that is enough. + +test('describeApplyFailure: a failure in the contributor\'s own file is not blamed on the author (issue #303)', () => { + const result = describeApplyFailure({ + ok: false, + failures: [`${FOO} has moved on`], + conflicts: [conflict(FOO, 4, [{ index: 0, line: 7, status: 'moved' }])] + }, { prUrl: PR_URL, prState: 'open', ownWorkPaths: [FOO] }); + + // The culprit is named, and it is not the pull request's age. + assert.match(result.headline, /Your own work is in the way/); + assert.doesNotMatch(result.headline, /older trunk/); + // No rebase to ask for, and no button offering one. The advice says the ask + // would not help; what it never does is hand the work to the author. + assert.doesNotMatch(result.advice, /author's work/); + // The pull request is still reachable — reading it is how the contributor + // decides whether to start a clean ticket for it. Only the ask is gone. + assert.equal(result.prButton, 'Open the pull request'); + assert.equal(result.prUrl, PR_URL); + // What actually moves it forward: a copy, then a clean ticket or a discard. + assert.match(result.advice, /Save a patch of your work/); + assert.match(result.advice, /discard it here/); + // The scale still says how much missed — that decision is unchanged. + assert.match(result.headline, /1 of its 4 changes, in 1 file/); +}); + +test('describeApplyFailure: a failure in files the ticket never touched keeps the stale framing (issue #303)', () => { + const result = describeApplyFailure({ + ok: false, + failures: [`${FOO} has moved on`], + conflicts: [conflict(FOO, 4, [{ index: 0, line: 7, status: 'moved' }])] + }, { prUrl: PR_URL, prState: 'open', ownWorkPaths: [BAR] }); + + assert.match(result.headline, /written against an older trunk/); + assert.doesNotMatch(result.headline, /Your own work/); + assert.match(result.advice, /author's work/); + assert.equal(result.prButton, 'Ask its author for a rebase'); +}); + +test('describeApplyFailure: a failure in both kinds of file says both (issue #303)', () => { + const result = describeApplyFailure({ + ok: false, + failures: [`${FOO} has moved on`, `${BAR} has moved on`], + conflicts: [ + conflict(FOO, 3, [{ index: 0, line: 7, status: 'moved' }]), + conflict(BAR, 2, [{ index: 0, line: 11, status: 'moved' }]) + ] + }, { prUrl: PR_URL, prState: 'open', ownWorkPaths: [FOO] }); + + // Each file is named on the side it belongs to; neither is picked over the + // other, because the app has no way to know which one really failed first. + assert.match(result.headline, new RegExp(`Your own work is in the way in ${FOO}`)); + assert.match(result.headline, new RegExp(`the pull request being behind trunk: ${BAR}`)); + // The rebase ask survives, because there is a file it genuinely applies to. + assert.match(result.advice, /author's work/); + assert.match(result.advice, /Save a patch of your work/); + assert.equal(result.prButton, 'Ask its author for a rebase'); + assert.equal(result.prUrl, PR_URL); +}); + +// The headline names files, so the two things that break a list break it here: +// a patch that fails the same file twice, and a patch that fails more files than +// a sentence can carry. The panel prints every failing file underneath anyway, +// so past a few the headline counts instead of listing. +test('describeApplyFailure: the named files are deduplicated and capped (issue #303)', () => { + const twice = describeApplyFailure({ + ok: false, + failures: [`${FOO} has moved on`, `${FOO} has moved on`], + conflicts: [ + conflict(FOO, 2, [{ index: 0, line: 7, status: 'moved' }]), + conflict(FOO, 2, [{ index: 0, line: 40, status: 'moved' }]) + ] + }, { prUrl: PR_URL, prState: 'open', ownWorkPaths: [FOO] }); + + // A concatenated patch failing the same file twice must not name it twice — + // nor count it twice in the same sentence that goes on to list it. + assert.match(twice.headline, new RegExp(`in ${FOO}\\.$`)); + assert.match(twice.headline, /in 1 file,/); + + const many = ['a', 'b', 'c', 'd', 'e'].map((n) => `src/wp-includes/${n}.php`); + const wide = describeApplyFailure({ + ok: false, + failures: many.map((p) => `${p} has moved on`), + conflicts: many.map((p) => conflict(p, 1, [{ index: 0, line: 1, status: 'moved' }])) + }, { prUrl: PR_URL, prState: 'open', ownWorkPaths: many }); + + assert.match(wide.headline, /a\.php, src\/wp-includes\/b\.php, src\/wp-includes\/c\.php and 2 more files/); + assert.doesNotMatch(wide.headline, /e\.php/); +}); + +// Both sides can hold several files, and the sentence has to read as English +// either way — which is why neither list is followed by a verb agreeing with it. +test('describeApplyFailure: the mixed framing reads with several files on each side (issue #303)', () => { + const BAZ = 'src/wp-includes/baz.php'; + const result = describeApplyFailure({ + ok: false, + failures: [`${FOO} has moved on`, `${BAR} has moved on`, `${BAZ} has moved on`], + conflicts: [ + conflict(FOO, 1, [{ index: 0, line: 7, status: 'moved' }]), + conflict(BAR, 1, [{ index: 0, line: 11, status: 'moved' }]), + conflict(BAZ, 1, [{ index: 0, line: 13, status: 'moved' }]) + ] + }, { prUrl: PR_URL, prState: 'open', ownWorkPaths: [FOO] }); + + assert.match(result.headline, new RegExp(`the pull request being behind trunk: ${BAR}, ${BAZ}\\.$`)); + assert.equal(result.prButton, 'Ask its author for a rebase'); +}); + +test('describeApplyFailure: own work does not change the closed or landed framing (issue #303)', () => { + const closed = describeApplyFailure({ + ok: false, + failures: [`${FOO} has moved on`], + conflicts: [conflict(FOO, 3, [{ index: 0, line: 7, status: 'moved' }])] + }, { prUrl: PR_URL, prState: 'closed', ownWorkPaths: [FOO] }); + + // Nobody is coming back to a closed pull request whatever is in the way, and + // its discussion is still the thing worth reading. + assert.match(closed.headline, /closed and was written against an older trunk/); + assert.equal(closed.prButton, 'See why it was closed'); + + const landed = describeApplyFailure({ + ok: false, + failures: [`${FOO} has moved on`], + conflicts: [conflict(FOO, 2, [ + { index: 0, line: 4, status: 'already-applied' }, + { index: 1, line: 9, status: 'already-applied' } + ])] + }, { prUrl: PR_URL, prState: 'closed', ownWorkPaths: [FOO] }); + + assert.match(landed.headline, /likely committed to core/); + assert.equal(landed.prButton, null); +}); + +// A patch from disk or a Trac attachment has no author to misblame, so the +// own-work list changes nothing there — the full per-region breakdown is still +// the only way out, and it stays. +test('describeApplyFailure: a loose patch is unaffected by the own-work list (issue #303)', () => { + const result = describeApplyFailure({ + ok: false, + failures: [`${FOO} has moved on`], + conflicts: [conflict(FOO, 3, [{ index: 0, line: 7, status: 'moved', lines: ['-x', '+y'] }])] + }, { ownWorkPaths: [FOO] }); + + assert.match(result.headline, /1 of this patch's 3 changes/); + assert.equal(result.advice, ''); + assert.equal(result.items[0].regions.length, 1); +}); + test('describeApplyFailure: a loose patch keeps the full breakdown — there is no author to send to (issue #282)', () => { const result = describeApplyFailure({ ok: false, From 12c045ccca2110ae2faa2229af8c8ad69b687a3e Mon Sep 17 00:00:00 2001 From: JuanMa Date: Wed, 12 Aug 2026 13:48:28 +0200 Subject: [PATCH 4/7] Say when a ticket's base is unknown instead of guessing at it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A ticket's base — the trunk snapshot its branch was born at — is what the patch flow measures against: the contributor's own work, the generated patch, and the files the pre-apply warning names (#301). It was read through a helper that answered with an oid or null, folding four situations into two, both folds silent. A ticket branch with no recorded base got today's `refs/heads/trunk` substituted for it, which an update has very likely moved past the point the branch was really born at. And any throw came back as null, which every caller reads as "this site is on trunk" — sending the measurement back to HEAD, the exact reading #301 exists to remove, reached through the failure path. So the status is the answer: `patchBase` returns {status: trunk|recorded|unrecorded|unreadable, baseOid}, and the callers read the last two as themselves. An unrecorded base still measures against today's trunk, because there is nothing better available, but every surface that shows that answer hedges it — the preview's warning, and the handoff patch's Base line. An unreadable base refuses: no patch, no pull request, no discard, and a preview that fails through the fail-closed path it already had rather than reporting a clean tree. Recorded and trunk sites are untouched, which the tests pin. Fixes #308. Co-Authored-By: Claude Fable 5 --- src/main.js | 187 ++++++++++++++++++------- src/patch-provenance.cjs | 19 ++- src/renderer/index.jsx | 19 ++- src/renderer/ticket-base.cjs | 130 +++++++++++++++++ src/trunk-update.js | 5 +- test/ipc-wiring.test.cjs | 174 ++++++++++++++++++++++- test/patch-provenance.test.cjs | 10 ++ test/ticket-base.test.cjs | 81 +++++++++++ test/trunk-update.integration.test.cjs | 4 +- 9 files changed, 559 insertions(+), 70 deletions(-) create mode 100644 src/renderer/ticket-base.cjs create mode 100644 test/ticket-base.test.cjs diff --git a/src/main.js b/src/main.js index 86641ef..8c68c1b 100644 --- a/src/main.js +++ b/src/main.js @@ -31,6 +31,7 @@ const { normalizeEol } = require('./git-update.cjs'); const { ensureAutocrlf, readTrunkInfo, collectDirtyFiles, discardChanges, discardToBase, updateToLatestTrunk } = require('./trunk-update'); const { applyPatchToDir } = require('./patch-apply'); const { parsePatchFiles, planApply } = require('./patch-plan.cjs'); +const { BASE_STATUS, baseIsApproximate, baseUnreadableMessage } = require('./renderer/ticket-base.cjs'); const { fetchLinkedPrs, fetchPrDiff } = require('./github-prs'); const { getClientId: getGithubClientId, requestDeviceCode, pollForToken, fetchViewer } = require('./github-auth.cjs'); const { openPullRequest, buildPullRequestBody, testMode: githubTestMode } = require('./github-pr.cjs'); @@ -579,9 +580,17 @@ async function collectPullRequestFiles(dir, baseOid = null) { return { baseOid: base, files: entries }; } +// A base the app could not read is a patch it cannot take (#308). Every +// generator below refuses instead of falling back, because the fallback is +// HEAD — and HEAD on a resumed ticket is the contributor's own parked work, so +// the "patch" would come back empty and look like there was nothing to submit. +const NO_BASE_PATCH_ERROR = baseUnreadableMessage('no patch was created'); + ipcMain.handle('git:get-patch', async (_e, sitePath) => { try { - const patch = await createMinimalPatchForDir(sitePath, await patchBaseOid(sitePath)); + const base = await patchBase(sitePath); + if (base.status === BASE_STATUS.UNREADABLE) return { ok: false, error: NO_BASE_PATCH_ERROR }; + const patch = await createMinimalPatchForDir(sitePath, base.baseOid); return { ok: true, patch }; } catch (e) { return { ok: false, error: String(e) }; @@ -589,14 +598,20 @@ ipcMain.handle('git:get-patch', async (_e, sitePath) => { }); ipcMain.handle('git:create-patch', async (_e, sitePath) => { - try { - const patch = await createMinimalPatchForDir(sitePath, await patchBaseOid(sitePath)); + const showPatchWindow = (text) => { const win = new BrowserWindow({ width: 900, height: 700, webPreferences: { contextIsolation: true, nodeIntegration: false } }); - win.loadURL('data:text/html;charset=utf-8,' + encodeURIComponent(buildPatchHtml(patch))); + win.loadURL('data:text/html;charset=utf-8,' + encodeURIComponent(buildPatchHtml(text))); + }; + try { + const base = await patchBase(sitePath); + if (base.status === BASE_STATUS.UNREADABLE) { + showPatchWindow(NO_BASE_PATCH_ERROR); + return { ok: false, error: NO_BASE_PATCH_ERROR }; + } + showPatchWindow(await createMinimalPatchForDir(sitePath, base.baseOid)); return { ok: true }; } catch (e) { - const win = new BrowserWindow({ width: 900, height: 700, webPreferences: { contextIsolation: true, nodeIntegration: false } }); - win.loadURL('data:text/html;charset=utf-8,' + encodeURIComponent(buildPatchHtml('Failed to generate diff: ' + String(e)))); + showPatchWindow('Failed to generate diff: ' + String(e)); return { ok: false, error: String(e) }; } }); @@ -612,8 +627,9 @@ ipcMain.handle('git:create-patch', async (_e, sitePath) => { ipcMain.handle('git:save-patch', async (_e, sitePath, options) => { try { const handoff = Boolean(options && options.handoff); - const baseOid = await patchBaseOid(sitePath); - const patch = await createMinimalPatchForDir(sitePath, baseOid); + const base = await patchBase(sitePath); + if (base.status === BASE_STATUS.UNREADABLE) return { ok: false, error: NO_BASE_PATCH_ERROR }; + const patch = await createMinimalPatchForDir(sitePath, base.baseOid); // The header describes what was diffed, so it is read from the same // recorded state the status handler reports, not asked of the caller: @@ -635,7 +651,7 @@ ipcMain.handle('git:save-patch', async (_e, sitePath, options) => { // forward since (#108). Reading the date off that commit rather // than the site record keeps the two halves of the line // describing the same commit. - ...(await baseProvenance(sitePath, baseOid, meta)), + ...(await baseProvenance(sitePath, base, meta)), generatedAt: new Date().toISOString() }); name = handoffFilename({ handle, ticketId: meta.tracTicket }); @@ -779,7 +795,14 @@ ipcMain.handle('github:open-pr', async (event, sitePath, options = {}) => { let collected; try { - collected = await collectPullRequestFiles(sitePath, await patchBaseOid(sitePath)); + // The base is the pull request's parent commit, so an unreadable one + // cannot be papered over with HEAD: that would open a pull request + // against the contributor's own parked work (#308). + const base = await patchBase(sitePath); + if (base.status === BASE_STATUS.UNREADABLE) { + return { ok: false, reason: 'error', error: baseUnreadableMessage('no pull request was opened'), stage: 'collect' }; + } + collected = await collectPullRequestFiles(sitePath, base.baseOid); } catch (e) { return { ok: false, reason: 'error', error: String(e), stage: 'collect' }; } @@ -866,9 +889,10 @@ async function migrateSiteToBranches(sitePath) { const existing = await listTicketBranches(sitePath); // A branch that already exists was not created by this app, so its fork // point is not on record and cannot be recovered on a depth-1 clone. - // Left null deliberately: patchBaseOid has one documented fallback for - // exactly this, and guessing here would put a second, wrong one in the - // codebase. (`trunkOid` is the *current* tip, not the fork point.) + // Left null deliberately: `patchBase` answers `unrecorded` for exactly + // this and hedges every surface that shows it, where guessing here + // would put a second, silent guess in the codebase. (`trunkOid` is the + // *current* tip, not the fork point.) const baseOid = existing.includes(ref) ? null : (await startTicketBranch(sitePath, m.tracTicket)).baseOid; @@ -1059,30 +1083,46 @@ async function mergeBranchMeta(sitePath, ref, patch) { } /** - * The diff base for whatever is checked out: a ticket branch's recorded branch - * point, or null on trunk — where the patch generator falls back to HEAD, which - * is what it has always diffed against. + * The diff base for whatever is checked out, and how well the app knows it. + * + * Four answers, not two (#308): `trunk` has no branch point and the callers + * fall back to HEAD as they always have; `recorded` is the branch point the app + * wrote down when it created the branch; `unrecorded` is a ticket branch with + * none, measured against today's trunk because there is nothing better and + * qualified as approximate wherever it is shown; `unreadable` is a read that + * failed, which is emphatically not "this site is on trunk" — resolving it to + * that sends every measurement back to HEAD through the failure path. * * @param {string} sitePath + * @return {Promise<{status: string, baseOid: ?string}>} */ -async function patchBaseOid(sitePath) { +async function patchBase(sitePath) { try { // What is checked out decides this, so ask the worktree before the // registry: a site on trunk — every site before #108, and every site // whose owner never linked a ticket — answers without a store read at // all, and takes exactly the path it always took. - let ref = null; - try { ref = await currentBranchName(sitePath); } catch {} - if (!ref || ref === TRUNK) return null; + // + // A *null* branch (a detached HEAD, an empty repository) is trunk's + // answer and always was. A *throw* is not: that is the read failing, + // and it falls through to the catch below. + const ref = await currentBranchName(sitePath); + if (!ref || ref === TRUNK) return { status: BASE_STATUS.TRUNK, baseOid: null }; const recorded = ((await readSiteMeta(sitePath)).branches || {})[ref]; - if (recorded && recorded.baseOid) return recorded.baseOid; - // A ticket branch with no recorded base (registry edited by hand, or a - // branch the user made themselves). The live trunk ref is the closest - // honest answer available. - return await git.resolveRef({ fs, dir: sitePath, ref: 'refs/heads/trunk' }); + if (recorded && recorded.baseOid) return { status: BASE_STATUS.RECORDED, baseOid: recorded.baseOid }; + // A ticket branch with no recorded base (registry edited by hand, a + // site adopted from disk, a branch the contributor made themselves). + // The live trunk ref is the closest thing available — but it is where + // trunk is *now*, which an update has very likely moved past the point + // the branch was born at, so the status says so and every surface that + // shows this answer hedges it. + return { + status: BASE_STATUS.UNRECORDED, + baseOid: await git.resolveRef({ fs, dir: sitePath, ref: 'refs/heads/trunk' }) + }; } catch { - return null; + return { status: BASE_STATUS.UNREADABLE, baseOid: null }; } } @@ -1097,20 +1137,27 @@ async function patchBaseOid(sitePath) { * dropped rather than guessed: a header with no date is honest, one that dates a * commit it is not describing is not. * - * @param {string} dir Site working directory. - * @param {?string} baseOid Base the patch was diffed against, or null on trunk. - * @param {Object} meta The site's stored metadata. - * @return {Promise<{trunkOid: ?string, trunkDate: ?string}>} Header fields. + * A branch with no recorded base is the same argument one step further (#308): + * today's trunk is what the patch was diffed against, so it is what the header + * names, but it is flagged approximate — a mentor reading "Base: trunk @ ..." + * as fact would rebase against a commit this ticket may never have seen. + * + * @param {string} dir Site working directory. + * @param {Object} base A `patchBase` result. + * @param {Object} meta The site's stored metadata. + * @return {Promise<{trunkOid: ?string, trunkDate: ?string, baseApproximate: boolean}>} Header fields. */ -async function baseProvenance(dir, baseOid, meta) { +async function baseProvenance(dir, base, meta) { + const baseApproximate = baseIsApproximate(base.status); + const { baseOid } = base; if (!baseOid || baseOid === meta.trunkOid) { - return { trunkOid: meta.trunkOid, trunkDate: meta.trunkDate }; + return { trunkOid: meta.trunkOid, trunkDate: meta.trunkDate, baseApproximate }; } try { const { commit } = await git.readCommit({ fs, dir, oid: baseOid }); - return { trunkOid: baseOid, trunkDate: new Date(commit.committer.timestamp * 1000).toISOString() }; + return { trunkOid: baseOid, trunkDate: new Date(commit.committer.timestamp * 1000).toISOString(), baseApproximate }; } catch { - return { trunkOid: baseOid, trunkDate: null }; + return { trunkOid: baseOid, trunkDate: null, baseApproximate }; } } @@ -1118,18 +1165,30 @@ async function baseProvenance(dir, baseOid, meta) { * The files standing between where this ticket started and where it is now — * the note's measurement (#239). Same base and same walk as the patch, filtered * to the rows the patch would actually speak about, so the note and the modal - * are two renderings of one answer. On trunk `patchBaseOid` answers null and - * the walk falls back to HEAD, which is what the note has always read there. + * are two renderings of one answer. On trunk `patchBase` answers no oid and the + * walk falls back to HEAD, which is what the note has always read there. + * + * Throws when the base could not be read (#308) rather than measuring against + * HEAD: on a resumed ticket HEAD is the contributor's own parked work, so the + * fallback answers "nothing here" about the very tree this exists to speak for. + * The status rides back with the files so the surfaces that show this answer + * can say how exact it is. * * @param {string} sitePath - * @return {Promise>} Paths, gitignored ones excluded. + * @return {Promise<{baseStatus: string, files: Array}>} Paths, gitignored ones excluded. */ async function collectUnsubmittedFiles(sitePath) { - const baseOid = await patchBaseOid(sitePath); - const { files } = await collectChangedFiles(sitePath, baseOid); - return files - .filter((file) => classifyChangedFile(file).kind !== 'unchanged') - .map((file) => file.path); + const base = await patchBase(sitePath); + if (base.status === BASE_STATUS.UNREADABLE) { + throw new Error(baseUnreadableMessage('your work could not be measured against it')); + } + const { files } = await collectChangedFiles(sitePath, base.baseOid); + return { + baseStatus: base.status, + files: files + .filter((file) => classifyChangedFile(file).kind !== 'unchanged') + .map((file) => file.path) + }; } // Two questions, deliberately two channels (#239). This one asks "are there @@ -1155,7 +1214,10 @@ ipcMain.handle('git:worktree-dirty', async (_e, sitePath) => { // switch — which is exactly the work the note exists to speak about. ipcMain.handle('git:unsubmitted-work', async (_e, sitePath) => { try { - const files = await collectUnsubmittedFiles(sitePath); + // The reply keeps its shape: the card's note counts files and does not + // yet say how exact the measurement is — qualifying it there is a + // follow-up, not something to smuggle in on this channel. + const { files } = await collectUnsubmittedFiles(sitePath); return { ok: true, dirty: files.length > 0, changedCount: files.length, files }; } catch (e) { return { ok: false, error: String(e) }; @@ -1164,22 +1226,34 @@ ipcMain.handle('git:unsubmitted-work', async (_e, sitePath) => { // "Discard all changes" in the review-and-submit modal: throws away everything // the modal shows, which on a ticket branch is measured from the branch point -// (#108/#239) and so includes the parked WIP commit. Rewinds to `patchBaseOid` +// (#108/#239) and so includes the parked WIP commit. Rewinds to `patchBase` // — the same base the diff was taken against — so the modal ends on "No // changes". The branch survives and the ticket stays linked; only its work is -// gone. On trunk (or a branch with no recorded base) there is nothing past -// HEAD to rewind, so it falls back to the uncommitted-only reset. +// gone. On trunk there is nothing past HEAD to rewind, so it falls back to the +// uncommitted-only reset. +// +// A base the app could not read refuses outright (#308). The uncommitted-only +// reset is not a safe stand-in for it: it looks like a discard, leaves the +// parked WIP commit untouched, and the modal would come back still listing the +// work it just promised to throw away. ipcMain.handle('git:discard-to-base', async (_e, sitePath) => { try { - const baseOid = await patchBaseOid(sitePath); - if (baseOid) { - await discardToBase(sitePath, baseOid); + const base = await patchBase(sitePath); + if (base.status === BASE_STATUS.UNREADABLE) { + return { ok: false, error: baseUnreadableMessage('nothing was discarded') }; + } + if (base.baseOid) { + // On an unrecorded base this is today's trunk, which need not be an + // ancestor of the branch — `discardToBase` refuses to rewind onto a + // base that is not one and clears the uncommitted work only, so the + // guess cannot take a commit with it. + await discardToBase(sitePath, base.baseOid); } else { await discardChanges(sitePath); } await writeWorkMeta(sitePath, { appliedPatch: null }); let files = null; - try { files = await collectUnsubmittedFiles(sitePath); } catch {} + try { ({ files } = await collectUnsubmittedFiles(sitePath)); } catch {} return files ? { ok: true, dirty: files.length > 0, changedCount: files.length } : { ok: true }; @@ -1204,7 +1278,7 @@ ipcMain.handle('git:discard-changes', async (_e, sitePath) => { // that fails does not turn a discard that succeeded into an error, the // reply just says less and the next probe fills it in. let files = null; - try { files = await collectUnsubmittedFiles(sitePath); } catch {} + try { ({ files } = await collectUnsubmittedFiles(sitePath)); } catch {} return files ? { ok: true, dirty: files.length > 0, changedCount: files.length } : { ok: true }; @@ -1447,16 +1521,21 @@ ipcMain.handle('git:preview-patch', async (_e, sitePath, patchText) => { const parsed = parsePatchFiles(patchText); if (!parsed.ok) return { ok: false, error: parsed.error }; let dirtyPaths; + let baseStatus; try { - dirtyPaths = await collectUnsubmittedFiles(sitePath); + ({ files: dirtyPaths, baseStatus } = await collectUnsubmittedFiles(sitePath)); } catch (e) { // Failing open here would promise "no collisions" precisely when the - // app could not look — surface the failure instead. + // app could not look — surface the failure instead. An unreadable + // base arrives here as a throw for exactly that reason (#308). logError('git:preview-patch', String(e && e.stack ? e.stack : e)); return { ok: false, error: 'Could not check your work for conflicts, so the preview was not shown.' }; } const plan = planApply({ files: parsed.files, dirtyPaths }); - return { ok: true, ...plan, files: parsed.files.map((f) => ({ kind: f.kind, path: f.path })) }; + // How exact that collision list is rides back with it: on a branch with + // no recorded base it was measured against today's trunk, and the panel + // says so rather than presenting it in the same voice as an exact one. + return { ok: true, ...plan, baseStatus, files: parsed.files.map((f) => ({ kind: f.kind, path: f.path })) }; } catch (e) { return { ok: false, error: String(e) }; } diff --git a/src/patch-provenance.cjs b/src/patch-provenance.cjs index d528cb8..a65464e 100644 --- a/src/patch-provenance.cjs +++ b/src/patch-provenance.cjs @@ -124,15 +124,16 @@ function ticketNumber(ticketId) { * recorded. * * @param {Object} details - * @param {string} [details.handle] WordPress.org handle, already validated. - * @param {string} [details.event] Where it was written — a WordCamp, a meetup. + * @param {string} [details.handle] WordPress.org handle, already validated. + * @param {string} [details.event] Where it was written — a WordCamp, a meetup. * @param {number|string} [details.ticketId] * @param {string} [details.trunkOid] - * @param {string} [details.trunkDate] ISO timestamp of the base commit. - * @param {string} [details.generatedAt] ISO timestamp for "now". + * @param {string} [details.trunkDate] ISO timestamp of the base commit. + * @param {boolean} [details.baseApproximate] The branch point was not recorded (#308). + * @param {string} [details.generatedAt] ISO timestamp for "now". * @return {string} */ -function buildProvenanceHeader({ handle, event, ticketId, trunkOid, trunkDate, generatedAt } = {}) { +function buildProvenanceHeader({ handle, event, ticketId, trunkOid, trunkDate, baseApproximate, generatedAt } = {}) { const lines = []; const contributor = field(handle); @@ -152,7 +153,13 @@ function buildProvenanceHeader({ handle, event, ticketId, trunkOid, trunkDate, g const based = day(trunkDate); if (oid || based) { const base = [oid ? oid.slice(0, SHORT_OID_LENGTH) : null, based].filter(Boolean).join(', '); - lines.push(`# Base: trunk @ ${base}`); + // A base the app worked out rather than recorded is still worth naming — + // a mentor with no commit at all has nothing to apply this against — but + // it is named as the guess it is (#308). Read as fact, it would send + // someone rebasing onto a commit this ticket may never have been on. + lines.push(baseApproximate + ? `# Base: trunk @ ${base} (approximate — this ticket's starting point was not recorded)` + : `# Base: trunk @ ${base}`); } const generated = day(generatedAt); diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index f96da9c..159029f 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -30,6 +30,7 @@ import { pathBasename } from './path-basename.cjs'; import { sanitizeSiteFolder, resolveTargetDir, directoryFromFileEntry } from './site-folder.cjs'; import { noticeForOpenResult } from './open-failure.cjs'; import { describeApplyFailure, otherPatchCount } from './apply-conflict.cjs'; +import { describeOwnWorkWarning } from './ticket-base.cjs'; import { trunkAgeInfo, planUpdateSteps, updateStepStatuses, SKIP_INSTALL_MESSAGE, planApplySteps, planWatchImpact, APPLY_STATE_TO_STEP, planSetupSteps, SETUP_STATE_TO_STEP, setupOutcome } from './update-plan.cjs'; import { pickLatest } from '../latest-patch.cjs'; import { beginSetup, adoptSetupPath, discardSetup, rowPathAfterStatus } from './pending-setup.cjs'; @@ -99,6 +100,15 @@ const UPDATE_STEP_MARKS = { complete: { symbol: '✓', color: '#0f5132' }, current: { symbol: '›', color: '#0b5d95' } }; +// How the preview renders what it has to say about the contributor's own work +// (#308). `warning` is the amber block that has always been there — a patch is +// about to land on files someone edited. `note` is the quieter case: nothing +// collided, but the base it was measured against was approximate, so the +// silence needs a sentence rather than an alert. +const OWN_WORK_NOTICE_STYLES = { + warning: { role: 'alert', style: { marginTop: 10, padding: '8px 10px', background: '#fcf9e8', border: '1px solid #dba617', borderRadius: 6, fontSize: 12, color: '#6e5406' } }, + note: { role: undefined, style: { marginTop: 10, fontSize: 12, color: '#6c6f72' } } +}; // The file manager has a name on the two platforms that have one; everywhere // else it is whatever the desktop provides, so it is called what it is. // @@ -2728,6 +2738,9 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit }); const applySteps = planApplySteps({ needsInstall: applyNeedsInstall, buildByWatcher: applyBuildByWatcher }); const applyStepStates = updateStepStatuses(applySteps, applyState, APPLY_STATE_TO_STEP); + // What the preview says about the contributor's own work, and how confidently + // — the preview carries the status of the base it was measured against (#308). + const applyOwnWorkNotice = applyPreview ? describeOwnWorkWarning(applyPreview) : null; // --- Initial setup, as one chain (#246) --- // The third chain, and the only one nobody starts: between the clone, the @@ -4725,9 +4738,9 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit
{applyPreview.paths.map((p) =>
{p}
)}
- {applyPreview.conflicts.length ? ( -
- You have your own edits to {applyPreview.conflicts.join(', ')}. The patch is applied on top of them: it succeeds if the changes do not overlap, and fails without touching anything if they do. Save a patch of your work first if you want a copy. + {applyOwnWorkNotice ? ( +
+ {applyOwnWorkNotice.text}
) : null} {applyPreview.unsupported.length ? ( diff --git a/src/renderer/ticket-base.cjs b/src/renderer/ticket-base.cjs new file mode 100644 index 0000000..4442c37 --- /dev/null +++ b/src/renderer/ticket-base.cjs @@ -0,0 +1,130 @@ +// How much the app actually knows about the trunk a ticket started from, and +// what it is allowed to say once it knows that (#308). +// +// A ticket's base is the measurement everything in the patch flow rests on: +// what counts as the contributor's own work, what a generated patch contains, +// which files the pre-apply warning names (#301). It used to come back as an +// oid or `null`, which folded four situations into two answers — and both of +// the folds were silent: +// +// - a branch with no recorded base got today's `trunk` substituted for it, and +// "Update to latest trunk" has very likely moved that past where the branch +// was really born, so the ticket was measured against a base it never had; +// - anything that threw while reading it came back as `null`, which every +// caller reads as "this site is on trunk" — sending the measurement back to +// HEAD, the exact reading #301 exists to remove. +// +// So the status is the answer, and the oid rides along with it. `unrecorded` +// still measures against today's trunk, because there is nothing better to +// measure against, but wherever that answer reaches the contributor it is +// qualified rather than stated. `unreadable` measures nothing at all. +// +// Pure and dependency-free like open-failure.cjs, and for the same reasons: the +// renderer bundle imports it, `node --test` requires it directly, the main +// process requires it for the statuses and the failure copy, and none of the +// three needs a DOM. +'use strict'; + +/** + * The four answers to "where did this ticket start". + * + * - `trunk` — nothing is checked out but trunk, so there is no branch + * point; callers fall back to HEAD, as they always have. + * - `recorded` — the app wrote the branch point down when it created the + * branch. This is the healthy path and the only exact one. + * - `unrecorded` — a ticket branch with no recorded base: a registry edited by + * hand, a site adopted from disk, a branch the contributor + * made in their own git client. Measured against today's + * trunk, and said to be approximate. + * - `unreadable` — the read itself failed. Not the same as "on trunk", and it + * must not resolve to it. + */ +const BASE_STATUS = { + TRUNK: 'trunk', + RECORDED: 'recorded', + UNRECORDED: 'unrecorded', + UNREADABLE: 'unreadable' +}; + +/** + * Whether a base is exact enough to speak about without hedging. Trunk counts: + * there is no ticket to be wrong about. + * + * @param {?string} status A `BASE_STATUS` value. + * @return {boolean} + */ +function baseIsApproximate(status) { + return status === BASE_STATUS.UNRECORDED; +} + +/** + * What the app says when it could not read the base at all. + * + * One shape, several consequences, because the consequence is the half that + * differs per surface and the half the contributor acts on: a preview that was + * not shown is a different next step from a patch that was not created. The + * failure is honest rather than fail-open — measuring against HEAD instead + * would answer the wrong question in the same voice as the right one. + * + * It says "which trunk", not "which trunk this ticket started from": the read + * that failed can be the branch itself, so this is also what a site with no + * ticket at all says when its folder has been moved or deleted, and naming a + * ticket there would point at something the contributor never linked. + * + * @param {string} consequence What did not happen, as a clause. + * @return {string} + */ +function baseUnreadableMessage(consequence) { + return `Could not work out which trunk to compare your work against, so ${consequence}.`; +} + +// The warning as it has always read on a site with a recorded base. Kept whole +// and unhedged: that path is exact, and softening it would teach contributors +// to skim the one warning that is never wrong. +function recordedWarning(conflicts) { + return `You have your own edits to ${conflicts.join(', ')}. The patch is applied on top of them: it succeeds if the changes do not overlap, and fails without touching anything if they do. Save a patch of your work first if you want a copy.`; +} + +// The same warning with its uncertainty said out loud. The hedge comes first +// because it changes what the list means: these are files that differ from +// today's trunk, which on a branch born before the last update includes files +// trunk itself moved on, not the contributor. +function unrecordedWarning(conflicts) { + return `${conflicts.join(', ')} may hold your own edits. This ticket has no record of the trunk it started from, so your work was compared against today's trunk instead — that can name files you never touched, and miss ones you did. The patch is applied on top of whatever is there: it succeeds if the changes do not overlap, and fails without touching anything if they do. Save a patch of your work first if you want a copy.`; +} + +// Nothing collided, but on an approximate base "nothing collided" is not a +// promise the app can make. Said quietly rather than as an alert: there is no +// problem here yet, only a check that was less than exact. +const UNRECORDED_CLEAR_NOTE = 'This ticket has no record of the trunk it started from, so the check for your own edits was made against today\'s trunk and is approximate.'; + +/** + * What the apply preview says about the contributor's own work, or null when + * there is nothing to say. + * + * `level` picks the styling and whether it is announced: `warning` is the amber + * block that has always been there, `note` is a quiet line that exists only to + * stop an approximate silence from reading as a clean bill of health. + * + * @param {Object} [root0] + * @param {string[]} [root0.conflicts] Files the patch touches that the ticket has work in. + * @param {?string} [root0.baseStatus] A `BASE_STATUS` value; anything else is treated as exact. + * @return {?{level: 'warning'|'note', text: string}} + */ +function describeOwnWorkWarning({ conflicts = [], baseStatus = BASE_STATUS.RECORDED } = {}) { + const files = Array.isArray(conflicts) ? conflicts.filter(Boolean) : []; + if (!baseIsApproximate(baseStatus)) { + return files.length ? { level: 'warning', text: recordedWarning(files) } : null; + } + return files.length + ? { level: 'warning', text: unrecordedWarning(files) } + : { level: 'note', text: UNRECORDED_CLEAR_NOTE }; +} + +module.exports = { + BASE_STATUS, + UNRECORDED_CLEAR_NOTE, + baseIsApproximate, + baseUnreadableMessage, + describeOwnWorkWarning +}; diff --git a/src/trunk-update.js b/src/trunk-update.js index 1dbe4cc..0b1663f 100644 --- a/src/trunk-update.js +++ b/src/trunk-update.js @@ -169,8 +169,9 @@ async function discardToBase(dir, baseOid) { } const ref = (await git.currentBranch({ fs, dir, fullname: false })) || 'trunk'; // Only rewind the ref when baseOid really is this branch's own history — its - // point off trunk. patchBaseOid falls back to the live `trunk` tip for a - // branch with no recorded base (hand-created, or a hand-edited registry); + // point off trunk. `patchBase` in main.js answers `unrecorded` for a branch + // with no recorded base (hand-created, or a hand-edited registry) and hands + // over the live `trunk` tip with it; // that tip need not be an ancestor of HEAD, and moving the ref onto it would // orphan the branch's commits and re-point it at unrelated work. When it is // not an ancestor, leave the ref alone and just reset the worktree to HEAD — diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index dbdfc31..3e75be9 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -31,6 +31,10 @@ const os = require('node:os'); const path = require('node:path'); const { EventEmitter } = require('node:events'); const git = require('isomorphic-git'); +// The one module these tests read an answer *through* rather than around: what +// the preview's base status means for the contributor is the module's to say, +// and #308's point is that the two halves agree. +const { describeOwnWorkWarning } = require('../src/renderer/ticket-base.cjs'); const SRC_DIR = path.join(__dirname, '..', 'src'); const MAIN_PATH = path.join(SRC_DIR, 'main.js'); @@ -989,7 +993,12 @@ test('a tree whose only change is binary still reports no changes (#85)', async // The other two entry points into the same patch path. They differ only in what // they do with the result — a window, or a save dialog — so what is checked here // is that they go through it at all rather than assembling a diff of their own. -test('git:create-patch and git:save-patch generate the patch the same way', async () => { +// A real repository rather than a path that is not one: reading the ticket's +// base is the handler's first step (#308), and a directory that is not a +// repository now stops there with "the base could not be read" instead of +// reaching the patch path this test is about. +test('git:create-patch and git:save-patch generate the patch the same way', async (t) => { + const dir = await fixtureRepo(t); for (const channel of ['git:create-patch', 'git:save-patch']) { // Throwing ends the handler at its first delegation — which is also what // keeps this test off the network, since the next step fetches @@ -997,9 +1006,9 @@ test('git:create-patch and git:save-patch generate the patch the same way', asyn const ensureAutocrlf = spy(async () => { throw new Error('not a repository'); }); const main = loadMain({ stubs: { ...silentLogging(), './trunk-update': { ensureAutocrlf } } }); - await main.invoke(channel, '/sites/wp'); + await main.invoke(channel, dir); - assert.deepEqual(ensureAutocrlf.calls, [['/sites/wp']], channel); + assert.deepEqual(ensureAutocrlf.calls, [[dir]], channel); } }); @@ -1821,6 +1830,165 @@ test('git:preview-patch stays quiet about files the ticket never touched (#301)' assert.deepEqual(preview.conflicts, []); }); +// --- what the app says when it does not know the base (#308) -------------- + +// The same repo the tests above use, with trunk moved on after the branch was +// born — the shape "Update to latest trunk" leaves, and the reason substituting +// today's trunk for an unrecorded base is a guess rather than a reading. The +// branch does not carry `src/wp-signup.php`, so measured against today's trunk +// it looks like the contributor deleted a file they have never opened. +async function movedOnTrunkRepo(t) { + const repo = await parkedTicketRepo(t, { workFile: 'src/wp-login.php' }); + const { dir } = repo; + const author = { name: 'test', email: 'test@example.com' }; + await git.checkout({ fs, dir, ref: 'trunk' }); + fs.writeFileSync(path.join(dir, 'src', 'wp-signup.php'), ' { + const { dir, baseOid } = await parkedTicketRepo(t, { workFile: 'src/wp-login.php' }); + const main = parkedTicketMain(dir, baseOid); + + const preview = await main.invoke('git:preview-patch', dir, LOGIN_DIFF); + assert.equal(preview.ok, true); + assert.equal(preview.baseStatus, 'recorded'); + assert.deepEqual(preview.conflicts, ['src/wp-login.php']); +}); + +// A site on trunk keeps its own answer: there is no ticket to be unsure about, +// and the walk falls back to HEAD as it always has. +test('git:preview-patch reports a site on trunk as on trunk (#308)', async (t) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ipc-wiring-base-trunk-')); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + await git.init({ fs, dir, defaultBranch: 'trunk' }); + fs.mkdirSync(path.join(dir, 'src')); + fs.writeFileSync(path.join(dir, 'src', 'wp-login.php'), ' { + const { dir } = await movedOnTrunkRepo(t); + const main = parkedTicketMain(dir, null); + + const preview = await main.invoke('git:preview-patch', dir, SIGNUP_DIFF); + + assert.equal(preview.ok, true, 'an unknown base still measures — there is nothing better available'); + assert.equal(preview.baseStatus, 'unrecorded'); + assert.deepEqual(preview.conflicts, ['src/wp-signup.php']); + // And that is exactly the announcement the contributor did not earn, which + // is why the panel hedges it rather than stating it. + const notice = describeOwnWorkWarning(preview); + assert.equal(notice.level, 'warning'); + assert.ok(notice.text.startsWith('src/wp-signup.php may hold your own edits.')); + assert.ok(notice.text.includes('no record of the trunk it started from')); +}); + +// The contributor's real work is still found on an unrecorded base — hedging +// the answer must not mean withholding it. +test('git:preview-patch on an unrecorded base still names the contributor\'s own work (#308)', async (t) => { + const { dir } = await movedOnTrunkRepo(t); + const main = parkedTicketMain(dir, null); + + const preview = await main.invoke('git:preview-patch', dir, LOGIN_DIFF); + assert.equal(preview.ok, true); + assert.deepEqual(preview.conflicts, ['src/wp-login.php']); +}); + +// A repo whose `trunk` ref is gone: the base cannot be recorded and cannot be +// worked out either. Before #308 this came back as null, which every caller +// reads as "this site is on trunk" — so the walk fell back to HEAD, a resumed +// ticket matches HEAD exactly, and the preview reported a clean tree over a +// morning's work. It has to fail instead. +async function unreadableBaseRepo(t) { + const repo = await parkedTicketRepo(t, { workFile: 'src/wp-login.php' }); + await git.deleteBranch({ fs, dir: repo.dir, ref: 'trunk' }); + return repo; +} + +test('git:preview-patch fails honestly when the base cannot be read (#308)', async (t) => { + const { dir } = await unreadableBaseRepo(t); + const main = parkedTicketMain(dir, null); + + const preview = await main.invoke('git:preview-patch', dir, LOGIN_DIFF); + + assert.equal(preview.ok, false, 'an unreadable base must not read as a clean tree'); + assert.match(preview.error, /could not/i); + assert.equal(preview.conflicts, undefined); +}); + +// The same failure on the question the card asks. Answering "nothing here" +// would be the #301 silence again, reached through the failure path. +test('git:unsubmitted-work fails rather than reporting a clean tree it could not measure (#308)', async (t) => { + const { dir } = await unreadableBaseRepo(t); + const main = parkedTicketMain(dir, null); + + const res = await main.invoke('git:unsubmitted-work', dir); + + assert.equal(res.ok, false); + assert.match(res.error, /which trunk to compare your work against/); +}); + +// And on the destructive one. The uncommitted-only reset is not a safe stand-in +// for a discard to base: it looks like it worked, leaves the parked WIP commit +// where it was, and the modal comes back listing the work it just promised to +// throw away. +test('git:discard-to-base refuses rather than half-discarding on an unreadable base (#308)', async (t) => { + const { dir, workFile } = await unreadableBaseRepo(t); + fs.writeFileSync(path.join(dir, 'loose.php'), ' { + const { dir } = await unreadableBaseRepo(t); + const main = parkedTicketMain(dir, null); + + const res = await main.invoke('git:get-patch', dir); + + assert.equal(res.ok, false); + assert.match(res.error, /which trunk to compare your work against/); + assert.equal(res.patch, undefined); +}); + // git:apply-patch reads the store for its guard before delegating, which is the // seam fakeSettingsStore stands in for — so it is a wired handler, not a hole. // It streams, so its result comes back on the :done channel, not the return. diff --git a/test/patch-provenance.test.cjs b/test/patch-provenance.test.cjs index 4c25e9f..98a95ce 100644 --- a/test/patch-provenance.test.cjs +++ b/test/patch-provenance.test.cjs @@ -58,6 +58,16 @@ test('buildProvenanceHeader: a base with only one half of it still says that hal assert.ok(buildProvenanceHeader({ trunkDate: FULL.trunkDate }).includes('# Base: trunk @ 2026-08-05\n')); }); +// A base the app worked out rather than recorded is still the commit the patch +// was diffed against, so it is named — but a mentor reading it as fact would +// rebase onto a commit this ticket may never have been on (issue #308). +test('buildProvenanceHeader: an approximate base says so on its own line (issue #308)', () => { + const header = buildProvenanceHeader({ ...FULL, baseApproximate: true }); + assert.ok(header.includes('# Base: trunk @ 59a1c3e, 2026-08-05 (approximate — this ticket\'s starting point was not recorded)\n')); + // The recorded path is untouched: no hedge appears where the base is exact. + assert.ok(!buildProvenanceHeader(FULL).includes('approximate')); +}); + test('buildProvenanceHeader: nothing to say produces no header at all (issue #166)', () => { assert.strictEqual(buildProvenanceHeader(), ''); assert.strictEqual(buildProvenanceHeader({}), ''); diff --git a/test/ticket-base.test.cjs b/test/ticket-base.test.cjs new file mode 100644 index 0000000..f674d88 --- /dev/null +++ b/test/ticket-base.test.cjs @@ -0,0 +1,81 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert'); +const { + BASE_STATUS, + UNRECORDED_CLEAR_NOTE, + baseIsApproximate, + baseUnreadableMessage, + describeOwnWorkWarning +} = require('../src/renderer/ticket-base.cjs'); + +// The healthy path, and the one that must not move: a recorded base is exact, +// so the warning reads exactly as it always has (issue #308). +test('describeOwnWorkWarning: a recorded base warns without hedging (issue #308)', () => { + const notice = describeOwnWorkWarning({ + conflicts: ['src/wp-login.php'], + baseStatus: BASE_STATUS.RECORDED + }); + assert.strictEqual(notice.level, 'warning'); + assert.strictEqual( + notice.text, + 'You have your own edits to src/wp-login.php. The patch is applied on top of them: it succeeds if the changes do not overlap, and fails without touching anything if they do. Save a patch of your work first if you want a copy.' + ); + assert.ok(!notice.text.includes('approximate')); + assert.ok(!notice.text.includes('may hold')); +}); + +test('describeOwnWorkWarning: an exact base with no collisions says nothing (issue #308)', () => { + assert.strictEqual(describeOwnWorkWarning({ conflicts: [], baseStatus: BASE_STATUS.RECORDED }), null); + assert.strictEqual(describeOwnWorkWarning({ conflicts: [], baseStatus: BASE_STATUS.TRUNK }), null); + // No status at all is the pre-#308 shape of the payload, and is treated as + // exact: a preview from an older main process must not start hedging. + assert.strictEqual(describeOwnWorkWarning({ conflicts: [] }), null); + assert.strictEqual(describeOwnWorkWarning(), null); +}); + +// The list is still shown — it is the best the app has — but as a list of +// candidates, not of facts. Measured against today's trunk, it can name files +// trunk moved on rather than the contributor. +test('describeOwnWorkWarning: an unrecorded base qualifies the warning (issue #308)', () => { + const notice = describeOwnWorkWarning({ + conflicts: ['src/wp-login.php', 'src/wp-signup.php'], + baseStatus: BASE_STATUS.UNRECORDED + }); + assert.strictEqual(notice.level, 'warning'); + assert.ok(notice.text.startsWith('src/wp-login.php, src/wp-signup.php may hold your own edits.')); + assert.ok(notice.text.includes('no record of the trunk it started from')); + assert.ok(notice.text.includes('today\'s trunk')); + // The advice that follows is unchanged — the hedge qualifies the list, not + // what applying the patch does. + assert.ok(notice.text.includes('fails without touching anything if they do')); +}); + +// Silence is an answer too, and on an approximate base it is not one the app +// can stand behind: a file the contributor did edit can be missing from a list +// measured against the wrong trunk. +test('describeOwnWorkWarning: an unrecorded base with no collisions is not a clean bill of health (issue #308)', () => { + const notice = describeOwnWorkWarning({ conflicts: [], baseStatus: BASE_STATUS.UNRECORDED }); + assert.strictEqual(notice.level, 'note'); + assert.strictEqual(notice.text, UNRECORDED_CLEAR_NOTE); + assert.ok(notice.text.includes('approximate')); +}); + +test('baseIsApproximate: only an unrecorded base is (issue #308)', () => { + assert.strictEqual(baseIsApproximate(BASE_STATUS.UNRECORDED), true); + assert.strictEqual(baseIsApproximate(BASE_STATUS.RECORDED), false); + assert.strictEqual(baseIsApproximate(BASE_STATUS.TRUNK), false); + assert.strictEqual(baseIsApproximate(BASE_STATUS.UNREADABLE), false); + assert.strictEqual(baseIsApproximate(undefined), false); +}); + +// One shape, several consequences: what did not happen is the half the +// contributor acts on, and it differs per surface. +test('baseUnreadableMessage: names the failure and its consequence (issue #308)', () => { + assert.strictEqual( + baseUnreadableMessage('no patch was created'), + 'Could not work out which trunk to compare your work against, so no patch was created.' + ); + assert.ok(baseUnreadableMessage('nothing was discarded').endsWith('so nothing was discarded.')); +}); diff --git a/test/trunk-update.integration.test.cjs b/test/trunk-update.integration.test.cjs index 215ea9b..90fafb3 100644 --- a/test/trunk-update.integration.test.cjs +++ b/test/trunk-update.integration.test.cjs @@ -99,8 +99,8 @@ test('discardToBase: a base that is not an ancestor of HEAD does not rewind the await git.add({ fs, dir, filepath: 'text.txt' }); const wip = await git.commit({ fs, dir, message: 'WIP', author: AUTHOR, parent: [trunkStart] }); // Trunk advances to a commit that is a sibling of the ticket HEAD, not an - // ancestor — the shape patchBaseOid's fallback would hand a branch with no - // recorded base once trunk has moved on. + // ancestor — the shape `patchBase` hands over for a branch with no recorded + // base once trunk has moved on. await git.checkout({ fs, dir, ref: 'trunk' }); fs.writeFileSync(path.join(dir, 'other.txt'), 'unrelated trunk work\n'); await git.add({ fs, dir, filepath: 'other.txt' }); From f449bd8426870de177b9934033773a6a087a8683 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Wed, 12 Aug 2026 13:48:54 +0200 Subject: [PATCH 5/7] Treat an applied patch as a named layer, not an undo blob MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A ticket is a branch (#108): trunk, at most one applied patch or pull request, and the contributor's own edits. The branch holds that faithfully; the app's bookkeeping did not. `appliedPatch` was remembered only as something to undo, so everything the UI said about the ticket was read off the bare diff — and every file the applied pull request brought was announced as the contributor's own writing. Attribution. The record already stores which paths the patch touched, so the pre-apply warning now names two owners instead of one: a file the layer brought and nobody has edited over is described as coming from it, and a file the contributor has also worked on keeps naming their work, which is the answer that decides what they do next. Neither is dropped from the warning. The same split reaches the failure narration: a revert that will not come back out is the contributor's own edits sitting on lines they applied, never a pull request its author should rebase. Absorption. `revertable` used to mean "we stored the text". It now means what the banner actually asks — can this layer still be lifted out of this checkout, right now — measured by `diagnoseRemoval`, which reverses the stored patch through the same `resolveFile`/`diagnoseHunks` machinery the apply path uses and writes nothing. Once the contributor's edits are on the patch's own lines it is absorbed: Revert stands down, and the honest exits are saving a copy of the work and discarding the ticket to its base, which on this project is a normal way forward rather than a defeat. Nothing persists an "absorbed" flag, so undoing the overlapping edit brings Revert back on its own — and absorption never frees the one-patch slot, because the record survives as provenance. The banner's decision lives in `src/renderer/applied-layer.cjs`, unit tested directly; the patch text still never crosses IPC. Fixes #306 Co-Authored-By: Claude Fable 5 --- src/main.js | 33 +++- src/patch-apply.js | 80 +++++++- src/renderer/applied-layer.cjs | 203 ++++++++++++++++++++ src/renderer/apply-conflict.cjs | 76 ++++++-- src/renderer/index.jsx | 124 +++++++++---- test/applied-layer.test.cjs | 258 ++++++++++++++++++++++++++ test/ipc-wiring.test.cjs | 146 ++++++++++++++- test/patch-apply.integration.test.cjs | 97 +++++++++- 8 files changed, 965 insertions(+), 52 deletions(-) create mode 100644 src/renderer/applied-layer.cjs create mode 100644 test/applied-layer.test.cjs diff --git a/src/main.js b/src/main.js index 8c68c1b..1dacbfc 100644 --- a/src/main.js +++ b/src/main.js @@ -29,7 +29,7 @@ const { buildMenuTemplate } = require('./menu'); const { killChildTree } = require('./kill-tree'); const { normalizeEol } = require('./git-update.cjs'); const { ensureAutocrlf, readTrunkInfo, collectDirtyFiles, discardChanges, discardToBase, updateToLatestTrunk } = require('./trunk-update'); -const { applyPatchToDir } = require('./patch-apply'); +const { applyPatchToDir, diagnoseRemoval } = require('./patch-apply'); const { parsePatchFiles, planApply } = require('./patch-plan.cjs'); const { BASE_STATUS, baseIsApproximate, baseUnreadableMessage } = require('./renderer/ticket-base.cjs'); const { fetchLinkedPrs, fetchPrDiff } = require('./github-prs'); @@ -1753,12 +1753,41 @@ ipcMain.handle('site:status', async (_e, sitePath) => { // Summarised rather than passed through: the stored patch text is only // needed by the main process to reverse it, and this is polled. + // + // `revertable` used to mean "we kept the text". It now means what the + // banner actually asks — can this layer still be lifted out, right now + // (#306) — which is a fact about the tree and has to be measured here, + // because the text does not cross IPC and the renderer cannot ask. + // The measurement is in-memory: it reads the patch's own files (one to a + // handful, on a call that already stats the checkout and reads a git + // object) and matches hunks against them. Nothing is written. It is + // bounded twice over — a patch bigger than REVERTABLE_PATCH_LIMIT has no + // stored text to check, and this handler is called on mount and after + // long operations, not on a timer. The costly shape is a large absorbed + // patch, which pays its per-hunk diagnosis on every one of those reads; + // if that is ever felt, a ceiling belongs in diagnoseRemoval, not here. + // + // Guarded, and it fails towards offering Revert: a check that could not + // run must not hide the exit. Pressing Revert then gives the real answer + // from the apply path, with its own explanation. + let absorbed = []; + if (work.appliedPatch && work.appliedPatch.text) { + try { + absorbed = diagnoseRemoval({ dir: sitePath, patchText: work.appliedPatch.text }).absorbed; + } catch (e) { + logError('site:status', `could not check whether ${describeRefused(work.appliedPatch.label)} still comes out of ${describeRefused(sitePath)}: ${String(e && e.stack ? e.stack : e)}`); + } + } const appliedPatch = work.appliedPatch ? { label: work.appliedPatch.label, appliedAt: work.appliedPatch.appliedAt, files: work.appliedPatch.files || [], - revertable: Boolean(work.appliedPatch.text) + // Whether a text was kept at all — the separate reason a patch + // cannot be reverted, and a different sentence from absorption. + kept: Boolean(work.appliedPatch.text), + absorbed, + revertable: Boolean(work.appliedPatch.text) && absorbed.length === 0 } : null; diff --git a/src/patch-apply.js b/src/patch-apply.js index e222a1a..8e74bee 100644 --- a/src/patch-apply.js +++ b/src/patch-apply.js @@ -377,6 +377,74 @@ function patchIsAbsent(dir, files) { return text.every((f) => !resolveFile(dir, f, { diagnose: false }).error); } +/** + * Whether the patch the app applied could still be taken back out of this + * checkout, asked right now rather than remembered (#306). + * + * The record the app keeps says only that a patch text was stored. That is not + * the question the banner needs answering: once the contributor's own edits sit + * on the patch's lines it has been **absorbed** — it has become their changes, + * and no revert can separate the two again. Undoing those edits makes it + * removable again, which is why this is measured on demand and never tracked as + * a flag. + * + * Nothing here writes. `resolveFile` reads each file and works out what a + * reverse *would* produce, and `diagnoseHunks` behind it says how much of the + * patch the file no longer holds — the same machinery the apply path uses, so + * there is no second way to ask whether a patch still fits. The resolved + * contents are discarded. + * + * A patch that is simply gone — a trunk update or a discard reset the tree — is + * not absorbed, and gets the same two-part guard the revert itself uses: not one + * file reversed, *and* the whole patch resolves forwards. Its record is stale, + * and Revert stays offered because pressing it is what clears it. + * + * @param {Object} root0 + * @param {string} root0.dir Site working directory. + * @param {string} root0.patchText The stored patch, forward as it was applied. + * @return {{absorbed: Array<{path: string, editedOver: boolean, failed: number, total: number}>, missing: boolean, error?: string}} + */ +function diagnoseRemoval({ dir, patchText }) { + const parsed = parsePatchFiles(String(patchText || '')); + if (!parsed.ok) return { absorbed: [], missing: false, error: parsed.error }; + + const absorbed = []; + let intact = 0; + for (const file of parsed.files) { + // Binary files were never applied, so they cannot be in the way of a + // removal either — the apply path skips them the same way. + if (file.kind === 'binary') continue; + const reversed = reverseFile(file); + const resolved = resolveFile(dir, reversed); + if (!resolved.error) { + intact += 1; + continue; + } + const conflict = resolved.conflict; + absorbed.push({ + // The forward path, not the reversed one: a rename's reverse names the + // file it moves *back* to, while the record of what was applied holds + // the name the patch moved it to. Keyed on the reversed name, the + // caller's attribution could never match its own record. + path: file.path, + // Not every refusal is an edit over the patch's lines: a file the + // contributor deleted outright, or one a reversed add can no longer + // find, blocks the removal without anything having been written over. + // Withholding Revert is right either way — the revert is all or + // nothing — but the sentence about it is not, so the two are told + // apart here rather than guessed at by the caller. + editedOver: Boolean(conflict), + failed: conflict ? conflict.regions.length : 0, + total: conflict ? conflict.total : 0 + }); + } + + if (absorbed.length && !intact && patchIsAbsent(dir, parsed.files)) { + return { absorbed: [], missing: true }; + } + return { absorbed, missing: false }; +} + /** * Puts back everything a failed run had already written. * @@ -450,10 +518,12 @@ async function applyPatchToDir({ dir, patchText, reverse = false, onLog = () => skipped.push(file.path); continue; } - // No diagnosis on a reverse: the panel discards it — the patch being - // reverted came through this app, and the ticket's other patches are no - // answer to a revert that failed. - const resolved = resolveFile(dir, file, { diagnose: !reverse }); + // Diagnosed on a reverse too (#306). The ticket's other patches are still + // no answer to a revert that failed, but the *reason* it failed is: a + // revert only fails because the contributor's own edits are on the + // patch's lines, and naming how many of them, and where, is the whole + // difference between an explanation and a generic error. + const resolved = resolveFile(dir, file); if (resolved.error) { failures.push(resolved.error); if (resolved.conflict) conflicts.push(resolved.conflict); @@ -536,4 +606,4 @@ async function applyPatchToDir({ dir, patchText, reverse = false, onLog = () => return { ok: true, applied, skipped }; } -module.exports = { applyPatchToDir, resolveInside, reverseFile, dominantEol, rollback, diagnoseHunks }; +module.exports = { applyPatchToDir, diagnoseRemoval, resolveInside, reverseFile, dominantEol, rollback, diagnoseHunks }; diff --git a/src/renderer/applied-layer.cjs b/src/renderer/applied-layer.cjs new file mode 100644 index 0000000..4716c05 --- /dev/null +++ b/src/renderer/applied-layer.cjs @@ -0,0 +1,203 @@ +// What the app says about the one patch a ticket has applied (#306). +// +// A ticket is a branch (#108): trunk, plus at most one applied patch or pull +// request, plus the contributor's own edits. The branch holds that faithfully. +// What the app *said* about it did not — the applied patch was remembered as an +// undo blob, so every file it brought was announced as the contributor's own +// writing, and "can this be reverted" meant no more than "we kept the text". +// +// Two answers live here, both pure: +// +// - `attributeConflicts` — whose changes are the ones a new patch would land +// on. A file only the applied layer touched is named as the layer's; a file +// the contributor has also edited over keeps naming their work, because that +// is the one that decides what they do next. +// - `describeAppliedLayer` — the banner's two faces. While the layer still +// comes out cleanly, Revert is offered. Once the contributor's edits sit on +// its lines it is **absorbed**: it has become their changes, and the honest +// exits are saving a copy and discarding the ticket to its base. +// +// Absorption is measured, never tracked — the main process re-answers it on +// every status read, so undoing the overlapping edit brings Revert back on its +// own. And it never frees the one-patch slot: the record survives as provenance. +// +// Pure and dependency-free like update-plan.cjs and apply-conflict.cjs, for the +// same reason: the renderer bundle imports it, `node --test` requires it +// directly, and neither needs a DOM. +'use strict'; + +// Saving a copy and then discarding is a recommendable way forward on this +// project, not a defeat: a ticket's changes are one afternoon's work on a +// checkout that gets thrown away, and redoing them is cheaper than untangling +// them. Said once, here, so both faces that offer it say it the same way. +const DISPOSABLE_EXIT = 'Save a copy of your work first and the ticket is safe to discard back to its base — on this project that is a normal way forward, not a lost afternoon.'; + +// The slot does not open when a patch is absorbed. Saying so is what stops the +// contributor reading "it is part of your changes now" as "so I can apply +// another one". +const SLOT_HELD = 'It still counts as this ticket\'s one applied patch, so another cannot be applied until this ticket is reverted or discarded.'; + +/** + * `a`, `a and b`, `a, b and c` — a list a person reads rather than a join. + * + * @param {string[]} items + * @return {string} + */ +function listOf(items) { + if (items.length <= 1) return items[0] || ''; + return `${items.slice(0, -1).join(', ')} and ${items[items.length - 1]}`; +} + +/** + * The paths of an applied layer whose lines the contributor has edited over. + * + * @param {?Object} appliedPatch + * @return {string[]} + */ +function absorbedPaths(appliedPatch) { + const list = appliedPatch && Array.isArray(appliedPatch.absorbed) ? appliedPatch.absorbed : []; + return list.map((entry) => (typeof entry === 'string' ? entry : entry && entry.path)).filter(Boolean); +} + +/** + * Who owns each file a patch about to be applied would land on. + * + * The pre-apply warning exists to say "your work is here, and this could fail + * without touching it". Counting the applied layer's files as the contributor's + * own writing is the kind of wrong that teaches people to ignore the warning — + * so both are named, separately, and neither is dropped. + * + * The measurement behind `absorbed` is per-region: a file is the layer's when + * the layer's own lines are untouched. A contributor edit *elsewhere* in the + * same file therefore reads as the layer's rather than as theirs. The file is + * still named and the "fails without touching anything" caveat still covers it, + * so the warning does not go quiet — it is the attribution that is coarse, and + * only in that direction. + * + * @param {Object} root0 + * @param {string[]} [root0.conflicts] Paths from the preview's plan. + * @param {?Object} [root0.appliedPatch] The status record, or null. + * @return {{yours: string[], fromLayer: string[], sentences: string[]}} + */ +function attributeConflicts({ conflicts = [], appliedPatch = null } = {}) { + const paths = Array.isArray(conflicts) ? conflicts.filter(Boolean) : []; + const layerFiles = new Set(appliedPatch && Array.isArray(appliedPatch.files) ? appliedPatch.files : []); + const editedOver = new Set(absorbedPaths(appliedPatch)); + + const fromLayer = appliedPatch ? paths.filter((p) => layerFiles.has(p) && !editedOver.has(p)) : []; + const claimed = new Set(fromLayer); + const yours = paths.filter((p) => !claimed.has(p)); + + const sentences = []; + if (yours.length) { + sentences.push(`You have your own edits to ${listOf(yours)}. Save a patch of your work first if you want a copy.`); + } + if (fromLayer.length) { + const label = appliedPatch.label || 'the patch you applied'; + sentences.push(`${listOf(fromLayer)} ${fromLayer.length === 1 ? 'was' : 'were'} changed by ${label}, which you applied — not by you.`); + } + if (sentences.length) { + sentences.push('The patch is applied on top of those changes: it succeeds if they do not overlap, and fails without touching anything if they do.'); + } + return { yours, fromLayer, sentences }; +} + +/** + * The applied-layer banner, in whichever face the checkout has earned. + * + * Three, not two, because "cannot be reverted" has two different causes and + * they need different sentences: a patch too large to keep a copy of was never + * revertable, and a patch whose lines have been edited over stopped being. + * + * `when` is passed in already formatted — the locale-dependent part is the + * component's, and keeping it out of here is what lets this be asserted on. + * + * @param {?Object} appliedPatch The `site:status` record, or null. + * @param {Object} [options] + * @param {string} [options.when] Formatted apply time, or '' when unknown. + * @return {?Object} + */ +function describeAppliedLayer(appliedPatch, { when = '' } = {}) { + if (!appliedPatch) return null; + + const label = appliedPatch.label || 'A patch'; + const files = Array.isArray(appliedPatch.files) ? appliedPatch.files : []; + const absorbed = absorbedPaths(appliedPatch); + const kept = appliedPatch.kept === undefined ? Boolean(appliedPatch.revertable) : Boolean(appliedPatch.kept); + + const summary = `is applied — ${files.length} file${files.length === 1 ? '' : 's'}${when ? `, ${when}` : ''}.`; + + if (kept && !absorbed.length) { + return { label, summary, canRevert: true, absorbed: false, explanation: '', detail: [], note: '', offerCopy: false }; + } + + // Too large to have kept a copy of. Nothing about the tree changes this one, + // so it says so plainly and goes straight to the exit that always works. + if (!kept) { + return { + label, + summary, + canRevert: false, + absorbed: false, + explanation: `${label} was too large to keep a copy of for an undo, so it cannot be lifted back out on its own.`, + detail: [], + note: `${DISPOSABLE_EXIT} ${SLOT_HELD}`, + offerCopy: true + }; + } + + // Absorbed: the contributor's edits are on the patch's own lines, so there + // is no longer a patch and an edit — there is one body of changes. Said as a + // state of the work rather than as a failure, and with the way back named, + // because undoing the overlapping edit really does bring Revert back. + // + // Not every file in the way was written over, though. One the contributor + // deleted outright blocks the removal just as firmly, and calling that "your + // edits are on its lines" would send them looking at lines that are not + // there — so the two get their own sentence, and either alone is enough for + // the revert to be off, because a revert is all or nothing. + const entries = (appliedPatch.absorbed || []).filter((entry) => entry && entry.path); + const written = entries.filter((entry) => entry.editedOver !== false).map((entry) => entry.path); + const gone = entries.filter((entry) => entry.editedOver === false).map((entry) => entry.path); + + const reasons = []; + if (written.length) reasons.push(`your own edits are on the lines it brought to ${listOf(written)}`); + if (gone.length) reasons.push(`${listOf(gone)} ${gone.length === 1 ? 'is' : 'are'} no longer where it left ${gone.length === 1 ? 'it' : 'them'}`); + + return { + label, + summary, + canRevert: false, + absorbed: true, + explanation: `${label} is part of your changes now and cannot be lifted back out on its own: ${listOf(reasons)}. Undo those edits and Revert comes back on its own.`, + detail: entries + .filter((entry) => entry.total) + .map((entry) => `${entry.path} — you have edited ${entry.failed} of the ${entry.total} change${entry.total === 1 ? '' : 's'} it brought`), + note: `${DISPOSABLE_EXIT} ${SLOT_HELD}`, + offerCopy: true + }; +} + +/** + * Whichever of the two absorbed exits failed, said where they were offered. + * + * Both report through state that belongs to somewhere else on screen — the + * changes note and the patch modal — and the absorbed banner is neither. A + * refusal that lands there is a button that did nothing, on the one way out + * this banner recommends, so it is repeated here rather than left behind. + * + * The save goes first: it is the step that makes discarding safe, and its + * failure is the one that must not be missed. + * + * @param {Object} root0 + * @param {string} [root0.patchSaveError] + * @param {string} [root0.discardError] + * @return {{message: string}} + */ +function absorbedExitFailure({ patchSaveError = '', discardError = '' } = {}) { + if (patchSaveError) return { message: `The copy could not be saved: ${patchSaveError}` }; + if (discardError) return { message: discardError }; + return { message: '' }; +} + +module.exports = { attributeConflicts, describeAppliedLayer, absorbedExitFailure, listOf, DISPOSABLE_EXIT, SLOT_HELD }; diff --git a/src/renderer/apply-conflict.cjs b/src/renderer/apply-conflict.cjs index b62d647..2cdbe5a 100644 --- a/src/renderer/apply-conflict.cjs +++ b/src/renderer/apply-conflict.cjs @@ -30,17 +30,29 @@ const REASONS = { moved: 'the code around it has changed' }; +// The same two statuses read backwards, for a revert (#306). `already-applied` +// is derived by testing the inverse of what is being applied, so on a reverse it +// means the *forward* hunk fits — that region's change is not in the checkout +// any more. Rendering the forward wording there would put "already in your +// checkout" under a headline saying the contributor has edited over it. +const REVERT_REASONS = { + 'already-applied': 'looks like that change is not in your checkout any more', + moved: 'the code around it has changed' +}; + /** * One failing region, ready to render. * - * @param {Object} region + * @param {Object} region + * @param {boolean} [reversing] Whether the failed run was a revert. * @return {Object} */ -function describeRegion(region) { +function describeRegion(region, reversing = false) { + const reasons = reversing ? REVERT_REASONS : REASONS; return { line: region.line, status: region.status, - reason: REASONS[region.status] || 'it no longer fits', + reason: reasons[region.status] || 'it no longer fits', // A line to search for, not a number to go to: the hunk's line numbers // are in the patched file's coordinates and miss by the drift the patch // failed on. The panel leads with this and keeps `line` as the fallback. @@ -248,6 +260,36 @@ function prFraming(conflicts, prState, ownWorkPaths = []) { }; } +/** + * The framing for a revert that would not come back out (#306). + * + * A revert fails for one reason: the contributor's own edits are on the lines + * the patch brought. There is no third party here and no rebase to ask anyone + * for — the patch and the edits are one body of work now, which is what + * absorption means. So the sentence names that, and the two ways forward are + * undoing the overlapping edits, or saving a copy and discarding the ticket to + * its base, which on this project is a normal step rather than a defeat. + * + * Regions are kept, unlike the pull-request framing: these lines are the + * contributor's own, so pointing at them is pointing at their work. + * + * @param {Array} conflicts + * @param {string} label + * @return {{headline: string, advice: string, prButton: ?string}} + */ +function revertFraming(conflicts, label) { + const failed = conflicts.reduce((sum, c) => sum + c.regions.length, 0); + const total = conflicts.reduce((sum, c) => sum + c.total, 0); + const files = conflicts.length; + const where = files === 1 ? '' : `, across ${files} files`; + + return { + headline: `${label} cannot be lifted back out on its own: your own edits are on ${failed} of its ${total} change${total === 1 ? '' : 's'}${where}.`, + advice: 'It is part of your changes now. Undoing your edits on those lines brings Revert back; otherwise save a copy of your work and discard the ticket to its base — on this project that is a normal way forward, not a lost afternoon.', + prButton: null + }; +} + /** * Everything the panel needs to explain a failed apply, or null when there is * nothing to explain. @@ -273,9 +315,10 @@ function prFraming(conflicts, prState, ownWorkPaths = []) { * @param {?string} [options.prState] Its state, when known. * @param {string[]} [options.ownWorkPaths] Files this ticket has work in, from * the preview's collision list (#303). + * @param {?string} [options.reverting] Label of the layer being reverted. * @return {?Object} */ -function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, prUrl = null, prState = null, ownWorkPaths = [] } = {}) { +function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, prUrl = null, prState = null, ownWorkPaths = [], reverting = null } = {}) { if (!result || result.ok) return null; const failures = Array.isArray(result.failures) ? result.failures : []; @@ -285,7 +328,10 @@ function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, pr // the panel keeps rendering it exactly as it did before. if (!failures.length && !conflicts.length) return null; - const fromPr = Boolean(prUrl); + // A revert is never "someone else's patch does not fit": it is the + // contributor's own edits sitting on lines they applied. The pull-request + // framing would send them to an author who has nothing to do with it. + const fromPr = Boolean(prUrl) && !reverting; // Each conflict is consumed as it is matched, not looked up in a map: a // concatenated patch can fail the same file twice with the identical @@ -304,7 +350,7 @@ function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, pr failed: conflict.regions.length, // For a pull request the regions are the author's problem; the file // row with its counts is the whole story the contributor needs. - regions: fromPr ? [] : conflict.regions.map(describeRegion) + regions: fromPr ? [] : conflict.regions.map((region) => describeRegion(region, Boolean(reverting))) }; }); @@ -314,9 +360,13 @@ function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, pr let advice = ''; let prButton = fromPr ? 'Open the pull request' : null; if (conflicts.length) { - const framing = fromPr - ? prFraming(conflicts, prState, ownWorkPaths) - : { headline: headlineFor(conflicts), advice: '', prButton: null }; + // A failed revert is never the pull request having gone stale, so it is + // asked first: the only thing that stops a layer coming back out is the + // contributor's own work sitting on its lines (#306). + let framing; + if (reverting) framing = revertFraming(conflicts, reverting); + else if (fromPr) framing = prFraming(conflicts, prState, ownWorkPaths); + else framing = { headline: headlineFor(conflicts), advice: '', prButton: null }; headline = framing.headline; advice = framing.advice; prButton = framing.prButton; @@ -325,13 +375,17 @@ function describeApplyFailure(result, { otherPatchCount: othersAvailable = 0, pr headline, advice, items, + // The absorbed exit, for the panel to offer alongside the sentence. Only + // a revert has one: every other failure left the checkout untouched, so + // there is nothing to save a copy of that is not already safe. + offerDiscardToBase: Boolean(reverting) && conflicts.length > 0, // Both are offered only when they lead somewhere, the way open-failure.cjs // withholds its picker: a way out that returns to the same dead end is // worse than no button, because it costs a click to find that out. - offerOtherPatches: othersAvailable > 0, + offerOtherPatches: !reverting && othersAvailable > 0, prUrl: prUrl && prButton ? prUrl : null, prButton }; } -module.exports = { describeApplyFailure, headlineFor, otherPatchCount, REASONS }; +module.exports = { describeApplyFailure, headlineFor, otherPatchCount, revertFraming, REASONS, REVERT_REASONS }; diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index 159029f..a8914d2 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -30,7 +30,8 @@ import { pathBasename } from './path-basename.cjs'; import { sanitizeSiteFolder, resolveTargetDir, directoryFromFileEntry } from './site-folder.cjs'; import { noticeForOpenResult } from './open-failure.cjs'; import { describeApplyFailure, otherPatchCount } from './apply-conflict.cjs'; -import { describeOwnWorkWarning } from './ticket-base.cjs'; +import { baseIsApproximate, UNRECORDED_CLEAR_NOTE } from './ticket-base.cjs'; +import { describeAppliedLayer, attributeConflicts, absorbedExitFailure } from './applied-layer.cjs'; import { trunkAgeInfo, planUpdateSteps, updateStepStatuses, SKIP_INSTALL_MESSAGE, planApplySteps, planWatchImpact, APPLY_STATE_TO_STEP, planSetupSteps, SETUP_STATE_TO_STEP, setupOutcome } from './update-plan.cjs'; import { pickLatest } from '../latest-patch.cjs'; import { beginSetup, adoptSetupPath, discardSetup, rowPathAfterStatus } from './pending-setup.cjs'; @@ -2742,6 +2743,22 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // — the preview carries the status of the base it was measured against (#308). const applyOwnWorkNotice = applyPreview ? describeOwnWorkWarning(applyPreview) : null; + // The applied patch as a layer with a name (#306), not an undo blob. Both + // answers come from the same record: whether it can still be lifted out — + // measured in main on every status read, so it comes back on its own when the + // overlapping edit is undone — and whose changes the next patch would land on. + const appliedLayer = describeAppliedLayer(appliedPatch, { + when: appliedPatch?.appliedAt ? new Date(appliedPatch.appliedAt).toLocaleString() : '' + }); + const previewAttribution = attributeConflicts({ conflicts: applyPreview?.conflicts, appliedPatch }); + const previewBaseApproximate = Boolean(applyPreview) && baseIsApproximate(applyPreview.baseStatus); + // The absorbed exits reach the same two operations the changes note does, so + // they go through the same guard: a discard is a force checkout, and running + // it under a live dev server or a half-finished install rewrites the tree + // from under it. Not re-derived here — that is how a second answer starts. + const absorbedExitBlocked = discardBlocked({ isUpdating, installing, building, devServerActive: isDevProcessActive, discarding }); + const absorbedExit = absorbedExitFailure({ patchSaveError, discardError }); + // --- Initial setup, as one chain (#246) --- // The third chain, and the only one nobody starts: between the clone, the // install and the build there is no decision to make, so making the @@ -3189,24 +3206,27 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // A conflict is where the panel used to stop: one file named, the // rest of the failures left in the terminal, and no sense of whether // one region of twenty missed or all of them. The breakdown is what - // turns that into a decision (#282). A reverse is left out — the - // patch it names came from this app and the ticket's other patches - // are no help against it. - setApplyConflict(reverse ? null : describeApplyFailure(res, { - otherPatchCount: otherPatchCount({ - label: preview?.label, - prs: ticketPatches?.items, - attachments: patchAttachments - }), - prUrl: preview?.prUrl || null, - prState: preview?.prState || null, - // The preview's own collision list: the files this ticket has work - // in, measured from its base (#301). Without it an open pull request - // is always narrated as stale, so a failure caused by the - // contributor's own edits sends them to ask a stranger for a rebase - // that would not help (#303). - ownWorkPaths: preview?.conflicts || [] - })); + // turns that into a decision (#282). A reverse gets its own framing + // (#306): it fails only because the contributor's own edits are on the + // patch's lines, so the ticket's other patches and the pull request's + // author are both the wrong place to send them. + setApplyConflict(describeApplyFailure(res, reverse + ? { reverting: appliedPatch?.label || 'That patch' } + : { + otherPatchCount: otherPatchCount({ + label: preview?.label, + prs: ticketPatches?.items, + attachments: patchAttachments + }), + prUrl: preview?.prUrl || null, + prState: preview?.prState || null, + // The preview's own collision list: the files this ticket has work + // in, measured from its base (#301). Without it an open pull + // request is always narrated as stale, so a failure caused by the + // contributor's own edits sends them to ask a stranger for a + // rebase that would not help (#303). + ownWorkPaths: preview?.conflicts || [] + })); finishApply(); return; } @@ -4714,19 +4734,38 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit
) : null} - {appliedPatch && !isApplying ? ( -
-
- {appliedPatch.label} is applied — {appliedPatch.files.length} file{appliedPatch.files.length === 1 ? '' : 's'} - {appliedPatch.appliedAt ? `, ${new Date(appliedPatch.appliedAt).toLocaleString()}` : ''}. + {appliedLayer && !isApplying ? ( +
+
+ {appliedLayer.label} {appliedLayer.summary}
+ {appliedLayer.explanation ? ( +
{appliedLayer.explanation}
+ ) : null} + {appliedLayer.detail.map((line) => ( +
{line}
+ ))} + {appliedLayer.note ? ( +
{appliedLayer.note}
+ ) : null}
- {appliedPatch.revertable ? ( + {appliedLayer.canRevert ? ( - ) : ( - Too large to undo automatically — use Update to latest trunk to reset. - )} + ) : null} + {appliedLayer.offerCopy ? ( + <> + + + + ) : null}
+ {/* Both exits report failure through state the changes note and the + patch modal own, and neither is on screen here — so a save that + could not write, or a discard that refused, would be a button + that did nothing on the one way out this banner recommends. */} + {absorbedExit.message ? ( +
{absorbedExit.message}
+ ) : null}
) : null} @@ -4738,10 +4777,21 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit
{applyPreview.paths.map((p) =>
{p}
)}
- {applyOwnWorkNotice ? ( -
- {applyOwnWorkNotice.text} + {/* Who the colliding work belongs to (#306) is the sentence, and + how sure the app is of the base it was measured from (#308) + rides with it: on an unrecorded base the list can name files + the contributor never touched and miss ones they did, so an + unhedged sentence would overstate what was checked. With no + collisions the hedge is all that is left, said quietly — + "nothing collided" is not a promise an approximate base can + make. */} + {previewAttribution.sentences.length ? ( +
+ {previewAttribution.sentences.map((sentence) =>
{sentence}
)} + {previewBaseApproximate ?
{UNRECORDED_CLEAR_NOTE}
: null}
+ ) : previewBaseApproximate ? ( +
{UNRECORDED_CLEAR_NOTE}
) : null} {applyPreview.unsupported.length ? (
@@ -4838,8 +4888,14 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit
{applyConflict.advice}
) : null} - {applyConflict.offerOtherPatches || applyConflict.prUrl ? ( + {applyConflict.offerOtherPatches || applyConflict.prUrl || applyConflict.offerDiscardToBase ? (
+ {applyConflict.offerDiscardToBase ? ( + <> + + + + ) : null} {applyConflict.offerOtherPatches ? (
) : null}
diff --git a/test/applied-layer.test.cjs b/test/applied-layer.test.cjs new file mode 100644 index 0000000..bbb0599 --- /dev/null +++ b/test/applied-layer.test.cjs @@ -0,0 +1,258 @@ +'use strict'; + +// The applied patch as a layer with a name (#306): who owns which file, and +// which of the banner's faces the checkout has earned. Pure — the module holds +// the branching so the component does not, which is what makes this reachable. + +const test = require('node:test'); +const assert = require('node:assert'); +const { attributeConflicts, describeAppliedLayer, absorbedExitFailure, listOf } = require('../src/renderer/applied-layer.cjs'); +const { describeApplyFailure } = require('../src/renderer/apply-conflict.cjs'); + +const FOO = 'src/wp-login.php'; +const BAR = 'src/wp-admin/edit.php'; + +const layer = (over = {}) => ({ + label: 'PR #123', + appliedAt: '2026-08-12T10:00:00.000Z', + files: [FOO], + kept: true, + absorbed: [], + revertable: true, + ...over +}); + +// --- attribution ---------------------------------------------------------- + +test('attributeConflicts: a file only the applied patch touched is not called your work (#306)', () => { + const { yours, fromLayer, sentences } = attributeConflicts({ + conflicts: [FOO], + appliedPatch: layer() + }); + + assert.deepStrictEqual(yours, []); + assert.deepStrictEqual(fromLayer, [FOO]); + // The warning does not go quiet — the file is still named, just attributed. + assert.ok(sentences.some((s) => s.includes(FOO) && s.includes('PR #123'))); + assert.ok(!sentences.some((s) => s.startsWith('You have your own edits'))); +}); + +test('attributeConflicts: a file the contributor also edited keeps naming their work (#306)', () => { + const { yours, fromLayer, sentences } = attributeConflicts({ + conflicts: [FOO], + appliedPatch: layer({ absorbed: [{ path: FOO, failed: 1, total: 3 }], revertable: false }) + }); + + assert.deepStrictEqual(yours, [FOO]); + assert.deepStrictEqual(fromLayer, []); + assert.ok(sentences[0].startsWith('You have your own edits')); +}); + +test('attributeConflicts: the two owners are named separately, not merged (#306)', () => { + const { yours, fromLayer, sentences } = attributeConflicts({ + conflicts: [FOO, BAR], + appliedPatch: layer() + }); + + assert.deepStrictEqual(yours, [BAR]); + assert.deepStrictEqual(fromLayer, [FOO]); + assert.strictEqual(sentences.length, 3); + assert.ok(sentences[0].includes(BAR) && !sentences[0].includes(FOO)); + assert.ok(sentences[1].includes(FOO) && !sentences[1].includes(BAR)); +}); + +test('attributeConflicts: with no patch applied every file is the contributor\'s (#306)', () => { + const { yours, fromLayer, sentences } = attributeConflicts({ conflicts: [FOO, BAR], appliedPatch: null }); + + assert.deepStrictEqual(yours, [FOO, BAR]); + assert.deepStrictEqual(fromLayer, []); + assert.ok(sentences[0].startsWith('You have your own edits')); +}); + +test('attributeConflicts: a clean tree says nothing at all (#306)', () => { + assert.deepStrictEqual(attributeConflicts({ conflicts: [], appliedPatch: layer() }).sentences, []); + assert.deepStrictEqual(attributeConflicts().sentences, []); +}); + +test('listOf: reads as a sentence rather than a join (#306)', () => { + assert.strictEqual(listOf([]), ''); + assert.strictEqual(listOf(['a']), 'a'); + assert.strictEqual(listOf(['a', 'b']), 'a and b'); + assert.strictEqual(listOf(['a', 'b', 'c']), 'a, b and c'); +}); + +// --- the banner's faces --------------------------------------------------- + +test('describeAppliedLayer: nothing applied, nothing to say (#306)', () => { + assert.strictEqual(describeAppliedLayer(null), null); +}); + +test('describeAppliedLayer: while it still comes out, Revert is the only offer (#306)', () => { + const face = describeAppliedLayer(layer(), { when: '12/08/2026' }); + + assert.strictEqual(face.canRevert, true); + assert.strictEqual(face.absorbed, false); + assert.strictEqual(face.offerCopy, false); + assert.strictEqual(face.explanation, ''); + assert.strictEqual(face.summary, 'is applied — 1 file, 12/08/2026.'); +}); + +test('describeAppliedLayer: absorbed drops Revert and offers the copy-and-discard exit (#306)', () => { + const face = describeAppliedLayer(layer({ + revertable: false, + absorbed: [{ path: FOO, failed: 2, total: 5 }] + })); + + assert.strictEqual(face.canRevert, false); + assert.strictEqual(face.absorbed, true); + assert.strictEqual(face.offerCopy, true); + assert.ok(face.explanation.includes(FOO)); + assert.ok(face.explanation.includes('part of your changes now')); + // Not a one-way door, and the banner has to say so. + assert.ok(/Undo those edits and Revert comes back/.test(face.explanation)); + assert.deepStrictEqual(face.detail, [`${FOO} — you have edited 2 of the 5 changes it brought`]); +}); + +// The constraint the whole design rests on: absorption is not a way to apply a +// second patch. The banner has to say the slot is still taken. +test('describeAppliedLayer: absorption does not free the one-patch slot (#306)', () => { + const face = describeAppliedLayer(layer({ revertable: false, absorbed: [{ path: FOO, failed: 1, total: 1 }] })); + + assert.ok(/one applied patch/.test(face.note)); + assert.ok(/reverted or discarded/.test(face.note)); +}); + +// Discarding is a recommendable step here, not an admission of failure — the +// wording is the point, so it is asserted rather than left to drift. +test('describeAppliedLayer: the exit is worded as a normal way forward (#306)', () => { + const face = describeAppliedLayer(layer({ revertable: false, absorbed: [{ path: FOO, failed: 1, total: 1 }] })); + + assert.ok(/not a lost afternoon/.test(face.note)); + assert.ok(/Save a copy of your work/.test(face.note)); +}); + +test('describeAppliedLayer: a patch too large to keep is a different sentence from absorption (#306)', () => { + const face = describeAppliedLayer(layer({ kept: false, revertable: false })); + + assert.strictEqual(face.canRevert, false); + assert.strictEqual(face.absorbed, false); + assert.strictEqual(face.offerCopy, true); + assert.ok(/too large to keep a copy of/.test(face.explanation)); + assert.deepStrictEqual(face.detail, []); +}); + +// A record written before this change carries `revertable` and no `kept`. +test('describeAppliedLayer: an older record without `kept` still reads correctly (#306)', () => { + assert.strictEqual(describeAppliedLayer({ label: 'x', files: [FOO], revertable: true }).canRevert, true); + assert.strictEqual(describeAppliedLayer({ label: 'x', files: [FOO], revertable: false }).canRevert, false); +}); + +// --- the revert failure's narration --------------------------------------- + +const revertFailure = { + ok: false, + failures: [`${FOO} has moved on since the patch was written, so none of its 3 changes still fits`], + conflicts: [{ + path: FOO, + error: `${FOO} has moved on since the patch was written, so none of its 3 changes still fits`, + total: 3, + regions: [ + { index: 0, line: 10, status: 'moved', anchor: 'a', lines: [] }, + { index: 1, line: 20, status: 'moved', anchor: 'b', lines: [] }, + { index: 2, line: 30, status: 'moved', anchor: 'c', lines: [] } + ] + }] +}; + +test('describeApplyFailure: a failed revert blames the contributor\'s own edits, not the author (#306)', () => { + const notice = describeApplyFailure(revertFailure, { reverting: 'PR #123', prUrl: 'https://example.invalid/pr/1', prState: 'open' }); + + assert.ok(notice.headline.startsWith('PR #123 cannot be lifted back out on its own')); + assert.ok(/your own edits are on 3 of its 3 changes/.test(notice.headline)); + // The pull request's author has nothing to do with a revert. + assert.strictEqual(notice.prUrl, null); + assert.strictEqual(notice.prButton, null); + assert.ok(!/rebase/.test(notice.advice)); +}); + +test('describeApplyFailure: a failed revert offers the copy-and-discard exit, not another patch (#306)', () => { + const notice = describeApplyFailure(revertFailure, { reverting: 'PR #123', otherPatchCount: 4 }); + + assert.strictEqual(notice.offerDiscardToBase, true); + assert.strictEqual(notice.offerOtherPatches, false); + assert.ok(/Undoing your edits on those lines brings Revert back/.test(notice.advice)); + assert.ok(/not a lost afternoon/.test(notice.advice)); + // The contributor owns these lines, so the per-region detail is theirs to see. + assert.strictEqual(notice.items[0].regions.length, 3); +}); + +test('describeApplyFailure: a forward apply is unaffected by the revert framing (#306)', () => { + const notice = describeApplyFailure(revertFailure, { prUrl: 'https://example.invalid/pr/1', prState: 'open', otherPatchCount: 2 }); + + assert.strictEqual(notice.offerDiscardToBase, false); + assert.strictEqual(notice.offerOtherPatches, true); + assert.strictEqual(notice.prUrl, 'https://example.invalid/pr/1'); + assert.ok(/author/.test(notice.advice)); +}); + +// A file blocked for a reason that is not an edit over the patch's lines — the +// contributor deleted it outright — must not be described as one. Withholding +// Revert is still right; the sentence about it is what changes. +test('describeAppliedLayer: a file that is simply gone gets its own sentence (#306)', () => { + const face = describeAppliedLayer(layer({ + revertable: false, + absorbed: [{ path: FOO, editedOver: false, failed: 0, total: 0 }] + })); + + assert.strictEqual(face.canRevert, false); + assert.ok(!/your own edits are on the lines/.test(face.explanation)); + assert.ok(/no longer where it left it/.test(face.explanation)); + // Nothing to count, so nothing is counted. + assert.deepStrictEqual(face.detail, []); +}); + +test('describeAppliedLayer: both reasons at once read as one sentence (#306)', () => { + const face = describeAppliedLayer(layer({ + files: [FOO, BAR], + revertable: false, + absorbed: [ + { path: FOO, editedOver: true, failed: 1, total: 2 }, + { path: BAR, editedOver: false, failed: 0, total: 0 } + ] + })); + + assert.ok(face.explanation.includes(`your own edits are on the lines it brought to ${FOO}`)); + assert.ok(face.explanation.includes(`${BAR} is no longer where it left it`)); + assert.deepStrictEqual(face.detail, [`${FOO} — you have edited 1 of the 2 changes it brought`]); +}); + +// `already-applied` is derived by testing the inverse of what is being applied, +// so on a revert it means the opposite of what it means going forwards. Reading +// the forward wording out loud there contradicts the headline above it. +test('describeApplyFailure: a revert reads `already-applied` backwards (#306)', () => { + const withApplied = { + ...revertFailure, + conflicts: [{ ...revertFailure.conflicts[0], regions: [{ index: 0, line: 10, status: 'already-applied', anchor: 'a', lines: [] }] }] + }; + + const reverting = describeApplyFailure(withApplied, { reverting: 'PR #123' }); + assert.strictEqual(reverting.items[0].regions[0].reason, 'looks like that change is not in your checkout any more'); + + const forward = describeApplyFailure(withApplied, {}); + assert.strictEqual(forward.items[0].regions[0].reason, 'looks like it is already in your checkout'); +}); + +// --- the exits' own failures --------------------------------------------- + +test('absorbedExitFailure: silence when neither exit has failed (#306)', () => { + assert.strictEqual(absorbedExitFailure().message, ''); + assert.strictEqual(absorbedExitFailure({ patchSaveError: '', discardError: '' }).message, ''); +}); + +test('absorbedExitFailure: the save is the failure that must not be missed (#306)', () => { + const both = absorbedExitFailure({ patchSaveError: 'EACCES', discardError: 'could not reset' }); + assert.ok(both.message.includes('EACCES')); + assert.ok(!both.message.includes('could not reset')); + + assert.strictEqual(absorbedExitFailure({ discardError: 'could not reset' }).message, 'could not reset'); +}); diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index 3e75be9..5b497e2 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -31,10 +31,11 @@ const os = require('node:os'); const path = require('node:path'); const { EventEmitter } = require('node:events'); const git = require('isomorphic-git'); -// The one module these tests read an answer *through* rather than around: what +// The two modules these tests read an answer *through* rather than around: what // the preview's base status means for the contributor is the module's to say, // and #308's point is that the two halves agree. const { describeOwnWorkWarning } = require('../src/renderer/ticket-base.cjs'); +const { attributeConflicts } = require('../src/renderer/applied-layer.cjs'); const SRC_DIR = path.join(__dirname, '..', 'src'); const MAIN_PATH = path.join(SRC_DIR, 'main.js'); @@ -3823,3 +3824,146 @@ test('every IPC channel is classified: wired, or explicitly not', () => { assert.deepEqual(stale, [], 'Classified channels that main.js no longer registers'); }); + +// --- the applied patch as a layer, measured not remembered (#306) --------- +// +// `revertable` used to mean "we kept the text". It now means what the banner +// asks: can this layer still be lifted out of *this* checkout, right now. That +// is a fact about the tree, so these run against a real repo — a stub would +// mock the very thing under test. + +// The parked ticket with LOGIN_DIFF applied on top of it, plus the record the +// app writes when it applies one. `content` is what ends up in the file, so a +// test can hand-edit over the patch's own line and ask again. +async function ticketWithAppliedPatch(t, content) { + const { dir, baseOid, workFile } = await parkedTicketRepo(t, { workFile: 'src/wp-login.php' }); + fs.writeFileSync(path.join(dir, workFile), content); + const settings = fakeSettingsStore({ + sites: [dir], + siteMeta: { + [dir]: { + tracTicket: 62281, + branches: { + 'ticket/62281': { + tracTicket: 62281, + baseOid, + appliedPatch: { + label: 'PR #123', + appliedAt: '2026-08-12T10:00:00.000Z', + files: ['src/wp-login.php'], + text: LOGIN_DIFF + } + } + } + } + } + }); + const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs } }); + return { dir, main, settings, workFile }; +} + +const PATCH_APPLIED = ' { + const { dir, main } = await ticketWithAppliedPatch(t, PATCH_APPLIED); + + const status = await main.invoke('site:status', dir); + + assert.equal(status.appliedPatch.label, 'PR #123'); + assert.equal(status.appliedPatch.kept, true); + assert.equal(status.appliedPatch.revertable, true); + assert.deepEqual(status.appliedPatch.absorbed, []); +}); + +test('site:status: editing the patch\'s own lines absorbs it, and Revert stands down (#306)', async (t) => { + const { dir, main } = await ticketWithAppliedPatch(t, EDITED_OVER); + + const status = await main.invoke('site:status', dir); + + assert.equal(status.appliedPatch.revertable, false); + // The text is still stored — this is absorption, not a patch too large to + // keep, and the two get different sentences. + assert.equal(status.appliedPatch.kept, true); + assert.deepEqual(status.appliedPatch.absorbed, [{ path: 'src/wp-login.php', editedOver: true, failed: 1, total: 1 }]); +}); + +// Not a one-way door, and nothing persisted says otherwise: the same site, +// the same record, answers differently the moment the overlapping edit goes. +test('site:status: undoing the overlapping edit brings Revert back on its own (#306)', async (t) => { + const { dir, main, workFile } = await ticketWithAppliedPatch(t, EDITED_OVER); + + assert.equal((await main.invoke('site:status', dir)).appliedPatch.revertable, false); + + fs.writeFileSync(path.join(dir, workFile), PATCH_APPLIED); + + const back = await main.invoke('site:status', dir); + assert.equal(back.appliedPatch.revertable, true); + assert.deepEqual(back.appliedPatch.absorbed, []); +}); + +// The constraint the design rests on: the slot frees on revert or on discarding +// to base, never by absorption. The record has to survive as provenance. +test('site:status: absorption does not free the one-patch slot (#306)', async (t) => { + const { dir, main } = await ticketWithAppliedPatch(t, EDITED_OVER); + + const status = await main.invoke('site:status', dir); + assert.notEqual(status.appliedPatch, null, 'the record is provenance, not just an undo blob'); + + // And the guard still refuses a second patch on its behalf. + const event = createIpcEvent(); + const { applyId } = await main.invokeWith('git:apply-patch', event, dir, { patchText: LOGIN_DIFF, label: 'PR #456' }); + const done = await applyDone(event, applyId); + assert.equal(done.ok, false); + assert.match(done.error, /PR #123 is already applied/); +}); + +// A patch a trunk update reset away is not absorbed: its record is stale, and +// Revert has to stay on screen because pressing it is what clears it. +test('site:status: a patch the tree no longer holds keeps its Revert (#306)', async (t) => { + const { dir, main } = await ticketWithAppliedPatch(t, ' { + const { dir, main, workFile } = await ticketWithAppliedPatch(t, EDITED_OVER); + const before = fs.readFileSync(path.join(dir, workFile), 'utf8'); + + await main.invoke('site:status', dir); + + assert.equal(fs.readFileSync(path.join(dir, workFile), 'utf8'), before); +}); + +// The other half of #306: the pre-apply warning names the files, and the +// renderer's attribution module decides which of them are the contributor's. +// The two have to agree on the same paths, which is what this pins. +test('git:preview-patch and the applied record agree on who owns a file (#306)', async (t) => { + const { dir, main } = await ticketWithAppliedPatch(t, PATCH_APPLIED); + + const preview = await main.invoke('git:preview-patch', dir, LOGIN_DIFF); + const status = await main.invoke('site:status', dir); + + assert.deepEqual(preview.conflicts, ['src/wp-login.php']); + const attributed = attributeConflicts({ conflicts: preview.conflicts, appliedPatch: status.appliedPatch }); + assert.deepEqual(attributed.fromLayer, ['src/wp-login.php']); + assert.deepEqual(attributed.yours, []); +}); + +// And once the contributor has edited over it, the same file goes back to +// naming their work — the answer that decides what they do next. +test('git:preview-patch: a file edited over the patch is the contributor\'s again (#306)', async (t) => { + const { dir, main } = await ticketWithAppliedPatch(t, EDITED_OVER); + + const preview = await main.invoke('git:preview-patch', dir, LOGIN_DIFF); + const status = await main.invoke('site:status', dir); + + const attributed = attributeConflicts({ conflicts: preview.conflicts, appliedPatch: status.appliedPatch }); + assert.deepEqual(attributed.yours, ['src/wp-login.php']); + assert.deepEqual(attributed.fromLayer, []); +}); diff --git a/test/patch-apply.integration.test.cjs b/test/patch-apply.integration.test.cjs index 24ec39b..a4b80ff 100644 --- a/test/patch-apply.integration.test.cjs +++ b/test/patch-apply.integration.test.cjs @@ -7,7 +7,7 @@ const os = require('os'); const path = require('path'); const git = require('isomorphic-git'); const JsDiff = require('diff'); -const { applyPatchToDir, resolveInside, dominantEol, rollback, diagnoseHunks } = require('../src/patch-apply'); +const { applyPatchToDir, diagnoseRemoval, resolveInside, dominantEol, rollback, diagnoseHunks } = require('../src/patch-apply'); const { parsePatchFiles } = require('../src/patch-plan.cjs'); // A real on-disk repo, like trunk-update.integration.test.cjs: applyPatchToDir @@ -792,3 +792,98 @@ test('diagnoseHunks: overlapping hunks that each pass alone yield null, not zero // Sanity: the single hunk applies, so per-hunk diagnosis finds no failures. assert.strictEqual(diagnoseHunks(body, file), null); }); + +// --- diagnoseRemoval: can the applied layer still be taken out? (#306) ------ +// +// The record the app keeps only ever said "a patch text was stored". These +// exercise the question the banner actually asks, against a real checkout: is +// the layer still separable from the contributor's own work, right now. + +test('diagnoseRemoval: a freshly applied patch still comes out cleanly (issue #306)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + await applyPatchToDir({ dir, patchText: FOO_PATCH }); + + assert.deepStrictEqual(diagnoseRemoval({ dir, patchText: FOO_PATCH }), { absorbed: [], missing: false }); +}); + +// The absorption case: the contributor has edited the very line the patch +// brought, so there is no longer a patch and an edit — there is one body of +// work, and no revert can separate them. +test('diagnoseRemoval: editing the patch\'s own line absorbs it (issue #306)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + await applyPatchToDir({ dir, patchText: FOO_PATCH }); + fs.writeFileSync(path.join(dir, FOO), 'one\nMY OWN VERSION\nthree\n'); + + const answer = diagnoseRemoval({ dir, patchText: FOO_PATCH }); + assert.strictEqual(answer.missing, false); + assert.deepStrictEqual(answer.absorbed.map((a) => a.path), [FOO]); + // The counts come from diagnoseHunks, so the banner can say how much of the + // patch has been written over rather than only that something has. + assert.deepStrictEqual(answer.absorbed[0], { path: FOO, editedOver: true, failed: 1, total: 1 }); +}); + +// Not a one-way door, and this is the test that pins it: nothing persists an +// "absorbed" flag, so putting the line back makes the layer removable again +// with no further action from anyone. +test('diagnoseRemoval: undoing the overlapping edit makes it removable again (issue #306)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + await applyPatchToDir({ dir, patchText: FOO_PATCH }); + const patched = fs.readFileSync(path.join(dir, FOO), 'utf8'); + + fs.writeFileSync(path.join(dir, FOO), 'one\nMY OWN VERSION\nthree\n'); + assert.strictEqual(diagnoseRemoval({ dir, patchText: FOO_PATCH }).absorbed.length, 1); + + fs.writeFileSync(path.join(dir, FOO), patched); + assert.deepStrictEqual(diagnoseRemoval({ dir, patchText: FOO_PATCH }), { absorbed: [], missing: false }); +}); + +// Work on a file the patch never touched is not absorption. It is the ordinary +// state of a ticket, and it must not take Revert away. +test('diagnoseRemoval: edits to other files leave the layer removable (issue #306)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY, [BAR]: BAR_BODY }); + await applyPatchToDir({ dir, patchText: FOO_PATCH }); + fs.writeFileSync(path.join(dir, BAR), 'my own work\n'); + + assert.deepStrictEqual(diagnoseRemoval({ dir, patchText: FOO_PATCH }), { absorbed: [], missing: false }); +}); + +// A patch a trunk update or a discard reset away is not absorbed — its record +// is merely stale. Reporting absorption there would hide Revert, which is the +// one button that clears the record. +test('diagnoseRemoval: a patch the tree no longer holds reads as missing, not absorbed (issue #306)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + await applyPatchToDir({ dir, patchText: FOO_PATCH }); + fs.writeFileSync(path.join(dir, FOO), FOO_BODY); + + assert.deepStrictEqual(diagnoseRemoval({ dir, patchText: FOO_PATCH }), { absorbed: [], missing: true }); +}); + +// Nothing here may write. The whole point of measuring instead of tracking is +// that it can run on a status read. +test('diagnoseRemoval: the checkout is not touched (issue #306)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY, [BAR]: BAR_BODY }); + await applyPatchToDir({ dir, patchText: FOO_PATCH }); + fs.writeFileSync(path.join(dir, FOO), 'one\nMY OWN VERSION\nthree\n'); + const before = snapshot(dir); + + diagnoseRemoval({ dir, patchText: FOO_PATCH }); + + assert.deepStrictEqual(snapshot(dir), before); +}); + +// A revert that fails now carries the same per-region breakdown a forward apply +// does, because that is what the panel narrates the absorption from. +test('applyPatchToDir: a failing reverse reports which regions were edited over (issue #306)', async (t) => { + const dir = await makeRepo(t, { [FOO]: FOO_BODY }); + await applyPatchToDir({ dir, patchText: FOO_PATCH }); + fs.writeFileSync(path.join(dir, FOO), 'one\nMY OWN VERSION\nthree\n'); + + const res = await applyPatchToDir({ dir, patchText: FOO_PATCH, reverse: true }); + + assert.strictEqual(res.ok, false); + assert.strictEqual(res.notApplied, undefined); + assert.strictEqual(res.conflicts.length, 1); + assert.strictEqual(res.conflicts[0].path, FOO); + assert.strictEqual(res.conflicts[0].total, 1); + assert.strictEqual(res.conflicts[0].regions.length, 1); +}); From ecc03b2b3e62ea45b748146fd81b562bd574cb95 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Wed, 12 Aug 2026 14:07:11 +0200 Subject: [PATCH 6/7] Compose the own-work notice from one answer, not two competing blocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #306 and #308 each replaced the block above Apply — one to say whose work a patch would land on, the other to say how sure the app is of the base that was measured from. Rebased together they collided, and neither should simply win: the attribution owns the file list, and the base is what says how far that list can be trusted. describePreviewNotice composes them, so the renderer picks a style and renders sentences rather than deciding between two modules. The fuller hedge is used where files are named, since the consequence — a list built against today's trunk can name files the contributor never touched and miss ones they did — is the actionable half; the quiet one stays for the case where nothing collided. describeOwnWorkWarning is superseded and goes with its per-file builders. Its cases move onto the composed answer rather than being deleted with it. Co-Authored-By: Claude Fable 5 --- src/renderer/applied-layer.cjs | 33 ++++++++++++++++++++- src/renderer/index.jsx | 32 +++++++-------------- src/renderer/ticket-base.cjs | 52 +++++++--------------------------- test/ipc-wiring.test.cjs | 10 +++---- test/ticket-base.test.cjs | 16 ++++++++--- 5 files changed, 69 insertions(+), 74 deletions(-) diff --git a/src/renderer/applied-layer.cjs b/src/renderer/applied-layer.cjs index 4716c05..fe96446 100644 --- a/src/renderer/applied-layer.cjs +++ b/src/renderer/applied-layer.cjs @@ -26,6 +26,8 @@ // directly, and neither needs a DOM. 'use strict'; +const { baseIsApproximate, UNRECORDED_CLEAR_NOTE, UNRECORDED_MEASUREMENT_NOTE } = require('./ticket-base.cjs'); + // Saving a copy and then discarding is a recommendable way forward on this // project, not a defeat: a ticket's changes are one afternoon's work on a // checkout that gets thrown away, and redoing them is cheaper than untangling @@ -200,4 +202,33 @@ function absorbedExitFailure({ patchSaveError = '', discardError = '' } = {}) { return { message: '' }; } -module.exports = { attributeConflicts, describeAppliedLayer, absorbedExitFailure, listOf, DISPOSABLE_EXIT, SLOT_HELD }; +/** + * The whole block above Apply: whose work a new patch would land on (#306), and + * how sure the app is of the base that was measured from (#308). + * + * The two were separate blocks that replaced each other, which is a choice + * neither of them should win: the attribution owns the file list, and the base + * is what says how much that list can be trusted. On an unrecorded base it can + * name files the contributor never touched and miss ones they did, so the + * sentences carry the hedge rather than standing unqualified. + * + * With nothing colliding the hedge is all that is left, and it is said quietly: + * "nothing collided" is not a promise an approximate base can make, but it is + * not an alert either. + * + * @param {Object} [root0] + * @param {string[]} [root0.conflicts] Files the incoming patch touches that the ticket has work in. + * @param {?Object} [root0.appliedPatch] The layer record, when one is applied. + * @param {?string} [root0.baseStatus] A `BASE_STATUS` value from the preview. + * @return {?{level: 'warning'|'note', sentences: string[]}} + */ +function describePreviewNotice({ conflicts = [], appliedPatch = null, baseStatus = null } = {}) { + const { sentences } = attributeConflicts({ conflicts, appliedPatch }); + const approximate = baseIsApproximate(baseStatus); + if (sentences.length) { + return { level: 'warning', sentences: approximate ? sentences.concat(UNRECORDED_MEASUREMENT_NOTE) : sentences }; + } + return approximate ? { level: 'note', sentences: [UNRECORDED_CLEAR_NOTE] } : null; +} + +module.exports = { attributeConflicts, describeAppliedLayer, describePreviewNotice, absorbedExitFailure, listOf, DISPOSABLE_EXIT, SLOT_HELD }; diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index a8914d2..b49b2d2 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -30,8 +30,7 @@ import { pathBasename } from './path-basename.cjs'; import { sanitizeSiteFolder, resolveTargetDir, directoryFromFileEntry } from './site-folder.cjs'; import { noticeForOpenResult } from './open-failure.cjs'; import { describeApplyFailure, otherPatchCount } from './apply-conflict.cjs'; -import { baseIsApproximate, UNRECORDED_CLEAR_NOTE } from './ticket-base.cjs'; -import { describeAppliedLayer, attributeConflicts, absorbedExitFailure } from './applied-layer.cjs'; +import { describeAppliedLayer, describePreviewNotice, absorbedExitFailure } from './applied-layer.cjs'; import { trunkAgeInfo, planUpdateSteps, updateStepStatuses, SKIP_INSTALL_MESSAGE, planApplySteps, planWatchImpact, APPLY_STATE_TO_STEP, planSetupSteps, SETUP_STATE_TO_STEP, setupOutcome } from './update-plan.cjs'; import { pickLatest } from '../latest-patch.cjs'; import { beginSetup, adoptSetupPath, discardSetup, rowPathAfterStatus } from './pending-setup.cjs'; @@ -2739,10 +2738,6 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit }); const applySteps = planApplySteps({ needsInstall: applyNeedsInstall, buildByWatcher: applyBuildByWatcher }); const applyStepStates = updateStepStatuses(applySteps, applyState, APPLY_STATE_TO_STEP); - // What the preview says about the contributor's own work, and how confidently - // — the preview carries the status of the base it was measured against (#308). - const applyOwnWorkNotice = applyPreview ? describeOwnWorkWarning(applyPreview) : null; - // The applied patch as a layer with a name (#306), not an undo blob. Both // answers come from the same record: whether it can still be lifted out — // measured in main on every status read, so it comes back on its own when the @@ -2750,8 +2745,12 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit const appliedLayer = describeAppliedLayer(appliedPatch, { when: appliedPatch?.appliedAt ? new Date(appliedPatch.appliedAt).toLocaleString() : '' }); - const previewAttribution = attributeConflicts({ conflicts: applyPreview?.conflicts, appliedPatch }); - const previewBaseApproximate = Boolean(applyPreview) && baseIsApproximate(applyPreview.baseStatus); + // What the preview says about the contributor's own work: whose it is (#306), + // and how confidently, since the preview carries the status of the base it was + // measured against (#308). + const previewNotice = applyPreview + ? describePreviewNotice({ conflicts: applyPreview.conflicts, appliedPatch, baseStatus: applyPreview.baseStatus }) + : null; // The absorbed exits reach the same two operations the changes note does, so // they go through the same guard: a discard is a force checkout, and running // it under a live dev server or a half-finished install rewrites the tree @@ -4777,21 +4776,10 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit
{applyPreview.paths.map((p) =>
{p}
)}
- {/* Who the colliding work belongs to (#306) is the sentence, and - how sure the app is of the base it was measured from (#308) - rides with it: on an unrecorded base the list can name files - the contributor never touched and miss ones they did, so an - unhedged sentence would overstate what was checked. With no - collisions the hedge is all that is left, said quietly — - "nothing collided" is not a promise an approximate base can - make. */} - {previewAttribution.sentences.length ? ( -
- {previewAttribution.sentences.map((sentence) =>
{sentence}
)} - {previewBaseApproximate ?
{UNRECORDED_CLEAR_NOTE}
: null} + {previewNotice ? ( +
+ {previewNotice.sentences.map((sentence) =>
{sentence}
)}
- ) : previewBaseApproximate ? ( -
{UNRECORDED_CLEAR_NOTE}
) : null} {applyPreview.unsupported.length ? (
diff --git a/src/renderer/ticket-base.cjs b/src/renderer/ticket-base.cjs index 4442c37..5582ac5 100644 --- a/src/renderer/ticket-base.cjs +++ b/src/renderer/ticket-base.cjs @@ -78,53 +78,21 @@ function baseUnreadableMessage(consequence) { return `Could not work out which trunk to compare your work against, so ${consequence}.`; } -// The warning as it has always read on a site with a recorded base. Kept whole -// and unhedged: that path is exact, and softening it would teach contributors -// to skim the one warning that is never wrong. -function recordedWarning(conflicts) { - return `You have your own edits to ${conflicts.join(', ')}. The patch is applied on top of them: it succeeds if the changes do not overlap, and fails without touching anything if they do. Save a patch of your work first if you want a copy.`; -} - -// The same warning with its uncertainty said out loud. The hedge comes first -// because it changes what the list means: these are files that differ from -// today's trunk, which on a branch born before the last update includes files -// trunk itself moved on, not the contributor. -function unrecordedWarning(conflicts) { - return `${conflicts.join(', ')} may hold your own edits. This ticket has no record of the trunk it started from, so your work was compared against today's trunk instead — that can name files you never touched, and miss ones you did. The patch is applied on top of whatever is there: it succeeds if the changes do not overlap, and fails without touching anything if they do. Save a patch of your work first if you want a copy.`; -} +// What an approximate base does to the file list beside it. Said in full where +// files are named, because the consequence is the actionable half: a list built +// against today's trunk can include files trunk itself moved on, and miss ones +// the contributor really did edit. +const UNRECORDED_MEASUREMENT_NOTE = 'This ticket has no record of the trunk it started from, so your work was compared against today\'s trunk instead — that can name files you never touched, and miss ones you did.'; -// Nothing collided, but on an approximate base "nothing collided" is not a -// promise the app can make. Said quietly rather than as an alert: there is no -// problem here yet, only a check that was less than exact. +// The same fact when nothing collided, where there is no list to qualify and no +// problem yet — only a check that was less than exact. Kept quiet for that +// reason: an alert here would cry wolf. const UNRECORDED_CLEAR_NOTE = 'This ticket has no record of the trunk it started from, so the check for your own edits was made against today\'s trunk and is approximate.'; -/** - * What the apply preview says about the contributor's own work, or null when - * there is nothing to say. - * - * `level` picks the styling and whether it is announced: `warning` is the amber - * block that has always been there, `note` is a quiet line that exists only to - * stop an approximate silence from reading as a clean bill of health. - * - * @param {Object} [root0] - * @param {string[]} [root0.conflicts] Files the patch touches that the ticket has work in. - * @param {?string} [root0.baseStatus] A `BASE_STATUS` value; anything else is treated as exact. - * @return {?{level: 'warning'|'note', text: string}} - */ -function describeOwnWorkWarning({ conflicts = [], baseStatus = BASE_STATUS.RECORDED } = {}) { - const files = Array.isArray(conflicts) ? conflicts.filter(Boolean) : []; - if (!baseIsApproximate(baseStatus)) { - return files.length ? { level: 'warning', text: recordedWarning(files) } : null; - } - return files.length - ? { level: 'warning', text: unrecordedWarning(files) } - : { level: 'note', text: UNRECORDED_CLEAR_NOTE }; -} - module.exports = { BASE_STATUS, UNRECORDED_CLEAR_NOTE, + UNRECORDED_MEASUREMENT_NOTE, baseIsApproximate, - baseUnreadableMessage, - describeOwnWorkWarning + baseUnreadableMessage }; diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index 5b497e2..52dd02c 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -34,8 +34,8 @@ const git = require('isomorphic-git'); // The two modules these tests read an answer *through* rather than around: what // the preview's base status means for the contributor is the module's to say, // and #308's point is that the two halves agree. -const { describeOwnWorkWarning } = require('../src/renderer/ticket-base.cjs'); -const { attributeConflicts } = require('../src/renderer/applied-layer.cjs'); +const { UNRECORDED_MEASUREMENT_NOTE } = require('../src/renderer/ticket-base.cjs'); +const { attributeConflicts, describePreviewNotice } = require('../src/renderer/applied-layer.cjs'); const SRC_DIR = path.join(__dirname, '..', 'src'); const MAIN_PATH = path.join(SRC_DIR, 'main.js'); @@ -1908,10 +1908,10 @@ test('git:preview-patch still measures an unrecorded base, and says it is approx assert.deepEqual(preview.conflicts, ['src/wp-signup.php']); // And that is exactly the announcement the contributor did not earn, which // is why the panel hedges it rather than stating it. - const notice = describeOwnWorkWarning(preview); + const notice = describePreviewNotice(preview); assert.equal(notice.level, 'warning'); - assert.ok(notice.text.startsWith('src/wp-signup.php may hold your own edits.')); - assert.ok(notice.text.includes('no record of the trunk it started from')); + assert.ok(notice.sentences[0].startsWith('You have your own edits to src/wp-signup.php.')); + assert.ok(notice.sentences.includes(UNRECORDED_MEASUREMENT_NOTE)); }); // The contributor's real work is still found on an unrecorded base — hedging diff --git a/test/ticket-base.test.cjs b/test/ticket-base.test.cjs index f674d88..0fbe7c3 100644 --- a/test/ticket-base.test.cjs +++ b/test/ticket-base.test.cjs @@ -7,8 +7,16 @@ const { UNRECORDED_CLEAR_NOTE, baseIsApproximate, baseUnreadableMessage, - describeOwnWorkWarning + UNRECORDED_MEASUREMENT_NOTE } = require('../src/renderer/ticket-base.cjs'); +// The preview notice these sentences end up inside is composed there (#306/#308 +// name the same block), so its cases live beside the attribution they qualify. +const { describePreviewNotice } = require('../src/renderer/applied-layer.cjs'); + +const describeOwnWorkWarning = (preview) => { + const notice = describePreviewNotice(preview || {}); + return notice ? { level: notice.level, text: notice.sentences.join(' ') } : null; +}; // The healthy path, and the one that must not move: a recorded base is exact, // so the warning reads exactly as it always has (issue #308). @@ -20,7 +28,7 @@ test('describeOwnWorkWarning: a recorded base warns without hedging (issue #308) assert.strictEqual(notice.level, 'warning'); assert.strictEqual( notice.text, - 'You have your own edits to src/wp-login.php. The patch is applied on top of them: it succeeds if the changes do not overlap, and fails without touching anything if they do. Save a patch of your work first if you want a copy.' + 'You have your own edits to src/wp-login.php. Save a patch of your work first if you want a copy. The patch is applied on top of those changes: it succeeds if they do not overlap, and fails without touching anything if they do.' ); assert.ok(!notice.text.includes('approximate')); assert.ok(!notice.text.includes('may hold')); @@ -44,8 +52,8 @@ test('describeOwnWorkWarning: an unrecorded base qualifies the warning (issue #3 baseStatus: BASE_STATUS.UNRECORDED }); assert.strictEqual(notice.level, 'warning'); - assert.ok(notice.text.startsWith('src/wp-login.php, src/wp-signup.php may hold your own edits.')); - assert.ok(notice.text.includes('no record of the trunk it started from')); + assert.ok(notice.text.startsWith('You have your own edits to src/wp-login.php and src/wp-signup.php.')); + assert.ok(notice.text.includes(UNRECORDED_MEASUREMENT_NOTE)); assert.ok(notice.text.includes('today\'s trunk')); // The advice that follows is unchanged — the hedge qualifies the list, not // what applying the patch does. From e20e1e9dcebab20c2833fdcd4a96a54daf49aa06 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Wed, 12 Aug 2026 14:05:48 +0200 Subject: [PATCH 7/7] Measure staleness from where trunk really is, not from the calendar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The amber staleness signal answered "how old is this snapshot", by date. Age is a proxy for distance from trunk and it misses both ways: a three-day-old snapshot can be dozens of commits behind in a busy week, and a two-week-old one can be nearly current. Ask the remote instead. src/trunk-remote.js reads refs/heads/trunk with git.listServerRefs — a refs lookup over protocol v2, not a fetch, so no objects are downloaded and no dependency is added. site:status starts the probe behind its reply and never awaits it, stamps the attempt before making it so a failure backs off for the full hour, and pushes the answer on trunk:remote when it lands. Opening a site never waits on the network, and offline, proxied or rate-limited all fall back to the 14-day threshold exactly as before. The decision stays pure: trunkAgeInfo takes the oid as data and returns a three-valued `behind`, and a new trunkUpdateAdvice owns every sentence the contributor reads, so the renderer holds no branching. Two things the copy is careful about. It does not nag toward destruction: an applied patch is removed by an update, so it turns the recommendation off and changes the wording rather than urging. And it says what updating fixes — the site. A ticket branch keeps the base it was born at, deliberately; carrying it forward is #305's action, and this copy must not imply otherwise. The signal also appears where it is cheapest to act on: just before a ticket is linked. Fixes #307. Co-Authored-By: Claude Fable 5 --- src/main.js | 85 ++++++++++++++++- src/preload.js | 9 ++ src/renderer/index.jsx | 82 +++++++++++++--- src/renderer/update-plan.cjs | 161 +++++++++++++++++++++++++++++--- src/trunk-remote.js | 126 +++++++++++++++++++++++++ test/ipc-wiring.test.cjs | 155 +++++++++++++++++++++++++++++- test/preload-listeners.test.cjs | 6 +- test/trunk-remote.test.cjs | 146 +++++++++++++++++++++++++++++ test/update-plan.test.cjs | 147 +++++++++++++++++++++++++++++ 9 files changed, 888 insertions(+), 29 deletions(-) create mode 100644 src/trunk-remote.js create mode 100644 test/trunk-remote.test.cjs diff --git a/src/main.js b/src/main.js index 1dacbfc..864c493 100644 --- a/src/main.js +++ b/src/main.js @@ -29,6 +29,7 @@ const { buildMenuTemplate } = require('./menu'); const { killChildTree } = require('./kill-tree'); const { normalizeEol } = require('./git-update.cjs'); const { ensureAutocrlf, readTrunkInfo, collectDirtyFiles, discardChanges, discardToBase, updateToLatestTrunk } = require('./trunk-update'); +const { remoteProbeDue, readRemoteTrunkOid } = require('./trunk-remote'); const { applyPatchToDir, diagnoseRemoval } = require('./patch-apply'); const { parsePatchFiles, planApply } = require('./patch-plan.cjs'); const { BASE_STATUS, baseIsApproximate, baseUnreadableMessage } = require('./renderer/ticket-base.cjs'); @@ -66,6 +67,11 @@ const SWITCH_PROGRESS_CHANNEL = 'switch:progress'; // step of an operation, and describing it as progress would have the panel say // "Saving your work…" about trunk — which is the one thing this refuses to do. const CARRIED_WORK_CHANNEL = 'ticket:carried-work'; +// The probe's answer, pushed when it lands (#307). site:status is not polled — +// it is read on mount and after the long operations — so without this the oid +// would sit in the site record until the next launch and the signal would +// always be a session late. +const REMOTE_TRUNK_CHANNEL = 'trunk:remote'; const { parseTicketRef } = require('./renderer/trac-ticket.cjs'); const { parseHandle } = require('./wporg-handle.cjs'); const { parseEventName, buildProvenanceHeader, handoffFilename } = require('./patch-provenance.cjs'); @@ -846,6 +852,61 @@ async function mergeSiteMeta(sitePath, patch) { s.set('siteMeta', meta); } +/** + * Refresh this site's record of where trunk is on the remote (#307). + * + * Started, never awaited: `site:status` is what a site being opened waits on, + * so a network round trip inside it would make opening a site depend on the + * network — the one thing this must not cost. The answer is written to the site + * record *and* pushed on REMOTE_TRUNK_CHANNEL, because `site:status` is not on + * a timer: it is read on mount and after the long operations, so a stored-only + * answer would first be read on the next launch. + * + * The attempt is stamped *before* it runs, which does two jobs at once: a + * `site:status` a moment later (mount, then an install finishing) sees a fresh + * stamp and does not launch a second probe, and a failure backs off for the + * whole interval instead of retrying on every call. Offline is not an error + * condition to retry out of — it is a normal state for this app's users, and + * the calendar fallback already covers it. + * + * The failure is logged to the app log and nowhere else, deliberately. The "no + * silent catch" rule exists so a contributor is never left with a button that + * did nothing; nobody pressed anything here, and streaming "could not reach + * github.com" into the terminal of an offline contributor every hour would be + * noise about a thing that is working as designed. + * + * @param {string} sitePath + * @param {?Object} sender The renderer to push the answer to, when it lands. + * @return {Promise} + */ +async function refreshRemoteTrunk(sitePath, sender) { + // The probe holds this path across a network wait the site can be deleted + // during, and mergeSiteMeta creates the key it writes to. Without this the + // answer would resurrect the record `sites:delete` just removed, leaving a + // phantom entry for a directory that is gone — and a site later created at + // the same path would adopt its oid and skip its first probe. + const stillRegistered = async () => { + const store = await getStore(); + return (store.get('sites') || []).includes(sitePath); + }; + try { + if (!await stillRegistered()) return; + await mergeSiteMeta(sitePath, { remoteTrunkCheckedAt: new Date().toISOString() }); + // `null` is stored, not skipped: an answer of "this remote has no trunk" + // has to be able to clear an oid an earlier probe stored, or a site + // would compare against a commit nothing will ever match and report + // itself behind for good. + const remoteTrunkOid = await readRemoteTrunkOid({ url: WORDPRESS_GIT_URL }); + if (!await stillRegistered()) return; + await mergeSiteMeta(sitePath, { remoteTrunkOid }); + try { + if (sender && !sender.isDestroyed()) sender.send(REMOTE_TRUNK_CHANNEL, { sitePath, remoteTrunkOid }); + } catch {} + } catch (e) { + logError('trunk-remote', `could not read trunk from the remote: ${String(e && e.message ? e.message : e)}`); + } +} + // --- Ticket branches (#108) --- git mechanics live in src/ticket-branches.js; // what follows is the electron-store half: which branch is active and what // context each one carries. @@ -1341,7 +1402,13 @@ ipcMain.handle('git:update-trunk', async (event, sitePath) => { // the dirty-tree modal always discards and then updates.) await mergeSiteMeta(sitePath, { trunkOid: result.upToDate ? result.oldOid : result.newOid, - trunkDate: result.trunkDate + trunkDate: result.trunkDate, + // The fetch just asked the remote where trunk is, which is the + // same question the background probe asks (#307) — recording it + // here means the staleness signal clears the moment the update + // finishes, instead of an hour later when the probe next runs. + remoteTrunkOid: result.newOid, + remoteTrunkCheckedAt: new Date().toISOString() }); // HEAD has moved but install/build have not run yet: persist the // incomplete flag now so the state survives a crash or quit @@ -1719,7 +1786,7 @@ ipcMain.handle('sites:getAll', async () => { return { sites: s.get('sites'), siteMeta: s.get('siteMeta') }; }); -ipcMain.handle('site:status', async (_e, sitePath) => { +ipcMain.handle('site:status', async (event, sitePath) => { try { const nmDir = path.join(sitePath, 'node_modules'); const hasNodeModules = fs.existsSync(nmDir) && (() => { try { return fs.readdirSync(nmDir).length > 0; } catch { return false; } })(); @@ -1745,6 +1812,16 @@ ipcMain.handle('site:status', async (_e, sitePath) => { } } catch {} + // Where trunk is on the remote (#307), which is what actually decides + // staleness — the snapshot's age is only the offline fallback. The + // stored answer is returned as it stands and the refresh runs behind + // this reply, so nothing on this path touches the network before + // returning. See refreshRemoteTrunk. + const remoteTrunkOid = m.remoteTrunkOid || null; + if (remoteProbeDue({ checkedAt: m.remoteTrunkCheckedAt })) { + void refreshRemoteTrunk(sitePath, event && event.sender); + } + // The applied patch and the incomplete-update flag belong to the ticket // being worked on, not to the site (#108) — otherwise switching tickets // would carry the other one's "patch applied · Revert" banner over, and @@ -1791,9 +1868,9 @@ ipcMain.handle('site:status', async (_e, sitePath) => { } : null; - return { hasNodeModules, hasBuilt, skipInitWizard: Boolean(m.skipInitWizard), initialized: Boolean(m.initialized), installFailed: Boolean(m.installFailed), trunkOid, trunkDate, updateIncomplete: Boolean(work.updateIncomplete), tracTicket: m.tracTicket || null, appliedPatch }; + return { hasNodeModules, hasBuilt, skipInitWizard: Boolean(m.skipInitWizard), initialized: Boolean(m.initialized), installFailed: Boolean(m.installFailed), trunkOid, trunkDate, remoteTrunkOid, updateIncomplete: Boolean(work.updateIncomplete), tracTicket: m.tracTicket || null, appliedPatch }; } catch { - return { hasNodeModules: false, hasBuilt: false, skipInitWizard: false, initialized: false, installFailed: false, trunkOid: null, trunkDate: null, updateIncomplete: false, tracTicket: null, appliedPatch: null }; + return { hasNodeModules: false, hasBuilt: false, skipInitWizard: false, initialized: false, installFailed: false, trunkOid: null, trunkDate: null, remoteTrunkOid: null, updateIncomplete: false, tracTicket: null, appliedPatch: null }; } }); diff --git a/src/preload.js b/src/preload.js index 780f557..c63b329 100644 --- a/src/preload.js +++ b/src/preload.js @@ -98,6 +98,15 @@ contextBridge.exposeInMainWorld('api', { ipcRenderer.on('ticket:carried-work', h); return () => ipcRenderer.removeListener('ticket:carried-work', h); } +, + // Where trunk is on the remote (#307), pushed when the probe lands rather + // than waited for: site:status answers immediately with whatever was last + // known, and this arrives a moment later if it changed anything. + subscribeRemoteTrunk: (handler) => { + const h = (_e, payload) => handler && handler(payload); + ipcRenderer.on('trunk:remote', h); + return () => ipcRenderer.removeListener('trunk:remote', h); + } , subscribeSetupProgress: (handler) => { const h = (_e, payload) => handler && handler(payload); diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index b49b2d2..329c6d8 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -31,7 +31,7 @@ import { sanitizeSiteFolder, resolveTargetDir, directoryFromFileEntry } from './ import { noticeForOpenResult } from './open-failure.cjs'; import { describeApplyFailure, otherPatchCount } from './apply-conflict.cjs'; import { describeAppliedLayer, describePreviewNotice, absorbedExitFailure } from './applied-layer.cjs'; -import { trunkAgeInfo, planUpdateSteps, updateStepStatuses, SKIP_INSTALL_MESSAGE, planApplySteps, planWatchImpact, APPLY_STATE_TO_STEP, planSetupSteps, SETUP_STATE_TO_STEP, setupOutcome } from './update-plan.cjs'; +import { trunkAgeInfo, trunkUpdateAdvice, planUpdateSteps, updateStepStatuses, SKIP_INSTALL_MESSAGE, planApplySteps, planWatchImpact, APPLY_STATE_TO_STEP, planSetupSteps, SETUP_STATE_TO_STEP, setupOutcome } from './update-plan.cjs'; import { pickLatest } from '../latest-patch.cjs'; import { beginSetup, adoptSetupPath, discardSetup, rowPathAfterStatus } from './pending-setup.cjs'; import { parsePrRef } from '../patch-sources.cjs'; @@ -784,13 +784,17 @@ function App() { // Staleness surfaces in the sidebar before the site is even // opened (#94): amber = old trunk snapshot, red = an update that // moved trunk but never finished install/build. - const trunkAge = trunkAgeInfo({ trunkDate: meta.trunkDate }); + const trunkAge = trunkAgeInfo({ trunkDate: meta.trunkDate, trunkOid: meta.trunkOid, remoteTrunkOid: meta.remoteTrunkOid }); + // The dot is shown whenever trunk has moved on, including when the + // site has work that updating would disturb: the advice below it is + // what softens, never the fact. + const trunkAdvice = trunkUpdateAdvice({ trunkAge }); let staleDotColor = null; if (meta.updateIncomplete) staleDotColor = '#d63638'; else if (trunkAge.stale) staleDotColor = '#dba617'; const staleDotTitle = meta.updateIncomplete ? 'Update incomplete — code is new, built assets are old' - : `WordPress code is ${trunkAge.ageDays} days old — update to latest trunk`; + : trunkAdvice.dotTitle; const staleDot = staleDotColor ? ( { loadStatus(); }, [loadStatus]); + // The remote probe (#307) answers after site:status has already replied — it + // is started behind that reply so opening a site never waits on the network. + // Without this subscription the answer would only be read on the next launch, + // because site:status is called on mount and after long operations, not on a + // timer. Two consumers: this card, and the sidebar dot through the meta patch. + useEffect(() => { + const unsub = window.api.subscribeRemoteTrunk((p) => { + if (!p || p.sitePath !== sitePath) return; + setRemoteTrunkOid(p.remoteTrunkOid || null); + if (metaPatchRef.current) metaPatchRef.current(sitePath, { remoteTrunkOid: p.remoteTrunkOid || null }); + }); + return () => { if (unsub) unsub(); }; + }, [sitePath]); + // Deliberately not part of loadStatus: that one is called after every long // operation, and the branch list only changes when a ticket is linked, // resumed or deleted — the three paths that call this themselves. @@ -2665,7 +2690,14 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit ); // --- Update to latest trunk (#94) --- - const age = trunkAgeInfo({ trunkDate }); + const age = trunkAgeInfo({ trunkDate, trunkOid, remoteTrunkOid }); + // Every sentence the contributor reads about staleness, and whether the app + // pushes at all — it stays quiet when updating would cost them something. + const trunkAdvice = trunkUpdateAdvice({ + trunkAge: age, + appliedPatch: Boolean(appliedPatch), + ticketLinked: Boolean(tracTicket) + }); const isUpdating = updateState !== 'idle'; // Where the note goes moves with the ticket: a change that belongs to // #12345 is news for the ticket card, one that belongs to nothing is news @@ -4047,7 +4079,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit applyPreview: Boolean(applyPreview), updateIncomplete, isUpdating, - stale: age.stale, + stale: trunkAdvice.recommendUpdate, running, hasChanges: Boolean(worktreeDirty && worktreeDirty.dirty), ticketLinked: Boolean(tracTicket) @@ -4100,7 +4132,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit {age.stale ? (
) : null} - {age.stale && !updateIncomplete && !isUpdating ? ( + {trunkAdvice.recommendUpdate && !updateIncomplete && !isUpdating ? (
- This site's WordPress code is {age.ageDays} days old - Patches you create now may not apply on Trac. Updating takes a few minutes. + {trunkAdvice.headline} + {trunkAdvice.detail} Updating takes a few minutes.
) : null} + {trunkAdvice.atRisk && !updateIncomplete && !isUpdating ? ( + /* The same fact, without the push. Updating resets the working tree, so + urging it at someone holding an applied patch or unsaved edits would + be nagging them toward losing it — but hiding that trunk has moved + would leave them to find out from a patch that will not apply. So it + is stated, in the panel's own neutral voice, with no button: the + control is still in the header menu when they want it. */ +
+ {trunkAdvice.headline} + {trunkAdvice.detail} +
+ ) : null} {isUpdating ? (
@@ -4697,6 +4741,22 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit app asks before moving or discarding anything, so this only has to be true, not load-bearing. Said without asking the worktree, so it costs nothing. */} + {/* The one moment the staleness signal is worth interrupting for + (#307): a ticket linked onto trunk that has already moved starts + behind, and every patch written on it inherits that. Updating + afterwards would not move the ticket — that is #305's action — + so this is said here or not at all. */} + {trunkAdvice.preLinkNote ? ( +
+ {trunkAdvice.preLinkNote} + +
+ ) : null}
If you have edited anything already, you will be asked what should happen to those edits.
@@ -5169,7 +5229,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit
{!patchLoading && age.stale && (
- This site's WordPress code is {age.ageDays} days old — this patch may not apply on Trac. Consider updating to the latest trunk first. + {trunkAdvice.headline} — this patch may not apply on Trac. Consider updating to the latest trunk first.
)} {!patchLoading && !patchHasChanges && ( diff --git a/src/renderer/update-plan.cjs b/src/renderer/update-plan.cjs index b60bd9d..55c72d0 100644 --- a/src/renderer/update-plan.cjs +++ b/src/renderer/update-plan.cjs @@ -16,33 +16,169 @@ * (same convention as setup-steps.cjs and dev-server-command.cjs). */ -// A site older than this shows the staleness dot and notice. Local-only: -// staleness is judged from the snapshot's own age, never from a network probe, -// so it works offline and never talks to GitHub on app launch. A spuriously -// stale site just gets "Already up to date." when the user clicks Update. +// How old a snapshot has to be before the calendar alone calls it stale. +// +// This used to be the *only* test, and the invariant written here said so: +// staleness was judged from the snapshot's own age and never from a network +// probe. That was chosen so the dot worked offline — but it answered the wrong +// question. Age is a proxy for distance from trunk, and it misses in both +// directions: a three-day-old snapshot can be dozens of commits behind in a +// busy week, and a two-week-old one can be nearly current. +// +// So since #307 the primary test is the remote's own answer: `src/trunk-remote.js` +// asks where `refs/heads/trunk` is (a refs lookup, not a fetch) in the main +// process, and the resulting oid arrives here as plain data — this module stays +// pure and DOM-free, and does no I/O of its own. +// +// The threshold survives as the fallback, and that is what keeps the offline +// promise intact: with no probe answer (offline, proxied, rate-limited, or +// simply not asked yet) the calendar decides exactly as it did before, and +// nothing waits on the network to render. const STALE_THRESHOLD_DAYS = 14; const DAY_MS = 24 * 60 * 60 * 1000; /** - * Describes the age of a site's trunk snapshot from its stored commit date. - * Unknown or invalid dates are never reported stale — a missing date means an + * Describes a site's trunk snapshot: how old it is, and — when the remote has + * been asked — whether trunk has actually moved past it. + * + * Unknown or invalid dates are never reported stale: a missing date means an * older site record, not an old checkout. * - * @param {Object} root0 - * @param {string} [root0.trunkDate] - * @param {number} [root0.now] + * `behind` is deliberately three-valued, because "we asked and trunk is where + * this snapshot is" and "we could not ask" are different facts and lead to + * different sentences: + * + * - `true` — the remote's trunk oid differs from this snapshot's. Stale, + * whatever the calendar says. + * - `false` — the remote's trunk oid is this snapshot's. NOT stale, even if the + * snapshot is months old: a quiet trunk is not an out-of-date one. + * - `null` — no answer to compare against; the calendar decides. + * + * A differing oid is read as "the remote has moved on" rather than "the two + * have diverged" because the app only ever advances local `trunk` by fetching + * this same remote — it commits contributor work to ticket branches (#108), + * never to trunk. + * + * @param {Object} root0 + * @param {string} [root0.trunkDate] Committer date of the local snapshot. + * @param {?string} [root0.trunkOid] Commit local `trunk` points at. + * @param {?string} [root0.remoteTrunkOid] Commit the remote's trunk points at, + * from the last successful probe. + * @param {number} [root0.now] */ -function trunkAgeInfo({ trunkDate, now = Date.now() } = {}) { +function trunkAgeInfo({ trunkDate, trunkOid, remoteTrunkOid, now = Date.now() } = {}) { + const comparable = Boolean(trunkOid) && Boolean(remoteTrunkOid); + const behind = comparable ? trunkOid !== remoteTrunkOid : null; + const ts = trunkDate ? Date.parse(trunkDate) : NaN; if (!Number.isFinite(ts)) { - return { known: false, ageDays: null, stale: false, label: '' }; + // No date to show and none to judge by. The probe can still answer, and + // when it does it is the whole answer — this is the site record that + // predates the date being stored, not a young checkout. + return { known: false, ageDays: null, stale: behind === true, label: '', behind, source: comparable ? 'remote' : 'calendar' }; } const ageDays = Math.max(0, Math.floor((now - ts) / DAY_MS)); const label = `trunk as of ${new Date(ts).toLocaleDateString(undefined, { year: 'numeric', month: 'short', day: 'numeric' })}`; - return { known: true, ageDays, stale: ageDays > STALE_THRESHOLD_DAYS, label }; + const stale = comparable ? behind : ageDays > STALE_THRESHOLD_DAYS; + return { known: true, ageDays, stale, label, behind, source: comparable ? 'remote' : 'calendar' }; +} + +/** + * What the app should say about a stale snapshot, and whether it should say it + * loudly. Every string the contributor reads about staleness is decided here, + * so the renderer holds none of the branching. + * + * Two things this copy is careful about, and both are the reason it is a + * function rather than a template in the view: + * + * **It does not nag toward destruction.** Updating parks the ticket and resets + * the working tree, which takes an applied patch off disk with it (see the + * applied-patch clearing in `src/main.js`'s `git:update-trunk`). So an applied + * patch turns `recommendUpdate` false: the dot and the label still say trunk has + * moved — the contributor is not kept in the dark — but the amber "you should + * update" block and the next-action cue stay away, and the wording says what + * updating would cost instead of urging it. + * + * Uncommitted edits are deliberately NOT treated the same way, and the + * difference is not an oversight. An applied patch is removed by an update + * without being asked about; edits are not — `startTrunkUpdate` asks the narrow + * "is the worktree dirty" question first and opens a dialog that offers saving + * them (#234). Work that cannot be lost without a prompt is not a reason to go + * quiet, so it is stated in the copy instead of suppressing the advice. Which + * also keeps this function away from a measure it would get wrong: the value + * the panel holds is the branch-point one (#239), and it counts parked work a + * force checkout survives. + * + * **It says what updating actually fixes: the site.** "Update to latest trunk" + * moves the site's copy of WordPress. It does not move a ticket branch, which + * keeps the base it was born at on purpose — a ticket's diff would otherwise + * swallow everything trunk moved. Carrying a ticket forward is its own action + * (#305), so the copy must never imply this one does it. + * + * The one moment worth interrupting for is *before* a ticket exists, which is + * what `preLinkNote` is: updating first is the cheapest way to stop a ticket + * being born on trunk it will have to fight later. + * + * @param {Object} root0 + * @param {Object} root0.trunkAge The `trunkAgeInfo` result. + * @param {boolean} [root0.appliedPatch] A patch is applied on this ticket. + * @param {boolean} [root0.ticketLinked] A ticket is linked to this site. + * @return {{recommendUpdate: boolean, atRisk: boolean, headline: string, detail: string, dotTitle: string, preLinkNote: string}} + */ +function trunkUpdateAdvice({ trunkAge, appliedPatch, ticketLinked } = {}) { + const age = trunkAge || {}; + const quiet = { recommendUpdate: false, atRisk: false, headline: '', detail: '', dotTitle: '', preLinkNote: '' }; + if (!age.stale) return quiet; + + const atRisk = Boolean(appliedPatch); + + // The probe's answer is a fact about trunk; the calendar's is a fact about + // the snapshot. Say whichever one was actually established. + const headline = age.behind === true + ? 'Trunk has moved since this snapshot' + : `This site\u2019s WordPress code is ${age.ageDays} days old`; + + // The dot's tooltip is the one place the signal appears with no room for the + // detail below it, so it carries the reason not to act when there is one: + // suggesting an update to someone holding a patch it would remove is the + // nag this function exists to avoid, and a tooltip is no exception. + let dotTitle; + if (atRisk) { + dotTitle = age.behind === true + ? 'Trunk has moved since this snapshot — updating would remove the patch you applied' + : `WordPress code is ${age.ageDays} days old — updating would remove the patch you applied`; + } else { + dotTitle = age.behind === true + ? 'Trunk has moved since this snapshot — update this site to latest trunk' + : `WordPress code is ${age.ageDays} days old — update to latest trunk`; + } + + // Always true, and true whether or not there are edits right now: the update + // path asks before it resets anything. Saying it unconditionally is what + // lets this function stay out of the business of measuring the worktree. + const cost = appliedPatch + ? 'It also resets the working tree, so the patch you have applied would be removed. Revert it first if you still need it.' + : 'It also resets the working tree, so any edits you have not written down yet are asked about first.'; + + const detail = [ + 'Updating brings this site\u2019s copy of WordPress up to date, so patches you write are measured against current trunk.', + ticketLinked + ? 'The ticket you have linked keeps the trunk it was created from — updating the site does not move it.' + : '', + cost + ].filter(Boolean).join(' '); + + // Only before a ticket exists, and only when updating is a safe thing to + // suggest: this is the one prompt that claims a moment of the contributor's + // attention rather than waiting to be read. + const preLinkNote = (!ticketLinked && !atRisk) + ? `${headline}. Updating first means this ticket is not born behind.` + : ''; + + return { recommendUpdate: !atRisk, atRisk, headline, detail, dotTitle, preLinkNote }; } const SKIP_INSTALL_MESSAGE = 'Dependencies unchanged — skipping npm install'; @@ -236,6 +372,7 @@ module.exports = { planWatchImpact, planSetupSteps, trunkAgeInfo, + trunkUpdateAdvice, planUpdateSteps, updateStepStatuses, setupOutcome, diff --git a/src/trunk-remote.js b/src/trunk-remote.js new file mode 100644 index 0000000..56aec05 --- /dev/null +++ b/src/trunk-remote.js @@ -0,0 +1,126 @@ +'use strict'; + +/** + * Where trunk really is on the remote (#307). + * + * The staleness signal used to be a calendar reading of the site's own + * snapshot, and age is a proxy that misses in both directions: a three-day-old + * snapshot can be dozens of commits behind in a busy week, and a two-week-old + * one can be nearly current. This module answers the question the app was + * actually asking — has trunk moved since this snapshot — by asking the remote + * for one ref. + * + * It is a refs lookup (`git.listServerRefs`, protocol v2 `ls-refs`), not a + * fetch: no objects are downloaded, no dependency is added, and the answer is a + * single 40-character oid. All git I/O goes through isomorphic-git, as + * everywhere else in this app — nothing shells out to a git binary. + * + * Two things this module deliberately does NOT do: + * + * - **It never says by how many commits.** The refs protocol carries oids, not + * distances; counting would mean downloading the objects between them, which + * is the fetch this exists to avoid. "Trunk has moved" is what can be known + * for free, and it is already truer than a date. + * - **It never decides anything.** Offline, behind a proxy, rate-limited or + * air-gapped are all normal states for a Contributor Day laptop, and this + * module reports them as a rejection rather than absorbing them. Absorbing + * them is `refreshRemoteTrunk`'s job in src/main.js, precisely because that + * is where "unknown" becomes "fall back to the calendar". + */ + +const git = require('isomorphic-git'); +const http = require('isomorphic-git/http/node'); + +const TRUNK_REF = 'refs/heads/trunk'; + +/** + * How long a probe's answer is trusted before another one is worth making. + * + * One hour, because the signal it feeds is measured in days: a contributor is + * told trunk has moved so they update before writing a patch, and an answer + * that is up to an hour old never changes that recommendation. It also bounds + * the traffic: `site:status` is read on mount and again after every install, + * build, update, apply and ticket switch, and without a stamp every one of + * those would be a request. + */ +const REMOTE_PROBE_INTERVAL_MS = 60 * 60 * 1000; + +// How long one probe may take before it is abandoned. Generous, because a slow +// answer is still a useful one and nothing is waiting on it; bounded, because a +// request that never settles is the one failure mode a captive portal produces. +const PROBE_TIMEOUT_MS = 15 * 1000; + +/** + * Whether the remote is worth asking again. + * + * A never-probed site (no stamp, or a stamp from an older app version that did + * not write one) is always due. A stamp in the future is treated as due too: + * that is a clock that moved backwards, and the alternative is a site that + * never probes again until the stamp's hour arrives. + * + * @param {Object} root0 + * @param {?string} [root0.checkedAt] ISO stamp of the last probe attempt. + * @param {number} [root0.now] + * @param {number} [root0.intervalMs] + * @return {boolean} + */ +function remoteProbeDue({ checkedAt, now = Date.now(), intervalMs = REMOTE_PROBE_INTERVAL_MS } = {}) { + const last = checkedAt ? Date.parse(checkedAt) : NaN; + if (!Number.isFinite(last)) return true; + if (last > now) return true; + return now - last >= intervalMs; +} + +/** + * The oid `refs/heads/trunk` points at on the remote, or null if the remote + * answered but has no such branch. + * + * A remote that could not be reached rejects rather than resolving null, so the + * caller can tell "trunk is gone" from "we could not ask" — the second is the + * one that must leave the calendar fallback in charge. `refreshRemoteTrunk` in + * src/main.js is where that rejection is absorbed. + * + * `prefix` narrows the server's answer to the one ref, so a repository with + * tens of thousands of refs — wordpress-develop's `refs/pull/*` alone is + * enormous — still costs one small response. It is a prefix and not an exact + * match, though: `refs/heads/trunk-experiment` comes back under it too, which + * is why the row is picked by full ref name rather than taken as the first. + * + * The deadline is not belt-and-braces. The network this app runs on is a + * conference or café one, and the characteristic failure there is a captive + * portal that black-holes the connection rather than refusing it — + * `listServerRefs` has no deadline of its own, so without this a probe could + * stay pending for the whole session while the next hour's probe starts behind + * it. + * + * `listServerRefs` is injectable so the exact-ref filtering and the deadline can + * be tested without a network, which is the house pattern for platform- and + * environment-dependent code in this repo. + * + * @param {Object} root0 + * @param {string} root0.url Clone URL of the remote. + * @param {Function} [root0.listServerRefs] Seam for tests. + * @param {number} [root0.timeoutMs] Deadline for the whole request. + * @return {Promise} + */ +async function readRemoteTrunkOid({ url, listServerRefs = git.listServerRefs, timeoutMs = PROBE_TIMEOUT_MS } = {}) { + let timer; + const deadline = new Promise((_resolve, reject) => { + timer = setTimeout(() => reject(new Error(`no answer from ${url} within ${timeoutMs}ms`)), timeoutMs); + }); + const request = listServerRefs({ http, url, prefix: TRUNK_REF }); + // The race can be decided by the deadline while the request is still in + // flight; a rejection arriving after that has nobody left to await it, and + // Node treats an unhandled rejection as fatal. + request.catch(() => {}); + let refs; + try { + refs = await Promise.race([request, deadline]); + } finally { + clearTimeout(timer); + } + const match = (refs || []).find((r) => r && r.ref === TRUNK_REF); + return (match && match.oid) || null; +} + +module.exports = { REMOTE_PROBE_INTERVAL_MS, PROBE_TIMEOUT_MS, TRUNK_REF, remoteProbeDue, readRemoteTrunkOid }; diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index 52dd02c..e8b50d6 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -218,6 +218,13 @@ function spy(implementation = () => undefined) { return fn; } +// The default `readRemoteTrunkOid`. Rejecting rather than resolving null is on +// purpose: the probe's failure path is what the app takes offline, so the +// default stub puts every unrelated test on the honest side of it. +const NEVER_REACH_THE_NETWORK = async () => { + throw new Error('the suite does not talk to the network'); +}; + // Returns the recorders plus `invoke`, which calls a handler the way ipcMain // would. function loadMain({ stubs = {} } = {}) { @@ -247,7 +254,14 @@ function loadMain({ stubs = {} } = {}) { try { // Inside the hook, deliberately: building the stubs requires the real // modules, and src/logging.js requires `electron`. - resolveStubs(stubs, stubbed); + // + // `./trunk-remote` is stubbed by default and overridable, for the same + // reason `electron` is replaced outright: `site:status` starts a remote + // refs lookup behind its reply (#307), so every one of the many + // `site:status` tests below would otherwise reach github.com — slowly, + // flakily, and invisibly, since the call is deliberately not awaited. + // A test that wants to exercise the probe passes its own stub. + resolveStubs({ './trunk-remote': { readRemoteTrunkOid: NEVER_REACH_THE_NETWORK }, ...stubs }, stubbed); require(MAIN_PATH); } finally { Module._load = originalLoad; @@ -436,6 +450,145 @@ test('site:status reports the trunk snapshot trunk-update read, not its own gues assert.equal(settings.values.siteMeta['/sites/wp'].trunkOid, 'abc123'); }); +// --- site:status -> src/trunk-remote.js (#307) --------------------------- + +// The probe is started behind the reply and never awaited, so the assertion has +// to wait for the store write rather than for the handler. +async function waitFor(condition, message) { + for (let i = 0; i < 200; i++) { + if (condition()) return; + await new Promise((resolve) => setTimeout(resolve, 5)); + } + assert.fail(message); +} + +test('site:status asks trunk-remote where trunk is, without making the reply wait for it (issue #307)', async () => { + let released; + const inFlight = new Promise((resolve) => { released = resolve; }); + const readRemoteTrunkOid = spy(async () => { + await inFlight; + return 'remote-oid'; + }); + const settings = fakeSettingsStore({ sites: ['/sites/wp'], siteMeta: { '/sites/wp': {} } }); + const main = loadMain({ + stubs: { + ...silentLogging(), + ...settings.stubs, + './trunk-update': { readTrunkInfo: async () => ({ trunkOid: 'local-oid', trunkDate: '2026-01-01T00:00:00Z' }) }, + './trunk-remote': { readRemoteTrunkOid } + } + }); + + // The probe has not answered yet — and must not be able to hold this up, + // because opening a site waits on this handler. + const event = createIpcEvent(); + const status = await main.invokeWith('site:status', event, '/sites/wp'); + assert.equal(status.remoteTrunkOid, null, 'the reply must not wait on the probe'); + assert.equal(status.trunkOid, 'local-oid'); + assert.equal(readRemoteTrunkOid.calls.length, 1, 'the handler never reached trunk-remote'); + assert.equal(readRemoteTrunkOid.calls[0][0].url, 'https://github.com/WordPress/wordpress-develop.git'); + + released(); + await waitFor( + () => settings.values.siteMeta['/sites/wp'].remoteTrunkOid === 'remote-oid', + 'the probe answer was never written to the site record' + ); + + // And pushed. site:status is read on mount and after long operations, never + // on a timer, so without this send the answer would sit in the store until + // the next launch and the signal would always be a session late. + await waitFor(() => event.sent.length > 0, 'the probe answer was never sent to the renderer'); + assert.deepEqual(event.sent[0], { + channel: 'trunk:remote', + payload: { sitePath: '/sites/wp', remoteTrunkOid: 'remote-oid' } + }); + + // A later status read agrees with what was pushed. + const next = await main.invoke('site:status', '/sites/wp'); + assert.equal(next.remoteTrunkOid, 'remote-oid'); +}); + +test('a remote that answers with no trunk clears the oid rather than keeping the old one (issue #307)', async () => { + const readRemoteTrunkOid = spy(async () => null); + const settings = fakeSettingsStore({ + sites: ['/sites/wp'], + siteMeta: { '/sites/wp': { remoteTrunkOid: 'oid-from-an-earlier-probe' } } + }); + const main = loadMain({ + stubs: { + ...silentLogging(), + ...settings.stubs, + './trunk-update': { readTrunkInfo: async () => ({ trunkOid: 'local-oid', trunkDate: '2026-01-01T00:00:00Z' }) }, + './trunk-remote': { readRemoteTrunkOid } + } + }); + + await main.invoke('site:status', '/sites/wp'); + + // Keeping the stale oid would leave the site comparing against a commit + // nothing will ever match, reporting itself behind for good. This is the one + // case that must not be treated like an unreachable remote. + await waitFor( + () => settings.values.siteMeta['/sites/wp'].remoteTrunkOid === null, + 'a null answer left the previous oid in place' + ); +}); + +test('a probe that fails is silent, and site:status still answers from the calendar (issue #307)', async () => { + const logError = spy(); + const readRemoteTrunkOid = spy(async () => { throw new Error('getaddrinfo ENOTFOUND github.com'); }); + const settings = fakeSettingsStore({ sites: ['/sites/wp'], siteMeta: { '/sites/wp': {} } }); + const main = loadMain({ + stubs: { + ...silentLogging(), + './logging': { ...silentLogging()['./logging'], logError }, + ...settings.stubs, + './trunk-update': { readTrunkInfo: async () => ({ trunkOid: 'local-oid', trunkDate: '2026-01-01T00:00:00Z' }) }, + './trunk-remote': { readRemoteTrunkOid } + } + }); + + // Offline is a normal state, not a failure to report: the handler answers as + // it always did, and the age fields the calendar fallback reads are intact. + const status = await main.invoke('site:status', '/sites/wp'); + assert.equal(status.remoteTrunkOid, null); + assert.equal(status.trunkDate, '2026-01-01T00:00:00Z'); + + await waitFor( + () => logError.calls.length > 0, + 'the failure was swallowed without even reaching the app log' + ); + assert.equal(logError.calls[0][0], 'trunk-remote'); + // Never a stored answer from a failed probe — a wrong oid would report a + // site as up to date forever. + assert.equal(settings.values.siteMeta['/sites/wp'].remoteTrunkOid, undefined); +}); + +test('the probe is throttled, not run on every site:status (issue #307)', async () => { + const readRemoteTrunkOid = spy(async () => 'remote-oid'); + const settings = fakeSettingsStore({ sites: ['/sites/wp'], siteMeta: { '/sites/wp': {} } }); + const main = loadMain({ + stubs: { + ...silentLogging(), + ...settings.stubs, + './trunk-update': { readTrunkInfo: async () => ({ trunkOid: 'local-oid', trunkDate: '2026-01-01T00:00:00Z' }) }, + './trunk-remote': { readRemoteTrunkOid } + } + }); + + await main.invoke('site:status', '/sites/wp'); + await waitFor( + () => Boolean(settings.values.siteMeta['/sites/wp'].remoteTrunkOid), + 'the first probe never completed' + ); + + // site:status is read on mount and again after every install, build, update, + // apply and ticket switch. Only the real throttle, read from the stamp the + // probe wrote, keeps each of those from being a request to github.com. + for (let i = 0; i < 5; i++) await main.invoke('site:status', '/sites/wp'); + assert.equal(readRemoteTrunkOid.calls.length, 1, 'the probe ran again inside its interval'); +}); + // --- git:* -> src/trunk-update.js ---------------------------------------- test('git:worktree-dirty reports what trunk-update found, not its own guess', async () => { diff --git a/test/preload-listeners.test.cjs b/test/preload-listeners.test.cjs index 1883dca..1b52774 100644 --- a/test/preload-listeners.test.cjs +++ b/test/preload-listeners.test.cjs @@ -466,7 +466,11 @@ const SUBSCRIPTIONS = [ { name: 'subscribeSwitchProgress', channel: 'switch:progress' }, { name: 'subscribeCarriedWork', channel: 'ticket:carried-work' }, { name: 'subscribeSetupProgress', channel: 'download:progress' }, - { name: 'subscribeSetupStatus', channel: 'download:status' } + { name: 'subscribeSetupStatus', channel: 'download:status' }, + // The remote-trunk probe's answer (#307), and the first channel where every + // mounted SiteRow is a live subscriber at once — which is precisely the leak + // shape described above, so it belongs in this list rather than beside it. + { name: 'subscribeRemoteTrunk', channel: 'trunk:remote' } ]; for (const sub of SUBSCRIPTIONS) { diff --git a/test/trunk-remote.test.cjs b/test/trunk-remote.test.cjs new file mode 100644 index 0000000..88f0607 --- /dev/null +++ b/test/trunk-remote.test.cjs @@ -0,0 +1,146 @@ +'use strict'; + +// The remote half of the staleness signal (#307): when the app is allowed to +// ask where trunk is, and what it does with the answer. +// +// Nothing here touches the network. `listServerRefs` is injected, which is also +// the only way to exercise the two things that would otherwise only fail in +// front of a contributor: a prefix match that returns more than the one ref, +// and a request that never settles. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { + REMOTE_PROBE_INTERVAL_MS, + TRUNK_REF, + remoteProbeDue, + readRemoteTrunkOid +} = require('../src/trunk-remote'); + +const NOW = Date.parse('2026-08-05T12:00:00Z'); +const at = (offsetMs) => new Date(NOW + offsetMs).toISOString(); + +test('remoteProbeDue: a site that has never been asked is due (issue #307)', () => { + // Including a record written by a version of the app that stored no stamp: + // the field is simply absent, and that must read as "ask", not as "asked at + // the epoch" or as "never ask". + for (const checkedAt of [undefined, null, '', 'not-a-date']) { + assert.equal(remoteProbeDue({ checkedAt, now: NOW }), true, `checkedAt=${String(checkedAt)}`); + } +}); + +test('remoteProbeDue: inside the interval, the stored answer is reused (issue #307)', () => { + // This is the whole point of the stamp: site:status is called on mount and + // after every long operation, and each of those would otherwise be a request. + assert.equal(remoteProbeDue({ checkedAt: at(-1000), now: NOW }), false); + assert.equal(remoteProbeDue({ checkedAt: at(-(REMOTE_PROBE_INTERVAL_MS - 1)), now: NOW }), false); +}); + +test('remoteProbeDue: the interval boundary is due, not one tick short of it (issue #307)', () => { + assert.equal(remoteProbeDue({ checkedAt: at(-REMOTE_PROBE_INTERVAL_MS), now: NOW }), true); + assert.equal(remoteProbeDue({ checkedAt: at(-2 * REMOTE_PROBE_INTERVAL_MS), now: NOW }), true); +}); + +test('remoteProbeDue: a stamp in the future is due, not a site that never probes again (issue #307)', () => { + // A clock that moved backwards — a laptop that woke in another timezone, or + // a machine whose time was wrong until NTP corrected it. Reading this as + // "not due" would silence the signal until the stamp's hour came round. + assert.equal(remoteProbeDue({ checkedAt: at(5 * REMOTE_PROBE_INTERVAL_MS), now: NOW }), true); +}); + +test('remoteProbeDue: the caller can set its own interval (issue #307)', () => { + assert.equal(remoteProbeDue({ checkedAt: at(-5000), now: NOW, intervalMs: 1000 }), true); + assert.equal(remoteProbeDue({ checkedAt: at(-5000), now: NOW, intervalMs: 60000 }), false); +}); + +test('readRemoteTrunkOid: asks for one ref, and reads the one it asked for (issue #307)', async () => { + const calls = []; + const listServerRefs = async (args) => { + calls.push(args); + // What a real server answers: `prefix` is a prefix, so every branch whose + // name merely starts with `refs/heads/trunk` comes back too. Taking the + // first row would pin the site against a branch nobody is working on and + // report it behind for ever. + return [ + { ref: 'refs/heads/trunk-experiment', oid: 'wrong-one' }, + { ref: TRUNK_REF, oid: 'the-real-trunk' }, + { ref: 'refs/heads/trunkish', oid: 'also-wrong' } + ]; + }; + + const oid = await readRemoteTrunkOid({ url: 'https://example.test/wp.git', listServerRefs }); + + assert.equal(oid, 'the-real-trunk'); + assert.equal(calls.length, 1); + assert.equal(calls[0].url, 'https://example.test/wp.git'); + // Narrowing at the server, not after it: wordpress-develop's refs/pull/* + // alone would otherwise be tens of thousands of rows over the wire. + assert.equal(calls[0].prefix, TRUNK_REF); +}); + +test('readRemoteTrunkOid: a remote with no trunk answers null rather than throwing (issue #307)', async () => { + // "Answered, and there is no trunk" is a fact; the caller stores it and it + // clears any oid an earlier probe left. Only "could not ask" rejects. + assert.equal(await readRemoteTrunkOid({ url: 'https://example.test/wp.git', listServerRefs: async () => [] }), null); + assert.equal(await readRemoteTrunkOid({ url: 'https://example.test/wp.git', listServerRefs: async () => null }), null); +}); + +test('readRemoteTrunkOid: an unreachable remote rejects, so the caller can fall back (issue #307)', async () => { + await assert.rejects( + readRemoteTrunkOid({ + url: 'https://example.test/wp.git', + listServerRefs: async () => { throw new Error('getaddrinfo ENOTFOUND example.test'); } + }), + /ENOTFOUND/ + ); +}); + +test('readRemoteTrunkOid: a request that never settles is abandoned (issue #307)', async () => { + // The characteristic conference-network failure is not a refusal, it is a + // captive portal that swallows the connection. listServerRefs has no + // deadline of its own, so without this the probe would stay pending for the + // whole session and the next hour's would start behind it. + await assert.rejects( + readRemoteTrunkOid({ + url: 'https://example.test/wp.git', + listServerRefs: () => new Promise(() => {}), + timeoutMs: 20 + }), + /no answer from https:\/\/example\.test\/wp\.git/ + ); +}); + +test('readRemoteTrunkOid: a prompt answer does not leave its deadline timer running (issue #307)', async () => { + // An uncleared deadline would keep a handle alive per site per hour in the + // app, and here it would hold node --test open past the last assertion. + // Asserted through a fake clock rather than through process internals: the + // timer has to be cleared, and by the handle setTimeout returned. + const timers = []; + const originalSetTimeout = global.setTimeout; + const originalClearTimeout = global.clearTimeout; + global.setTimeout = (fn, ms) => { + const handle = originalSetTimeout(fn, ms); + timers.push({ handle, cleared: false }); + return handle; + }; + global.clearTimeout = (handle) => { + for (const t of timers) if (t.handle === handle) t.cleared = true; + return originalClearTimeout(handle); + }; + + try { + assert.equal( + await readRemoteTrunkOid({ + url: 'https://example.test/wp.git', + listServerRefs: async () => [{ ref: TRUNK_REF, oid: 'abc' }] + }), + 'abc' + ); + } finally { + global.setTimeout = originalSetTimeout; + global.clearTimeout = originalClearTimeout; + } + + assert.equal(timers.length, 1, 'exactly one deadline was armed'); + assert.equal(timers[0].cleared, true, 'the deadline timer was left running'); +}); diff --git a/test/update-plan.test.cjs b/test/update-plan.test.cjs index bce1a66..4a79f7d 100644 --- a/test/update-plan.test.cjs +++ b/test/update-plan.test.cjs @@ -7,6 +7,7 @@ const { SKIP_INSTALL_MESSAGE, SETUP_STATE_TO_STEP, trunkAgeInfo, + trunkUpdateAdvice, planUpdateSteps, planSetupSteps, updateStepStatuses, @@ -62,6 +63,152 @@ test('trunkAgeInfo: a future date clamps to age 0, not negative (issue #94)', () assert.strictEqual(info.stale, false); }); +// --- staleness measured from the remote, not the calendar (#307) --------- +// +// The probe's answer arrives as `remoteTrunkOid`; the module itself does no +// I/O. Three inputs, three behaviours: it has moved, it has not, and we could +// not ask. + +test('trunkAgeInfo: trunk has moved past this snapshot -> stale, whatever the calendar says (issue #307)', () => { + // Three days old and already behind — exactly the case the calendar misses. + const info = trunkAgeInfo({ + trunkDate: daysAgo(3), + trunkOid: 'aaaaaaa', + remoteTrunkOid: 'bbbbbbb', + now: NOW + }); + + assert.strictEqual(info.behind, true); + assert.strictEqual(info.stale, true); + assert.strictEqual(info.source, 'remote'); + assert.strictEqual(info.ageDays, 3, 'the age is still reported; it is just no longer the test'); +}); + +test('trunkAgeInfo: the remote says this snapshot IS trunk -> not stale, however old it is (issue #307)', () => { + // Two months old and current: a quiet trunk is not an out-of-date one, and + // telling this contributor to spend minutes updating would be a lie. + const info = trunkAgeInfo({ + trunkDate: daysAgo(60), + trunkOid: 'aaaaaaa', + remoteTrunkOid: 'aaaaaaa', + now: NOW + }); + + assert.strictEqual(info.behind, false); + assert.strictEqual(info.stale, false); + assert.strictEqual(info.source, 'remote'); + assert.strictEqual(info.ageDays, 60); +}); + +test('trunkAgeInfo: no probe answer falls back to the calendar, unchanged (issue #307)', () => { + // Offline, rate-limited, proxied, or simply not asked yet. This is the + // promise the threshold survives to keep: the dot still works with no + // network, and behaves exactly as it did before #307. + for (const probe of [{}, { remoteTrunkOid: null }, { trunkOid: 'aaaaaaa' }, { remoteTrunkOid: 'bbbbbbb' }]) { + const fresh = trunkAgeInfo({ trunkDate: daysAgo(13), now: NOW, ...probe }); + assert.strictEqual(fresh.behind, null, JSON.stringify(probe)); + assert.strictEqual(fresh.source, 'calendar'); + assert.strictEqual(fresh.stale, false); + + const old = trunkAgeInfo({ trunkDate: daysAgo(15), now: NOW, ...probe }); + assert.strictEqual(old.behind, null, JSON.stringify(probe)); + assert.strictEqual(old.stale, true); + } +}); + +test('trunkAgeInfo: a site record with no date can still be judged by the probe (issue #307)', () => { + const info = trunkAgeInfo({ trunkOid: 'aaaaaaa', remoteTrunkOid: 'bbbbbbb', now: NOW }); + + assert.strictEqual(info.known, false, 'there is still no date to show'); + assert.strictEqual(info.label, ''); + assert.strictEqual(info.stale, true, 'but the probe answered, and it is the whole answer'); +}); + +// --- what the contributor is told about it (#307) ------------------------- + +const behindAge = () => trunkAgeInfo({ trunkDate: daysAgo(3), trunkOid: 'aaaaaaa', remoteTrunkOid: 'bbbbbbb', now: NOW }); +const calendarAge = () => trunkAgeInfo({ trunkDate: daysAgo(20), now: NOW }); + +test('trunkUpdateAdvice: a current site is told nothing at all (issue #307)', () => { + const advice = trunkUpdateAdvice({ + trunkAge: trunkAgeInfo({ trunkDate: daysAgo(60), trunkOid: 'a', remoteTrunkOid: 'a', now: NOW }) + }); + + assert.strictEqual(advice.recommendUpdate, false); + assert.strictEqual(advice.headline, ''); + assert.strictEqual(advice.preLinkNote, ''); +}); + +test('trunkUpdateAdvice: the message names trunk when trunk is what was measured (issue #307)', () => { + const advice = trunkUpdateAdvice({ trunkAge: behindAge() }); + + assert.strictEqual(advice.recommendUpdate, true); + assert.strictEqual(advice.headline, 'Trunk has moved since this snapshot'); + assert.doesNotMatch(advice.headline, /days old/, 'the age is not what was measured'); + assert.match(advice.dotTitle, /Trunk has moved/); +}); + +test('trunkUpdateAdvice: the calendar fallback keeps saying what it can honestly say (issue #307)', () => { + const advice = trunkUpdateAdvice({ trunkAge: calendarAge() }); + + assert.strictEqual(advice.recommendUpdate, true); + assert.match(advice.headline, /WordPress code is 20 days old/); + assert.doesNotMatch(advice.headline, /has moved/, 'nothing was measured against trunk here'); +}); + +test('trunkUpdateAdvice: an applied patch is never nagged toward its own destruction (issue #307)', () => { + const advice = trunkUpdateAdvice({ trunkAge: behindAge(), appliedPatch: true }); + + // Updating resets the working tree and the applied patch goes with it, so + // the amber "update now" block and the next-action cue stay away... + assert.strictEqual(advice.recommendUpdate, false); + assert.strictEqual(advice.atRisk, true); + // ...but the fact is not hidden: the dot and the panel still say it. + assert.match(advice.headline, /Trunk has moved/); + assert.match(advice.detail, /patch you have applied would be removed/); + // Including the tooltip, which has no detail line under it to soften it. + assert.match(advice.dotTitle, /updating would remove the patch you applied/); + assert.doesNotMatch(advice.dotTitle, /update this site to latest trunk/); +}); + +test('trunkUpdateAdvice: uncommitted edits are named, not silenced over (issue #307)', () => { + // The opposite call to the applied patch above, and deliberate. An update + // removes an applied patch without asking; it asks before it touches edits + // (#234), so going quiet would withhold the signal over work that is not + // actually in danger — and would rest on the branch-point measure the panel + // holds, which counts parked work a force checkout survives (#239). + const advice = trunkUpdateAdvice({ trunkAge: behindAge() }); + + assert.strictEqual(advice.recommendUpdate, true); + assert.match(advice.detail, /edits you have not written down yet are asked about first/); +}); + +test('trunkUpdateAdvice: the copy never implies updating carries a ticket forward (issue #305)', () => { + const advice = trunkUpdateAdvice({ trunkAge: behindAge(), ticketLinked: true }); + + // Updating moves the site. A ticket branch keeps the base it was born at, + // deliberately — bringing it forward is #305's action, not this one, and + // promising it here would be a lie the contributor discovers the hard way. + assert.match(advice.detail, /copy of WordPress/); + assert.match(advice.detail, /keeps the trunk it was created from/); + assert.match(advice.detail, /does not move it/); +}); + +test('trunkUpdateAdvice: the pre-link prompt appears only where it is both true and safe (issue #307)', () => { + // The cheapest moment: a ticket not yet born on a trunk that has moved. + assert.match( + trunkUpdateAdvice({ trunkAge: behindAge() }).preLinkNote, + /Trunk has moved since this snapshot\. Updating first means this ticket is not born behind\./ + ); + + // Not once a ticket exists — updating would not bring that one forward. + assert.strictEqual(trunkUpdateAdvice({ trunkAge: behindAge(), ticketLinked: true }).preLinkNote, ''); + // Not over an applied patch, which updating would remove. + assert.strictEqual(trunkUpdateAdvice({ trunkAge: behindAge(), appliedPatch: true }).preLinkNote, ''); + // Not when there is nothing to say. + assert.strictEqual(trunkUpdateAdvice({ trunkAge: trunkAgeInfo({ trunkDate: daysAgo(1), now: NOW }) }).preLinkNote, ''); +}); + test('planUpdateSteps: install runs when the lockfile changed (issue #94)', () => { const steps = planUpdateSteps({ lockfileChanged: true }); assert.deepStrictEqual(steps.map((s) => s.key), ['fetch', 'install', 'build']);