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

[AI Tutor] CT-441: Finish UI refactor #57889

Merged
merged 48 commits into from Apr 12, 2024
Merged

Conversation

ebeastlake
Copy link
Contributor

@ebeastlake ebeastlake commented Apr 9, 2024

This completes a refactor of the AI Tutor code that @Erin007 and @kakiha11 started.

Forgive me for the massive diff 🙈 I have tried to leave comments and screenshots of manually generated diffs where possible. Please feel free to leave specific feedback on things you feel could have been separated out despite the nature of the refactor and ask as many clarifying questions as needed!

In the following PR:

  • All of the AI Tutor UI is updated to align more closely with what we use for AI TA
  • We no longer track three separate tutors with a different UI for each; instead, there is one chat interface that uses three different types of questions under the hood
  • All of the files are moved to apps/src/aiTutor from being split between that folder and apps/src/code-studio/components
  • All React components are given PascalCase names instead of camelCase
  • Any code that AITutor was using from the aichat directory has been copied into the aiTutor directory, so the folks working on generative AI are free to do with aichat what they wish

Old UI:
Screenshot 2024-04-08 at 2 23 00 PM

New UI:
Screenshot 2024-04-10 at 11 01 45 PM

Links

Jira ticket: https://codedotorg.atlassian.net/browse/CT-441

Testing story

Since no one is using this until the closed beta, I tested that most things work manually.

This included each of the three buttons (submit, "Why doesn't my code compile?", "Why aren't my tests passing?") posting correctly to Open AI, receiving responses, and persisting the chat history to the database. I also tested that scrolling still works and that the view is appropriately locked down when the tutor is disabled.

I tested some of these things before I got the CSS on the buttons on the bottom correct, so ignore any wonky CSS in the footer (the image above is the source of truth for the footer UI).

Compilation working:
Screenshot 2024-04-10 at 9 28 26 PM

Validation working:
Screenshot 2024-04-10 at 9 29 38 PM

Scrolling still working:
Screenshot 2024-04-10 at 10 12 21 PM

Locked out view:
Screenshot 2024-04-10 at 10 42 08 PM

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

};
thunkAPI.dispatch(addChatMessage(newMessage));
}
const newMessage: ChatCompletionMessage = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, every scenario (general chat/compilation/validation) adds its "question" (the inputted question/"Why doesn't my code compile?"/"Why aren't my tests passing?") to state.

studentInput: string;
tutorType: AITutorTypesValue | undefined;
studentCode: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the student code to the ChatContext because I'll now need to include it in the formattedQuestion along with the studentInput, since I'm not using three different system prompts anymore.

import {useAppSelector} from '@cdo/apps/util/reduxHooks';
import Message from './Message';

const AITutorChatWorkspace: React.FunctionComponent = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the existing logic in this component changed. I just removed unused logic and unnecessary divs/styling. I renamed these files and restructured them so that the organization of AITutorContainer, AITutorChatWorkspace, and AITutorFooter matches the structure of RubricContainer, RubricContent, and RubricSubmitFooter (the AI TA components). This allowed me to snag their most recent styling :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -0,0 +1,79 @@
import classnames from 'classnames';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-04-10 at 11 13 52 PM Screenshot 2024-04-10 at 11 14 04 PM Screenshot 2024-04-10 at 11 14 15 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component changed significantly to accommodate the new organization/UI. I also made it draggable because it was helpful for testing. I've pasted the diff between this and the old equivalent, which was aiTutorPanel: https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/code-studio/components/aiTutor/aiTutorPanel.tsx

@@ -39,7 +39,7 @@ const AITutorFloatingActionButton: React.FunctionComponent = () => {
type="button"
/>
<Provider store={store}>
<AITutorPanel open={isOpen} />
<AITutorContainer open={isOpen} closeTutor={handleClick} />
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing this handler enables a close with an "X" in the upper right-hand corner.

state => state.aiTutor.isWaitingForChatResponse
);

const level = useAppSelector(state => state.aiTutor.level);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this logic isn't new, and is borrowed from a previous component called userChatMessageEditor: https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/code-studio/components/aiTutor/userChatMessageEditor.tsx

Here's the diff...
Screenshot 2024-04-10 at 11 20 17 PM
Screenshot 2024-04-10 at 11 20 30 PM
Screenshot 2024-04-10 at 11 20 45 PM
Screenshot 2024-04-10 at 11 21 00 PM
Screenshot 2024-04-10 at 11 21 14 PM
Screenshot 2024-04-10 at 11 21 28 PM

@@ -76,6 +79,9 @@ const ChatMessage: React.FunctionComponent<ChatMessageProps> = ({message}) => {

{isAssistant(message.role) && (
<div className={style.assistantMessageContainer}>
<Typography semanticTag="h5" visualAppearance="heading-xs">
AI Tutor ({message.role})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This header distinguishes the AI Tutor chats without a big circular image taking up horizontal bandwidth like we had before.

Screenshot 2024-04-10 at 11 24 35 PM

@import 'font';
@import '@cdo/apps/componentLibrary/common/styles/mixins';

.aiTutorContainer {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these CSS changes add the updated AI TA-like UI. I duplicated everything to guarantee we wouldn't break anything with AI TA.

import {StudentAccessData} from '@cdo/apps/aiTutor/types';

/**
* Renders toggles to control student access to AI Tutor.
*/

interface AITutorTeacherControlsProps {
interface AccessControlsProps {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially started renaming things because I was having trouble with git not detecting case-only changes in filenames when I moved from camelCase to PascalCase on these components (I had to modify my git config and using git mv to get it to pick all of them up). But I left the simpler names in place. Now that these are all scoped to the src/aiTutor directory, it doesn't feel like they all need to be prepended with AITutor.

@@ -1,4 +1,3 @@
export const CHAT_COMPLETION_URL = '/openai/chat_completion';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This endpoint is only used in the chatApi.ts file that aichat folks say they don't want in their directory anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything now lives in apps/src/aiTutor. Navigating back and forth between apps/src/aiTutor and apps/src/code-studio/components/aiTutor was tough, especially when apps/src/aichat is very similar to apps/src/aiTutor and I was often accidentally opening the wrong copy of a file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from apps/src/code-studio/components/aiTutor/chat-workspace.module.scss.

@ebeastlake ebeastlake marked this pull request as ready for review April 11, 2024 06:41
@@ -10,7 +10,7 @@ import {
AI_CUSTOMIZATIONS_LABELS,
} from '../views/modelCustomization/constants';
import {initialChatMessages} from '../constants';
import {getChatCompletionMessage} from '../chatApi';
import {getChatCompletionMessage} from '../../aiTutor/chatApi';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Emily - will you be calling on getChatCompletionMessage from this file? I see you have it in aiTutorRedux.ts. FWIW - I'm currently working on a PR that is updating this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, Alice! No, AI Tutor doesn't need this. Your PR will be good to update it after this one.

Copy link
Contributor

@molly-moen molly-moen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem reasonable to me!

@bencodeorg
Copy link
Contributor

changes within aichat seem fine to me!

@ebeastlake ebeastlake merged commit 6c590a1 into staging Apr 12, 2024
2 checks passed
@ebeastlake ebeastlake deleted the emily/ct-441/finish-ui-refactor branch April 12, 2024 03:49
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.

None yet

5 participants