Revert "refactor(genai): migrate region tags from generative-ai to genai folder (Batch 1)"#4339
Conversation
product-auto-label
Bot
added
samples
![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 cleans up region tags in existing files and introduces a new suite of Node.js code snippets and tests under generative-ai/snippets/ for various Vertex AI Gemini features. The review feedback highlights several improvement opportunities: resolving a logical bug in safetySettings.js where a safety termination message is printed unconditionally, using CAIP_PROJECT_ID consistently in tests, simplifying assertions in gemini-translate.test.js by avoiding unnecessary stringification, and applying optional chaining across multiple snippets to safely access nested API response properties and prevent potential runtime crashes.
| for await (const item of responseStream.stream) { | ||
| if (item.candidates[0].finishReason === 'SAFETY') { | ||
| console.log('This response stream terminated due to safety concerns.'); | ||
| break; | ||
| } else { | ||
| process.stdout.write(item.candidates[0].content.parts[0].text); | ||
| } | ||
| } | ||
| console.log('This response stream terminated due to safety concerns.'); |
There was a problem hiding this comment.
There is a logical bug in this sample: the message 'This response stream terminated due to safety concerns.' is printed unconditionally at the end of the function, even if the stream completes successfully without any safety triggers. Additionally, accessing item.candidates[0] directly without checking if candidates exists can lead to a TypeError if the response is completely blocked or empty. We can resolve both issues by using a boolean flag to track safety termination and applying optional chaining for robust property access.
let safetyBlocked = false;
for await (const item of responseStream.stream) {
if (item.candidates?.[0]?.finishReason === 'SAFETY') {
safetyBlocked = true;
break;
} else {
process.stdout.write(item.candidates?.[0]?.content?.parts?.[0]?.text ?? '');
}
}
if (safetyBlocked) {
console.log('\nThis response stream terminated due to safety concerns.');
} else {
console.log();
}| const execSync = cmd => cp.execSync(cmd, {encoding: 'utf-8'}); | ||
|
|
||
| const projectId = process.env.GOOGLE_SAMPLES_PROJECT; | ||
| const location = process.env.LOCATION; |
There was a problem hiding this comment.
This test uses process.env.GOOGLE_SAMPLES_PROJECT to retrieve the project ID, whereas all other 16 test files in this suite consistently use process.env.CAIP_PROJECT_ID. To prevent potential test failures in CI environments where GOOGLE_SAMPLES_PROJECT is not set, please use CAIP_PROJECT_ID for consistency.
| const location = process.env.LOCATION; | |
| const projectId = process.env.CAIP_PROJECT_ID; |
References
- Maintain established boilerplate patterns across samples within a repository for consistency.
| `node ./gemini-translate.js ${projectId} ${location} ${model}` | ||
| ); | ||
|
|
||
| assert(JSON.stringify(response).match(/Bonjour/)); |
There was a problem hiding this comment.
Calling JSON.stringify(response) on a string before matching it with a regular expression is unnecessary and can introduce unexpected formatting characters. Since assert is imported from Node's strict assertion module, you can use assert.match directly on the string response.
| assert(JSON.stringify(response).match(/Bonjour/)); | |
| assert.match(response, /Bonjour/); |
| const fullTextResponse = | ||
| response.response.candidates[0].content.parts[0].text; |
There was a problem hiding this comment.
To prevent potential runtime crashes (e.g., TypeError: Cannot read properties of undefined) if the API response is empty or blocked by safety filters, use optional chaining and a fallback value when accessing deeply nested properties.
| const fullTextResponse = | |
| response.response.candidates[0].content.parts[0].text; | |
| const fullTextResponse = | |
| response.response?.candidates?.[0]?.content?.parts?.[0]?.text ?? ''; |
| const fullTextResponse = | ||
| aggregatedResponse.candidates[0].content.parts[0].text; |
There was a problem hiding this comment.
To prevent potential runtime crashes (e.g., TypeError: Cannot read properties of undefined) if the API response is empty or blocked by safety filters, use optional chaining and a fallback value when accessing deeply nested properties.
| const fullTextResponse = | |
| aggregatedResponse.candidates[0].content.parts[0].text; | |
| const fullTextResponse = | |
| aggregatedResponse.candidates?.[0]?.content?.parts?.[0]?.text ?? ''; |
| for await (const item of result1.stream) { | ||
| console.log(item.candidates[0].content.parts[0].text); | ||
| } |
There was a problem hiding this comment.
To prevent potential runtime crashes (e.g., TypeError: Cannot read properties of undefined) if the API response is empty or blocked by safety filters, use optional chaining and a fallback value when accessing deeply nested properties.
| for await (const item of result1.stream) { | |
| console.log(item.candidates[0].content.parts[0].text); | |
| } | |
| for await (const item of result1.stream) { | |
| console.log(item.candidates?.[0]?.content?.parts?.[0]?.text ?? ''); | |
| } |
| for await (const item of responseStream.stream) { | ||
| process.stdout.write(item.candidates[0].content.parts[0].text); | ||
| } |
There was a problem hiding this comment.
To prevent potential runtime crashes (e.g., TypeError: Cannot read properties of undefined) if the API response is empty or blocked by safety filters, use optional chaining and a fallback value when accessing deeply nested properties.
| for await (const item of responseStream.stream) { | |
| process.stdout.write(item.candidates[0].content.parts[0].text); | |
| } | |
| for await (const item of responseStream.stream) { | |
| process.stdout.write(item.candidates?.[0]?.content?.parts?.[0]?.text ?? ''); | |
| } |
|
Here is the summary of changes. You are about to add 10 region tags.
You are about to delete 10 region tags.
This comment is generated by snippet-bot.
|
angelcaamal
changed the title
angelcaamal
changed the title
2 participants
Description
Fixes #
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
npm test(see Testing)npm run lint(see Style)GoogleCloudPlatform/nodejs-docs-samples. Not a fork.