docs: basic sankey example + profit sankey - #16
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review limit reached
Next review available in: 15 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 Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (22)
📝 WalkthroughWalkthroughAdds basic and Apple FY22 Sankey conformance cases for TanStack and Recharts. The changes include typed flow models, responsive renderers, validation tests, documentation, D3 Sankey packages, and catalog count updates. ChangesSankey conformance cases
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)Basic Sankey rendering flowsequenceDiagram
participant ConformanceInput
participant basicSankeyData
participant d3Sankey
participant BasicSankeyRenderer
ConformanceInput->>basicSankeyData: provide revision
basicSankeyData->>d3Sankey: provide nodes and links
d3Sankey->>BasicSankeyRenderer: return laid-out graph
BasicSankeyRenderer->>ConformanceInput: return rendered scene nodes and chart points
Income-statement Sankey rendering flowsequenceDiagram
participant ConformanceInput
participant incomeStatementData
participant d3Sankey
participant SankeyRenderer
ConformanceInput->>incomeStatementData: provide revision
incomeStatementData->>d3Sankey: provide income-statement nodes and links
d3Sankey->>SankeyRenderer: return resolved endpoints and layout bounds
SankeyRenderer->>ConformanceInput: return rendered links, nodes, labels, values, and chart points
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (3)
benchmarks/conformance/cases/111-basic-sankey/tanstack.ts (1)
20-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
responsiveLayout/clamphelpers across renderers.Lines 136-149 (
responsiveLayout,clamp) are byte-for-byte identical to the same functions inrecharts.ts(lines 100-113). Consider extracting them to a shared module (for examplemodel.tsor a smalllayout.tsalongside it) so both renderers stay in sync when layout tuning changes. This duplication may be intentional to keep each conformance renderer self-contained for side-by-side comparison, so treat this as optional.🤖 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 `@benchmarks/conformance/cases/111-basic-sankey/tanstack.ts` around lines 20 - 134, Optionally extract the shared responsiveLayout and clamp helpers used by basicSankey and the corresponding recharts renderer into a common layout or model module, then update both renderers to import and reuse them so layout tuning remains synchronized while preserving current behavior.benchmarks/conformance/cases/111-sankey-flow/recharts.ts (1)
115-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRender the title outside the node renderer.
The title is emitted only when
index === 0, so it depends on Recharts calling the node renderer for the first data node and on that group not being clipped or reordered. Render the title as a sibling ofSankeyinside a wrapping element, or through a Recharts label component. This also removes the hidden coupling to node order.🤖 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 `@benchmarks/conformance/cases/111-sankey-flow/recharts.ts` around lines 115 - 136, The title rendering currently depends on the first node invocation in the renderer. Move the incomeStatementTitle element out of the index === 0 branch in the node renderer and render it as a sibling of Sankey within a wrapping element, or via a Recharts label component, while preserving its existing positioning and styling.benchmarks/conformance/cases/111-sankey-flow/tanstack.ts (1)
208-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sharing the layout helpers with the Recharts implementation.
responsiveLayout,clamp, andlabelBackdropBoundsare identical tobenchmarks/conformance/cases/111-sankey-flow/recharts.tslines 167-209. Both renderers must stay pixel-comparable for the conformance comparison. If one copy changes, the case silently loses parity. Move the three helpers into a shared module in the case directory, for examplelayout.ts, and import them from both files.🤖 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 `@benchmarks/conformance/cases/111-sankey-flow/tanstack.ts` around lines 208 - 250, Move responsiveLayout, clamp, and labelBackdropBounds into a shared layout module in the sankey-flow case directory, then remove their local definitions and import the shared helpers from both tanstack.ts and recharts.ts. Preserve the existing helper behavior and signatures so both renderers remain pixel-comparable.
🤖 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 `@benchmarks/conformance/cases/111-sankey-flow/model.ts`:
- Around line 143-150: Update the depth-0 node ordering in the Sankey case so
services has a unique order value that matches its intended position after
wearables. Move the services template into that corresponding position in the
node array and remove its existing later duplicate, preserving the other nodes’
relative order.
In `@benchmarks/conformance/cases/111-sankey-flow/recharts.ts`:
- Line 11: Update the Sankey type import to use Recharts’ public renamed
typings, SankeyNodeProps and SankeyLinkProps, instead of the internal Sankey
NodeProps and LinkProps exports. Apply the renamed types to the node/link render
callbacks, or define equivalent local prop interfaces if the public types are
unavailable.
---
Nitpick comments:
In `@benchmarks/conformance/cases/111-basic-sankey/tanstack.ts`:
- Around line 20-134: Optionally extract the shared responsiveLayout and clamp
helpers used by basicSankey and the corresponding recharts renderer into a
common layout or model module, then update both renderers to import and reuse
them so layout tuning remains synchronized while preserving current behavior.
In `@benchmarks/conformance/cases/111-sankey-flow/recharts.ts`:
- Around line 115-136: The title rendering currently depends on the first node
invocation in the renderer. Move the incomeStatementTitle element out of the
index === 0 branch in the node renderer and render it as a sibling of Sankey
within a wrapping element, or via a Recharts label component, while preserving
its existing positioning and styling.
In `@benchmarks/conformance/cases/111-sankey-flow/tanstack.ts`:
- Around line 208-250: Move responsiveLayout, clamp, and labelBackdropBounds
into a shared layout module in the sankey-flow case directory, then remove their
local definitions and import the shared helpers from both tanstack.ts and
recharts.ts. Preserve the existing helper behavior and signatures so both
renderers remain pixel-comparable.
🪄 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 Plus
Run ID: 53809767-7cad-4011-91f6-4f029e20f03f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
benchmarks/conformance/cases/111-basic-sankey/case.jsonbenchmarks/conformance/cases/111-basic-sankey/model.tsbenchmarks/conformance/cases/111-basic-sankey/recharts.tsbenchmarks/conformance/cases/111-basic-sankey/tanstack.test.tsbenchmarks/conformance/cases/111-basic-sankey/tanstack.tsbenchmarks/conformance/cases/111-sankey-flow/case.jsonbenchmarks/conformance/cases/111-sankey-flow/model.tsbenchmarks/conformance/cases/111-sankey-flow/recharts.tsbenchmarks/conformance/cases/111-sankey-flow/tanstack.test.tsbenchmarks/conformance/cases/111-sankey-flow/tanstack.tsdocs/examples/networks-and-hierarchies.mdexamples/conformance/package.jsonpackage.jsonpackages/charts-core/docs/examples/networks-and-hierarchies.mdscripts/catalog-artifact.mjs
2 participants
Added a couple examples + conformance tests against Recharts like other examples had.
Not sure if these are as simple as they should be, so please hack away at them or re-implement them if this just serves as a decent reference.
These use a d3 plugin d3-sankey, so had to install that too
Screenshots
Summary by CodeRabbit