Build packages before testing them, and report PR build on every PR - #1500
Conversation
Two changes, both prerequisites for making `build (24.x)` a required check on develop. It is not required today, which is how #1493 merged with six consecutive red builds behind it. Build order. apps/web resolves `@ecency/sdk` to the COMMITTED packages/sdk/dist, so running the tests first tests a stale build whenever a PR changes SDK source. #1493 spent six runs red on 36 tests whose source and mocks were both correct: `isAuthorMuted` existed in src, but not yet in the committed dist, and only landed there when the changeset commit rebuilt it. A required check that fails for a reason its author cannot act on is exactly what teaches people to merge past CI, so consumers now test what the PR actually builds. staging.yml and master.yml had the same ordering, and get the same move. Trigger. PR build opted in by branch prefix, which leaves a branch named anything else with no build at all, and would leave such a PR unable ever to satisfy the check once it is required: Dependabot, Renovate, forks and hotfix/* would be blocked with no way to produce it. Keying on the pull request closes both holes, and matches what lint.yml and typecheck.yml already do for the same reason. Making the check required is a repo setting and cannot ride along in a PR. It should be flipped once this merges.
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
PR Summary by QodoMake PR build run on all PRs and build packages before tests
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Include the pull request identity in the concurrency key
On pull_request runs, github.head_ref contains only the source branch name, so PRs from different forks with a common name such as main or patch-1 share this concurrency group. If those builds overlap, cancel-in-progress: true cancels one PR's required build even though the commits are unrelated, leaving that PR blocked until it is rerun. Key the group by github.event.pull_request.number or include the head repository's full name.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code Review by Qodo
1. Cross-PR build cancellations
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| on: | ||
| push: | ||
| branches: | ||
| # Every prefix in real use, not just the two most common. This list is an | ||
| # opt-IN, so a branch named anything else silently gets no build at all -- | ||
| # which is how a change to next.config.js (a build-time file that neither | ||
| # tsc nor eslint evaluates) reached review with nothing having compiled it. | ||
| # Add new prefixes here, or they inherit that same silent gap. | ||
| - 'bugfix/*' | ||
| - 'feature/*' | ||
| - 'fix/*' | ||
| - 'chore/*' | ||
| - 'refactor/*' | ||
| - 'docs/*' | ||
| # Bot-authored PRs were previously merged without ever being built. | ||
| - 'seer/*' | ||
| pull_request: |
There was a problem hiding this comment.
1. Cross-pr build cancellations 🐞 Bug ☼ Reliability
After switching PR build to run on pull_request, the workflow-level concurrency group now keys primarily on github.head_ref (source branch name), so different PRs with the same branch name (common with forks/bots) can cancel each other’s runs. This can intermittently prevent a PR’s latest commit from ever getting a completed PR build status, making a required check unreliable.
Agent Prompt
## Issue description
`PR build` now triggers on `pull_request`, but its concurrency group uses `${{ github.head_ref || github.ref }}`. On PR events, `github.head_ref` is just the source branch name, which can collide across unrelated PRs (especially forks), causing runs to cancel each other.
## Issue Context
When `cancel-in-progress: true`, any workflow run in the same concurrency group cancels the in-flight run. If two PRs share the same `head_ref`, their runs can interfere.
## Fix Focus Areas
- .github/workflows/PR-branch.yml[12-14]
## Proposed change
Update the concurrency group to be PR-unique, e.g.:
```yaml
concurrency:
group: ${{ github.workflow }}-pr-${{ github.event.pull_request.number }}
cancel-in-progress: true
```
(If this workflow might later run on non-PR events, add a fallback using `github.ref`.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Groundwork for making
build (24.x)a required check ondevelop. It is not required today, which is how #1493 merged with six consecutive red builds and 36 failing tests behind it:lint (24.x)andtypecheck (24.x)are the only required contexts, neither runs tests, and both were green.Two things have to change before that setting can be flipped safely.
Build packages before running them
apps/webresolves@ecency/sdkto the committedpackages/sdk/dist, so running tests first tests a stale build whenever a PR changes SDK source.That is precisely what reddened #1493.
isAuthorMutedwas added to src at 07:18 but only reached the committed dist at 08:34, when the label-triggered changeset commit rebuilt it. Every run in between failed withNo "isAuthorMuted" export is defined on the "@ecency/sdk" mock, while the source and the mocks were both correct throughout. Re-running that suite on current develop gives 2742 passing.A required check that fails for a reason its author cannot act on is what teaches everyone to merge past CI, so consumers now test what the PR actually builds.
staging.ymlandmaster.ymlcarried the same ordering and get the same move.Report on every pull request
PR buildopted in by branch prefix. Two holes: a branch named anything outside the list got no build at all, and once this check is required, such a PR could never satisfy it. Dependabot, Renovate, forks andhotfix/*would be blocked with no way to produce the check.Keying on the pull request closes both. This is what
lint.ymlandtypecheck.ymlalready do, and their comments cite this same trap.Remaining manual step
Adding
build (24.x)to the required contexts ondevelopis a repo setting and cannot ride along in a PR. Worth doing once this merges, so the check reports on every PR first.This PR exercises its own change: the
PR buildrun below is the new trigger and the new ordering.