-
Notifications
You must be signed in to change notification settings - Fork 30
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
chore(chat): refactor chat component #1950
base: development
Are you sure you want to change the base?
chore(chat): refactor chat component #1950
Conversation
4759b66
to
6ec155b
Compare
/deploy-review
|
2 similar comments
/deploy-review
|
/deploy-review
|
6ec155b
to
c8762c0
Compare
/deploy-review
|
/deploy-review
|
/deploy-review
|
c7b6e05
to
bf61c6b
Compare
/deploy-review
|
bf61c6b
to
985b8b0
Compare
… chat component into custom hook
0f1ad97
to
3d5dcf9
Compare
/deploy-review
|
package.json
Outdated
@@ -168,5 +170,6 @@ | |||
"fs": false, | |||
"net": false, | |||
"tls": false | |||
} | |||
}, | |||
"packageManager": "[email protected]+sha1.4ba7fc5c6e704fce2066ecbfb0b0d8976fe62447" |
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.
I think we don't need this
@@ -13,7 +13,7 @@ import { ModelsSelectors } from '@/src/store/models/models.reducers'; | |||
|
|||
import { Combobox } from '../Common/Combobox'; | |||
import { DisableOverlay } from '../Common/DisableOverlay'; | |||
import { ModelSelectRow } from './ConversationSettings'; |
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 component still use state directly (useAppSelector
)
@@ -17,7 +17,7 @@ import { SettingsSelectors } from '@/src/store/settings/settings.reducers'; | |||
import { SendMessageButton } from '@/src/components/Chat/ChatInput/SendMessageButton'; | |||
import Tooltip from '@/src/components/Common/Tooltip'; | |||
|
|||
import RefreshCW from '../../../../public/images/icons/refresh-cw.svg'; | |||
import RefreshCW from '@/public/images/icons/refresh-cw.svg'; |
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 component still use state directly (useAppSelector
, useAppDispatch
)
@@ -5,7 +5,8 @@ import { Message } from '@/src/types/chat'; | |||
import { ConversationsSelectors } from '@/src/store/conversations/conversations.reducers'; | |||
import { useAppSelector } from '@/src/store/hooks'; | |||
|
|||
import { ChatInputFooter } from './ChatInputFooter'; | |||
import { ChatInputFooter } from '@/src/components/Chat/common/ChatInputFooter'; |
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 component still use state directly (useAppSelector
)
@@ -35,12 +35,12 @@ import { UISelectors } from '@/src/store/ui/ui.reducers'; | |||
|
|||
import { errorsMessages } from '@/src/constants/errors'; | |||
|
|||
import { ChatControls } from '@/src/components/Chat/ChatInput/ChatControls'; | |||
import { AdjustedTextarea } from '@/src/components/Common/AdjustedTextarea'; |
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 component still use state directly (useAppSelector
, useAppDispatch
)
import { Combobox } from '../Common/Combobox'; | ||
import Loader from '../Common/Loader'; | ||
import ShareIcon from '../Common/ShareIcon'; | ||
import { ModelIcon } from '@/src/components/Chatbar/ModelIcon'; |
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 component still use state directly (useAppSelector
)
import { SettingsSelectors } from '@/src/store/settings/settings.reducers'; | ||
|
||
import { REPLAY_AS_IS_MODEL } from '@/src/constants/chat'; | ||
|
||
import { Spinner } from '../Common/Spinner'; | ||
import { Spinner } from '../../Common/Spinner'; |
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 component still use state directly (useAppSelector
)
@@ -24,16 +24,16 @@ import { | |||
} from '@/src/constants/chat'; | |||
import { DEFAULT_ASSISTANT_SUBMODEL_ID } from '@/src/constants/default-ui-settings'; | |||
|
|||
import { TemperatureSlider } from './components/Temperature'; |
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 component still use state directly (useAppSelector
, useAppDispatch
)
import Tooltip from '../Common/Tooltip'; | ||
import ChatMDComponent from '../Markdown/ChatMDComponent'; | ||
import { VisualizerRenderer } from '../VisualalizerRenderer/VisualizerRenderer'; | ||
|
||
import Link from '@/public/images/icons/arrow-up-right-from-square.svg'; |
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 component still use state directly (useAppSelector
, useAppDispatch
)
import { Spinner } from '../Common/Spinner'; | ||
import ChatMDComponent from '../Markdown/ChatMDComponent'; | ||
import { MessageAttachments } from './MessageAttachments'; | ||
|
||
import ChevronDown from '@/public/images/icons/chevron-down.svg'; |
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 component still use state directly (useAppSelector
)
@@ -21,9 +21,9 @@ import { | |||
import { useAppDispatch, useAppSelector } from '@/src/store/hooks'; | |||
import { UISelectors } from '@/src/store/ui/ui.reducers'; | |||
|
|||
import { ChatInputFooter } from '@/src/components/Chat/common/ChatInputFooter'; |
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 component still use state directly (useAppSelector
, useAppDispatch
)
Main problem of this approach is prop drilling from top parent component to target child component. We collect a lot of data and handlers on top level into big object and than drill them down to target. On any changes data-object will be new - rerender. At this moment the goal to make pure Chat-component wasn't achieved because a lot of child components are using state directly ( |
Description:
Refactor component by splitting the code into sub-components and custom hooks to enhance readability and maintainability
Issues:
UI changes
Checklist:
fix(<scope>):
,feat(<scope>):
,feature(<scope>):
,chore(<scope>):
,hotfix(<scope>):
ore2e(<scope>):
. If contains breaking changes then the pull request name must start withfix(<scope>)!:
,feat(<scope>)!:
,feature(<scope>)!:
,chore(<scope>)!:
,hotfix(<scope>)!:
ore2e(<scope>)!:
where<scope>
is name of affected project:chat
,chat-e2e
,overlay
,shared
,sandbox-overlay
, etc.(Issue #<TICKET_ID>)
(comma-separated list of issues)