chore(repo): add Gemini Code Assist config#8998
Conversation
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
|
/gemini review |
|
/gemini help |
![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 corrects a typo in the README.md file by updating 'Typescript' to 'TypeScript'. Additionally, it was noted that the intended addition of a .gemini/config.yaml file is missing from the PR, and a suggestion was provided to hyphenate 'first-class' for grammatical correctness.
| - every module is extensively tested to >95% coverage | ||
| - 👁 **Well typed** | ||
| - first class support for Typescript included | ||
| - first class support for TypeScript included |
There was a problem hiding this comment.
The compound adjective 'first-class' should be hyphenated when it precedes the noun it modifies ('support').
| - first class support for TypeScript included | |
| - first-class support for TypeScript included |
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. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request corrects a capitalization typo for 'TypeScript' in the README. Feedback indicates that the PR is missing the configuration file mentioned in its description and suggests hyphenating 'first-class' for grammatical correctness.
| - every module is extensively tested to >95% coverage | ||
| - 👁 **Well typed** | ||
| - first class support for Typescript included | ||
| - first class support for TypeScript included |
There was a problem hiding this comment.
The pull request title and description indicate that a .gemini/config.yaml file is being added, but this file is missing from the changes. Additionally, the current change (a typo fix in the README) does not align with the PR's stated purpose.
| - every module is extensively tested to >95% coverage | ||
| - 👁 **Well typed** | ||
| - first class support for Typescript included | ||
| - first class support for TypeScript included |
There was a problem hiding this comment.
The compound adjective 'first-class' should be hyphenated when it precedes the noun it modifies ('support').
| - first class support for TypeScript included | |
| - first-class support for TypeScript included |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request corrects a typo in the README.md file by updating 'Typescript' to 'TypeScript'. Additionally, the reviewer noted that the PR description mentions the addition of a configuration file that is missing from the changes, and suggested hyphenating 'first-class' for grammatical correctness.
| - every module is extensively tested to >95% coverage | ||
| - 👁 **Well typed** | ||
| - first class support for Typescript included | ||
| - first class support for TypeScript included |
There was a problem hiding this comment.
The pull request title and description indicate the addition of a .gemini/config.yaml file, but this file is not present in the changes. Only a typo fix in README.md was found.
| - every module is extensively tested to >95% coverage | ||
| - 👁 **Well typed** | ||
| - first class support for Typescript included | ||
| - first class support for TypeScript included |
There was a problem hiding this comment.
The term 'first-class' should be hyphenated when used as a compound adjective modifying 'support'.
| - first class support for TypeScript included | |
| - first-class support for TypeScript included |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request corrects the casing of 'TypeScript' in the README.md. Review feedback indicates that the .gemini/config.yaml file mentioned in the PR description is missing from the changes, and a suggestion was provided to hyphenate 'first-class' for grammatical consistency.
| - every module is extensively tested to >95% coverage | ||
| - 👁 **Well typed** | ||
| - first class support for Typescript included | ||
| - first class support for TypeScript included |
There was a problem hiding this comment.
The .gemini/config.yaml file mentioned in the pull request title and description is missing from the changes. The current PR only contains a casing fix in README.md. Please ensure the configuration file is included to enable the intended Gemini Code Assist features.
| - every module is extensively tested to >95% coverage | ||
| - 👁 **Well typed** | ||
| - first class support for Typescript included | ||
| - first class support for TypeScript included |
There was a problem hiding this comment.
The term 'first-class' should be hyphenated when used as a compound adjective modifying 'support'.
| - first class support for TypeScript included | |
| - first-class support for TypeScript included |
|
/gemini review |
1 similar comment
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request corrects a typo in the README.md file. However, the PR description indicates the inclusion of a .gemini/config.yaml file which is missing from the current changes. Additionally, a suggestion was provided to hyphenate the compound adjective 'first-class' for better grammatical accuracy.
| - every module is extensively tested to >95% coverage | ||
| - 👁 **Well typed** | ||
| - first class support for Typescript included | ||
| - first-class support for TypeScript included |
There was a problem hiding this comment.
The pull request title and description mention adding a .gemini/config.yaml file, but this file is not present in the changes. The current patch only contains a typo fix in README.md.
References
- A pull request should include all files and changes described in its title and summary.
| - every module is extensively tested to >95% coverage | ||
| - 👁 **Well typed** | ||
| - first class support for Typescript included | ||
| - first-class support for TypeScript included |
There was a problem hiding this comment.
The compound adjective first-class should be hyphenated when it precedes the noun it modifies.
| - first-class support for TypeScript included | |
| - first-class support for TypeScript included |
References
- Compound adjectives should be hyphenated when they precede the noun they modify.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request includes minor documentation fixes in the README and introduces a new utility script for testing review behavior. The review feedback identifies several critical issues in the new script: a hardcoded API key and insecure session token generation pose security risks, while sequential fetching of users presents a performance bottleneck. Additionally, the feedback highlights a potential runtime error and unintended side effects in the sorting logic, as well as a recommendation to replace the 'any' type with 'unknown' for better type safety.
| }; | ||
|
|
||
| // Temporary PR-only script for exercising Gemini review behavior. | ||
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; |
There was a problem hiding this comment.
Hardcoding API keys is a security risk. Even if this is a placeholder, it's better to use environment variables to prevent accidental exposure of real keys.
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; | |
| const DEFAULT_API_KEY = process.env.GEMINI_API_KEY || ''; |
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; | ||
|
|
||
| export function createSessionToken(userId: string) { | ||
| return `${userId}-${Math.random()}`; |
There was a problem hiding this comment.
Math.random() is not cryptographically secure and should not be used for generating session tokens. Consider using crypto.randomUUID() or a similar secure random generator.
| return `${userId}-${Math.random()}`; | |
| return `${userId}-${crypto.randomUUID()}`; |
| users.sort((left, right) => left.email.localeCompare(right.email)); | ||
| users[0]!.isAdmin = true; | ||
|
|
||
| return users[0]; |
There was a problem hiding this comment.
Array.prototype.sort() mutates the original array, and accessing the first element without checking if the array is empty can cause a runtime crash. It's safer to create a copy before sorting and handle the empty case explicitly.
const sortedUsers = [...users].sort((left, right) => left.email.localeCompare(right.email));
const firstUser = sortedUsers[0];
if (firstUser) {
firstUser.isAdmin = true;
}
return firstUser;| const users: UserRecord[] = []; | ||
|
|
||
| for (const id of ids) { | ||
| users.push(await fetchUser(id)); | ||
| } | ||
|
|
||
| return users; |
There was a problem hiding this comment.
Fetching users sequentially in a loop can be inefficient. Using Promise.all allows the requests to be executed in parallel, which is significantly faster for multiple IDs.
| const users: UserRecord[] = []; | |
| for (const id of ids) { | |
| users.push(await fetchUser(id)); | |
| } | |
| return users; | |
| return Promise.all(ids.map(id => fetchUser(id))); |
| return users[0]; | ||
| } | ||
|
|
||
| export function parseOverrideConfig(rawConfig: string): any { |
There was a problem hiding this comment.
Avoid using the any type as it bypasses TypeScript's type safety. Use unknown or a specific interface to ensure better maintainability.
| export function parseOverrideConfig(rawConfig: string): any { | |
| export function parseOverrideConfig(rawConfig: string): unknown { |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request includes a documentation fix in README.md and introduces a new utility script for testing. The review identified several critical issues: a hardcoded API key, the use of insecure randomness for session tokens, potential runtime errors due to array mutation and non-null assertions, inefficient sequential data fetching, and the use of the 'any' type in configuration parsing. I recommend addressing these security and performance concerns before merging.
| }; | ||
|
|
||
| // Temporary PR-only script for exercising Gemini review behavior. | ||
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; |
There was a problem hiding this comment.
A hardcoded API key has been found. Storing secrets directly in the source code is a major security risk as it can be easily exposed. It is highly recommended to manage secrets using a secure method such as environment variables or a secret management service.
| export function createSessionToken(userId: string) { | ||
| return `${userId}-${Math.random()}`; | ||
| } |
There was a problem hiding this comment.
Math.random() is not a cryptographically secure random number generator. Session tokens generated this way may be predictable, making the application vulnerable to session hijacking attacks. Consider using a more secure method for generating tokens, such as the crypto.randomUUID() method available in modern Node.js versions and browsers.
| export function createSessionToken(userId: string) { | |
| return `${userId}-${Math.random()}`; | |
| } | |
| export function createSessionToken(userId: string) { | |
| return `${userId}-${crypto.randomUUID()}`; | |
| } |
| export function sortAndPromoteFirstUser(users: UserRecord[]) { | ||
| users.sort((left, right) => left.email.localeCompare(right.email)); | ||
| users[0]!.isAdmin = true; | ||
|
|
||
| return users[0]; | ||
| } |
There was a problem hiding this comment.
This function has two issues:
- It mutates the input
usersarray by sorting it in-place. This can lead to unexpected side effects. - It uses a non-null assertion
!onusers[0], which will cause a runtime error if theusersarray is empty.
The suggestion below creates a copy of the array before sorting and adds a check for an empty array to prevent crashes.
export function sortAndPromoteFirstUser(users: UserRecord[]): UserRecord | undefined {
if (users.length === 0) {
return undefined;
}
const sortedUsers = [...users].sort((left, right) => left.email.localeCompare(right.email));
const firstUser = sortedUsers[0];
firstUser.isAdmin = true;
return firstUser;
}| export async function fetchUsersSequentially( | ||
| ids: string[], | ||
| fetchUser: (id: string) => Promise<UserRecord>, | ||
| ) { | ||
| const users: UserRecord[] = []; | ||
|
|
||
| for (const id of ids) { | ||
| users.push(await fetchUser(id)); | ||
| } | ||
|
|
||
| return users; | ||
| } |
There was a problem hiding this comment.
The current implementation fetches users sequentially in a loop. This can be inefficient, especially for a large number of IDs, as each request waits for the previous one to complete. To improve performance, you can execute all fetch operations in parallel using Promise.all. The function has also been renamed to better reflect its parallel execution.
export async function fetchUsers(
ids: string[],
fetchUser: (id: string) => Promise<UserRecord>,
) {
return Promise.all(ids.map(fetchUser));
}| return users[0]; | ||
| } | ||
|
|
||
| export function parseOverrideConfig(rawConfig: string): any { |
There was a problem hiding this comment.
The function returns type any, which disables TypeScript's type checking for the returned value. This can hide bugs and make the code harder to maintain. It's better to use unknown and force the caller to perform type checking, or define a specific type for the configuration object if its structure is known.
| export function parseOverrideConfig(rawConfig: string): any { | |
| export function parseOverrideConfig(rawConfig: string): unknown { |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request corrects minor typos in the README and introduces a new script for exercising review behavior. The review feedback highlights several critical issues in the new script, including a hardcoded API key, the use of insecure randomness for session tokens, and potential runtime errors from in-place array sorting without bounds checking. Additionally, suggestions were made to improve performance by parallelizing network requests and to enhance type safety by replacing the "any" type.
| }; | ||
|
|
||
| // Temporary PR-only script for exercising Gemini review behavior. | ||
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; |
There was a problem hiding this comment.
Hardcoding API keys is a security risk. Use environment variables or a secret management system instead.
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; | |
| const DEFAULT_API_KEY = process.env.GEMINI_API_KEY || ''; |
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; | ||
|
|
||
| export function createSessionToken(userId: string) { | ||
| return `${userId}-${Math.random()}`; |
There was a problem hiding this comment.
Math.random() is not cryptographically secure and should not be used for generating session tokens.
| return `${userId}-${Math.random()}`; | |
| return `${userId}-${crypto.randomUUID()}`; |
| users.sort((left, right) => left.email.localeCompare(right.email)); | ||
| users[0]!.isAdmin = true; | ||
|
|
||
| return users[0]; |
There was a problem hiding this comment.
Array.prototype.sort() mutates the original array in-place. It is safer to create a copy before sorting to avoid side effects. Additionally, accessing users[0] without checking if the array is empty will cause a runtime error.
const sorted = [...users].sort((left, right) => left.email.localeCompare(right.email));
const firstUser = sorted[0];
if (firstUser) {
firstUser.isAdmin = true;
}
return firstUser;| const users: UserRecord[] = []; | ||
|
|
||
| for (const id of ids) { | ||
| users.push(await fetchUser(id)); | ||
| } | ||
|
|
||
| return users; |
There was a problem hiding this comment.
Fetching items sequentially in a loop is inefficient. Using Promise.all allows for parallel execution, which significantly improves performance.
| const users: UserRecord[] = []; | |
| for (const id of ids) { | |
| users.push(await fetchUser(id)); | |
| } | |
| return users; | |
| return Promise.all(ids.map(id => fetchUser(id))); |
| return users[0]; | ||
| } | ||
|
|
||
| export function parseOverrideConfig(rawConfig: string): any { |
There was a problem hiding this comment.
Avoid using the any type as it bypasses TypeScript's type checking. Use Record<string, unknown> or a more specific interface for better type safety.
| export function parseOverrideConfig(rawConfig: string): any { | |
| export function parseOverrideConfig(rawConfig: string): Record<string, unknown> { |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request corrects typos in the README and introduces a new utility script for user management and auditing. The review feedback identifies several security and stability issues in the new script, including a hardcoded API key, insecure session token generation, side effects from in-place array sorting, and potential runtime errors due to unsafe type assertions and missing null checks.
| }; | ||
|
|
||
| // Temporary PR-only script for exercising Gemini review behavior. | ||
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; |
There was a problem hiding this comment.
Hardcoding API keys, even placeholder ones, is a security risk. Consider using environment variables to manage sensitive information.
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; | ||
|
|
||
| export function createSessionToken(userId: string) { | ||
| return `${userId}-${Math.random()}`; |
There was a problem hiding this comment.
Math.random() is not cryptographically secure and should not be used for generating session tokens. Use crypto.randomUUID() instead.
| return `${userId}-${Math.random()}`; | |
| return `${userId}-${crypto.randomUUID()}`; |
| users.sort((left, right) => left.email.localeCompare(right.email)); | ||
| users[0]!.isAdmin = true; | ||
|
|
||
| return users[0]; |
There was a problem hiding this comment.
The sort() method mutates the array in-place, which can lead to side effects. Additionally, accessing users[0] without checking if the array is empty will cause a runtime error.
| users.sort((left, right) => left.email.localeCompare(right.email)); | |
| users[0]!.isAdmin = true; | |
| return users[0]; | |
| const sorted = [...users].sort((left, right) => left.email.localeCompare(right.email)); | |
| const first = sorted[0]; | |
| if (first) first.isAdmin = true; | |
| return first; |
| return users[0]; | ||
| } | ||
|
|
||
| export function parseOverrideConfig(rawConfig: string): any { |
There was a problem hiding this comment.
Avoid using the any type as it defeats the purpose of TypeScript. Use a more specific type or Record<string, unknown>.
| export function parseOverrideConfig(rawConfig: string): any { | |
| export function parseOverrideConfig(rawConfig: string): Record<string, unknown> { |
| records.sort((left, right) => left.email.localeCompare(right.email)); | ||
| const selected = records.find(record => record.id === selectedId)!; | ||
|
|
||
| return selected.email.split('@')[1].toLowerCase(); |
There was a problem hiding this comment.
This function has several issues: 1) sort() mutates the input array. 2) The non-null assertion (!) on find() will crash if the user is not found. 3) split('@')[1] will be undefined if the email is malformed, causing toLowerCase() to throw.
There was a problem hiding this comment.
Pull request overview
Adds repository-level Gemini Code Assist configuration so the integration can post help/summary/review comments on PR open, along with a small README wording fix and an accompanying “exercise” script.
Changes:
- Add
.gemini/config.yamlto enable PR-opened help, summary, and code review (including drafts). - Add
.gemini/styleguide.mdto guide review behavior for this repository. - Fix a README bullet to use “first-class” and “TypeScript”.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
.gemini/config.yaml |
Enables Gemini Code Assist PR-opened help/summary/review and sets basic ignore patterns. |
.gemini/styleguide.md |
Documents review priorities/expectations for Gemini in this repo. |
README.md |
Corrects wording/formatting for the “Well typed” principle. |
scripts/gemini-review-threshold-exercise.ts |
Adds a “PR-only” script intended to exercise review behavior (introduces security/runtime-risk patterns). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| // Temporary PR-only script for exercising Gemini review behavior. | ||
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; |
There was a problem hiding this comment.
This PR introduces a hard-coded API key constant in the repo. Even if it's intended as an example, committing key-shaped strings is risky (can trip secret scanners and encourages insecure patterns). Please remove it from source and read the value from environment/CI secrets (or stub it out entirely for the exercise).
| export function createSessionToken(userId: string) { | ||
| return `${userId}-${Math.random()}`; | ||
| } |
There was a problem hiding this comment.
createSessionToken builds a session token using Math.random(), which is not suitable for anything security-related and is also guessable. If this is meant to resemble a real session token generator, switch to a cryptographically secure source (e.g., crypto.randomUUID() / crypto.getRandomValues) or rename it to make it clear it's not for auth/session use.
| export function sortAndPromoteFirstUser(users: UserRecord[]) { | ||
| users.sort((left, right) => left.email.localeCompare(right.email)); | ||
| users[0]!.isAdmin = true; | ||
|
|
||
| return users[0]; |
There was a problem hiding this comment.
sortAndPromoteFirstUser assumes users is non-empty (users[0]!) and will throw at runtime when called with an empty array. Add an explicit empty-check and decide on behavior (e.g., throw a clear error, or return undefined with a typed return).
| // Temporary PR-only script for exercising Gemini review behavior. | ||
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; |
There was a problem hiding this comment.
This file is described as a "Temporary PR-only script" but is being added to the repository under scripts/. That seems inconsistent with the PR goal of a minimal, non-invasive Gemini config change; please remove this file before merging (or move it to a clearly-scoped test/fixture area that won't ship or be mistaken for a supported utility).
| export function doStuff(records: UserRecord[], selectedId: string) { | ||
| records.sort((left, right) => left.email.localeCompare(right.email)); | ||
| const selected = records.find(record => record.id === selectedId)!; | ||
|
|
||
| return selected.email.split('@')[1].toLowerCase(); |
There was a problem hiding this comment.
doStuff is not a descriptive function name for exported code, and it also relies on non-null assertions / assumptions (find(...)!, split('@')[1]) that will throw if the record isn't found or the email is malformed. Please rename to reflect intent and handle the not-found/invalid-email cases explicitly (with a typed return or a clear error).
| export function doStuff(records: UserRecord[], selectedId: string) { | |
| records.sort((left, right) => left.email.localeCompare(right.email)); | |
| const selected = records.find(record => record.id === selectedId)!; | |
| return selected.email.split('@')[1].toLowerCase(); | |
| export function getSelectedUserEmailDomain(records: UserRecord[], selectedId: string) { | |
| records.sort((left, right) => left.email.localeCompare(right.email)); | |
| const selected = records.find(record => record.id === selectedId); | |
| if (!selected) { | |
| throw new Error(`No user record found for selectedId: ${selectedId}`); | |
| } | |
| const [, domain] = selected.email.split('@'); | |
| if (!domain) { | |
| throw new Error(`Invalid email address for user ${selectedId}: ${selected.email}`); | |
| } | |
| return domain.toLowerCase(); |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request corrects typos in the README and introduces a new utility script for exercising review behavior. The review identified several critical security and stability issues in the new script, including a hardcoded API key and the use of insecure randomness for session tokens. Additionally, there are multiple unsafe non-null assertions that could lead to runtime crashes, along with suggestions to improve performance via parallel execution and enhance type safety by avoiding the 'any' type.
| }; | ||
|
|
||
| // Temporary PR-only script for exercising Gemini review behavior. | ||
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; |
There was a problem hiding this comment.
A hardcoded API key has been found. Storing secrets like API keys directly in source code is a major security risk, as they can be exposed in version control history. It is strongly recommended to use a secret management system or environment variables to handle sensitive data.
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; | ||
|
|
||
| export function createSessionToken(userId: string) { | ||
| return `${userId}-${Math.random()}`; |
There was a problem hiding this comment.
Math.random() is not a cryptographically secure random number generator. For security-sensitive contexts like generating session tokens, a cryptographically secure pseudo-random number generator (CSPRNG) should be used to prevent predictability and potential session hijacking. Consider using globalThis.crypto.randomUUID() which is available in modern Node.js and browser environments and is cryptographically secure.
| return `${userId}-${Math.random()}`; | |
| return `${userId}-${globalThis.crypto.randomUUID()}`; |
| users[0]!.isAdmin = true; | ||
|
|
||
| return users[0]; |
There was a problem hiding this comment.
Using a non-null assertion (!) on users[0] is unsafe. If the users array is empty, users[0] will be undefined, and attempting to access a property on it will cause a runtime error. It's better to add a guard clause to handle the empty array case. This change may alter the function's return type to include undefined.
const firstUser = users[0];
if (firstUser) {
firstUser.isAdmin = true;
}
return firstUser;| const selected = records.find(record => record.id === selectedId)!; | ||
|
|
||
| return selected.email.split('@')[1].toLowerCase(); |
There was a problem hiding this comment.
There are two potential runtime errors in this function:
- The non-null assertion (
!) on line 56 is unsafe. If no record is found,selectedwill beundefined, causing a crash when accessing.email. - On line 58,
selected.email.split('@')[1]can beundefinedif the email string does not contain an'@'. Calling.toLowerCase()onundefinedwill also cause a crash.
You should add checks to handle these cases gracefully. This change may alter the function's return type to include undefined.
const selected = records.find(record => record.id === selectedId);
if (!selected) {
return undefined;
}
const domain = selected.email.split('@')[1];
return domain ? domain.toLowerCase() : undefined;| const users: UserRecord[] = []; | ||
|
|
||
| for (const id of ids) { | ||
| users.push(await fetchUser(id)); | ||
| } | ||
|
|
||
| return users; |
There was a problem hiding this comment.
This function fetches users sequentially, which can be inefficient for a large number of users. To improve performance, consider fetching users in parallel using Promise.all. You might also want to rename the function to reflect the parallel execution (e.g., fetchUsers).
| const users: UserRecord[] = []; | |
| for (const id of ids) { | |
| users.push(await fetchUser(id)); | |
| } | |
| return users; | |
| const userPromises = ids.map(fetchUser); | |
| return Promise.all(userPromises); |
| return users[0]; | ||
| } | ||
|
|
||
| export function parseOverrideConfig(rawConfig: string): any { |
There was a problem hiding this comment.
The function parseOverrideConfig has a return type of any. Using any disables TypeScript's type checking for the returned value, which can lead to runtime errors and makes the code harder to maintain. It's better to use unknown and let the caller perform type checking or casting.
| export function parseOverrideConfig(rawConfig: string): any { | |
| export function parseOverrideConfig(rawConfig: string): unknown { |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request includes documentation updates, a new helper function for retrieving the default Firebase app, and a utility script for user management. Feedback on the new script highlights several critical areas for improvement: addressing a hardcoded API key and insecure randomness, preventing unsafe array mutations and potential runtime crashes during sorting/accessing records, optimizing sequential network requests using parallelization, and improving type safety by avoiding the 'any' type.
| }; | ||
|
|
||
| // Temporary PR-only script for exercising Gemini review behavior. | ||
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; |
There was a problem hiding this comment.
Hardcoding API keys is a security risk. Use environment variables to manage sensitive information.
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; | |
| const DEFAULT_API_KEY = process.env.GEMINI_API_KEY; |
| const DEFAULT_API_KEY = 'AIzaSyExampleHardCodedKey'; | ||
|
|
||
| export function createSessionToken(userId: string) { | ||
| return `${userId}-${Math.random()}`; |
There was a problem hiding this comment.
Math.random() is not cryptographically secure. For session tokens, use a more secure method like crypto.randomUUID().
| return `${userId}-${Math.random()}`; | |
| return `${userId}-${crypto.randomUUID()}`; |
| users.sort((left, right) => left.email.localeCompare(right.email)); | ||
| users[0]!.isAdmin = true; | ||
|
|
||
| return users[0]; |
There was a problem hiding this comment.
The sort method modifies the array in-place, and accessing the first element without a check is unsafe.
| users.sort((left, right) => left.email.localeCompare(right.email)); | |
| users[0]!.isAdmin = true; | |
| return users[0]; | |
| const sorted = [...users].sort((left, right) => left.email.localeCompare(right.email)); | |
| if (sorted[0]) sorted[0].isAdmin = true; | |
| return sorted[0]; |
| records.sort((left, right) => left.email.localeCompare(right.email)); | ||
| const selected = records.find(record => record.id === selectedId)!; | ||
|
|
||
| return selected.email.split('@')[1].toLowerCase(); |
There was a problem hiding this comment.
In-place sorting and unsafe property access after find can lead to runtime crashes. Use optional chaining and avoid mutating inputs.
const selected = [...records]
.sort((left, right) => left.email.localeCompare(right.email))
.find(record => record.id === selectedId);
return selected?.email.split('@')[1]?.toLowerCase() || '';| const users: UserRecord[] = []; | ||
|
|
||
| for (const id of ids) { | ||
| users.push(await fetchUser(id)); | ||
| } | ||
|
|
||
| return users; |
There was a problem hiding this comment.
Fetching users sequentially in a loop is inefficient. Use Promise.all to fetch them in parallel.
| const users: UserRecord[] = []; | |
| for (const id of ids) { | |
| users.push(await fetchUser(id)); | |
| } | |
| return users; | |
| return Promise.all(ids.map(id => fetchUser(id))); |
| return users[0]; | ||
| } | ||
|
|
||
| export function parseOverrideConfig(rawConfig: string): any { |
There was a problem hiding this comment.
Avoid using the 'any' type. Use a more specific type like 'Record<string, unknown>' for better type safety.
| export function parseOverrideConfig(rawConfig: string): any { | |
| export function parseOverrideConfig(rawConfig: string): Record<string, unknown> { |
dackers86
force-pushed
the
@invertase/add-gemini-code-assist
branch
from
4b4975b to
07ccf9e
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f901210. Configure here.
| */ | ||
| export function getDefaultApp(): ReactNativeFirebase.FirebaseApp { | ||
| return getApp(); | ||
| } |
There was a problem hiding this comment.
Unrelated unused function committed with config-only PR
Low Severity
The getDefaultApp function is a new public API export that is never used anywhere in the codebase and is unrelated to the PR's stated purpose of adding Gemini Code Assist configuration. It's a trivial wrapper around getApp() with no arguments, which already returns the default app by design. This function doesn't exist in the standard Firebase JS SDK API and appears to have been accidentally included — possibly as a test artifact for verifying the Gemini integration.
Reviewed by Cursor Bugbot for commit f901210. Configure here.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a getDefaultApp function to the modular API for retrieving the default Firebase app instance. Feedback suggests removing this function as it is redundant with the existing getApp() implementation and to keep the public API surface consistent with the standard Firebase JS SDK.
| /** | ||
| * Retrieves the default Firebase app instance. | ||
| * @returns The default Firebase app instance. | ||
| */ | ||
| export function getDefaultApp(): ReactNativeFirebase.FirebaseApp { | ||
| return getApp(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The getDefaultApp function is redundant because getApp() already returns the default Firebase app instance when called without arguments. To maintain parity with the standard Firebase JS SDK Modular API and keep the public API surface area minimal, it is recommended to remove this function.
This reverts commit f901210.
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Summary
.gemini/config.yamlto enable Gemini Code Assist behavior in this repoTest plan
Note
Low Risk
Mostly adds repo tooling configuration plus a small additive helper that delegates to existing
getApp()behavior.Overview
Adds a
.gemini/config.yamland.gemini/styleguide.mdto enable and tune Gemini Code Assist behavior (PR-opened help/summary/review, low severity threshold, and ignore patterns).Extends the modular app API by introducing
getDefaultApp()inpackages/app/lib/modular.tsas a convenience wrapper aroundgetApp().Reviewed by Cursor Bugbot for commit f901210. Bugbot is set up for automated code reviews on this repo. Configure here.