-
Notifications
You must be signed in to change notification settings - Fork 1
feat(cli): log dashboard link and default login to false #22
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/[email protected]Prefer not to change any import paths? Install using npm alias so your code still imports npm i "base44@npm:@base44-preview/[email protected]"Or add it to your {
"dependencies": {
"base44": "npm:@base44-preview/[email protected]"
}
}
Preview published to npm registry — try new features instantly! |
Pull Request Review: feat(cli): log dashboard link and default login to falseSummaryThis PR adds two features:
Code Quality ✅The code changes are clean and well-structured:
Issues Found 🔴1. Hardcoded Preview URL (Critical)Location: The dashboard URL is hardcoded to log.success(`Dashboard link:\n${chalk.bold(`https://pr-2624.velino.org/apps/${projectId}/editor/preview`)}`);Problem: This will break in production and point users to the wrong environment. Recommendation: Use the same environment-aware approach as the API client: const apiUrl = getBase44ApiUrl(); // Returns BASE44_API_URL or default
const dashboardUrl = apiUrl.replace('app.base44.com', 'app.base44.com'); // Or separate env var
log.success(`Dashboard link:\n${chalk.bold(`${dashboardUrl}/apps/${projectId}/editor/preview`)}`);Consider adding a 2. No Validation for
|
Code Review for PR #22: feat(cli): log dashboard link and default login to falseSummaryThis PR adds two improvements to the project creation flow:
Positive Aspects ✅
Issues & Suggestions 🔍1. Hardcoded Default Behavior (High Priority)Location: public_settings: "public_without_login"Issue: This hardcodes all new projects to be public without login. This is a significant security default that:
Suggestions:
2. Missing Type Safety for public_settingsLocation: Issue: The value Suggestion: Consider creating a type or enum: type PublicSettings = "public_without_login" | "public_with_login" | "private";3. URL Construction Without ValidationLocation: `https://app.base44.com/apps/${projectId}/editor/preview`Issue: The Suggestions:
4. Inconsistent Message FormattingLocation: log.success(`Dashboard link:\n${chalk.bold(`https://app.base44.com/apps/${projectId}/editor/preview`)}`);Issue: The newline Suggestion: log.success(`Project ${chalk.bold(name)} has been initialized!`);
log.info(`Dashboard: ${chalk.bold(`https://app.base44.com/apps/${projectId}/editor/preview`)}`);This provides visual separation and uses a more semantic log level for informational content. 5. Missing Test CoverageIssue: No tests were added for:
Suggestion: Add tests to
6. Documentation GapIssue: The PR description is empty, making it unclear:
Suggestion: Add context in the PR description explaining the rationale. Performance Considerations ⚡No performance concerns identified. The changes are minimal and don't introduce any expensive operations. Security Concerns 🔒Critical: The most significant concern is the security implication of defaulting to
Recommendation: This should be either:
Additional NotesConsider adding:
Overall AssessmentThe code quality is good with proper TypeScript usage, but the security implications of the default behavior need attention. I'd recommend addressing the public-by-default behavior before merging. Would you like me to suggest specific implementation approaches for any of these items? |
kfirstri
left a comment
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.
🚀
Pull Request ReviewOverviewThis PR adds a dashboard link to the output after project creation and defaults new projects to ✅ Positive Aspects
🐛 Issues Found1. Unused Import (src/cli/commands/project/create.ts:10)The // Current:
import { runCommand, runTask, printBanner, onPromptCancel } from "../../utils/index.js";
// Should be:
import { runCommand, runTask, onPromptCancel } from "../../utils/index.js";
|
Pull Request ReviewSummaryThis PR adds two features:
Code Quality & Best Practices✅ Strengths
|
No description provided.