-
Notifications
You must be signed in to change notification settings - Fork 14.2k
Cherrypick #10900 #11116
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
Cherrypick #10900 #11116
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,27 +4,43 @@ | |||||||||||||||||||||||||||
| * SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| import { Box, Text } from 'ink'; | ||||||||||||||||||||||||||||
| import { Box, Text, useIsScreenReaderEnabled } from 'ink'; | ||||||||||||||||||||||||||||
| import { useAppContext } from '../contexts/AppContext.js'; | ||||||||||||||||||||||||||||
| import { useUIState } from '../contexts/UIStateContext.js'; | ||||||||||||||||||||||||||||
| import { theme } from '../semantic-colors.js'; | ||||||||||||||||||||||||||||
| import { StreamingState } from '../types.js'; | ||||||||||||||||||||||||||||
| import { UpdateNotification } from './UpdateNotification.js'; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| import { homedir } from 'node:os'; | ||||||||||||||||||||||||||||
| import path from 'node:path'; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const settingsPath = path.join(homedir(), '.gemini', 'settings.json'); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| export const Notifications = () => { | ||||||||||||||||||||||||||||
| const { startupWarnings } = useAppContext(); | ||||||||||||||||||||||||||||
| const { initError, streamingState, updateInfo } = useUIState(); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const isScreenReaderEnabled = useIsScreenReaderEnabled(); | ||||||||||||||||||||||||||||
| const showStartupWarnings = startupWarnings.length > 0; | ||||||||||||||||||||||||||||
| const showInitError = | ||||||||||||||||||||||||||||
| initError && streamingState !== StreamingState.Responding; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if (!showStartupWarnings && !showInitError && !updateInfo) { | ||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||
| !showStartupWarnings && | ||||||||||||||||||||||||||||
| !showInitError && | ||||||||||||||||||||||||||||
| !updateInfo && | ||||||||||||||||||||||||||||
| !isScreenReaderEnabled | ||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||
| <> | ||||||||||||||||||||||||||||
| {isScreenReaderEnabled && ( | ||||||||||||||||||||||||||||
| <Text> | ||||||||||||||||||||||||||||
| You are currently in screen reader-friendly view. To switch out, open{' '} | ||||||||||||||||||||||||||||
| {settingsPath} and remove the entry for {'"screenReader"'}. | ||||||||||||||||||||||||||||
| </Text> | ||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||
|
Comment on lines
+38
to
+43
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid confusing users by pointing them to a potentially incorrect settings file, the notification message should be made more generic. Instead of a specific file path, it's better to refer to the setting name. Ideally, you could guide them to use the settings dialog, since
Suggested change
|
||||||||||||||||||||||||||||
| {updateInfo && <UpdateNotification message={updateInfo.message} />} | ||||||||||||||||||||||||||||
| {showStartupWarnings && ( | ||||||||||||||||||||||||||||
| <Box | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
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.
This hardcoded path to the user settings file is problematic because the
screenReadersetting can be configured in multiple locations (e.g., project or system-level files). Instructing the user to edit this specific file can be misleading. As this constant and its imports will be unused with the suggested change to the notification message, these lines should be removed.