AI Chat: refactor redux file #63775
AI Chat: refactor redux file #63775
Conversation
There was a problem hiding this comment.
Very cool refactor - I think it significantly improves readability! I can't believe aichatRedux is currently almost 1K in lines - I remember when it was a baby - RIP. 😁
| @@ -25,7 +25,9 @@ const ChatEventsList: React.FunctionComponent<ChatEventsListProps> = ({ | |||
| }) => { | |||
| const [inProgrammaticScroll, setInProgrammaticScroll] = useState(false); | |||
| const [showScrollToBottom, setShowScrollToBottom] = useState(true); | |||
| const {isWaitingForChatResponse} = useAppSelector(state => state.aichat); | |||
| const isWaitingForChatResponse = useAppSelector( | |||
| state => !!state.aichat.chatMessagePending | |||
| // This thunk is used when a student fills out a model card and "publishes" their model, | ||
| // enabling access to a "presentation view" where they can interact with their model | ||
| // and view its details (temperature, system prompt, etc) in a summary view. | ||
| export const publishModelCard = createAsyncThunk( |
There was a problem hiding this comment.
Notice that this used to be called publishModel but think it makes sense to call it publishModelCard.
There was a problem hiding this comment.
Yep good catch! Renamed for clarity (forgot to include in description)
apps/src/aichat/redux/slice.ts
Outdated
| }; | ||
| type ModalType = ModalTypes.WARNING | ModalTypes.TEACHER_ONBOARDING | undefined; | ||
|
|
||
| export interface AichatState { |
There was a problem hiding this comment.
Because of the size of this file and its name, I think it may make sense to storing state in a separate file - but not a strong feeling!
There was a problem hiding this comment.
Makes sense! I may actually pull this and other local type (ModalType, etc) into a separate types file?
There was a problem hiding this comment.
Updated! Actually ended up slimming this down a bit more
- Got rid of the
MessageLocationtype since it can be inferred from the return type ofgetUpdateMessageLocation - Got rid of
ModalTypesince it's equivalent toModalTypes | undefined - Which left only the
AichatStatetype. Moved this into a separate file calledstate.ts
This is an attempt to break up our nearly 900-line redux file into smaller, targeted files, grouped by type (selectors, thunks). We still export a single
index.tsfile in theaichat/reduxdirectory so other components can still just import fromaichat/reduxdirectly.Details:
redux/thunksfolder, with a separate file per thunk. Some of the thunks are quite large, so this helps organize things (even if others are pretty small)redux/thunks/helperssubdirectoryredux/selectors/index.tsfile. I opted to keep these in one file since they're individually pretty small.aichatReduxis just the state schema and slice definition, so I renamed itslice.ts. It's still not that small, but is focused on just defining the state and actions. I debated pulling outAichatStateinto a separate file too but kept it there for now. Happy to reconsider though.redux/index.tsfile.Like #63710 this is mostly copy+paste only, but there were a few minor changes needed to get everything working correctly:
aichat/reduxinstead ofaichat/redux/aichatReduxwaitingForResponse: booleanstate value, and instead just compute this based on the presence of achatMessagePending. This was primarily so I could get rid of theextraReducersin the slice and avoid a circular dependency between the thunk file and the slice file. We setwaitingForResponsetotrueonly when we're in the process of sending a chat message, which is also only whenchatMessagePendingis defined, so I felt this made sense.Testing story
Tested locally. No functional/behavioral changes.
Follow-up work
I'd like to clean up the
utils.tsfile too, but I'll leave this for a separate change that tackles all the various util files together - there's a bunch of util files spread out across these three files that all kind of get used in different places.