Show the pull requests linked to a ticket, and apply one by juanmaguitar · Pull Request #136 · WordPress/contributor-toolkit

juanmaguitar · 2026-08-06T11:26:29Z

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

Copilot

Pull request overview

Adds linked GitHub PR discovery and application to the ticket workflow built by #123 and #135.

Changes:

Fetches, verifies, caches, and displays ticket-linked PRs.
Routes PR diffs through the existing preview/apply flow.
Adds parser and IPC coverage.

Review: 4 [fix here] findings.

Reviewed changes

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

Show a summary per file

File	Description
`src/github-prs.js`	Implements GitHub API requests.
`src/patch-sources.cjs`	Parses and verifies linked PRs.
`src/main.js`	Adds IPC handlers and caching.
`src/preload.js`	Exposes linked-PR APIs.
`src/renderer/index.jsx`	Displays and applies linked PRs.
`test/patch-sources.test.cjs`	Tests parsing and classification.
`test/ipc-wiring.test.cjs`	Covers PR-diff IPC wiring.

 	['wordpress:setup', 'calls ensureAutocrlf and readTrunkInfo only after cloning wordpress-develop over the network'],
-	['git:apply-patch', 'reads electron-store for the applied-patch guard before it delegates to patch-apply']
+	['git:apply-patch', 'reads electron-store for the applied-patch guard before it delegates to patch-apply'],
+	['git:list-ticket-patches', 'reads electron-store for the ticket before it can reach github-prs']


+ * @param {Object} [headers]
+ * @return {Promise<{status: number, headers: Object, body: string}>}
+ */
+function httpGet(url, headers = {}) {


+	const query = encodeURIComponent(`repo:${REPO} is:pr ${id}`);
+	const url = `https://api.github.com/search/issues?q=${query}&per_page=30`;


+  useEffect(() => {
+    if (tracTicket) loadTicketPatches();
+    else setTicketPatches(null);
+  }, [tracTicket, loadTicketPatches]);


With a site linked to a Trac ticket (#109) and an engine that applies a patch (#11), this connects the two: it lists the work already on the ticket so a contributor can see it — and test it — before adding their own. That is the Contributor-Day failure the flow exists to prevent: several people writing overlapping patches because none could see anybody else's. On the busiest tickets the real work is a wordpress-develop pull request, not a Trac attachment — the attachment list is emptiest exactly where activity is highest. So this ships the PR half first. PRs are discovered through GitHub's documented convention: a PR cites its ticket in the body, so a broad search for the number is verified narrowly, locally, against the ticket URL — GitHub's tokeniser matches the bare number in unrelated text, so the verification is what makes the list trustworthy. Applying a PR fetches its diff and hands it to the existing apply engine, so a downloaded file and a linked PR are one path from the preview onward. Two constraints shaped the network code: - The web .diff routes (github.com/.../pull/N.diff and patch-diff) return 422 to unauthenticated clients now, so the diff is fetched through the REST API with the diff media type instead. Requests go through Electron's net rather than 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 its 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. Trac attachments — behind the proof-of-work interstitial — are deliberately not listed yet; the panel points at the ticket for those. The parse, verify and failure-classification logic is a pure module (src/patch-sources.cjs) unit tested without a network. Refs #109, #11, part of #110. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…opilot #136 review) - fetchLinkedPrs requests per_page=100 (one request, not pagination — the shared unauthenticated quota is the constraint) and treats incomplete_results or total_count beyond the page as not-authoritative, returning a non-ok status so the handler falls back to the last-known-good cache rather than caching a partial list as complete (#3). - httpGet gains a small injectable seam (net + timers) so its success, transport-error, timeout, and settle-once paths are tested without the network; electron is now required lazily so the standalone test never reaches it (#4). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Every SiteRow stays mounted (the parent hides inactive ones with display:none), so the load-on-link effect fired a GitHub search for every linked site on each launch — against the shared, unauthenticated 60/hour quota. Gate the fetch to the active site and remember the last-loaded ticket, so a launch costs at most one search; a relink and the Refresh button still fetch, and unlinking clears the list. (#1) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…le (Copilot #136 review) The handler reads the store for the ticket and delegates to fetchLinkedPrs — reachable through the fakeSettingsStore seam the apply-patch tests already use. Move it from NOT_REACHABLE to WIRED with handler tests: an ok fetch delegates with the stored ticket, a failed fetch falls back to the cached last-known-good list, and no linked ticket short-circuits without calling github-prs. (#2) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…-review) A failed initial fetch is deliberately not retried on every re-activation — that could keep spending a rate-limited quota — so the Refresh button is the retry. Documenting the intent flagged in self-review; no behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

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>

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>

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 force-pushed the juanmaguitar/list-ticket-patches branch from 74fdf43 to 09d3df1 Compare August 6, 2026 11:32

This was referenced Aug 6, 2026

List and apply a ticket's Trac attachments #139

Merged

Mark which patch on a ticket is the latest, PR or attachment #140

Merged

Fix the apply panel's error/copy states, and apply a PR by URL #142

Merged

juanmaguitar force-pushed the juanmaguitar/list-ticket-patches branch from 09d3df1 to 32d9967 Compare August 7, 2026 05:19

juanmaguitar force-pushed the juanmaguitar/list-ticket-patches branch from 32d9967 to 7e01a31 Compare August 7, 2026 06:27

juanmaguitar force-pushed the juanmaguitar/list-ticket-patches branch from 7e01a31 to e641541 Compare August 7, 2026 06:47

juanmaguitar requested a balanced review from Copilot August 7, 2026 06:56

Copilot started reviewing on behalf of juanmaguitar August 7, 2026 06:56 View session

Copilot AI reviewed Aug 7, 2026

View reviewed changes

juanmaguitar force-pushed the juanmaguitar/list-ticket-patches branch from e641541 to c3e6c91 Compare August 7, 2026 07:58

juanmaguitar and others added 4 commits August 7, 2026 11:01

juanmaguitar merged commit 6a0f407 into trunk Aug 7, 2026
3 checks passed

juanmaguitar deleted the juanmaguitar/list-ticket-patches branch August 11, 2026 11:29

Navigation Menu

Uh oh!

Show the pull requests linked to a ticket, and apply one - #136

Show the pull requests linked to a ticket, and apply one#136
juanmaguitar merged 5 commits into
juanmaguitar/issue-11-apply-patchfrom
juanmaguitar/list-ticket-patches

juanmaguitar commented Aug 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		const query = encodeURIComponent(`repo:${REPO} is:pr ${id}`);
		const url = `https://api.github.com/search/issues?q=${query}&per_page=30`;

Uh oh!

Conversation

juanmaguitar commented Aug 6, 2026

Why

What this does

How, and the constraints that shaped it

Testing

Self-review (per AGENTS.md)

Not done

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants