fix: make themeVariables typesafe #5631
Open
+386
−32
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5631 +/- ##
=======================================
Coverage 5.86% 5.86%
=======================================
Files 274 273 -1
Lines 41086 41075 -11
Branches 488 512 +24
=======================================
Hits 2408 2408
+ Misses 38678 38667 -11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
Just type changes so hopefully should be easy to merge, cc @sidharthv96 |
sidharthv96
requested changes
Jul 9, 2024
|
EDIT: I am running into more TS errors so will push for the fixes soon |
Code Review Agent Run #e7aeaa
High-level FeedbackConsider adding explicit checks and logging warnings for undefined theme variables to improve error handling. Use default color values instead of 'null' to prevent runtime errors. Validate theme variable values to ensure they are within acceptable ranges and are valid CSS color strings.Actionable Issues
📄 packages/mermaid/src/diagrams/quadrant-chart/quadrantDb.ts
Issues: Total - 1, High importance - 1
📄 packages/mermaid/src/config.type.ts
Issues: Total - 1, High importance - 1
📄 packages/mermaid/src/diagrams/pie/pieRenderer.ts
Issues: Total - 1, High importance - 1
AI Code Review powered by |
Comment on lines
+129
to
+143
| 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, |
There was a problem hiding this comment.
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
| * Theme variables to create a custom theme. | ||
| * | ||
| */ | ||
| export interface ThemeVariables { |
There was a problem hiding this comment.
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
Comment on lines
+79
to
+90
| 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, |
There was a problem hiding this comment.
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
|
Hi @ad1992, I’m Rohan, the Developer Community Manager at Bito. The reviews you’re seeing are powered by Bito’s AI Code Review Agent, and I’ve also reached out to @sidharthv96 to take a closer look. As you’ve been an active contributor, I’d love to hear your thoughts on the output so far. Are there any specific areas you’d like us to focus on or other PRs you’d like us to review? We’re keen to explore how well our tool can identify issues and offer suggestions across different scenarios. And a big shoutout to @knsv for creating this fantastic project—I’m confident our code review agent will be a valuable asset for the rest of the Mermaid repositories. Looking forward to your feedback and hoping our agent can lend a helping hand to your projects. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📑 Summary
Brief description about the content of your PR.
Resolves #
📏 Design Decisions
Describe the way your implementation works or what design decisions you made if applicable.
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSIONis used for all new features.developbranch