Conversation
Summary of ChangesHello @AustinBenoit, 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 significantly refactors the Firebase Functions client library by migrating its core logic from C++ SWIG bindings to a pure C# implementation. This change streamlines the codebase, reduces external dependencies, and provides more direct control over HTTP request handling and data serialization within the C# environment. It also introduces new internal helper classes for HTTP operations and error parsing, ensuring a robust and consistent experience for developers while maintaining backward compatibility. 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
The rewrite of Firebase Functions in C# significantly improves maintainability by removing the C++ dependency. The implementation correctly handles the core protocol and serialization. However, there are technical concerns regarding resource management and code quality. Specifically, HttpClient should be shared at the service level to avoid socket exhaustion, and the reflection-based event subscription in FirebaseFunctions is fragile and potentially leaks memory. Additionally, several 'TODO' comments should be resolved, and CancellationToken support should be added to the public API to align with modern .NET practices.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and welcome rewrite of the Firebase Functions SDK, moving it from a C++/SWIG implementation to a pure C# implementation. This removes a native dependency and simplifies the build process, as reflected in the CMakeLists.txt changes. The refactoring also includes moving shared utilities like HttpHelpers and FirebaseInterops to a common app/src/internal location, which is a good improvement for code sharing across Firebase products.
My review has identified a few critical issues:
- A memory leak in
FirebaseFunctions.csdue to incorrect event handler disposal. - A syntax error in
FunctionsException.csthat will break compilation.
I've also included some suggestions to improve maintainability regarding the use of reflection and to clean up leftover debug code. Overall, this is a great direction for the SDK.
AustinBenoit
force-pushed
the
functionsrewrite
branch
4 times, most recently
from
0cb7d97 to
9ed1eb4
Compare
This is the first phase where I get things building again.
AustinBenoit
force-pushed
the
functionsrewrite
branch
from
9ed1eb4 to
fa0e5a3
Compare
|
/gemini review |
AustinBenoit
added
the
tests-requested: quick
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable rewrite of the Firebase Functions implementation, moving from a C++/SWIG wrapper to a pure C# implementation using HttpClient. This greatly improves maintainability and portability. The new implementation of serialization and error parsing seems correct. However, I've found a few critical issues that need to be addressed: a memory leak due to incorrect event handler removal, and a race condition in instance management. I've also pointed out a couple of minor issues like a leftover debug log and some unused using statements.
github-actions
bot
added
tests: in-progress
❌ Integration test FAILEDRequested by @firebase-workflow-trigger[bot] on commit 7148241
|
firebase-workflow-trigger
bot
removed
the
tests: in-progress
|
/gemini review |
AustinBenoit
added
tests-requested: quick
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the Firebase Functions implementation in C#, moving away from the C++ SWIG bindings to a pure C# approach. This change involves a complete rewrite of the FirebaseFunctions and HttpsCallableReference classes, along with the introduction of new internal serialization and error parsing utilities. The build system has also been updated to reflect this change, removing native library dependencies and packaging C# source files directly. Additionally, a configurable authentication token prefix has been introduced for greater flexibility in handling Firebase tokens.
Overall, the changes represent a substantial improvement in maintainability and potentially performance by removing the C++ interop layer. However, there are a few areas that could be improved to enhance robustness and clarity.
github-actions
bot
added
tests: in-progress
firebase-workflow-trigger
bot
removed
the
tests: in-progress
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed rewrite of the Firebase Functions SDK, moving it from a C++/SWIG wrapper to a pure C# implementation. This removes the native dependency and modernizes the codebase using HttpClient and async/await. The changes are consistent across the C# code, build scripts, and packaging configuration. My review includes a couple of suggestions to further improve the new implementation, focusing on adding cancellation support to async operations and enhancing error message clarity for better debuggability.
| UnityEngine.Debug.Log("Request:\n" + request.Content); | ||
| #endif | ||
| // TODO pipe through cancellation tokens | ||
| var response = await _firebaseFunctions.HttpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead); |
There was a problem hiding this comment.
As the TODO on the previous line suggests, it would be good to support cancellation. I recommend adding a CancellationToken parameter to the public CallAsync methods and passing it down to HttpClient.SendAsync here. This would make the API more robust and allow callers to cancel long-running requests.
For example, you could change the method signatures like this:
public Task<HttpsCallableResult> CallAsync(CancellationToken cancellationToken = default)
// ...
public Task<HttpsCallableResult> CallAsync(object data, CancellationToken cancellationToken = default)
// ...
private async Task<HttpsCallableResult> InternalCallAsync(object data, CancellationToken cancellationToken = default) var response = await _firebaseFunctions.HttpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken);| throw new FunctionsException(FunctionsErrorCode.Internal, "Response is missing data field."); | ||
| } | ||
|
|
||
| throw new FunctionsException(FunctionsErrorCode.Internal, "INTERNAL"); |
There was a problem hiding this comment.
The exception message "INTERNAL" is not very descriptive. If deserialization results in something other than a dictionary, it would be helpful for debugging to know what was received. Consider providing a more informative error message.
throw new FunctionsException(FunctionsErrorCode.Internal, $"Response is not a valid JSON object. Received: {data}");1 participant
Description
Functions rewrite in C# with the goal of making this a non breaking change.
There is a couple of quirks with the reimplementation. In the previous iteration we used the C++ Variant object class that would do some type conversion. Now I have attempt to rewrite these type conversion in C#. This is just a bit odd as technically we are sending all ints as longs to the firebase functions. This is consistent with the previous implementation just a bit odd.
The second quirk is the how the bearer token is expected to be handled between AI logic and Functions. So we need to pipe through this specific name in each product.
Testing
Ran both the functions and AI test locally in unity.
Type of Change
Place an
xthe applicable box: