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 4 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
52 changes: 51 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 @@ -55,14 +55,64 @@ export type DOMPurifyConfiguration = import('dompurify').Config;
*/
export type CSSFontSize = string | number;

export interface ThemeVariables {
fontFamily?: string;
fontSize?: CSSFontSize;
fontWeight?: number;
primaryColor?: string;
primaryTextColor?: 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;
yAxisLabelColor?: string;
yAxisTickColor?: string;
yAxisLineColor?: string;
plotColorPalette?: string;
};
THEME_COLOR_LIMIT?: number;

}
export interface MermaidConfig {
/**
* Theme, the CSS style sheet.
* You may also use `themeCSS` to override this value.
*
*/
theme?: 'default' | 'forest' | 'dark' | 'neutral' | 'null';
themeVariables?: any;
themeVariables?: ThemeVariables;
themeCSS?: string;
/**
* The maximum allowed size of the users text diagram
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