feat(ava/advisor): use gpt-vis to render chart #812
Conversation
Summary of ChangesHello @leondt1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the chart advising functionality by integrating the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature to render charts using @antv/gpt-vis. The changes are extensive, touching the advisor pipeline, data types, and adding new rendering logic with React components. Overall, the direction is good, but there are several issues to address. I've found a bug in the pipeline logic, some unused code, and a few areas where the new rendering API is either misleading or incomplete. Specifically, the handling of rendering parameters like the container and UI configurations needs to be improved for a robust and clear API. I've also pointed out a minor code style issue for better maintainability.
| type RenderChartParams = { encode: AdviseChart['encode']; data: Data; metasMap: Record<string, Meta> }; | ||
|
|
||
| const getTrendOrDistributionSpec = <T extends keyof DataTypeMap>( | ||
| params: RenderChartParams & { | ||
| category: T; | ||
| } | ||
| ) => { | ||
| const { encode, data, metasMap, category } = params; | ||
| const newEncode = ENCODE_TO_GPT_VIS_ENCODE[category]; | ||
| const xFieldKey = encode.x[0]; | ||
| const yFieldKey = encode.y[0]; | ||
| const sFieldKey = encode.s?.[0]; | ||
| const newXFieldKey = newEncode.x; | ||
| const newYFieldKey = newEncode.y; | ||
| const newSFieldKey = newEncode.s; | ||
| const axisXTitle = metasMap[xFieldKey]?.name; | ||
| const axisYTitle = metasMap[yFieldKey]?.name; | ||
| const newData = data.map((item) => { | ||
| return { | ||
| [newXFieldKey]: item[xFieldKey], | ||
| [newYFieldKey]: item[yFieldKey], | ||
| ...(newSFieldKey && sFieldKey ? { [newSFieldKey]: item[sFieldKey] } : {}), | ||
| }; | ||
| }) as DataTypeMap[T]; | ||
|
|
||
| return { | ||
| data: newData, | ||
| axisXTitle, | ||
| axisYTitle, | ||
| }; | ||
| }; | ||
|
|
||
| export const CHART_RENDER_MAP = { | ||
| [CHART_NAME.line]: (params: RenderChartParams) => { | ||
| const { data, axisXTitle, axisYTitle } = getTrendOrDistributionSpec<'TREND'>({ ...params, category: 'TREND' }); | ||
| return ( | ||
| <Line | ||
| data={data} | ||
| axisXTitle={axisXTitle} | ||
| axisYTitle={axisYTitle} | ||
| theme="default" | ||
| containerStyle={{ height: 300 }} | ||
| style={{ | ||
| lineWidth: 2, | ||
| backgroundColor: '#fff', | ||
| }} | ||
| /> | ||
| ); | ||
| }, | ||
| [CHART_NAME.area]: (params: RenderChartParams) => { | ||
| const { data, axisXTitle, axisYTitle } = getTrendOrDistributionSpec<'TREND'>({ ...params, category: 'TREND' }); | ||
| return ( | ||
| <Area | ||
| data={data} | ||
| axisXTitle={axisXTitle} | ||
| axisYTitle={axisYTitle} | ||
| theme="default" | ||
| containerStyle={{ height: 300 }} | ||
| style={{ | ||
| lineWidth: 2, | ||
| backgroundColor: '#fff', | ||
| }} | ||
| /> | ||
| ); | ||
| }, | ||
| [CHART_NAME.column]: (params: RenderChartParams) => { | ||
| const { data, axisXTitle, axisYTitle } = getTrendOrDistributionSpec<'DISTRIBUTION'>({ | ||
| ...params, | ||
| category: 'DISTRIBUTION', | ||
| }); | ||
| return ( | ||
| <Column | ||
| data={data} | ||
| stack={false} | ||
| group | ||
| axisXTitle={axisXTitle} | ||
| axisYTitle={axisYTitle} | ||
| theme="default" | ||
| containerStyle={{ height: 300 }} | ||
| style={{ | ||
| backgroundColor: '#fff', | ||
| }} | ||
| /> | ||
| ); | ||
| }, | ||
| [CHART_NAME.bar]: (params: RenderChartParams) => { | ||
| const { data, axisXTitle, axisYTitle } = getTrendOrDistributionSpec<'DISTRIBUTION'>({ | ||
| ...params, | ||
| category: 'DISTRIBUTION', | ||
| }); | ||
| return ( | ||
| <Bar | ||
| data={data} | ||
| stack={false} | ||
| group | ||
| axisXTitle={axisXTitle} | ||
| axisYTitle={axisYTitle} | ||
| theme="default" | ||
| containerStyle={{ height: 300 }} | ||
| style={{ | ||
| backgroundColor: '#fff', | ||
| }} | ||
| /> | ||
| ); | ||
| }, | ||
| }; | ||
|
|
||
| export const renderChart = (params: RenderParams) => { | ||
| const { chartConfig, data, metas } = params; | ||
| const { type, encode } = chartConfig; | ||
| const metasMap = transMetasToMap(metas); | ||
| const render = CHART_RENDER_MAP[type]; | ||
| return render({ | ||
| encode, | ||
| data, | ||
| metasMap, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
The uiConfig property from RenderParams (which includes width, height, and palette) is being ignored when rendering charts. Instead, properties like height are hardcoded (e.g., containerStyle={{ height: 300 }}), which prevents users from customizing the chart's appearance.
To fix this, you should:
- Update
RenderChartParamsto includeuiConfig. - Update
renderChartto passuiConfigto the chart-specific render functions. - Update the functions in
CHART_RENDER_MAPto use thewidthandheightfromuiConfig.
type RenderChartParams = { encode: AdviseChart['encode']; data: Data; metasMap: Record<string, Meta>, uiConfig?: RenderParams['uiConfig'] };
const getTrendOrDistributionSpec = <T extends keyof DataTypeMap>(
params: RenderChartParams & {
category: T;
}
) => {
const { encode, data, metasMap, category } = params;
const newEncode = ENCODE_TO_GPT_VIS_ENCODE[category];
const xFieldKey = encode.x[0];
const yFieldKey = encode.y[0];
const sFieldKey = encode.s?.[0];
const newXFieldKey = newEncode.x;
const newYFieldKey = newEncode.y;
const newSFieldKey = newEncode.s;
const axisXTitle = metasMap[xFieldKey]?.name;
const axisYTitle = metasMap[yFieldKey]?.name;
const newData = data.map((item) => {
return {
[newXFieldKey]: item[xFieldKey],
[newYFieldKey]: item[yFieldKey],
...(newSFieldKey && sFieldKey ? { [newSFieldKey]: item[sFieldKey] } : {}),
};
}) as DataTypeMap[T];
return {
data: newData,
axisXTitle,
axisYTitle,
};
};
export const CHART_RENDER_MAP = {
[CHART_NAME.line]: (params: RenderChartParams) => {
const { data, axisXTitle, axisYTitle } = getTrendOrDistributionSpec<'TREND'>({ ...params, category: 'TREND' });
return (
<Line
data={data}
axisXTitle={axisXTitle}
axisYTitle={axisYTitle}
theme="default"
containerStyle={{ height: params.uiConfig?.height ?? 300, width: params.uiConfig?.width }}
style={{
lineWidth: 2,
backgroundColor: '#fff',
}}
/>
);
},
[CHART_NAME.area]: (params: RenderChartParams) => {
const { data, axisXTitle, axisYTitle } = getTrendOrDistributionSpec<'TREND'>({ ...params, category: 'TREND' });
return (
<Area
data={data}
axisXTitle={axisXTitle}
axisYTitle={axisYTitle}
theme="default"
containerStyle={{ height: params.uiConfig?.height ?? 300, width: params.uiConfig?.width }}
style={{
lineWidth: 2,
backgroundColor: '#fff',
}}
/>
);
},
[CHART_NAME.column]: (params: RenderChartParams) => {
const { data, axisXTitle, axisYTitle } = getTrendOrDistributionSpec<'DISTRIBUTION'>({
...params,
category: 'DISTRIBUTION',
});
return (
<Column
data={data}
stack={false}
group
axisXTitle={axisXTitle}
axisYTitle={axisYTitle}
theme="default"
containerStyle={{ height: params.uiConfig?.height ?? 300, width: params.uiConfig?.width }}
style={{
backgroundColor: '#fff',
}}
/>
);
},
[CHART_NAME.bar]: (params: RenderChartParams) => {
const { data, axisXTitle, axisYTitle } = getTrendOrDistributionSpec<'DISTRIBUTION'>({
...params,
category: 'DISTRIBUTION',
});
return (
<Bar
data={data}
stack={false}
group
axisXTitle={axisXTitle}
axisYTitle={axisYTitle}
theme="default"
containerStyle={{ height: params.uiConfig?.height ?? 300, width: params.uiConfig?.width }}
style={{
backgroundColor: '#fff',
}}
/>
);
},
};
export const renderChart = (params: RenderParams) => {
const { chartConfig, data, metas, uiConfig } = params;
const { type, encode } = chartConfig;
const metasMap = transMetasToMap(metas);
const render = CHART_RENDER_MAP[type];
return render({
encode,
data,
metasMap,
uiConfig,
});
};
| /** render DOM container */ | ||
| container: HTMLElement; |
There was a problem hiding this comment.
The RenderParams interface includes a container: HTMLElement property, which implies that the render method will render the chart into this DOM element. However, the current implementation of renderChart in render.tsx does not use this property and instead returns a React element. This is misleading for users of the API.
To fix this, you could either:
- Implement the rendering logic inside
renderChartusingreact-domto mount the component into the providedcontainer. - Adjust the API to reflect its behavior. This would involve removing the
containerproperty and perhaps renaming therendermethod to something likegetChartComponentto clarify that it returns a renderable component, not that it performs the rendering itself.
Given this is a library, the second option is often preferred as it gives consumers more control. If you choose this path, the container property should be removed.
| '@typescript-eslint/camelcase': 0, | ||
| '@typescript-eslint/no-explicit-any': 0, | ||
| '@typescript-eslint/type-annotation-spacing': 0, | ||
| 'no-plusplus': 0, |
There was a problem hiding this comment.
The no-plusplus rule is defined at line 29 and then removed here. While this has no functional impact since it was already disabled, it's good practice to keep the ESLint configuration clean. This change is fine, but you could also remove the initial declaration on line 29 in a follow-up to avoid redundancy.
| let _llmCompleted = false; | ||
| let _llmCostTime = ''; |
There was a problem hiding this comment.
These variables, _llmCompleted and _llmCostTime, are assigned but their values are never used. It appears they are remnants from a previous implementation where they were part of the result. To improve code clarity and remove dead code, they should be deleted.
|
|
||
| export interface AdviseChart { | ||
| /** chart type */ | ||
| chartType: string; |
There was a problem hiding this comment.
The property chartType has been renamed to type. This is a good change for consistency. However, type is a very generic name and can sometimes conflict with other properties or keywords in TypeScript/JavaScript. Consider if a more specific name like chartType (as it was before) or chartName would be clearer and less prone to conflicts, especially in a complex interface.
2 participants
PR includes