Conversation
Summary of ChangesHello @christhompsongoogle, 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 introduces a robust mechanism for migrating projects from Firebase Studio to Antigravity. It streamlines the transition by automating the setup of development environments, integrating necessary Antigravity components, and ensuring project configurations are aligned with the new platform. The primary goal is to provide a smooth path for users to move their existing Firebase Studio applications, starting with Next.js projects, into the Antigravity ecosystem. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 studio:export command to migrate Firebase Studio projects. The implementation is quite comprehensive, covering metadata extraction, file transformations, and setting up the new project structure.
My review focuses on improving robustness, maintainability, and adherence to repository best practices. Key areas for improvement include:
- Handling of command-line arguments, which should be done through the
commanderframework instead of directprocess.argvparsing. - More robust error handling, especially for missing project IDs.
- Addressing potential failures from unauthenticated GitHub API calls.
- Improving test strategy and code style in line with the repository's guidelines.
- Minor improvements for consistency and maintainability in build scripts and code.
| } | ||
|
|
||
| // TODO revisit quota limits | ||
| async function downloadGitHubDir(apiUrl: string, localPath: string): Promise<void> { |
There was a problem hiding this comment.
The downloadGitHubDir function makes unauthenticated requests to the GitHub API. These requests are subject to a strict rate limit (60 requests per hour per IP). If a project's migration involves fetching many files (e.g., from multiple skills), this limit could be easily reached, causing the command to fail unexpectedly.
Consider adding support for using an authenticated GitHub token (e.g., via an environment variable GITHUB_TOKEN) to make authenticated API requests, which have a much higher rate limit. At a minimum, it would be good to warn the user about potential rate-limiting issues.
| } else { | ||
| // TODO need a mitigation here | ||
| logger.info(`✅ Failed to determine the Firebase Project ID`); | ||
| } |
There was a problem hiding this comment.
When a projectId cannot be determined, the function logs a message but continues execution. Subsequent steps either skip operations or use default values, which can result in a partially configured and non-functional project. This can be confusing for the user.
If a projectId is essential for a successful migration, it's better to throw a FirebaseError to fail fast and provide a clear error message, as recommended by the style guide.
throw new FirebaseError(
"Could not determine a Firebase project ID. Please run `firebase use <project_id>` or specify a project with the --project flag.",
{ exit: 1 },
);References
- Throw
FirebaseError(src/error.ts) for expected, user-facing errors. If the error is due to a violation of a precondition (e.g. something that is null but should never be), specify a non-zero exit code. (link)
src/firebase_studio/migrate.ts
Outdated
| const args = process.argv.slice(2); | ||
| const noStartAgyFlag = args.includes("--nostart_agy"); |
There was a problem hiding this comment.
The migrate function directly parses process.argv to check for the --nostart_agy flag. This is brittle and bypasses the command-line argument parsing handled by commander. For instance, this will fail to correctly parse flags if they are not in the expected position.
This flag should be defined as an option on the studio:export command and its value should be passed down to the migrate function through the options object.
Here's how you can refactor this:
-
In
src/commands/studio-export.ts, add the option to the command and pass it tomigrate:export const command = new Command("studio:export [path]") .description("export Firebase Studio apps for migration to Antigravity") .option("--no-start-agy", "Do not start agy after export") .action(async (exportPath: string | undefined, options: Options) => { // ... await migrate(rootPath, options.noStartAgy as boolean); });
-
In this file (
src/firebase_studio/migrate.ts), update themigratefunction signature on line 406 toexport async function migrate(rootPath: string, noStartAgyFlag = false): Promise<void> {and remove these lines.
package.json
Outdated
| "build:watch": "npm run build && tsc --watch", | ||
| "clean": "node -e \"fs.rmSync('lib', { recursive: true, force: true }); fs.rmSync('dev', { recursive: true, force: true });\"", | ||
| "copyfiles": "node -e \"const fs = require('fs'); fs.mkdirSync('./lib', {recursive:true}); fs.copyFileSync('./src/dynamicImport.js', './lib/dynamicImport.js')\"", | ||
| "copyfiles": "node -e \"const fs = require('fs'); fs.mkdirSync('./lib/firebase_studio', {recursive:true}); fs.copyFileSync('./src/dynamicImport.js', './lib/dynamicImport.js'); fs.cpSync('./src/firebase_studio', './lib/firebase_studio', {recursive: true, filter: (src) => fs.statSync(src).isDirectory() || src.endsWith('.md')});\"", |
There was a problem hiding this comment.
The copyfiles script has become quite long and complex, which reduces readability and makes it harder to maintain directly within package.json. It's generally better to move complex inline scripts to separate files.
| "copyfiles": "node -e \"const fs = require('fs'); fs.mkdirSync('./lib/firebase_studio', {recursive:true}); fs.copyFileSync('./src/dynamicImport.js', './lib/dynamicImport.js'); fs.cpSync('./src/firebase_studio', './lib/firebase_studio', {recursive: true, filter: (src) => fs.statSync(src).isDirectory() || src.endsWith('.md')});\"", | |
| "copyfiles": "node scripts/copy-files.js", |
src/firebase_studio/migrate.spec.ts
Outdated
| it("should perform a full migration successfully", async function() { | ||
| this.timeout(5000); | ||
|
|
||
| // Stub global fetch | ||
| const fetchStub = sandbox.stub(global, "fetch"); | ||
|
|
||
| // Mock GitHub API for skills listing | ||
| fetchStub.withArgs("https://api.github.com/repos/firebase/agent-skills/contents/skills") | ||
| .resolves({ | ||
| ok: true, | ||
| json: async () => [ | ||
| { name: "test-skill", type: "dir", url: "https://api.github.com/repos/firebase/agent-skills/contents/skills/test-skill" } | ||
| ] | ||
| } as any); | ||
|
|
||
| // Mock GitHub API for specific skill content | ||
| fetchStub.withArgs("https://api.github.com/repos/firebase/agent-skills/contents/skills/test-skill") | ||
| .resolves({ | ||
| ok: true, | ||
| json: async () => [] | ||
| } as any); | ||
|
|
||
| // Mock GitHub API for Genkit skill content | ||
| fetchStub.withArgs("https://api.github.com/repos/genkit-ai/skills/contents/skills/developing-genkit-js?ref=main") | ||
| .resolves({ | ||
| ok: true, | ||
| json: async () => [] | ||
| } as any); | ||
|
|
||
| // Mock filesystem | ||
| sandbox.stub(fs, "readFile").callsFake(async (p: any) => { | ||
| const pStr = p.toString(); | ||
| if (pStr.endsWith("metadata.json")) { | ||
| return JSON.stringify({ projectId: "test-project", appName: "Test App" }); | ||
| } | ||
| if (pStr.endsWith("readme_template.md")) { | ||
| return "# ${appName}\nExport Date: ${exportDate}\n${blueprintContent}"; | ||
| } | ||
| if (pStr.endsWith("system_instructions.md")) { | ||
| return "Project: ${appName}"; | ||
| } | ||
| if (pStr.endsWith("startup_workflow.md")) { | ||
| return "Step 1: Build"; | ||
| } | ||
| if (pStr.endsWith(".firebaserc")) { | ||
| return JSON.stringify({ projects: { default: "test-project" } }); | ||
| } | ||
| if (pStr.endsWith("blueprint.md")) { | ||
| return "# **App Name**: Test App\nSome blueprint content"; | ||
| } | ||
| throw new Error(`Unexpected readFile: ${pStr}`); | ||
| }); | ||
|
|
||
| sandbox.stub(fs, "writeFile").resolves(); | ||
| sandbox.stub(fs, "mkdir").resolves(); | ||
| sandbox.stub(fs, "unlink").resolves(); | ||
| sandbox.stub(fs, "readdir").resolves([]); | ||
| sandbox.stub(fs, "access").rejects({ code: "ENOENT" }); | ||
|
|
||
| // Mock App Hosting backends | ||
| sandbox.stub(apphosting, "listBackends").resolves({ | ||
| backends: [ | ||
| { name: "projects/test-project/locations/us-central1/backends/studio", uri: "example.com", servingLocality: "GLOBAL_ACCESS", labels: {}, createTime: "", updateTime: "" } | ||
| ] as any[], | ||
| unreachable: [] | ||
| }); | ||
|
|
||
| // Mock prompt | ||
| sandbox.stub(prompt, "confirm").resolves(false); | ||
|
|
||
| // Mock execSync | ||
| const child_process = require("child_process"); | ||
| sandbox.stub(child_process, "execSync").returns(Buffer.from("1.0.0")); | ||
|
|
||
| await migrate(testRoot); | ||
|
|
||
| // Verify key files were written | ||
| const writeStub = fs.writeFile as sinon.SinonStub; | ||
|
|
||
| expect(writeStub.calledWith(path.join(testRoot, ".firebaserc"), sinon.match(/test-project/))).to.be.true; | ||
| expect(writeStub.calledWith(path.join(testRoot, "firebase.json"), sinon.match(/"backendId": "studio"/))).to.be.true; | ||
| expect(writeStub.calledWith(path.join(testRoot, "README.md"), sinon.match(/Test App/))).to.be.true; | ||
| }); |
There was a problem hiding this comment.
This unit test relies on a large number of mocks for various modules (fetch, fs, apphosting, prompt, child_process). According to the repository's style guide, tests that require extensive mocking may be better suited as integration tests. This approach can make the test brittle and difficult to maintain as the underlying implementation changes.
Consider refactoring this into an integration test located in /scripts/[feature]-tests/ to provide more robust validation of the migration logic.
References
- Avoid excessive mocking in unit tests. If a test requires many mocks, it might be better as an integration test in
/scripts/[feature]-tests/. (link)
| json: async () => [ | ||
| { name: "test-skill", type: "dir", url: "https://api.github.com/repos/firebase/agent-skills/contents/skills/test-skill" } | ||
| ] | ||
| } as any); |
There was a problem hiding this comment.
The repository style guide advises against using any as an escape hatch. This type assertion is used multiple times in this test file (e.g., lines 35, 42, 49, 52, 85), which bypasses type safety.
Please define proper interfaces/types or use type guards. For mocking fetch responses, you could use as unknown as Response or define a partial mock object that satisfies the Response interface.
References
- Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards. (link)
src/firebase_studio/migrate.ts
Outdated
| const readmeTemplate = await fs.readFile(path.join(__dirname, "readme_template.md"), "utf8"); | ||
| const newReadme = readmeTemplate | ||
| .replace(/\${appName}/g, appName) | ||
| .replace("${exportDate}", new Date().toLocaleDateString()) |
There was a problem hiding this comment.
The use of new Date().toLocaleDateString() will produce a date string that is dependent on the locale of the machine running the command. This can lead to inconsistent date formats in the generated README.md file.
For consistency, it's better to use a fixed date format, such as YYYY-MM-DD.
.replace("${exportDate}", new Date().toISOString().split("T")[0])
joehan
left a comment
joehan
left a comment
There was a problem hiding this comment.
Seems good enough for a first pass and testing after changes + addressing linter issue
src/commands/studio-export.ts
Outdated
| .description("export Firebase Studio apps to continue development locally") | ||
| .action(() => { | ||
| export const command = new Command("studio:export [path]") | ||
| .description("export Firebase Studio apps for migration to Antigravity") |
There was a problem hiding this comment.
Nit: Could be a little clearer about what path should be. Is this a path to the export.zip file? Or to the directory you get after unzipping?
| @@ -0,0 +1,10 @@ | |||
| # ${appName} | |||
There was a problem hiding this comment.
These 2 files should probably be moved to a new subdir in the /templates directory
src/firebase_studio/migrate.ts
Outdated
| logger.debug(`Could not delete ${modifiedPath}: ${err}`); | ||
| } | ||
| } | ||
| async function askToOpenAgy( |
There was a problem hiding this comment.
Nit: Prefer askToOpenAntigravity
src/firebase_studio/migrate.ts
Outdated
|
|
||
| export async function migrate(rootPath: string): Promise<void> { | ||
| const args = process.argv.slice(2); | ||
| const noStartAgyFlag = args.includes("--nostart_agy"); |
There was a problem hiding this comment.
Flag seems wrong here - should it be --no-start-agy
joehan
left a comment
joehan
left a comment
There was a problem hiding this comment.
LGTM after you remove the shrinkwrap change
npm-shrinkwrap.json
Outdated
| "integrity": "sha512-8iUql50EUR+uUcdRQ3HDqa6EVyo3docL8g5WJ3FNcWmu62IbkGUue/pEyLBW8VGKKucTPgqeks4fIU1DA4yowQ==", | ||
| "requires": { | ||
| "ajv": "^8.17.1" | ||
| "ajv": "^8.0.0" |
There was a problem hiding this comment.
Revert this change - I think you may need to update your version of npm
2 participants
AGY logic to transform a FBS project to AGY.
Current functionality is mostly NextJS to AppHosting
TODO:
Genkit
Flutter
Windows