From 9a1ab2018872e110e3b97839d1ff3a28e819b8ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Sat, 8 Aug 2026 00:07:08 +0200 Subject: [PATCH] =?UTF-8?q?feat(github):=20show=20an=20Updating=E2=80=A6?= =?UTF-8?q?=20indicator=20in=20the=20PR=20status=20popover=20(#2402)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kandev Agent --- .../components/github/multi-pr-ci-popover.tsx | 2 +- .../github/pr-ci-popover.automation.test.tsx | 1 + .../github/pr-ci-popover.footer.test.tsx | 139 +++++++++++ apps/web/components/github/pr-ci-popover.tsx | 42 +++- apps/web/components/github/pr-status-chip.tsx | 4 +- .../components/github/pr-topbar-button.tsx | 4 +- apps/web/e2e/pages/session-page.ts | 5 + .../e2e/tests/pr/pr-topbar-popover.spec.ts | 59 +++++ .../domains/github/use-pr-ci-popover.test.ts | 229 ++++++++++++++++++ .../hooks/domains/github/use-pr-ci-popover.ts | 61 ++++- apps/web/hooks/domains/github/use-task-pr.ts | 13 +- .../hooks/use-min-visible-duration.test.ts | 88 +++++++ apps/web/hooks/use-min-visible-duration.ts | 44 ++++ apps/web/src/locales/en/github.json | 1 + apps/web/src/locales/pseudo/github.json | 1 + apps/web/src/locales/pt-pt/github.json | 1 + apps/web/src/locales/zh-cn/github.json | 1 + 17 files changed, 671 insertions(+), 24 deletions(-) create mode 100644 apps/web/components/github/pr-ci-popover.footer.test.tsx create mode 100644 apps/web/hooks/domains/github/use-pr-ci-popover.test.ts create mode 100644 apps/web/hooks/use-min-visible-duration.test.ts create mode 100644 apps/web/hooks/use-min-visible-duration.ts diff --git a/apps/web/components/github/multi-pr-ci-popover.tsx b/apps/web/components/github/multi-pr-ci-popover.tsx index ad7756899e..71659d4cae 100644 --- a/apps/web/components/github/multi-pr-ci-popover.tsx +++ b/apps/web/components/github/multi-pr-ci-popover.tsx @@ -122,7 +122,7 @@ export function MultiPRCIPopover({ prs: TaskPR[]; enabled: boolean; onOpenDetailPanel?: (pr: TaskPR) => void; - refreshTaskPR?: () => void; + refreshTaskPR?: () => void | Promise; onRemovePR?: (pr: TaskPR) => Promise; onCollapseFocus?: (remainingPR: TaskPR) => void; }) { diff --git a/apps/web/components/github/pr-ci-popover.automation.test.tsx b/apps/web/components/github/pr-ci-popover.automation.test.tsx index 7260bd60e8..247626f555 100644 --- a/apps/web/components/github/pr-ci-popover.automation.test.tsx +++ b/apps/web/components/github/pr-ci-popover.automation.test.tsx @@ -31,6 +31,7 @@ vi.mock("@/hooks/domains/github/use-pr-ci-popover", () => ({ usePRCIPopover: () => ({ feedback: null, isFetching: false, + isRefreshing: false, lastUpdatedAt: null, refetch: vi.fn(), }), diff --git a/apps/web/components/github/pr-ci-popover.footer.test.tsx b/apps/web/components/github/pr-ci-popover.footer.test.tsx new file mode 100644 index 0000000000..8e50fdc762 --- /dev/null +++ b/apps/web/components/github/pr-ci-popover.footer.test.tsx @@ -0,0 +1,139 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { cleanup, render, screen } from "@testing-library/react"; +import { TooltipProvider } from "@kandev/ui/tooltip"; +import { StateProvider } from "@/components/state-provider"; +import { ToastProvider } from "@/components/toast-provider"; +import type { TaskPR } from "@/lib/types/github"; + +const popoverState = vi.hoisted(() => ({ + isFetching: false, + isRefreshing: false, + lastUpdatedAt: null as number | null, +})); + +vi.mock("@/hooks/domains/github/use-pr-ci-popover", () => ({ + usePRFeedbackBackgroundSync: vi.fn(), + usePRCIPopover: () => ({ + feedback: null, + isFetching: popoverState.isFetching, + isRefreshing: popoverState.isRefreshing, + lastUpdatedAt: popoverState.lastUpdatedAt, + refetch: vi.fn(), + }), +})); + +vi.mock("@/hooks/domains/github/use-task-ci-options", () => ({ + useTaskCIAutomationOptions: () => ({ + options: null, + loading: false, + saving: false, + error: null, + refresh: vi.fn(), + update: vi.fn(), + resetPrompt: vi.fn(), + }), +})); + +import { PRCIPopover } from "./pr-ci-popover"; + +const UPDATING_LABEL = "Updating…"; + +function makePR(): TaskPR { + return { + id: "id", + task_id: "task-1", + owner: "acme", + repo: "app", + pr_number: 7, + pr_url: "https://github.com/acme/app/pull/7", + pr_title: "Test PR", + head_branch: "feat", + base_branch: "main", + author_login: "alice", + state: "open", + review_state: "", + checks_state: "success", + mergeable_state: "clean", + review_count: 0, + pending_review_count: 0, + comment_count: 0, + unresolved_review_threads: 0, + checks_total: 1, + checks_passing: 1, + additions: 0, + deletions: 0, + created_at: "", + merged_at: null, + closed_at: null, + last_synced_at: null, + updated_at: "", + }; +} + +function renderPopover() { + return render( + + + + + + + , + ); +} + +describe("PR popover footer refresh indicator", () => { + beforeEach(() => { + popoverState.isFetching = false; + popoverState.isRefreshing = false; + popoverState.lastUpdatedAt = null; + }); + + afterEach(() => { + cleanup(); + }); + + it("shows a spinner and 'Updating…' instead of a freshness claim while refreshing", () => { + popoverState.isRefreshing = true; + popoverState.lastUpdatedAt = Date.now(); + renderPopover(); + + const updating = screen.getByTestId("pr-popover-updating"); + expect(updating.textContent).toContain(UPDATING_LABEL); + expect(updating.querySelector(".animate-spin")).not.toBeNull(); + expect(screen.queryByTestId("pr-popover-updated-at")).toBeNull(); + }); + + it("announces the refresh to assistive tech", () => { + popoverState.isRefreshing = true; + popoverState.lastUpdatedAt = Date.now(); + renderPopover(); + + expect(screen.getByRole("status").textContent).toContain(UPDATING_LABEL); + }); + + it("shows the elapsed label once the refresh settles", () => { + popoverState.isRefreshing = false; + popoverState.lastUpdatedAt = Date.now(); + renderPopover(); + + expect(screen.getByTestId("pr-popover-updated-at").textContent).toBe("updated just now"); + expect(screen.queryByTestId("pr-popover-updating")).toBeNull(); + }); + + it("shows the indicator on a first open, when there is no cached timestamp yet", () => { + popoverState.isRefreshing = true; + popoverState.lastUpdatedAt = null; + renderPopover(); + + expect(screen.getByTestId("pr-popover-updating").textContent).toContain(UPDATING_LABEL); + }); + + it("renders no footer at all when there is nothing to report", () => { + popoverState.isRefreshing = false; + popoverState.lastUpdatedAt = null; + renderPopover(); + + expect(screen.queryByTestId("pr-popover-footer")).toBeNull(); + }); +}); diff --git a/apps/web/components/github/pr-ci-popover.tsx b/apps/web/components/github/pr-ci-popover.tsx index 2232b8cba8..994a641a12 100644 --- a/apps/web/components/github/pr-ci-popover.tsx +++ b/apps/web/components/github/pr-ci-popover.tsx @@ -8,6 +8,7 @@ import { IconCircleX, IconExternalLink, IconGitPullRequest, + IconLoader2, IconMessageCircle, IconPlus, } from "@tabler/icons-react"; @@ -493,7 +494,13 @@ function elapsedLabel( return t("github:updatedAgo", { elapsed: formatElapsedShort(elapsed) }); } -function PRPopoverFooter({ lastUpdatedAt }: { lastUpdatedAt: number | null }) { +function PRPopoverFooter({ + lastUpdatedAt, + isRefreshing, +}: { + lastUpdatedAt: number | null; + isRefreshing: boolean; +}) { const { t } = useTranslation(); // Capture a "now" snapshot only inside the setInterval callback (an event // handler — no impurity in render, no setState during commit). When no @@ -509,18 +516,31 @@ function PRPopoverFooter({ lastUpdatedAt }: { lastUpdatedAt: number | null }) { lastUpdatedAt == null ? null : Math.max(0, Math.floor(((now ?? lastUpdatedAt) - lastUpdatedAt) / 1000)); - if (lastUpdatedAt == null) return null; + // The first open of a PR has no cache entry yet — that's the slowest fetch of + // all, so the footer has to exist for the spinner even with nothing to date. + if (lastUpdatedAt == null && !isRefreshing) return null; return (
- - {elapsedLabel(elapsed, t)} - + {isRefreshing ? ( + + + ) : ( + + {elapsedLabel(elapsed, t)} + + )}
); } @@ -561,12 +581,12 @@ export function PRCIPopover({ pr: TaskPR; enabled: boolean; onOpenDetailPanel?: () => void; - refreshTaskPR?: () => void; + refreshTaskPR?: () => void | Promise; }) { const workspaceId = useAppStore((s) => s.workspaces.activeId); const { status: ghStatus } = useGitHubStatus(workspaceId); const authLost = ghStatus !== null && !ghStatus.authenticated; - const { feedback, isFetching, lastUpdatedAt, refetch } = usePRCIPopover( + const { feedback, isFetching, isRefreshing, lastUpdatedAt, refetch } = usePRCIPopover( workspaceId, pr, enabled && !authLost, @@ -600,7 +620,7 @@ export function PRCIPopover({ )} - + ); } diff --git a/apps/web/components/github/pr-status-chip.tsx b/apps/web/components/github/pr-status-chip.tsx index bdd8721433..5ba68919ef 100644 --- a/apps/web/components/github/pr-status-chip.tsx +++ b/apps/web/components/github/pr-status-chip.tsx @@ -68,14 +68,14 @@ type TriggerRef = { current: HTMLButtonElement | null }; type SingleChipProps = { pr: TaskPR; automation: AutomationFlags; - refreshTaskPR: () => void; + refreshTaskPR: () => void | Promise; triggerRef?: TriggerRef; }; type MultiChipProps = { prs: TaskPR[]; statusPrs?: TaskPR[]; automation: AutomationFlags; - refreshTaskPR: () => void; + refreshTaskPR: () => void | Promise; onRemovePR?: (pr: TaskPR) => Promise; triggerRef?: TriggerRef; }; diff --git a/apps/web/components/github/pr-topbar-button.tsx b/apps/web/components/github/pr-topbar-button.tsx index 376eda0cc6..82163276be 100644 --- a/apps/web/components/github/pr-topbar-button.tsx +++ b/apps/web/components/github/pr-topbar-button.tsx @@ -145,7 +145,7 @@ function PRSingleButton({ triggerRef, }: { pr: TaskPR; - refreshTaskPR: () => void; + refreshTaskPR: () => void | Promise; triggerRef?: TriggerRef; }) { const addPRPanel = useDockviewStore((s) => s.addPRPanel); @@ -229,7 +229,7 @@ function PRMultiButton({ triggerRef, }: { prs: TaskPR[]; - refreshTaskPR: () => void; + refreshTaskPR: () => void | Promise; onRemovePR: (associationId: string) => Promise; triggerRef?: TriggerRef; }) { diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index 62df0cbbf3..5fe8840359 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -773,6 +773,11 @@ export class SessionPage { return this.prTopbarPopover().getByTestId("pr-popover-updated-at"); } + /** Footer spinner + "Updating…", shown while a refresh is in flight. */ + prPopoverUpdating(): Locator { + return this.prTopbarPopover().getByTestId("pr-popover-updating"); + } + /** Empty-state row when the PR has no checks yet. */ prChecksEmpty(): Locator { return this.prTopbarPopover().getByTestId("pr-checks-empty"); 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 6cad183899..5368133cc0 100644 --- a/apps/web/e2e/tests/pr/pr-topbar-popover.spec.ts +++ b/apps/web/e2e/tests/pr/pr-topbar-popover.spec.ts @@ -609,4 +609,63 @@ test.describe("PR top-bar CI popover", () => { }); await expect(session.prCheckGroupCount("passed")).toHaveText("25", { timeout: 5_000 }); }); + + test("footer reports the refresh instead of claiming the data is fresh", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + const title = "Refresh Indicator"; + const seed = await seedTask( + apiClient, + seedData.workspaceId, + seedData.agentProfileId, + seedData.repositoryId, + title, + ); + await associatePR(apiClient, seed.taskId, { + checks_state: "success", + checks_total: 3, + checks_passing: 3, + }); + // Feedback must actually resolve to something cacheable, or the footer has + // no timestamp to fall back to once the refresh settles. + await apiClient.mockGitHubSeedPRFeedback({ + owner: OWNER, + repo: REPO, + pr_number: PR_NUMBER, + checks: [ + { name: "CI / build", status: "completed", conclusion: "success", html_url: "" }, + { name: "CI / lint", status: "completed", conclusion: "success", html_url: "" }, + { name: "CI / test", status: "completed", conclusion: "success", html_url: "" }, + ], + }); + + // Hold the PRFeedback fetch open so the indicator is observable. Asserting + // on a real refresh would race the response, which usually lands in well + // under a frame against the mock backend. + let releaseFeedback: () => void = () => {}; + const feedbackHeld = new Promise((resolve) => { + releaseFeedback = resolve; + }); + await testPage.route( + (url) => /\/api\/v1\/github\/prs\/[^/]+\/[^/]+\/\d+$/.test(url.pathname), + async (route) => { + await feedbackHeld; + await route.continue(); + }, + ); + + const session = await openTaskAndWait(testPage, seed, title); + await session.hoverPRTopbar(); + await session.prTopbarPopover().hover(); + + await expect(session.prPopoverUpdating()).toBeVisible({ timeout: 10_000 }); + await expect(session.prPopoverUpdatedAt()).toHaveCount(0); + + releaseFeedback(); + await expect(session.prPopoverUpdatedAt()).toBeVisible({ timeout: 10_000 }); + await expect(session.prPopoverUpdating()).toHaveCount(0); + }); }); diff --git a/apps/web/hooks/domains/github/use-pr-ci-popover.test.ts b/apps/web/hooks/domains/github/use-pr-ci-popover.test.ts new file mode 100644 index 0000000000..533800e609 --- /dev/null +++ b/apps/web/hooks/domains/github/use-pr-ci-popover.test.ts @@ -0,0 +1,229 @@ +import { act, renderHook, waitFor } from "@testing-library/react"; +import { createElement, type ReactNode } from "react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { StateProvider } from "@/components/state-provider"; +import type { TaskPR } from "@/lib/types/github"; +import { usePRCIPopover } from "./use-pr-ci-popover"; + +const getPRFeedbackMock = vi.hoisted(() => vi.fn()); +vi.mock("@/lib/api/domains/github-api", () => ({ + getPRFeedback: getPRFeedbackMock, +})); + +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise; + }); + return { promise, resolve }; +} + +function makePR(): TaskPR { + return { + id: "id", + task_id: "task-1", + owner: "acme", + repo: "app", + pr_number: 7, + pr_url: "https://github.com/acme/app/pull/7", + pr_title: "Test PR", + head_branch: "feat", + base_branch: "main", + author_login: "alice", + state: "open", + review_state: "", + checks_state: "success", + mergeable_state: "clean", + review_count: 0, + pending_review_count: 0, + comment_count: 0, + unresolved_review_threads: 0, + checks_total: 1, + checks_passing: 1, + additions: 0, + deletions: 0, + created_at: "", + merged_at: null, + closed_at: null, + last_synced_at: null, + updated_at: "", + }; +} + +function wrapper({ children }: { children: ReactNode }) { + return createElement(StateProvider, null, children); +} + +function renderPopoverHook(refreshTaskPR?: () => void | Promise) { + return renderHook( + ({ enabled }: { enabled: boolean }) => + usePRCIPopover("workspace-1", makePR(), enabled, refreshTaskPR), + { initialProps: { enabled: false }, wrapper }, + ); +} + +/** Lets the open-transition `queueMicrotask` fire and its state land. */ +async function flushOpen() { + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); +} + +describe("usePRCIPopover refresh indicator", () => { + beforeEach(() => { + getPRFeedbackMock.mockReset(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("is idle before the popover opens", () => { + getPRFeedbackMock.mockResolvedValue(null); + const { result } = renderPopoverHook(); + expect(result.current.isRefreshing).toBe(false); + expect(getPRFeedbackMock).not.toHaveBeenCalled(); + }); + + it("reports refreshing while the feedback fetch is in flight", async () => { + const feedback = deferred(); + getPRFeedbackMock.mockReturnValue(feedback.promise); + + const { result, rerender } = renderPopoverHook(); + rerender({ enabled: true }); + await flushOpen(); + + expect(result.current.isRefreshing).toBe(true); + + await act(async () => { + feedback.resolve(null); + await feedback.promise; + }); + + await waitFor(() => expect(result.current.isRefreshing).toBe(false)); + }); + + it("stays refreshing until the TaskPR summary sync settles too", async () => { + getPRFeedbackMock.mockResolvedValue(null); + const sync = deferred(); + const refreshTaskPR = vi.fn(() => sync.promise); + + const { result, rerender } = renderPopoverHook(refreshTaskPR); + rerender({ enabled: true }); + await flushOpen(); + + // The feedback fetch has already resolved; the summary sync has not. + await waitFor(() => expect(result.current.isFetching).toBe(false)); + expect(result.current.isRefreshing).toBe(true); + + await act(async () => { + sync.resolve(); + await sync.promise; + }); + + await waitFor(() => expect(result.current.isRefreshing).toBe(false)); + }); + + it("does not hang when the refresh callback returns void", async () => { + getPRFeedbackMock.mockResolvedValue(null); + const refreshTaskPR = vi.fn(() => undefined); + + const { result, rerender } = renderPopoverHook(refreshTaskPR); + rerender({ enabled: true }); + await flushOpen(); + + expect(refreshTaskPR).toHaveBeenCalled(); + await waitFor(() => expect(result.current.isRefreshing).toBe(false)); + }); + + it("clears the indicator when the summary sync rejects", async () => { + getPRFeedbackMock.mockResolvedValue(null); + const refreshTaskPR = vi.fn(() => Promise.reject(new Error("socket down"))); + + const { result, rerender } = renderPopoverHook(refreshTaskPR); + rerender({ enabled: true }); + await flushOpen(); + + await waitFor(() => expect(result.current.isRefreshing).toBe(false)); + }); +}); + +// Split from the block above only to stay under the 100-line function limit. +describe("usePRCIPopover refresh indicator timing", () => { + beforeEach(() => { + getPRFeedbackMock.mockReset(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("ignores a stale sync settling after the popover was reopened", async () => { + vi.useFakeTimers(); + getPRFeedbackMock.mockResolvedValue(null); + const first = deferred(); + const second = deferred(); + const refreshTaskPR = vi + .fn<() => Promise>() + .mockReturnValueOnce(first.promise) + .mockReturnValueOnce(second.promise); + + const { result, rerender } = renderHook( + ({ enabled }: { enabled: boolean }) => + usePRCIPopover("workspace-1", makePR(), enabled, refreshTaskPR), + { initialProps: { enabled: false }, wrapper }, + ); + rerender({ enabled: true }); + await flushOpen(); + rerender({ enabled: false }); + rerender({ enabled: true }); + await flushOpen(); + expect(refreshTaskPR).toHaveBeenCalledTimes(2); + + // The first open's sync lands late; the second is still in flight. + await act(async () => { + first.resolve(); + await first.promise; + }); + // Drain the minimum-visible window first — it would otherwise hold the + // indicator up on its own and hide a wrongly-cleared flag. + await act(async () => { + vi.advanceTimersByTime(2_000); + }); + expect(result.current.isRefreshing).toBe(true); + + await act(async () => { + second.resolve(); + await second.promise; + }); + await act(async () => { + vi.advanceTimersByTime(2_000); + }); + expect(result.current.isRefreshing).toBe(false); + }); + + it("keeps the indicator up long enough to read a cache-fast refresh", async () => { + vi.useFakeTimers(); + getPRFeedbackMock.mockResolvedValue(null); + + const { result, rerender } = renderPopoverHook(); + rerender({ enabled: true }); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + + expect(result.current.isRefreshing).toBe(true); + // Both sources have settled, but the minimum visible window has not. + await act(async () => { + vi.advanceTimersByTime(100); + }); + expect(result.current.isRefreshing).toBe(true); + + await act(async () => { + vi.advanceTimersByTime(400); + }); + expect(result.current.isRefreshing).toBe(false); + }); +}); diff --git a/apps/web/hooks/domains/github/use-pr-ci-popover.ts b/apps/web/hooks/domains/github/use-pr-ci-popover.ts index 56dfb82f5e..d9ecd4a44f 100644 --- a/apps/web/hooks/domains/github/use-pr-ci-popover.ts +++ b/apps/web/hooks/domains/github/use-pr-ci-popover.ts @@ -3,8 +3,15 @@ import { useCallback, useEffect, useRef, useState } from "react"; import { getPRFeedback } from "@/lib/api/domains/github-api"; import { useAppStore } from "@/components/state-provider"; +import { useMinVisibleDuration } from "@/hooks/use-min-visible-duration"; import type { PRFeedback, TaskPR } from "@/lib/types/github"; +/** + * How long the "Updating…" footer stays up once a refresh starts. Long enough + * to read, short enough that it never feels like the popover is stuck. + */ +export const PR_REFRESH_INDICATOR_MIN_MS = 450; + export function prFeedbackKey(pr: { owner: string; repo: string; pr_number: number }): string { return `${pr.owner}/${pr.repo}#${pr.pr_number}`; } @@ -14,6 +21,12 @@ type Result = { feedback: PRFeedback | null; /** True while a fetch is in flight. Drives skeleton loading in PRCheckGroup. */ isFetching: boolean; + /** + * True while *any* part of the popover's own refresh cycle is in flight — + * the PRFeedback fetch and the TaskPR summary sync, which land separately. + * Held for a minimum duration so it can actually be read. Drives the footer. + */ + isRefreshing: boolean; /** Wallclock ms when the cache entry was last updated. */ lastUpdatedAt: number | null; /** Trigger a refetch immediately (used as a hover-open safety net). */ @@ -86,11 +99,13 @@ export function usePRCIPopover( workspaceId: string | null, pr: TaskPR | null, enabled: boolean, - refreshTaskPR?: () => void, + refreshTaskPR?: () => void | Promise, ): Result { const key = pr ? prFeedbackKey(pr) : null; const cached = useAppStore((state) => (key ? (state.prFeedbackCache.byKey[key] ?? null) : null)); const { refetch, isFetching } = useFeedbackFetch(workspaceId, pr); + const { isSyncing, trackSync } = useTaskPRSyncTracker(); + const isRefreshing = useMinVisibleDuration(isFetching || isSyncing, PR_REFRESH_INDICATOR_MIN_MS); const wasEnabledRef = useRef(false); useEffect(() => { @@ -99,14 +114,54 @@ export function usePRCIPopover( if (!opened) return; queueMicrotask(() => { refetch(); - refreshTaskPR?.(); + trackSync(refreshTaskPR?.()); }); - }, [enabled, refetch, refreshTaskPR]); + }, [enabled, refetch, refreshTaskPR, trackSync]); return { feedback: cached?.feedback ?? null, isFetching, + isRefreshing, lastUpdatedAt: cached?.lastUpdatedAt ?? null, refetch, }; } + +/** + * Tracks whether the TaskPR summary sync is still in flight. That sync is owned + * by `useTaskPR` and reaches the popover as an opaque callback, so the only + * handle on it is the promise it returns — older call sites return `void`, in + * which case there is nothing to wait for and the tracker stays idle. + */ +function useTaskPRSyncTracker(): { + isSyncing: boolean; + trackSync: (result: void | Promise) => void; +} { + const [isSyncing, setIsSyncing] = useState(false); + // Only the newest sync may settle the flag: reopening the popover mid-flight + // starts a second one, and the first landing must not clear the second's. + const syncRef = useRef(0); + const mountedRef = useRef(true); + useEffect(() => { + mountedRef.current = true; + return () => { + mountedRef.current = false; + }; + }, []); + + const trackSync = useCallback((result: void | Promise) => { + if (!result) return; + const syncId = ++syncRef.current; + setIsSyncing(true); + const settle = () => { + if (!mountedRef.current || syncRef.current !== syncId) return; + setIsSyncing(false); + }; + // `then(settle, settle)` rather than `finally`, which would re-throw a + // rejection into an unhandled promise. A failed sync is not this hook's + // to report — it just means the summary stayed stale. + void result.then(settle, settle); + }, []); + + return { isSyncing, trackSync }; +} diff --git a/apps/web/hooks/domains/github/use-task-pr.ts b/apps/web/hooks/domains/github/use-task-pr.ts index 936e284a5b..bec0c1d55b 100644 --- a/apps/web/hooks/domains/github/use-task-pr.ts +++ b/apps/web/hooks/domains/github/use-task-pr.ts @@ -98,10 +98,13 @@ export function useTaskPR(taskId: string | null) { // call from the previous task can race that reset. const requestRef = useRef(0); - const refresh = useCallback(() => { - if (!taskId) return; + // Resolves when the sync settles (or immediately when it can't be sent), so + // callers can show an "updating" affordance for exactly as long as the + // summary is actually stale. Never rejects — failures are absorbed below. + const refresh = useCallback((): Promise => { + if (!taskId) return Promise.resolve(); const client = getWebSocketClient(); - if (!client) return; + if (!client) return Promise.resolve(); // Backend returns `{prs: TaskPR[], permanent?: boolean}` — multi-repo // tasks have one row per repo. We push each into the store so the @@ -112,7 +115,7 @@ export function useTaskPR(taskId: string | null) { // exhaust the retry counter so the 5s interval below clears itself. const requestId = ++requestRef.current; const requestedTaskId = taskId; - client + return client .request("github.task_pr.sync", { task_id: requestedTaskId }) .then((result) => { // Drop responses that aren't the latest in-flight request for @@ -226,7 +229,7 @@ export function useTaskPR(taskId: string | null) { } as { pr: TaskPR | null; prs: TaskPR[]; - refresh: () => void; + refresh: () => Promise; unlink: (associationId: string) => Promise; loaded: boolean; }; diff --git a/apps/web/hooks/use-min-visible-duration.test.ts b/apps/web/hooks/use-min-visible-duration.test.ts new file mode 100644 index 0000000000..35550cb6a6 --- /dev/null +++ b/apps/web/hooks/use-min-visible-duration.test.ts @@ -0,0 +1,88 @@ +import { act, renderHook } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useMinVisibleDuration } from "./use-min-visible-duration"; + +describe("useMinVisibleDuration", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("turns on immediately", () => { + const { result, rerender } = renderHook(({ active }) => useMinVisibleDuration(active, 450), { + initialProps: { active: false }, + }); + expect(result.current).toBe(false); + + rerender({ active: true }); + expect(result.current).toBe(true); + }); + + it("holds on for the minimum duration when the flag clears immediately", () => { + const { result, rerender } = renderHook(({ active }) => useMinVisibleDuration(active, 450), { + initialProps: { active: true }, + }); + + rerender({ active: false }); + expect(result.current).toBe(true); + + act(() => { + vi.advanceTimersByTime(449); + }); + expect(result.current).toBe(true); + + act(() => { + vi.advanceTimersByTime(1); + }); + expect(result.current).toBe(false); + }); + + it("clears without extra delay once the minimum has already elapsed", () => { + const { result, rerender } = renderHook(({ active }) => useMinVisibleDuration(active, 450), { + initialProps: { active: true }, + }); + + act(() => { + vi.advanceTimersByTime(600); + }); + expect(result.current).toBe(true); + + rerender({ active: false }); + expect(result.current).toBe(false); + }); + + it("restarts the window when it reactivates during the tail", () => { + const { result, rerender } = renderHook(({ active }) => useMinVisibleDuration(active, 450), { + initialProps: { active: true }, + }); + + rerender({ active: false }); + act(() => { + vi.advanceTimersByTime(400); + }); + rerender({ active: true }); + rerender({ active: false }); + + // The pending 50ms tail from the first window must not settle the second. + act(() => { + vi.advanceTimersByTime(100); + }); + expect(result.current).toBe(true); + + act(() => { + vi.advanceTimersByTime(350); + }); + expect(result.current).toBe(false); + }); + + it("stays off while the flag is never set", () => { + const { result } = renderHook(() => useMinVisibleDuration(false, 450)); + act(() => { + vi.advanceTimersByTime(2000); + }); + expect(result.current).toBe(false); + }); +}); diff --git a/apps/web/hooks/use-min-visible-duration.ts b/apps/web/hooks/use-min-visible-duration.ts new file mode 100644 index 0000000000..f369051818 --- /dev/null +++ b/apps/web/hooks/use-min-visible-duration.ts @@ -0,0 +1,44 @@ +"use client"; + +import { useEffect, useRef, useState } from "react"; + +/** + * Holds a transient "busy" flag on for at least `minMs` once it turns on. + * + * A refresh that resolves from cache can settle in well under a frame, which + * makes a spinner appear and vanish as a flicker — strictly worse than showing + * nothing, because the user registers movement without reading it. Stretching + * the tail keeps the indicator legible; it never delays the data itself, only + * the disappearance of the indicator. + * + * Turning on is always immediate. Re-activating while the tail is still running + * restarts the window from that moment. + */ +export function useMinVisibleDuration(active: boolean, minMs: number): boolean { + const [visible, setVisible] = useState(active); + const shownAtRef = useRef(active ? Date.now() : null); + + // Stamped in its own effect, keyed only on `active`. Folded into the effect + // below it would restamp on the re-render that `setVisible(true)` causes — + // `visible` is one of that effect's deps — starting the window a render late. + useEffect(() => { + if (active) shownAtRef.current = Date.now(); + }, [active]); + + useEffect(() => { + if (active) { + setVisible(true); + return; + } + if (!visible) return; + const remaining = minMs - (Date.now() - (shownAtRef.current ?? 0)); + if (remaining <= 0) { + setVisible(false); + return; + } + const timer = setTimeout(() => setVisible(false), remaining); + return () => clearTimeout(timer); + }, [active, visible, minMs]); + + return visible; +} diff --git a/apps/web/src/locales/en/github.json b/apps/web/src/locales/en/github.json index 49c70fd673..2c80b0cb75 100644 --- a/apps/web/src/locales/en/github.json +++ b/apps/web/src/locales/en/github.json @@ -520,6 +520,7 @@ "updated": "Updated ", "updatedAgo": "updated {{elapsed}} ago", "updatedJustNow": "updated just now", + "updatingStatus": "Updating…", "useAnAppWhenAutomationNeeds": "Use an App when automation needs its own identity, short-lived tokens, and repository-level installation control. Setup is more involved and requires a publicly reachable HTTPS URL.", "useCommaSeparatedOwnerRepoValues": "Use comma-separated owner/repo values.", "useDefault": "Use default", diff --git a/apps/web/src/locales/pseudo/github.json b/apps/web/src/locales/pseudo/github.json index 1847a7dae9..7cd6c4df1b 100644 --- a/apps/web/src/locales/pseudo/github.json +++ b/apps/web/src/locales/pseudo/github.json @@ -520,6 +520,7 @@ "updated": "Ũƥďàţēď ", "updatedAgo": "ũƥďàţēď {{elapsed}} àĝō", "updatedJustNow": "ũƥďàţēď ĵũśţ ńōŵ", + "updatingStatus": "Ũƥďàţĩńĝ…", "useAnAppWhenAutomationNeeds": "Ũśē àń Àƥƥ ŵĥēń àũţōḿàţĩōń ńēēďś ĩţś ōŵń ĩďēńţĩţŷ, śĥōŕţ-ĺĩvēď ţōķēńś, àńď ŕēƥōśĩţōŕŷ-ĺēvēĺ ĩńśţàĺĺàţĩōń ćōńţŕōĺ. Śēţũƥ ĩś ḿōŕē ĩńvōĺvēď àńď ŕēqũĩŕēś à ƥũƀĺĩćĺŷ ŕēàćĥàƀĺē ĤŢŢƤŚ ŨŔĹ.", "useCommaSeparatedOwnerRepoValues": "Ũśē ćōḿḿà-śēƥàŕàţēď ōŵńēŕ/ŕēƥō vàĺũēś.", "useDefault": "Ũśē ďēƒàũĺţ", diff --git a/apps/web/src/locales/pt-pt/github.json b/apps/web/src/locales/pt-pt/github.json index 53cd1414ed..b1fef304ca 100644 --- a/apps/web/src/locales/pt-pt/github.json +++ b/apps/web/src/locales/pt-pt/github.json @@ -520,6 +520,7 @@ "updated": "Atualizado ", "updatedAgo": "atualizado há {{elapsed}}", "updatedJustNow": "atualizado agora mesmo", + "updatingStatus": "A atualizar…", "useAnAppWhenAutomationNeeds": "Use uma App quando a automatização precisar de identidade própria, tokens de curta duração e controlo da instalação ao nível do repositório. A configuração é mais trabalhosa e exige um URL HTTPS acessível publicamente.", "useCommaSeparatedOwnerRepoValues": "Use valores owner/repo separados por vírgulas.", "useDefault": "Usar a pré-definição", diff --git a/apps/web/src/locales/zh-cn/github.json b/apps/web/src/locales/zh-cn/github.json index f9231c9ee2..178c4c4a03 100644 --- a/apps/web/src/locales/zh-cn/github.json +++ b/apps/web/src/locales/zh-cn/github.json @@ -520,6 +520,7 @@ "updated": "已更新 ", "updatedAgo": "{{elapsed}} 前更新", "updatedJustNow": "刚刚更新", + "updatingStatus": "更新中…", "useAnAppWhenAutomationNeeds": "当自动化需要独立身份、短期令牌和仓库级安装控制时，请使用应用。设置过程更复杂，并且需要可公开访问的 HTTPS URL。", "useCommaSeparatedOwnerRepoValues": "使用逗号分隔的 owner/repo 值。", "useDefault": "使用默认",