Conversation
|
/gemini review |
![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 error handling for analytics tracking in src/command.ts to ensure that failures in tracking do not cause commands to exit with an error. While the changes improve robustness, the review feedback points out that a trackGA4 call remains outside the try block in the success path, which could still trigger an unintended error exit if it fails. Additionally, the reviewer suggests refactoring the logic to reduce nesting and duplication, adhering to the repository's style guide.
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of analytics tracking in src/command.ts by wrapping tracking logic in try/catch blocks and logging failures with logger.debug. This ensures that analytics errors do not crash the CLI. A review comment suggests refactoring the success path to reduce nesting, consistent with the repository's style guide.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces error handling for analytics tracking in src/command.ts by wrapping tracking calls in try-catch blocks and logging failures as debug messages. This prevents analytics failures from interfering with the command execution flow. The success path was also refactored to use an array-based approach for managing multiple tracking promises. Feedback suggests applying the same refactoring to the error path for consistency and better readability, replacing the current ternary operator and Promise.resolve() logic.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces error handling for analytics tracking in both the success and error paths of command execution. The changes wrap tracking logic in try-catch blocks to prevent tracking failures from interrupting the main process. Review feedback suggests refactoring these blocks to use promise-based .catch() methods to reduce nesting, adhering to the repository's style guide, and recommends using optional chaining for safer access to properties on the error object.
joehan
left a comment
joehan
left a comment
There was a problem hiding this comment.
LGTM, but please add a CHANGELOG entry saying something like:
- Fixed an issue where functions deployments would silently fail (#6989)
src/command.ts
Outdated
| ); | ||
| ); | ||
| } | ||
| await withTimeout(5000, Promise.all(tracks)); |
There was a problem hiding this comment.
Nit: 5s is insanely long to wait for these calls, probably safe to lower this to 1s - same for the other one.
3 participants
Fixes: #6989
Deployments can falsely exit with success code 0 even when internal tasks fail. Right before shutting down, the CLI tries to send background analytics. In secure or restrictive CI/CD environments, this analytics ping hangs and times out. That timeout crashes the CLI wrapper's error-handler prematurely, masking the original deployment failure before it can set the non-zero exit code.
Solution: We wrapped the background analytics timeouts in protective try...catch blocks. Now, if the telemetry tracker hangs or fails to reach the portals, the CLI simply ignores the analytics failure and continues with its shutdown routine. This ensures the CLI always returns the authentic deployment failure code to the host system.
Did not add new unit tests for this specific logic because the wrapper lives at the top-level process exit boundary. Writing automated tests here requires faking CLI process shutdowns and restricted networks, which creates brittle mocks. Verified the logic safety using an isolated sandbox playground and confirmed all existing test trees continue to pass.