Skip to content
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

Draft
wants to merge 29 commits into
base: development
Choose a base branch
from

Conversation

sergesha
Copy link
Contributor

Description:

Refactor component by splitting the code into sub-components and custom hooks to enhance readability and maintainability

Issues:

  • No #<TICKET_ID>

UI changes

  • No UI changes

Checklist:

  • the pull request name complies with Conventional Commits
  • the pull request name starts with fix(<scope>):, feat(<scope>):, feature(<scope>):, chore(<scope>):, hotfix(<scope>): or e2e(<scope>):. If contains breaking changes then the pull request name must start with fix(<scope>)!:, feat(<scope>)!:, feature(<scope>)!:, chore(<scope>)!:, hotfix(<scope>)!: or e2e(<scope>)!: where <scope> is name of affected project: chat, chat-e2e, overlay, shared, sandbox-overlay, etc.
  • the pull request name ends with (Issue #<TICKET_ID>) (comma-separated list of issues)
  • I confirm that do not share any confidential information like API keys or any other secrets and private URLs

@sergesha sergesha changed the title refactor(chat): refactor chat component chore(chat): refactor chat component Aug 21, 2024
@sergesha sergesha force-pushed the feature/refactor-chat-component branch from 4759b66 to 6ec155b Compare August 21, 2024 14:20
@IlyaBondar
Copy link
Collaborator

IlyaBondar commented Aug 21, 2024

/deploy-review

Environment URL: https://chat-ai-dial-chat-pr-1950.nightly-test.deltixhub.io
E2E tests status: failed

2 similar comments
@IlyaBondar
Copy link
Collaborator

IlyaBondar commented Aug 21, 2024

/deploy-review

Environment URL: https://chat-ai-dial-chat-pr-1950.nightly-test.deltixhub.io
E2E tests status: failed

@IlyaBondar
Copy link
Collaborator

IlyaBondar commented Aug 21, 2024

/deploy-review

Environment URL: https://chat-ai-dial-chat-pr-1950.nightly-test.deltixhub.io
E2E tests status: failed

@sergesha sergesha force-pushed the feature/refactor-chat-component branch from 6ec155b to c8762c0 Compare August 23, 2024 10:42
@IlyaBondar
Copy link
Collaborator

IlyaBondar commented Aug 23, 2024

/deploy-review

Environment URL: https://chat-ai-dial-chat-pr-1950.nightly-test.deltixhub.io
E2E tests status:

@IlyaBondar
Copy link
Collaborator

IlyaBondar commented Aug 23, 2024

/deploy-review

Environment URL: https://chat-ai-dial-chat-pr-1950.nightly-test.deltixhub.io
E2E tests status: failed

@IlyaBondar
Copy link
Collaborator

IlyaBondar commented Aug 26, 2024

/deploy-review

Environment URL: https://chat-ai-dial-chat-pr-1950.nightly-test.deltixhub.io
E2E tests status: success

@sergesha sergesha force-pushed the feature/refactor-chat-component branch from c7b6e05 to bf61c6b Compare August 26, 2024 09:46
@IlyaBondar
Copy link
Collaborator

IlyaBondar commented Aug 26, 2024

/deploy-review

Environment URL: https://chat-ai-dial-chat-pr-1950.nightly-test.deltixhub.io
E2E tests status: success

@sergesha sergesha force-pushed the feature/refactor-chat-component branch from bf61c6b to 985b8b0 Compare August 27, 2024 13:24
@sergesha sergesha force-pushed the feature/refactor-chat-component branch from 0f1ad97 to 3d5dcf9 Compare August 28, 2024 07:52
@IlyaBondar
Copy link
Collaborator

IlyaBondar commented Sep 4, 2024

/deploy-review

package.json Outdated
@@ -168,5 +170,6 @@
"fs": false,
"net": false,
"tls": false
}
},
"packageManager": "yarn@1.22.19+sha1.4ba7fc5c6e704fce2066ecbfb0b0d8976fe62447"
Copy link
Collaborator

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';
Copy link
Collaborator

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';
Copy link
Collaborator

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';
Copy link
Collaborator

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';
Copy link
Collaborator

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';
Copy link
Collaborator

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';
Copy link
Collaborator

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';
Copy link
Collaborator

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';
Copy link
Collaborator

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';
Copy link
Collaborator

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';
Copy link
Collaborator

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)

@IlyaBondar
Copy link
Collaborator

IlyaBondar commented Sep 11, 2024

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.
If we need new property on lowest level we need to drill it down from all levels between top and lowest level. I.e. will be problem with supporting and maintaining this code.

At this moment the goal to make pure Chat-component wasn't achieved because a lot of child components are using state directly (useAppSelector, useAppDispatch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants