feat: Implement Redux for user authentication and integrate with Goog…#39
Conversation
WalkthroughThis PR introduces Redux state management to the application. It establishes a Redux store with user authentication state, integrates Redux and session synchronization into the provider tree, adds typed Redux hooks, and updates components to retrieve user data from the Redux store. New dependencies include Redux Toolkit, react-redux, and next-redux-wrapper. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
apps/web/store/types.ts (1)
1-5: Potential circular dependency in type definitions.The current setup creates an import cycle:
types.tsimportsstorefromindex.tsindex.tsimportsrootReducerfromroot-reducer.tsWhile TypeScript typically handles this, a cleaner pattern would be to derive
AppDispatchfrom theAppStoretype rather than thestoreinstance:🔎 Suggested refactor to eliminate circular import
import { rootReducer } from "./root-reducer"; -import { store } from "./index"; +import type { AppStore } from "./index"; export type RootState= ReturnType<typeof rootReducer> -export type AppDispatch = typeof store.dispatch; +export type AppDispatch = AppStore['dispatch'];This avoids importing the store instance and relies only on the type.
apps/web/app/components/nodes/GoogleSheetFormClient.tsx (1)
48-65: OAuth popup implementation looks good, but consider completion handling.The popup window creation and centering logic is well-implemented with appropriate fallbacks for blocked popups. However, there's no mechanism to detect when the OAuth flow completes and refresh the credentials list.
Consider adding:
- A message listener or polling mechanism to detect OAuth completion
- Automatic credential list refresh after successful authentication
- Window reference cleanup to avoid memory leaks
apps/web/store/slices/userSlice.ts (7)
1-1: Minor formatting: Add space after opening brace.For consistency with TypeScript conventions, add a space after the opening brace in the import statement.
🔎 Proposed fix
-import {createSlice, PayloadAction } from '@reduxjs/toolkit' +import { createSlice, PayloadAction } from '@reduxjs/toolkit'
3-3: Consider adding a loading state to AuthStatus.The current AuthStatus type only includes 'Authenticated' and 'Unauthenticated'. In real-world applications, authentication often involves asynchronous operations (checking tokens, validating sessions, etc.). Adding a 'Loading' or 'Pending' state would improve UX by allowing components to display loading indicators during authentication checks.
🔎 Proposed enhancement
-export type AuthStatus = 'Authenticated' | 'Unauthenticated' +export type AuthStatus = 'Authenticated' | 'Unauthenticated' | 'Loading'You would then initialize with:
const initialState: UserState = { userId: null, status: 'Loading' // or 'Unauthenticated' based on your needs };
4-7: Rename interface to UserState for clarity.The interface is named
UserSlice, but it represents the state shape, not the slice itself. Renaming it toUserStatewould better convey its purpose and align with Redux conventions.🔎 Proposed refactor
-export interface UserSlice { +export interface UserState { userId: string | null; status: AuthStatus }And update the usage:
-const initialState: UserSlice = { +const initialState: UserState = { userId: null, status: 'Unauthenticated' };
17-23: Consider atomic user state updates.Having separate
setUserIdandsetUserStatusactions could lead to inconsistent state. For example, ifuserIdis set to a value butstatusremains 'Unauthenticated' (or vice versa), the state becomes contradictory. Consider adding a single action that updates both fields atomically.🔎 Proposed enhancement
reducers: { + setUser(state, action: PayloadAction<{ userId: string; status: AuthStatus }>){ + state.userId = action.payload.userId; + state.status = action.payload.status; + }, setUserId(state, action: PayloadAction<string | null>){ state.userId = action.payload; },Then dispatch atomically:
dispatch(userActions.setUser({ userId: 'user123', status: 'Authenticated' }));You can keep the individual setters for edge cases where you need to update fields separately.
24-26: Add state parameter to clearUser for consistency.The
clearUserreducer doesn't accept astateparameter, which is inconsistent with the other reducers. While Redux Toolkit allows this, adding the parameter makes the function signature consistent and clearer.🔎 Proposed refactor
- clearUser(){ + clearUser(state){ return initialState; }Alternatively, you can use the void action pattern:
- clearUser(){ - return initialState; + clearUser(state, action: PayloadAction<void>){ + return initialState; }
17-20: Minor formatting: Fix spacing inconsistencies.There are minor formatting issues with spacing that should be addressed for consistency.
🔎 Proposed fixes
- reducers:{ - setUserId(state, action: PayloadAction<string |null>){ + reducers: { + setUserId(state, action: PayloadAction<string | null>){ state.userId = action.payload; },
30-31: Use plural naming for actions export.The export is named
userActionbut it contains multiple actions. Following Redux conventions, it should beuserActions(plural) to indicate it's a collection of action creators.🔎 Proposed refactor
export const userReducer = userSlice.reducer -export const userAction = userSlice.actions +export const userActions = userSlice.actionsThis would make the usage more intuitive:
import { userActions } from './store/slices/userSlice'; dispatch(userActions.setUserId('user123'));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/web/app/components/nodes/GoogleSheetFormClient.tsx(4 hunks)apps/web/app/components/providers.tsx(1 hunks)apps/web/app/hooks/redux.ts(1 hunks)apps/web/app/layout.tsx(1 hunks)apps/web/package.json(2 hunks)apps/web/store/index.ts(1 hunks)apps/web/store/root-reducer.ts(1 hunks)apps/web/store/slices/userSlice.ts(1 hunks)apps/web/store/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
apps/web/store/types.ts (2)
apps/web/store/root-reducer.ts (1)
rootReducer(4-6)apps/web/store/index.ts (1)
store(9-9)
apps/web/store/root-reducer.ts (1)
apps/web/store/slices/userSlice.ts (1)
userReducer(30-30)
apps/web/app/components/providers.tsx (3)
apps/web/app/hooks/redux.ts (1)
useAppDispatch(6-6)apps/web/store/slices/userSlice.ts (1)
userAction(31-31)apps/web/store/index.ts (1)
store(9-9)
apps/web/app/hooks/redux.ts (1)
apps/web/store/types.ts (2)
AppDispatch(5-5)RootState(4-4)
apps/web/app/layout.tsx (1)
apps/web/app/components/providers.tsx (1)
Providers(31-49)
apps/web/store/index.ts (1)
apps/web/store/root-reducer.ts (1)
rootReducer(4-6)
🔇 Additional comments (5)
apps/web/app/layout.tsx (1)
4-4: LGTM! Clean provider integration.The wrapping of the application content with the
Providerscomponent is the correct approach for injecting Redux, session, and theme context into the App Router.Also applies to: 9-12
apps/web/store/root-reducer.ts (1)
1-6: LGTM! Standard Redux Toolkit root reducer.The root reducer setup follows Redux Toolkit best practices, correctly combining the user slice reducer.
apps/web/store/index.ts (1)
1-11: LGTM! Clean Redux Toolkit store configuration.The store setup follows Redux Toolkit best practices with:
- Factory function for testability
- Conditional devTools for production builds
- Proper type exports
apps/web/app/components/providers.tsx (1)
31-49: LGTM! Provider composition is correctly ordered.The provider nesting follows the correct hierarchy:
ReduxProvider(outermost) - makes store availableSessionProvider- provides auth sessionNextThemesProvider- provides theme contextSessionSync- can access both Redux dispatch and session hooksThis ordering ensures all hooks have access to their required contexts.
apps/web/app/hooks/redux.ts (1)
1-7: LGTM! Properly typed Redux hooks.This follows the recommended Redux Toolkit pattern for creating typed versions of
useDispatchanduseSelector, providing type safety throughout the application.
| import { handleSaveConfig } from './actions'; | ||
| import { useCredentials } from '@/app/hooks/useCredential'; | ||
| import { BACKEND_URL } from '@repo/common/zod'; | ||
| import { useAppSelector } from '@/app/hooks/redux'; |
There was a problem hiding this comment.
Unused userId prop creates confusion.
The component receives userId as a prop (line 21) but immediately overrides it by reading from the Redux store (line 38). This makes the prop declaration misleading and unused.
Consider one of these approaches:
- Remove the
userIdprop from the interface since it's not used - Use the prop as a fallback:
const userId = useAppSelector(s=>s.user.userId) || props.userId || "" - Rename the Redux-sourced value if both are needed
🔎 Option 1: Remove unused prop
Remove userId from the props interface at line 21:
interface GoogleSheetFormClientProps {
initialData: {
credentials: Array<{ id: string }>;
authUrl?: string;
hasCredentials: boolean;
};
- userId: string;
nodeId: string;
}Also applies to: 38-39
🤖 Prompt for AI Agents
In apps/web/app/components/nodes/GoogleSheetFormClient.tsx around lines 13, 21
and 38-39, the component declares a userId prop but then immediately overwrites
it by reading the same value from Redux, making the prop unused and confusing;
either remove userId from the props interface and from any call sites, or keep
the prop as a fallback by assigning: const userId = useAppSelector(s =>
s.user.userId) || props.userId || "" (or alternatively rename the Redux-sourced
variable if both values must be preserved); apply one of these fixes
consistently and update typings/usage accordingly.
| {authUrl ? ( | ||
| // <SelectItem value="create-new"> | ||
| <a href={authUrl} target="_blank" rel="noopener noreferrer" className="text-blue-500"> | ||
| <div onClick={()=>{openAuthWindow(authUrl)}} className="text-blue-500 cursor-default"> | ||
| + Create new credential | ||
| </a> | ||
| </div> | ||
| // </SelectItem> |
There was a problem hiding this comment.
Accessibility and UX issues with credential creation trigger.
Replacing the SelectItem with a <div> breaks several interaction patterns:
- Accessibility: No keyboard navigation or focus support
- Cursor styling:
cursor-defaultcontradicts clickable behavior (should becursor-pointer) - Select integration: The div is rendered inside
SelectContentbut doesn't follow Select component patterns
🔎 Recommended fix for accessible clickable option
- {authUrl ? (
- // <SelectItem value="create-new">
- <div onClick={()=>{openAuthWindow(authUrl)}} className="text-blue-500 cursor-default">
- + Create new credential
- </div>
- // </SelectItem>
- ) : (
+ {authUrl && (
+ <SelectItem
+ value="create-new"
+ onSelect={(e) => {
+ e.preventDefault();
+ openAuthWindow(authUrl);
+ }}
+ className="text-blue-500 cursor-pointer"
+ >
+ + Create new credential
+ </SelectItem>
+ )}
+ {!authUrl && (
<>
{
response?.map((cred: any) => (Alternatively, use a button outside the Select component for better semantics.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/app/components/nodes/GoogleSheetFormClient.tsx around lines 159 to
164, the replacement of SelectItem with a non-interactive <div> breaks
accessibility and Select integration; restore an accessible clickable option by
either: 1) replacing the <div> with the original SelectItem (or a component
compatible with your Select library) and wire its onSelect/onClick to
openAuthWindow(authUrl), ensure it is focusable and keyboard-activatable, and
set cursor to pointer; or 2) move the "Create new credential" control outside
the SelectContent and render it as a semantic <button> that calls
openAuthWindow(authUrl) (focusable, keyboard operable, and styled with
cursor-pointer). Ensure the chosen approach keeps Select semantics, keyboard
navigation, and proper styling.
| function SessionSync(){ | ||
| const { status, data } = useSession(); | ||
| const dispatch = useAppDispatch(); | ||
|
|
||
| React.useEffect(()=>{ | ||
| if(status === 'authenticated'){ | ||
| dispatch(userAction.setUserStatus('Authenticated')); | ||
| const id = (data?.user as any)?.id ?? null; | ||
| console.log("user from Session Sync ", id) | ||
| dispatch(userAction.setUserId(id)) | ||
| } | ||
| if(status === 'unauthenticated'){ | ||
| dispatch(userAction.setUserId(null)) | ||
| dispatch(userAction.setUserStatus('Unauthenticated')) | ||
| } | ||
| }, [status, data, dispatch]) | ||
|
|
||
| return null | ||
| } |
There was a problem hiding this comment.
SessionSync implementation has type safety and state handling gaps.
Two concerns:
-
Type assertion risk (line 18):
(data?.user as any)?.idbypasses type safety since by default, theidproperty does not exist on session. Proper typing requires custom NextAuth configuration via callbacks. -
Missing loading state (line 16-25): The
useEffectonly handles'authenticated'and'unauthenticated'states but ignores'loading'. Best practice is to check all three status states before syncing session data to Redux.
Recommended fix:
- Add a check for
status === 'loading'to prevent undefined behavior during initial session fetch - Configure NextAuth callbacks to properly expose the user id in the session object, then remove the type assertion
🤖 Prompt for AI Agents
In apps/web/app/components/providers.tsx around lines 11–29, SessionSync is
using a risky type assertion for the user id and ignores the 'loading' status;
update the code to early-return or skip dispatching when status === 'loading',
and stop using (data?.user as any)?.id by properly typing the session: add a
NextAuth session callback that injects user.id into the session, augment
NextAuth's Session/User types (or provide useSession generic) so session.user.id
is typed, then read session.user.id safely (nullable) and dispatch only when the
typed id is present or explicitly handle null.
| }, | ||
| "dependencies": { | ||
| "@radix-ui/react-dialog": "^1.1.15", | ||
| "@reduxjs/toolkit": "^2.11.2", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for usage of next-redux-wrapper
rg -n "next-redux-wrapper" --type=ts --type=tsx --type=js --type=jsx -iRepository: Dev-Pross/BuildFlow
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Find and read the providers.tsx file
fd "providers.tsx" -t fRepository: Dev-Pross/BuildFlow
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Search for next-redux-wrapper without specific type filter
rg "next-redux-wrapper" -iRepository: Dev-Pross/BuildFlow
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Read the providers.tsx file
cat -n apps/web/app/components/providers.tsxRepository: Dev-Pross/BuildFlow
Length of output: 1770
🏁 Script executed:
#!/bin/bash
# Check the package.json for Redux-related dependencies
cat apps/web/package.jsonRepository: Dev-Pross/BuildFlow
Length of output: 1525
Remove unused next-redux-wrapper dependency.
The next-redux-wrapper package is not imported or used anywhere in the codebase. The Redux setup in apps/web/app/components/providers.tsx correctly uses the App Router pattern with ReduxProvider directly from react-redux, which does not require a wrapper library. Remove next-redux-wrapper from apps/web/package.json line 32.
🤖 Prompt for AI Agents
In apps/web/package.json around line 32 (dependency list), remove the unused
"next-redux-wrapper" entry from the dependencies array; update the package.json
so the dependency key/value pair is deleted and then run npm/yarn install (or
update lockfile) to keep lockfiles in sync.
Vamsi-o
left a comment
Vamsi-o
left a comment
There was a problem hiding this comment.
Accessibility and UX issues with credential creation trigger.
Replacing the SelectItem with a
Accessibility: No keyboard navigation or focus support
Cursor styling: cursor-default contradicts clickable behavior (should be cursor-pointer)
Select integration: The div is rendered inside SelectContent but doesn't follow Select component patterns
3 participants
…le Sheets API
Summary by CodeRabbit
New Features
Infrastructure
✏️ Tip: You can customize this high-level summary in your review settings.