Skip to content

Commit a588e26

Browse files
Thomas StrombergThomas Stromberg
Thomas Stromberg
authored and
Thomas Stromberg
committed
sort blockers first
1 parent 1c28330 commit a588e26

File tree

2 files changed

+130
-13
lines changed

2 files changed

+130
-13
lines changed

‎pkg/home/ui.go‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -110,10 +110,15 @@ func BuildPRSections(incoming, outgoing []PR) []slack.Block {
110110

111111
// Incoming PRs section
112112
if len(incoming) > 0 {
113-
// Sort by most recent first
113+
// Sort: blocked first, then by most recent within each group
114114
prs := make([]PR, len(incoming))
115115
copy(prs, incoming)
116116
sort.Slice(prs, func(i, j int) bool {
117+
iBlocked := prs[i].IsBlocked || prs[i].NeedsReview
118+
jBlocked := prs[j].IsBlocked || prs[j].NeedsReview
119+
if iBlocked != jBlocked {
120+
return iBlocked // blocked items first
121+
}
117122
return prs[i].UpdatedAt.After(prs[j].UpdatedAt)
118123
})
119124

@@ -135,13 +140,13 @@ func BuildPRSections(incoming, outgoing []PR) []slack.Block {
135140
var indicator string
136141
switch {
137142
case prs[i].NeedsReview:
138-
indicator = ":green_square:"
143+
indicator = ":large_red_square:"
139144
case prs[i].IsBlocked:
140145
indicator = ":large_red_square:"
141146
case prs[i].ActionKind != "":
142147
indicator = ":speech_balloon:"
143148
default:
144-
indicator = "•"
149+
indicator = ":white_small_square:"
145150
}
146151

147152
// Build line
@@ -172,10 +177,15 @@ func BuildPRSections(incoming, outgoing []PR) []slack.Block {
172177

173178
// Outgoing PRs section
174179
if len(outgoing) > 0 {
175-
// Sort by most recent first
180+
// Sort: blocked first, then by most recent within each group
176181
prs := make([]PR, len(outgoing))
177182
copy(prs, outgoing)
178183
sort.Slice(prs, func(i, j int) bool {
184+
iBlocked := prs[i].IsBlocked
185+
jBlocked := prs[j].IsBlocked
186+
if iBlocked != jBlocked {
187+
return iBlocked // blocked items first
188+
}
179189
return prs[i].UpdatedAt.After(prs[j].UpdatedAt)
180190
})
181191

@@ -197,13 +207,13 @@ func BuildPRSections(incoming, outgoing []PR) []slack.Block {
197207
var indicator string
198208
switch {
199209
case prs[i].NeedsReview:
200-
indicator = ":green_square:"
210+
indicator = ":large_green_square:"
201211
case prs[i].IsBlocked:
202-
indicator = ":large_red_square:"
212+
indicator = ":large_green_square:"
203213
case prs[i].ActionKind != "":
204214
indicator = ":speech_balloon:"
205215
default:
206-
indicator = "•"
216+
indicator = ":white_small_square:"
207217
}
208218

209219
// Build line

‎pkg/home/ui_test.go‎

Lines changed: 113 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -139,17 +139,17 @@ func TestBuildBlocks(t *testing.T) {
139139
t.Error("expected 'blocked on you' message in header")
140140
}
141141

142-
// Should have PR with green square (incoming blocked indicator)
142+
// Should have PR with large red square (incoming blocked indicator)
143143
foundBlockedPR := false
144144
for _, block := range blocks {
145145
if sb, ok := block.(*slack.SectionBlock); ok {
146-
if sb.Text != nil && strings.Contains(sb.Text.Text, ":green_square:") {
146+
if sb.Text != nil && strings.Contains(sb.Text.Text, ":large_red_square:") {
147147
foundBlockedPR = true
148148
}
149149
}
150150
}
151151
if !foundBlockedPR {
152-
t.Error("expected PR with :green_square: indicating blocked incoming PR")
152+
t.Error("expected PR with :large_red_square: indicating blocked incoming PR")
153153
}
154154
},
155155
},
@@ -191,17 +191,17 @@ func TestBuildBlocks(t *testing.T) {
191191
t.Error("expected 'Outgoing' section")
192192
}
193193

194-
// Should have PR with red square (outgoing blocked indicator)
194+
// Should have PR with large green square (outgoing blocked indicator)
195195
foundBlocked := false
196196
for _, block := range blocks {
197197
if sb, ok := block.(*slack.SectionBlock); ok {
198-
if sb.Text != nil && strings.Contains(sb.Text.Text, ":large_red_square:") {
198+
if sb.Text != nil && strings.Contains(sb.Text.Text, ":large_green_square:") {
199199
foundBlocked = true
200200
}
201201
}
202202
}
203203
if !foundBlocked {
204-
t.Error("expected PR with :large_red_square: for blocked outgoing PR")
204+
t.Error("expected PR with :large_green_square: for blocked outgoing PR")
205205
}
206206
},
207207
},
@@ -310,3 +310,110 @@ func TestBuildBlocks_DividersBetweenSections(t *testing.T) {
310310
t.Errorf("expected at least 2 dividers, got %d", dividerCount)
311311
}
312312
}
313+
314+
// TestBuildPRSections_SortOrder verifies blocked PRs appear first, then by recency.
315+
func TestBuildPRSections_SortOrder(t *testing.T) {
316+
baseTime := time.Now()
317+
318+
// Create incoming PRs with mixed blocked/unblocked and timestamps
319+
incoming := []PR{
320+
{Number: 1, Title: "Oldest non-blocked", Repository: "org/repo", URL: "https://github.com/org/repo/pull/1", UpdatedAt: baseTime.Add(-4 * time.Hour), NeedsReview: false},
321+
{Number: 2, Title: "Newest blocked", Repository: "org/repo", URL: "https://github.com/org/repo/pull/2", UpdatedAt: baseTime.Add(-1 * time.Hour), NeedsReview: true},
322+
{Number: 3, Title: "Middle non-blocked", Repository: "org/repo", URL: "https://github.com/org/repo/pull/3", UpdatedAt: baseTime.Add(-2 * time.Hour), NeedsReview: false},
323+
{Number: 4, Title: "Oldest blocked", Repository: "org/repo", URL: "https://github.com/org/repo/pull/4", UpdatedAt: baseTime.Add(-5 * time.Hour), NeedsReview: true},
324+
}
325+
326+
// Create outgoing PRs with mixed blocked/unblocked and timestamps
327+
outgoing := []PR{
328+
{Number: 5, Title: "Middle non-blocked out", Repository: "org/repo", URL: "https://github.com/org/repo/pull/5", UpdatedAt: baseTime.Add(-3 * time.Hour), IsBlocked: false},
329+
{Number: 6, Title: "Newest blocked out", Repository: "org/repo", URL: "https://github.com/org/repo/pull/6", UpdatedAt: baseTime.Add(-1 * time.Hour), IsBlocked: true},
330+
{Number: 7, Title: "Oldest blocked out", Repository: "org/repo", URL: "https://github.com/org/repo/pull/7", UpdatedAt: baseTime.Add(-6 * time.Hour), IsBlocked: true},
331+
{Number: 8, Title: "Newest non-blocked out", Repository: "org/repo", URL: "https://github.com/org/repo/pull/8", UpdatedAt: baseTime.Add(-2 * time.Hour), IsBlocked: false},
332+
}
333+
334+
blocks := BuildPRSections(incoming, outgoing)
335+
336+
// Should have 2 blocks (incoming and outgoing sections)
337+
if len(blocks) != 2 {
338+
t.Fatalf("expected 2 blocks, got %d", len(blocks))
339+
}
340+
341+
// Check incoming section order
342+
incomingBlock, ok := blocks[0].(*slack.SectionBlock)
343+
if !ok {
344+
t.Fatal("expected first block to be SectionBlock")
345+
}
346+
incomingText := incomingBlock.Text.Text
347+
348+
// Verify blocked PRs appear before non-blocked
349+
// Expected order: PR#2 (newest blocked), PR#4 (oldest blocked), PR#3 (middle non-blocked), PR#1 (oldest non-blocked)
350+
idx2 := strings.Index(incomingText, "repo#2")
351+
idx4 := strings.Index(incomingText, "repo#4")
352+
idx3 := strings.Index(incomingText, "repo#3")
353+
idx1 := strings.Index(incomingText, "repo#1")
354+
355+
if idx2 < 0 || idx4 < 0 || idx3 < 0 || idx1 < 0 {
356+
t.Fatal("not all incoming PRs found in output")
357+
}
358+
359+
// Blocked PRs (2, 4) should come before non-blocked PRs (3, 1)
360+
if idx2 > idx3 || idx2 > idx1 {
361+
t.Error("blocked PR#2 should appear before non-blocked PRs")
362+
}
363+
if idx4 > idx3 || idx4 > idx1 {
364+
t.Error("blocked PR#4 should appear before non-blocked PRs")
365+
}
366+
367+
// Within blocked group: PR#2 (newer) should come before PR#4 (older)
368+
if idx2 > idx4 {
369+
t.Error("newer blocked PR#2 should appear before older blocked PR#4")
370+
}
371+
372+
// Within non-blocked group: PR#3 (newer) should come before PR#1 (older)
373+
if idx3 > idx1 {
374+
t.Error("newer non-blocked PR#3 should appear before older non-blocked PR#1")
375+
}
376+
377+
// Check outgoing section order
378+
outgoingBlock, ok := blocks[1].(*slack.SectionBlock)
379+
if !ok {
380+
t.Fatal("expected second block to be SectionBlock")
381+
}
382+
outgoingText := outgoingBlock.Text.Text
383+
384+
// Expected order: PR#6 (newest blocked), PR#7 (oldest blocked), PR#8 (newest non-blocked), PR#5 (middle non-blocked)
385+
idx6 := strings.Index(outgoingText, "repo#6")
386+
idx7 := strings.Index(outgoingText, "repo#7")
387+
idx8 := strings.Index(outgoingText, "repo#8")
388+
idx5 := strings.Index(outgoingText, "repo#5")
389+
390+
if idx6 < 0 || idx7 < 0 || idx8 < 0 || idx5 < 0 {
391+
t.Fatal("not all outgoing PRs found in output")
392+
}
393+
394+
// Blocked PRs (6, 7) should come before non-blocked PRs (8, 5)
395+
if idx6 > idx8 || idx6 > idx5 {
396+
t.Error("blocked PR#6 should appear before non-blocked PRs")
397+
}
398+
if idx7 > idx8 || idx7 > idx5 {
399+
t.Error("blocked PR#7 should appear before non-blocked PRs")
400+
}
401+
402+
// Within blocked group: PR#6 (newer) should come before PR#7 (older)
403+
if idx6 > idx7 {
404+
t.Error("newer blocked PR#6 should appear before older blocked PR#7")
405+
}
406+
407+
// Within non-blocked group: PR#8 (newer) should come before PR#5 (older)
408+
if idx8 > idx5 {
409+
t.Error("newer non-blocked PR#8 should appear before older non-blocked PR#5")
410+
}
411+
412+
// Verify color indicators
413+
if !strings.Contains(incomingText, ":large_red_square:") {
414+
t.Error("incoming blocked PRs should use :large_red_square:")
415+
}
416+
if !strings.Contains(outgoingText, ":large_green_square:") {
417+
t.Error("outgoing blocked PRs should use :large_green_square:")
418+
}
419+
}

0 commit comments

Comments
 (0)