vite/src/lib/phosphor-icons.tsx (1)

6-6: Unnecessary double cast – rely on the library’s own typings.

icons is already exported as IconEntry[] by @phosphor-icons/core; the as unknown as IconEntry[] cast is redundant and may hide real typing issues.

-export const PhosphorIcons = icons as unknown as IconEntry[];
+export const PhosphorIcons = icons;

vite/src/components/settings/sections/spaces/icon-picker.tsx (1)

18-25: State duplication can be avoided – use useMemo instead of storing a second copy.

iconList is static (PhosphorIcons), yet it is stored in state only to be copied again into filteredIcons. This triggers an extra render on mount and is unnecessary work. Derive iconList with useMemo (or just reference PhosphorIcons directly) and initialise filteredIcons from that.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

electron/browser/utility/menu/items/spaces.ts (1)

30-59: Hard-coded asset path is fragile inside ASAR and when package layout changes.

Deriving the SVG file path with path joins assumes the package structure never changes and that Electron can read outside the ASAR bundle at runtime.

Instead of reading SVG files from the filesystem, consider using the icon metadata directly from the Phosphor icon package:

function getPhosphorIconPath(pascalName: string): string | null {
  const name = getIconNameFromPascalCase(pascalName);
  if (!name) return null;

-  try {
-    // Find the icon directly from node_modules in dev, or from app.asar in production
-    let iconPath: string;
-
-    // Use the packaged path if the app is packaged
-    if (app.isPackaged) {
-      const pkgPath = path.join(process.resourcesPath, "duotone", `${name}-duotone.svg`);
-      iconPath = pkgPath;
-    } else {
-      const devPath = path.join(
-        require.resolve("@phosphor-icons/core"),
-        "..",
-        "..",
-        "assets",
-        "duotone",
-        `${name}-duotone.svg`
-      );
-      iconPath = devPath;
-    }
-
-    return iconPath;
-  } catch (error) {
-    console.error("Failed to resolve phosphor-icons path:", error);
-    return null;
-  }
+  try {
+    // Find the icon directly from the Phosphor icon metadata
+    const icon = PhosphorIcons.find(icon => icon.name === name);
+    if (!icon) return null;
+    
+    // Use the SVG content directly if available
+    if (icon.svg) return icon.svg;
+    
+    // Or use the file_path if provided
+    if (icon.file_path) return icon.file_path;
+    
+    return null;
+  } catch (error) {
+    console.error("Failed to resolve phosphor icon:", error);
+    return null;
+  }
}

electron/browser/utility/menu/items/spaces.ts (2)

20-20: Type assertion needs validation to ensure runtime safety.

The type assertion as unknown as IconEntry[] is a workaround that bypasses TypeScript's type checking. This could lead to runtime errors if the actual structure doesn't match the expected IconEntry type.

Consider adding validation or a more type-safe approach:

-const PhosphorIcons = icons as unknown as IconEntry[];
+// Validate the structure matches IconEntry before casting
+const PhosphorIcons = Array.isArray(icons) && icons.every(icon => 
+  'name' in icon && 'pascal_name' in icon
+) ? icons as IconEntry[] : [];

---

87-105: Icon retrieval function lacks memory management.

The icon cache grows unbounded, which could lead to memory leaks if many different icons are used over time.

Consider implementing a size limit or LRU cache:

// Icon cache
type IconCacheKey = `${string}-${number | undefined}`;
+const MAX_CACHE_SIZE = 100; // Limit cache size
const iconCache = new Map<IconCacheKey, NativeImage>();

async function getIconAsNativeImage(name: string, padding?: number): Promise<NativeImage | null> {
  const cacheKey = `${name}-${padding}` as IconCacheKey;

  // Check cache first
  if (iconCache.has(cacheKey)) {
    return iconCache.get(cacheKey) as NativeImage;
  }

  // Create new icon if not in cache
  const iconPath = getPhosphorIconPath(name);
  if (!iconPath) return null;

  const image = await createSvgFromIconPath(iconPath);
  if (image) {
    iconCache.set(cacheKey, image);
+    // Basic cache size management
+    if (iconCache.size > MAX_CACHE_SIZE) {
+      const oldestKey = iconCache.keys().next().value;
+      iconCache.delete(oldestKey);
+    }
  }

  return image;
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

electron/browser/utility/menu/items/spaces.ts (2)

30-59: Hard-coded asset path is fragile inside ASAR and when package layout changes.

This implementation still uses hardcoded paths and file system operations, which is fragile when the package structure changes or when accessing files within an ASAR archive.

Consider using the official API approach mentioned in the past review - each IconEntry might already contain an svg field that could be used directly, eliminating file system IO and path brittleness.

If you must use file paths, implement more robust path resolution with fallbacks:

function getPhosphorIconPath(pascalName: string): string | null {
  const name = getIconNameFromPascalCase(pascalName);
  if (!name) return null;

  try {
-    // Find the icon directly from node_modules in dev, or from app.asar in production
-    let iconPath: string;
-
-    // Use the packaged path if the app is packaged
-    if (app.isPackaged) {
-      const pkgPath = path.join(process.resourcesPath, "duotone", `${name}-duotone.svg`);
-      iconPath = pkgPath;
-    } else {
-      const devPath = path.join(
-        require.resolve("@phosphor-icons/core"),
-        "..",
-        "..",
-        "assets",
-        "duotone",
-        `${name}-duotone.svg`
-      );
-      iconPath = devPath;
-    }
+    // Define potential paths in order of preference
+    const possiblePaths = [];
+    
+    // Try to get icon SVG directly from the IconEntry if available
+    const iconEntry = PhosphorIcons.find(icon => icon.name === name);
+    if (iconEntry?.svg) {
+      // If the icon has an SVG string, no need for file path
+      return null; // Will handle inline SVG in getIconAsNativeImage
+    }
+    
+    // Add packaged path for production
+    if (app.isPackaged) {
+      possiblePaths.push(path.join(process.resourcesPath, "duotone", `${name}-duotone.svg`));
+      // Add alternative locations as fallbacks
+      possiblePaths.push(path.join(process.resourcesPath, "assets", "duotone", `${name}-duotone.svg`));
+    } else {
+      // Development paths
+      try {
+        const basePath = path.dirname(require.resolve("@phosphor-icons/core/package.json"));
+        possiblePaths.push(path.join(basePath, "assets", "duotone", `${name}-duotone.svg`));
+      } catch (err) {
+        console.warn("Could not resolve @phosphor-icons/core package location");
+      }
+    }
+    
+    // Return the first path that exists (would need to check synchronously)
+    // For simplicity in this example, just return the first path
+    return possiblePaths[0] || null;

    return iconPath;
  } catch (error) {
    console.error("Failed to resolve phosphor-icons path:", error);
    return null;
  }
}

Additionally, if IconEntry has file path information, consider using that directly.

---

62-92: SVG fill color modification is brittle and may not work with complex SVGs.

While the approach has been improved with a more robust regex replacement, it's still string-based manipulation which can be fragile with complex SVGs.

Consider using an XML/SVG parser like jsdom or cheerio for more reliable SVG manipulation:

async function createSvgFromIconPath(iconPath: string): Promise<NativeImage | null> {
  if (!svgPlatforms.includes(process.platform)) {
    return null;
  }

  try {
    let svgString = await readFile(iconPath, "utf8");

-    // Make SVG white using a more robust approach
-    // 1. Handle existing fill attributes on the SVG root
-    svgString = svgString.replace(/<svg([^>]*)>/, (match, attributes) => {
-      // Remove any existing fill attribute
-      const cleanedAttributes = attributes.replace(/\s*fill="[^"]*"\s*/g, " ");
-      return `<svg${cleanedAttributes} fill="white">`;
-    });
-
-    // 2. Add style to ensure all elements inherit the white color
-    svgString = svgString.replace(/<svg([^>]*)>/, (match) => {
-      return `${match}<style>* { fill: white; stroke: white; }</style>`;
-    });
+    // Simplify by injecting an outer SVG with a filter
+    // This approach avoids complex parsing but ensures all nested elements are white
+    svgString = `<svg xmlns="http://www.w3.org/2000/svg" width="100%" height="100%">
+      <defs>
+        <filter id="whitefill">
+          <feColorMatrix type="matrix" values="0 0 0 0 1 0 0 0 0 1 0 0 0 0 1 0 0 0 1 0"/>
+        </filter>
+      </defs>
+      <g filter="url(#whitefill)">
+        ${svgString}
+      </g>
+    </svg>`;

    // Convert to native image
    const iconBuffer = await sharp(Buffer.from(svgString)).png().resize(16, 16).toBuffer();

    return nativeImage.createFromBuffer(iconBuffer);
  } catch (error) {
    console.error("Error creating SVG from path:", error);
    return null;
  }
}

This approach uses an SVG filter to reliably convert all colors to white instead of complex string replacements.

electron/browser/utility/menu/items/spaces.ts (3)

20-20: Consider using a more type-safe approach than double type assertion.

The double type assertion (as unknown as IconEntry[]) is potentially unsafe and bypasses TypeScript's type checking. If the structure of the imported icons doesn't match IconEntry[], it could lead to runtime errors.

-const PhosphorIcons = icons as unknown as IconEntry[];
+// Verify the structure matches IconEntry[] before casting
+const PhosphorIcons = Array.isArray(icons) && 
+  icons.every(icon => 'pascal_name' in icon && 'name' in icon) 
+  ? icons as IconEntry[] 
+  : [];
+
+// Log warning if the icons array is empty after validation
+if (PhosphorIcons.length === 0) {
+  console.warn('PhosphorIcons array is empty or has incorrect structure');
+}

---

25-28: Improve resilience with better fallback handling.

The function returns a default icon "dot-outline" when the icon isn't found, but doesn't verify if this fallback icon actually exists in the Phosphor icon set.

function getIconNameFromPascalCase(pascalCaseName: string): string {
  const icon = PhosphorIcons.find((icon) => icon.pascal_name === pascalCaseName);
-  return icon?.name || "dot-outline";
+  // Return the found icon name, a verified fallback, or null
+  if (icon?.name) return icon.name;
+  
+  // Verify fallback exists
+  const fallbackIcon = PhosphorIcons.find((icon) => icon.name === "dot-outline");
+  if (fallbackIcon) return "dot-outline";
+  
+  // Last resort fallback to the first available icon
+  return PhosphorIcons[0]?.name || "";
}

---

98-116: Add handling for inline SVG content.

The function assumes icons are always loaded from files, but if the previous function is modified to use IconEntry.svg property, this function should handle inline SVG strings as well.

-async function getIconAsNativeImage(name: string, padding?: number): Promise<NativeImage | null> {
+async function getIconAsNativeImage(
+  name: string, 
+  padding?: number,
+  inlineSvg?: string
+): Promise<NativeImage | null> {
  const cacheKey = `${name}-${padding}` as IconCacheKey;

  // Check cache first
  if (iconCache.has(cacheKey)) {
    return iconCache.get(cacheKey) as NativeImage;
  }

-  // Create new icon if not in cache
-  const iconPath = getPhosphorIconPath(name);
-  if (!iconPath) return null;
-
-  const image = await createSvgFromIconPath(iconPath);
+  let image: NativeImage | null = null;
+  
+  // Handle inline SVG if provided
+  if (inlineSvg) {
+    // Process inline SVG similarly to file content
+    try {
+      // Similar SVG modifications as in createSvgFromIconPath
+      let svgString = inlineSvg;
+      svgString = svgString.replace(/<svg([^>]*)>/, (match, attributes) => {
+        const cleanedAttributes = attributes.replace(/\s*fill="[^"]*"\s*/g, " ");
+        return `<svg${cleanedAttributes} fill="white">`;
+      });
+      
+      svgString = svgString.replace(/<svg([^>]*)>/, (match) => {
+        return `${match}<style>* { fill: white; stroke: white; }</style>`;
+      });
+      
+      const iconBuffer = await sharp(Buffer.from(svgString)).png().resize(16, 16).toBuffer();
+      image = nativeImage.createFromBuffer(iconBuffer);
+    } catch (error) {
+      console.error("Error processing inline SVG:", error);
+    }
+  } else {
+    // Fall back to file path approach
+    const iconPath = getPhosphorIconPath(name);
+    if (iconPath) {
+      image = await createSvgFromIconPath(iconPath);
+    }
+  }
+  
  if (image) {
    iconCache.set(cacheKey, image);
  }

  return image;
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

electron/browser/utility/menu/items/spaces.ts (1)

30-59: 🛠️ Refactor suggestion

Hard-coded asset path remains fragile.

While you've improved the path resolution by handling both packaged and development environments, this approach still has issues: it relies on hard-coded relative paths that could break if package structures change, and it performs unnecessary filesystem IO.

Consider using the IconEntry data directly from Phosphor:

function getPhosphorIconPath(pascalName: string): string | null {
  const name = getIconNameFromPascalCase(pascalName);
  if (!name) return null;

  try {
-    // Find the icon directly from node_modules in dev, or from app.asar in production
-    let iconPath: string;
-
-    // Use the packaged path if the app is packaged
-    if (app.isPackaged) {
-      const pkgPath = path.join(process.resourcesPath, "duotone", `${name}-duotone.svg`);
-      iconPath = pkgPath;
-    } else {
-      const devPath = path.join(
-        require.resolve("@phosphor-icons/core"),
-        "..",
-        "..",
-        "assets",
-        "duotone",
-        `${name}-duotone.svg`
-      );
-      iconPath = devPath;
-    }
-
-    return iconPath;
+    // Find the icon directly from the icon data
+    const icon = PhosphorIcons.find(icon => icon.name === name);
+    return icon?.svg ? icon.svg : null;

  } catch (error) {
    console.error("Failed to resolve phosphor-icons path:", error);
    return null;
  }
}

electron/browser/utility/menu/items/spaces.ts (3)

20-20: Type casting via unknown should be avoided.

The type casting icons as unknown as IconEntry[] is a TypeScript anti-pattern that bypasses type safety. This could lead to runtime errors if the actual structure doesn't match the expected interface.

Consider using a more type-safe approach:

-const PhosphorIcons = icons as unknown as IconEntry[];
+// Define an explicit type guard to verify structure
+function isIconEntryArray(obj: any): obj is IconEntry[] {
+  return Array.isArray(obj) && obj.length > 0 && 'pascal_name' in obj[0] && 'name' in obj[0];
+}
+
+const PhosphorIcons = isIconEntryArray(icons) ? icons : [];

---

62-92: SVG manipulation has been improved but could be more robust.

Your improved SVG manipulation is better than the previous approach, but still uses regular expressions to modify XML/SVG which can be brittle with complex SVGs. This addresses part of the previous review comment, but could be further improved.

For even more robust handling of SVGs, consider using a proper XML parser:

async function createSvgFromIconPath(iconPath: string): Promise<NativeImage | null> {
  if (!svgPlatforms.includes(process.platform)) {
    return null;
  }

  try {
-    let svgString = await readFile(iconPath, "utf8");
-
-    // Make SVG white using a more robust approach
-    // 1. Handle existing fill attributes on the SVG root
-    svgString = svgString.replace(/<svg([^>]*)>/, (match, attributes) => {
-      // Remove any existing fill attribute
-      const cleanedAttributes = attributes.replace(/\s*fill="[^"]*"\s*/g, " ");
-      return `<svg${cleanedAttributes} fill="white">`;
-    });
-
-    // 2. Add style to ensure all elements inherit the white color
-    svgString = svgString.replace(/<svg([^>]*)>/, (match) => {
-      return `${match}<style>* { fill: white; stroke: white; }</style>`;
-    });
+    // If using the direct SVG string approach:
+    let svgString = typeof iconPath === 'string' && iconPath.startsWith('<svg') 
+      ? iconPath 
+      : await readFile(iconPath, "utf8");
+
+    // Use a lightweight XML/SVG parser like 'svgson' if added as a dependency
+    // For now, continue with the improved regex approach:
+    // 1. Handle existing fill attributes on the SVG root
+    svgString = svgString.replace(/<svg([^>]*)>/, (match, attributes) => {
+      // Remove any existing fill attribute
+      const cleanedAttributes = attributes.replace(/\s*fill="[^"]*"\s*/g, " ");
+      return `<svg${cleanedAttributes} fill="white">`;
+    });
+
+    // 2. Add style to ensure all elements inherit the white color
+    svgString = svgString.replace(/<svg([^>]*)>/, (match) => {
+      return `${match}<style>* { fill: white; stroke: white; }</style>`;
+    });

    // Convert to native image
    const iconBuffer = await sharp(Buffer.from(svgString)).png().resize(16, 16).toBuffer();

    return nativeImage.createFromBuffer(iconBuffer);
  } catch (error) {
    console.error("Error creating SVG from path:", error);
    return null;
  }
}

---

95-116: Icon cache may grow unbounded.

The current implementation of icon caching uses a Map that grows indefinitely as new icons are loaded, which could potentially lead to memory issues over time.

Consider adding a size limit or an LRU cache implementation:

// Icon cache
type IconCacheKey = `${string}-${number | undefined}`;
-const iconCache = new Map<IconCacheKey, NativeImage>();
+const MAX_CACHE_SIZE = 100; // Adjust based on expected icon usage
+class LimitedIconCache {
+  private cache = new Map<IconCacheKey, { image: NativeImage, lastUsed: number }>();
+  
+  has(key: IconCacheKey): boolean {
+    return this.cache.has(key);
+  }
+  
+  get(key: IconCacheKey): NativeImage | undefined {
+    const entry = this.cache.get(key);
+    if (entry) {
+      entry.lastUsed = Date.now();
+      return entry.image;
+    }
+    return undefined;
+  }
+  
+  set(key: IconCacheKey, image: NativeImage): void {
+    // Evict least recently used if cache is full
+    if (this.cache.size >= MAX_CACHE_SIZE) {
+      let oldestKey: IconCacheKey | null = null;
+      let oldestTime = Infinity;
+      
+      for (const [k, v] of this.cache.entries()) {
+        if (v.lastUsed < oldestTime) {
+          oldestTime = v.lastUsed;
+          oldestKey = k;
+        }
+      }
+      
+      if (oldestKey) this.cache.delete(oldestKey);
+    }
+    
+    this.cache.set(key, { image, lastUsed: Date.now() });
+  }
+}
+
+const iconCache = new LimitedIconCache();

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

electron/browser/utility/menu/items/spaces.ts (1)

30-59: 🛠️ Refactor suggestion

Hard-coded asset path is fragile inside ASAR and when package layout changes.

Deriving the SVG file path with path.join(require.resolve(...), "..", "..", "assets", "duotone", ...) assumes the package structure never changes. This approach is improved from the previous implementation by using process.resourcesPath for packaged apps, but it's still brittle.

Consider using the icon data directly from the IconEntry object:

function getPhosphorIconPath(pascalName: string): string | null {
  const name = getIconNameFromPascalCase(pascalName);
  if (!name) return null;

  try {
-    // Find the icon directly from node_modules in dev, or from app.asar in production
-    let iconPath: string;
-
-    // Use the packaged path if the app is packaged
-    if (app.isPackaged) {
-      const pkgPath = path.join(process.resourcesPath, "duotone", `${name}-duotone.svg`);
-      iconPath = pkgPath;
-    } else {
-      const devPath = path.join(
-        require.resolve("@phosphor-icons/core"),
-        "..",
-        "..",
-        "assets",
-        "duotone",
-        `${name}-duotone.svg`
-      );
-      iconPath = devPath;
-    }
-
-    return iconPath;
+    // Find the icon in the PhosphorIcons array
+    const icon = PhosphorIcons.find((icon) => icon.name === name);
+    if (!icon) return null;
+    
+    // Use the file_path if available, otherwise use the svg content directly
+    if (icon.file_path) {
+      return icon.file_path;
+    } else if (icon.svg) {
+      // If we have SVG content directly, we can skip file reading
+      // Save to a temporary file or pass directly to createSvgFromSvgString
+      // This would require modifying createSvgFromIconPath to accept SVG strings
+      const tempPath = path.join(app.getPath('temp'), `${name}-duotone.svg`);
+      fs.writeFileSync(tempPath, icon.svg);
+      return tempPath;
+    }
+    
+    return null;
  } catch (error) {
    console.error("Failed to resolve phosphor-icons path:", error);
    return null;
  }
}

This would also require adding a complementary function:

async function createSvgFromSvgString(svgString: string): Promise<NativeImage | null> {
  if (!svgPlatforms.includes(process.platform)) {
    return null;
  }

  try {
    // Apply the same SVG modifications as in createSvgFromIconPath
    // [...]
    
    const iconBuffer = await sharp(Buffer.from(svgString)).png().resize(16, 16).toBuffer();
    return nativeImage.createFromBuffer(iconBuffer);
  } catch (error) {
    console.error("Error creating SVG from string:", error);
    return null;
  }
}

electron/browser/utility/menu/items/spaces.ts (4)

20-20: Consider using proper TypeScript typing for icons.

Using as unknown as IconEntry[] type casting is problematic as it bypasses TypeScript's type checking. This could lead to runtime errors if the structure of icons doesn't match what you're expecting.

-const PhosphorIcons = icons as unknown as IconEntry[];
+const PhosphorIcons: IconEntry[] = icons as IconEntry[];

---

95-95: Simplify IconCacheKey type definition.

The template literal type IconCacheKey = \${string}`is unnecessarily complex for this use case since it's functionally equivalent to just usingstring`.

-type IconCacheKey = `${string}`;
+type IconCacheKey = string;

---

140-140: Menu item ID changes based on checked state.

Including the checked state in the ID (id: \space-${space.id}-${checked ? "checked" : "unchecked"}``) makes the ID unstable across state changes. If any code relies on consistent IDs, this could cause issues.

-id: `space-${space.id}-${checked ? "checked" : "unchecked"}`,
+id: `space-${space.id}`,

If the checked state needs to be identifiable from the ID for some reason, consider adding it as a separate data attribute instead.

---

156-156: Unused Browser parameter.

The _browser parameter in createSpacesMenu is prefixed with an underscore indicating it's not used, but still required by the function signature. This suggests a potential misalignment with the function's intended usage.

If the parameter is truly not needed, consider updating the API where this function is called to remove the parameter.

Alternatively, if it should be used (e.g., to get the focused window through Browser's methods), update the implementation to utilize it properly:

-export async function createSpacesMenu(_browser: Browser): Promise<MenuItemConstructorOptions> {
+export async function createSpacesMenu(browser: Browser): Promise<MenuItemConstructorOptions> {
  try {
    const spaces = await getSpaces();

-    const focusedWindow = getFocusedWindow();
+    const focusedWindow = browser.getFocusedWindow();

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro