chore: code quality housekeeping — remove deprecated props, fix types - #612
Merged
Conversation
The legacy colorThresholds prop (JSON string) was replaced by stylingRules (structured StylingRule[]) in v0.8. The migration happens at the card-container level via resolveStylingConfig(), so the backward-compat code in chart components was dead weight. Removed from: - BarChart, LineChart, PieChart, SingleValueChart (prop + parsing) - resolveItemColor() — removed unused thresholds parameter - chart-utils.ts — removed ColorThreshold/resolveThresholdColor imports - All 4 plugin components — removed colorThresholds pass-through - PluginProps interface — removed colorThresholds field Tests: - Converted 3 SingleValueChart threshold tests to use stylingRules - Removed 1 test for invalid JSON colorThresholds (prop no longer exists) The card-container still reads chartOptions.colorThresholds for migration purposes — old dashboards are auto-upgraded on load. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ing type sunburst-chart.tsx: - Replace \`sort: sortFn as any\` with proper typed \`undefined\` for no-sort - Change null to undefined for ECharts compatibility chart-plugin-registry.ts: - Keep \`any\` for transformWithMapping mapping parameter but add documentation explaining it's due to cross-package type boundary (ColumnMapping lives in component/, registry in app/) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThis PR removes the deprecated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
component/src/charts/chart-utils.ts (1)
342-355:⚠️ Potential issue | 🟡 MinorStale doc comment — drop the "legacy color thresholds" mention.
The JSDoc still advertises "legacy color thresholds as fallback", but that path is gone. Trim it so the contract matches the implementation.
📝 Proposed fix
/** - * Resolve a color for a numeric value using styling rules (preferred) or - * legacy color thresholds as fallback. Returns undefined when no rule matches. + * Resolve a color for a numeric value using styling rules. + * Returns undefined when no rule matches or no rules are provided. */ export function resolveItemColor( value: number, stylingRules: StylingRule[] | undefined, paramValues?: Record<string, unknown>, ): string | undefined {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/charts/chart-utils.ts` around lines 342 - 355, Update the JSDoc for resolveItemColor to remove the mention of "legacy color thresholds as fallback" so the comment matches the implementation; specifically edit the comment above the function resolveItemColor (and adjust any wording referencing fallback behavior) to state that it resolves a color using styling rules and returns undefined when no rule matches, referencing resolveStylingRuleColor if needed.
🧹 Nitpick comments (2)
component/src/charts/single-value-chart.tsx (1)
128-128: Drop the redundant alias.
thresholdColoris justtextColorunder a stale name. Inline it at the use site to keep the cleanup honest.♻️ Proposed fix
- const thresholdColor = textColor; - const trendColor =- style={{ color: thresholdColor ?? autoContrast ?? undefined }} + style={{ color: textColor ?? autoContrast ?? undefined }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/charts/single-value-chart.tsx` at line 128, Remove the redundant alias declaration "const thresholdColor = textColor;" and replace all uses of thresholdColor within the SingleValueChart component (or surrounding scope) with textColor directly; ensure no other logic depends on thresholdColor and delete the declaration to avoid the stale name.component/src/charts/sunburst-chart.tsx (1)
147-147: Remove redundant type assertion on line 147.
sortFnis already inferred as"asc" | "desc" | undefinedfrom lines 59–60, so the explicit cast adds no type safety and masks future type changes. Drop it.Proposed change
- sort: sortFn as "desc" | "asc" | undefined, + sort: sortFn,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/charts/sunburst-chart.tsx` at line 147, The property assignment uses a redundant type assertion: remove the "as \"desc\" | \"asc\" | undefined" cast and just pass sortFn directly (i.e., change sort: sortFn as ... to sort: sortFn) because sortFn is already inferred as "asc" | "desc" | undefined (see the variable declaration around lines 59–60); update the object where sort is set (the place using sort: ...) and run the TypeScript check to ensure no further casts are needed.
🤖 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 `@component/src/charts/chart-utils.ts`:
- Around line 342-355: Update the JSDoc for resolveItemColor to remove the
mention of "legacy color thresholds as fallback" so the comment matches the
implementation; specifically edit the comment above the function
resolveItemColor (and adjust any wording referencing fallback behavior) to state
that it resolves a color using styling rules and returns undefined when no rule
matches, referencing resolveStylingRuleColor if needed.
---
Nitpick comments:
In `@component/src/charts/single-value-chart.tsx`:
- Line 128: Remove the redundant alias declaration "const thresholdColor =
textColor;" and replace all uses of thresholdColor within the SingleValueChart
component (or surrounding scope) with textColor directly; ensure no other logic
depends on thresholdColor and delete the declaration to avoid the stale name.
In `@component/src/charts/sunburst-chart.tsx`:
- Line 147: The property assignment uses a redundant type assertion: remove the
"as \"desc\" | \"asc\" | undefined" cast and just pass sortFn directly (i.e.,
change sort: sortFn as ... to sort: sortFn) because sortFn is already inferred
as "asc" | "desc" | undefined (see the variable declaration around lines 59–60);
update the object where sort is set (the place using sort: ...) and run the
TypeScript check to ensure no further casts are needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 56553b7e-1010-4a38-aa0b-be2cbcb89e31
📒 Files selected for processing (13)
app/src/lib/plugin/chart-plugin-registry.tsapp/src/plugins/bar/component.tsxapp/src/plugins/line/component.tsxapp/src/plugins/pie/component.tsxapp/src/plugins/single-value/component.tsxapp/src/plugins/utils.tscomponent/src/charts/__tests__/single-value-chart.test.tsxcomponent/src/charts/bar-chart.tsxcomponent/src/charts/chart-utils.tscomponent/src/charts/line-chart.tsxcomponent/src/charts/pie-chart.tsxcomponent/src/charts/single-value-chart.tsxcomponent/src/charts/sunburst-chart.tsx
💤 Files with no reviewable changes (5)
- app/src/plugins/utils.ts
- app/src/plugins/single-value/component.tsx
- app/src/plugins/line/component.tsx
- app/src/plugins/pie/component.tsx
- app/src/plugins/bar/component.tsx
|
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
2 participants
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
Tech debt cleanup across the codebase.
Deprecated colorThresholds removal
colorThresholdsprop from BarChart, LineChart, PieChart, SingleValueChartcolorThresholdsfrom PluginProps interface and all 4 plugin componentsthresholdsparameter fromresolveItemColor()(was unused by all callers)parseColorThresholdsandresolveThresholdColorimports from chart-utilsresolveStylingConfig) is preserved for old dashboardsType safety improvements
as anycast in sunburst sort function (useundefinedinstead ofnull)anyin chart-plugin-registrytransformWithMapping(cross-package boundary)Impact
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor
colorThresholdsconfiguration from charts; usestylingRulesfor all styling insteadImprovements
Documentation