Open
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds Sankey and Sunburst chart components (with layout utilities), Storybook stories, docs, examples, tests, Nuxt playground pages, d3-sankey dependency, selector type widening, and related exports/fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Vue Component<br/>(Sankey)
participant D3 as D3 Sankey<br/>Layout
participant Store as Redux Store
participant Render as SVG Renderer
participant Slot as Slot System
Component->>D3: provide nodes/links + dimensions
D3-->>Component: return layout nodes & links
Component->>Store: register tooltip entry settings
Component->>Render: render SVG layers (nodes, links)
alt node slot provided
Render->>Slot: pass node geometry & payload
Slot-->>Render: custom SVG node
else
Render->>Render: default rect node
end
alt link slot provided
Render->>Slot: pass link path & width
Slot-->>Render: custom SVG link
else
Render->>Render: default path link
end
Component->>Store: dispatch hover/click actions
Store-->>Render: update active state / tooltip coords
sequenceDiagram
participant Component as Vue Component<br/>(SunburstChart)
participant Hierarchy as D3 Hierarchy
participant D3 as D3 Partition
participant Store as Redux Store
participant Render as SVG Renderer
participant Slot as Slot System
Component->>Hierarchy: build hierarchy from data
Component->>D3: compute partition layout (values)
D3-->>Component: angular & radial fractions
Component->>Component: convert fractions to radii & angles
Component->>Store: register tooltip entries
Component->>Render: render sectors list
loop per sector
alt content slot provided
Render->>Slot: pass sector props
Slot-->>Render: custom sector SVG
else
Render->>Render: default sector shape
end
end
Component->>Store: dispatch hover/click actions
Store-->>Render: update active state / tooltip display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Deploying vue-charts with
|
| Latest commit: |
1061467
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b2bf1b8f.vue-charts.pages.dev |
| Branch Preview URL: | https://feat-sankey-chart.vue-charts.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (1)
packages/vue/src/chart/__tests__/sunburstUtils.spec.ts (1)
153-168: Add a payload-resolution assertion fortooltipIndex.The regex check validates format only, not correctness. Please add an assertion that
tooltipIndexresolves back to the same source node payload (especially with unsorted values) to catch path/order regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue/src/chart/__tests__/sunburstUtils.spec.ts` around lines 153 - 168, Extend the test that uses computeSunburstLayout to not only assert tooltipIndex format but also resolve that tooltipIndex against the original nestedData to ensure it points to the same payload: for each node in nodes, parse node.tooltipIndex (e.g. "children[0].children[1]") and traverse nestedData following those child indices to obtain a resolvedPayload, then assert resolvedPayload === node.payload (or deep-equal) to catch path/order regressions; reference computeSunburstLayout, nestedData, node.tooltipIndex and node.payload when adding this resolver-based assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/3.charts/11.sankey.md`:
- Around line 35-38: The prop table has swapped/missing entries: fix the rows
for nodeStroke and linkFill and add a linkStroke row. Specifically, ensure
nodeFill stays `'#0088fe'` with description "Default fill color for node
rectangles", change nodeStroke to default `undefined` with description "Stroke
color for node rectangles", add a new `linkStroke` prop with default `'#0088fe'`
and description "Default stroke color for link paths", and set `linkFill` to
default `undefined` with description "Fill color for link paths"; keep
`isAnimationActive` as `true`. Update the table entries for nodeFill,
nodeStroke, linkFill, linkStroke, and isAnimationActive accordingly.
In `@docs/superpowers/plans/2026-04-06-sankey-chart.md`:
- Around line 25-27: The plan references stale repo paths in the table rows
(e.g., the strings "playground/nuxt/pages/sankey.vue",
"docs/content/2.charts/sankey.md", and "tasks/todo.md") and the same outdated
entries at the other mentioned ranges; update each occurrence to the current
repository locations used by the project (replace the old path strings with
their new equivalents across the file, including the other occurrences at the
noted ranges) so links and follow-up tasks point to the correct files and ensure
the table rows and any linked mentions use the repo's canonical path format.
In `@docs/superpowers/specs/2026-04-06-sankey-chart-design.md`:
- Around line 29-35: The markdown contains fenced code blocks without language
identifiers (the package tree block and the props/flow diagram block), which
triggers markdownlint MD040; update each triple-backtick fence to include a
language (e.g., ```text) for both the block showing the package tree and the
block showing the props.data → computeSankeyLayout → Animate → Layer diagram so
the linter recognizes them as code blocks.
In `@docs/superpowers/specs/2026-04-11-sunburst-chart-design.md`:
- Around line 63-72: The fenced code block showing the SunburstChart component
tree is missing a language token and triggers markdownlint MD040; update the
opening fence from ``` to ```text (or another appropriate language) so it
becomes ```text and ensure the closing fence remains ``` — target the fenced
block that contains "SunburstChart (wrapper) ├──
provideStore(createRechartsStore(sunburstOptions)) └── SunburstInner ..." to
apply this change.
In `@packages/vue/src/chart/__stories__/Sankey.stories.tsx`:
- Line 3: The import in this story uses a relative path; replace the relative
import of the Sankey component with the project alias form (use
"@/chart/Sankey") so it uses the "@/..." alias convention used for imports from
packages/vue/src; update the import statement that references Sankey accordingly
to maintain consistency and path stability.
In `@packages/vue/src/chart/__tests__/Sankey.spec.tsx`:
- Around line 44-47: Update the test rendering of the Sankey component to
disable animations for deterministic output: in the 'returns null when nodes are
empty' test, pass isAnimationActive={false} to the <Sankey ... /> render call so
the component renders deterministically (consistent with the suite's policy to
set isAnimationActive={false} and mock getBoundingClientRect() in beforeEach);
change the render invocation for Sankey to include that prop.
- Around line 4-7: The test currently imports Sankey directly from an internal
path; change the import to use the public API (import { Sankey } from '@/index')
so the test validates public exports, keep internal-only helpers like
mockGetBoundingClientRect as direct imports, and update test render calls to use
the render(() => <Sankey ... />) pattern (referencing the Sankey symbol and any
SunburstChart later) to follow the testing guideline.
In `@packages/vue/src/chart/__tests__/sankeyUtils.spec.ts`:
- Around line 1-2: Rename the test file from sankeyUtils.spec.ts to
sankeyUtils.spec.tsx to match the repo convention for chart tests; keep the
existing imports and test code (e.g., the describe/it blocks and the
computeSankeyLayout import) unchanged, and ensure the test runner/tsconfig
accepts .tsx test files if necessary so the file is discovered under the
__tests__ pattern.
In `@packages/vue/src/chart/__tests__/SunburstChart.spec.tsx`:
- Line 35: The describe title "SunburstChart" violates the
test/prefer-lowercase-title rule; update the describe call in
SunburstChart.spec.tsx (the describe('SunburstChart', ...) invocation) to use a
lowercase string (for example "sunburst chart" or "sunburstchart") so the title
complies with the lint rule while leaving the describe block and its tests
unchanged.
- Around line 4-5: The test imports internal modules directly; update imports to
use the public API entrypoint by replacing direct imports of SunburstChart and
Tooltip with imports from '@/index' (e.g., import { SunburstChart, Tooltip }
from '@/index'), and ensure the test uses the recommended render style by
calling render(() => <SunburstChart ... />) instead of rendering via other
patterns; update references to the SunburstChart and Tooltip symbols accordingly
so the test exercises the public API surface.
- Around line 127-141: The test currently only triggers hover without asserting
the tooltip; update the test in SunburstChart.spec.tsx to simulate hover via
fireEvent(chartWrapper, new MouseEvent('mousemove', {clientX, clientY})) on the
element with class '.v-charts-wrapper' using coordinates computed from
sectors[0].getBoundingClientRect() (instead of fireEvent.mouseEnter), call
nextTick() twice before asserting, then select the tooltip element (e.g.
querySelector for the tooltip container/class) and assert its displayed content
matches the expected label/value; if you are testing behavior that depends on
defaultIndex use three nextTick() calls per the guideline.
- Around line 35-39: Tests for SunburstChart are flaky because they rely on
default animations and lack a getBoundingClientRect mock; update all
SunburstChart renders in this suite (e.g., the render calls creating
<SunburstChart ... />) to pass isAnimationActive={false} and add a beforeEach
that mocks Element.prototype.getBoundingClientRect to return a stable size for
the rendered SVG/container; ensure the mock is restored/cleared in afterEach if
present so each test is deterministic.
In `@packages/vue/src/chart/Sankey.tsx`:
- Around line 93-94: The Sankey component's link rendering incorrectly uses
props.linkFill for the SVG stroke, ignoring the linkStroke prop; update the
rendering logic in the function that creates link paths (where stroke is
currently set to props.linkFill) to use props.linkStroke (with a fallback to
props.linkFill if linkStroke is 'none' or unset) so consumers can control stroke
via the linkStroke prop (refer to the Sankey component and the link drawing
block where stroke and fill are assigned).
- Line 8: The import in Sankey.tsx uses a relative path for internal modules;
update the import(s) to use the repository alias (@"...") instead of relative
paths—replace the import of RechartsWrapper (and the other import at line 32) so
they use "@/chart/RechartsWrapper" (or the correct path under packages/vue/src)
to match the codebase convention; ensure the import identifiers (RechartsWrapper
and the other referenced symbol) remain unchanged and that any ESLint/TS path
alias resolution still passes.
In `@packages/vue/src/chart/SunburstChart.tsx`:
- Around line 70-71: The SunburstChart props ringPadding and padding are
declared but never used; update the SunburstChart component so ringPadding and
padding are consumed by the layout and rendering code: wire the props into the
layout calculation (e.g., computeLayout / generateArcs / computeArcs) to shrink
outer/inner radii and to add radial gaps between rings (adjust radiusScale or
per-level radius calculations) and add angular gap handling to the
arcGenerator/renderArc (use padAngle or equivalent) so both radial and angular
spacing respect the numeric props; if supporting spacing is undesirable, remove
or mark ringPadding and padding as deprecated in the component API and delete
any unused references (ensure tests and prop docs are updated accordingly).
- Around line 42-47: The guard in sunburstPayloadSearcher incorrectly treats
activeIndex === 0 as absent; change the falsy check to a nullish check (use
activeIndex == null) so zero is allowed; keep the rest of the function and
return get(data, activeIndex) only when data and activeIndex are non-null.
In `@packages/vue/src/chart/sunburstUtils.ts`:
- Around line 41-47: buildTooltipIndex currently uses the child's index in the
post-sort hierarchy, which doesn't match the original data used by get(…,
tooltipIndex); modify buildTooltipIndex to use the original unsorted positions
instead: before sorting the children (where .sort() is called), attach the
original index to each child node (e.g., originalIndex or similar) or keep a
reference to the unsorted children array on the parent, then in
buildTooltipIndex use that originalIndex or search the parent's unsorted
children to compute idx so tooltipIndex points into the original data.children
order expected by get.
In `@tasks/todo.md`:
- Line 154: Remove the accidental shell environment command "export
https_proxy=http://127.0.0.1:7890 http_proxy=http://127.0.0.1:7890
all_proxy=socks5://127.0.0.1:7890" from the markdown (it was committed into the
task tracker); locate the exact line containing that export string and delete it
so the document contains only task-related content.
---
Nitpick comments:
In `@packages/vue/src/chart/__tests__/sunburstUtils.spec.ts`:
- Around line 153-168: Extend the test that uses computeSunburstLayout to not
only assert tooltipIndex format but also resolve that tooltipIndex against the
original nestedData to ensure it points to the same payload: for each node in
nodes, parse node.tooltipIndex (e.g. "children[0].children[1]") and traverse
nestedData following those child indices to obtain a resolvedPayload, then
assert resolvedPayload === node.payload (or deep-equal) to catch path/order
regressions; reference computeSunburstLayout, nestedData, node.tooltipIndex and
node.payload when adding this resolver-based assertion.
🪄 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: 66d976a6-053b-4382-9a93-7d3b04eac421
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
docs/app/charts/sankey-charts/custom-node-sankey-chart.vuedocs/app/charts/sankey-charts/simple-sankey-chart.vuedocs/content/3.charts/11.sankey.mddocs/superpowers/plans/2026-04-06-sankey-chart.mddocs/superpowers/specs/2026-04-06-sankey-chart-design.mddocs/superpowers/specs/2026-04-11-sunburst-chart-design.mdpackages/vue/package.jsonpackages/vue/src/chart/Sankey.tsxpackages/vue/src/chart/SunburstChart.tsxpackages/vue/src/chart/__stories__/Sankey.stories.tsxpackages/vue/src/chart/__stories__/SunburstChart.stories.tsxpackages/vue/src/chart/__tests__/Sankey.spec.tsxpackages/vue/src/chart/__tests__/SunburstChart.spec.tsxpackages/vue/src/chart/__tests__/sankeyUtils.spec.tspackages/vue/src/chart/__tests__/sunburstUtils.spec.tspackages/vue/src/chart/index.tspackages/vue/src/chart/sankeyUtils.tspackages/vue/src/chart/sunburstUtils.tsplayground/nuxt/app/pages/index.vueplayground/nuxt/app/pages/sankey-charts.vueplayground/nuxt/app/pages/sunburst-charts.vuetasks/todo.md
Comment on lines
+25
to
+27
| | `playground/nuxt/pages/sankey.vue` | Interactive playground page | | ||
| | `docs/content/2.charts/sankey.md` | MDC docs page | | ||
| | `tasks/todo.md` | Mark §1.2 complete | |
There was a problem hiding this comment.
Update stale file paths in the implementation plan.
Several referenced paths are outdated (e.g., playground/nuxt/pages/..., docs/content/2.charts/...), which can send follow-up work to wrong locations. Please align these with current repo paths.
Also applies to: 1059-1060, 1118-1120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-06-sankey-chart.md` around lines 25 - 27, The
plan references stale repo paths in the table rows (e.g., the strings
"playground/nuxt/pages/sankey.vue", "docs/content/2.charts/sankey.md", and
"tasks/todo.md") and the same outdated entries at the other mentioned ranges;
update each occurrence to the current repository locations used by the project
(replace the old path strings with their new equivalents across the file,
including the other occurrences at the noted ranges) so links and follow-up
tasks point to the correct files and ensure the table rows and any linked
mentions use the repo's canonical path format.
Comment on lines
+29
to
+35
| ``` | ||
| packages/vue/src/chart/ | ||
| ├── Sankey.tsx # Wrapper + Inner component (defineComponent + JSX) | ||
| ├── sankeyUtils.ts # computeSankeyLayout() — wraps d3-sankey | ||
| ├── __tests__/Sankey.spec.tsx | ||
| └── __stories__/Sankey.stories.tsx | ||
| ``` |
There was a problem hiding this comment.
Specify languages for fenced code blocks.
These fences are missing language identifiers and trigger markdownlint MD040.
📝 Suggested fix
-```
+```text
packages/vue/src/chart/
├── Sankey.tsx # Wrapper + Inner component (defineComponent + JSX)
├── sankeyUtils.ts # computeSankeyLayout() — wraps d3-sankey
├── __tests__/Sankey.spec.tsx
└── __stories__/Sankey.stories.tsx...
- +text
props.data: { nodes: SankeyNode[], links: SankeyLink[] }
│
▼
computeSankeyLayout() ──── d3-sankey ────► { nodes: LayoutNode[], links: LayoutLink[] }
│
▼
Animate (0 → 1 progress)
│
▼
├── links group (rendered first → behind nodes)
└── nodes group
Also applies to: 44-57
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-04-06-sankey-chart-design.md` around lines 29 -
35, The markdown contains fenced code blocks without language identifiers (the
package tree block and the props/flow diagram block), which triggers
markdownlint MD040; update each triple-backtick fence to include a language
(e.g., ```text) for both the block showing the package tree and the block
showing the props.data → computeSankeyLayout → Animate → Layer diagram so the
linter recognizes them as code blocks.
Comment on lines
+63
to
+72
| ``` | ||
| SunburstChart (wrapper) | ||
| ├── provideStore(createRechartsStore(sunburstOptions)) | ||
| └── SunburstInner | ||
| ├── RechartsWrapper | ||
| │ └── Surface | ||
| │ └── Layer (sectors) | ||
| │ └── Sector (per node) or #content slot | ||
| └── Tooltip (from default slot) | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced code block.
This block is missing a language token, which trips markdownlint (MD040).
📝 Suggested fix
-```
+```text
SunburstChart (wrapper)
├── provideStore(createRechartsStore(sunburstOptions))
└── SunburstInner
├── RechartsWrapper
│ └── Surface
│ └── Layer (sectors)
│ └── Sector (per node) or `#content` slot
└── Tooltip (from default slot)</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/superpowers/specs/2026-04-11-sunburst-chart-design.md around lines 63 -
72, The fenced code block showing the SunburstChart component tree is missing a
language token and triggers markdownlint MD040; update the opening fence from
totext (or another appropriate language) so it becomes text and ensure the closing fence remains — target the fenced block that contains
"SunburstChart (wrapper) ├── provideStore(createRechartsStore(sunburstOptions))
└── SunburstInner ..." to apply this change.
</details>
<!-- fingerprinting:phantom:poseidon:hawk:b35ce60b-f23a-44f0-88c6-b0dc9f74d4cb -->
<!-- This is an auto-generated comment by CodeRabbit -->
| @@ -0,0 +1,94 @@ | |||
| import type { Meta, StoryObj } from '@storybook/vue3-vite' | |||
| import { Tooltip } from '@/components/Tooltip' | |||
| import { Sankey } from '../Sankey' | |||
There was a problem hiding this comment.
Use @/ alias for internal chart imports.
Line 3 uses a relative import; this should use the project alias for consistency and path stability.
♻️ Suggested fix
-import { Sankey } from '../Sankey'
+import { Sankey } from '@/chart/Sankey'As per coding guidelines, "use @/ alias for imports from packages/vue/src/."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { Sankey } from '../Sankey' | |
| import { Sankey } from '@/chart/Sankey' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vue/src/chart/__stories__/Sankey.stories.tsx` at line 3, The import
in this story uses a relative path; replace the relative import of the Sankey
component with the project alias form (use "@/chart/Sankey") so it uses the
"@/..." alias convention used for imports from packages/vue/src; update the
import statement that references Sankey accordingly to maintain consistency and
path stability.
Comment on lines
+42
to
+47
| export const sunburstPayloadSearcher: TooltipPayloadSearcher = ( | ||
| data: unknown, | ||
| activeIndex: TooltipIndex, | ||
| ) => { | ||
| if (!data || !activeIndex) return undefined | ||
| return get(data, activeIndex) |
There was a problem hiding this comment.
Use a nullish check for activeIndex instead of a falsy check.
Line 46 (!activeIndex) will also reject 0. Safer guard is activeIndex == null.
🧰 Tools
🪛 ESLint
[error] 46-46: Expect newline after if
(antfu/if-newline)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vue/src/chart/SunburstChart.tsx` around lines 42 - 47, The guard in
sunburstPayloadSearcher incorrectly treats activeIndex === 0 as absent; change
the falsy check to a nullish check (use activeIndex == null) so zero is allowed;
keep the rest of the function and return get(data, activeIndex) only when data
and activeIndex are non-null.
| | 2026-04-06 | Sankey 桑基图（P3） | d3-sankey 布局，#node/#link slots，节点+链接双 tooltip，13 tests，5 stories | | ||
| | 2026-04-11 | SunburstChart 旭日图（P3） | d3-hierarchy partition 布局，#content slot，Sector 渲染，onClick 回调，15 tests，5 stories | | ||
|
|
||
| export https_proxy=http://127.0.0.1:7890 http_proxy=http://127.0.0.1:7890 all_proxy=socks5://127.0.0.1:7890 |
There was a problem hiding this comment.
Remove accidental environment command from task tracker.
Line 154 looks like a local shell command accidentally committed into the markdown log; it should be removed from this document.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tasks/todo.md` at line 154, Remove the accidental shell environment command
"export https_proxy=http://127.0.0.1:7890 http_proxy=http://127.0.0.1:7890
all_proxy=socks5://127.0.0.1:7890" from the markdown (it was committed into the
task tracker); locate the exact line containing that export string and delete it
so the document contains only task-related content.
rick-hup
force-pushed
the
feat/sankey-chart
branch
from
85e96a8 to
3b93c19
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/vue/src/chart/SunburstChart.tsx (1)
8-8: Replace relative import with@/alias.Line 8 should use the
@/alias pattern for internal imports.♻️ Suggested fix
-import { RechartsWrapper } from './RechartsWrapper' +import { RechartsWrapper } from '@/chart/RechartsWrapper'As per coding guidelines: "use
@/alias for imports frompackages/vue/src/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue/src/chart/SunburstChart.tsx` at line 8, Replace the relative import of RechartsWrapper with the project alias: change the import that currently references './RechartsWrapper' in SunburstChart.tsx to use the '@/...' alias (e.g., import RechartsWrapper from '@/chart/RechartsWrapper'), so the RechartsWrapper symbol is imported via the internal src alias instead of a relative path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/vue/src/chart/SunburstChart.tsx`:
- Line 8: Replace the relative import of RechartsWrapper with the project alias:
change the import that currently references './RechartsWrapper' in
SunburstChart.tsx to use the '@/...' alias (e.g., import RechartsWrapper from
'@/chart/RechartsWrapper'), so the RechartsWrapper symbol is imported via the
internal src alias instead of a relative path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf5e8250-7e45-411e-b86a-5eca496fbe2a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
docs/app/charts/sankey-charts/custom-node-sankey-chart.vuedocs/app/charts/sankey-charts/simple-sankey-chart.vuedocs/content/3.charts/11.sankey.mdpackages/vue/package.jsonpackages/vue/src/chart/Sankey.tsxpackages/vue/src/chart/SunburstChart.tsxpackages/vue/src/chart/__stories__/Sankey.stories.tsxpackages/vue/src/chart/__stories__/SunburstChart.stories.tsxpackages/vue/src/chart/__tests__/Sankey.spec.tsxpackages/vue/src/chart/__tests__/SunburstChart.spec.tsxpackages/vue/src/chart/__tests__/sankeyUtils.spec.tspackages/vue/src/chart/__tests__/sunburstUtils.spec.tspackages/vue/src/chart/index.tspackages/vue/src/chart/sankeyUtils.tspackages/vue/src/chart/sunburstUtils.tsplayground/nuxt/app/pages/index.vueplayground/nuxt/app/pages/sankey-charts.vueplayground/nuxt/app/pages/sunburst-charts.vue
✅ Files skipped from review due to trivial changes (9)
- packages/vue/package.json
- playground/nuxt/app/pages/sunburst-charts.vue
- playground/nuxt/app/pages/sankey-charts.vue
- packages/vue/src/chart/tests/sankeyUtils.spec.ts
- docs/app/charts/sankey-charts/custom-node-sankey-chart.vue
- packages/vue/src/chart/tests/Sankey.spec.tsx
- docs/content/3.charts/11.sankey.md
- docs/app/charts/sankey-charts/simple-sankey-chart.vue
- packages/vue/src/chart/tests/sunburstUtils.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/vue/src/chart/index.ts
- playground/nuxt/app/pages/index.vue
- packages/vue/src/chart/stories/Sankey.stories.tsx
- packages/vue/src/chart/stories/SunburstChart.stories.tsx
…eLabel combineActiveLabel returns `string | number | undefined` (since TickItem.value can be a number), but selectActiveLabel was annotated as `string | undefined`, causing TS2322. Also update .gitignore.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vue/src/state/selectors/selectors.ts (1)
137-156:⚠️ Potential issue | 🟠 MajorPropagate
activeLabeltype widening tocombineTooltipPayloadand remove suppressionLine 137 now returns
string | number | undefined, butcombineTooltipPayloadstill takesactiveLabel: string | undefined(packages/vue/src/state/selectors/tooltipSelectors.ts, Line 103). The mismatch is currently hidden by@ts-expect-errorat Line 144. Please update the combiner contract (and any label comparisons inside it) to accept numeric labels too, then remove the suppression.Suggested fix
# packages/vue/src/state/selectors/tooltipSelectors.ts -export function combineTooltipPayload(..., activeLabel: string | undefined, ...): TooltipPayload | undefined { +export function combineTooltipPayload(..., activeLabel: string | number | undefined, ...): TooltipPayload | undefined {# packages/vue/src/state/selectors/selectors.ts export const selectTooltipPayload: ( state: RechartsRootState, tooltipEventType: TooltipEventType, trigger: TooltipTrigger, defaultIndex: TooltipIndex | undefined, - // `@ts-expect-error` ) => TooltipPayload | undefined = createSelector(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue/src/state/selectors/selectors.ts` around lines 137 - 156, The selector types and combiner disagree: update combineTooltipPayload's parameter type for activeLabel to accept string | number | undefined (matching selectTooltipPayload's return) and revise any label comparisons inside combineTooltipPayload (in tooltipSelectors.ts) to handle numeric labels safely—e.g. coerce types consistently or use === after conversion—then remove the `@ts-expect-error` suppression in the selectTooltipPayload selector definition so TypeScript types align without error; ensure selectTooltipPayload's createSelector input order (selectActiveLabel) still matches the combiner signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/vue/src/state/selectors/selectors.ts`:
- Around line 137-156: The selector types and combiner disagree: update
combineTooltipPayload's parameter type for activeLabel to accept string | number
| undefined (matching selectTooltipPayload's return) and revise any label
comparisons inside combineTooltipPayload (in tooltipSelectors.ts) to handle
numeric labels safely—e.g. coerce types consistently or use === after
conversion—then remove the `@ts-expect-error` suppression in the
selectTooltipPayload selector definition so TypeScript types align without
error; ensure selectTooltipPayload's createSelector input order
(selectActiveLabel) still matches the combiner signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b1e5b84-08da-4125-a5eb-47929af23553
📒 Files selected for processing (2)
.gitignorepackages/vue/src/state/selectors/selectors.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
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
#node/#linkslots, dual tooltip support for nodes and links. 13 tests, 5 stories, playground demo, docs page.#contentslot for custom sector rendering, tooltip integration via Redux store. 15 tests (7 layout + 8 component), 5 stories, playground demo.sunburstPayloadSearcher/sankeyPayloadSearcherfor tooltip path traversal.Test plan
pnpm test)/sankey-chartsand/sunburst-chartsrender with tooltip interactionimport { Sankey, SunburstChart } from 'vccs'Summary by CodeRabbit
New Features
Documentation
Demos / Stories
Tests
Chores