diff --git a/.changeset/fn-1467-pr-feedback-fixes.md b/.changeset/fn-1467-pr-feedback-fixes.md new file mode 100644 index 0000000000..18a8d10399 --- /dev/null +++ b/.changeset/fn-1467-pr-feedback-fixes.md @@ -0,0 +1,3 @@ +"@runfusion/fusion": patch + +Fix project selector review regressions around optional selection handlers and bookmarked search matches, and tighten retry/backoff timeout and rate-limit handling. diff --git a/packages/dashboard/app/components/Header.tsx b/packages/dashboard/app/components/Header.tsx index 4437c31e83..11ac2f5fd1 100644 --- a/packages/dashboard/app/components/Header.tsx +++ b/packages/dashboard/app/components/Header.tsx @@ -836,7 +836,7 @@ export function Header({ )} {/* Project Selector - Back button when project selected, dropdown when 2+ projects (tablet + desktop) */} - {!isMobile && projects.length >= 1 && onViewAllProjects && ( + {!isMobile && projects.length >= 1 && onViewAllProjects && onSelectProject && ( { const recentIds = new Set(recentProjects.map((p) => p.id)); const currentId = currentProject?.id; + const hasSearch = Boolean(searchQuery.trim()); // Bookmarked projects (excluding current) - const bookmarked = filteredProjects.filter( - (p) => - p.id !== currentId && - bookmarkedIds.has(p.id) && - !recentIds.has(p.id) - ); + const bookmarked = hasSearch + ? [] + : filteredProjects.filter( + (p) => + p.id !== currentId && + bookmarkedIds.has(p.id) && + !recentIds.has(p.id) + ); - // Exclude current, bookmarked, and recent from "others" + // Exclude current, bookmarked, and recent from "others" only when those + // sections are visible. Search mode surfaces every matching project here. const bookmarkedAndRecentIds = new Set([ ...bookmarked.map((p) => p.id), - ...recentIds, + ...(hasSearch ? [] : recentIds), ]); const others = filteredProjects.filter( (p) => p.id !== currentId && !bookmarkedAndRecentIds.has(p.id) ); return { - bookmarked: searchQuery.trim() ? [] : bookmarked, - recent: searchQuery.trim() ? [] : recentProjects, + bookmarked, + recent: hasSearch ? [] : recentProjects, others, }; }, [filteredProjects, recentProjects, currentProject, searchQuery, bookmarkedIds]); @@ -228,13 +232,13 @@ export function ProjectSelector({ if (highlightedIndex < bookmarkedCount) { // Select bookmarked project - onSelect(displayProjects.bookmarked[highlightedIndex]); + onSelect?.(displayProjects.bookmarked[highlightedIndex]); } else if (highlightedIndex < bookmarkedCount + recentCount) { // Select recent project - onSelect(displayProjects.recent[highlightedIndex - bookmarkedCount]); + onSelect?.(displayProjects.recent[highlightedIndex - bookmarkedCount]); } else if (highlightedIndex < bookmarkedCount + recentCount + othersCount) { // Select other project - onSelect(displayProjects.others[highlightedIndex - bookmarkedCount - recentCount]); + onSelect?.(displayProjects.others[highlightedIndex - bookmarkedCount - recentCount]); } else { // View All onViewAll(); @@ -297,7 +301,7 @@ export function ProjectSelector({ // Handle project selection const handleSelectProject = useCallback( (project: ProjectInfo) => { - onSelect(project); + onSelect?.(project); setIsOpen(false); setSearchQuery(""); }, diff --git a/packages/dashboard/app/components/__tests__/ProjectSelector.test.tsx b/packages/dashboard/app/components/__tests__/ProjectSelector.test.tsx index 28baea625b..7b51335dcf 100644 --- a/packages/dashboard/app/components/__tests__/ProjectSelector.test.tsx +++ b/packages/dashboard/app/components/__tests__/ProjectSelector.test.tsx @@ -193,6 +193,23 @@ describe("ProjectSelector", () => { expect(onSelect).toHaveBeenCalledWith(projectTwo); }); + it("does not throw when clicking a project without onSelect", () => { + render( + + ); + + fireEvent.click(screen.getByTestId("project-selector-trigger")); + expect(() => fireEvent.click(screen.getByText("Project Two"))).not.toThrow(); + expect(screen.queryByTestId("project-selector-dropdown")).toBeNull(); + }); + it("closes dropdown after selection", () => { const onSelect = vi.fn(); @@ -731,6 +748,53 @@ describe("ProjectSelector", () => { expect(screen.getByTestId("project-selector-no-results")).toBeDefined(); }); + it("shows bookmarked projects that match an active search", () => { + mockBookmarkedIds = new Set(["proj_2"]); + + render( + + ); + + fireEvent.click(screen.getByTestId("project-selector-trigger")); + fireEvent.change(screen.getByPlaceholderText("Search projects..."), { + target: { value: "Starred" }, + }); + + expect(screen.getByText("Starred")).toBeDefined(); + expect(screen.getByText("Project")).toBeDefined(); + expect(screen.queryByTestId("project-selector-no-results")).toBeNull(); + }); + + it("does not throw when pressing Enter on a highlighted project without onSelect", async () => { + render( + + ); + + fireEvent.click(screen.getByTestId("project-selector-trigger")); + const searchInput = screen.getByPlaceholderText("Search projects..."); + fireEvent.change(searchInput, { target: { value: "Beta" } }); + await waitFor(() => { + expect(screen.getByTestId("project-selector-item-proj_2").className).toContain("highlighted"); + }); + expect(() => fireEvent.keyDown(searchInput, { key: "Enter" })).not.toThrow(); + expect(screen.queryByTestId("project-selector-dropdown")).toBeNull(); + }); + it("clear button clears search query", () => { render( { expect(onRetry).not.toHaveBeenCalled(); }); + it("does not let a custom retry check override RateLimitError instances", async () => { + const fn = vi.fn().mockRejectedValue(new RateLimitError("429")); + const onRetry = vi.fn(); + + await expect( + withRetry(fn, { + baseDelayMs: 100, + onRetry, + isRetryable: () => true, + }), + ).rejects.toThrow("429"); + + expect(fn).toHaveBeenCalledTimes(1); + expect(onRetry).not.toHaveBeenCalled(); + }); + it("applies exponential backoff with increasing delays", async () => { const fn = vi .fn() @@ -586,6 +602,27 @@ describe("withRetry", () => { const retryErr = onRetry.mock.calls[0][2]; expect(retryErr.code).toBe("TIMEOUT"); }); + + it("passes a per-attempt abort signal that is aborted on timeout", async () => { + let attemptSignal: AbortSignal | undefined; + const fn = vi.fn((signal?: AbortSignal) => { + attemptSignal = signal; + return new Promise(() => {}); + }); + + const promise = withRetry(fn, { + maxRetries: 0, + timeoutMs: 500, + }); + const assertion = expect(promise).rejects.toBeInstanceOf(TimeoutError); + + await vi.advanceTimersByTimeAsync(600); + + await assertion; + expect(attemptSignal).toBeInstanceOf(AbortSignal); + expect(attemptSignal?.aborted).toBe(true); + expect(attemptSignal?.reason).toBeInstanceOf(TimeoutError); + }); }); describe("withRetryResult", () => { diff --git a/packages/engine/src/engine-errors.ts b/packages/engine/src/engine-errors.ts index c5368e3c25..8f242d0ed8 100644 --- a/packages/engine/src/engine-errors.ts +++ b/packages/engine/src/engine-errors.ts @@ -244,7 +244,7 @@ export function classifyThrownError(err: unknown): EngineError { return new ServiceUnavailableError(message, undefined, undefined, err instanceof Error ? err : undefined); } - if (/"type":"server_error"|\"code\":\"server_error\"/i.test(message)) { + if (/"type":"server_error"|"code":"server_error"/i.test(message)) { return new ServiceUnavailableError(message, 500, undefined, err instanceof Error ? err : undefined); } diff --git a/packages/engine/src/retry-with-backoff.ts b/packages/engine/src/retry-with-backoff.ts index 286c28ab51..6a313c23b5 100644 --- a/packages/engine/src/retry-with-backoff.ts +++ b/packages/engine/src/retry-with-backoff.ts @@ -40,7 +40,13 @@ * ``` */ -import { classifyThrownError, isRetryableError, type EngineError } from "./engine-errors.js"; +import { + classifyThrownError, + isRetryableError, + RateLimitError, + TimeoutError, + type EngineError, +} from "./engine-errors.js"; import { isUsageLimitError } from "./usage-limit-detector.js"; // ── Types ─────────────────────────────────────────────────────────────── @@ -168,7 +174,7 @@ export function cancellableSleep(ms: number, signal?: AbortSignal): Promise( - fn: () => Promise, + fn: (signal?: AbortSignal) => Promise, timeoutMs: number, parentSignal?: AbortSignal, ): Promise { @@ -188,11 +194,12 @@ function withTimeout( const timer = setTimeout(() => { if (settled) return; settled = true; - ac.abort(new Error(`Operation timed out after ${timeoutMs}ms`)); - reject(new Error(`Operation timed out after ${timeoutMs}ms`)); + const timeoutErr = new TimeoutError(`Operation timed out after ${timeoutMs}ms`, timeoutMs); + ac.abort(timeoutErr); + reject(timeoutErr); }, timeoutMs); - fn() + fn(ac.signal) .then((result) => { if (settled) return; settled = true; @@ -229,7 +236,7 @@ function withTimeout( * @returns The return value of `fn()` */ export async function withRetry( - fn: () => Promise, + fn: (signal?: AbortSignal) => Promise, options: RetryOptions = {}, ): Promise { const { @@ -243,9 +250,7 @@ export async function withRetry( isRetryable: customIsRetryable, } = options; - const startTime = Date.now(); let lastError: EngineError | undefined; - let retryCount = 0; for (let attempt = 0; attempt <= maxRetries; attempt++) { // Check abort before each attempt @@ -257,14 +262,14 @@ export async function withRetry( // Wrap with timeout if configured const result = timeoutMs ? await withTimeout(fn, timeoutMs, signal) - : await fn(); + : await fn(signal); return result; } catch (err: unknown) { // Classify the error into a structured type const classified = classifyThrownError(err); // Rate-limit errors: never retry locally — re-throw immediately - if (isUsageLimitError(classified.message)) { + if (classified instanceof RateLimitError || isUsageLimitError(classified.message)) { throw classified; } @@ -297,8 +302,6 @@ export async function withRetry( // Sleep with cancellation support await cancellableSleep(delay, signal); - - retryCount++; } } @@ -317,17 +320,14 @@ export async function withRetry( * @returns A `RetryResult` with value and retry metadata */ export async function withRetryResult( - fn: () => Promise, + fn: (signal?: AbortSignal) => Promise, options: RetryOptions = {}, ): Promise> { const startTime = Date.now(); let retries = 0; - const result = await withRetry(async () => { - if (retries > 0) { - // We're in a retry — count it - } - return fn(); + const result = await withRetry(async (signal) => { + return fn(signal); }, { ...options, onRetry: (attempt, delayMs, err) => {