From d76944dd9341b399d22f462bbeabf90a1c8346a9 Mon Sep 17 00:00:00 2001 From: Alan Trebugeais Date: Thu, 7 May 2026 12:01:11 +0200 Subject: [PATCH] fix: NitPick comments from CodeRabbits --- src/components/launch/SourceSelector.tsx | 70 ++++++++----------- .../hooks/useLaunchWindowSystemState.ts | 12 +++- .../launch/popovers/SourcePopover.tsx | 28 +------- .../launch/popovers/launchPopoverTypes.ts | 35 ++++++++++ 4 files changed, 75 insertions(+), 70 deletions(-) diff --git a/src/components/launch/SourceSelector.tsx b/src/components/launch/SourceSelector.tsx index a7c6ee40..b0bba991 100644 --- a/src/components/launch/SourceSelector.tsx +++ b/src/components/launch/SourceSelector.tsx @@ -5,20 +5,10 @@ import { useScopedT } from "@/contexts/I18nContext"; import { Popover, PopoverContent, PopoverTrigger } from "@/components/ui/popover"; import { Button } from "@/components/ui/button"; import { cn } from "@/lib/utils"; +import { mapRawSource, type DesktopSource, type RawDesktopSource } from "./popovers/launchPopoverTypes"; import "./launchTheme.css"; import "./SourceSelector.css"; -interface DesktopSource { - id: string; - name: string; - thumbnail: string | null; - display_id: string; - appIcon: string | null; - sourceType?: "screen" | "window"; - appName?: string; - windowTitle?: string; -} - interface SourceSelectorProps { /** List of available screen sources */ screenSources?: DesktopSource[]; @@ -212,31 +202,6 @@ export const SourceSelector = React.memo(function SourceSelector({ const loading = propsLoading ?? internalLoading; const selectedSource = propsSelectedSource ?? internalSelectedSource; - // Mapping logic for Electron sources - const mapSource = useCallback((s: any): DesktopSource => { - const isWindow = s.id.startsWith("window:"); - const type = s.sourceType ?? (isWindow ? "window" : "screen"); - let displayName = s.name; - let appName = s.appName; - if (isWindow && !appName && s.name.includes(" — ")) { - const parts = s.name.split(" — "); - appName = parts[0]?.trim(); - displayName = parts.slice(1).join(" — ").trim() || s.name; - } else if (isWindow && s.windowTitle) { - displayName = s.windowTitle; - } - return { - id: s.id, - name: displayName, - thumbnail: s.thumbnail, - display_id: s.display_id, - appIcon: s.appIcon, - sourceType: type, - appName, - windowTitle: s.windowTitle ?? displayName, - }; - }, []); - // Default fetching logic const defaultFetchSources = useCallback(async () => { if (!window.electronAPI) return; @@ -247,13 +212,13 @@ export const SourceSelector = React.memo(function SourceSelector({ thumbnailSize: { width: 160, height: 90 }, fetchWindowIcons: true, }); - setInternalSources(rawSources.map(mapSource)); + setInternalSources(rawSources.map((s) => mapRawSource(s as RawDesktopSource))); } catch (error) { console.error("Failed to fetch sources:", error); } finally { setInternalLoading(false); } - }, [mapSource]); + }, []); const onFetchSources = propsOnFetchSources ?? defaultFetchSources; @@ -291,21 +256,42 @@ export const SourceSelector = React.memo(function SourceSelector({ const windowSources = propsWindowSources ?? internalWindowSources; const hasPrefetchedRef = useRef(false); + const fetchInFlightRef = useRef(false); + const lastFetchedAtRef = useRef(0); + + const fetchSourcesOnce = useCallback( + async (allowRecentSkip: boolean) => { + if (fetchInFlightRef.current) { + return; + } + if (allowRecentSkip && Date.now() - lastFetchedAtRef.current < 750) { + return; + } + fetchInFlightRef.current = true; + try { + await onFetchSources(); + lastFetchedAtRef.current = Date.now(); + } finally { + fetchInFlightRef.current = false; + } + }, + [onFetchSources], + ); const prefetchSources = React.useCallback(() => { if (hasPrefetchedRef.current) { return; } hasPrefetchedRef.current = true; - void onFetchSources(); - }, [onFetchSources]); + void fetchSourcesOnce(false); + }, [fetchSourcesOnce]); // Fetch sources when popover opens useEffect(() => { if (open) { - void onFetchSources(); + void fetchSourcesOnce(true); } - }, [open, onFetchSources]); + }, [open, fetchSourcesOnce]); // In autonomous mode, we might want to start open useEffect(() => { diff --git a/src/components/launch/hooks/useLaunchWindowSystemState.ts b/src/components/launch/hooks/useLaunchWindowSystemState.ts index 3c22eebb..56793fa3 100644 --- a/src/components/launch/hooks/useLaunchWindowSystemState.ts +++ b/src/components/launch/hooks/useLaunchWindowSystemState.ts @@ -16,11 +16,19 @@ export function useLaunchWindowSystemState( }, []); useEffect(() => { + let cancelled = false; const load = async () => { - const result = await window.electronAPI.getRecordingsDirectory(); - if (result.success) setRecordingsDirectory(result.path); + try { + const result = await window.electronAPI.getRecordingsDirectory(); + if (!cancelled && result.success) setRecordingsDirectory(result.path); + } catch (error) { + console.error("Failed to load recordings directory:", error); + } }; void load(); + return () => { + cancelled = true; + }; }, []); useEffect(() => { diff --git a/src/components/launch/popovers/SourcePopover.tsx b/src/components/launch/popovers/SourcePopover.tsx index 194df8fe..3f82dab8 100644 --- a/src/components/launch/popovers/SourcePopover.tsx +++ b/src/components/launch/popovers/SourcePopover.tsx @@ -1,7 +1,7 @@ import { useCallback, useMemo, type ReactNode, useState } from "react"; import { SourceSelector } from "../SourceSelector"; import { useLaunchPopoverCoordinator } from "./LaunchPopoverCoordinator"; -import type { DesktopSource } from "./launchPopoverTypes"; +import { mapRawSource, type DesktopSource, type RawDesktopSource } from "./launchPopoverTypes"; const POPOVER_ID = "sources"; @@ -30,31 +30,7 @@ export function SourcePopover({ thumbnailSize: { width: 160, height: 90 }, fetchWindowIcons: true, }); - setSources( - rawSources.map((s) => { - const isWindow = s.id.startsWith("window:"); - const type = s.sourceType ?? (isWindow ? "window" : "screen"); - let displayName = s.name; - let appName = s.appName; - if (isWindow && !appName && s.name.includes(" — ")) { - const parts = s.name.split(" — "); - appName = parts[0]?.trim(); - displayName = parts.slice(1).join(" — ").trim() || s.name; - } else if (isWindow && s.windowTitle) { - displayName = s.windowTitle; - } - return { - id: s.id, - name: displayName, - thumbnail: s.thumbnail, - display_id: s.display_id, - appIcon: s.appIcon, - sourceType: type, - appName, - windowTitle: s.windowTitle ?? displayName, - }; - }), - ); + setSources(rawSources.map((s) => mapRawSource(s as RawDesktopSource))); } catch (error) { console.error("Failed to fetch sources:", error); } finally { diff --git a/src/components/launch/popovers/launchPopoverTypes.ts b/src/components/launch/popovers/launchPopoverTypes.ts index 3690111c..16dece16 100644 --- a/src/components/launch/popovers/launchPopoverTypes.ts +++ b/src/components/launch/popovers/launchPopoverTypes.ts @@ -9,6 +9,41 @@ export interface DesktopSource { windowTitle?: string; } +export interface RawDesktopSource { + id: string; + name: string; + thumbnail: string | null; + display_id: string; + appIcon: string | null; + sourceType?: "screen" | "window"; + appName?: string; + windowTitle?: string; +} + +export function mapRawSource(s: RawDesktopSource): DesktopSource { + const isWindow = s.id.startsWith("window:"); + const type = s.sourceType ?? (isWindow ? "window" : "screen"); + let displayName = s.name; + let appName = s.appName; + if (isWindow && !appName && s.name.includes(" — ")) { + const parts = s.name.split(" — "); + appName = parts[0]?.trim(); + displayName = parts.slice(1).join(" — ").trim() || s.name; + } else if (isWindow && s.windowTitle) { + displayName = s.windowTitle; + } + return { + id: s.id, + name: displayName, + thumbnail: s.thumbnail, + display_id: s.display_id, + appIcon: s.appIcon, + sourceType: type, + appName, + windowTitle: s.windowTitle ?? displayName, + }; +} + export interface DeviceOption { deviceId: string; label: string;