Skip to content

Add improved FAL regionality & convert trigger type from HTTPS to blocking#1848

Open
inlined wants to merge 1 commit intomasterfrom
inlined.fal.regional-and-type
Open

Add improved FAL regionality & convert trigger type from HTTPS to blocking#1848
inlined wants to merge 1 commit intomasterfrom
inlined.fal.regional-and-type

Conversation

@inlined
Copy link
Member

@inlined inlined commented Mar 20, 2026

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.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, 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 WebhookOptions, it enables more granular control over function deployment locations, particularly for regional webhooks. Concurrently, it transitions these functions from traditional HTTPS triggers to a new blocking trigger system, which is crucial for enabling comprehensive end-to-end testing workflows.

Highlights

  • Improved Regionality for AI Webhooks: The WebhookOptions interface for AI functions has been enhanced with a generic Regional type, allowing location to accept an array of strings (multiple locations) when regionalWebhook is true. This provides more flexible regional deployment options.
  • Transition to Blocking Triggers: The beforeGenerateContent and afterGenerateContent AI functions have been converted from using HTTPS triggers to blocking triggers. This change facilitates end-to-end testing and aligns with the new blocking trigger mechanism.
  • Regional Webhook Option for Blocking Triggers: The regionalWebhook option is now correctly passed to the new blocking triggers, ensuring that regional configurations are respected when these functions are invoked.
  • New Typing Tests: Added new TypeScript typing tests to validate the correct behavior of the WebhookOptions interface, specifically ensuring that regional webhooks can specify multiple locations while global webhooks are restricted to a single location.

🧠 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 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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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 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

  1. 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.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +85 to 88
export interface WebhookOptions<Regional extends boolean = false> extends Omit<EventHandlerOptions, "location"> {
location?: string | Expression<string> | MultipleLocationsIf<Regional> | ResetValue;
regionalWebhook?: Regional;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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;
}

Comment on lines +258 to +275
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"] }, () => {});
// });
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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"] }, () => {});
// });
});

Copy link
Contributor

@shettyvarun268 shettyvarun268 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
(Minor linting errors)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants