Refactor `ChatProvider` into a Redux store #826

fredrikekelund · 2025-01-21T14:05:35Z

Related issues

Fixes https://github.com/Automattic/dotcom-forge/issues/10269

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

Before applying this PR, make sure that you have some existing Assistant conversations
Quit Studio before checking out the PR
Check out this PR
npm install
Start Studio
Ensure that your previous Assistant conversations load as expected
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 · 2025-01-22T15:46:56Z

src/stores/index.ts

+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 ) );
+	},
+} );


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 🙂

nightnei

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

nightnei · 2025-01-28T18:57:10Z

src/stores/index.ts

+		getDefaultMiddleware().prepend( listenerMiddleware.middleware ),
+} );
+
+export default store;


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.

Sure, makes sense 👍

nightnei · 2025-01-28T19:03:58Z

src/stores/index.ts

+
+export default store;
+
+export type AppDispatch = typeof store.dispatch;


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.

Good idea 👍

nightnei · 2025-01-28T19:08:17Z

src/stores/index.ts

+import { CHAT_ID_STORE_KEY, CHAT_MESSAGES_STORE_KEY } from 'src/constants';
+import chatReducer from 'src/stores/chat-slice';
+
+export type RootState = {


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

nightnei · 2025-01-28T19:10:11Z

src/stores/chat-slice.ts

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


I would avoid default exports, as mentioned it above

nightnei · 2025-01-28T19:14:40Z

src/stores/index.ts

+
+const listenerMiddleware = createListenerMiddleware< RootState >();
+
+listenerMiddleware.startListening( {


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.

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.

Yeah, makes sense 👍

nightnei · 2025-01-28T20:48:18Z

src/stores/chat-slice.ts

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


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,

Returning initialState makes sense 👍 Less of a workaround

nightnei · 2025-01-28T20:56:36Z

src/stores/chat-slice.ts

+export const { updateFromTheme, setMessages, setChatInput, updateMessage, resetChatState } =
+	chatSlice.actions;
+
+export const { selectChatInput, selectMessages, selectChatApiId, selectIsLoading } =
+	chatSlice.selectors;


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.

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

nightnei · 2025-01-28T21:17:14Z

src/components/content-tab-assistant.tsx

-						handleKeyDown={ handleKeyDown }
+						input={ chatInput }
+						setInput={ ( input ) => {
+							dispatch( setChatInput( { siteId: selectedSite.id, input } ) );


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.

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.

nightnei · 2025-01-28T21:54:39Z

src/stores/chat-slice.ts

+			.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;
+			} )


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.

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.

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?

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.

nightnei · 2025-01-28T22:14:18Z

src/stores/chat-slice.ts

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


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.

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

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 👍

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, .......

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.

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.

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

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 }, });

Maybe we can use it for our case

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.

nightnei

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

fredrikekelund · 2025-01-29T13:26:43Z

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

katinthehatsite · 2025-01-29T15:41:52Z

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

sejas

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.

sejas · 2025-01-29T14:37:57Z

package.json

@@ -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",


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",

nightnei

Looks marvelous 🙂👍

fredrikekelund · 2025-01-30T09:52:54Z

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 · 2025-01-30T10:08:33Z

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 added 4 commits January 21, 2025 12:06

Allow non-relative imports starting with src/ or vendor/

60bea5e

Fix lint errors

30aac87

Small fix

2b0fd1a

Merge branch 'trunk' into f26d/absolute-imports

3c87ebe

fredrikekelund requested a review from a team January 21, 2025 14:05

fredrikekelund self-assigned this Jan 21, 2025

fredrikekelund added 4 commits January 21, 2025 15:05

WIP

ca8bc22

Tweaks

4dd8a3c

WIP

afcfb2f

WIP

a02cb28

fredrikekelund force-pushed the f26d/redux-chat-slice branch from 300e467 to a02cb28 Compare January 21, 2025 14:06

fredrikekelund mentioned this pull request Jan 21, 2025

Allow non-relative imports starting with src/ or vendor/ #825

Merged

1 task

Base automatically changed from f26d/absolute-imports to trunk January 22, 2025 09:05

fredrikekelund added 11 commits January 22, 2025 10:07

Merge branch 'trunk' into f26d/redux-chat-slice

f801f9a

WIP

b9ebf6d

Send feedback from Redux

a55bd4d

Completely remove useAssistant hook

b672d12

Small fixes

9b4bfe3

Optimize rendering

bcaea76

Use a site dict for plugin and theme lists

6076d79

Use middleware to save to localStorage

1bd6a3b

Store prompt usage

72437ce

Support import aliases in Jest

daeebd3

Fix test

57674c3

fredrikekelund commented Jan 22, 2025

View reviewed changes

fredrikekelund added 5 commits January 23, 2025 10:40

Fix some tests

eef5f8e

Store messages, chatId and loading state by instanceId instead of siteId

d4b9700

Wrap submitPrompt in useCallback

48bbe9b

Small fixes

f8a9949

Rename chatId to chatApiId

f19284e

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

fredrikekelund added 5 commits January 24, 2025 09:40

Redux reducer tests

4ce8938

More tests

029c158

Remove unused isSiteLoadedDict state

f1c1b19

Fix

0522a75

Merge branch 'trunk' into f26d/redux-chat-slice

4ef2eef

nightnei requested changes Jan 29, 2025

View reviewed changes

fredrikekelund added 3 commits January 29, 2025 14:12

Address review feedback

32c332f

Merge branch 'trunk' into f26d/redux-chat-slice

c8730bf

Export actions and selectors under predefined namespace

fb7edf2

fredrikekelund requested a review from nightnei January 29, 2025 13:26

fredrikekelund added 2 commits January 29, 2025 14:53

Reload values from localStorage when reading initial state

7bf88ab

Move the reset state action to a test util file

4e9d661

sejas reviewed Jan 29, 2025

View reviewed changes

nightnei approved these changes Jan 29, 2025

View reviewed changes

fredrikekelund added 6 commits January 30, 2025 09:17

Fix updateMessage action

3c853d6

Fix tests

f8b64a5

Update @reduxjs/toolkit to latest version

8e23de1

Fix warnings in content-tab-assistant.test.tsx

3de62b0

Test isRetry: true calls to fetchAssistantThunk

68714f0

Export thunks under chatThunks namespace

bee5f84

fredrikekelund merged commit c421b61 into trunk Jan 30, 2025
7 checks passed

fredrikekelund deleted the f26d/redux-chat-slice branch January 30, 2025 10:11

fredrikekelund mentioned this pull request Jan 31, 2025

Fix regression with login button in Assistant tab #869

Merged

1 task

fredrikekelund mentioned this pull request Feb 18, 2025

Clear chat API ID when clearing Assistant convo #949

Merged

1 task


		export default store;

		export type AppDispatch = typeof store.dispatch;


		const listenerMiddleware = createListenerMiddleware< RootState >();

		listenerMiddleware.startListening( {

Refactor ChatProvider into a Redux store #826

Refactor ChatProvider into a Redux store #826

Conversation

fredrikekelund commented Jan 21, 2025 • edited Loading

Related issues

Proposed Changes

Testing Instructions

Pre-merge Checklist

Choose a reason for hiding this comment

nightnei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightnei left a comment

Choose a reason for hiding this comment

fredrikekelund commented Jan 29, 2025

katinthehatsite commented Jan 29, 2025 • edited Loading

sejas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightnei left a comment

Choose a reason for hiding this comment

fredrikekelund commented Jan 30, 2025

nightnei commented Jan 30, 2025

Refactor `ChatProvider` into a Redux store #826

Refactor `ChatProvider` into a Redux store #826

fredrikekelund commented Jan 21, 2025 •

edited

Loading

katinthehatsite commented Jan 29, 2025 •

edited

Loading