Skip to content

Conversation

@dor-chaouat
Copy link
Contributor

Description

Load templates dynamically from the base44/apps-examples GitHub repository at runtime instead of bundling them in the CLI, allowing templates to be updated without releasing a new CLI version.

Related Issue

Fixes #38

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Other (please describe):

Changes Made

  • Create remote.ts module for fetching templates from GitHub
  • Fetch template list from base44/apps-examples repository
  • Cache templates locally in ~/.base44/templates/ with 24-hour TTL
  • Combine bundled and remote templates in listTemplates()
  • Support both bundled and remote templates in renderTemplate()
  • Add source field to Template schema for tracking origin
  • Fallback to bundled templates when offline

Testing

  • I have tested these changes locally
  • I have added/updated tests as needed
  • All tests pass (npm test)

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings
  • I have updated AGENTS.md if I made architectural changes

Additional Notes

Architecture

The CLI now:

  1. Tries to fetch templates from https://github.com/base44/apps-examples
  2. Caches them locally with 24-hour TTL
  3. Falls back to bundled templates if offline or fetch fails

Remote Repository Requirements

The base44/apps-examples repo should have either:

  • A templates.json file at the root listing templates
  • Or directories at the root that will be auto-discovered as templates

Cache Location

Templates are cached in ~/.base44/templates/ to avoid repeated downloads.

- Add remote template fetching from GitHub base44/apps-examples repository
- Cache templates locally in ~/.base44/templates/ with 24-hour TTL
- Combine bundled and remote templates in listTemplates()
- Support both bundled and remote templates in renderTemplate()
- Add 'source' field to Template schema for tracking origin
- Fallback to bundled templates when offline

The CLI now dynamically fetches templates from the apps-examples repo,
allowing templates to be updated without releasing a new CLI version.

Fixes #38
@claude
Copy link

claude bot commented Jan 17, 2026

Code Review: Remote Templates Feature

Summary

This PR implements dynamic template loading from the base44/apps-examples GitHub repository with local caching. The implementation is solid overall with good architecture, but there are several areas that need attention before merging.


Critical Issues 🔴

1. Tarball Extraction Logic Issues (src/core/project/remote.ts:172-198)

The extractTemplateFolder function has a critical bug:

  • The strip: 1 option removes the top-level directory from ALL paths
  • The filter function tries to match paths BEFORE stripping, but the extraction happens AFTER stripping
  • This means the template folder structure may not be preserved correctly

Recommendation: Fix the strip logic and path handling to properly extract the template subdirectory.

2. Race Condition in Template Download (src/core/project/remote.ts:119-166)

Multiple concurrent calls to downloadRemoteTemplate for the same template will trigger multiple downloads. The check-then-download pattern is not atomic.

Recommendation: Implement an in-memory promise cache to prevent concurrent downloads of the same template.

3. Incomplete Error Handling (src/core/project/template.ts:45-50)

When both bundled and remote templates fail to load, the error message is generic and doesn't expose the actual errors that occurred.

Recommendation: Capture and include the actual error messages to help with debugging.


High Priority Issues 🟡

4. Missing Resource Cleanup

The temporary tarball cleanup silently ignores errors (src/core/project/remote.ts:160-165), which could lead to disk space leaks over time.

Recommendation: At minimum, log cleanup failures. Better yet, use OS temp directories with auto-cleanup.

5. No Request Timeout or Retry Logic

Both GitHub API calls lack timeouts and retry logic (src/core/project/remote.ts:71, 127). This could cause hangs on slow networks or transient failures.

Recommendation: Add AbortController-based timeouts (10s) and implement basic retry logic for transient failures.

6. Cache Invalidation Issues

  • No mechanism to force refresh templates when users know new ones are available
  • Corrupted cache silently refreshes without user notification

Recommendation: Add a --refresh flag to CLI commands and expose cache management utilities.

7. Type Safety Concerns

The source field is optional in schema but treated as required in logic (src/core/project/template.ts:51).

Recommendation: Make source required with a default value in the Zod schema.


Security Concerns 🔒

8. Lack of Content Verification

Templates are fetched from GitHub without integrity checks:

  • No SHA verification of downloaded content
  • No validation of file types or sizes before extraction
  • Potential for zip bomb attacks via malicious tarballs

Recommendation: Add size limits and validation before extraction. Consider checksums in templates.json.

9. Path Traversal Risk

The tarball extraction doesn't explicitly prevent path traversal attacks using .. in file paths.

Recommendation: Add explicit path validation in the extraction filter to reject paths containing ...

10. Hardcoded API Endpoints

GitHub API endpoints are hardcoded, limiting enterprise users who may use GitHub Enterprise.

Recommendation: Make the API base URL configurable via environment variables.


Performance Considerations ⚡

11. Full Tarball Download for Single Template

The code downloads the entire repository tarball even when only one template is needed. For repos with many large templates, this wastes bandwidth and time.

Recommendation: Use GitHub's contents API to fetch individual directories, or document that apps-examples should be kept lightweight.

12. Synchronous Directory Checks

Multiple sequential pathExists checks could be parallelized for better performance.


Test Coverage ❌

13. No Tests for New Functionality

The PR adds 216 lines of complex logic in remote.ts but no tests:

  • No unit tests for listRemoteTemplates()
  • No tests for cache expiration logic
  • No tests for error handling paths
  • No integration tests for the full flow

Recommendation: Add comprehensive unit and integration tests covering cache behavior, network failures, extraction logic, and error scenarios.


Code Quality & Best Practices ✨

Positive Aspects ✅

  1. Good separation of concerns - Remote template logic is isolated in remote.ts
  2. Proper use of Zod schemas - Type-safe configuration parsing
  3. Fallback mechanism - Graceful degradation to bundled templates
  4. Clear documentation - Good JSDoc comments explaining functionality
  5. Cache strategy - 24-hour TTL is reasonable for this use case

Minor Improvements 🔧

  • Inconsistent error handling patterns (silent failures vs. throws)
  • Consider making CACHE_TTL_MS configurable via environment variable
  • Check if Readable and createWriteStream imports are still needed
  • Add explicit return type annotations to all exported functions

Recommendations Summary

Before merging (Critical):

  1. Fix the tarball extraction logic
  2. Add download race condition protection
  3. Improve error messages with details
  4. Add comprehensive tests

Before merging (High Priority):
5. Add request timeouts and retry logic
6. Implement proper resource cleanup
7. Add path traversal protection

Post-merge enhancements:

  • Add cache management CLI commands
  • Implement content integrity verification
  • Add metrics/logging for template downloads
  • Make GitHub API endpoint configurable
  • Optimize to download only needed templates

Conclusion

This is a valuable feature that improves the CLI's flexibility. The architecture is sound, but the implementation needs hardening before production use. The critical issues around tarball extraction and race conditions must be addressed, and test coverage is essential for such complex logic.

Overall Assessment: Needs Changes - Please address critical issues and add tests before merging.

Great work on the feature! Looking forward to the improvements. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants