feat(create with template): add an option to choose a template#16
feat(create with template): add an option to choose a template#16
Conversation
🚀 Package Preview Available!Install this PR's preview build with npm: npm i @base44-preview/cli@0.0.1-pr.16.6c1f17fPrefer 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.16.6c1f17f"Or add it to your {
"dependencies": {
"base44": "npm:@base44-preview/cli@0.0.1-pr.16.6c1f17f"
}
}
Preview published to npm registry — try new features instantly! |
Code Review - PR #16: Template-based Project CreationSummaryThis PR introduces a template system for project creation, replacing the simple ✅ StrengthsArchitecture & Design
Code Quality
|
PR Review: Template-based Project CreationThank you for this contribution! This is a solid refactoring that adds template support to the project creation flow. ✅ Strengths
🔴 Critical Security IssuePath Traversal Vulnerability (src/core/project/template.ts:42-54)
🐛 High Priority Bugs
🟡 Medium Priority Issues
🧪 Test CoverageMissing Tests:
Recommendation: Add test fixture template directory and test the full rendering pipeline 📝 Code Quality Suggestions
⚡ Performance
🔒 Security Summary
🎯 Architecture Notes
🚀 Recommended ActionsMust Fix Before Merge:
Should Fix: Nice to Have: 📊 Summary
Overall: Solid feature with good architecture. Main blocker is the path traversal security issue. Once fixed, ready to merge! Great work on this refactoring! Let me know if you have questions about any of the feedback. |
PR Review: Add Template-Based Project CreationSummaryThis PR successfully refactors the project initialization from a simple config generator to a template-based system. The changes are well-structured and follow existing codebase patterns. Overall, this is a solid implementation with a few areas for improvement. ✅ Strengths
🐛 Potential Bugs & IssuesCritical
Medium Priority
🔒 Security Concerns
🧪 Test CoverageMissing Tests for:
This is a significant gap that should be addressed. 📝 Code QualityGood: Type safety, async handling, error messages, code organization Areas for Improvement:
🎯 Template Issues
🔄 Migration PathThe removal of init.ts is a breaking change. Consider deprecation notice, migration docs, and that base44 init will break. 📊 Overall Assessment
✅ RecommendationsMust Fix (P0)
Should Fix (P1)
Nice to Have (P2)
Great work overall! The template system is a solid foundation. Once the security and testing gaps are addressed, this will be a robust feature. 🚀 |
Pull Request Review - PR #16SummaryThis PR replaces the ✅ Strengths
🐛 Potential Bugs & IssuesCritical Issues1. Race Condition in Directory Creation (src/core/project/create.ts:24-32) // Check if project already exists
const existingConfigs = await globby(getProjectConfigPatterns(), {
cwd: basePath,
absolute: true,
});Issue: If 2. Template Rendering Error Handling (src/core/project/template.ts:41-55) for (const file of files) {
const srcPath = join(templateDir, file);
// No try-catch, no rollback mechanism
}Recommendation: Wrap in try-catch and implement cleanup on failure, or consider creating in a temp directory first and moving on success. 3. Missing Path Validation (src/cli/commands/project/create.ts:47-54)
4. Template Path Injection Risk (src/core/project/template.ts:32) const templateDir = join(getTemplatesDir(), template.path);If if (template.path.includes('..') || isAbsolute(template.path)) {
throw new Error('Invalid template path');
}Minor Issues5. .ejs File Replacement is Brittle (src/core/project/template.ts:46) const destFile = file.replace(".ejs", "");This would incorrectly handle a file like const destFile = file.replace(/\.ejs$/, "");6. Empty Description Handling Inconsistency 7. Missing Type Assertion Safety (src/cli/commands/project/create.ts:61) const resolvedPath = resolve(projectPath as string);The if (typeof projectPath !== 'string') {
throw new Error('Invalid project path');
}🔒 Security Concerns
⚡ Performance Considerations
await Promise.all(files.map(async (file) => { /* process */ }));
🧪 Test CoverageMissing Tests - Critical gap:
The only existing test file ( Recommendations:
📝 Code Quality & Best PracticesGood Practices
Improvements Needed1. Magic String in template.path const TEMPLATE_EXTENSION = '.ejs';2. Missing JSDoc Comments 3. Inconsistent Naming
4. Prompt Utils Refactoring (src/cli/utils/prompts.ts) 5. Type Safety 📋 Additional Recommendations
🎯 VerdictThis is a solid feature addition with good architecture, but should not be merged without addressing:
The template system design is sound and extensible. Once the critical issues are addressed and tests are added, this will be a valuable improvement to the CLI. Overall Assessment: 🟡 Needs Work - Good foundation, but requires tests and bug fixes before merge. |
PR Review: Template-based Project CreationSummaryThis PR successfully refactors the project initialization system from a simple config+env generation to a flexible template-based approach. The implementation is solid with good separation of concerns, but there are several areas that need attention before merging. Critical Issues1. Security: Missing Path Traversal Validation in User Input 🔴Location: The const resolvedPath = resolve(projectPath as string);While Recommendation: // Add validation before resolve
if (projectPath.includes('..') || isAbsolute(projectPath) && !projectPath.startsWith(process.cwd())) {
throw new Error('Invalid project path');
}
const resolvedPath = resolve(projectPath as string);2. API Call Before File System Check 🟡Location: The code creates a project via API before checking if the destination path is writable or if template rendering will succeed. If template rendering fails, you'll have orphaned projects in the database. Recommendation: Reverse the order or use a transaction pattern: // Validate template can be rendered first (dry-run or path checks)
await validateTemplateAccess(template);
// Then create the project
const { projectId } = await createProject(name, description);
// Then render with rollback on failure
try {
await renderTemplate(template, basePath, { name, description, projectId });
} catch (error) {
// Consider: await deleteProject(projectId); // rollback
throw error;
}Code Quality Issues3. Missing Error Handling in Template Rendering 🟡Location: The loop processes template files but doesn't handle partial failures well. If file 50 out of 100 fails, you get a partially created project with no cleanup. Recommendation:
4. Type Assertion Could Be Avoided 🟡Location: const resolvedPath = resolve(projectPath as string);The 5. Empty Template Files 🟡Location: The backend-only template has empty
6. Inconsistent Naming: "Project" vs "App" 🔵Location: Multiple files
Recommendation: Standardize terminology throughout the codebase. Missing Test Coverage7. No Tests for New Functionality 🔴The PR adds significant new functionality but doesn't update tests:
Existing test file ( Recommendation: Add comprehensive tests: describe("Template System", () => {
describe("listTemplates", () => {
it("returns all available templates");
it("validates template schema");
});
describe("renderTemplate", () => {
it("renders .ejs files correctly");
it("copies non-.ejs files");
it("rejects path traversal attempts");
it("handles missing template directory");
it("cleans up on render failure");
});
describe("createProjectFiles", () => {
it("creates project with backend-only template");
it("creates project with backend-and-client template");
it("rejects duplicate project in same location");
it("handles API failures gracefully");
});
});Performance Considerations8. Sequential File Processing 🔵Location: Files are processed sequentially in a for loop. For templates with many files (backend-and-client has 33 files), this could be slow. Recommendation: Consider parallel processing: await Promise.all(files.map(async (file) => {
// ... processing logic
}));Though be careful with file system pressure - might want to batch in groups of 10-20. Best Practices & Polish9. Missing JSDoc for Public APIs 🔵
10. Template Validation 🟡Location:
Recommendation: export async function listTemplates(): Promise<Template[]> {
const configPath = join(getTemplatesDir(), "templates.json");
const parsed = await readJsonFile(configPath);
const result = TemplatesConfigSchema.parse(parsed);
// Verify each template directory exists
for (const template of result.templates) {
const templateDir = join(getTemplatesDir(), template.path);
if (!(await pathExists(templateDir))) {
throw new Error(`Template directory not found: ${template.path}`);
}
}
return result.templates;
}11. Consider Adding a .env.local to Backend-Only Template 🔵The old Positive Highlights ✅
Recommendations SummaryMust Fix Before Merge:
Should Fix: Nice to Have: VerdictThis is a well-structured refactor that sets up a good foundation for template-based project creation. However, the security issue with path validation and lack of test coverage are blockers. Once those are addressed, this will be ready to merge. Estimated effort to address critical issues: 2-3 hours |
1 participant
Summary
Choose a template from a list to generate a new project