refactor: replace whole-slice selectors with granular field selectors#5
Open
ashkanrdn wants to merge 1 commit intorefactor/redux-cleanupfrom
Open
refactor: replace whole-slice selectors with granular field selectors#5ashkanrdn wants to merge 1 commit intorefactor/redux-cleanupfrom
ashkanrdn wants to merge 1 commit intorefactor/redux-cleanupfrom
Conversation
- Add lib/features/filters/selectors.ts with individual selectors for every FilterState field (prevents re-renders on unrelated state changes) - MapStory: replace useSelector(state => state.filters) destructure with 8 granular selectors; remove unused RootState import - FilterSidebar: replace whole-slice destructure with 9 granular selectors - MetricsCards: replace whole-slice destructure with 3 granular selectors - CountyRank: migrate to selectSelectedMetric, selectIsPerCapita, selectSelectedCounty (removes last inline state.filters / state.map accesses) Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideRefactors Redux state access to use new granular, typed selectors for the filters slice (and existing map selectors), reducing unnecessary re-renders and centralizing selector logic in a dedicated module. Sequence diagram for granular selector-based re-renderingsequenceDiagram
actor User
participant FiltersSidebar
participant MetricsCards
participant MapStory
participant ReduxStore as Redux_Store
participant FiltersSlice
participant FiltersSelectors
User->>FiltersSidebar: Adjust filter control
FiltersSidebar->>ReduxStore: dispatch(setFilterAction)
ReduxStore->>FiltersSlice: Apply setFilterAction
FiltersSlice-->>ReduxStore: Update filteredData and activeFilters
ReduxStore->>FiltersSelectors: selectFilteredData(state)
FiltersSelectors-->>MapStory: filteredData
ReduxStore->>FiltersSelectors: selectActiveFilters(state)
FiltersSelectors-->>FiltersSidebar: activeFilters
ReduxStore->>FiltersSelectors: selectCsvDataSources(state)
FiltersSelectors-->>MetricsCards: csvDataSources
MapStory->>MapStory: Re-render because filteredData changed
FiltersSidebar->>FiltersSidebar: Re-render because activeFilters changed
MetricsCards->>MetricsCards: Re-render only if csvDataSources changed
Note over MapStory,MetricsCards: Each component re-renders only for the specific fields it selects instead of the entire filters slice
Class diagram for RootState, filters selectors, and consumersclassDiagram
class RootState {
FiltersState filters
MapState map
}
class FiltersState {
any filteredData
any activeFilters
string selectedMetric
boolean isPerCapita
string selectedDataSource
any selectedCounties
any yearRange
any csvDataSources
any filters
any dataSourcesStatus
any dataSourcesErrors
}
class MapState {
any selectedCounty
any rankedCounties
}
class FiltersSelectors {
+selectFilteredData(state RootState) FiltersState.filteredData
+selectActiveFilters(state RootState) FiltersState.activeFilters
+selectSelectedMetric(state RootState) FiltersState.selectedMetric
+selectIsPerCapita(state RootState) FiltersState.isPerCapita
+selectSelectedDataSource(state RootState) FiltersState.selectedDataSource
+selectSelectedCounties(state RootState) FiltersState.selectedCounties
+selectYearRange(state RootState) FiltersState.yearRange
+selectCsvDataSources(state RootState) FiltersState.csvDataSources
+selectFilters(state RootState) FiltersState.filters
+selectDataSourcesStatus(state RootState) FiltersState.dataSourcesStatus
+selectDataSourcesErrors(state RootState) FiltersState.dataSourcesErrors
}
class MapSelectors {
+selectRankedCounties(state RootState) MapState.rankedCounties
+selectSelectedCounty(state RootState) MapState.selectedCounty
}
class MapStory {
+useSelector(selectFilteredData)
+useSelector(selectSelectedMetric)
+useSelector(selectSelectedDataSource)
+useSelector(selectIsPerCapita)
+useSelector(selectSelectedCounties)
+useSelector(selectCsvDataSources)
+useSelector(selectActiveFilters)
+useSelector(selectSelectedCounty)
}
class FiltersSidebar {
+useSelector(selectFilters)
+useSelector(selectActiveFilters)
+useSelector(selectCsvDataSources)
+useSelector(selectDataSourcesStatus)
+useSelector(selectDataSourcesErrors)
+useSelector(selectFilteredData)
+useSelector(selectSelectedDataSource)
+useSelector(selectYearRange)
+useSelector(selectSelectedCounties)
}
class MetricsCards {
+useSelector(selectCsvDataSources)
+useSelector(selectDataSourcesStatus)
+useSelector(selectActiveFilters)
+useSelector(selectSelectedCounty)
}
class CountyRank {
+useSelector(selectRankedCounties)
+useSelector(selectSelectedMetric)
+useSelector(selectIsPerCapita)
+useSelector(selectSelectedCounty)
}
RootState --> FiltersState
RootState --> MapState
FiltersSelectors --> RootState
MapSelectors --> RootState
MapStory ..> FiltersSelectors
MapStory ..> MapSelectors
FiltersSidebar ..> FiltersSelectors
MetricsCards ..> FiltersSelectors
MetricsCards ..> MapSelectors
CountyRank ..> FiltersSelectors
CountyRank ..> MapSelectors
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Since multiple components need
selectedCounties ?? [], consider baking the defaulting behavior intoselectSelectedCounties(e.g., return an empty array when undefined) so that callers don’t have to remember to coalesce. - You might want to add a
selectFiltersState = (state: RootState) => state.filtershelper and build the field selectors off that to avoid repeatingstate.filtersin every selector and keep things consistent if the slice key ever changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since multiple components need `selectedCounties ?? []`, consider baking the defaulting behavior into `selectSelectedCounties` (e.g., return an empty array when undefined) so that callers don’t have to remember to coalesce.
- You might want to add a `selectFiltersState = (state: RootState) => state.filters` helper and build the field selectors off that to avoid repeating `state.filters` in every selector and keep things consistent if the slice key ever changes.
## Individual Comments
### Comment 1
<location> `app/components/widgets/FilterSidebar.tsx:171` </location>
<code_context>
+ const filteredData = useSelector(selectFilteredData);
+ const selectedDataSource = useSelector(selectSelectedDataSource);
+ const yearRange = useSelector(selectYearRange);
+ const selectedCounties = useSelector(selectSelectedCounties) ?? [];
// Get current source's data, status, and error
</code_context>
<issue_to_address>
**suggestion:** The `?? []` defaulting for `selectedCounties` might be better centralized in the selector.
Since you’re defaulting `selectedCounties` to `[]` here, consider moving that fallback into `selectSelectedCounties` (e.g. `state.filters.selectedCounties ?? []`). This keeps the behavior consistent for all consumers and avoids repeating the same fallback in multiple components.
Suggested implementation:
```typescript
const selectedCounties = useSelector(selectSelectedCounties);
```
To fully implement your suggestion, the `selectSelectedCounties` selector should be updated to centralize the `[]` defaulting. In the selector definition file (for example, wherever `selectSelectedCounties` is defined, such as `app/store/selectors/filters.ts` or similar), update its implementation to return a non-null array:
<<<<<<< SEARCH
export const selectSelectedCounties = (state: RootState) => state.filters.selectedCounties;
=======
export const selectSelectedCounties = (state: RootState) =>
state.filters.selectedCounties ?? [];
>>>>>>> REPLACE
You may also want to ensure the selector’s TypeScript type reflects that it always returns an array (e.g. `string[]` instead of `string[] | null | undefined`), and adjust any other selectors/components that were manually defaulting `selectedCounties` so they now rely on the updated selector behavior.
</issue_to_address>
### Comment 2
<location> `app/components/widgets/CountyRank.tsx:13` </location>
<code_context>
import { formatMetricLabel } from '@/lib/utils/metricFormatters';
export default function CountyRank() {
const dispatch = useDispatch();
- const counties = useSelector(selectRankedCounties);
- const selectedMetric = useSelector((state: RootState) => state.filters.selectedMetric);
</code_context>
<issue_to_address>
**suggestion:** The untyped `useDispatch` here is inconsistent with other components that use a typed dispatch.
Components like `MapStory`, `FiltersSidebar`, and `MetricsCards` use `useDispatch<AppDispatch>()` for stronger typing. To keep type safety and consistency, please type this dispatch as well (or use a shared `useAppDispatch` hook if available).
Suggested implementation:
```typescript
import { formatMetricLabel } from '@/lib/utils/metricFormatters';
import type { AppDispatch } from '@/lib/store';
```
```typescript
export default function CountyRank() {
const dispatch = useDispatch<AppDispatch>();
```
1. Ensure the `AppDispatch` import path matches the rest of the codebase. For example, if other components (e.g. `MapStory`, `FiltersSidebar`, `MetricsCards`) import it from a different module (such as `@/lib/redux/store` or similar), update the import path here to be identical.
2. If the project already has a `useAppDispatch` hook (e.g. in `@/lib/hooks`), you may prefer to:
- Import `useAppDispatch` instead of `useDispatch`.
- Replace `const dispatch = useDispatch<AppDispatch>();` with `const dispatch = useAppDispatch();`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const filteredData = useSelector(selectFilteredData); | ||
| const selectedDataSource = useSelector(selectSelectedDataSource); | ||
| const yearRange = useSelector(selectYearRange); | ||
| const selectedCounties = useSelector(selectSelectedCounties) ?? []; |
There was a problem hiding this comment.
suggestion: The ?? [] defaulting for selectedCounties might be better centralized in the selector.
Since you’re defaulting selectedCounties to [] here, consider moving that fallback into selectSelectedCounties (e.g. state.filters.selectedCounties ?? []). This keeps the behavior consistent for all consumers and avoids repeating the same fallback in multiple components.
Suggested implementation:
const selectedCounties = useSelector(selectSelectedCounties);To fully implement your suggestion, the selectSelectedCounties selector should be updated to centralize the [] defaulting. In the selector definition file (for example, wherever selectSelectedCounties is defined, such as app/store/selectors/filters.ts or similar), update its implementation to return a non-null array:
<<<<<<< SEARCH
export const selectSelectedCounties = (state: RootState) => state.filters.selectedCounties;
export const selectSelectedCounties = (state: RootState) =>
state.filters.selectedCounties ?? [];
REPLACE
You may also want to ensure the selector’s TypeScript type reflects that it always returns an array (e.g. string[] instead of string[] | null | undefined), and adjust any other selectors/components that were manually defaulting selectedCounties so they now rely on the updated selector behavior.
| import { formatMetricLabel } from '@/lib/utils/metricFormatters'; | ||
|
|
||
| export default function CountyRank() { | ||
| const dispatch = useDispatch(); |
There was a problem hiding this comment.
suggestion: The untyped useDispatch here is inconsistent with other components that use a typed dispatch.
Components like MapStory, FiltersSidebar, and MetricsCards use useDispatch<AppDispatch>() for stronger typing. To keep type safety and consistency, please type this dispatch as well (or use a shared useAppDispatch hook if available).
Suggested implementation:
import { formatMetricLabel } from '@/lib/utils/metricFormatters';
import type { AppDispatch } from '@/lib/store';export default function CountyRank() {
const dispatch = useDispatch<AppDispatch>();- Ensure the
AppDispatchimport path matches the rest of the codebase. For example, if other components (e.g.MapStory,FiltersSidebar,MetricsCards) import it from a different module (such as@/lib/redux/storeor similar), update the import path here to be identical. - If the project already has a
useAppDispatchhook (e.g. in@/lib/hooks), you may prefer to:- Import
useAppDispatchinstead ofuseDispatch. - Replace
const dispatch = useDispatch<AppDispatch>();withconst dispatch = useAppDispatch();.
- Import
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lib/features/filters/selectors.tswith individual typed selectors for everyFilterStatefield — components no longer re-render on unrelated state changesMapStory: replaceuseSelector(state => state.filters)destructure with 8 granular selectors; remove unusedRootStateimportFilterSidebar: replace whole-slice destructure with 9 granular selectorsMetricsCards: replace whole-slice destructure with 3 granular selectorsCountyRank: migrate toselectSelectedMetric,selectIsPerCapita,selectSelectedCounty— removes last inlinestate.filters/state.mapaccessesTest plan
npm run buildpasses with no TypeScript errors/map— map renders counties with colors🤖 Generated with Claude Code
Summary by Sourcery
Introduce granular typed selectors for the filters slice and update map-related components to use them for more targeted Redux subscriptions.
Enhancements: