fix: changed branch and auto focus changes panel (#549)

This commit is contained in:
Carlos Florêncio
2026-04-06 18:59:37 +01:00
committed by GitHub
parent 52d6f7e9bf
commit a44489640b
10 changed files with 457 additions and 26 deletions
+40 -2
View File
@@ -26,9 +26,12 @@ type TaskBranchInfo struct {
Branch string
}
// TaskBranchProvider lists tasks that should have PR watches.
// TaskBranchProvider lists tasks that should have PR watches and resolves branches.
type TaskBranchProvider interface {
ListTasksNeedingPRWatch(ctx context.Context) ([]TaskBranchInfo, error)
// ResolveBranchForSession returns the current branch for a task+session pair.
// Used to detect branch renames and update stale PR watches.
ResolveBranchForSession(ctx context.Context, taskID, sessionID string) string
}
// Poller runs background loops for PR monitoring and review queue checking.
@@ -231,11 +234,17 @@ func (p *Poller) SetTaskBranchProvider(provider TaskBranchProvider) {
p.taskBranchProvider = provider
}
// reconcileWatches ensures PR watches exist for all tasks that need them.
// reconcileWatches ensures PR watches exist for all tasks that need them,
// and refreshes stale branches on existing watches that haven't found a PR yet.
func (p *Poller) reconcileWatches(ctx context.Context) {
if p.taskBranchProvider == nil {
return
}
// 1. Refresh branches on existing pr_number=0 watches (branch may have changed).
p.refreshStaleBranches(ctx)
// 2. Create new watches for sessions that don't have one.
tasks, err := p.taskBranchProvider.ListTasksNeedingPRWatch(ctx)
if err != nil {
p.logger.Error("failed to list tasks needing PR watch", zap.Error(err))
@@ -251,6 +260,35 @@ func (p *Poller) reconcileWatches(ctx context.Context) {
}
}
// refreshStaleBranches re-resolves branches for watches that haven't found a PR yet.
// If the user renamed/changed the branch, the watch is updated so the next poll
// searches on the correct branch.
func (p *Poller) refreshStaleBranches(ctx context.Context) {
watches, err := p.service.ListActivePRWatches(ctx)
if err != nil {
return
}
for _, watch := range watches {
if watch.PRNumber != 0 {
continue // already found a PR, branch is correct
}
currentBranch := p.taskBranchProvider.ResolveBranchForSession(
ctx, watch.TaskID, watch.SessionID,
)
if currentBranch == "" || currentBranch == watch.Branch {
continue
}
p.logger.Info("PR watch branch changed, updating",
zap.String("session_id", watch.SessionID),
zap.String("old_branch", watch.Branch),
zap.String("new_branch", currentBranch))
if updateErr := p.service.UpdatePRWatchBranchIfSearching(ctx, watch.ID, currentBranch); updateErr != nil {
p.logger.Error("failed to update PR watch branch",
zap.String("watch_id", watch.ID), zap.Error(updateErr))
}
}
}
// reviewQueueLoop polls review watches for new PRs.
func (p *Poller) reviewQueueLoop(ctx context.Context) {
defer p.wg.Done()
+144 -2
View File
@@ -199,14 +199,22 @@ func TestCheckSinglePRWatch_OpenPR_SyncsOnChange(t *testing.T) {
// mockTaskBranchProvider implements TaskBranchProvider for testing.
type mockTaskBranchProvider struct {
tasks []TaskBranchInfo
err error
tasks []TaskBranchInfo
err error
branches map[string]string // sessionID -> branch
}
func (m *mockTaskBranchProvider) ListTasksNeedingPRWatch(_ context.Context) ([]TaskBranchInfo, error) {
return m.tasks, m.err
}
func (m *mockTaskBranchProvider) ResolveBranchForSession(_ context.Context, _, sessionID string) string {
if m.branches != nil {
return m.branches[sessionID]
}
return ""
}
func TestReconcileWatches_CreatesWatchesForTasks(t *testing.T) {
poller, _, _, store := setupPollerTest(t)
ctx := context.Background()
@@ -359,3 +367,137 @@ func TestCheckSinglePRWatch_OpenPR_NoChange_NoSync(t *testing.T) {
t.Error("expected PR watch to still exist")
}
}
func TestRefreshStaleBranches_UpdatesBranchWhenChanged(t *testing.T) {
poller, _, _, store := setupPollerTest(t)
ctx := context.Background()
// Create a watch with pr_number=0 on old branch.
watch := &PRWatch{
SessionID: "s1",
TaskID: "t1",
Owner: "myorg",
Repo: "myrepo",
PRNumber: 0,
Branch: "old-branch",
}
if err := store.CreatePRWatch(ctx, watch); err != nil {
t.Fatalf("create PR watch: %v", err)
}
// Provider resolves a different branch for this session.
prov := &mockTaskBranchProvider{
branches: map[string]string{
"s1": "new-branch",
},
}
poller.SetTaskBranchProvider(prov)
poller.refreshStaleBranches(ctx)
// Verify the watch branch was updated.
updated, err := store.GetPRWatchBySession(ctx, "s1")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if updated == nil {
t.Fatal("expected PR watch to exist")
}
if updated.Branch != "new-branch" {
t.Errorf("expected branch %q, got %q", "new-branch", updated.Branch)
}
}
func TestRefreshStaleBranches_SkipsWhenBranchUnchanged(t *testing.T) {
poller, _, _, store := setupPollerTest(t)
ctx := context.Background()
watch := &PRWatch{
SessionID: "s1",
TaskID: "t1",
Owner: "myorg",
Repo: "myrepo",
PRNumber: 0,
Branch: "same-branch",
}
if err := store.CreatePRWatch(ctx, watch); err != nil {
t.Fatalf("create PR watch: %v", err)
}
prov := &mockTaskBranchProvider{
branches: map[string]string{
"s1": "same-branch",
},
}
poller.SetTaskBranchProvider(prov)
poller.refreshStaleBranches(ctx)
updated, _ := store.GetPRWatchBySession(ctx, "s1")
if updated.Branch != "same-branch" {
t.Errorf("expected branch unchanged, got %q", updated.Branch)
}
}
func TestRefreshStaleBranches_SkipsWatchesWithPR(t *testing.T) {
poller, _, _, store := setupPollerTest(t)
ctx := context.Background()
// Watch that already found a PR (pr_number > 0).
watch := &PRWatch{
SessionID: "s1",
TaskID: "t1",
Owner: "myorg",
Repo: "myrepo",
PRNumber: 42,
Branch: "old-branch",
}
if err := store.CreatePRWatch(ctx, watch); err != nil {
t.Fatalf("create PR watch: %v", err)
}
prov := &mockTaskBranchProvider{
branches: map[string]string{
"s1": "new-branch",
},
}
poller.SetTaskBranchProvider(prov)
poller.refreshStaleBranches(ctx)
// Branch should NOT be updated because PR was already found.
updated, _ := store.GetPRWatchBySession(ctx, "s1")
if updated.Branch != "old-branch" {
t.Errorf("expected branch unchanged for PR watch, got %q", updated.Branch)
}
}
func TestRefreshStaleBranches_SkipsWhenResolverReturnsEmpty(t *testing.T) {
poller, _, _, store := setupPollerTest(t)
ctx := context.Background()
watch := &PRWatch{
SessionID: "s1",
TaskID: "t1",
Owner: "myorg",
Repo: "myrepo",
PRNumber: 0,
Branch: "old-branch",
}
if err := store.CreatePRWatch(ctx, watch); err != nil {
t.Fatalf("create PR watch: %v", err)
}
// Provider returns empty string (can't resolve).
prov := &mockTaskBranchProvider{
branches: map[string]string{},
}
poller.SetTaskBranchProvider(prov)
poller.refreshStaleBranches(ctx)
updated, _ := store.GetPRWatchBySession(ctx, "s1")
if updated.Branch != "old-branch" {
t.Errorf("expected branch unchanged when resolver returns empty, got %q", updated.Branch)
}
}
+5
View File
@@ -213,6 +213,11 @@ func (s *Service) UpdatePRWatchBranch(ctx context.Context, id, branch string) er
return s.store.UpdatePRWatchBranch(ctx, id, branch)
}
// UpdatePRWatchBranchIfSearching atomically updates branch only when pr_number = 0.
func (s *Service) UpdatePRWatchBranchIfSearching(ctx context.Context, id, branch string) error {
return s.store.UpdatePRWatchBranchIfSearching(ctx, id, branch)
}
// UpdatePRWatchPRNumber updates a PR watch's PR number after discovery.
func (s *Service) UpdatePRWatchPRNumber(ctx context.Context, id string, prNumber int) error {
return s.store.UpdatePRWatchPRNumber(ctx, id, prNumber)
+9
View File
@@ -187,6 +187,15 @@ func (s *Store) UpdatePRWatchBranch(ctx context.Context, id, branch string) erro
return err
}
// UpdatePRWatchBranchIfSearching atomically updates branch only when pr_number = 0,
// preventing races with concurrent PR association.
func (s *Store) UpdatePRWatchBranchIfSearching(ctx context.Context, id, branch string) error {
_, err := s.db.ExecContext(ctx,
`UPDATE github_pr_watches SET branch = ?, updated_at = ? WHERE id = ? AND pr_number = 0`,
branch, time.Now().UTC(), id)
return err
}
// --- TaskPR operations ---
// CreateTaskPR associates a PR with a task.
@@ -62,6 +62,9 @@ func (s *Service) handleGitStatusUpdate(ctx context.Context, data watcher.GitEve
_ = s.eventBus.Publish(ctx, events.BuildGitWSEventSubject(data.SessionID), event)
}
// Update PR watch branch if the user changed branches (e.g. renamed)
s.syncPRWatchBranch(ctx, data.SessionID, data.Status.Branch)
// Push detection: when ahead goes from >0 to 0, a push happened
s.trackPushAndAssociatePR(ctx, data)
}
@@ -79,6 +82,10 @@ func (s *Service) trackPushAndAssociatePR(ctx context.Context, data watcher.GitE
}
// Push detected: ahead went from >0 to 0
if data.Status.Ahead == 0 && data.Status.RemoteBranch != "" {
s.logger.Info("git push detected, starting PR association",
zap.String("session_id", data.SessionID),
zap.String("task_id", data.TaskID),
zap.String("branch", data.Status.Branch))
go s.detectPushAndAssociatePR(
context.Background(),
data.SessionID,
@@ -88,6 +95,36 @@ func (s *Service) trackPushAndAssociatePR(ctx context.Context, data watcher.GitE
}
}
// syncPRWatchBranch updates the PR watch branch if the live git branch
// differs from what's stored (e.g. user renamed the branch).
// Only updates watches that haven't found a PR yet (pr_number=0).
func (s *Service) syncPRWatchBranch(ctx context.Context, sessionID, liveBranch string) {
if s.githubService == nil || liveBranch == "" {
return
}
watch, err := s.githubService.GetPRWatchBySession(ctx, sessionID)
if err != nil {
s.logger.Warn("failed to get PR watch for branch sync",
zap.String("session_id", sessionID),
zap.Error(err))
return
}
if watch == nil || watch.PRNumber != 0 {
return
}
if watch.Branch == liveBranch {
return
}
s.logger.Info("PR watch branch changed, updating from git status",
zap.String("session_id", sessionID),
zap.String("old_branch", watch.Branch),
zap.String("new_branch", liveBranch))
if updateErr := s.githubService.UpdatePRWatchBranchIfSearching(ctx, watch.ID, liveBranch); updateErr != nil {
s.logger.Error("failed to update PR watch branch",
zap.String("session_id", sessionID), zap.Error(updateErr))
}
}
// handleContextWindowUpdated handles context window updates and persists them to session metadata
func (s *Service) handleContextWindowUpdated(ctx context.Context, data watcher.ContextWindowData) {
s.logger.Debug("handling context window update",
@@ -25,6 +25,7 @@ type GitHubService interface {
EnsurePRWatch(ctx context.Context, sessionID, taskID, owner, repo, branch string) (*github.PRWatch, error)
GetPRWatchBySession(ctx context.Context, sessionID string) (*github.PRWatch, error)
UpdatePRWatchBranch(ctx context.Context, id, branch string) error
UpdatePRWatchBranchIfSearching(ctx context.Context, id, branch string) error
UpdatePRWatchPRNumber(ctx context.Context, id string, prNumber int) error
AssociatePRWithTask(ctx context.Context, taskID string, pr *github.PR) (*github.TaskPR, error)
GetTaskPR(ctx context.Context, taskID string) (*github.TaskPR, error)
@@ -323,9 +324,18 @@ func (s *Service) detectPushAndAssociatePR(
zap.Duration("delay", delay))
continue
}
s.logger.Info("PR found after push, associating with task",
zap.String("session_id", sessionID),
zap.String("task_id", taskID),
zap.Int("pr_number", foundPR.Number),
zap.String("branch", branch))
s.associatePRFromPush(ctx, sessionID, taskID, owner, repoName, branch, foundPR)
return
}
s.logger.Warn("exhausted all retries, no PR found after push",
zap.String("session_id", sessionID),
zap.String("task_id", taskID),
zap.String("branch", branch))
}
// searchPRForExistingWatch handles the case where a PR watch exists with pr_number=0
@@ -336,8 +346,9 @@ func (s *Service) searchPRForExistingWatch(
sessionID, taskID, branch string,
) {
// Update branch if the agent switched branches since the watch was created.
// Use the atomic variant to guard against a concurrent PR association.
if watch.Branch != branch {
if err := s.githubService.UpdatePRWatchBranch(ctx, watch.ID, branch); err != nil {
if err := s.githubService.UpdatePRWatchBranchIfSearching(ctx, watch.ID, branch); err != nil {
s.logger.Warn("failed to update PR watch branch",
zap.String("watch_id", watch.ID),
zap.String("old_branch", watch.Branch),
@@ -571,6 +582,12 @@ func (s *Service) ListTasksNeedingPRWatch(ctx context.Context) ([]github.TaskBra
return s.buildTaskBranchList(ctx, store)
}
// ResolveBranchForSession returns the current branch for a task+session.
// This is used by the poller to detect branch renames on existing PR watches.
func (s *Service) ResolveBranchForSession(ctx context.Context, taskID, sessionID string) string {
return s.resolvePRWatchBranch(ctx, taskID, sessionID, "")
}
// buildTaskBranchList queries sessions with branches, filters out those with
// existing PR watches, and resolves GitHub owner/repo for the remainder.
func (s *Service) buildTaskBranchList(ctx context.Context, store repoStore) ([]github.TaskBranchInfo, error) {
@@ -54,6 +54,11 @@ func (m *mockGitHubService) UpdatePRWatchBranch(_ context.Context, _, branch str
m.updatedBranch = branch
return nil
}
func (m *mockGitHubService) UpdatePRWatchBranchIfSearching(_ context.Context, _, branch string) error {
m.updateBranchCalls++
m.updatedBranch = branch
return nil
}
func (m *mockGitHubService) UpdatePRWatchPRNumber(_ context.Context, _ string, prNumber int) error {
m.updatePRNumberCalls++
m.updatedPRNumber = prNumber
+20 -20
View File
@@ -12,32 +12,19 @@ import { useAppStore } from "@/components/state-provider";
import { useSessionGitStatus } from "@/hooks/domains/session/use-session-git-status";
import { useSessionCommits } from "@/hooks/domains/session/use-session-commits";
import { useDockviewStore } from "@/lib/state/dockview-store";
import { focusOrAddPanel } from "@/lib/state/dockview-layout-builders";
import { cn } from "@kandev/ui/lib/utils";
/** Auto-activate the changes panel in the right sidebar, or quietly add to center if missing. */
/** Auto-activate the changes panel only when it lives in the right sidebar. */
function autoActivateChangesPanel(): void {
const { api, rightTopGroupId, centerGroupId } = useDockviewStore.getState();
const { api, rightTopGroupId } = useDockviewStore.getState();
if (!api) return;
const panel = api.getPanel("changes");
// Only auto-focus when the panel is in the right sidebar.
// When it's in the center group (e.g. plan mode layout), never steal focus
// from the active chat/session panel.
if (panel && panel.group.id === rightTopGroupId) {
panel.api.setActive();
return;
}
if (!panel) {
focusOrAddPanel(
api,
{
id: "changes",
component: "changes",
tabComponent: "changesTab",
title: "Changes",
position: { referenceGroup: centerGroupId },
},
true,
);
}
}
@@ -55,9 +42,16 @@ export function ChangesTab(props: IDockviewPanelHeaderProps) {
const fileCount = gitStatus?.files ? Object.keys(gitStatus.files).length : 0;
const totalCount = fileCount + commits.length;
// gitStatus is undefined until the first WS git-status event arrives,
// which marks the end of the initial data load for this session.
const gitStatusLoaded = gitStatus !== undefined;
const prevTotalRef = useRef(totalCount);
const seenCountRef = useRef(api.isActive ? totalCount : 0);
const flashTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
// Armed once we know the initial git data has settled. Until then, any
// 0→N transition is treated as an initial load, not a real new change.
const initializedRef = useRef(false);
const [isFlashing, setIsFlashing] = useState(false);
const [badgeCount, setBadgeCount] = useState(0);
@@ -85,7 +79,13 @@ export function ChangesTab(props: IDockviewPanelHeaderProps) {
const increased = totalCount > prev && totalCount > 0;
const decreased = totalCount < prev;
if (increased && prev === 0) {
// Auto-activate when changes appear for the first time (0 → N), but only
// after initial git data has settled. gitStatusLoaded is false until the
// first WS git-status event arrives, guaranteeing data has loaded before we
// arm auto-activate (handles both page-refresh and clean-session cases).
if (!initializedRef.current) {
if (gitStatusLoaded) initializedRef.current = true;
} else if (increased && prev === 0) {
autoActivateChangesPanel();
}
@@ -100,7 +100,7 @@ export function ChangesTab(props: IDockviewPanelHeaderProps) {
const unseen = Math.max(0, totalCount - seenCountRef.current);
requestAnimationFrame(() => setBadgeCount(unseen));
}
}, [totalCount, api]);
}, [totalCount, api, gitStatusLoaded]);
// Cleanup flash timer on unmount
useEffect(() => {
@@ -0,0 +1,174 @@
import { test, expect } from "../../fixtures/test-base";
import { SessionPage } from "../../pages/session-page";
import fs from "node:fs";
import path from "node:path";
import { execSync } from "node:child_process";
/** Minimal git helper for E2E tests - runs git commands in the test repository. */
class GitHelper {
constructor(
private repoDir: string,
private env: NodeJS.ProcessEnv,
) {}
exec(cmd: string): string {
return execSync(cmd, { cwd: this.repoDir, env: this.env, encoding: "utf8" });
}
createFile(name: string, content: string) {
fs.writeFileSync(path.join(this.repoDir, name), content);
}
stageAll() {
this.exec("git add -A");
}
commit(message: string) {
this.exec(`git commit -m "${message}"`);
}
}
test.describe("Changes panel focus behavior", () => {
/**
* Verifies the changes panel does NOT steal focus from the chat tab
* on page refresh when the task has existing git changes/commits.
*
* Root cause of the bug: the changes-tab component auto-activated the
* changes panel whenever totalCount went from 0 → N. On page refresh,
* hooks start with totalCount=0 (no data loaded), then async git data
* arrives making totalCount > 0 — triggering the auto-activate.
*/
test("changes panel does not auto-focus on page refresh", async ({
testPage,
apiClient,
seedData,
backend,
}) => {
test.setTimeout(90_000);
const repoDir = path.join(backend.tmpDir, "repos", "e2e-repo");
const gitEnv = {
...process.env,
HOME: backend.tmpDir,
GIT_AUTHOR_NAME: "E2E Test",
GIT_AUTHOR_EMAIL: "e2e@test.local",
GIT_COMMITTER_NAME: "E2E Test",
GIT_COMMITTER_EMAIL: "e2e@test.local",
};
const git = new GitHelper(repoDir, gitEnv);
// Create a task and wait for the agent to be ready
const task = await apiClient.createTaskWithAgent(
seedData.workspaceId,
"Focus test task",
seedData.agentProfileId,
{
workflow_id: seedData.workflowId,
workflow_step_id: seedData.startStepId,
repository_ids: [seedData.repositoryId],
},
);
await testPage.goto(`/t/${task.id}`);
const session = new SessionPage(testPage);
await session.waitForLoad();
await expect(session.idleInput()).toBeVisible({ timeout: 30_000 });
// Create a file and commit so there are existing changes
git.createFile("test-file.txt", "hello world");
git.stageAll();
git.commit("test commit");
// Wait for the changes panel to show the commit
await session.clickTab("Changes");
await expect(session.changes).toBeVisible({ timeout: 10_000 });
await expect(session.changes.getByText("test commit")).toBeVisible({ timeout: 10_000 });
// Switch back to chat tab — this is the tab that should be active after refresh
await session.clickSessionChatTab();
await expect(session.chat).toBeVisible({ timeout: 5_000 });
// Refresh the page
await testPage.reload();
await session.waitForLoad();
// Wait for the git data to load (changes tab should show count)
await expect(testPage.locator(".dv-default-tab:has-text('Changes')")).toBeVisible({
timeout: 15_000,
});
// The chat/session panel should be the active tab, NOT changes
const changesTab = testPage.locator(".dv-default-tab:has-text('Changes')");
await expect(changesTab).not.toHaveClass(/dv-active-tab/, { timeout: 5_000 });
// Chat should be visible (active in center group)
await expect(session.chat).toBeVisible({ timeout: 5_000 });
});
/**
* Verifies the changes panel does NOT auto-focus when it is in the center
* group (e.g. plan mode layout). Even when new changes appear, the chat
* panel should stay focused.
*/
test("changes panel does not auto-focus when in center group", async ({
testPage,
apiClient,
seedData,
backend,
}) => {
test.setTimeout(90_000);
const repoDir = path.join(backend.tmpDir, "repos", "e2e-repo");
const gitEnv = {
...process.env,
HOME: backend.tmpDir,
GIT_AUTHOR_NAME: "E2E Test",
GIT_AUTHOR_EMAIL: "e2e@test.local",
GIT_COMMITTER_NAME: "E2E Test",
GIT_COMMITTER_EMAIL: "e2e@test.local",
};
const git = new GitHelper(repoDir, gitEnv);
const task = await apiClient.createTaskWithAgent(
seedData.workspaceId,
"Center group focus test",
seedData.agentProfileId,
{
workflow_id: seedData.workflowId,
workflow_step_id: seedData.startStepId,
repository_ids: [seedData.repositoryId],
},
);
await testPage.goto(`/t/${task.id}`);
const session = new SessionPage(testPage);
await session.waitForLoad();
await expect(session.idleInput()).toBeVisible({ timeout: 30_000 });
// Toggle plan mode — this moves changes into the center group
await session.togglePlanMode();
await expect(session.planPanel).toBeVisible({ timeout: 10_000 });
await expect(session.chat).toBeVisible();
// Verify the chat/session tab is active, not changes
const changesTab = testPage.locator(".dv-default-tab:has-text('Changes')");
await expect(changesTab).not.toHaveClass(/dv-active-tab/, { timeout: 5_000 });
// Create new changes — this would previously trigger auto-activate
git.createFile("new-file.txt", "new content");
git.stageAll();
git.commit("new commit");
// Wait for the changes badge to update
await expect(testPage.locator(".dv-default-tab:has-text('Changes')")).toBeVisible({
timeout: 15_000,
});
// Wait a bit for any async auto-activate to fire
await testPage.waitForTimeout(2_000);
// Changes tab should NOT have stolen focus
await expect(changesTab).not.toHaveClass(/dv-active-tab/, { timeout: 5_000 });
// Chat should still be visible (active)
await expect(session.chat).toBeVisible();
});
});
+5 -1
View File
@@ -36,7 +36,11 @@ export function resolveGroupIds(api: DockviewApi): LayoutGroupIds {
return {
sidebarGroupId: findGroupId(api, SIDEBAR_GROUP, "sidebar"),
centerGroupId: findGroupId(api, CENTER_GROUP, "chat"),
rightTopGroupId: findGroupId(api, RIGHT_TOP_GROUP, "changes"),
// Always use the well-known constant — do NOT fall back to the "changes"
// panel's current group. In plan mode the "changes" panel moves into the
// center group; a panel-based fallback would return the center group ID and
// defeat the auto-focus guard in changes-tab.tsx.
rightTopGroupId: RIGHT_TOP_GROUP,
rightBottomGroupId: findGroupId(api, RIGHT_BOTTOM_GROUP, TERMINAL_DEFAULT_ID),
};
}