Skip to content

Let a site know which Trac ticket it is being used for - #123

Merged
juanmaguitar merged 5 commits into
trunkfrom
juanmaguitar/issue-110-improve-contribution-experience
Aug 7, 2026
Merged

Let a site know which Trac ticket it is being used for#123
juanmaguitar merged 5 commits into
trunkfrom
juanmaguitar/issue-110-improve-contribution-experience

Conversation

@juanmaguitar

@juanmaguitar juanmaguitar commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Closes #109. First step of #110, following Option A.

Why

The app knows where a site is and what state its build is in, but not what the contributor is working on — and Core work is organised around Trac tickets, not directories. That gap is why a generated patch carries no ticket number (#107), why there is nowhere to show a ticket's existing patches, and why applying someone else's patch (#11) has no context to start from.

What this does

A site can now be associated with a Trac ticket:

  • asked optionally at creation — "What are you working on?"
  • set, changed or cleared later from a new panel on the site screen
  • stored in siteMeta, so it survives restarts

Parsing accepts what a contributor actually types or pastes: a bare number, a number with a #, and any core Trac ticket URL with a comment anchor, trailing slash or ?format= query still attached. Anything else is rejected with a message aimed at the contributor rather than at a log.

The panel sits between the action row and the Terminal; nothing else on the page moves.

What this deliberately does not do

No network. Linking a ticket never depends on Trac being reachable.

That is a design decision, not an omission. While building this I measured every documented Trac read path — ?format=csv, the ticket HTML, raw-attachment/ticket/<id>/<file>, and even robots.txt — and all of them currently return 403 behind a proof-of-work interstitial for any client that is not a browser (SHA-256 hashcash, _hcc cookie, escalating to an "I am human" checkbox on repeat hits; a spoofed browser User-Agent gets a bare nginx 403 instead). This also breaks core's own grunt patch, which has parsed that page for years.

So showing a ticket's patches needs a fetch strategy of its own and lands separately. Two things shape it:

  • A Contributor Day room shares one NAT IP — exactly the pattern that escalates Trac's challenge to interactive, and that exhausts unauthenticated GitHub's 60 requests/hour. Any network path has to degrade to opening the contributor's own browser, where the challenge clears itself.
  • The real fix is upstream: meta #8202 proposes a ticket/ JSON endpoint returning exactly the attachment data (filename, author, date, size) that panel wants. The follow-up should keep its fetch source swappable so it can adopt that endpoint when it lands.

Also out of scope here: applying patches (#11), the ticket number in the patch header (#107), tickets as branches (#108), opening PRs from the app (#118).

Testing

  • test/trac-ticket.test.cjs — 14 tests over every accepted and rejected input form. parseTicketRef is a pure, dependency-free module so it runs under node --test with no DOM, following the setup-steps.cjs / update-plan.cjs convention.
  • npm test and npm run test:electron: 172/172 pass on both.
  • npm run lint (now repo-wide after Finish the lint baseline and widen the CI check to the whole repo #119): clean.

Manual: create a site with and without a ticket; link, unlink and re-link from the panel; paste 62281, #62281 and a URL with #comment:3; restart the app and confirm the ticket survives; confirm "Open in Trac" opens the external browser.

Fixed after manual testing

Manual testing surfaced a wrong-message bug: a bare word like abc (and 62281abc, #abc) was rejected with "Only core.trac.wordpress.org tickets are supported" — pointing the contributor at a host they never named. new URL('https://abc') succeeds because a bare word is a legal hostname, so non-URL input slipped past into the host check instead of falling to the generic message. Fixed to only treat input as a URL when it has a scheme, a path separator or a dotted host; everything else now gets "Enter a ticket number like 62281, or a core.trac.wordpress.org ticket URL." A github.com URL still correctly names the host as the reason. The reproducing test asserts the message, not just rejection — the gap the original suite missed.

Rebased onto trunk after #122, so no generated bundle is included — the diff is five source files.

🤖 Generated with Claude Code

@juanmaguitar
juanmaguitar force-pushed the juanmaguitar/issue-110-improve-contribution-experience branch from 908c468 to dbb4a71 Compare August 6, 2026 07:31
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

AI review: 0 findings across architecture, security, performance, cross-platform, tests.

Reviewed the Trac-ticket-association feature (src/renderer/trac-ticket.cjs, the sites:set-ticket IPC handler, and the create-site/site-panel UI in src/renderer/index.jsx) against the five dimensions in .github/ai-review-rules.md.

  • parseTicketRef is a pure, dependency-free .cjs module following the existing setup-steps.cjs/update-plan.cjs convention, with thorough unit tests in test/trac-ticket.test.cjs.
  • The Trac hostname check and ticket-id bounds are enforced server-side in sites:set-ticket and in wordpress:setup, not just in the renderer — the renderer's own validation is a UX nicety, not the trust boundary.
  • openExternal is only ever called with URLs built by ticketUrl() from a validated numeric id, or a hardcoded constant — no user-controlled string reaches shell.openExternal. This PR doesn't touch the pre-existing unscoped url:open handler itself.
  • The new tracTicket field is additive to siteMeta; absence on existing records is handled with m.tracTicket || null, so no migration is needed.
  • No new dependencies, no new spawns, no new listeners, no sync FS calls on a hot path.
Style / process notes (non-blocking)

Nothing beyond ESLint's remit noticed.

@juanmaguitar
juanmaguitar force-pushed the juanmaguitar/issue-110-improve-contribution-experience branch from dbb4a71 to 7122a5a Compare August 6, 2026 09:15
@juanmaguitar
juanmaguitar requested a balanced review from Copilot August 6, 2026 10:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds offline Trac ticket associations to site metadata.

Changes:

  • Adds ticket reference parsing and tests.
  • Adds creation and site-management UI.
  • Persists associations through IPC and electron-store.

Review findings: 4 [fix here] · 0 [follow-up]

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/main.js Persists and retrieves ticket associations.
src/preload.js Exposes ticket updates through IPC.
src/renderer/index.jsx Adds ticket creation and management UI.
src/renderer/trac-ticket.cjs Parses and canonicalizes ticket references.
test/trac-ticket.test.cjs Tests accepted and rejected references.

Comment thread src/main.js Outdated
Comment thread src/main.js
Comment on lines +675 to +679
ipcMain.handle('sites:set-ticket', async (_e, sitePath, ref) => {
try {
// Empty means unlink — the panel's Unlink button and a cleared field
// both land here, and neither is an error.
const raw = typeof ref === 'string' ? ref.trim() : '';
Comment thread src/main.js
Comment on lines +684 to +686
const parsed = parseTicketRef(raw);
if (!parsed.ok) return { ok: false, error: parsed.error };
await mergeSiteMeta(sitePath, { tracTicket: parsed.id });
Comment thread src/renderer/index.jsx Outdated
juanmaguitar and others added 5 commits August 7, 2026 08:39
The app knows where a site is and what state its build is in, but not what
the contributor is working on — and Core work is organised around Trac
tickets, not directories. That gap is why a generated patch carries no ticket
number, why there is nowhere to show a ticket's existing patches, and why
applying someone else's patch has no context to start from.

A site can now be associated with a Trac ticket: asked optionally at creation
("What are you working on?"), set or cleared later from a panel on the site
screen, and stored in siteMeta so it survives restarts.

Parsing lives in a pure, dependency-free module so it can be unit tested
without a DOM, following the setup-steps.cjs / update-plan.cjs convention. It
accepts what a contributor actually types or pastes — a bare number, a number
with a #, and any core Trac ticket URL with a comment anchor, trailing slash
or query still attached — and rejects the rest with a message meant for the
contributor rather than a log.

The association is deliberately local and offline: linking a ticket never
depends on Trac being reachable. That matters more than it looks. Every
documented Trac read path (?format=csv, the ticket HTML, raw-attachment/...)
currently returns 403 behind a proof-of-work interstitial for any non-browser
client, so showing a ticket's patches needs a fetch strategy of its own and
lands separately. The upstream fix is meta #8202, whose proposed ticket/
endpoint returns exactly the attachment data that panel will want.

Refs #109, part of #110.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Typing a bare word like `abc` into the Trac ticket panel reported
"Only core.trac.wordpress.org tickets are supported." — telling the
contributor their host is wrong when they named no host at all.

`new URL('https://abc')` succeeds (a bare word is a legal hostname), so
non-numeric input reached the host comparison instead of falling through
to the generic message. Only enter the URL branch when the input looks
like a URL — a scheme, a path separator or a dotted host.

Adds a test asserting the message (not just rejection) for `abc`,
`62281abc`, `#abc`, `not a url` and `https://`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: juanmaguitar <422576+juanmaguitar@users.noreply.github.com>
@juanmaguitar
juanmaguitar force-pushed the juanmaguitar/issue-110-improve-contribution-experience branch from 649427d to f9b3982 Compare August 7, 2026 06:39
@juanmaguitar
juanmaguitar requested a balanced review from Copilot August 7, 2026 07:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/main.js:643

  • Tests · 🔵 low · [fix here] Creation-time ticket persistence is untested: wordpress:setup is still classified as unreachable by the IPC harness, so dropping options.tracTicket or this assignment would leave the suite green. Extract the metadata construction behind a testable seam or make the clone dependencies injectable, then assert that valid input is persisted (and invalid input is not).
		const ticket = parseTicketRef(options.tracTicket);
		if (ticket.ok && !Object.prototype.hasOwnProperty.call(existingMeta, 'tracTicket')) {
			meta[siteDir].tracTicket = ticket.id;

src/main.js:549

  • Architecture · 🟡 medium · [fix here] m is captured before the awaited readTrunkInfo. If the user links or unlinks a ticket while that status request is pending, this returns the old value and loadStatus writes it back into both renderer states, making a successful save appear undone until the next reload. Re-read this field after the await (or ignore stale status results in the renderer).
		return { hasNodeModules, hasBuilt, skipInitWizard: Boolean(m.skipInitWizard), initialized: Boolean(m.initialized), installFailed: Boolean(m.installFailed), trunkOid, trunkDate, updateIncomplete: Boolean(m.updateIncomplete), tracTicket: m.tracTicket || null };

src/renderer/index.jsx:2080

  • Architecture · 🟡 medium · [fix here] This panel is mounted while isPending is true, but wordpress:setup does not register the site until cloning finishes (src/main.js:625-650). The input, Enter handler, Link button, and Unlink button remain active during that interval, so every attempted change deterministically fails with “Site is not registered.” Disable the mutating controls while pending or queue the change until registration completes.
      <div style={{ padding: 20, border: '1px solid #dcdcde', borderRadius: 12, background: '#fff' }}>
        <div style={{ fontWeight: 600, fontSize: 16, color: '#1d2327' }}>Trac ticket</div>

test/ipc-wiring.test.cjs:939

  • Tests · 🔵 low · [fix here] The new tests only exercise parse rejection and the unregistered-site gate; both persistence calls in the handler can still be removed while the suite stays green. Add a registered-site test that asserts a valid reference is written to siteMeta and an empty reference clears it.
test('sites:set-ticket validates the reference through trac-ticket', async () => {

@juanmaguitar
juanmaguitar merged commit c81c9c5 into trunk Aug 7, 2026
4 checks passed
juanmaguitar added a commit that referenced this pull request Aug 7, 2026
Part of #110. **Closes #11** (the stack applies a patch from a file, a
linked PR, and a Trac attachment — the whole of #11). **Stacked on
#123** (base is that branch, not `trunk`) — review #123 first; this diff
is only the apply engine on top of it. The whole ticket-to-tested-patch
flow lands as one merge once the stack is complete.

## Why

Associating a site with a Trac ticket (#123) only pays off if a
contributor can act on what is on that ticket. This is the first half of
acting: take a `.diff`/`.patch` file, apply it to the
`wordpress-develop` checkout, and rebuild so the change can be tested.
Discovering the patches from the ticket itself is the next branch in the
stack.

## What this does

- **Choose a patch file → preview → apply and rebuild.** The preview
lists the files the patch touches, flags any that collide with the
contributor's own edits, names binary files it will skip, and says
whether `package-lock.json` moves (so an install runs before the
rebuild).
- **Revert.** The applied patch is recorded so it can be undone after a
restart, and the record is cleared when a trunk update or a discard
resets the tree out from under it.
- The apply → install? → rebuild chain reuses the trunk-update step
helper and the npm wrappers, so exit codes, cancellation and terminal
streaming behave as everywhere else.

## How, and why it is built the way it is

There is no `git` binary to shell out to and isomorphic-git has no apply
primitive, so hunks are matched and written by hand with the `diff`
package the app already bundles for generating patches.

- **All or nothing.** Every file is resolved in memory first; if any
hunk fails, nothing is written. Because a write can still fail on the
way out (a path that is really a file, a read-only attribute, Windows
holding a file open, a full disk), prior contents are captured during
resolution and restored if a write throws midway. A half-applied tree is
worse than an unapplied one.
- **Three patch dialects, one path vocabulary.** Subversion-style Trac
attachments (no `a/` `b/` prefixes, sometimes the pre-`src/` layout),
git-style PR `.diff`s, and the app's own `createTwoFilesPatch` output
all normalise to repo-relative paths for today's layout, in a pure
module (`src/patch-plan.cjs`) shared by the main process and the applier
and kept out of the renderer bundle.
- **Local and safe.** Applying never touches the network. Line endings
are matched on LF but written back as the file had them, so a
genuinely-CRLF fixture is not silently rewritten. Paths are resolved
through symlinks and refused if they escape the site folder, since a
patch is untrusted input.

## Testing

- `test/patch-plan.test.cjs` and `test/patch-apply.integration.test.cjs`
— the applier tests run against real on-disk isomorphic-git repos. The
parser is also validated against the real diff of wordpress-develop PR
#7990.
- `npm test` / `npm run test:electron`: **213/213** on both runtimes.
`npm run lint`: clean.

## Self-review (per AGENTS.md)

Ran the review in `.github/instructions/code-review.instructions.md`
with the judgement pass dispatched to a fresh-context subagent. It
returned **5 `[fix here]` · 6 `[follow-up]`; all 11 were fixed before
this PR:**

- **All-or-nothing did not hold across the write loop** — added the
capture-and-rollback described above. Verified by disabling the rollback
and confirming the new test fails (it reproduces the bug).
- **`appliedPatch` desynced from the tree** — cleared on discard and on
a trunk update's force checkout; a second apply is refused by name
rather than silently orphaning the first.
- **Renames were classified but not implemented** — real rename branch,
and a 100%-similarity rename (no hunks) is no longer rejected as "not a
patch", which would have killed any PR diff that moved a file.
- **Ctrl+C could not cancel an apply** — the chain now sets the kill
hook like the others.
- **The test named for the invariant tested the wrong path** — added
mid-write rollback, both rename shapes, reverting a deletion, CRLF
preservation, and the symlink-escape case.
- Follow-ups also fixed: symlink-aware path containment, EOL
preservation, corrected conflict copy, a `+86 KB → +11 KB` bundle
reduction by moving the parser out of the renderer tree, a
floating-promise reset, and an install-step label that showed "skipped"
while installing.

## Not done

The `ipcMain` handler layer has no unit tests (the repo has no precedent
for mocking handlers; the logic beneath them is covered), and **nothing
has been exercised in the running app yet** — same manual-pass gap as
#123. Both are called out rather than implied.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
juanmaguitar added a commit that referenced this pull request Aug 7, 2026
Part of #110. Refs #109, #11. **Stacked on #135** (base is that branch,
not `trunk`), which is itself stacked on #123. Review order: #123#135
→ this. The full ticket-to-tested-patch flow merges once the stack is
complete.

## Why

#123 links a site to a Trac ticket; #135 applies a patch file. This
joins them: it shows the work already on the ticket so a contributor can
see and test it before adding their own — the Contributor-Day failure
the whole flow exists to prevent (several people writing overlapping
patches because none could see anybody else's).

## What this does

When a site has a ticket linked, the panel lists the ticket's **linked
pull requests**, newest first, each with its state and last-updated
date. "Apply…" fetches that PR's diff and drops it into the existing
preview → apply → rebuild flow from #135 — so a downloaded `.diff` and a
linked PR are one path from the preview onward. A Refresh button
re-checks on demand.

Ships the PR half of "patches on a ticket" first, deliberately: on the
busiest tickets the real work is a wordpress-develop PR, and the Trac
attachment list is emptiest exactly where activity is highest. Trac
attachments (behind the proof-of-work interstitial) are not listed yet —
the panel points at the ticket for those.

## How, and the constraints that shaped it

- **PRs are found by GitHub's convention and verified locally.** A PR
cites its ticket in the body, so a broad search for the number is
verified narrowly against `core.trac.wordpress.org/ticket/<id>` —
GitHub's tokeniser matches the bare number in unrelated text, so the
local verification is what makes the list trustworthy rather than merely
plausible.
- **The web `.diff` routes return 422 unauthenticated** (verified
2026-08-06, both `github.com/.../pull/N.diff` and
`patch-diff.githubusercontent.com`), so the diff is fetched through the
REST API with the diff media type. Requests go through **Electron
`net`**, not a new HTTP dependency.
- **Unauthenticated GitHub is 60/hour and a Contributor-Day room shares
one NAT IP.** So lookups are manual, not polled; each ticket's result is
cached in `electron-store` as last-known-good with a timestamp; and a
rate-limited or offline answer shows the cached list *labelled with
when*, never a short list presented as complete. `classifyHttpFailure`
tells a spent limit — primary and secondary — apart from an empty
ticket.

## Testing

- `test/patch-sources.test.cjs` — the parse, verify (including the
`65820` vs `658200` precision case) and rate-limit-classification logic,
unit tested without a network.
- `npm test` / `npm run test:electron`: **220/220** on both runtimes.
`npm run lint`: clean.

## Self-review (per AGENTS.md)

Ran the review with the judgement pass on fresh context. It returned **1
`[fix here]` · 1 `[follow-up]`, both fixed:**

- 🔴 **Temporal-dead-zone crash** — a `useEffect` referenced the
`loadTicketPatches` callback in its dependency array ~660 lines before
the `const` was defined, throwing `ReferenceError` on every `SiteRow`
render. The `node --test` suite has no DOM, so 220 green tests did not
catch it; the review did. Fixed by placing the effect after the
definition. **This is exactly why the manual pass below still matters.**
- 🔵 GitHub's *secondary* (abuse) rate limit is a 403 with `Retry-After`
while the primary quota is unspent — now classified as rate-limited
rather than a generic error, with a test.

Verified clean by the review: `httpGet` settle-once/timeout/abort, the
cache cannot be poisoned by a failed lookup (only `ok` writes), no new
trust-boundary hole (PR number stripped to digits; diff flows through
#135's existing engine unchanged), additive `electron-store` keys so no
migration.

## Not done

The Electron `net` client and the two IPC handlers are not unit-tested
(no repo precedent for mocking `net`; the logic lives in the pure
module, which is covered). And **nothing in this stack has been
exercised in the running app yet** — the TDZ crash above shows why that
pass is owed before the final merge, and it is the main thing left.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
juanmaguitar added a commit that referenced this pull request Aug 7, 2026
Part of #110. Refs #109, #11. **Stacked on #136** (base is that branch),
which stacks on #135#123. Review order: #123#135#136 → this.
The whole flow merges once the stack is complete.

## Why

PRs are only half of "the work already on a ticket". On many tickets —
good-first-bugs especially — the patch a contributor wants to try is a
`.diff` **attached to the ticket**, not a PR (verified live: ticket
#37578 has three `.diff` attachments and one PR; before this, the app
showed only the PR). This adds the attachment half, under the PR list,
loaded on demand.

## What this does

A **"Show Trac attachments"** button under the linked-PR list. Because
Trac serves the attachment list only to a real browser (everything else
hits the proof-of-work interstitial), clicking it opens the real ticket
in an embedded window where the contributor clears the challenge
**once**, the app scrapes the `#attachments` block, and the window
closes — it is a means, not the UI. The attachments appear as a native
in-app list (filename · author · date · size), each `.diff`/`.patch`
with **Apply…**; non-patches (e.g. a `.txt`) are shown but marked "not a
patch". Applying downloads the file through that same challenge-passing
session and hands it to the existing preview → apply engine, so an
attachment, a PR and a chosen file are one path from the preview onward.

On demand, not on link: opening a Trac window can surface the challenge,
so it happens when the contributor asks, not for every ticket. The
persistent session means the challenge is passed once, not per open.

## Verified end to end against live Trac (#37578)

A deterministic Electron harness exercised the real `trac-view.js`:
- the window passes the challenge unattended;
- the parser reads the **real** markup — 4 attachments with authors and
**absolute timestamps**, the `.txt` correctly marked not-a-patch;
- **the raw-attachment download is authorised by the session cookie** —
a real `wp-admin/includes/dashboard.php` diff (1701 bytes) comes back.
This was the one load-bearing runtime assumption, now confirmed.

Also confirmed in the running app: the panel renders, the attachment
list populates from the live scrape, and the PR apply→preview wiring
works.

## Security (the app's first remote, untrusted content — AGENTS.md)

- The window: `contextIsolation`, no `nodeIntegration`, `sandbox`, a
dedicated `persist:trac` partition, and **no preload** — the page cannot
reach the app or Node; only the `#attachments` HTML crosses back, read
from the main process via `executeJavaScript`.
- Navigation pinned to `core.trac.wordpress.org` against **both
`will-navigate` and `will-redirect`** (the latter catches 3xx / `<meta
refresh>`).
- The parser **never emits an off-host URL**, and `fetchAttachment`
re-checks the host before sending the session cookie — so a poisoned
ticket page cannot get an attacker link in front of the user or leak the
cookie off-host.
- The downloaded diff is untrusted → flows through #135's apply engine,
which defends against path traversal.

## Testing

- `test/trac-attachments.test.cjs` — the pure parser: dedup, encoded
names, off-host and cross-ticket rejection, absolute-date extraction,
missing-metadata rows, empty input. The fixture matches the live markup
(confirmed by the harness above).
- `npm test` / `npm run test:electron`: **229/229** both runtimes. `npm
run lint`: clean.
- `src/trac-view.js`'s window/net glue is untested, consistent with the
other `net` client (`github-prs.js`) — no repo precedent for mocking
BrowserWindow; the logic lives in the covered pure parser.

## Self-review (per AGENTS.md)

Ran the review with the judgement pass on fresh context. It returned **2
`[fix here]` (both 🔵), both fixed before this PR:**
- Navigation lock covered only `will-navigate` → added `will-redirect`
(3xx / meta-refresh could otherwise move the pinned window off-origin).
- The link regex accepted an absolute off-host href (Apply was safe, but
the filename rendered as an `openExternal` link) → the parser now
rejects any non-Trac-host URL, with a test.

Reviewer verified clean: window config, `setWindowOpenHandler`, host
re-check before the cookie fetch, poll-loop lifecycle with `isDestroyed`
guards + `destroy()` in `finally`, additive `electron-store` (no
migration), no new dependency.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
juanmaguitar added a commit that referenced this pull request Aug 7, 2026
Part of #110. Refs #109, #11. **Stacked on #139** (base is that branch),
which stacks on #136#135#123. Review order: #123#135#136#139 → this.

## Why

A contributor arriving at a ticket wants to try the latest fix. The
panel listed PRs and Trac attachments as two separate sections, but
nothing said which one — across both — was the most recent. Usually it's
a PR (visible immediately); but sometimes the newest fix is a `.diff`
uploaded to Trac, and that was buried under the attachments button with
no signal.

## What this does

- The newest known patch carries a **"Latest" pill**, whether it's a PR
row or a Trac-attachment row.
- When the newest is an attachment, a note says so explicitly: *"The
most recent patch on this ticket is a file attachment, not a pull
request."*
- Because attachments load on demand, "latest" is judged across what's
loaded: **PRs alone until the contributor opens attachments** (the
normal case), then both. Opening attachments isn't forced on every
ticket — the verdict simply completes once they're looked at.

## How

The comparison is a pure module (`src/latest-patch.cjs`), unit tested. A
PR is dated by `updatedAt`, an attachment by its scraped upload time. A
`.txt` never competes; a relative-only date ("15 months ago") can't win.
The attachment timestamp is **anchored to UTC by hand** rather than
parsed in the machine's local zone, so two contributors in different
timezones see the same patch marked latest — a consistent answer is the
whole point.

## Testing

- `test/latest-patch.test.cjs` — PR-wins (normal),
attachment-wins-once-loaded (the case the feature exists for), `.txt`
excluded, relative-date can't win, attachments-only, empty→null, and the
UTC-determinism of the date parse (PM/AM and the 12-o'clock edge).
- `npm test` / `npm run test:electron`: **238/238** both runtimes. `npm
run lint`: clean.

## Self-review (per AGENTS.md)

Ran the review with the judgement pass on fresh context. It returned **0
`[fix here]` · 1 `[follow-up]`** — the follow-up was the timezone skew
(PR date is UTC ISO, attachment date was parsed in local time), which
two contributors could see differently. **Fixed before this PR** by
anchoring the attachment parse to UTC, with a determinism test. The
reviewer verified the key/equality logic (exactly one row marked, right
row), NaN/`.txt` exclusion, null-safety, and stale-state reset on ticket
switch.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
juanmaguitar added a commit that referenced this pull request Aug 7, 2026
Part of #110. Refs #109, #11. **Stacked on #140** (base is that branch),
atop #139#136#135#123. From hands-on testing of the stack.

## Three fixes reported from testing

1. **The apply error persisted after Cancel.** Applying a PR that no
longer fits ("…has moved on since the patch was written") showed an
error; clicking Cancel dismissed the preview but left the error on
screen. Cancel now clears it too.
2. **The "Try someone else's patch" sub-copy contradicted itself during
a PR preview** — it said "Apply a `.diff`/`.patch` file…" while showing
"PR #4496 changes 1 file". The sub-copy is now hidden whenever a preview
or the apply chain is showing (the preview card speaks for itself), and
the idle copy names pull requests as well as files.
3. **The failure message ran two sentences together** ("…no longer
applies The checkout was not changed."). A period is inserted when the
reason doesn't already end in punctuation.

## One requested addition

**Apply a PR straight from a pasted URL or number**, without it having
to be linked to the ticket. A "Paste a pull request URL or number" input
+ "Apply PR" button in the panel's idle state.

`parsePrRef` (pure, `src/patch-sources.cjs`) accepts a wordpress-develop
PR URL or a bare number and rejects other repos, other hosts, issue URLs
and junk — including crafted `..` paths (URL normalisation collapses
them, then the repo/anchor check rejects). The number rides the
**existing** `previewPr → fetchPrDiff → preview → apply` flow, so a
pasted PR shares the same trust boundary as the linked-PR list:
`fetchPrDiff` hardcodes the repo and strips the number to digits, and
the diff flows through the apply engine's path-traversal defence.

## Testing

- `test/patch-sources.test.cjs` — `parsePrRef`: bare/`#`number, URL with
trailing `/files` and `#…`, scheme-less, wrong-repo rejection, and
issue/foreign-host/junk rejects.
- `npm test` / `npm run test:electron`: **242/242** both runtimes. `npm
run lint`: clean.

## Self-review (per AGENTS.md)

Judgement pass on fresh context: **0 findings** across the five
dimensions. It independently confirmed the `..`-traversal and
foreign-repo/host rejections, that the pasted-number path introduces no
new trust boundary (same `fetchPrDiff` + apply engine), that the
punctuation ternary can't throw (error is truthy-guarded), and that
hiding the sub-copy never strips the idle call-to-action.

Pending your visual confirm in the running app for the three UI fixes
and the paste-a-PR flow — these are your reported issues, verified here
by tests + review.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@juanmaguitar
juanmaguitar deleted the juanmaguitar/issue-110-improve-contribution-experience branch August 11, 2026 11:29
juanmaguitar added a commit that referenced this pull request Aug 12, 2026
With a Gutenberg site now cloning, building and serving, the remaining half of
"see what a Gutenberg PR does" is reading the PR and applying it. Both were
hard-wired to WordPress Core:

- Patch paths were always rewritten into wordpress-develop's src/ layout. That
  rewrite exists because a patch attached to a Trac ticket years ago still names
  `wp-admin/…`, but a Gutenberg diff is already repo-relative, and a top-level
  `wp-`-prefixed path in one would be moved under a `src/` directory Gutenberg
  does not have. parsePatchFiles now takes a `layout`, applyPatchToDir passes it
  through, and preview, apply and revert all resolve it from the site so they
  cannot disagree about where a file lives.
- Pull requests were always read from WordPress/wordpress-develop. parsePrRef,
  fetchLinkedPrs and fetchPrDiff now take the site's upstream, so a Gutenberg
  site lists and fetches WordPress/gutenberg pull requests — and still refuses a
  PR from the other project, whose diff would not fit its checkout.
- "Which PRs belong to this work item" is a different question per provider: a
  Core PR cites a Trac URL, a Gutenberg PR cites its issue as `#1234`. Added
  bodyCitesIssue and citesWorkItemFor; the verification stays narrow (`#658`
  must not match inside `#6580`, and a bare number is not a citation).

The linked-PR cache key now includes the repository: Trac ticket #123 and
Gutenberg issue #123 are different work items and shared one entry before.

Every new parameter defaults to Core's behaviour, so a site with no project type
is unchanged and needs no migration.

Verified by hand that a `packages/…` diff applies to a Gutenberg-shaped tree
under repo-relative, and that the same patch fails under Core's layout — which
is the bug this prevents.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Work on a ticket: associate a site with a Trac ticket and show its patches

3 participants