Threshold percentage on labels for pie charts #1187
Conversation
Ishankoradia
changed the title
WalkthroughModified Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 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: 1
🧹 Nitpick comments (1)
ddpui/core/charts/echarts_config_generator.py (1)
211-211: Consider adding validation for the label threshold value.The
label_thresholdis retrieved from customizations without validation. If a non-numeric, negative, or out-of-range value (e.g., > 100 for percentage) is provided, it could cause unexpected behavior in the frontend.Consider adding validation:
- label_threshold = customizations.get("labelThreshold", 5) # Default 5% threshold + label_threshold = customizations.get("labelThreshold", 5) # Default 5% threshold + # Validate threshold is numeric and within reasonable range + if not isinstance(label_threshold, (int, float)) or label_threshold < 0 or label_threshold > 100: + label_threshold = 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ddpui/core/charts/echarts_config_generator.py(2 hunks)
| # Pass threshold as a custom property for frontend processing | ||
| "labelThreshold": label_threshold, | ||
| "labelFormat": label_format, |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify the purpose of passing both labelFormat and formatter.
The code passes both labelFormat (the raw format string like "percentage") and formatter (the mapped ECharts format like "{d}%"). Since the comment indicates these are "custom properties for frontend processing," it would be helpful to clarify:
- Are
labelThresholdandlabelFormatintended as non-standard properties for custom frontend logic? - Why is
labelFormatneeded whenformatteralready provides the formatted output?
If these are indeed custom properties, the frontend will need to handle them explicitly. Otherwise, ECharts will ignore them.
🏁 Script executed:
#!/bin/bash
# Search for frontend code that processes these custom properties
rg -n --type=js --type=ts -C3 'labelThreshold|labelFormat' -g '!**/node_modules/**' -g '!**/dist/**'
Length of output: 148
---
🏁 Script executed:
```shell
#!/bin/bash
# View the context around the lines in question
cat ddpui/core/charts/echarts_config_generator.py | sed -n '270,290p'
Length of output: 938
🏁 Script executed:
#!/bin/bash
# Search more broadly without type filters for frontend references
rg -n 'labelThreshold|labelFormat' -g '!**/node_modules/**' -C2
Length of output: 1830
🏁 Script executed:
#!/bin/bash
# Look for where this config is used or sent to frontend
rg -n 'echarts_config_generator|to_json|serialize' -g '!**/node_modules/**' | head -20
Length of output: 1857
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the function containing these lines (look around line 280)
sed -n '150,290p' ddpui/core/charts/echarts_config_generator.py | head -150
Length of output: 6135
🏁 Script executed:
#!/bin/bash
# Find all function definitions in this file
rg -n 'def ' ddpui/core/charts/echarts_config_generator.py
Length of output: 609
🏁 Script executed:
#!/bin/bash
# Search for where echarts_config_generator functions are imported/called
rg -n 'from.*echarts_config_generator|import.*echarts_config_generator' -g '!**/node_modules/**'
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Look for API views or serializers that might return this config
rg -n 'echarts_config|chart.*config' -g '!**/node_modules/**' -C1 | grep -E '\.(py|js|ts):' | head -20
Length of output: 2071
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the charts_api.py file to see how config is used
cat ddpui/api/charts_api.py | head -100
Length of output: 3454
🏁 Script executed:
#!/bin/bash
# Search for all usages of generate_pie_config
rg -n 'generate_pie_config' -g '!**/node_modules/**' -C3
Length of output: 1170
🏁 Script executed:
#!/bin/bash
# Look for any API endpoints that return echarts_config or pie configuration
rg -n '@.*route|def.*view|def.*api' ddpui/api/charts_api.py -A5 | head -50
Length of output: 2245
🏁 Script executed:
#!/bin/bash
# Check if labelThreshold or labelFormat are referenced anywhere else beyond definition/assignment
rg -n 'labelThreshold|labelFormat' -g '!**/node_modules/**' --type=py
Length of output: 472
Remove or clarify the purpose of labelThreshold and labelFormat custom properties.
Verification confirms no frontend code processes these custom properties. They are only defined in the backend (lines 281-282) and never referenced elsewhere in the codebase. When placed inside ECharts' label object, these non-standard properties will be ignored by ECharts.
Either:
- Remove
labelThresholdandlabelFormatif they're unused - Move them outside the
labelobject if intended for custom frontend processing, and implement corresponding frontend logic - Document why they're passed if they serve a specific purpose
🤖 Prompt for AI Agents
In ddpui/core/charts/echarts_config_generator.py around lines 280 to 282, the
custom properties "labelThreshold" and "labelFormat" are being inserted inside
the ECharts "label" object but are not consumed by frontend code and will be
ignored by ECharts; either remove these two properties from the generated config
if they are unused, or move them out of the "label" object into a top-level or
custom namespace in the config (and add corresponding frontend handling), or add
an inline code comment/docstring explaining their intended purpose and
where/when frontend logic will read them so their presence is justified.
Codecov Report❌ Patch coverage is
Additional details and impacted files☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit