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

Refactor ChatProvider into a Redux store #826

Merged
merged 45 commits into from
Jan 30, 2025
Merged

Conversation

fredrikekelund
Copy link
Contributor

@fredrikekelund fredrikekelund commented Jan 21, 2025

Related issues

Proposed Changes

Tip

Feel free to ping me when reviewing this PR. I know the diff is large, and I'm happy to pair as needed.

We recently decided (see https://github.com/Automattic/dotcom-forge/issues/10209) to add Redux Toolkit to the Studio codebase. As part of the decision process, we looked at a mock re-implementation of ChatProvider as a Redux store. Following up on that work, this PR implements an actual refactoring of ChatProvider as a Redux store.

I originally thought we could keep this PR smaller, but once I started pulling the threads, I realized that it didn't make sense to keep the useAssistant, useAssistantApi, and useSendFeedback hooks. So, that functionality now also lives in src/stores/chat-slice.ts.

Aside from better code organization and easier-to-reason-about primitives, the fact that we can subscribe to state changes with Redux and persist state as needed is a big win.

This Redux implementation should hopefully serve as a reference for future Redux implementations (fresh ones and refactorings).

Testing Instructions

  1. Before applying this PR, make sure that you have some existing Assistant conversations
  2. Quit Studio before checking out the PR
  3. Check out this PR
  4. npm install
  5. Start Studio
  6. Ensure that your previous Assistant conversations load as expected
  7. Do a full smoke test of the Assistant, including:
    7.1 Switching between sites
    7.2 Basic chatting
    7.3 Running WP CLI commands
    7.4 Sending feedback
    7.5 Explicitly referencing previous messages ("what did I say in my previous message?")
    7.6 Reload the main view to ensure your new messages are persisted
    7.7 Log out, make sure your conversations disappear
    7.8 Log back in, make sure your conversations are restored
    7.9 Make sure you can see your remaining message count in the app settings

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@fredrikekelund fredrikekelund requested a review from a team January 21, 2025 14:05
@fredrikekelund fredrikekelund self-assigned this Jan 21, 2025
Base automatically changed from f26d/absolute-imports to trunk January 22, 2025 09:05
Comment on lines 11 to 19
listenerMiddleware.startListening( {
predicate( action, currentState, previousState ) {
return currentState.chat.messagesDict !== previousState.chat.messagesDict;
},
effect( action, listenerApi ) {
const state = listenerApi.getState() as RootState;
localStorage.setItem( CHAT_MESSAGES_STORE_KEY, JSON.stringify( state.chat.messagesDict ) );
},
} );
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 want to highlight this piece in particular because it illustrates how we could use Redux logic to more effectively update appdata-v1.json, too.

We are subscribing to state changes here, with the predicate function using a plain old === equality check (that works because Redux data is immutable). In the effect callback, we can access the entire state tree and do what we want with it 🙂

@fredrikekelund fredrikekelund marked this pull request as ready for review January 23, 2025 16:22
Copy link
Contributor

@nightnei nightnei left a comment

Choose a reason for hiding this comment

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

Tested, works well 👍
And teh code in general is amazing 😍
Redux-toolkit much much better than clear Redux.

getDefaultMiddleware().prepend( listenerMiddleware.middleware ),
} );

export default store;
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about avoiding default exports?
I liked such an approach in Tumblr, and IMO, there were reasonable arguments about simplifying looking for import names and consistency with imported names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, makes sense 👍


export default store;

export type AppDispatch = typeof store.dispatch;
Copy link
Contributor

Choose a reason for hiding this comment

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

We already started copy-pasting and importing it everywhere. WDYT about creating useAppDispatch and use it, instead of useDispatch? I think code will be less verbose and clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea 👍

import { CHAT_ID_STORE_KEY, CHAT_MESSAGES_STORE_KEY } from 'src/constants';
import chatReducer from 'src/stores/chat-slice';

export type RootState = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking, what about creating useAppSelector, similar to what I mention here. Reaso and arguments for it are the same

https://github.com/Automattic/studio/pull/826/files#diff-a962836e7056c855240fcab71b4e4f6a56e8462e298844f0b928226d230cda83R41

export const { selectChatInput, selectMessages, selectChatApiId, selectIsLoading } =
chatSlice.selectors;

export default chatSlice.reducer;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid default exports, as mentioned it above


const listenerMiddleware = createListenerMiddleware< RootState >();

listenerMiddleware.startListening( {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not critical, more just thinking out loud - should we unsubscribe during logout?
I don't predict any issues with it and it's not about performance problems, since Studio is small. But maybe as an initial example, maybe would be nice to implement it.

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 think this would be easier to accomplish if the auth data was also stored in Redux. Given that it's not necessary, I would defer this until later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, makes sense 👍

Comment on lines 233 to 236
resetChatState: ( state ) => {
// Reassigning `initialState` to `state` doesn't work. Probably because of Immer.js.
Object.assign( state, initialState );
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean state = initialState? If so, it's because arguments don't work by reference, as in PHP with prefix &. So with assigning you just change the variable inside this function.

Anyay, I found more right approach https://stackoverflow.com/questions/59424523/reset-state-to-initial-with-redux-toolkit

Suggested change
resetChatState: ( state ) => {
// Reassigning `initialState` to `state` doesn't work. Probably because of Immer.js.
Object.assign( state, initialState );
},
resetChatState: ( state ) => initialState,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returning initialState makes sense 👍 Less of a workaround

Comment on lines 378 to 382
export const { updateFromTheme, setMessages, setChatInput, updateMessage, resetChatState } =
chatSlice.actions;

export const { selectChatInput, selectMessages, selectChatApiId, selectIsLoading } =
chatSlice.selectors;
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about exporting chatSelectors and chatActions, or something similar, but don't spread them to keep them behind some isolated space. It will be easier to read in code chatAction.setMessage instead of setMessage, since it looks not obvious that it's action and specifically from chat reducer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea – I like it 👍 Will make even more sense once we have more stores.

handleKeyDown={ handleKeyDown }
input={ chatInput }
setInput={ ( input ) => {
dispatch( setChatInput( { siteId: selectedSite.id, input } ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking and:

  1. I don't see benefits to store temporary value in global store. We can achieve the same with storing in inside this component's state.
  2. Every keydown triggers a lot of redundant store checks, even every prodicate inside listenerMiddleware.startListening is triggered.

Maybe I am missing something, but so far it looks like overkill.
WDYT?

UPDATE: I see, we store values for different chats... sad that we trigger store so many times during typing, but looks like it's optimal option. Unless, we consider saving it in localStorage, it's typical UX approach, to avoid losing data in case of some errors or reloads of window. But it's more as an improvement. So I think it's better to leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Storing this state in the Redux store helps maintain the bug fixes introduced in #788. Sounds like we're on the same page already, but to respond to your points:

I don't see benefits to store temporary value in global store. We can achieve the same with storing in inside this component's state.

The component state is reset when navigating between tabs, and will soon (see https://github.com/Automattic/dotcom-forge/issues/10219) be reset when navigating between sites, too.

Every keydown triggers a lot of redundant store checks, even every prodicate inside listenerMiddleware.startListening is triggered.

The middleware predicates are very cheap. Worth keeping in mind.

Comment on lines 287 to 293
.addCase( updateFromSiteThunk.pending, ( state, action ) => {
const { site } = action.meta.arg;

state.currentURL = `http://localhost:${ site.port }`;
state.phpVersion = site.phpVersion ?? DEFAULT_PHP_VERSION;
state.siteName = site.name;
} )
Copy link
Contributor

Choose a reason for hiding this comment

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

I was looking at code and looks like there is no sense to create separate case for pending, we can keep everything in one place fulfilled. We have there access to meta too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if it takes a very short time to resolve updateFromSiteThunk, it still technically makes sense to store data that's immediately available before resolving the promise. If we moved these assignments, we'd also have to duplicate them in the rejected handler.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we moved these assignments, we'd also have to duplicate them in the rejected handler.

But we don't have rejected handler for this case.
And, do we use currentURL etc w/o fulfilling of updateFromSiteThunk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we don't have rejected handler for this case.

Then we'd need to add one. Duplicating the logic (or abstracting it) is not very appealing.

And, do we use currentURL etc w/o fulfilling of updateFromSiteThunk?

Only if a user manages to send a message before updateFromSiteThunk is resolved, which admittedly is very unlikely.

reducers: {
resetChatState: ( state ) => {
// Reassigning `initialState` to `state` doesn't work. Probably because of Immer.js.
Object.assign( state, initialState );
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I am thinking - initial state stores initial values of messagesDict and chatApiIdDict. I mean they were read and saved during initial rendering of Studio. I didn't get completely what is chatApiIdDict but I have assumption that we potentially can have bug here with resetting localSotage to data which were during the initial render of Studio.
Just thoughts out loud, maybe I am wrong, since don't have the whole picture of how everything works yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, more thoughts - looks like you went another approach and we can remove this action:
https://github.com/Automattic/studio/pull/826/files#diff-d3d6432d7d681344c73a14e2e30339dbdd7fe784d4446baf42efaf59eed40b33R396-R398

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, resetChatState is really only used in unit tests (and that's also what it's intended for). I'll add a comment about that to clarify 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

The last one thing is I wanted gained to comeback this this specific message #826 (comment)

It's cool that we added comment, makes sense, but theoretically, somebody can use it in code in the future and I have assumption that it can be buggy.
I don't completely understood chatApiIdDict and why do we store a few chats in messagesDict , maybe it's the reason of my next concern, but I would like to discuss it:

For the initialState we retrieve data from localStorage during starting Studio:

const storedMessages = localStorage.getItem( CHAT_MESSAGES_STORE_KEY );
const storedChatIds = localStorage.getItem( CHAT_ID_STORE_KEY );

Then we store it inside initialState object. And these fields are never updated.
So, if somebody decided to use resetChatState, then it will errase all chat history to the initial state of Studio.

Maybe I wasn't enough clear, so I will provide the code, which I think will be selfexplanation of m concern:

const getInitialState = () => (

  const messagesDict = storedMessages ? JSON.parse( storedMessages ) : {};
  const chatApiIdDict = storedChatIds ? JSON.parse( storedChatIds ) : {};
  // !!! here messagesDict and chatApiIdDict should be somehow cleaned up to keep everything except the current chat. Not to the initial state which were during initial loading of Studio

  return {
	currentURL: '',
	pluginListDict: {},
    .......
	messagesDict,
	chatApiIdDict,
	.......

Copy link
Contributor

Choose a reason for hiding this comment

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

And in general, as for me, it's very bad practice to store something like this action in code, only for test purposes.

I propose either to find some way to patch store in test to add this method or at least to add __ prefix to this action and keep the comment of why it's here. In this case we will avoid using it in the future by mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Point taken! 👍 Given that resetChatState is indeed meant for tests mainly, I think it's better not to include any logic that modifies localStorage in this file. I did change it so that we re-read the stored messages and chat API IDs every time the state resets, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And in general, as for me, it's very bad practice to store something like this action in code, only for test purposes.

I get your point. Let me think on this and see if I can come up with a better approach

Copy link
Contributor

Choose a reason for hiding this comment

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

I found this:

  // Inject a new reducer inside the test
store.replaceReducer({
    chat: (state = initialState, action) => {
      if (action.type === 'test/resetChatState') {
        return initialState; // Reset state manually
      }
      return chatReducer(state, action); // Otherwise, use original reducer
    },
  });

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can use it for our case

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 moved the reset state action to a test util file per your suggestion in this thread, @nightnei. Great suggestion 👍 Much cleaner this way.

testReducer is also reusable now, which is an added benefit.

Copy link
Contributor

@nightnei nightnei left a comment

Choose a reason for hiding this comment

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

Tested, works well 👍
And teh code in general is amazing 😍
Redux-toolkit much much better than clear Redux.

@fredrikekelund
Copy link
Contributor Author

Thanks for the review, @nightnei 🙏 I've addressed all your feedback

@katinthehatsite
Copy link
Contributor

katinthehatsite commented Jan 29, 2025

So far the changes look good to me and everything persists when I switch between tabs, sites etc. I noticed one small regression compared to trunk. This is how you can reproduce it:

  • Open assistant tab on a site
  • Ask it to give some CLI commands
  • Run a CLI command from the code block
  • Ensure that you see the outcome
  • Switch between tabs e.g to the Settings tab
  • Come back to the Assistant tab
  • Observe that you do not see the outcome of the command that you ran while the outcome should persist:
Screen.Recording.2025-01-29.at.4.43.48.PM.mov

Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

The Assistant behaviour looks good feature wise. The only issue is what @katinthehatsite mentioned about persisting the wp-cli output.
The rest looks good.

I took a quick look to the code, mostly to learn about react toolkit. Couldn't find any suggestion.
I've seen people creating a hook that returns the actions, like useChatActions to avoid repeating the useAppDispatch in every component but it's not a huge improvement.

package.json Outdated
@@ -108,6 +108,7 @@
"@php-wasm/node": "^0.9.44",
"@php-wasm/scopes": "^0.9.44",
"@php-wasm/universal": "^0.9.44",
"@reduxjs/toolkit": "^2.5.0",
Copy link
Member

Choose a reason for hiding this comment

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

A new version 2.5.1 was released 3 days ago. We could update it.

Suggested change
"@reduxjs/toolkit": "^2.5.0",
"@reduxjs/toolkit": "^2.5.1",

Copy link
Contributor

@nightnei nightnei left a comment

Choose a reason for hiding this comment

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

Looks marvelous 🙂👍

@fredrikekelund
Copy link
Contributor Author

Thanks for the reviews, everyone! 🙏 I've fixed the issues with persisting the output of WP CLI commands.

I've seen people creating a hook that returns the actions, like useChatActions to avoid repeating the useAppDispatch in every component

I changed the way the thunk actions are exported from chat-slice.ts so they all live in a chatThunks object. It's not exactly the same as your proposal, but it matches chatActions and chatSelectors more closely.

@nightnei
Copy link
Contributor

I changed the way the thunk actions are exported from chat-slice.ts so they all live in a chatThunks object. It's not exactly the same as your proposal, but it matches chatActions and chatSelectors more closely.

Very nice idea, looks a bit easier to work with it 👍

@fredrikekelund fredrikekelund merged commit c421b61 into trunk Jan 30, 2025
7 checks passed
@fredrikekelund fredrikekelund deleted the f26d/redux-chat-slice branch January 30, 2025 10:11
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.

4 participants