Skip to content

[AI Improvement] [Task] Fix no-floating-promises in CLI command helpers (open, trackGA4, throttler, cloudbilling) #11467

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.

Sign up for GitHub

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

Open
joehan wants to merge 1 commit into
base: main
Choose a base branch
from
+13 −7
Open

[AI Improvement] [Task] Fix no-floating-promises in CLI command helpers (open, trackGA4, throttler, cloudbilling) #11467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/command.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@
load: () => void;
}

export function isCommandModule(value: unknown): value is CommandModule {

Check warning on line 28 in src/command.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Missing JSDoc comment
return typeof value === "function" && typeof (value as any).load === "function";

Check warning on line 29 in src/command.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Unexpected any. Specify a different type

Check warning on line 29 in src/command.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Unsafe member access .load on an `any` value
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand All @@ -41,7 +41,7 @@
export interface CLIClient {
cli: CommanderStatic;
errorOut: (e: Error) => void;
[key: string]: any;

Check warning on line 44 in src/command.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Unexpected any. Specify a different type
}

/**
Expand Down Expand Up @@ -81,7 +81,7 @@

/**
* Sets an alias for a command.
* @param aliases an alternativre name for the command. Users will be able to call the command via this name.

Check warning on line 84 in src/command.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Expected @PARAM names to be "alias". Got "aliases"
* @return the command, for chaining.
*/
alias(alias: string): Command {
Expand All @@ -90,10 +90,10 @@
}

/**
* Sets any options for the command.

Check warning on line 93 in src/command.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Expected only 0 line after block description
*
* @example
* command.option("-d, --debug", "turn on debugging", false)

Check warning on line 96 in src/command.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Expected no lines between tags
*
* @param args the commander-style option definition.
* @return the command, for chaining.
Expand All @@ -105,10 +105,10 @@
}

/**
* Sets up --force flag for the command.

Check warning on line 108 in src/command.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Expected only 0 line after block description
*
* @param message overrides the description for --force for this command
* @returns the command, for chaining

Check warning on line 111 in src/command.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Invalid JSDoc tag (preference). Replace "returns" JSDoc tag with "return"
*/
withForce(message?: string): Command {
this.options.push(["-f, --force", message || "automatically accept all interactive prompts"]);
Expand All @@ -132,7 +132,7 @@
*
* This text is displayed when:
* - the `--help` flag is passed to the command, or
* - the `help <command>` command is used.

Check warning on line 135 in src/command.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Expected only 0 line after block description
*
* @param t the human-readable help text.
* @return the command, for chaining.
Expand Down Expand Up @@ -516,7 +516,7 @@
if (PROJECT_ID_REGEX.test(project)) {
return;
}
trackGA4("error", {
void trackGA4("error", {
error_type: "Error (User)",
details: "Invalid project ID",
});
Expand Down
7 changes: 5 additions & 2 deletions src/commands/hosting-channel-open.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ import { bold } from "colorette";
import * as open from "open";

import { Command } from "../command";
import { FirebaseError } from "../error";
import { FirebaseError, getErrMsg } from "../error";
import { logger } from "../logger";
import { getChannel, listChannels, normalizeName } from "../hosting/api";
import { requirePermissions } from "../requirePermissions";
import { needProjectId } from "../projectUtils";
Expand Down Expand Up @@ -55,7 +56,9 @@ export const command = new Command("hosting:channel:open [channelId]")

logLabeledBullet("hosting:channel", channel.url);
if (!options.nonInteractive) {
open(channel.url);
open(channel.url).catch((err: unknown) => {
logger.debug(`Unable to open URL: ${getErrMsg(err)}`);
});
Comment on lines +59 to +61

@gemini-code-assist gemini-code-assist Bot Nov 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Instead of importing and calling open directly from the open package, reuse the existing openInBrowser utility from src/utils.ts. This utility is specifically designed to wrap open so that it can be easily stubbed out in unit tests.

Please also update the imports at the top of the file to include openInBrowser from ../utils and remove the direct import of open.

Suggested change
open(channel.url).catch((err: unknown) => {
logger.debug(`Unable to open URL: ${getErrMsg(err)}`);
});
openInBrowser(channel.url).catch((err: unknown) => {
logger.debug("Unable to open URL: " + getErrMsg(err));
});
References
  1. Reuse or extend existing utilities: Check imported modules, the immediate subsystem, and src/utils.ts first; reuse or extend existing helpers rather than writing bespoke inline logic. (link)

}

return { url: channel.url };
Expand Down
6 changes: 4 additions & 2 deletions src/commands/open.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as clc from "colorette";
import * as open from "open";

import { FirebaseError } from "../error";
import { FirebaseError, getErrMsg } from "../error";
import * as api from "../api";
import { Command } from "../command";
import { logger } from "../logger";
Expand Down Expand Up @@ -114,5 +114,7 @@ export const command = new Command("open [link]")
logger.info(`Opening ${clc.bold(link.name)} link in your default browser:`);
logger.info(clc.bold(clc.underline(url)));

open(url);
open(url).catch((err: unknown) => {
logger.debug(`Unable to open URL: ${getErrMsg(err)}`);
});
Comment on lines +117 to +119

@gemini-code-assist gemini-code-assist Bot Nov 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Instead of calling open directly, reuse the existing openInBrowser utility from src/utils.ts (which is already imported as utils in this file). This ensures consistency and allows the browser opening behavior to be stubbed out in unit tests.

You can also remove the unused import * as open from "open"; from the top of the file.

Suggested change
open(url).catch((err: unknown) => {
logger.debug(`Unable to open URL: ${getErrMsg(err)}`);
});
utils.openInBrowser(url).catch((err: unknown) => {
logger.debug("Unable to open URL: " + getErrMsg(err));
});
References
  1. Reuse or extend existing utilities: Check imported modules, the immediate subsystem, and src/utils.ts first; reuse or extend existing helpers rather than writing bespoke inline logic. (link)

});
2 changes: 1 addition & 1 deletion src/gcp/cloudbilling.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ async function openBillingAccount(
): Promise<boolean> {
if (open) {
try {
opn(url);
await opn(url);

@gemini-code-assist gemini-code-assist Bot Nov 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Instead of calling opn directly, reuse the existing openInBrowser utility from src/utils.ts (which is already imported as utils in this file). This ensures consistency and allows the browser opening behavior to be stubbed out in unit tests.

You can also remove the unused import * as opn from "open"; from the top of the file.

Suggested change
await opn(url);
await utils.openInBrowser(url);
References
  1. Reuse or extend existing utilities: Check imported modules, the immediate subsystem, and src/utils.ts first; reuse or extend existing helpers rather than writing bespoke inline logic. (link)

} catch (err: any) {
logger.debug("Unable to open billing URL: " + err.stack);
}
Expand Down
3 changes: 2 additions & 1 deletion src/throttler/throttler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,8 @@ export abstract class Throttler<T, R> {
}

this.active++;
this.handle(this.nextWaitingTaskIndex());
// handle() records task success and failure itself and is intentionally not awaited (process() is sync and drives concurrency).
void this.handle(this.nextWaitingTaskIndex());
}

async handle(cursorIndex: number): Promise<void> {
Expand Down
Loading