Skip to content

Aariz#10529

Open
khanaarizkhan008-spec wants to merge 2 commits into
firebase:mainfrom
khanaarizkhan008-spec:main
Open

Aariz#10529
khanaarizkhan008-spec wants to merge 2 commits into
firebase:mainfrom
khanaarizkhan008-spec:main

Conversation

@khanaarizkhan008-spec
Copy link
Copy Markdown

Description

Scenarios Tested

Sample Commands

@google-cla
Copy link
Copy Markdown

google-cla Bot commented May 19, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors logging and error handling within the Crashlytics build tools helper. Key changes include simplifying a debug log statement, ensuring outputs.stdout is present before logging warnings, and including the process exit status when throwing a FirebaseError. A review comment correctly identified that wrapping the warning message in a new Error object to access its .message property is redundant and suggested a more direct implementation.

Comment on lines +62 to 64
if (!debug && outputs.stdout) {
utils.logWarning(new Error(outputs.stdout?.toString() || "An unknown error occurred").message);
}
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

The use of new Error(...).message is redundant and adds unnecessary overhead, as it simply returns the string passed to the constructor. Since outputs.stdout is already checked for truthiness in the if condition, you can directly call .toString() on it. Additionally, the optional chaining operator ?. is unnecessary given the preceding check.

Suggested change
if (!debug && outputs.stdout) {
utils.logWarning(new Error(outputs.stdout?.toString() || "An unknown error occurred").message);
}
if (!debug && outputs.stdout) {
utils.logWarning(outputs.stdout.toString() || "An unknown error occurred");
}

@wiz-9635d3485b
Copy link
Copy Markdown

Wiz Scan Summary

Scanner Findings
Vulnerability Finding Vulnerabilities -
Data Finding Sensitive Data -
Secret Finding Secrets -
IaC Misconfiguration IaC Misconfigurations -
SAST Finding SAST Findings 2 Medium
Software Management Finding Software Management Findings -
Total 2 Medium

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants