Fix: [Section] Scalable Application Structure - #323
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive guide on building a scalable, feature-based architecture for React-Redux applications. The reviewer provided valuable feedback on the code examples within the documentation, pointing out public API violations where internal paths are imported directly, type-safety risks associated with conditional store injection, and a lack of code splitting for disabled features. Actionable suggestions were provided to resolve these architectural issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Import reducers conditionally | ||
| import authReducer from '../features/auth/slices/authSlice'; | ||
| import dashboardReducer from '../features/dashboard/slices/dashboardSlice'; | ||
| import settingsReducer from '../features/settings/slices/settingsSlice'; |
There was a problem hiding this comment.
Public API Violation
Importing directly from internal paths like ../features/auth/slices/authSlice bypasses the feature's public API (index.ts). This violates the encapsulation and modularity goals described earlier in the guide.
To maintain strict boundaries, always import from the feature's root entry point.
| // Import reducers conditionally | |
| import authReducer from '../features/auth/slices/authSlice'; | |
| import dashboardReducer from '../features/dashboard/slices/dashboardSlice'; | |
| import settingsReducer from '../features/settings/slices/settingsSlice'; | |
| // Import reducers conditionally from the public API of each feature | |
| import { authReducer } from '../features/auth'; | |
| import { dashboardReducer } from '../features/dashboard'; | |
| import { settingsReducer } from '../features/settings'; |
| const reducer = { | ||
| ...(FEATURE_FLAGS.AUTH && { auth: authReducer }), | ||
| ...(FEATURE_FLAGS.DASHBOARD && { dashboard: dashboardReducer }), | ||
| ...(FEATURE_FLAGS.SETTINGS && { settings: settingsReducer }), | ||
| }; |
There was a problem hiding this comment.
Issues with Conditional Store Injection
Conditionally injecting reducers using object spreading can lead to runtime and type-safety issues:
- Type Safety: If
RootStateis inferred fromstore.getState(), the state slices for disabled features will be typed as optional or potentiallyundefined. If selectors are not written defensively (e.g., using optional chaining likestate.auth?.user), the application will crash at runtime when a disabled feature's selector is invoked. - Predictability: Redux store structure is generally expected to be static and predictable.
Recommended Alternatives:
- Static Reducers with UI-only Toggling: Keep all reducers registered in the store. This ensures type safety and predictable state structure, while feature flags only control UI rendering and action dispatching.
- Dynamic Reducer Injection: For large-scale applications where bundle size of reducers is a concern, use Redux Toolkit's dynamic reducer injection or a custom Reducer Manager to dynamically inject reducers when the feature is loaded.
| const reducer = { | |
| ...(FEATURE_FLAGS.AUTH && { auth: authReducer }), | |
| ...(FEATURE_FLAGS.DASHBOARD && { dashboard: dashboardReducer }), | |
| ...(FEATURE_FLAGS.SETTINGS && { settings: settingsReducer }), | |
| }; | |
| const reducer = { | |
| auth: authReducer, | |
| dashboard: dashboardReducer, | |
| settings: settingsReducer, | |
| }; |
| import { FEATURE_FLAGS } from '../config/featureFlags'; | ||
| import { AuthPage } from '../features/auth/components/AuthPage'; | ||
| import { Dashboard } from '../features/dashboard/components/Dashboard'; | ||
| import { Settings } from '../features/settings/components/Settings'; | ||
|
|
||
| function App() { | ||
| return ( | ||
| <div> | ||
| {FEATURE_FLAGS.AUTH && <AuthPage />} | ||
| {FEATURE_FLAGS.DASHBOARD && <Dashboard />} | ||
| {FEATURE_FLAGS.SETTINGS && <Settings />} | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Public API Violation & Lack of Code Splitting
There are two main architectural issues with this implementation:
- Public API Violation: Importing directly from internal paths like
../features/auth/components/AuthPagebypasses the feature's public API (index.ts). The entry components should be exported from the feature'sindex.tsand imported from the feature root. - Lack of Code Splitting: Statically importing all features means they will all be bundled together, even if their feature flags are disabled. Using
React.lazyandSuspenseensures that disabled features are not loaded by the client, which is crucial for a scalable application.
| import { FEATURE_FLAGS } from '../config/featureFlags'; | |
| import { AuthPage } from '../features/auth/components/AuthPage'; | |
| import { Dashboard } from '../features/dashboard/components/Dashboard'; | |
| import { Settings } from '../features/settings/components/Settings'; | |
| function App() { | |
| return ( | |
| <div> | |
| {FEATURE_FLAGS.AUTH && <AuthPage />} | |
| {FEATURE_FLAGS.DASHBOARD && <Dashboard />} | |
| {FEATURE_FLAGS.SETTINGS && <Settings />} | |
| </div> | |
| ); | |
| } | |
| import React, { Suspense, lazy } from 'react'; | |
| import { FEATURE_FLAGS } from '../config/featureFlags'; | |
| // Lazy load features from their public APIs to enable code-splitting | |
| const AuthPage = lazy(() => import('../features/auth').then(m => ({ default: m.AuthPage }))); | |
| const Dashboard = lazy(() => import('../features/dashboard').then(m => ({ default: m.Dashboard }))); | |
| const Settings = lazy(() => import('../features/settings').then(m => ({ default: m.Settings }))); | |
| function App() { | |
| return ( | |
| <Suspense fallback={<div>Loading...</div>}> | |
| <div> | |
| {FEATURE_FLAGS.AUTH && <AuthPage />} | |
| {FEATURE_FLAGS.DASHBOARD && <Dashboard />} | |
| {FEATURE_FLAGS.SETTINGS && <Settings />} | |
| </div> | |
| </Suspense> | |
| ); | |
| } |
|
Closing as duplicate. The latest fix is in PR #338. Apologies for the noise — improving the automation. |
Fixes #42
This PR addresses the IssueHunt-funded issue #42.