feat: ai samples impl text gen/chat#1070
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a modular React and Vite-based application showcasing Firebase AI capabilities, including text generation and multi-turn chat. Feedback on the changes highlights several critical issues: an invalid model name ('gemini-3.5-flash') is used across multiple service files, which will cause runtime failures; the Chat view lacks error handling during initialization, potentially leading to application crashes; Firebase configuration values are hardcoded instead of using environment variables; and a deprecated @types/react-router-dom dependency is present, risking type conflicts with React Router v7.
| const handleResetChat = () => { | ||
| chatSessionRef.current = startNewChat(); | ||
| setMessages([]); | ||
| setError(null); | ||
| setInput(''); | ||
| }; |
There was a problem hiding this comment.
If Firebase is not configured or fails to initialize, startNewChat() will throw an unhandled exception on mount inside useEffect, causing the entire React application to crash with a white screen. Wrapping this initialization in a try/catch block and setting the error state allows the UI to gracefully display the error.
const handleResetChat = () => {
try {
chatSessionRef.current = startNewChat();
setError(null);
} catch (err: any) {
setError(err.message || 'Failed to initialize chat session. Please check your Firebase configuration.');
}
setMessages([]);
setInput('');
};
75197b1 to
0b9284c
Compare
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
hsubox76
left a comment
There was a problem hiding this comment.
Apps look good so far.
Can you add instructions in the README.md about how to install and run the samples, and the different options? Cover
- how to put your project config in
- npm install
- how to run full app or individual samples
- where to go in the browser
- Can cut and paste code out of service.ts file for platform agnostic use
Also, add App Check debug initialization as we discussed (I guess just in firebaseAIService.ts).
| measurementId: import.meta.env.VITE_FIREBASE_MEASUREMENT_ID || "YOUR_MEASUREMENT_ID" | ||
| }; | ||
|
|
||
| const app = !getApps().length ? initializeApp(firebaseConfig) : getApp(); |
There was a problem hiding this comment.
Do we still need to do this check? initializeApp should be idempotent. Are there any errors when we don't do this check first?
| import { getAI, getGenerativeModel } from 'firebase/ai'; | ||
|
|
||
| const firebaseConfig = { | ||
| apiKey: import.meta.env.VITE_FIREBASE_API_KEY || "YOUR_API_KEY", |
There was a problem hiding this comment.
I don't think we want to encourage people making 8 Vite environment variables, that's probably a pretty inconvenient way of setting up a Firebase config. If you're trying to avoid accidentally leaving your project config in a commit, how about const firebaseConfig = import.meta.env.VITE_FIREBASE_CONFIG ? JSON.parse(import.meta.env.VITE_FIREBASE_CONFIG) : { /* or copy and paste your Firebase project config here */} . Then store your project config in the vite env variable VITE_FIREBASE_CONFIG in JSON stringified format when developing. A user could just paste their project config in to the braces if they don't want to bother with JSON stringified project configs but you could use it if you wanted to guarantee you don't accidentally commit any project info.
| "test": "scripts/test.sh" | ||
| }, | ||
| "dependencies": { | ||
| "@firebase/ai": "^2.13.1", |
There was a problem hiding this comment.
These dependencies shouldn't be included at the top level, they will affect all the products in this repo. Also, no need to add @firebase/ai separately.
| * @param prompt The string instruction sent to the model. | ||
| * @param onChunk Callback fired for each non-empty text chunk. | ||
| */ | ||
| export async function streamText(prompt: string, onChunk: (chunk: string) => void): Promise<void> { |
There was a problem hiding this comment.
This and generateWithSystemInstruction are currently unused. Are they to be added later?
There was a problem hiding this comment.
Let's use npm instead of yarn as it's used everywhere else in this repo. Delete yarn.lock and npm install to generate a package-lock.json.
Implement text-generation and chat samples. Add decoupled logic so that user can run script to start either sample.