Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 52 additions & 5 deletions internal/issue/blocking.go
Original file line number Diff line number Diff line change
Expand Up @@ -445,15 +445,25 @@ func (o *subtreeOverlay) effectiveBlockedBy(iss *Issue) []string {
// buildSubtreeOverlay builds subtrees from parent fields, collects
// external blockers per display root, and marks all descendants.
//
// A "display root" is the shallowest open/deferred ancestor with children.
// When an ancestor is in_progress or in_review it is already claimed work, so
// we drill past it to surface the actual next-step descendants.
// A "display root" is the shallowest open ancestor with children that has not
// yet been claimed. We drill past a node (recursing into its children to
// surface the actual next-step frontier) when:
// - it is in_progress or in_review — already claimed work; or
// - it is open but a descendant is already in_progress/in_review — work has
// begun inside the subtree even though the node itself was never started,
// so the node is effectively underway and is suppressed from its own
// listing; or
// - it is deferred with an unexpired defer date — the node is hidden from
// ready, but its open, unblocked children should still surface as the
// frontier rather than being collapsed away with the hidden parent.
func (s *Store) buildSubtreeOverlay() *subtreeOverlay {
overlay := &subtreeOverlay{
descendants: make(map[string]bool),
externalBlockers: make(map[string][]string),
}

now := s.Now()

// Load all non-closed issues
var allIDs []string
for _, status := range []string{"open", "in_progress", "in_review", "deferred"} {
Expand All @@ -473,23 +483,60 @@ func (s *Store) buildSubtreeOverlay() *subtreeOverlay {
}
}

// subtreeHasClaimedWork reports whether any descendant of id (not id
// itself) is in_progress or in_review.
var subtreeHasClaimedWork func(id string) bool
subtreeHasClaimedWork = func(id string) bool {
for _, c := range children[id] {
ci := issues[c]
if ci == nil {
continue
}
if ci.Status == "in_progress" || ci.Status == "in_review" {
return true
}
if subtreeHasClaimedWork(c) {
return true
}
}
return false
}

// Walk down from each top-level structural root to find display roots.
// Claimed nodes (in_progress/in_review) are not display roots themselves —
// we recurse into their children to surface the actual ready frontier.
var displayRoots []string
var walk func(id string)
walk = func(id string) {
iss := issues[id]
if iss == nil {
return
}
// Claimed work: drill past to surface the frontier beneath it.
if iss.Status == "in_progress" || iss.Status == "in_review" {
for _, c := range children[id] {
walk(c)
}
return
}
// Future-deferred node: hidden from ready, but its children should
// still surface. Drill past without listing it (the deferred loop in
// Ready will not re-add it while its deferral is unexpired).
if iss.Status == "deferred" && !IsDeferralExpired(iss.DeferUntil, now) {
for _, c := range children[id] {
walk(c)
}
return
}
if len(children[id]) > 0 {
// An open node with claimed work inside it is effectively
// underway. Drill past it to surface the live frontier, and
// suppress the node from its own listing.
if subtreeHasClaimedWork(id) {
overlay.descendants[id] = true
for _, c := range children[id] {
walk(c)
}
return
}
displayRoots = append(displayRoots, id)
}
}
Expand Down
144 changes: 144 additions & 0 deletions internal/issue/blocking_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1528,6 +1528,150 @@ func TestReadyStopsAtOpenIntermediateNode(t *testing.T) {
}
}

// When a child is started directly (without first starting the parent epic),
// the epic stays open but work has begun inside it. Ready() should drill past
// the open epic and surface the open sibling frontier — not collapse to just
// the epic and hide the actionable siblings.
func TestReadyDrillsPastOpenRootWithClaimedChild(t *testing.T) {
env := testutil.NewEnv(t)
defer env.Cleanup()

epic, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"})
childB, _ := env.Store.Create("Child B", issue.CreateOpts{Parent: epic.ID})
childC, _ := env.Store.Create("Child C", issue.CreateOpts{Parent: epic.ID})
env.CommitIntent("setup")

// Start child B directly; the epic is left open.
if _, err := env.Store.Start(childB.ID, ""); err != nil {
t.Fatalf("Start child: %v", err)
}
env.CommitIntent("start " + childB.ID)

ready, err := env.Store.Ready()
if err != nil {
t.Fatalf("Ready: %v", err)
}
ids := make(map[string]bool)
for _, r := range ready {
ids[r.ID] = true
}
if !ids[childC.ID] {
t.Errorf("open sibling C should surface as the frontier, got %v", ids)
}
if ids[epic.ID] {
t.Errorf("open epic should be suppressed (work claimed inside it), got %v", ids)
}
if ids[childB.ID] {
t.Errorf("in_progress child B should not appear in ready, got %v", ids)
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// When work is claimed two levels down (a grandchild started while both the
// epic and the intermediate node stay open), Ready() must still recognize the
// subtree as underway: suppress the open epic AND the open intermediate, and
// surface the open sibling as the frontier. This exercises the recursive
// descent in subtreeHasClaimedWork
func TestReadyDrillsPastOpenRootWithTransitivelyClaimedChild(t *testing.T) {
env := testutil.NewEnv(t)
defer env.Cleanup()
epic, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"})
mid, _ := env.Store.Create("Mid", issue.CreateOpts{Parent: epic.ID})
leaf, _ := env.Store.Create("Leaf", issue.CreateOpts{Parent: mid.ID})
sib, _ := env.Store.Create("Sib", issue.CreateOpts{Parent: epic.ID})
env.CommitIntent("setup")
// Start the grandchild directly; epic and mid are left open.
if _, err := env.Store.Start(leaf.ID, ""); err != nil {
t.Fatalf("Start leaf: %v", err)
}
env.CommitIntent("start " + leaf.ID)
ready, err := env.Store.Ready()
if err != nil {
t.Fatalf("Ready: %v", err)
}
ids := make(map[string]bool)
for _, r := range ready {
ids[r.ID] = true
}
if !ids[sib.ID] {
t.Errorf("open sibling should surface as the frontier, got %v", ids)
}
if ids[epic.ID] {
t.Errorf("open epic should be suppressed (transitively claimed work), got %v", ids)
}
if ids[mid.ID] {
t.Errorf("open intermediate node should be suppressed (claimed child below it), got %v", ids)
}
if ids[leaf.ID] {
t.Errorf("in_progress leaf should not appear in ready, got %v",
}
}

// An open epic with no claimed work inside it keeps the original behavior:
// the epic is the display root and its children stay hidden until it's started.
func TestReadyOpenRootNoClaimedWorkStillCollapses(t *testing.T) {
env := testutil.NewEnv(t)
defer env.Cleanup()

epic, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"})
childB, _ := env.Store.Create("Child B", issue.CreateOpts{Parent: epic.ID})
childC, _ := env.Store.Create("Child C", issue.CreateOpts{Parent: epic.ID})
env.CommitIntent("setup")

ready, err := env.Store.Ready()
if err != nil {
t.Fatalf("Ready: %v", err)
}
ids := make(map[string]bool)
for _, r := range ready {
ids[r.ID] = true
}
if !ids[epic.ID] {
t.Errorf("epic should be the display root, got %v", ids)
}
if ids[childB.ID] || ids[childC.ID] {
t.Errorf("children should stay hidden under an unstarted epic, got %v", ids)
}
}

// A future-deferred epic is hidden from ready, but its open, unblocked children
// should still surface as the frontier rather than being suppressed along with
// the hidden parent.
func TestReadyDeferredParentSurfacesOpenChildren(t *testing.T) {
env := testutil.NewEnv(t)
defer env.Cleanup()

epic, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"})
childE, _ := env.Store.Create("Child E", issue.CreateOpts{Parent: epic.ID})
childF, _ := env.Store.Create("Child F", issue.CreateOpts{Parent: epic.ID})
env.CommitIntent("setup")

// Defer the epic far into the future; children remain open and unblocked.
deferred := "deferred"
deferDate := "2099-01-01"
if _, err := env.Store.Update(epic.ID, issue.UpdateOpts{Status: &deferred, DeferUntil: &deferDate}); err != nil {
t.Fatalf("defer epic: %v", err)
}
env.CommitIntent("defer " + epic.ID)

ready, err := env.Store.Ready()
if err != nil {
t.Fatalf("Ready: %v", err)
}
ids := make(map[string]bool)
for _, r := range ready {
ids[r.ID] = true
}
if !ids[childE.ID] || !ids[childF.ID] {
t.Errorf("open children of a deferred epic should surface, got %v", ids)
}
if ids[epic.ID] {
t.Errorf("future-deferred epic should not appear in ready, got %v", ids)
}
}

// When work is claimed two levels down (a grandchild started while both the
// epic and the intermediate node stay open), Ready() must still recognize the
// subtree as underway: suppress the open epic AND the open intermediate, and
// surface the open sibling as the frontier. This exercises the recursive
// descent in subtreeHasClaimedWork.
func TestReadyDrillsPastOpenRootWithTransitivelyClaimedChild(t *testing.T) {
env := testutil.NewEnv(t)
defer env.Cleanup()

epic, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"})
mid, _ := env.Store.Create("Mid", issue.CreateOpts{Parent: epic.ID})
leaf, _ := env.Store.Create("Leaf", issue.CreateOpts{Parent: mid.ID})
sib, _ := env.Store.Create("Sib", issue.CreateOpts{Parent: epic.ID})
env.CommitIntent("setup")

// Start the grandchild directly; epic and mid are left open.
if _, err := env.Store.Start(leaf.ID, ""); err != nil {
t.Fatalf("Start leaf: %v", err)
}
env.CommitIntent("start " + leaf.ID)

ready, err := env.Store.Ready()
if err != nil {
t.Fatalf("Ready: %v", err)
}
ids := make(map[string]bool)
for _, r := range ready {
ids[r.ID] = true
}
if !ids[sib.ID] {
t.Errorf("open sibling should surface as the frontier, got %v", ids)
}
if ids[epic.ID] {
t.Errorf("open epic should be suppressed (transitively claimed work), got %v", ids)
}
if ids[mid.ID] {
t.Errorf("open intermediate node should be suppressed (claimed child below it), got %v", ids)
}
if ids[leaf.ID] {
t.Errorf("in_progress leaf should not appear in ready, got %v", ids)
}
}

// External blockers should bubble to the display root (which can be a deeper
// node than the top-level epic when the epic is in_progress).
func TestReadyExternalBlockerBubblesToDisplayRoot(t *testing.T) {
Expand Down
Loading