refactor: consolidate shared types and extract constants#3
Open
ashkanrdn wants to merge 1 commit intofix/dead-code-and-bugsfrom
Open
refactor: consolidate shared types and extract constants#3ashkanrdn wants to merge 1 commit intofix/dead-code-and-bugsfrom
ashkanrdn wants to merge 1 commit intofix/dead-code-and-bugsfrom
Conversation
- Add app/types/map.ts with canonical EnhancedFeature interface - Add lib/constants/mapConfig.ts (LOAD_DELAY_MS, FLY_TO_ZOOM, TRANSITION_DURATION_MS, UNSELECTED_COUNTY_COLOR, MAP_BOUNDS, MIN_ZOOM) - Add lib/constants/dataDescriptions.ts with data source descriptions - Remove duplicate EnhancedFeature definitions from MapStory, worker, MapTooltip - Replace inline magic numbers in MapStory with named constants - Move inline dataDescriptions from MapStory to lib/constants 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 code by introducing shared types and configuration constants, moving previously duplicated interfaces and inline literals into dedicated modules, and updating map components to import from these canonical sources. Class diagram for shared map types and configuration constantsclassDiagram
class Feature {
}
class EnhancedFeature {
+properties name string
+properties metricKey any
+properties rawValue number
+properties perCapitaValue number
+properties rowCount number
+properties totalCostValue number
+properties avgCostPerPrisonerValue number
}
Feature <|-- EnhancedFeature
class mapConfig {
<<module>>
+LOAD_DELAY_MS number
+FLY_TO_ZOOM number
+TRANSITION_DURATION_MS number
+UNSELECTED_COUNTY_COLOR number[4]
+MAP_BOUNDS number[2][2]
+MIN_ZOOM number
}
class dataDescriptionsModule {
<<module>>
+dataDescriptions arrest
+dataDescriptions county_prison
+dataDescriptions jail
}
class dataDescriptions_arrest {
+name string
+description string
+metrics_Arrest_rate string
+metrics_Total_Arrests string
}
class dataDescriptions_county_prison {
+name string
+description string
+metrics_Imprisonments string
+metrics_Cost_per_prisoner string
+metrics_Total_Cost string
}
class dataDescriptions_jail {
+name string
+description string
+metrics_ADPtotrate string
+metrics_ADPtotal string
+metrics_Felony string
+metrics_Misd string
+metrics_Postdisp string
+metrics_Predisp string
}
dataDescriptionsModule --> dataDescriptions_arrest
dataDescriptionsModule --> dataDescriptions_county_prison
dataDescriptionsModule --> dataDescriptions_jail
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:
- The MAP_BOUNDS values in mapConfig look reversed (minLongitude is -117.59 and maxLongitude is -120.99); consider swapping or renaming to avoid incorrect bounding behavior and confusion.
- EnhancedFeature.properties uses an index signature with
any; consider tightening this tounknownor a more specific union and/or splitting computed properties into a separate typed sub-object to catch more type issues at compile time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The MAP_BOUNDS values in mapConfig look reversed (minLongitude is -117.59 and maxLongitude is -120.99); consider swapping or renaming to avoid incorrect bounding behavior and confusion.
- EnhancedFeature.properties uses an index signature with `any`; consider tightening this to `unknown` or a more specific union and/or splitting computed properties into a separate typed sub-object to catch more type issues at compile time.
## Individual Comments
### Comment 1
<location> `lib/constants/mapConfig.ts:17-19` </location>
<code_context>
+ * Geographic bounds constraining map panning (roughly California).
+ * [ [minLongitude, minLatitude], [maxLongitude, maxLatitude] ]
+ */
+export const MAP_BOUNDS: [[number, number], [number, number]] = [
+ [-117.595944, 33.386416], // Southwest corner
+ [-120.999866, 42.183974], // Northeast corner
+];
+
</code_context>
<issue_to_address>
**issue (bug_risk):** MAP_BOUNDS longitudes appear reversed (west > east), which may break consumers expecting [west, south], [east, north].
These bounds are `[-117.595944, 33.386416]` to `[-120.999866, 42.183974]`. Since -120.999866 < -117.595944, this encodes `[east, south]` → `[west, north]` instead of `[west, south]` → `[east, north]`. Many mapping APIs (Mapbox, deck.gl) assume west < east and may fail or behave oddly with reversed longitudes, especially now that this constant is shared. If this isn’t intentional, please swap the longitudes and align the comments with the correct convention.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+17
to
+19
| export const MAP_BOUNDS: [[number, number], [number, number]] = [ | ||
| [-117.595944, 33.386416], // Southwest corner | ||
| [-120.999866, 42.183974], // Northeast corner |
There was a problem hiding this comment.
issue (bug_risk): MAP_BOUNDS longitudes appear reversed (west > east), which may break consumers expecting [west, south], [east, north].
These bounds are [-117.595944, 33.386416] to [-120.999866, 42.183974]. Since -120.999866 < -117.595944, this encodes [east, south] → [west, north] instead of [west, south] → [east, north]. Many mapping APIs (Mapbox, deck.gl) assume west < east and may fail or behave oddly with reversed longitudes, especially now that this constant is shared. If this isn’t intentional, please swap the longitudes and align the comments with the correct convention.
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
app/types/map.tswith canonicalEnhancedFeatureinterface (was duplicated in 3 files)lib/constants/mapConfig.tswith named constants (LOAD_DELAY_MS,FLY_TO_ZOOM,TRANSITION_DURATION_MS,UNSELECTED_COUNTY_COLOR,MAP_BOUNDS,MIN_ZOOM)lib/constants/dataDescriptions.ts— moved from 34 inline lines inMapStory.tsxMapStory.tsx,dataProcessor.worker.ts,MapTooltip.tsxto import from canonical locationsTest plan
npm run buildpasses with no TypeScript errors/map— map renders counties with colors🤖 Generated with Claude Code
Summary by Sourcery
Consolidate shared map-related types and configuration into reusable modules and update consumers to import from these canonical sources.
Enhancements: