-
Notifications
You must be signed in to change notification settings - Fork 29
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
Conversation
300e467
to
a02cb28
Compare
src/stores/index.ts
Outdated
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 ) ); | ||
}, | ||
} ); |
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 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 🙂
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.
Tested, works well 👍
And teh code in general is amazing 😍
Redux-toolkit much much better than clear Redux.
src/stores/index.ts
Outdated
getDefaultMiddleware().prepend( listenerMiddleware.middleware ), | ||
} ); | ||
|
||
export default store; |
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.
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.
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.
Sure, makes sense 👍
|
||
export default store; | ||
|
||
export type AppDispatch = typeof store.dispatch; |
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.
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.
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.
Good idea 👍
import { CHAT_ID_STORE_KEY, CHAT_MESSAGES_STORE_KEY } from 'src/constants'; | ||
import chatReducer from 'src/stores/chat-slice'; | ||
|
||
export type RootState = { |
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 am thinking, what about creating useAppSelector
, similar to what I mention here. Reaso and arguments for it are the same
src/stores/chat-slice.ts
Outdated
export const { selectChatInput, selectMessages, selectChatApiId, selectIsLoading } = | ||
chatSlice.selectors; | ||
|
||
export default chatSlice.reducer; |
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 would avoid default exports, as mentioned it above
|
||
const listenerMiddleware = createListenerMiddleware< RootState >(); | ||
|
||
listenerMiddleware.startListening( { |
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.
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.
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 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.
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.
Yeah, makes sense 👍
src/stores/chat-slice.ts
Outdated
resetChatState: ( state ) => { | ||
// Reassigning `initialState` to `state` doesn't work. Probably because of Immer.js. | ||
Object.assign( state, initialState ); | ||
}, |
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.
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
resetChatState: ( state ) => { | |
// Reassigning `initialState` to `state` doesn't work. Probably because of Immer.js. | |
Object.assign( state, initialState ); | |
}, | |
resetChatState: ( state ) => initialState, |
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.
Returning initialState
makes sense 👍 Less of a workaround
src/stores/chat-slice.ts
Outdated
export const { updateFromTheme, setMessages, setChatInput, updateMessage, resetChatState } = | ||
chatSlice.actions; | ||
|
||
export const { selectChatInput, selectMessages, selectChatApiId, selectIsLoading } = | ||
chatSlice.selectors; |
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.
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.
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.
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 } ) ); |
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 was thinking and:
- 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.
- 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.
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.
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.
src/stores/chat-slice.ts
Outdated
.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; | ||
} ) |
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 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.
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.
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.
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.
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?
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.
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.
src/stores/chat-slice.ts
Outdated
reducers: { | ||
resetChatState: ( state ) => { | ||
// Reassigning `initialState` to `state` doesn't work. Probably because of Immer.js. | ||
Object.assign( state, initialState ); |
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.
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.
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.
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
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.
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 👍
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.
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,
.......
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.
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.
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.
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.
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.
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
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 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
},
});
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.
Maybe we can use it for our case
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 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.
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.
Tested, works well 👍
And teh code in general is amazing 😍
Redux-toolkit much much better than clear Redux.
Thanks for the review, @nightnei 🙏 I've addressed all your feedback |
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:
Screen.Recording.2025-01-29.at.4.43.48.PM.mov |
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.
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", |
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.
A new version 2.5.1 was released 3 days ago. We could update it.
"@reduxjs/toolkit": "^2.5.0", | |
"@reduxjs/toolkit": "^2.5.1", |
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.
Looks marvelous 🙂👍
Thanks for the reviews, everyone! 🙏 I've fixed the issues with persisting the output of WP CLI commands.
I changed the way the thunk actions are exported from |
Very nice idea, looks a bit easier to work with it 👍 |
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 ofChatProvider
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
, anduseSendFeedback
hooks. So, that functionality now also lives insrc/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
npm install
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