Refactor dataconnect to use the common appUtils models #9286

schnecle · 2025-10-08T19:56:30Z

Description

Refactor dataconnect tools to use common appUtils
Increase test coverage

Scenarios Tested

Platform detection in /firebase:init for web, ios, android and multiple. Example output:

> /firebase:init


ℹRequest cancelled.
 

> Can you print out the prompt you just received

✦ Your goal is to help the user setup Firebase services in this workspace. Firebase is a large platform with many potential uses, so you will:

   1. Detect which Firebase services are already in use in the workspace, if any
   2. Determine which new Firebase services will help the user build their app
   3. Provision and configure the services requested by the user

  Workspace Info

  Use this information to determine which Firebase services the user is already using (if any).

  Workspace platform(s): ANDROID, IOS

Ran firebase init on a web application

Sample Commands

/firebase:init
firebase init

src/mcp/prompts/core/init.ts

fredzqm · 2025-10-08T20:11:45Z

src/management/apps.ts

  // Detect what platform based on current user
-  let targetPlatform = await getPlatformFromFolder(appDir);
-  if (targetPlatform === Platform.NONE) {
+  let targetPlatforms = await getPlatformsFromFolder(appDir);


@maneesht is the right person to take a look at management/apps.ts~

I took a glance. Don't see any issues.

fredzqm

I am able to verify the FDC app detection logics still work.

joehan · 2025-10-09T22:57:12Z

/gemini review

gemini-code-assist

Code Review

This pull request does a great job of refactoring the app detection logic into a common appUtils module, which improves code organization and reusability. The increased test coverage is also a welcome addition. I've found a critical issue with a new deduplication function and a couple of other points that I've detailed in the specific comments. Once these are addressed, this will be a solid improvement.

gemini-code-assist · 2025-10-09T22:58:21Z

src/init/features/dataconnect/sdk.ts

+function dedupeAppsByPlatformAndDirectory(apps: App[]): App[] {
+  // detectApps creates unique apps by appId and bundleId, but this method operates
+  // on platform, directory, and frameworks alone. Deduping here to retain the
+  // same behavior
+  return [
+    ...new Set(
+      apps.map(
+        (app) =>
+          ({
+            platform: app.platform,
+            directory: app.directory,
+            frameworks: app.frameworks,
+          }) as App,
+      ),
+    ),
+  ];
+}


The dedupeAppsByPlatformAndDirectory function is implemented using new Set() on newly created objects inside a map. This will not correctly deduplicate the apps because objects in JavaScript are compared by reference, and each iteration of map creates a new object instance. This can lead to duplicate app configurations being processed downstream.

To fix this, you should generate a unique key for each app based on its properties and use a Map to ensure uniqueness.

Suggested change

function dedupeAppsByPlatformAndDirectory(apps: App[]): App[] {

// detectApps creates unique apps by appId and bundleId, but this method operates

// on platform, directory, and frameworks alone. Deduping here to retain the

// same behavior

return [

...new Set(

apps.map(

(app) =>

({

platform: app.platform,

directory: app.directory,

frameworks: app.frameworks,

}) as App,

),

),

];

}

function dedupeAppsByPlatformAndDirectory(apps: App[]): App[] {

// detectApps creates unique apps by appId and bundleId, but this method operates

// on platform, directory, and frameworks alone. Deduping here to retain the

// same behavior

const uniqueApps = new Map<string, App>();

for (const app of apps) {

// Sorting frameworks for consistent key generation

const frameworkKey = app.frameworks ? [...app.frameworks].sort().join(",") : "";

const key = `${app.platform}:${app.directory}:${frameworkKey}`;

if (!uniqueApps.has(key)) {

// Create a new object with only the desired properties to avoid carrying over others like appId

uniqueApps.set(key, {

platform: app.platform,

directory: app.directory,

frameworks: app.frameworks,

});

}

}

return Array.from(uniqueApps.values());

}

This does seem legit and worth fixing.

gemini-code-assist · 2025-10-09T22:58:21Z

src/init/features/dataconnect/sdk.spec.ts

+  it("should deduplicate apps with the same platform and directory", async () => {
+    const apps: App[] = [
+      { platform: Platform.WEB, directory: "web", frameworks: [Framework.REACT], appId: "app1" },
+      { platform: Platform.WEB, directory: "web", frameworks: [Framework.REACT], appId: "app2" },
+      { platform: Platform.ANDROID, directory: "android" },
+    ];
+    detectAppsStub.resolves(apps);
+    promptStub.resolves([
+      { platform: Platform.WEB, directory: "web", frameworks: [Framework.REACT] },
+    ]);
+
+    const selectedApps = await chooseApp();
+
+    expect(selectedApps).to.deep.equal([
+      { platform: Platform.WEB, directory: "web", frameworks: [Framework.REACT] },
+    ]);
+  });


This test case should deduplicate apps with the same platform and directory doesn't properly validate the deduplication logic. It mocks the result of the user prompt, which hides the fact that chooseApp would present duplicated choices to the user due to the faulty dedupeAppsByPlatformAndDirectory function.

To make this test more robust, you should assert that the choices argument passed to the prompt.checkbox stub contains deduplicated entries.

gemini-code-assist · 2025-10-09T22:58:21Z

src/management/apps.ts

+
+  if (targetPlatforms.length !== 1) {
+    if (targetPlatforms.length === 0) {
      logBullet(`Couldn't automatically detect app your in directory ${appDir}.`);


There's a minor typo in this log message.

Suggested change

logBullet(`Couldn't automatically detect app your in directory ${appDir}.`);

logBullet(`Couldn't automatically detect your app in directory ${appDir}.`);

joehan · 2025-10-09T22:58:39Z

src/init/features/dataconnect/sdk.ts

  // Check for environment variables override.
  const envAppFolder = envOverride(FDC_APP_FOLDER, "");
-  const envPlatform = envOverride(FDC_SDK_PLATFORM_ENV, Platform.NONE) as Platform;
+  const envPlatform: Platform | undefined = envOverride(FDC_SDK_PLATFORM_ENV, "") as Platform;


Nit: This typing seems a little wonky to me? I believe this will either be a string from the env var, or "", neither of which are undefined.

github-project-automation bot added this to [Cloud] Extensions + Functions Oct 8, 2025

fredzqm reviewed Oct 8, 2025

View reviewed changes

schnecle requested a review from maneesht October 9, 2025 15:04

schnecle force-pushed the schnecle/cleanup-dataconnect-apputils branch from b6bdceb to 79b8215 Compare October 9, 2025 19:51

schnecle marked this pull request as ready for review October 9, 2025 19:51

schnecle added 2 commits October 9, 2025 19:52

Refactor dataconnect to use the common appUtils models

1c30a33

Print all platforms in the init prompt

0eca209

schnecle force-pushed the schnecle/cleanup-dataconnect-apputils branch from 79b8215 to 0eca209 Compare October 9, 2025 19:52

schnecle requested a review from joehan October 9, 2025 19:55

fredzqm approved these changes Oct 9, 2025

View reviewed changes

github-project-automation bot moved this to Approved [PR] in [Cloud] Extensions + Functions Oct 9, 2025

gemini-code-assist bot reviewed Oct 9, 2025

View reviewed changes

joehan reviewed Oct 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor dataconnect to use the common appUtils models #9286

Refactor dataconnect to use the common appUtils models #9286

Uh oh!

schnecle commented Oct 8, 2025 •

edited

Loading

Uh oh!

Uh oh!

fredzqm Oct 8, 2025

Uh oh!

fredzqm left a comment

Uh oh!

joehan commented Oct 9, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Oct 9, 2025

Uh oh!

joehan Oct 9, 2025

Uh oh!

gemini-code-assist bot Oct 9, 2025

Uh oh!

gemini-code-assist bot Oct 9, 2025

Uh oh!

joehan Oct 9, 2025

Uh oh!

Uh oh!

	logBullet(`Couldn't automatically detect app your in directory ${appDir}.`);
	logBullet(`Couldn't automatically detect your app in directory ${appDir}.`);

Refactor dataconnect to use the common appUtils models #9286

Are you sure you want to change the base?

Refactor dataconnect to use the common appUtils models #9286

Uh oh!

Conversation

schnecle commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Scenarios Tested

Sample Commands

Uh oh!

Uh oh!

fredzqm Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

fredzqm left a comment

Choose a reason for hiding this comment

Uh oh!

joehan commented Oct 9, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

joehan Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

joehan Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schnecle commented Oct 8, 2025 •

edited

Loading