From ffda4638e922f0971fe33fc8d643e2c6afa94c1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Thu, 18 Jun 2026 22:28:56 +0100 Subject: [PATCH] fix(web): constrain ci checks popover height (#1441) --- .../orchestrator/prompt_idle_session_test.go | 88 +++++++++++++++++++ .../internal/orchestrator/task_operations.go | 12 +++ apps/web/components/github/pr-ci-popover.tsx | 3 + .../components/github/pr-status-chip.test.tsx | 22 +++++ apps/web/components/github/pr-status-chip.tsx | 6 +- .../components/github/pr-topbar-button.tsx | 6 +- .../e2e/tests/pr/pr-topbar-popover.spec.ts | 70 +++++++++++++++ 7 files changed, 201 insertions(+), 6 deletions(-) diff --git a/apps/backend/internal/orchestrator/prompt_idle_session_test.go b/apps/backend/internal/orchestrator/prompt_idle_session_test.go index 52486b4a25..cf2533b082 100644 --- a/apps/backend/internal/orchestrator/prompt_idle_session_test.go +++ b/apps/backend/internal/orchestrator/prompt_idle_session_test.go @@ -5,10 +5,14 @@ import ( "errors" "fmt" "strings" + "sync/atomic" "testing" "time" + "github.com/kandev/kandev/internal/agent/runtime/lifecycle" "github.com/kandev/kandev/internal/orchestrator/executor" + "github.com/kandev/kandev/internal/orchestrator/queue" + "github.com/kandev/kandev/internal/orchestrator/scheduler" "github.com/kandev/kandev/internal/task/models" v1 "github.com/kandev/kandev/pkg/api/v1" ) @@ -400,3 +404,87 @@ func TestEnsureSessionRunning_WaitsForPromptReadyWhenExecutionExists(t *testing. t.Fatal("ensureSessionRunning did not return after prompt readiness") } } + +func TestPromptTask_LazyResumeExecutionNotFoundFallsBackToFreshLaunch(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + + seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateIdle) + session, err := repo.GetTaskSession(ctx, "session1") + if err != nil { + t.Fatalf("failed to load session: %v", err) + } + session.AgentExecutionID = "exec-before-restart" + session.AgentProfileID = "profile1" + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("failed to update session: %v", err) + } + seedExecutorRunning(t, repo, session.ID, session.TaskID, "exec-before-restart") + + var launchCalls atomic.Int32 + launchPrompts := make(chan string, 2) + agentMgr := &mockAgentManager{ + repoForExecutionLookup: repo, + promptErr: lifecycle.ErrExecutionNotFound, + isAgentRunningFn: func(_ context.Context, _ string) bool { + return launchCalls.Load() > 0 + }, + isAgentReadyFn: func(_ context.Context, _ string) bool { + return launchCalls.Load() > 0 + }, + launchAgentFunc: func(_ context.Context, req *executor.LaunchAgentRequest) (*executor.LaunchAgentResponse, error) { + call := launchCalls.Add(1) + launchPrompts <- req.TaskDescription + if call == 1 { + go func(sessionID string) { + tick := time.NewTicker(5 * time.Millisecond) + defer tick.Stop() + timeout := time.After(5 * time.Second) + for { + select { + case <-tick.C: + sess, err := repo.GetTaskSession(context.Background(), sessionID) + if err == nil && sess != nil && sess.State == models.TaskSessionStateStarting { + sess.State = models.TaskSessionStateWaitingForInput + sess.UpdatedAt = time.Now().UTC() + _ = repo.UpdateTaskSession(context.Background(), sess) + return + } + case <-timeout: + return + } + } + }(req.SessionID) + } + return &executor.LaunchAgentResponse{AgentExecutionID: fmt.Sprintf("exec-resumed-%d", call)}, nil + }, + } + + taskRepo := newMockTaskRepo() + taskRepo.tasks["task1"] = &v1.Task{ + ID: "task1", + Title: "Test Task", + State: v1.TaskStateInProgress, + } + svc := createTestServiceWithAgent(repo, newMockStepGetter(), taskRepo, agentMgr) + exec := executor.NewExecutor(agentMgr, repo, testLogger(), executor.ExecutorConfig{}) + svc.executor = exec + svc.scheduler = scheduler.NewScheduler(queue.NewTaskQueue(100), exec, taskRepo, testLogger(), scheduler.DefaultSchedulerConfig()) + + prompt := "follow-up after restart" + if _, err := svc.PromptTask(ctx, "task1", "session1", prompt, "", false, nil, false); err != nil { + t.Fatalf("expected fresh-launch fallback to recover missing execution, got: %v", err) + } + + if got := launchCalls.Load(); got != 2 { + t.Fatalf("expected resume launch plus fresh fallback launch, got %d launches", got) + } + <-launchPrompts // resume prompt; empty for the lazy resume path. + freshPrompt := <-launchPrompts + if !strings.Contains(freshPrompt, prompt) { + t.Fatalf("fresh launch prompt %q does not contain original prompt %q", freshPrompt, prompt) + } + if len(agentMgr.capturedPromptCalls) != 1 { + t.Fatalf("expected one failed PromptAgent attempt before fallback, got %d", len(agentMgr.capturedPromptCalls)) + } +} diff --git a/apps/backend/internal/orchestrator/task_operations.go b/apps/backend/internal/orchestrator/task_operations.go index 9c65ccc20b..b46b1e667f 100644 --- a/apps/backend/internal/orchestrator/task_operations.go +++ b/apps/backend/internal/orchestrator/task_operations.go @@ -2095,6 +2095,8 @@ func (s *Service) PromptTask(ctx context.Context, taskID, sessionID string, prom // Ensure the agent process is actually running. After a lazy backend restart, // the session may be in WAITING_FOR_INPUT but no agent process exists yet. + _, hadExecutionBeforeEnsure := s.executor.GetExecutionBySession(sessionID) + resumedForPrompt := !hadExecutionBeforeEnsure if err := s.ensureSessionRunning(ctx, sessionID, session); err != nil { return nil, fmt.Errorf("failed to ensure session is running: %w", err) } @@ -2139,6 +2141,16 @@ func (s *Service) PromptTask(ctx context.Context, taskID, sessionID string, prom promptCtx := context.WithoutCancel(ctx) result, err := s.executor.Prompt(promptCtx, taskID, sessionID, effectivePrompt, attachments, dispatchOnly, session) if err != nil { + if resumedForPrompt && errors.Is(err, executor.ErrExecutionNotFound) { + s.logger.Warn("prompt after lazy resume hit missing execution; falling back to fresh launch", + zap.String("task_id", taskID), + zap.String("session_id", sessionID)) + if freshErr := s.fallbackFreshLaunchOnMissingExecution(ctx, taskID, sessionID, prompt, planMode, nil, attachments); freshErr == nil { + return &PromptResult{}, nil + } else { + err = freshErr + } + } return nil, s.handlePromptError(ctx, taskID, sessionID, previousSessionState, err) } return &PromptResult{ diff --git a/apps/web/components/github/pr-ci-popover.tsx b/apps/web/components/github/pr-ci-popover.tsx index d5d97a4829..7845aad676 100644 --- a/apps/web/components/github/pr-ci-popover.tsx +++ b/apps/web/components/github/pr-ci-popover.tsx @@ -36,6 +36,9 @@ type CountsView = { const CHECK_GROUP_ORDER: CheckBucket[] = ["passed", "in_progress", "failed"]; +export const PR_CI_DESKTOP_POPOVER_SCROLL_CLASS = + "max-h-[min(28rem,calc(100vh-8rem))] overflow-y-auto overscroll-contain"; + function countForBucket(counts: CountsView, kind: CheckBucket): number { if (kind === "passed") return counts.passed; if (kind === "in_progress") return counts.inProgress; diff --git a/apps/web/components/github/pr-status-chip.test.tsx b/apps/web/components/github/pr-status-chip.test.tsx index 532ede2c57..1b7595b437 100644 --- a/apps/web/components/github/pr-status-chip.test.tsx +++ b/apps/web/components/github/pr-status-chip.test.tsx @@ -5,6 +5,7 @@ import { TooltipProvider } from "@kandev/ui/tooltip"; import { StateProvider } from "@/components/state-provider"; import { ToastProvider } from "@/components/toast-provider"; import { PRStatusChip, aggregateChipStatus } from "./pr-status-chip"; +import { PR_CI_DESKTOP_POPOVER_SCROLL_CLASS } from "./pr-ci-popover"; import type { AppState } from "@/lib/state/store"; import type { TaskPR } from "@/lib/types/github"; @@ -90,6 +91,17 @@ function multiState(prs: TaskPR[]): Partial { return { taskPRs: { byTaskId: { "task-1": prs } } }; } +async function expectDesktopHoverPopoverConstrained() { + fireEvent.pointerEnter(screen.getByTestId(CHIP_TESTID)); + const inner = await screen.findByTestId("pr-topbar-popover-inner"); + const content = inner.closest(".overflow-y-auto"); + expect(content).not.toBeNull(); + const classNames = content!.className.split(/\s+/); + expect(classNames).toEqual( + expect.arrayContaining(PR_CI_DESKTOP_POPOVER_SCROLL_CLASS.split(/\s+/)), + ); +} + describe("PRStatusChip", () => { it("returns null when the task has no PR", () => { renderWithStore(undefined, ); @@ -127,6 +139,11 @@ describe("PRStatusChip", () => { expect(document.querySelector(DRAWER_SELECTOR)).toBeNull(); }); + it("constrains the hover popover to the available viewport height", async () => { + renderWithStore(seededState, ); + await expectDesktopHoverPopoverConstrained(); + }); + it("exposes the canonical data attributes that desktop tests rely on", () => { renderWithStore(seededState, ); const chip = screen.getByTestId(CHIP_TESTID); @@ -239,6 +256,11 @@ describe("PRStatusChip — multiple PRs", () => { expect(chip.getAttribute(ATTR_STATUS)).toBe("failed"); }); + it("constrains the aggregate hover popover to the available viewport height", async () => { + renderWithStore(multiState(TWO_OPEN), ); + await expectDesktopHoverPopoverConstrained(); + }); + it("stays visible while at least one PR is still open", () => { renderWithStore( multiState([makePR({ id: "a", state: "merged" }), makePR({ id: "b", pr_number: 2 })]), diff --git a/apps/web/components/github/pr-status-chip.tsx b/apps/web/components/github/pr-status-chip.tsx index 6010bf0bde..d1be9dbc8a 100644 --- a/apps/web/components/github/pr-status-chip.tsx +++ b/apps/web/components/github/pr-status-chip.tsx @@ -21,7 +21,7 @@ import { import { Button } from "@kandev/ui/button"; import { useTaskPR } from "@/hooks/domains/github/use-task-pr"; import { usePRFeedbackBackgroundSync } from "@/hooks/domains/github/use-pr-ci-popover"; -import { PRCIPopover } from "@/components/github/pr-ci-popover"; +import { PR_CI_DESKTOP_POPOVER_SCROLL_CLASS, PRCIPopover } from "@/components/github/pr-ci-popover"; import { MultiPRCIPopover } from "@/components/github/multi-pr-ci-popover"; import { isPRAwaitingReview, @@ -176,7 +176,7 @@ function PRStatusChipHoverCard({ pr }: { pr: TaskPR }) { side="top" align="start" sideOffset={8} - className="w-80 p-2.5" + className={`w-80 p-2.5 ${PR_CI_DESKTOP_POPOVER_SCROLL_CLASS}`} onPointerDownOutside={onPointerDownOutside} > @@ -233,7 +233,7 @@ function PRStatusChipMultiHoverCard({ prs }: { prs: TaskPR[] }) { side="top" align="start" sideOffset={8} - className="w-96 p-2.5" + className={`w-96 p-2.5 ${PR_CI_DESKTOP_POPOVER_SCROLL_CLASS}`} onPointerDownOutside={onPointerDownOutside} > diff --git a/apps/web/components/github/pr-topbar-button.tsx b/apps/web/components/github/pr-topbar-button.tsx index a4e4ef615b..f666747736 100644 --- a/apps/web/components/github/pr-topbar-button.tsx +++ b/apps/web/components/github/pr-topbar-button.tsx @@ -29,7 +29,7 @@ import { isPRReadyToMerge, } from "@/components/github/pr-task-icon"; import { prTaskKey } from "@/components/github/pr-detail-panel"; -import { PRCIPopover } from "@/components/github/pr-ci-popover"; +import { PR_CI_DESKTOP_POPOVER_SCROLL_CLASS, PRCIPopover } from "@/components/github/pr-ci-popover"; import { MultiPRCIPopover } from "@/components/github/multi-pr-ci-popover"; import { useAppStore } from "@/components/state-provider"; import type { TaskPR } from "@/lib/types/github"; @@ -186,7 +186,7 @@ function PRSingleButton({ pr }: { pr: TaskPR }) { data-testid="pr-topbar-popover" align="end" sideOffset={4} - className="w-80" + className={`w-80 ${PR_CI_DESKTOP_POPOVER_SCROLL_CLASS}`} onMouseEnter={handleEnter} onMouseLeave={handleLeave} onOpenAutoFocus={(e) => e.preventDefault()} @@ -258,7 +258,7 @@ function PRMultiButton({ prs }: { prs: TaskPR[] }) { data-testid="pr-topbar-popover" align="end" sideOffset={4} - className="w-96" + className={`w-96 ${PR_CI_DESKTOP_POPOVER_SCROLL_CLASS}`} onMouseEnter={handleEnter} onMouseLeave={handleLeave} onOpenAutoFocus={(e) => e.preventDefault()} diff --git a/apps/web/e2e/tests/pr/pr-topbar-popover.spec.ts b/apps/web/e2e/tests/pr/pr-topbar-popover.spec.ts index 5aa02593cd..bbd41ff791 100644 --- a/apps/web/e2e/tests/pr/pr-topbar-popover.spec.ts +++ b/apps/web/e2e/tests/pr/pr-topbar-popover.spec.ts @@ -2,6 +2,7 @@ import { test, expect } from "../../fixtures/test-base"; import { KanbanPage } from "../../pages/kanban-page"; import { SessionPage } from "../../pages/session-page"; import type { ApiClient } from "../../helpers/api-client"; +import type { Locator, Page } from "@playwright/test"; const OWNER = "acme"; const REPO = "demo"; @@ -90,6 +91,37 @@ async function associatePR( }); } +function manyRunningChecks(count: number) { + return Array.from({ length: count }, (_, index) => ({ + name: `E2E Shard ${index + 1}/${count} / run`, + status: "in_progress", + html_url: `https://example.com/checks/${index + 1}`, + })); +} + +async function expectScrollablePopoverWithinViewport(testPage: Page, locator: Locator) { + const metrics = await locator.evaluate((node) => { + const content = node.classList.contains("overflow-y-auto") + ? node + : node.closest(".overflow-y-auto"); + const rect = content?.getBoundingClientRect(); + return { + top: rect?.top ?? -1, + bottom: rect?.bottom ?? -1, + clientHeight: content?.clientHeight ?? 0, + scrollHeight: content?.scrollHeight ?? 0, + overflowY: content ? getComputedStyle(content).overflowY : "", + overscrollBehavior: content ? getComputedStyle(content).overscrollBehavior : "", + }; + }); + const viewportHeight = testPage.viewportSize()?.height ?? 0; + expect(metrics.top).toBeGreaterThanOrEqual(0); + expect(metrics.bottom).toBeLessThanOrEqual(viewportHeight); + expect(metrics.overflowY).toBe("auto"); + expect(metrics.overscrollBehavior).toBe("contain"); + expect(metrics.scrollHeight).toBeGreaterThan(metrics.clientHeight); +} + async function openTaskAndWait( testPage: import("@playwright/test").Page, apiClient: ApiClient, @@ -211,6 +243,44 @@ test.describe("PR top-bar CI popover", () => { await expect(session.prWorkflowRow("E2E")).toContainText("4/5 passed"); }); + test("desktop CI popovers scroll instead of overflowing the viewport", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + const title = "Many Running Checks"; + const seed = await seedTask( + apiClient, + seedData.workspaceId, + seedData.agentProfileId, + seedData.repositoryId, + title, + ); + await associatePR(apiClient, seed.taskId, { + checks_state: "pending", + checks_total: 30, + checks_passing: 0, + }); + await apiClient.mockGitHubSeedPRFeedback({ + owner: OWNER, + repo: REPO, + pr_number: PR_NUMBER, + checks: manyRunningChecks(30), + }); + const session = await openTaskAndWait(testPage, apiClient, seed, title); + + await expect(session.prStatusChip()).toBeVisible(); + await session.prStatusChip().hover(); + const popoverInner = testPage.getByTestId("pr-topbar-popover-inner"); + await expect(popoverInner).toBeVisible({ timeout: 10_000 }); + await expectScrollablePopoverWithinViewport(testPage, popoverInner); + + await testPage.mouse.move(0, 0); + await session.hoverPRTopbar(); + await expectScrollablePopoverWithinViewport(testPage, session.prTopbarPopover()); + }); + test("review row shows N / M required + unresolved comments count", async ({ testPage, apiClient,