feat(cli): Introduced start:flutter command for Flutter integration, added --write-env-file option to start#4972
feat(cli): Introduced start:flutter command for Flutter integration, added --write-env-file option to start#4972pavelgj wants to merge 1 commit intogenkit-ai:mainfrom
start:flutter command for Flutter integration, added --write-env-file option to start#4972Conversation
… add `--write-env-file` option to `start`
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Genkit CLI by introducing dedicated support for Flutter development through a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new start:flutter command for better integration with Flutter projects and adds a --write-env-file option to the existing start command. The changes look good overall, but there are a few areas for improvement regarding code duplication, correctness, and efficiency. I've identified a high-severity issue where the --noui flag is not handled in the new start:flutter command, and another high-severity efficiency issue where a function is called unnecessarily when using --write-env-file. Additionally, there are opportunities to clean up unused imports, simplify complex logic, and remove dead code. Please see my detailed comments for suggestions.
| let port: number; | ||
| if (options.port) { | ||
| port = Number(options.port); | ||
| if (isNaN(port) || port < 0) { | ||
| logger.error(`"${options.port}" is not a valid port number`); | ||
| return; | ||
| } | ||
| } else { | ||
| port = await getPort({ port: makeRange(4000, 4099) }); | ||
| } | ||
| startServer(manager, port); | ||
| if (options.open) { | ||
| open(`http://localhost:${port}`); | ||
| } |
There was a problem hiding this comment.
The --noui option is defined for this command but it's not being handled. The Dev UI server is started unconditionally. This logic should be wrapped in a check for !options.noui to respect the user's choice, similar to how it's handled in the start command.
| let port: number; | |
| if (options.port) { | |
| port = Number(options.port); | |
| if (isNaN(port) || port < 0) { | |
| logger.error(`"${options.port}" is not a valid port number`); | |
| return; | |
| } | |
| } else { | |
| port = await getPort({ port: makeRange(4000, 4099) }); | |
| } | |
| startServer(manager, port); | |
| if (options.open) { | |
| open(`http://localhost:${port}`); | |
| } | |
| if (!options.noui) { | |
| let port: number; | |
| if (options.port) { | |
| port = Number(options.port); | |
| if (isNaN(port) || port < 0) { | |
| logger.error(`"${options.port}" is not a valid port number`); | |
| return; | |
| } | |
| } else { | |
| port = await getPort({ port: makeRange(4000, 4099) }); | |
| } | |
| startServer(manager, port); | |
| if (options.open) { | |
| open(`http://localhost:${port}`); | |
| } | |
| } |
| let envVars: Record<string, string> | undefined; | ||
| if (options.writeEnvFile) { | ||
| const devEnv = await getDevEnvVars(projectRoot, { | ||
| disableRealtimeTelemetry: options.disableRealtimeTelemetry, | ||
| corsOrigin: options.corsOrigin, | ||
| experimentalReflectionV2: options.experimentalReflectionV2, | ||
| }); | ||
| envVars = devEnv.envVars; | ||
| const content = Object.entries(envVars) | ||
| .map(([k, v]) => `${k}=${v}`) | ||
| .join('\n'); | ||
| fs.writeFileSync(options.writeEnvFile, content); | ||
| logger.info(`Wrote environment variables to ${options.writeEnvFile}`); | ||
| } |
There was a problem hiding this comment.
When using the --write-env-file option, getDevEnvVars is called here, but only envVars is stored. The other returned values (reflectionV2Port and telemetryServerUrl) are discarded. This leads to startDevProcessManager calling getDevEnvVars again, which is inefficient. You should store all returned values and pass them to startDevProcessManager to avoid the redundant call.
| let envVars: Record<string, string> | undefined; | |
| if (options.writeEnvFile) { | |
| const devEnv = await getDevEnvVars(projectRoot, { | |
| disableRealtimeTelemetry: options.disableRealtimeTelemetry, | |
| corsOrigin: options.corsOrigin, | |
| experimentalReflectionV2: options.experimentalReflectionV2, | |
| }); | |
| envVars = devEnv.envVars; | |
| const content = Object.entries(envVars) | |
| .map(([k, v]) => `${k}=${v}`) | |
| .join('\n'); | |
| fs.writeFileSync(options.writeEnvFile, content); | |
| logger.info(`Wrote environment variables to ${options.writeEnvFile}`); | |
| } | |
| let envVars: Record<string, string> | undefined; | |
| let reflectionV2Port: number | undefined; | |
| let telemetryServerUrl: string | undefined; | |
| if (options.writeEnvFile) { | |
| const devEnv = await getDevEnvVars(projectRoot, { | |
| disableRealtimeTelemetry: options.disableRealtimeTelemetry, | |
| corsOrigin: options.corsOrigin, | |
| experimentalReflectionV2: options.experimentalReflectionV2, | |
| }); | |
| envVars = devEnv.envVars; | |
| reflectionV2Port = devEnv.reflectionV2Port; | |
| telemetryServerUrl = devEnv.telemetryServerUrl; | |
| const content = Object.entries(envVars) | |
| .map(([k, v]) => `${k}=${v}`) | |
| .join('\n'); | |
| fs.writeFileSync(options.writeEnvFile, content); | |
| logger.info(`Wrote environment variables to ${options.writeEnvFile}`); | |
| } |
| disableRealtimeTelemetry: options.disableRealtimeTelemetry, | ||
| corsOrigin: options.corsOrigin, | ||
| experimentalReflectionV2: options.experimentalReflectionV2, | ||
| envVars, |
There was a problem hiding this comment.
Following up on my other comment, to prevent the redundant call to getDevEnvVars, you should also pass reflectionV2Port and telemetryServerUrl to startDevProcessManager if they have been calculated. The options object should be updated to include them.
envVars,
reflectionV2Port,
telemetryServerUrl,| import fs from 'fs'; | ||
| import getPort, { makeRange } from 'get-port'; | ||
| import open from 'open'; | ||
| import os from 'os'; | ||
| import path from 'path'; |
There was a problem hiding this comment.
These imports for fs, os, and path are not used within the file. To maintain code cleanliness, they should be removed.
| interface FlutterRunOptions { | ||
| port?: string; | ||
| open?: boolean; | ||
| corsOrigin?: string; | ||
| } |
There was a problem hiding this comment.
The FlutterRunOptions interface is incomplete. It's missing options defined for the command, such as noui and disableRealtimeTelemetry. This can lead to type safety issues and makes the code harder to understand. Please add the missing properties.
| interface FlutterRunOptions { | |
| port?: string; | |
| open?: boolean; | |
| corsOrigin?: string; | |
| } | |
| interface FlutterRunOptions { | |
| noui?: boolean; | |
| port?: string; | |
| open?: boolean; | |
| corsOrigin?: string; | |
| disableRealtimeTelemetry?: boolean; | |
| } |
| const { | ||
| envVars: calculatedEnvVars, | ||
| reflectionV2Port: calculatedReflectionV2Port, | ||
| telemetryServerUrl: calculatedTelemetryServerUrl, | ||
| } = options?.envVars && | ||
| options?.telemetryServerUrl && | ||
| (!options?.experimentalReflectionV2 || options?.reflectionV2Port) | ||
| ? { | ||
| envVars: options.envVars, | ||
| reflectionV2Port: options.reflectionV2Port, | ||
| telemetryServerUrl: options.telemetryServerUrl, | ||
| } | ||
| : await getDevEnvVars(projectRoot, options); | ||
|
|
||
| const envVars = options?.envVars || calculatedEnvVars; | ||
| const reflectionV2Port = options?.reflectionV2Port || calculatedReflectionV2Port; | ||
| const telemetryServerUrl = | ||
| options?.telemetryServerUrl || calculatedTelemetryServerUrl; |
There was a problem hiding this comment.
The logic for determining envVars, reflectionV2Port, and telemetryServerUrl is a bit complex and contains redundant assignments. This can be simplified to improve readability and maintainability by removing the intermediate calculated... variables and the subsequent || assignments.
| const { | |
| envVars: calculatedEnvVars, | |
| reflectionV2Port: calculatedReflectionV2Port, | |
| telemetryServerUrl: calculatedTelemetryServerUrl, | |
| } = options?.envVars && | |
| options?.telemetryServerUrl && | |
| (!options?.experimentalReflectionV2 || options?.reflectionV2Port) | |
| ? { | |
| envVars: options.envVars, | |
| reflectionV2Port: options.reflectionV2Port, | |
| telemetryServerUrl: options.telemetryServerUrl, | |
| } | |
| : await getDevEnvVars(projectRoot, options); | |
| const envVars = options?.envVars || calculatedEnvVars; | |
| const reflectionV2Port = options?.reflectionV2Port || calculatedReflectionV2Port; | |
| const telemetryServerUrl = | |
| options?.telemetryServerUrl || calculatedTelemetryServerUrl; | |
| const { | |
| envVars, | |
| reflectionV2Port, | |
| telemetryServerUrl, | |
| } = (options?.envVars && | |
| options?.telemetryServerUrl && | |
| (!options?.experimentalReflectionV2 || options?.reflectionV2Port)) | |
| ? { | |
| envVars: options.envVars, | |
| reflectionV2Port: options.reflectionV2Port, | |
| telemetryServerUrl: options.telemetryServerUrl, | |
| } | |
| : await getDevEnvVars(projectRoot, options); |
| if (this.appProcess?.stdin) { | ||
| process.stdin.unpipe(this.appProcess.stdin); | ||
| } | ||
| this.originalStdIn = undefined; |
There was a problem hiding this comment.
The originalStdIn property is no longer used after refactoring the process spawning logic. This line, along with the property's declaration, can be removed to clean up dead code.
1 participant
Checklist (if applicable):