Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make themeVariables typesafe #5631

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
+386 −32
Open

fix: make themeVariables typesafe #5631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1061231
fix: make themeVariables typesafe
ad1992 Jul 8, 2024
18278dc
make themeVars optional
ad1992 Jul 8, 2024
fe41f1b
use nullish coalescing operator
ad1992 Jul 8, 2024
c89f1ea
pretty fix
ad1992 Jul 8, 2024
b6fa365
update config-schema.yaml
ad1992 Jul 9, 2024
33eaeb2
add title to schema
ad1992 Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 200 additions & 1 deletion packages/mermaid/src/config.type.ts
View file
Open in desktop
ad1992 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export interface MermaidConfig {
*
*/
theme?: 'default' | 'forest' | 'dark' | 'neutral' | 'null';
themeVariables?: any;
themeVariables?: ThemeVariables;
themeCSS?: string;
/**
* The maximum allowed size of the users text diagram
Expand Down Expand Up @@ -171,6 +171,205 @@ export interface MermaidConfig {
*/
suppressErrorRendering?: boolean;
}
/**
* Theme variables to create a custom theme.
*
*/
export interface ThemeVariables {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bito Code Review Agent Run #e7aeaa - 08/12/2024, 02:50 pm

🔴 High importance
Issue: The 'ThemeVariables' interface does not enforce any constraints on the values of its properties. This could lead to invalid or inconsistent theme configurations.
Fix: Add validation logic to ensure that the values of the properties in the 'ThemeVariables' interface are valid. For example, ensure that color properties are valid CSS color strings and that font size and weight are within acceptable ranges.
Code suggestion 
 @@ -178,7 +178,7 @@
  export interface ThemeVariables {
 +  fontFamily?: string;
 +  fontSize?: number;
 +  fontWeight?: number;
 +  primaryColor?: string;
 +  primaryTextColor?: string;
 +  noteBkgColor?: string;
 +  noteTextColor?: string;
 +  pie1?: string;
 +  pie2?: string;
 +  pie3?: string;
 +  pie4?: string;
 +  pie5?: string;
 +  pie6?: string;
 +  pie7?: string;
 +  pie8?: string;
 +  pie9?: string;
 +  pie10?: string;
 +  pie11?: string;
 +  pie12?: string;
 +  pieOuterStrokeWidth?: number;
 +  quadrant1Fill?: string;
 +  quadrant2Fill?: string;
 +  quadrant3Fill?: string;
 +  quadrant4Fill?: string;
 +  quadrant1TextFill?: string;
 +  quadrant2TextFill?: string;
 +  quadrant3TextFill?: string;
 +  quadrant4TextFill?: string;
 +  quadrantPointFill?: string;
 +  quadrantPointTextFill?: string;
 +  quadrantXAxisTextFill?: string;
 +  quadrantYAxisTextFill?: string;
 +  quadrantInternalBorderStrokeFill?: string;
 +  quadrantExternalBorderStrokeFill?: string;
 +  quadrantTitleFill?: string;
 +  xyChart?: {
 +    backgroundColor?: string;
 +    titleColor?: string;
 +    xAxisTitleColor?: string;
 +    xAxisLabelColor?: string;
 +    xAxisTickColor?: string;
 +    xAxisLineColor?: string;
 +    yAxisTitleColor?: string;
 +  };
 +}
 +
 +function validateThemeVariables(theme: ThemeVariables): void {
 +  if (theme.fontSize && (theme.fontSize < 8 || theme.fontSize > 72)) {
 +    throw new Error('Font size must be between 8 and 72.');
 +  }
 +  if (theme.fontWeight && (theme.fontWeight < 100 || theme.fontWeight > 900)) {
 +    throw new Error('Font weight must be between 100 and 900.');
 +  }
 +  const colorProperties = [
 +    theme.primaryColor,
 +    theme.primaryTextColor,
 +    theme.noteBkgColor,
 +    theme.noteTextColor,
 +    theme.pie1,
 +    theme.pie2,
 +    theme.pie3,
 +    theme.pie4,
 +    theme.pie5,
 +    theme.pie6,
 +    theme.pie7,
 +    theme.pie8,
 +    theme.pie9,
 +    theme.pie10,
 +    theme.pie11,
 +    theme.pie12,
 +    theme.quadrant1Fill,
 +    theme.quadrant2Fill,
 +    theme.quadrant3Fill,
 +    theme.quadrant4Fill,
 +    theme.quadrant1TextFill,
 +    theme.quadrant2TextFill,
 +    theme.quadrant3TextFill,
 +    theme.quadrant4TextFill,
 +    theme.quadrantPointFill,
 +    theme.quadrantPointTextFill,
 +    theme.quadrantXAxisTextFill,
 +    theme.quadrantYAxisTextFill,
 +    theme.quadrantInternalBorderStrokeFill,
 +    theme.quadrantExternalBorderStrokeFill,
 +    theme.quadrantTitleFill,
 +    theme.xyChart?.backgroundColor,
 +    theme.xyChart?.titleColor,
 +    theme.xyChart?.xAxisTitleColor,
 +    theme.xyChart?.xAxisLabelColor,
 +    theme.xyChart?.xAxisTickColor,
 +    theme.xyChart?.xAxisLineColor,
 +    theme.xyChart?.yAxisTitleColor,
 +  ];
 +  colorProperties.forEach(color => {
 +    if (color && !/^#[0-9A-F]{6}$/i.test(color) && !/^rgb/.test(color) && !/^hsl/.test(color)) {
 +      throw new Error('Invalid color value: ${color}');
 +    }
 +  });
 +}

Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

/**
* The CSS `font-family` to use.
*/
fontFamily?: string;
/**
* The font size to use.
*/
fontSize?: number;
/**
* The font weight to use.
*/
fontWeight?: number;
/**
* The primary color to use.
*/
primaryColor?: string;
/**
* The primary text color to use.
*/
primaryTextColor?: string;
/**
* The note background color to use.
*/
noteBkgColor?: string;
/**
* The note text color to use.
*/
noteTextColor?: string;
/**
* The color to use for the first pie chart color.
*/
pie1?: string;
/**
* The color to use for the second pie chart color.
*/
pie2?: string;
/**
* The color to use for the third pie chart color.
*/
pie3?: string;
/**
* The color to use for the fourth pie chart color.
*/
pie4?: string;
/**
* The color to use for the fifth pie chart color.
*/
pie5?: string;
/**
* The color to use for the sixth pie chart color.
*/
pie6?: string;
/**
* The color to use for the seventh pie chart color.
*/
pie7?: string;
/**
* The color to use for the eighth pie chart color.
*/
pie8?: string;
/**
* The color to use for the ninth pie chart color.
*/
pie9?: string;
/**
* The color to use for the tenth pie chart color.
*/
pie10?: string;
/**
* The color to use for the eleventh pie chart color.
*/
pie11?: string;
/**
* The color to use for the twelfth pie chart color.
*/
pie12?: string;
/**
* The width of the stroke around the pie chart.
*/
pieOuterStrokeWidth?: number;
/**
* The color to use for the first quadrant fill.
*/
quadrant1Fill?: string;
/**
* The color to use for the second quadrant fill.
*/
quadrant2Fill?: string;
/**
* The color to use for the third quadrant fill.
*/
quadrant3Fill?: string;
/**
* The color to use for the fourth quadrant fill.
*/
quadrant4Fill?: string;
/**
* The color to use for the first quadrant text fill.
*/
quadrant1TextFill?: string;
/**
* The color to use for the second quadrant text fill.
*/
quadrant2TextFill?: string;
/**
* The color to use for the third quadrant text fill.
*/
quadrant3TextFill?: string;
/**
* The color to use for the fourth quadrant text fill.
*/
quadrant4TextFill?: string;
/**
* The color to use for the quadrant point fill.
*/
quadrantPointFill?: string;
/**
* The color to use for the quadrant point text fill.
*/
quadrantPointTextFill?: string;
/**
* The color to use for the quadrant x-axis text fill.
*/
quadrantXAxisTextFill?: string;
/**
* The color to use for the quadrant y-axis text fill.
*/
quadrantYAxisTextFill?: string;
/**
* The color to use for the quadrant internal border stroke fill.
*/
quadrantInternalBorderStrokeFill?: string;
/**
* The color to use for the quadrant external border stroke fill.
*/
quadrantExternalBorderStrokeFill?: string;
/**
* The color to use for the quadrant title fill.
*/
quadrantTitleFill?: string;
/**
* The color to use for the xy chart.
*/
xyChart?: {
/**
* The color to use for the xy chart background.
*/
backgroundColor?: string;
/**
* The color to use for the xy chart title.
*/
titleColor?: string;
/**
* The color to use for the xy chart x-axis title.
*/
xAxisTitleColor?: string;
/**
* The color to use for the xy chart x-axis label.
*/
xAxisLabelColor?: string;
/**
* The color to use for the xy chart x-axis tick.
*/
xAxisTickColor?: string;
/**
* The color to use for the xy chart x-axis line.
*/
xAxisLineColor?: string;
/**
* The color to use for the xy chart y-axis title.
*/
yAxisTitleColor?: string;
/**
* The color to use for the xy chart y-axis label.
*/
yAxisLabelColor?: string;
/**
* The color to use for the xy chart y-axis tick.
*/
yAxisTickColor?: string;
/**
* The color to use for the xy chart y-axis line.
*/
yAxisLineColor?: string;
/**
* The color to use for the xy chart plot color palette.
*/
plotColorPalette?: string;
};
/**
* The maximum number of colors allowed in the theme.
*/
THEME_COLOR_LIMIT?: number;
}
/**
* The object containing configurations specific for flowcharts
*
Expand Down
28 changes: 14 additions & 14 deletions packages/mermaid/src/diagrams/pie/pieRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export const draw: DrawDefinition = (text, id, _version, diagObj) => {
group.attr('transform', 'translate(' + pieWidth / 2 + ',' + height / 2 + ')');

const { themeVariables } = globalConfig;
let [outerStrokeWidth] = parseFontSize(themeVariables.pieOuterStrokeWidth);
let [outerStrokeWidth] = parseFontSize(themeVariables?.pieOuterStrokeWidth);
outerStrokeWidth ??= 2;

const textPosition: number = pieConfig.textPosition;
Expand All @@ -76,21 +76,21 @@ export const draw: DrawDefinition = (text, id, _version, diagObj) => {
const arcs: d3.PieArcDatum<D3Section>[] = createPieArcs(sections);

const myGeneratedColors = [
themeVariables.pie1,
themeVariables.pie2,
themeVariables.pie3,
themeVariables.pie4,
themeVariables.pie5,
themeVariables.pie6,
themeVariables.pie7,
themeVariables.pie8,
themeVariables.pie9,
themeVariables.pie10,
themeVariables.pie11,
themeVariables.pie12,
themeVariables?.pie1 ?? null,
themeVariables?.pie2 ?? null,
themeVariables?.pie3 ?? null,
themeVariables?.pie4 ?? null,
themeVariables?.pie5 ?? null,
themeVariables?.pie6 ?? null,
themeVariables?.pie7 ?? null,
themeVariables?.pie8 ?? null,
themeVariables?.pie9 ?? null,
themeVariables?.pie10 ?? null,
themeVariables?.pie11 ?? null,
themeVariables?.pie12 ?? null,
Comment on lines +79 to +90
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bito Code Review Agent Run #e7aeaa - 08/12/2024, 02:50 pm

🔴 High importance
Issue: The use of nullish coalescing with 'null' as a fallback value can lead to 'null' values being passed to the 'scaleOrdinal' function, which might not handle 'null' values correctly and could result in runtime errors or unexpected behavior.
Fix: Instead of using 'null' as a fallback, use a default color value to ensure that the 'scaleOrdinal' function always receives valid color values.
Code suggestion 
 @@ -79,12 +79,12 @@
  const myGeneratedColors = [
 -    themeVariables?.pie1 ?? null,
 -    themeVariables?.pie2 ?? null,
 -    themeVariables?.pie3 ?? null,
 -    themeVariables?.pie4 ?? null,
 -    themeVariables?.pie5 ?? null,
 -    themeVariables?.pie6 ?? null,
 -    themeVariables?.pie7 ?? null,
 -    themeVariables?.pie8 ?? null,
 -    themeVariables?.pie9 ?? null,
 -    themeVariables?.pie10 ?? null,
 -    themeVariables?.pie11 ?? null,
 -    themeVariables?.pie12 ?? null,
 +    themeVariables?.pie1 ?? '#000000',
 +    themeVariables?.pie2 ?? '#000000',
 +    themeVariables?.pie3 ?? '#000000',
 +    themeVariables?.pie4 ?? '#000000',
 +    themeVariables?.pie5 ?? '#000000',
 +    themeVariables?.pie6 ?? '#000000',
 +    themeVariables?.pie7 ?? '#000000',
 +    themeVariables?.pie8 ?? '#000000',
 +    themeVariables?.pie9 ?? '#000000',
 +    themeVariables?.pie10 ?? '#000000',
 +    themeVariables?.pie11 ?? '#000000',
 +    themeVariables?.pie12 ?? '#000000',

Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

];
// Set the color scale
const color: d3.ScaleOrdinal<string, 12, never> = scaleOrdinal(myGeneratedColors);
const color: d3.ScaleOrdinal<string, string | null, never> = scaleOrdinal(myGeneratedColors);

// Build the pie chart: each part of the pie is a path that we build using the arc function.
group
Expand Down
30 changes: 15 additions & 15 deletions packages/mermaid/src/diagrams/quadrant-chart/quadrantDb.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,21 +126,21 @@ function getQuadrantData() {
quadrantBuilder.setConfig(quadrantChartConfig);
}
quadrantBuilder.setThemeConfig({
quadrant1Fill: themeVariables.quadrant1Fill,
quadrant2Fill: themeVariables.quadrant2Fill,
quadrant3Fill: themeVariables.quadrant3Fill,
quadrant4Fill: themeVariables.quadrant4Fill,
quadrant1TextFill: themeVariables.quadrant1TextFill,
quadrant2TextFill: themeVariables.quadrant2TextFill,
quadrant3TextFill: themeVariables.quadrant3TextFill,
quadrant4TextFill: themeVariables.quadrant4TextFill,
quadrantPointFill: themeVariables.quadrantPointFill,
quadrantPointTextFill: themeVariables.quadrantPointTextFill,
quadrantXAxisTextFill: themeVariables.quadrantXAxisTextFill,
quadrantYAxisTextFill: themeVariables.quadrantYAxisTextFill,
quadrantExternalBorderStrokeFill: themeVariables.quadrantExternalBorderStrokeFill,
quadrantInternalBorderStrokeFill: themeVariables.quadrantInternalBorderStrokeFill,
quadrantTitleFill: themeVariables.quadrantTitleFill,
quadrant1Fill: themeVariables?.quadrant1Fill,
quadrant2Fill: themeVariables?.quadrant2Fill,
quadrant3Fill: themeVariables?.quadrant3Fill,
quadrant4Fill: themeVariables?.quadrant4Fill,
quadrant1TextFill: themeVariables?.quadrant1TextFill,
quadrant2TextFill: themeVariables?.quadrant2TextFill,
quadrant3TextFill: themeVariables?.quadrant3TextFill,
quadrant4TextFill: themeVariables?.quadrant4TextFill,
quadrantPointFill: themeVariables?.quadrantPointFill,
quadrantPointTextFill: themeVariables?.quadrantPointTextFill,
quadrantXAxisTextFill: themeVariables?.quadrantXAxisTextFill,
quadrantYAxisTextFill: themeVariables?.quadrantYAxisTextFill,
quadrantExternalBorderStrokeFill: themeVariables?.quadrantExternalBorderStrokeFill,
quadrantInternalBorderStrokeFill: themeVariables?.quadrantInternalBorderStrokeFill,
quadrantTitleFill: themeVariables?.quadrantTitleFill,
Comment on lines +129 to +143
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bito Code Review Agent Run #e7aeaa - 08/12/2024, 02:50 pm

🔴 High importance
Issue: The use of optional chaining ('?.') is a good practice for avoiding runtime errors when accessing potentially undefined properties. However, it can mask issues where 'themeVariables' or its properties are unexpectedly undefined. This can lead to silent failures where the UI does not render correctly, and the root cause is harder to trace. A similar issue was also found in packages/mermaid/src/diagrams/pie/pieRenderer.ts (line 53).
Fix: Instead of using optional chaining, consider adding explicit checks and logging warnings or errors if 'themeVariables' or its properties are undefined. This will make it easier to diagnose issues during development and ensure that the UI fails gracefully.
Code suggestion 
 @@ -128,21 +128,29 @@
    quadrantBuilder.setThemeConfig({
 -    quadrant1Fill: themeVariables?.quadrant1Fill,
 -    quadrant2Fill: themeVariables?.quadrant2Fill,
 -    quadrant3Fill: themeVariables?.quadrant3Fill,
 -    quadrant4Fill: themeVariables?.quadrant4Fill,
 -    quadrant1TextFill: themeVariables?.quadrant1TextFill,
 -    quadrant2TextFill: themeVariables?.quadrant2TextFill,
 -    quadrant3TextFill: themeVariables?.quadrant3TextFill,
 -    quadrant4TextFill: themeVariables?.quadrant4TextFill,
 -    quadrantPointFill: themeVariables?.quadrantPointFill,
 -    quadrantPointTextFill: themeVariables?.quadrantPointTextFill,
 -    quadrantXAxisTextFill: themeVariables?.quadrantXAxisTextFill,
 -    quadrantYAxisTextFill: themeVariables?.quadrantYAxisTextFill,
 -    quadrantExternalBorderStrokeFill: themeVariables?.quadrantExternalBorderStrokeFill,
 -    quadrantInternalBorderStrokeFill: themeVariables?.quadrantInternalBorderStrokeFill,
 -    quadrantTitleFill: themeVariables?.quadrantTitleFill,
 +    quadrant1Fill: themeVariables.quadrant1Fill ?? logWarning('quadrant1Fill is undefined'),
 +    quadrant2Fill: themeVariables.quadrant2Fill ?? logWarning('quadrant2Fill is undefined'),
 +    quadrant3Fill: themeVariables.quadrant3Fill ?? logWarning('quadrant3Fill is undefined'),
 +    quadrant4Fill: themeVariables.quadrant4Fill ?? logWarning('quadrant4Fill is undefined'),
 +    quadrant1TextFill: themeVariables.quadrant1TextFill ?? logWarning('quadrant1TextFill is undefined'),
 +    quadrant2TextFill: themeVariables.quadrant2TextFill ?? logWarning('quadrant2TextFill is undefined'),
 +    quadrant3TextFill: themeVariables.quadrant3TextFill ?? logWarning('quadrant3TextFill is undefined'),
 +    quadrant4TextFill: themeVariables.quadrant4TextFill ?? logWarning('quadrant4TextFill is undefined'),
 +    quadrantPointFill: themeVariables.quadrantPointFill ?? logWarning('quadrantPointFill is undefined'),
 +    quadrantPointTextFill: themeVariables.quadrantPointTextFill ?? logWarning('quadrantPointTextFill is undefined'),
 +    quadrantXAxisTextFill: themeVariables.quadrantXAxisTextFill ?? logWarning('quadrantXAxisTextFill is undefined'),
 +    quadrantYAxisTextFill: themeVariables.quadrantYAxisTextFill ?? logWarning('quadrantYAxisTextFill is undefined'),
 +    quadrantExternalBorderStrokeFill: themeVariables.quadrantExternalBorderStrokeFill ?? logWarning('quadrantExternalBorderStrokeFill is undefined'),
 +    quadrantInternalBorderStrokeFill: themeVariables.quadrantInternalBorderStrokeFill ?? logWarning('quadrantInternalBorderStrokeFill is undefined'),
 +    quadrantTitleFill: themeVariables.quadrantTitleFill ?? logWarning('quadrantTitleFill is undefined'),
    });

Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

});
quadrantBuilder.setData({ titleText: getDiagramTitle() });
return quadrantBuilder.build();
Expand Down
2 changes: 1 addition & 1 deletion packages/mermaid/src/diagrams/xychart/xychartDb.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ interface NormalTextType {
function getChartDefaultThemeConfig(): XYChartThemeConfig {
const defaultThemeVariables = getThemeVariables();
const config = configApi.getConfig();
return cleanAndMerge(defaultThemeVariables.xyChart, config.themeVariables.xyChart);
return cleanAndMerge(defaultThemeVariables.xyChart, config.themeVariables?.xyChart);
}
function getChartDefaultConfig(): XYChartConfig {
const config = configApi.getConfig();
Expand Down
Loading
Loading