From 365a3bdd249b9855045bd9bdb1d26ae3bba30bf4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Thu, 25 Jun 2026 14:13:39 +0100 Subject: [PATCH] fix: recreate github review tasks after reset (#1492) --- apps/backend/internal/github/controller.go | 12 +- .../internal/github/controller_test.go | 240 ++++++++++++++++++ .../internal/github/controller_watch_reset.go | 6 +- apps/backend/internal/github/handlers.go | 14 +- .../internal/github/service_reviews.go | 23 +- apps/backend/internal/github/service_test.go | 15 +- .../internal/github/service_watch_reset.go | 30 ++- .../web/components/github/github-settings.tsx | 9 +- apps/web/e2e/helpers/api-client.ts | 12 +- .../integrations/github-watch-reset.spec.ts | 58 +++-- .../pr/pr-watcher-cleanup-policy.spec.ts | 18 +- .../pr/pr-watcher-merged-cleanup.spec.ts | 12 +- .../domains/github/use-review-watches.ts | 26 +- apps/web/lib/api/domains/github-api.ts | 13 +- 14 files changed, 403 insertions(+), 85 deletions(-) diff --git a/apps/backend/internal/github/controller.go b/apps/backend/internal/github/controller.go index cf49347e56..d56ed22aac 100644 --- a/apps/backend/internal/github/controller.go +++ b/apps/backend/internal/github/controller.go @@ -639,6 +639,9 @@ func (c *Controller) httpDeleteReviewWatch(ctx *gin.Context) { func (c *Controller) httpTriggerReviewWatch(ctx *gin.Context) { id := ctx.Param("id") + if !c.requireReviewWatchInWorkspace(ctx, id) { + return + } watch, err := c.service.GetReviewWatch(ctx.Request.Context(), id) if err != nil { ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) @@ -648,14 +651,19 @@ func (c *Controller) httpTriggerReviewWatch(ctx *gin.Context) { ctx.JSON(http.StatusNotFound, gin.H{"error": "review watch not found"}) return } - newPRs, err := c.service.CheckReviewWatch(ctx.Request.Context(), watch) + newPRs, err := c.service.TriggerReviewWatch(ctx.Request.Context(), watch) if err != nil { ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } // Clean up tasks for merged/closed PRs that haven't been started. cleaned, _ := c.service.CleanupMergedReviewTasks(ctx.Request.Context(), watch) - ctx.JSON(http.StatusOK, gin.H{"new_prs": len(newPRs), "prs": newPRs, "cleaned": cleaned}) + ctx.JSON(http.StatusOK, gin.H{ + "new_prs": len(newPRs), + "new_prs_found": len(newPRs), + "prs": newPRs, + "cleaned": cleaned, + }) } func (c *Controller) httpTriggerAllReviewChecks(ctx *gin.Context) { diff --git a/apps/backend/internal/github/controller_test.go b/apps/backend/internal/github/controller_test.go index 05bbccf1d3..b492bcc133 100644 --- a/apps/backend/internal/github/controller_test.go +++ b/apps/backend/internal/github/controller_test.go @@ -19,6 +19,7 @@ import ( "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/db" taskmodels "github.com/kandev/kandev/internal/task/models" + taskservice "github.com/kandev/kandev/internal/task/service" ) // stubClient implements Client with no-op defaults; override fields as needed. @@ -178,6 +179,245 @@ func setupControllerStoreTest(t *testing.T) (*gin.Engine, *Store) { return router, store } +func TestHttpTriggerReviewWatchPublishesNewPREvents(t *testing.T) { + gin.SetMode(gin.TestMode) + tmp := t.TempDir() + dbConn, err := db.OpenSQLite(filepath.Join(tmp, "github.db")) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + sqlxDB := sqlx.NewDb(dbConn, "sqlite3") + t.Cleanup(func() { _ = sqlxDB.Close() }) + store, err := NewStore(sqlxDB, sqlxDB) + if err != nil { + t.Fatalf("new store: %v", err) + } + client := NewMockClient() + client.AddPR(&PR{ + Number: 42, + Title: "Review me", + HTMLURL: "https://github.com/acme/widget/pull/42", + State: "open", + HeadBranch: "feature/review", + BaseBranch: "main", + RepoOwner: "acme", + RepoName: "widget", + RequestedReviewers: []RequestedReviewer{ + {Login: "test-user", Type: "user"}, + }, + }) + + watch := &ReviewWatch{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Repos: []RepoFilter{{Owner: "acme", Name: "widget"}}, + ReviewScope: ReviewScopeUserAndTeams, + Enabled: true, + PollIntervalSeconds: defaultWatchPollIntervalSec, + CleanupPolicy: CleanupPolicyNever, + } + if err := store.CreateReviewWatch(context.Background(), watch); err != nil { + t.Fatalf("create review watch: %v", err) + } + + log := newControllerTestLogger() + eb := &mockEventBus{} + svc := NewService(client, "pat", nil, store, eb, log) + router := gin.New() + NewController(svc, log).RegisterHTTPRoutes(router) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/github/watches/review/"+watch.ID+"/trigger?workspace_id=ws-1", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", w.Code, w.Body.String()) + } + var body struct { + NewPRs int `json:"new_prs"` + NewPRsFound int `json:"new_prs_found"` + } + if err := json.NewDecoder(w.Body).Decode(&body); err != nil { + t.Fatalf("decode trigger response: %v", err) + } + if body.NewPRs != 1 || body.NewPRsFound != 1 { + t.Fatalf("response counts = %+v, want both counts 1", body) + } + if got := eb.publishedCount(); got != 1 { + t.Fatalf("published events = %d, want 1", got) + } +} + +func waitForPublishedCount(t *testing.T, eb *mockEventBus, want int) { + t.Helper() + timeout := time.After(time.Second) + for { + if got := eb.publishedCount(); got == want { + return + } + select { + case <-eb.publishedCh: + case <-timeout: + t.Fatalf("published events = %d, want %d", eb.publishedCount(), want) + } + } +} + +type noopCascadeTaskDeleter struct{} + +func (noopCascadeTaskDeleter) DeleteTaskTree( + context.Context, + string, + bool, +) (*taskservice.CascadeOutcome, error) { + return &taskservice.CascadeOutcome{}, nil +} + +func TestResetReviewWatchPublishesNewPREvents(t *testing.T) { + tmp := t.TempDir() + dbConn, err := db.OpenSQLite(filepath.Join(tmp, "github.db")) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + sqlxDB := sqlx.NewDb(dbConn, "sqlite3") + t.Cleanup(func() { _ = sqlxDB.Close() }) + store, err := NewStore(sqlxDB, sqlxDB) + if err != nil { + t.Fatalf("new store: %v", err) + } + client := NewMockClient() + client.AddPR(&PR{ + Number: 42, + Title: "Review me again", + HTMLURL: "https://github.com/acme/widget/pull/42", + State: "open", + HeadBranch: "feature/review-again", + BaseBranch: "main", + RepoOwner: "acme", + RepoName: "widget", + RequestedReviewers: []RequestedReviewer{ + {Login: "test-user", Type: "user"}, + }, + }) + + watch := &ReviewWatch{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Repos: []RepoFilter{{Owner: "acme", Name: "widget"}}, + ReviewScope: ReviewScopeUserAndTeams, + Enabled: true, + PollIntervalSeconds: defaultWatchPollIntervalSec, + CleanupPolicy: CleanupPolicyNever, + } + if err := store.CreateReviewWatch(context.Background(), watch); err != nil { + t.Fatalf("create review watch: %v", err) + } + if err := store.CreateReviewPRTask(context.Background(), &ReviewPRTask{ + ReviewWatchID: watch.ID, + RepoOwner: "acme", + RepoName: "widget", + PRNumber: 42, + PRURL: "https://github.com/acme/widget/pull/42", + TaskID: "task-old", + }); err != nil { + t.Fatalf("create review PR task: %v", err) + } + + log := newControllerTestLogger() + eb := &mockEventBus{publishedCh: make(chan struct{}, 1)} + svc := NewService(client, "pat", nil, store, eb, log) + svc.SetCascadeTaskDeleter(noopCascadeTaskDeleter{}) + + deleted, err := svc.ResetReviewWatch(context.Background(), watch.ID) + if err != nil { + t.Fatalf("reset review watch: %v", err) + } + if deleted != 1 { + t.Fatalf("deleted = %d, want 1", deleted) + } + waitForPublishedCount(t, eb, 1) +} + +func TestHttpResetReviewWatchPublishesNewPREvents(t *testing.T) { + gin.SetMode(gin.TestMode) + tmp := t.TempDir() + dbConn, err := db.OpenSQLite(filepath.Join(tmp, "github.db")) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + sqlxDB := sqlx.NewDb(dbConn, "sqlite3") + t.Cleanup(func() { _ = sqlxDB.Close() }) + store, err := NewStore(sqlxDB, sqlxDB) + if err != nil { + t.Fatalf("new store: %v", err) + } + client := NewMockClient() + client.AddPR(&PR{ + Number: 42, + Title: "Review me through reset", + HTMLURL: "https://github.com/acme/widget/pull/42", + State: "open", + HeadBranch: "feature/review-reset", + BaseBranch: "main", + RepoOwner: "acme", + RepoName: "widget", + RequestedReviewers: []RequestedReviewer{ + {Login: "test-user", Type: "user"}, + }, + }) + + watch := &ReviewWatch{ + WorkspaceID: "ws-1", + WorkflowID: "wf-1", + WorkflowStepID: "step-1", + Repos: []RepoFilter{{Owner: "acme", Name: "widget"}}, + ReviewScope: ReviewScopeUserAndTeams, + Enabled: true, + PollIntervalSeconds: defaultWatchPollIntervalSec, + CleanupPolicy: CleanupPolicyNever, + } + if err := store.CreateReviewWatch(context.Background(), watch); err != nil { + t.Fatalf("create review watch: %v", err) + } + if err := store.CreateReviewPRTask(context.Background(), &ReviewPRTask{ + ReviewWatchID: watch.ID, + RepoOwner: "acme", + RepoName: "widget", + PRNumber: 42, + PRURL: "https://github.com/acme/widget/pull/42", + TaskID: "task-old", + }); err != nil { + t.Fatalf("create review PR task: %v", err) + } + + log := newControllerTestLogger() + eb := &mockEventBus{publishedCh: make(chan struct{}, 1)} + svc := NewService(client, "pat", nil, store, eb, log) + svc.SetCascadeTaskDeleter(noopCascadeTaskDeleter{}) + router := gin.New() + NewController(svc, log).RegisterHTTPRoutes(router) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/github/watches/review/"+watch.ID+"/reset?workspace_id=ws-1", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", w.Code, w.Body.String()) + } + var body struct { + TasksDeleted int `json:"tasksDeleted"` + } + if err := json.NewDecoder(w.Body).Decode(&body); err != nil { + t.Fatalf("decode reset response: %v", err) + } + if body.TasksDeleted != 1 { + t.Fatalf("tasksDeleted = %d, want 1", body.TasksDeleted) + } + waitForPublishedCount(t, eb, 1) +} + func TestHttpLinkTaskIssue_SuccessAndInvalidJSON(t *testing.T) { router, ctrl := setupControllerTest(&stubClient{ getIssueFunc: func(_ context.Context, owner, repo string, number int) (*Issue, error) { diff --git a/apps/backend/internal/github/controller_watch_reset.go b/apps/backend/internal/github/controller_watch_reset.go index e58c13a42e..4fac5fdc2c 100644 --- a/apps/backend/internal/github/controller_watch_reset.go +++ b/apps/backend/internal/github/controller_watch_reset.go @@ -25,8 +25,8 @@ func (c *Controller) httpPreviewResetReviewWatch(ctx *gin.Context) { // httpResetReviewWatch executes the destructive reset: cascade-deletes // every task previously created by the review watch (including archived), -// wipes its dedup table, and nulls last_polled_at so the next poll -// re-imports every currently-matching PR. +// wipes its dedup table, and schedules the watch to re-run so currently +// matching PRs are published for task creation. func (c *Controller) httpResetReviewWatch(ctx *gin.Context) { id := ctx.Param("id") if !c.requireReviewWatchInWorkspace(ctx, id) { @@ -69,7 +69,7 @@ func (c *Controller) httpResetIssueWatch(ctx *gin.Context) { ctx.JSON(http.StatusOK, gin.H{"tasksDeleted": n}) } -// requireReviewWatchInWorkspace guards the reset endpoints against IDOR: +// requireReviewWatchInWorkspace guards review watch mutation endpoints against IDOR: // the caller must supply `?workspace_id=...` matching the watch's stored // workspace. Mismatch and not-found both return 404 so a probing client // can't tell whether a watch ID exists in another workspace. diff --git a/apps/backend/internal/github/handlers.go b/apps/backend/internal/github/handlers.go index 5572b03b30..2cce5c49e4 100644 --- a/apps/backend/internal/github/handlers.go +++ b/apps/backend/internal/github/handlers.go @@ -336,18 +336,26 @@ func wsTriggerReviewWatch(svc *Service, _ *logger.Logger) func(ctx context.Conte if id == "" { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, errMsgIDRequired, nil) } + workspaceID, _ := payload["workspace_id"].(string) + if workspaceID == "" { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "workspace_id is required", nil) + } watch, err := svc.GetReviewWatch(ctx, id) if err != nil { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, err.Error(), nil) } - if watch == nil { + if watch == nil || watch.WorkspaceID != workspaceID { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeNotFound, "review watch not found", nil) } - newPRs, err := svc.CheckReviewWatch(ctx, watch) + newPRs, err := svc.TriggerReviewWatch(ctx, watch) if err != nil { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, err.Error(), nil) } - return ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{"new_prs": len(newPRs), "prs": newPRs}) + return ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{ + "new_prs": len(newPRs), + "new_prs_found": len(newPRs), + "prs": newPRs, + }) } } diff --git a/apps/backend/internal/github/service_reviews.go b/apps/backend/internal/github/service_reviews.go index 17a2b4082a..a3471ce515 100644 --- a/apps/backend/internal/github/service_reviews.go +++ b/apps/backend/internal/github/service_reviews.go @@ -65,15 +65,12 @@ func (s *Service) CreateReviewWatch(ctx context.Context, req *CreateReviewWatchR // initialReviewCheck runs a single poll for a newly created review watch. func (s *Service) initialReviewCheck(ctx context.Context, watch *ReviewWatch) { - newPRs, err := s.CheckReviewWatch(ctx, watch) + newPRs, err := s.TriggerReviewWatch(ctx, watch) if err != nil { s.logger.Debug("initial review check failed", zap.String("watch_id", watch.ID), zap.Error(err)) return } - for _, pr := range newPRs { - s.publishNewReviewPREvent(ctx, watch, pr) - } if len(newPRs) > 0 { s.logger.Info("initial review check found PRs", zap.String("watch_id", watch.ID), @@ -272,6 +269,19 @@ func (s *Service) CheckReviewWatch(ctx context.Context, watch *ReviewWatch) ([]* return newPRs, nil } +// TriggerReviewWatch checks one watch and publishes events for every newly +// observed PR so the orchestrator can create the corresponding tasks. +func (s *Service) TriggerReviewWatch(ctx context.Context, watch *ReviewWatch) ([]*PR, error) { + newPRs, err := s.CheckReviewWatch(ctx, watch) + if err != nil { + return nil, err + } + for _, pr := range newPRs { + s.publishNewReviewPREvent(ctx, watch, pr) + } + return newPRs, nil +} + // fetchReviewPRs fetches PRs needing review based on the watch configuration. // When repo filters are set, they are always applied — even when a custom query is present // (the filter qualifier is appended to the query for each repo). @@ -658,15 +668,12 @@ func (s *Service) TriggerAllReviewChecks(ctx context.Context, workspaceID string if !watch.Enabled { continue } - newPRs, err := s.CheckReviewWatch(ctx, watch) + newPRs, err := s.TriggerReviewWatch(ctx, watch) if err != nil { s.logger.Error("failed to check review watch", zap.String("id", watch.ID), zap.Error(err)) continue } - for _, pr := range newPRs { - s.publishNewReviewPREvent(ctx, watch, pr) - } totalNew += len(newPRs) } s.logger.Info("review checks completed", diff --git a/apps/backend/internal/github/service_test.go b/apps/backend/internal/github/service_test.go index 2abf0ba470..05612ad68e 100644 --- a/apps/backend/internal/github/service_test.go +++ b/apps/backend/internal/github/service_test.go @@ -673,17 +673,24 @@ func TestFindLatestCommentTime(t *testing.T) { } } -// --- mockEventBus for SyncTaskPR tests --- +// --- mockEventBus shared by SyncTaskPR and review-watch tests --- type mockEventBus struct { - mu sync.Mutex - events []*bus.Event + mu sync.Mutex + events []*bus.Event + publishedCh chan struct{} } func (m *mockEventBus) Publish(_ context.Context, _ string, event *bus.Event) error { m.mu.Lock() - defer m.mu.Unlock() m.events = append(m.events, event) + m.mu.Unlock() + if m.publishedCh != nil { + select { + case m.publishedCh <- struct{}{}: + default: + } + } return nil } diff --git a/apps/backend/internal/github/service_watch_reset.go b/apps/backend/internal/github/service_watch_reset.go index 5efb8173a9..dd3cd69b10 100644 --- a/apps/backend/internal/github/service_watch_reset.go +++ b/apps/backend/internal/github/service_watch_reset.go @@ -4,6 +4,8 @@ import ( "context" "errors" + "go.uber.org/zap" + "github.com/kandev/kandev/internal/watchreset" ) @@ -82,8 +84,8 @@ func (s *Service) PreviewResetReviewWatch(ctx context.Context, watchID string) ( // ResetReviewWatch is destructive: cascade-deletes every task previously // created by the review watch (including archived), wipes the per-watch -// dedup rows, and nulls last_polled_at so the next poll re-imports every -// currently-matching PR. Returns the count of tasks deleted. +// dedup rows, and then schedules the watch to re-run so currently-matching +// PRs are published for task creation. Returns the count of tasks deleted. func (s *Service) ResetReviewWatch(ctx context.Context, watchID string) (int, error) { if s.store == nil { return 0, errStoreUnavailable @@ -97,5 +99,27 @@ func (s *Service) ResetReviewWatch(ctx context.Context, watchID string) (int, er res, err := watchreset.Run(ctx, &githubReviewWatchResetter{store: s.store, watchID: watchID}, td, s.logger) - return res.TasksDeleted, err + if err != nil { + return res.TasksDeleted, err + } + go s.reimportReviewWatchAfterReset(context.Background(), watchID) + return res.TasksDeleted, nil +} + +func (s *Service) reimportReviewWatchAfterReset(ctx context.Context, watchID string) { + watch, err := s.GetReviewWatch(ctx, watchID) + if err != nil { + s.logger.Warn("reset review watch: could not load watch for re-import", + zap.String("watch_id", watchID), + zap.Error(err)) + return + } + if watch == nil || !watch.Enabled { + return + } + if _, err := s.TriggerReviewWatch(ctx, watch); err != nil { + s.logger.Warn("reset review watch: re-import failed", + zap.String("watch_id", watch.ID), + zap.Error(err)) + } } diff --git a/apps/web/components/github/github-settings.tsx b/apps/web/components/github/github-settings.tsx index d6554a92f5..46fae9b464 100644 --- a/apps/web/components/github/github-settings.tsx +++ b/apps/web/components/github/github-settings.tsx @@ -92,8 +92,13 @@ function useWatchActions(workspaceId?: string | null) { const handleTrigger = useCallback( async (id: string) => { + const watch = watches.find((item) => item.id === id); + if (!watch) { + toast({ description: "Review watch not found", variant: "error" }); + return; + } try { - const result = await trigger(id); + const result = await trigger(id, watch.workspace_id); const count = result?.new_prs_found ?? 0; if (count > 0) { toast({ @@ -107,7 +112,7 @@ function useWatchActions(workspaceId?: string | null) { toast({ description: "Failed to check for PRs", variant: "error" }); } }, - [trigger, toast], + [trigger, toast, watches], ); const handleToggleEnabled = useCallback( diff --git a/apps/web/e2e/helpers/api-client.ts b/apps/web/e2e/helpers/api-client.ts index 0f4752d5ae..10517a2914 100644 --- a/apps/web/e2e/helpers/api-client.ts +++ b/apps/web/e2e/helpers/api-client.ts @@ -1355,8 +1355,16 @@ export class ApiClient { await this.request("DELETE", `/api/v1/github/watches/review/${watchId}`); } - async triggerReviewWatch(watchId: string): Promise<{ new_prs: number; cleaned?: number }> { - return this.request("POST", `/api/v1/github/watches/review/${watchId}/trigger`, undefined); + async triggerReviewWatch( + watchId: string, + workspaceId: string, + ): Promise<{ new_prs: number; new_prs_found: number; cleaned?: number }> { + const params = new URLSearchParams({ workspace_id: workspaceId }); + return this.request( + "POST", + `/api/v1/github/watches/review/${watchId}/trigger?${params}`, + undefined, + ); } /** diff --git a/apps/web/e2e/tests/integrations/github-watch-reset.spec.ts b/apps/web/e2e/tests/integrations/github-watch-reset.spec.ts index 5a8f949761..060bfb12ad 100644 --- a/apps/web/e2e/tests/integrations/github-watch-reset.spec.ts +++ b/apps/web/e2e/tests/integrations/github-watch-reset.spec.ts @@ -5,12 +5,10 @@ import { test, expect } from "../../fixtures/test-base"; * 1. Watch creates a task for a mock PR (via trigger). * 2. User clicks Reset on the settings page → confirmation dialog shows * the preview count ("delete N task(s)"). - * 3. Confirm → backend cascade-deletes the task, wipes dedup, nulls - * last_polled_at; next trigger re-imports the same PR. + * 3. Confirm → backend cascade-deletes the task, wipes dedup, and queues + * the same PR for re-import. * - * The same controller wiring backs Jira / Linear / Sentry / GitHub-issue - * watches (`watchreset.Run`), so this single spec exercises the shared - * orchestration end-to-end. Per-integration store coverage lives in the + * Store-level reset coverage for the shared `watchreset.Run` flow lives in * Go unit tests. */ test.describe("GitHub review watch reset", () => { @@ -45,7 +43,7 @@ test.describe("GitHub review watch reset", () => { { repos: [{ owner: "testorg", name: "testrepo" }] }, ); - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); await expect .poll( async () => { @@ -74,13 +72,20 @@ test.describe("GitHub review watch reset", () => { expect(reset.status).toBe(200); expect(await reset.json()).toMatchObject({ tasksDeleted: 1 }); - // Task is gone from the workspace. - const { tasks: afterReset } = await apiClient.listTasks(seedData.workspaceId); - expect(afterReset.find((t) => t.title.includes("PR #5101"))).toBeUndefined(); + // Reset queues the matching PR again, so a replacement task + // appears without waiting for a manual trigger or poll tick. + await expect + .poll( + async () => { + const { tasks } = await apiClient.listTasks(seedData.workspaceId); + return tasks.filter((t) => t.title.includes("PR #5101")).length; + }, + { timeout: 15_000 }, + ) + .toBe(1); - // Watch's polling cursor was cleared so the next poll cycle will - // re-evaluate every currently-matching PR. last_polled_at sits on - // the watch row exposed by the list endpoint. + // Watch's polling cursor reflects the re-check that reset scheduled. + // last_polled_at sits on the watch row exposed by the list endpoint. const listRes = await apiClient.rawRequest( "GET", `/api/v1/github/watches/review?workspace_id=${seedData.workspaceId}`, @@ -91,16 +96,12 @@ test.describe("GitHub review watch reset", () => { }; const refreshed = list.watches.find((w) => w.id === watch.id); expect(refreshed).toBeDefined(); - // The Go model marshals *time.Time with omitempty, so a nil cursor is - // serialised as either null or omitted — both are valid "cleared" shapes. - expect(refreshed!.last_polled_at ?? null).toBeNull(); + expect(refreshed!.last_polled_at).toBeTruthy(); - // CheckReviewWatch now reports PR #5101 as new again — the dedup - // wipe means the periodic poller will publish a NewReviewPR event for - // it on the next tick. (The HTTP trigger handler intentionally only - // probes; event publishing is the poller's job.) - const trigger = await apiClient.triggerReviewWatch(watch.id); - expect(trigger.new_prs).toBe(1); + // The reset-triggered re-import already reserved the PR, so a manual + // trigger has nothing new to create. + const trigger = await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); + expect(trigger.new_prs).toBe(0); }); test("settings page reset flow shows preview count and deletes the task", async ({ @@ -135,7 +136,7 @@ test.describe("GitHub review watch reset", () => { { repos: [{ owner: "testorg", name: "testrepo" }] }, ); - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); await expect .poll( async () => { @@ -172,8 +173,15 @@ test.describe("GitHub review watch reset", () => { // Success toast surfaces the deletion count. await expect(testPage.getByText(/deleted 1 task/i)).toBeVisible({ timeout: 10_000 }); - // The task was actually removed. - const { tasks } = await apiClient.listTasks(seedData.workspaceId); - expect(tasks.find((t) => t.title.includes("PR #5201"))).toBeUndefined(); + // The matching PR is recreated after the reset clears the old task. + await expect + .poll( + async () => { + const { tasks } = await apiClient.listTasks(seedData.workspaceId); + return tasks.filter((t) => t.title.includes("PR #5201")).length; + }, + { timeout: 15_000 }, + ) + .toBe(1); }); }); diff --git a/apps/web/e2e/tests/pr/pr-watcher-cleanup-policy.spec.ts b/apps/web/e2e/tests/pr/pr-watcher-cleanup-policy.spec.ts index 210e170992..0be5472950 100644 --- a/apps/web/e2e/tests/pr/pr-watcher-cleanup-policy.spec.ts +++ b/apps/web/e2e/tests/pr/pr-watcher-cleanup-policy.spec.ts @@ -79,7 +79,7 @@ test.describe("PR watcher cleanup policy", () => { { repos: [{ owner: "testorg", name: "testrepo" }] }, ); - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); let prTask: { id: string; title: string } | undefined; await expect .poll( @@ -121,7 +121,7 @@ test.describe("PR watcher cleanup policy", () => { }, ]); - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); await expect(kanban.taskCardByTitle("PR #401: Auto-start only")).not.toBeVisible({ timeout: 15_000, @@ -170,7 +170,7 @@ test.describe("PR watcher cleanup policy", () => { }, ); - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); await expect .poll( async () => { @@ -200,7 +200,7 @@ test.describe("PR watcher cleanup policy", () => { repo_name: "testrepo", }, ]); - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); const manualResult = await apiClient.cleanupMergedReviewTasks(); expect(manualResult.deleted).toBe(0); @@ -280,7 +280,7 @@ test.describe("PR watcher cleanup policy", () => { }, ); - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); let prTask: { id: string; title: string } | undefined; await expect .poll( @@ -319,7 +319,7 @@ test.describe("PR watcher cleanup policy", () => { }, ]); - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); await expect(kanban.taskCardByTitle("PR #601: Always delete")).not.toBeVisible({ timeout: 15_000, @@ -366,7 +366,7 @@ test.describe("PR watcher cleanup policy", () => { { repos: [{ owner: "testorg", name: "testrepo" }] }, ); - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); await expect .poll( async () => { @@ -445,7 +445,7 @@ test.describe("PR watcher cleanup policy", () => { { repos: [{ owner: "testorg", name: "testrepo" }] }, ); - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); await expect .poll( async () => { @@ -509,7 +509,7 @@ test.describe("PR watcher cleanup policy", () => { { repos: [{ owner: "testorg", name: "testrepo" }] }, ); - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); await expect .poll( async () => { diff --git a/apps/web/e2e/tests/pr/pr-watcher-merged-cleanup.spec.ts b/apps/web/e2e/tests/pr/pr-watcher-merged-cleanup.spec.ts index 61e906a585..5667b0f887 100644 --- a/apps/web/e2e/tests/pr/pr-watcher-merged-cleanup.spec.ts +++ b/apps/web/e2e/tests/pr/pr-watcher-merged-cleanup.spec.ts @@ -59,7 +59,7 @@ test.describe("PR watcher merged cleanup", () => { // because the atomic dedup reservation means whichever poll runs first // claims the PR, and the subsequent trigger correctly returns 0 "new". // The real check is the task-exists poll below. - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); // Task creation is async (goroutine), poll until it appears let prTask: { id: string; title: string } | undefined; @@ -96,7 +96,7 @@ test.describe("PR watcher merged cleanup", () => { ]); // --- Trigger watch again → should detect merged PR and delete the task --- - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); // Verify task was deleted await expect(kanban.taskCardByTitle("PR #101: Feature to review")).not.toBeVisible({ @@ -183,7 +183,7 @@ test.describe("PR watcher merged cleanup", () => { ); // Trigger → task created in Inbox - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); let prTask: { id: string; title: string } | undefined; await expect @@ -233,7 +233,7 @@ test.describe("PR watcher merged cleanup", () => { ]); // Trigger watch → should NOT delete task (user already worked on it) - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); // Task should still be visible await expect(kanban.taskCardByTitle("PR #202: Reviewed feature")).toBeVisible({ @@ -285,7 +285,7 @@ test.describe("PR watcher merged cleanup", () => { ); // Trigger → task created - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); await expect .poll( @@ -316,7 +316,7 @@ test.describe("PR watcher merged cleanup", () => { ]); // Trigger watch → should detect approved PR and delete the task - await apiClient.triggerReviewWatch(watch.id); + await apiClient.triggerReviewWatch(watch.id, watch.workspace_id); // Verify task was deleted await expect(kanban.taskCardByTitle("PR #303: Quick fix to approve")).not.toBeVisible({ diff --git a/apps/web/hooks/domains/github/use-review-watches.ts b/apps/web/hooks/domains/github/use-review-watches.ts index b4e54ec0e3..4ce23467fc 100644 --- a/apps/web/hooks/domains/github/use-review-watches.ts +++ b/apps/web/hooks/domains/github/use-review-watches.ts @@ -11,7 +11,7 @@ import { previewResetReviewWatch, resetReviewWatch, } from "@/lib/api/domains/github-api"; -import { useAppStore, useAppStoreApi } from "@/components/state-provider"; +import { useAppStore } from "@/components/state-provider"; import type { CreateReviewWatchRequest, UpdateReviewWatchRequest } from "@/lib/types/github"; // useReviewWatches has three modes: @@ -27,10 +27,6 @@ export function useReviewWatches(workspaceId?: string | null) { const addWatch = useAppStore((state) => state.addReviewWatch); const updateWatch = useAppStore((state) => state.updateReviewWatch); const removeWatch = useAppStore((state) => state.removeReviewWatch); - // storeApi exposes getState() without subscribing — used in reset() to - // read the current watch row outside of the React render cycle so the - // callback doesn't need `items` as a dependency. - const storeApi = useAppStoreApi(); useEffect(() => { if (workspaceId === null || loaded || loading) return; @@ -73,8 +69,8 @@ export function useReviewWatches(workspaceId?: string | null) { [removeWatch], ); - const trigger = useCallback(async (id: string) => { - return triggerReviewWatch(id); + const trigger = useCallback(async (id: string, watchWorkspaceId: string) => { + return triggerReviewWatch(id, watchWorkspaceId); }, []); const triggerAll = useCallback(async () => { @@ -88,14 +84,16 @@ export function useReviewWatches(workspaceId?: string | null) { const reset = useCallback( async (id: string, watchWorkspaceId: string) => { - const res = await resetReviewWatch(id, watchWorkspaceId); - // Patch the cached watch so the "Last polled" column reflects the - // reset immediately without waiting for the next poll tick. - const current = storeApi.getState().reviewWatches.items.find((w) => w.id === id); - if (current) updateWatch({ ...current, last_polled_at: null }); - return res; + const result = await resetReviewWatch(id, watchWorkspaceId); + try { + const response = await listReviewWatches(workspaceId ?? undefined, { cache: "no-store" }); + setReviewWatches(response?.watches ?? []); + } catch { + // Reset succeeded; a stale settings table is less harmful than failing the action. + } + return result; }, - [storeApi, updateWatch], + [setReviewWatches, workspaceId], ); return { diff --git a/apps/web/lib/api/domains/github-api.ts b/apps/web/lib/api/domains/github-api.ts index 84d93c0abe..55d035d541 100644 --- a/apps/web/lib/api/domains/github-api.ts +++ b/apps/web/lib/api/domains/github-api.ts @@ -274,8 +274,13 @@ export async function deleteReviewWatch(id: string, options?: ApiRequestOptions) }); } -export async function triggerReviewWatch(id: string, options?: ApiRequestOptions) { - return fetchJson(`/api/v1/github/watches/review/${id}/trigger`, { +export async function triggerReviewWatch( + id: string, + workspaceId: string, + options?: ApiRequestOptions, +) { + const params = new URLSearchParams({ workspace_id: workspaceId }); + return fetchJson(`/api/v1/github/watches/review/${id}/trigger?${params}`, { ...options, init: { method: "POST", ...(options?.init ?? {}) }, }); @@ -298,8 +303,8 @@ export async function previewResetReviewWatch( } // resetReviewWatch deletes every task previously created by the review -// watch (including archived), wipes its dedup table, and nulls -// last_polled_at so the next poll re-imports every currently-matching PR. +// watch (including archived), wipes its dedup table, and schedules the +// watch to re-run so currently-matching PRs are queued for task creation. export async function resetReviewWatch( id: string, workspaceId: string,