-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add dashboard command to open app dashboard #82
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
- Add new 'dashboard' command under project commands - Opens browser to app dashboard page using platform-specific command - Throws error if BASE44_CLIENT_ID is not configured - Logs 'Opening dashboard...' message before opening - Falls back to displaying URL if browser opening fails Resolves #80 Co-authored-by: Kfir Stri <kfirstri@users.noreply.github.com>
🚀 Package Preview Available!Install this PR's preview build with npm: npm i @base44-preview/cli@0.0.11-pr.82.5adefb8Prefer not to change any import paths? Install using npm alias so your code still imports npm i "base44@npm:@base44-preview/cli@0.0.11-pr.82.5adefb8"Or add it to your {
"dependencies": {
"base44": "npm:@base44-preview/cli@0.0.11-pr.82.5adefb8"
}
}
Preview published to npm registry — try new features instantly! |
Code Review for PR #82 - Add dashboard commandThank you for this contribution! The implementation is generally solid and follows the repository's patterns. Below is my detailed review: ✅ Strengths
🐛 Potential Issues1. Security Concern: Shell Injection Risk (High Priority)Location: await execa(openCommand, [dashboardUrl], { shell: true });Issue: Using Recommendation: Remove await execa(openCommand, [dashboardUrl]);The array syntax already provides proper escaping, and 2. Windows Command Issue (High Priority)Location: } else if (platform === "win32") {
openCommand = "start";Issue: The Recommendation: Use a proper Windows approach: } else if (platform === "win32") {
openCommand = "cmd";
await execa(openCommand, ["/c", "start", '""', dashboardUrl]);Or better yet, use the 3. Missing Dependency (Medium Priority)Issue: The codebase doesn't use the industry-standard
Recommendation: Add npm install openThen simplify the implementation: import open from 'open';
// Replace lines 23-43 with:
try {
await open(dashboardUrl);
} catch (error) {
log.warn("Could not open browser automatically");
log.message(`${theme.styles.header("Dashboard")}: ${theme.colors.links(dashboardUrl)}`);
return {};
}This is much more robust and maintainable. 📝 Code Quality Observations1. Authentication Not RequiredLocation: await runCommand(openDashboard);Observation: Unlike the Recommendation: Consider adding: await runCommand(openDashboard, { requireAuth: true });2. Error Message FormattingLocation: The error message is clear but could be more concise: throw new Error(
"App not configured. BASE44_CLIENT_ID environment variable is required. Set it in your .env.local file."
);Suggestion: Consider matching the tone of other error messages in the codebase: throw new Error(
"App not configured. Set BASE44_CLIENT_ID in your .env.local file."
);🧪 Test CoverageIssue: No tests were added for this new command. Recommendation: Add tests in
Example test structure: describe('dashboard command', () => {
it('should throw error when BASE44_CLIENT_ID is not set', async () => {
// Test implementation
});
it('should construct correct dashboard URL', async () => {
// Test implementation
});
it('should handle browser open failure gracefully', async () => {
// Test implementation
});
});🎯 Performance ConsiderationsNo performance concerns identified. The command is lightweight and performs minimal operations. 📋 Summary of Recommended ChangesMust Fix (High Priority):
Should Fix (Medium Priority): Nice to Have (Low Priority): Overall AssessmentThis is a useful feature that follows the project's conventions well. The main concerns are the shell injection risk and Windows compatibility. Once those are addressed, this will be ready to merge! Let me know if you'd like help implementing any of these suggestions. |
Pull Request Review - Add Dashboard CommandSummaryThis PR adds a new ✅ Positives
🔍 Issues & Concerns1. Unused Imports (Code Quality)Severity: Minor The dashboard command imports import { log } from "@clack/prompts"; // ❌ Not used
import { execa } from "execa"; // ❌ Not used
import { runCommand, theme } from "../../utils/index.js"; // theme not usedRecommendation: Remove unused imports for cleaner code: import { Command } from "commander";
import open from 'open';
import { getBase44ApiUrl, getBase44ClientId, loadProjectEnv } from "@core/config.js";
import { runCommand } from "../../utils/index.js";
import type { RunCommandResult } from "../../utils/runCommand.js";2. Missing User Feedback (UX)Severity: Minor The command doesn't provide feedback to the user that it's opening the browser. Compared to similar operations in the codebase, users typically see a loading message. The issue description (#80) specifically requested: "write a nice log saying 'Opening dashboard...'" Current behavior: await open(dashboardUrl);
return { outroMessage: `Dashboard opened at ${dashboardUrl}` };Recommendation: Use await runTask(
"Opening dashboard...",
async () => {
await open(dashboardUrl);
},
{
successMessage: "Dashboard opened",
errorMessage: "Failed to open dashboard",
}
);3. Incorrect Dependency Declaration (Package Management)Severity: Medium The "devDependencies": {
// ...
"open": "^11.0.0" // ❌ Should be in dependencies
}Impact: When the CLI is installed via npm, Recommendation: Move "dependencies": {
"open": "^11.0.0"
},
"devDependencies": {
// ... existing dev dependencies
}4. Missing Authentication Check (Potential Issue)Severity: Low-Medium Other project-related commands like Current: await runCommand(openDashboard);Consider: await runCommand(openDashboard, { requireAuth: true });5. No Test CoverageSeverity: Medium The PR doesn't include any tests for the new command. While testing CLI commands can be tricky, the core logic should be testable. Recommendations:
Example test structure: describe('dashboard command', () => {
it('should throw error when BASE44_CLIENT_ID is not configured', async () => {
// Test implementation
});
it('should construct correct dashboard URL', async () => {
// Test implementation
});
it('should call open with correct URL', async () => {
// Test implementation
});
});🔒 SecurityNo security concerns identified. The command:
⚡ PerformanceNo performance concerns. The command is lightweight and the 📝 Code StyleThe code generally follows the project's conventions:
🎯 Recommendations Priority
🎉 ConclusionThis is a solid implementation that adds useful functionality. The main blocking issue is the incorrect dependency declaration which would cause runtime failures in production. Once that's fixed along with the unused imports and user feedback, this will be ready to merge. The code is clean, follows project patterns, and the feature request is well-addressed. Nice work! 🚀 Review generated with Claude Code |
Code Review for PR #82: Add dashboard commandThank you for this contribution! The implementation is clean and follows most of the project conventions. Here are my findings: ✅ Strengths
🔧 Issues & Recommendations1. Critical: Dependency in wrong section (src/cli/commands/project/dashboard.ts:2)The Fix needed in "dependencies": {
"open": "^11.0.0",
// ... other deps
},
"devDependencies": {
// Remove from here
}2. Style: Inconsistent quotes (src/cli/commands/project/dashboard.ts:2)Uses single quotes for the Change: import open from 'open'; // ❌
import open from "open"; // ✅3. Consideration: URL path consistency (src/cli/commands/project/dashboard.ts:19)The dashboard URL uses Question: Are these intentionally different endpoints? If so, this is fine. If they should be the same, consider using a consistent path or creating a shared helper function. 4. Enhancement: Error handling for
|
Pull Request ReviewSummaryThis PR adds a new ✅ Strengths
🔍 Code Quality ObservationsMinor Issues
Suggested ImprovementsFor the dependency issue: // Move from devDependencies to dependencies
"dependencies": {
"open": "^11.0.0"
}For better error handling: try {
await open(dashboardUrl);
return { outroMessage: `Dashboard opened at ${dashboardUrl}` };
} catch (error) {
// Fallback if browser opening fails (e.g., SSH session, headless environment)
return { outroMessage: `Could not open browser automatically. Visit: ${dashboardUrl}` };
}For clarity: const clientId = getBase44ClientId(); // or rename variable to match what it represents🧪 Test Coverage
🔒 Security
⚡ Performance
📝 Documentation
RecommendationApprove with minor changes suggested. The implementation is solid and follows best practices. The main issue is the dependency placement which should be fixed before merging. The error handling improvement is optional but would enhance user experience in edge cases. Great work overall! 🎉 |
Summary
Adds a new
dashboardcommand that opens the app's dashboard page in the user's browser.Changes
src/cli/commands/project/dashboard.tswith the new commandsrc/cli/index.tsResolves #80
Generated with Claude Code