Skip to content

Commit 4509d01

Browse files
pierrespierres
committed
Add user feedback when package compare limit is exceeded
Previously, exceeding the compare limit silently truncated the list with no indication to the user. Now both /packages and /compare/packages show an info alert with a link to remove the excess packages. The + button is also hidden in search results when the limit is reached. The limit is raised from 10 to 15 (matching the number of chart colors) and defined as a shared constant in layout.MaxComparePackages.
1 parent 097b7ce commit 4509d01

7 files changed

Lines changed: 167 additions & 15 deletions

File tree

‎internal/ui/compare/handler.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,6 @@ import (
1111
"pkgstatsd/internal/web"
1212
)
1313

14-
const maxPackages = 10
15-
1614
type Handler struct {
1715
repo packages.Repository
1816
manifest *layout.Manifest
@@ -30,8 +28,10 @@ func (h *Handler) HandleCompare(w http.ResponseWriter, r *http.Request) {
3028
}
3129

3230
names := strings.Split(namesParam, ",")
33-
if len(names) > maxPackages {
34-
names = names[:maxPackages]
31+
var excessNames []string
32+
if len(names) > layout.MaxComparePackages {
33+
excessNames = names[layout.MaxComparePackages:]
34+
names = names[:layout.MaxComparePackages]
3535
}
3636

3737
var allSeries []packages.PackagePopularity
@@ -64,7 +64,7 @@ func (h *Handler) HandleCompare(w http.ResponseWriter, r *http.Request) {
6464

6565
layout.Render(w, r,
6666
layout.Page{Title: "Compare packages", Description: "Compare the popularity of Arch Linux packages side by side.", Path: "/packages", Manifest: h.manifest, CanonicalPath: "/compare/packages/" + strings.Join(escapedNames, ",")},
67-
CompareContent(names, data),
67+
CompareContent(names, excessNames, data),
6868
)
6969
}
7070

‎internal/ui/compare/handler_test.go‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,59 @@ func TestHandleCompare_SeriesError(t *testing.T) {
7878
}
7979
}
8080

81+
func TestHandleCompare_ExceedsLimit(t *testing.T) {
82+
manifest, _ := layout.NewManifest([]byte(`{}`))
83+
repo := &mockRepo{
84+
findSeriesByNameFunc: func(ctx context.Context, name string, _, _, _, _ int) (*packages.PackagePopularityList, error) {
85+
return &packages.PackagePopularityList{
86+
Total: 1,
87+
PackagePopularities: []packages.PackagePopularity{
88+
{Name: name, StartMonth: 202501, EndMonth: 202501, Popularity: 10.5},
89+
},
90+
}, nil
91+
},
92+
}
93+
handler := NewHandler(repo, manifest)
94+
95+
// Build a list of more than MaxComparePackages
96+
names := make([]string, layout.MaxComparePackages+2)
97+
for i := range names {
98+
names[i] = "pkg" + strings.Repeat("x", i)
99+
}
100+
namesParam := strings.Join(names, ",")
101+
102+
req := httptest.NewRequest(http.MethodGet, "/compare/packages/"+namesParam, nil)
103+
req.SetPathValue("names", namesParam)
104+
rr := httptest.NewRecorder()
105+
106+
handler.HandleCompare(rr, req)
107+
108+
if rr.Code != http.StatusOK {
109+
t.Errorf("expected status 200, got %d", rr.Code)
110+
}
111+
112+
body := rr.Body.String()
113+
114+
// Should show the limit warning
115+
if !strings.Contains(body, "You can only compare up to") {
116+
t.Error("expected body to contain limit warning")
117+
}
118+
119+
// Should show the excess package names with a remove link
120+
excessNames := names[layout.MaxComparePackages:]
121+
for _, name := range excessNames {
122+
if !strings.Contains(body, name) {
123+
t.Errorf("expected body to contain excess package name %q", name)
124+
}
125+
}
126+
127+
// The remove link should point to a URL with only the first MaxComparePackages
128+
trimmedNames := strings.Join(names[:layout.MaxComparePackages], ",")
129+
if !strings.Contains(body, "/compare/packages/"+trimmedNames) {
130+
t.Error("expected body to contain remove link with trimmed package list")
131+
}
132+
}
133+
81134
func TestHandleLegacyCompare(t *testing.T) {
82135
manifest, _ := layout.NewManifest([]byte(`{}`))
83136
handler := NewHandler(nil, manifest)

‎internal/ui/compare/templates.templ‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,23 @@
11
package compare
22

33
import (
4+
"fmt"
45
"pkgstatsd/internal/chartdata"
6+
"pkgstatsd/internal/ui/layout"
57
"strings"
68
)
79

8-
templ CompareContent(names []string, data chartdata.Data) {
10+
templ CompareContent(names []string, excessNames []string, data chartdata.Data) {
911
<h1 class="mb-3">Compare packages</h1>
1012
<p class="mb-3">Comparing: <strong>{ strings.Join(names, ", ") }</strong></p>
13+
if len(excessNames) > 0 {
14+
<div role="alert" class="alert alert-info mt-3">
15+
{ fmt.Sprintf("You can only compare up to %d packages. Only the first %d are shown.", layout.MaxComparePackages, layout.MaxComparePackages) }
16+
<a class="alert-link" href={ templ.SafeURL("/compare/packages/" + strings.Join(names, ",")) }>
17+
Remove { strings.Join(excessNames, ", ") } from list
18+
</a>
19+
</div>
20+
}
1121
<popularity-chart>
1222
@templ.JSONScript("", data)
1323
</popularity-chart>

‎internal/ui/layout/constants.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
11
package layout
22

3-
const SeriesLimit = 10000
3+
const (
4+
SeriesLimit = 10000
5+
MaxComparePackages = 15
6+
)

‎internal/ui/packagepage/handler.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import (
1414
const (
1515
defaultLimit = 25
1616
maxLimit = 250
17-
maxCompare = 10
1817
)
1918

2019
type Handler struct {
@@ -62,8 +61,8 @@ func (h *Handler) fetchComparePackages(r *http.Request, compare string, currentM
6261
}
6362

6463
names := strings.Split(compare, ",")
65-
if len(names) > maxCompare {
66-
names = names[:maxCompare]
64+
if len(names) > layout.MaxComparePackages {
65+
names = names[:layout.MaxComparePackages]
6766
}
6867

6968
var result []packages.PackagePopularity

‎internal/ui/packagepage/handler_test.go‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,63 @@ func TestHandlePackages_WithCompare(t *testing.T) {
8888
}
8989
}
9090

91+
func TestHandlePackages_CompareExceedsLimit(t *testing.T) {
92+
manifest, _ := layout.NewManifest([]byte(`{}`))
93+
94+
// Build a list of more than MaxComparePackages
95+
names := make([]string, layout.MaxComparePackages+2)
96+
for i := range names {
97+
names[i] = "pkg" + strings.Repeat("x", i)
98+
}
99+
100+
repo := &mockRepo{
101+
findAllFunc: func(_ context.Context, _ string, _, _, _, _ int) (*packages.PackagePopularityList, error) {
102+
return &packages.PackagePopularityList{
103+
Total: 1,
104+
PackagePopularities: []packages.PackagePopularity{
105+
{Name: "newpkg", Popularity: 1.0},
106+
},
107+
}, nil
108+
},
109+
}
110+
handler := NewHandler(repo, manifest)
111+
112+
req := httptest.NewRequest(http.MethodGet, "/packages?query=pkg&compare="+strings.Join(names, ","), nil)
113+
rr := httptest.NewRecorder()
114+
115+
handler.HandlePackages(rr, req)
116+
117+
if rr.Code != http.StatusOK {
118+
t.Errorf("expected status 200, got %d", rr.Code)
119+
}
120+
121+
body := rr.Body.String()
122+
123+
// Should show the limit warning
124+
if !strings.Contains(body, "You can only compare up to") {
125+
t.Error("expected body to contain limit warning")
126+
}
127+
128+
// Should show excess package names with a remove link
129+
excessNames := names[layout.MaxComparePackages:]
130+
for _, name := range excessNames {
131+
if !strings.Contains(body, name) {
132+
t.Errorf("expected body to contain excess package name %q", name)
133+
}
134+
}
135+
136+
// The compare table should have individual remove links (title="Remove X") only for the first MaxComparePackages
137+
beyondLimit := names[layout.MaxComparePackages]
138+
if strings.Contains(body, "title=\"Remove "+beyondLimit+"\"") {
139+
t.Error("expected compare table NOT to contain packages beyond the limit as removable entries")
140+
}
141+
142+
// The "newpkg" row should NOT have a + link since we're at the limit
143+
if strings.Contains(body, "Add newpkg") {
144+
t.Error("expected + link to be hidden when at compare limit")
145+
}
146+
}
147+
91148
func TestHandlePackages_CompareError(t *testing.T) {
92149
manifest, _ := layout.NewManifest([]byte(`{}`))
93150
repo := &mockRepo{

‎internal/ui/packagepage/templates.templ‎

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"fmt"
55
"pkgstatsd/internal/packages"
66
"pkgstatsd/internal/ui/components"
7+
"pkgstatsd/internal/ui/layout"
78
"strings"
89
)
910

@@ -14,17 +15,20 @@ templ PackagesContent(list *packages.PackagePopularityList, query string, curren
1415
if len(selectedPackages) > 0 {
1516
<div class="mb-2">
1617
@CompareTable(selectedPackages, query, currentOffset, limit, compare)
17-
if len(selectedPackages) > 1 && len(selectedPackages) <= 10 {
18+
if len(selectedPackages) > 1 {
1819
<a
1920
class="d-inline-flex btn btn-primary"
2021
href={ templ.SafeURL("/compare/packages/" + compareNames(selectedPackages)) }
2122
>
2223
Compare
2324
</a>
2425
}
25-
if len(selectedPackages) > 10 {
26-
<div role="alert" class="alert alert-info mb-4">
27-
You can only compare up to 10 packages.
26+
if compareCount(compare) > layout.MaxComparePackages {
27+
<div role="alert" class="alert alert-info mt-3 mb-4">
28+
{ fmt.Sprintf("You can only compare up to %d packages. Only the first %d are shown.", layout.MaxComparePackages, layout.MaxComparePackages) }
29+
<a class="alert-link" href={ trimCompareURL(query, currentOffset, limit, compare) }>
30+
Remove { excessNames(compare) } from list
31+
</a>
2832
</div>
2933
}
3034
</div>
@@ -126,7 +130,7 @@ templ PackageTable(pkgs []packages.PackagePopularity, query string, offset int,
126130
>
127131
@templ.Raw(iconTrash)
128132
</a>
129-
} else {
133+
} else if compareCount(compare) < layout.MaxComparePackages {
130134
<a
131135
class="text-primary"
132136
href={ toggleURL(query, offset, limit, compare, pkg.Name) }
@@ -243,3 +247,29 @@ func compareNames(pkgs []packages.PackagePopularity) string {
243247
}
244248
return strings.Join(names, ",")
245249
}
250+
251+
func compareCount(compare string) int {
252+
if compare == "" {
253+
return 0
254+
}
255+
return len(strings.Split(compare, ","))
256+
}
257+
258+
func excessNames(compare string) string {
259+
names := strings.Split(compare, ",")
260+
if len(names) <= layout.MaxComparePackages {
261+
return ""
262+
}
263+
return strings.Join(names[layout.MaxComparePackages:], ", ")
264+
}
265+
266+
func trimCompareURL(query string, offset, limit int, compare string) templ.SafeURL {
267+
names := strings.Split(compare, ",")
268+
trimmed := strings.Join(names[:layout.MaxComparePackages], ",")
269+
url := fmt.Sprintf("/packages?limit=%d&offset=%d", limit, offset)
270+
if query != "" {
271+
url += "&query=" + query
272+
}
273+
url += "&compare=" + trimmed
274+
return templ.SafeURL(url)
275+
}

0 commit comments

Comments
 (0)