fix: align "part N of M" with <kbd> labels in stacked PR footers by wmichelin · Pull Request #13363 · gitbutlerapp/gitbutler

wmichelin · 2026-04-17T17:42:36Z

This bug may also exist on the Desktop version of the app, but I haven't tested that yet.

🧢 Changes

Change nth = stack_length - stack_index to nth = stack_index + 1 in generate_footer (Rust)
Apply same fix in TypeScript mirror (prFooter.ts), plus reverse iteration order to list top-first (matching Rust)
Update test expectation in test_generate_footer_large_stack

☕️ Reasoning

The <kbd> labels number the stack with 1 = base and highest = top, but the "part N of M" text used the opposite convention. This made the base PR say "part 3 of 3" while its <kbd> label said 1.

🎫 Affected issues

Fixes: #13362

The `generate_footer` function computed `nth = stack_length - stack_index`, counting from the top of the stack. But the `<kbd>` labels count from the base (1 = base, highest = top). This made the base PR say "part 3 of 3" while its <kbd> label said 1. Change to `nth = stack_index + 1` so both numbering systems agree. Also fix the TypeScript mirror in prFooter.ts: same nth formula, plus reverse the iteration order to list top-first (matching Rust output). Fixes: gitbutlerapp#13362

These tests previously only checked the <kbd> label and emoji, not the "part N of M" text. Adding the assertion ensures the numbering stays consistent and prevents the original bug from regressing.

wmichelin · 2026-04-17T17:51:14Z

don't seem to be able to run CI myself, understandably 😄

Byron · 2026-04-19T07:17:51Z

@codex review Also consider my message #13362 (comment) which seems to indicate that it works as expected, at least with the current Nightly build.

Byron · 2026-04-19T07:19:14Z

Thanks a lot for taking a stab at this!

I definitely like the addition of the part assertion to the tests, which seems very valuable, given that it seems to be inverted sometimes the question is just why it seems to work for me. Please see this message for reference.

Copilot

Pull request overview

Fixes inconsistent “part N of M” numbering in stacked PR footers so it matches the <kbd> stack labels (1 = base, M = top), addressing issue #13362.

Changes:

Adjust Rust footer generation to use nth = stack_index + 1 (base-first numbering).
Apply the same numbering fix in Desktop’s TypeScript mirror and reverse iteration to list top-of-stack first (matching Rust).
Update Rust test expectations to assert the corrected “part N of M” text.

Reviewed changes

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

File	Description
`crates/but-forge/src/review.rs`	Fixes stack “part N” calculation and updates tests to validate corrected numbering.
`apps/desktop/src/lib/forge/shared/prFooter.ts`	Mirrors the corrected “part N” logic and aligns listing order with Rust by reversing iteration.

Copilot · 2026-04-19T07:20:03Z

+	[...allPrNumbers].reverse().forEach((prNumber, i) => {
+		const current = prNumber === forPrNumber;
 		footer += `- <kbd>&nbsp;${stackLength - i}&nbsp;</kbd> ${symbol}${prNumber} ${current ? "👈 " : ""}\n`;
 	});


This change updates footer ordering/numbering on Desktop, but generateFooter doesn’t appear to have unit tests. Adding a small Vitest suite for the header "part N of M" and the <kbd> numbering/order (mirroring the Rust tests) would help prevent regressions.

Copilot · 2026-04-19T07:20:03Z

@@ -138,13 +138,13 @@ function clearFooter(body: string | undefined) {
 function generateFooter(forPrNumber: number, allPrNumbers: number[], symbol: string) {
 	const stackLength = allPrNumbers.length;
 	const stackIndex = allPrNumbers.findIndex((number) => number === forPrNumber);


allPrNumbers.findIndex(...) returns -1 when forPrNumber isn’t present. With nth = stackIndex + 1, that would render "part 0 of N". Consider handling the -1 case explicitly (e.g., fallback to 1, or return/throw) to avoid generating an invalid footer when inputs are inconsistent.

Suggested change

const stackIndex = allPrNumbers.findIndex((number) => number === forPrNumber);

const stackIndex = allPrNumbers.findIndex((number) => number === forPrNumber);

if (stackIndex === -1) {

throw new Error(`Pull request #${forPrNumber} is not part of the provided stack.`);

}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2014f2d944

ℹ️ 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".

chatgpt-codex-connector · 2026-04-19T07:23:44Z

 	const stackLength = allPrNumbers.length;
 	const stackIndex = allPrNumbers.findIndex((number) => number === forPrNumber);
-	const nth = stackLength - stackIndex;
+	const nth = stackIndex + 1;


Derive part number from stack order used by desktop

generateFooter() now assumes allPrNumbers is base→top (stackIndex + 1), but desktop callers pass stack order as top→base: in the same file, updateStackPrs() builds allPrNumbers directly from branchDetails and iterates branchDetails from end to start to compute targetBase, which only works for top→base order. With that input, this change makes base PRs render as the highest part number (e.g. base becomes part 3 of 3), effectively reintroducing inverted stack numbering in desktop footers.

Useful? React with 👍 / 👎.

BranchDetails might also be ordered 'the other way around'. New data structures put the top-most stack into slot 0, to traversal is top-to-bottom, while older data structures do it the other way around.
Maybe this also plays a role.

wmichelin · 2026-04-19T07:49:45Z

Thanks a lot for taking a stab at this!

I definitely like the addition of the part assertion to the tests, which seems very valuable, given that it seems to be inverted sometimes the question is just why it seems to work for me. Please see this message for reference.

I was using the CLI. Have you tried that? Maybe this bug only exists there?

I believe the logic is replicated between the typescript and rust codebases.

Byron · 2026-04-19T08:39:10Z

Right, I think that's the core of the problem.

The graphical user interface already seems to be doing it correctly, so I'm not sure about the related changes in this PR, but the backend may have had its very own implementation. And maybe that's where it went wrong.

This bug may also exist on the Desktop version of the app, but I haven't tested that yet.

Since I tested that and wasn't able to reproduce it, it's probably not present there.

It definitely seems the auto review is worth investigating.

wmichelin and others added 3 commits April 17, 2026 13:39

Merge branch 'master' into fix/stacked-pr-footer-numbering

1a39cf8

test: assert "part N" matches <kbd> label for base and top PRs

2014f2d

These tests previously only checked the <kbd> label and emoji, not the "part N of M" text. Adding the assertion ensures the numbering stays consistent and prevents the original bug from regressing.

wmichelin marked this pull request as ready for review April 17, 2026 17:51

Byron requested a review from Copilot April 19, 2026 07:17

Copilot started reviewing on behalf of Byron April 19, 2026 07:17 View session

Copilot AI reviewed Apr 19, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Apr 19, 2026

View reviewed changes

Byron marked this pull request as draft April 19, 2026 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: align "part N of M" with <kbd> labels in stacked PR footers#13363

fix: align "part N of M" with <kbd> labels in stacked PR footers#13363
wmichelin wants to merge 3 commits intogitbutlerapp:masterfrom
wmichelin:fix/stacked-pr-footer-numbering

wmichelin commented Apr 17, 2026 •

edited

Loading

Uh oh!

wmichelin commented Apr 17, 2026

Uh oh!

Byron commented Apr 19, 2026

Uh oh!

Byron commented Apr 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Apr 19, 2026

Uh oh!

Byron Apr 19, 2026

Uh oh!

wmichelin commented Apr 19, 2026

Uh oh!

Byron commented Apr 19, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

wmichelin commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🧢 Changes

☕️ Reasoning

🎫 Affected issues

Uh oh!

wmichelin commented Apr 17, 2026

Uh oh!

Byron commented Apr 19, 2026

Uh oh!

Byron commented Apr 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Byron Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

wmichelin commented Apr 19, 2026

Uh oh!

Byron commented Apr 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wmichelin commented Apr 17, 2026 •

edited

Loading

Byron commented Apr 19, 2026 •

edited

Loading