From ffec4aade9db5c80be41d68d15ca7bbdfe47f4ad Mon Sep 17 00:00:00 2001 From: Henry Date: Wed, 2 Sep 2026 16:55:31 +0000 Subject: [PATCH] feat(gitlab): filter issues by milestone (#3278) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 Co-authored-by: Carlos Florencio --- apps/backend/internal/gitlab/client.go | 8 +- .../backend/internal/gitlab/client_helpers.go | 73 +- .../gitlab/client_helpers_milestone_test.go | 228 +++ apps/backend/internal/gitlab/controller.go | 22 +- .../internal/gitlab/controller_test.go | 62 + apps/backend/internal/gitlab/mock_client.go | 15 - .../internal/gitlab/mock_client_issues.go | 109 ++ .../gitlab/mock_client_issues_test.go | 334 ++++ apps/backend/internal/gitlab/models.go | 1 + apps/backend/internal/gitlab/noop_client.go | 4 +- apps/backend/internal/gitlab/pat_client.go | 8 +- .../gitlab/pat_client_search_actions_test.go | 2 +- .../internal/gitlab/pat_client_test.go | 7 +- .../gitlab/service_issue_query_test.go | 2 +- .../internal/gitlab/service_issue_watches.go | 2 +- .../internal/gitlab/service_mentions.go | 2 +- .../internal/gitlab/service_mentions_test.go | 2 +- .../gitlab/service_merge_search_test.go | 4 +- .../backend/internal/gitlab/service_search.go | 10 +- .../internal/gitlab/service_watches.go | 14 +- .../internal/gitlab/service_watches_test.go | 8 + apps/web/app/gitlab/gitlab-page-client.tsx | 169 +-- .../app/gitlab/use-gitlab-page-state.test.ts | 424 ++++++ apps/web/app/gitlab/use-gitlab-page-state.ts | 421 ++++++ .../gitlab/my-gitlab/issue-list.test.tsx | 91 ++ .../gitlab/my-gitlab/issue-list.tsx | 14 + .../gitlab/my-gitlab/list-toolbar.test.tsx | 124 ++ .../gitlab/my-gitlab/list-toolbar.tsx | 111 +- .../my-gitlab/quick-task-launcher.test.tsx | 1 + .../my-gitlab/use-gitlab-search.test.ts | 68 + .../gitlab/my-gitlab/use-gitlab-search.ts | 71 +- .../my-gitlab/use-saved-presets.test.ts | 98 ++ .../gitlab/my-gitlab/use-saved-presets.ts | 33 +- .../gitlab-issue-milestone-filter-helpers.ts | 30 + .../gitlab-issue-milestone-filter.spec.ts | 210 +++ ...bile-gitlab-issue-milestone-filter.spec.ts | 45 + apps/web/lib/api/domains/gitlab-api.test.ts | 25 + apps/web/lib/api/domains/gitlab-api.ts | 2 + apps/web/lib/types/gitlab.ts | 1 + apps/web/src/locales/en/gitlab.json | 2 + apps/web/src/locales/pseudo/gitlab.json | 2 + apps/web/src/locales/pt-pt/gitlab.json | 2 + apps/web/src/locales/zh-cn/gitlab.json | 2 + apps/web/src/locales/zh-hk/gitlab.json | 2 + apps/web/src/locales/zh-tw/gitlab.json | 4 +- .../gitlab-issue-milestone-filter/spec.md | 1347 +++++++++++++++++ docs/specs/spec-lint-exceptions.tsv | 1 + 47 files changed, 3945 insertions(+), 272 deletions(-) create mode 100644 apps/backend/internal/gitlab/client_helpers_milestone_test.go create mode 100644 apps/backend/internal/gitlab/mock_client_issues.go create mode 100644 apps/backend/internal/gitlab/mock_client_issues_test.go create mode 100644 apps/web/app/gitlab/use-gitlab-page-state.test.ts create mode 100644 apps/web/app/gitlab/use-gitlab-page-state.ts create mode 100644 apps/web/components/gitlab/my-gitlab/issue-list.test.tsx create mode 100644 apps/web/components/gitlab/my-gitlab/list-toolbar.test.tsx create mode 100644 apps/web/e2e/tests/gitlab/gitlab-issue-milestone-filter-helpers.ts create mode 100644 apps/web/e2e/tests/gitlab/gitlab-issue-milestone-filter.spec.ts create mode 100644 apps/web/e2e/tests/gitlab/mobile-gitlab-issue-milestone-filter.spec.ts create mode 100644 docs/specs/gitlab-issue-milestone-filter/spec.md diff --git a/apps/backend/internal/gitlab/client.go b/apps/backend/internal/gitlab/client.go index fd9b7cbd1f..9ec1e10253 100644 --- a/apps/backend/internal/gitlab/client.go +++ b/apps/backend/internal/gitlab/client.go @@ -106,8 +106,10 @@ type Client interface { // ListIssues searches for open issues. filter is an optional // additional API filter; customQuery, when non-empty, replaces the - // entire generated query. - ListIssues(ctx context.Context, filter, customQuery string) ([]*Issue, error) + // entire generated query. milestone, when non-empty, restricts results + // to that exact milestone title (folded into customQuery when both are + // set and customQuery doesn't already name a milestone). + ListIssues(ctx context.Context, filter, customQuery, milestone string) ([]*Issue, error) // SearchMRs searches for MRs matching the given query. SearchMRs(ctx context.Context, filter, customQuery string) ([]*MR, error) @@ -117,7 +119,7 @@ type Client interface { SearchMRsPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*MRSearchPage, error) // ListIssuesPaged is the paginated variant of ListIssues. - ListIssuesPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*IssueSearchPage, error) + ListIssuesPaged(ctx context.Context, filter, customQuery, milestone string, page, perPage int) (*IssueSearchPage, error) // GetIssueState returns the state of a single issue ("opened" or "closed"). GetIssueState(ctx context.Context, projectPath string, iid int) (string, error) diff --git a/apps/backend/internal/gitlab/client_helpers.go b/apps/backend/internal/gitlab/client_helpers.go index 7fb994abdd..d6577d110f 100644 --- a/apps/backend/internal/gitlab/client_helpers.go +++ b/apps/backend/internal/gitlab/client_helpers.go @@ -70,16 +70,17 @@ type rawProject struct { } type rawIssue struct { - ID int64 `json:"id"` - IID int `json:"iid"` - ProjectID int64 `json:"project_id"` - Title string `json:"title"` - Description string `json:"description"` - State string `json:"state"` - WebURL string `json:"web_url"` - Author rawUser `json:"author"` - Labels []string `json:"labels"` - Assignees []rawUser `json:"assignees"` + ID int64 `json:"id"` + IID int `json:"iid"` + ProjectID int64 `json:"project_id"` + Title string `json:"title"` + Description string `json:"description"` + State string `json:"state"` + WebURL string `json:"web_url"` + Author rawUser `json:"author"` + Labels []string `json:"labels"` + Assignees []rawUser `json:"assignees"` + Milestone *rawMilestone `json:"milestone"` References struct { Full string `json:"full"` } `json:"references"` @@ -88,6 +89,12 @@ type rawIssue struct { ClosedAt *time.Time `json:"closed_at"` } +// rawMilestone is GitLab's milestone sub-object on an issue. GitLab returns +// milestone: null for an issue with no milestone. +type rawMilestone struct { + Title string `json:"title"` +} + type rawDiscussion struct { ID string `json:"id"` IndividualNote bool `json:"individual_note"` @@ -226,6 +233,10 @@ func convertRawIssue(raw *rawIssue) *Issue { assignees = append(assignees, a.Username) } } + milestone := "" + if raw.Milestone != nil { + milestone = raw.Milestone.Title + } return &Issue{ ID: raw.ID, IID: raw.IID, @@ -240,6 +251,7 @@ func convertRawIssue(raw *rawIssue) *Issue { ProjectPath: projectPath, Labels: append([]string(nil), raw.Labels...), Assignees: assignees, + Milestone: milestone, CreatedAt: raw.CreatedAt, UpdatedAt: raw.UpdatedAt, ClosedAt: raw.ClosedAt, @@ -485,9 +497,12 @@ func buildMRSearchQuery(filter, customQuery string) string { return values.Encode() } -func buildIssueSearchQuery(filter, customQuery string) string { +func buildIssueSearchQuery(filter, customQuery, milestone string) string { if customQuery != "" { - return customQuery + if milestone == "" { + return customQuery + } + return foldMilestoneIntoQuery(customQuery, milestone) } values := url.Values{} values.Set("state", gitlabStateOpened) @@ -495,9 +510,43 @@ func buildIssueSearchQuery(filter, customQuery string) string { if filter != "" { appendFilter(values, filter) } + if milestone != "" { + values.Set("milestone", milestone) + } return values.Encode() } +// appendQueryParam adds an escaped key/value pair to a raw query unless that +// key already exists. URL fragments are kept at the end, outside the query. +// This also preserves malformed-query behavior for callers that do not validate +// their input at the HTTP boundary. +func appendQueryParam(customQuery, key, value string) string { + if parsed, err := url.ParseQuery(customQuery); err == nil && parsed.Has(key) { + return customQuery + } + fragment := "" + if index := strings.IndexByte(customQuery, '#'); index >= 0 { + fragment = customQuery[index:] + customQuery = customQuery[:index] + } + encoded := url.QueryEscape(value) + if customQuery == "" { + return key + "=" + encoded + fragment + } + return customQuery + "&" + key + "=" + encoded + fragment +} + +// foldMilestoneIntoQuery merges a milestone into an existing, non-empty +// customQuery string, mirroring appendLabelsToQuery's precedent: a custom +// query that already names the `milestone` key wins (even if the value is +// empty), and an unparseable custom query still gets the milestone appended +// rather than silently dropping the user's filter selection. The only caller, +// buildIssueSearchQuery, guards both arguments non-empty before calling this, +// so there is no empty-customQuery case to handle here. +func foldMilestoneIntoQuery(customQuery, milestone string) string { + return appendQueryParam(customQuery, "milestone", milestone) +} + // filterTokenReviewRequested is the /gitlab page tab value that maps to // GitLab's reviewer_username param. Defined as a constant because it appears // in multiple spots across the package (translator, MR controller, issues diff --git a/apps/backend/internal/gitlab/client_helpers_milestone_test.go b/apps/backend/internal/gitlab/client_helpers_milestone_test.go new file mode 100644 index 0000000000..6879f808f1 --- /dev/null +++ b/apps/backend/internal/gitlab/client_helpers_milestone_test.go @@ -0,0 +1,228 @@ +package gitlab + +import ( + "net/url" + "testing" +) + +func TestBuildIssueSearchQuery_PresetPathComposesMilestoneAfterFilter(t *testing.T) { + got := buildIssueSearchQuery("scope=assigned_to_me", "", "Next") + values, err := url.ParseQuery(got) + if err != nil { + t.Fatalf("ParseQuery(%q): %v", got, err) + } + if got := values.Get("scope"); got != "assigned_to_me" { + t.Errorf("scope = %q, want assigned_to_me (milestone must not clobber the preset)", got) + } + if got := values.Get("state"); got != "opened" { + t.Errorf("state = %q, want opened", got) + } + if got := values.Get("milestone"); got != "Next" { + t.Errorf("milestone = %q, want Next", got) + } +} + +func TestBuildIssueSearchQuery_EmptyMilestoneIsByteIdenticalToBeforeFeature(t *testing.T) { + got := buildIssueSearchQuery("scope=assigned_to_me", "", "") + want := buildIssueSearchQueryWithoutMilestoneForTest("scope=assigned_to_me", "") + if got != want { + t.Errorf("query = %q, want %q (empty milestone must add nothing)", got, want) + } +} + +// buildIssueSearchQueryWithoutMilestoneForTest reproduces the pre-feature +// query builder so the byte-identical claim can be checked without a second +// production code path. +func buildIssueSearchQueryWithoutMilestoneForTest(filter, customQuery string) string { + if customQuery != "" { + return customQuery + } + values := url.Values{} + values.Set("state", gitlabStateOpened) + values.Set("scope", "all") + if filter != "" { + appendFilter(values, filter) + } + return values.Encode() +} + +func TestBuildIssueSearchQuery_CustomQueryWithMilestoneWins(t *testing.T) { + got := buildIssueSearchQuery("", "state=closed&milestone=Old", "Next") + if got != "state=closed&milestone=Old" { + t.Errorf("query = %q, want custom query unchanged (custom milestone wins)", got) + } +} + +// Scenario 31, live-path clause: a custom query carrying a repeated +// milestone key is forwarded upstream unchanged, both pairs included — the +// live path never resolves which one wins, only the mock does (Get, +// first-value). +func TestBuildIssueSearchQuery_CustomQueryWithRepeatedMilestoneKeyForwardedUnchanged(t *testing.T) { + got := buildIssueSearchQuery("", "milestone=Old&milestone=Next", "Next") + want := "milestone=Old&milestone=Next" + if got != want { + t.Errorf("query = %q, want %q (both pairs forwarded verbatim)", got, want) + } +} + +func TestBuildIssueSearchQuery_CustomQueryWithoutMilestoneIsExtended(t *testing.T) { + got := buildIssueSearchQuery("", "state=closed", "Next") + values, err := url.ParseQuery(got) + if err != nil { + t.Fatalf("ParseQuery(%q): %v", got, err) + } + if got := values.Get("state"); got != "closed" { + t.Errorf("state = %q, want closed", got) + } + if got := values.Get("milestone"); got != "Next" { + t.Errorf("milestone = %q, want Next", got) + } + if values.Has("scope") { + t.Errorf("scope present in %q, want absent (custom query defaults are not reintroduced)", got) + } +} + +func TestBuildIssueSearchQuery_CustomQueryFragmentKeepsMilestoneInQuery(t *testing.T) { + got := buildIssueSearchQuery("", "state=closed#client-fragment", "Next") + want := "state=closed&milestone=Next#client-fragment" + if got != want { + t.Errorf("query = %q, want %q (milestone must precede the URL fragment)", got, want) + } +} + +// The HTTP controller rejects malformed custom queries before this helper is +// called. Keep this builder-level case to pin its defensive append behavior +// for other callers that construct the query directly. +func TestBuildIssueSearchQuery_MalformedCustomQueryAppendsMilestone(t *testing.T) { + got := buildIssueSearchQuery("", "%zz", "Next") + want := "%zz&milestone=Next" + if got != want { + t.Errorf("query = %q, want %q", got, want) + } +} + +func TestBuildIssueSearchQuery_EmptyMilestoneKeyInCustomQueryStillWins(t *testing.T) { + got := buildIssueSearchQuery("", "state=closed&milestone=", "Next") + if got != "state=closed&milestone=" { + t.Errorf("query = %q, want custom query unchanged (empty milestone key still wins)", got) + } +} + +func TestBuildIssueSearchQuery_MalformedFilterCannotTakeMilestoneDownWithIt(t *testing.T) { + got := buildIssueSearchQuery("%zz", "", "Next") + values, err := url.ParseQuery(got) + if err != nil { + t.Fatalf("ParseQuery(%q): %v", got, err) + } + if got := values.Get("milestone"); got != "Next" { + t.Errorf("milestone = %q, want Next (unparseable filter must not drop it)", got) + } + if got := values.Get("state"); got != "opened" { + t.Errorf("state = %q, want opened", got) + } + if got := values.Get("scope"); got != "all" { + t.Errorf("scope = %q, want all", got) + } +} + +func TestBuildIssueSearchQuery_MilestoneEscapedExactlyOnce(t *testing.T) { + // Preset path: values.Encode() does the single escape. + got := buildIssueSearchQuery("", "", "Q3 & Q4=x") + values, err := url.ParseQuery(got) + if err != nil { + t.Fatalf("ParseQuery(%q): %v", got, err) + } + if got := values.Get("milestone"); got != "Q3 & Q4=x" { + t.Errorf("milestone = %q, want %q", got, "Q3 & Q4=x") + } + if len(values) != 3 { + t.Errorf("values = %v, want exactly state/scope/milestone (no injected key)", values) + } + + // Escape-hatch path: url.QueryEscape does the single escape. + got = buildIssueSearchQuery("", "state=closed", "Q3 & Q4=x") + values, err = url.ParseQuery(got) + if err != nil { + t.Fatalf("ParseQuery(%q): %v", got, err) + } + if got := values.Get("milestone"); got != "Q3 & Q4=x" { + t.Errorf("milestone = %q, want %q", got, "Q3 & Q4=x") + } + if len(values) != 2 { + t.Errorf("values = %v, want exactly state/milestone (no injected key)", values) + } +} + +// Scenario 22: GitLab's predefined milestone values (None, Any, Upcoming) +// are forwarded verbatim on "milestone" — kandev applies no special +// handling and never translates any of them onto "milestone_id". +func TestBuildIssueSearchQuery_PredefinedMilestoneValuesPassThroughUntranslated(t *testing.T) { + for _, value := range []string{"None", "Any", "Upcoming"} { + got := buildIssueSearchQuery("", "", value) + values, err := url.ParseQuery(got) + if err != nil { + t.Fatalf("ParseQuery(%q): %v", got, err) + } + if got := values.Get("milestone"); got != value { + t.Errorf("milestone = %q, want %q", got, value) + } + if values.Has("milestone_id") { + t.Errorf("query %q carries milestone_id, want it absent for value %q", got, value) + } + } +} + +func TestConvertRawIssue_MilestoneTitle(t *testing.T) { + raw := &rawIssue{Milestone: &rawMilestone{Title: "Next"}} + issue := convertRawIssue(raw) + if issue.Milestone != "Next" { + t.Errorf("Milestone = %q, want Next", issue.Milestone) + } +} + +func TestConvertRawIssue_NullMilestoneDecodesToEmptyString(t *testing.T) { + raw := &rawIssue{Milestone: nil} + issue := convertRawIssue(raw) + if issue.Milestone != "" { + t.Errorf("Milestone = %q, want empty for a null milestone", issue.Milestone) + } +} + +// trimGitLabWhitespace is the Go-side normative trim helper: Unicode +// White_Space together with U+FEFF. strings.TrimSpace already covers the +// Unicode White_Space set, including U+0085, but it does not cover U+FEFF. +// Scenario 27. +func TestTrimGitLabWhitespace_MatchesNormativeSet(t *testing.T) { + trimmed := []rune{ + 0x0009, 0x000A, 0x000B, 0x000C, 0x000D, 0x0020, 0x0085, 0x00A0, + 0x1680, 0x2000, 0x2028, 0x2029, 0x202F, 0x205F, 0x3000, 0xFEFF, + } + for _, r := range trimmed { + got := trimGitLabWhitespace(string(r)) + if got != "" { + t.Errorf("trimGitLabWhitespace(%U) = %q, want empty", r, got) + } + } + + preserved := []rune{0x200B, 0x180E, 0x2060, 0x00B7} + for _, r := range preserved { + got := trimGitLabWhitespace(string(r)) + if got != string(r) { + t.Errorf("trimGitLabWhitespace(%U) = %q, want unchanged", r, got) + } + } + + nelPrefixed := string(rune(0x0085)) + "Next" + string(rune(0xFEFF)) + if got := trimGitLabWhitespace(nelPrefixed); got != "Next" { + t.Errorf("trimGitLabWhitespace(NEL+Next+BOM) = %q, want Next", got) + } + if got := trimGitLabWhitespace(" Q1 Planning "); got != "Q1 Planning" { + t.Errorf("trimGitLabWhitespace(padded) = %q, want interior space preserved", got) + } + + // The helper adds the one normative rune that TrimSpace does not trim. + bomOnly := string(rune(0xFEFF)) + if got := trimGitLabWhitespace(bomOnly); got != "" { + t.Errorf("trimGitLabWhitespace(BOM alone) = %q, want empty (TrimSpace keeps U+FEFF)", got) + } +} diff --git a/apps/backend/internal/gitlab/controller.go b/apps/backend/internal/gitlab/controller.go index 059cf4055b..01700bd1b3 100644 --- a/apps/backend/internal/gitlab/controller.go +++ b/apps/backend/internal/gitlab/controller.go @@ -3,7 +3,10 @@ package gitlab import ( "errors" "net/http" + "net/url" "strconv" + "strings" + "unicode" "github.com/gin-gonic/gin" @@ -284,6 +287,12 @@ func (c *Controller) httpSearchUserIssues(ctx *gin.Context) { page, perPage := paginationFromQuery(ctx) filter := ctx.Query("filter") customQuery := ctx.Query("custom_query") + if customQuery != "" { + if _, err := url.ParseQuery(customQuery); err != nil { + ctx.JSON(http.StatusBadRequest, gin.H{responseErrorKey: "custom_query must be a valid URL query"}) + return + } + } if customQuery == "" { if filter == filterTokenReviewRequested { ctx.JSON(http.StatusBadRequest, gin.H{responseErrorKey: "review_requested is not supported for issues"}) @@ -297,8 +306,9 @@ func (c *Controller) httpSearchUserIssues(ctx *gin.Context) { if !ok { return } + milestone := trimGitLabWhitespace(ctx.Query("milestone")) result, err := client.ListIssuesPaged( - ctx.Request.Context(), filter, customQuery, page, perPage, + ctx.Request.Context(), filter, customQuery, milestone, page, perPage, ) if err != nil { writeWorkspaceClientActionError(ctx, err, "issue search") @@ -337,6 +347,16 @@ func (c *Controller) translateMRFilter(ctx *gin.Context, filter string) (string, return translateUserSearchFilter(filter, username), nil } +// trimGitLabWhitespace trims Unicode White_Space plus U+FEFF (byte order +// mark). strings.TrimSpace already covers Unicode White_Space, including +// U+0085 (NEL), but it does not trim U+FEFF. The extra rune keeps a pasted BOM +// from surviving as a non-empty milestone that cannot match. +func trimGitLabWhitespace(s string) string { + return strings.TrimFunc(s, func(r rune) bool { + return unicode.IsSpace(r) || r == '\uFEFF' + }) +} + // paginationFromQuery reads ?page=&per_page= with the same clamps SearchMRsPaged // applies internally — surfaced here so 400s on bad input are uniform. func paginationFromQuery(ctx *gin.Context) (int, int) { diff --git a/apps/backend/internal/gitlab/controller_test.go b/apps/backend/internal/gitlab/controller_test.go index 06ed1aa61a..58c1362fcc 100644 --- a/apps/backend/internal/gitlab/controller_test.go +++ b/apps/backend/internal/gitlab/controller_test.go @@ -403,3 +403,65 @@ func TestHttpSearchUserIssues_TranslatesTabFilters(t *testing.T) { }) } } + +// httpSearchUserIssues wiring is otherwise only unit-tested one layer down +// (buildIssueSearchQuery, trimGitLabWhitespace); nothing previously asserted +// end to end that the ?milestone= query key is actually read off the +// request and reaches the outbound GitLab call. A typo in the query key +// name would have passed every other test in this package. +func TestHttpSearchUserIssues_MilestoneReachesUpstreamRequest(t *testing.T) { + router, rec, stop := newControllerFixture(t, "alice") + defer stop() + + resp := hit(router, "/api/v1/gitlab/user/issues?filter=assigned_to_me&milestone="+url.QueryEscape("Next")) + if resp.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", resp.Code, resp.Body.String()) + } + req := rec.findByPath("/api/v4/issues") + if req == nil { + t.Fatal("/api/v4/issues was never called") + } + if got := req.Query.Get("milestone"); got != "Next" { + t.Errorf("milestone = %q, want Next", got) + } + if got := req.Query.Get("scope"); got != "assigned_to_me" { + t.Errorf("scope = %q, want assigned_to_me (milestone must not clobber the preset)", got) + } +} + +// A whitespace-only milestone (Scenario 3's second boundary) trims to "" and +// must produce a request identical to omitting the parameter entirely — no +// milestone key on the wire at all. +func TestHttpSearchUserIssues_WhitespaceOnlyMilestoneOmitsKey(t *testing.T) { + router, rec, stop := newControllerFixture(t, "alice") + defer stop() + + resp := hit(router, "/api/v1/gitlab/user/issues?filter=assigned_to_me&milestone="+url.QueryEscape(" ")) + if resp.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", resp.Code, resp.Body.String()) + } + req := rec.findByPath("/api/v4/issues") + if req == nil { + t.Fatal("/api/v4/issues was never called") + } + if req.Query.Has("milestone") { + t.Errorf("query = %v, want no milestone key for a whitespace-only value", req.Query) + } +} + +func TestHttpSearchUserIssues_MalformedCustomQueryReturns400(t *testing.T) { + router, rec, stop := newControllerFixture(t, "alice") + defer stop() + + resp := hit( + router, + "/api/v1/gitlab/user/issues?custom_query="+url.QueryEscape("%zz")+ + "&milestone="+url.QueryEscape("Next"), + ) + if resp.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400 (body: %s)", resp.Code, resp.Body.String()) + } + if req := rec.findByPath("/api/v4/issues"); req != nil { + t.Errorf("/api/v4/issues was called with query %v — malformed custom query should be rejected", req.Query) + } +} diff --git a/apps/backend/internal/gitlab/mock_client.go b/apps/backend/internal/gitlab/mock_client.go index 5f19d00626..39f7c0d311 100644 --- a/apps/backend/internal/gitlab/mock_client.go +++ b/apps/backend/internal/gitlab/mock_client.go @@ -559,21 +559,6 @@ func (c *MockClient) GetRepoFileContent( return content, nil } -func (c *MockClient) ListIssues(context.Context, string, string) ([]*Issue, error) { - c.mu.Lock() - defer c.mu.Unlock() - out := []*Issue{} - for _, i := range c.issues { - out = append(out, i) - } - return out, nil -} - -func (c *MockClient) ListIssuesPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*IssueSearchPage, error) { - issues, _ := c.ListIssues(ctx, filter, customQuery) - return &IssueSearchPage{Issues: issues, TotalCount: len(issues), Page: page, PerPage: perPage}, nil -} - func (c *MockClient) SearchMRs(context.Context, string, string) ([]*MR, error) { c.mu.Lock() defer c.mu.Unlock() diff --git a/apps/backend/internal/gitlab/mock_client_issues.go b/apps/backend/internal/gitlab/mock_client_issues.go new file mode 100644 index 0000000000..2ef6da46d7 --- /dev/null +++ b/apps/backend/internal/gitlab/mock_client_issues.go @@ -0,0 +1,109 @@ +package gitlab + +import ( + "context" + "net/url" + "sort" +) + +// resolveEffectiveMilestone derives the milestone MockClient.ListIssues +// filters on from the three raw search inputs the controller passes through +// unmodified — MockClient never calls buildIssueSearchQuery, so it must +// reproduce the fold precedence itself. The first matching rule wins: +// +// 1. customQuery carries a "milestone" key: its first value, even if empty +// (an empty key still wins outright — it must not fall through to the +// milestone parameter, matching how an empty fold reaches GitLab as an +// empty milestone on the live path). +// 2. the milestone parameter, if non-empty. +// 3. filter carries a non-empty "milestone" key: its first value. +// 4. otherwise, empty. +func resolveEffectiveMilestone(filter, customQuery, milestone string) string { + if parsed, err := url.ParseQuery(customQuery); err == nil && parsed.Has("milestone") { + return parsed.Get("milestone") + } + if milestone != "" { + return milestone + } + if parsed, err := url.ParseQuery(filter); err == nil { + if v := parsed.Get("milestone"); v != "" { + return v + } + } + return "" +} + +// ListIssues returns seeded issues, optionally narrowed to an exact +// case-sensitive milestone match. See resolveEffectiveMilestone for how +// filter/customQuery/milestone combine. Every other filter key is ignored — +// PATClient is the only client that talks to real GitLab filtering. +func (c *MockClient) ListIssues(_ context.Context, filter, customQuery, milestone string) ([]*Issue, error) { + effective := resolveEffectiveMilestone(filter, customQuery, milestone) + + c.mu.Lock() + defer c.mu.Unlock() + out := []*Issue{} + for _, i := range c.issues { + if effective != "" && i.Milestone != effective { + continue + } + out = append(out, i) + } + sort.Slice(out, func(a, b int) bool { + if out[a].ProjectPath != out[b].ProjectPath { + return out[a].ProjectPath < out[b].ProjectPath + } + return out[a].IID < out[b].IID + }) + return out, nil +} + +// ListIssuesPaged pages over ListIssues' filtered, sorted result. page < 1 is +// normalised to 1; perPage < 1 normalises to 0 (no issues, any page). There +// is no upper clamp on perPage: the echoed Page/PerPage are the normalised +// values actually used, from those two rules only. TotalCount is always the +// full filtered count, not the length of the returned slice. +func (c *MockClient) ListIssuesPaged( + ctx context.Context, filter, customQuery, milestone string, page, perPage int, +) (*IssueSearchPage, error) { + issues, err := c.ListIssues(ctx, filter, customQuery, milestone) + if err != nil { + return nil, err + } + + normPage := page + if normPage < 1 { + normPage = 1 + } + normPerPage := perPage + if normPerPage < 1 { + normPerPage = 0 + } + + // Guard the multiplication itself: normPage is not bounded above, so + // (normPage-1)*normPerPage can overflow int before the "start > + // len(issues)" clamp below ever runs. Comparing via division first is + // safe from overflow and gives the same answer. + var start int + if normPerPage == 0 || normPage-1 > len(issues)/normPerPage { + start = len(issues) + } else { + start = (normPage - 1) * normPerPage + } + + // Guard start+normPerPage the same way: normPerPage is unbounded above + // (no clamp), so the addition can itself overflow int. Comparing against + // the remaining count first avoids ever computing the sum when it would + // exceed len(issues). + end := len(issues) + if normPerPage < len(issues)-start { + end = start + normPerPage + } + + return &IssueSearchPage{ + Issues: issues[start:end], + TotalCount: len(issues), + Page: normPage, + PerPage: normPerPage, + }, nil +} diff --git a/apps/backend/internal/gitlab/mock_client_issues_test.go b/apps/backend/internal/gitlab/mock_client_issues_test.go new file mode 100644 index 0000000000..85203dfa08 --- /dev/null +++ b/apps/backend/internal/gitlab/mock_client_issues_test.go @@ -0,0 +1,334 @@ +package gitlab + +import ( + "math" + "testing" +) + +func seedMilestoneIssues(t *testing.T, c *MockClient) { + t.Helper() + c.SeedIssue("acme/beta", &Issue{IID: 1, ProjectPath: "acme/beta", Title: "beta-1", Milestone: "Next"}) + c.SeedIssue("acme/alpha", &Issue{IID: 2, ProjectPath: "acme/alpha", Title: "alpha-2", Milestone: "Old"}) + c.SeedIssue("acme/alpha", &Issue{IID: 1, ProjectPath: "acme/alpha", Title: "alpha-1", Milestone: "Next"}) + c.SeedIssue("acme/alpha", &Issue{IID: 3, ProjectPath: "acme/alpha", Title: "alpha-3", Milestone: ""}) +} + +func TestMockClient_ListIssues_EmptyMilestoneReturnsEverything(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + got, err := c.ListIssues(t.Context(), "", "", "") + if err != nil { + t.Fatalf("ListIssues() error = %v", err) + } + if len(got) != 4 { + t.Fatalf("len(got) = %d, want 4", len(got)) + } +} + +func TestMockClient_ListIssues_ParameterMilestoneFiltersExact(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + got, err := c.ListIssues(t.Context(), "", "", "Next") + if err != nil { + t.Fatalf("ListIssues() error = %v", err) + } + if len(got) != 2 { + t.Fatalf("len(got) = %d, want 2: %#v", len(got), got) + } + // Deterministic order: ProjectPath asc, then IID asc. + if got[0].ProjectPath != "acme/alpha" || got[0].IID != 1 { + t.Errorf("got[0] = %+v, want acme/alpha#1", got[0]) + } + if got[1].ProjectPath != "acme/beta" || got[1].IID != 1 { + t.Errorf("got[1] = %+v, want acme/beta#1", got[1]) + } +} + +func TestMockClient_ListIssues_DeterministicOrderAcrossAllIssues(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + got, err := c.ListIssues(t.Context(), "", "", "") + if err != nil { + t.Fatalf("ListIssues() error = %v", err) + } + want := []struct { + Project string + IID int + }{ + {"acme/alpha", 1}, {"acme/alpha", 2}, {"acme/alpha", 3}, {"acme/beta", 1}, + } + if len(got) != len(want) { + t.Fatalf("len(got) = %d, want %d", len(got), len(want)) + } + for i, w := range want { + if got[i].ProjectPath != w.Project || got[i].IID != w.IID { + t.Errorf("got[%d] = %s#%d, want %s#%d", i, got[i].ProjectPath, got[i].IID, w.Project, w.IID) + } + } +} + +func TestMockClient_ListIssues_CustomQueryMilestoneWinsOverParameter(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + got, err := c.ListIssues(t.Context(), "", "milestone=Old", "Next") + if err != nil { + t.Fatalf("ListIssues() error = %v", err) + } + if len(got) != 1 || got[0].Milestone != "Old" { + t.Fatalf("got = %#v, want exactly one Old-milestone issue", got) + } +} + +func TestMockClient_ListIssues_CustomQueryEmptyMilestoneKeySuppressesParameterFallback(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + // Rule 1: an empty milestone= key in customQuery wins outright (does not + // fall through to the milestone parameter), matching the live-path fold. + got, err := c.ListIssues(t.Context(), "", "milestone=", "Next") + if err != nil { + t.Fatalf("ListIssues() error = %v", err) + } + if len(got) != 4 { + t.Fatalf("len(got) = %d, want 4 (empty milestone key means unfiltered)", len(got)) + } +} + +func TestMockClient_ListIssues_HandwrittenCustomQueryUsesFirstMilestoneValue(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + got, err := c.ListIssues(t.Context(), "", "milestone=Old&milestone=Next", "") + if err != nil { + t.Fatalf("ListIssues() error = %v", err) + } + if len(got) != 1 || got[0].Milestone != "Old" { + t.Fatalf("got = %#v, want exactly one Old-milestone issue (first value wins)", got) + } +} + +func TestMockClient_ListIssues_FilterMilestoneFallbackWhenNonEmpty(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + got, err := c.ListIssues(t.Context(), "milestone=Old", "", "") + if err != nil { + t.Fatalf("ListIssues() error = %v", err) + } + if len(got) != 1 || got[0].Milestone != "Old" { + t.Fatalf("got = %#v, want exactly one Old-milestone issue", got) + } +} + +func TestMockClient_ListIssues_FilterMilestoneEmptyValueIsIgnored(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + got, err := c.ListIssues(t.Context(), "milestone=", "", "") + if err != nil { + t.Fatalf("ListIssues() error = %v", err) + } + if len(got) != 4 { + t.Fatalf("len(got) = %d, want 4 (empty filter milestone value is not adopted)", len(got)) + } +} + +func TestMockClient_ListIssuesPaged_PagesOverFilteredSortedResult(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + page, err := c.ListIssuesPaged(t.Context(), "", "", "", 1, 2) + if err != nil { + t.Fatalf("ListIssuesPaged() error = %v", err) + } + if page.TotalCount != 4 { + t.Errorf("TotalCount = %d, want 4", page.TotalCount) + } + if len(page.Issues) != 2 || page.Issues[0].IID != 1 || page.Issues[1].IID != 2 { + t.Fatalf("page 1 issues = %#v, want alpha#1, alpha#2", page.Issues) + } + + page2, err := c.ListIssuesPaged(t.Context(), "", "", "", 2, 2) + if err != nil { + t.Fatalf("ListIssuesPaged() error = %v", err) + } + if page2.TotalCount != 4 { + t.Errorf("TotalCount = %d, want 4", page2.TotalCount) + } + if len(page2.Issues) != 2 || page2.Issues[0].IID != 3 || page2.Issues[1].ProjectPath != "acme/beta" { + t.Fatalf("page 2 issues = %#v, want alpha#3, beta#1", page2.Issues) + } +} + +func TestMockClient_ListIssuesPaged_PageBeyondEndYieldsEmptyWithSameTotal(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + page, err := c.ListIssuesPaged(t.Context(), "", "", "", 5, 2) + if err != nil { + t.Fatalf("ListIssuesPaged() error = %v", err) + } + if page.TotalCount != 4 { + t.Errorf("TotalCount = %d, want 4", page.TotalCount) + } + if len(page.Issues) != 0 { + t.Errorf("len(Issues) = %d, want 0", len(page.Issues)) + } + if page.Page != 5 || page.PerPage != 2 { + t.Errorf("Page/PerPage = %d/%d, want 5/2", page.Page, page.PerPage) + } +} + +// Scenario 16: 30 seeded issues, 26 in milestone "Next", per_page=25 pages +// the filtered set (not the unfiltered 30), with TotalCount staying at the +// filtered count on every page including the beyond-end one. +func TestMockClient_ListIssuesPaged_PagesOverFilteredMilestoneSetAcrossMultiplePages(t *testing.T) { + c := NewMockClient(DefaultHost) + for i := 1; i <= 26; i++ { + c.SeedIssue("acme/repo", &Issue{IID: i, ProjectPath: "acme/repo", Milestone: "Next"}) + } + for i := 27; i <= 30; i++ { + c.SeedIssue("acme/repo", &Issue{IID: i, ProjectPath: "acme/repo", Milestone: "Old"}) + } + + page1, err := c.ListIssuesPaged(t.Context(), "", "", "Next", 1, 25) + if err != nil { + t.Fatalf("ListIssuesPaged() error = %v", err) + } + if page1.TotalCount != 26 { + t.Errorf("page1 TotalCount = %d, want 26", page1.TotalCount) + } + if len(page1.Issues) != 25 { + t.Errorf("page1 len(Issues) = %d, want 25", len(page1.Issues)) + } + + page2, err := c.ListIssuesPaged(t.Context(), "", "", "Next", 2, 25) + if err != nil { + t.Fatalf("ListIssuesPaged() error = %v", err) + } + if page2.TotalCount != 26 { + t.Errorf("page2 TotalCount = %d, want 26", page2.TotalCount) + } + if len(page2.Issues) != 1 { + t.Errorf("page2 len(Issues) = %d, want 1", len(page2.Issues)) + } + + page3, err := c.ListIssuesPaged(t.Context(), "", "", "Next", 3, 25) + if err != nil { + t.Fatalf("ListIssuesPaged() error = %v", err) + } + if page3.TotalCount != 26 { + t.Errorf("page3 TotalCount = %d, want 26", page3.TotalCount) + } + if len(page3.Issues) != 0 { + t.Errorf("page3 len(Issues) = %d, want 0", len(page3.Issues)) + } +} + +func TestMockClient_ListIssuesPaged_NegativePageNormalisedToOne(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + page, err := c.ListIssuesPaged(t.Context(), "", "", "", -3, 2) + if err != nil { + t.Fatalf("ListIssuesPaged() error = %v", err) + } + if page.Page != 1 { + t.Errorf("Page = %d, want normalised to 1", page.Page) + } + if len(page.Issues) != 2 || page.Issues[0].IID != 1 { + t.Fatalf("issues = %#v, want the first page's worth", page.Issues) + } +} + +func TestMockClient_ListIssuesPaged_NonPositivePerPageReturnsNoIssues(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + page, err := c.ListIssuesPaged(t.Context(), "", "", "", 1, 0) + if err != nil { + t.Fatalf("ListIssuesPaged() error = %v", err) + } + if len(page.Issues) != 0 { + t.Errorf("len(Issues) = %d, want 0", len(page.Issues)) + } + if page.TotalCount != 4 { + t.Errorf("TotalCount = %d, want 4 (full filtered count, even with no results)", page.TotalCount) + } + if page.PerPage != 0 { + t.Errorf("PerPage = %d, want normalised to 0", page.PerPage) + } + + negative, err := c.ListIssuesPaged(t.Context(), "", "", "", 1, -5) + if err != nil { + t.Fatalf("ListIssuesPaged() error = %v", err) + } + if len(negative.Issues) != 0 { + t.Errorf("len(Issues) = %d, want 0", len(negative.Issues)) + } + if negative.PerPage != 0 { + t.Errorf("PerPage = %d, want normalised to 0", negative.PerPage) + } +} + +// The HTTP layer's paginationFromQuery only floors page/perPage, it never +// caps the upper bound (unlike PATClient's clampSearchPage), so an +// out-of-range per_page can reach the mock directly. ListIssuesPaged must +// not overflow its start/end arithmetic and panic on the resulting slice. +// Spec (spec.md:736-747) authorizes only two normalisations — page < 1 and +// perPage < 1 — so an extreme-but-positive perPage is echoed back as given, +// not clamped to a page size the spec never mentions. +func TestMockClient_ListIssuesPaged_ExtremePerPageDoesNotOverflowOrPanic(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + first, err := c.ListIssuesPaged(t.Context(), "", "", "", 1, math.MaxInt) + if err != nil { + t.Fatalf("ListIssuesPaged() error = %v", err) + } + if len(first.Issues) != 4 { + t.Errorf("len(Issues) = %d, want all 4 on page 1", len(first.Issues)) + } + if first.PerPage != math.MaxInt { + t.Errorf("PerPage = %d, want echoed back as math.MaxInt, not clamped", first.PerPage) + } + if first.TotalCount != 4 { + t.Errorf("TotalCount = %d, want 4", first.TotalCount) + } + + page, err := c.ListIssuesPaged(t.Context(), "", "", "", 2, math.MaxInt) + if err != nil { + t.Fatalf("ListIssuesPaged() error = %v", err) + } + if page.TotalCount != 4 { + t.Errorf("TotalCount = %d, want 4", page.TotalCount) + } + if len(page.Issues) != 0 { + t.Errorf("len(Issues) = %d, want 0 (page 2 is beyond the end of the unfiltered set)", len(page.Issues)) + } + if page.PerPage != math.MaxInt { + t.Errorf("PerPage = %d, want echoed back as math.MaxInt, not clamped", page.PerPage) + } +} + +func TestMockClient_ListIssuesPaged_ForwardsMilestoneWithoutResolvingTwice(t *testing.T) { + c := NewMockClient(DefaultHost) + seedMilestoneIssues(t, c) + + page, err := c.ListIssuesPaged(t.Context(), "", "", "Next", 1, 10) + if err != nil { + t.Fatalf("ListIssuesPaged() error = %v", err) + } + if page.TotalCount != 2 { + t.Errorf("TotalCount = %d, want 2", page.TotalCount) + } + for _, issue := range page.Issues { + if issue.Milestone != "Next" { + t.Errorf("issue %+v has milestone %q, want Next", issue, issue.Milestone) + } + } +} diff --git a/apps/backend/internal/gitlab/models.go b/apps/backend/internal/gitlab/models.go index da28dff9f4..c809cef110 100644 --- a/apps/backend/internal/gitlab/models.go +++ b/apps/backend/internal/gitlab/models.go @@ -349,6 +349,7 @@ type Issue struct { ProjectPath string `json:"project_path"` Labels []string `json:"labels"` Assignees []string `json:"assignees"` + Milestone string `json:"milestone,omitempty"` CreatedAt time.Time `json:"created_at"` UpdatedAt time.Time `json:"updated_at"` ClosedAt *time.Time `json:"closed_at,omitempty"` diff --git a/apps/backend/internal/gitlab/noop_client.go b/apps/backend/internal/gitlab/noop_client.go index 370d88cba7..5306a64a25 100644 --- a/apps/backend/internal/gitlab/noop_client.go +++ b/apps/backend/internal/gitlab/noop_client.go @@ -123,11 +123,11 @@ func (c *NoopClient) GetRepoFileContent(context.Context, string, string, string) return nil, ErrNoClient } -func (c *NoopClient) ListIssues(context.Context, string, string) ([]*Issue, error) { +func (c *NoopClient) ListIssues(context.Context, string, string, string) ([]*Issue, error) { return nil, ErrNoClient } -func (c *NoopClient) ListIssuesPaged(context.Context, string, string, int, int) (*IssueSearchPage, error) { +func (c *NoopClient) ListIssuesPaged(context.Context, string, string, string, int, int) (*IssueSearchPage, error) { return nil, ErrNoClient } diff --git a/apps/backend/internal/gitlab/pat_client.go b/apps/backend/internal/gitlab/pat_client.go index 7307f783e0..e2465aae30 100644 --- a/apps/backend/internal/gitlab/pat_client.go +++ b/apps/backend/internal/gitlab/pat_client.go @@ -262,17 +262,17 @@ func (c *PATClient) SearchMRsPaged(ctx context.Context, filter, customQuery stri }, nil } -func (c *PATClient) ListIssues(ctx context.Context, filter, customQuery string) ([]*Issue, error) { - page, err := c.ListIssuesPaged(ctx, filter, customQuery, defaultPageStart, defaultPageSize) +func (c *PATClient) ListIssues(ctx context.Context, filter, customQuery, milestone string) ([]*Issue, error) { + page, err := c.ListIssuesPaged(ctx, filter, customQuery, milestone, defaultPageStart, defaultPageSize) if err != nil { return nil, err } return page.Issues, nil } -func (c *PATClient) ListIssuesPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*IssueSearchPage, error) { +func (c *PATClient) ListIssuesPaged(ctx context.Context, filter, customQuery, milestone string, page, perPage int) (*IssueSearchPage, error) { page, perPage = clampSearchPage(page, perPage) - query := buildIssueSearchQuery(filter, customQuery) + query := buildIssueSearchQuery(filter, customQuery, milestone) endpoint := fmt.Sprintf("/issues?%s&page=%d&per_page=%d", query, page, perPage) var raw []rawIssue total, err := c.getWithTotal(ctx, endpoint, &raw) diff --git a/apps/backend/internal/gitlab/pat_client_search_actions_test.go b/apps/backend/internal/gitlab/pat_client_search_actions_test.go index 3854049115..a90eeda84a 100644 --- a/apps/backend/internal/gitlab/pat_client_search_actions_test.go +++ b/apps/backend/internal/gitlab/pat_client_search_actions_test.go @@ -54,7 +54,7 @@ func TestPATClientListIssuesUsesDefaultPagination(t *testing.T) { })) t.Cleanup(stop) - issues, err := NewPATClient(host, "tok").ListIssues(t.Context(), "open", "assignee_username=alice") + issues, err := NewPATClient(host, "tok").ListIssues(t.Context(), "open", "assignee_username=alice", "") if err != nil { t.Fatalf("ListIssues() error = %v", err) } diff --git a/apps/backend/internal/gitlab/pat_client_test.go b/apps/backend/internal/gitlab/pat_client_test.go index 8fc8e967ba..4d4d5406cc 100644 --- a/apps/backend/internal/gitlab/pat_client_test.go +++ b/apps/backend/internal/gitlab/pat_client_test.go @@ -430,7 +430,7 @@ func TestPATClient_ListIssuesPaged_ScopeAssignedToMe(t *testing.T) { defer stop() c := NewPATClient(host, "tok") - if _, err := c.ListIssuesPaged(context.Background(), "scope=assigned_to_me", "", 1, 25); err != nil { + if _, err := c.ListIssuesPaged(context.Background(), "scope=assigned_to_me", "", "Next", 1, 25); err != nil { t.Fatalf("err = %v", err) } if receivedPath != "/issues" { @@ -442,6 +442,9 @@ func TestPATClient_ListIssuesPaged_ScopeAssignedToMe(t *testing.T) { if got := receivedQuery.Get("state"); got != "opened" { t.Errorf("state = %q, want opened", got) } + if got := receivedQuery.Get("milestone"); got != "Next" { + t.Errorf("milestone = %q, want Next — must reach the server", got) + } } func TestPATClient_ListIssuesPaged_HonoursTotalHeader(t *testing.T) { @@ -467,7 +470,7 @@ func TestPATClient_ListIssuesPaged_HonoursTotalHeader(t *testing.T) { defer stop() c := NewPATClient(host, "tok") - page, err := c.ListIssuesPaged(context.Background(), "", "", 2, 20) + page, err := c.ListIssuesPaged(context.Background(), "", "", "", 2, 20) if err != nil { t.Fatalf("err = %v", err) } diff --git a/apps/backend/internal/gitlab/service_issue_query_test.go b/apps/backend/internal/gitlab/service_issue_query_test.go index fb5282bfab..b47a6e4e94 100644 --- a/apps/backend/internal/gitlab/service_issue_query_test.go +++ b/apps/backend/internal/gitlab/service_issue_query_test.go @@ -13,7 +13,7 @@ type captureIssueQueryClient struct { listCalls int } -func (c *captureIssueQueryClient) ListIssues(_ context.Context, filter, customQuery string) ([]*Issue, error) { +func (c *captureIssueQueryClient) ListIssues(_ context.Context, filter, customQuery, _ string) ([]*Issue, error) { c.filter = filter c.customQuery = customQuery c.listCalls++ diff --git a/apps/backend/internal/gitlab/service_issue_watches.go b/apps/backend/internal/gitlab/service_issue_watches.go index 1ba2d4183b..86f4ceb1b4 100644 --- a/apps/backend/internal/gitlab/service_issue_watches.go +++ b/apps/backend/internal/gitlab/service_issue_watches.go @@ -287,7 +287,7 @@ func (s *Service) fetchIssues(ctx context.Context, watch *IssueWatch) ([]*Issue, filter += "&labels=" + url.QueryEscape(strings.Join(watch.Labels, ",")) } } - issues, err := client.ListIssues(ctx, filter, customQuery) + issues, err := client.ListIssues(ctx, filter, customQuery, "") if err != nil { return nil, fmt.Errorf("list issues: %w", err) } diff --git a/apps/backend/internal/gitlab/service_mentions.go b/apps/backend/internal/gitlab/service_mentions.go index d394027569..0b3a43100f 100644 --- a/apps/backend/internal/gitlab/service_mentions.go +++ b/apps/backend/internal/gitlab/service_mentions.go @@ -151,7 +151,7 @@ func (s *Service) SearchMentionIssuesForWorkspace( return collectMentionSearchPages(ctx, scope, limit, MentionIssueURL, func( ctx context.Context, pageNumber, pageSize int, ) (mentionSearchPage, error) { - page, err := client.ListIssuesPaged(ctx, "", titleQuery, pageNumber, pageSize) + page, err := client.ListIssuesPaged(ctx, "", titleQuery, "", pageNumber, pageSize) if err != nil { return mentionSearchPage{}, fmt.Errorf("search GitLab issue mentions: %w", err) } diff --git a/apps/backend/internal/gitlab/service_mentions_test.go b/apps/backend/internal/gitlab/service_mentions_test.go index d2f34401eb..181f37e597 100644 --- a/apps/backend/internal/gitlab/service_mentions_test.go +++ b/apps/backend/internal/gitlab/service_mentions_test.go @@ -18,7 +18,7 @@ type mentionRecordingClient struct { func (c *mentionRecordingClient) ListIssuesPaged( ctx context.Context, - filter, customQuery string, + filter, customQuery, _ string, page, perPage int, ) (*IssueSearchPage, error) { return c.issues(ctx, filter, customQuery, page, perPage) diff --git a/apps/backend/internal/gitlab/service_merge_search_test.go b/apps/backend/internal/gitlab/service_merge_search_test.go index 1942b4f289..e28add5e11 100644 --- a/apps/backend/internal/gitlab/service_merge_search_test.go +++ b/apps/backend/internal/gitlab/service_merge_search_test.go @@ -67,8 +67,8 @@ func TestServiceClientActionsReturnErrNoClient(t *testing.T) { {name: "branches", call: func() error { _, err := service.ListProjectBranches(t.Context(), "acme/widget"); return err }}, {name: "search MRs", call: func() error { _, err := service.SearchUserMRs(t.Context(), "", ""); return err }}, {name: "search MRs paged", call: func() error { _, err := service.SearchUserMRsPaged(t.Context(), "", "", 1, 10); return err }}, - {name: "search issues", call: func() error { _, err := service.SearchUserIssues(t.Context(), "", ""); return err }}, - {name: "search issues paged", call: func() error { _, err := service.SearchUserIssuesPaged(t.Context(), "", "", 1, 10); return err }}, + {name: "search issues", call: func() error { _, err := service.SearchUserIssues(t.Context(), "", "", ""); return err }}, + {name: "search issues paged", call: func() error { _, err := service.SearchUserIssuesPaged(t.Context(), "", "", "", 1, 10); return err }}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/apps/backend/internal/gitlab/service_search.go b/apps/backend/internal/gitlab/service_search.go index 06e8d4f805..4c19ca715d 100644 --- a/apps/backend/internal/gitlab/service_search.go +++ b/apps/backend/internal/gitlab/service_search.go @@ -25,21 +25,21 @@ func (s *Service) SearchUserMRsPaged(ctx context.Context, filter, customQuery st } // SearchUserIssues returns issues matching a filter for the configured user. -func (s *Service) SearchUserIssues(ctx context.Context, filter, customQuery string) ([]*Issue, error) { +func (s *Service) SearchUserIssues(ctx context.Context, filter, customQuery, milestone string) ([]*Issue, error) { client := s.Client() if client == nil { return nil, ErrNoClient } - return client.ListIssues(ctx, filter, customQuery) + return client.ListIssues(ctx, filter, customQuery, milestone) } // SearchUserIssuesPaged returns paginated issues. -func (s *Service) SearchUserIssuesPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*IssueSearchPage, error) { +func (s *Service) SearchUserIssuesPaged(ctx context.Context, filter, customQuery, milestone string, page, perPage int) (*IssueSearchPage, error) { client := s.Client() if client == nil { return nil, ErrNoClient } - return client.ListIssuesPaged(ctx, filter, customQuery, page, perPage) + return client.ListIssuesPaged(ctx, filter, customQuery, milestone, page, perPage) } // GetStats aggregates open MR / awaiting-review / open-issue counts. @@ -85,7 +85,7 @@ func (s *Service) getStatsWithClient(ctx context.Context, client Client) (*Stats } else if awaiting != nil { awaitingMRs = awaiting.TotalCount } - issues, err := client.ListIssuesPaged(ctx, "scope=assigned_to_me&state=opened", "", 1, 1) + issues, err := client.ListIssuesPaged(ctx, "scope=assigned_to_me&state=opened", "", "", 1, 1) openIssues := 0 if err != nil { s.logger.Warn("gitlab stats: open issues", zap.Error(err)) diff --git a/apps/backend/internal/gitlab/service_watches.go b/apps/backend/internal/gitlab/service_watches.go index 78d32f9c5b..94b7d95713 100644 --- a/apps/backend/internal/gitlab/service_watches.go +++ b/apps/backend/internal/gitlab/service_watches.go @@ -836,17 +836,11 @@ func (s *Service) requireStore() *Store { // appendLabelsToQuery merges a label list into an existing customQuery string. // If the query already has a `labels` key the caller's value is kept (we // don't want to silently double-up). url.ParseQuery is used for an exact -// key match — strings.Contains("labels=") would false-positive on keys -// like `mylabels=` and silently drop the watch's labels. +// key match by the shared appendQueryParam helper — strings.Contains("labels=") +// would false-positive on keys like `mylabels=` and silently drop the watch's +// labels. func appendLabelsToQuery(customQuery string, labels []string) string { - if parsed, err := url.ParseQuery(customQuery); err == nil && parsed.Has("labels") { - return customQuery - } - encoded := url.QueryEscape(strings.Join(labels, ",")) - if customQuery == "" { - return "labels=" + encoded - } - return customQuery + "&labels=" + encoded + return appendQueryParam(customQuery, "labels", strings.Join(labels, ",")) } func normalizeProjectFilters(in []ProjectFilter) []ProjectFilter { diff --git a/apps/backend/internal/gitlab/service_watches_test.go b/apps/backend/internal/gitlab/service_watches_test.go index 174ba34e81..ffba5c3bc2 100644 --- a/apps/backend/internal/gitlab/service_watches_test.go +++ b/apps/backend/internal/gitlab/service_watches_test.go @@ -38,6 +38,14 @@ func TestAppendLabelsToQuery_DoesNotMatchSimilarKeys(t *testing.T) { } } +func TestAppendLabelsToQuery_InsertsBeforeFragment(t *testing.T) { + got := appendLabelsToQuery("state=opened#client-fragment", []string{"bug"}) + want := "state=opened&labels=bug#client-fragment" + if got != want { + t.Fatalf("got %q, want %q (labels must precede the URL fragment)", got, want) + } +} + func TestIsValidCleanupPolicy(t *testing.T) { cases := map[string]bool{ "": true, diff --git a/apps/web/app/gitlab/gitlab-page-client.tsx b/apps/web/app/gitlab/gitlab-page-client.tsx index abb3629aae..3232085bc3 100644 --- a/apps/web/app/gitlab/gitlab-page-client.tsx +++ b/apps/web/app/gitlab/gitlab-page-client.tsx @@ -11,20 +11,12 @@ import { fetchGitLabStatus } from "@/lib/api/domains/gitlab-api"; import type { GitLabStatus, Issue, MR } from "@/lib/types/gitlab"; import { MRList } from "@/components/gitlab/my-gitlab/mr-list"; import { IssueList } from "@/components/gitlab/my-gitlab/issue-list"; -import { - PresetsSidebar, - type SidebarSelection, -} from "@/components/gitlab/my-gitlab/presets-sidebar"; +import type { SidebarSelection } from "@/components/gitlab/my-gitlab/presets-sidebar"; +import { PresetsSidebar } from "@/components/gitlab/my-gitlab/presets-sidebar"; import { PresetsScopeBar } from "@/components/gitlab/my-gitlab/presets-scope-bar"; -import { MR_PRESETS, ISSUE_PRESETS, presetLabel } from "@/components/gitlab/my-gitlab/presets"; -import { useGitLabSearch } from "@/components/gitlab/my-gitlab/use-gitlab-search"; -import { useSavedPresets, type SavedPreset } from "@/components/gitlab/my-gitlab/use-saved-presets"; -import { - useKnownProjects, - resetKnownProjectsStore, -} from "@/components/gitlab/my-gitlab/use-known-projects"; -import { useCommittedQuery } from "@/components/gitlab/my-gitlab/use-committed-query"; +import { resetKnownProjectsStore } from "@/components/gitlab/my-gitlab/use-known-projects"; import { ListToolbar } from "@/components/gitlab/my-gitlab/list-toolbar"; +import { useGitLabPageState, type GitLabPageState } from "./use-gitlab-page-state"; import { ResultsPagination } from "@/components/gitlab/my-gitlab/results-pagination"; import { SavePresetDialog } from "@/components/gitlab/my-gitlab/save-preset-dialog"; import { useMRKeyToTasks } from "@/hooks/domains/gitlab/use-mr-key-to-tasks"; @@ -67,23 +59,6 @@ function NotConnectedNotice({ reconnect }: { reconnect?: boolean }) { ); } -function resolveTitle( - selection: SidebarSelection, - saved: SavedPreset[], - t: (key: string, values?: Record) => string, -): string { - if (selection.source === "saved") { - return saved.find((p) => p.id === selection.id)?.label ?? t("gitlab:savedQueryFallback"); - } - const presets = selection.kind === "mr" ? MR_PRESETS : ISSUE_PRESETS; - return ( - presetLabel( - t, - presets.find((p) => p.value === selection.id), - ) ?? (selection.kind === "mr" ? t("gitlab:titleMergeRequests") : t("gitlab:titleIssues")) - ); -} - function ResultsList({ selection, items, @@ -132,137 +107,6 @@ function ResultsList({ ); } -type GitLabPageState = ReturnType; - -function useProjectOptions( - selection: SidebarSelection, - committedQuery: string, - items: Array, - projectFilter: string, -): string[] { - const pageProjects = useMemo( - () => items.filter((it) => !!it.project_path).map((it) => it.project_path), - [items], - ); - const resetKey = `${selection.kind}:${selection.source}:${selection.id}:${committedQuery.trim()}`; - const knownProjects = useKnownProjects(resetKey, pageProjects); - return useMemo(() => { - const set = new Set(knownProjects); - if (projectFilter) set.add(projectFilter); - return Array.from(set).sort(); - }, [knownProjects, projectFilter]); -} - -/** Lifted out of `useGitLabPageState`, which is at the 100-line function cap. */ -function initialSelection(): SidebarSelection { - return { kind: "mr", source: "preset", id: MR_PRESETS[0]?.value ?? "" }; -} - -function useGitLabPageState(searchEnabled: boolean, workspaceId?: string) { - const { t } = useTranslation(); - const [selection, setSelection] = useState(initialSelection); - const { - draft: customQuery, - committed: committedQuery, - setDraft: setCustomQuery, - setImmediate: setQueryImmediate, - commit: commitCustomQuery, - } = useCommittedQuery(""); - const [projectFilter, setProjectFilter] = useState(""); - const [saveDialogOpen, setSaveDialogOpen] = useState(false); - const { - presets: savedPresets, - save: saveSavedPreset, - remove: removeSavedPreset, - } = useSavedPresets(); - - const presets = selection.kind === "mr" ? MR_PRESETS : ISSUE_PRESETS; - const search = useGitLabSearch({ - workspaceId: workspaceId ?? "", - kind: selection.kind, - presets, - preset: selection.source === "preset" ? selection.id : "", - customQuery: committedQuery, - projectFilter, - enabled: searchEnabled && Boolean(workspaceId), - }); - const projectOptions = useProjectOptions( - selection, - committedQuery, - search.rawItems, - projectFilter, - ); - const title = useMemo( - () => resolveTitle(selection, savedPresets, t), - [selection, savedPresets, t], - ); - - const onSelect = useCallback( - (s: SidebarSelection) => { - setSelection(s); - if (s.source === "saved") { - const found = savedPresets.find((p) => p.id === s.id); - setQueryImmediate(found?.customQuery ?? ""); - setProjectFilter(found?.projectFilter ?? ""); - return; - } - setQueryImmediate(""); - setProjectFilter(""); - }, - [savedPresets, setQueryImmediate], - ); - - // Use committedQuery (not the unflushed draft) so the saved preset always - // matches what is currently displayed in the list. - const canSaveCurrent = committedQuery.trim().length > 0 || projectFilter.length > 0; - // i18n-exempt: persisted as the saved query's name, so it must not depend on the creating locale. - const suggestedLabel = - committedQuery.trim() || (projectFilter ? `In ${projectFilter}` : "Saved query"); - const onOpenSaveDialog = () => { - if (canSaveCurrent) setSaveDialogOpen(true); - }; - const onConfirmSave = (label: string) => { - const created = saveSavedPreset({ - kind: selection.kind, - label, - customQuery: committedQuery, - projectFilter, - }); - setSelection({ kind: selection.kind, source: "saved", id: created.id }); - }; - const onDeleteSaved = (id: string) => { - removeSavedPreset(id); - if (selection.source === "saved" && selection.id === id) { - const fallbackPresets = selection.kind === "mr" ? MR_PRESETS : ISSUE_PRESETS; - setSelection({ kind: selection.kind, source: "preset", id: fallbackPresets[0]?.value ?? "" }); - setQueryImmediate(""); - setProjectFilter(""); - } - }; - - return { - selection, - customQuery, - committedQuery, - setCustomQuery, - commitCustomQuery, - projectFilter, - setProjectFilter, - savedPresets, - search, - projectOptions, - title, - onSelect, - canSaveCurrent, - suggestedLabel, - saveDialogOpen, - setSaveDialogOpen, - onOpenSaveDialog, - onConfirmSave, - onDeleteSaved, - }; -} - function AuthenticatedLayout({ workspaceId, state, @@ -311,6 +155,11 @@ function AuthenticatedLayout({ onProjectFilterChange={state.setProjectFilter} projectOptions={projectOptions} onRefresh={search.refresh} + showMilestoneFilter={state.showMilestoneFilter} + milestone={state.milestone} + committedMilestone={state.committedMilestone} + onMilestoneChange={state.setMilestone} + onCommitMilestone={state.onCommitMilestone} />
Promise>(); +const searchUserIssuesMock = vi.fn<(input: unknown) => Promise>(); +vi.mock("@/lib/api/domains/gitlab-api", () => ({ + searchUserMRs: (args: unknown) => searchUserMRsMock(args), + searchUserIssues: (args: unknown) => searchUserIssuesMock(args), +})); + +const fetchUserSettingsMock = vi.fn(); +const updateUserSettingsMock = vi.fn(); +vi.mock("@/lib/api/domains/settings-api", () => ({ + fetchUserSettings: (...args: unknown[]) => fetchUserSettingsMock(...args), + updateUserSettings: (...args: unknown[]) => updateUserSettingsMock(...args), +})); + +import { + buildProjectOptionsResetKey, + trimGitLabMilestone, + useGitLabPageState, + useProjectOptions, +} from "./use-gitlab-page-state"; + +afterEach(() => cleanup()); + +const WORKSPACE_ID = "ws-1"; +const CREATED_AT = "2026-01-01T00:00:00Z"; + +function fakeIssue(overrides: Partial = {}): Issue { + return { + id: 1, + iid: 1, + project_id: 1, + title: "", + body: "", + url: "", + web_url: "", + state: "opened", + author_username: "alice", + project_namespace: "acme", + project_path: "acme/api", + labels: [], + assignees: [], + milestone: "", + created_at: "", + updated_at: "", + ...overrides, + }; +} + +function issuePage(items: Issue[]): IssueSearchPage { + return { issues: items, total_count: items.length, page: 1, per_page: 25 }; +} + +beforeEach(() => { + vi.clearAllMocks(); + resetKnownProjectsStore(); + __resetSnapshotForTests(); + fetchUserSettingsMock.mockResolvedValue({ settings: { gitlab_saved_presets: [] } }); + updateUserSettingsMock.mockResolvedValue({ settings: {} }); + searchUserMRsMock.mockResolvedValue({ mrs: [], total_count: 0, page: 1, per_page: 25 }); + searchUserIssuesMock.mockResolvedValue(issuePage([])); +}); + +describe("trimGitLabMilestone", () => { + it("trims ordinary leading/trailing whitespace", () => { + expect(trimGitLabMilestone(" v1.0 ")).toBe("v1.0"); + }); + + it("trims a leading BOM (\uFEFF) and a trailing NBSP (\u00A0)", () => { + expect(trimGitLabMilestone("\uFEFFv1.0\u00A0")).toBe("v1.0"); + }); + + it("trims a trailing NEL (\u0085), which the bare \\s regex class omits", () => { + expect(trimGitLabMilestone("v1.0\u0085")).toBe("v1.0"); + }); + + it("preserves internal whitespace", () => { + expect(trimGitLabMilestone(" Sprint 42 ")).toBe("Sprint 42"); + }); + + it("returns empty for a whitespace-only value including a NEL", () => { + expect(trimGitLabMilestone(" \u0085 ")).toBe(""); + }); + + // Mirrors the Go side's TestTrimGitLabWhitespace_MatchesNormativeSet + // (client_helpers_milestone_test.go) code point for code point: the + // normative set is Unicode White_Space together with U+FEFF, on BOTH + // sides, and the two languages' own defaults disagree with it and with + // each other in both directions (Scenario 27). Asserting only a handful + // of ad-hoc cases on one side does not establish the cross-language + // agreement the spec requires. + it("matches the normative set for every code point in the Go-side conformance table", () => { + const trimmed = [ + 0x0009, 0x000a, 0x000b, 0x000c, 0x000d, 0x0020, 0x0085, 0x00a0, 0x1680, 0x2000, 0x2028, + 0x2029, 0x202f, 0x205f, 0x3000, 0xfeff, + ]; + for (const cp of trimmed) { + expect(trimGitLabMilestone(String.fromCodePoint(cp))).toBe(""); + } + + // Preserved: not part of Unicode White_Space or U+FEFF. + const preserved = [0x200b, 0x180e, 0x2060, 0x00b7]; + for (const cp of preserved) { + const ch = String.fromCodePoint(cp); + expect(trimGitLabMilestone(ch)).toBe(ch); + } + }); +}); + +describe("buildProjectOptionsResetKey", () => { + it("keeps delimiter-containing query and milestone values distinct", () => { + const selection: SidebarSelection = { kind: "issue", source: "preset", id: "assigned" }; + + const queryWithDelimiter = buildProjectOptionsResetKey(selection, "labels=backend:v1", "Q2"); + const milestoneWithDelimiter = buildProjectOptionsResetKey( + selection, + "labels=backend", + "v1:Q2", + ); + + expect(queryWithDelimiter).not.toBe(milestoneWithDelimiter); + }); +}); + +describe("useProjectOptions — ordinary pagination (key unchanged)", () => { + beforeEach(() => resetKnownProjectsStore()); + + const SELECTION: SidebarSelection = { kind: "issue", source: "preset", id: "assigned" }; + + it("keeps projects seen on an earlier page while a same-key page turn is loading", () => { + const items = [fakeIssue({ project_path: "acme/api" })]; + const { result, rerender } = renderHook( + (props: { loading: boolean; items: Issue[] }) => + useProjectOptions({ + selection: SELECTION, + committedQuery: "", + milestone: "", + items: props.items, + loading: props.loading, + projectFilter: "", + }), + { initialProps: { loading: false, items } }, + ); + expect(result.current).toEqual(["acme/api"]); + + // Page 2 is loading: same key, so the accumulator must not clear. + rerender({ loading: true, items: [] }); + expect(result.current).toEqual(["acme/api"]); + + // Page 2 resolves with a different project; it accumulates on top. + rerender({ loading: false, items: [fakeIssue({ project_path: "acme/web" })] }); + expect(result.current).toEqual(["acme/api", "acme/web"]); + }); +}); + +describe("useGitLabPageState", () => { + it("does not leak the previous milestone's projects into the dropdown while the new milestone's fetch is in flight (Scenario 29)", async () => { + searchUserIssuesMock.mockResolvedValueOnce( + issuePage([fakeIssue({ project_path: "acme/api" })]), + ); + let resolveNext: (v: IssueSearchPage) => void = () => {}; + searchUserIssuesMock.mockReturnValueOnce( + new Promise((res) => { + resolveNext = res; + }), + ); + + const { result } = renderHook(() => useGitLabPageState(true, WORKSPACE_ID)); + act(() => result.current.onSelect({ kind: "issue", source: "preset", id: "assigned" })); + await waitFor(() => expect(result.current.projectOptions).toEqual(["acme/api"])); + + act(() => result.current.setMilestone("Next")); + act(() => result.current.onCommitMilestone()); + + // The new milestone's fetch is in flight; the previous milestone's + // project must not still be offered. + expect(result.current.projectOptions).toEqual([]); + + await act(async () => { + resolveNext(issuePage([fakeIssue({ project_path: "acme/web" })])); + await Promise.resolve(); + }); + await waitFor(() => expect(result.current.projectOptions).toEqual(["acme/web"])); + expect(result.current.projectOptions).not.toContain("acme/api"); + }); + + it("clears the milestone draft and committed value when the selection changes, including switching to the MR view", async () => { + const { result } = renderHook(() => useGitLabPageState(true, WORKSPACE_ID)); + act(() => result.current.onSelect({ kind: "issue", source: "preset", id: "assigned" })); + act(() => result.current.setMilestone("Next")); + act(() => result.current.onCommitMilestone()); + await waitFor(() => expect(result.current.committedMilestone).toBe("Next")); + + act(() => result.current.onSelect({ kind: "mr", source: "preset", id: MR_PRESETS[0].value })); + expect(result.current.milestone).toBe(""); + expect(result.current.committedMilestone).toBe(""); + expect(result.current.showMilestoneFilter).toBe(false); + }); + + it("onCommitMilestone normalizes the visible input via the trim helper, resets to page 1, and issues exactly one request (Scenario 7)", async () => { + searchUserIssuesMock.mockResolvedValue(issuePage([])); + const { result } = renderHook(() => useGitLabPageState(true, WORKSPACE_ID)); + act(() => result.current.onSelect({ kind: "issue", source: "preset", id: "assigned" })); + await waitFor(() => expect(result.current.search.loading).toBe(false)); + + act(() => result.current.search.setPage(3)); + await waitFor(() => expect(result.current.search.page).toBe(3)); + + const callsBeforeCommit = searchUserIssuesMock.mock.calls.length; + act(() => result.current.setMilestone(" Next ")); + act(() => result.current.onCommitMilestone()); + + expect(result.current.milestone).toBe("Next"); + expect(result.current.committedMilestone).toBe("Next"); + await waitFor(() => expect(result.current.search.page).toBe(1)); + + // Committing and resetting the page together must fire exactly one + // request for the new value — adding milestone to the existing + // [preset, customQuery, kind] reset effect would instead fire twice + // (once for the stale page, once for page 1); see spec "Client + // function, and where trimming happens". + await waitFor(() => expect(searchUserIssuesMock).toHaveBeenCalledTimes(callsBeforeCommit + 1)); + const lastCall = searchUserIssuesMock.mock.calls.at(-1)?.[0] as { + page: number; + milestone?: string; + }; + expect(lastCall.page).toBe(1); + expect(lastCall.milestone).toBe("Next"); + }); + + it("resets to page 1 when selecting a sidebar preset clears a committed milestone (Scenario 6, clause 3)", async () => { + searchUserIssuesMock.mockResolvedValue(issuePage([])); + const { result } = renderHook(() => useGitLabPageState(true, WORKSPACE_ID)); + act(() => result.current.onSelect({ kind: "issue", source: "preset", id: "assigned" })); + act(() => result.current.setMilestone("Next")); + act(() => result.current.onCommitMilestone()); + await waitFor(() => expect(result.current.committedMilestone).toBe("Next")); + + act(() => result.current.search.setPage(3)); + await waitFor(() => expect(result.current.search.page).toBe(3)); + + // Re-select the SAME preset: effectivePreset/customQuery/kind are all + // unchanged, so useGitLabSearch's `[preset, customQuery, kind]` reset + // effect will not fire on its own — only the milestone clears. + act(() => result.current.onSelect({ kind: "issue", source: "preset", id: "assigned" })); + expect(result.current.committedMilestone).toBe(""); + await waitFor(() => expect(result.current.search.page).toBe(1)); + }); +}); + +describe("useGitLabPageState — saved query save", () => { + it("canSaveCurrent and suggestedLabel consider the milestone alongside the query and project filter", async () => { + const { result } = renderHook(() => useGitLabPageState(true, WORKSPACE_ID)); + act(() => result.current.onSelect({ kind: "issue", source: "preset", id: "assigned" })); + expect(result.current.canSaveCurrent).toBe(false); + + act(() => result.current.setMilestone("Next")); + act(() => result.current.onCommitMilestone()); + expect(result.current.canSaveCurrent).toBe(true); + expect(result.current.suggestedLabel).toBe("Next"); + }); + + it("suggestedLabel precedence ladder: custom query beats project filter beats milestone beats the default (Scenario 25)", async () => { + const { result } = renderHook(() => useGitLabPageState(true, WORKSPACE_ID)); + act(() => result.current.onSelect({ kind: "issue", source: "preset", id: "assigned" })); + expect(result.current.suggestedLabel).toBe("Saved query"); + + act(() => result.current.setMilestone("Next")); + act(() => result.current.onCommitMilestone()); + expect(result.current.suggestedLabel).toBe("Next"); + + act(() => result.current.setProjectFilter("group/app")); + expect(result.current.suggestedLabel).toBe("In group/app"); + + act(() => result.current.setCustomQuery("state=closed")); + act(() => result.current.commitCustomQuery()); + await waitFor(() => expect(result.current.committedQuery).toBe("state=closed")); + expect(result.current.suggestedLabel).toBe("state=closed"); + }); + + it("onConfirmSave writes the committed (trimmed) milestone and the effective preset, never the raw draft", async () => { + const { result } = renderHook(() => useGitLabPageState(true, WORKSPACE_ID)); + act(() => result.current.onSelect({ kind: "issue", source: "preset", id: "assigned" })); + act(() => result.current.setMilestone(" Next ")); + act(() => result.current.onCommitMilestone()); + await waitFor(() => expect(result.current.committedMilestone).toBe("Next")); + + await act(async () => { + result.current.onConfirmSave("My saved query"); + await Promise.resolve(); + }); + + await waitFor(() => expect(updateUserSettingsMock).toHaveBeenCalled()); + const payload = updateUserSettingsMock.mock.calls.at(-1)?.[0] as { + gitlab_saved_presets: SavedPreset[]; + }; + const saved = payload.gitlab_saved_presets.at(-1); + expect(saved?.label).toBe("My saved query"); + expect(saved?.milestone).toBe("Next"); + expect(saved?.preset).toBe("assigned"); + expect(saved?.customQuery).toBe(""); + }); +}); + +describe("useGitLabPageState — saved query restore", () => { + it("restores both the milestone and the effective preset (including a preset-only query with no custom text) when the saved query is selected", async () => { + const created: SavedPreset = { + id: "g_1", + kind: "issue", + label: "Next sprint", + customQuery: "", + projectFilter: "", + milestone: "Next", + preset: "assigned", + createdAt: CREATED_AT, + }; + fetchUserSettingsMock.mockResolvedValue({ settings: { gitlab_saved_presets: [created] } }); + searchUserIssuesMock.mockResolvedValue(issuePage([fakeIssue({ project_path: "acme/api" })])); + + const { result } = renderHook(() => useGitLabPageState(true, WORKSPACE_ID)); + await waitFor(() => expect(result.current.savedPresets).toHaveLength(1)); + + act(() => result.current.onSelect({ kind: "issue", source: "saved", id: "g_1" })); + expect(result.current.committedMilestone).toBe("Next"); + expect(result.current.committedQuery).toBe(""); + // Preset-only saved query with no custom query text: the original + // preset filter (persisted as `preset`) must still drive the fetch. + await waitFor(() => expect(searchUserIssuesMock).toHaveBeenCalled()); + const lastCall = searchUserIssuesMock.mock.calls.at(-1)?.[0] as Record; + expect(lastCall.filter).toBe("assigned_to_me"); + }); + + it("resets to page 1 when selecting a saved query that differs from the current state only in its milestone (Scenario 6, clause 2)", async () => { + const created: SavedPreset = { + id: "g_1", + kind: "issue", + label: "Next sprint", + customQuery: "", + projectFilter: "", + milestone: "Next", + preset: "assigned", + createdAt: CREATED_AT, + }; + fetchUserSettingsMock.mockResolvedValue({ settings: { gitlab_saved_presets: [created] } }); + + const { result } = renderHook(() => useGitLabPageState(true, WORKSPACE_ID)); + await waitFor(() => expect(result.current.savedPresets).toHaveLength(1)); + + // Same effective preset ("assigned"), same empty custom query, same kind + // as the saved query below — only the milestone will differ, so the + // `[preset, customQuery, kind]` reset effect in useGitLabSearch will not + // fire on its own. + act(() => result.current.onSelect({ kind: "issue", source: "preset", id: "assigned" })); + await waitFor(() => expect(result.current.search.loading).toBe(false)); + act(() => result.current.search.setPage(3)); + await waitFor(() => expect(result.current.search.page).toBe(3)); + + act(() => result.current.onSelect({ kind: "issue", source: "saved", id: "g_1" })); + expect(result.current.committedMilestone).toBe("Next"); + await waitFor(() => expect(result.current.search.page).toBe(1)); + }); +}); + +describe("useGitLabPageState — saved query delete", () => { + it("clears milestone and resets to page 1 only when the deleted saved query was the active selection", async () => { + const other: SavedPreset = { + id: "g_other", + kind: "issue", + label: "Other", + customQuery: "labels=bug", + projectFilter: "", + milestone: "", + preset: "", + createdAt: CREATED_AT, + }; + const selected: SavedPreset = { + id: "g_selected", + kind: "issue", + label: "Selected", + customQuery: "", + projectFilter: "", + milestone: "Next", + preset: "assigned", + createdAt: CREATED_AT, + }; + fetchUserSettingsMock.mockResolvedValue({ + settings: { gitlab_saved_presets: [other, selected] }, + }); + + const { result } = renderHook(() => useGitLabPageState(true, WORKSPACE_ID)); + await waitFor(() => expect(result.current.savedPresets).toHaveLength(2)); + + act(() => result.current.onSelect({ kind: "issue", source: "saved", id: "g_selected" })); + await waitFor(() => expect(result.current.committedMilestone).toBe("Next")); + act(() => result.current.search.setPage(2)); + await waitFor(() => expect(result.current.search.page).toBe(2)); + + // Deleting an unrelated saved query leaves the active scope untouched. + act(() => result.current.onDeleteSaved("g_other")); + expect(result.current.committedMilestone).toBe("Next"); + expect(result.current.search.page).toBe(2); + + // Deleting the currently-selected saved query clears the scope and + // resets the page in the same batch. + act(() => result.current.onDeleteSaved("g_selected")); + expect(result.current.committedMilestone).toBe(""); + await waitFor(() => expect(result.current.search.page).toBe(1)); + expect(result.current.selection).toEqual({ + kind: "issue", + source: "preset", + id: ISSUE_PRESETS[0]?.value ?? "", + }); + }); +}); diff --git a/apps/web/app/gitlab/use-gitlab-page-state.ts b/apps/web/app/gitlab/use-gitlab-page-state.ts new file mode 100644 index 0000000000..5de1af6e84 --- /dev/null +++ b/apps/web/app/gitlab/use-gitlab-page-state.ts @@ -0,0 +1,421 @@ +"use client"; + +import { useCallback, useEffect, useMemo, useState } from "react"; +import { useTranslation } from "react-i18next"; +import type { Issue, MR } from "@/lib/types/gitlab"; +import type { SidebarSelection } from "@/components/gitlab/my-gitlab/presets-sidebar"; +import { MR_PRESETS, ISSUE_PRESETS, presetLabel } from "@/components/gitlab/my-gitlab/presets"; +import { useGitLabSearch } from "@/components/gitlab/my-gitlab/use-gitlab-search"; +import { useSavedPresets, type SavedPreset } from "@/components/gitlab/my-gitlab/use-saved-presets"; +import { useKnownProjects } from "@/components/gitlab/my-gitlab/use-known-projects"; +import { useCommittedQuery } from "@/components/gitlab/my-gitlab/use-committed-query"; + +// Normative trim set for the milestone filter: Unicode White_Space plus +// U+0085 (NEL), which JS's built-in String.prototype.trim() and the bare \s +// regex class both omit. Mirrors the backend's trimGitLabWhitespace. Every +// commit site for the milestone draft must go through this helper, never +// String.prototype.trim() directly. +export function trimGitLabMilestone(value: string): string { + return value.replace(/^[\s\u0085]+|[\s\u0085]+$/gu, ""); +} + +function resolveTitle( + selection: SidebarSelection, + saved: SavedPreset[], + t: (key: string, values?: Record) => string, +): string { + if (selection.source === "saved") { + return saved.find((p) => p.id === selection.id)?.label ?? t("gitlab:savedQueryFallback"); + } + const presets = selection.kind === "mr" ? MR_PRESETS : ISSUE_PRESETS; + return ( + presetLabel( + t, + presets.find((p) => p.value === selection.id), + ) ?? (selection.kind === "mr" ? t("gitlab:titleMergeRequests") : t("gitlab:titleIssues")) + ); +} + +const EMPTY_PROJECTS: string[] = []; + +export type UseProjectOptionsArgs = { + selection: SidebarSelection; + committedQuery: string; + milestone: string; + items: Array; + loading: boolean; + projectFilter: string; +}; + +/** Encode every reset-key component as a JSON tuple so user values cannot collide. */ +export function buildProjectOptionsResetKey( + selection: SidebarSelection, + committedQuery: string, + milestone: string, +): string { + return JSON.stringify([ + selection.kind, + selection.source, + selection.id, + committedQuery.trim(), + milestone, + ]); +} + +export function useProjectOptions({ + selection, + committedQuery, + milestone, + items, + loading, + projectFilter, +}: UseProjectOptionsArgs): string[] { + const resetKey = buildProjectOptionsResetKey(selection, committedQuery, milestone); + + // `useKnownProjects`'s accumulator clears the instant `resetKey` changes and + // immediately refills with whatever project list it is handed in that same + // effect call. On the render where a key change first lands, `items` (from + // useGitLabSearch) still holds the *previous* key's results, and `loading` + // still reads false, because the fetch for the new key is dispatched from + // an effect that has not run yet (state.loading only flips inside + // fetchData, which runs from an effect). `committedKey` mirrors `resetKey` + // one render late via an ordinary effect, so on that first render + // `committedKey !== resetKey` forces the page list empty regardless of + // what `loading` happens to read at that instant; by the time + // `committedKey` catches up, the fetch has been dispatched and `loading` + // governs the empty state correctly for the rest of the request. + const [committedKey, setCommittedKey] = useState(resetKey); + useEffect(() => { + setCommittedKey(resetKey); + }, [resetKey]); + + const pageProjects = useMemo(() => { + if (loading || committedKey !== resetKey) return EMPTY_PROJECTS; + return items.filter((it) => !!it.project_path).map((it) => it.project_path); + }, [loading, committedKey, resetKey, items]); + + const knownProjects = useKnownProjects(resetKey, pageProjects); + return useMemo(() => { + const set = new Set(knownProjects); + if (projectFilter) set.add(projectFilter); + return Array.from(set).sort(); + }, [knownProjects, projectFilter]); +} + +/** Lifted out of `useGitLabPageState`, which is at the 100-line function cap. */ +export function initialSelection(): SidebarSelection { + return { kind: "mr", source: "preset", id: MR_PRESETS[0]?.value ?? "" }; +} + +type SaveQueryDialogArgs = { + selection: SidebarSelection; + setSelection: (s: SidebarSelection) => void; + committedQuery: string; + projectFilter: string; + committedMilestone: string; + effectivePreset: string; + saveSavedPreset: ReturnType["save"]; + removeSavedPreset: ReturnType["remove"]; + clearScope: () => void; + resetPage: () => void; +}; + +/** Lifted out of `useGitLabPageState`, which is at the 100-line function cap. */ +function useSaveQueryDialog({ + selection, + setSelection, + committedQuery, + projectFilter, + committedMilestone, + effectivePreset, + saveSavedPreset, + removeSavedPreset, + clearScope, + resetPage, +}: SaveQueryDialogArgs) { + const [saveDialogOpen, setSaveDialogOpen] = useState(false); + + // Use committed values (not unflushed drafts) so the saved preset always + // matches what is currently displayed in the list. + const canSaveCurrent = + committedQuery.trim().length > 0 || projectFilter.length > 0 || committedMilestone.length > 0; + // i18n-exempt: persisted as the saved query's name, so it must not depend on the creating locale. + const suggestedLabel = + committedQuery.trim() || + (projectFilter ? `In ${projectFilter}` : "") || + committedMilestone || + "Saved query"; + + const onOpenSaveDialog = useCallback(() => { + if (canSaveCurrent) setSaveDialogOpen(true); + }, [canSaveCurrent]); + + const onConfirmSave = useCallback( + (label: string) => { + const created = saveSavedPreset({ + kind: selection.kind, + label, + customQuery: committedQuery, + projectFilter, + milestone: committedMilestone, + preset: effectivePreset, + }); + setSelection({ kind: selection.kind, source: "saved", id: created.id }); + }, + [ + selection.kind, + committedQuery, + projectFilter, + committedMilestone, + effectivePreset, + saveSavedPreset, + setSelection, + ], + ); + + const onDeleteSaved = useCallback( + (id: string) => { + removeSavedPreset(id); + if (selection.source === "saved" && selection.id === id) { + const fallbackPresets = selection.kind === "mr" ? MR_PRESETS : ISSUE_PRESETS; + setSelection({ + kind: selection.kind, + source: "preset", + id: fallbackPresets[0]?.value ?? "", + }); + clearScope(); + resetPage(); + } + }, + [removeSavedPreset, selection, setSelection, clearScope, resetPage], + ); + + return { + saveDialogOpen, + setSaveDialogOpen, + canSaveCurrent, + suggestedLabel, + onOpenSaveDialog, + onConfirmSave, + onDeleteSaved, + }; +} + +type ScopeActionsArgs = { + savedPresets: SavedPreset[]; + setSelection: (s: SidebarSelection) => void; + setQueryImmediate: (v: string) => void; + setProjectFilter: (v: string) => void; + setMilestoneImmediate: (v: string) => void; + milestone: string; + setPage: (p: number) => void; +}; + +/** Lifted out of `useGitLabPageState`, which is at the 100-line function cap. */ +function useScopeActions({ + savedPresets, + setSelection, + setQueryImmediate, + setProjectFilter, + setMilestoneImmediate, + milestone, + setPage, +}: ScopeActionsArgs) { + const onSelect = useCallback( + (s: SidebarSelection) => { + setSelection(s); + if (s.source === "saved") { + const found = savedPresets.find((p) => p.id === s.id); + setQueryImmediate(found?.customQuery ?? ""); + setProjectFilter(found?.projectFilter ?? ""); + setMilestoneImmediate(found?.milestone ?? ""); + setPage(1); + return; + } + setQueryImmediate(""); + setProjectFilter(""); + setMilestoneImmediate(""); + setPage(1); + }, + [ + savedPresets, + setSelection, + setQueryImmediate, + setProjectFilter, + setMilestoneImmediate, + setPage, + ], + ); + + // Every path that changes the committed milestone calls setPage(1) from + // inside the same event handler (commit, select, delete), so a stale later + // page is never left showing results for a filter that no longer applies — + // a saved query or sidebar preset can differ from the current state only + // in its milestone, in which case useGitLabSearch's own + // `[preset, customQuery, kind]` reset effect would not otherwise fire. + const onCommitMilestone = useCallback(() => { + setMilestoneImmediate(trimGitLabMilestone(milestone)); + setPage(1); + }, [milestone, setMilestoneImmediate, setPage]); + + const clearScope = useCallback(() => { + setQueryImmediate(""); + setProjectFilter(""); + setMilestoneImmediate(""); + }, [setQueryImmediate, setProjectFilter, setMilestoneImmediate]); + + const resetPage = useCallback(() => setPage(1), [setPage]); + + return { onSelect, onCommitMilestone, clearScope, resetPage }; +} + +type SearchAndProjectsArgs = { + searchEnabled: boolean; + workspaceId?: string; + selection: SidebarSelection; + committedQuery: string; + committedMilestone: string; + projectFilter: string; + savedPresets: SavedPreset[]; + t: (key: string, values?: Record) => string; +}; + +/** Lifted out of `useGitLabPageState`, which is at the 100-line function cap. */ +function useSearchAndProjects({ + searchEnabled, + workspaceId, + selection, + committedQuery, + committedMilestone, + projectFilter, + savedPresets, + t, +}: SearchAndProjectsArgs) { + const presets = selection.kind === "mr" ? MR_PRESETS : ISSUE_PRESETS; + // A saved query with no custom query text still needs the preset filter it + // was saved under, or restoring it would silently widen to "no filter". + const effectivePreset = useMemo(() => { + if (selection.source === "preset") return selection.id; + return savedPresets.find((p) => p.id === selection.id)?.preset ?? ""; + }, [selection, savedPresets]); + + const search = useGitLabSearch({ + workspaceId: workspaceId ?? "", + kind: selection.kind, + presets, + preset: effectivePreset, + customQuery: committedQuery, + milestone: committedMilestone, + projectFilter, + enabled: searchEnabled && Boolean(workspaceId), + }); + const projectOptions = useProjectOptions({ + selection, + committedQuery, + milestone: committedMilestone, + items: search.rawItems, + loading: search.loading, + projectFilter, + }); + const title = useMemo( + () => resolveTitle(selection, savedPresets, t), + [selection, savedPresets, t], + ); + + return { search, projectOptions, title, effectivePreset }; +} + +export function useGitLabPageState(searchEnabled: boolean, workspaceId?: string) { + const { t } = useTranslation(); + const [selection, setSelection] = useState(initialSelection); + const { + draft: customQuery, + committed: committedQuery, + setDraft: setCustomQuery, + setImmediate: setQueryImmediate, + commit: commitCustomQuery, + } = useCommittedQuery(""); + const { + draft: milestone, + committed: committedMilestone, + setDraft: setMilestone, + setImmediate: setMilestoneImmediate, + } = useCommittedQuery(""); + const [projectFilter, setProjectFilter] = useState(""); + const { + presets: savedPresets, + save: saveSavedPreset, + remove: removeSavedPreset, + } = useSavedPresets(); + + const { search, projectOptions, title, effectivePreset } = useSearchAndProjects({ + searchEnabled, + workspaceId, + selection, + committedQuery, + committedMilestone, + projectFilter, + savedPresets, + t, + }); + + const { onSelect, onCommitMilestone, clearScope, resetPage } = useScopeActions({ + savedPresets, + setSelection, + setQueryImmediate, + setProjectFilter, + setMilestoneImmediate, + milestone, + setPage: search.setPage, + }); + + const { + saveDialogOpen, + setSaveDialogOpen, + canSaveCurrent, + suggestedLabel, + onOpenSaveDialog, + onConfirmSave, + onDeleteSaved, + } = useSaveQueryDialog({ + selection, + setSelection, + committedQuery, + projectFilter, + committedMilestone, + effectivePreset, + saveSavedPreset, + removeSavedPreset, + clearScope, + resetPage, + }); + + return { + selection, + customQuery, + committedQuery, + setCustomQuery, + commitCustomQuery, + milestone, + committedMilestone, + setMilestone, + onCommitMilestone, + showMilestoneFilter: selection.kind === "issue", + projectFilter, + setProjectFilter, + savedPresets, + search, + projectOptions, + title, + onSelect, + canSaveCurrent, + suggestedLabel, + saveDialogOpen, + setSaveDialogOpen, + onOpenSaveDialog, + onConfirmSave, + onDeleteSaved, + }; +} + +export type GitLabPageState = ReturnType; diff --git a/apps/web/components/gitlab/my-gitlab/issue-list.test.tsx b/apps/web/components/gitlab/my-gitlab/issue-list.test.tsx new file mode 100644 index 0000000000..888c9b4b3f --- /dev/null +++ b/apps/web/components/gitlab/my-gitlab/issue-list.test.tsx @@ -0,0 +1,91 @@ +import { cleanup, render, screen, within } from "@testing-library/react"; +import { afterEach, describe, expect, it } from "vitest"; +import type { Issue } from "@/lib/types/gitlab"; +import { IssueList } from "./issue-list"; + +afterEach(() => cleanup()); + +function fakeIssue(overrides: Partial = {}): Issue { + return { + id: 1, + iid: 1, + project_id: 1, + title: "Fix the thing", + body: "", + url: "", + web_url: "", + state: "opened", + author_username: "alice", + project_namespace: "acme", + project_path: "acme/api", + labels: [], + assignees: [], + milestone: "", + created_at: new Date().toISOString(), + updated_at: "", + ...overrides, + }; +} + +describe("IssueList milestone chip", () => { + it("renders no chip when the issue has no milestone, and leaves the label chips unmoved (Scenario 14)", () => { + render( + , + ); + expect(screen.queryByTestId("gitlab-issue-milestone")).toBeNull(); + expect(screen.getByText("bug")).not.toBeNull(); + expect(screen.getByText("frontend")).not.toBeNull(); + }); + + it("positions the milestone chip after the project#iid/author/time metadata and before the first label chip (Scenario 14)", () => { + render( + , + ); + const row = screen.getByTestId("issue-row"); + const metadata = within(row).getByText(/acme\/api#1/); + const chip = within(row).getByTestId("gitlab-issue-milestone"); + const firstLabel = within(row).getByText("bug"); + + expect(chip.textContent).toBe("Next"); + // DOM order: metadata, then the milestone chip, then the first label chip. + expect( + // eslint-disable-next-line no-bitwise -- Node.compareDocumentPosition bitmask, not a numeric flag + metadata.compareDocumentPosition(chip) & Node.DOCUMENT_POSITION_FOLLOWING, + ).not.toBe(0); + expect( + // eslint-disable-next-line no-bitwise -- Node.compareDocumentPosition bitmask, not a numeric flag + chip.compareDocumentPosition(firstLabel) & Node.DOCUMENT_POSITION_FOLLOWING, + ).not.toBe(0); + }); + + it("does not consume a label slot: a milestone plus six labels renders the chip, four labels, and a +2 overflow indicator (Scenario 26)", () => { + render( + , + ); + const row = screen.getByTestId("issue-row"); + expect(within(row).getByTestId("gitlab-issue-milestone").textContent).toBe("Next"); + for (const label of ["l1", "l2", "l3", "l4"]) { + expect(within(row).getByText(label)).not.toBeNull(); + } + expect(within(row).queryByText("l5")).toBeNull(); + expect(within(row).queryByText("l6")).toBeNull(); + expect(within(row).getByText("+2")).not.toBeNull(); + }); +}); diff --git a/apps/web/components/gitlab/my-gitlab/issue-list.tsx b/apps/web/components/gitlab/my-gitlab/issue-list.tsx index 6228217706..fdcde0d219 100644 --- a/apps/web/components/gitlab/my-gitlab/issue-list.tsx +++ b/apps/web/components/gitlab/my-gitlab/issue-list.tsx @@ -21,6 +21,19 @@ type IssueListProps = { host?: string; }; +function IssueMilestoneChip({ milestone }: { milestone: string }) { + if (!milestone) return null; + return ( + + {milestone} + + ); +} + function IssueLabels({ labels }: { labels: string[] }) { if (!labels?.length) return null; return ( @@ -76,6 +89,7 @@ function IssueRow({ time: formatRelativeTime(issue.created_at), })} +
diff --git a/apps/web/components/gitlab/my-gitlab/list-toolbar.test.tsx b/apps/web/components/gitlab/my-gitlab/list-toolbar.test.tsx new file mode 100644 index 0000000000..e170bd7a49 --- /dev/null +++ b/apps/web/components/gitlab/my-gitlab/list-toolbar.test.tsx @@ -0,0 +1,124 @@ +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { TooltipProvider } from "@kandev/ui/tooltip"; +import { ListToolbar } from "./list-toolbar"; + +afterEach(() => cleanup()); + +const MILESTONE_TEST_ID = "gitlab-milestone-filter"; + +function renderToolbar({ + milestone = "", + committedMilestone = "", +}: { milestone?: string; committedMilestone?: string } = {}) { + const onMilestoneChange = vi.fn(); + const onCommitMilestone = vi.fn(); + render( + + + , + ); + return { onMilestoneChange, onCommitMilestone }; +} + +describe("MilestoneFilterInput", () => { + it("does not render when showMilestoneFilter is false", () => { + render( + + + , + ); + + expect(screen.queryByTestId(MILESTONE_TEST_ID)).toBeNull(); + }); + + it("commits on blur when the draft differs from the committed milestone", () => { + const { onCommitMilestone } = renderToolbar({ milestone: "Next", committedMilestone: "" }); + + fireEvent.blur(screen.getByTestId(MILESTONE_TEST_ID)); + + expect(onCommitMilestone).toHaveBeenCalledTimes(1); + }); + + it("does not commit on blur when the draft matches the committed milestone", () => { + const { onCommitMilestone } = renderToolbar({ milestone: "Next", committedMilestone: "Next" }); + + fireEvent.blur(screen.getByTestId(MILESTONE_TEST_ID)); + + expect(onCommitMilestone).not.toHaveBeenCalled(); + }); + + it("commits on Enter and prevents the default form submission", () => { + const { onCommitMilestone } = renderToolbar({ milestone: "Next", committedMilestone: "" }); + + const input = screen.getByTestId(MILESTONE_TEST_ID); + const event = fireEvent.keyDown(input, { key: "Enter" }); + + expect(onCommitMilestone).toHaveBeenCalledTimes(1); + expect(event).toBe(false); + }); + + it("does not commit while an IME is composing a milestone candidate", () => { + const { onCommitMilestone } = renderToolbar({ milestone: "次", committedMilestone: "" }); + + fireEvent.keyDown(screen.getByTestId(MILESTONE_TEST_ID), { + key: "Enter", + isComposing: true, + }); + + expect(onCommitMilestone).not.toHaveBeenCalled(); + }); + + it("does not commit on keys other than Enter", () => { + const { onCommitMilestone } = renderToolbar({ milestone: "Next", committedMilestone: "" }); + + fireEvent.keyDown(screen.getByTestId(MILESTONE_TEST_ID), { key: "Escape" }); + + expect(onCommitMilestone).not.toHaveBeenCalled(); + }); + + it("reports every keystroke through onMilestoneChange without committing", () => { + const { onMilestoneChange, onCommitMilestone } = renderToolbar(); + + fireEvent.change(screen.getByTestId(MILESTONE_TEST_ID), { target: { value: "Sprint 42" } }); + + expect(onMilestoneChange).toHaveBeenCalledWith("Sprint 42"); + expect(onCommitMilestone).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/components/gitlab/my-gitlab/list-toolbar.tsx b/apps/web/components/gitlab/my-gitlab/list-toolbar.tsx index 0cc212017b..3a553a4baf 100644 --- a/apps/web/components/gitlab/my-gitlab/list-toolbar.tsx +++ b/apps/web/components/gitlab/my-gitlab/list-toolbar.tsx @@ -1,6 +1,7 @@ "use client"; import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@kandev/ui/select"; +import { Input } from "@kandev/ui/input"; import { useTranslation } from "react-i18next"; import { IntegrationListToolbar } from "@/components/integrations/integration-list-toolbar"; @@ -19,8 +20,76 @@ type ListToolbarProps = { onProjectFilterChange: (value: string) => void; projectOptions: string[]; onRefresh: () => void; + showMilestoneFilter: boolean; + milestone: string; + committedMilestone: string; + onMilestoneChange: (value: string) => void; + onCommitMilestone: () => void; }; +function ProjectFilterSelect({ + projectFilter, + onProjectFilterChange, + projectOptions, +}: Pick) { + const { t } = useTranslation(); + const selectValue = projectFilter || ALL_PROJECTS; + return ( + + ); +} + +function MilestoneFilterInput({ + milestone, + committedMilestone, + onMilestoneChange, + onCommitMilestone, +}: Pick< + ListToolbarProps, + "milestone" | "committedMilestone" | "onMilestoneChange" | "onCommitMilestone" +>) { + const { t } = useTranslation(); + return ( + onMilestoneChange(event.target.value)} + onKeyDown={(event) => { + if (event.key === "Enter" && !event.nativeEvent.isComposing && event.keyCode !== 229) { + event.preventDefault(); + onCommitMilestone(); + } + }} + onBlur={() => { + if (milestone !== committedMilestone) onCommitMilestone(); + }} + placeholder={t("gitlab:eGSprint42")} + aria-label={t("gitlab:milestoneFilterLabel")} + className="h-11 w-full md:h-8 md:w-[180px]" + data-testid="gitlab-milestone-filter" + /> + ); +} + export function ListToolbar({ title, count, @@ -34,9 +103,13 @@ export function ListToolbar({ onProjectFilterChange, projectOptions, onRefresh, + showMilestoneFilter, + milestone, + committedMilestone, + onMilestoneChange, + onCommitMilestone, }: ListToolbarProps) { const { t } = useTranslation(); - const selectValue = projectFilter || ALL_PROJECTS; return ( onProjectFilterChange(value === ALL_PROJECTS ? "" : value)} - > - - - - - - {t("gitlab:allProjects")} - - {projectOptions.map((key) => ( - - {key} - - ))} - - + <> + + {showMilestoneFilter ? ( + + ) : null} + } queryPlaceholder={t("gitlab:customQueryPressEnterEG")} titleTestId="gitlab-list-toolbar-title" diff --git a/apps/web/components/gitlab/my-gitlab/quick-task-launcher.test.tsx b/apps/web/components/gitlab/my-gitlab/quick-task-launcher.test.tsx index c844d1a14a..fb551ab1e2 100644 --- a/apps/web/components/gitlab/my-gitlab/quick-task-launcher.test.tsx +++ b/apps/web/components/gitlab/my-gitlab/quick-task-launcher.test.tsx @@ -167,6 +167,7 @@ describe("GitLab QuickTaskLauncher recovery", () => { project_path: "group/project", labels: [], assignees: [], + milestone: "", created_at: FIXED_DATE, updated_at: FIXED_DATE, }, diff --git a/apps/web/components/gitlab/my-gitlab/use-gitlab-search.test.ts b/apps/web/components/gitlab/my-gitlab/use-gitlab-search.test.ts index f39ef70a01..740eabe154 100644 --- a/apps/web/components/gitlab/my-gitlab/use-gitlab-search.test.ts +++ b/apps/web/components/gitlab/my-gitlab/use-gitlab-search.test.ts @@ -110,6 +110,7 @@ function fakeIssue(overrides: Partial = {}): Issue { project_path: "acme/api", labels: [], assignees: [], + milestone: "", created_at: "", updated_at: "", ...overrides, @@ -225,6 +226,73 @@ describe("useGitLabSearch — fetch wiring", () => { }); }); +describe("useGitLabSearch — milestone", () => { + beforeEach(resetMocks); + + it("forwards the milestone to the issues API", async () => { + searchUserIssuesMock.mockResolvedValue({ issues: [], total_count: 0, page: 1, per_page: 25 }); + renderHook(() => + useGitLabSearch({ + workspaceId: WORKSPACE_ID, + kind: "issue", + presets: PRESETS, + preset: PRESET_ASSIGNED, + customQuery: "", + milestone: "Next", + }), + ); + await waitFor(() => expect(searchUserIssuesMock).toHaveBeenCalled()); + const args = searchUserIssuesMock.mock.calls[0][0] as Record; + expect(args.milestone).toBe("Next"); + }); + + it("never forwards a milestone to the MR API", async () => { + searchUserMRsMock.mockResolvedValue(EMPTY_PAGE); + renderHook(() => + useGitLabSearch({ + workspaceId: WORKSPACE_ID, + kind: "mr", + presets: PRESETS, + preset: PRESET_ASSIGNED, + customQuery: "", + milestone: "Next", + }), + ); + await waitFor(() => expect(searchUserMRsMock).toHaveBeenCalled()); + const args = searchUserMRsMock.mock.calls[0][0] as Record; + expect(args).not.toHaveProperty("milestone"); + }); + + it("refetches when the milestone changes, without moving the current page", async () => { + searchUserIssuesMock.mockResolvedValue({ issues: [], total_count: 0, page: 1, per_page: 25 }); + const { result, rerender } = renderHook( + ({ milestone }: { milestone: string }) => + useGitLabSearch({ + workspaceId: WORKSPACE_ID, + kind: "issue", + presets: PRESETS, + preset: PRESET_ASSIGNED, + customQuery: "", + milestone, + }), + { initialProps: { milestone: "" } }, + ); + await waitFor(() => expect(searchUserIssuesMock).toHaveBeenCalledTimes(1)); + act(() => result.current.setPage(3)); + await waitFor(() => expect(result.current.page).toBe(3)); + await waitFor(() => expect(searchUserIssuesMock).toHaveBeenCalledTimes(2)); + + rerender({ milestone: "Next" }); + await waitFor(() => expect(searchUserIssuesMock).toHaveBeenCalledTimes(3)); + const args = searchUserIssuesMock.mock.calls[2][0] as Record; + expect(args.milestone).toBe("Next"); + // The hook itself does not reset the page on a milestone change — callers + // that commit a milestone are responsible for calling setPage(1) in the + // same event handler (see use-gitlab-page-state.ts). + expect(args.page).toBe(3); + }); +}); + describe("useGitLabSearch — state", () => { beforeEach(resetMocks); diff --git a/apps/web/components/gitlab/my-gitlab/use-gitlab-search.ts b/apps/web/components/gitlab/my-gitlab/use-gitlab-search.ts index 43219f539c..8cf1678edf 100644 --- a/apps/web/components/gitlab/my-gitlab/use-gitlab-search.ts +++ b/apps/web/components/gitlab/my-gitlab/use-gitlab-search.ts @@ -23,6 +23,7 @@ type SearchState = { type FetchArgs = { filter: string; customQuery: string; + milestone: string; page: number; }; @@ -32,6 +33,7 @@ type UseGitLabSearchOptions = { presets: PresetOption[]; preset: string; customQuery: string; + milestone?: string; projectFilter?: string; // Gate the network fetch on the integration being connected. When GitLab is // not configured the page renders the connect notice instead of the list, so @@ -73,12 +75,42 @@ function filterByProject(items: Item[], project: string): Item[] { return items.filter((it) => it.project_path === project); } +async function fetchGitLabSearchPage( + kind: SearchKind, + workspaceId: string, + args: FetchArgs, +): Promise> { + const params = { + workspaceId, + filter: args.filter, + customQuery: args.customQuery, + page: args.page, + perPage: SEARCH_PAGE_SIZE, + }; + const response = + kind === "mr" + ? await searchUserMRs(params) + : await searchUserIssues({ ...params, milestone: args.milestone }); + const items: Item[] = + kind === "mr" + ? ((response as { mrs: MR[] | null } | null)?.mrs ?? []) + : ((response as { issues: Issue[] | null } | null)?.issues ?? []); + return { + items, + loading: false, + error: null, + lastFetchedAt: new Date(), + total: response?.total_count ?? items.length, + }; +} + export function useGitLabSearch({ workspaceId, kind, presets, preset, customQuery, + milestone = "", projectFilter = "", enabled = true, }: UseGitLabSearchOptions) { @@ -91,7 +123,7 @@ export function useGitLabSearch({ }, [preset, customQuery, kind]); const fetchData = useCallback( - async ({ filter: ef, customQuery: ec, page: epage }: FetchArgs) => { + async (args: FetchArgs) => { if (!enabled || !workspaceId) return; const seq = ++requestSeq.current; const requestedWorkspaceId = workspaceId; @@ -108,28 +140,9 @@ export function useGitLabSearch({ }, ); try { - const params = { - workspaceId, - filter: ef, - customQuery: ec, - page: epage, - perPage: SEARCH_PAGE_SIZE, - }; - const response = - kind === "mr" ? await searchUserMRs(params) : await searchUserIssues(params); + const next = await fetchGitLabSearchPage(kind, workspaceId, args); if (seq !== requestSeq.current) return; - const items: Item[] = - kind === "mr" - ? ((response as { mrs: MR[] | null } | null)?.mrs ?? []) - : ((response as { issues: Issue[] | null } | null)?.issues ?? []); - setState({ - items, - loading: false, - error: null, - lastFetchedAt: new Date(), - total: response?.total_count ?? items.length, - workspaceId: requestedWorkspaceId, - }); + setState({ ...next, workspaceId: requestedWorkspaceId }); } catch (err) { if (seq !== requestSeq.current) return; setState((s) => ({ @@ -155,12 +168,18 @@ export function useGitLabSearch({ // its run callback and its debounce effect) so a disabled integration never // schedules a fetch. The callback keeps its own guard to also cover refresh(). if (!enabled || !workspaceId) return; - void fetchData({ filter: resolved.filter, customQuery: resolved.customQuery, page }); - }, [fetchData, enabled, workspaceId, resolved.filter, resolved.customQuery, page]); + void fetchData({ + filter: resolved.filter, + customQuery: resolved.customQuery, + milestone, + page, + }); + }, [fetchData, enabled, workspaceId, resolved.filter, resolved.customQuery, milestone, page]); const refresh = useCallback( - () => fetchData({ filter: resolved.filter, customQuery: resolved.customQuery, page }), - [fetchData, resolved.filter, resolved.customQuery, page], + () => + fetchData({ filter: resolved.filter, customQuery: resolved.customQuery, milestone, page }), + [fetchData, resolved.filter, resolved.customQuery, milestone, page], ); const isCurrentWorkspace = state.workspaceId === workspaceId; diff --git a/apps/web/components/gitlab/my-gitlab/use-saved-presets.test.ts b/apps/web/components/gitlab/my-gitlab/use-saved-presets.test.ts index d2c0e5fc93..6dcf90b1f1 100644 --- a/apps/web/components/gitlab/my-gitlab/use-saved-presets.test.ts +++ b/apps/web/components/gitlab/my-gitlab/use-saved-presets.test.ts @@ -21,6 +21,8 @@ const valid: SavedPreset = { label: "My MRs", customQuery: "scope=created_by_me", projectFilter: "", + milestone: "", + preset: "", createdAt: "2026-01-01T00:00:00Z", }; @@ -45,6 +47,8 @@ describe("useSavedPresets (gitlab)", () => { label: "My MRs", customQuery: "scope=created_by_me", projectFilter: "", + milestone: "", + preset: "", }); }); expect(result.current.presets).toHaveLength(1); @@ -64,6 +68,8 @@ describe("useSavedPresets (gitlab)", () => { label: "Triage", customQuery: "labels=bug", projectFilter: "", + milestone: "", + preset: "", }); }); expect(result.current.presets).toHaveLength(1); @@ -80,6 +86,8 @@ describe("useSavedPresets (gitlab)", () => { label: "Shared", customQuery: "q", projectFilter: "", + milestone: "", + preset: "", }); }); expect(a.result.current.presets).toHaveLength(1); @@ -97,3 +105,93 @@ describe("useSavedPresets (gitlab)", () => { expect(updateUserSettings).not.toHaveBeenCalled(); }); }); + +const LEGACY_CREATED_AT = "2025-01-01T00:00:00Z"; + +describe("useSavedPresets (gitlab) — legacy preset field normalization", () => { + beforeEach(() => { + vi.clearAllMocks(); + window.localStorage.clear(); + __resetSnapshotForTests(); + vi.mocked(updateUserSettings).mockResolvedValue({ + settings: {}, + } as Awaited>); + }); + + it("keeps a legacy preset with neither a milestone nor a preset key, defaulting both to empty", async () => { + const legacy = { + id: "g_legacy", + kind: "issue", + label: "Legacy triage", + customQuery: "labels=bug", + projectFilter: "", + createdAt: LEGACY_CREATED_AT, + }; + vi.mocked(fetchUserSettings).mockResolvedValue({ + settings: { gitlab_saved_presets: [legacy] }, + } as Awaited>); + + const { result } = renderHook(() => useSavedPresets()); + + await waitFor(() => expect(result.current.presets).toHaveLength(1)); + expect(result.current.presets[0]).toMatchObject({ + id: "g_legacy", + milestone: "", + preset: "", + }); + }); + + it("normalizes a non-string milestone/preset (number, null, object) to empty rather than dropping the entry", async () => { + const withBadTypes = [ + { + id: "g_number", + kind: "issue", + label: "Number", + customQuery: "", + projectFilter: "", + createdAt: LEGACY_CREATED_AT, + milestone: 42, + preset: null, + }, + { + id: "g_object", + kind: "issue", + label: "Object", + customQuery: "", + projectFilter: "", + createdAt: LEGACY_CREATED_AT, + milestone: { title: "Next" }, + preset: 7, + }, + ]; + vi.mocked(fetchUserSettings).mockResolvedValue({ + settings: { gitlab_saved_presets: withBadTypes }, + } as Awaited>); + + const { result } = renderHook(() => useSavedPresets()); + + await waitFor(() => expect(result.current.presets).toHaveLength(2)); + expect(result.current.presets[0]).toMatchObject({ id: "g_number", milestone: "", preset: "" }); + expect(result.current.presets[1]).toMatchObject({ id: "g_object", milestone: "", preset: "" }); + }); + + it("still rejects an entry missing a required legacy field", async () => { + const missingLabel = [ + { + id: "g_bad", + kind: "issue", + customQuery: "", + projectFilter: "", + createdAt: LEGACY_CREATED_AT, + }, + ]; + vi.mocked(fetchUserSettings).mockResolvedValue({ + settings: { gitlab_saved_presets: missingLabel }, + } as Awaited>); + + const { result } = renderHook(() => useSavedPresets()); + + await waitFor(() => expect(fetchUserSettings).toHaveBeenCalled()); + expect(result.current.presets).toEqual([]); + }); +}); diff --git a/apps/web/components/gitlab/my-gitlab/use-saved-presets.ts b/apps/web/components/gitlab/my-gitlab/use-saved-presets.ts index 5355ae85f2..07944ced60 100644 --- a/apps/web/components/gitlab/my-gitlab/use-saved-presets.ts +++ b/apps/web/components/gitlab/my-gitlab/use-saved-presets.ts @@ -10,6 +10,12 @@ export type SavedPreset = { label: string; customQuery: string; projectFilter: string; + // Milestone title in effect when the query was saved, and the sidebar + // preset value it was saved under. Both "" when none. Presets written + // before this feature carry neither key; isSavedPreset tolerates a missing + // or non-string value for either and normalizeSavedPreset reads it as "". + milestone: string; + preset: string; createdAt: string; }; @@ -39,7 +45,10 @@ function publish(next: SavedPreset[]) { for (const l of listeners) l(); } -function isSavedPreset(p: unknown): p is SavedPreset { +// Checks the fields every SavedPreset has always had. milestone/preset are +// deliberately not checked here: a preset saved before this feature has +// neither key, and that SHALL NOT disqualify it (Scenario 13). +function isSavedPreset(p: unknown): p is Omit { return ( typeof p === "object" && p !== null && @@ -52,9 +61,29 @@ function isSavedPreset(p: unknown): p is SavedPreset { ); } +// A missing value, or one present but not a string, reads as "" (Scenario 23). +function normalizeStoredString(value: unknown): string { + return typeof value === "string" ? value : ""; +} + +function normalizeSavedPreset(p: unknown): SavedPreset | null { + if (!isSavedPreset(p)) return null; + const raw = p as Record; + return { + ...p, + milestone: normalizeStoredString(raw.milestone), + preset: normalizeStoredString(raw.preset), + }; +} + function readServerPresets(value: unknown): SavedPreset[] | null { if (!Array.isArray(value)) return null; - return value.filter(isSavedPreset); + const normalized: SavedPreset[] = []; + for (const item of value) { + const preset = normalizeSavedPreset(item); + if (preset) normalized.push(preset); + } + return normalized; } const syncServer = createQueuedUserSettingsSync((next) => ({ diff --git a/apps/web/e2e/tests/gitlab/gitlab-issue-milestone-filter-helpers.ts b/apps/web/e2e/tests/gitlab/gitlab-issue-milestone-filter-helpers.ts new file mode 100644 index 0000000000..ab42549e92 --- /dev/null +++ b/apps/web/e2e/tests/gitlab/gitlab-issue-milestone-filter-helpers.ts @@ -0,0 +1,30 @@ +import type { MockGitLabIssueSeed } from "../../helpers/api-client"; +import { GITLAB_PROJECT } from "../../helpers/gitlab"; + +export const ISSUES_ENDPOINT = /^\/api\/v1\/gitlab\/user\/issues$/; + +export function seededIssue( + iid: number, + title: string, + overrides: { milestone?: string } = {}, +): MockGitLabIssueSeed { + const now = new Date().toISOString(); + return { + id: iid + 20_000, + iid, + project_id: 101, + title, + body: `Body for ${title}`, + url: `https://gitlab.example.test/${GITLAB_PROJECT}/-/issues/${iid}`, + web_url: `https://gitlab.example.test/${GITLAB_PROJECT}/-/issues/${iid}`, + state: "opened", + author_username: "reporter", + project_namespace: "platform", + project_path: GITLAB_PROJECT, + labels: [], + assignees: [], + milestone: overrides.milestone ?? "", + created_at: now, + updated_at: now, + }; +} diff --git a/apps/web/e2e/tests/gitlab/gitlab-issue-milestone-filter.spec.ts b/apps/web/e2e/tests/gitlab/gitlab-issue-milestone-filter.spec.ts new file mode 100644 index 0000000000..c180345a59 --- /dev/null +++ b/apps/web/e2e/tests/gitlab/gitlab-issue-milestone-filter.spec.ts @@ -0,0 +1,210 @@ +import { test, expect } from "../../fixtures/test-base"; +import { waitForHttp } from "../../helpers/causal-waits"; +import { GITLAB_PROJECT } from "../../helpers/gitlab"; +import { GitLabPage } from "../../pages/gitlab-page"; +import { ISSUES_ENDPOINT, seededIssue } from "./gitlab-issue-milestone-filter-helpers"; + +test.describe("GitLab issue milestone filter", () => { + test("composes with a preset, narrows the list, and clears on preset/kind switch", async ({ + testPage, + apiClient, + seedData, + }) => { + await apiClient.configureGitLab(seedData.workspaceId); + await apiClient.mockGitLabAddIssues(seedData.workspaceId, GITLAB_PROJECT, [ + seededIssue(501, "Ship OAuth callback", { milestone: "Next" }), + seededIssue(502, "Improve onboarding copy"), + ]); + + const gitlab = new GitLabPage(testPage); + await gitlab.goto(); + const scopeBar = testPage.getByTestId("gitlab-presets-scope-bar"); + const initialIssuesLoaded = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await scopeBar.getByRole("button", { name: "Issues", exact: true }).click(); + await initialIssuesLoaded; + + await expect(gitlab.issueRow(501)).toBeVisible(); + await expect(gitlab.issueRow(502)).toBeVisible(); + // The chip reflects the issue's own milestone regardless of the filter. + await expect(gitlab.issueRow(501).getByTestId("gitlab-issue-milestone")).toHaveText("Next"); + await expect(gitlab.issueRow(502).getByTestId("gitlab-issue-milestone")).toHaveCount(0); + + const milestoneInput = testPage.getByTestId("gitlab-milestone-filter"); + await expect(milestoneInput).toBeVisible(); + + // Typing without committing issues no request: both rows stay put. + await milestoneInput.fill("Next"); + await expect(gitlab.issueRow(502)).toBeVisible(); + + const narrowed = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await milestoneInput.press("Enter"); + await narrowed; + await expect(gitlab.issueRow(501)).toBeVisible(); + await expect(gitlab.issueRow(502)).toHaveCount(0); + + // No match renders the empty state, not an error. + const noMatch = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await milestoneInput.fill("Nonexistent"); + await milestoneInput.press("Enter"); + await noMatch; + await expect(testPage.getByText("No issues match this filter.", { exact: true })).toBeVisible(); + + // Clearing the milestone restores the unfiltered list. + const cleared = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await milestoneInput.fill(""); + await milestoneInput.press("Enter"); + await cleared; + await expect(gitlab.issueRow(501)).toBeVisible(); + await expect(gitlab.issueRow(502)).toBeVisible(); + + const reNarrowed = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await milestoneInput.fill("Next"); + await milestoneInput.press("Enter"); + await reNarrowed; + await expect(gitlab.issueRow(502)).toHaveCount(0); + + // Switching the sidebar preset clears the committed milestone. + const presetSwitched = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await scopeBar.getByRole("button", { name: "Created", exact: true }).click(); + await presetSwitched; + await expect(milestoneInput).toHaveValue(""); + await expect(gitlab.issueRow(502)).toBeVisible(); + + // Merge requests view never renders the milestone control and is unaffected. + await scopeBar.getByRole("button", { name: "Merge requests", exact: true }).click(); + await expect(testPage.getByTestId("gitlab-milestone-filter")).toHaveCount(0); + + // Switching back to Issues shows an empty input over an unnarrowed list. + const backToIssues = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await scopeBar.getByRole("button", { name: "Issues", exact: true }).click(); + await backToIssues; + await expect(testPage.getByTestId("gitlab-milestone-filter")).toHaveValue(""); + await expect(gitlab.issueRow(501)).toBeVisible(); + await expect(gitlab.issueRow(502)).toBeVisible(); + }); + + test("trims the committed milestone, saves a query, and restores it on reselect", async ({ + testPage, + apiClient, + seedData, + }) => { + // This test chains many sequential network round-trips (commit, save, + // preset switch, reselect, delete); give it headroom beyond the 60s + // project default rather than relying on causal waits to also outrun + // per-step host-load variance. + test.setTimeout(180_000); + await apiClient.configureGitLab(seedData.workspaceId); + await apiClient.mockGitLabAddIssues(seedData.workspaceId, GITLAB_PROJECT, [ + seededIssue(601, "Milestone-scoped issue", { milestone: "Sprint 42" }), + seededIssue(602, "Unrelated issue"), + ]); + + const gitlab = new GitLabPage(testPage); + await gitlab.goto(); + const scopeBar = testPage.getByTestId("gitlab-presets-scope-bar"); + await scopeBar.getByRole("button", { name: "Issues", exact: true }).click(); + + const milestoneInput = testPage.getByTestId("gitlab-milestone-filter"); + const committed = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await milestoneInput.fill(" Sprint 42 "); + await milestoneInput.press("Enter"); + await committed; + // Commit normalizes the visible input, not just the outgoing request. + await expect(milestoneInput).toHaveValue("Sprint 42"); + await expect(gitlab.issueRow(601)).toBeVisible(); + await expect(gitlab.issueRow(602)).toHaveCount(0); + + const savedMenu = testPage.getByTestId("gitlab-saved-queries-menu"); + await savedMenu.click(); + const saveItem = testPage.getByRole("menuitem", { name: "Save current query" }); + await expect(saveItem).toBeEnabled(); + await saveItem.click(); + + const dialog = testPage.getByRole("dialog", { name: "Save GitLab query" }); + await expect(dialog).toBeVisible(); + // Suggested-label precedence: milestone alone, since query and project are empty. + await expect(dialog.getByLabel("Name")).toHaveValue("Sprint 42"); + await dialog.getByRole("button", { name: "Save", exact: true }).click(); + await expect(dialog).toBeHidden(); + await expect(savedMenu).toContainText("Sprint 42"); + + // Selecting a sidebar preset clears the milestone and widens the list again. + const presetSwitched = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await scopeBar.getByRole("button", { name: "Created", exact: true }).click(); + await presetSwitched; + await expect(milestoneInput).toHaveValue(""); + await expect(gitlab.issueRow(602)).toBeVisible(); + + // Reselecting the saved query restores its committed milestone. + await savedMenu.click(); + const reselected = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await testPage.getByRole("menuitem", { name: "Sprint 42", exact: true }).click(); + await reselected; + await expect(milestoneInput).toHaveValue("Sprint 42"); + await expect(gitlab.issueRow(601)).toBeVisible(); + await expect(gitlab.issueRow(602)).toHaveCount(0); + + // Selecting the item above closes the dropdown via Radix's own + // data-closed:animate-out exit transition (duration-100 in + // @kandev/ui/dropdown-menu.tsx); Radix keeps the outgoing menu instance + // mounted for that ~100ms while it animates out. Reopening the trigger + // before that finishes races the old instance's unmount and can leave + // the freshly reopened menu without content (root cause of this test's + // documented pre-existing flake — confirmed via a CI failure artifact + // showing the menu closed, not mid-click, at time of failure). Waiting + // for the menu to fully close first removes the race. + await expect(testPage.getByRole("menu")).toBeHidden(); + + // Deleting the selected saved query clears the milestone in the same update. + await savedMenu.click(); + const deleted = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await testPage.getByRole("menuitem", { name: "Delete Sprint 42 saved query" }).click(); + await deleted; + await expect(milestoneInput).toHaveValue(""); + await expect(gitlab.issueRow(602)).toBeVisible(); + }); + + test("resets pagination to page 1 when the committed milestone changes from page 2 (Scenario 6, clause 1)", async ({ + testPage, + apiClient, + seedData, + }) => { + // All 30 issues share one milestone, so filtering by it does not shrink the + // result count below the 25-per-page threshold: the page still resets even + // though the total (and therefore totalPages) is unchanged by the commit. + const issues = Array.from({ length: 30 }, (_, i) => + seededIssue(801 + i, `Milestone issue ${i + 1}`, { milestone: "Sprint 1" }), + ); + await apiClient.configureGitLab(seedData.workspaceId); + await apiClient.mockGitLabAddIssues(seedData.workspaceId, GITLAB_PROJECT, issues); + + const gitlab = new GitLabPage(testPage); + await gitlab.goto(); + const scopeBar = testPage.getByTestId("gitlab-presets-scope-bar"); + const initialIssuesLoaded = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await scopeBar.getByRole("button", { name: "Issues", exact: true }).click(); + await initialIssuesLoaded; + await expect(gitlab.issueRow(801)).toBeVisible(); + + const page2Loaded = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await testPage.getByRole("link", { name: "2", exact: true }).click(); + await page2Loaded; + await expect(gitlab.issueRow(830)).toBeVisible(); + await expect(testPage.getByRole("link", { name: "2", exact: true })).toHaveAttribute( + "aria-current", + "page", + ); + + const milestoneInput = testPage.getByTestId("gitlab-milestone-filter"); + const filtered = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await milestoneInput.fill("Sprint 1"); + await milestoneInput.press("Enter"); + await filtered; + + await expect(gitlab.issueRow(801)).toBeVisible(); + await expect(testPage.getByRole("link", { name: "1", exact: true })).toHaveAttribute( + "aria-current", + "page", + ); + }); +}); diff --git a/apps/web/e2e/tests/gitlab/mobile-gitlab-issue-milestone-filter.spec.ts b/apps/web/e2e/tests/gitlab/mobile-gitlab-issue-milestone-filter.spec.ts new file mode 100644 index 0000000000..d4fd64ea0f --- /dev/null +++ b/apps/web/e2e/tests/gitlab/mobile-gitlab-issue-milestone-filter.spec.ts @@ -0,0 +1,45 @@ +import { test, expect } from "../../fixtures/test-base"; +import { waitForHttp } from "../../helpers/causal-waits"; +import { assertNoDocumentHorizontalOverflow } from "../../helpers/layout-assertions"; +import { GITLAB_PROJECT } from "../../helpers/gitlab"; +import { GitLabPage } from "../../pages/gitlab-page"; +import { ISSUES_ENDPOINT, seededIssue } from "./gitlab-issue-milestone-filter-helpers"; + +test.describe("Mobile GitLab issue milestone filter", () => { + test("filters issues from the mobile sheet without clipping the milestone input", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + await apiClient.configureGitLab(seedData.workspaceId); + await apiClient.mockGitLabAddIssues(seedData.workspaceId, GITLAB_PROJECT, [ + seededIssue(901, "Mobile milestone issue", { milestone: "Next" }), + seededIssue(902, "Mobile unrelated issue"), + ]); + + const gitlab = new GitLabPage(testPage); + await gitlab.goto(); + await gitlab.mobileFiltersButton.tap(); + await expect(gitlab.mobileSidebar).toBeVisible(); + + const issuesLoaded = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await gitlab.mobileSidebar.getByTestId("gitlab-kind-issue").tap(); + await issuesLoaded; + await expect(gitlab.mobileSidebar).toBeHidden(); + + const milestoneInput = testPage.getByTestId("gitlab-milestone-filter"); + await expect(milestoneInput).toBeVisible(); + const inputBox = await milestoneInput.boundingBox(); + expect(inputBox, "mobile milestone input has no bounding box").not.toBeNull(); + if (inputBox) expect(Math.round(inputBox.height)).toBeGreaterThanOrEqual(44); + + const filtered = waitForHttp(testPage, "GET", ISSUES_ENDPOINT); + await milestoneInput.fill("Next"); + await milestoneInput.press("Enter"); + await filtered; + await expect(gitlab.issueRow(901)).toBeVisible(); + await expect(gitlab.issueRow(902)).toHaveCount(0); + await assertNoDocumentHorizontalOverflow(testPage, "mobile GitLab issue milestone filter"); + }); +}); diff --git a/apps/web/lib/api/domains/gitlab-api.test.ts b/apps/web/lib/api/domains/gitlab-api.test.ts index e2d725846e..22a9dc696f 100644 --- a/apps/web/lib/api/domains/gitlab-api.test.ts +++ b/apps/web/lib/api/domains/gitlab-api.test.ts @@ -242,6 +242,31 @@ describe("gitlab-api — user search", () => { const init = fetchSpy.mock.calls[0]![1] as RequestInit; expect(init.cache).toBe("no-store"); }); + + it("searchUserIssues includes the milestone param when set", async () => { + fetchSpy.mockResolvedValueOnce(mockResponse({ issues: [], total_count: 0 })); + await searchUserIssues({ workspaceId: "ws-2", milestone: "Next" }); + const url = fetchSpy.mock.calls[0]![0] as string; + expect(new URL(url).searchParams.get("milestone")).toBe("Next"); + }); + + it("searchUserIssues omits the milestone param when empty, byte-identical to before", async () => { + fetchSpy.mockResolvedValueOnce(mockResponse({ issues: [], total_count: 0 })); + await searchUserIssues({ workspaceId: "ws-2", filter: "created_by_me", perPage: 10 }); + const withoutMilestone = fetchSpy.mock.calls[0]![0] as string; + + fetchSpy.mockResolvedValueOnce(mockResponse({ issues: [], total_count: 0 })); + await searchUserIssues({ + workspaceId: "ws-2", + filter: "created_by_me", + perPage: 10, + milestone: "", + }); + const withEmptyMilestone = fetchSpy.mock.calls[1]![0] as string; + + expect(withEmptyMilestone).toBe(withoutMilestone); + expect(new URL(withoutMilestone).searchParams.has("milestone")).toBe(false); + }); }); describe("gitlab-api — workspace watch actions", () => { diff --git a/apps/web/lib/api/domains/gitlab-api.ts b/apps/web/lib/api/domains/gitlab-api.ts index a064d0084f..8f37febf47 100644 --- a/apps/web/lib/api/domains/gitlab-api.ts +++ b/apps/web/lib/api/domains/gitlab-api.ts @@ -195,12 +195,14 @@ export async function searchUserIssues(params: { workspaceId: string; filter?: string; customQuery?: string; + milestone?: string; page?: number; perPage?: number; }) { const qs = new URLSearchParams({ workspace_id: params.workspaceId }); if (params.filter) qs.set("filter", params.filter); if (params.customQuery) qs.set("custom_query", params.customQuery); + if (params.milestone) qs.set("milestone", params.milestone); if (params.page) qs.set("page", String(params.page)); if (params.perPage) qs.set("per_page", String(params.perPage)); return fetchJson(`/api/v1/gitlab/user/issues?${qs.toString()}`, { diff --git a/apps/web/lib/types/gitlab.ts b/apps/web/lib/types/gitlab.ts index ad1fa8a142..34bcca1275 100644 --- a/apps/web/lib/types/gitlab.ts +++ b/apps/web/lib/types/gitlab.ts @@ -161,6 +161,7 @@ export type Issue = { project_path: string; labels: string[]; assignees: string[]; + milestone?: string; created_at: string; updated_at: string; closed_at?: string; diff --git a/apps/web/src/locales/en/gitlab.json b/apps/web/src/locales/en/gitlab.json index bcb67b5b5d..3899461f9b 100644 --- a/apps/web/src/locales/en/gitlab.json +++ b/apps/web/src/locales/en/gitlab.json @@ -66,6 +66,7 @@ "discussions": "Discussions", "draft": "Draft", "eGNeedsMyReview": "e.g. Needs my review", + "eGSprint42": "e.g. Sprint 42", "editIssueWatch": "Edit GitLab issue watch", "editReviewWatch": "Edit GitLab review watch", "editWatch": "Edit watch", @@ -159,6 +160,7 @@ "mergeRequestsAndIssues": "{{host}} · merge requests and issues", "mergeThisMergeRequest": "Merge this merge request?", "mergeability": "Mergeability", + "milestoneFilterLabel": "Filter by milestone", "mockTest": "Mock (test)", "mrs": "MRs", "name": "Name", diff --git a/apps/web/src/locales/pseudo/gitlab.json b/apps/web/src/locales/pseudo/gitlab.json index 68d4b1b8ca..19b04724a2 100644 --- a/apps/web/src/locales/pseudo/gitlab.json +++ b/apps/web/src/locales/pseudo/gitlab.json @@ -66,6 +66,7 @@ "discussions": "Ďĩśćũśśĩōńś", "draft": "Ďŕàƒţ", "eGNeedsMyReview": "ē.ĝ. Ńēēďś ḿŷ ŕēvĩēŵ", + "eGSprint42": "ē.ĝ. Śƥŕĩńţ 42", "editIssueWatch": "Ēďĩţ ĜĩţĹàƀ ĩśśũē ŵàţćĥ", "editReviewWatch": "Ēďĩţ ĜĩţĹàƀ ŕēvĩēŵ ŵàţćĥ", "editWatch": "Ēďĩţ ŵàţćĥ", @@ -159,6 +160,7 @@ "mergeRequestsAndIssues": "{{host}} · ḿēŕĝē ŕēqũēśţś àńď ĩśśũēś", "mergeThisMergeRequest": "Ḿēŕĝē ţĥĩś ḿēŕĝē ŕēqũēśţ?", "mergeability": "Ḿēŕĝēàƀĩĺĩţŷ", + "milestoneFilterLabel": "Ƒĩĺţēŕ ƀŷ ḿĩĺēśţōńē", "mockTest": "Ḿōćķ (ţēśţ)", "mrs": "ḾŔś", "name": "Ńàḿē", diff --git a/apps/web/src/locales/pt-pt/gitlab.json b/apps/web/src/locales/pt-pt/gitlab.json index d23a37caff..bcb9fdc30c 100644 --- a/apps/web/src/locales/pt-pt/gitlab.json +++ b/apps/web/src/locales/pt-pt/gitlab.json @@ -66,6 +66,7 @@ "discussions": "Discussões", "draft": "Rascunho", "eGNeedsMyReview": "ex.: Precisa da minha revisão", + "eGSprint42": "ex.: Sprint 42", "editIssueWatch": "Editar a vigilância de issues do GitLab", "editReviewWatch": "Editar a vigilância de revisões do GitLab", "editWatch": "Editar vigilância", @@ -159,6 +160,7 @@ "mergeRequestsAndIssues": "{{host}} · merge requests e issues", "mergeThisMergeRequest": "Fazer merge deste merge request?", "mergeability": "Possibilidade de merge", + "milestoneFilterLabel": "Filtrar por marco", "mockTest": "Mock (teste)", "mrs": "MRs", "name": "Nome", diff --git a/apps/web/src/locales/zh-cn/gitlab.json b/apps/web/src/locales/zh-cn/gitlab.json index 0d574acf0a..c31cab4436 100644 --- a/apps/web/src/locales/zh-cn/gitlab.json +++ b/apps/web/src/locales/zh-cn/gitlab.json @@ -66,6 +66,7 @@ "discussions": "讨论", "draft": "草稿", "eGNeedsMyReview": "例如：需要我审阅", + "eGSprint42": "例如 Sprint 42", "editIssueWatch": "编辑 GitLab 议题监控", "editReviewWatch": "编辑 GitLab 审阅监控", "editWatch": "编辑监控", @@ -159,6 +160,7 @@ "mergeRequestsAndIssues": "{{host}} · 合并请求与议题", "mergeThisMergeRequest": "合并此合并请求？", "mergeability": "可合并性", + "milestoneFilterLabel": "按里程碑筛选", "mockTest": "模拟（测试）", "mrs": "MR", "name": "名称", diff --git a/apps/web/src/locales/zh-hk/gitlab.json b/apps/web/src/locales/zh-hk/gitlab.json index c0d2920db8..26ebe20181 100644 --- a/apps/web/src/locales/zh-hk/gitlab.json +++ b/apps/web/src/locales/zh-hk/gitlab.json @@ -66,6 +66,7 @@ "discussions": "討論", "draft": "草稿", "eGNeedsMyReview": "例如：需要我審閱", + "eGSprint42": "例如 Sprint 42", "editIssueWatch": "編輯 GitLab 議題監控", "editReviewWatch": "編輯 GitLab 審閱監控", "editWatch": "編輯監控", @@ -159,6 +160,7 @@ "mergeRequestsAndIssues": "{{host}} · 合併請求與議題", "mergeThisMergeRequest": "合併此合併請求？", "mergeability": "可合併性", + "milestoneFilterLabel": "按里程碑篩選", "mockTest": "模擬（測試）", "mrs": "MR", "name": "名稱", diff --git a/apps/web/src/locales/zh-tw/gitlab.json b/apps/web/src/locales/zh-tw/gitlab.json index 977ac4aa8a..b0ffc7c175 100644 --- a/apps/web/src/locales/zh-tw/gitlab.json +++ b/apps/web/src/locales/zh-tw/gitlab.json @@ -66,6 +66,7 @@ "discussions": "討論", "draft": "草稿", "eGNeedsMyReview": "例如：需要我審閱", + "eGSprint42": "例如 Sprint 42", "editIssueWatch": "編輯 GitLab 議題監控", "editReviewWatch": "編輯 GitLab 審閱監控", "editWatch": "編輯監控", @@ -159,6 +160,7 @@ "mergeRequestsAndIssues": "{{host}} · 合併請求與議題", "mergeThisMergeRequest": "合併此合併請求？", "mergeability": "可合併性", + "milestoneFilterLabel": "按里程碑篩選", "mockTest": "模擬（測試）", "mrs": "MR", "name": "名稱", @@ -207,7 +209,7 @@ "promptSentToTheSelectedAgent": "為每個新匹配項傳送給所選代理程式設定檔的提示詞。", "placeholderActionTitle": "合併請求或問題的標題", "placeholderActionUrl": "合併請求或問題的 URL", - "placeholderDescription": "描述內容", + "placeholderDescription": "描述正文", "placeholderIssueIid": "問題 IID", "placeholderMergeRequestIid": "合併請求 IID", "placeholderProjectPath": "專案路徑", diff --git a/docs/specs/gitlab-issue-milestone-filter/spec.md b/docs/specs/gitlab-issue-milestone-filter/spec.md new file mode 100644 index 0000000000..55074ac00a --- /dev/null +++ b/docs/specs/gitlab-issue-milestone-filter/spec.md @@ -0,0 +1,1347 @@ +--- +status: draft +created: 2026-09-01 +owner: tbd +issue: https://github.com/kdlbs/kandev/issues/2798 +--- + +# GitLab issue search by milestone + +## Why + +The `/gitlab` page lists the connected user's GitLab issues. The only +structured narrowing it offers is a preset (assigned to me / created by me) and +a project dropdown. Every other GitLab issue attribute — state, labels, +milestone — is reachable only through the free-text **GitLab query parameters** +input in the toolbar. + +That input is not a filter that composes; it is a total override. +`buildIssueSearchQuery` (`apps/backend/internal/gitlab/client_helpers.go:488`) +returns the custom query **verbatim** and discards the generated defaults +whenever it is non-empty: + +```go +func buildIssueSearchQuery(filter, customQuery string) string { + if customQuery != "" { + return customQuery + } + values := url.Values{} + values.Set("state", gitlabStateOpened) + values.Set("scope", "all") + ... +} +``` + +So typing `milestone=Next` today does return milestone-filtered issues, but it +silently drops `scope=all`, and GitLab's `GET /issues` documents that `scope` +"Defaults to `created_by_me`". The user who selected the **Assigned** preset and +then typed a milestone gets issues they *authored* in that milestone — a +different set, with no indication anything changed. There is no way to express +"assigned to me **and** in milestone Next". + +This feature adds milestone as a first-class filter that composes with the +selected preset instead of replacing it. + +## Prior art + +**Wiki leg — receipt.** Searched the Obsidian vault resolved through +`~/.obsidian-wiki/config.henry` → `OBSIDIAN_VAULT_PATH=/Users/henry/Documents/henry/wiki`, +QMD collection `wiki` (441 docs), via the qmd MCP server (semantic path +healthy, not the grep fallback). Five sub-queries across two calls: lex +`GitLab issue filter milestone label query integration`; vec `how should an +external issue tracker integration expose structured filters versus a raw query +escape hatch`; a hyde passage describing structured-filters-plus-escape-hatch +composition; lex `Kandev GitLab integration custom query escape hatch filter +UI`; vec `Kandev integration settings issue watches filters design decisions`. + +**Result: the wiki leg returned nothing useful.** The top hits were +`synthesis/ai-sdlc-platforms-landscape.md`, `concepts/dormant-gate-anti-pattern.md`, +`concepts/guard-allowlist-ratchet-pattern.md` and `concepts/conductor-loop.md` — +all about agent orchestration and policy ratchets, none about integration query +composition or filter UI. There is no prior recorded position on structured +filters versus raw query passthrough, so nothing here departs from an existing +wiki page. + +**saas-kb leg — receipt.** `search_fsm_docs` with `category: "ai_sdlc"`, three +queries: `filter issues by milestone label state in issue tracker integration`; +`GitLab issue search milestone query parameter`; `milestone cycle sprint +grouping issues`. + +**Result: the saas-kb leg returned nothing useful.** Every hit scored ≤ 0.0102 +(noise floor) and none described issue-filter UI: Multica architecture and +inbox pages, Factory.ai sessions/computers API reference, Warp billing FAQ and +2022 changelog, Augment Code artifact search. No vendor in the corpus documents +a milestone filter, so there is no shipped design to copy or deliberately +diverge from. + +**The one source that did answer the design fork** is GitLab's own REST +documentation for `GET /issues`, read directly (see API surface). It is what +settles free-text-versus-dropdown and the special-value question below. + +## What + +- The `/gitlab` page's **Issues** view SHALL offer a milestone filter that + narrows results to issues whose milestone title matches, and that composes + with the currently selected preset rather than replacing it. +- The milestone filter SHALL be a free-text input carrying a milestone + **title**, mirroring GitLab's own `milestone` attribute. It SHALL NOT be a + dropdown: the page searches across every project the user can see, and + GitLab exposes no cross-project milestone listing endpoint (milestones are + only listed per project, `/projects/:id/milestones`, or per group). A + dropdown would require an unbounded fan-out of upstream calls to populate. +- Filtering SHALL happen **server-side at GitLab**, so `total_count` and + pagination stay exact. This is deliberately unlike the existing project + dropdown, which narrows client-side (see Selection, ordering and counts). +- A milestone value SHALL be forwarded to GitLab verbatim after URL escaping, + with exactly one normalisation: outer whitespace is trimmed (see Defaults and + boundaries, and the trimming boundary in Client function). Beyond that trim, + kandev SHALL NOT validate, normalise, case-fold, or interpret it. +- The **Merge requests** view SHALL NOT gain a milestone control, and + `GET /api/v1/gitlab/user/mrs` SHALL NOT accept a `milestone` parameter. Its + request shape and results SHALL be byte-identical before and after this + feature. +- With no milestone entered, every request the page issues SHALL be + byte-identical to today's. +- All new user-facing copy SHALL go through `t()` into + `apps/web/src/locales/en/gitlab.json` and its four sibling catalogs. + +## Data model + +No new persistent backend state. Two shape changes: + +### `gitlab.Issue` gains a milestone title + +`apps/backend/internal/gitlab/models.go` `Issue`, and its mirror +`apps/web/lib/types/gitlab.ts` `Issue`: + +| Field | Type | JSON | Meaning | +|---|---|---|---| +| `Milestone` | `string` | `milestone,omitempty` | Title of the issue's milestone; `""` when the issue has none | + +Populated in `convertRawIssue` from a new `rawIssue.Milestone` struct field +decoded from GitLab's `milestone` object, reading its `title` only. GitLab +returns `"milestone": null` for an unassigned issue; that MUST decode to `""`, +not panic and not the string `"null"`. + +This field exists for two reasons, both load-bearing: the issue row displays it +(see Presentation), and `MockClient` cannot filter on an attribute the model +does not carry (see Mock and e2e). + +No other `rawIssue` milestone sub-field (`id`, `iid`, `state`, `due_date`, +`web_url`) is decoded or exposed. + +### `SavedPreset` gains a milestone and the preset it was saved under + +`apps/web/components/gitlab/my-gitlab/use-saved-presets.ts`: + +| Field | Type | Meaning | +|---|---|---| +| `milestone` | `string` | Milestone title stored with the saved query; `""` when none | +| `preset` | `string` | Value of the sidebar preset in effect at save time; `""` when none was | + +Saved presets persist server-side under the `gitlab_saved_presets` user +setting. **Presets written before this change have neither key**, and +`isSavedPreset` (`use-saved-presets.ts:42`) today tests every declared string +field with `typeof ... === "string"` (and `kind` against its two literals) and +rejects any object missing one; returning +`false` causes `readServerPresets` to `filter` the preset out entirely +(`use-saved-presets.ts:57`). Adding either field to that conjunction unchanged +would delete every saved query the user already has, on the first page load +after the upgrade, silently. Therefore, for **both** new fields identically: + +- `isSavedPreset` SHALL treat a **missing** `milestone` or `preset` as valid. +- A missing value, or one present but not a `string`, SHALL be read as `""`. A + legacy preset SHALL NOT be dropped, and SHALL behave exactly as it did before + this feature. + +#### Why `preset` is here, and why this feature is what forces it + +Without it, this feature would ship a fresh instance of the exact bug it exists +to fix. + +`useGitLabPageState` feeds the search hook +`preset: selection.source === "preset" ? selection.id : ""` +(`gitlab-page-client.tsx:184`). A **saved** query therefore always searches with +an empty preset, `pickFilter` returns `filter: ""`, and the request goes +upstream as `state=opened&scope=all`. Selecting a saved query silently discards +the scope it was created under — the same silent scope change described under +Why, arriving by a different route. + +That is a **pre-existing** defect: it is reachable today by saving a query with +only a project filter set. But this feature makes it *newly reachable, and +newly likely*, because `canSaveCurrent` is today +`committedQuery.trim().length > 0 || projectFilter.length > 0` and SHALL gain +`|| milestone.length > 0` (see "Saving a query: when it is offered, and what +it is called", and Scenario 12). A milestone-only query becomes saveable for the +first time, and the natural way +to create one is to pick **Assigned**, type a milestone, and save. Round-tripping +that as `scope=all` would be the feature contradicting its own premise. + +The cost is one optional-tolerant string field on a type this feature is already +widening, governed by the tolerance rule already written above for `milestone`. +The alternative considered and rejected was to narrow Scenario 12's claim and +record the loss under Out of scope; it was rejected because it knowingly ships +the defect the feature is about, and because the fix is smaller than the +paragraph explaining why it was skipped. + +#### The effective preset, defined once and used twice + +Both the search input and the save payload SHALL read the same derived value: + +``` +effectivePreset = + selection.source === "preset" ? selection.id + : (selected saved preset)?.preset ?? "" +``` + +- **Search.** `useGitLabPageState` SHALL pass `preset: effectivePreset` to the + search hook, replacing today's expression. For a sidebar preset this is + identical to today. For a saved query it resolves to the stored value, which + is `""` for every preset written before this feature — so **legacy saved + queries keep exactly today's behaviour** and nothing already persisted + changes meaning. +- **Save.** `onConfirmSave` SHALL persist `preset: effectivePreset`, so saving + while a saved query is selected captures the scope actually in effect rather + than resetting it to `""`. The definition is not recursive: it reads the + *stored* value of the currently selected preset and never re-derives it from + the value being written. + +#### Boundaries on the stored preset + +- **A custom query still wins.** `pickFilter` returns early when the committed + custom query is non-empty, ignoring `preset` entirely. A saved query carrying + both a custom query and a preset therefore searches by the custom query alone, + exactly as today. The stored preset is not lost, and applies again if the + custom query is later cleared. +- **An unresolvable preset degrades, it does not error.** `pickFilter` does + `presets.find((p) => p.value === preset)` and falls back to `filter: ""`. A + stored preset value that no longer exists in the preset list — renamed, + removed, or saved against the other kind — therefore behaves exactly as `""` + does: `state=opened&scope=all`, no warning, no dropped preset. The stored + value SHALL NOT be validated against the preset list at read time, and an + unknown value SHALL NOT cause `isSavedPreset` to reject the entry. +- **The preset list is chosen by the saved preset's `kind`.** Selecting a saved + query already sets `selection.kind`, and the search hook is handed the preset + list for that kind. An issue preset value is therefore never resolved against + the MR preset list. +- **The sidebar selection does not move.** Restoring a stored preset changes + what the *search* sends; it SHALL NOT change `selection`, which stays + `{source: "saved", id}` so the saved query remains the highlighted row. The + sidebar preset row SHALL NOT light up as though it had been selected + directly. + +## API surface + +### Upstream: GitLab `GET /issues` + +Read from GitLab's REST documentation for the list-issues endpoint, and quoted +here because three contract decisions below rest on it: + +- `milestone` — "The milestone title." String. Optional. +- `milestone_id` — separate attribute supporting `None`, `Any`, `Upcoming`, + `Started`. "`milestone` and `milestone_id` are mutually exclusive." +- `milestone` also accepts the predefined values `None` ("lists all issues with + no milestone") and `Any` ("lists all issues that have an assigned + milestone"), but GitLab flags those two on this attribute as slated for + deprecation in favour of `milestone_id`. +- `scope` — "`created_by_me`, `assigned_to_me` or `all`", and it "Defaults to + `created_by_me`." +- `state` — "Return `all` issues or just those that are `opened` or `closed`." + No default documented. + +Consequences, stated as contract: + +- kandev sends `milestone`, never `milestone_id`. `milestone_id` is out of + scope (see Out of scope). +- `None` and `Any` therefore reach GitLab and behave as GitLab defines them, + because kandev passes the value through untouched. This is emergent, not a + kandev feature: kandev SHALL NOT special-case them, document them in the UI, + or promise their behaviour beyond GitLab's. +- `Upcoming` and `Started` are `milestone_id`-only. Typing either SHALL be + forwarded as an ordinary title, matching only a milestone literally so named. + kandev SHALL NOT translate them onto `milestone_id`. + +### kandev: `GET /api/v1/gitlab/user/issues` + +Handled by `httpSearchUserIssues` +(`apps/backend/internal/gitlab/controller.go:283`). One new optional query +parameter, joining the existing `workspace_id`, `filter`, `custom_query`, +`page`, `per_page`: + +| Param | Type | Required | Default | Meaning | +|---|---|---|---|---| +| `milestone` | string | No | `""` | Milestone title to narrow by | + +Ordered evaluation. Each step runs only if the previous one did not return: + +1. The existing `review_requested` rejection runs **first and entirely + unchanged**, including its condition. That guard is nested inside + `if customQuery == ""`, so it rejects with 400 + `review_requested is not supported for issues` **only when `custom_query` is + empty**. This feature SHALL NOT widen it: + - `filter=review_requested` with an empty `custom_query`, with or without a + milestone, SHALL still be rejected with that 400. The milestone SHALL NOT + be evaluated, escaped, or logged. + - `filter=review_requested` with a **non-empty** `custom_query` SHALL keep + today's pass-through behaviour: no 400, the request proceeds down the + escape-hatch path below, and the milestone is folded into the custom query + like any other. Hoisting the guard out of the `customQuery` block to make + the rejection unconditional is **out of scope** and SHALL NOT be done here; + it would change behaviour that exists today independently of milestone. + Preset translation (`translateUserSearchFilter`) is nested in the same + `customQuery == ""` block and is likewise unchanged. +2. `workspace_id` resolution (`c.workspaceClient`) runs **after** step 1, as it + does today, and is unchanged. Its failure short-circuits before any milestone + handling. +3. `milestone` is trimmed of leading and trailing whitespace, using the + normative set defined under "Client function, and where trimming happens" + (Unicode `White_Space` together with U+FEFF) rather than `strings.TrimSpace`. +4. If the trimmed value is `""`, the request proceeds exactly as today: no + `milestone` key is added to any query string anywhere. +5. Otherwise the trimmed value is composed as below. It is escaped **exactly + once** on both paths, but by different mechanisms, because the two paths + carry the query in different representations. + +### How the milestone reaches the query builder + +The milestone travels as its **own parameter**, alongside `filter` and +`customQuery`, from the controller all the way to `buildIssueSearchQuery`. On the +preset path it is never smuggled inside either string. That requires widening the +two issue-search methods on the `Client` interface +(`apps/backend/internal/gitlab/client.go`), and this spec scopes that change in +rather than leaving the builder to discover it: + +| Method | Today | After | +|---|---|---| +| `ListIssues` | `(ctx, filter, customQuery string)` | `(ctx, filter, customQuery, milestone string)` | +| `ListIssuesPaged` | `(ctx, filter, customQuery string, page, perPage int)` | `(ctx, filter, customQuery, milestone string, page, perPage int)` | + +`buildIssueSearchQuery` takes the same third string +(`buildIssueSearchQuery(filter, customQuery, milestone string) string`), and +`PATClient.ListIssuesPaged` — its only caller — forwards what it was handed. The +`Service` wrappers `SearchUserIssues` and `SearchUserIssuesPaged` +(`service_search.go`) gain the parameter and pass it through unchanged. + +**A positional parameter, not an options struct.** The package's search methods +are uniformly positional, and `SearchMRsPaged(ctx, filter, customQuery, page, +perPage)` is deliberately the mirror image of `ListIssuesPaged`. Introducing an +options struct for issues alone would break that symmetry for the sake of one +added field; the single options struct in the package +(`TaskMRAutomationOptions`) belongs to an unrelated domain and is not a general +convention. The accepted cost is three adjacent string parameters, which a +transposed call would compile. + +Two properties bound that risk, and neither is "the call sites are all listed": + +1. **The arity change is the guard.** `milestone` is appended **after** + `customQuery`, so `filter` and `customQuery` keep their existing positions + and the arity goes 3 to 4 and 5 to 6. No existing call compiles unchanged. + The compiler therefore visits every call site in the repository, and a + silently reinterpreted *untouched* call is impossible. The residual risk is a + transposition introduced while editing a call site or writing a new one — a + normal review risk, not a hidden one. +2. **The blast radius is one package.** Every caller of these two methods is + inside `apps/backend/internal/gitlab/`, verified by search. Production call + sites are enumerated under Files touched. Test sites are **not** enumerated + there and are not zero; there are five, and the implementer will meet every + one as a compile error. Three are calls — + `pat_client_search_actions_test.go:57`, `pat_client_test.go:433` and + `pat_client_test.go:470` — and two are fake clients that *implement* the + interface: `service_issue_query_test.go:16` + (`captureIssueQueryClient.ListIssues`) and `service_mentions_test.go:19` + (`mentionRecordingClient.ListIssuesPaged`). The fakes are implementations + rather than calls, so they must be updated for the package to build at all; + updating only the three calls leaves the package uncompilable. + +**Scope guard.** `apps/backend/internal/github` declares its **own** `Client` +interface with identically named `ListIssues` and `ListIssuesPaged` methods +carrying today's signatures. It is a different interface in a different package +and SHALL NOT change. GitHub gains no milestone filter (see Out of scope), and +the name collision SHALL NOT be read as a shared contract. + +**Callers with no milestone pass `""`** and keep today's behaviour exactly. That +includes `service_issue_watches.go`, whose feature-level exclusion is unaffected: +issue watches gain no milestone capability, no DB column, and no dialog field. +Updating that call site is a mechanical consequence of the signature change, not +a feature, and the two SHALL NOT be conflated. + +Composing the milestone into the generated query rather than into the custom +query follows the `Client` interface's own `filter`-versus-`customQuery` split, +which has the same meaning on every method that takes the pair. Cited precisely, +because the exact wording differs by method: + +- `ListReviewRequestedMRs` (`apps/backend/internal/gitlab/client.go:37`) + documents `filter` as "an optional additional GitLab API filter (e.g. + `project_id=123` or `milestone=v1`)" and `customQuery` as the thing that + "replaces the entire generated query". **`milestone=v1` is the interface's own + example of a `filter` value — but it appears on this MR method**, whose + milestone support this spec puts out of scope. +- `ListIssues` (`apps/backend/internal/gitlab/client.go:107`) documents the same + pair more tersely — "filter is an optional additional API filter; customQuery, + when non-empty, replaces the entire generated query" — and does **not** name + milestone. + +So the precedent is the shared semantics of the pair, not a milestone example on +the issue method. A builder MAY align `ListIssues`'s doc comment with its +sibling's wording; nothing in this spec depends on that. + +Composition, by whether `custom_query` is present: + +- **`custom_query` empty** (preset path). The milestone is set as a + **structured key on the `url.Values` that `buildIssueSearchQuery` already + builds**, after the defaults and after `appendFilter` has merged the + preset-translated filter: + + 1. `state=opened` and `scope=all` are set as today. + 2. `appendFilter(values, filter)` merges the preset-translated filter over + them, exactly as today. + 3. `values.Set("milestone", )` — the raw trimmed title, not a + pre-escaped one. `values.Encode()` performs the single escape. + + The milestone SHALL NOT be string-concatenated onto the `filter` argument. + This ordering is load-bearing twice over: + + - **Appending before preset translation would lose the preset.** + `translateUserSearchFilter` returns `""` for any token containing `=` or + `&`, so `assigned_to_me&milestone=Next` would translate to nothing and + `scope=assigned_to_me` would silently vanish — the exact bug this feature + exists to fix. + - **Appending into the filter string at all would let the milestone be + silently dropped.** `appendFilter` does `url.ParseQuery(filter)` and returns + early on error; its own doc comment says "Unparseable filters are ignored". + A filter that fails to parse (for example `filter=%zz`, which + `translateUserSearchFilter` passes through untouched because it contains no + `=` or `&`) would take the concatenated milestone down with it, and the user + would get an unfiltered `state=opened&scope=all` listing with no indication + the milestone was discarded. Setting the key **after** `appendFilter` makes + the milestone structurally immune to that branch. See the Failure modes row + for an unparseable `filter`. + + For the Assigned preset and milestone `Next`, the query GitLab receives is + `scope=assigned_to_me&state=opened&milestone=Next` (parameter order is + whatever `url.Values.Encode` produces; only the set of key/value pairs is + specified). +- **`custom_query` non-empty** (escape-hatch path). The milestone is folded + into the custom query under these rules, in order: + 1. Parse `custom_query` with `url.ParseQuery`. + 2. If parsing **succeeds** and the result already contains a `milestone` key + (with any value, including empty), the custom query is used **unchanged** + and the `milestone` parameter is ignored. The custom query wins. + 3. Otherwise (parse succeeded without a `milestone` key, **or** parsing + failed) append `&milestone=` before any `#` fragment. Malformed + values are rejected; direct builders also keep defensive append. + This mirrors the established `appendLabelsToQuery` rule + (`apps/backend/internal/gitlab/service_watches.go:840`), including its + parse-failure branch, so the two folds behave identically. +- The escape-hatch path SHALL NOT re-introduce `scope=all` or `state=opened`. + Today's "custom query replaces the generated defaults" semantics are + preserved unchanged; this feature narrows nothing else and widens nothing. + +Escaping is a correctness **and** injection boundary. `milestone` arrives +already URL-decoded by the HTTP layer. A title containing `&`, `=`, `#`, `%`, +`+`, or a space MUST NOT introduce, remove, or alter any other query parameter +seen by GitLab. Escaping SHALL be applied **exactly once on each path** — a +value escaped twice (`Q3+%2F+Q4` becoming `Q3%2B%252F%2BQ4`) is a defect, and so +is a value not escaped at all. + +The mechanism differs by path because the two paths hold the query differently, +and the builder SHALL NOT unify them: + +| Path | Representation | Who escapes | Input to it | +|---|---|---|---| +| Preset (`custom_query` empty) | `url.Values` | `values.Encode()` | the **raw trimmed** title | +| Escape hatch (`custom_query` non-empty) | opaque string returned verbatim | `url.QueryEscape` | the **raw trimmed** title | + +Passing an already-escaped value to `values.Set` would double-escape it; passing +a raw value into the custom-query concatenation would under-escape it. Each path +takes the raw trimmed title and escapes once. + +Response shape is unchanged apart from the new optional `milestone` field on +each issue. Malformed values return HTTP 400 before lookup; upstream errors +continue through +`writeWorkspaceClientActionError(ctx, err, "issue search")`. + +### Client function, and where trimming happens + +**The trimming boundary is the commit, and it is normative.** The milestone has +a draft value and a committed value (Scenario 7, mirroring the custom-query +input). The **commit** trims: when a draft is committed by Enter or by blur, the +value written to the committed state SHALL be the draft with leading and +trailing whitespace removed, "whitespace" meaning the set defined immediately +below. Every consumer downstream therefore sees an already-trimmed string, and +there is exactly one place in the frontend that trims. + +**What "whitespace" means: one normative set, stated explicitly.** Neither +language's default trim is the contract, because they are not the same +function. Go's `strings.TrimSpace` uses `unicode.IsSpace`, which is the Unicode +`White_Space` property; JavaScript's `String.prototype.trim` removes ECMAScript +*WhiteSpace* + *LineTerminator*. Those two sets differ, and they differ in +**both** directions, so "both sides trim" is not a specification: + +| code point | `strings.TrimSpace` alone | `String.prototype.trim` alone | +|---|---|---| +| U+0085 (NEL) | trimmed | **kept** | +| U+FEFF (BOM) | **kept** | trimmed | + +The normative set for this feature is **Unicode `White_Space` together with +U+FEFF**. Trimming means removing leading and trailing runes in that set, and +nothing else. Neither side SHALL call its language default; each SHALL +implement that set: + +| Side | Implementation | +|---|---| +| Go (controller) | `strings.TrimFunc(s, func(r rune) bool { return unicode.IsSpace(r) \|\| r == '\uFEFF' })` | +| TypeScript (commit) | `s.replace(/^[\s\u0085]+\|[\s\u0085]+$/gu, "")` | + +`\s` in a JavaScript regular expression already covers ECMAScript *WhiteSpace* ++ *LineTerminator*, which includes U+FEFF; adding `\u0085` explicitly makes the +class exactly `White_Space` together with U+FEFF, matching the Go side. Each +expression is one line, and each SHALL live behind a single named helper on its +own side so the set is written once per language rather than re-derived at each +call site. + +These two expressions were measured against U+0009, U+000A, U+000B, U+000C, +U+000D, U+0020, U+0085, U+00A0, U+1680, U+2000, U+2028, U+2029, U+202F, U+205F, +U+3000, U+FEFF, U+200B, U+180E, U+2060 and U+00B7. They agree on every one: the +first sixteen are trimmed, the last four are preserved. That agreement is a +property a test SHALL assert on each side (Scenario 27), not an assumption the +reader is asked to take on faith. + +**Characters deliberately NOT trimmed**, because they are whitespace under +neither definition and a milestone title may legitimately contain them: U+200B +(zero-width space), U+2060 (word joiner), and U+180E (Mongolian vowel +separator, category `Cf` since Unicode 6.3). A title consisting only of one of +these is non-empty, is sent, and matches only a GitLab milestone literally so +named. + +**How that is achieved, because the shared hook does not trim by itself.** +`useCommittedQuery` exposes `commit()`, which takes no argument and copies the +draft into the committed value verbatim. It does not trim, and it SHALL NOT be +taught to: the custom-query input shares that hook and its behaviour is out of +scope. The milestone input SHALL therefore commit by calling +`setImmediate(trimGitLabMilestone(draft))` rather than `commit()`. `setImmediate` writes the +draft and the committed value together, which gives both halves of the rule: + +- the committed value is trimmed at the instant of commit, as required above; and +- the visible input is normalised to the same trimmed text on Enter or blur. + That is intended, not a side effect. An input still reading `" Next "` while + the committed value is `"Next"` invites a second, pointless commit. + +Trimming SHALL happen only on commit, never per keystroke: trimming the draft on +every change would make a leading space impossible to type through. + +**Every change to the committed milestone resets pagination, in the same +handler.** The rule is stated over the value, not over one gesture, because the +committed milestone changes on four paths and only one of them is a commit: +pressing Enter or blurring the input; selecting a saved query (restore); +selecting a sidebar preset (clear); and deleting the selected saved query +(clear). Each SHALL call the search hook's `setPage(1)` from inside the same +event handler that changes the milestone, so React batches both state updates +into one render. + +The three non-commit paths need saying because the existing page-reset effect +does not cover them. Its dependencies are `[preset, customQuery, kind]`, and a +saved query can differ from the current state **only** in its milestone — same +effective preset, same empty custom query, same kind — in which case the effect +never fires, the page stays where it was, and a milestone-narrowed fetch is +issued for page 3 of a result set that now has one page. The same holds for the +delete path when the fallback preset happens to equal the effective preset +already in force. Resetting in the handler is uniform across all four paths and +does not depend on which of them the effect would have caught anyway. + +The mechanism is specified because the obvious alternative is wrong. +`useGitLabSearch` already resets the page from an effect whose dependencies are +`[preset, customQuery, kind]`. Adding `milestone` to that array would satisfy +Scenario 6 and break Scenario 7: the reset effect and the fetch effect run in the +same commit phase, so the fetch would fire once for the **previous** page +carrying the **new** milestone, and only then fire again for page 1. `requestSeq` +discards the stale response, but two requests would have left the browser, and +Scenario 7 requires exactly one. Committing and resetting together yields one +render and therefore one request, whatever page the user was on. + +The existing `customQuery` entry in that dependency array is left exactly as it +is. Its double-fetch predates this feature, fixing it is out of scope, and the +resulting asymmetry is deliberate: the milestone SHALL NOT be moved into that +effect for consistency's sake. + +**The commit also invalidates the accumulated project list.** The project +dropdown is not populated from an endpoint; `useKnownProjects` accumulates the +`project_path` values seen across pages and resets only when its `resetKey` +changes. That key is built at `gitlab-page-client.tsx:147` from four segments — +`selection.kind`, `selection.source`, `selection.id`, and the trimmed committed +query — and the milestone is not among them. Left alone, narrowing to a +milestone would keep offering every project the user had browsed before, +including ones with no issue in that milestone; selecting one would return an +empty list from a dropdown that claimed to list what is there. + +The **committed** milestone SHALL therefore be appended as a fifth segment of +`resetKey`. The draft SHALL NOT be, or the list would reset on every keystroke. +The segments are joined with `:` and a milestone title may itself contain `:`, +which is harmless here: `resetKey` is only ever compared for equality and is +never parsed back into segments, so a colon cannot make two distinct states +collide — it can only ever be part of one state's identity. + +**That alone is not enough, and the spec says so rather than leaving the gap for +the implementer to discover.** `recordForKey` clears the accumulator when the key +changes and then records whatever it was handed **in that same call** +(`use-known-projects.ts:30-46`). On the render where the milestone commits, the +key is already new but `search.rawItems` still holds the **previous** result set, +because `fetchData` sets `loading: true` while spreading the old state and +therefore keeps the old items on screen (`use-gitlab-search.ts:99-100`). The +effect would clear the set and immediately refill it with the stale projects +under the new key; when the real results arrived, `recordForKey` would only +**add** to them, since the key had stopped changing. The reset would have +accomplished nothing. + +So `useProjectOptions` SHALL pass an **empty** project list while a search is in +flight, deriving `pageProjects` from `search.rawItems` only when +`search.loading` is false. That yields the right behaviour in both directions: +on a key change the clear lands with nothing to refill and the new results +populate a genuinely empty set; on ordinary pagination, where the key does not +change, the in-flight empty list clears nothing and the next page's projects are +still accumulated on top of the previous ones. + +This also repairs the identical staleness for `committedQuery`, which shares the +key and the effect. That is a consequence, not a second feature: there is one +accumulator and one call site, and the milestone cannot be made correct while +leaving the neighbouring segment broken. It is distinct from, and does not +touch, the `customQuery` double-fetch disclaimed above. + +`useProjectOptions` takes four positional parameters today and would take six. +Six exceeds the `max-params` cap of 5 (`apps/web/eslint.config.mjs:47`, warn, +the same warn-level family as the `max-lines` cap cited under Constraints), so it +SHALL take a single options object instead: +`{ selection, committedQuery, milestone, items, loading, projectFilter }`. The +signature change is free — the function is being moved into +`use-gitlab-page-state.ts` by the extraction specified under Constraints, and it +has exactly one call site. (Scenario 29.) + +**Deleting the selected saved query clears the milestone.** `onDeleteSaved` +already resets `customQuery` and `projectFilter` when the deleted query is the +selected one, and it SHALL reset the milestone — draft and committed, both to +`""` — in that same branch and the same batch. Without it, deleting a saved +query whose milestone was `Next` would leave the milestone filter applied with +nothing on screen explaining why the list is narrow. + +Deleting a saved query that is **not** the selected one SHALL change no filter +state and SHALL trigger no refetch, exactly as today. (Scenario 30.) + +That single rule settles three things that would otherwise disagree: + +- `searchUserIssues` (`apps/web/lib/api/domains/gitlab-api.ts:194`) accepts an + optional `milestone` and SHALL set the `milestone` search param **only** when + the value is a non-empty string — matching how it already treats `filter` and + `customQuery`, and requiring no trim of its own. Because the committed value + is already trimmed, a whitespace-only entry is `""` by the time it reaches + here, the param is omitted, and the outgoing + `/api/v1/gitlab/user/issues` URL is byte-identical to today's. **This is what + makes Scenario 3 true.** Had trimming lived only in the controller, a + whitespace-only entry would have produced a frontend URL carrying + `milestone=++` and Scenario 3 would be false. +- The value persisted into a `SavedPreset` is the **committed (trimmed)** value, + never the raw draft. Two saved queries whose titles differ only in outer + whitespace are therefore the same query, and a saved preset always reproduces + the result set that was on screen when it was saved (Scenario 12). +- The controller **also** trims (API surface, step 3), applying the same + normative set. That is not redundant: it is the boundary for direct API + callers, who never pass through the frontend. The two trims agree **because + each implements the set above**, not because the two languages happen to + agree — they do not, and the earlier draft of this spec was wrong to say they + did. Given that shared set, a value that survives one survives the other + unchanged, and trimming an already-trimmed string is a no-op, so the double + application is idempotent rather than a double-normalisation. + +A request with no milestone SHALL produce a byte-identical URL to today's. + +## Selection, ordering and counts + +- **Result ordering is GitLab's and is not changed.** The milestone parameter + SHALL NOT introduce a sort, an `order_by`, or a re-sort of the returned page. + Two requests differing only in milestone SHALL preserve upstream order within + their own result sets. kandev states no tiebreak for the live path because it + imposes no ordering there. +- **The badge count.** `displayedCount` in + `apps/web/app/gitlab/gitlab-page-client.tsx` currently shows + `search.items.length` when `projectFilter` is set and `search.total` + otherwise, because the project dropdown narrows client-side and would + otherwise read "47" beside three rows. The milestone filter narrows + **server-side**, so `search.total` is already the milestone-narrowed total. + The milestone SHALL NOT be added to that condition: with a milestone set and + no project filter, the badge SHALL show `search.total`. With both set, the + existing project-filter branch continues to win and the badge shows + `search.items.length`. +- **Pagination.** `search.total` reflects the milestone-narrowed count, so page + count is exact. Committing a milestone change SHALL reset the view to page 1 + (see Scenario 6). + +## Mock and e2e + +`MockClient` is selected by `newClient` when the environment variable +**`KANDEV_MOCK_GITLAB`** is `"true"` (`apps/backend/internal/gitlab/factory.go:37`). +It is not selected by `KANDEV_E2E_MOCK`. E2E runs get the mock only because the +`e2e` profile in `profiles.yaml` sets `KANDEV_MOCK_GITLAB: e2e: "true"` as a +separate entry alongside `KANDEV_E2E_MOCK`. Anyone running the mock outside that +profile SHALL set `KANDEV_MOCK_GITLAB` explicitly; setting `KANDEV_E2E_MOCK` +alone does not produce a `MockClient`. + +Today the mock has **three** deficiencies that block a demonstration of this +feature. `MockClient.ListIssues` **ignores both `filter` and `customQuery` +entirely** and returns every seeded issue in Go map-iteration order; and +`MockClient.ListIssuesPaged` — which is the method the controller actually calls, +delegating to `ListIssues` — **ignores `page` and `perPage` entirely**, returning +the whole list with `TotalCount: len(issues)` for every requested page. All three +are in scope for the mock path only: + +- `MockClient` receives the same three search inputs as any other client + (`filter`, `customQuery`, `milestone`) and receives them **as the controller + passed them**: it never calls `buildIssueSearchQuery`, so it sees the raw + `custom_query` from the request, not a folded one. It SHALL resolve an + **effective milestone** from those three, by the first rule that applies: + 1. if `customQuery` parses and carries a `milestone` key, the effective + milestone is that key's **first** value, **including when that value is + empty**. First, not last and not all of them: `url.Values.Get` returns the + first, and the mock SHALL use `Get` so that a hand-written + `milestone=Old&milestone=Next` picks `Old` (Scenario 31); + 2. otherwise, if the `milestone` parameter is non-empty, it is the effective + milestone; + 3. otherwise, if `filter` parses and carries a non-empty `milestone` key, its + **first** value is the effective milestone, on the same `Get` rule — + retained only so a Go caller that hand-builds a filter string still behaves + sensibly; the controller never puts one there; + 4. otherwise the effective milestone is empty. + A `customQuery` carrying a `milestone` key therefore wins over the parameter, + reproducing the controller's fold precedence (Scenario 4) without the mock + having to replicate the fold itself. Rule 1 deliberately lets an **empty** + key win rather than falling through: on the live path an empty key likewise + suppresses the fold and reaches GitLab as an empty `milestone` (Scenario 21), + so a mock that fell through to the parameter would filter where GitLab would + not. +- With a non-empty effective milestone, `ListIssues` SHALL return only seeded + issues whose `Milestone` equals it, compared as an exact case-sensitive + string. With an empty one, every seeded issue is returned, as today. +- `MockClient.ListIssuesPaged` SHALL forward the milestone to `ListIssues` + rather than resolving it a second time, so the two can never disagree. +- `MockClient.ListIssues` SHALL return issues in a **deterministic order**: + `ProjectPath` ascending, then `IID` ascending. This is a named tiebreak on + named fields; the pair is unique because `MockClient` keys its issue map on + exactly `{Project, IID}`. Without it a multi-issue assertion is flaky by + construction. +- `MockClient.ListIssuesPaged` SHALL page over the filtered, sorted result. + Given the ordered list produced above, it SHALL return the slice starting at + `(page - 1) * perPage` of length at most `perPage`, and SHALL set + `TotalCount` to the length of the **whole filtered list**, not the length of + the returned slice. A `page` beyond the end yields an empty `Issues` slice + with the same `TotalCount`; `page` and `perPage` are echoed back as given. + This is scoped in rather than excluded because the spec asserts an exact badge + count (Scenario 9) and a page reset (Scenario 6), and neither is demonstrable + against a mock that returns every issue for every page. +- **Paging bounds are defined, so the mock cannot panic.** Over HTTP, + `paginationFromQuery` already clamps `page` to `>= 1` and `perPage` to `> 0`, + so the degenerate values below are unreachable from the controller. But + `ListIssuesPaged` is also called directly from Go tests, where nothing clamps + first, and a naive `(page - 1) * perPage` slice would panic on a negative + index. `MockClient.ListIssuesPaged` SHALL therefore treat `page < 1` as `1` + and `perPage < 1` as "return no issues", computing the start offset only + after that normalisation, and SHALL clamp the end offset to the length of the + filtered list. `TotalCount` SHALL be the full filtered count in every one of + these cases, and the `Page` and `PerPage` echoed back SHALL be the normalised + values actually used, not the raw arguments — so a caller can always tell + which window it received. +- Every filter key other than `milestone` SHALL continue to be ignored by the + mock. This feature does not teach it `state`, `scope`, `labels`, or + `assignee_username`. +- `POST /mock/issues` gains no new field of its own: it already decodes + `[]Issue`, so `milestone` becomes seedable the moment the model carries it. +- These are properties of `MockClient` only. `PATClient` SHALL NOT gain any + local filtering or sorting; GitLab remains the only thing that filters real + results. + +## Presentation + +- The milestone control renders in the toolbar only when the Issues view is + selected. In the Merge requests view it SHALL NOT render **at all** — not + hidden by a CSS class, not rendered disabled. The MR view's toolbar DOM is + byte-for-byte what it is today (Scenario 10). +- It is a single-line text input, sitting alongside the existing project + dropdown inside `ListToolbar`'s `filter` slot. The shared + `IntegrationListToolbar` (used by GitHub and Linear) SHALL NOT change its + props or markup; GitLab composes both controls into the one existing + `filter` node. + +**`ListToolbar`'s new props, specified because there is only one call site.** +`ListToolbar` is rendered once (`gitlab-page-client.tsx:301`) for both views, so +the Issues-only rule above cannot be expressed by rendering a different +component; it is a prop. Five props are added, and all five are **REQUIRED** — +optional props with defaults would let the MR view silently acquire a milestone +input if a future call site forgot one: + +| Prop | Type | Meaning | +|---|---|---| +| `showMilestoneFilter` | `boolean` | The page passes `selection.kind === "issue"`. When `false`, the input subtree is not rendered. | +| `milestone` | `string` | The **draft** value; what the input displays. | +| `committedMilestone` | `string` | The committed value. Used **only** to decide whether a blur commits, mirroring how `committedQuery` is used for the custom-query input. | +| `onMilestoneChange` | `(value: string) => void` | Draft setter, fired per keystroke. | +| `onCommitMilestone` | `() => void` | Promotes draft to committed. | + +The milestone input implements Enter-to-commit and blur-to-commit itself, on the +same terms as the custom-query input (Scenario 7). `committedMilestone` exists +for the same single reason `committedQuery` does in +`integration-list-toolbar.tsx`: to compute dirtiness. The shared toolbar's +existing "press Enter" hint is driven by `customQuery !== committedQuery` and +SHALL remain scoped to the custom-query input; the milestone input SHALL NOT +render a hint of its own. Adding a second hint would mean two competing +affordances in one toolbar row for what is, to the user, one habit. + +- Its placeholder is an example milestone title and stays an untranslated + literal: a translated example stops being a usable example. Because a + placeholder is a **JSX attribute**, it is seen by `i18next/no-literal-string` + — which is jsx-only and an error tree-wide — so the marker SHALL be + `// eslint-disable-next-line i18next/no-literal-string -- example milestone title`, + following `components/gitlab/watch-dialog.tsx:132,148,173`. It SHALL NOT be + `// i18n-exempt:`, which addresses a different tool + (`scripts/check-nonjsx-copy.mjs`, covering positions the eslint rule cannot + see) and would leave the eslint error standing and CI red. The two markers are + not interchangeable; see Saving a query for the contrasting case where + `// i18n-exempt:` is the correct one. Its visible label and accessible name go + through `t()`. Note that the nearest neighbour in this same toolbar, + `list-toolbar.tsx:60`, is `t("gitlab:allProjects")` — a translated + placeholder — so the exemption rests on the example-value argument alone and + not on any local precedent. +- Test id: `gitlab-milestone-filter`. +- An issue row SHALL render its milestone title as a plain text chip in the + same chip row as the existing labels (`issue-list.tsx`) when `milestone` is + non-empty, and render nothing there when it is empty. The chip is text only: + no link, no icon, no click behaviour, no interaction with the milestone + filter. +- **Order is specified, and it is specified against the whole row, not just + against the chips.** That row is not chips alone: in `issue-list.tsx` the same + flex container holds, in order, `project_path#iid`, a separator, the + "by author ... ago" text, and only then the label chips rendered by + `IssueLabels`. The milestone chip SHALL be inserted **immediately before the + first label chip** — after the existing author and time metadata, not at the + head of the row. It leads the chips; it does not displace the row's + identifying text. Within the chips the single higher-level grouping leads, + since a row has at most one milestone and may have many labels. +- The milestone chip SHALL carry its own stable test id, + `gitlab-issue-milestone`, distinct from the label chips', so that this + position is assertable in a test rather than eyeballed: the requirement is + "identifiable in the DOM", not "looks different". +- The existing `labels.slice(0, 4)` cap on rendered label chips is unchanged, + and the milestone chip SHALL NOT count against it: a row with a milestone and + four labels renders five chips. + +## Failure modes + +| Condition | Behaviour | +|---|---| +| Milestone empty or whitespace-only | Treated as absent. No `milestone` key sent. Identical to today. | +| Milestone matches no issues | Empty result set, `total_count` 0, existing empty state rendered. Not an error. | +| Milestone set while `filter=review_requested` | Existing 400 `review_requested is not supported for issues`, unchanged. Milestone never evaluated. | +| `custom_query` already contains `milestone` | Custom query wins; the `milestone` parameter is silently ignored. No error, no warning. | +| `custom_query` unparseable by `url.ParseQuery` | HTTP 400; no GitLab call. | +| `filter` unparseable by `url.ParseQuery` (e.g. `filter=%zz`) | Today `appendFilter` returns early and the whole filter is silently dropped. That is unchanged and out of scope. The **milestone still applies**, because it is set on `url.Values` after `appendFilter` returns rather than concatenated into the `filter` string. A malformed filter therefore degrades to "no preset, milestone honoured", never to "milestone silently lost". | +| Milestone title contains `&`, `=`, `#`, `%`, `+`, or spaces | Escaped once; reaches GitLab as one parameter value. No other parameter is added or altered. | +| GitLab returns an error | Existing `writeWorkspaceClientActionError` path, unchanged. | +| GitLab returns `"milestone": null` on an issue | Decodes to `""`. No chip rendered. | +| GitLab is not configured / not connected | Existing connect notice; no search fires. Unchanged. | +| Legacy saved preset without a `milestone` key | Loads normally with `milestone: ""`. Never dropped. | + +## Persistence guarantees + +- The milestone filter is **not** persisted as page state. Reloading `/gitlab` + restores the default selection with an empty milestone, exactly as the + project filter and custom query behave today. +- A milestone is persisted only when the user explicitly saves a query, as part + of that `SavedPreset`, through the existing `gitlab_saved_presets` user + setting and its existing `createQueuedUserSettingsSync` writer. +- Saved-preset writes remain last-write-wins across concurrent tabs, unchanged. + This feature adds a field to the persisted object; it does not add + reconciliation, merging, or conflict detection. + +## Idempotency, retry and concurrency + +- `GET /api/v1/gitlab/user/issues` is read-only. Repeating a request with the + same parameters is safe and SHALL produce the same upstream query string + byte-for-byte. There is no request id, no dedup, and no server-side caching. +- The custom-query fold is idempotent **when the custom query parses**: + applying it to a query that already carries `milestone` returns that query + unchanged. When the custom query does **not** parse, the fold is append-only + by design — it mirrors `appendLabelsToQuery`, whose parse-failure branch + appends unconditionally — so the "already has the key" guard can never fire + and a second application would append a second `&milestone=`. That is + unreachable in practice, and structurally rather than luckily so: every + request re-folds from the raw `custom_query` it was handed, and a folded query + is never fed back in as input. A retry therefore cannot accumulate + `&milestone=` twice on either branch. +- Two callers reading the same workspace concurrently do not interact; the + handler holds no per-request mutable state. +- **Two callers, same row.** The only row this feature writes is the + `gitlab_saved_presets` user setting, and it writes it only on an explicit + save or delete. Two tabs saving concurrently resolve last-write-wins: the + second write replaces the whole preset array, so a preset created in the + first tab and not yet visible to the second is lost. This is exactly today's + behaviour for `customQuery` and `projectFilter`, and this feature SHALL NOT + change it. Adding `milestone` to the object does not introduce a new + conflict surface, and no merge, version check, or reconciliation is added. + Search itself is read-only and has no row to contend for. +- **In-flight ordering in the browser.** `useGitLabSearch` already discards + stale responses through its `requestSeq` counter. The milestone SHALL flow + through that same `fetchData` path, so a slow response for an earlier + milestone value can never overwrite a newer one. It SHALL NOT be applied by a + second, parallel fetch. +- **Mock concurrency.** `MockClient.ListIssues` filters and sorts while holding + the existing `c.mu` lock, so a concurrent `SeedIssue` cannot observe or + produce a torn slice. + +## Defaults and boundaries + +- **Default.** The milestone is `""` everywhere it appears: the API parameter + when omitted, the toolbar input on first render, a legacy `SavedPreset`, and + an issue with no milestone. `""` always means "do not filter" on the request + side and "render nothing" on the display side. There is no sentinel value and + no null. +- **Whitespace.** Leading and trailing whitespace is trimmed before the + emptiness test and before escaping, so `" "` is absent and `" Next "` + filters on `Next`. **"Whitespace" here means one specific set: Unicode + `White_Space` together with U+FEFF**, defined normatively under "Client + function, and where trimming happens". It is deliberately neither + `strings.TrimSpace` nor `String.prototype.trim`, which disagree with each + other in both directions; both sides implement the stated set instead. + Interior whitespace is preserved: `Q1 Planning` is one title, not two tokens. + The trim happens **at commit** in the frontend and **again** in the controller + for direct API callers. Because the frontend commits a trimmed value, the + client function never sees a whitespace-only string and omits the parameter + outright — which is what makes the URL in Scenario 3 byte-identical rather + than merely equivalent. **Because both implement the same set**, trimming an + already-trimmed value is a no-op and the two trims cannot disagree; that + conclusion follows from the shared set, not from the two languages' defaults. +- **Case.** Not folded. Matching is GitLab's, which treats milestone titles as + case-sensitive; kandev SHALL NOT lower-case, upper-case, or otherwise + normalise the value. The `MockClient` comparison is likewise exact and + case-sensitive, so the mock and the live path agree. +- **Length.** No minimum and no maximum is imposed by kandev. A title longer + than GitLab accepts produces GitLab's own response, surfaced through the + existing error path. There is no client-side `maxLength` on the input. +- **Character set.** Any Unicode string is accepted, including one that is + entirely punctuation or entirely non-Latin. The only transformation applied + is a single `url.QueryEscape`. +- **Multiplicity.** Exactly one milestone; no attempt is made to OR several + titles together. A repeated key resolves to the **first** value at every place + kandev has to choose, because every such reader is `url.Values.Get`, which + returns the first. That rule covers three distinct places, and they agree: + 1. a repeated `milestone` parameter on the kandev HTTP request (controller); + 2. a repeated `milestone` key inside `custom_query`, when the controller's + fold inspects it to decide whether to append (see How the milestone reaches + the query builder); + 3. a repeated `milestone` key inside `custom_query` or `filter` when + `MockClient` resolves its effective milestone. + + Only the mock ever *filters* on the chosen value. On the live path the + non-empty custom query is forwarded upstream **unchanged**, both pairs + included, and GitLab applies its own resolution — kandev inspects the + duplicate to decide whether to fold, and then does not have to choose + (Scenario 31). +- **Page size and pagination bounds.** Unchanged. `SEARCH_PAGE_SIZE` stays 25, + and the milestone does not alter `page` or `per_page` handling beyond the + reset to page 1 on commit. + +## Scenarios + +1. **Milestone composes with a preset.** GIVEN the Issues view with the + **Assigned** preset selected and no custom query, WHEN the user commits the + milestone `Next`, THEN the request to GitLab carries the pairs + `scope=assigned_to_me`, `state=opened` and `milestone=Next`, and the list + shows only issues assigned to the user in milestone `Next`. + +2. **Milestone does not silently reset scope.** GIVEN the same state as + Scenario 1, WHEN the milestone is committed, THEN `scope` SHALL NOT be + absent from the upstream query and SHALL NOT be `created_by_me`. + +3. **Empty milestone changes nothing.** GIVEN the Issues view with any preset, + WHEN the milestone input is empty, or contains only whitespace **and has + been committed**, THEN the committed value is `""`, the client omits the + `milestone` search param entirely, and the outgoing + `/api/v1/gitlab/user/issues` URL is byte-identical to the one produced + before this feature — as is the upstream GitLab query. + + **And at the other boundary.** GIVEN a caller that bypasses the frontend and + requests `/api/v1/gitlab/user/issues?milestone=%20%20`, WHEN the controller + trims, THEN the value is empty, no `milestone` pair is added to the upstream + query, and the response is identical to the same request with the parameter + omitted. + +4. **Custom query wins.** GIVEN a custom query of + `state=closed&milestone=Old`, WHEN the user also commits the milestone + `Next`, THEN the upstream query is exactly `state=closed&milestone=Old` and + no second `milestone` pair is present. + +5. **Custom query without a milestone is extended.** GIVEN a custom query of + `state=closed`, WHEN the user commits the milestone `Next`, THEN the + upstream query carries both `state=closed` and `milestone=Next`, and SHALL + NOT carry `scope=all`. + +6. **Any change to the committed milestone resets pagination.** GIVEN the user + is on page 3 of an unfiltered issue list, WHEN a milestone is committed, THEN + the view requests page 1 and the pagination control reports page 1. GIVEN the + user is on page 3 and selects a saved query that differs from the current + state **only** in its stored `milestone` — same effective preset, same empty + custom query, same kind, so the existing `[preset, customQuery, kind]` reset + effect does not fire — THEN the view still requests page 1. GIVEN the user is + on page 3 with a committed milestone and selects a sidebar preset, or deletes + the selected saved query, THEN the milestone clears and the view requests + page 1 in the same render. + +7. **Commit semantics match the custom-query input.** GIVEN the milestone input + has focus, WHEN the user types without pressing Enter, THEN no request is + issued; WHEN the user presses Enter, or blurs the input while its draft + differs from the committed value, THEN exactly one request is issued for the + new value. + +8. **Special characters cannot inject a parameter.** GIVEN the milestone title + `Q3 & Q4=x`, WHEN it is committed, THEN GitLab receives one `milestone` + parameter whose decoded value is exactly `Q3 & Q4=x`, and the query carries + no additional or altered parameter. + +9. **No match is an empty list, not an error.** GIVEN a milestone title no + issue carries, WHEN it is committed, THEN the list renders its empty state, + the badge reads 0, and no error banner is shown. + +10. **Merge requests are untouched.** GIVEN the Merge requests view, WHEN it is + displayed, THEN no milestone control renders, and the request to + `/api/v1/gitlab/user/mrs` is byte-identical to today's. + +11. **Switching selection clears the milestone.** GIVEN a committed milestone + on the Issues view, WHEN the user selects a different sidebar preset, THEN + both the milestone draft and its committed value become `""` and the + refetch carries no `milestone`. WHEN the user instead selects a saved + query, THEN both become that preset's stored `milestone` (which is `""` for + one saved before this feature), and the query sent for it uses that + preset's stored `preset` as its scope. WHEN the user switches to the Merge + requests view, THEN the milestone clears on that same path — the control is + not merely unrendered while a live committed value persists behind it — so + switching back to Issues shows an empty input over an unnarrowed list. + +12. **Saving a query captures the trimmed milestone.** GIVEN a milestone + committed from the draft `" Next "` and an otherwise empty custom query + and project filter, WHEN the user saves the current query, THEN saving is + offered — `canSaveCurrent` is true **only because it gained the milestone + term**, since the other two inputs are empty — the persisted `SavedPreset` + carries `milestone: "Next"` (the committed value, never the raw draft) and + `preset` set to the effective preset in force at save time, and + re-selecting the saved query reproduces exactly the result set that was on + screen at save time. + +13. **Legacy saved queries survive.** GIVEN a `gitlab_saved_presets` setting + written before this feature, containing objects with neither a `milestone` + nor a `preset` key, WHEN the page loads, THEN every one of those presets is + listed and selectable, each with `milestone: ""` and `preset: ""`, and none + is filtered out. WHEN one of them is selected, THEN the scope sent is + whatever the sidebar preset happens to be, exactly as today — the empty + stored `preset` restores nothing and changes nothing. + +14. **Milestone is visible on the row, in a specified position.** GIVEN an + issue in milestone `Next` that also carries labels, WHEN it renders in the + list, THEN the row shows a `Next` chip carrying the milestone test id, and + that chip appears **after** the `project_path#iid` and "by author ... ago" + metadata and **before** the first label chip in DOM order; GIVEN an issue + with no milestone, THEN the row shows no milestone chip and the label chips + are unmoved. + +15. **The mock demonstrates the filter.** GIVEN seeded issues where only some + carry `milestone: "Next"`, WHEN the milestone `Next` is committed against + the mock provider, THEN only the matching issues are returned, in + `ProjectPath` then `IID` ascending order, and repeating the request returns + the same sequence. + +16. **The mock pages the filtered set.** GIVEN 30 seeded issues of which 26 + carry `milestone: "Next"`, WHEN the milestone `Next` is requested with + `per_page=25`, THEN page 1 returns the first 25 in `ProjectPath` then `IID` + order with `total_count` 26, page 2 returns the remaining 1 with + `total_count` 26, and page 3 returns an empty list with `total_count` 26 + and no error. + +17. **Degenerate paging arguments do not panic the mock.** GIVEN a Go caller + invoking `MockClient.ListIssuesPaged` directly with `page` 0 or negative, + WHEN the call is made, THEN it behaves as `page` 1; and with `perPage` 0 or + negative it returns no issues. In both cases `TotalCount` is the full + filtered count and the echoed `Page`/`PerPage` are the normalised values + used. + +18. **Commit normalises the visible input.** GIVEN the milestone input + containing the draft `" Next "`, WHEN the user presses Enter or blurs the + field, THEN the committed value is `Next` and the input itself also reads + `Next`; and WHEN the draft is only whitespace, THEN both the committed value + and the input become empty. GIVEN the user is mid-edit and has typed a + leading space without committing, THEN that space is preserved in the draft. + +19. **A malformed custom query is rejected at the request boundary.** GIVEN + `%zz`, WHEN `Next` is committed, THEN HTTP 400 is returned, no upstream call + and the response reports an invalid `custom_query`. + +20. **A malformed filter cannot take the milestone down with it.** GIVEN + `filter=%zz` and an empty custom query, WHEN the milestone `Next` is + committed, THEN the upstream query carries `milestone=Next` alongside + `state=opened` and `scope=all`, and the unparseable filter is ignored + exactly as it is today. + +21. **An empty milestone key in a custom query still wins.** GIVEN a custom + query of `state=closed&milestone=`, WHEN the milestone `Next` is committed, + THEN the custom query is used unchanged, `Next` is not appended, and GitLab + receives a single `milestone` parameter whose value is empty. + +22. **GitLab's predefined values pass through untranslated.** GIVEN the + milestone `None`, WHEN it is committed, THEN GitLab receives + `milestone=None` and kandev applies no special handling; likewise for `Any`. + GIVEN `Upcoming`, THEN it is forwarded as an ordinary title on `milestone`, + never on `milestone_id`, and matches only a milestone literally so named. + +23. **A legacy preset with a non-string milestone survives.** GIVEN a + `gitlab_saved_presets` entry whose `milestone` is a number, `null`, or an + object, WHEN the page loads, THEN `isSavedPreset` accepts the preset, its + milestone reads as `""`, and it is not filtered out of the list. + +24. **The mock honours custom-query precedence.** GIVEN seeded issues in + milestones `Next` and `Old`, WHEN `MockClient` is called with a custom query + of `milestone=Old` and the milestone parameter `Next`, THEN only the `Old` + issues are returned; and WHEN it is called with an empty custom query and + the parameter `Next`, THEN only the `Next` issues are returned. + +25. **The suggested save name follows the precedence ladder.** GIVEN no custom + query, no project filter, and a committed milestone `Next`, WHEN the save + dialog opens, THEN the suggested name is `Next`. GIVEN a project filter + `group/app` in addition, THEN it is `In group/app`. GIVEN a committed custom + query in addition, THEN it is that custom query. GIVEN none of the three, + THEN it is `Saved query`. + +26. **A milestone chip does not displace a label chip.** GIVEN an issue in + milestone `Next` carrying six labels, WHEN it renders, THEN the row shows + the milestone chip, then four label chips, then the `+2` overflow + indicator — the milestone does not consume one of the four label slots. + +27. **The two trims agree on the normative set.** GIVEN the twenty code points + listed under "Client function, and where trimming happens", WHEN each is + passed alone through the Go helper and through the TypeScript helper, THEN + the two agree on every one: the first sixteen yield `""` and the last four + (U+200B, U+180E, U+2060, U+00B7) are returned unchanged. GIVEN the string + U+0085 + `Next` + U+FEFF, THEN both yield `Next`. GIVEN `" Q1 Planning "`, + THEN both yield `Q1 Planning` with the interior space intact. This SHALL be + asserted by a test on each side, and the Go test SHALL additionally assert + that the helper is not `strings.TrimSpace` by showing U+FEFF alone trims to + `""`, which `strings.TrimSpace` does not do. + +28. **A saved query restores its own scope.** GIVEN the sidebar preset + **Assigned** is selected and a milestone `Next` is committed, WHEN the user + saves that query, then selects the sidebar preset **Created**, then selects + the saved query, THEN the request carries `scope=assigned_to_me` — the + scope stored with the saved query, not the **Created** scope that was + selected a moment earlier — and the sidebar selection remains on the saved + query rather than moving to **Assigned**. GIVEN instead a saved query whose + stored `preset` is a value no longer present in the issue preset list, THEN + the request carries an empty `filter`, the list renders normally, and no + error is surfaced. + +29. **Changing the milestone does not strand the project dropdown.** GIVEN the + Issues view showing results from three projects, WHEN a milestone is + committed whose issues exist in only one of them, THEN once the new results + have arrived the dropdown offers that one project and no other. GIVEN the + intermediate render, where `resetKey` has already changed but the previous + result set is still on screen because the fetch is in flight, THEN the + accumulator is **not** refilled with those stale projects. GIVEN instead that + the user pages forward without changing any filter, THEN projects seen on + earlier pages remain in the dropdown — the in-flight empty list SHALL NOT + clear an accumulation whose key did not change. GIVEN a project filter is + active, THEN its value remains offered regardless, as today. + +30. **Deleting the selected saved query clears the milestone too.** GIVEN a + saved query is selected and its stored `milestone` is `Next`, WHEN the user + deletes that saved query, THEN the milestone draft and its committed value + both become `""` in the same update that clears `customQuery` and + `projectFilter`, and the refetch carries no `milestone`. GIVEN instead that + the deleted saved query is **not** the selected one, THEN no filter state + changes and the deletion triggers no refetch. + +31. **A repeated milestone key in a custom query resolves to the first.** GIVEN + the mock client and the custom query `milestone=Old&milestone=Next`, WHEN + the list is fetched, THEN the mock filters on `Old` — the first value — and + not on `Next`, and not on both. GIVEN the live path and the same custom + query, THEN the string is forwarded upstream unchanged, both pairs included, + and GitLab's own resolution applies. + +## Saving a query: when it is offered, and what it is called + +**`canSaveCurrent` gains a milestone term.** It is today +`committedQuery.trim().length > 0 || projectFilter.length > 0` and SHALL become +`committedQuery.trim().length > 0 || projectFilter.length > 0 || milestone.length > 0`, +where `milestone` is the **committed** value. A milestone-only query is +therefore saveable, which it is not today. The `.length > 0` form is used for +the milestone rather than a second `.trim()` because the committed value is +already trimmed by definition (see Client function); re-trimming here would +imply a second normalisation boundary where the spec states there is one. + +A saved query's `preset` is captured whether or not `canSaveCurrent` was made +true by the milestone — the effective preset is always persisted, including for +a save triggered by a custom query or a project filter alone (see Data model). + +**`suggestedLabel`** seeds the save-query dialog's name field. It currently +reads the committed custom query, else `In `, else `Saved query`. With +milestone added the precedence SHALL be, in order: + +1. the trimmed committed custom query, when non-empty; +2. `In `, when a project filter is set; +3. the milestone title, when non-empty; +4. `Saved query`. + +The stored `preset` SHALL NOT appear in the suggested name at any rung. It is +restored state, not something the user typed, and adding it would change the +name of queries that have nothing to do with this feature. + +Like the existing values, this string is persisted as the query's name and is +therefore i18n-exempt: it must not depend on the creating locale. **Here +`// i18n-exempt: ` genuinely is the correct marker** — this is a plain +expression in a `.tsx` file, invisible to the jsx-only +`i18next/no-literal-string` rule and gated instead by +`scripts/check-nonjsx-copy.mjs`. `gitlab-page-client.tsx:218` already carries +exactly such a marker on today's `suggestedLabel`, and it SHALL be kept. +Contrast the toolbar placeholder under Presentation, which is a JSX attribute +and needs an eslint disable instead; the two markers are different mechanisms +and are not interchangeable. + +## Out of scope + +Each exclusion below is a contract: these are decided as *not built*, not left +open. + +- **`milestone_id`, and its `Upcoming` / `Started` values.** GitLab documents + `milestone` and `milestone_id` as mutually exclusive, so supporting both means + a mutual-exclusion rule in the UI and a second control. The issue asks for + title search. A follow-up wanting `Upcoming`/`Started` would need: a new + `milestone_id` parameter, a rule rejecting requests that set both, and a UI + affordance that makes the exclusivity visible. +- **Milestone filtering for merge requests.** GitLab's `/merge_requests` + accepts the same attribute and the change would be symmetrical, but the issue + is scoped to issues and MR search has its own preset set including + `review_requested`. Not built; `GET /user/mrs` is asserted unchanged above. +- **A milestone dropdown or autocomplete.** Requires enumerating milestones + across every visible project; GitLab has no cross-project milestone endpoint. + A follow-up would need to decide which projects to enumerate, how to cache + the result, and what to show before it loads. +- **Milestone on GitLab issue watches.** `IssueWatch` has a structured `labels` + field and could take a milestone the same way, but a watch is a poller, not a + search, and its dialog already documents `state=opened&milestone=Next` in the + custom-query placeholder — the capability is reachable there today. A + follow-up would need a DB column, a store migration, `applyIssueWatchPatch` + handling, and a fold in `fetchIssues` next to the existing label fold. +- **GitHub, Linear, Jira or Azure DevOps parity.** No other integration gains a + milestone filter, and none of their request shapes change. +- **Teaching `MockClient` any filter key other than `milestone`.** `state`, + `scope`, `labels` and `assignee_username` continue to be ignored by the mock. +- **Sorting or `order_by` controls.** Upstream order is preserved for the live + path; the only ordering this feature specifies is the mock's. +- **Persisting the milestone as page state across reloads.** Matches the + existing behaviour of the project filter and custom query. +- **Multiple milestones at once.** GitLab's `milestone` attribute takes one + title. One input, one value. + +## Files touched + +Enumerated because the milestone's transport is an interface signature change, +and that is exactly the kind of change whose blast radius a planner under-counts. + +**Backend, `apps/backend/internal/gitlab/` only:** + +| File | Change | +|---|---| +| `client.go` | `Client` interface: the new `milestone` parameter on `ListIssues` and `ListIssuesPaged`, and their doc comments | +| `client_helpers.go` | `buildIssueSearchQuery` takes the milestone; the custom-query fold; `rawIssue` gains its milestone sub-struct and `convertRawIssue` reads its `title` | +| `models.go` | `Issue.Milestone` | +| `pat_client.go` | both methods forward the new parameter | +| `mock_client.go` | both methods: effective-milestone resolution, filtering, the named sort, and paging | +| `noop_client.go` | both signatures; the `ErrNoClient` bodies are unchanged | +| `controller.go` | `httpSearchUserIssues` reads and trims `milestone`, then passes it | +| `service_search.go` | `SearchUserIssues` and `SearchUserIssuesPaged` gain the parameter; the stats call passes `""` | +| `service_issue_watches.go` | call site passes `""` — mechanical, not a feature change | +| `service_mentions.go` | call site passes `""` | + +`rawIssue` and `convertRawIssue` live in `client_helpers.go`, **not** in +`models.go`, which holds only the `Issue` type. `glab_client.go` needs no change: +`GLabClient` embeds `*PATClient` and inherits both methods. +`apps/backend/internal/github/**` needs no change and SHALL NOT be touched. + +**Frontend:** + +| File | Change | +|---|---| +| `apps/web/lib/types/gitlab.ts` | `Issue.milestone`, mirroring the Go model | +| `apps/web/lib/api/domains/gitlab-api.ts` | `searchUserIssues` accepts an optional `milestone`, setting the param only when non-empty | +| `apps/web/components/gitlab/my-gitlab/use-gitlab-search.ts` | milestone in the hook options, in `FetchArgs`, in the fetch effect's dependencies, and in `refresh` | +| `apps/web/app/gitlab/gitlab-page-client.tsx` | loses `useGitLabPageState`; wires the toolbar control | +| `apps/web/app/gitlab/use-gitlab-page-state.ts` | **new file** — the extracted hook, `useProjectOptions`, `initialSelection`, `GitLabPageState`, `useSaveQueryDialog`, and the milestone state | +| `apps/web/components/gitlab/my-gitlab/use-saved-presets.ts` | `SavedPreset.milestone`, `SavedPreset.preset`, and the shared `isSavedPreset` tolerance rule covering both | +| `apps/web/components/gitlab/my-gitlab/list-toolbar.tsx` | the milestone input and its five new props (below) | +| `apps/web/components/gitlab/my-gitlab/issue-list.tsx` | the milestone chip and its test id | +| `apps/web/src/locales/{en,pt-pt,zh-cn,zh-hk,zh-tw}/` | the control's visible label and accessible name | + +`list-toolbar.tsx` is on this list because the milestone input renders inside it, +and it has to be, because **there is exactly one `ListToolbar` call site** +(`gitlab-page-client.tsx:301`) and it serves the Issues view and the Merge +requests view alike. A second call site is not an option, so the Issues-only +rule is a prop rather than a rendering location. Its five new props are REQUIRED, +not optional, and are specified under Presentation. + +`components/integrations/integration-list-toolbar.tsx` is **not** on this list. +`ListToolbar` composes it and passes the milestone input down as part of the +existing `filter` slot; the shared integration toolbar's own props do not change, +so GitHub and every other integration that renders it are untouched. + +`use-committed-query.ts` is deliberately absent from this list and SHALL NOT be +edited. The milestone reuses it as-is, one more instance of the draft/committed +pair it already provides; nothing about the hook needs to know a milestone +exists. + +## Constraints + +- **The extraction is specified, not left to the implementer.** + `apps/web/app/gitlab/gitlab-page-client.tsx` is 589 lines against a 600-line + file cap (`max-lines`, warn), and `useGitLabPageState` spans lines 161-264 — + **104 lines, already over the 100-line `max-lines-per-function` cap** before + this feature adds anything. An in-file comment at line 156 records that + `useProjectOptions` was already lifted out for this reason. Adding milestone + state in place would push both further over, so the implementation SHALL: + - move `useGitLabPageState`, its co-located helpers `useProjectOptions` and + `initialSelection`, and the `GitLabPageState` type into a new + `apps/web/app/gitlab/use-gitlab-page-state.ts`, leaving + `gitlab-page-client.tsx` well under the file cap; and + - within that new file, extract the save-query concern into a + `useSaveQueryDialog` hook, so `useGitLabPageState` itself lands **below** + 100 lines with the milestone state added rather than merely relocating an + existing warning. What moves, named as the file actually names it today: + `saveDialogOpen` / `setSaveDialogOpen`, `canSaveCurrent`, `suggestedLabel`, + and the `onOpenSaveDialog`, `onConfirmSave` and `onDeleteSaved` handlers. + There is no `saveName` state to move: the dialog's name field is local + `value` state inside `SavePresetForm` (`save-preset-dialog.tsx`), it is not + lifted into the page hook today, and this feature SHALL NOT lift it. + + The milestone's draft/committed pair SHALL reuse the existing + `useCommittedQuery` hook rather than adding a second mechanism, and SHALL use + it **unmodified**. The hook supplies `draft`, `committed`, `setDraft`, + `setImmediate` and `commit`; the trimming boundary is met by committing + through `setImmediate(trimGitLabMilestone(draft))`, as + specified under "Client function, and where trimming happens". `commit()` + copies the draft verbatim and SHALL NOT be changed, because the custom-query + input shares this hook. + `suggestedLabel` and `canSaveCurrent` move with the dialog hook and gain the + milestone term defined under "Saving a query: when it is offered, and what it + is called". +- New copy must land in all five catalogs (`en`, `pt-pt`, `zh-cn`, `zh-hk`, + `zh-tw`); `check-i18n-keys.mjs` fails on a missing key, a dropped + placeholder, or a value left identical to English. Use `pnpm run i18n:zh-hant` + for the Traditional Chinese pair. +- No Unicode em dash (U+2014) in any user-facing copy or locale value. +- Backend and frontend `Issue` types must change together; + `apps/web/lib/types/gitlab.ts` mirrors `apps/backend/internal/gitlab/models.go`. + +## Related + +- Issue: https://github.com/kdlbs/kandev/issues/2798 +- `docs/specs/gitlab-integration/` — the umbrella GitLab integration spec +- `apps/backend/internal/integrations/AGENTS.md` — integration conventions diff --git a/docs/specs/spec-lint-exceptions.tsv b/docs/specs/spec-lint-exceptions.tsv index 6361fd7967..0a71f5b3ce 100644 --- a/docs/specs/spec-lint-exceptions.tsv +++ b/docs/specs/spec-lint-exceptions.tsv @@ -1,3 +1,4 @@ +docs/specs/gitlab-issue-milestone-filter/spec.md 80514 docs/specs/office-task-content-editing/spec.md 84103 docs/specs/task-cost-ledger/spec.md 91931 docs/specs/task-delivery-ledger/spec.md 157207