Stack Entry: Carry associated review information#12367
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
bot
added
rust
There was a problem hiding this comment.
Pull request overview
This PR extends legacy stack/branch UI data to carry associated forge review information (e.g. PR/MR number) so CLI flows can more reliably determine whether a branch already has an associated review.
Changes:
- Add an optional
review_idfield toStackHeadInfoand populate it from branch metadata / segment metadata. - Update PR creation flow to treat branches with an existing associated review as “not eligible” even if there are no currently-open reviews in the cached review map.
- Add metadata helper to read the associated forge PR/MR number from
virtualbranches.tomlmetadata.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| crates/but/src/command/legacy/forge/review.rs | Updates “branches without PRs” selection logic to also consider stack head associated review info. |
| crates/but-workspace/src/legacy/ui.rs | Adds new StackHeadInfo field to expose associated forge review data to consumers. |
| crates/but-workspace/src/legacy/stacks.rs | Populates the new field from stack branches/segments and metadata. |
| crates/but-api/src/legacy/virtual_branches.rs | Ensures newly-created stack entries include the new associated review field. |
|
Sorry for the reviewers, this was supposed to be a draft |
estib-vega
force-pushed
the
stack-entry-reviews
branch
from
7d7f416 to
ac96c21
Compare
| name: &gix::refs::FullNameRef, | ||
| meta: &VirtualBranchesTomlMetadata, | ||
| ) -> anyhow::Result<Option<usize>> { | ||
| let ref_meta = meta.branch(name)?; |
There was a problem hiding this comment.
Should this use meta.branch_opt. just .branch will error if there isn't a corresponding virtual_branches.toml entry.
Since it seems like the only time we call this function is in the case where we haven't already found the virtual_branches.toml entry, it seems quite likely that this will cause unexpected errors.
There was a problem hiding this comment.
Oooh didn't know that, thanks!
| // no associated reviews. | ||
| // Check whether there's an associated forge review. | ||
| if head.review_id.is_none() { | ||
| // If there's no associated review, the append the branch |
There was a problem hiding this comment.
Typo in comment: "the append" should be "then append".
| // If there's no associated review, the append the branch | |
| // If there's no associated review, then append the branch |
estib-vega
force-pushed
the
stack-entry-reviews
branch
from
ac96c21 to
a537bc7
Compare
Stack entries also know the associtated forge review ID.
estib-vega
force-pushed
the
stack-entry-reviews
branch
from
a537bc7 to
773881a
Compare
| branches_without_prs.push(branch_name); | ||
| // This means that there are no associated reviews that are open, but that doesn't mean that there are | ||
| // no associated reviews. | ||
| // Check whether there's an associated forge review. |
There was a problem hiding this comment.
The comment says "Check whether there's an associated forge review" but the code is checking for a PR/MR number (stored in review_id, which should be named pr_number). The comment should say "Check whether there's an associated PR/MR number" to be more accurate.
| // Check whether there's an associated forge review. | |
| // Check whether there's an associated PR/MR number. |
| // no associated reviews. | ||
| // Check whether there's an associated forge review. | ||
| if head.review_id.is_none() { | ||
| // If there's no associated review, the append the branch |
There was a problem hiding this comment.
There's a typo in the comment: "the append the branch" should be "then append the branch".
| // If there's no associated review, the append the branch | |
| // If there's no associated review, then append the branch |
2 participants
Stack entries also know the associtated forge review ID.
This makes it easier to check whether a branch has an associated PR already.