Add improved FAL regionality & convert trigger type from HTTPS to blocking#1848
Add improved FAL regionality & convert trigger type from HTTPS to blocking#1848
Conversation
Summary of ChangesHello, 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 invocation mechanism and regional configuration for AI functions. By introducing a generic type to 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. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces improved regionality support for Firebase AI functions and converts the trigger type from HTTPS to a blocking trigger. The changes look good overall, but there's a significant issue with the use of location instead of region for specifying deployment regions. This will cause the region configuration to be ignored. I've left specific comments with suggestions on how to fix this. Once that's addressed, the PR should be in good shape.
| export interface WebhookOptions<Regional extends boolean = false> extends Omit<EventHandlerOptions, "location"> { | ||
| location?: string | Expression<string> | MultipleLocationsIf<Regional> | ResetValue; | ||
| regionalWebhook?: Regional; | ||
| } |
There was a problem hiding this comment.
The property location appears to be incorrect. The standard property for specifying deployment regions in the Firebase Functions SDK is region. The current implementation uses location, which will be ignored by the optionsToEndpoint helper, causing the region/location setting for the function to not be applied.
Additionally, Omit<EventHandlerOptions, "location"> seems to be a typo and should likely be Omit<EventHandlerOptions, "region">, as EventHandlerOptions has a region property to be overridden, but no location property.
Please change location to region to align with the rest of the SDK and ensure the option is correctly processed.
| export interface WebhookOptions<Regional extends boolean = false> extends Omit<EventHandlerOptions, "location"> { | |
| location?: string | Expression<string> | MultipleLocationsIf<Regional> | ResetValue; | |
| regionalWebhook?: Regional; | |
| } | |
| export interface WebhookOptions<Regional extends boolean = false> extends Omit<EventHandlerOptions, "region"> { | |
| region?: string | Expression<string> | MultipleLocationsIf<Regional> | ResetValue; | |
| regionalWebhook?: Regional; | |
| } |
| describe("Typings", () => { | ||
| it("should allow regional webhooks to specify multiple locations", () => { | ||
| ai.beforeGenerateContent({ regionalWebhook: true, location: ["us-central1", "europe-west1"] }, () => {}); | ||
| }); | ||
|
|
||
| it("should allow global webhooks to specify a single location", () => { | ||
| ai.beforeGenerateContent({ location: "us-central1" }, () => {}); | ||
| }); | ||
|
|
||
| it("should allow regional webhooks to specify a single location", () => { | ||
| ai.beforeGenerateContent({ regionalWebhook: true, location: "us-central1" }, () => {}); | ||
| }); | ||
|
|
||
| // Compilation failure tests (commented out): | ||
| // it("should NOT allow global webhooks to specify multiple locations", () => { | ||
| // ai.beforeGenerateContent({ location: ["us-central1", "europe-west1"] }, () => {}); | ||
| // }); | ||
| }); |
There was a problem hiding this comment.
To align with the suggested change from location to region in the WebhookOptions interface, these tests need to be updated to use the region property instead of location.
| describe("Typings", () => { | |
| it("should allow regional webhooks to specify multiple locations", () => { | |
| ai.beforeGenerateContent({ regionalWebhook: true, location: ["us-central1", "europe-west1"] }, () => {}); | |
| }); | |
| it("should allow global webhooks to specify a single location", () => { | |
| ai.beforeGenerateContent({ location: "us-central1" }, () => {}); | |
| }); | |
| it("should allow regional webhooks to specify a single location", () => { | |
| ai.beforeGenerateContent({ regionalWebhook: true, location: "us-central1" }, () => {}); | |
| }); | |
| // Compilation failure tests (commented out): | |
| // it("should NOT allow global webhooks to specify multiple locations", () => { | |
| // ai.beforeGenerateContent({ location: ["us-central1", "europe-west1"] }, () => {}); | |
| // }); | |
| }); | |
| describe("Typings", () => { | |
| it("should allow regional webhooks to specify multiple locations", () => { | |
| ai.beforeGenerateContent({ regionalWebhook: true, region: ["us-central1", "europe-west1"] }, () => {}); | |
| }); | |
| it("should allow global webhooks to specify a single location", () => { | |
| ai.beforeGenerateContent({ region: "us-central1" }, () => {}); | |
| }); | |
| it("should allow regional webhooks to specify a single location", () => { | |
| ai.beforeGenerateContent({ regionalWebhook: true, region: "us-central1" }, () => {}); | |
| }); | |
| // Compilation failure tests (commented out): | |
| // it("should NOT allow global webhooks to specify multiple locations", () => { | |
| // ai.beforeGenerateContent({ region: ["us-central1", "europe-west1"] }, () => {}); | |
| // }); | |
| }); |
shettyvarun268
left a comment
There was a problem hiding this comment.
LGTM
(Minor linting errors)
Implement go/fal-cf3-locality as well as making this a new blocking trigger instead of HTTPS trigger so the teams can test e2e
You can review now or wait until the API is formally approved.