Skip to content

Fix minor code review items#21

Merged
brianfunk merged 2 commits intodevfrom
fix/minor-code-review-items
Feb 11, 2026
Merged

Fix minor code review items#21
brianfunk merged 2 commits intodevfrom
fix/minor-code-review-items

Conversation

@brianfunk
Copy link
Collaborator

Summary

  • Script error handling: Added onerror handlers to all 5 preview components (D3, ECharts, Highcharts, Leaflet, OpenLayers) so users see a clear error message when a CDN library fails to load
  • Script cleanup: Added proper script/link element cleanup in useEffect return for D3Preview and OpenLayersPreview (ECharts, Highcharts, and Leaflet already had this)
  • Lat/lng bounds validation: Added geographic bounds checking (-90..90 lat, -180..180 lng) in normalizeMapRows to silently discard invalid coordinates
  • Stable React keys: Added id field to ManualInputRow using crypto.randomUUID() and replaced key={index} with key={row.id} in DataInput.tsx for correct reconciliation when rows are added/removed

Test plan

  • All 16 unit tests passing
  • Lint clean
  • Build successful
  • Verify preview components show error message when CDN is unreachable (disable network)
  • Verify manual data entry rows maintain state correctly when deleting a middle row
  • Verify map data with out-of-bounds lat/lng is properly filtered

🤖 Generated with Claude Code

…le keys

Add onerror handlers to all dynamic CDN script loaders in preview components
(D3, ECharts, Highcharts, Leaflet, OpenLayers) so users see a clear error
message when a library fails to load. Add proper script/link cleanup in
useEffect return for D3Preview and OpenLayersPreview. Add lat/lng bounds
validation (-90..90, -180..180) in normalizeMapRows. Replace index-based
React keys with stable UUIDs on ManualInputRow for correct reconciliation
when rows are added or removed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9d386caf10

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +301 to +303
if (link?.parentNode) {
link.parentNode.removeChild(link);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve OpenLayers CSS when rerendering map preview

In OpenLayersPreview's useEffect, this cleanup removes the dynamically added ol.css link, but subsequent renders often take the window.ol fast path and do not re-add the stylesheet; this happens after any dependency change (for example, changing theme or data) once OpenLayers has loaded once. The result is that later map renders run without required OpenLayers CSS, causing broken/unusable map controls and styling even though JS still executes.

Useful? React with 👍 / 👎.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @chatgpt-codex-connector. D3Preview and OpenLayersPreview both use a "library already loaded?" fast path — once the library is on window, subsequent renders skip the dynamic <script>/<link> creation. Removing the ol.css link in cleanup meant it was never re-added, breaking map styling on re-renders.

Fixed in bb44404: both D3Preview and OpenLayersPreview now leave their script/link elements in the DOM on cleanup (just clear innerHTML). This is correct because the libraries persist on window regardless, and the elements are only ever created once. ECharts, Highcharts, and Leaflet don't have this issue — they always re-create their script tags on each render.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have reached your Codex usage limits. You can see your limits in the Codex usage dashboard.

D3Preview and OpenLayersPreview check if the library is already on window
before loading. Removing script/link tags in useEffect cleanup meant that
subsequent renders (theme/data changes) took the fast path without
re-adding the ol.css stylesheet, breaking OpenLayers map styling. Leave
these elements in the DOM since the libraries persist on window anyway.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@brianfunk brianfunk merged commit b241c98 into dev Feb 11, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant