-
Notifications
You must be signed in to change notification settings - Fork 60.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add google api safety settings by Settings page #4847
Conversation
@yeung66 is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe recent changes enhance the application by introducing configurable Google safety settings. Users can now select a content filtering safety threshold through a new UI element in the settings interface. This choice dynamically influences API interactions, allowing for more personalized safety measures. Updates span across constants, localization files, state management, and API configurations, streamlining the overall user experience. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
app/components/settings.tsx (1)
Line range hint
696-701
: Add keyboard accessibility for mouse events.- <IconButton - icon={<ResetIcon></ResetIcon>} - text={Locale.Settings.Usage.Check} - onClick={() => checkUsage(true)} - /> + <IconButton + icon={<ResetIcon></ResetIcon>} + text={Locale.Settings.Usage.Check} + onClick={() => checkUsage(true)} + onKeyUp={(e) => e.key === 'Enter' && checkUsage(true)} + />Actions triggered by mouse clicks should also be accessible via keyboard events to support users who rely on keyboard navigation.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- app/client/platforms/google.ts (4 hunks)
- app/components/settings.tsx (2 hunks)
- app/constant.ts (1 hunks)
- app/locales/cn.ts (1 hunks)
- app/locales/en.ts (1 hunks)
- app/store/access.ts (2 hunks)
Files skipped from review due to trivial changes (3)
- app/locales/cn.ts
- app/locales/en.ts
- app/store/access.ts
Additional context used
Biome
app/client/platforms/google.ts
[error] 196-196: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
app/components/settings.tsx
[error] 104-104: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 116-116: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 339-339: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 360-360: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 375-375: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 393-393: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 406-406: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 417-417: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 435-435: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 449-449: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 459-459: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 689-689: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 696-701: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 738-738: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 755-755: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 795-795: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 811-811: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 827-827: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 847-848: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 864-864: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
Additional comments not posted (3)
app/constant.ts (1)
77-82
: Great addition of theGoogleSafetySettingsThreshold
enum to manage Google API safety settings.This enumeration will facilitate the dynamic configuration of safety thresholds, enhancing flexibility for users. Good job on implementing this feature.
app/client/platforms/google.ts (1)
Line range hint
67-103
: Ensure consistency in safety settings usage.The usage of
googleSafetySettings
for all categories ensures consistent behavior across different types of content. However, consider adding a mechanism to allow different settings for different categories if needed in the future.Verification successful
The verification confirms that
googleSafetySettings
is used consistently across all categories in the specified file.
app/client/platforms/google.ts
: Consistent usage ofaccessStore.googleSafetySettings
for all categories.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all categories use the `googleSafetySettings` value from the `accessStore`. # Test: Search for the usage of `googleSafetySettings`. Expect: Consistent usage across all categories. rg --type typescript 'googleSafetySettings'Length of output: 78
Script:
#!/bin/bash # Description: Verify that all categories use the `googleSafetySettings` value from the `accessStore`. # Test: Search for the usage of `googleSafetySettings`. Expect: Consistent usage across all categories. rg --type ts 'googleSafetySettings'Length of output: 665
app/components/settings.tsx (1)
1126-1154
: Excellent implementation of the Google Safety Settings in the UI.This addition allows users to easily modify their Google API safety settings directly from the settings page. The use of a dropdown menu for selection is user-friendly and effective.
Your build has completed! |
So,Has the feature of this security settings interface been implemented? Still not on the agenda? |
It has already been implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please resolve the conflict ~
the conflict already resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
app/client/platforms/google.ts (1)
74-74
: Issues Found: Lack of Test Coverage forGeminiProApi
The
GeminiProApi
class, which includes dynamic safety settings, does not appear to have any associated test cases in the repository. It is crucial to add comprehensive tests to ensure the reliability and correctness of these dynamic settings.
- File:
app/client/platforms/google.ts
- File:
app/client/api.ts
Please add test cases to cover various scenarios and edge cases for the
GeminiProApi
class.Analysis chain
Approved: Dynamic configuration of safety settings in
GeminiProApi
.The use of
accessStore.googleSafetySettings
to dynamically set safety thresholds is a robust implementation that enhances flexibility and user control.Ensure thorough testing of this dynamic setting, particularly for edge cases where the safety setting might switch frequently.
Also applies to: 97-109
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration and testing of dynamic safety settings. # Test: Search for test cases related to `GeminiProApi`. Expect: Comprehensive tests covering various safety settings. rg --type python "GeminiProApi" -A 10Length of output: 68
Script:
#!/bin/bash # Find all Python files and search for occurrences of `GeminiProApi` within them. fd -e py -x rg "GeminiProApi" {}Length of output: 34
Script:
#!/bin/bash # Search for occurrences of `GeminiProApi` across all files in the repository. rg "GeminiProApi"Length of output: 220
Script:
#!/bin/bash # Search for test files or directories that might contain tests for `GeminiProApi`. fd -e py -e js -e ts -e tsx -e jsx -e test -e specLength of output: 2317
Script:
#!/bin/bash # Search for test cases within the source files that might be testing the `GeminiProApi` class. rg "GeminiProApi" -A 10Length of output: 1833
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- app/client/platforms/google.ts (2 hunks)
- app/components/settings.tsx (2 hunks)
- app/constant.ts (1 hunks)
- app/locales/cn.ts (1 hunks)
- app/locales/en.ts (1 hunks)
- app/store/access.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- app/locales/cn.ts
Additional comments not posted (5)
app/store/access.ts (1)
63-63
: Approved: Addition ofgoogleSafetySettings
toDEFAULT_ACCESS_STATE
.The addition of
googleSafetySettings
with a default value ofBLOCK_ONLY_HIGH
is a good implementation, ensuring that the system has a default, safe behavior.Ensure that this new setting is properly integrated and tested across components that utilize
DEFAULT_ACCESS_STATE
.Verification successful
Verified:
googleSafetySettings
is properly integrated across the codebase.The
googleSafetySettings
property is utilized in various components, including settings and the Google platform client, confirming that it is well-integrated and managed correctly.
app/store/access.ts
app/components/settings.tsx
app/client/platforms/google.ts
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `googleSafetySettings` is properly integrated. # Test: Search for usage of `DEFAULT_ACCESS_STATE` in other components. Expect: Proper handling of `googleSafetySettings`. rg --type python "DEFAULT_ACCESS_STATE" -A 5Length of output: 75
Script:
#!/bin/bash # Description: Verify that `googleSafetySettings` is properly integrated. # Test: Search for usage of `DEFAULT_ACCESS_STATE` in other components. Expect: Proper handling of `googleSafetySettings`. rg --type js "DEFAULT_ACCESS_STATE" -A 5Length of output: 40
Script:
#!/bin/bash # Description: Verify that `googleSafetySettings` is properly integrated. # Test: Search for usage of `DEFAULT_ACCESS_STATE` in other components. Expect: Proper handling of `googleSafetySettings`. rg --type js,ts "DEFAULT_ACCESS_STATE" -A 5Length of output: 73
Script:
#!/bin/bash # Description: Verify that `googleSafetySettings` is properly integrated. # Test: Search for usage of `DEFAULT_ACCESS_STATE` in other components. Expect: Proper handling of `googleSafetySettings`. # Search in JavaScript files rg --type js "DEFAULT_ACCESS_STATE" -A 5 # Search in TypeScript files rg --type ts "DEFAULT_ACCESS_STATE" -A 5Length of output: 538
Script:
#!/bin/bash # Description: Verify that `googleSafetySettings` is properly integrated. # Test: Search for usage of `googleSafetySettings` in other components. rg "googleSafetySettings" -A 5Length of output: 2186
app/constant.ts (1)
92-97
: Approved: Addition ofGoogleSafetySettingsThreshold
enum.The enum provides clear, descriptive values for different safety settings, which enhances code readability and maintainability.
Verify that these enum values (
BLOCK_NONE
,BLOCK_ONLY_HIGH
, etc.) are consistent with the Google API's expected input values.Verification successful
Approved: Addition of
GoogleSafetySettingsThreshold
enum.The enum provides clear, descriptive values for different safety settings, which enhances code readability and maintainability. The enum values are used consistently within the codebase in the following files:
app/store/access.ts
app/components/settings.tsx
Verify that these enum values (
BLOCK_NONE
,BLOCK_ONLY_HIGH
, etc.) are consistent with the Google API's expected input values by consulting the Google API documentation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency of enum values with Google API. # Test: Manually check the Google API documentation to ensure the values match. echo "Check Google API documentation for expected safety settings values."Length of output: 144
Script:
#!/bin/bash # Search for the usage of GoogleSafetySettingsThreshold enum values within the codebase rg 'GoogleSafetySettingsThreshold' -A 5Length of output: 1972
app/locales/en.ts (1)
395-398
: Approved: Addition ofGoogleSafetySettings
section in localization file.The new section for selecting a safety filtering level is well-defined and enhances user interface clarity.
Verify that the text is correctly localized and clear, ensuring that it is consistent across different languages if applicable.
Verification successful
Verified: Localization and integration of
GoogleSafetySettings
section.The
GoogleSafetySettings
section is consistently localized in both English (en.ts
) and Chinese (cn.ts
). The integration into application settings and constants is also confirmed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify localization of `GoogleSafetySettings`. # Test: Search for localization files and ensure consistency of `GoogleSafetySettings` section. rg --type python "GoogleSafetySettings" -A 5Length of output: 75
Script:
#!/bin/bash # Description: Verify localization of `GoogleSafetySettings`. # Test: Search for localization files and ensure consistency of `GoogleSafetySettings` section. # Searching within TypeScript files rg --type ts "GoogleSafetySettings" -A 5 # Searching within JSON files rg --type json "GoogleSafetySettings" -A 5Length of output: 3248
app/components/settings.tsx (2)
60-60
: Review of new import: GoogleSafetySettingsThresholdThe import of
GoogleSafetySettingsThreshold
is necessary for the new feature. Ensure that this is a well-defined and exported entity from the constants module.
1129-1157
: Review of the new Google Safety Settings UI componentThis new
ListItem
component allows users to select a Google safety setting threshold. The implementation uses aSelect
component populated with options fromGoogleSafetySettingsThreshold
.Observations:
- The
Select
component is correctly bound toaccessStore.googleSafetySettings
for its value, which ensures that the UI reflects the current setting.- The
onChange
handler updates the store appropriately, which is crucial for the functionality to work correctly.Suggestions:
- Validation and Error Handling: Ensure that the values provided by
GoogleSafetySettingsThreshold
are valid and handled correctly by the Google API.- UI Feedback: Consider providing immediate feedback or confirmation to the user when the safety setting is changed. This could be implemented using a toast notification or similar mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/platforms/google.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/platforms/google.ts
After merging the main branch, I found a bug about generating the gemini request url. Therefor I had a extra commit to fix the problem#5014 |
@yeung66 some update for google ,please resolve again |
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/platforms/google.ts (2 hunks)
- app/constant.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/client/platforms/google.ts
- app/constant.ts
problem
Google's Gemini API has safety settings and will block some content that they think dangerous.
Google supports setting a security filtering level to control the scope of filtering. The current security filtering level is hard-coded as BLOCK_ONLY_HIGH, which will only block content that Google deems highly likely to be unsafe. This hardcoded approach is not adjustable and lacks flexibility.
So an option for selecting the security filtering level has been added to the settings page. The default remains BLOCK_ONLY_HIGH, but users can now choose other security levels.
related issues:
Summary by CodeRabbit