-
Notifications
You must be signed in to change notification settings - Fork 1
feat(create-app): add a create app command to init a new base44 project #14
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
Conversation
🚀 Package Preview Available!Install this PR's preview build with npm: npm i @base44-preview/cli@0.0.1-pr.14.3c9bc6fPrefer not to change any import paths? Install using npm alias so your code still imports npm i "base44@npm:@base44-preview/cli@0.0.1-pr.14.3c9bc6f"Or add it to your {
"dependencies": {
"base44": "npm:@base44-preview/cli@0.0.1-pr.14.3c9bc6f"
}
}
Preview published to npm registry — try new features instantly! |
Pull Request Review: Add Create App CommandSummaryThis PR adds a new 🟢 StrengthsGood Architecture
Code Quality
🟡 Issues & Concerns1. CRITICAL: Broken Test Import
|
Pull Request Review: feat(create-app) - Add create app commandThank you for this contribution! Overall, this is a well-structured implementation with good separation of concerns. ✅ Strengths
🐛 Critical Issues1. Inconsistent .env.local location (src/core/project/init.ts:32)
2. getBase44ClientId() validation missing (src/core/utils/httpClient.ts:80)
3. Duplicate template files
4. Template path resolution (src/core/project/templates/index.ts:7-8)
🐛 Moderate Issues
🔒 Security Concerns (Low Risk)
⚡ Performance
🧪 Test CoverageNO TESTS for new functionality:
Recommended: Add tests for project creation, existing project detection, template rendering, error handling, and path resolution. 📋 Code Quality Suggestions
📦 DependenciesAll appropriate: dotenv@17.2.3, ejs@3.1.10, @types/ejs@3.1.5 🎯 Summary: Approve with Changes RequestedMust fix before merge:
Should fix: Great work! Once critical issues are addressed, this will be a solid addition. 🚀 |
Pull Request Review: Create App CommandThis PR adds a 'base44 init' command to initialize new Base44 projects. The implementation is generally solid with good separation of concerns. Strengths
CRITICAL: Template Path Resolution IssueLocation: src/core/project/templates/index.ts:5-8 After bundling, import.meta.url points to the bundled output, but templates are copied to dist/cli/templates/. The path resolution assumes templates are in a templates/ subdirectory relative to the module location, which may not match the actual bundled structure. This will cause template rendering to FAIL at runtime. Recommendation: Verify the actual bundled structure and test template resolution. Consider using explicit paths based on the known bundle structure. Minor Issues
Security Concerns
Test CoverageMissing tests for new initProject() function. Need tests for:
Recommendations Before Merge
Overall AssessmentSolid implementation following existing patterns. Main concern is template path resolution. Code is readable, well-structured, good UX. Rating: 7.5/10 (9/10 after fixing template path) Review generated by Claude Code |
PR Review: feat(create-app): add a create app command to init a new base44 projectOverviewThis PR adds a base44 init command to initialize new Base44 projects. The implementation creates project structure, makes an API call to register the project, and generates config files from templates. Overall, this is a solid foundation, but there are several issues that should be addressed. Critical Issues1. Breaking Change: Function Signature ChangeLocation: src/core/config.ts:61 The function getAppId() was renamed to getBase44ClientId() and the behavior changed:
Location Affected: src/core/utils/httpClient.ts:80 Risk: If getBase44ClientId() returns undefined, the URL becomes /api/apps/undefined/, which will fail silently or with confusing errors. Recommendation: Add explicit checks before using it in getAppClient(). 2. .env.local Placement InconsistencyLocation: src/core/project/init.ts:32 The .env.local file is created inside the base44/ subdirectory but loadProjectEnv() looks for it at the project root (src/core/config.ts:44). Risk: The created .env.local file will not be found by loadProjectEnv(). Recommendation: Clarify where .env.local should live and make it consistent. 3. Inconsistent Error HandlingLocation: src/core/project/init.ts:40-44 The initProject function checks for existing projects before calling the API, but if the API call fails, no rollback occurs. This could leave the user in an inconsistent state. Recommendation: Add error handling to rollback or provide clearer error messages. Major Issues4. Race Condition in loadProjectEnvLocation: src/core/config.ts:35-46 The function does not prevent multiple simultaneous calls. Add a guard variable to ensure it only loads once. 5. Missing Test CoverageNo tests were added for the new functionality. Recommend adding unit tests for initProject(), createProject(), template rendering, and error cases. 6. Missing Validation for Empty DescriptionLocation: src/cli/commands/project/init.ts:26-29 The description prompt allows empty strings. Trim and convert empty strings to undefined. Minor Issues7. Security: Template InjectionLocation: src/core/project/api.ts:8 The description parameter is interpolated directly without sanitization. Remove quotes from the template string. 8. Potential Path TraversalLocation: src/cli/commands/project/init.ts:39 Add validation to prevent creating projects in system directories. 9. Missing JSDocAdd documentation for createProject(), renderConfigTemplate(), and renderEnvTemplate(). Positive Aspects
Recommendations SummaryMust Fix (Before Merge):
Should Fix: Great work on this feature! The architecture is solid. Addressing the critical issues will make this ready to merge. |
Pull Request Review: feat(create-app): add a create app command to init a new base44 projectSummaryThis PR adds a new 🐛 Critical Issues1. Template Path Resolution Bug (High Priority)Location: The template path resolution has a critical bug that will cause runtime failures after bundling. The templates are nested in Recommendation: Either copy templates to 2. Empty Description HandlingLocation: The Recommendation: Add 3. Error Handling: API Call Before File System OperationsLocation: The code calls the API to create a project before writing files. If the file write fails after the API call succeeds, you've created an orphaned project on the server with no local configuration. Recommendation: Add cleanup logic or transaction-like behavior to handle failures gracefully. (Review continues in next comment...) |
|
🔍 Code Quality Issues6. Missing Error Context in fs.tsLocation: The refactored Recommendation: Restore the try-catch blocks with contextual error messages. 7. Redundant Error HandlingLocation: The Recommendation: Follow the pattern in 8. Inconsistent Banner UsageLocation: The Recommendation: Either use 9. Circular Dependency RiskLocation: This creates a circular dependency: While this might work in practice due to how ES modules handle cycles, it's fragile and can cause initialization issues. Recommendation: Move 10. Missing Return Type on API FunctionLocation: The |
📝 Minor Issues & Suggestions11. Missing TestsNo tests were added for the new functionality. Consider adding tests for:
12. Missing Newline at End of FileLocation: 13. Success Message Uses Untrimmed NameLocation: Uses ✅ What's Done Well
🧪 Test CoverageMissing test coverage for the new functionality. Consider adding unit tests for RecommendationRequest Changes - The template path resolution bug (issue #1) is critical and will cause runtime failures after bundling. The security issues around template injection (issue #4) and missing .gitignore (issue #5) also need to be addressed before merging. Happy to discuss any of these points or provide more specific code examples if helpful! |
No description provided.