Conversation
Summary of ChangesHello @Ehesp, 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 RFC proposes a comprehensive and modern JavaScript/TypeScript client SDK for Genkit. It aims to significantly enhance the developer experience by providing a client factory for centralized configuration, end-to-end type safety for flows, robust built-in retry mechanisms, and framework-specific integrations for React and Vue. The design addresses current limitations by reducing boilerplate, improving resilience, and offering a flexible chat primitive, making it easier to consume Genkit flows in client-side 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. Changelog
Activity
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. 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
|
There was a problem hiding this comment.
Code Review
The RFC for the Genkit JS Client is comprehensive and well-thought-out. It effectively addresses the limitations of the current minimal client by proposing a factory pattern, robust type inference, and framework-specific integrations. The inclusion of both client-managed and server-managed chat history approaches via a flexible adapter pattern is a highlight. My feedback focuses on ensuring resource efficiency for long-running chat sessions and clarifying lifecycle management in the React hooks.
| function useFlow<A extends Action>( | ||
| client: GenkitClient, | ||
| path: string, | ||
| options?: UseFlowOptions<A>, | ||
| ): UseFlowReturn<A>; |
There was a problem hiding this comment.
To ensure consistency with the Vue implementation (line 660), the React useFlow hook should explicitly manage the AbortController lifecycle. It is important that any in-flight requests are automatically aborted when the component unmounts to prevent memory leaks and unnecessary server-side execution.
| **Pros:** Simple, no server-side session state, works with any deployment (serverless, edge). | ||
| **Cons:** Full history sent every request (grows with conversation length), no server-side session persistence across page reloads. |
There was a problem hiding this comment.
For 'Approach A' (Client-Managed History), sending the full message array on every request can become problematic as conversations grow. This could eventually hit HTTP payload limits or exceed the model's context window. Consider adding a recommendation or a built-in option for a sliding window or message truncation strategy within the ChatAdapter or hook configuration.
| 4. **Lightweight flow contract type**: The type inference approach requires `import type { MyFlow } from '../server/flows'`, which imports the `Action` type from `genkit`. While `import type` has zero runtime cost, it still couples the client's type-checking to the server package. Should there be a lighter-weight "flow contract" type that avoids this coupling? For example: | ||
|
|
||
| ```typescript | ||
| // Server | ||
| export type MyFlowContract = FlowContract<string, { greeting: string }, string>; | ||
|
|
||
| // Client -- no dependency on genkit's Action type | ||
| import type { MyFlowContract } from '../shared/contracts'; | ||
| client.runFlow<MyFlowContract>('/myFlow', 'hello'); | ||
| ``` | ||
|
|
There was a problem hiding this comment.
The 'Lightweight flow contract type' is a very important consideration for large-scale applications. Importing the full Action type (and its zod dependencies) from the server can sometimes lead to circular dependencies or bloated type-checking times in complex monorepos. Prioritizing a dependency-agnostic contract definition would improve the scalability of this approach.
This is an RFC for the Genkit JS Client.
See https://github.com/Ehesp/genkit/blob/js-genkit-client/docs/js-genkit-client-rfc.md for more details.