Skip to content

Fix the pull request preview listing trunk's files when the site's trunk is behind - #702

Merged
ryanwelcher merged 2 commits into
trunkfrom
ryanwelcher/a-pull-requests-file-list-includes-trunk-commits
Oct 20, 2026
Merged

ryanwelcher merged 2 commits into
trunkfrom
ryanwelcher/a-pull-requests-file-list-includes-trunk-commits

Conversation

@ryanwelcher

Copy link
Copy Markdown
Collaborator

Why

When a site's local trunk is older than the commit a pull request branched from, the pull request preview lists every file trunk changed in between as well as the pull request's own. @zaerl hit this on #677: WordPress/wordpress-develop#14541 changes 2 files, and the app listed dozens.

What changes

The root cause: describePullRequestHead measured the list from the merge base with the site's local trunk, which only moves when the contributor updates trunk.

  • The git:preview-pr handler now fetches origin's trunk before the pull request head and measures the list from that commit. It reads the commit from FETCH_HEAD, so it works on a site with no remote-tracking ref. The site's own trunk and the checkout are not moved.
  • If that fetch fails, the preview falls back to the site's trunk, logs the error, and the preview dialog says: "Could not download the latest trunk, so this list may also include changes from trunk that are not part of this pull request."

Deliberately not in this PR: the checkout itself and the recorded base of pr/N are unchanged. Only the preview list was wrong.

How to test this

Platforms: any. Use the Buildkite build for the current head, 0f91e22.

Starting state: a wordpress-develop site on trunk with no edits, whose trunk is older than WordPress/wordpress-develop#14541. To make one, quit the app and run this from the site's folder (needs Git on your machine):

git reset --hard HEAD~300

Write down the commit it prints, or run git rev-parse trunk and keep the output.

  1. Open the app and select the site. Do not click to update trunk.
  2. On the Apply a patch or PR card, on the Pull request tab, type 14541 in Pull request URL or number and click Apply PR.
    • Expected: the dialog titled Apply PR #14541 says "PR #14541 changes 2 files."
    • Expected: the "Could not download the latest trunk…" line is not shown.
  3. Click Cancel.
  4. From the site's folder, run git rev-parse trunk.
    • Expected: the same commit you wrote down before step 1.

To see the bug, run the same steps on a trunk build: step 2 says "changes" followed by a much larger number of files.

What must not have happened:

  • The site's trunk moved (step 4). The preview downloads origin's trunk but must not update the site's own.
  • The checkout changed, or a rebuild or npm install started, when you only opened the preview and cancelled.

Tests: the two git:preview-pr tests in tests/unit/ipc-wiring.test.cjs fail on the old handler. The trunk-download failure case is covered by those tests and by the new describePrPreview test in tests/unit/pr-checkout-renderer.test.cjs.

Risks and limitations

  • The fallback notice can't be tested by hand. It needs the trunk fetch to fail while the pull request fetch straight after it succeeds, and I could not stage that.
  • Every preview now makes one more network round trip. The fetch brings commits and trees only, with no file contents.
  • A pull request based on a release branch is still measured against trunk's merge base, so its list also carries that branch's own commits. That is unchanged and documented in describePullRequestHead.
  • Review: 0 to fix here, 1 follow-up, fixed in this PR.

Related

Fixes #701. Reported on #677.


Design decisions and alternatives considered
  • Fetch only on the preview, not on checkout. The checkout records pr/N's base from the fetched head and does not use the list, so the bug was confined to the preview.
  • Fall back instead of refusing. A failed trunk fetch should not block testing a pull request. The preview still works and says its list may be too long.
  • One fetch for both refs would save the round trip, but it needs a new multi-line FETCH_HEAD reader in git-read.cjs. Not worth it for now.
Review outcome (required — see AGENTS.md)

0 [fix here] · 1 [follow-up]. The follow-up is fixed in 0f91e22.

  • Fixed: when the trunk fetch failed, the preview fell back without saying so, which brought A pull request's file list includes trunk commits when the site's trunk is behind #701 back silently. The handler now returns measuredFromSite, and the dialog shows a sentence when it is true.
  • Fixed (style): the fetchOriginTrunk comment said the list was measured against "the trunk it was written on". It now says origin's current trunk.
  • Checked with no finding: the trunk commit is read before the PR fetch overwrites FETCH_HEAD; the partial clone fetches no file contents; the wiring tests fail without the fix; bundled Git only, the quit sweep covers the new fetch, no store writes.

Records:

  • Review: completed. Separate read-only agent context, following .github/instructions/code-review.instructions.md. Reviewed head 991f291 / base e43e9c8. Lint clean, unit suite 2113 pass / 0 fail, PR journeys (pr-checkout, apply-card, patch-apply, gutenberg-site) 17 passed. Outcome: 1 follow-up.
  • Since review: 991f291 → 0f91e22 checked. This commit is the follow-up fix and the comment correction. Lint clean, unit suite 2114 pass / 0 fail, the same 17 journeys pass. Not re-reviewed by a separate context.
Implementation notes
  • fetchOriginTrunk in src/pr-checkout.js is fetchBranch(dir, origin, trunk). It goes through streamGit on the bundled binary and is registered with trackGitChild, so quitting ends it.
  • measuredFromSite travels in the preview result, which use-apply-patch.jsx already spreads into applyPreview. describePrPreview turns it into trunkNote, and previewWords passes that to the dialog.
Screenshots or recording

None. The only visible change is the fallback sentence, which can't be staged by hand (see Risks). The file count in step 2 is the before and after.

🤖 Generated with Claude Code

A site whose trunk is behind the commit a pull request branched from listed every file trunk changed in between as the pull request's. The preview now fetches origin's trunk first and measures from that commit, falling back to the site's trunk when it cannot be fetched. Fixes #701.
When origin's trunk cannot be fetched, the preview falls back to the site's own trunk, and the list can carry trunk's changes again. The handler now reports that fallback and the preview dialog adds a sentence saying so. Also corrects the fetchOriginTrunk comment, which said the list was measured against the trunk the pull request was written on.
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 57 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: WordPress/contributor-toolkit/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9e350088-6a10-4863-9274-f26f31b5ce72
📥 Commits

Reviewing files that changed from the base of the PR and between e43e9c8 and 0f91e22.

📒 Files selected for processing: 10
  • src/main.js
  • src/pr-checkout.js
  • src/renderer/apply-card.cjs
  • src/renderer/components/apply-card.jsx
  • src/renderer/index.jsx
  • src/renderer/pr-checkout.cjs
  • tests/unit/apply-card.test.cjs
  • tests/unit/ipc-wiring.test.cjs
  • tests/unit/pr-checkout-renderer.test.cjs
  • tests/unit/pr-checkout.integration.test.cjs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ryanwelcher

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@ryanwelcher
ryanwelcher requested a review from zaerl October 20, 2026 13:49
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@zaerl zaerl 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.

Work as expected, thanks.

@ryanwelcher
ryanwelcher merged commit 2dda92c into trunk Oct 20, 2026
13 checks passed
@ryanwelcher
ryanwelcher deleted the ryanwelcher/a-pull-requests-file-list-includes-trunk-commits branch October 20, 2026 14:06
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.

A pull request's file list includes trunk commits when the site's trunk is behind

2 participants