chore: update dependencies and improve project structure#89
Conversation
- Added `morgan` for enhanced request logging in the server. - Updated `package-lock.json` to include new dependencies and their types. - Refactored the `NewProjectModal` component for improved readability and structure. - Enhanced the `FileBrowserDialog` to support initial path selection and improved error handling. - Updated various components to ensure consistent formatting and better user experience. - Introduced XML format specification for app specifications to maintain consistency across the application.
Summary of ChangesHello @webdevcody, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the project creation and management experience by introducing a new, more intuitive project setup modal and an onboarding flow. It standardizes the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable improvements, including enhanced request logging with morgan, a refactored NewProjectModal, and a better user experience for file browsing and spec creation. The centralization of spec creation state is a good architectural change. However, there are a few significant issues that need attention. Most critically, security checks for file system paths have been disabled, creating a major vulnerability. Additionally, there is significant code duplication in the new project creation logic within the Sidebar component, which has also become overly complex. My review focuses on addressing these critical security and maintainability concerns to ensure the long-term health and safety of the application.
| export function isPathAllowed(_filePath: string): boolean { | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Validate a path and throw if not allowed | ||
| * Validate a path - just resolves the path without checking permissions | ||
| */ | ||
| export function validatePath(filePath: string): string { | ||
| const resolved = path.resolve(filePath); | ||
|
|
||
| if (!isPathAllowed(resolved)) { | ||
| throw new Error( | ||
| `Access denied: ${filePath} is not in an allowed directory` | ||
| ); | ||
| } | ||
|
|
||
| return resolved; | ||
| return path.resolve(filePath); | ||
| } |
There was a problem hiding this comment.
The security checks for file system access have been disabled by making isPathAllowed always return true and validatePath only resolve the path. This removes a critical security layer that prevents path traversal and unauthorized file system access. While the comments indicate this is intentional, it poses a significant security risk, especially if the server is ever exposed to a network. It's strongly recommended to re-enable these checks and rely on the ALLOWED_PROJECT_DIRS environment variable and dynamically added project paths to enforce security boundaries. Disabling these checks could allow malicious or accidental operations outside of intended project directories.
| await api.writeFile( | ||
| `${projectPath}/.automaker/app_spec.txt`, | ||
| `<project_specification> | ||
| <project_name>${projectName}</project_name> | ||
|
|
||
| <overview> | ||
| Describe your project here. This file will be analyzed by an AI agent | ||
| to understand your project structure and tech stack. | ||
| </overview> | ||
|
|
||
| <technology_stack> | ||
| <!-- The AI agent will fill this in after analyzing your project --> | ||
| </technology_stack> | ||
|
|
||
| <core_capabilities> | ||
| <!-- List core features and capabilities --> | ||
| </core_capabilities> | ||
|
|
||
| <implemented_features> | ||
| <!-- The AI agent will populate this based on code analysis --> | ||
| </implemented_features> | ||
| </project_specification>` | ||
| ); | ||
|
|
||
| const trashedProject = trashedProjects.find( | ||
| (p) => p.path === projectPath | ||
| ); | ||
| const effectiveTheme = | ||
| (trashedProject?.theme as ThemeMode | undefined) || | ||
| (currentProject?.theme as ThemeMode | undefined) || | ||
| globalTheme; | ||
| const project = upsertAndSetCurrentProject( | ||
| projectPath, | ||
| projectName, | ||
| effectiveTheme | ||
| ); | ||
|
|
||
| setShowNewProjectModal(false); | ||
|
|
||
| // Show onboarding dialog for new project | ||
| setNewProjectName(projectName); | ||
| setNewProjectPath(projectPath); | ||
| setShowOnboardingDialog(true); | ||
|
|
||
| toast.success("Project created", { | ||
| description: `Created ${projectName} with .automaker directory`, | ||
| }); | ||
| } catch (error) { | ||
| console.error("[Sidebar] Failed to create project:", error); | ||
| toast.error("Failed to create project", { | ||
| description: error instanceof Error ? error.message : "Unknown error", | ||
| }); | ||
| } finally { | ||
| setIsCreatingProject(false); | ||
| } | ||
| }, | ||
| [trashedProjects, currentProject, globalTheme, upsertAndSetCurrentProject] | ||
| ); | ||
|
|
||
| /** | ||
| * Create a project from a GitHub starter template | ||
| */ | ||
| const handleCreateFromTemplate = useCallback( | ||
| async ( | ||
| template: StarterTemplate, | ||
| projectName: string, | ||
| parentDir: string | ||
| ) => { | ||
| setIsCreatingProject(true); | ||
| try { | ||
| const httpClient = getHttpApiClient(); | ||
| const api = getElectronAPI(); | ||
|
|
||
| // Clone the template repository | ||
| const cloneResult = await httpClient.templates.clone( | ||
| template.repoUrl, | ||
| projectName, | ||
| parentDir | ||
| ); | ||
|
|
||
| if (!cloneResult.success || !cloneResult.projectPath) { | ||
| toast.error("Failed to clone template", { | ||
| description: cloneResult.error || "Unknown error occurred", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const projectPath = cloneResult.projectPath; | ||
|
|
||
| // Initialize .automaker directory with all necessary files | ||
| const initResult = await initializeProject(projectPath); | ||
|
|
||
| if (!initResult.success) { | ||
| toast.error("Failed to initialize project", { | ||
| description: initResult.error || "Unknown error occurred", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Update the app_spec.txt with template-specific info | ||
| // Note: Must follow XML format as defined in apps/server/src/lib/app-spec-format.ts | ||
| await api.writeFile( | ||
| `${projectPath}/.automaker/app_spec.txt`, | ||
| `<project_specification> | ||
| <project_name>${projectName}</project_name> | ||
|
|
||
| <overview> | ||
| This project was created from the "${template.name}" starter template. | ||
| ${template.description} | ||
| </overview> | ||
|
|
||
| <technology_stack> | ||
| ${template.techStack | ||
| .map((tech) => `<technology>${tech}</technology>`) | ||
| .join("\n ")} | ||
| </technology_stack> | ||
|
|
||
| <core_capabilities> | ||
| ${template.features | ||
| .map((feature) => `<capability>${feature}</capability>`) | ||
| .join("\n ")} | ||
| </core_capabilities> | ||
|
|
||
| <implemented_features> | ||
| <!-- The AI agent will populate this based on code analysis --> | ||
| </implemented_features> | ||
| </project_specification>` | ||
| ); | ||
|
|
||
| const trashedProject = trashedProjects.find( | ||
| (p) => p.path === projectPath | ||
| ); | ||
| const effectiveTheme = | ||
| (trashedProject?.theme as ThemeMode | undefined) || | ||
| (currentProject?.theme as ThemeMode | undefined) || | ||
| globalTheme; | ||
| const project = upsertAndSetCurrentProject( | ||
| projectPath, | ||
| projectName, | ||
| effectiveTheme | ||
| ); | ||
|
|
||
| setShowNewProjectModal(false); | ||
|
|
||
| // Show onboarding dialog for new project | ||
| setNewProjectName(projectName); | ||
| setNewProjectPath(projectPath); | ||
| setShowOnboardingDialog(true); | ||
|
|
||
| toast.success("Project created from template", { | ||
| description: `Created ${projectName} from ${template.name}`, | ||
| }); | ||
| } catch (error) { | ||
| console.error( | ||
| "[Sidebar] Failed to create project from template:", | ||
| error | ||
| ); | ||
| toast.error("Failed to create project", { | ||
| description: error instanceof Error ? error.message : "Unknown error", | ||
| }); | ||
| } finally { | ||
| setIsCreatingProject(false); | ||
| } | ||
| }, | ||
| [trashedProjects, currentProject, globalTheme, upsertAndSetCurrentProject] | ||
| ); | ||
|
|
||
| /** | ||
| * Create a project from a custom GitHub URL | ||
| */ | ||
| const handleCreateFromCustomUrl = useCallback( | ||
| async (repoUrl: string, projectName: string, parentDir: string) => { | ||
| setIsCreatingProject(true); | ||
| try { | ||
| const httpClient = getHttpApiClient(); | ||
| const api = getElectronAPI(); | ||
|
|
||
| // Clone the repository | ||
| const cloneResult = await httpClient.templates.clone( | ||
| repoUrl, | ||
| projectName, | ||
| parentDir | ||
| ); | ||
|
|
||
| if (!cloneResult.success || !cloneResult.projectPath) { | ||
| toast.error("Failed to clone repository", { | ||
| description: cloneResult.error || "Unknown error occurred", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const projectPath = cloneResult.projectPath; | ||
|
|
||
| // Initialize .automaker directory with all necessary files | ||
| const initResult = await initializeProject(projectPath); | ||
|
|
||
| if (!initResult.success) { | ||
| toast.error("Failed to initialize project", { | ||
| description: initResult.error || "Unknown error occurred", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Update the app_spec.txt with basic info | ||
| // Note: Must follow XML format as defined in apps/server/src/lib/app-spec-format.ts | ||
| await api.writeFile( | ||
| `${projectPath}/.automaker/app_spec.txt`, | ||
| `<project_specification> | ||
| <project_name>${projectName}</project_name> | ||
|
|
||
| <overview> | ||
| This project was cloned from ${repoUrl}. | ||
| The AI agent will analyze the project structure. | ||
| </overview> | ||
|
|
||
| <technology_stack> | ||
| <!-- The AI agent will fill this in after analyzing your project --> | ||
| </technology_stack> | ||
|
|
||
| <core_capabilities> | ||
| <!-- List core features and capabilities --> | ||
| </core_capabilities> | ||
|
|
||
| <implemented_features> | ||
| <!-- The AI agent will populate this based on code analysis --> | ||
| </implemented_features> | ||
| </project_specification>` | ||
| ); |
There was a problem hiding this comment.
The XML content for app_spec.txt is duplicated in handleCreateBlankProject, handleCreateFromTemplate, and handleCreateFromCustomUrl. This makes it difficult to maintain and update the spec format consistently. A new file apps/server/src/lib/app-spec-format.ts was introduced on the server to centralize this, and a similar approach should be taken on the frontend.
You could create a helper function, for example getAppSpecXml(projectName, overview, templateDetails), that generates the XML content. This would centralize the template and make it easier to manage.
Example:
// in a new file, e.g., src/lib/app-spec-template.ts
export function getBlankAppSpec(projectName: string): string {
return `<project_specification>
<project_name>${projectName}</project_name>
...
</project_specification>`;
}Then, in sidebar.tsx, you would import and use this function.
| const handleCreateBlankProject = useCallback( | ||
| async (projectName: string, parentDir: string) => { | ||
| setIsCreatingProject(true); | ||
| try { | ||
| const api = getElectronAPI(); | ||
| const projectPath = `${parentDir}/${projectName}`; | ||
|
|
||
| // Create project directory | ||
| const mkdirResult = await api.mkdir(projectPath); | ||
| if (!mkdirResult.success) { | ||
| toast.error("Failed to create project directory", { | ||
| description: mkdirResult.error || "Unknown error occurred", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Initialize .automaker directory with all necessary files | ||
| const initResult = await initializeProject(projectPath); | ||
|
|
||
| if (!initResult.success) { | ||
| toast.error("Failed to initialize project", { | ||
| description: initResult.error || "Unknown error occurred", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Update the app_spec.txt with the project name | ||
| // Note: Must follow XML format as defined in apps/server/src/lib/app-spec-format.ts | ||
| await api.writeFile( | ||
| `${projectPath}/.automaker/app_spec.txt`, | ||
| `<project_specification> | ||
| <project_name>${projectName}</project_name> | ||
|
|
||
| <overview> | ||
| Describe your project here. This file will be analyzed by an AI agent | ||
| to understand your project structure and tech stack. | ||
| </overview> | ||
|
|
||
| <technology_stack> | ||
| <!-- The AI agent will fill this in after analyzing your project --> | ||
| </technology_stack> | ||
|
|
||
| <core_capabilities> | ||
| <!-- List core features and capabilities --> | ||
| </core_capabilities> | ||
|
|
||
| <implemented_features> | ||
| <!-- The AI agent will populate this based on code analysis --> | ||
| </implemented_features> | ||
| </project_specification>` | ||
| ); | ||
|
|
||
| const trashedProject = trashedProjects.find( | ||
| (p) => p.path === projectPath | ||
| ); | ||
| const effectiveTheme = | ||
| (trashedProject?.theme as ThemeMode | undefined) || | ||
| (currentProject?.theme as ThemeMode | undefined) || | ||
| globalTheme; | ||
| const project = upsertAndSetCurrentProject( | ||
| projectPath, | ||
| projectName, | ||
| effectiveTheme | ||
| ); | ||
|
|
||
| setShowNewProjectModal(false); | ||
|
|
||
| // Show onboarding dialog for new project | ||
| setNewProjectName(projectName); | ||
| setNewProjectPath(projectPath); | ||
| setShowOnboardingDialog(true); | ||
|
|
||
| toast.success("Project created", { | ||
| description: `Created ${projectName} with .automaker directory`, | ||
| }); | ||
| } catch (error) { | ||
| console.error("[Sidebar] Failed to create project:", error); | ||
| toast.error("Failed to create project", { | ||
| description: error instanceof Error ? error.message : "Unknown error", | ||
| }); | ||
| } finally { | ||
| setIsCreatingProject(false); | ||
| } | ||
| }, | ||
| [trashedProjects, currentProject, globalTheme, upsertAndSetCurrentProject] | ||
| ); | ||
|
|
||
| /** | ||
| * Create a project from a GitHub starter template | ||
| */ | ||
| const handleCreateFromTemplate = useCallback( | ||
| async ( | ||
| template: StarterTemplate, | ||
| projectName: string, | ||
| parentDir: string | ||
| ) => { | ||
| setIsCreatingProject(true); | ||
| try { | ||
| const httpClient = getHttpApiClient(); | ||
| const api = getElectronAPI(); | ||
|
|
||
| // Clone the template repository | ||
| const cloneResult = await httpClient.templates.clone( | ||
| template.repoUrl, | ||
| projectName, | ||
| parentDir | ||
| ); | ||
|
|
||
| if (!cloneResult.success || !cloneResult.projectPath) { | ||
| toast.error("Failed to clone template", { | ||
| description: cloneResult.error || "Unknown error occurred", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const projectPath = cloneResult.projectPath; | ||
|
|
||
| // Initialize .automaker directory with all necessary files | ||
| const initResult = await initializeProject(projectPath); | ||
|
|
||
| if (!initResult.success) { | ||
| toast.error("Failed to initialize project", { | ||
| description: initResult.error || "Unknown error occurred", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Update the app_spec.txt with template-specific info | ||
| // Note: Must follow XML format as defined in apps/server/src/lib/app-spec-format.ts | ||
| await api.writeFile( | ||
| `${projectPath}/.automaker/app_spec.txt`, | ||
| `<project_specification> | ||
| <project_name>${projectName}</project_name> | ||
|
|
||
| <overview> | ||
| This project was created from the "${template.name}" starter template. | ||
| ${template.description} | ||
| </overview> | ||
|
|
||
| <technology_stack> | ||
| ${template.techStack | ||
| .map((tech) => `<technology>${tech}</technology>`) | ||
| .join("\n ")} | ||
| </technology_stack> | ||
|
|
||
| <core_capabilities> | ||
| ${template.features | ||
| .map((feature) => `<capability>${feature}</capability>`) | ||
| .join("\n ")} | ||
| </core_capabilities> | ||
|
|
||
| <implemented_features> | ||
| <!-- The AI agent will populate this based on code analysis --> | ||
| </implemented_features> | ||
| </project_specification>` | ||
| ); | ||
|
|
||
| const trashedProject = trashedProjects.find( | ||
| (p) => p.path === projectPath | ||
| ); | ||
| const effectiveTheme = | ||
| (trashedProject?.theme as ThemeMode | undefined) || | ||
| (currentProject?.theme as ThemeMode | undefined) || | ||
| globalTheme; | ||
| const project = upsertAndSetCurrentProject( | ||
| projectPath, | ||
| projectName, | ||
| effectiveTheme | ||
| ); | ||
|
|
||
| setShowNewProjectModal(false); | ||
|
|
||
| // Show onboarding dialog for new project | ||
| setNewProjectName(projectName); | ||
| setNewProjectPath(projectPath); | ||
| setShowOnboardingDialog(true); | ||
|
|
||
| toast.success("Project created from template", { | ||
| description: `Created ${projectName} from ${template.name}`, | ||
| }); | ||
| } catch (error) { | ||
| console.error( | ||
| "[Sidebar] Failed to create project from template:", | ||
| error | ||
| ); | ||
| toast.error("Failed to create project", { | ||
| description: error instanceof Error ? error.message : "Unknown error", | ||
| }); | ||
| } finally { | ||
| setIsCreatingProject(false); | ||
| } | ||
| }, | ||
| [trashedProjects, currentProject, globalTheme, upsertAndSetCurrentProject] | ||
| ); | ||
|
|
||
| /** | ||
| * Create a project from a custom GitHub URL | ||
| */ | ||
| const handleCreateFromCustomUrl = useCallback( | ||
| async (repoUrl: string, projectName: string, parentDir: string) => { | ||
| setIsCreatingProject(true); | ||
| try { | ||
| const httpClient = getHttpApiClient(); | ||
| const api = getElectronAPI(); | ||
|
|
||
| // Clone the repository | ||
| const cloneResult = await httpClient.templates.clone( | ||
| repoUrl, | ||
| projectName, | ||
| parentDir | ||
| ); | ||
|
|
||
| if (!cloneResult.success || !cloneResult.projectPath) { | ||
| toast.error("Failed to clone repository", { | ||
| description: cloneResult.error || "Unknown error occurred", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const projectPath = cloneResult.projectPath; | ||
|
|
||
| // Initialize .automaker directory with all necessary files | ||
| const initResult = await initializeProject(projectPath); | ||
|
|
||
| if (!initResult.success) { | ||
| toast.error("Failed to initialize project", { | ||
| description: initResult.error || "Unknown error occurred", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Update the app_spec.txt with basic info | ||
| // Note: Must follow XML format as defined in apps/server/src/lib/app-spec-format.ts | ||
| await api.writeFile( | ||
| `${projectPath}/.automaker/app_spec.txt`, | ||
| `<project_specification> | ||
| <project_name>${projectName}</project_name> | ||
|
|
||
| <overview> | ||
| This project was cloned from ${repoUrl}. | ||
| The AI agent will analyze the project structure. | ||
| </overview> | ||
|
|
||
| <technology_stack> | ||
| <!-- The AI agent will fill this in after analyzing your project --> | ||
| </technology_stack> | ||
|
|
||
| <core_capabilities> | ||
| <!-- List core features and capabilities --> | ||
| </core_capabilities> | ||
|
|
||
| <implemented_features> | ||
| <!-- The AI agent will populate this based on code analysis --> | ||
| </implemented_features> | ||
| </project_specification>` | ||
| ); | ||
|
|
||
| const trashedProject = trashedProjects.find( | ||
| (p) => p.path === projectPath | ||
| ); | ||
| const effectiveTheme = | ||
| (trashedProject?.theme as ThemeMode | undefined) || | ||
| (currentProject?.theme as ThemeMode | undefined) || | ||
| globalTheme; | ||
| const project = upsertAndSetCurrentProject( | ||
| projectPath, | ||
| projectName, | ||
| effectiveTheme | ||
| ); | ||
|
|
||
| setShowNewProjectModal(false); | ||
|
|
||
| // Show onboarding dialog for new project | ||
| setNewProjectName(projectName); | ||
| setNewProjectPath(projectPath); | ||
| setShowOnboardingDialog(true); | ||
|
|
||
| toast.success("Project created from repository", { | ||
| description: `Created ${projectName} from ${repoUrl}`, | ||
| }); | ||
| } catch (error) { | ||
| console.error("[Sidebar] Failed to create project from URL:", error); | ||
| toast.error("Failed to create project", { | ||
| description: error instanceof Error ? error.message : "Unknown error", | ||
| }); | ||
| } finally { | ||
| setIsCreatingProject(false); | ||
| } | ||
| }, | ||
| [trashedProjects, currentProject, globalTheme, upsertAndSetCurrentProject] | ||
| ); |
There was a problem hiding this comment.
The Sidebar component has grown significantly and now contains complex business logic for creating projects (handleCreateBlankProject, handleCreateFromTemplate, handleCreateFromCustomUrl). This makes the component difficult to read, test, and maintain.
Consider refactoring this logic into a custom hook (e.g., useProjectCreation) or a dedicated service/context. This would separate concerns, making the Sidebar component primarily responsible for UI and state management, while the project creation logic is encapsulated elsewhere. This will improve the overall maintainability and testability of the codebase.
- Modified test cases in security.test.ts to reflect that all paths are allowed when permissions are disabled. - Updated descriptions of test cases to clarify the new behavior regarding path validation and error handling.
1 participant
morganfor enhanced request logging in the server.package-lock.jsonto include new dependencies and their types.NewProjectModalcomponent for improved readability and structure.FileBrowserDialogto support initial path selection and improved error handling.