Skip to content

Fix order-dependent PR-stack grouping via leaf-rooting#815

Merged
backnotprop merged 2 commits into
feat/single-server-runtimefrom
fix/pr-stack-grouping
May 28, 2026
Merged

Fix order-dependent PR-stack grouping via leaf-rooting#815
backnotprop merged 2 commits into
feat/single-server-runtimefrom
fix/pr-stack-grouping

Conversation

@backnotprop
Copy link
Copy Markdown
Owner

The bug

buildStacks (project dashboard PR grouping in LandingPage.tsx) walked PR chains only downward (toward the base) and marked each PR consumed as it went. A descendant discovered after its parent chain was already built could not attach and was dumped into loose.

Concretely, a 3-deep stack A(base=main) ← B(base=A) ← C(base=B) grouped correctly only when the input arrived leaf-first ([C, B, A]). For base-first ([A, B, C]) or interleaved orderings, B was consumed into [A, B] first; then C walked down, hit already-consumed B, had length 1, and orphaned into loose. The grouping depended on the order the API returned PRs — which is wrong.

The fix

Root every chain from a leaf — a PR whose headBranch is not any other PR's baseBranch — then walk down from each leaf following baseBranch ← headBranch edges. A leaf-rooted walk captures the full chain in one pass regardless of input order.

Output shape is unchanged:

  • stacks: chains of length > 1, ordered base → leaf, labelled #<base> → #<leaf>.
  • loose: everything else, in original input order.

The base PR (whose base is the default branch) is still included via the walk. Cycles fall through to loose (their members are never leaves, and the walk guard prevents infinite loops). Forks (a branch that is the base of two PRs) degrade to one stack plus a loose sibling without double-counting the shared ancestor.

To make it testable, buildStacks and the PRStack interface were extracted into a new pure module apps/frontend/src/components/landing/buildStacks.ts (no React imports). LandingPage.tsx now imports both; the call-site signature is unchanged.

Test coverage

New table-driven vitest suite apps/frontend/src/components/landing/buildStacks.test.ts:

  • 3-deep stack given leaf-first, base-first, and interleaved orderings → all produce the same single #1 → #3 stack with 3 PRs.
  • 4-deep stack (multiple orderings).
  • Two independent stacks in one input.
  • All-loose (every PR based on the default branch).
  • A single non-default-based PR with no parent in the set → loose.
  • Empty input.
  • Permutation-invariance: every permutation of a 3-deep stack, a 4-deep stack, and two independent stacks yields identical grouping.
  • Cycle (does not loop, lands in loose) and fork (no double-counted base) edge cases.

The suite was confirmed to fail against the old downward-only algorithm (12 failures, including the base-first 3-deep case) and pass against the fix (18 passing). A separate parity check over 2000 random leaf-first forests confirmed the new algorithm agrees with the old one wherever the old one was correct, i.e. the fix is a strict generalization.

Verification

In apps/frontend:

  • bun run test → 18 passed.
  • bun run typecheck → exit 0 (the only diagnostic is a pre-existing @vitest/browser-playwright module-resolution error in vitest.browser.config.ts, present on the base branch and unrelated to this change).

Also marked the "PR stack splitting is order-dependent" backlog entry as DONE in goals/frontend-session-lifecycle/backlog.md.

buildStacks walked only downward (toward the base) and marked PRs
consumed as it went, so a descendant discovered after its parent chain
was already built could not attach and was dumped into loose. A 3-deep
stack A(base=main) <- B(base=A) <- C(base=B) grouped correctly only when
the input arrived leaf-first ([C,B,A]); for base-first or interleaved
orderings the chain split and orphaned PRs into loose. The result
depended on the order the API returned PRs.

Root every chain from a leaf (a PR whose head branch is not any other
PR's base branch) and walk down toward its base. A leaf-rooted walk
captures the full chain in one pass regardless of input order. Output
shape is unchanged: stacks (chains > 1, base->leaf, '#<base> -> #<leaf>'
labels) and loose (everything else, in original order).

Extracted buildStacks and the PRStack interface into a pure module
(no React imports) so it is unit-testable, and added a table-driven
vitest suite asserting that every permutation of the same stack yields
identical grouping, plus cycle and fork edge cases.
…ch param

Addresses the 4-model review of #815:

- Remove the unused `_defaultBranch` param from `buildStacks` and drop it from
  the `LandingPage` useMemo + deps, fixing the latent stale-default split (UI
  held "main" while the real default was "master").
- Make fork grouping deterministic: visit candidate leaves in ascending PR
  number so the lowest-numbered leaf claims a shared ancestor regardless of
  input order; the sibling falls through to loose.
- Resolve duplicate head branches deterministically in `byHead` (prefer open,
  then lower number) so chain-following no longer depends on input order.
- Sort the returned `stacks` (by base PR number) and `loose` (by number) so
  independent stacks never swap positions between 30s polls.
- Tidy the length-1 release to `stacked.delete(chain[0].id)`.
- Tests: rewrite the fork test to assert a deterministic outcome across ALL
  permutations; add duplicate-head and leaf-into-cycle permutation tests; drop
  the sorted() wrappers so tests assert the now-deterministic output order.
- Soften the doc comment + backlog: order-independent grouping including the
  fork tiebreak, explicitly stated as a 'one child wins' policy, not full fork
  collapse.
@backnotprop
Copy link
Copy Markdown
Owner Author

Fix pass applied from the 4-model interrogate review. Pushed to fix/pr-stack-grouping.

Fix 1 — drop dead _defaultBranch (UNANIMOUS). Removed the unused param from buildStacks and from the only call site (LandingPage useMemo + its deps). This also kills the latent stale-default split bug: when the real default is master but the UI still holds its initial "main", the old useMemo dep produced wrong groupings. Edits are confined to the useMemo hunk (lines 562-565) — disjoint from #814's GitLab placeholder/error region.

Fix 2 — deterministic forks (UNANIMOUS). Candidate leaves are now visited in ascending PR-number order, so when two PRs share a base, the lowest-numbered child deterministically claims the shared ancestor and the sibling falls through to loose — no longer order-dependent. The fork test now asserts this across all permutations of [root, child1, child2] (lowest child wins, other loose) instead of a hardcoded toContain(72). Doc comment + backlog softened: this is an explicit "one child wins, rest loose" policy (not full fork collapse), but the choice is order-independent.

Fix 3 — deterministic byHead (Consider). Head-branch collisions (e.g. a merged + open PR on the same branch from state=all) now resolve to the open PR, then the lower number, via a preferHead tiebreak. Added a permutation test where a tip follows a duplicated mid head — always resolves #89 → #91 with the merged duplicate loose.

Fix 4 — stable output order (Consider). stacks sorted by base PR number, loose by number, so independent stacks never swap on-screen between 30s polls. Permutation tests now assert the unsorted output arrays (removed the sorted()/sortedNums() wrappers that were masking order).

Fix 5 — cycle test + loop tidy (Consider). Added a leaf-into-cycle permutation test pinning current bounded behaviour (terminates via the stacked guard, forms a #66 → #67 stack). Simplified the length-1 release to stacked.delete(chain[0].id).

Verification.

  • Meaningfulness proven: the rewritten fork, duplicate-head, and output-order tests all produce 2 distinct outcomes against the old downward-only / last-write-wins algorithm (i.e. they FAIL the old code), and exactly 1 against the new one.
  • Brute-forced all 5040 permutations of a mixed input (linear stack + fork + loose) → 1 distinct outcome.
  • apps/frontend: bun run test 20/20 pass; bun run typecheck clean (the only error is the pre-existing vitest.browser.config.ts missing-dep, unrelated).
  • Root bun test: my changes add passing assertions and introduce zero new failures (the 2 pre-existing fail / 2 errors live in apps/pi-extension / remote.test.ts, unrelated to stack grouping).

@backnotprop backnotprop merged commit 769fcee into feat/single-server-runtime May 28, 2026
@backnotprop backnotprop deleted the fix/pr-stack-grouping branch May 29, 2026 01:58
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.

1 participant