Add charts to the homepage#2
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds a home page carousel showcasing data signals, backed by a new resource-chart component library (types, data transform, Plot-based rendering) and CKAN/DMS resource-fetching helpers. It also migrates CSV parsing across the codebase to shared ChangesData Signal Carousel and Chart Infrastructure
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Page as Home Page
participant Carousel as HomeDataSignalCarousel
participant CKAN as CKAN Resource API
participant Chart as ResourceChart
participant Client as HomeDataSignalCarouselClient
Page->>Carousel: render()
Carousel->>CKAN: getResourceByName(slide.name)
CKAN-->>Carousel: resource or null
Carousel->>CKAN: getDatasetDetails(resource.package_id)
CKAN-->>Carousel: dataset details or null
Carousel->>Chart: render(resourceId, config)
Chart->>CKAN: fetchCkanResourceRows(resourceId)
CKAN-->>Chart: CSV rows
Chart-->>Carousel: chart element
Carousel->>Client: render(slides, charts)
Client-->>Page: carousel UI
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/components/resource-chart/ResourceChart.tsx (1)
25-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider simplifying the IIFE to a straightforward try/catch.
The
await (async () => { ... })()pattern works but adds cognitive overhead. A plain try/catch with aletbinding is more idiomatic.♻️ Proposed refactor
- const state = await (async () => { - try { - const rows = await fetchRows(resourceId); - return buildResourceChartState(rows, config); - } catch (error) { - console.error(`Failed to render resource chart for ${resourceId}:`, error); - return { status: "empty" as const, message: "This chart could not be loaded." }; - } - })(); + let state: ResourceChartState; + try { + const rows = await fetchRows(resourceId); + state = buildResourceChartState(rows, config); + } catch (error) { + console.error(`Failed to render resource chart for ${resourceId}:`, error); + state = { status: "empty", message: "This chart could not be loaded." }; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/resource-chart/ResourceChart.tsx` around lines 25 - 33, The async IIFE used to assign state in ResourceChart is unnecessarily complex; replace it with a direct try/catch around the fetchRows/resource chart state creation logic. Use a simple let binding for state in ResourceChart and keep the existing error handling and fallback state behavior unchanged.src/components/resource-chart/transform.ts (1)
16-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRegex character class has a redundant comma.
The
,appears twice in/[$,%\s,]/g. It's functionally harmless but slightly confusing — a reader might wonder if the second comma was meant to be a different character.🧹 Proposed cleanup
- const normalized = value.replace(/[$,%\s,]/g, ""); + const normalized = value.replace(/[$,%\s]/g, "");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/resource-chart/transform.ts` around lines 16 - 23, The toNumber helper in transform.ts has a redundant comma in the cleanup regex, which is confusing to read. Update the regex used in toNumber to remove the duplicate comma character while preserving the existing normalization behavior for values handled by toNumber.src/app/[locale]/page.tsx (1)
196-197: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winWrap
HomeDataSignalCarouselinSuspenseto avoid blocking page render.
HomeDataSignalCarouselis an async server component that fetches CKAN resources and dataset details. Without a Suspense boundary, the entire page waits for this data before sending any HTML. Wrapping it allows the hero section and stats to render immediately while the carousel streams in.⚡ Proposed fix
+import { Suspense, type CSSProperties } from "react";</div> + + <Suspense fallback={<div className="min-h-[380px]" />}> + <HomeDataSignalCarousel /> + </Suspense> </Container>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[locale]/page.tsx around lines 196 - 197, Wrap HomeDataSignalCarousel in a Suspense boundary so the page can render the hero/stats immediately while the async server component streams in. Update the page component in the locale page to render HomeDataSignalCarousel with Suspense and provide a lightweight fallback, keeping the existing HomeDataSignalCarousel usage intact.src/lib/charts/usePlot.ts (1)
22-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider re-rendering the plot when the builder closure changes.
The main effect depends only on
[ref], so theResizeObserveris set up once.builderRef.currentis updated via the ref-update effect, but the plot only rebuilds on container resize. If the underlying data changes (e.g., props update causing a new builder closure), the plot shows stale content until the next resize event. This is fine for the current carousel (static data), but could cause confusion if the hook is reused with dynamic data.A simple fix: extract the render logic into a helper and also call it from the ref-update effect, or add a
depsparameter to the hook's main effect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/charts/usePlot.ts` around lines 22 - 40, The plot rendering in usePlot only updates on ResizeObserver events, so a new builder closure can leave stale content visible until resize. Update usePlot so the render path is also triggered when builderRef.current changes, either by extracting the plot निर्माण logic into a shared helper and calling it from the ref-update effect or by adding a deps parameter to the main effect. Keep the ResizeObserver setup in usePlot, but ensure the latest builder closure rebuilds the plot immediately after props/data changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/resource-chart/ResourceChartClient.tsx`:
- Around line 89-156: The horizontal bar chart in BarChart can be taller than
the carousel slide height, causing the plot content to be clipped by the outer
h-full overflow-hidden container. Update the bar-h slide configuration in the
carousel (where config.height is set) so it is at least 330, or lower the bar
chart limit so BarChart’s minHeight stays within the available height; use the
BarChart component and the fossil-co2-emitters slide config as the key locations
to adjust.
In `@src/components/resource-chart/transform.ts`:
- Around line 63-72: The aggregate helper currently returns 0 for empty buckets,
which makes empty min/max buckets look like real zero-valued points. Update
aggregate in transform.ts so the min/max branches do not hit the generic
empty-values fallback; instead return null or NaN when acc.values is empty, and
widen the return type accordingly. Keep count and avg behavior intact, and
ensure the downstream Number.isFinite filter in the resource chart transform
continues to drop these empty min/max results.
- Around line 97-104: `limitPoints` has a division-by-zero edge case when
`limit` is 1 and `preserveRange` is true, causing the sampled result to become
empty instead of returning one point. Update the `limitPoints` logic to handle
the `limit === 1` case explicitly before computing `step`, and return a single
preserved point (for example the first or only sampled point) when used by the
line-chart path. Keep the fix localized in `limitPoints` and ensure the existing
`preserveRange` sampling behavior still works for `limit > 1`.
---
Nitpick comments:
In `@src/app/`[locale]/page.tsx:
- Around line 196-197: Wrap HomeDataSignalCarousel in a Suspense boundary so the
page can render the hero/stats immediately while the async server component
streams in. Update the page component in the locale page to render
HomeDataSignalCarousel with Suspense and provide a lightweight fallback, keeping
the existing HomeDataSignalCarousel usage intact.
In `@src/components/resource-chart/ResourceChart.tsx`:
- Around line 25-33: The async IIFE used to assign state in ResourceChart is
unnecessarily complex; replace it with a direct try/catch around the
fetchRows/resource chart state creation logic. Use a simple let binding for
state in ResourceChart and keep the existing error handling and fallback state
behavior unchanged.
In `@src/components/resource-chart/transform.ts`:
- Around line 16-23: The toNumber helper in transform.ts has a redundant comma
in the cleanup regex, which is confusing to read. Update the regex used in
toNumber to remove the duplicate comma character while preserving the existing
normalization behavior for values handled by toNumber.
In `@src/lib/charts/usePlot.ts`:
- Around line 22-40: The plot rendering in usePlot only updates on
ResizeObserver events, so a new builder closure can leave stale content visible
until resize. Update usePlot so the render path is also triggered when
builderRef.current changes, either by extracting the plot निर्माण logic into a
shared helper and calling it from the ref-update effect or by adding a deps
parameter to the main effect. Keep the ResizeObserver setup in usePlot, but
ensure the latest builder closure rebuilds the plot immediately after props/data
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bae728e-eeca-4f0e-a759-0e94ce62efb7
📒 Files selected for processing (20)
messages/en.jsonsrc/app/[locale]/page.tsxsrc/components/csv-explorer/DataProvider.tsxsrc/components/csv-explorer/SearchDataForm.tsxsrc/components/home/HomeDataSignalCarousel.tsxsrc/components/home/HomeDataSignalCarouselClient.tsxsrc/components/resource-chart/ResourceChart.tsxsrc/components/resource-chart/ResourceChartClient.tsxsrc/components/resource-chart/client.tssrc/components/resource-chart/fetchers.tssrc/components/resource-chart/index.tssrc/components/resource-chart/transform.tssrc/components/resource-chart/types.tssrc/lib/charts/format.tssrc/lib/charts/usePlot.tssrc/lib/ckan/api.tssrc/lib/ckan/resource.tssrc/lib/csv.tssrc/lib/dms.tssrc/lib/parseCsv.ts
1 participant
Issue: datopian/portaljs#1618
Summary by CodeRabbit
New Features
Bug Fixes