diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index 5536869de8..5345fb1688 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -748,6 +748,9 @@ func registerTaskRoutes(p routeParams, planService *taskservice.PlanService, han taskhandlers.RegisterWorkspaceRoutes(p.router, p.gateway.Dispatcher, p.taskSvc, p.log) taskhandlers.RegisterWorkflowRoutes(p.router, p.gateway.Dispatcher, p.taskSvc, p.services.Workflow, p.log) taskH := taskhandlers.RegisterTaskRoutes(p.router, p.gateway.Dispatcher, p.taskSvc, p.orchestratorSvc, p.taskRepo, planService, p.log) + if p.services != nil && p.services.User != nil { + taskH.SetTaskCreateLastUsedRecorder(p.services.User) + } if handoffSvc != nil { taskH.SetHandoffService(handoffSvc) } diff --git a/apps/backend/internal/task/handlers/task_handlers.go b/apps/backend/internal/task/handlers/task_handlers.go index ac93a733d5..1a0fcdb8f4 100644 --- a/apps/backend/internal/task/handlers/task_handlers.go +++ b/apps/backend/internal/task/handlers/task_handlers.go @@ -10,6 +10,7 @@ import ( "github.com/kandev/kandev/internal/task/dto" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/task/service" + usermodels "github.com/kandev/kandev/internal/user/models" ws "github.com/kandev/kandev/pkg/websocket" "go.uber.org/zap" ) @@ -28,13 +29,18 @@ type handlerRepo interface { } type TaskHandlers struct { - service *service.Service - orchestrator OrchestratorStarter - repo handlerRepo - planService *service.PlanService - handoffSvc *service.HandoffService - onTaskCreatedWithPR func(ctx context.Context, taskID, sessionID, prURL, branch string) - logger *logger.Logger + service *service.Service + orchestrator OrchestratorStarter + repo handlerRepo + planService *service.PlanService + handoffSvc *service.HandoffService + taskCreateLastUsedRecorder taskCreateLastUsedRecorder + onTaskCreatedWithPR func(ctx context.Context, taskID, sessionID, prURL, branch string) + logger *logger.Logger +} + +type taskCreateLastUsedRecorder interface { + RecordTaskCreateLastUsed(ctx context.Context, patch usermodels.TaskCreateLastUsed) error } // SetHandoffService wires the office task-handoffs service used by the @@ -45,6 +51,10 @@ func (h *TaskHandlers) SetHandoffService(svc *service.HandoffService) { h.handoffSvc = svc } +func (h *TaskHandlers) SetTaskCreateLastUsedRecorder(recorder taskCreateLastUsedRecorder) { + h.taskCreateLastUsedRecorder = recorder +} + // SetOnTaskCreatedWithPR sets a callback invoked when a task is created with a PR URL // in one of its repository inputs. The callback runs in a background goroutine. func (h *TaskHandlers) SetOnTaskCreatedWithPR(fn func(ctx context.Context, taskID, sessionID, prURL, branch string)) { diff --git a/apps/backend/internal/task/handlers/task_http_handlers.go b/apps/backend/internal/task/handlers/task_http_handlers.go index 8d6f7e1d71..4f50465a80 100644 --- a/apps/backend/internal/task/handlers/task_http_handlers.go +++ b/apps/backend/internal/task/handlers/task_http_handlers.go @@ -15,6 +15,7 @@ import ( "github.com/kandev/kandev/internal/task/dto" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/task/service" + usermodels "github.com/kandev/kandev/internal/user/models" "github.com/kandev/kandev/internal/worktree" v1 "github.com/kandev/kandev/pkg/api/v1" "go.uber.org/zap" @@ -589,6 +590,7 @@ func (h *TaskHandlers) httpCreateTask(c *gin.Context) { // Use the backend-resolved workflow step ID (from the created task) instead of the request's resolvedStepID := taskDTO.WorkflowStepID h.handlePostCreateTaskSession(c, &response, taskDTO.ID, taskDTO.Description, body, resolvedStepID) + h.recordTaskCreateLastUsed(c.Request.Context(), body, repos) // Associate PR with task if any repository input contains a PR URL h.associatePRFromRepoInputs(taskDTO.ID, response.TaskSessionID, body.Repositories) @@ -596,6 +598,54 @@ func (h *TaskHandlers) httpCreateTask(c *gin.Context) { c.JSON(http.StatusOK, response) } +func (h *TaskHandlers) recordTaskCreateLastUsed(ctx context.Context, body httpCreateTaskRequest, repos []dto.TaskRepositoryInput) { + if h.taskCreateLastUsedRecorder == nil { + return + } + patch := buildTaskCreateLastUsedPatch(body, repos) + if err := h.taskCreateLastUsedRecorder.RecordTaskCreateLastUsed(ctx, patch); err != nil { + h.logger.Warn("failed to record task-create last-used settings", zap.Error(err)) + } +} + +func buildTaskCreateLastUsedPatch(body httpCreateTaskRequest, repos []dto.TaskRepositoryInput) usermodels.TaskCreateLastUsed { + patch := usermodels.TaskCreateLastUsed{ + AgentProfileID: body.AgentProfileID, + ExecutorProfileID: body.ExecutorProfileID, + } + for i, repo := range repos { + if repo.RepositoryID == "" { + continue + } + branch := taskCreateLastUsedBranch(body, i, repo) + patch.RepositoryID = repo.RepositoryID + patch.Branch = branch + break + } + return patch +} + +func taskCreateLastUsedBranch( + body httpCreateTaskRequest, + index int, + repo dto.TaskRepositoryInput, +) string { + if index < len(body.Repositories) && body.Repositories[index].FreshBranch { + raw := body.Repositories[index] + return firstNonEmpty(raw.BaseBranch, raw.CheckoutBranch) + } + return firstNonEmpty(repo.CheckoutBranch, repo.BaseBranch) +} + +func firstNonEmpty(values ...string) string { + for _, value := range values { + if value != "" { + return value + } + } + return "" +} + // commitFreshBranch wraps the post-CreateTask fresh-branch sequence: run the // destructive checkout, compensate by deleting the task if it fails, then // persist the rewritten BaseBranch onto the now-existing task. Returns false diff --git a/apps/backend/internal/task/handlers/task_http_handlers_test.go b/apps/backend/internal/task/handlers/task_http_handlers_test.go index a63240b5e8..8cf618638f 100644 --- a/apps/backend/internal/task/handlers/task_http_handlers_test.go +++ b/apps/backend/internal/task/handlers/task_http_handlers_test.go @@ -18,9 +18,12 @@ import ( "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/orchestrator" + "github.com/kandev/kandev/internal/task/dto" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/task/service" + usermodels "github.com/kandev/kandev/internal/user/models" v1 "github.com/kandev/kandev/pkg/api/v1" + ws "github.com/kandev/kandev/pkg/websocket" ) // captureOrchestrator records every LaunchSession request so tests can assert @@ -148,10 +151,28 @@ type captureCreateTaskRepo struct { updateStateErr error } +type captureTaskCreateLastUsedRecorder struct { + calls int + got usermodels.TaskCreateLastUsed +} + +func (m *captureTaskCreateLastUsedRecorder) RecordTaskCreateLastUsed(_ context.Context, patch usermodels.TaskCreateLastUsed) error { + m.calls++ + m.got = patch + return nil +} + func (m *captureCreateTaskRepo) GetWorkspaceTaskPrefix(_ context.Context, _ string) (string, string, error) { return "KAN", "wf-office", nil } +func (m *captureCreateTaskRepo) GetRepository(_ context.Context, id string) (*models.Repository, error) { + if id == "repo-2" { + return &models.Repository{ID: id, WorkspaceID: "ws-1", DefaultBranch: "main"}, nil + } + return nil, errors.New("repository not found") +} + func (m *captureCreateTaskRepo) CreateTask(_ context.Context, task *models.Task) error { m.captured = task return nil @@ -213,6 +234,230 @@ func TestHTTPCreateTask_ProjectIDReachesOfficePath(t *testing.T) { assert.Equal(t, "wf-office", repo.captured.WorkflowID, "office workflow should be auto-resolved") } +func TestHTTPCreateTaskRecordsFinalLastUsedSelections(t *testing.T) { + gin.SetMode(gin.TestMode) + log := newTestLogger(t) + + repo := &captureCreateTaskRepo{} + svc := service.NewService(service.Repos{ + Workspaces: repo, Tasks: repo, TaskRepos: repo, + Workflows: repo, Messages: repo, Turns: repo, + Sessions: repo, GitSnapshots: repo, RepoEntities: repo, + Executors: repo, Environments: repo, TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + recorder := &captureTaskCreateLastUsedRecorder{} + h := &TaskHandlers{service: svc, taskCreateLastUsedRecorder: recorder, logger: log} + + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Request = httptest.NewRequest(http.MethodPost, "/tasks", strings.NewReader(`{ + "workspace_id": "ws-1", + "workflow_id": "wf-1", + "workflow_step_id": "step-1", + "title": "Use current selections", + "repositories": [{ + "repository_id": "repo-2", + "base_branch": "main", + "checkout_branch": "feature/current" + }], + "agent_profile_id": "agent-2", + "executor_profile_id": "exec-profile-2" + }`)) + c.Request.Header.Set("Content-Type", "application/json") + + h.httpCreateTask(c) + + require.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String()) + require.Equal(t, 1, recorder.calls) + assert.Equal(t, usermodels.TaskCreateLastUsed{ + RepositoryID: "repo-2", + Branch: "feature/current", + AgentProfileID: "agent-2", + ExecutorProfileID: "exec-profile-2", + }, recorder.got) +} + +func TestHTTPCreateTaskRecordsRepositoryWithoutProfileIDs(t *testing.T) { + gin.SetMode(gin.TestMode) + log := newTestLogger(t) + + repo := &captureCreateTaskRepo{} + svc := service.NewService(service.Repos{ + Workspaces: repo, Tasks: repo, TaskRepos: repo, + Workflows: repo, Messages: repo, Turns: repo, + Sessions: repo, GitSnapshots: repo, RepoEntities: repo, + Executors: repo, Environments: repo, TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + recorder := &captureTaskCreateLastUsedRecorder{} + h := &TaskHandlers{service: svc, taskCreateLastUsedRecorder: recorder, logger: log} + + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Request = httptest.NewRequest(http.MethodPost, "/tasks", strings.NewReader(`{ + "workspace_id": "ws-1", + "workflow_id": "wf-1", + "workflow_step_id": "step-1", + "title": "Passive API task", + "repositories": [{ + "repository_id": "repo-2", + "base_branch": "main" + }] + }`)) + c.Request.Header.Set("Content-Type", "application/json") + + h.httpCreateTask(c) + + require.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String()) + require.Equal(t, 1, recorder.calls) + assert.Equal(t, usermodels.TaskCreateLastUsed{ + RepositoryID: "repo-2", + Branch: "main", + }, recorder.got) +} + +func TestBuildTaskCreateLastUsedPatchRecordsFirstWorkspaceRepository(t *testing.T) { + patch := buildTaskCreateLastUsedPatch(httpCreateTaskRequest{ + AgentProfileID: "agent-2", + ExecutorProfileID: "exec-profile-2", + }, []dto.TaskRepositoryInput{ + {RepositoryID: "repo-without-branch"}, + {RepositoryID: "repo-with-branch", CheckoutBranch: "feature/current"}, + }) + + assert.Equal(t, usermodels.TaskCreateLastUsed{ + RepositoryID: "repo-without-branch", + AgentProfileID: "agent-2", + ExecutorProfileID: "exec-profile-2", + }, patch) +} + +func TestBuildTaskCreateLastUsedPatchUsesFreshBranchRequestBase(t *testing.T) { + patch := buildTaskCreateLastUsedPatch(httpCreateTaskRequest{ + Repositories: []httpTaskRepositoryInput{{ + RepositoryID: "repo-2", + BaseBranch: "main", + CheckoutBranch: "task/use-current-selections", + FreshBranch: true, + }}, + }, []dto.TaskRepositoryInput{{ + RepositoryID: "repo-2", + BaseBranch: "task/use-current-selections", + }}) + + assert.Equal(t, usermodels.TaskCreateLastUsed{ + RepositoryID: "repo-2", + Branch: "main", + }, patch) +} + +func TestBuildTaskCreateLastUsedPatchRecordsRepositoryWithoutBranch(t *testing.T) { + patch := buildTaskCreateLastUsedPatch(httpCreateTaskRequest{ + AgentProfileID: "agent-2", + }, []dto.TaskRepositoryInput{ + {RepositoryID: "repo-without-branch"}, + }) + + assert.Equal(t, usermodels.TaskCreateLastUsed{ + RepositoryID: "repo-without-branch", + AgentProfileID: "agent-2", + }, patch) +} + +func TestBuildTaskCreateLastUsedPatchSkipsBranchWithoutWorkspaceRepository(t *testing.T) { + patch := buildTaskCreateLastUsedPatch(httpCreateTaskRequest{ + AgentProfileID: "agent-2", + }, []dto.TaskRepositoryInput{ + {LocalPath: "/tmp/repo", CheckoutBranch: "feature/local"}, + {GitHubURL: "https://github.com/kdlbs/example", BaseBranch: "main"}, + }) + + assert.Equal(t, usermodels.TaskCreateLastUsed{ + AgentProfileID: "agent-2", + }, patch) +} + +func TestWSCreateTaskRecordsFinalLastUsedSelections(t *testing.T) { + log := newTestLogger(t) + + repo := &captureCreateTaskRepo{} + svc := service.NewService(service.Repos{ + Workspaces: repo, Tasks: repo, TaskRepos: repo, + Workflows: repo, Messages: repo, Turns: repo, + Sessions: repo, GitSnapshots: repo, RepoEntities: repo, + Executors: repo, Environments: repo, TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + recorder := &captureTaskCreateLastUsedRecorder{} + h := &TaskHandlers{service: svc, taskCreateLastUsedRecorder: recorder, logger: log} + + msg, err := ws.NewRequest("msg-1", ws.ActionTaskCreate, map[string]any{ + "workspace_id": "ws-1", + "workflow_id": "wf-1", + "workflow_step_id": "step-1", + "title": "Use current selections", + "agent_profile_id": "agent-2", + "executor_profile_id": "exec-profile-2", + "repositories": []map[string]any{{ + "repository_id": "repo-2", + "checkout_branch": "feature/current", + }}, + }) + require.NoError(t, err) + + resp, err := h.wsCreateTask(context.Background(), msg) + + require.NoError(t, err) + require.Equal(t, ws.MessageTypeResponse, resp.Type) + require.Equal(t, 1, recorder.calls) + assert.Equal(t, usermodels.TaskCreateLastUsed{ + RepositoryID: "repo-2", + Branch: "feature/current", + AgentProfileID: "agent-2", + ExecutorProfileID: "exec-profile-2", + }, recorder.got) +} + +func TestWSCreateTaskRecordsFreshBranchRequestBase(t *testing.T) { + log := newTestLogger(t) + + repo := &captureCreateTaskRepo{} + svc := service.NewService(service.Repos{ + Workspaces: repo, Tasks: repo, TaskRepos: repo, + Workflows: repo, Messages: repo, Turns: repo, + Sessions: repo, GitSnapshots: repo, RepoEntities: repo, + Executors: repo, Environments: repo, TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + recorder := &captureTaskCreateLastUsedRecorder{} + h := &TaskHandlers{service: svc, taskCreateLastUsedRecorder: recorder, logger: log} + + msg, err := ws.NewRequest("msg-1", ws.ActionTaskCreate, map[string]any{ + "workspace_id": "ws-1", + "workflow_id": "wf-1", + "workflow_step_id": "step-1", + "title": "Use fresh branch", + "repositories": []map[string]any{{ + "repository_id": "repo-2", + "base_branch": "main", + "checkout_branch": "task/use-current-selections", + "fresh_branch": true, + }}, + }) + require.NoError(t, err) + + resp, err := h.wsCreateTask(context.Background(), msg) + + require.NoError(t, err) + require.Equal(t, ws.MessageTypeResponse, resp.Type) + require.Equal(t, 1, recorder.calls) + assert.Equal(t, usermodels.TaskCreateLastUsed{ + RepositoryID: "repo-2", + Branch: "main", + }, recorder.got) +} + func TestHTTPCreateTask_StartAgentReturnsSchedulingTask(t *testing.T) { gin.SetMode(gin.TestMode) log := newTestLogger(t) diff --git a/apps/backend/internal/task/handlers/task_ws_handlers.go b/apps/backend/internal/task/handlers/task_ws_handlers.go index fd2770e727..67719b3e48 100644 --- a/apps/backend/internal/task/handlers/task_ws_handlers.go +++ b/apps/backend/internal/task/handlers/task_ws_handlers.go @@ -177,6 +177,11 @@ func (h *TaskHandlers) wsCreateTask(ctx context.Context, msg *ws.Message) (*ws.M response.TaskSessionID = launchResp.SessionID response.AgentExecutionID = launchResp.AgentExecutionID } + h.recordTaskCreateLastUsed(ctx, httpCreateTaskRequest{ + AgentProfileID: req.AgentProfileID, + ExecutorProfileID: req.ExecutorProfileID, + Repositories: req.Repositories, + }, repos) return ws.NewResponse(msg.ID, msg.Action, response) } diff --git a/apps/backend/internal/user/models/models.go b/apps/backend/internal/user/models/models.go index e8623b3e43..440f60b5a5 100644 --- a/apps/backend/internal/user/models/models.go +++ b/apps/backend/internal/user/models/models.go @@ -129,7 +129,6 @@ type SidebarTaskPrefs struct { } type TaskCreateLastUsed struct { - // Empty fields are treated as "no change" when used as an update patch. RepositoryID string `json:"repository_id"` Branch string `json:"branch"` AgentProfileID string `json:"agent_profile_id"` diff --git a/apps/backend/internal/user/service/service.go b/apps/backend/internal/user/service/service.go index 0508ffe893..e54e117701 100644 --- a/apps/backend/internal/user/service/service.go +++ b/apps/backend/internal/user/service/service.go @@ -144,13 +144,41 @@ func (s *Service) UpdateUserSettings(ctx context.Context, req *UpdateUserSetting return nil, fmt.Errorf("%w: %s", ErrValidation, err.Error()) } settings.UpdatedAt = time.Now().UTC() - if err := s.repo.UpsertUserSettings(ctx, settings); err != nil { + var taskCreatePatch *models.TaskCreateLastUsed + if req.TaskCreateLastUsed != nil && !taskCreateLastUsedPatchEmpty(*req.TaskCreateLastUsed) { + taskCreatePatch = req.TaskCreateLastUsed + } + settings, err = s.repo.UpsertUserSettingsPreservingTaskCreateLastUsed(ctx, settings, taskCreatePatch) + if err != nil { return nil, err } s.publishUserSettingsEvent(ctx, settings) return settings, nil } +func (s *Service) RecordTaskCreateLastUsed(ctx context.Context, patch models.TaskCreateLastUsed) error { + if taskCreateLastUsedPatchEmpty(patch) { + return nil + } + settings, err := s.updateTaskCreateLastUsed(ctx, patch) + if err != nil { + return err + } + s.publishUserSettingsEvent(ctx, settings) + return nil +} + +func (s *Service) updateTaskCreateLastUsed(ctx context.Context, patch models.TaskCreateLastUsed) (*models.UserSettings, error) { + return s.repo.UpdateTaskCreateLastUsed(ctx, s.defaultUser, patch) +} + +func taskCreateLastUsedPatchEmpty(patch models.TaskCreateLastUsed) bool { + return patch.RepositoryID == "" && + patch.Branch == "" && + patch.AgentProfileID == "" && + patch.ExecutorProfileID == "" +} + // applyBasicSettings copies simple (non-validated) fields from req to settings. func applyBasicSettings(settings *models.UserSettings, req *UpdateUserSettingsRequest) error { if req.WorkspaceID != nil { @@ -418,9 +446,6 @@ func applyUserPreferenceBlobs(settings *models.UserSettings, req *UpdateUserSett if req.SidebarTaskPrefs != nil { settings.SidebarTaskPrefs = *req.SidebarTaskPrefs } - if req.TaskCreateLastUsed != nil { - mergeTaskCreateLastUsed(&settings.TaskCreateLastUsed, *req.TaskCreateLastUsed) - } if err := applyUserPreferenceBlob("jira_saved_views", req.JiraSavedViews, &settings.JiraSavedViews); err != nil { return err } @@ -470,21 +495,6 @@ func validateUserPreferenceBlob(field string, value json.RawMessage) error { } } -func mergeTaskCreateLastUsed(current *models.TaskCreateLastUsed, patch models.TaskCreateLastUsed) { - if patch.RepositoryID != "" { - current.RepositoryID = patch.RepositoryID - } - if patch.Branch != "" { - current.Branch = patch.Branch - } - if patch.AgentProfileID != "" { - current.AgentProfileID = patch.AgentProfileID - } - if patch.ExecutorProfileID != "" { - current.ExecutorProfileID = patch.ExecutorProfileID - } -} - func (s *Service) publishUserSettingsEvent(ctx context.Context, settings *models.UserSettings) { if s.eventBus == nil || settings == nil { return @@ -581,7 +591,8 @@ func (s *Service) ClearDefaultEditorID(ctx context.Context, editorID string) err } settings.DefaultEditorID = "" settings.UpdatedAt = time.Now().UTC() - if err := s.repo.UpsertUserSettings(ctx, settings); err != nil { + settings, err = s.repo.UpsertUserSettingsPreservingTaskCreateLastUsed(ctx, settings, nil) + if err != nil { return err } s.publishUserSettingsEvent(ctx, settings) diff --git a/apps/backend/internal/user/service/service_test.go b/apps/backend/internal/user/service/service_test.go index 7615eada81..20200c220b 100644 --- a/apps/backend/internal/user/service/service_test.go +++ b/apps/backend/internal/user/service/service_test.go @@ -1,12 +1,20 @@ package service import ( + "context" "encoding/json" + "errors" "fmt" "strings" "testing" + "time" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/user/models" + "github.com/kandev/kandev/internal/user/store" + "go.uber.org/zap" ) func ptr[T any](v T) *T { return &v } @@ -553,14 +561,206 @@ func TestApplyUserPreferenceBlobs(t *testing.T) { if settings.TaskCreateLastUsed.RepositoryID != "repo-1" { t.Fatalf("expected repository id to be preserved, got %q", settings.TaskCreateLastUsed.RepositoryID) } - if settings.TaskCreateLastUsed.Branch != "feature" { - t.Fatalf("expected branch to update, got %q", settings.TaskCreateLastUsed.Branch) + if settings.TaskCreateLastUsed.Branch != "main" { + t.Fatalf("expected task-create last-used to stay unchanged, got %q", settings.TaskCreateLastUsed.Branch) } if string(settings.GitHubSavedPresets) != `[{"id":"p1"}]` { t.Fatalf("expected GitHub presets to apply, got %s", string(settings.GitHubSavedPresets)) } } +func TestUpdateUserSettingsCombinesSettingsAndTaskCreatePatch(t *testing.T) { + log, err := logger.NewFromZap(zap.NewNop()) + if err != nil { + t.Fatalf("logger.NewFromZap: %v", err) + } + patch := models.TaskCreateLastUsed{ + RepositoryID: "repo-2", + Branch: "feature", + AgentProfileID: "agent-2", + } + updatedSettings := &models.UserSettings{ + UserID: store.DefaultUserID, + TerminalFontSize: 16, + TaskCreateLastUsed: models.TaskCreateLastUsed{ + RepositoryID: "repo-2", + Branch: "feature", + AgentProfileID: "agent-2", + }, + } + repo := &recordingUserRepository{ + getSettings: &models.UserSettings{UserID: store.DefaultUserID}, + preservingSettings: updatedSettings, + updateSettings: updatedSettings, + } + eventBus := &recordingEventBus{} + svc := NewService(repo, eventBus, log) + + settings, err := svc.UpdateUserSettings(context.Background(), &UpdateUserSettingsRequest{ + TerminalFontSize: ptr(16), + TaskCreateLastUsed: &patch, + }) + if err != nil { + t.Fatalf("UpdateUserSettings: %v", err) + } + + if settings != updatedSettings { + t.Fatalf("expected returned settings from preserving writer, got %+v", settings) + } + if repo.upsertUserSettingsPreservingLastUsedCalls != 1 { + t.Fatalf("expected one preserving settings write, got %d", repo.upsertUserSettingsPreservingLastUsedCalls) + } + if repo.updateCalls != 0 { + t.Fatalf("expected task-create patch to be folded into settings write, got %d separate update calls", repo.updateCalls) + } + if repo.preservingPatch == nil || *repo.preservingPatch != patch { + t.Fatalf("expected preserving write patch %+v, got %+v", patch, repo.preservingPatch) + } + if len(eventBus.publishedEvents) != 1 { + t.Fatalf("expected one settings event, got %d", len(eventBus.publishedEvents)) + } +} + +func TestClearDefaultEditorIDPreservesTaskCreateLastUsed(t *testing.T) { + log, err := logger.NewFromZap(zap.NewNop()) + if err != nil { + t.Fatalf("logger.NewFromZap: %v", err) + } + updatedSettings := &models.UserSettings{ + UserID: store.DefaultUserID, + DefaultEditorID: "", + TaskCreateLastUsed: models.TaskCreateLastUsed{ + RepositoryID: "repo-2", + Branch: "feature", + }, + } + repo := &recordingUserRepository{ + getSettings: &models.UserSettings{ + UserID: store.DefaultUserID, + DefaultEditorID: "editor-1", + TaskCreateLastUsed: models.TaskCreateLastUsed{ + RepositoryID: "repo-1", + Branch: "main", + }, + }, + preservingSettings: updatedSettings, + } + eventBus := &recordingEventBus{} + svc := NewService(repo, eventBus, log) + + if err := svc.ClearDefaultEditorID(context.Background(), "editor-1"); err != nil { + t.Fatalf("ClearDefaultEditorID: %v", err) + } + + if repo.upsertUserSettingsPreservingLastUsedCalls != 1 { + t.Fatalf("expected preserving settings upsert, got %d calls", repo.upsertUserSettingsPreservingLastUsedCalls) + } + if len(eventBus.publishedEvents) != 1 { + t.Fatalf("expected one settings event, got %d", len(eventBus.publishedEvents)) + } + data, ok := eventBus.publishedEvents[0].Data.(map[string]interface{}) + if !ok { + t.Fatalf("expected event data map, got %T", eventBus.publishedEvents[0].Data) + } + if data["task_create_last_used"] != updatedSettings.TaskCreateLastUsed { + t.Fatalf("expected event to include preserved task-create state %+v, got %+v", updatedSettings.TaskCreateLastUsed, data["task_create_last_used"]) + } +} + +func TestRecordTaskCreateLastUsed(t *testing.T) { + newTestService := func(repo *recordingUserRepository, eventBus *recordingEventBus) *Service { + log, err := logger.NewFromZap(zap.NewNop()) + if err != nil { + t.Fatalf("logger.NewFromZap: %v", err) + } + return NewService(repo, eventBus, log) + } + + t.Run("empty patch skips repo update and publish", func(t *testing.T) { + repo := &recordingUserRepository{} + eventBus := &recordingEventBus{} + svc := newTestService(repo, eventBus) + + if err := svc.RecordTaskCreateLastUsed(context.Background(), models.TaskCreateLastUsed{}); err != nil { + t.Fatalf("RecordTaskCreateLastUsed: %v", err) + } + if repo.updateCalls != 0 { + t.Fatalf("expected no repo update, got %d", repo.updateCalls) + } + if len(eventBus.publishedEvents) != 0 { + t.Fatalf("expected no settings event, got %d", len(eventBus.publishedEvents)) + } + }) + + t.Run("non-empty patch updates repo and publishes settings event", func(t *testing.T) { + patch := models.TaskCreateLastUsed{ + RepositoryID: "repo-1", + Branch: "feature", + AgentProfileID: "agent-1", + ExecutorProfileID: "exec-1", + } + updatedSettings := &models.UserSettings{ + UserID: store.DefaultUserID, + UpdatedAt: time.Unix(123, 0).UTC(), + TaskCreateLastUsed: models.TaskCreateLastUsed{ + RepositoryID: "repo-1", + Branch: "feature", + }, + } + repo := &recordingUserRepository{updateSettings: updatedSettings} + eventBus := &recordingEventBus{} + svc := newTestService(repo, eventBus) + + if err := svc.RecordTaskCreateLastUsed(context.Background(), patch); err != nil { + t.Fatalf("RecordTaskCreateLastUsed: %v", err) + } + if repo.updateCalls != 1 { + t.Fatalf("expected one update call, got %d", repo.updateCalls) + } + if repo.updateUserID != store.DefaultUserID { + t.Fatalf("expected update user id %q, got %q", store.DefaultUserID, repo.updateUserID) + } + if repo.updatePatch != patch { + t.Fatalf("expected patch %+v, got %+v", patch, repo.updatePatch) + } + if len(eventBus.publishedEvents) != 1 { + t.Fatalf("expected one published event, got %d", len(eventBus.publishedEvents)) + } + if eventBus.publishedSubjects[0] != events.UserSettingsUpdated { + t.Fatalf("expected subject %q, got %q", events.UserSettingsUpdated, eventBus.publishedSubjects[0]) + } + published := eventBus.publishedEvents[0] + if published.Type != events.UserSettingsUpdated { + t.Fatalf("expected event type %q, got %q", events.UserSettingsUpdated, published.Type) + } + data, ok := published.Data.(map[string]interface{}) + if !ok { + t.Fatalf("expected event data map, got %T", published.Data) + } + if data["task_create_last_used"] != updatedSettings.TaskCreateLastUsed { + t.Fatalf("expected event task-create state %+v, got %+v", updatedSettings.TaskCreateLastUsed, data["task_create_last_used"]) + } + }) + + t.Run("repo error is propagated without publishing", func(t *testing.T) { + repoErr := errors.New("update failed") + repo := &recordingUserRepository{updateErr: repoErr} + eventBus := &recordingEventBus{} + svc := newTestService(repo, eventBus) + + err := svc.RecordTaskCreateLastUsed(context.Background(), models.TaskCreateLastUsed{Branch: "feature"}) + if !errors.Is(err, repoErr) { + t.Fatalf("expected repo error, got %v", err) + } + if repo.updateCalls != 1 { + t.Fatalf("expected one update call, got %d", repo.updateCalls) + } + if len(eventBus.publishedEvents) != 0 { + t.Fatalf("expected no events, got %d", len(eventBus.publishedEvents)) + } + }) +} + func TestApplyUserPreferenceBlobsValidation(t *testing.T) { t.Run("accepts arrays objects and null", func(t *testing.T) { settings := &models.UserSettings{} @@ -605,6 +805,112 @@ func TestApplyUserPreferenceBlobsValidation(t *testing.T) { }) } +type recordingUserRepository struct { + getUserCalls int + getDefaultUserCalls int + getUserSettingsCalls int + upsertUserSettingsPreservingLastUsedCalls int + updateCalls int + updateUserID string + updatePatch models.TaskCreateLastUsed + updateSettings *models.UserSettings + updateErr error + getSettings *models.UserSettings + getErr error + preservingSettings *models.UserSettings + preservingPatch *models.TaskCreateLastUsed + preservingErr error + closeCalls int +} + +func (r *recordingUserRepository) GetUser(context.Context, string) (*models.User, error) { + r.getUserCalls++ + return nil, errors.New("unexpected GetUser call") +} + +func (r *recordingUserRepository) GetDefaultUser(context.Context) (*models.User, error) { + r.getDefaultUserCalls++ + return nil, errors.New("unexpected GetDefaultUser call") +} + +func (r *recordingUserRepository) GetUserSettings(context.Context, string) (*models.UserSettings, error) { + r.getUserSettingsCalls++ + if r.getErr != nil { + return nil, r.getErr + } + if r.getSettings != nil { + return r.getSettings, nil + } + return nil, errors.New("unexpected GetUserSettings call") +} + +func (r *recordingUserRepository) UpsertUserSettingsPreservingTaskCreateLastUsed( + _ context.Context, + _ *models.UserSettings, + patch *models.TaskCreateLastUsed, +) (*models.UserSettings, error) { + r.upsertUserSettingsPreservingLastUsedCalls++ + if patch != nil { + patchCopy := *patch + r.preservingPatch = &patchCopy + } + if r.preservingErr != nil { + return nil, r.preservingErr + } + if r.preservingSettings != nil { + return r.preservingSettings, nil + } + return nil, errors.New("unexpected UpsertUserSettingsPreservingTaskCreateLastUsed call") +} + +func (r *recordingUserRepository) UpdateTaskCreateLastUsed( + _ context.Context, + userID string, + patch models.TaskCreateLastUsed, +) (*models.UserSettings, error) { + r.updateCalls++ + r.updateUserID = userID + r.updatePatch = patch + if r.updateErr != nil { + return nil, r.updateErr + } + return r.updateSettings, nil +} + +func (r *recordingUserRepository) Close() error { + r.closeCalls++ + return nil +} + +type recordingEventBus struct { + publishedSubjects []string + publishedEvents []*bus.Event +} + +func (b *recordingEventBus) Publish(_ context.Context, subject string, event *bus.Event) error { + b.publishedSubjects = append(b.publishedSubjects, subject) + b.publishedEvents = append(b.publishedEvents, event) + return nil +} + +func (b *recordingEventBus) Subscribe(string, bus.EventHandler) (bus.Subscription, error) { + return nil, errors.New("unexpected Subscribe call") +} + +func (b *recordingEventBus) QueueSubscribe(string, string, bus.EventHandler) (bus.Subscription, error) { + return nil, errors.New("unexpected QueueSubscribe call") +} + +func (b *recordingEventBus) Request(context.Context, string, *bus.Event, time.Duration) (*bus.Event, error) { + return nil, errors.New("unexpected Request call") +} + +func (b *recordingEventBus) Close() {} + +func (b *recordingEventBus) IsConnected() bool { + return true +} + func TestApplyVoiceMode(t *testing.T) { t.Run("nil value leaves settings unchanged", func(t *testing.T) { settings := &models.UserSettings{ diff --git a/apps/backend/internal/user/store/sqlite.go b/apps/backend/internal/user/store/sqlite.go index b930a5e700..8e8f3ff867 100644 --- a/apps/backend/internal/user/store/sqlite.go +++ b/apps/backend/internal/user/store/sqlite.go @@ -4,10 +4,12 @@ import ( "context" "encoding/json" "fmt" + "strings" "time" "github.com/jmoiron/sqlx" + "github.com/kandev/kandev/internal/db/dialect" "github.com/kandev/kandev/internal/user/models" ) @@ -97,7 +99,11 @@ func (r *sqliteRepository) GetDefaultUser(ctx context.Context) (*models.User, er } func (r *sqliteRepository) GetUserSettings(ctx context.Context, userID string) (*models.UserSettings, error) { - row := r.ro.QueryRowContext(ctx, r.ro.Rebind(` + return r.getUserSettings(ctx, r.ro, userID) +} + +func (r *sqliteRepository) getUserSettings(ctx context.Context, conn *sqlx.DB, userID string) (*models.UserSettings, error) { + row := conn.QueryRowContext(ctx, conn.Rebind(` SELECT settings, updated_at FROM users WHERE id = ? `), userID) @@ -108,11 +114,191 @@ func (r *sqliteRepository) GetUserSettings(ctx context.Context, userID string) ( return settings, nil } -func (r *sqliteRepository) UpsertUserSettings(ctx context.Context, settings *models.UserSettings) error { +func (r *sqliteRepository) UpsertUserSettingsPreservingTaskCreateLastUsed( + ctx context.Context, + settings *models.UserSettings, + patch *models.TaskCreateLastUsed, +) (*models.UserSettings, error) { settings.UpdatedAt = time.Now().UTC() if settings.CreatedAt.IsZero() { settings.CreatedAt = settings.UpdatedAt } + settingsPayload, err := marshalUserSettingsPayload(settings) + if err != nil { + return nil, err + } + if dialect.IsPostgres(r.db.DriverName()) { + return r.upsertUserSettingsPreservingTaskCreateLastUsedPostgres(ctx, settings, settingsPayload, patch) + } + settingsExpr := `json_set( + json(?), + '$.task_create_last_used', + json(COALESCE(json_extract( + CASE WHEN settings IS NULL OR settings = 'null' OR settings = '' THEN '{}' ELSE settings END, + '$.task_create_last_used' + ), '{}')) + )` + args := []any{string(settingsPayload)} + if patch != nil { + patchArgs := makeTaskCreateLastUsedJSONSetArgs(*patch) + if len(patchArgs) > 0 { + placeholders := strings.TrimSuffix(strings.Repeat("?, ?, ", len(patchArgs)/2), ", ") + settingsExpr = fmt.Sprintf("json_set(%s, %s)", settingsExpr, placeholders) + args = append(args, patchArgs...) + } + } + query := fmt.Sprintf(` + UPDATE users + SET settings = %s, updated_at = ? + WHERE id = ? + `, settingsExpr) + args = append(args, settings.UpdatedAt, settings.UserID) + result, err := r.db.ExecContext(ctx, r.db.Rebind(query), args...) + if err != nil { + return nil, err + } + if err := checkUserSettingsRowsAffected(result, settings.UserID); err != nil { + return nil, err + } + return r.getUserSettings(ctx, r.db, settings.UserID) +} + +func (r *sqliteRepository) upsertUserSettingsPreservingTaskCreateLastUsedPostgres( + ctx context.Context, + settings *models.UserSettings, + settingsPayload []byte, + patch *models.TaskCreateLastUsed, +) (*models.UserSettings, error) { + query, patchArgs := buildPostgresUserSettingsPreservingTaskCreateLastUsedUpdate(patch) + args := append([]any{string(settingsPayload)}, patchArgs...) + args = append(args, settings.UpdatedAt, settings.UserID) + result, err := r.db.ExecContext(ctx, r.db.Rebind(query), args...) + if err != nil { + return nil, err + } + if err := checkUserSettingsRowsAffected(result, settings.UserID); err != nil { + return nil, err + } + return r.getUserSettings(ctx, r.db, settings.UserID) +} + +func (r *sqliteRepository) UpdateTaskCreateLastUsed(ctx context.Context, userID string, patch models.TaskCreateLastUsed) (*models.UserSettings, error) { + if dialect.IsPostgres(r.db.DriverName()) { + return r.updateTaskCreateLastUsedPostgres(ctx, userID, patch) + } + patchArgs := makeTaskCreateLastUsedJSONSetArgs(patch) + if len(patchArgs) == 0 { + return r.getUserSettings(ctx, r.db, userID) + } + base := "CASE WHEN settings IS NULL OR settings = 'null' OR settings = '' THEN '{}' ELSE settings END" + settingsExpr := fmt.Sprintf( + "json_set(%s, '$.task_create_last_used', json(COALESCE(json_extract(%s, '$.task_create_last_used'), '{}')))", + base, + base, + ) + placeholders := strings.TrimSuffix(strings.Repeat("?, ?, ", len(patchArgs)/2), ", ") + settingsExpr = fmt.Sprintf("json_set(%s, %s)", settingsExpr, placeholders) + query := ` + UPDATE users + SET settings = %s, updated_at = ? + WHERE id = ? + ` + now := time.Now().UTC() + args := append([]any{}, patchArgs...) + args = append(args, now, userID) + result, err := r.db.ExecContext(ctx, r.db.Rebind(fmt.Sprintf(query, settingsExpr)), args...) + if err != nil { + return nil, err + } + if err := checkUserSettingsRowsAffected(result, userID); err != nil { + return nil, err + } + return r.getUserSettings(ctx, r.db, userID) +} + +func (r *sqliteRepository) updateTaskCreateLastUsedPostgres(ctx context.Context, userID string, patch models.TaskCreateLastUsed) (*models.UserSettings, error) { + query, args := buildPostgresTaskCreateLastUsedUpdate(patch) + if len(args) == 0 { + return r.getUserSettings(ctx, r.db, userID) + } + now := time.Now().UTC() + args = append(args, now, userID) + result, err := r.db.ExecContext(ctx, r.db.Rebind(query), args...) + if err != nil { + return nil, err + } + if err := checkUserSettingsRowsAffected(result, userID); err != nil { + return nil, err + } + return r.getUserSettings(ctx, r.db, userID) +} + +func makeTaskCreateLastUsedJSONSetArgs(patch models.TaskCreateLastUsed) []any { + args := []any{} + if patch.RepositoryID != "" { + args = append(args, "$.task_create_last_used.repository_id", patch.RepositoryID) + } + if patch.Branch != "" || patch.RepositoryID != "" { + args = append(args, "$.task_create_last_used.branch", patch.Branch) + } + if patch.AgentProfileID != "" { + args = append(args, "$.task_create_last_used.agent_profile_id", patch.AgentProfileID) + } + if patch.ExecutorProfileID != "" { + args = append(args, "$.task_create_last_used.executor_profile_id", patch.ExecutorProfileID) + } + return args +} + +func buildPostgresTaskCreateLastUsedUpdate(patch models.TaskCreateLastUsed) (string, []any) { + base := "(CASE WHEN settings IS NULL OR settings = 'null' OR settings = '' THEN '{}'::jsonb ELSE settings::jsonb END)" + expr := fmt.Sprintf("jsonb_set(%s, '{task_create_last_used}', COALESCE(%s->'task_create_last_used', '{}'::jsonb), true)", base, base) + args := []any{} + expr, args = applyPostgresTaskCreateLastUsedPatch(expr, patch, args) + query := fmt.Sprintf(` + UPDATE users + SET settings = %s::text, updated_at = ? + WHERE id = ? + `, expr) + return query, args +} + +func buildPostgresUserSettingsPreservingTaskCreateLastUsedUpdate(patch *models.TaskCreateLastUsed) (string, []any) { + base := "(CASE WHEN settings IS NULL OR settings = 'null' OR settings = '' THEN '{}'::jsonb ELSE settings::jsonb END)" + expr := fmt.Sprintf("jsonb_set(?::jsonb, '{task_create_last_used}', COALESCE(%s->'task_create_last_used', '{}'::jsonb), true)", base) + args := []any{} + if patch != nil { + expr, args = applyPostgresTaskCreateLastUsedPatch(expr, *patch, args) + } + query := fmt.Sprintf(` + UPDATE users + SET settings = %s::text, updated_at = ? + WHERE id = ? + `, expr) + return query, args +} + +func applyPostgresTaskCreateLastUsedPatch(expr string, patch models.TaskCreateLastUsed, args []any) (string, []any) { + if patch.RepositoryID != "" { + expr = fmt.Sprintf("jsonb_set(%s, '{task_create_last_used,repository_id}', to_jsonb(?::text), true)", expr) + args = append(args, patch.RepositoryID) + } + if patch.Branch != "" || patch.RepositoryID != "" { + expr = fmt.Sprintf("jsonb_set(%s, '{task_create_last_used,branch}', to_jsonb(?::text), true)", expr) + args = append(args, patch.Branch) + } + if patch.AgentProfileID != "" { + expr = fmt.Sprintf("jsonb_set(%s, '{task_create_last_used,agent_profile_id}', to_jsonb(?::text), true)", expr) + args = append(args, patch.AgentProfileID) + } + if patch.ExecutorProfileID != "" { + expr = fmt.Sprintf("jsonb_set(%s, '{task_create_last_used,executor_profile_id}', to_jsonb(?::text), true)", expr) + args = append(args, patch.ExecutorProfileID) + } + return expr, args +} + +func marshalUserSettingsPayload(settings *models.UserSettings) ([]byte, error) { lspAutoStart := settings.LspAutoStartLanguages if lspAutoStart == nil { lspAutoStart = []string{} @@ -138,7 +324,7 @@ func (r *sqliteRepository) UpsertUserSettings(ctx context.Context, settings *mod if keyboardShortcuts == nil { keyboardShortcuts = map[string]interface{}{} } - settingsPayload, err := json.Marshal(map[string]interface{}{ + return json.Marshal(map[string]interface{}{ "workspace_id": settings.WorkspaceID, "kanban_view_mode": settings.KanbanViewMode, "workflow_filter_id": settings.WorkflowFilterID, @@ -175,27 +361,23 @@ func (r *sqliteRepository) UpsertUserSettings(ctx context.Context, settings *mod "system_metrics_display": settings.SystemMetricsDisplay, "voice_mode": settings.VoiceMode, }) - if err != nil { - return err - } - result, err := r.db.ExecContext(ctx, r.db.Rebind(` - UPDATE users - SET settings = ?, updated_at = ? - WHERE id = ? - `), string(settingsPayload), settings.UpdatedAt, settings.UserID) - if err != nil { - return err - } +} + +func checkUserSettingsRowsAffected(result sqlResult, userID string) error { rows, err := result.RowsAffected() if err != nil { return fmt.Errorf("failed to check rows affected: %w", err) } if rows == 0 { - return fmt.Errorf("user not found: %s", settings.UserID) + return fmt.Errorf("user not found: %s", userID) } return nil } +type sqlResult interface { + RowsAffected() (int64, error) +} + func scanUser(scanner interface{ Scan(dest ...any) error }) (*models.User, error) { user := &models.User{} if err := scanner.Scan(&user.ID, &user.Email, &user.CreatedAt, &user.UpdatedAt); err != nil { diff --git a/apps/backend/internal/user/store/sqlite_test.go b/apps/backend/internal/user/store/sqlite_test.go index 23078bcd2e..d200339ece 100644 --- a/apps/backend/internal/user/store/sqlite_test.go +++ b/apps/backend/internal/user/store/sqlite_test.go @@ -3,12 +3,14 @@ package store import ( "context" "encoding/json" + "strings" "testing" "time" "github.com/jmoiron/sqlx" _ "github.com/mattn/go-sqlite3" + "github.com/kandev/kandev/internal/testutil" "github.com/kandev/kandev/internal/user/models" ) @@ -16,6 +18,17 @@ type settingsScanner struct { raw string } +func upsertUserSettingsForTest(t *testing.T, repo *sqliteRepository, ctx context.Context, settings *models.UserSettings) { + t.Helper() + var patch *models.TaskCreateLastUsed + if settings.TaskCreateLastUsed != (models.TaskCreateLastUsed{}) { + patch = &settings.TaskCreateLastUsed + } + if _, err := repo.UpsertUserSettingsPreservingTaskCreateLastUsed(ctx, settings, patch); err != nil { + t.Fatalf("upsert settings: %v", err) + } +} + func (s settingsScanner) Scan(dest ...any) error { *(dest[0].(*string)) = s.raw *(dest[1].(*time.Time)) = time.Time{} @@ -90,9 +103,7 @@ func TestSQLiteRepositorySystemMetricsDisplayRoundTrip(t *testing.T) { t.Fatalf("get defaults: %v", err) } settings.SystemMetricsDisplay = models.SystemMetricsDisplaySettings{ShowInTopbar: true} - if err := repo.UpsertUserSettings(ctx, settings); err != nil { - t.Fatalf("upsert settings: %v", err) - } + upsertUserSettingsForTest(t, repo, ctx, settings) got, err := repo.GetUserSettings(ctx, DefaultUserID) if err != nil { t.Fatalf("get settings: %v", err) @@ -102,6 +113,343 @@ func TestSQLiteRepositorySystemMetricsDisplayRoundTrip(t *testing.T) { } } +func TestSQLiteRepositoryUpdateTaskCreateLastUsedPatchesNonEmptyFields(t *testing.T) { + conn, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + conn.SetMaxOpenConns(1) + t.Cleanup(func() { _ = conn.Close() }) + repo, err := newSQLiteRepositoryWithDB(conn, conn) + if err != nil { + t.Fatalf("new repo: %v", err) + } + + ctx := context.Background() + settings, err := repo.GetUserSettings(ctx, DefaultUserID) + if err != nil { + t.Fatalf("get defaults: %v", err) + } + settings.SidebarActiveViewID = "view-1" + settings.TaskCreateLastUsed = models.TaskCreateLastUsed{ + RepositoryID: "repo-1", + Branch: "main", + AgentProfileID: "agent-1", + ExecutorProfileID: "exec-1", + } + upsertUserSettingsForTest(t, repo, ctx, settings) + + got, err := repo.UpdateTaskCreateLastUsed(ctx, DefaultUserID, models.TaskCreateLastUsed{ + Branch: "feature", + AgentProfileID: "agent-2", + }) + if err != nil { + t.Fatalf("update task-create last-used: %v", err) + } + + if got.TaskCreateLastUsed.RepositoryID != "repo-1" { + t.Fatalf("repository id should be preserved, got %q", got.TaskCreateLastUsed.RepositoryID) + } + if got.TaskCreateLastUsed.Branch != "feature" { + t.Fatalf("branch should update, got %q", got.TaskCreateLastUsed.Branch) + } + if got.TaskCreateLastUsed.AgentProfileID != "agent-2" { + t.Fatalf("agent profile should update, got %q", got.TaskCreateLastUsed.AgentProfileID) + } + if got.TaskCreateLastUsed.ExecutorProfileID != "exec-1" { + t.Fatalf("executor profile should be preserved, got %q", got.TaskCreateLastUsed.ExecutorProfileID) + } + if got.SidebarActiveViewID != "view-1" { + t.Fatalf("unrelated settings should be preserved, got active view %q", got.SidebarActiveViewID) + } +} + +func TestSQLiteRepositoryUpdateTaskCreateLastUsedClearsBranchOnRepositoryChange(t *testing.T) { + conn, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + conn.SetMaxOpenConns(1) + t.Cleanup(func() { _ = conn.Close() }) + repo, err := newSQLiteRepositoryWithDB(conn, conn) + if err != nil { + t.Fatalf("new repo: %v", err) + } + + ctx := context.Background() + settings, err := repo.GetUserSettings(ctx, DefaultUserID) + if err != nil { + t.Fatalf("get defaults: %v", err) + } + settings.TaskCreateLastUsed = models.TaskCreateLastUsed{ + RepositoryID: "repo-before", + Branch: "main", + AgentProfileID: "agent-1", + ExecutorProfileID: "exec-1", + } + upsertUserSettingsForTest(t, repo, ctx, settings) + + got, err := repo.UpdateTaskCreateLastUsed(ctx, DefaultUserID, models.TaskCreateLastUsed{ + RepositoryID: "repo-after", + }) + if err != nil { + t.Fatalf("update task-create last-used: %v", err) + } + + if got.TaskCreateLastUsed.RepositoryID != "repo-after" { + t.Fatalf("repository id should update, got %q", got.TaskCreateLastUsed.RepositoryID) + } + if got.TaskCreateLastUsed.Branch != "" { + t.Fatalf("branch should clear on repository change, got %q", got.TaskCreateLastUsed.Branch) + } + if got.TaskCreateLastUsed.AgentProfileID != "agent-1" { + t.Fatalf("agent profile should be preserved, got %q", got.TaskCreateLastUsed.AgentProfileID) + } + if got.TaskCreateLastUsed.ExecutorProfileID != "exec-1" { + t.Fatalf("executor profile should be preserved, got %q", got.TaskCreateLastUsed.ExecutorProfileID) + } +} + +func TestBuildPostgresTaskCreateLastUsedUpdatePatchesNonEmptyFields(t *testing.T) { + query, args := buildPostgresTaskCreateLastUsedUpdate(models.TaskCreateLastUsed{ + RepositoryID: "repo-1", + Branch: "feature", + AgentProfileID: "agent-1", + ExecutorProfileID: "exec-1", + }) + + if strings.Contains(query, "json(") || strings.Contains(query, "json_extract") { + t.Fatalf("postgres update must not use sqlite JSON functions: %s", query) + } + if !strings.Contains(query, "jsonb_set") { + t.Fatalf("postgres update should use jsonb_set: %s", query) + } + if !strings.Contains(query, "{task_create_last_used,repository_id}") || + !strings.Contains(query, "{task_create_last_used,branch}") || + !strings.Contains(query, "{task_create_last_used,agent_profile_id}") || + !strings.Contains(query, "{task_create_last_used,executor_profile_id}") { + t.Fatalf("postgres update should patch task-create fields: %s", query) + } + if len(args) != 4 { + t.Fatalf("expected one arg per task-create field, got %d", len(args)) + } +} + +func TestBuildPostgresTaskCreateLastUsedUpdateClearsBranchOnRepositoryChange(t *testing.T) { + query, args := buildPostgresTaskCreateLastUsedUpdate(models.TaskCreateLastUsed{ + RepositoryID: "repo-after", + }) + + if !strings.Contains(query, "{task_create_last_used,repository_id}") || + !strings.Contains(query, "{task_create_last_used,branch}") { + t.Fatalf("postgres update should patch repository and clear branch: %s", query) + } + if strings.Contains(query, "{task_create_last_used,agent_profile_id}") || + strings.Contains(query, "{task_create_last_used,executor_profile_id}") { + t.Fatalf("postgres update should not patch profile fields: %s", query) + } + if len(args) != 2 { + t.Fatalf("expected repository and empty branch args, got %d", len(args)) + } + if args[0] != "repo-after" || args[1] != "" { + t.Fatalf("expected repo-after and empty branch args, got %#v", args) + } +} + +func TestBuildPostgresUserSettingsPreservingTaskCreateLastUsedUpdateUsesJSONB(t *testing.T) { + patch := models.TaskCreateLastUsed{ + RepositoryID: "repo-1", + Branch: "feature", + AgentProfileID: "agent-1", + ExecutorProfileID: "exec-1", + } + query, args := buildPostgresUserSettingsPreservingTaskCreateLastUsedUpdate(&patch) + + if strings.Contains(query, "json(") || strings.Contains(query, "json_extract") { + t.Fatalf("postgres update must not use sqlite JSON functions: %s", query) + } + if !strings.Contains(query, "?::jsonb") || !strings.Contains(query, "jsonb_set") { + t.Fatalf("postgres update should merge payload with jsonb_set: %s", query) + } + if !strings.Contains(query, "{task_create_last_used,repository_id}") || + !strings.Contains(query, "{task_create_last_used,branch}") || + !strings.Contains(query, "{task_create_last_used,agent_profile_id}") || + !strings.Contains(query, "{task_create_last_used,executor_profile_id}") { + t.Fatalf("postgres update missing task-create paths: %s", query) + } + if len(args) != 4 { + t.Fatalf("expected one arg per task-create field, got %d", len(args)) + } +} + +func TestPostgresRepositoryTaskCreateLastUsedRoundTrip(t *testing.T) { + conn := testutil.OpenIsolatedPostgres(t, testutil.PostgresDSNFromEnv(t)) + repo, err := newSQLiteRepositoryWithDB(conn, conn) + if err != nil { + t.Fatalf("new postgres repo: %v", err) + } + + ctx := context.Background() + staleSettings, err := repo.GetUserSettings(ctx, DefaultUserID) + if err != nil { + t.Fatalf("get defaults: %v", err) + } + staleSettings.SidebarActiveViewID = "view-before" + staleSettings.TaskCreateLastUsed = models.TaskCreateLastUsed{ + RepositoryID: "repo-before", + Branch: "main", + AgentProfileID: "agent-before", + ExecutorProfileID: "exec-before", + } + upsertUserSettingsForTest(t, repo, ctx, staleSettings) + + got, err := repo.UpdateTaskCreateLastUsed(ctx, DefaultUserID, models.TaskCreateLastUsed{ + RepositoryID: "repo-after", + Branch: "feature", + AgentProfileID: "agent-after", + }) + if err != nil { + t.Fatalf("update postgres task-create last-used: %v", err) + } + if got.SidebarActiveViewID != "view-before" { + t.Fatalf("unrelated postgres setting should be preserved, got %q", got.SidebarActiveViewID) + } + if got.TaskCreateLastUsed.RepositoryID != "repo-after" || + got.TaskCreateLastUsed.Branch != "feature" || + got.TaskCreateLastUsed.AgentProfileID != "agent-after" || + got.TaskCreateLastUsed.ExecutorProfileID != "exec-before" { + t.Fatalf("postgres task-create update mismatch: %+v", got.TaskCreateLastUsed) + } + + staleSettings.SidebarActiveViewID = "view-after" + staleSettings.TaskCreateLastUsed = models.TaskCreateLastUsed{ + RepositoryID: "repo-stale", + Branch: "stale", + } + got, err = repo.UpsertUserSettingsPreservingTaskCreateLastUsed(ctx, staleSettings, nil) + if err != nil { + t.Fatalf("upsert preserving postgres task-create last-used: %v", err) + } + if got.SidebarActiveViewID != "view-after" { + t.Fatalf("expected unrelated postgres setting to update, got %q", got.SidebarActiveViewID) + } + if got.TaskCreateLastUsed.RepositoryID != "repo-after" || + got.TaskCreateLastUsed.Branch != "feature" || + got.TaskCreateLastUsed.AgentProfileID != "agent-after" || + got.TaskCreateLastUsed.ExecutorProfileID != "exec-before" { + t.Fatalf("postgres preserving upsert should keep current task-create values: %+v", got.TaskCreateLastUsed) + } +} + +func TestSQLiteRepositoryUpsertSettingsPreservesCurrentTaskCreateLastUsed(t *testing.T) { + conn, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + conn.SetMaxOpenConns(1) + t.Cleanup(func() { _ = conn.Close() }) + repo, err := newSQLiteRepositoryWithDB(conn, conn) + if err != nil { + t.Fatalf("new repo: %v", err) + } + + ctx := context.Background() + staleSettings, err := repo.GetUserSettings(ctx, DefaultUserID) + if err != nil { + t.Fatalf("get defaults: %v", err) + } + staleSettings.SidebarActiveViewID = "view-before" + staleSettings.TaskCreateLastUsed = models.TaskCreateLastUsed{RepositoryID: "repo-before"} + upsertUserSettingsForTest(t, repo, ctx, staleSettings) + + // Simulate a task-create write that lands after another settings caller + // read the old blob but before that caller writes its unrelated change. + if _, err := repo.UpdateTaskCreateLastUsed(ctx, DefaultUserID, models.TaskCreateLastUsed{ + RepositoryID: "repo-after", + Branch: "feature", + }); err != nil { + t.Fatalf("update task-create last-used: %v", err) + } + + staleSettings.SidebarActiveViewID = "view-after" + got, err := repo.UpsertUserSettingsPreservingTaskCreateLastUsed(ctx, staleSettings, nil) + if err != nil { + t.Fatalf("upsert preserving task-create last-used: %v", err) + } + + if got.SidebarActiveViewID != "view-after" { + t.Fatalf("expected unrelated setting to update, got %q", got.SidebarActiveViewID) + } + if got.TaskCreateLastUsed.RepositoryID != "repo-after" { + t.Fatalf("expected current task-create repository to survive stale write, got %q", got.TaskCreateLastUsed.RepositoryID) + } + if got.TaskCreateLastUsed.Branch != "feature" { + t.Fatalf("expected current task-create branch to survive stale write, got %q", got.TaskCreateLastUsed.Branch) + } +} + +func TestSQLiteRepositoryUpsertSettingsPreservingTaskCreateLastUsedAppliesPatch(t *testing.T) { + conn, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + conn.SetMaxOpenConns(1) + t.Cleanup(func() { _ = conn.Close() }) + repo, err := newSQLiteRepositoryWithDB(conn, conn) + if err != nil { + t.Fatalf("new repo: %v", err) + } + + ctx := context.Background() + staleSettings, err := repo.GetUserSettings(ctx, DefaultUserID) + if err != nil { + t.Fatalf("get defaults: %v", err) + } + staleSettings.SidebarActiveViewID = "view-before" + staleSettings.TaskCreateLastUsed = models.TaskCreateLastUsed{ + RepositoryID: "repo-before", + Branch: "main", + AgentProfileID: "agent-before", + ExecutorProfileID: "exec-before", + } + upsertUserSettingsForTest(t, repo, ctx, staleSettings) + if _, err := repo.UpdateTaskCreateLastUsed(ctx, DefaultUserID, models.TaskCreateLastUsed{ + RepositoryID: "repo-current", + Branch: "current", + ExecutorProfileID: "exec-current", + }); err != nil { + t.Fatalf("update current task-create last-used: %v", err) + } + + staleSettings.SidebarActiveViewID = "view-after" + staleSettings.TaskCreateLastUsed = models.TaskCreateLastUsed{ + RepositoryID: "repo-stale", + Branch: "stale", + AgentProfileID: "agent-stale", + } + patch := models.TaskCreateLastUsed{AgentProfileID: "agent-after"} + got, err := repo.UpsertUserSettingsPreservingTaskCreateLastUsed(ctx, staleSettings, &patch) + if err != nil { + t.Fatalf("upsert preserving task-create last-used with patch: %v", err) + } + + if got.SidebarActiveViewID != "view-after" { + t.Fatalf("expected unrelated setting to update, got %q", got.SidebarActiveViewID) + } + if got.TaskCreateLastUsed.RepositoryID != "repo-current" { + t.Fatalf("expected current repository to survive stale write, got %q", got.TaskCreateLastUsed.RepositoryID) + } + if got.TaskCreateLastUsed.Branch != "current" { + t.Fatalf("expected current branch to survive stale write, got %q", got.TaskCreateLastUsed.Branch) + } + if got.TaskCreateLastUsed.AgentProfileID != "agent-after" { + t.Fatalf("expected patch agent profile to apply, got %q", got.TaskCreateLastUsed.AgentProfileID) + } + if got.TaskCreateLastUsed.ExecutorProfileID != "exec-current" { + t.Fatalf("expected current executor profile to survive stale write, got %q", got.TaskCreateLastUsed.ExecutorProfileID) + } +} + func TestSQLiteRepositorySidebarViewStateRoundTrip(t *testing.T) { conn, err := sqlx.Open("sqlite3", ":memory:") if err != nil { @@ -144,9 +492,7 @@ func TestSQLiteRepositorySidebarViewStateRoundTrip(t *testing.T) { Sort: models.SidebarViewSort{Key: "updatedAt", Direction: "desc"}, Group: "workflow", } - if err := repo.UpsertUserSettings(ctx, settings); err != nil { - t.Fatalf("upsert settings: %v", err) - } + upsertUserSettingsForTest(t, repo, ctx, settings) got, err := repo.GetUserSettings(ctx, DefaultUserID) if err != nil { t.Fatalf("get settings: %v", err) diff --git a/apps/backend/internal/user/store/store.go b/apps/backend/internal/user/store/store.go index cbbfbd628b..b486193b6c 100644 --- a/apps/backend/internal/user/store/store.go +++ b/apps/backend/internal/user/store/store.go @@ -10,6 +10,7 @@ type Repository interface { GetUser(ctx context.Context, id string) (*models.User, error) GetDefaultUser(ctx context.Context) (*models.User, error) GetUserSettings(ctx context.Context, userID string) (*models.UserSettings, error) - UpsertUserSettings(ctx context.Context, settings *models.UserSettings) error + UpsertUserSettingsPreservingTaskCreateLastUsed(ctx context.Context, settings *models.UserSettings, patch *models.TaskCreateLastUsed) (*models.UserSettings, error) + UpdateTaskCreateLastUsed(ctx context.Context, userID string, patch models.TaskCreateLastUsed) (*models.UserSettings, error) Close() error } diff --git a/apps/cli/src/release-config.test.ts b/apps/cli/src/release-config.test.ts index 8602891897..89a4f973d0 100644 --- a/apps/cli/src/release-config.test.ts +++ b/apps/cli/src/release-config.test.ts @@ -94,7 +94,21 @@ function findPnpmSetupVersion( } function matchPnpmSetupUsesLine(line: string): RegExpMatchArray | null { - return line.match(/^(\s*)(?:-\s*)?uses:\s*["']?pnpm\/action-setup@[0-9a-f]{40}["']?\s*(?:#.*)?$/); + const match = line.match( + /^(\s*)(?:-\s*)?uses:\s*["']?pnpm\/action-setup@([^"'\s#]+)["']?\s*(#.*)?$/, + ); + if (match === null) return null; + const ref = match[2]; + const comment = match[3] ?? ""; + const versionTagPattern = /^v[0-9]+(?:\.[0-9]+\.[0-9]+)?$/; + const versionTag = versionTagPattern.test(ref); + const shaPinned = + /^[0-9a-f]{40}$/.test(ref) && /^#\s*v[0-9]+(?:\.[0-9]+\.[0-9]+)?$/.test(comment); + expect( + versionTag || shaPinned, + "pnpm/action-setup must use a version tag or a 40-character SHA with the version tag in a comment", + ).toBe(true); + return match; } function findStepIndent(lines: string[], usesLineIndex: number, usesIndent: number): number { @@ -150,6 +164,14 @@ function assertWorkflowPnpmVersions(file: string, expectedVersion: string): numb } describe("release runtime tooling configuration", () => { + it("rejects malformed SHA version comments for pnpm/action-setup", () => { + expect(() => + matchPnpmSetupUsesLine( + "uses: pnpm/action-setup@0123456789abcdef0123456789abcdef01234567 # v4-old", + ), + ).toThrow(/40-character SHA with the version tag/); + }); + it("pins runtime tooling consistently across Docker and GitHub Actions", () => { const packagePnpmVersion = extractPackageManagerPnpmVersion(readRepoFile("apps/package.json")); const dockerfile = readRepoFile("Dockerfile"); diff --git a/apps/web/components/state-provider.test.tsx b/apps/web/components/state-provider.test.tsx index f9b967b136..7dc3b6c55c 100644 --- a/apps/web/components/state-provider.test.tsx +++ b/apps/web/components/state-provider.test.tsx @@ -3,6 +3,11 @@ import { useEffect } from "react"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { defaultSettingsState } from "@/lib/state/slices/settings/settings-slice"; import { STORAGE_KEYS } from "@/lib/settings/constants"; +import { + readQueuedTaskCreateLastUsedState, + resetTaskCreateLastUsedSync, + syncTaskCreateLastUsed, +} from "./task-create-dialog-handlers"; import { StateProvider, useAppStore } from "./state-provider"; function ShowMetricsPreference({ label }: { label: string }) { @@ -40,12 +45,13 @@ function ObserveLastUsedCache({ onSeen }: { onSeen: (value: string | null) => vo return null; } -describe("StateProvider", () => { - beforeEach(() => { - window.localStorage.clear(); - vi.restoreAllMocks(); - }); +beforeEach(() => { + window.localStorage.clear(); + resetTaskCreateLastUsedSync({ clearQueued: true }); + vi.restoreAllMocks(); +}); +describe("StateProvider", () => { it("reuses the parent store for nested route providers", async () => { render( @@ -68,7 +74,9 @@ describe("StateProvider", () => { fireEvent.click(screen.getByRole("button", { name: "Enable metrics" })); expect(await screen.findByText("root:on")).toBeTruthy(); }); +}); +describe("StateProvider task-create cache", () => { it("syncs task-create last-used settings to localStorage without redundant writes", async () => { function UpdateUnrelatedSetting() { const setUserSettings = useAppStore((state) => state.setUserSettings); @@ -147,3 +155,70 @@ describe("StateProvider", () => { expect(onSeen).toHaveBeenCalledWith(JSON.stringify("repo-1")); }); }); + +describe("StateProvider task-create cache clearing", () => { + it("lets children read cached task-create choices before clearing missing backend values", async () => { + const onSeen = vi.fn(); + window.localStorage.setItem(STORAGE_KEYS.LAST_REPOSITORY_ID, JSON.stringify("repo-cached")); + window.localStorage.setItem(STORAGE_KEYS.LAST_BRANCH, JSON.stringify("feature-cached")); + window.localStorage.setItem(STORAGE_KEYS.LAST_AGENT_PROFILE_ID, JSON.stringify("agent-cached")); + window.localStorage.setItem( + STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID, + JSON.stringify("exec-cached"), + ); + + render( + + + , + ); + + expect(onSeen).toHaveBeenCalledWith(JSON.stringify("repo-cached")); + await waitFor(() => { + expect(window.localStorage.getItem(STORAGE_KEYS.LAST_REPOSITORY_ID)).toBeNull(); + expect(window.localStorage.getItem(STORAGE_KEYS.LAST_BRANCH)).toBeNull(); + expect(window.localStorage.getItem(STORAGE_KEYS.LAST_AGENT_PROFILE_ID)).toBeNull(); + expect(window.localStorage.getItem(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID)).toBeNull(); + }); + }); +}); + +describe("StateProvider task-create queued overlay", () => { + it("clears the queued overlay when loaded settings catch up", () => { + syncTaskCreateLastUsed({ repository_id: "repo-1", branch: "main" }); + + render( + +
ready
+ , + ); + + expect(readQueuedTaskCreateLastUsedState()).toEqual({}); + }); +}); diff --git a/apps/web/components/state-provider.tsx b/apps/web/components/state-provider.tsx index 21406ac453..f285f80780 100644 --- a/apps/web/components/state-provider.tsx +++ b/apps/web/components/state-provider.tsx @@ -8,6 +8,7 @@ import type { AppState, StoreProviderProps } from "@/lib/state/store"; import { createAppStore } from "@/lib/state/store"; import { removeLocalStorage, setLocalStorage } from "@/lib/local-storage"; import { STORAGE_KEYS } from "@/lib/settings/constants"; +import { clearQueuedTaskCreateLastUsedIfSynced } from "./task-create-dialog-handlers"; const StoreContext = createContext | null>(null); @@ -65,27 +66,32 @@ export function StateProvider({ children, initialState }: StoreProviderProps) { function syncTaskCreateLastUsedCache(state: AppState) { if (!state.userSettings.loaded) return; const lastUsed = state.userSettings.taskCreateLastUsed; - if (!lastUsed) return; - if (lastUsed.repositoryId) { - setLocalStorage(STORAGE_KEYS.LAST_REPOSITORY_ID, lastUsed.repositoryId); - } else { - removeLocalStorage(STORAGE_KEYS.LAST_REPOSITORY_ID); - } - if (lastUsed.branch) { - setLocalStorage(STORAGE_KEYS.LAST_BRANCH, lastUsed.branch); - } else { - removeLocalStorage(STORAGE_KEYS.LAST_BRANCH); - } - if (lastUsed.agentProfileId) { - setLocalStorage(STORAGE_KEYS.LAST_AGENT_PROFILE_ID, lastUsed.agentProfileId); - } else { - removeLocalStorage(STORAGE_KEYS.LAST_AGENT_PROFILE_ID); - } - if (lastUsed.executorProfileId) { - setLocalStorage(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID, lastUsed.executorProfileId); - } else { - removeLocalStorage(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID); + syncTaskCreateLastUsedCacheField(STORAGE_KEYS.LAST_REPOSITORY_ID, lastUsed?.repositoryId); + syncTaskCreateLastUsedCacheField(STORAGE_KEYS.LAST_BRANCH, lastUsed?.branch); + syncTaskCreateLastUsedCacheField(STORAGE_KEYS.LAST_AGENT_PROFILE_ID, lastUsed?.agentProfileId); + syncTaskCreateLastUsedCacheField( + STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID, + lastUsed?.executorProfileId, + ); + clearQueuedTaskCreateLastUsedIfSynced(lastUsed); +} + +function syncTaskCreateLastUsedCacheField(key: string, value: string | null | undefined) { + if (value) { + setLocalStorage(key, value); + return; } + deferRemoveTaskCreateLastUsedCacheField(key); +} + +function deferRemoveTaskCreateLastUsedCacheField(key: string) { + const cachedValue = window.localStorage.getItem(key); + if (cachedValue === null) return; + window.setTimeout(() => { + if (window.localStorage.getItem(key) === cachedValue) { + removeLocalStorage(key); + } + }, 0); } function taskCreateLastUsedEqual( diff --git a/apps/web/components/task-create-dialog-autopick.ts b/apps/web/components/task-create-dialog-autopick.ts index b36b304e72..2be9c5b705 100644 --- a/apps/web/components/task-create-dialog-autopick.ts +++ b/apps/web/components/task-create-dialog-autopick.ts @@ -66,7 +66,7 @@ function getAgentAutopickGate(input: AgentProfileAutopickInput): AutopickDecisio return { kind: "defer", reason: "executor-not-selected" }; } if (input.compatibleAgentProfiles.length === 0) return { kind: "skip", reason: "no-compatible" }; - if (!input.lastAgentProfileId && input.userSettingsLoaded === false) { + if (input.userSettingsLoaded === false) { return { kind: "defer", reason: "user-settings-not-loaded" }; } return null; @@ -126,13 +126,13 @@ function resolveLastUsedAgentProfileId( settingsAgentProfileId?: string | null, ) { const localId = getLocalStorage(STORAGE_KEYS.LAST_AGENT_PROFILE_ID, null); - if (localId && compatibleAgentProfiles.some((p) => p.id === localId)) return localId; if ( settingsAgentProfileId && compatibleAgentProfiles.some((p) => p.id === settingsAgentProfileId) ) { return settingsAgentProfileId; } + if (localId && compatibleAgentProfiles.some((p) => p.id === localId)) return localId; return null; } @@ -141,7 +141,11 @@ export function useWorkflowAgentProfileEffect( workflows: Array<{ id: string; agent_profile_id?: string }>, agentProfiles: AgentProfileOption[], compatibleAgentProfiles: AgentProfileOption[], - options: { lastUsedAgentProfileId?: string | null; authLoaded?: boolean } = {}, + options: { + lastUsedAgentProfileId?: string | null; + authLoaded?: boolean; + userSettingsLoaded?: boolean; + } = {}, ) { const { lastUsedAgentProfileId, authLoaded = true } = options; const { @@ -200,6 +204,13 @@ export function useWorkflowAgentProfileEffect( }); return; } + if (options.userSettingsLoaded === false) { + setAgentProfileId(""); + workflowAutopickDebug("workflow-no-override-defer", { + reason: "user-settings-not-loaded", + }); + return; + } // Restore the user's last-used agent profile when unlocking. Filter // against `compatibleAgentProfiles` (not the full `agentProfiles` list) // so an executor-incompatible id from a previous session - including @@ -227,6 +238,7 @@ export function useWorkflowAgentProfileEffect( compatibleAgentProfiles, lastUsedAgentProfileId, authLoaded, + options.userSettingsLoaded, setAgentProfileId, setWorkflowAgentProfileId, ]); diff --git a/apps/web/components/task-create-dialog-effects-executor.test.ts b/apps/web/components/task-create-dialog-effects-executor.test.ts index 980d0597bd..07c7d0b463 100644 --- a/apps/web/components/task-create-dialog-effects-executor.test.ts +++ b/apps/web/components/task-create-dialog-effects-executor.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { renderHook, waitFor } from "@testing-library/react"; import { useDefaultSelectionsEffect } from "./task-create-dialog-effects"; import type { DialogFormState, StoreSelections } from "@/components/task-create-dialog-types"; +import { STORAGE_KEYS } from "@/lib/settings/constants"; beforeEach(() => { localStorage.clear(); @@ -10,6 +11,7 @@ beforeEach(() => { const PROFILE_DOCKER = "profile-docker"; const PROFILE_LOCAL = "profile-local"; const PROFILE_WORKTREE = "profile-worktree"; +const PROFILE_WORKTREE_B = "profile-worktree-b"; type DefaultSelFake = Pick< DialogFormState, @@ -76,7 +78,10 @@ function worktreeExecutor(): StoreSelections["executors"][number] { return { id: "exec-worktree", type: "worktree", - profiles: [{ id: PROFILE_WORKTREE, executor_type: "worktree" }], + profiles: [ + { id: PROFILE_WORKTREE, executor_type: "worktree" }, + { id: PROFILE_WORKTREE_B, executor_type: "worktree" }, + ], } as unknown as StoreSelections["executors"][number]; } @@ -161,6 +166,127 @@ describe("useDefaultSelectionsEffect - executor profile defaults", () => { }); }); +describe("useDefaultSelectionsEffect - executor profile restoration", () => { + it("defers executor profile fallback until user settings have loaded or settled", async () => { + window.localStorage.removeItem(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID); + const fs = makeDefaultSelFs({ executorProfileId: "", executorId: "" }); + const worktree = worktreeExecutor(); + const selBefore = makeSel({ + executors: [worktree], + userSettingsLoaded: false, + }); + + const { rerender } = renderHook(({ sel }) => useDefaultSelectionsEffect(fs, true, sel, []), { + initialProps: { sel: selBefore }, + }); + + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(fs.setExecutorId).not.toHaveBeenCalled(); + expect(fs.setExecutorProfileId).not.toHaveBeenCalled(); + + const selAfter = makeSel({ + executors: [worktree], + userSettingsLoaded: true, + }); + rerender({ sel: selAfter }); + + await waitFor(() => expect(fs.setExecutorProfileId).toHaveBeenCalledWith(PROFILE_WORKTREE)); + }); + + it("keeps deferring when a valid cached executor profile exists but settings are loading", async () => { + window.localStorage.setItem( + STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID, + JSON.stringify(PROFILE_WORKTREE), + ); + const fs = makeDefaultSelFs({ executorProfileId: "", executorId: "" }); + const sel = makeSel({ + executors: [worktreeExecutor()], + userSettingsLoaded: false, + }); + + renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); + + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(fs.setExecutorId).not.toHaveBeenCalled(); + expect(fs.setExecutorProfileId).not.toHaveBeenCalled(); + }); + + it("keeps deferring when cached executor profile is ineligible for the source mode", async () => { + window.localStorage.setItem( + STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID, + JSON.stringify(PROFILE_WORKTREE), + ); + const fs = makeDefaultSelFs({ executorProfileId: "", executorId: "", noRepository: true }); + const sel = makeSel({ + executors: [worktreeExecutor(), localExecutor()], + userSettingsLoaded: false, + }); + + renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); + + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(fs.setExecutorProfileId).not.toHaveBeenCalled(); + }); +}); + +describe("useDefaultSelectionsEffect - executor profile settings restoration", () => { + it("restores executor profile from store-backed settings when localStorage is not primed", async () => { + window.localStorage.removeItem(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID); + const fs = makeDefaultSelFs({ executorProfileId: "", executorId: "" }); + const worktree = worktreeExecutor(); + const sel = makeSel({ + executors: [worktree], + lastUsedExecutorProfileId: PROFILE_WORKTREE_B, + userSettingsLoaded: true, + }); + + renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); + + await waitFor(() => expect(fs.setExecutorProfileId).toHaveBeenCalledWith(PROFILE_WORKTREE_B)); + }); + + it("does not pick a fallback executor id while a valid last-used profile is restoring", async () => { + window.localStorage.removeItem(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID); + const local = localExecutor(); + const worktree = worktreeExecutor(); + const sel = makeSel({ + executors: [local, worktree], + workspaceDefaults: { default_executor_id: local.id } as StoreSelections["workspaceDefaults"], + lastUsedExecutorProfileId: PROFILE_WORKTREE_B, + userSettingsLoaded: true, + }); + + const setExecutorId = vi.fn(); + const setExecutorProfileId = vi.fn(); + const { rerender } = renderHook( + ({ formState }) => useDefaultSelectionsEffect(formState, true, sel, []), + { + initialProps: { + formState: makeDefaultSelFs({ + executorProfileId: "", + executorId: "", + setExecutorId, + setExecutorProfileId, + }), + }, + }, + ); + + await waitFor(() => expect(setExecutorProfileId).toHaveBeenCalledWith(PROFILE_WORKTREE_B)); + rerender({ + formState: makeDefaultSelFs({ + executorProfileId: PROFILE_WORKTREE_B, + executorId: "", + setExecutorId, + setExecutorProfileId, + }), + }); + + await waitFor(() => expect(setExecutorId).toHaveBeenCalledWith(worktree.id)); + expect(setExecutorId).not.toHaveBeenCalledWith(local.id); + }); +}); + describe("useDefaultSelectionsEffect - multi-repo guard counts Remote rows", () => { it("swaps a non-worktree profile to worktree when 2+ Remote URL rows are set", async () => { // Regression: the guard used to count only workspace/local rows, so 2 diff --git a/apps/web/components/task-create-dialog-effects.test.ts b/apps/web/components/task-create-dialog-effects.test.ts index ce82a39d11..697d4edc31 100644 --- a/apps/web/components/task-create-dialog-effects.test.ts +++ b/apps/web/components/task-create-dialog-effects.test.ts @@ -2,17 +2,12 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { renderHook, waitFor } from "@testing-library/react"; import { useDefaultSelectionsEffect, - useRepositoryAutoSelectEffect, useWorkflowAgentProfileEffect, } from "./task-create-dialog-effects"; import { decideAgentProfileAutopick } from "./task-create-dialog-autopick"; -import type { - DialogFormState, - StoreSelections, - TaskRepoRow, -} from "@/components/task-create-dialog-types"; +import type { DialogFormState, StoreSelections } from "@/components/task-create-dialog-types"; import type { AgentProfileOption } from "@/lib/state/slices"; -import type { Repository, Workspace } from "@/lib/types/http"; +import type { Workspace } from "@/lib/types/http"; import { STORAGE_KEYS } from "@/lib/settings/constants"; // Minimal fake of DialogFormState - the hook destructures only three fields, @@ -52,106 +47,6 @@ beforeEach(() => { localStorage.clear(); }); -function makeRepository(id: string): Repository { - return { - id, - workspace_id: "ws-1", - name: "repo", - source_type: "local", - local_path: "/repo", - default_branch: "main", - created_at: new Date().toISOString(), - updated_at: new Date().toISOString(), - } as Repository; -} - -function makeRepoAutoSelectFs( - rows: TaskRepoRow[], - setRepositories: DialogFormState["setRepositories"], -): DialogFormState { - return { - repositories: rows, - useRemote: false, - setRepositories, - } as unknown as DialogFormState; -} - -describe("useRepositoryAutoSelectEffect", () => { - it("waits for store-backed settings before falling back to an empty row", async () => { - window.localStorage.removeItem(STORAGE_KEYS.LAST_REPOSITORY_ID); - const setRepositories = vi.fn(); - const fs = makeRepoAutoSelectFs([], setRepositories); - - const { rerender } = renderHook( - ({ loaded, lastUsedRepositoryId }) => - useRepositoryAutoSelectEffect( - fs, - true, - "ws-1", - [makeRepository("repo-1"), makeRepository("repo-2")], - { - lastUsedRepositoryId, - userSettingsLoaded: loaded, - }, - ), - { initialProps: { loaded: false, lastUsedRepositoryId: null as string | null } }, - ); - - await new Promise((resolve) => setTimeout(resolve, 10)); - expect(setRepositories).not.toHaveBeenCalled(); - - rerender({ loaded: true, lastUsedRepositoryId: "repo-2" }); - - await waitFor(() => expect(setRepositories).toHaveBeenCalled()); - const updater = setRepositories.mock.calls[0]![0] as (prev: TaskRepoRow[]) => TaskRepoRow[]; - expect(updater([])).toEqual([{ key: "row-0", repositoryId: "repo-2", branch: "" }]); - }); - - it("fills an untouched placeholder row from store-backed settings when localStorage is not primed", async () => { - window.localStorage.removeItem(STORAGE_KEYS.LAST_REPOSITORY_ID); - const setRepositories = vi.fn(); - const fs = makeRepoAutoSelectFs([{ key: "row-0", branch: "" }], setRepositories); - - renderHook(() => - useRepositoryAutoSelectEffect( - fs, - true, - "ws-1", - [makeRepository("repo-1"), makeRepository("repo-2")], - { lastUsedRepositoryId: "repo-2" }, - ), - ); - - await waitFor(() => expect(setRepositories).toHaveBeenCalled()); - const updater = setRepositories.mock.calls[0]![0] as (prev: TaskRepoRow[]) => TaskRepoRow[]; - - expect(updater([{ key: "row-0", branch: "" }])).toEqual([ - { key: "row-0", repositoryId: "repo-2", branch: "" }, - ]); - }); - - it("fills an empty repo row list from store-backed settings when localStorage is not primed", async () => { - window.localStorage.removeItem(STORAGE_KEYS.LAST_REPOSITORY_ID); - const setRepositories = vi.fn(); - const fs = makeRepoAutoSelectFs([], setRepositories); - - renderHook(() => - useRepositoryAutoSelectEffect( - fs, - true, - "ws-1", - [makeRepository("repo-1"), makeRepository("repo-2")], - { lastUsedRepositoryId: "repo-1" }, - ), - ); - - await waitFor(() => expect(setRepositories).toHaveBeenCalled()); - const updater = setRepositories.mock.calls[0]![0] as (prev: TaskRepoRow[]) => TaskRepoRow[]; - - expect(updater([])).toEqual([{ key: "row-0", repositoryId: "repo-1", branch: "" }]); - }); -}); - describe("useWorkflowAgentProfileEffect", () => { it("clears workflowAgentProfileId and leaves agentProfileId alone when no workflow is selected", () => { localStorage.setItem(STORAGE_KEYS.LAST_AGENT_PROFILE_ID, JSON.stringify("stale-id")); @@ -382,30 +277,6 @@ function makeSel(overrides: Partial = {}): StoreSelections { }; } -const WORKTREE_EXECUTOR_ID = "exec-worktree"; -const WORKTREE_PROFILE_B = "profile-b"; -const LOCAL_EXECUTOR_ID = "exec-local"; -const LOCAL_PROFILE_ID = "profile-local"; - -function makeWorktreeExecutor(): StoreSelections["executors"][number] { - return { - id: WORKTREE_EXECUTOR_ID, - type: "worktree", - profiles: [ - { id: "profile-a", executor_id: WORKTREE_EXECUTOR_ID, name: "A" }, - { id: WORKTREE_PROFILE_B, executor_id: WORKTREE_EXECUTOR_ID, name: "B" }, - ], - } as unknown as StoreSelections["executors"][number]; -} - -function makeLocalExecutor(): StoreSelections["executors"][number] { - return { - id: LOCAL_EXECUTOR_ID, - type: "local", - profiles: [{ id: LOCAL_PROFILE_ID, executor_id: LOCAL_EXECUTOR_ID, name: "Local" }], - } as unknown as StoreSelections["executors"][number]; -} - describe("decideAgentProfileAutopick — user settings deferral", () => { it("defers agent auto-pick until user settings have loaded or settled", () => { const cursor = makeProfile("cursor"); @@ -443,6 +314,28 @@ describe("decideAgentProfileAutopick — user settings deferral", () => { expect(picked).toEqual({ kind: "pick", source: "first", id: cursor.id }); }); + + it("defers auto-pick while user settings load even when localStorage has a valid profile", () => { + const cursor = makeProfile("cursor"); + localStorage.setItem(STORAGE_KEYS.LAST_AGENT_PROFILE_ID, JSON.stringify(cursor.id)); + + const deferred = decideAgentProfileAutopick({ + open: true, + agentProfileId: "", + workflowAgentProfileId: "", + workflowHasAgent: false, + agentProfiles: [cursor], + compatibleAgentProfiles: [cursor], + authLoaded: true, + executorProfileId: "", + hasExecutors: false, + lastAgentProfileId: cursor.id, + userSettingsLoaded: false, + defaultAgentProfileId: null, + }); + + expect(deferred).toEqual({ kind: "defer", reason: "user-settings-not-loaded" }); + }); }); describe("useDefaultSelectionsEffect — executor-aware agent restoration", () => { @@ -561,104 +454,6 @@ describe("useDefaultSelectionsEffect — executor-aware agent restoration", () = }); }); -describe("useDefaultSelectionsEffect — executor profile restoration", () => { - it("defers executor profile fallback until user settings have loaded or settled", async () => { - window.localStorage.removeItem(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID); - const fs = makeDefaultSelFs({ executorProfileId: "", executorId: "" }); - const worktreeExecutor = makeWorktreeExecutor(); - const selBefore = makeSel({ - executors: [worktreeExecutor], - userSettingsLoaded: false, - } as Partial); - - const { rerender } = renderHook(({ sel }) => useDefaultSelectionsEffect(fs, true, sel, []), { - initialProps: { sel: selBefore }, - }); - - await new Promise((resolve) => setTimeout(resolve, 10)); - expect(fs.setExecutorId).not.toHaveBeenCalled(); - expect(fs.setExecutorProfileId).not.toHaveBeenCalled(); - - const selAfter = makeSel({ - executors: [worktreeExecutor], - userSettingsLoaded: true, - } as Partial); - rerender({ sel: selAfter }); - - await waitFor(() => expect(fs.setExecutorProfileId).toHaveBeenCalledWith("profile-a")); - }); - - it("keeps deferring when cached executor profile is ineligible for the source mode", async () => { - window.localStorage.setItem(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID, JSON.stringify("profile-a")); - const fs = makeDefaultSelFs({ executorProfileId: "", executorId: "", noRepository: true }); - const sel = makeSel({ - executors: [makeWorktreeExecutor(), makeLocalExecutor()], - userSettingsLoaded: false, - } as Partial); - - renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); - - await new Promise((resolve) => setTimeout(resolve, 10)); - expect(fs.setExecutorProfileId).not.toHaveBeenCalled(); - }); - - it("restores executor profile from store-backed settings when localStorage is not primed", async () => { - window.localStorage.removeItem(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID); - const fs = makeDefaultSelFs({ executorProfileId: "", executorId: "" }); - const worktreeExecutor = makeWorktreeExecutor(); - const sel = makeSel({ - executors: [worktreeExecutor], - lastUsedExecutorProfileId: WORKTREE_PROFILE_B, - userSettingsLoaded: true, - } as Partial); - - renderHook(() => useDefaultSelectionsEffect(fs, true, sel, [])); - - await waitFor(() => expect(fs.setExecutorProfileId).toHaveBeenCalledWith(WORKTREE_PROFILE_B)); - }); - - it("does not pick a fallback executor id while a valid last-used profile is restoring", async () => { - window.localStorage.removeItem(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID); - const localExecutor = makeLocalExecutor(); - const worktreeExecutor = makeWorktreeExecutor(); - const sel = makeSel({ - executors: [localExecutor, worktreeExecutor], - workspaceDefaults: { default_executor_id: LOCAL_EXECUTOR_ID }, - lastUsedExecutorProfileId: WORKTREE_PROFILE_B, - userSettingsLoaded: true, - } as Partial); - - const setExecutorId = vi.fn(); - const setExecutorProfileId = vi.fn(); - const { rerender } = renderHook( - ({ formState }) => useDefaultSelectionsEffect(formState, true, sel, []), - { - initialProps: { - formState: makeDefaultSelFs({ - executorProfileId: "", - executorId: "", - setExecutorId, - setExecutorProfileId, - }), - }, - }, - ); - - await waitFor(() => expect(setExecutorProfileId).toHaveBeenCalledWith(WORKTREE_PROFILE_B)); - rerender({ - formState: makeDefaultSelFs({ - executorProfileId: WORKTREE_PROFILE_B, - executorId: "", - setExecutorId, - setExecutorProfileId, - }), - }); - - await waitFor(() => expect(setExecutorId).toHaveBeenCalledWith(WORKTREE_EXECUTOR_ID)); - expect(setExecutorId).not.toHaveBeenCalledWith(LOCAL_EXECUTOR_ID); - }); -}); - describe("useDefaultSelectionsEffect — auth-spec load race", () => { it("defers auto-pick while executorProfileId is empty even when authLoaded (regression for race that restored an incompatible lastId)", async () => { // The other half of the executor-aware autopick race: `authLoaded` flips diff --git a/apps/web/components/task-create-dialog-effects.ts b/apps/web/components/task-create-dialog-effects.ts index 8e2964cc47..6e195c4776 100644 --- a/apps/web/components/task-create-dialog-effects.ts +++ b/apps/web/components/task-create-dialog-effects.ts @@ -230,13 +230,13 @@ function pickDefaultExecutorProfileId( if (eligibleProfiles.length === 0) return null; const lastId = getLocalStorage(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID, null); - if (lastId && eligibleProfiles.some((p) => p.id === lastId)) return lastId; if ( lastUsedExecutorProfileId && eligibleProfiles.some((p) => p.id === lastUsedExecutorProfileId) ) { return lastUsedExecutorProfileId; } + if (lastId && eligibleProfiles.some((p) => p.id === lastId)) return lastId; const executorId = pickDefaultExecutorId( executors, @@ -294,11 +294,9 @@ function getExecutorProfileLastUsedState( function shouldDeferExecutorProfileAutopick( context: ExecutorAutopickContext, - lastUsed: ExecutorProfileLastUsedState, + _lastUsed: ExecutorProfileLastUsedState, ) { - return ( - context.userSettingsLoaded === false && !lastUsed.localStorageValid && !lastUsed.settingsValid - ); + return context.userSettingsLoaded === false; } export function shouldWaitForLastUsedExecutorProfile(context: ExecutorAutopickContext) { @@ -590,6 +588,7 @@ export function useTaskCreateDialogEffects(fs: DialogFormState, args: TaskCreate useWorkflowAgentProfileEffect(fs, workflows, agentProfiles, compatibleAgentProfiles, { lastUsedAgentProfileId: args.lastUsedAgentProfileId, authLoaded, + userSettingsLoaded: args.userSettingsLoaded, }); useRepositoryAutoSelectEffect(fs, open, workspaceId, repositories, { lastUsedRepositoryId: args.lastUsedRepositoryId, diff --git a/apps/web/components/task-create-dialog-handlers.test.ts b/apps/web/components/task-create-dialog-handlers.test.ts index 108b3c1c66..40fe086ee7 100644 --- a/apps/web/components/task-create-dialog-handlers.test.ts +++ b/apps/web/components/task-create-dialog-handlers.test.ts @@ -1,69 +1,28 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; -import { waitFor } from "@testing-library/react"; -import { updateUserSettings } from "@/lib/api/domains/settings-api"; +import { beforeEach, describe, expect, it } from "vitest"; import { + queueTaskCreateLastUsedFromPayload, readQueuedTaskCreateLastUsedState, resetTaskCreateLastUsedSync, syncTaskCreateLastUsed, } from "./task-create-dialog-handlers"; -const PENDING_LAST_USED_SYNC_KEY = "kandev.taskCreateLastUsed.pendingSync"; - -vi.mock("@/lib/api/domains/settings-api", () => ({ - updateUserSettings: vi.fn(), -})); - describe("syncTaskCreateLastUsed", () => { beforeEach(() => { window.localStorage.clear(); resetTaskCreateLastUsedSync({ clearQueued: true }); - vi.mocked(updateUserSettings).mockReset(); }); - it("persists failed last-used patches and retries them on the next sync", async () => { - vi.mocked(updateUserSettings).mockRejectedValueOnce(new Error("network")); - + it("queues selector changes locally without writing a backend settings patch", () => { syncTaskCreateLastUsed({ branch: "feature" }); - await waitFor(() => { - expect(updateUserSettings).toHaveBeenCalledWith({ - task_create_last_used: { branch: "feature" }, - }); - }); - expect(JSON.parse(window.localStorage.getItem(PENDING_LAST_USED_SYNC_KEY) ?? "null")).toEqual({ + expect(readQueuedTaskCreateLastUsedState()).toMatchObject({ branch: "feature", }); - - vi.mocked(updateUserSettings).mockResolvedValueOnce({ settings: {} } as Awaited< - ReturnType - >); - - syncTaskCreateLastUsed({}); - - await waitFor(() => { - expect(updateUserSettings).toHaveBeenLastCalledWith({ - task_create_last_used: { branch: "feature" }, - }); - expect(window.localStorage.getItem(PENDING_LAST_USED_SYNC_KEY)).toBeNull(); - }); }); - it("retains prior queued fields after a successful sync clears pending state", async () => { - vi.mocked(updateUserSettings).mockResolvedValue({ settings: {} } as Awaited< - ReturnType - >); - + it("retains prior queued fields after a later selector change", () => { syncTaskCreateLastUsed({ branch: "feature" }); - await waitFor(() => { - expect(window.localStorage.getItem(PENDING_LAST_USED_SYNC_KEY)).toBeNull(); - }); - syncTaskCreateLastUsed({ agent_profile_id: "agent-2" }); - await waitFor(() => { - expect(updateUserSettings).toHaveBeenLastCalledWith({ - task_create_last_used: { agent_profile_id: "agent-2" }, - }); - }); expect(readQueuedTaskCreateLastUsedState()).toMatchObject({ branch: "feature", @@ -71,37 +30,176 @@ describe("syncTaskCreateLastUsed", () => { }); }); - it("keeps queued fields when dialog close resets pending sync state", async () => { - let resolveSync!: (response: Awaited>) => void; - vi.mocked(updateUserSettings).mockReturnValue( - new Promise((resolve) => { - resolveSync = resolve; - }), - ); + it("clears dependent queued fields with explicit null values", () => { + syncTaskCreateLastUsed({ repository_id: "repo-1", branch: "feature" }); + syncTaskCreateLastUsed({ repository_id: "repo-2", branch: null }); + + expect(readQueuedTaskCreateLastUsedState()).toMatchObject({ + repositoryId: "repo-2", + branch: null, + }); + }); + + it("clears queued fields when dialog close resets canceled selections", () => { + syncTaskCreateLastUsed({ branch: "feature" }); + + resetTaskCreateLastUsedSync({ clearQueued: true }); + + expect(readQueuedTaskCreateLastUsedState()).toEqual({}); + }); + + it("keeps queued fields when create-time close preserves pending backend writes", () => { syncTaskCreateLastUsed({ branch: "feature" }); - await waitFor(() => { - expect(updateUserSettings).toHaveBeenCalledWith({ - task_create_last_used: { branch: "feature" }, - }); - }); - expect(JSON.parse(window.localStorage.getItem(PENDING_LAST_USED_SYNC_KEY) ?? "null")).toEqual({ - branch: "feature", - }); resetTaskCreateLastUsedSync(); - expect(JSON.parse(window.localStorage.getItem(PENDING_LAST_USED_SYNC_KEY) ?? "null")).toEqual({ - branch: "feature", - }); expect(readQueuedTaskCreateLastUsedState()).toMatchObject({ branch: "feature", }); - expect(readQueuedTaskCreateLastUsedState().agentProfileId).toBeUndefined(); + }); - resolveSync({ settings: {} } as Awaited>); - await waitFor(() => { - expect(window.localStorage.getItem(PENDING_LAST_USED_SYNC_KEY)).toBeNull(); + it("keeps queued fields when preserved settings have not caught up", () => { + syncTaskCreateLastUsed({ branch: "feature" }); + + resetTaskCreateLastUsedSync({ + syncedSettings: { + repositoryId: null, + branch: "main", + agentProfileId: null, + executorProfileId: null, + }, + }); + + expect(readQueuedTaskCreateLastUsedState()).toMatchObject({ + branch: "feature", + }); + }); + + it("clears queued fields when preserved settings already match", () => { + syncTaskCreateLastUsed({ branch: "feature" }); + + resetTaskCreateLastUsedSync({ + syncedSettings: { + repositoryId: null, + branch: "feature", + agentProfileId: null, + executorProfileId: null, + }, + }); + + expect(readQueuedTaskCreateLastUsedState()).toEqual({}); + }); +}); + +describe("queueTaskCreateLastUsedFromPayload", () => { + beforeEach(() => { + window.localStorage.clear(); + resetTaskCreateLastUsedSync({ clearQueued: true }); + }); + + it("leaves the queued overlay unchanged for null or undefined payloads", () => { + syncTaskCreateLastUsed({ branch: "feature" }); + + queueTaskCreateLastUsedFromPayload(undefined); + queueTaskCreateLastUsedFromPayload(null); + + expect(readQueuedTaskCreateLastUsedState()).toEqual({ branch: "feature" }); + }); + + it("compacts empty repository payloads to profile selections only", () => { + queueTaskCreateLastUsedFromPayload({ + repositories: [], + agent_profile_id: "agent-1", + executor_profile_id: "exec-1", + }); + + expect(readQueuedTaskCreateLastUsedState()).toEqual({ + agentProfileId: "agent-1", + executorProfileId: "exec-1", + }); + }); + + it("uses the first workspace repository and skips rows without repository ids", () => { + queueTaskCreateLastUsedFromPayload({ + repositories: [ + { checkout_branch: "ignored-local" }, + { repository_id: "repo-2", checkout_branch: "feature" }, + ], + }); + + expect(readQueuedTaskCreateLastUsedState()).toEqual({ + repositoryId: "repo-2", + branch: "feature", + }); + }); + + it("prefers the base branch for fresh-branch repository payloads", () => { + queueTaskCreateLastUsedFromPayload({ + repositories: [ + { + repository_id: "repo-1", + base_branch: "main", + checkout_branch: "feature", + fresh_branch: true, + }, + ], + }); + + expect(readQueuedTaskCreateLastUsedState()).toEqual({ + repositoryId: "repo-1", + branch: "main", + }); + }); + + it("falls back to checkout branch for fresh-branch payloads without a base branch", () => { + queueTaskCreateLastUsedFromPayload({ + repositories: [ + { + repository_id: "repo-1", + base_branch: "", + checkout_branch: "feature", + fresh_branch: true, + }, + ], + }); + + expect(readQueuedTaskCreateLastUsedState()).toEqual({ + repositoryId: "repo-1", + branch: "feature", + }); + }); + + it("prefers the checkout branch for normal repository payloads", () => { + queueTaskCreateLastUsedFromPayload({ + repositories: [ + { + repository_id: "repo-1", + base_branch: "main", + checkout_branch: "feature", + }, + ], + }); + + expect(readQueuedTaskCreateLastUsedState()).toEqual({ + repositoryId: "repo-1", + branch: "feature", + }); + }); + + it("falls back to base branch for normal payloads without a checkout branch", () => { + queueTaskCreateLastUsedFromPayload({ + repositories: [ + { + repository_id: "repo-1", + base_branch: "main", + }, + ], + }); + + expect(readQueuedTaskCreateLastUsedState()).toEqual({ + repositoryId: "repo-1", + branch: "main", }); }); }); diff --git a/apps/web/components/task-create-dialog-handlers.ts b/apps/web/components/task-create-dialog-handlers.ts index 7ef68de66c..772f7e3fce 100644 --- a/apps/web/components/task-create-dialog-handlers.ts +++ b/apps/web/components/task-create-dialog-handlers.ts @@ -5,71 +5,76 @@ import type { Repository } from "@/lib/types/http"; import type { DialogFormState, TaskRepoRow } from "@/components/task-create-dialog-types"; import { removeLocalStorage, setLocalStorage } from "@/lib/local-storage"; import { STORAGE_KEYS } from "@/lib/settings/constants"; -import { updateUserSettings } from "@/lib/api/domains/settings-api"; import { createDebugLogger } from "@/lib/debug/log"; import type { TaskCreateLastUsedState } from "@/lib/state/slices/settings/types"; type TaskCreateLastUsedPatch = { - repository_id?: string; - branch?: string; + repository_id?: string | null; + branch?: string | null; + agent_profile_id?: string | null; + executor_profile_id?: string | null; +}; + +type TaskCreateLastUsedPayload = { + repositories?: Array<{ + repository_id?: string; + base_branch?: string; + checkout_branch?: string; + fresh_branch?: boolean; + }>; agent_profile_id?: string; executor_profile_id?: string; }; -let pendingLastUsed: TaskCreateLastUsedPatch = {}; -let lastUsedSync = Promise.resolve(); let lastQueuedLastUsed: Partial = {}; -const PENDING_LAST_USED_SYNC_KEY = "kandev.taskCreateLastUsed.pendingSync"; const LOCAL_STORAGE_WRITE_EVENT = "localStorage-write"; const lastUsedDebug = createDebugLogger("task-create:last-used"); /** - * Clears pending in-flight last-used sync state while preserving its persisted - * retry payload until the matching PATCH succeeds. + * Clears task-create last-used overlay state. * Pass `clearQueued` when test setup or teardown should also wipe the queued * overlay that protects settings fetches from stale server values. */ -export function resetTaskCreateLastUsedSync(options: { clearQueued?: boolean } = {}) { - pendingLastUsed = {}; - if (options.clearQueued) lastQueuedLastUsed = {}; - lastUsedDebug("pending-reset"); -} - -function readPendingLastUsedSync(): TaskCreateLastUsedPatch { - if (typeof window === "undefined") return {}; - try { - const raw = window.localStorage.getItem(PENDING_LAST_USED_SYNC_KEY); - if (!raw) return {}; - const parsed = JSON.parse(raw) as unknown; - if (!parsed || typeof parsed !== "object") return {}; - const record = parsed as Record; - return { - repository_id: typeof record.repository_id === "string" ? record.repository_id : undefined, - branch: typeof record.branch === "string" ? record.branch : undefined, - agent_profile_id: - typeof record.agent_profile_id === "string" ? record.agent_profile_id : undefined, - executor_profile_id: - typeof record.executor_profile_id === "string" ? record.executor_profile_id : undefined, - }; - } catch { - lastUsedDebug("pending-read-failed"); - return {}; +export function resetTaskCreateLastUsedSync( + options: { + clearQueued?: boolean; + syncedSettings?: TaskCreateLastUsedState | null | undefined; + } = {}, +) { + if (options.clearQueued) { + lastQueuedLastUsed = {}; + } else if (taskCreateLastUsedSettingsMatchQueue(options.syncedSettings)) { + lastQueuedLastUsed = {}; } -} - -function persistPendingLastUsedSync(patch: TaskCreateLastUsedPatch) { - setLocalStorage(PENDING_LAST_USED_SYNC_KEY, patch as Record); -} - -export function readPendingTaskCreateLastUsedState(): Partial { - const pending = { ...readPendingLastUsedSync(), ...pendingLastUsed }; - return mapTaskCreateLastUsedPatch(pending); + lastUsedDebug("overlay-reset"); } export function readQueuedTaskCreateLastUsedState(): Partial { return lastQueuedLastUsed; } +export function clearQueuedTaskCreateLastUsedIfSynced( + settings: TaskCreateLastUsedState | null | undefined, +) { + if (!hasQueuedTaskCreateLastUsed()) return; + if (!taskCreateLastUsedSettingsMatchQueue(settings)) return; + lastQueuedLastUsed = {}; + lastUsedDebug("overlay-cleared-after-settings-sync"); +} + +function hasQueuedTaskCreateLastUsed() { + return Object.values(lastQueuedLastUsed).some((value) => value !== undefined); +} + +function taskCreateLastUsedSettingsMatchQueue( + settings: TaskCreateLastUsedState | null | undefined, +) { + return Object.entries(lastQueuedLastUsed).every(([key, value]) => { + if (value === undefined) return true; + return settings?.[key as keyof TaskCreateLastUsedState] === value; + }); +} + function mapTaskCreateLastUsedPatch( pending: TaskCreateLastUsedPatch, ): Partial { @@ -88,35 +93,40 @@ function compactTaskCreateLastUsedState(state: Partial) } export function syncTaskCreateLastUsed(patch: TaskCreateLastUsedPatch) { - pendingLastUsed = { ...readPendingLastUsedSync(), ...pendingLastUsed, ...patch }; - const payload = { ...pendingLastUsed }; lastQueuedLastUsed = { ...lastQueuedLastUsed, - ...compactTaskCreateLastUsedState(mapTaskCreateLastUsedPatch(payload)), + ...compactTaskCreateLastUsedState(mapTaskCreateLastUsedPatch(patch)), }; - lastUsedDebug("sync-queued", { patch, payload }); - persistPendingLastUsedSync(payload); - lastUsedSync = lastUsedSync - .catch(() => undefined) - .then(() => - updateUserSettings({ task_create_last_used: payload }) - .then(() => { - lastUsedDebug("sync-success", { payload }); - const persistedPending = readPendingLastUsedSync(); - if (JSON.stringify(pendingLastUsed) === JSON.stringify(payload)) { - pendingLastUsed = {}; - } - if (JSON.stringify(persistedPending) === JSON.stringify(payload)) { - removeLocalStorage(PENDING_LAST_USED_SYNC_KEY); - } - }) - .catch((error) => { - lastUsedDebug("sync-failed", { - payload, - error: error instanceof Error ? error.message : String(error), - }); - }), - ); + lastUsedDebug("overlay-updated", { patch, queued: lastQueuedLastUsed }); +} + +export function replaceQueuedTaskCreateLastUsed(patch: TaskCreateLastUsedPatch) { + lastQueuedLastUsed = compactTaskCreateLastUsedState(mapTaskCreateLastUsedPatch(patch)); + lastUsedDebug("overlay-replaced", { patch, queued: lastQueuedLastUsed }); +} + +export function queueTaskCreateLastUsedFromPayload( + payload: TaskCreateLastUsedPayload | null | undefined, +) { + if (!payload) return; + const firstWorkspaceRepo = payload.repositories?.find((repo) => repo.repository_id); + replaceQueuedTaskCreateLastUsed({ + repository_id: firstWorkspaceRepo?.repository_id, + branch: firstWorkspaceRepo ? taskCreateLastUsedPayloadBranch(firstWorkspaceRepo) : undefined, + agent_profile_id: payload.agent_profile_id, + executor_profile_id: payload.executor_profile_id, + }); +} + +function taskCreateLastUsedPayloadBranch( + repo: NonNullable[number], +) { + if (repo.fresh_branch) return firstNonEmpty(repo.base_branch, repo.checkout_branch); + return firstNonEmpty(repo.checkout_branch, repo.base_branch); +} + +function firstNonEmpty(...values: Array) { + return values.find((value) => value) ?? undefined; } /** @@ -157,12 +167,17 @@ function useRepositoryHandlers(fs: DialogFormState, repositories: Repository[]) fs.updateRepository(key, patch); if (isWorkspaceRepo) { setLocalStorage(STORAGE_KEYS.LAST_REPOSITORY_ID, value); - syncTaskCreateLastUsed({ repository_id: value }); + removeLocalStorage(STORAGE_KEYS.LAST_BRANCH); + syncTaskCreateLastUsed({ repository_id: value, branch: null }); lastUsedDebug(LOCAL_STORAGE_WRITE_EVENT, { key: "lastRepositoryId", value, row_key: key, }); + } else { + removeLocalStorage(STORAGE_KEYS.LAST_REPOSITORY_ID); + removeLocalStorage(STORAGE_KEYS.LAST_BRANCH); + syncTaskCreateLastUsed({ repository_id: null, branch: null }); } // Switching the repo invalidates whatever local-status the fresh-branch // panel had cached. @@ -239,7 +254,12 @@ function useGitHubAndFreshBranchHandlers(fs: DialogFormState) { // toggle Remote on) and the submit gate's mode-aware checks would produce // confusing results. Mirror the no-repo handler which already clears // useRemote when flipping the other way. - if (next) fs.setNoRepository(false); + if (next) { + fs.setNoRepository(false); + removeLocalStorage(STORAGE_KEYS.LAST_REPOSITORY_ID); + removeLocalStorage(STORAGE_KEYS.LAST_BRANCH); + syncTaskCreateLastUsed({ repository_id: null, branch: null }); + } clearFreshBranch(fs); }, [fs]); @@ -267,6 +287,12 @@ function useGitHubAndFreshBranchHandlers(fs: DialogFormState) { // non-worktree default (worktree is unworkable in no-repo mode). fs.setExecutorId(""); fs.setExecutorProfileId(""); + removeLocalStorage(STORAGE_KEYS.LAST_EXECUTOR_PROFILE_ID); + syncTaskCreateLastUsed({ + repository_id: null, + branch: null, + executor_profile_id: null, + }); } else { fs.setWorkspacePath(""); } diff --git a/apps/web/components/task-create-dialog-helpers.test.ts b/apps/web/components/task-create-dialog-helpers.test.ts index 3e513c2359..807df8be62 100644 --- a/apps/web/components/task-create-dialog-helpers.test.ts +++ b/apps/web/components/task-create-dialog-helpers.test.ts @@ -12,13 +12,13 @@ describe("autoSelectBranch", () => { { name: "feature", type: "local" as const }, ]; - it("prefers a valid localStorage branch over a store-backed branch", () => { + it("prefers a store-backed branch over a divergent localStorage branch", () => { const setBranch = vi.fn(); localStorage.setItem(STORAGE_KEYS.LAST_BRANCH, JSON.stringify("feature")); autoSelectBranch(branches, setBranch, { lastUsedBranch: "main" }); - expect(setBranch).toHaveBeenCalledWith("feature"); + expect(setBranch).toHaveBeenCalledWith("main"); }); it("uses store-backed last-used branch when localStorage is not primed", () => { @@ -61,6 +61,15 @@ describe("autoSelectBranch", () => { expect(setBranch).not.toHaveBeenCalled(); }); + it("defers when localStorage branch is valid but user settings are still loading", () => { + const setBranch = vi.fn(); + localStorage.setItem(STORAGE_KEYS.LAST_BRANCH, JSON.stringify("feature")); + + autoSelectBranch(branches, setBranch, { userSettingsLoaded: false }); + + expect(setBranch).not.toHaveBeenCalled(); + }); + it("falls back to preferred branch after user settings have loaded without a valid last-used branch", () => { const setBranch = vi.fn(); diff --git a/apps/web/components/task-create-dialog-helpers.ts b/apps/web/components/task-create-dialog-helpers.ts index b610bffa86..b1576d6d7f 100644 --- a/apps/web/components/task-create-dialog-helpers.ts +++ b/apps/web/components/task-create-dialog-helpers.ts @@ -59,17 +59,6 @@ export function autoSelectBranch( const settingsBranch = options.lastUsedBranch ?? null; const localStorageValid = isBranchSelectable(branchList, localStorageBranch); const settingsValid = isBranchSelectable(branchList, settingsBranch); - if (localStorageBranch && localStorageValid) { - selectionDebug(BRANCH_AUTOPICK_DEBUG, { - source: "localStorage:lastBranch", - pick: localStorageBranch, - local_storage_value: localStorageBranch, - local_storage_valid: true, - branch_count: branchList.length, - }); - setBranch(localStorageBranch); - return; - } if (settingsBranch && settingsValid) { selectionDebug(BRANCH_AUTOPICK_DEBUG, { source: "settings:taskCreateLastUsed", @@ -91,6 +80,17 @@ export function autoSelectBranch( }); return; } + if (localStorageBranch && localStorageValid) { + selectionDebug(BRANCH_AUTOPICK_DEBUG, { + source: "localStorage:lastBranch", + pick: localStorageBranch, + local_storage_value: localStorageBranch, + local_storage_valid: true, + branch_count: branchList.length, + }); + setBranch(localStorageBranch); + return; + } const preferredBranch = selectPreferredBranch(branchList); selectionDebug(BRANCH_AUTOPICK_DEBUG, { source: preferredBranch ? "preferred" : "none", diff --git a/apps/web/components/task-create-dialog-repository-autopick.test.ts b/apps/web/components/task-create-dialog-repository-autopick.test.ts new file mode 100644 index 0000000000..2f81ec2f96 --- /dev/null +++ b/apps/web/components/task-create-dialog-repository-autopick.test.ts @@ -0,0 +1,168 @@ +import { renderHook, waitFor } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { useRepositoryAutoSelectEffect } from "./task-create-dialog-repository-autopick"; +import type { DialogFormState, TaskRepoRow } from "@/components/task-create-dialog-types"; +import type { Repository } from "@/lib/types/http"; +import { STORAGE_KEYS } from "@/lib/settings/constants"; +import { + readQueuedTaskCreateLastUsedState, + resetTaskCreateLastUsedSync, +} from "./task-create-dialog-handlers"; + +beforeEach(() => { + localStorage.clear(); + resetTaskCreateLastUsedSync({ clearQueued: true }); +}); + +function makeRepository(id: string): Repository { + return { + id, + workspace_id: "ws-1", + name: "repo", + source_type: "local", + local_path: "/repo", + default_branch: "main", + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + } as Repository; +} + +function makeRepoAutoSelectFs( + rows: TaskRepoRow[], + setRepositories: DialogFormState["setRepositories"], +): DialogFormState { + return { + repositories: rows, + useRemote: false, + setRepositories, + } as unknown as DialogFormState; +} + +describe("useRepositoryAutoSelectEffect loading gates", () => { + it("waits for store-backed settings before falling back to an empty row", async () => { + window.localStorage.removeItem(STORAGE_KEYS.LAST_REPOSITORY_ID); + const setRepositories = vi.fn(); + const fs = makeRepoAutoSelectFs([], setRepositories); + + const { rerender } = renderHook( + ({ loaded, lastUsedRepositoryId }) => + useRepositoryAutoSelectEffect( + fs, + true, + "ws-1", + [makeRepository("repo-1"), makeRepository("repo-2")], + { + lastUsedRepositoryId, + userSettingsLoaded: loaded, + }, + ), + { initialProps: { loaded: false, lastUsedRepositoryId: null as string | null } }, + ); + + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(setRepositories).not.toHaveBeenCalled(); + + rerender({ loaded: true, lastUsedRepositoryId: "repo-2" }); + + await waitFor(() => expect(setRepositories).toHaveBeenCalled()); + const updater = setRepositories.mock.calls[0]![0] as (prev: TaskRepoRow[]) => TaskRepoRow[]; + expect(updater([])).toEqual([{ key: "row-0", repositoryId: "repo-2", branch: "" }]); + }); + + it("waits for store-backed settings before trusting a valid cached repository id", async () => { + window.localStorage.setItem(STORAGE_KEYS.LAST_REPOSITORY_ID, JSON.stringify("repo-1")); + const setRepositories = vi.fn(); + const fs = makeRepoAutoSelectFs([], setRepositories); + + renderHook(() => + useRepositoryAutoSelectEffect( + fs, + true, + "ws-1", + [makeRepository("repo-1"), makeRepository("repo-2")], + { + lastUsedRepositoryId: null, + userSettingsLoaded: false, + }, + ), + ); + + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(setRepositories).not.toHaveBeenCalled(); + expect(readQueuedTaskCreateLastUsedState()).toEqual({}); + }); +}); + +describe("useRepositoryAutoSelectEffect defaults", () => { + it("queues a consumed cached repository id until backend settings catch up", async () => { + window.localStorage.setItem(STORAGE_KEYS.LAST_REPOSITORY_ID, JSON.stringify("repo-1")); + const setRepositories = vi.fn(); + const fs = makeRepoAutoSelectFs([], setRepositories); + + renderHook(() => + useRepositoryAutoSelectEffect( + fs, + true, + "ws-1", + [makeRepository("repo-1"), makeRepository("repo-2")], + { + lastUsedRepositoryId: null, + userSettingsLoaded: true, + }, + ), + ); + + await waitFor(() => expect(setRepositories).toHaveBeenCalled()); + const updater = setRepositories.mock.calls[0]![0] as (prev: TaskRepoRow[]) => TaskRepoRow[]; + + expect(updater([])).toEqual([{ key: "row-0", repositoryId: "repo-1", branch: "" }]); + expect(readQueuedTaskCreateLastUsedState()).toMatchObject({ + repositoryId: "repo-1", + branch: null, + }); + }); + + it("fills an untouched placeholder row from store-backed settings when localStorage is not primed", async () => { + window.localStorage.removeItem(STORAGE_KEYS.LAST_REPOSITORY_ID); + const setRepositories = vi.fn(); + const fs = makeRepoAutoSelectFs([{ key: "row-0", branch: "" }], setRepositories); + + renderHook(() => + useRepositoryAutoSelectEffect( + fs, + true, + "ws-1", + [makeRepository("repo-1"), makeRepository("repo-2")], + { lastUsedRepositoryId: "repo-2" }, + ), + ); + + await waitFor(() => expect(setRepositories).toHaveBeenCalled()); + const updater = setRepositories.mock.calls[0]![0] as (prev: TaskRepoRow[]) => TaskRepoRow[]; + + expect(updater([{ key: "row-0", branch: "" }])).toEqual([ + { key: "row-0", repositoryId: "repo-2", branch: "" }, + ]); + }); + + it("fills an empty repo row list from store-backed settings when localStorage is not primed", async () => { + window.localStorage.removeItem(STORAGE_KEYS.LAST_REPOSITORY_ID); + const setRepositories = vi.fn(); + const fs = makeRepoAutoSelectFs([], setRepositories); + + renderHook(() => + useRepositoryAutoSelectEffect( + fs, + true, + "ws-1", + [makeRepository("repo-1"), makeRepository("repo-2")], + { lastUsedRepositoryId: "repo-1" }, + ), + ); + + await waitFor(() => expect(setRepositories).toHaveBeenCalled()); + const updater = setRepositories.mock.calls[0]![0] as (prev: TaskRepoRow[]) => TaskRepoRow[]; + + expect(updater([])).toEqual([{ key: "row-0", repositoryId: "repo-1", branch: "" }]); + }); +}); diff --git a/apps/web/components/task-create-dialog-repository-autopick.ts b/apps/web/components/task-create-dialog-repository-autopick.ts index 844ac75577..36da2d55b6 100644 --- a/apps/web/components/task-create-dialog-repository-autopick.ts +++ b/apps/web/components/task-create-dialog-repository-autopick.ts @@ -6,6 +6,7 @@ import type { DialogFormState, TaskRepoRow } from "@/components/task-create-dial import { getLocalStorage } from "@/lib/local-storage"; import { STORAGE_KEYS } from "@/lib/settings/constants"; import { createDebugLogger, isDebug } from "@/lib/debug/log"; +import { syncTaskCreateLastUsed } from "@/components/task-create-dialog-handlers"; const selectionDebug = createDebugLogger("task-create:selection"); @@ -49,6 +50,9 @@ export function useRepositoryAutoSelectEffect( if (decision.defer) return; const { pickId } = decision; if (rows.length > 0 && !canReplaceEmptyRepositoryPlaceholder(rows, pickId)) return; + if (shouldQueueRepositoryFallback(decision)) { + syncTaskCreateLastUsed({ repository_id: pickId, branch: null }); + } void Promise.resolve().then(() => { setRepositories((prev) => { if (prev.length > 0) return replaceSeededRepositoryRows(prev, pickId); @@ -91,14 +95,6 @@ function decideRepositoryAutoPick( const settingsRepoId = lastUsedRepositoryId ?? null; const localStorageValid = isRepositoryIdValid(localStorageRepoId, repositories); const settingsValid = isRepositoryIdValid(settingsRepoId, repositories); - if (localStorageRepoId && localStorageValid) { - return buildRepositoryAutoPickDecision("localStorage:lastRepositoryId", localStorageRepoId, { - localStorageRepoId, - localStorageValid, - settingsRepoId, - settingsValid, - }); - } if (settingsRepoId && settingsValid) { return buildRepositoryAutoPickDecision("settings:taskCreateLastUsed", settingsRepoId, { localStorageRepoId, @@ -107,7 +103,7 @@ function decideRepositoryAutoPick( settingsValid, }); } - if (!userSettingsLoaded && repositories.length !== 1) { + if (!userSettingsLoaded) { return buildRepositoryAutoPickDecision("user-settings-loading", null, { defer: true, localStorageRepoId, @@ -116,6 +112,14 @@ function decideRepositoryAutoPick( settingsValid, }); } + if (localStorageRepoId && localStorageValid) { + return buildRepositoryAutoPickDecision("localStorage:lastRepositoryId", localStorageRepoId, { + localStorageRepoId, + localStorageValid, + settingsRepoId, + settingsValid, + }); + } return buildRepositoryAutoPickDecision( repositories.length === 1 ? "single-workspace-repo" : "empty-row", repositories.length === 1 ? repositories[0].id : null, @@ -145,6 +149,12 @@ function isRepositoryIdValid(repositoryId: string | null, repositories: Reposito return Boolean(repositoryId && repositories.some((r: Repository) => r.id === repositoryId)); } +function shouldQueueRepositoryFallback( + decision: RepositoryAutoPickDecision, +): decision is RepositoryAutoPickDecision & { pickId: string } { + return decision.source === "localStorage:lastRepositoryId" && Boolean(decision.pickId); +} + function logRepositoryAutoPick( workspaceId: string, repoCount: number, diff --git a/apps/web/components/task-create-dialog-submit.test.tsx b/apps/web/components/task-create-dialog-submit.test.tsx index 7ff454d85c..1290fea83a 100644 --- a/apps/web/components/task-create-dialog-submit.test.tsx +++ b/apps/web/components/task-create-dialog-submit.test.tsx @@ -35,13 +35,31 @@ vi.mock("@/lib/services/session-launch-helpers", () => ({ buildStartRequest: () => ({ request: { taskId: "t", agentProfileId: "a" } }), })); -const buildCreateTaskPayloadMock = vi.fn((..._args: unknown[]) => ({ +type BuildCreateTaskPayloadCall = { + repositoriesPayload?: Array<{ + repository_id?: string; + base_branch?: string; + checkout_branch?: string; + fresh_branch?: boolean; + }>; + agentProfileId: string; + executorId: string; + executorProfileId: string; + withAgent: boolean; + trimmedDescription: string; +}; + +const buildCreateTaskPayloadMock = vi.fn((args: BuildCreateTaskPayloadCall) => ({ workflow_step_id: "step-1", + repositories: args.repositoriesPayload, + agent_profile_id: args.agentProfileId || undefined, + executor_id: args.executorId || undefined, + executor_profile_id: args.executorProfileId || undefined, })); const validateCreateInputsMock = vi.fn((..._args: unknown[]) => true); vi.mock("@/components/task-create-dialog-helpers", () => ({ activatePlanMode: vi.fn(), - buildCreateTaskPayload: (...args: unknown[]) => buildCreateTaskPayloadMock(...args), + buildCreateTaskPayload: (args: BuildCreateTaskPayloadCall) => buildCreateTaskPayloadMock(args), buildRepositoriesPayload: () => [], findDuplicateRemoteRepo: () => null, validateCreateInputs: (...args: unknown[]) => validateCreateInputsMock(...args), @@ -64,6 +82,11 @@ vi.mock("@/components/task-create-dialog-fresh-branch-consent", () => ({ })); import { useTaskSubmitHandlers } from "./task-create-dialog-submit"; +import { + readQueuedTaskCreateLastUsedState, + resetTaskCreateLastUsedSync, + syncTaskCreateLastUsed, +} from "./task-create-dialog-handlers"; import type { SubmitHandlersDeps, TaskFormInputsHandle } from "./task-create-dialog-types"; function makeRef(value: string): React.RefObject { @@ -127,6 +150,7 @@ function makeDeps(overrides: Partial): SubmitHandlersDeps { } beforeEach(() => { + resetTaskCreateLastUsedSync({ clearQueued: true }); buildCreateTaskPayloadMock.mockClear(); validateCreateInputsMock.mockClear(); createTaskRetryMock.mockClear(); @@ -156,9 +180,13 @@ describe("useTaskSubmitHandlers — handleCreateSubmit (CLI-mode parity)", () => }); it("creates the task with the user's prompt when cli_passthrough=true and prompt is provided", async () => { + const preserveLastUsed = vi.fn(); + const onOpenChange = vi.fn(); const deps = makeDeps({ isPassthroughProfile: true, descriptionInputRef: makeRef("run npm test"), + onOpenChange, + preserveTaskCreateLastUsedOnClose: preserveLastUsed, }); const { result } = renderHook(() => useTaskSubmitHandlers(deps)); @@ -173,6 +201,11 @@ describe("useTaskSubmitHandlers — handleCreateSubmit (CLI-mode parity)", () => }; expect(payloadArg.withAgent).toBe(true); expect(payloadArg.trimmedDescription).toBe("run npm test"); + expect(preserveLastUsed).toHaveBeenCalledTimes(1); + expect(onOpenChange).toHaveBeenCalledWith(false); + expect(preserveLastUsed.mock.invocationCallOrder[0]).toBeLessThan( + onOpenChange.mock.invocationCallOrder[0]!, + ); }); it("still creates the task in ACP mode when prompt is provided", async () => { @@ -193,4 +226,29 @@ describe("useTaskSubmitHandlers — handleCreateSubmit (CLI-mode parity)", () => expect(payloadArg.withAgent).toBe(true); expect(payloadArg.trimmedDescription).toBe("refactor module"); }); + + it("replaces the queued last-used overlay with the final create payload", async () => { + syncTaskCreateLastUsed({ + repository_id: null, + branch: null, + agent_profile_id: "agent-before-workflow", + executor_profile_id: null, + }); + const deps = makeDeps({ + agentProfileId: "agent-from-workflow", + executorProfileId: "execp-autopick", + descriptionInputRef: makeRef("run tests"), + noRepository: true, + }); + const { result } = renderHook(() => useTaskSubmitHandlers(deps)); + + await act(async () => { + await result.current.handleSubmit({ preventDefault: () => {} } as never); + }); + + expect(readQueuedTaskCreateLastUsedState()).toEqual({ + agentProfileId: "agent-from-workflow", + executorProfileId: "execp-autopick", + }); + }); }); diff --git a/apps/web/components/task-create-dialog-submit.tsx b/apps/web/components/task-create-dialog-submit.tsx index 618301cd3f..0b4aaff3d8 100644 --- a/apps/web/components/task-create-dialog-submit.tsx +++ b/apps/web/components/task-create-dialog-submit.tsx @@ -10,6 +10,7 @@ import { useToast } from "@/components/toast-provider"; import { linkToTask } from "@/lib/links"; import type { SubmitHandlersDeps } from "@/components/task-create-dialog-types"; import { useFreshBranchConsent } from "@/components/task-create-dialog-fresh-branch-consent"; +import { queueTaskCreateLastUsedFromPayload } from "@/components/task-create-dialog-handlers"; import { activatePlanMode, @@ -46,6 +47,7 @@ export function useTaskSubmitHandlers({ onSuccess, onCreateSession, onOpenChange, + preserveTaskCreateLastUsedOnClose, taskId, parentTaskId, descriptionInputRef, @@ -316,8 +318,9 @@ export function useTaskSubmitHandlers({ attachments?: ReturnType; }) => { if (!workspaceId || !effectiveWorkflowId) return; - const buildPayload = (c: string[]) => - buildCreateTaskPayload({ + let submittedPayload: ReturnType | null = null; + const buildPayload = (c: string[]) => { + const payload = buildCreateTaskPayload({ workspaceId, effectiveWorkflowId, trimmedTitle: opts.trimmedTitle, @@ -336,6 +339,9 @@ export function useTaskSubmitHandlers({ // "empty path string" on the wire. workspacePath: noRepository ? workspacePath.trim() || undefined : undefined, }); + submittedPayload = payload; + return payload; + }; const taskResponse = await createTaskWithFreshBranchRetry(buildPayload, opts.consented); if (!taskResponse) return; const newSessionId = taskResponse.session_id ?? taskResponse.primary_session_id ?? null; @@ -343,6 +349,8 @@ export function useTaskSubmitHandlers({ (opts.withAgent && isPassthroughProfile) || !!(opts.planMode && newSessionId); onSuccess?.(taskResponse, "create", { taskSessionId: newSessionId, willNavigate }); clearDraft(); + queueTaskCreateLastUsedFromPayload(submittedPayload); + preserveTaskCreateLastUsedOnClose?.(); onOpenChange(false); if (opts.planMode && newSessionId) { activatePlanMode({ @@ -368,6 +376,7 @@ export function useTaskSubmitHandlers({ workspacePath, onSuccess, onOpenChange, + preserveTaskCreateLastUsedOnClose, clearDraft, setActiveDocument, setPlanMode, @@ -539,6 +548,7 @@ export function useTaskSubmitHandlers({ if (consent === null) return; setIsCreatingTask(true); try { + let submittedPayload: ReturnType | null = null; const buildPayload = (c: string[]) => { const p = buildCreateTaskPayload({ workspaceId, @@ -553,12 +563,15 @@ export function useTaskSubmitHandlers({ workspacePath: noRepository ? workspacePath.trim() || undefined : undefined, }); p.workflow_step_id = effectiveDefaultStepId; + submittedPayload = p; return p; }; const taskResponse = await createTaskWithFreshBranchRetry(buildPayload, consent); if (!taskResponse) return; onSuccess?.(taskResponse, "create"); clearDraft(); + queueTaskCreateLastUsedFromPayload(submittedPayload); + preserveTaskCreateLastUsedOnClose?.(); onOpenChange(false); } catch (error) { toast({ @@ -586,6 +599,7 @@ export function useTaskSubmitHandlers({ createTaskWithFreshBranchRetry, onSuccess, onOpenChange, + preserveTaskCreateLastUsedOnClose, clearDraft, toast, descriptionInputRef, diff --git a/apps/web/components/task-create-dialog-types.ts b/apps/web/components/task-create-dialog-types.ts index 0d760d1d96..d481c2b5bb 100644 --- a/apps/web/components/task-create-dialog-types.ts +++ b/apps/web/components/task-create-dialog-types.ts @@ -338,6 +338,7 @@ export type SubmitHandlersDeps = { ) => void; onCreateSession?: (data: { prompt: string; agentProfileId: string; executorId: string }) => void; onOpenChange: (open: boolean) => void; + preserveTaskCreateLastUsedOnClose?: () => void; taskId: string | null; parentTaskId?: string; descriptionInputRef: React.RefObject; diff --git a/apps/web/components/task-create-dialog.tsx b/apps/web/components/task-create-dialog.tsx index 060bd3c5f2..05693b61bd 100644 --- a/apps/web/components/task-create-dialog.tsx +++ b/apps/web/components/task-create-dialog.tsx @@ -1,10 +1,11 @@ "use client"; -import { FormEvent, useCallback, useEffect, useState } from "react"; +import { FormEvent, useCallback, useEffect, useRef, useState } from "react"; import type { JiraTicket } from "@/lib/types/jira"; import type { LinearIssue } from "@/lib/types/linear"; import { Dialog, DialogContent, DialogHeader, DialogFooter } from "@kandev/ui/dialog"; import type { Task, Repository } from "@/lib/types/http"; +import type { TaskCreateLastUsedState } from "@/lib/state/slices/settings/types"; import { SHORTCUTS } from "@/lib/keyboard/constants"; import { useIsUtilityConfigured } from "@/hooks/use-is-utility-configured"; import { useKeyboardShortcutHandler } from "@/hooks/use-keyboard-shortcut"; @@ -43,6 +44,7 @@ import { buildDialogFormBodyProps, } from "@/components/task-create-dialog-prop-builders"; import { resetTaskCreateLastUsedSync } from "@/components/task-create-dialog-handlers"; +import { useAppStore } from "@/components/state-provider"; import { TaskCreateDialogPopoverContainerProvider } from "@/hooks/use-task-create-dialog-popover-container"; export interface TaskCreateDialogProps { @@ -301,6 +303,7 @@ type SubmitWiringArgs = { repositoryLocalPath: string; isSessionMode: boolean; isEditMode: boolean; + preserveQueuedLastUsedOnClose: () => void; }; function useSubmitHandlersWiring({ @@ -311,6 +314,7 @@ function useSubmitHandlersWiring({ repositoryLocalPath, isSessionMode, isEditMode, + preserveQueuedLastUsedOnClose, }: SubmitWiringArgs) { const { workspaceId, workflowId, editingTask, onSuccess, onCreateSession, onOpenChange } = props; const { parentTaskId } = props; @@ -337,6 +341,7 @@ function useSubmitHandlersWiring({ onSuccess, onCreateSession, onOpenChange, + preserveTaskCreateLastUsedOnClose: preserveQueuedLastUsedOnClose, taskId, parentTaskId, descriptionInputRef: fs.descriptionInputRef, @@ -375,7 +380,10 @@ function resolveSingleRowLocalPath(fs: DialogFormState, repositories: Repository return ""; } -export function useTaskCreateDialogSetup(props: TaskCreateDialogProps) { +export function useTaskCreateDialogSetup( + props: TaskCreateDialogProps, + options: { preserveQueuedLastUsedOnClose?: () => void } = {}, +) { const { open, mode = "create", workspaceId, workflowId, defaultStepId } = props; const { editingTask, initialValues } = props; const isSessionMode = mode === "session"; @@ -428,6 +436,7 @@ export function useTaskCreateDialogSetup(props: TaskCreateDialogProps) { repositoryLocalPath, isSessionMode, isEditMode, + preserveQueuedLastUsedOnClose: options.preserveQueuedLastUsedOnClose ?? (() => undefined), }); const guardedHandleSubmit = useGuardedSubmit( submitHandlers.handleSubmit, @@ -492,12 +501,38 @@ function useGuardedSubmit( const VOICE_SUBMIT_EVENT = { preventDefault: () => {} } as unknown as FormEvent; export function TaskCreateDialog(props: TaskCreateDialogProps) { - const setup = useTaskCreateDialogSetup(props); + const syncedTaskCreateLastUsed = useAppStore((state) => state.userSettings.taskCreateLastUsed); + const preserveQueuedLastUsedOnCloseRef = useRef<{ + syncedSettings: TaskCreateLastUsedState | null | undefined; + } | null>(null); + const queuedLastUsedResetHandledRef = useRef(false); + const preserveQueuedLastUsedOnClose = useCallback(() => { + preserveQueuedLastUsedOnCloseRef.current = { syncedSettings: syncedTaskCreateLastUsed }; + }, [syncedTaskCreateLastUsed]); + const resetQueuedLastUsedOnClose = useCallback(() => { + const preserveQueued = preserveQueuedLastUsedOnCloseRef.current; + resetTaskCreateLastUsedSync({ + clearQueued: !preserveQueued, + syncedSettings: preserveQueued?.syncedSettings, + }); + preserveQueuedLastUsedOnCloseRef.current = null; + queuedLastUsedResetHandledRef.current = true; + }, []); + const setup = useTaskCreateDialogSetup(props, { preserveQueuedLastUsedOnClose }); const { guardedHandleSubmit } = setup; const [popoverContainer, setPopoverContainer] = useState(null); useEffect(() => { - if (!props.open) resetTaskCreateLastUsedSync(); - }, [props.open]); + if (props.open) { + preserveQueuedLastUsedOnCloseRef.current = null; + queuedLastUsedResetHandledRef.current = false; + return resetQueuedLastUsedOnClose; + } + if (queuedLastUsedResetHandledRef.current) { + queuedLastUsedResetHandledRef.current = false; + return; + } + resetQueuedLastUsedOnClose(); + }, [props.open, resetQueuedLastUsedOnClose]); // Voice auto-send invokes the same submit handler as the in-form Submit // button. Every existing validation gate (missing title/repo/branch/agent, // `submitBlockedReason`, in-flight create) still applies because they live diff --git a/apps/web/hooks/use-ensure-user-settings.test.ts b/apps/web/hooks/use-ensure-user-settings.test.ts index 0e399fe5ed..33d3d2761b 100644 --- a/apps/web/hooks/use-ensure-user-settings.test.ts +++ b/apps/web/hooks/use-ensure-user-settings.test.ts @@ -13,12 +13,6 @@ const mockFetchUserSettings = vi.fn(); const mockSetUserSettings = vi.fn((settings: UserSettingsState) => { mockState.userSettings = settings; }); -const mockReadPendingTaskCreateLastUsedState = vi.fn<[], PendingTaskCreateLastUsed>(() => ({ - repositoryId: undefined, - branch: undefined, - agentProfileId: undefined, - executorProfileId: undefined, -})); const mockReadQueuedTaskCreateLastUsedState = vi.fn<[], PendingTaskCreateLastUsed>(() => ({ repositoryId: undefined, branch: undefined, @@ -42,7 +36,6 @@ vi.mock("@/lib/api/domains/settings-api", () => ({ })); vi.mock("@/components/task-create-dialog-handlers", () => ({ - readPendingTaskCreateLastUsedState: () => mockReadPendingTaskCreateLastUsedState(), readQueuedTaskCreateLastUsedState: () => mockReadQueuedTaskCreateLastUsedState(), })); @@ -154,8 +147,8 @@ describe("useEnsureUserSettings", () => { await waitFor(() => expect(mockSetUserSettings).toHaveBeenCalled()); }); - it("merges only defined pending task-create fields over fetched settings", async () => { - mockReadPendingTaskCreateLastUsedState.mockReturnValue({ + it("merges only defined queued task-create fields over fetched settings", async () => { + mockReadQueuedTaskCreateLastUsedState.mockReturnValue({ repositoryId: undefined, branch: undefined, agentProfileId: "agent-2", @@ -181,9 +174,9 @@ describe("useEnsureUserSettings", () => { }); }); - it("preserves pending task-create fields when multiple callers join the same fetch", async () => { + it("preserves queued task-create fields when multiple callers join the same fetch", async () => { let resolveFetch: (value: unknown) => void = () => undefined; - mockReadPendingTaskCreateLastUsedState.mockReturnValue({ + mockReadQueuedTaskCreateLastUsedState.mockReturnValue({ repositoryId: undefined, branch: "feature", agentProfileId: undefined, @@ -239,6 +232,37 @@ describe("useEnsureUserSettings", () => { }); }); +describe("useEnsureUserSettings — loaded settings overlay", () => { + it("applies queued task-create fields when settings are already loaded", () => { + mockState.userSettings = { + ...makeUnloadedSettings(), + loaded: true, + taskCreateLastUsed: { + repositoryId: "repo-1", + branch: "main", + agentProfileId: "agent-1", + executorProfileId: "exec-1", + }, + }; + mockReadQueuedTaskCreateLastUsedState.mockReturnValue({ + repositoryId: "repo-2", + branch: "feature", + agentProfileId: undefined, + executorProfileId: undefined, + }); + + const { result } = renderHook(() => useEnsureUserSettings(true)); + + expect(mockFetchUserSettings).not.toHaveBeenCalled(); + expect(result.current.userSettings.taskCreateLastUsed).toEqual({ + repositoryId: "repo-2", + branch: "feature", + agentProfileId: "agent-1", + executorProfileId: "exec-1", + }); + }); +}); + describe("useEnsureUserSettings — stale settings fetches", () => { it("keeps task-create edits made while a settings fetch is in flight", async () => { let resolveFetch: (value: unknown) => void = () => undefined; diff --git a/apps/web/hooks/use-ensure-user-settings.ts b/apps/web/hooks/use-ensure-user-settings.ts index c823f9bbb8..70d3eb0045 100644 --- a/apps/web/hooks/use-ensure-user-settings.ts +++ b/apps/web/hooks/use-ensure-user-settings.ts @@ -2,10 +2,7 @@ import { useEffect, useState } from "react"; import { useAppStore } from "@/components/state-provider"; -import { - readPendingTaskCreateLastUsedState, - readQueuedTaskCreateLastUsedState, -} from "@/components/task-create-dialog-handlers"; +import { readQueuedTaskCreateLastUsedState } from "@/components/task-create-dialog-handlers"; import { fetchUserSettings } from "@/lib/api/domains/settings-api"; import { mapUserSettingsResponse } from "@/lib/ssr/user-settings"; import type { TaskCreateLastUsedState, UserSettingsState } from "@/lib/state/slices/settings/types"; @@ -56,10 +53,14 @@ function compactTaskCreateLastUsedOverlay(pending: Partial