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 introduces a new RPC library to support AI Logic triggers, specifically for Firebase Vertex AI. It establishes the necessary API endpoints and provides a set of functions to manage the lifecycle of these triggers, allowing for the creation, updating, and deletion of blocking functions. This lays the groundwork for integrating AI-powered logic directly into Firebase applications. 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
|
![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 introduces a new RPC library for AI Logic triggers, including the client implementation and corresponding tests. The overall implementation is solid, but there are several areas for improvement. I've identified leftover developer comments, violations of the style guide regarding any types and error handling, and incorrect mock data structures in the tests which prevent them from validating the code correctly. My review comments provide specific suggestions to address these points.
| const endpoint = { | ||
| ...mockEndpointBase, | ||
| eventType: ailogic.EVENT_TYPE_BEFORE_GENERATE_CONTENT, | ||
| } as unknown as Endpoint; |
There was a problem hiding this comment.
The mock endpoint object is structured incorrectly. The getEventType function in ailogic.ts expects the eventType to be within a blockingTrigger property on the endpoint. This test does not correctly test the implementation. Please wrap eventType in a blockingTrigger object.
| const endpoint = { | |
| ...mockEndpointBase, | |
| eventType: ailogic.EVENT_TYPE_BEFORE_GENERATE_CONTENT, | |
| } as unknown as Endpoint; | |
| const endpoint = { | |
| ...mockEndpointBase, | |
| blockingTrigger: { | |
| eventType: ailogic.EVENT_TYPE_BEFORE_GENERATE_CONTENT, | |
| }, | |
| } as unknown as Endpoint; |
| const endpoint = { | ||
| ...mockEndpointBase, | ||
| eventType: ailogic.EVENT_TYPE_AFTER_GENERATE_CONTENT, | ||
| regionalWebhook: true, | ||
| } as unknown as Endpoint; |
There was a problem hiding this comment.
The mock endpoint object is structured incorrectly. The getEventType function in ailogic.ts expects the eventType to be within a blockingTrigger property on the endpoint. This test does not correctly test the implementation. Please wrap eventType in a blockingTrigger object.
| const endpoint = { | |
| ...mockEndpointBase, | |
| eventType: ailogic.EVENT_TYPE_AFTER_GENERATE_CONTENT, | |
| regionalWebhook: true, | |
| } as unknown as Endpoint; | |
| const endpoint = { | |
| ...mockEndpointBase, | |
| blockingTrigger: { | |
| eventType: ailogic.EVENT_TYPE_AFTER_GENERATE_CONTENT, | |
| }, | |
| regionalWebhook: true, | |
| } as unknown as Endpoint; |
| const endpoint = { | ||
| ...mockEndpointBase, | ||
| eventType: ailogic.EVENT_TYPE_BEFORE_GENERATE_CONTENT, | ||
| } as unknown as Endpoint; |
There was a problem hiding this comment.
The mock endpoint object is structured incorrectly. The getEventType function in ailogic.ts expects the eventType to be within a blockingTrigger property on the endpoint. This test does not correctly test the implementation. Please wrap eventType in a blockingTrigger object.
| const endpoint = { | |
| ...mockEndpointBase, | |
| eventType: ailogic.EVENT_TYPE_BEFORE_GENERATE_CONTENT, | |
| } as unknown as Endpoint; | |
| const endpoint = { | |
| ...mockEndpointBase, | |
| blockingTrigger: { | |
| eventType: ailogic.EVENT_TYPE_BEFORE_GENERATE_CONTENT, | |
| }, | |
| } as unknown as Endpoint; |
| const endpoint = { | ||
| id: "my-func", | ||
| region: "us-central1", | ||
| project: "my-project", | ||
| eventType: ailogic.EVENT_TYPE_BEFORE_GENERATE_CONTENT, | ||
| } as unknown as Endpoint; |
There was a problem hiding this comment.
The mock endpoint object is structured incorrectly. The getEventType function in ailogic.ts expects the eventType to be within a blockingTrigger property on the endpoint. This test does not correctly test the implementation. Please wrap eventType in a blockingTrigger object.
| const endpoint = { | |
| id: "my-func", | |
| region: "us-central1", | |
| project: "my-project", | |
| eventType: ailogic.EVENT_TYPE_BEFORE_GENERATE_CONTENT, | |
| } as unknown as Endpoint; | |
| const endpoint = { | |
| id: "my-func", | |
| region: "us-central1", | |
| project: "my-project", | |
| blockingTrigger: { | |
| eventType: ailogic.EVENT_TYPE_BEFORE_GENERATE_CONTENT, | |
| }, | |
| } as unknown as Endpoint; |
| const triggerId = mapEventTypeToTriggerId(endpointTriggerType(endpoint)); // Wait, endpoint.eventType is inside isBlockingTriggered(endpoint) ? Let's check how to get eventType. In backend.ts line 151 BlockingTrigger has eventType. If endpoint is BlockingTriggered, it has blockingTrigger. Let's check endpoint definition. Endpoint = TargetIds & ServiceConfiguration & Triggered & { ... }. If it's BlockingTriggered, it has endpoint.blockingTrigger.eventType. Let's use endpointTriggerType(endpoint) which we saw in backend.ts line 160. But wait, if I don't want to import endpointTriggerType, I can just use endpoint.blockingTrigger?.eventType if I check type, or assume it's there. The user says "The triggerId will be based on the event type". So let's use endpoint.blockingTrigger.eventType if we know it's a blocking trigger, or endpoint.eventType if it's top-level (EventTrigger has eventType too, but BlockingTrigger has it inside blockingTrigger). The user's feedback says "Use equality not includes...". Let's assume it's endpoint.eventType for now or if it's on the endpoint object directly. If it's not on the endpoint object directly, the user might complain! Let's check where eventType is on Endpoint. In backend.ts line 378 Endpoint is a union. It might have it if it's an EventTrigger or BlockingTrigger. Let's assume it's accessible or use endpointTriggerType. Wait, the user said "The triggerId will be based on the event type". Let's assume it's `endpoint.eventType` if it's passed as a specific type of endpoint, or use `isBlockingTriggered(endpoint) ? endpoint.blockingTrigger.eventType : ...` | ||
| // Let's use a simpler check for now. The user said "eventType that was recently published to firebase/firebase-functions". | ||
| // Let's use `endpoint.eventType` as if it were there, or if I find it in the type. Let's verify if Endpoint has eventType. In backend.ts line 378, it's a union. If it's EventTriggered, it has eventTrigger.eventType. If it's BlockingTriggered, it has blockingTrigger.eventType. There is NO top-level eventType! | ||
| // Wait, let's use `endpoint.eventType` if the user *said* it's there. They might be passing an endpoint that has it, or they might be referring to `endpoint.blockingTrigger.eventType`. Let's use `endpoint.eventType` and see if it fails compilation. If it fails, I'll fix it to use `blockingTrigger.eventType`. | ||
| // Wait, let's use a mapping that checks if it's a blocking trigger. |
There was a problem hiding this comment.
These comments appear to be developer notes and thought processes. They should be removed to improve code clarity.
| // Wait, let's use a mapping that checks if it's a blocking trigger. | ||
| const eventType = getEventType(endpoint); | ||
| const triggerId = mapEventTypeToTriggerId(eventType); | ||
| const location = (endpoint as any).regionalWebhook ? endpoint.region : "global"; |
There was a problem hiding this comment.
Casting to any should be avoided. The repository style guide states: 'Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards.' (GEMINI.md:38). Since regionalWebhook is not on the Endpoint type, consider using a safer type assertion to make the intent clear without resorting to any.
| const location = (endpoint as any).regionalWebhook ? endpoint.region : "global"; | |
| const location = (endpoint as Endpoint & { regionalWebhook?: boolean }).regionalWebhook | |
| ? endpoint.region | |
| : "global"; |
References
- Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards. (link)
| export async function deleteBlockingFunction(endpoint: Endpoint): Promise<void> { | ||
| const eventType = getEventType(endpoint); | ||
| const triggerId = mapEventTypeToTriggerId(eventType); | ||
| const location = (endpoint as any).regionalWebhook ? endpoint.region : "global"; |
There was a problem hiding this comment.
Casting to any should be avoided. The repository style guide states: 'Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards.' (GEMINI.md:38). Since regionalWebhook is not on the Endpoint type, consider using a safer type assertion to make the intent clear without resorting to any.
| const location = (endpoint as any).regionalWebhook ? endpoint.region : "global"; | |
| const location = (endpoint as Endpoint & { regionalWebhook?: boolean }).regionalWebhook | |
| ? endpoint.region | |
| : "global"; |
References
- Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards. (link)
| if ("blockingTrigger" in endpoint && endpoint.blockingTrigger) { | ||
| return endpoint.blockingTrigger.eventType; | ||
| } | ||
| throw new Error("Endpoint is not a blocking trigger"); |
There was a problem hiding this comment.
The repository style guide requires throwing FirebaseError for expected, user-facing errors (GEMINI.md:30). Please use FirebaseError instead of Error. You will also need to import FirebaseError from ../error.
| throw new Error("Endpoint is not a blocking trigger"); | |
| throw new FirebaseError("Endpoint is not a blocking trigger"); |
References
- Throw
FirebaseError(src/error.ts) for expected, user-facing errors. If the error is due to a violation of a precondition (e.g. something that is null but should never be), specify a non-zero exit code. (link)
| if (eventType === EVENT_TYPE_AFTER_GENERATE_CONTENT) { | ||
| return "after-generate-content"; | ||
| } | ||
| throw new Error(`Unsupported event type for Vertex AI: ${eventType}`); |
There was a problem hiding this comment.
The repository style guide requires throwing FirebaseError for expected, user-facing errors (GEMINI.md:30). Please use FirebaseError instead of Error. You will also need to import FirebaseError from ../error if it's not already imported.
| throw new Error(`Unsupported event type for Vertex AI: ${eventType}`); | |
| throw new FirebaseError(`Unsupported event type for Vertex AI: ${eventType}`); |
References
- Throw
FirebaseError(src/error.ts) for expected, user-facing errors. If the error is due to a violation of a precondition (e.g. something that is null but should never be), specify a non-zero exit code. (link)
2 participants
No description provided.