Update all quickstarts from SDK repo#1443
Conversation
![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 upgrades multiple Firebase Unity test applications to Unity 2021.3.43f1, updates platform configurations, and introduces a helper script copy_testapp.sh. It also adds new features such as consent settings in Analytics, App Check and Gemini 3.5 support in FirebaseAI, batch writes and transactions in Firestore, and asynchronous TVOS support in Storage. The review identified several critical issues, including a C# compilation error due to parameter shadowing in Messaging, potential NullReferenceException bugs in Firestore and FirebaseAI, and a bug in Storage where the bucket name is overwritten by a placeholder. Additionally, improvements were suggested for the copy_testapp.sh script to fix portability and casing issues, along with a typo correction in Analytics.
| private bool ObjectDeepEquals(object left, object right) { | ||
| if (left == right) { | ||
| return true; | ||
| } else if (left == null) { | ||
| return right == null; | ||
| } else if (left is IEnumerable && right is IEnumerable) { | ||
| if (left is IDictionary && right is IDictionary) { | ||
| return DictionaryDeepEquals(left as IDictionary, right as IDictionary); | ||
| } | ||
| return EnumerableDeepEquals(left as IEnumerable, right as IEnumerable); | ||
| } else if (!left.GetType().Equals(right.GetType())) { | ||
| return false; | ||
| } else { | ||
| return left.Equals(right); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of ObjectDeepEquals has a critical bug: if left is non-null and right is null, evaluating !left.GetType().Equals(right.GetType()) will throw a NullReferenceException because right.GetType() is called on a null object. Additionally, comparing strings character-by-character via IEnumerable is highly inefficient, and comparing different types of IEnumerable (like string and List<char>) can lead to incorrect equality results. Enforcing type equality first and handling string explicitly resolves these issues.
private bool ObjectDeepEquals(object left, object right) {\n if (left == right) {\n return true;\n } else if (left == null || right == null) {\n return false;\n } else if (!left.GetType().Equals(right.GetType())) {\n return false;\n } else if (left is string) {\n return left.Equals(right);\n } else if (left is IEnumerable) {\n if (left is IDictionary) {\n return DictionaryDeepEquals(left as IDictionary, right as IDictionary);\n }\n return EnumerableDeepEquals(left as IEnumerable, right as IEnumerable);\n } else {\n return left.Equals(right);\n }\n }| storage = FirebaseStorage.DefaultInstance; | ||
| if (!String.IsNullOrEmpty(appBucket)) { | ||
| MyStorageBucket = String.Format("gs://{0}/", appBucket); | ||
| MyStorageBucket = "REPLACE_WITH_YOUR_STORAGE_BUCKET"; |
There was a problem hiding this comment.
This change introduces a critical bug: if appBucket is successfully read from the Firebase configuration, MyStorageBucket is overwritten with the placeholder string "REPLACE_WITH_YOUR_STORAGE_BUCKET" instead of being formatted with the actual bucket name. This completely breaks automatic bucket detection. It should be restored to dynamically format the bucket URI.
MyStorageBucket = String.Format("gs://{0}/", appBucket);| Firebase.Messaging.FirebaseMessaging.SubscribeAsync(topic).ContinueWithOnMainThread(task => { | ||
| LogTaskCompletion(task, "SubscribeAsync"); | ||
| }); |
There was a problem hiding this comment.
The nested lambda parameter task shadows the outer lambda parameter task. In C#, a lambda parameter cannot shadow a parameter or local variable from an enclosing scope, which will cause a compilation error (CS0136). Rename the inner parameter to subscribeTask to resolve this.
Firebase.Messaging.FirebaseMessaging.SubscribeAsync(topic).ContinueWithOnMainThread(subscribeTask => {\n LogTaskCompletion(subscribeTask, "SubscribeAsync");\n });| if (task.IsCanceled) { | ||
| DebugLog("Session ID fetch was canceled."); | ||
| } else if (task.IsFaulted) { | ||
| DebugLog(String.Format("Encounted an error fetching session ID {0}", |
There was a problem hiding this comment.
There is a typo in the log message: "Encounted" should be "Encountered".
DebugLog(String.Format("Encountered an error fetching session ID {0}",| MODULE_CAP=$(echo "$MODULE" | awk -F_ '{for(i=1;i<=NF;i++) printf "%s", toupper(substr($i,1,1)) substr($i,2); print ""}') | ||
|
|
||
| # Define base source and destination paths | ||
| SRC_BASE="$HOME/Documents/GitHub/firebase-unity-sdk/${MODULE}/testapp" |
There was a problem hiding this comment.
The script uses a hardcoded absolute path ($HOME/Documents/GitHub/firebase-unity-sdk) for SRC_BASE. This makes the script non-portable and fragile for other developers or CI environments. Consider using an environment variable with a fallback to allow customization.
| SRC_BASE="$HOME/Documents/GitHub/firebase-unity-sdk/${MODULE}/testapp" | |
| SRC_BASE="${FIREBASE_UNITY_SDK_PATH:-$HOME/Documents/GitHub/firebase-unity-sdk}/${MODULE}/testapp" |
| MODULE="$1" | ||
|
|
||
| # Convert snake_case to PascalCase (e.g., remote_config -> RemoteConfig, database -> Database) | ||
| MODULE_CAP=$(echo "$MODULE" | awk -F_ '{for(i=1;i<=NF;i++) printf "%s", toupper(substr($i,1,1)) substr($i,2); print ""}') |
There was a problem hiding this comment.
The snake_case to PascalCase conversion fails for the firebaseai module, as it converts it to Firebaseai instead of FirebaseAI (with capital AI), which causes subsequent copy operations to fail. Consider adding a special case to handle this module correctly.
| MODULE_CAP=$(echo "$MODULE" | awk -F_ '{for(i=1;i<=NF;i++) printf "%s", toupper(substr($i,1,1)) substr($i,2); print ""}') | |
| MODULE_CAP=$(echo "$MODULE" | awk -F_ '{for(i=1;i<=NF;i++) printf "%s", toupper(substr($i,1,1)) substr($i,2); print ""}')\nif [ "$MODULE_CAP" = "Firebaseai" ]; then\n MODULE_CAP="FirebaseAI"\nfi |
| foreach (var detail in m.CacheTokensDetails) { | ||
| DebugLog($" {detail.Modality}: {detail.TokenCount}"); | ||
| } |
There was a problem hiding this comment.
If m.CacheTokensDetails is null, iterating over it will throw a NullReferenceException. It is safer to add a null check before the foreach loop.
if (m.CacheTokensDetails != null) {\n foreach (var detail in m.CacheTokensDetails) {\n DebugLog($" {detail.Modality}: {detail.TokenCount}");\n }\n }1 participant
Update all the product quickstarts from the changes in firebase-unity-sdk. A majority of the changes are updating the editor version, but there are some functionality changes, especially in Analytics and FirebaseAI. Along with that, remove the Dynamic Links one, as that is no longer supported.