diff --git a/apps/backend/internal/github/service.go b/apps/backend/internal/github/service.go index d148f5baf8..1cb0222cfc 100644 --- a/apps/backend/internal/github/service.go +++ b/apps/backend/internal/github/service.go @@ -400,6 +400,8 @@ func (s *Service) ListTaskPRs(ctx context.Context, taskIDs []string) (map[string } // SyncTaskPR updates a TaskPR record with the latest PR status. +// It only publishes a github.task_pr.updated event when data actually changed, +// preventing feedback loops with frontend sync handlers. func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatus) error { if status == nil || status.PR == nil { return fmt.Errorf("sync task PR: missing PR data for task %s", taskID) @@ -409,6 +411,17 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu return err } + changed := tp.State != status.PR.State || + tp.PRTitle != status.PR.Title || + tp.Additions != status.PR.Additions || + tp.Deletions != status.PR.Deletions || + tp.ReviewState != status.ReviewState || + tp.ChecksState != status.ChecksState || + tp.ReviewCount != status.ReviewCount || + tp.PendingReviewCount != status.PendingReviewCount || + !timeEqual(tp.MergedAt, status.PR.MergedAt) || + !timeEqual(tp.ClosedAt, status.PR.ClosedAt) + tp.State = status.PR.State tp.PRTitle = status.PR.Title tp.Additions = status.PR.Additions @@ -427,7 +440,7 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu return fmt.Errorf("update task PR: %w", err) } - if s.eventBus != nil { + if changed && s.eventBus != nil { event := bus.NewEvent(events.GitHubTaskPRUpdated, "github", tp) if err := s.eventBus.Publish(ctx, events.GitHubTaskPRUpdated, event); err != nil { s.logger.Debug("failed to publish task PR updated event", zap.Error(err)) @@ -436,6 +449,17 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu return nil } +// timeEqual compares two nullable time pointers for equality. +func timeEqual(a, b *time.Time) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + return a.Equal(*b) +} + // --- PR info and feedback (live) --- // GetPR fetches basic PR details from GitHub. diff --git a/apps/backend/internal/github/service_test.go b/apps/backend/internal/github/service_test.go index 39a41a6a61..2d26372b2b 100644 --- a/apps/backend/internal/github/service_test.go +++ b/apps/backend/internal/github/service_test.go @@ -2,9 +2,17 @@ package github import ( "context" + "database/sql" "strings" + "sync" "testing" "time" + + "github.com/jmoiron/sqlx" + _ "github.com/mattn/go-sqlite3" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/events/bus" ) func TestGetPR_NilClient(t *testing.T) { @@ -420,6 +428,221 @@ func TestFindLatestCommentTime(t *testing.T) { } } +// --- mockEventBus for SyncTaskPR tests --- + +type mockEventBus struct { + mu sync.Mutex + events []*bus.Event +} + +func (m *mockEventBus) Publish(_ context.Context, _ string, event *bus.Event) error { + m.mu.Lock() + defer m.mu.Unlock() + m.events = append(m.events, event) + return nil +} + +func (m *mockEventBus) Subscribe(string, bus.EventHandler) (bus.Subscription, error) { + return nil, nil +} + +func (m *mockEventBus) QueueSubscribe(string, string, bus.EventHandler) (bus.Subscription, error) { + return nil, nil +} + +func (m *mockEventBus) Request(context.Context, string, *bus.Event, time.Duration) (*bus.Event, error) { + return nil, nil +} + +func (m *mockEventBus) Close() {} + +func (m *mockEventBus) IsConnected() bool { return true } + +func (m *mockEventBus) publishedCount() int { + m.mu.Lock() + defer m.mu.Unlock() + return len(m.events) +} + +// setupSyncTest creates a Service backed by in-memory SQLite and a mock event bus. +func setupSyncTest(t *testing.T) (*Service, *Store, *mockEventBus) { + t.Helper() + + rawDB, err := sql.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + rawDB.SetMaxOpenConns(1) + rawDB.SetMaxIdleConns(1) + sqlxDB := sqlx.NewDb(rawDB, "sqlite3") + t.Cleanup(func() { _ = sqlxDB.Close() }) + + store, err := NewStore(sqlxDB, sqlxDB) + if err != nil { + t.Fatalf("new store: %v", err) + } + + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"}) + eb := &mockEventBus{} + svc := NewService(nil, "pat", nil, store, eb, log) + + return svc, store, eb +} + +func TestSyncTaskPR_PublishesEventOnChange(t *testing.T) { + svc, store, eb := setupSyncTest(t) + ctx := context.Background() + + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", + Owner: "owner", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/owner/repo/pull/1", + PRTitle: "Initial", + HeadBranch: "feat", + BaseBranch: "main", + State: "open", + }); err != nil { + t.Fatalf("create task PR: %v", err) + } + + status := &PRStatus{ + PR: &PR{ + Number: 1, + Title: "Updated title", + State: "open", + RepoOwner: "owner", + RepoName: "repo", + }, + } + + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync task PR: %v", err) + } + + if got := eb.publishedCount(); got != 1 { + t.Errorf("expected 1 event after change, got %d", got) + } +} + +func TestSyncTaskPR_NoEventWhenUnchanged(t *testing.T) { + svc, store, eb := setupSyncTest(t) + ctx := context.Background() + + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", + Owner: "owner", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/owner/repo/pull/1", + PRTitle: "Same title", + HeadBranch: "feat", + BaseBranch: "main", + State: "open", + Additions: 10, + Deletions: 5, + ReviewState: "approved", + ChecksState: "success", + ReviewCount: 2, + }); err != nil { + t.Fatalf("create task PR: %v", err) + } + + status := &PRStatus{ + PR: &PR{ + Number: 1, + Title: "Same title", + State: "open", + Additions: 10, + Deletions: 5, + RepoOwner: "owner", + RepoName: "repo", + }, + ReviewState: "approved", + ChecksState: "success", + ReviewCount: 2, + } + + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync task PR: %v", err) + } + + if got := eb.publishedCount(); got != 0 { + t.Errorf("expected 0 events when unchanged, got %d", got) + } +} + +func TestSyncTaskPR_SecondIdenticalSyncNoEvent(t *testing.T) { + svc, store, eb := setupSyncTest(t) + ctx := context.Background() + + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", + Owner: "owner", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/owner/repo/pull/1", + PRTitle: "Original", + HeadBranch: "feat", + BaseBranch: "main", + State: "open", + }); err != nil { + t.Fatalf("create task PR: %v", err) + } + + status := &PRStatus{ + PR: &PR{ + Number: 1, + Title: "Changed", + State: "open", + RepoOwner: "owner", + RepoName: "repo", + }, + } + + // First sync: data changed -> event. + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("first sync: %v", err) + } + if got := eb.publishedCount(); got != 1 { + t.Fatalf("expected 1 event after first sync, got %d", got) + } + + // Second sync with same data -> no additional event. + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("second sync: %v", err) + } + if got := eb.publishedCount(); got != 1 { + t.Errorf("expected still 1 event after identical second sync, got %d", got) + } +} + +func TestTimeEqual(t *testing.T) { + now := time.Now() + later := now.Add(time.Second) + + tests := []struct { + name string + a, b *time.Time + want bool + }{ + {"both nil", nil, nil, true}, + {"a nil", nil, &now, false}, + {"b nil", &now, nil, false}, + {"equal", &now, &now, true}, + {"not equal", &now, &later, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := timeEqual(tt.a, tt.b); got != tt.want { + t.Errorf("timeEqual() = %v, want %v", got, tt.want) + } + }) + } +} + func TestTriggerPRSync_SyncsExistingWatch(t *testing.T) { _, svc, mockClient, store := setupPollerTest(t) ctx := context.Background() diff --git a/apps/web/hooks/domains/github/use-task-pr.ts b/apps/web/hooks/domains/github/use-task-pr.ts index fdd17ef46c..033c2a051b 100644 --- a/apps/web/hooks/domains/github/use-task-pr.ts +++ b/apps/web/hooks/domains/github/use-task-pr.ts @@ -64,14 +64,16 @@ export function useTaskPR(taskId: string | null) { retryRef.current = 0; }, [taskId]); + // Sync once when the task becomes active (freshness check). + // Intentionally excludes `pr` so WS-driven store updates don't re-trigger. useEffect(() => { if (!taskId) return; - - // Always sync once when the task becomes active (freshness) refresh(); + }, [taskId, refresh]); - // If no PR in store yet, retry periodically until found - if (pr) return; + // Retry polling when no PR is in the store yet. + useEffect(() => { + if (!taskId || pr) return; const interval = setInterval(() => { if (retryRef.current >= SYNC_MAX_RETRIES) { diff --git a/apps/web/lib/state/dockview-layout-builders.ts b/apps/web/lib/state/dockview-layout-builders.ts index aa9cb612b9..6a2ed90e38 100644 --- a/apps/web/lib/state/dockview-layout-builders.ts +++ b/apps/web/lib/state/dockview-layout-builders.ts @@ -1,6 +1,8 @@ import type { DockviewApi, AddPanelOptions } from "dockview-react"; import { SIDEBAR_LOCK, + SIDEBAR_GROUP, + CENTER_GROUP, LAYOUT_SIDEBAR_MAX_PX, LAYOUT_RIGHT_MAX_PX, RIGHT_TOP_GROUP, @@ -50,15 +52,21 @@ export function focusOrAddPanel( return; } // Guard: if the referenced group no longer exists (stale ID after layout - // transition), fall back to the active panel's group or the first group. + // transition), try the well-known center group, then the first non-sidebar + // group. Avoid falling back to the active panel's group because the user + // may have just clicked in the sidebar, which would place the new panel there. const pos = options.position; if (pos && "referenceGroup" in pos) { const groupExists = api.groups.some((g) => g.id === pos.referenceGroup); if (!groupExists) { - if (api.activePanel) { - options = { ...options, position: { referencePanel: api.activePanel.id } }; + const centerGroup = api.groups.find((g) => g.id === CENTER_GROUP); + const nonSidebarGroup = api.groups.find((g) => g.id !== SIDEBAR_GROUP); + if (centerGroup) { + options = { ...options, position: { ...pos, referenceGroup: centerGroup.id } }; + } else if (nonSidebarGroup) { + options = { ...options, position: { ...pos, referenceGroup: nonSidebarGroup.id } }; } else if (api.groups.length > 0) { - options = { ...options, position: { referenceGroup: api.groups[0].id } }; + options = { ...options, position: { ...pos, referenceGroup: api.groups[0].id } }; } else { options = Object.fromEntries( Object.entries(options).filter(([k]) => k !== "position"),