Studio: Refactor AuthProvider into Redux slice#2778
Conversation
fredrikekelund
left a comment
There was a problem hiding this comment.
Thanks for tackling this, @katinthehatsite.
The end goal with this refactor should be to remove AuthProvider altogether. That, and doing something about setWpcomClient from src/stores/wpcom-api are the two big changes needed here, IMO.
There was a problem hiding this comment.
We should remove this context provider altogether. Components can easily consume Redux state and dispatch actions. There's no point in keeping this provider as a thin wrapper around the Redux slice.
There was a problem hiding this comment.
Should be done ✅
| export const selectIsAuthenticated = ( state: RootState ) => state.auth.isAuthenticated; | ||
| export const selectUser = ( state: RootState ) => state.auth.user ?? undefined; |
There was a problem hiding this comment.
| export const selectIsAuthenticated = ( state: RootState ) => state.auth.isAuthenticated; | |
| export const selectUser = ( state: RootState ) => state.auth.user ?? undefined; | |
| export const authThunks = { | |
| handleInvalidToken, | |
| initializeAuth, | |
| authTokenReceived, | |
| authLogout, | |
| }; | |
| export const authSelectors = { | |
| selectIsAuthenticated: ( state: RootState ) => state.auth.isAuthenticated, | |
| selectUser: ( state: RootState ) => state.auth.user ?? undefined, | |
| }; |
We've followed this pattern in some other Redux slices. I think it's helpful to namespace selectors, actions, and thunks this way.
There was a problem hiding this comment.
This isn't exactly a blocker, but I still think it's good practice.
…r-auth-provider-in-redux-slice
📊 Performance Test ResultsComparing f2c6f44 vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
Port removal of noisy Sentry.captureException calls from auth-provider.tsx to auth-slice.ts (handleInvalidToken, authLogout token revoke, authLogout clearAuthenticationToken), keeping the intentional deletion of auth-provider.tsx. Made-with: Cursor
…r-auth-provider-in-redux-slice
|
I think all the comments have now been addressed so I am requesting another review on this. 🙇 |
…r-auth-provider-in-redux-slice
fredrikekelund
left a comment
There was a problem hiding this comment.
Sorry for the delayed review, @katinthehatsite. This is looking pretty good overall! 👍
The biggest remaining question to me is use-auth.ts, which I argue we should also remove. Beyond that, I left a couple of comments about plumbing/wiring and raised a few comments from my previous review that you didn't address or push back on.
There was a problem hiding this comment.
Same as with apps/studio/src/components/auth-provider.tsx, I argue we should remove this file. Components can easily consume the selectors used in this hook directly, and dispatch the authLogout action directly, too.
There was a problem hiding this comment.
I went ahead and removed it 👍
| export const selectIsAuthenticated = ( state: RootState ) => state.auth.isAuthenticated; | ||
| export const selectUser = ( state: RootState ) => state.auth.user ?? undefined; |
There was a problem hiding this comment.
This isn't exactly a blocker, but I still think it's good practice.
There was a problem hiding this comment.
Could we make the wpcomClient part of the auth slice? Either part of the state or keep using the current logic, but export getWpcomClient. This is what I was getting at in #2778 (comment), too
|
Thanks so much for the review @fredrikekelund 🙇 I am planning to take a closer look today or tomorrow and address the rest of the comments 😃 |
…r-auth-provider-in-redux-slice
…r-auth-provider-in-redux-slice
|
I think this should be ready for another review @fredrikekelund |
|
@katinthehatsite should we merge trunk in and solve conflicts? |
|
@katinthehatsite, given that we are cutting over to the agentic UI soon and that landing this would still require a non-trivial amount of work, I suggest abandoning this PR and closing the related issue. |
This sounds good to me 👍 I will proceed with closing |
Related issues
Fixes https://linear.app/a8c/issue/STU-1379/studio-refactor-authprovider-into-redux-slice
How AI was used in this PR
I used it in the plan mode to plan the refactor, then gave feedback on the plan and used it for implementation, making further manual corrections.
Proposed Changes
This PR refactors
AuthProviderinto Redux slice.Testing Instructions
npm start~/Library/Application Support/Studio/appdata-v1.jsonauthTokenPre-merge Checklist