Revert "[Firebase 12] Remove Firebase Dynamic Links quickstart" #1708
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ncooke3, 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 is a complete revert of a previous change, effectively restoring the Firebase Dynamic Links quickstart examples for iOS (Objective-C, Swift UIKit, and Swift SwiftUI) to the repository. The primary motivation for this restoration is to preserve existing documentation functions, ensuring that relevant content remains accessible despite the Firebase Dynamic Links service being deprecated.
Highlights
- Quickstart Restoration: This pull request fully reverts a previous change, restoring the Firebase Dynamic Links quickstart examples for Objective-C, Swift (UIKit), and Swift (SwiftUI) to the repository.
- Documentation Preservation: The primary intent behind this revert is to ensure the continued availability and functionality of documentation related to Firebase Dynamic Links.
- Deprecation Notice Update: The
README.mdfor the Dynamic Links quickstart has been updated to prominently feature a deprecation notice for the Firebase Dynamic Links service.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| name: cocoapods | ||
| runs-on: macOS-15 | ||
| env: | ||
| SPM: false | ||
| LEGACY: false | ||
| OS: iOS | ||
| DEVICE: iPhone 16 | ||
| TEST: true | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@master | ||
| - name: Setup | ||
| run: | | ||
| cd dynamiclinks | ||
| gem install bundler | ||
| bundle install | ||
| gem install xcpretty | ||
| bundle exec pod install --repo-update | ||
| ../scripts/install_prereqs/dynamiclinks.sh | ||
| - name: Build Swift | ||
| run: ./scripts/test.sh | ||
| env: | ||
| SWIFT_SUFFIX: Swift |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
There was a problem hiding this comment.
Code Review
This pull request reverts the removal of the Firebase Dynamic Links quickstart. The re-added code provides examples in Objective-C, Swift (UIKit), and Swift (SwiftUI).
Identified areas for improvement:
- Force-unwrapping optionals (especially
userActivity.webpageURLinAppDelegate.swift) could lead to runtime crashes. - The
install_prereqs.shscript doesn't update theDOMAIN_URI_PREFIXplaceholder for the Swift UIKit and SwiftUI examples. - Some UI code relies on subview indexing (e.g.,
view.subviews[0]). - The Objective-C
AppDelegate.mcontains a fallback toUIAlertView, which is dead code. - A logic issue in
ViewController.mfor checking if a text field is empty. - A small inconsistency in the README regarding iOS deployment targets for the UIKit examples.
| func application(_ application: UIApplication, continue userActivity: NSUserActivity, | ||
| restorationHandler: @escaping ([UIUserActivityRestoring]?) -> Void) -> Bool { | ||
| let handled = DynamicLinks.dynamicLinks() | ||
| .handleUniversalLink(userActivity.webpageURL!) { dynamiclink, error in |
There was a problem hiding this comment.
Force-unwrapping userActivity.webpageURL! can lead to a runtime crash if webpageURL is nil. It's safer to use optional binding (e.g., if let pageURL = userActivity.webpageURL) before passing it to handleUniversalLink.
| .handleUniversalLink(userActivity.webpageURL!) { dynamiclink, error in | |
| guard let webpageURL = userActivity.webpageURL else { | |
| return false | |
| } | |
| let handled = DynamicLinks.dynamicLinks() | |
| .handleUniversalLink(webpageURL) { dynamiclink, error in |
| sed -i '' 's#DYNAMIC_LINK_DOMAIN#qpf6m.app.goo.gl#' DynamicLinksExample/DynamicLinksExample.entitlements | ||
| sed -i '' 's#YOUR_DOMAIN_URI_PREFIX";#https://qpf6m.app.goo.gl";#' DynamicLinksExample/ViewController.m |
There was a problem hiding this comment.
The script updates the DOMAIN_URI_PREFIX placeholder only in the Objective-C ViewController.m. It's missing updates for:
ViewController.swift(for the Swift UIKit example)DynamicLinksExampleApp.swift(for the SwiftUI example)
This omission will leave the Swift and SwiftUI examples non-functional out-of-the-box after running this script, as they will still contain the placeholder YOUR_DOMAIN_URI_PREFIX.
| BOOL handled = [[FIRDynamicLinks dynamicLinks] handleUniversalLink:userActivity.webpageURL | ||
| completion:^(FIRDynamicLink * _Nullable dynamicLink, | ||
| NSError * _Nullable error) { | ||
| // [START_EXCLUDE] | ||
| [self handleDynamicLink:dynamicLink]; | ||
| // [END_EXCLUDE] | ||
| }]; |
There was a problem hiding this comment.
The userActivity.webpageURL property is nullable. Accessing it without a check before passing to handleUniversalLink: could lead to issues if the SDK doesn't gracefully handle a nil URL. It's safer to ensure webpageURL is non-nil.
| } else { | ||
| UIAlertView *alertView = [[UIAlertView alloc] initWithTitle:title | ||
| message:message | ||
| delegate:nil | ||
| cancelButtonTitle:buttonTitle | ||
| otherButtonTitles:nil]; | ||
| [alertView show]; | ||
| } |
There was a problem hiding this comment.
The project's deployment target is iOS 13.0. UIAlertView was deprecated in iOS 9. Therefore, the else block (lines 134-140) that uses UIAlertView is effectively dead code and will not be executed. It should be removed to simplify the code and avoid confusion.
}
}
| if (_dictionary[Link].text == nil) { | ||
| NSLog(@"%@", @"Link can not be empty!"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Checking _dictionary[Link].text == nil is not a reliable way to determine if a UITextField is empty. The text property of an empty UITextField returns an empty string (@""), not nil. This check should verify _dictionary[Link].text.length == 0 to correctly identify an empty link input.
if (_dictionary[Link].text.length == 0) {
NSLog(@"%@", @"Link can not be empty!");
return;
}
|
|
||
| - (void)tableView:(UITableView *)tableView willDisplayHeaderView:(UIView *)view forSection:(NSInteger)section { | ||
| if (section == 2) { | ||
| view.subviews[0].backgroundColor = UIColor.yellowColor; |
There was a problem hiding this comment.
Accessing view.subviews[0] to set the background color relies on the internal view hierarchy of UITableViewHeaderFooterView, which could change in future iOS versions, making this code fragile. It's safer to cast view to UITableViewHeaderFooterView and set the contentView.backgroundColor.
UITableViewHeaderFooterView *headerView = (UITableViewHeaderFooterView *)view;
headerView.contentView.backgroundColor = UIColor.yellowColor;
| override func tableView(_ tableView: UITableView, willDisplayHeaderView view: UIView, | ||
| forSection section: Int) { | ||
| if section == 2 { | ||
| view.subviews[0].backgroundColor = .yellow |
There was a problem hiding this comment.
Accessing view.subviews[0] to set the background color relies on the internal view hierarchy of UITableViewHeaderFooterView, which might change in future iOS versions, making this code fragile. Consider casting view to UITableViewHeaderFooterView and setting contentView.backgroundColor for better robustness.
| view.subviews[0].backgroundColor = .yellow | |
| if let headerView = view as? UITableViewHeaderFooterView { | |
| headerView.contentView.backgroundColor = .yellow | |
| } |
| XCTAssertTrue([_app navigationBars][header].exists); | ||
|
|
||
| // Type url to convert. | ||
| XCUIElement *inputText = [[_app textFields] elementBoundByIndex:0]; |
There was a problem hiding this comment.
Accessing UI elements by index (e.g., [[_app textFields] elementBoundByIndex:0]) can make tests brittle, as changes to the UI hierarchy or the addition/removal of elements can easily break the test. Consider using accessibility identifiers for more robust element selection.
| XCUIElement *shortLink = | ||
| [[[_app staticTexts] matchingPredicate:linkPredicate] elementBoundByIndex:1]; |
There was a problem hiding this comment.
Relying on elementBoundByIndex:1 for selecting the short link text field is fragile. Using accessibility identifiers would make this test more resilient to UI changes.
| Note: You will need a device running iOS 11.0+ for the ObjC and Swift (UIKit) | ||
| versions of the quickstart, or iOS 14.0+ for the Swift (SwiftUI) version. |
There was a problem hiding this comment.
The README states iOS 11.0+ is required for the ObjC and Swift (UIKit) examples. However, the Podfile specifies platform :ios, '13.0' and the Xcode project settings also indicate a deployment target of 13.0 for these examples. This discrepancy should be resolved to avoid confusion.
Rolling back to preserve docs functions.
Reverts #1707