From e64d8e4cf3a0a809b7ec2ce29e3b352189ff299b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Fri, 11 Sep 2026 16:18:10 +0100 Subject: [PATCH] feat(workspaces): save repository set base branches (#3269) --- .github/workflows/e2e-tests.yml | 19 + apps/backend/internal/task/dto/dto.go | 2 + .../internal/task/dto/repository_set_test.go | 5 +- ...epository_set_base_branch_handlers_test.go | 63 +++ .../task/handlers/repository_set_handlers.go | 60 ++- apps/backend/internal/task/models/models.go | 6 +- .../internal/task/repository/interface.go | 6 +- .../task/repository/sqlite/base_migrations.go | 1 + .../task/repository/sqlite/base_schema.go | 7 +- .../task/repository/sqlite/repository_set.go | 30 +- ...epository_set_base_branch_postgres_test.go | 88 +++++ .../sqlite/repository_set_base_branch_test.go | 121 ++++++ .../sqlite/repository_set_postgres_test.go | 2 +- .../sqlite/repository_set_review_test.go | 9 +- .../repository/sqlite/repository_set_test.go | 5 +- .../task/service/service_repository_sets.go | 98 ++++- ...ervice_repository_sets_base_branch_test.go | 150 +++++++ .../use-workspace-repository-sets.ts | 34 +- ...orkspace-repository-set-editor-members.tsx | 352 +++++++++++++++++ .../workspace-repository-set-editor.test.tsx | 183 +++++++++ .../workspace-repository-set-editor.tsx | 366 ++++++++++-------- ...workspace-repository-sets-section.test.tsx | 100 ++++- .../workspace-repository-sets-section.tsx | 13 +- apps/web/components/branch-picker-options.tsx | 9 +- apps/web/components/combobox.test.tsx | 21 + apps/web/components/combobox.tsx | 73 +++- ...sk-create-dialog-add-repository-button.tsx | 47 +++ .../task-create-dialog-branch-options.tsx | 1 + .../components/task-create-dialog-handlers.ts | 22 +- ...k-create-dialog-helpers-saved-base.test.ts | 23 ++ ...k-create-dialog-helpers.multi-repo.test.ts | 49 ++- .../components/task-create-dialog-helpers.ts | 34 +- .../components/task-create-dialog-options.tsx | 7 +- .../task-create-dialog-pill.test.tsx | 13 +- .../components/task-create-dialog-pill.tsx | 105 +++-- .../task-create-dialog-prop-builders.ts | 9 +- .../task-create-dialog-repo-base-branch.tsx | 114 ++++++ .../task-create-dialog-repo-chip-parts.tsx | 70 +++- .../task-create-dialog-repo-chip-utils.tsx | 127 ++++++ .../task-create-dialog-repo-chips.test.tsx | 125 +++++- .../task-create-dialog-repo-chips.tsx | 92 +++-- ...reate-dialog-repository-sets-save.test.tsx | 56 ++- ...ask-create-dialog-repository-sets-save.tsx | 34 +- ...task-create-dialog-repository-sets.test.ts | 80 +++- .../task-create-dialog-repository-sets.ts | 38 +- .../task-create-dialog-setup.test.ts | 25 +- .../components/task-create-dialog-setup.ts | 142 ++++++- .../components/task-create-dialog-types.ts | 5 + ...reate-dialog-workspace-repo-chips.test.tsx | 43 ++ ...ask-create-dialog-workspace-repo-chips.tsx | 230 +++++------ apps/web/e2e/README.md | 7 +- apps/web/e2e/fixtures/kubernetes-pins.ts | 2 +- apps/web/e2e/fixtures/kubernetes-tools.ts | 3 - apps/web/e2e/helpers/api-client.ts | 21 +- apps/web/e2e/scripts/kubernetes-pins.test.ts | 17 + .../mobile-workspace-repository-sets.spec.ts | 155 ++++++++ .../workspace-repository-sets.spec.ts | 97 ++++- .../task/create-task-repository-sets.spec.ts | 30 +- ...mobile-create-task-repository-sets.spec.ts | 28 +- .../workspace/use-repository-branches.ts | 3 + .../api/domains/repository-sets-api.test.ts | 29 ++ apps/web/lib/api/domains/workspace-api.ts | 51 ++- apps/web/lib/types/backend.ts | 2 +- apps/web/lib/types/http.ts | 6 +- .../lib/ws/handlers/repository-sets.test.ts | 3 +- apps/web/lib/ws/handlers/repository-sets.ts | 1 + apps/web/src/locales/en/task.json | 8 +- apps/web/src/locales/en/workspaces.json | 22 +- apps/web/src/locales/pseudo/task.json | 8 +- apps/web/src/locales/pseudo/workspaces.json | 22 +- apps/web/src/locales/pt-pt/task.json | 8 +- apps/web/src/locales/pt-pt/workspaces.json | 22 +- apps/web/src/locales/zh-cn/task.json | 8 +- apps/web/src/locales/zh-cn/workspaces.json | 22 +- apps/web/src/locales/zh-hk/task.json | 8 +- apps/web/src/locales/zh-hk/workspaces.json | 22 +- apps/web/src/locales/zh-tw/task.json | 8 +- apps/web/src/locales/zh-tw/workspaces.json | 22 +- .../task-06-kind-e2e-ci.md | 14 +- .../repository-set-base-branches/plan.md | 155 ++++++++ .../task-01-persist-member-base-branches.md | 94 +++++ .../task-02-apply-saved-bases.md | 104 +++++ .../task-03-responsive-inline-editor.md | 135 +++++++ .../task-04-saved-base-e2e.md | 107 +++++ .../task-05-public-documentation.md | 87 +++++ docs/public/tasks-and-workflows.md | 74 ++-- docs/specs/workspaces/README.md | 3 +- .../requirements/repository-sets.md | 335 ++++++---------- .../system-design/repository-sets.md | 281 ++++++++++++++ 89 files changed, 4481 insertions(+), 857 deletions(-) create mode 100644 apps/backend/internal/task/handlers/repository_set_base_branch_handlers_test.go create mode 100644 apps/backend/internal/task/repository/sqlite/repository_set_base_branch_postgres_test.go create mode 100644 apps/backend/internal/task/repository/sqlite/repository_set_base_branch_test.go create mode 100644 apps/backend/internal/task/service/service_repository_sets_base_branch_test.go create mode 100644 apps/web/app/settings/workspace/workspace-repository-set-editor-members.tsx create mode 100644 apps/web/app/settings/workspace/workspace-repository-set-editor.test.tsx create mode 100644 apps/web/components/task-create-dialog-add-repository-button.tsx create mode 100644 apps/web/components/task-create-dialog-helpers-saved-base.test.ts create mode 100644 apps/web/components/task-create-dialog-repo-base-branch.tsx create mode 100644 apps/web/components/task-create-dialog-repo-chip-utils.tsx create mode 100644 docs/plans/repository-set-base-branches/plan.md create mode 100644 docs/plans/repository-set-base-branches/task-01-persist-member-base-branches.md create mode 100644 docs/plans/repository-set-base-branches/task-02-apply-saved-bases.md create mode 100644 docs/plans/repository-set-base-branches/task-03-responsive-inline-editor.md create mode 100644 docs/plans/repository-set-base-branches/task-04-saved-base-e2e.md create mode 100644 docs/plans/repository-set-base-branches/task-05-public-documentation.md create mode 100644 docs/specs/workspaces/system-design/repository-sets.md diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 34bf8f70f6..b5b47e882f 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -598,6 +598,25 @@ jobs: - name: Verify Docker is available run: docker info + # Pull the immutable application-image base before the Playwright test. + # The fixture image only copies this run's artifacts, so package mirrors + # are not on the critical path of every PR's compatibility test. + - name: Pull pinned Kubernetes fixture base image + env: + KUBERNETES_E2E_BASE_IMAGE: ghcr.io/kdlbs/kandev-ci:runtime-sha-6f288a23c526@sha256:b9636e1c20adb0fcce1c65858a767fb1b0fe48a15ac5688b9c623163575efb8d + run: | + set -euo pipefail + for attempt in 1 2 3; do + if timeout --kill-after=10s 180s docker pull "$KUBERNETES_E2E_BASE_IMAGE"; then + exit 0 + fi + if (( attempt < 3 )); then + sleep "$((attempt * 5))" + fi + done + echo "Failed to pull $KUBERNETES_E2E_BASE_IMAGE after 3 attempts." >&2 + exit 1 + - name: Set up pnpm uses: pnpm/action-setup@a8198c4bff370c8506180b035930dea56dbd5288 # v5 with: diff --git a/apps/backend/internal/task/dto/dto.go b/apps/backend/internal/task/dto/dto.go index 8cdac1209c..a2b7b13f3d 100644 --- a/apps/backend/internal/task/dto/dto.go +++ b/apps/backend/internal/task/dto/dto.go @@ -105,6 +105,7 @@ type RepositorySetDTO struct { type RepositorySetItemDTO struct { RepositoryID string `json:"repository_id"` Position int `json:"position"` + BaseBranch string `json:"base_branch"` } type RepositoryBranchPolicyDTO struct { @@ -779,6 +780,7 @@ func FromRepositorySet(set *models.RepositorySet) RepositorySetDTO { items = append(items, RepositorySetItemDTO{ RepositoryID: item.RepositoryID, Position: item.Position, + BaseBranch: item.BaseBranch, }) } return RepositorySetDTO{ diff --git a/apps/backend/internal/task/dto/repository_set_test.go b/apps/backend/internal/task/dto/repository_set_test.go index 9210f43c08..448e2380af 100644 --- a/apps/backend/internal/task/dto/repository_set_test.go +++ b/apps/backend/internal/task/dto/repository_set_test.go @@ -16,7 +16,7 @@ func TestFromRepositorySetCarriesOrderedMembership(t *testing.T) { Name: "Full-stack", Description: "web + gateway", Items: []models.RepositorySetItem{ - {ID: "item-1", RepositorySetID: "set-1", RepositoryID: "repo-web", Position: 0}, + {ID: "item-1", RepositorySetID: "set-1", RepositoryID: "repo-web", Position: 0, BaseBranch: "develop"}, {ID: "item-2", RepositorySetID: "set-1", RepositoryID: "repo-gateway", Position: 1}, }, CreatedAt: created, @@ -37,6 +37,9 @@ func TestFromRepositorySetCarriesOrderedMembership(t *testing.T) { if got.Repositories[0].RepositoryID != "repo-web" || got.Repositories[0].Position != 0 { t.Fatalf("first member = %+v", got.Repositories[0]) } + if got.Repositories[0].BaseBranch != "develop" { + t.Fatalf("first member base branch = %q, want %q", got.Repositories[0].BaseBranch, "develop") + } if got.Repositories[1].RepositoryID != "repo-gateway" || got.Repositories[1].Position != 1 { t.Fatalf("second member = %+v", got.Repositories[1]) } diff --git a/apps/backend/internal/task/handlers/repository_set_base_branch_handlers_test.go b/apps/backend/internal/task/handlers/repository_set_base_branch_handlers_test.go new file mode 100644 index 0000000000..76d7144db1 --- /dev/null +++ b/apps/backend/internal/task/handlers/repository_set_base_branch_handlers_test.go @@ -0,0 +1,63 @@ +package handlers + +import ( + "encoding/json" + "net/http" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/kandev/kandev/internal/task/dto" +) + +func TestHTTPRepositorySetCarriesMemberBaseBranch(t *testing.T) { + router, _ := newRepositorySetTestRouter(t) + + created := createSetViaHTTP(t, router, map[string]any{ + "name": "Base branches", + "repositories": []map[string]any{ + {"repository_id": "repo-web", "base_branch": "develop"}, + }, + }) + + require.Len(t, created.Repositories, 1) + require.Equal(t, "repo-web", created.Repositories[0].RepositoryID) + require.Equal(t, "develop", created.Repositories[0].BaseBranch) + + var fetched dto.RepositorySetDTO + recorder := doJSON(t, router, http.MethodGet, "/api/v1/repository-sets/"+created.ID, nil) + require.Equal(t, http.StatusOK, recorder.Code, recorder.Body.String()) + require.NoError(t, json.Unmarshal(recorder.Body.Bytes(), &fetched)) + require.Equal(t, "develop", fetched.Repositories[0].BaseBranch) +} + +func TestHTTPRepositorySetUpdateCarriesMemberBaseBranch(t *testing.T) { + router, _ := newRepositorySetTestRouter(t) + created := createSetViaHTTP(t, router, map[string]any{ + "name": "Base branches", + "repository_ids": []string{"repo-web"}, + }) + + recorder := doJSON(t, router, http.MethodPatch, "/api/v1/repository-sets/"+created.ID, map[string]any{ + "repositories": []map[string]any{ + {"repository_id": "repo-gateway", "base_branch": "release"}, + }, + }) + require.Equal(t, http.StatusOK, recorder.Code, recorder.Body.String()) + var updated dto.RepositorySetDTO + require.NoError(t, json.Unmarshal(recorder.Body.Bytes(), &updated)) + require.Len(t, updated.Repositories, 1) + require.Equal(t, "repo-gateway", updated.Repositories[0].RepositoryID) + require.Equal(t, "release", updated.Repositories[0].BaseBranch) +} + +func TestHTTPRepositorySetRejectsConflictingMemberInputs(t *testing.T) { + router, _ := newRepositorySetTestRouter(t) + + recorder := doJSON(t, router, http.MethodPost, "/api/v1/workspaces/ws-1/repository-sets", map[string]any{ + "name": "Conflict", + "repository_ids": []string{"repo-web"}, + "repositories": []map[string]any{{"repository_id": "repo-gateway"}}, + }) + require.Equal(t, http.StatusBadRequest, recorder.Code, recorder.Body.String()) +} diff --git a/apps/backend/internal/task/handlers/repository_set_handlers.go b/apps/backend/internal/task/handlers/repository_set_handlers.go index 897e2be26a..6e4e5de7bd 100644 --- a/apps/backend/internal/task/handlers/repository_set_handlers.go +++ b/apps/backend/internal/task/handlers/repository_set_handlers.go @@ -3,6 +3,7 @@ package handlers import ( "context" "errors" + "fmt" "net/http" "github.com/gin-gonic/gin" @@ -76,19 +77,39 @@ func repositorySetsToListResponse(sets []*models.RepositorySet) dto.ListReposito // repositorySetCreateBody is the create payload. workspace_id is read from the // path on HTTP and from the payload over WebSocket. type repositorySetCreateBody struct { - WorkspaceID string `json:"workspace_id"` - Name string `json:"name"` - Description string `json:"description"` - RepositoryIDs []string `json:"repository_ids"` + WorkspaceID string `json:"workspace_id"` + Name string `json:"name"` + Description string `json:"description"` + RepositoryIDs *[]string `json:"repository_ids"` + Repositories *[]service.RepositorySetMemberInput `json:"repositories"` +} + +func (b repositorySetCreateBody) serviceRequest(workspaceID string) (*service.CreateRepositorySetRequest, error) { + if b.RepositoryIDs != nil && b.Repositories != nil { + return nil, fmt.Errorf("%w: repositories and repository_ids cannot both be provided", service.ErrInvalidRepositorySet) + } + req := &service.CreateRepositorySetRequest{ + WorkspaceID: workspaceID, + Name: b.Name, + Description: b.Description, + } + if b.RepositoryIDs != nil { + req.RepositoryIDs = *b.RepositoryIDs + } + if b.Repositories != nil { + req.Repositories = *b.Repositories + } + return req, nil } // repositorySetUpdateBody patches a set. Pointers distinguish "absent" from // "set to empty": an absent repository_ids leaves membership alone, while an // explicitly empty one is a rejected request rather than a silent wipe. type repositorySetUpdateBody struct { - Name *string `json:"name"` - Description *string `json:"description"` - RepositoryIDs *[]string `json:"repository_ids"` + Name *string `json:"name"` + Description *string `json:"description"` + RepositoryIDs *[]string `json:"repository_ids"` + Repositories *[]service.RepositorySetMemberInput `json:"repositories"` } // repositorySetStatus maps a service error to its HTTP status. Categories are @@ -160,12 +181,12 @@ func (h *RepositorySetHandlers) httpCreateRepositorySet(c *gin.Context) { if !h.resolveWritableWorkspace(c, workspaceID) { return } - set, err := h.service.CreateRepositorySet(c.Request.Context(), &service.CreateRepositorySetRequest{ - WorkspaceID: workspaceID, - Name: body.Name, - Description: body.Description, - RepositoryIDs: body.RepositoryIDs, - }) + request, err := body.serviceRequest(workspaceID) + if err != nil { + h.abortWithRepositorySetError(c, "create repository set", err) + return + } + set, err := h.service.CreateRepositorySet(c.Request.Context(), request) if err != nil { h.abortWithRepositorySetError(c, "create repository set", err) return @@ -200,6 +221,7 @@ func (h *RepositorySetHandlers) httpUpdateRepositorySet(c *gin.Context) { Name: body.Name, Description: body.Description, RepositoryIDs: body.RepositoryIDs, + Repositories: body.Repositories, }) if err != nil { h.abortWithRepositorySetError(c, "update repository set", err) @@ -300,12 +322,11 @@ func (h *RepositorySetHandlers) wsCreateRepositorySet(ctx context.Context, msg * if errMsg, blocked := h.wsRejectReadOnlyWorkspace(ctx, msg, req.WorkspaceID); blocked { return errMsg, nil } - set, err := h.service.CreateRepositorySet(ctx, &service.CreateRepositorySetRequest{ - WorkspaceID: req.WorkspaceID, - Name: req.Name, - Description: req.Description, - RepositoryIDs: req.RepositoryIDs, - }) + request, err := req.serviceRequest(req.WorkspaceID) + if err != nil { + return h.wsRepositorySetError(msg, "create repository set", err) + } + set, err := h.service.CreateRepositorySet(ctx, request) if err != nil { return h.wsRepositorySetError(msg, "create repository set", err) } @@ -341,6 +362,7 @@ func (h *RepositorySetHandlers) wsUpdateRepositorySet(ctx context.Context, msg * Name: req.Name, Description: req.Description, RepositoryIDs: req.RepositoryIDs, + Repositories: req.Repositories, }) if err != nil { return h.wsRepositorySetError(msg, "update repository set", err) diff --git a/apps/backend/internal/task/models/models.go b/apps/backend/internal/task/models/models.go index dff1850919..dafc8e6343 100644 --- a/apps/backend/internal/task/models/models.go +++ b/apps/backend/internal/task/models/models.go @@ -2003,9 +2003,8 @@ type RepositoryBranchPolicy struct { // RepositorySet is a named, reusable group of workspace repositories that fills // the task-creation repository picker in one action. // -// A set deliberately stores no branch. Branch choice belongs to a task and is -// already modelled on TaskRepository; a branch cached here would go stale -// against the repository's real refs. +// A set stores an optional base branch for each member. The task draft copies +// that preference when the set is applied; checkout state remains task-owned. type RepositorySet struct { ID string `json:"id"` WorkspaceID string `json:"workspace_id"` @@ -2024,6 +2023,7 @@ type RepositorySetItem struct { RepositorySetID string `json:"repository_set_id"` RepositoryID string `json:"repository_id"` Position int `json:"position"` + BaseBranch string `json:"base_branch"` CreatedAt time.Time `json:"created_at"` UpdatedAt time.Time `json:"updated_at"` } diff --git a/apps/backend/internal/task/repository/interface.go b/apps/backend/internal/task/repository/interface.go index fc5e9bb6e7..54835270b2 100644 --- a/apps/backend/internal/task/repository/interface.go +++ b/apps/backend/internal/task/repository/interface.go @@ -493,10 +493,10 @@ type RepositorySetRepository interface { // ListRepositorySetIDsByRepository reports which sets hold a repository, so a // caller can publish their new shape after a deletion prunes membership. ListRepositorySetIDsByRepository(ctx context.Context, repositoryID string) ([]string, error) - // UpdateRepositorySet writes the set's fields and, when repositoryIDs is + // UpdateRepositorySet writes the set's fields and, when repositoryItems is // non-nil, replaces its whole membership in the same transaction so the two - // cannot land apart. A nil repositoryIDs leaves membership untouched. - UpdateRepositorySet(ctx context.Context, set *models.RepositorySet, repositoryIDs *[]string) error + // cannot land apart. A nil repositoryItems leaves membership untouched. + UpdateRepositorySet(ctx context.Context, set *models.RepositorySet, repositoryItems *[]models.RepositorySetItem) error DeleteRepositorySet(ctx context.Context, id string) (bool, error) } diff --git a/apps/backend/internal/task/repository/sqlite/base_migrations.go b/apps/backend/internal/task/repository/sqlite/base_migrations.go index 767d1d235d..1e0b6e652e 100644 --- a/apps/backend/internal/task/repository/sqlite/base_migrations.go +++ b/apps/backend/internal/task/repository/sqlite/base_migrations.go @@ -65,6 +65,7 @@ func (r *Repository) runMigrations() error { if err := r.ensureTeamAccessSchema(); err != nil { return err } + _ = r.migrate.Apply("repository_set_items.base_branch", `ALTER TABLE repository_set_items ADD COLUMN base_branch TEXT NOT NULL DEFAULT ''`) if err := r.ensureRepositoryBranchPoliciesSchema(); err != nil { return err } diff --git a/apps/backend/internal/task/repository/sqlite/base_schema.go b/apps/backend/internal/task/repository/sqlite/base_schema.go index 8fb3eac4f5..1d21fc0e04 100644 --- a/apps/backend/internal/task/repository/sqlite/base_schema.go +++ b/apps/backend/internal/task/repository/sqlite/base_schema.go @@ -557,9 +557,9 @@ func (r *Repository) initTaskSchema() error { // repositorySetsSchemaDDL declares the repository-set tables. It runs after // initCoreSchema so `workspaces` and `repositories` exist for the foreign keys. // -// Membership positions are contiguous from zero and carry no branch: branch -// choice belongs to a task (task_repositories), which is exactly what the user -// still decides after applying a set. +// Membership positions are contiguous from zero. A saved base branch is an +// optional task-form default; checkout and task branch choices remain task +// state. const repositorySetsSchemaDDL = ` CREATE TABLE IF NOT EXISTS repository_sets ( id TEXT PRIMARY KEY, @@ -577,6 +577,7 @@ const repositorySetsSchemaDDL = ` repository_set_id TEXT NOT NULL, repository_id TEXT NOT NULL, position INTEGER NOT NULL DEFAULT 0, + base_branch TEXT NOT NULL DEFAULT '', created_at TIMESTAMP NOT NULL, updated_at TIMESTAMP NOT NULL, FOREIGN KEY (repository_set_id) REFERENCES repository_sets(id) ON DELETE CASCADE, diff --git a/apps/backend/internal/task/repository/sqlite/repository_set.go b/apps/backend/internal/task/repository/sqlite/repository_set.go index 67036e4d52..557debb9fc 100644 --- a/apps/backend/internal/task/repository/sqlite/repository_set.go +++ b/apps/backend/internal/task/repository/sqlite/repository_set.go @@ -82,9 +82,10 @@ func (r *Repository) insertRepositorySetItems( items[i].UpdatedAt = now _, err := tx.ExecContext(ctx, r.db.Rebind(` INSERT INTO repository_set_items ( - id, repository_set_id, repository_id, position, created_at, updated_at - ) VALUES (?, ?, ?, ?, ?, ?) - `), items[i].ID, setID, items[i].RepositoryID, items[i].Position, now, now) + id, repository_set_id, repository_id, position, base_branch, created_at, updated_at + ) VALUES (?, ?, ?, ?, ?, ?, ?) + `), items[i].ID, setID, items[i].RepositoryID, items[i].Position, + items[i].BaseBranch, now, now) if err != nil { return err } @@ -190,7 +191,8 @@ func (r *Repository) listRepositorySetItems( return itemsBySet, nil } query, args, err := sqlx.In(` - SELECT i.id, i.repository_set_id, i.repository_id, i.position, i.created_at, i.updated_at + SELECT i.id, i.repository_set_id, i.repository_id, i.position, i.base_branch, + i.created_at, i.updated_at FROM repository_set_items i INNER JOIN repository_sets s ON s.id = i.repository_set_id INNER JOIN repositories rep ON rep.id = i.repository_id @@ -211,7 +213,7 @@ func (r *Repository) listRepositorySetItems( for rows.Next() { item := models.RepositorySetItem{} if err := rows.Scan(&item.ID, &item.RepositorySetID, &item.RepositoryID, - &item.Position, &item.CreatedAt, &item.UpdatedAt); err != nil { + &item.Position, &item.BaseBranch, &item.CreatedAt, &item.UpdatedAt); err != nil { return nil, err } itemsBySet[item.RepositorySetID] = append(itemsBySet[item.RepositorySetID], item) @@ -244,18 +246,18 @@ func (r *Repository) ListRepositorySetIDsByRepository( return ids, rows.Err() } -// UpdateRepositorySet writes the set's own fields and, when repositoryIDs is +// UpdateRepositorySet writes the set's own fields and, when repositoryItems is // non-nil, replaces its whole membership in the SAME transaction. // // The two must commit together: a name change that lands while the membership // replacement fails leaves the set renamed but still holding the old // repositories, with the API reporting failure and publishing nothing. A nil -// repositoryIDs leaves membership untouched, which is how an update that only +// repositoryItems leaves membership untouched, which is how an update that only // changes name or description is expressed. func (r *Repository) UpdateRepositorySet( ctx context.Context, set *models.RepositorySet, - repositoryIDs *[]string, + repositoryItems *[]models.RepositorySetItem, ) error { tx, err := r.db.BeginTxx(ctx, nil) if err != nil { @@ -280,8 +282,8 @@ func (r *Repository) UpdateRepositorySet( return repoerrors.ErrRepositorySetNotFound } - if repositoryIDs != nil { - if err := r.replaceItemsTx(ctx, tx, set.ID, *repositoryIDs, set.UpdatedAt); err != nil { + if repositoryItems != nil { + if err := r.replaceItemsTx(ctx, tx, set.ID, *repositoryItems, set.UpdatedAt); err != nil { return err } } @@ -295,18 +297,14 @@ func (r *Repository) replaceItemsTx( ctx context.Context, tx *sqlx.Tx, setID string, - repositoryIDs []string, + repositoryItems []models.RepositorySetItem, now time.Time, ) error { if _, err := tx.ExecContext(ctx, r.db.Rebind( `DELETE FROM repository_set_items WHERE repository_set_id = ?`), setID); err != nil { return err } - items := make([]models.RepositorySetItem, 0, len(repositoryIDs)) - for _, repositoryID := range repositoryIDs { - items = append(items, models.RepositorySetItem{RepositoryID: repositoryID}) - } - return r.insertRepositorySetItems(ctx, tx, setID, items, now) + return r.insertRepositorySetItems(ctx, tx, setID, repositoryItems, now) } // DeleteRepositorySet removes a set and, by cascade, its membership rows. It diff --git a/apps/backend/internal/task/repository/sqlite/repository_set_base_branch_postgres_test.go b/apps/backend/internal/task/repository/sqlite/repository_set_base_branch_postgres_test.go new file mode 100644 index 0000000000..7eb6e92225 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/repository_set_base_branch_postgres_test.go @@ -0,0 +1,88 @@ +package sqlite + +import ( + "context" + "testing" + "time" + + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/testutil" +) + +func TestPostgresRepositorySetBaseBranchMigrationPreservesItems(t *testing.T) { + db := testutil.OpenIsolatedPostgres(t, testutil.PostgresDSNFromEnv(t)) + repo, err := NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("init postgres schema: %v", err) + } + ctx := context.Background() + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-pg-base", Name: "ws-pg-base"}); err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + if err := repo.CreateRepository(ctx, &models.Repository{ + ID: "repo-pg-base", WorkspaceID: "ws-pg-base", Name: "repo-pg-base", + SourceType: "local", LocalPath: "/tmp/repo-pg-base", + }); err != nil { + t.Fatalf("CreateRepository: %v", err) + } + set := &models.RepositorySet{WorkspaceID: "ws-pg-base", Name: "Legacy base"} + if err := repo.CreateRepositorySet(ctx, set); err != nil { + t.Fatalf("CreateRepositorySet: %v", err) + } + if _, err := repo.db.Exec(`DROP TABLE repository_set_items`); err != nil { + t.Fatalf("drop current repository_set_items: %v", err) + } + if _, err := repo.db.Exec(` + CREATE TABLE repository_set_items ( + id TEXT PRIMARY KEY, + repository_set_id TEXT NOT NULL, + repository_id TEXT NOT NULL, + position INTEGER NOT NULL DEFAULT 0, + created_at TIMESTAMP NOT NULL, + updated_at TIMESTAMP NOT NULL, + FOREIGN KEY (repository_set_id) REFERENCES repository_sets(id) ON DELETE CASCADE, + FOREIGN KEY (repository_id) REFERENCES repositories(id) ON DELETE CASCADE, + UNIQUE(repository_set_id, repository_id) + ) + `); err != nil { + t.Fatalf("create legacy repository_set_items: %v", err) + } + now := time.Now().UTC() + if _, err := repo.db.Exec(repo.db.Rebind(` + INSERT INTO repository_set_items + (id, repository_set_id, repository_id, position, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?) + `), "item-pg-base", set.ID, "repo-pg-base", 0, now, now); err != nil { + t.Fatalf("insert legacy repository-set item: %v", err) + } + + if err := repo.runMigrations(); err != nil { + t.Fatalf("run base-branch migration: %v", err) + } + assertBaseBranchColumn := func(stage string) { + var count int + err := repo.db.Get(&count, repo.db.Rebind(` + SELECT COUNT(*) + FROM information_schema.columns + WHERE table_schema = current_schema() AND table_name = ? AND column_name = ? + `), "repository_set_items", "base_branch") + if err != nil { + t.Fatalf("inspect base_branch column after %s migration: %v", stage, err) + } + if count != 1 { + t.Fatalf("base_branch column count after %s migration = %d, want 1", stage, count) + } + } + assertBaseBranchColumn("first") + if err := repo.runMigrations(); err != nil { + t.Fatalf("replay base-branch migration: %v", err) + } + assertBaseBranchColumn("replayed") + loaded, err := repo.GetRepositorySet(ctx, set.ID) + if err != nil { + t.Fatalf("GetRepositorySet after migration: %v", err) + } + if len(loaded.Items) != 1 || loaded.Items[0].BaseBranch != "" { + t.Fatalf("migrated item = %+v, want one item with empty base", loaded.Items) + } +} diff --git a/apps/backend/internal/task/repository/sqlite/repository_set_base_branch_test.go b/apps/backend/internal/task/repository/sqlite/repository_set_base_branch_test.go new file mode 100644 index 0000000000..433ab7c2b6 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/repository_set_base_branch_test.go @@ -0,0 +1,121 @@ +package sqlite + +import ( + "context" + "database/sql" + "testing" + "time" + + "github.com/kandev/kandev/internal/task/models" +) + +func TestRepositorySetItemsSchemaIncludesBaseBranch(t *testing.T) { + repo := newRepoForSetTests(t) + + rows, err := repo.db.Queryx(`PRAGMA table_info(repository_set_items)`) + if err != nil { + t.Fatalf("inspect repository_set_items schema: %v", err) + } + defer func() { _ = rows.Close() }() + + for rows.Next() { + var cid, notNull, primaryKey int + var name, columnType string + var defaultValue sql.NullString + if err := rows.Scan(&cid, &name, &columnType, ¬Null, &defaultValue, &primaryKey); err != nil { + t.Fatalf("scan repository_set_items schema: %v", err) + } + if name == "base_branch" { + return + } + } + if err := rows.Err(); err != nil { + t.Fatalf("iterate repository_set_items schema: %v", err) + } + t.Fatal("repository_set_items is missing base_branch") +} + +func TestRepositorySetRoundTripsSavedBaseBranch(t *testing.T) { + repo := newRepoForSetTests(t) + seedWorkspace(t, repo, "ws-base") + seedSetRepository(t, repo, "ws-base", "repo-base") + + item := models.RepositorySetItem{RepositoryID: "repo-base", BaseBranch: "develop"} + + set := &models.RepositorySet{ + WorkspaceID: "ws-base", + Name: "Base branch", + Items: []models.RepositorySetItem{item}, + } + if err := repo.CreateRepositorySet(context.Background(), set); err != nil { + t.Fatalf("CreateRepositorySet: %v", err) + } + + loaded, err := repo.GetRepositorySet(context.Background(), set.ID) + if err != nil { + t.Fatalf("GetRepositorySet: %v", err) + } + got := loaded.Items[0].BaseBranch + if got != "develop" { + t.Fatalf("saved base branch = %q, want %q", got, "develop") + } +} + +func TestRepositorySetBaseBranchMigrationPreservesLegacyItems(t *testing.T) { + repo := newRepoForSetTests(t) + ctx := context.Background() + seedWorkspace(t, repo, "ws-legacy-base") + seedSetRepository(t, repo, "ws-legacy-base", "repo-legacy-base") + + set := &models.RepositorySet{ + WorkspaceID: "ws-legacy-base", + Name: "Legacy base", + } + if err := repo.CreateRepositorySet(ctx, set); err != nil { + t.Fatalf("CreateRepositorySet: %v", err) + } + if _, err := repo.db.Exec(`DROP TABLE repository_set_items`); err != nil { + t.Fatalf("drop current repository_set_items: %v", err) + } + if _, err := repo.db.Exec(` + CREATE TABLE repository_set_items ( + id TEXT PRIMARY KEY, + repository_set_id TEXT NOT NULL, + repository_id TEXT NOT NULL, + position INTEGER NOT NULL DEFAULT 0, + created_at TIMESTAMP NOT NULL, + updated_at TIMESTAMP NOT NULL, + FOREIGN KEY (repository_set_id) REFERENCES repository_sets(id) ON DELETE CASCADE, + FOREIGN KEY (repository_id) REFERENCES repositories(id) ON DELETE CASCADE, + UNIQUE(repository_set_id, repository_id) + ) + `); err != nil { + t.Fatalf("create legacy repository_set_items: %v", err) + } + now := time.Now().UTC() + if _, err := repo.db.Exec(repo.db.Rebind(` + INSERT INTO repository_set_items + (id, repository_set_id, repository_id, position, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?) + `), "legacy-item", set.ID, "repo-legacy-base", 0, now, now); err != nil { + t.Fatalf("insert legacy repository-set item: %v", err) + } + + if err := repo.runMigrations(); err != nil { + t.Fatalf("run base-branch migration: %v", err) + } + if err := repo.runMigrations(); err != nil { + t.Fatalf("replay base-branch migration: %v", err) + } + + loaded, err := repo.GetRepositorySet(ctx, set.ID) + if err != nil { + t.Fatalf("GetRepositorySet after migration: %v", err) + } + if len(loaded.Items) != 1 { + t.Fatalf("migrated items = %+v, want one item", loaded.Items) + } + if loaded.Items[0].RepositoryID != "repo-legacy-base" || loaded.Items[0].BaseBranch != "" { + t.Fatalf("migrated item = %+v, want legacy membership with empty base", loaded.Items[0]) + } +} diff --git a/apps/backend/internal/task/repository/sqlite/repository_set_postgres_test.go b/apps/backend/internal/task/repository/sqlite/repository_set_postgres_test.go index 0c90ee9794..79a494c374 100644 --- a/apps/backend/internal/task/repository/sqlite/repository_set_postgres_test.go +++ b/apps/backend/internal/task/repository/sqlite/repository_set_postgres_test.go @@ -58,7 +58,7 @@ func TestPostgresRepositorySetRoundTrip(t *testing.T) { t.Fatalf("GetRepositorySetByName = %+v", byName) } - pgMembers := []string{"repo-pg-web"} + pgMembers := []models.RepositorySetItem{{RepositoryID: "repo-pg-web"}} if err := repo.UpdateRepositorySet(ctx, set, &pgMembers); err != nil { t.Fatalf("UpdateRepositorySet: %v", err) } diff --git a/apps/backend/internal/task/repository/sqlite/repository_set_review_test.go b/apps/backend/internal/task/repository/sqlite/repository_set_review_test.go index feb1804bdf..0d24464b5f 100644 --- a/apps/backend/internal/task/repository/sqlite/repository_set_review_test.go +++ b/apps/backend/internal/task/repository/sqlite/repository_set_review_test.go @@ -26,7 +26,10 @@ func TestUpdateRepositorySetRollsBackMetadataWhenMembershipFails(t *testing.T) { renamed := *set renamed.Name = "Renamed" renamed.Description = "changed" - members := []string{"repo-web", "repo-does-not-exist"} + members := []models.RepositorySetItem{ + {RepositoryID: "repo-web"}, + {RepositoryID: "repo-does-not-exist"}, + } if err := repo.UpdateRepositorySet(ctx, &renamed, &members); err == nil { t.Fatal("UpdateRepositorySet accepted a member that does not exist") } @@ -55,7 +58,7 @@ func TestUpdateRepositorySetAppliesBothHalvesTogether(t *testing.T) { } set.Name = "Backend" - members := []string{"repo-orders"} + members := []models.RepositorySetItem{{RepositoryID: "repo-orders"}} if err := repo.UpdateRepositorySet(ctx, set, &members); err != nil { t.Fatalf("UpdateRepositorySet: %v", err) } @@ -84,7 +87,7 @@ func TestUpdateRepositorySetOnDeletedSetReportsNotFound(t *testing.T) { // An empty membership list touches no item rows, so without checking the // metadata UPDATE's RowsAffected this used to commit successfully and let the // service publish an update for a set that no longer exists. - empty := []string{} + empty := []models.RepositorySetItem{} if err := repo.UpdateRepositorySet(ctx, set, &empty); !errors.Is(err, repoerrors.ErrRepositorySetNotFound) { t.Fatalf("UpdateRepositorySet error = %v, want ErrRepositorySetNotFound", err) } diff --git a/apps/backend/internal/task/repository/sqlite/repository_set_test.go b/apps/backend/internal/task/repository/sqlite/repository_set_test.go index d4fdbb1f1b..6d28278b6d 100644 --- a/apps/backend/internal/task/repository/sqlite/repository_set_test.go +++ b/apps/backend/internal/task/repository/sqlite/repository_set_test.go @@ -208,7 +208,10 @@ func TestReplaceRepositorySetItemsRewritesOrderContiguously(t *testing.T) { } // Reorder and drop one member in a single replace. - members := []string{"repo-orders", "repo-web"} + members := []models.RepositorySetItem{ + {RepositoryID: "repo-orders"}, + {RepositoryID: "repo-web"}, + } if err := repo.UpdateRepositorySet(ctx, set, &members); err != nil { t.Fatalf("UpdateRepositorySet: %v", err) } diff --git a/apps/backend/internal/task/service/service_repository_sets.go b/apps/backend/internal/task/service/service_repository_sets.go index 6a5450f5d9..304a152811 100644 --- a/apps/backend/internal/task/service/service_repository_sets.go +++ b/apps/backend/internal/task/service/service_repository_sets.go @@ -9,6 +9,7 @@ import ( "go.uber.org/zap" + "github.com/kandev/kandev/internal/common/securityutil" "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/task/models" @@ -35,22 +36,31 @@ var ( ErrUnknownRepositorySetMembers = errors.New("unknown repository set members") ) -// CreateRepositorySetRequest creates a set. RepositoryIDs is ordered and defines -// the apply order; it carries no branch, because branch choice belongs to a task. +// RepositorySetMemberInput is the wire/service input for one ordered set +// member. BaseBranch is optional; an empty value preserves task-form defaults. +type RepositorySetMemberInput struct { + RepositoryID string `json:"repository_id"` + BaseBranch string `json:"base_branch,omitempty"` +} + +// CreateRepositorySetRequest creates a set. Repositories is the new ordered +// member shape. RepositoryIDs remains the no-base compatibility input. type CreateRepositorySetRequest struct { - WorkspaceID string `json:"workspace_id"` - Name string `json:"name"` - Description string `json:"description"` - RepositoryIDs []string `json:"repository_ids"` + WorkspaceID string `json:"workspace_id"` + Name string `json:"name"` + Description string `json:"description"` + RepositoryIDs []string `json:"repository_ids"` + Repositories []RepositorySetMemberInput `json:"repositories"` } // UpdateRepositorySetRequest patches a set. Every field is optional; an absent // field is left alone. A supplied RepositoryIDs replaces the whole membership // list, which is also how reordering is expressed. type UpdateRepositorySetRequest struct { - Name *string `json:"name"` - Description *string `json:"description"` - RepositoryIDs *[]string `json:"repository_ids"` + Name *string `json:"name"` + Description *string `json:"description"` + RepositoryIDs *[]string `json:"repository_ids"` + Repositories *[]RepositorySetMemberInput `json:"repositories"` } // CreateRepositorySet validates the whole request, then writes the set and its @@ -69,14 +79,18 @@ func (s *Service) CreateRepositorySet( if err := s.assertRepositorySetNameFree(ctx, req.WorkspaceID, name, ""); err != nil { return nil, err } - if err := s.validateRepositorySetMembers(ctx, req.WorkspaceID, req.RepositoryIDs); err != nil { + items, err := repositorySetItemsFromCreateRequest(req) + if err != nil { + return nil, err + } + if err := s.validateRepositorySetMembers(ctx, req.WorkspaceID, items); err != nil { return nil, err } set := &models.RepositorySet{ WorkspaceID: req.WorkspaceID, Name: name, Description: strings.TrimSpace(req.Description), - Items: repositorySetItemsFor(req.RepositoryIDs), + Items: items, } if err := s.repositorySets.CreateRepositorySet(ctx, set); err != nil { return nil, err @@ -125,7 +139,11 @@ func (s *Service) UpdateRepositorySet( // One call, one transaction: a rename that lands while the membership // replacement fails would leave the set renamed but still holding the old // repositories, with this method reporting failure and publishing nothing. - if err := s.repositorySets.UpdateRepositorySet(ctx, set, req.RepositoryIDs); err != nil { + var repositoryItems *[]models.RepositorySetItem + if req.Repositories != nil || req.RepositoryIDs != nil { + repositoryItems = &set.Items + } + if err := s.repositorySets.UpdateRepositorySet(ctx, set, repositoryItems); err != nil { return nil, err } updated, err := s.repositorySets.GetRepositorySet(ctx, set.ID) @@ -156,10 +174,22 @@ func (s *Service) prepareRepositorySetUpdate( if req.Description != nil { set.Description = strings.TrimSpace(*req.Description) } - if req.RepositoryIDs != nil { - if err := s.validateRepositorySetMembers(ctx, set.WorkspaceID, *req.RepositoryIDs); err != nil { + if req.Repositories != nil && req.RepositoryIDs != nil { + return fmt.Errorf("%w: repositories and repository_ids cannot both be provided", ErrInvalidRepositorySet) + } + if req.Repositories != nil { + items := repositorySetItemsForInputs(*req.Repositories) + if err := s.validateRepositorySetMembers(ctx, set.WorkspaceID, items); err != nil { return err } + set.Items = items + } + if req.RepositoryIDs != nil { + items := repositorySetItemsFor(*req.RepositoryIDs) + if err := s.validateRepositorySetMembers(ctx, set.WorkspaceID, items); err != nil { + return err + } + set.Items = items } return nil } @@ -253,13 +283,14 @@ func (s *Service) assertRepositorySetNameFree( func (s *Service) validateRepositorySetMembers( ctx context.Context, workspaceID string, - repositoryIDs []string, + items []models.RepositorySetItem, ) error { - if len(repositoryIDs) == 0 { + if len(items) == 0 { return fmt.Errorf("%w: at least one repository is required", ErrInvalidRepositorySet) } - seen := make(map[string]struct{}, len(repositoryIDs)) - for _, id := range repositoryIDs { + seen := make(map[string]struct{}, len(items)) + for _, item := range items { + id := item.RepositoryID if strings.TrimSpace(id) == "" { return fmt.Errorf("%w: repository ids must not be blank", ErrInvalidRepositorySet) } @@ -267,6 +298,9 @@ func (s *Service) validateRepositorySetMembers( return fmt.Errorf("%w: repository %q listed more than once", ErrInvalidRepositorySet, id) } seen[id] = struct{}{} + if item.BaseBranch != "" && !securityutil.IsValidBaseBranchRef(item.BaseBranch) { + return fmt.Errorf("%w: unsafe base branch for repository %q", ErrInvalidRepositorySet, id) + } } // One workspace listing rather than a lookup per id, and it already excludes // soft-deleted rows and other workspaces. @@ -281,9 +315,9 @@ func (s *Service) validateRepositorySetMembers( } } unknown := make([]string, 0) - for _, id := range repositoryIDs { - if _, ok := available[id]; !ok { - unknown = append(unknown, id) + for _, item := range items { + if _, ok := available[item.RepositoryID]; !ok { + unknown = append(unknown, item.RepositoryID) } } if len(unknown) > 0 { @@ -292,6 +326,27 @@ func (s *Service) validateRepositorySetMembers( return nil } +func repositorySetItemsFromCreateRequest(req *CreateRepositorySetRequest) ([]models.RepositorySetItem, error) { + if req.Repositories != nil && req.RepositoryIDs != nil { + return nil, fmt.Errorf("%w: repositories and repository_ids cannot both be provided", ErrInvalidRepositorySet) + } + if len(req.Repositories) > 0 { + return repositorySetItemsForInputs(req.Repositories), nil + } + return repositorySetItemsFor(req.RepositoryIDs), nil +} + +func repositorySetItemsForInputs(inputs []RepositorySetMemberInput) []models.RepositorySetItem { + items := make([]models.RepositorySetItem, 0, len(inputs)) + for _, input := range inputs { + items = append(items, models.RepositorySetItem{ + RepositoryID: input.RepositoryID, + BaseBranch: input.BaseBranch, + }) + } + return items +} + // repositorySetItemsFor turns an ordered id list into membership models. The // store assigns positions from slice order. func repositorySetItemsFor(repositoryIDs []string) []models.RepositorySetItem { @@ -317,6 +372,7 @@ func (s *Service) publishRepositorySetEvent( repositories = append(repositories, map[string]interface{}{ "repository_id": item.RepositoryID, "position": item.Position, + "base_branch": item.BaseBranch, }) } data := map[string]interface{}{ diff --git a/apps/backend/internal/task/service/service_repository_sets_base_branch_test.go b/apps/backend/internal/task/service/service_repository_sets_base_branch_test.go new file mode 100644 index 0000000000..bf4010776e --- /dev/null +++ b/apps/backend/internal/task/service/service_repository_sets_base_branch_test.go @@ -0,0 +1,150 @@ +package service + +import ( + "context" + "errors" + "testing" +) + +func TestCreateRepositorySetPersistsMemberBasesAndPublishesThem(t *testing.T) { + svc, eventBus, repo := createTestService(t) + seedSetWorkspace(t, svc, repo) + + set, err := svc.CreateRepositorySet(context.Background(), &CreateRepositorySetRequest{ + WorkspaceID: "ws-1", + Name: "Branch set", + Repositories: []RepositorySetMemberInput{ + {RepositoryID: "repo-web", BaseBranch: "develop"}, + {RepositoryID: "repo-gateway"}, + }, + }) + if err != nil { + t.Fatalf("CreateRepositorySet: %v", err) + } + if len(set.Items) != 2 || set.Items[0].BaseBranch != "develop" || set.Items[1].BaseBranch != "" { + t.Fatalf("created items = %+v", set.Items) + } + + events := eventBus.GetPublishedEvents() + if len(events) != 1 { + t.Fatalf("published events = %#v, want one event", events) + } + data := events[0].Data.(map[string]interface{}) + items := data["repositories"].([]map[string]interface{}) + if items[0]["base_branch"] != "develop" || items[1]["base_branch"] != "" { + t.Fatalf("event members = %#v", items) + } +} + +func TestRepositorySetRejectsUnsafeBaseWithoutWriting(t *testing.T) { + svc, _, repo := createTestService(t) + seedSetWorkspace(t, svc, repo) + + _, err := svc.CreateRepositorySet(context.Background(), &CreateRepositorySetRequest{ + WorkspaceID: "ws-1", + Name: "Unsafe", + Repositories: []RepositorySetMemberInput{ + {RepositoryID: "repo-web", BaseBranch: "--upload-pack=evil"}, + }, + }) + if !errors.Is(err, ErrInvalidRepositorySet) { + t.Fatalf("unsafe base error = %v, want ErrInvalidRepositorySet", err) + } + sets, err := svc.ListRepositorySets(context.Background(), "ws-1") + if err != nil { + t.Fatalf("ListRepositorySets: %v", err) + } + if len(sets) != 0 { + t.Fatalf("rejected set wrote %d rows", len(sets)) + } +} + +func TestRepositorySetRejectsMalformedBaseRefWithoutWriting(t *testing.T) { + for _, baseBranch := range []string{"main/", "a//b"} { + t.Run(baseBranch, func(t *testing.T) { + svc, _, repo := createTestService(t) + seedSetWorkspace(t, svc, repo) + + _, err := svc.CreateRepositorySet(context.Background(), &CreateRepositorySetRequest{ + WorkspaceID: "ws-1", + Name: "Malformed base", + Repositories: []RepositorySetMemberInput{ + {RepositoryID: "repo-web", BaseBranch: baseBranch}, + }, + }) + if !errors.Is(err, ErrInvalidRepositorySet) { + t.Fatalf("malformed base error = %v, want ErrInvalidRepositorySet", err) + } + }) + } +} + +func TestRepositorySetUpdateReplacesBasesAtomically(t *testing.T) { + svc, _, repo := createTestService(t) + seedSetWorkspace(t, svc, repo) + set := createFullStackSet(t, svc) + + updated, err := svc.UpdateRepositorySet(context.Background(), set.ID, &UpdateRepositorySetRequest{ + Repositories: &[]RepositorySetMemberInput{ + {RepositoryID: "repo-orders", BaseBranch: "release"}, + }, + }) + if err != nil { + t.Fatalf("UpdateRepositorySet: %v", err) + } + if len(updated.Items) != 1 || updated.Items[0].BaseBranch != "release" { + t.Fatalf("updated items = %+v", updated.Items) + } + + bad := []RepositorySetMemberInput{{RepositoryID: "repo-web", BaseBranch: "bad branch"}} + _, err = svc.UpdateRepositorySet(context.Background(), set.ID, &UpdateRepositorySetRequest{ + Name: stringPointer("Rejected"), + Repositories: &bad, + }) + if !errors.Is(err, ErrInvalidRepositorySet) { + t.Fatalf("unsafe update error = %v, want ErrInvalidRepositorySet", err) + } + loaded, err := svc.GetRepositorySet(context.Background(), set.ID) + if err != nil { + t.Fatalf("GetRepositorySet: %v", err) + } + if loaded.Name != "Full-stack" || len(loaded.Items) != 1 || loaded.Items[0].BaseBranch != "release" { + t.Fatalf("rejected update changed set = %+v", loaded) + } +} + +func TestRepositorySetRejectsConflictingMemberInputs(t *testing.T) { + svc, _, repo := createTestService(t) + seedSetWorkspace(t, svc, repo) + ids := []string{"repo-web"} + members := []RepositorySetMemberInput{{RepositoryID: "repo-gateway"}} + + _, err := svc.CreateRepositorySet(context.Background(), &CreateRepositorySetRequest{ + WorkspaceID: "ws-1", + Name: "Conflict", + RepositoryIDs: ids, + Repositories: members, + }) + if !errors.Is(err, ErrInvalidRepositorySet) { + t.Fatalf("conflicting create error = %v, want ErrInvalidRepositorySet", err) + } + + _, err = svc.CreateRepositorySet(context.Background(), &CreateRepositorySetRequest{ + WorkspaceID: "ws-1", + Name: "Conflict empty members", + RepositoryIDs: ids, + Repositories: []RepositorySetMemberInput{}, + }) + if !errors.Is(err, ErrInvalidRepositorySet) { + t.Fatalf("conflicting create with empty members error = %v, want ErrInvalidRepositorySet", err) + } + + set := createFullStackSet(t, svc) + _, err = svc.UpdateRepositorySet(context.Background(), set.ID, &UpdateRepositorySetRequest{ + RepositoryIDs: &ids, + Repositories: &members, + }) + if !errors.Is(err, ErrInvalidRepositorySet) { + t.Fatalf("conflicting update error = %v, want ErrInvalidRepositorySet", err) + } +} diff --git a/apps/web/app/settings/workspace/use-workspace-repository-sets.ts b/apps/web/app/settings/workspace/use-workspace-repository-sets.ts index f9f2bdaeb6..5768c0a0a9 100644 --- a/apps/web/app/settings/workspace/use-workspace-repository-sets.ts +++ b/apps/web/app/settings/workspace/use-workspace-repository-sets.ts @@ -11,14 +11,21 @@ import type { RepositorySet } from "@/lib/types/http"; /** * An in-progress set edit. `setId` is null while creating. * - * `repositoryIds` is ordered, and that order is what applying the set will use, - * so the editor's selection order is meaningful rather than incidental. + * `members` is ordered, and that order is what applying the set will use, so + * the editor's order is meaningful rather than incidental. A member's base is + * optional; an empty value means that task creation should use its normal + * default for that repository. */ +export type RepositorySetDraftMember = { + repositoryId: string; + baseBranch: string; +}; + export type RepositorySetDraft = { setId: string | null; name: string; description: string; - repositoryIds: string[]; + members: RepositorySetDraftMember[]; }; /** @@ -39,7 +46,7 @@ export function useWorkspaceRepositorySets({ workspaceId }: { workspaceId: strin const startCreate = useCallback(() => { setError(null); - setDraft({ setId: null, name: "", description: "", repositoryIds: [] }); + setDraft({ setId: null, name: "", description: "", members: [] }); }, []); const startEdit = useCallback((set: RepositorySet) => { @@ -48,7 +55,10 @@ export function useWorkspaceRepositorySets({ workspaceId }: { workspaceId: strin setId: set.id, name: set.name, description: set.description, - repositoryIds: set.repositories.map((member) => member.repository_id as string), + members: set.repositories.map((member) => ({ + repositoryId: member.repository_id as string, + baseBranch: member.base_branch ?? "", + })), }); }, []); @@ -64,22 +74,26 @@ export function useWorkspaceRepositorySets({ workspaceId }: { workspaceId: strin const submitDraft = useCallback(async () => { if (!draft || saving) return; const name = draft.name.trim(); - if (!name || draft.repositoryIds.length === 0) return; + if (!name || draft.members.length === 0) return; setSaving(true); setError(null); try { - // A supplied repository_ids replaces the whole membership list, which is - // also how the editor expresses a reorder. + // A supplied repositories list replaces the whole membership list, which + // is also how the editor expresses a reorder and base change. + const repositories = draft.members.map((member) => ({ + repositoryId: member.repositoryId, + baseBranch: member.baseBranch, + })); const saved = draft.setId ? await updateRepositorySet(draft.setId, { name, description: draft.description.trim(), - repositoryIds: draft.repositoryIds, + repositories, }) : await createRepositorySet(workspaceId, { name, description: draft.description.trim(), - repositoryIds: draft.repositoryIds, + repositories, }); upsertRepositorySet(workspaceId, saved); setDraft(null); diff --git a/apps/web/app/settings/workspace/workspace-repository-set-editor-members.tsx b/apps/web/app/settings/workspace/workspace-repository-set-editor-members.tsx new file mode 100644 index 0000000000..b7c179929e --- /dev/null +++ b/apps/web/app/settings/workspace/workspace-repository-set-editor-members.tsx @@ -0,0 +1,352 @@ +"use client"; + +import { useMemo, useState } from "react"; +import { IconArrowDown, IconArrowUp, IconGitBranch, IconRefresh, IconX } from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; +import { Input } from "@kandev/ui/input"; +import { controlSizingClassName } from "@kandev/ui/control-sizing"; +import { useTranslation } from "react-i18next"; + +import { Pill } from "@/components/task-create-dialog-pill"; +import { buildBaseBranchOptions } from "@/components/task-create-dialog-repo-base-branch"; +import { Combobox, type ComboboxOption } from "@/components/combobox"; +import { useBranches } from "@/hooks/domains/workspace/use-repository-branches"; +import { scoreBranch } from "@/lib/utils/branch-filter"; +import type { Repository } from "@/lib/types/http"; +import type { RepositorySetDraft, RepositorySetDraftMember } from "./use-workspace-repository-sets"; + +type RepositorySetMembersFieldProps = { + workspaceId: string; + draft: RepositorySetDraft; + repositories: Repository[]; + onChange: (patch: Partial) => void; +}; + +function useRepositorySetMemberActions({ + draft, + repositories, + onChange, +}: Omit) { + const [memberSearch, setMemberSearch] = useState(""); + const selectedIds = useMemo( + () => new Set(draft.members.map((member) => member.repositoryId)), + [draft.members], + ); + const availableRepositories = useMemo( + () => repositories.filter((repository) => !selectedIds.has(repository.id as string)), + [repositories, selectedIds], + ); + const visibleMembers = useMemo(() => { + const query = memberSearch.trim().toLowerCase(); + return draft.members + .map((member, index) => ({ member, index })) + .filter(({ member }) => { + if (!query) return true; + const repository = repositories.find((candidate) => candidate.id === member.repositoryId); + return [repository?.name, repository?.local_path, member.baseBranch] + .filter(Boolean) + .some((value) => value?.toLowerCase().includes(query)); + }); + }, [draft.members, memberSearch, repositories]); + + const addRepository = (repositoryId: string) => { + if (!repositoryId || selectedIds.has(repositoryId)) return; + onChange({ members: [...draft.members, { repositoryId, baseBranch: "" }] }); + }; + const updateMember = (index: number, patch: Partial) => { + onChange({ + members: draft.members.map((member, memberIndex) => + memberIndex === index ? { ...member, ...patch } : member, + ), + }); + }; + const removeMember = (index: number) => { + onChange({ members: draft.members.filter((_, memberIndex) => memberIndex !== index) }); + }; + const moveMember = (index: number, delta: number) => { + const target = index + delta; + if (target < 0 || target >= draft.members.length) return; + const members = [...draft.members]; + [members[index], members[target]] = [members[target], members[index]]; + onChange({ members }); + }; + const resetBases = () => { + onChange({ members: draft.members.map((member) => ({ ...member, baseBranch: "" })) }); + }; + + return { + memberSearch, + setMemberSearch, + availableRepositories, + visibleMembers, + addRepository, + updateMember, + removeMember, + moveMember, + resetBases, + hasSavedBases: draft.members.some((member) => member.baseBranch !== ""), + }; +} + +export function RepositorySetMembersField({ + workspaceId, + draft, + repositories, + onChange, +}: RepositorySetMembersFieldProps) { + const { t } = useTranslation(); + const { + memberSearch, + setMemberSearch, + availableRepositories, + visibleMembers, + addRepository, + updateMember, + removeMember, + moveMember, + resetBases, + hasSavedBases, + } = useRepositorySetMemberActions({ draft, repositories, onChange }); + + return ( +
+
+
+

{t("workspaces:repositorySetsMembersLabel")}

+

+ {t("workspaces:repositorySetsMembersHint")} +

+
+ +
+ {draft.members.length > 0 ? ( +
+ setMemberSearch(event.target.value)} + placeholder={t("workspaces:repositorySetsSearchSelected")} + aria-label={t("workspaces:repositorySetsSearchSelected")} + data-testid="repository-set-member-search" + /> +
+ {visibleMembers.length === 0 ? ( +

+ {t("workspaces:repositorySetsNoMatchingMembers")} +

+ ) : ( + visibleMembers.map(({ member, index }) => ( + candidate.id === member.repositoryId, + )} + canMoveUp={!memberSearch && index > 0} + canMoveDown={!memberSearch && index < draft.members.length - 1} + onBaseBranchChange={(baseBranch) => updateMember(index, { baseBranch })} + onMoveUp={() => moveMember(index, -1)} + onMoveDown={() => moveMember(index, 1)} + onRemove={() => removeMember(index)} + /> + )) + )} +
+
+ ) : ( +

+ {t("workspaces:repositorySetsNoMembersSelected")} +

+ )} + +
+ ); +} + +function repositoryOption(repository: Repository): ComboboxOption { + const label = repository.name || repository.provider_name || (repository.id as string); + const description = repository.local_path || repository.remote_url || ""; + return { + value: repository.id as string, + label, + description, + keywords: [label, description, repository.provider_owner, repository.provider_name].filter( + Boolean, + ), + renderLabel: () => ( + + {label} + {description ? ( + {description} + ) : null} + + ), + }; +} + +type RepositorySetMemberRowProps = { + workspaceId: string; + member: RepositorySetDraftMember; + repository?: Repository; + canMoveUp: boolean; + canMoveDown: boolean; + onBaseBranchChange: (value: string) => void; + onMoveUp: () => void; + onMoveDown: () => void; + onRemove: () => void; +}; + +function RepositorySetMemberRow({ + workspaceId, + member, + repository, + canMoveUp, + canMoveDown, + onBaseBranchChange, + onMoveUp, + onMoveDown, + onRemove, +}: RepositorySetMemberRowProps) { + const { t } = useTranslation(); + return ( +
+
+

{repository?.name ?? member.repositoryId}

+ {repository?.local_path ? ( +

+ {repository.local_path} +

+ ) : null} +
+ {repository ? ( + + ) : ( +

{t("workspaces:repositorySetsMemberMissing")}

+ )} +
+ + + +
+
+ ); +} + +function RepositorySetBaseBranchPicker({ + workspaceId, + member, + repository, + onChange, +}: { + workspaceId: string; + member: RepositorySetDraftMember; + repository: Repository; + onChange: (value: string) => void; +}) { + const { t } = useTranslation(); + const [branchOpen, setBranchOpen] = useState(false); + const { branches, isLoaded, isLoading, refresh } = useBranches( + { kind: "id", workspaceId, repositoryId: repository.id as string }, + branchOpen, + ); + const options = useMemo(() => { + return buildBaseBranchOptions({ + branches, + branchesLoaded: isLoaded, + savedBaseBranch: member.baseBranch, + defaultBranch: repository.default_branch ?? "", + }); + }, [branches, isLoaded, member.baseBranch, repository.default_branch]); + const taskDefaultLabel = + options.find((option) => option.value === "")?.label ?? + t("workspaces:repositorySetsTaskDefaultNoBranch"); + + return ( + } + value={member.baseBranch || taskDefaultLabel} + selectedValue={member.baseBranch} + options={options} + onSelect={onChange} + disabled={false} + placeholder={taskDefaultLabel} + searchPlaceholder={t("task:searchBranches")} + emptyMessage={t("task:noBranches")} + onRefresh={refresh ? () => void refresh() : undefined} + refreshing={isLoading} + ariaLabel={t("workspaces:repositorySetsBaseBranchFor", { name: repository.name })} + testId={`repository-set-base-${member.repositoryId}`} + dropdownTestId={`repository-set-base-dropdown-${member.repositoryId}`} + triggerClassName={controlSizingClassName( + "standard", + "min-w-0 w-full border border-input bg-background px-2 hover:bg-background", + )} + onOpenChange={setBranchOpen} + filter={scoreBranch} + /> + ); +} diff --git a/apps/web/app/settings/workspace/workspace-repository-set-editor.test.tsx b/apps/web/app/settings/workspace/workspace-repository-set-editor.test.tsx new file mode 100644 index 0000000000..09da0a7b60 --- /dev/null +++ b/apps/web/app/settings/workspace/workspace-repository-set-editor.test.tsx @@ -0,0 +1,183 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { TooltipProvider } from "@kandev/ui/tooltip"; + +import type { Repository } from "@/lib/types/http"; +import { repositoryId, workspaceId } from "@/lib/types/ids"; +import type { RepositorySetDraft } from "./use-workspace-repository-sets"; + +const mockUseBranches = vi.fn(); +const mockRefresh = vi.fn(); + +vi.mock("@/hooks/use-responsive-breakpoint", () => ({ + useResponsiveBreakpoint: () => ({ isMobile: false }), +})); + +vi.mock("@/hooks/domains/workspace/use-repository-branches", () => ({ + useBranches: (...args: unknown[]) => mockUseBranches(...args), +})); + +import { RepositorySetEditorDialog } from "./workspace-repository-set-editor"; + +const repository: Repository = { + id: repositoryId("repo-web"), + workspace_id: workspaceId("ws-1"), + name: "web", + source_type: "local", + local_path: "/repos/web", + provider: "", + provider_repo_id: "", + provider_owner: "", + provider_name: "", + default_branch: "main", + worktree_branch_prefix: "feature/", + pull_before_worktree: false, + setup_script: "", + cleanup_script: "", + dev_script: "", + copy_files: "", + created_at: "2026-08-17T09:00:00Z", + updated_at: "2026-08-17T09:00:00Z", +}; +const repositoryWithoutDefaultBranch: Repository = { ...repository, default_branch: "" }; +const BASE_BRANCH_TEST_ID = "repository-set-base-repo-web"; + +function draft(baseBranch = ""): RepositorySetDraft { + return { + setId: "set-1", + name: "Full-stack", + description: "", + members: [{ repositoryId: "repo-web", baseBranch }], + }; +} + +function renderEditor( + currentDraft = draft(), + onChange = vi.fn(), + currentRepository: Repository = repository, +) { + return render( + + + , + ); +} + +beforeEach(() => { + mockUseBranches.mockReset(); + mockRefresh.mockReset(); + mockUseBranches.mockImplementation((_source: unknown, enabled: boolean) => ({ + branches: enabled + ? [ + { name: "main", type: "local", remote: "" }, + { name: "develop", type: "local", remote: "" }, + { name: "main", type: "remote", remote: "origin" }, + ] + : [], + isLoaded: enabled, + isLoading: false, + refresh: mockRefresh, + })); +}); + +afterEach(() => cleanup()); + +describe("RepositorySetEditorDialog base branch picker", () => { + it("does not load branches until a member picker opens", () => { + renderEditor(); + + expect(mockUseBranches).toHaveBeenCalledWith( + { kind: "id", workspaceId: "ws-1", repositoryId: "repo-web" }, + false, + ); + expect(mockUseBranches).not.toHaveBeenCalledWith( + { kind: "id", workspaceId: "ws-1", repositoryId: "repo-web" }, + true, + ); + + fireEvent.click(screen.getByTestId(BASE_BRANCH_TEST_ID)); + + expect(mockUseBranches).toHaveBeenCalledWith( + { kind: "id", workspaceId: "ws-1", repositoryId: "repo-web" }, + true, + ); + }); + + it("keeps a saved branch visible before the lazy branch request settles", () => { + renderEditor(draft("retired")); + + expect(screen.getByTestId(BASE_BRANCH_TEST_ID).textContent).toContain("retired"); + }); + + it("keeps an unavailable saved branch visible and disabled after loading", () => { + renderEditor(draft("retired")); + + fireEvent.click(screen.getByTestId(BASE_BRANCH_TEST_ID)); + + const unavailable = screen + .getAllByRole("option") + .find((option) => option.textContent?.includes("retired")); + expect(unavailable).toBeDefined(); + expect(unavailable?.getAttribute("aria-disabled")).toBe("true"); + expect(screen.getByTestId(BASE_BRANCH_TEST_ID).textContent).toContain("retired"); + }); + + it("shares the New Task grouped branch picker search, origin labels, and refresh", () => { + renderEditor(); + + fireEvent.click(screen.getByTestId(BASE_BRANCH_TEST_ID)); + + expect(screen.getByPlaceholderText("Search branches...")).toBeTruthy(); + expect(screen.getByText("Branches")).toBeTruthy(); + expect(screen.getByText("origin/main")).toBeTruthy(); + expect(screen.getByText("origin")).toBeTruthy(); + expect(screen.getAllByText("local").length).toBeGreaterThan(0); + + fireEvent.change(screen.getByPlaceholderText("Search branches..."), { + target: { value: "origin" }, + }); + expect(screen.getByText("origin/main")).toBeTruthy(); + expect(screen.queryByText("develop")).toBeNull(); + + fireEvent.click(screen.getByTestId("branch-refresh-button")); + expect(mockRefresh).toHaveBeenCalledOnce(); + }); + + it("uses a no-branch Task default label when the repository has no default", () => { + renderEditor(draft(), vi.fn(), repositoryWithoutDefaultBranch); + + expect(screen.getByTestId(BASE_BRANCH_TEST_ID).textContent).toBe("Task default"); + fireEvent.click(screen.getByTestId(BASE_BRANCH_TEST_ID)); + expect(screen.getByRole("option", { name: "Task default" })).toBeTruthy(); + }); + + it("disables reordering while the member list is filtered", () => { + renderEditor({ + ...draft(), + members: [ + { repositoryId: "repo-web", baseBranch: "" }, + { repositoryId: "repo-missing", baseBranch: "retired" }, + ], + }); + + fireEvent.change(screen.getByTestId("repository-set-member-search"), { + target: { value: "retired" }, + }); + + expect( + (screen.getByTestId("repository-set-move-up-repo-missing") as HTMLButtonElement).disabled, + ).toBe(true); + expect( + (screen.getByTestId("repository-set-move-down-repo-missing") as HTMLButtonElement).disabled, + ).toBe(true); + }); +}); diff --git a/apps/web/app/settings/workspace/workspace-repository-set-editor.tsx b/apps/web/app/settings/workspace/workspace-repository-set-editor.tsx index 9c8953ea9f..98469890c1 100644 --- a/apps/web/app/settings/workspace/workspace-repository-set-editor.tsx +++ b/apps/web/app/settings/workspace/workspace-repository-set-editor.tsx @@ -1,8 +1,7 @@ "use client"; -import { IconArrowDown, IconArrowUp } from "@tabler/icons-react"; +import { useState } from "react"; import { Button } from "@kandev/ui/button"; -import { Checkbox } from "@kandev/ui/checkbox"; import { Dialog, DialogContent, @@ -11,14 +10,26 @@ import { DialogHeader, DialogTitle, } from "@kandev/ui/dialog"; +import { + Drawer, + DrawerContent, + DrawerDescription, + DrawerFooter, + DrawerHeader, + DrawerTitle, +} from "@kandev/ui/drawer"; import { Input } from "@kandev/ui/input"; import { Textarea } from "@kandev/ui/textarea"; import { useTranslation } from "react-i18next"; +import { useResponsiveBreakpoint } from "@/hooks/use-responsive-breakpoint"; +import { TaskCreateDialogPopoverContainerProvider } from "@/hooks/use-task-create-dialog-popover-container"; import type { Repository } from "@/lib/types/http"; +import { RepositorySetMembersField } from "./workspace-repository-set-editor-members"; import type { RepositorySetDraft } from "./use-workspace-repository-sets"; type RepositorySetEditorDialogProps = { + workspaceId: string; draft: RepositorySetDraft | null; repositories: Repository[]; error: string | null; @@ -28,12 +39,74 @@ type RepositorySetEditorDialogProps = { onSubmit: () => void; }; +type RepositorySetEditorFooterProps = { + formId: string; + canSave: boolean; + isMobile: boolean; + onClose: () => void; +}; + +function RepositorySetEditorFooter({ + formId, + canSave, + isMobile, + onClose, +}: RepositorySetEditorFooterProps) { + const { t } = useTranslation(); + if (isMobile) { + return ( + + + + + ); + } + return ( + + + + + ); +} + /** - * Creates or edits one set: name, description, and which repositories belong in - * which order. Member selection offers only this workspace's live repositories, - * so a set can never be given a member the backend would reject. + * Creates or edits one set. The desktop dialog and phone drawer share the same + * form body, so member order and saved-base behavior do not diverge by device. */ export function RepositorySetEditorDialog({ + workspaceId, draft, repositories, error, @@ -43,13 +116,65 @@ export function RepositorySetEditorDialog({ onSubmit, }: RepositorySetEditorDialogProps) { const { t } = useTranslation(); + const { isMobile } = useResponsiveBreakpoint(); + const [popoverContainer, setPopoverContainer] = useState(null); if (!draft) return null; - const canSave = draft.name.trim().length > 0 && draft.repositoryIds.length > 0 && !saving; + + const formId = "repository-set-editor-form"; + const canSave = draft.name.trim().length > 0 && draft.members.length > 0 && !saving; + const body = ( + + + + ); + const footer = ( + + ); + + if (isMobile) { + return ( + !open && onClose()}> + + + + {draft.setId + ? t("workspaces:repositorySetsEditTitle") + : t("workspaces:repositorySetsCreateTitle")} + + {t("workspaces:repositorySetsEditorDescription")} + + {body} + {footer} + + + ); + } return ( - (next ? undefined : onClose())}> - - + !open && onClose()}> + + {draft.setId ? t("workspaces:repositorySetsEditTitle") @@ -57,173 +182,84 @@ export function RepositorySetEditorDialog({ {t("workspaces:repositorySetsEditorDescription")} -
- -