Conversation
WalkthroughThis PR introduces a new utility module at Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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)
src/utils.js (1)
4-12: Consider adding JSDoc comments for the exported utility object.Since this is a public-facing utility module, documenting the expected usage patterns and parameter requirements would help consumers use it correctly.
📝 Add documentation
+/** + * Utility helpers for tiny-charts. + * `@property` {Object} tooltip - Tooltip utilities + * `@property` {Function} tooltip.formatter - Builds tooltip HTML from a config object. + * Note: This is NOT a direct ECharts formatter. It expects (tipConfig, tooltipOptions) + * where tipConfig = { title, children, hideEmpty, isMobile }. + * `@property` {Object} echarts - The echarts library namespace + * `@property` {Object} format - Text formatting utilities + * `@property` {Function} format.truncateText - Truncates text to fit within a given width + */ const util = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.js` around lines 4 - 12, Add JSDoc comments to the exported util object: document the util object itself and each child property (tooltip with formatter -> getTooltipContentHtmlStr, echarts, and format.truncateText) describing purpose, expected input types/return types, and usage examples; annotate that tooltip.formatter expects the same args as getTooltipContentHtmlStr, note that format.truncateText forwards to echarts.format.truncateText and specify its parameter meanings (text, options), and include `@module/`@exports tags so consumers can discover these utilities easily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils.js`:
- Around line 5-7: The current util export exposes getTooltipContentHtmlStr
which expects a pre-built tipConfig ({title, children, hideEmpty, isMobile}) but
ECharts calls tooltip.formatter with (params, ticket, callback); fix by adding a
wrapper factory named createEChartsTooltipFormatter (or similar) that returns a
function with the ECharts signature which converts the incoming params into the
tipConfig shape and then calls getTooltipContentHtmlStr, and update
util.tooltip.formatter to reference the new wrapper (or rename
getTooltipContentHtmlStr to buildTooltipHtml and export both the helper and the
wrapper); ensure the wrapper handles both single and array params and preserves
the optional async callback path so existing callers (e.g.,
ProcessChart/handleOption.js and BarChart/handleSeries.js) can still use the
helper directly if needed.
---
Nitpick comments:
In `@src/utils.js`:
- Around line 4-12: Add JSDoc comments to the exported util object: document the
util object itself and each child property (tooltip with formatter ->
getTooltipContentHtmlStr, echarts, and format.truncateText) describing purpose,
expected input types/return types, and usage examples; annotate that
tooltip.formatter expects the same args as getTooltipContentHtmlStr, note that
format.truncateText forwards to echarts.format.truncateText and specify its
parameter meanings (text, options), and include `@module/`@exports tags so
consumers can discover these utilities easily.
| tooltip: { | ||
| formatter: getTooltipContentHtmlStr | ||
| }, |
There was a problem hiding this comment.
Function signature mismatch: getTooltipContentHtmlStr cannot be used directly as an ECharts formatter.
The exposed getTooltipContentHtmlStr expects (tipConfig, tooltip) where tipConfig is a pre-constructed object with { title, children, hideEmpty, isMobile }. However, ECharts calls the formatter callback with (params, ticket, callback).
If a user assigns util.tooltip.formatter directly to their chart's tooltip.formatter, it will receive raw ECharts params instead of the expected config object, causing incorrect output or errors.
Looking at existing usages (e.g., ProcessChart/handleOption.js:172, BarChart/handleSeries.js:684), all properly wrap this function to transform ECharts params before calling it.
Consider either:
- Renaming to clarify it's a helper, not a drop-in formatter (e.g.,
buildTooltipHtml) - Providing a wrapper that handles the ECharts signature
- Adding documentation explaining the required transformation
Option 1: Rename to clarify usage
const util = {
tooltip: {
- formatter: getTooltipContentHtmlStr
+ buildTooltipHtml: getTooltipContentHtmlStr
},Option 2: Provide a factory function
+// Factory that creates an ECharts-compatible formatter
+function createTooltipFormatter(options = {}) {
+ return (params, ticket, callback) => {
+ const config = {
+ title: Array.isArray(params) ? params[0]?.name : params?.name,
+ children: [],
+ hideEmpty: options.hideEmpty,
+ isMobile: options.isMobile
+ };
+ // User should customize children population based on their data
+ return getTooltipContentHtmlStr(config, options);
+ };
+}
+
const util = {
tooltip: {
- formatter: getTooltipContentHtmlStr
+ buildTooltipHtml: getTooltipContentHtmlStr,
+ createFormatter: createTooltipFormatter
},📝 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.
| tooltip: { | |
| formatter: getTooltipContentHtmlStr | |
| }, | |
| tooltip: { | |
| buildTooltipHtml: getTooltipContentHtmlStr | |
| }, |
| tooltip: { | |
| formatter: getTooltipContentHtmlStr | |
| }, | |
| // Factory that creates an ECharts-compatible formatter | |
| function createTooltipFormatter(options = {}) { | |
| return (params, ticket, callback) => { | |
| const config = { | |
| title: Array.isArray(params) ? params[0]?.name : params?.name, | |
| children: [], | |
| hideEmpty: options.hideEmpty, | |
| isMobile: options.isMobile | |
| }; | |
| // User should customize children population based on their data | |
| return getTooltipContentHtmlStr(config, options); | |
| }; | |
| } | |
| const util = { | |
| tooltip: { | |
| buildTooltipHtml: getTooltipContentHtmlStr, | |
| createFormatter: createTooltipFormatter | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils.js` around lines 5 - 7, The current util export exposes
getTooltipContentHtmlStr which expects a pre-built tipConfig ({title, children,
hideEmpty, isMobile}) but ECharts calls tooltip.formatter with (params, ticket,
callback); fix by adding a wrapper factory named createEChartsTooltipFormatter
(or similar) that returns a function with the ECharts signature which converts
the incoming params into the tipConfig shape and then calls
getTooltipContentHtmlStr, and update util.tooltip.formatter to reference the new
wrapper (or rename getTooltipContentHtmlStr to buildTooltipHtml and export both
the helper and the wrapper); ensure the wrapper handles both single and array
params and preserves the optional async callback path so existing callers (e.g.,
ProcessChart/handleOption.js and BarChart/handleSeries.js) can still use the
helper directly if needed.
1 participant
Summary by CodeRabbit