From 3c09b1590a1fc93486516c02e9d4b996cb9be692 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Sat, 25 Jul 2026 11:11:03 +0100 Subject: [PATCH] fix: make remote repository entry reliable (#1936) --- .../internal/github/controller_test.go | 37 ++++-- apps/backend/internal/github/service_pr.go | 19 ++- .../internal/github/service_reviews.go | 41 +++++++ apps/backend/internal/github/service_test.go | 104 ++++++++++++++-- .../internal/gitlab/controller_test.go | 96 +++++++++++++++ .../internal/gitlab/controller_watches.go | 19 ++- apps/backend/internal/gitlab/pat_client.go | 4 +- .../internal/gitlab/pat_client_test.go | 18 +++ .../task-create-dialog-form-body.test.tsx | 2 + ...reate-dialog-remote-repo-chip-url.test.tsx | 42 +++++-- ...sk-create-dialog-remote-repo-chip.test.tsx | 113 ++++++++++-------- .../task-create-dialog-remote-repo-chip.tsx | 112 +++++++++++------ ...k-create-dialog-remote-repo-chips.test.tsx | 34 ++++++ .../task-create-dialog-remote-repo-chips.tsx | 9 ++ .../task-create-dialog-repo-chips.test.tsx | 31 ++--- .../task-create-dialog-submit.test.tsx | 1 + .../components/task-create-dialog.test.tsx | 2 + .../tests/task/create-task-github-url.spec.ts | 4 +- .../mobile-create-task-remote-repo.spec.ts | 98 +++++++++++++++ .../github/use-branches-by-url.test.ts | 40 ++++++- .../domains/github/use-branches-by-url.ts | 33 +++-- .../domains/github/use-pr-info-by-url.test.ts | 41 +++++++ .../domains/github/use-pr-info-by-url.ts | 24 +++- apps/web/lib/api/domains/gitlab-api.test.ts | 15 +++ apps/web/lib/api/domains/gitlab-api.ts | 5 +- .../plan.md | 94 +++++++++++++++ .../task-01-public-provider-resolution.md | 51 ++++++++ .../task-02-explicit-url-entry.md | 45 +++++++ .../task-03-resolution-errors-and-retry.md | 50 ++++++++ .../task-04-mobile-e2e.md | 45 +++++++ docs/public/tasks-and-workflows.md | 6 +- docs/specs/gitlab-integration/spec.md | 16 ++- docs/specs/tasks/multi-branch/spec.md | 11 ++ 33 files changed, 1099 insertions(+), 163 deletions(-) create mode 100644 docs/plans/remote-repository-entry-reliability/plan.md create mode 100644 docs/plans/remote-repository-entry-reliability/task-01-public-provider-resolution.md create mode 100644 docs/plans/remote-repository-entry-reliability/task-02-explicit-url-entry.md create mode 100644 docs/plans/remote-repository-entry-reliability/task-03-resolution-errors-and-retry.md create mode 100644 docs/plans/remote-repository-entry-reliability/task-04-mobile-e2e.md diff --git a/apps/backend/internal/github/controller_test.go b/apps/backend/internal/github/controller_test.go index 7644617e8b..8566a4003f 100644 --- a/apps/backend/internal/github/controller_test.go +++ b/apps/backend/internal/github/controller_test.go @@ -936,23 +936,46 @@ func TestHttpGetPRInfo_ServiceError(t *testing.T) { } func TestHttpGetPRInfo_NoClient(t *testing.T) { + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + t.Errorf("method = %s, want GET", r.Method) + } + if r.URL.Path != "/repos/acme/widget/pulls/99" { + t.Errorf("path = %q, want public PR endpoint", r.URL.Path) + } + if got := r.Header.Get("Accept"); got != githubAccept { + t.Errorf("Accept = %q, want %q", got, githubAccept) + } + if got := r.Header.Get("X-GitHub-Api-Version"); got != githubAPIVersion { + t.Errorf("X-GitHub-Api-Version = %q, want %q", got, githubAPIVersion) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"number":99,"title":"Public widget","state":"open","user":{"login":"octo"},"head":{"ref":"feature/public","sha":"abc123"},"base":{"ref":"main"}}`)) + })) + t.Cleanup(api.Close) + + originalAPIBase := anonymousAPIBase + anonymousAPIBase = api.URL + t.Cleanup(func() { anonymousAPIBase = originalAPIBase }) + router, _ := setupControllerTest(nil) req := httptest.NewRequest(http.MethodGet, "/api/v1/github/prs/acme/widget/99/info", nil) w := httptest.NewRecorder() router.ServeHTTP(w, req) - if w.Code != http.StatusServiceUnavailable { - t.Fatalf("expected 503, got %d: %s", w.Code, w.Body.String()) - } - var got struct { - Code string `json:"code"` + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) } + var got PR if err := json.NewDecoder(w.Body).Decode(&got); err != nil { t.Fatalf("decode response: %v", err) } - if got.Code != "github_not_configured" { - t.Fatalf("expected github_not_configured code, got %q", got.Code) + if got.Number != 99 || got.Title != "Public widget" { + t.Fatalf("PR = %#v, want public PR details", got) + } + if got.RepoOwner != "acme" || got.RepoName != "widget" || got.HeadBranch != "feature/public" || got.BaseBranch != "main" || got.AuthorLogin != "octo" { + t.Fatalf("PR fallback fields = %#v", got) } } diff --git a/apps/backend/internal/github/service_pr.go b/apps/backend/internal/github/service_pr.go index 5be4e68f92..f3b80f84f4 100644 --- a/apps/backend/internal/github/service_pr.go +++ b/apps/backend/internal/github/service_pr.go @@ -2,6 +2,7 @@ package github import ( "context" + "errors" "fmt" "strings" "sync" @@ -116,19 +117,25 @@ func pickDefaultMergeMethod(m RepoMergeMethods) string { // GetPR fetches basic PR details from GitHub. func (s *Service) GetPR(ctx context.Context, owner, repo string, number int) (*PR, error) { - if s.client == nil { - return nil, ErrNoClient + if s.client != nil { + pr, err := s.client.GetPR(ctx, owner, repo, number) + if !errors.Is(err, ErrNoClient) { + return pr, err + } } - return s.client.GetPR(ctx, owner, repo, number) + return getPRAnonymous(ctx, owner, repo, number) } // GetIssue fetches basic issue details from GitHub. The create-task dialog is // currently the only caller and dedupes requests per URL on the frontend. func (s *Service) GetIssue(ctx context.Context, owner, repo string, number int) (*Issue, error) { - if s.client == nil { - return nil, ErrNoClient + if s.client != nil { + issue, err := s.client.GetIssue(ctx, owner, repo, number) + if !errors.Is(err, ErrNoClient) { + return issue, err + } } - return s.client.GetIssue(ctx, owner, repo, number) + return getIssueAnonymous(ctx, owner, repo, number) } // GetPRFeedback fetches live PR feedback from GitHub. Cached briefly with diff --git a/apps/backend/internal/github/service_reviews.go b/apps/backend/internal/github/service_reviews.go index 6870baae12..5daa7a6596 100644 --- a/apps/backend/internal/github/service_reviews.go +++ b/apps/backend/internal/github/service_reviews.go @@ -539,6 +539,47 @@ func listRepoBranchesAnonymous(ctx context.Context, owner, repo string) ([]RepoB return branches, nil } +func getPRAnonymous(ctx context.Context, owner, repo string, number int) (*PR, error) { + var raw patPR + endpoint := fmt.Sprintf("/repos/%s/%s/pulls/%d", url.PathEscape(owner), url.PathEscape(repo), number) + if err := getAnonymous(ctx, endpoint, &raw); err != nil { + return nil, fmt.Errorf("get PR #%d: %w", number, err) + } + return convertPatPR(&raw, owner, repo), nil +} + +func getIssueAnonymous(ctx context.Context, owner, repo string, number int) (*Issue, error) { + var raw patIssue + endpoint := fmt.Sprintf("/repos/%s/%s/issues/%d", url.PathEscape(owner), url.PathEscape(repo), number) + if err := getAnonymous(ctx, endpoint, &raw); err != nil { + return nil, fmt.Errorf("get issue #%d: %w", number, err) + } + return convertPatIssue(&raw, owner, repo), nil +} + +func getAnonymous(ctx context.Context, endpoint string, result any) error { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, anonymousAPIBase+endpoint, nil) + if err != nil { + return ErrNoClient + } + req.Header.Set("Accept", githubAccept) + req.Header.Set("X-GitHub-Api-Version", githubAPIVersion) + + resp, err := anonymousHTTPClient.Do(req) + if err != nil { + return ErrNoClient + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { + body, _ := io.ReadAll(resp.Body) + return &GitHubAPIError{StatusCode: resp.StatusCode, Endpoint: endpoint, Body: string(body)} + } + if err := json.NewDecoder(resp.Body).Decode(result); err != nil { + return fmt.Errorf("decode GitHub response: %w", err) + } + return nil +} + // parseLinkNext extracts the URL for rel="next" from a GitHub Link header. // Returns "" if no next page is present. func parseLinkNext(link string) string { diff --git a/apps/backend/internal/github/service_test.go b/apps/backend/internal/github/service_test.go index a929695612..26f6ccab7c 100644 --- a/apps/backend/internal/github/service_test.go +++ b/apps/backend/internal/github/service_test.go @@ -133,6 +133,99 @@ func TestListRepoBranches_NoopClientFallback_NotFound(t *testing.T) { } } +func TestGetPRAndIssue_FallBackWithoutClient(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/repos/owner/repo/pulls/7": + _, _ = w.Write([]byte(`{"number":7,"title":"Public PR","state":"open","head":{"ref":"feature"},"base":{"ref":"main"},"user":{"login":"alice"}}`)) + case "/repos/owner/repo/issues/8": + _, _ = w.Write([]byte(`{"number":8,"title":"Public issue","state":"open","user":{"login":"alice"}}`)) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + orig := anonymousAPIBase + anonymousAPIBase = srv.URL + defer func() { anonymousAPIBase = orig }() + + for _, client := range []Client{nil, &NoopClient{}} { + svc := &Service{client: client} + pr, err := svc.GetPR(context.Background(), "owner", "repo", 7) + if err != nil { + t.Fatalf("GetPR() error = %v", err) + } + if pr.Title != "Public PR" { + t.Fatalf("GetPR() title = %q, want public PR", pr.Title) + } + + issue, err := svc.GetIssue(context.Background(), "owner", "repo", 8) + if err != nil { + t.Fatalf("GetIssue() error = %v", err) + } + if issue.Title != "Public issue" { + t.Fatalf("GetIssue() title = %q, want public issue", issue.Title) + } + } +} + +func TestGetPRAndIssue_AuthenticatedErrorsAreAuthoritative(t *testing.T) { + orig := anonymousAPIBase + anonymousAPIBase = "http://127.0.0.1:1" + defer func() { anonymousAPIBase = orig }() + + prErr := &GitHubAPIError{StatusCode: http.StatusForbidden} + issueErr := &GitHubAPIError{StatusCode: http.StatusNotFound} + svc := &Service{client: &stubClient{ + getPRFunc: func(context.Context, string, string, int) (*PR, error) { return nil, prErr }, + getIssueFunc: func(context.Context, string, string, int) (*Issue, error) { + return nil, issueErr + }, + }} + + if _, err := svc.GetPR(context.Background(), "owner", "repo", 7); !errors.Is(err, prErr) { + t.Fatalf("GetPR() error = %v, want authenticated 403", err) + } + if _, err := svc.GetIssue(context.Background(), "owner", "repo", 8); !errors.Is(err, issueErr) { + t.Fatalf("GetIssue() error = %v, want authenticated 404", err) + } +} + +func TestGetPRAndIssue_AnonymousStatusIsPreserved(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/pulls/") { + w.WriteHeader(http.StatusForbidden) + return + } + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + + orig := anonymousAPIBase + anonymousAPIBase = srv.URL + defer func() { anonymousAPIBase = orig }() + + svc := &Service{} + for _, request := range []struct { + name string + call func() error + status int + }{ + {name: "PR", call: func() error { _, err := svc.GetPR(t.Context(), "owner", "repo", 7); return err }, status: http.StatusForbidden}, + {name: "issue", call: func() error { _, err := svc.GetIssue(t.Context(), "owner", "repo", 8); return err }, status: http.StatusNotFound}, + } { + t.Run(request.name, func(t *testing.T) { + err := request.call() + var apiErr *GitHubAPIError + if !errors.As(err, &apiErr) || apiErr.StatusCode != request.status { + t.Fatalf("error = %v, want GitHub API status %d", err, request.status) + } + }) + } +} + func TestSortBranchesMainFirst(t *testing.T) { tests := []struct { input []string @@ -173,17 +266,6 @@ func TestSortBranchesMainFirst(t *testing.T) { } } -func TestGetPR_NilClient(t *testing.T) { - svc := &Service{client: nil} - _, err := svc.GetPR(context.Background(), "owner", "repo", 1) - if err == nil { - t.Fatal("expected error when client is nil") - } - if !errors.Is(err, ErrNoClient) { - t.Errorf("err = %v, want ErrNoClient", err) - } -} - func TestGetPRFeedback_NilClient(t *testing.T) { svc := &Service{client: nil} _, err := svc.GetPRFeedback(context.Background(), "owner", "repo", 1) diff --git a/apps/backend/internal/gitlab/controller_test.go b/apps/backend/internal/gitlab/controller_test.go index 8c5e11b5e7..06ed1aa61a 100644 --- a/apps/backend/internal/gitlab/controller_test.go +++ b/apps/backend/internal/gitlab/controller_test.go @@ -1,6 +1,7 @@ package gitlab import ( + "io" "net/http" "net/http/httptest" "net/url" @@ -11,6 +12,12 @@ import ( "github.com/gin-gonic/gin" ) +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + // newControllerFixture wires a real PATClient + Controller against an // httptest.NewServer GitLab stub. The returned *requestLog captures every // path + query the stub observed so tests can assert exactly which params @@ -100,6 +107,95 @@ func hit(router *gin.Engine, target string) *httptest.ResponseRecorder { return w } +func TestHttpListProjectBranches_UnconfiguredPublicGitLab(t *testing.T) { + gin.SetMode(gin.TestMode) + store := newTestStore(t) + seedWorkspace(t, store, "workspace-test") + svc := NewService(DefaultHost, NewNoopClient(DefaultHost), AuthMethodNone, nil, newTestLogger(t)) + svc.SetStore(store) + router := gin.New() + NewController(svc, newTestLogger(t)).RegisterHTTPRoutes(router) + + originalTransport := http.DefaultTransport + http.DefaultTransport = roundTripFunc(func(req *http.Request) (*http.Response, error) { + if req.URL.Host != "gitlab.com" { + t.Fatalf("anonymous request host = %q, want gitlab.com", req.URL.Host) + } + if got := req.Header.Get("PRIVATE-TOKEN"); got != "" { + t.Fatalf("PRIVATE-TOKEN = %q, want absent for anonymous read", got) + } + return &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader(`[{"name":"main"}]`)), + Request: req, + }, nil + }) + t.Cleanup(func() { http.DefaultTransport = originalTransport }) + + resp := hit(router, "/api/v1/gitlab/projects/branches?project=group%2Fproject&expected_host=https%3A%2F%2Fgitlab.com") + if resp.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", resp.Code, resp.Body.String()) + } + if !strings.Contains(resp.Body.String(), `"name":"main"`) { + t.Fatalf("response = %s, want main branch", resp.Body.String()) + } +} + +func TestHttpListProjectBranches_ConfiguredUpstreamNotFound(t *testing.T) { + gin.SetMode(gin.TestMode) + host, stop := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + t.Cleanup(stop) + + store := newTestStore(t) + seedWorkspace(t, store, "workspace-test") + if err := store.UpsertConfigForWorkspace(t.Context(), "workspace-test", &GitLabConfig{ + Host: host, AuthMethod: AuthMethodPAT, + }); err != nil { + t.Fatalf("seed GitLab config: %v", err) + } + svc := NewService(host, NewNoopClient(host), AuthMethodNone, nil, newTestLogger(t)) + svc.SetStore(store) + svc.SetWorkspaceSecretStore(&configTestSecrets{values: map[string]string{ + SecretKeyForWorkspace("workspace-test"): "token", + }}) + router := gin.New() + NewController(svc, newTestLogger(t)).RegisterHTTPRoutes(router) + + resp := hit(router, "/api/v1/gitlab/projects/branches?project=group%2Fmissing&expected_host="+url.QueryEscape(host)) + if resp.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404; body=%s", resp.Code, resp.Body.String()) + } +} + +func TestHttpListProjectBranches_UnconfiguredPublicNotFound(t *testing.T) { + gin.SetMode(gin.TestMode) + store := newTestStore(t) + seedWorkspace(t, store, "workspace-test") + svc := NewService(DefaultHost, NewNoopClient(DefaultHost), AuthMethodNone, nil, newTestLogger(t)) + svc.SetStore(store) + router := gin.New() + NewController(svc, newTestLogger(t)).RegisterHTTPRoutes(router) + + originalTransport := http.DefaultTransport + http.DefaultTransport = roundTripFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusNotFound, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader(`{"message":"not found"}`)), + Request: req, + }, nil + }) + t.Cleanup(func() { http.DefaultTransport = originalTransport }) + + resp := hit(router, "/api/v1/gitlab/projects/branches?project=group%2Fmissing&expected_host=https%3A%2F%2Fgitlab.com") + if resp.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404; body=%s", resp.Code, resp.Body.String()) + } +} + // Regression for the /gitlab page tabs: each tab value must reach GitLab // as a real scoping param. Before the translator was added the bare token // became an empty-value key (e.g. `assigned_to_me=`) and the page served diff --git a/apps/backend/internal/gitlab/controller_watches.go b/apps/backend/internal/gitlab/controller_watches.go index faaa1735b3..c5af0b77c9 100644 --- a/apps/backend/internal/gitlab/controller_watches.go +++ b/apps/backend/internal/gitlab/controller_watches.go @@ -3,6 +3,7 @@ package gitlab import ( "errors" "net/http" + "strings" "github.com/gin-gonic/gin" ) @@ -353,9 +354,16 @@ func (c *Controller) httpListProjectBranches(ctx *gin.Context) { ctx.JSON(http.StatusBadRequest, gin.H{responseErrorKey: "project required"}) return } - client, ok := c.workspaceClient(ctx) - if !ok { - return + workspaceID := c.workspaceID(ctx) + expectedHost := ctx.Query("expected_host") + client, err := c.service.ClientForWorkspaceHost(ctx.Request.Context(), workspaceID, expectedHost) + if err != nil { + if errors.Is(err, ErrNotConfigured) && isPublicGitLabHost(expectedHost) { + client = NewPATClient(DefaultHost, "") + } else { + writeWorkspaceClientActionError(ctx, err, "project branches") + return + } } branches, err := client.ListProjectBranches(ctx.Request.Context(), project) if err != nil { @@ -365,6 +373,11 @@ func (c *Controller) httpListProjectBranches(ctx *gin.Context) { ctx.JSON(http.StatusOK, gin.H{"branches": branches}) } +func isPublicGitLabHost(host string) bool { + normalized, err := normalizeHostOrigin(host) + return err == nil && strings.EqualFold(normalized, DefaultHost) +} + func (c *Controller) httpGetProjectMergeMethods(ctx *gin.Context) { project := ctx.Query("project") if project == "" { diff --git a/apps/backend/internal/gitlab/pat_client.go b/apps/backend/internal/gitlab/pat_client.go index 47d925a8a1..06fee09ea1 100644 --- a/apps/backend/internal/gitlab/pat_client.go +++ b/apps/backend/internal/gitlab/pat_client.go @@ -67,7 +67,9 @@ func NewPATClient(host, token string) *PATClient { func (c *PATClient) Host() string { return c.host } func (c *PATClient) setHeaders(req *http.Request) { - req.Header.Set("PRIVATE-TOKEN", c.token) + if c.token != "" { + req.Header.Set("PRIVATE-TOKEN", c.token) + } req.Header.Set("Accept", "application/json") } diff --git a/apps/backend/internal/gitlab/pat_client_test.go b/apps/backend/internal/gitlab/pat_client_test.go index 4eb8e45f69..193eaa54b7 100644 --- a/apps/backend/internal/gitlab/pat_client_test.go +++ b/apps/backend/internal/gitlab/pat_client_test.go @@ -59,6 +59,24 @@ func TestPATClient_GetAuthenticatedUser(t *testing.T) { } } +func TestPATClient_AnonymousReadOmitsPrivateToken(t *testing.T) { + host, stop := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("PRIVATE-TOKEN"); got != "" { + t.Fatalf("PRIVATE-TOKEN = %q, want absent", got) + } + _, _ = w.Write([]byte(`[{"name":"main"}]`)) + })) + t.Cleanup(stop) + + branches, err := NewPATClient(host, "").ListProjectBranches(t.Context(), "group/project") + if err != nil { + t.Fatalf("ListProjectBranches() error = %v", err) + } + if len(branches) != 1 || branches[0].Name != "main" { + t.Fatalf("branches = %#v, want main", branches) + } +} + func TestPATClient_GetAuthenticatedUser_AuthFailure(t *testing.T) { host, stop := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusUnauthorized) diff --git a/apps/web/components/task-create-dialog-form-body.test.tsx b/apps/web/components/task-create-dialog-form-body.test.tsx index f4e6f695b7..b99b7ecff9 100644 --- a/apps/web/components/task-create-dialog-form-body.test.tsx +++ b/apps/web/components/task-create-dialog-form-body.test.tsx @@ -107,12 +107,14 @@ function makeFs(): DialogFormState { branchesByUrl: { branches: () => [], loading: () => false, + error: () => undefined, ensure: () => undefined, clear: () => undefined, }, prInfoByUrl: { info: () => undefined, loading: () => false, + error: () => undefined, ensure: () => undefined, clear: () => undefined, }, diff --git a/apps/web/components/task-create-dialog-remote-repo-chip-url.test.tsx b/apps/web/components/task-create-dialog-remote-repo-chip-url.test.tsx index a86633d919..58c6d26a4c 100644 --- a/apps/web/components/task-create-dialog-remote-repo-chip-url.test.tsx +++ b/apps/web/components/task-create-dialog-remote-repo-chip-url.test.tsx @@ -49,12 +49,13 @@ function openInput(): HTMLInputElement { } describe("RemoteRepoChip URL entry", () => { - it("writes a GitHub URL with source=paste on Enter", () => { + it("commits a trimmed GitHub URL exactly once on plain Enter", () => { const onURLChange = vi.fn(); renderChip(onURLChange); const input = openInput(); - fireEvent.change(input, { target: { value: "https://github.com/acme/api" } }); + fireEvent.change(input, { target: { value: " https://github.com/acme/api " } }); fireEvent.keyDown(input, { key: "Enter" }); + expect(onURLChange).toHaveBeenCalledTimes(1); expect(onURLChange).toHaveBeenCalledWith("https://github.com/acme/api", "paste"); }); @@ -67,30 +68,55 @@ describe("RemoteRepoChip URL entry", () => { expect(onURLChange).toHaveBeenCalledWith("git@gitlab.com:acme/api.git", "paste"); }); - it("commits a pasted GitHub URL immediately", () => { + it("does not submit URL input during IME composition or modified Enter", () => { + const onURLChange = vi.fn(); + renderChip(onURLChange); + const input = openInput(); + fireEvent.change(input, { target: { value: "https://github.com/acme/api" } }); + fireEvent.keyDown(input, { key: "Enter", isComposing: true }); + fireEvent.keyDown(input, { key: "Enter", ctrlKey: true }); + + expect(onURLChange).not.toHaveBeenCalled(); + }); + + it("keeps a pasted GitHub URL editable until Enter", () => { const onURLChange = vi.fn(); renderChip(onURLChange); const input = openInput(); fireEvent.paste(input, { clipboardData: { getData: () => URL_FOO_BAR_PR } }); - expect(onURLChange).toHaveBeenCalledWith(URL_FOO_BAR_PR, "paste"); + expect(input.value).toBe(URL_FOO_BAR_PR); + expect(onURLChange).not.toHaveBeenCalled(); }); - it("commits a typed GitHub URL on blur", () => { + it("keeps a typed GitHub URL editable on blur", () => { const onURLChange = vi.fn(); renderChip(onURLChange); const input = openInput(); fireEvent.change(input, { target: { value: "https://github.com/foo/bar/issues/42" } }); fireEvent.blur(input); - expect(onURLChange).toHaveBeenCalledWith("https://github.com/foo/bar/issues/42", "paste"); + expect(input.value).toBe("https://github.com/foo/bar/issues/42"); + expect(onURLChange).not.toHaveBeenCalled(); }); - it("commits a typed GitHub PR URL on Tab", () => { + it("keeps a typed GitHub PR URL editable on Tab", () => { const onURLChange = vi.fn(); renderChip(onURLChange); const input = openInput(); fireEvent.change(input, { target: { value: URL_FOO_BAR_PR } }); fireEvent.keyDown(input, { key: "Tab" }); - expect(onURLChange).toHaveBeenCalledWith(URL_FOO_BAR_PR, "paste"); + expect(input.value).toBe(URL_FOO_BAR_PR); + expect(onURLChange).not.toHaveBeenCalled(); + }); + + it("shows the Remote URL Enter hint for URL-shaped input", () => { + const onURLChange = vi.fn(); + renderChip(onURLChange); + const input = openInput(); + fireEvent.change(input, { target: { value: "https://github.com/foo/bar" } }); + + expect(screen.getByText("Remote URL")).toBeTruthy(); + expect(screen.getByText(/press Enter to submit it/i)).toBeTruthy(); + expect(onURLChange).not.toHaveBeenCalled(); }); it("surfaces an inline error for an unsupported provider URL", () => { diff --git a/apps/web/components/task-create-dialog-remote-repo-chip.test.tsx b/apps/web/components/task-create-dialog-remote-repo-chip.test.tsx index e3aa0725b0..9f75173202 100644 --- a/apps/web/components/task-create-dialog-remote-repo-chip.test.tsx +++ b/apps/web/components/task-create-dialog-remote-repo-chip.test.tsx @@ -48,7 +48,7 @@ function remoteTestURL(repo: AccessibleRepo): string { } return `https://${repo.provider}.com/${repo.owner}/${repo.name}`; } -import { RemoteRepoChip } from "./task-create-dialog-remote-repo-chip"; +import { RemoteRepoChip, type RemoteRepoChipProps } from "./task-create-dialog-remote-repo-chip"; const TRIGGER_TID = "remote-repo-chip-trigger"; const INPUT_TID = "remote-repo-input"; @@ -66,18 +66,60 @@ function githubSite(overrides: Partial = {}): AccessibleRepo { ...overrides, }; } -afterEach(() => { - cleanup(); -}); +afterEach(cleanup); function row(overrides: Partial = {}): TaskRemoteRepoRow { return { key: "remote-0", url: "", branch: "", source: "paste", ...overrides }; } function renderInProvider(ui: Parameters[0]) { return render({ui}); } +function renderRemoteRepoChip(overrides: Partial = {}) { + return renderInProvider( + , + ); +} const noopBranch = () => undefined; const noopRemove = () => undefined; describe("RemoteRepoChip — write paths", () => { + it("keeps the committed URL visible and exposes an actionable resolution retry", () => { + const onRetry = vi.fn(); + renderInProvider( + , + ); + + expect(screen.getByTestId("remote-repo-chip").getAttribute("data-remote-url")).toBe( + URL_ACME_SITE, + ); + expect(screen.getByTestId("remote-repo-chip-wrapper").className).toContain("flex-col"); + expect(screen.getByTestId("remote-repo-chip").className).toContain("inline-flex"); + const alert = screen.getByRole("alert"); + expect(alert.className).toContain("max-w-full"); + expect(alert.textContent).toContain("GitHub rate limit exceeded"); + fireEvent.click(screen.getByRole("button", { name: /retry remote repository resolution/i })); + expect(onRetry).toHaveBeenCalledOnce(); + }); +}); + +describe("RemoteRepoChip — selection write paths", () => { it("picker selection writes URL + picker metadata (incl. default_branch) via onURLChange", () => { const accessibleRepos = makeAccessible({ repos: [githubSite({ default_branch: "trunk" })], @@ -322,17 +364,7 @@ describe("RemoteRepoChip — picker focus", () => { }); describe("RemoteRepoChip — branch pill", () => { it("is disabled when the URL is empty", () => { - renderInProvider( - , - ); + renderRemoteRepoChip(); const branchTrigger = screen.getByTestId("remote-branch-chip-trigger") as HTMLButtonElement; expect(branchTrigger.disabled).toBe(true); }); @@ -341,33 +373,16 @@ describe("RemoteRepoChip — branch pill", () => { { name: "main", type: "remote", remote: "origin" }, { name: "develop", type: "remote", remote: "origin" }, ]; - renderInProvider( - , - ); + renderRemoteRepoChip({ row: row({ url: URL_ACME_SITE }), branches }); const branchTrigger = screen.getByTestId("remote-branch-chip-trigger") as HTMLButtonElement; expect(branchTrigger.disabled).toBe(false); }); it("is enabled when the row already has a branch even if branch options haven't loaded yet", () => { - renderInProvider( - , - ); + renderRemoteRepoChip({ + row: row({ url: URL_ACME_SITE, branch: "trunk" }), + branchesLoading: true, + }); const branchTrigger = screen.getByTestId("remote-branch-chip-trigger") as HTMLButtonElement; expect(branchTrigger.disabled).toBe(false); expect(branchTrigger.textContent).toContain("trunk"); @@ -477,18 +492,20 @@ describe("RemoteRepoChip — picker loading state", () => { }); describe("RemoteRepoChip — popover content", () => { + it.each([ + "git@github.com:acme/api.git", + "git@gitlab.com:acme/api.git", + "git@ssh.dev.azure.com:v3/acme/project/api", + ])("shows the Enter hint for supported SCP remote %s", (remoteURL) => { + renderRemoteRepoChip(); + fireEvent.click(screen.getByTestId(TRIGGER_TID)); + fireEvent.change(screen.getByTestId(INPUT_TID), { target: { value: remoteURL } }); + + expect(screen.getByText(/press Enter to submit it/i)).toBeTruthy(); + }); + it("renders one top-level input for both repository search and GitHub URLs", () => { - renderInProvider( - , - ); + renderRemoteRepoChip(); fireEvent.click(screen.getByTestId(TRIGGER_TID)); const input = screen.getByTestId(INPUT_TID) as HTMLInputElement; expect(input.placeholder).toBe("Search repositories or paste a remote URL"); diff --git a/apps/web/components/task-create-dialog-remote-repo-chip.tsx b/apps/web/components/task-create-dialog-remote-repo-chip.tsx index b1509f00f9..5cfc0d6cd3 100644 --- a/apps/web/components/task-create-dialog-remote-repo-chip.tsx +++ b/apps/web/components/task-create-dialog-remote-repo-chip.tsx @@ -13,6 +13,7 @@ import { import { cn } from "@/lib/utils"; import type { Branch } from "@/lib/types/http"; import { Badge } from "@kandev/ui/badge"; +import { Button } from "@kandev/ui/button"; import { Popover, PopoverContent, PopoverTrigger } from "@kandev/ui/popover"; import { Spinner } from "@kandev/ui/spinner"; import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; @@ -47,6 +48,7 @@ export type RemoteRepoChipProps = { branches: Branch[]; branchesLoading: boolean; prInfo?: PRInfo; + resolutionError?: Error; accessibleRepos: UseRemoteRepositoriesResult; /** Identities selected by other rows. Matching entries remain selectable. */ selectedRepositoryIdentities?: string[]; @@ -63,6 +65,7 @@ export type RemoteRepoChipProps = { }, ) => void; onBranchChange: (branch: string) => void; + onRetry?: () => void; onRemove: () => void; }; @@ -81,33 +84,63 @@ export function RemoteRepoChip({ branches, branchesLoading, prInfo, + resolutionError, accessibleRepos, selectedRepositoryIdentities = [], onURLChange, onBranchChange, + onRetry, onRemove, }: RemoteRepoChipProps) { useRowBranchAutoSelect({ row, branches, prInfo, onBranchChange }); return ( - - - - + + + + + + {resolutionError && onRetry ? ( + + ) : null} + + ); +} + +function RemoteResolutionError({ error, onRetry }: { error: Error; onRetry: () => void }) { + return ( + + + Could not resolve remote repository: {error.message} + + ); } @@ -299,7 +332,7 @@ function RemoteRepoPopoverContent({ }, [value, triggerSearch]); const commitURL = (candidate: string) => { const trimmed = candidate.trim(); - if (!parseGitHubAnyUrl(trimmed) && !looksLikeSupportedRemoteURL(trimmed)) { + if (!isSupportedRemoteURL(trimmed)) { if (looksLikeURL(trimmed)) { setUrlError("Enter a GitHub, GitLab, or Azure DevOps repository URL."); } @@ -310,6 +343,7 @@ function RemoteRepoPopoverContent({ return true; }; const visibleUrlError = accessible.unavailable ? null : urlError; + const hasStagedURL = isSupportedRemoteURL(value.trim()); const { showProviderTabs, selectedProvider, visibleRepos } = visibleProviderRepositories( accessible, activeProvider, @@ -323,30 +357,24 @@ function RemoteRepoPopoverContent({ setValue(event.target.value); setUrlError(null); }} - onBlur={(event) => { - const popoverContent = event.currentTarget.closest('[data-slot="popover-content"]'); - if ( - popoverContent && - event.relatedTarget instanceof Node && - popoverContent.contains(event.relatedTarget) - ) { - return; - } - commitURL(value); - }} onPaste={(event) => { const pasted = event.clipboardData.getData("text"); - const isURL = looksLikeURL(pasted.trim()); - if (!commitURL(pasted) && !isURL) return; event.preventDefault(); - setValue(pasted.trim()); + setValue(pasted); }} onKeyDown={(event) => { - if (event.key === "Tab") { - commitURL(value); + if ( + event.key !== "Enter" || + event.defaultPrevented || + event.repeat || + event.altKey || + event.ctrlKey || + event.metaKey || + event.shiftKey || + event.nativeEvent.isComposing || + event.keyCode === 229 + ) return; - } - if (event.key !== "Enter") return; const isURL = looksLikeURL(value.trim()); if (commitURL(value) || isURL) event.preventDefault(); }} @@ -361,6 +389,12 @@ function RemoteRepoPopoverContent({ visibleUrlError && "border-destructive focus:border-destructive", )} /> + {hasStagedURL ? ( +
+ Remote URL — press Enter to submit + it. +
+ ) : null} ({ row, onRemove, onURLChange, + onRetry, selectedRepositoryIdentities = [], }: { row: TaskRemoteRepoRow; onRemove: () => void; onURLChange: ChipURLChange; + onRetry?: () => void; selectedRepositoryIdentities?: string[]; }) => (
({ > paste + {onRetry ? ( + + ) : null}
), })); @@ -90,6 +97,7 @@ function makeBranchesByUrl(ensure = vi.fn()) { return { branches: () => [], loading: () => false, + error: () => undefined, ensure, clear: () => undefined, }; @@ -99,6 +107,7 @@ function makePrInfoByUrl(ensure = vi.fn()) { return { info: () => undefined, loading: () => false, + error: () => undefined, ensure, clear: () => undefined, }; @@ -222,6 +231,31 @@ describe("RemoteRepoChipsRow identity tracking", () => { }); describe("RemoteRepoChipsRow controls", () => { + it("retries both resolution paths for the failed row", () => { + const branchEnsure = vi.fn(); + const branchClear = vi.fn(); + const prEnsure = vi.fn(); + const prClear = vi.fn(); + const fs = makeFs({ + remoteRepos: [{ key: "remote-0", url: URL_AB, branch: "main", source: "paste" }], + branchesByUrl: { + ...makeBranchesByUrl(branchEnsure), + error: () => new Error("GitHub failed"), + clear: branchClear, + }, + prInfoByUrl: { ...makePrInfoByUrl(prEnsure), clear: prClear }, + }); + renderInProvider( + , + ); + + fireEvent.click(screen.getByTestId("remote-chip-retry")); + expect(branchClear).toHaveBeenCalledWith(URL_AB); + expect(prClear).toHaveBeenCalledWith(URL_AB); + expect(branchEnsure).toHaveBeenLastCalledWith(URL_AB, ""); + expect(prEnsure).toHaveBeenLastCalledWith(URL_AB); + }); + it("renders one chip per row in fs.remoteRepos", () => { const fs = makeFs({ remoteRepos: [ diff --git a/apps/web/components/task-create-dialog-remote-repo-chips.tsx b/apps/web/components/task-create-dialog-remote-repo-chips.tsx index c5ce4b6d40..968e4ee3b5 100644 --- a/apps/web/components/task-create-dialog-remote-repo-chips.tsx +++ b/apps/web/components/task-create-dialog-remote-repo-chips.tsx @@ -79,10 +79,12 @@ export function RemoteRepoChipsRow({ branches={fs.branchesByUrl.branches(row.url)} branchesLoading={fs.branchesByUrl.loading(row.url)} prInfo={fs.prInfoByUrl.info(row.url)} + resolutionError={fs.branchesByUrl.error(row.url) ?? fs.prInfoByUrl.error(row.url)} accessibleRepos={accessibleRepos} selectedRepositoryIdentities={selectedRepositoryIdentities} onURLChange={makeURLChange(onUpdateRow, row.key)} onBranchChange={(branch) => onUpdateRow(row.key, { branch })} + onRetry={() => retryRemoteResolution(fs, row.url, workspaceId)} onRemove={() => onRemoveRow(row.key)} /> ); @@ -92,6 +94,13 @@ export function RemoteRepoChipsRow({ ); } +function retryRemoteResolution(fs: DialogFormState, url: string, workspaceId: string): void { + fs.branchesByUrl.clear(url); + fs.prInfoByUrl.clear(url); + fs.branchesByUrl.ensure(url, workspaceId); + fs.prInfoByUrl.ensure(url); +} + /** * Wires a per-row `onURLChange` so paste-mode strips picker metadata while * picker-mode propagates it. Picker-mode also pre-fills `branch` with the diff --git a/apps/web/components/task-create-dialog-repo-chips.test.tsx b/apps/web/components/task-create-dialog-repo-chips.test.tsx index 54aec465dd..91e23183e4 100644 --- a/apps/web/components/task-create-dialog-repo-chips.test.tsx +++ b/apps/web/components/task-create-dialog-repo-chips.test.tsx @@ -46,25 +46,23 @@ function makeRepo(id: string, name: string): Repository { source_type: "local", local_path: `/repos/${name}`, default_branch: "main", - created_at: new Date().toISOString(), - updated_at: new Date().toISOString(), } as Repository; } - function row(overrides: Partial = {}): TaskRepoRow { return { key: `row-${Math.random()}`, branch: "", ...overrides }; } - function makeFs(overrides: Partial): DialogFormState { - const branchesByUrl = { - branches: (url: string) => - url === (overrides.remoteRepos?.[0]?.url ?? "") - ? ((overrides as Partial).branchesByUrl?.branches(url) ?? []) - : [], - loading: () => false, - ensure: () => undefined, - }; return { + branchesByUrl: { + branches: (url: string) => + url === (overrides.remoteRepos?.[0]?.url ?? "") + ? ((overrides as Partial).branchesByUrl?.branches(url) ?? []) + : [], + loading: () => false, + error: () => undefined, + ensure: () => undefined, + clear: () => undefined, + }, repositories: [] as TaskRepoRow[], useRemote: false, discoveredRepositories: [], @@ -74,10 +72,10 @@ function makeFs(overrides: Partial): DialogFormState { removeRemoteRepo: vi.fn(), updateRemoteRepo: vi.fn(), githubUrlError: null, - branchesByUrl, prInfoByUrl: { info: () => undefined, loading: () => false, + error: () => undefined, ensure: () => undefined, clear: () => undefined, }, @@ -90,11 +88,8 @@ function makeFs(overrides: Partial): DialogFormState { } const NOOP = (_key: string, _value: string) => undefined; - -function renderInProvider(ui: Parameters[0]) { - return render({ui}); -} - +const renderInProvider = (ui: Parameters[0]) => + render({ui}); // eslint-disable-next-line max-lines-per-function -- test describe block, splitting hurts readability describe("RepoChipsRow", () => { it("renders one chip per row plus an Add button", () => { diff --git a/apps/web/components/task-create-dialog-submit.test.tsx b/apps/web/components/task-create-dialog-submit.test.tsx index c9bb29a8a9..ae7446c70c 100644 --- a/apps/web/components/task-create-dialog-submit.test.tsx +++ b/apps/web/components/task-create-dialog-submit.test.tsx @@ -132,6 +132,7 @@ function makeDeps(overrides: Partial): SubmitHandlersDeps { prInfoByUrl: { info: () => undefined, loading: () => false, + error: () => undefined, ensure: () => undefined, clear: () => undefined, }, diff --git a/apps/web/components/task-create-dialog.test.tsx b/apps/web/components/task-create-dialog.test.tsx index 76af3b7704..d8a645dfbf 100644 --- a/apps/web/components/task-create-dialog.test.tsx +++ b/apps/web/components/task-create-dialog.test.tsx @@ -275,12 +275,14 @@ function buildMockFs(initialDescription = ORIGINAL_PROMPT): DialogFormState { branchesByUrl: { branches: () => [], loading: () => false, + error: () => undefined, ensure: () => undefined, clear: () => undefined, }, prInfoByUrl: { info: () => undefined, loading: () => false, + error: () => undefined, ensure: () => undefined, clear: () => undefined, }, diff --git a/apps/web/e2e/tests/task/create-task-github-url.spec.ts b/apps/web/e2e/tests/task/create-task-github-url.spec.ts index 40db500048..f96c99a1fd 100644 --- a/apps/web/e2e/tests/task/create-task-github-url.spec.ts +++ b/apps/web/e2e/tests/task/create-task-github-url.spec.ts @@ -9,7 +9,7 @@ useRegularMode(); /** * Helper: switch the create-task dialog to the Remote tab and type a URL - * into the (newly nested) chip popover. Replaces the old "click + * into the (newly nested) chip popover and submit it with Enter. Replaces the old "click * toggle-github-url + fill github-url-input" pair after Task 5/8 swapped the * top-level URL input for a per-row chip + popover. */ @@ -20,7 +20,7 @@ async function openRemoteAndPasteURL(testPage: Page, url: string): Promise const urlInput = testPage.getByTestId("remote-repo-input"); await expect(urlInput).toBeVisible(); await urlInput.fill(url); - await urlInput.press("Tab"); + await urlInput.press("Enter"); } test.describe("Task creation from GitHub URL", () => { diff --git a/apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts b/apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts index af9a59ae89..f4af615480 100644 --- a/apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts +++ b/apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts @@ -28,11 +28,109 @@ async function expectPopoverFitsViewport(testPage: Page): Promise { await expect(input).toHaveCSS("height", "44px"); } +async function expectNoDocumentHorizontalOverflow(testPage: Page): Promise { + await expect + .poll(() => + testPage.evaluate( + () => document.documentElement.scrollWidth <= document.documentElement.clientWidth, + ), + ) + .toBe(true); +} + +async function expectLocatorFitsViewport(testPage: Page, testId: string): Promise { + const viewport = testPage.viewportSize(); + const box = await testPage.getByTestId(testId).boundingBox(); + expect(viewport).not.toBeNull(); + expect(box).not.toBeNull(); + expect(box!.x).toBeGreaterThanOrEqual(0); + expect(box!.x + box!.width).toBeLessThanOrEqual(viewport!.width); + expect(box!.y).toBeGreaterThanOrEqual(0); + expect(box!.y + box!.height).toBeLessThanOrEqual(viewport!.height); +} + test.describe("Create task Remote repo picker on mobile", () => { test.beforeEach(async ({ apiClient }) => { await apiClient.mockGitHubReset(); }); + test("stages a pasted URL until Enter without resolving it", async ({ testPage, apiClient }) => { + const owner = "phone-entry-owner"; + const repo = "phone-entry-repo"; + const url = `https://github.com/${owner}/${repo}`; + const branchPattern = `**/api/v1/github/repos/${owner}/${repo}/branches`; + let branchRequests = 0; + await apiClient.mockGitHubAddBranches(owner, repo, [{ name: "main" }]); + await testPage.route(branchPattern, async (route) => { + branchRequests += 1; + await route.continue(); + }); + + await openRemotePicker(testPage); + const input = testPage.getByTestId("remote-repo-input"); + await input.fill(`${url}-draft`); + await expect(input).toHaveValue(`${url}-draft`); + await expect(testPage.getByText("Remote URL", { exact: true })).toBeVisible(); + await expect(testPage.getByText(/press Enter to submit it/i)).toBeVisible(); + await expectPopoverFitsViewport(testPage); + expect(branchRequests).toBe(0); + + await input.fill(url); + await input.press("Enter"); + + await expect(testPage.getByTestId("remote-repo-chip")).toHaveAttribute("data-remote-url", url); + await expect(testPage.getByTestId("remote-branch-chip-trigger")).toContainText("main"); + await expect.poll(() => branchRequests).toBe(1); + await expectNoDocumentHorizontalOverflow(testPage); + }); + + test("keeps a failed URL row touch-usable and retries its branch resolution", async ({ + testPage, + apiClient, + }) => { + const owner = "phone-retry-owner"; + const repo = "phone-retry-repo"; + const url = `https://github.com/${owner}/${repo}`; + const branchPattern = `**/api/v1/github/repos/${owner}/${repo}/branches`; + let branchRequests = 0; + await apiClient.mockGitHubAddBranches(owner, repo, [{ name: "main" }]); + await testPage.route(branchPattern, async (route) => { + branchRequests += 1; + if (branchRequests === 1) { + await route.fulfill({ + status: 503, + contentType: "application/json", + body: JSON.stringify({ error: "Temporary provider outage" }), + }); + return; + } + await route.continue(); + }); + + await openRemotePicker(testPage); + const input = testPage.getByTestId("remote-repo-input"); + await input.fill(url); + await input.press("Enter"); + + const row = testPage.getByTestId("remote-repo-chip"); + const retry = testPage.getByRole("button", { name: "Retry remote repository resolution" }); + await expect(row).toHaveAttribute("data-remote-url", url); + await expect(testPage.getByRole("alert")).toContainText("Temporary provider outage"); + await expect(retry).toBeVisible(); + await expectLocatorFitsViewport(testPage, "remote-repo-chip-wrapper"); + const retryBox = await retry.boundingBox(); + expect(retryBox).not.toBeNull(); + expect(retryBox!.height).toBeGreaterThanOrEqual(44); + await expectNoDocumentHorizontalOverflow(testPage); + + await retry.tap(); + + await expect.poll(() => branchRequests).toBe(2); + await expect(testPage.getByRole("alert")).not.toBeVisible(); + await expect(testPage.getByTestId("remote-branch-chip-trigger")).toContainText("main"); + await expectNoDocumentHorizontalOverflow(testPage); + }); + test("pastes a GitHub issue URL without clipping the picker", async ({ testPage, apiClient }) => { await apiClient.mockGitHubAddBranches("issue-owner", "issue-repo", [{ name: "main" }]); await apiClient.mockGitHubAddIssues([ diff --git a/apps/web/hooks/domains/github/use-branches-by-url.test.ts b/apps/web/hooks/domains/github/use-branches-by-url.test.ts index ffdfb75740..ce7a902899 100644 --- a/apps/web/hooks/domains/github/use-branches-by-url.test.ts +++ b/apps/web/hooks/domains/github/use-branches-by-url.test.ts @@ -117,7 +117,10 @@ describe("useBranchesByURL provider routing", () => { expect(listProjectBranchesMock).toHaveBeenCalledWith( WORKSPACE_ID, "acme/platform/api", - expect.anything(), + expect.objectContaining({ + expectedHost: "https://gitlab.com", + init: expect.objectContaining({ signal: expect.any(AbortSignal) }), + }), ); expect(listAzureDevOpsBranchesMock).toHaveBeenCalledWith( WORKSPACE_ID, @@ -139,7 +142,10 @@ describe("useBranchesByURL provider routing", () => { expect(listProjectBranchesMock).toHaveBeenCalledWith( WORKSPACE_ID, "acme/platform/api", - expect.anything(), + expect.objectContaining({ + expectedHost: "https://gitlab.internal:8443", + init: expect.objectContaining({ signal: expect.any(AbortSignal) }), + }), ); expect(result.current.branches(gitlab)).toEqual([ expect.objectContaining({ name: "main", type: "remote" }), @@ -209,10 +215,26 @@ describe("useBranchesByURL cache behavior", () => { }); describe("useBranchesByURL — failure & invalidation", () => { - it("retries on the next ensure() after a failed fetch", async () => { - // First fetch rejects — the hook MUST NOT mark the URL as loaded, so a - // subsequent ensure() for the same URL triggers a fresh fetch instead - // of short-circuiting on the cached failure. + it("retains a failure for its URL without losing a successful sibling", async () => { + const failure = new Error("GitHub rate limit exceeded"); + fetchRepoBranchesMock + .mockResolvedValueOnce({ branches: [{ name: "main" }] }) + .mockRejectedValueOnce(failure); + const { result } = renderHook(() => useBranchesByURL()); + + act(() => { + result.current.ensure(REPO_A); + result.current.ensure(REPO_B); + }); + + await waitFor(() => expect(result.current.loading(REPO_B)).toBe(false)); + expect(result.current.branches(REPO_A)).toMatchObject([{ name: "main" }]); + expect( + (result.current as unknown as { error: (url: string) => Error | undefined }).error(REPO_B), + ).toBe(failure); + }); + + it("retries a failed fetch only after clear()", async () => { fetchRepoBranchesMock.mockRejectedValueOnce(new Error("network boom")); const { result } = renderHook(() => useBranchesByURL()); @@ -228,6 +250,12 @@ describe("useBranchesByURL — failure & invalidation", () => { act(() => { result.current.ensure(REPO_A); }); + expect(fetchRepoBranchesMock).toHaveBeenCalledTimes(1); + + act(() => { + result.current.clear(REPO_A); + result.current.ensure(REPO_A); + }); await waitFor(() => expect(result.current.branches(REPO_A)).toHaveLength(1)); expect(fetchRepoBranchesMock).toHaveBeenCalledTimes(2); }); diff --git a/apps/web/hooks/domains/github/use-branches-by-url.ts b/apps/web/hooks/domains/github/use-branches-by-url.ts index d2f2f59b60..f304c43265 100644 --- a/apps/web/hooks/domains/github/use-branches-by-url.ts +++ b/apps/web/hooks/domains/github/use-branches-by-url.ts @@ -32,11 +32,13 @@ import type { Branch } from "@/lib/types/http"; type URLState = { branches: Branch[]; loading: boolean; + error?: Error; }; export type UseBranchesByURLResult = { branches: (url: string) => Branch[]; loading: (url: string) => boolean; + error: (url: string) => Error | undefined; ensure: (url: string, workspaceId?: string) => void; /** * Forget the cached entry for `url` so the next `ensure(url)` re-fetches. @@ -99,15 +101,25 @@ function handleSuccess( setState((prev) => ({ ...prev, [url]: { branches, loading: false } })); } -/** Marks the URL as no longer loading on failure. Does NOT add to loadedRef: - * leaving it unmarked lets the next ensure() call retry instead of - * short-circuiting on the cached failure. */ -function handleFailure(refs: Refs, setState: SetState, url: string, seq: number): void { +/** Records the failure as settled so only an explicit clear() + ensure() retry + * can re-run it. */ +function handleFailure( + refs: Refs, + setState: SetState, + url: string, + seq: number, + error: unknown, +): void { if (!refs.mountedRef.current) return; if (refs.seqRef.current.get(url) !== seq) return; + refs.loadedRef.current.add(url); setState((prev) => ({ ...prev, - [url]: { branches: prev[url]?.branches ?? [], loading: false }, + [url]: { + branches: prev[url]?.branches ?? [], + loading: false, + error: error instanceof Error ? error : new Error("Could not load branches."), + }, })); } @@ -181,7 +193,7 @@ export function useBranchesByURL(): UseBranchesByURLResult { const { seq, signal } = initRequest(refs, setState, url); request(signal) .then((res) => handleSuccess(refs, setState, url, seq, res)) - .catch(() => handleFailure(refs, setState, url, seq)) + .catch((error) => handleFailure(refs, setState, url, seq, error)) .finally(() => finalizeRequest(refs, url, seq)); }, []); @@ -212,8 +224,12 @@ export function useBranchesByURL(): UseBranchesByURLResult { (rawUrl: string): boolean => Boolean(state[rawUrl.trim()]?.loading), [state], ); + const error = useCallback( + (rawUrl: string): Error | undefined => state[rawUrl.trim()]?.error, + [state], + ); - return { branches, loading, ensure, clear }; + return { branches, loading, error, ensure, clear }; } type BranchRequest = (signal: AbortSignal) => Promise<{ branches?: Array<{ name: string }> }>; @@ -253,7 +269,8 @@ function gitLabBranchRequest(parsed: URL, workspaceId: string): BranchRequest | if (!workspaceId) return null; const project = parsed.pathname.replace(/^\//, "").replace(/\.git$/, ""); if (!project.includes("/")) return null; - return (signal) => listProjectBranches(workspaceId, project, { init: { signal } }); + return (signal) => + listProjectBranches(workspaceId, project, { expectedHost: parsed.origin, init: { signal } }); } function azureHTTPSBranchRequest(parsed: URL, workspaceId: string): BranchRequest | null { diff --git a/apps/web/hooks/domains/github/use-pr-info-by-url.test.ts b/apps/web/hooks/domains/github/use-pr-info-by-url.test.ts index 3b4bce3ecf..d681999dd7 100644 --- a/apps/web/hooks/domains/github/use-pr-info-by-url.test.ts +++ b/apps/web/hooks/domains/github/use-pr-info-by-url.test.ts @@ -243,6 +243,45 @@ describe("usePRInfoByURL — non-PR and issue URLs", () => { }); describe("usePRInfoByURL — cache invalidation", () => { + it("retries failed metadata only after clear()", async () => { + fetchPRInfoMock.mockRejectedValueOnce(new Error("GitHub is unavailable")); + const { result } = renderHook(() => usePRInfoByURL()); + + act(() => { + result.current.ensure(PR_URL_A); + }); + await waitFor(() => expect(result.current.loading(PR_URL_A)).toBe(false)); + + fetchPRInfoMock.mockResolvedValueOnce(makePR({ number: 42 })); + act(() => { + result.current.ensure(PR_URL_A); + }); + expect(fetchPRInfoMock).toHaveBeenCalledTimes(1); + + act(() => { + result.current.clear(PR_URL_A); + result.current.ensure(PR_URL_A); + }); + await waitFor(() => expect(result.current.info(PR_URL_A)?.prNumber).toBe(42)); + }); + + it("retains a metadata failure for its URL without losing a successful sibling", async () => { + const failure = new Error("GitHub is unavailable"); + fetchPRInfoMock.mockResolvedValueOnce(makePR({ number: 42 })).mockRejectedValueOnce(failure); + const { result } = renderHook(() => usePRInfoByURL()); + + act(() => { + result.current.ensure(PR_URL_A); + result.current.ensure(PR_URL_B); + }); + + await waitFor(() => expect(result.current.loading(PR_URL_B)).toBe(false)); + expect(result.current.info(PR_URL_A)?.prNumber).toBe(42); + expect( + (result.current as unknown as { error: (url: string) => Error | undefined }).error(PR_URL_B), + ).toBe(failure); + }); + it("does not re-fetch when ensure() is called for an already-loaded URL", async () => { fetchPRInfoMock.mockResolvedValue(makePR({ number: 42 })); @@ -286,7 +325,9 @@ describe("usePRInfoByURL — cache invalidation", () => { expect(result.current.info("https://github.com/who/what/pull/1")).toBeUndefined(); expect(result.current.loading("https://github.com/who/what/pull/1")).toBe(false); }); +}); +describe("usePRInfoByURL — stale callback protection", () => { it("ignores a stale fetch resolved after clear() + ensure() restarted the request", async () => { // Simulates the race the per-URL sequence counter guards: the first // fetch is still in flight when the caller clear()s and re-ensure()s; diff --git a/apps/web/hooks/domains/github/use-pr-info-by-url.ts b/apps/web/hooks/domains/github/use-pr-info-by-url.ts index beae798148..21648c4bc7 100644 --- a/apps/web/hooks/domains/github/use-pr-info-by-url.ts +++ b/apps/web/hooks/domains/github/use-pr-info-by-url.ts @@ -37,12 +37,14 @@ export type PRInfo = { type URLState = { info: PRInfo | undefined; loading: boolean; + error?: Error; }; export type UsePRInfoByURLResult = { ensure: (url: string) => void; info: (url: string) => PRInfo | undefined; loading: (url: string) => boolean; + error: (url: string) => Error | undefined; clear: (url: string) => void; }; @@ -140,13 +142,23 @@ function handleSuccess(args: SuccessArgs): void { /** Marks loaded on failure (we don't want to retry in a tight loop) and * clears the loading flag. Callers that want to retry can clear() + ensure(). */ -function handleFailure(refs: Refs, setState: SetState, url: string, seq: number): void { +function handleFailure( + refs: Refs, + setState: SetState, + url: string, + seq: number, + error: unknown, +): void { if (!refs.mountedRef.current) return; if (refs.seqRef.current.get(url) !== seq) return; refs.loadedRef.current.add(url); setState((prev) => ({ ...prev, - [url]: { info: prev[url]?.info, loading: false }, + [url]: { + info: prev[url]?.info, + loading: false, + error: error instanceof Error ? error : new Error("Could not load GitHub metadata."), + }, })); } @@ -204,7 +216,7 @@ function runGitHubInfoRequest(args: { return; } request - .catch(() => handleFailure(refs, setState, url, seq)) + .catch((error) => handleFailure(refs, setState, url, seq, error)) .finally(() => finalizeRequest(refs, url, seq)); } @@ -272,6 +284,10 @@ export function usePRInfoByURL(): UsePRInfoByURLResult { (rawUrl: string): boolean => Boolean(state[rawUrl.trim()]?.loading), [state], ); + const error = useCallback( + (rawUrl: string): Error | undefined => state[rawUrl.trim()]?.error, + [state], + ); const clear = useCallback((rawUrl: string) => { const url = rawUrl.trim(); if (!url) return; @@ -293,5 +309,5 @@ export function usePRInfoByURL(): UsePRInfoByURLResult { }); }, []); - return { ensure, info, loading, clear }; + return { ensure, info, loading, error, clear }; } diff --git a/apps/web/lib/api/domains/gitlab-api.test.ts b/apps/web/lib/api/domains/gitlab-api.test.ts index 69d027bca4..e2d725846e 100644 --- a/apps/web/lib/api/domains/gitlab-api.test.ts +++ b/apps/web/lib/api/domains/gitlab-api.test.ts @@ -308,6 +308,21 @@ describe("gitlab-api — workspace watch actions", () => { ); } }); + + it("encodes public and self-managed expected hosts when listing project branches", async () => { + const expectedHosts = ["https://gitlab.com", SELF_MANAGED_HOST]; + await Promise.all( + expectedHosts.map((expectedHost) => + listProjectBranches(WORKSPACE_WITH_SPACE, PROJECT_PATH, { expectedHost }), + ), + ); + + expect( + fetchSpy.mock.calls.map((call) => + new URL(call[0] as string).searchParams.get("expected_host"), + ), + ).toEqual(expectedHosts); + }); }); describe("gitlab-api — merge request review", () => { diff --git a/apps/web/lib/api/domains/gitlab-api.ts b/apps/web/lib/api/domains/gitlab-api.ts index 0a9fe435c2..920bd8d6fe 100644 --- a/apps/web/lib/api/domains/gitlab-api.ts +++ b/apps/web/lib/api/domains/gitlab-api.ts @@ -457,13 +457,16 @@ export async function searchProjects(workspaceId: string, query: string) { ); } +type GitLabProjectBranchesOptions = ApiRequestOptions & { expectedHost?: string }; + export async function listProjectBranches( workspaceId: string, project: string, - options?: ApiRequestOptions, + options?: GitLabProjectBranchesOptions, ) { const qs = new URLSearchParams({ workspace_id: workspaceId }); qs.set("project", project); + if (options?.expectedHost) qs.set("expected_host", options.expectedHost); return fetchJson<{ branches: GitLabRepoBranch[] }>( `/api/v1/gitlab/projects/branches?${qs.toString()}`, options, diff --git a/docs/plans/remote-repository-entry-reliability/plan.md b/docs/plans/remote-repository-entry-reliability/plan.md new file mode 100644 index 0000000000..f7cec31edc --- /dev/null +++ b/docs/plans/remote-repository-entry-reliability/plan.md @@ -0,0 +1,94 @@ +--- +spec: docs/specs/tasks/multi-branch/spec.md +related_specs: + - docs/specs/gitlab-integration/spec.md +created: 2026-07-24 +status: completed +--- + +# Implementation Plan: Remote Repository Entry Reliability + +## Overview + +Make Remote task creation an explicit submit flow: URL-shaped input remains editable until Enter, then repository resolution begins. Preserve the committed URL when provider lookups fail, surface the exact failure with a retry action, and ensure preview environments can resolve public GitHub/GitLab resources without configured credentials. + +## Backend + +### Public GitHub task-creation metadata + +- `apps/backend/internal/github/service_pr.go`: let `GetPR` and `GetIssue` fall back only from an absent/no-op client to an unauthenticated public GitHub REST read. Authenticated-client failures remain authoritative. +- `apps/backend/internal/github/service_reviews.go` or a focused sibling file: reuse the existing anonymous GitHub request rules and error/status mapping for public branches, PRs, and issues. +- `apps/backend/internal/github/service_test.go`: prove public PR, issue, and branch reads work through the no-client path and preserve 404/403 errors. + +### Public GitLab branch discovery + +- `apps/backend/internal/gitlab/controller_watches.go`: keep workspace-scoped branch listing, but use an anonymous `gitlab.com` client only when that workspace has no configured GitLab connection and the request targets the public host. +- `apps/backend/internal/gitlab/pat_client.go`: omit `PRIVATE-TOKEN` when the client has no token so public REST reads are truly anonymous. +- `apps/backend/internal/gitlab/controller_test.go` and `pat_client_test.go`: prove unconfigured public branch lookup succeeds, private/not-found errors remain visible, and no other integration route gains anonymous behavior. + +## Frontend + +### Explicit URL submission + +- `apps/web/components/task-create-dialog-remote-repo-chip.tsx`: remove commit-on-paste, blur, and Tab. Keep the staged value in the input, submit only on plain Enter, and render a visible `Remote URL` / Enter hint when the staged text is URL-shaped. +- Preserve picker-option selection as an immediate explicit action. +- Clear validation errors while the user edits and keep unsupported-provider validation on Enter. + +### Resolution errors and retry + +- `apps/web/hooks/domains/github/use-branches-by-url.ts`: retain the request error per normalized URL and expose it through the hook result. +- `apps/web/hooks/domains/github/use-pr-info-by-url.ts`: retain PR/issue metadata request errors per normalized URL instead of swallowing them. +- `apps/web/components/task-create-dialog-remote-repo-chips.tsx`: pass per-row resolution state and a retry callback that clears and re-runs both lookups. +- `apps/web/components/task-create-dialog-remote-repo-chip.tsx`: render an accessible inline error/retry state attached to the committed repository row while preserving the URL and any successfully resolved branch. + +### Mobile design contract + +- Desktop outcome and mobile entry point: the existing Remote repository chip opens the same picker; Enter is the explicit keyboard submit action, while provider options remain touch-selectable. +- Nearest shipped exemplar: the existing Remote repository popover and its 44px mobile input/option rows remain the geometry and interaction baseline. +- Information hierarchy and surface: the popover remains the single scroll owner. The URL/search field is primary, the `Remote URL` hint sits directly below it, and the row-level error places Retry beside the failed repository without introducing another overlay. +- Shared behavior: staged input, URL classification, error state, and retry are viewport-independent. +- Touch and containment: Retry is a visible touch-sized control; the popover remains viewport-contained with no document horizontal overflow. + +## Tests + +- **What:** paste, blur, and Tab do not commit; Enter commits once; URL-shaped input shows the hint; unsupported URLs validate on Enter. **File:** `apps/web/components/task-create-dialog-remote-repo-chip-url.test.tsx`. **How:** component tests against the real popover input. +- **What:** branch failures retain their error and clear after explicit retry. **File:** `apps/web/hooks/domains/github/use-branches-by-url.test.ts`. **How:** hook tests with first-call rejection and second-call success. +- **What:** PR and issue failures retain errors and retry without stale callbacks overwriting the successful result. **File:** `apps/web/hooks/domains/github/use-pr-info-by-url.test.ts`. **How:** hook tests with controlled promises. +- **What:** the repository row preserves its URL, announces the error, and invokes retry. **Files:** `apps/web/components/task-create-dialog-remote-repo-chip.test.tsx` and `task-create-dialog-remote-repo-chips.test.tsx`. **How:** component tests with per-row failure fixtures. +- **What:** unconfigured public GitHub PR/issue and GitLab branch reads succeed while upstream errors keep their status. **Files:** backend service/controller tests named above. **How:** `httptest` provider servers through the real service/controller path. + +## E2E Tests + +- **Scenario:** GIVEN a supported URL on phone, WHEN it is pasted and edited before Enter, THEN no repository resolution request fires, the `Remote URL` hint is visible, and Enter commits the final URL. **File:** `apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts`. +- **Scenario:** GIVEN branch resolution fails, WHEN the user sees the preserved row and taps Retry after the transient failure clears, THEN the branch appears and task creation becomes available. **File:** `apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts`. +- **Scenario:** GIVEN no GitHub credentials in a preview-like backend, WHEN a public GitHub repository/PR/issue is entered, THEN its branches and public metadata resolve. **File:** focused backend integration coverage; the browser E2E remains mocked to avoid external-network flake. + +## Implementation Waves + +Wave 1: + +- [x] [task-01-public-provider-resolution](task-01-public-provider-resolution.md) — backend public GitHub/GitLab resolution + +Wave 2: + +- [x] [task-02-explicit-url-entry](task-02-explicit-url-entry.md) — staged input and Enter hint +- [x] [task-03-resolution-errors-and-retry](task-03-resolution-errors-and-retry.md) — hook errors and row retry UI + +Wave 3: + +- [x] [task-04-mobile-e2e](task-04-mobile-e2e.md) — phone interaction and retry coverage + +## Verification + +- `cd apps/backend && go test -run 'Test(ListRepoBranches|GetPR|GetIssue|HttpListProjectBranches|PATClient)' ./internal/github ./internal/gitlab` +- `cd apps && pnpm --filter @kandev/web test -- components/task-create-dialog-remote-repo-chip-url.test.tsx components/task-create-dialog-remote-repo-chip.test.tsx components/task-create-dialog-remote-repo-chips.test.tsx hooks/domains/github/use-branches-by-url.test.ts hooks/domains/github/use-pr-info-by-url.test.ts` +- `make build-backend build-web` +- `cd apps && pnpm --filter @kandev/web e2e -- e2e/tests/task/mobile-create-task-remote-repo.spec.ts --project=mobile-chrome` +- Post-commit change-aware `/verify`. + +## Risks + +- Anonymous provider APIs are rate-limited. Failures must remain visible and retryable; authenticated workspace connections still take precedence. +- Public fallbacks must activate only for absent credentials and explicitly named public-host resources, never as an authorization bypass after an authenticated request fails. +- A PR metadata retry and branch retry can settle in either order. Per-URL sequence guards must prevent stale results from clearing newer success/error state. +- Enter handling must not break selecting a provider repository option or IME composition. diff --git a/docs/plans/remote-repository-entry-reliability/task-01-public-provider-resolution.md b/docs/plans/remote-repository-entry-reliability/task-01-public-provider-resolution.md new file mode 100644 index 0000000000..2c406cb035 --- /dev/null +++ b/docs/plans/remote-repository-entry-reliability/task-01-public-provider-resolution.md @@ -0,0 +1,51 @@ +--- +id: "01-public-provider-resolution" +title: "Public provider resolution" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/tasks/multi-branch/spec.md" +--- + +# Task 01: Public provider resolution + +## Acceptance + +- Public GitHub repository branches plus PR/issue metadata resolve without a configured GitHub client. +- Public `gitlab.com` branches resolve for an unconfigured workspace without enabling anonymous browse/write routes. +- Authenticated-client and upstream 404/403 failures remain authoritative and preserve their HTTP status. + +## Verification + +- `cd apps/backend && go test -run 'Test(ListRepoBranches|GetPR|GetIssue|HttpListProjectBranches|PATClient)' ./internal/github ./internal/gitlab` + +## Files likely touched + +- `apps/backend/internal/github/service_pr.go` +- `apps/backend/internal/github/service_reviews.go` or a focused anonymous-read sibling +- `apps/backend/internal/github/service_test.go` +- `apps/backend/internal/gitlab/controller_watches.go` +- `apps/backend/internal/gitlab/pat_client.go` +- `apps/backend/internal/gitlab/controller_test.go` +- `apps/backend/internal/gitlab/pat_client_test.go` + +## Dependencies + +None. + +## Inputs + +- `docs/specs/tasks/multi-branch/spec.md` — Remote task-creation scenarios. +- `docs/specs/gitlab-integration/spec.md` — task-creation anonymous exception. +- Existing `listRepoBranchesAnonymous` GitHub implementation and GitLab workspace-client resolution. + +## Output contract + +Report the changed entry points, exact tests/results, risk tags, and uncertainties; update this task to `done` only after targeted verification passes. + +## Completion evidence + +- **Entry points:** GitHub anonymous branch, PR, and issue reads; GitLab public `gitlab.com` branch resolution. +- **Result:** backend focused verification passed: 75 tests. +- **Risks/uncertainties:** anonymous access remains limited to public read paths; authenticated clients and upstream 403/404 responses remain authoritative. diff --git a/docs/plans/remote-repository-entry-reliability/task-02-explicit-url-entry.md b/docs/plans/remote-repository-entry-reliability/task-02-explicit-url-entry.md new file mode 100644 index 0000000000..e2f94e3acf --- /dev/null +++ b/docs/plans/remote-repository-entry-reliability/task-02-explicit-url-entry.md @@ -0,0 +1,45 @@ +--- +id: "02-explicit-url-entry" +title: "Explicit remote URL entry" +status: done +wave: 2 +depends_on: ["01-public-provider-resolution"] +plan: "plan.md" +spec: "../../specs/tasks/multi-branch/spec.md" +--- + +# Task 02: Explicit remote URL entry + +## Acceptance + +- Paste, blur, and Tab leave a supported URL editable and do not call `onURLChange`. +- Plain Enter commits the trimmed supported URL exactly once. +- URL-shaped input displays a visible `Remote URL` hint that explains Enter submits it. + +## Verification + +- `cd apps && pnpm --filter @kandev/web test -- components/task-create-dialog-remote-repo-chip-url.test.tsx` + +## Files likely touched + +- `apps/web/components/task-create-dialog-remote-repo-chip.tsx` +- `apps/web/components/task-create-dialog-remote-repo-chip-url.test.tsx` + +## Dependencies + +- Task 01 establishes the public resolution behavior started after Enter. + +## Inputs + +- `docs/specs/tasks/multi-branch/spec.md` — explicit-submit scenarios. +- Existing Remote popover input and mobile geometry. + +## Output contract + +Report the changed interaction, exact tests/results, risk tags, and uncertainties; update this task to `done` only after targeted verification passes. + +## Completion + +- Implemented staged remote-URL input: paste, blur, and Tab preserve editable text; only plain Enter commits a trimmed supported URL. +- Added a visible `Remote URL` hint explaining Enter submission and retained picker-option immediate selection plus unsupported-provider validation. +- Verified with `cd apps && pnpm --filter @kandev/web test -- components/task-create-dialog-remote-repo-chip-url.test.tsx` (8 passing tests). diff --git a/docs/plans/remote-repository-entry-reliability/task-03-resolution-errors-and-retry.md b/docs/plans/remote-repository-entry-reliability/task-03-resolution-errors-and-retry.md new file mode 100644 index 0000000000..ae06e0524b --- /dev/null +++ b/docs/plans/remote-repository-entry-reliability/task-03-resolution-errors-and-retry.md @@ -0,0 +1,50 @@ +--- +id: "03-resolution-errors-and-retry" +title: "Remote resolution errors and retry" +status: done +wave: 2 +depends_on: ["01-public-provider-resolution"] +plan: "plan.md" +spec: "../../specs/tasks/multi-branch/spec.md" +--- + +# Task 03: Remote resolution errors and retry + +## Acceptance + +- Branch and GitHub PR/issue loaders retain per-URL errors without losing successful sibling data. +- A committed repository row shows an accessible error and visible Retry action while preserving its URL. +- Retry clears and re-runs both resolution paths; stale callbacks cannot overwrite the newer result. + +## Verification + +- `cd apps && pnpm --filter @kandev/web test -- hooks/domains/github/use-branches-by-url.test.ts hooks/domains/github/use-pr-info-by-url.test.ts components/task-create-dialog-remote-repo-chip.test.tsx components/task-create-dialog-remote-repo-chips.test.tsx` + +## Files likely touched + +- `apps/web/hooks/domains/github/use-branches-by-url.ts` +- `apps/web/hooks/domains/github/use-branches-by-url.test.ts` +- `apps/web/hooks/domains/github/use-pr-info-by-url.ts` +- `apps/web/hooks/domains/github/use-pr-info-by-url.test.ts` +- `apps/web/components/task-create-dialog-remote-repo-chips.tsx` +- `apps/web/components/task-create-dialog-remote-repo-chip.tsx` +- Corresponding component tests. + +## Dependencies + +- Task 01 supplies the public unauthenticated success path. + +## Inputs + +- `docs/specs/tasks/multi-branch/spec.md` — failure and retry scenarios. +- Existing per-URL abort and sequence guards in both hooks. + +## Output contract + +Report the changed state contract, exact tests/results, risk tags, and uncertainties; update this task to `done` only after targeted verification passes. + +## Completion evidence + +- **Entry points:** per-URL branch and GitHub PR/issue loaders; remote repository chip error and retry UI. +- **Result:** frontend focused verification passed: 67 tests; post-simplification hook suite passed: 33 tests. +- **Risks/uncertainties:** stale callbacks are sequence-guarded; provider failures can still persist until the user retries or corrects access. diff --git a/docs/plans/remote-repository-entry-reliability/task-04-mobile-e2e.md b/docs/plans/remote-repository-entry-reliability/task-04-mobile-e2e.md new file mode 100644 index 0000000000..3045349fab --- /dev/null +++ b/docs/plans/remote-repository-entry-reliability/task-04-mobile-e2e.md @@ -0,0 +1,45 @@ +--- +id: "04-mobile-e2e" +title: "Mobile remote entry coverage" +status: done +wave: 3 +depends_on: ["02-explicit-url-entry", "03-resolution-errors-and-retry"] +plan: "plan.md" +spec: "../../specs/tasks/multi-branch/spec.md" +--- + +# Task 04: Mobile remote entry coverage + +## Acceptance + +- Phone E2E proves pasted URL text remains editable until Enter and the hint is visible. +- Phone E2E proves a transient resolution error preserves the row and Retry completes branch selection. +- The popover/error controls remain viewport-contained, touch-usable, and introduce no document horizontal overflow. + +## Verification + +- `make build-backend build-web` +- `cd apps && pnpm --filter @kandev/web e2e -- e2e/tests/task/mobile-create-task-remote-repo.spec.ts --project=mobile-chrome` + +## Files likely touched + +- `apps/web/e2e/tests/task/mobile-create-task-remote-repo.spec.ts` + +## Dependencies + +- Tasks 02 and 03. + +## Inputs + +- Mobile design contract in `plan.md`. +- Existing `openRemotePicker` and viewport-containment helpers in the target spec. + +## Output contract + +Report the exact E2E result, screenshots/failure artifacts if any, risk tags, and uncertainties; update this task to `done` only after targeted verification passes. + +## Completion evidence + +- **Entry point:** `mobile-create-task-remote-repo.spec.ts` on the `mobile-chrome` project. +- **Result:** mobile E2E passed: 5 tests, with no failure artifacts; fresh PR screenshots exist under ignored `.pr-assets`. +- **Risks/uncertainties:** coverage proves the current Pixel 5 flow and viewport containment; provider responses remain externally dependent. diff --git a/docs/public/tasks-and-workflows.md b/docs/public/tasks-and-workflows.md index 9822148bc6..20b6eb467c 100644 --- a/docs/public/tasks-and-workflows.md +++ b/docs/public/tasks-and-workflows.md @@ -55,7 +55,7 @@ Use **New Task** in the sidebar. In an open task, the **Task** split button also | Source | Use it for | Important behavior | |---|---|---| | **Repo** | A configured, discovered, or new local repository | Select a base branch for each repository row. For a single-row new task, **Create new repository** initializes an empty `main` repository in a parent folder you choose. Add more rows for a multi-repository task. | - | **Remote** | A remote repository | Search configured GitHub, GitLab, or Azure DevOps repositories, or paste a supported URL; then select the branch. Clone and fetch require valid credentials. | + | **Remote** | A remote repository | Search configured GitHub, GitLab, or Azure DevOps repositories, or paste a supported URL. A pasted URL stays editable until you press Enter; then select the branch. Anonymous, credential-free reads include public GitHub repository branches, pull requests, and issues, plus public `gitlab.com` branch discovery. Private resources and authenticated browse/write features require valid provider credentials. | | **None** | Planning, research, or work outside Git | Use a scratch workspace or an optional folder on the Kandev host. Git worktree execution and repository-aware Changes, branch, and pull-request features are unavailable. | 4. Select a compatible executor profile and agent profile. A workflow default agent profile locks the task-level agent selector. Executor and agent compatibility is validated before launch. @@ -84,7 +84,9 @@ Select an option, then choose **Save changes**. The only affected Kandev MCP too ### Multiple repositories -A task can include several local or remote repository rows. Multi-repository task creation currently requires the **git-worktree** executor; the dialog leaves incompatible executors visible but disables them with `Multi-repo tasks only support the git-worktree executor.` Each remote needs credentials that can clone and fetch its selected base branch. +A task can include several local or remote repository rows. Multi-repository task creation currently requires the **git-worktree** executor; the dialog leaves incompatible executors visible but disables them with `Multi-repo tasks only support the git-worktree executor.` Public GitHub and GitLab repositories can be cloned and fetched anonymously. Private repositories and authenticated browse/write features need credentials that can access the selected base branch. + +If Kandev cannot resolve a pasted remote URL or its branch, the repository row keeps the URL and shows the provider error. Use **Retry** after correcting the URL or when a transient provider failure has cleared. Changes and review are scoped by repository. State the expected deliverable, base branch, and pull-request target for every attachment. See [Coordinate work](coordination.md) for adding branches after creation and splitting multi-repository work. diff --git a/docs/specs/gitlab-integration/spec.md b/docs/specs/gitlab-integration/spec.md index 6c651ffc99..aaaac95baf 100644 --- a/docs/specs/gitlab-integration/spec.md +++ b/docs/specs/gitlab-integration/spec.md @@ -31,6 +31,10 @@ workflows are not usable end to end. - GitLab browse, task-link, review, watch, and write endpoints require an authoritative `workspace_id` and resolve that workspace's connection. Data or credentials from another workspace are never used as fallback. +- Task creation is the narrow unauthenticated exception: branch discovery for + an explicitly entered public `gitlab.com` repository URL works without a + saved workspace connection. It does not expose private projects, browse + results, merge requests, issues, or write actions. - GitLab repository matching uses provider, normalized provider host, and full subgroup project path. Repositories with unknown or mismatched provider hosts are not eligible for GitLab linking or merge-request actions. @@ -165,6 +169,10 @@ validated against any supplied value. ### Browse and review +- `GET /projects/branches?workspace_id=&project=` uses the workspace + connection when configured. When the workspace is unconfigured and the + requested host is `gitlab.com`, it may list branches anonymously for the + explicitly named public project so Remote task creation can continue. - `GET /user/mrs?workspace_id=&filter=&page=&per_page=` and `GET /user/issues?workspace_id=&filter=&page=&per_page=` return the active workspace's paginated search results. @@ -228,7 +236,9 @@ protocol action name for compatibility. - `409` indicates a workspace/resource invariant conflict that cannot be applied idempotently. - `422` indicates GitLab rejected a valid write, such as an ineligible reviewer. -- `503` indicates the workspace connection is absent or currently unavailable. +- `503` indicates the workspace connection is absent or currently unavailable, + except for anonymous branch discovery of an explicitly named public + `gitlab.com` project. - Provider error bodies and logs are sanitized and never include tokens or authenticated remote URLs. @@ -323,6 +333,10 @@ protocol action name for compatibility. - **GIVEN** two workspaces connected to different GitLab hosts, **WHEN** each opens its GitLab page, **THEN** each sees only data fetched with its own host and credential. +- **GIVEN** a workspace without a GitLab connection, **WHEN** the user enters a + public `gitlab.com` repository URL in Remote task creation, **THEN** Kandev + lists its branches anonymously without making any other GitLab browse or + write capability available. - **GIVEN** a legacy global GitLab host and token, **WHEN** Kandev starts after upgrade, **THEN** one deterministic workspace receives the config and secret and other workspaces remain unconfigured. diff --git a/docs/specs/tasks/multi-branch/spec.md b/docs/specs/tasks/multi-branch/spec.md index c68d220159..f976e60522 100644 --- a/docs/specs/tasks/multi-branch/spec.md +++ b/docs/specs/tasks/multi-branch/spec.md @@ -50,6 +50,9 @@ Workarounds (sibling tasks, manually managing two worktrees) lost shared context - Worktrees are keyed by `worktree.id` in the Zustand store, so two worktrees with the same `repository_id` already coexist. - Repo chips in chat-message renderers now key on `(repository_id, checkout_branch)` so multi-branch tasks render distinct chips instead of collapsing. - In the task-creation dialog, every workspace, discovered-on-disk, and remote provider repository picker shows an accent-colored check (with an accessible `Already added` label) when another repository row already selects that repository. The current row does not mark its own selection. +- Typing or pasting a supported remote URL stages it in the Remote repository picker. Kandev does not resolve or commit the URL until the user presses Enter, so the user can finish editing it first. A visible `Remote URL` hint identifies URL-shaped input and tells the user to press Enter. +- A committed remote URL can resolve repository branches and GitHub PR/issue metadata without an authenticated provider connection when the upstream resource is public. Private-resource access still requires the matching configured integration. +- Remote URL resolution failures remain attached to their repository row. The row shows the provider error and an explicit retry action; retry repeats both branch and PR/issue metadata lookups without requiring the user to delete and re-enter the URL. - Marked options remain selectable so users can intentionally create a multi-branch task from one repository. Removing or changing the other row removes the marker immediately. - Repository-chip tooltips keep long local paths inside a viewport-safe, wrapping surface. Closing a repository picker does not reveal its tooltip until the pointer leaves and deliberately hovers the chip again. - Review surfaces expose one linked pull request at a time when a task has multiple PRs. A task-scoped selector defaults to the primary (oldest) PR, remembers an in-session override, and falls back to the primary PR when that override disappears. @@ -63,6 +66,12 @@ Workarounds (sibling tasks, manually managing two worktrees) lost shared context - **GIVEN** a task-creation Remote row selects a provider-backed repository, **WHEN** the user opens another Remote repository selector, **THEN** the same provider repository remains selectable and is visibly marked by the same compact accent-colored check. - **GIVEN** a repository is marked because another row selects it, **WHEN** the user changes or removes that other row, **THEN** the marker disappears from the open or next-opened selector. - **GIVEN** a row already selects a repository and no other row selects it, **WHEN** the user reopens that row's selector, **THEN** its current repository is not marked as a duplicate. +- **GIVEN** the Remote repository input contains a supported URL, **WHEN** the user pastes, edits, blurs, or tabs away without pressing Enter, **THEN** Kandev keeps the text editable and does not start repository resolution. +- **GIVEN** the Remote repository input contains a supported URL, **WHEN** the user presses Enter, **THEN** Kandev commits the trimmed URL, closes the picker, and begins branch plus applicable PR/issue metadata resolution. +- **GIVEN** the Remote repository input contains URL-shaped text, **WHEN** the picker is open, **THEN** a visible `Remote URL` hint explains that Enter submits it. +- **GIVEN** branch or GitHub PR/issue metadata resolution fails, **WHEN** the repository row renders, **THEN** it shows an actionable error and retry control while preserving the committed URL. +- **GIVEN** a failed remote repository resolution, **WHEN** the user retries and the provider responds successfully, **THEN** the error clears and the branch/metadata selection completes without re-entering the URL. +- **GIVEN** no GitHub or GitLab integration is configured, **WHEN** the user submits a public `github.com` or `gitlab.com` repository URL, **THEN** Kandev can discover its branches and create the task; private repositories continue to require credentials. - **GIVEN** a selected repository has a long unbroken local path, **WHEN** its tooltip opens, **THEN** the tooltip wraps the path and remains within the viewport instead of covering adjacent repository controls. - **GIVEN** a user selects a repository from a repository-chip picker, **WHEN** the picker closes while the pointer remains over the chip, **THEN** no repository tooltip opens until the pointer leaves and deliberately hovers the chip again. @@ -90,6 +99,8 @@ Workarounds (sibling tasks, manually managing two worktrees) lost shared context - `TestAddBranchToTask_RejectsDuplicate` — re-adding the same `(repo, branch)` errors. - `TestLaunchPreparedSession_MultiBranch_ReusesWorktreeIDsByBranchSlug` — a follow-on session for the same task reuses each existing branch worktree instead of preparing a new task directory. - Task-creation component and mobile E2E coverage prove the accessible, compact selected-repository marker for workspace/on-disk and Remote provider selectors while preserving option selection, viewport-safe long-path tooltips, and post-selection tooltip suppression. +- Remote-entry component and mobile E2E coverage prove paste/typing remains editable until Enter, the `Remote URL` hint is visible, failures preserve the URL and expose retry, and retry completes resolution. +- GitHub and GitLab backend tests prove public branch lookup works without configured credentials, while public GitHub PR/issue metadata lookup supplies the task-creation enrichment used by the Remote selector. - Web unit tests prove selected-PR default, override, task isolation, and removed-PR fallback behavior. - Desktop and mobile Playwright tests prove a two-PR task can switch Review from the primary PR to a sibling PR without stale files, overflow, or closing the surface.