Fix detectTargetBranch resolving head branch instead of base - #155
Conversation
.base-ref and .commit-ref no longer match the base branch on GitHub's modern Primer React PR header; .commit-ref in particular belongs to unrelated "force-pushed the X branch" timeline events. This made detectTargetBranch() return the PR's head branch instead of base, so the openshift/release presubmits YAML lookup 404'd and Test buttons fell back to raw check names (e.g. "/test ci/prow/4.23-e2e-test-aws" instead of "/test 4.23-e2e-test-aws"), as seen on openshift/oadp-operator#2206. Resolve the base branch from the PR header's /tree/ link that points at the current repo instead: base is always same-repo (head may be a fork) and always precedes head in DOM order, so this holds regardless of GitHub's generated CSS class names. Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesTarget branch detection
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes detectTargetBranch() in the content script so it reliably resolves a PR’s base branch (rather than accidentally picking up the head branch from unrelated DOM elements), preventing presubmit config lookups from 404’ing and forcing “Test” buttons to fall back to raw check context names.
Changes:
- Updated
detectTargetBranch()to derive the base branch from the first same-repo/tree/<branch>link in the PR header DOM instead of relying on stale.base-ref/.commit-refselectors. - Removed the
.commit-reffallback that could resolve to the head branch via “force-pushed … branch” timeline events. - Added a Backlog task record documenting the bug, root cause, and fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
content.js |
Changes base-branch detection logic used for presubmit job lookup to avoid selecting the PR head branch. |
backlog/tasks/task-14 - Fix-detectTargetBranch-resolving-head-branch-instead-of-base-branch.md |
Adds task documentation describing the issue and the implemented fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const prefix = `/${repo}/tree/`; | ||
| const treeLink = Array.from(document.querySelectorAll('a[href]')) | ||
| .find(a => a.getAttribute('href').startsWith(prefix)); | ||
| if (treeLink) { | ||
| const branch = treeLink.getAttribute('href').slice(prefix.length); | ||
| if (branch) return decodeURIComponent(branch); | ||
| } |
Summary
detectTargetBranch()used.base-ref/.commit-refselectors that no longer match the base branch on GitHub's modern Primer React PR header —.commit-refin particular belongs to unrelated "force-pushed the X branch" timeline events, so it resolved to the PR's head branch instead of base.lastPresubmitJobswas null and the Test buttons fell back to the raw check context (/test ci/prow/4.23-e2e-test-aws) instead of the bare job name (/test 4.23-e2e-test-aws)./tree/link that points at the current repo: base is always same-repo (head may be on a fork) and always precedes head in DOM order, so this holds regardless of GitHub's generated CSS class names.Test plan
npm test— 191/191 passingOADP-7943-fix-annotation-reconcile-1.6(head), new logic resolvesoadp-1.6(base)Note
Responses generated with Claude
Summary by CodeRabbit
Bug Fixes
Tests