refactor: eliminate Redux round-trip anti-pattern with createSelector#4
Open
ashkanrdn wants to merge 1 commit intorefactor/type-consolidationfrom
Open
refactor: eliminate Redux round-trip anti-pattern with createSelector#4ashkanrdn wants to merge 1 commit intorefactor/type-consolidationfrom
ashkanrdn wants to merge 1 commit intorefactor/type-consolidationfrom
Conversation
- mapSlice: replace colorScaleValues + barChartData with enhancedGeojson (single source) - Add lib/features/map/selectors.ts with selectBarChartData, selectRankedCounties, selectColorScaleValues (all memoized via createSelector) - filterSlice: remove rankedCounties — derived by selector instead - MapStory: dispatch setEnhancedGeojson once from worker; remove 3 useMemo+useEffect round-trips; colorScale built from selectColorScaleValues - BarChartWidget, CountyRank: read from selectors instead of raw state slices Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideRefactors map-related Redux state to store only worker-produced enhancedGeojson and derives color scale values, ranked counties, and bar chart data via memoized selectors, simplifying MapStory and updating dependent widgets to consume typed selectors instead of raw slices. Sequence diagram for worker-to-selectors map data flowsequenceDiagram
actor User
participant MapStory
participant Worker
participant ReduxStore
participant Selectors
participant BarChartWidget
participant CountyRank
User->>MapStory: Interacts with map / filters
MapStory->>Worker: postMessage(geojsonData, filteredData, metric, perCapita)
Worker-->>MapStory: message(enhancedGeojson)
MapStory->>ReduxStore: dispatch(setEnhancedGeojson(enhancedGeojson))
BarChartWidget->>ReduxStore: useSelector(selectBarChartData)
CountyRank->>ReduxStore: useSelector(selectRankedCounties)
MapStory->>ReduxStore: useSelector(selectColorScaleValues)
ReduxStore->>Selectors: selectEnhancedGeojson, selectSelectedMetric, selectIsPerCapita
Selectors-->>ReduxStore: derived colorScaleValues, rankedCounties, barChartData
ReduxStore-->>MapStory: colorScaleValues
ReduxStore-->>BarChartWidget: barChartData, colorScaleValues
ReduxStore-->>CountyRank: rankedCounties
MapStory->>MapStory: build d3 colorScale from colorScaleValues
BarChartWidget->>BarChartWidget: build d3 colorScale from colorScaleValues
CountyRank->>CountyRank: render ranking list
Class diagram for updated map and filter Redux slices with selectorsclassDiagram
class MapState {
string selectedCounty
EnhancedFeature[] enhancedGeojson
}
class FilterState {
CsvRow[] filteredData
number[2] yearRange
string selectedMetric
DataSourceType selectedDataSource
boolean isPerCapita
string[] selectedCounties
}
class MapSlice {
+MapState initialState
+setSelectedCounty(payload string) void
+setEnhancedGeojson(payload EnhancedFeature[]) void
}
class FilterSlice {
+FilterState initialState
+setCsvData(payload CsvRow[]) void
+toggleFilter(payload unknown) void
+setYear(payload number) void
+setSelectedDataSource(payload DataSourceType) void
+setIsPerCapita(payload boolean) void
+setSelectedCounties(payload string[]) void
+setFilteredData(payload CsvRow[]) void
}
class RootState {
MapState map
FilterState filters
}
class MapSelectors {
+selectEnhancedGeojson(state RootState) EnhancedFeature[]
+selectSelectedCounty(state RootState) string
+selectColorScaleValues(state RootState) number[]
+selectRankedCounties(state RootState) RankedCounty[]
+selectBarChartData(state RootState) BarChartDatum[]
}
class RankedCounty {
string name
number value
number rank
}
class BarChartDatum {
string county
number value
}
class EnhancedFeatureProperties {
string name
number rawValue
number perCapitaValue
number metricValue
}
class EnhancedFeature {
EnhancedFeatureProperties properties
unknown geometry
unknown id
}
MapSlice --> MapState
FilterSlice --> FilterState
RootState o-- MapState
RootState o-- FilterState
MapSelectors ..> RootState
MapSelectors ..> EnhancedFeature
MapSelectors ..> RankedCounty
MapSelectors ..> BarChartDatum
EnhancedFeature o-- EnhancedFeatureProperties
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 1 issue, and left some high level feedback:
- In
selectRankedCounties,value: f.properties[metric] as numberis assumed to be numeric without filtering, which can lead toNaNrankings; consider mirroring the numeric guard used inselectColorScaleValues(or reusing a shared helper) before sorting. selectSelectedCountyinselectors.tsis currently unused; if it's meant for future use consider wiring it into components or remove it to keep the selector surface minimal.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `selectRankedCounties`, `value: f.properties[metric] as number` is assumed to be numeric without filtering, which can lead to `NaN` rankings; consider mirroring the numeric guard used in `selectColorScaleValues` (or reusing a shared helper) before sorting.
- `selectSelectedCounty` in `selectors.ts` is currently unused; if it's meant for future use consider wiring it into components or remove it to keep the selector surface minimal.
## Individual Comments
### Comment 1
<location> `lib/features/map/selectors.ts:26` </location>
<code_context>
+ geojson
+ .map((f) => ({
+ name: f.properties.name,
+ value: f.properties[metric] as number,
+ rank: 0,
+ }))
</code_context>
<issue_to_address>
**issue:** Guard `value` against non-numeric or missing data before ranking counties.
If `f.properties[metric]` is missing or non-numeric, `value` becomes `NaN`, so `b.value - a.value` will also be `NaN` and the sort order undefined. Please either reuse the numeric filtering from `selectColorScaleValues` (e.g., filter to finite numbers before sorting) or coerce invalid values to a deterministic default (such as `0`) before ranking.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| geojson | ||
| .map((f) => ({ | ||
| name: f.properties.name, | ||
| value: f.properties[metric] as number, |
There was a problem hiding this comment.
issue: Guard value against non-numeric or missing data before ranking counties.
If f.properties[metric] is missing or non-numeric, value becomes NaN, so b.value - a.value will also be NaN and the sort order undefined. Please either reuse the numeric filtering from selectColorScaleValues (e.g., filter to finite numbers before sorting) or coerce invalid values to a deterministic default (such as 0) before ranking.
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
mapSlice: replacecolorScaleValues+barChartDatawithenhancedGeojson: EnhancedFeature[]— single worker output stored in Reduxlib/features/map/selectors.tswithselectBarChartData,selectRankedCounties,selectColorScaleValues(all memoized viacreateSelector)filterSlice: removerankedCounties— now derived by selectorMapStory: workeronmessagedispatches onesetEnhancedGeojson; removes 3useMemo + useEffectround-trip pairs (~60 lines);colorScalebuilt fromselectColorScaleValuesBarChartWidget,CountyRank: read from typed selectors instead of raw state slicesTest plan
npm run buildpasses with no TypeScript errors/map— map renders counties with colors🤖 Generated with Claude Code
Summary by Sourcery
Refactor map data flow so worker outputs a single enhanced geojson payload into Redux, with derived visualization data computed via memoized selectors.
New Features:
Enhancements: