-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: optimize memory usage for Swift project analysis #7346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" | ||
| import * as fs from "fs" | ||
| import * as path from "path" | ||
| import { isSwiftProject, getProjectFileLimit } from "../projectDetection" | ||
|
|
||
| vi.mock("fs", () => ({ | ||
| promises: { | ||
| readdir: vi.fn(), | ||
| stat: vi.fn(), | ||
| }, | ||
| })) | ||
|
|
||
| describe("projectDetection", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks() | ||
| }) | ||
|
|
||
| describe("isSwiftProject", () => { | ||
| it("should detect Swift project with Package.swift", async () => { | ||
| vi.mocked(fs.promises.readdir).mockResolvedValue(["Package.swift", "Sources", "Tests"] as any) | ||
|
|
||
| const result = await isSwiftProject("/test/project") | ||
| expect(result).toBe(true) | ||
| }) | ||
|
|
||
| it("should detect Swift project with .xcodeproj", async () => { | ||
| vi.mocked(fs.promises.readdir).mockResolvedValue(["MyApp.xcodeproj", "MyApp", "MyAppTests"] as any) | ||
|
|
||
| const result = await isSwiftProject("/test/project") | ||
| expect(result).toBe(true) | ||
| }) | ||
|
|
||
| it("should detect Swift project with .xcworkspace", async () => { | ||
| vi.mocked(fs.promises.readdir).mockResolvedValue(["MyApp.xcworkspace", "MyApp.xcodeproj", "Pods"] as any) | ||
|
|
||
| const result = await isSwiftProject("/test/project") | ||
| expect(result).toBe(true) | ||
| }) | ||
|
|
||
| it("should detect Swift project with Podfile", async () => { | ||
| vi.mocked(fs.promises.readdir).mockResolvedValue(["Podfile", "Podfile.lock", "MyApp"] as any) | ||
|
|
||
| const result = await isSwiftProject("/test/project") | ||
| expect(result).toBe(true) | ||
| }) | ||
|
|
||
| it("should detect Swift project with Cartfile", async () => { | ||
| vi.mocked(fs.promises.readdir).mockResolvedValue(["Cartfile", "Cartfile.resolved", "MyApp"] as any) | ||
|
|
||
| const result = await isSwiftProject("/test/project") | ||
| expect(result).toBe(true) | ||
| }) | ||
|
|
||
| it("should detect Swift project with .swift files in root", async () => { | ||
| vi.mocked(fs.promises.readdir).mockResolvedValue(["main.swift", "AppDelegate.swift", "README.md"] as any) | ||
|
|
||
| const result = await isSwiftProject("/test/project") | ||
| expect(result).toBe(true) | ||
| }) | ||
|
|
||
| it("should detect Swift project with Sources directory containing Swift files", async () => { | ||
| vi.mocked(fs.promises.readdir).mockResolvedValueOnce(["Sources", "Tests", "README.md"] as any) | ||
|
|
||
| vi.mocked(fs.promises.stat).mockResolvedValue({ | ||
| isDirectory: () => true, | ||
| } as any) | ||
|
|
||
| vi.mocked(fs.promises.readdir).mockResolvedValueOnce(["App.swift", "Model.swift"] as any) | ||
|
|
||
| const result = await isSwiftProject("/test/project") | ||
| expect(result).toBe(true) | ||
| }) | ||
|
|
||
| it("should not detect non-Swift project", async () => { | ||
| vi.mocked(fs.promises.readdir).mockResolvedValue([ | ||
| "package.json", | ||
| "node_modules", | ||
| "src", | ||
| "README.md", | ||
| ] as any) | ||
|
|
||
| const result = await isSwiftProject("/test/project") | ||
| expect(result).toBe(false) | ||
| }) | ||
|
|
||
| it("should handle errors gracefully", async () => { | ||
| vi.mocked(fs.promises.readdir).mockRejectedValue(new Error("Permission denied")) | ||
|
|
||
| const result = await isSwiftProject("/test/project") | ||
| expect(result).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| describe("getProjectFileLimit", () => { | ||
| it("should return reduced limit for Swift projects", async () => { | ||
| vi.mocked(fs.promises.readdir).mockResolvedValue(["Package.swift", "Sources"] as any) | ||
|
|
||
| const result = await getProjectFileLimit("/test/project", 200) | ||
| expect(result).toBe(100) | ||
| }) | ||
|
|
||
| it("should return reduced limit not exceeding 100 for Swift projects", async () => { | ||
| vi.mocked(fs.promises.readdir).mockResolvedValue(["MyApp.xcodeproj"] as any) | ||
|
|
||
| const result = await getProjectFileLimit("/test/project", 500) | ||
| expect(result).toBe(100) | ||
| }) | ||
|
|
||
| it("should return default limit for non-Swift projects", async () => { | ||
| vi.mocked(fs.promises.readdir).mockResolvedValue(["package.json", "src"] as any) | ||
|
|
||
| const result = await getProjectFileLimit("/test/project", 200) | ||
| expect(result).toBe(200) | ||
| }) | ||
|
|
||
| it("should return smaller default if it's less than 100 for Swift projects", async () => { | ||
| vi.mocked(fs.promises.readdir).mockResolvedValue(["Package.swift"] as any) | ||
|
|
||
| const result = await getProjectFileLimit("/test/project", 50) | ||
| expect(result).toBe(50) | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | |||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,86 @@ | |||||||||||||||||||||||||||||
| import * as fs from "fs" | |||||||||||||||||||||||||||||
| import * as path from "path" | |||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||
| /** | |||||||||||||||||||||||||||||
| * Detects if a directory contains a Swift project | |||||||||||||||||||||||||||||
| * @param dirPath - The directory path to check | |||||||||||||||||||||||||||||
| * @returns true if it's a Swift project, false otherwise | |||||||||||||||||||||||||||||
| */ | |||||||||||||||||||||||||||||
| export async function isSwiftProject(dirPath: string): Promise<boolean> { | |||||||||||||||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Performance consideration: This function is called for every file listing operation and performs multiple file system operations. Would it make sense to cache the result for the session? Something like:
Suggested change
|
|||||||||||||||||||||||||||||
| try { | |||||||||||||||||||||||||||||
| // Check for common Swift project indicators | |||||||||||||||||||||||||||||
| const swiftIndicators = [ | |||||||||||||||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this list comprehensive enough? We might be missing:
Would it be worth expanding the detection patterns? |
|||||||||||||||||||||||||||||
| "Package.swift", // Swift Package Manager | |||||||||||||||||||||||||||||
| "*.xcodeproj", // Xcode project | |||||||||||||||||||||||||||||
| "*.xcworkspace", // Xcode workspace | |||||||||||||||||||||||||||||
| "Podfile", // CocoaPods | |||||||||||||||||||||||||||||
| "Cartfile", // Carthage | |||||||||||||||||||||||||||||
| ] | |||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||
| const entries = await fs.promises.readdir(dirPath) | |||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||
| for (const entry of entries) { | |||||||||||||||||||||||||||||
| // Check for exact matches | |||||||||||||||||||||||||||||
| if (swiftIndicators.includes(entry)) { | |||||||||||||||||||||||||||||
| return true | |||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||
| // Check for pattern matches (e.g., *.xcodeproj) | |||||||||||||||||||||||||||||
| for (const indicator of swiftIndicators) { | |||||||||||||||||||||||||||||
| if (indicator.includes("*")) { | |||||||||||||||||||||||||||||
| const pattern = indicator.replace("*", "") | |||||||||||||||||||||||||||||
Check failureCode scanning / CodeQL Incomplete string escaping or encoding High
This replaces only the first occurrence of "*".
Copilot AutofixAI 3 months ago To properly address the issue and avoid incomplete pattern replacement when handling globs in Change: const pattern = indicator.replace("*", "")to: const pattern = indicator.replace(/\*/g, "")No new methods or complex imports are needed; this change can be made in place. Ensure the edit is only made at the highlighted line in
Suggested changeset
1
src/utils/projectDetection.ts
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
Positive FeedbackNegative Feedback
Refresh and try again.
|
|||||||||||||||||||||||||||||
| if (entry.endsWith(pattern)) { | |||||||||||||||||||||||||||||
| return true | |||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||
| // Also check if there are .swift files in the root directory | |||||||||||||||||||||||||||||
| const swiftFiles = entries.filter((entry) => entry.endsWith(".swift")) | |||||||||||||||||||||||||||||
| if (swiftFiles.length > 0) { | |||||||||||||||||||||||||||||
| return true | |||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||
| // Check for iOS/macOS specific directories | |||||||||||||||||||||||||||||
| const iosIndicators = ["Sources", "Tests", "UITests"] | |||||||||||||||||||||||||||||
| for (const indicator of iosIndicators) { | |||||||||||||||||||||||||||||
| const indicatorPath = path.join(dirPath, indicator) | |||||||||||||||||||||||||||||
| try { | |||||||||||||||||||||||||||||
| const stats = await fs.promises.stat(indicatorPath) | |||||||||||||||||||||||||||||
| if (stats.isDirectory()) { | |||||||||||||||||||||||||||||
| // Check if this directory contains Swift files | |||||||||||||||||||||||||||||
| const subEntries = await fs.promises.readdir(indicatorPath) | |||||||||||||||||||||||||||||
| const hasSwiftFiles = subEntries.some((entry) => entry.endsWith(".swift")) | |||||||||||||||||||||||||||||
| if (hasSwiftFiles) { | |||||||||||||||||||||||||||||
| return true | |||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
| } catch { | |||||||||||||||||||||||||||||
| // Directory doesn't exist, continue checking | |||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||
| return false | |||||||||||||||||||||||||||||
| } catch (error) { | |||||||||||||||||||||||||||||
| console.error(`Error detecting Swift project: ${error}`) | |||||||||||||||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use the extension's logging system instead of console.error for consistency with the rest of the codebase? |
|||||||||||||||||||||||||||||
| return false | |||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||
| /** | |||||||||||||||||||||||||||||
| * Gets the recommended file limit for a project based on its type | |||||||||||||||||||||||||||||
| * @param dirPath - The directory path to check | |||||||||||||||||||||||||||||
| * @param defaultLimit - The default limit to use | |||||||||||||||||||||||||||||
| * @returns The recommended file limit | |||||||||||||||||||||||||||||
| */ | |||||||||||||||||||||||||||||
| export async function getProjectFileLimit(dirPath: string, defaultLimit: number): Promise<number> { | |||||||||||||||||||||||||||||
| // For Swift projects, use a more conservative limit to prevent memory issues | |||||||||||||||||||||||||||||
| if (await isSwiftProject(dirPath)) { | |||||||||||||||||||||||||||||
| // Swift projects often have large dependency trees and generated files | |||||||||||||||||||||||||||||
| // Use a smaller limit to prevent memory exhaustion | |||||||||||||||||||||||||||||
| return Math.min(defaultLimit, 100) | |||||||||||||||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The hard-coded limit of 100 files might be too restrictive for some Swift projects or too generous for others. Could we make this configurable through settings? Perhaps:
Suggested change
|
|||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||
| return defaultLimit | |||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number alert! Could we extract this to a named constant for clarity?