ronantakizawa/codereview-bench huggingface dataset:
Repository: mermaid-js/mermaid
File: cypress/integration/rendering/flowchart-icon.spec.js (JavaScript)

Code before:
import { imgSnapshotTest } from '../../helpers/util.ts';

const themes = ['default', 'forest', 'dark', 'base', 'neutral'];

themes.forEach((theme, index) => {
  describe('Flowchart Icon', () => {
    it(`${index + 1}-icon: verify if icons are working from fontawesome library ${theme} theme`, () => {
      imgSnapshotTest(
        `flowchart TD
            A("fab:fa-twitter Twitter") --> B("fab:fa-facebook Facebook")
            B --> C("fa:fa-coffee Coffee")
            C --> D("fa:fa-car Car")
            D --> E("fab:fa-github GitHub")
        `,
        { theme }
      );
    });
  });
});

themes.forEach((theme, index) => {
  describe('Flowchart Icon', () => {
    it(`${index + 1}-icon: verify if registered icons are working on ${theme} theme`, () => {
      imgSnapshotTest(
        `flowchart TD
            A("fa:fa-bell Bell") 
        `,
        { theme }
      );
    });
  });
});


Review comment (suggestion):
These tests are flaky:

![Flowchart-Icon-2-icon-verify-if-icons-are-working-from-fontawesome-library-forest-theme diff](https://github.com/user-attachments/assets/34fc0da3-578f-4b24-811a-98022ebfd729)

I think we need to:

- Instead of using https://github.com/mermaid-js/mermaid/blob/2fb7bc2806f3d91dc9840ae988dc06e1329ca9a9/cypress/platform/e2e.html#L9-L12, we might want to use a local version of FontAwesome, like what the docs site uses: https://github.com/mermaid-js/mermaid/blob/3eaf8b717770a119cd0ea5303632b6430330f239/packages/mermaid/src/docs/package.json#L21
- We might need something like `await Promise.all(Array.from(document.fonts, (font) => font.load()))` to forcibly wait for all fonts to load before starting to render the diagram: https://github.com/mermaid-js/mermaid-cli/blob/7b19bf7f230b95fde1ba1a4bbda78bc96e696773/src/index.js#L255

Code after:
import { imgSnapshotTest } from '../../helpers/util.ts';

const themes = ['default', 'forest', 'dark', 'base', 'neutral'];

describe('when rendering flowchart with icons', () => {
  for (const theme of themes) {
    it(`should render icons from fontawesome library on theme ${theme}`, () => {
      imgSnapshotTest(
        `flowchart TD
            A("fab:fa-twitter Twitter") --> B("fab:fa-facebook Facebook")
            B --> C("fa:fa-coffee Coffee")
            C --> D("fa:fa-car Car")
            D --> E("fab:fa-github GitHub")
        `,
        { theme }
      );
    });

    it(`should render registered icons on theme ${theme}`, () => {
      imgSnapshotTest(
        `flowchart TD
            A("fa:fa-bell Bell") 
        `,
        { theme }
      );
    });
  }
});
Repository: mermaid-js/mermaid
File: packages/mermaid/src/diagrams/state/stateDb.js (JavaScript)

Code before:
    const item = this.getState(itemId);
    if (item !== undefined) {
      item.styles.push(styleText);
    }
  }

  /**
   * Add a text style to a state with the given id
   *
   * @param itemId The id of item to apply the css class to
   * @param cssClassName CSS class name
   */
  setTextStyle(itemId, cssClassName) {
    const item = this.getState(itemId);
    if (item !== undefined) {
      item.textStyles.push(cssClassName);
    }
  }

  getDirection() {
    const doc = this.rootDoc.find((doc) => doc.stmt === 'dir');
    this.direction = doc ? doc.value : 'TB';
    return this.direction;
  }
  setDirection(dir) {
    let doc = this.rootDoc.find((doc) => doc.stmt === 'dir');
    if (doc) {
      doc.value = dir;
    } else {
      this.rootDoc.unshift({ stmt: 'dir', value: dir });
    }
  }

  trimColon(str) {
    return str && str[0] === ':' ? str.substr(1).trim() : str.trim();
  }

  getData() {
    const config = getConfig();
    return {
      nodes: this.nodes,
      edges: this.edges,
      other: {},
      config,
      direction: getDir(this.getRootDocV2()),
    };
  }

  getConfig() {
    return getConfig().state;
  }

Review comment (suggestion):
```suggestion
    const doc = this.rootDoc.find((doc) => doc.stmt === 'dir');
```

Code after:
      foundState.classes.push(cssClassName);
    });
  }

  /**
   * Add a style to a state with the given id.
   * @example style stateId fill:#f9f,stroke:#333,stroke-width:4px
   *   where 'style' is the keyword
   *   stateId is the id of a state
   *   the rest of the string is the styleText (all of the attributes to be applied to the state)
   *
   * @param itemId The id of item to apply the style to
   * @param styleText - the text of the attributes for the style
   */
  setStyle(itemId, styleText) {
    const item = this.getState(itemId);
    if (item !== undefined) {
      item.styles.push(styleText);
    }
  }

  /**
   * Add a text style to a state with the given id
   *
   * @param itemId The id of item to apply the css class to
   * @param cssClassName CSS class name
   */
  setTextStyle(itemId, cssClassName) {
    const item = this.getState(itemId);
    if (item !== undefined) {
      item.textStyles.push(cssClassName);
    }
  }

  /**
   * Finds the direction statement in the root document.
   * @private
   * @returns {{ value: string } | undefined} - the direction statement if present
   */
  getDirectionStatement() {
    return this.rootDoc.find((doc) => doc.stmt === STMT_DIRECTION);
  }

  getDirection() {
    return this.getDirectionStatement()?.value ?? DEFAULT_DIAGRAM_DIRECTION;
  }

  setDirection(dir) {
    const doc = this.getDirectionStatement();
    if (doc) {
      doc.value = dir;
Repository: mermaid-js/mermaid
File: packages/mermaid/src/diagrams/state/stateDb.js (JavaScript)

Code before:
   * @param itemId The id of item to apply the style to
   * @param styleText - the text of the attributes for the style
   */
  setStyle(itemId, styleText) {
    const item = this.getState(itemId);
    if (item !== undefined) {
      item.styles.push(styleText);
    }
  }

  /**
   * Add a text style to a state with the given id
   *
   * @param itemId The id of item to apply the css class to
   * @param cssClassName CSS class name
   */
  setTextStyle(itemId, cssClassName) {
    const item = this.getState(itemId);
    if (item !== undefined) {
      item.textStyles.push(cssClassName);
    }
  }

  getDirection() {
    const doc = this.rootDoc.find((doc) => doc.stmt === 'dir');
    this.direction = doc ? doc.value : 'TB';
    return this.direction;
  }
  setDirection(dir) {
    let doc = this.rootDoc.find((doc) => doc.stmt === 'dir');
    if (doc) {
      doc.value = dir;
    } else {
      this.rootDoc.unshift({ stmt: 'dir', value: dir });
    }
  }

  trimColon(str) {
    return str && str[0] === ':' ? str.substr(1).trim() : str.trim();
  }

  getData() {
    const config = getConfig();
    return {
      nodes: this.nodes,
      edges: this.edges,
      other: {},
      config,
      direction: getDir(this.getRootDocV2()),
    };
  }

Review comment (refactor):
Please remove this.direction, as it's not used. 
Use constant `DEFAULT_DIAGRAM_DIRECTION` instead of 'TB'. 
You can change the value now. 

Code after:
        const trimmedId = id.trim();
        this.addState(trimmedId);
        foundState = this.getState(trimmedId);
      }
      foundState.classes.push(cssClassName);
    });
  }

  /**
   * Add a style to a state with the given id.
   * @example style stateId fill:#f9f,stroke:#333,stroke-width:4px
   *   where 'style' is the keyword
   *   stateId is the id of a state
   *   the rest of the string is the styleText (all of the attributes to be applied to the state)
   *
   * @param itemId The id of item to apply the style to
   * @param styleText - the text of the attributes for the style
   */
  setStyle(itemId, styleText) {
    const item = this.getState(itemId);
    if (item !== undefined) {
      item.styles.push(styleText);
    }
  }

  /**
   * Add a text style to a state with the given id
   *
   * @param itemId The id of item to apply the css class to
   * @param cssClassName CSS class name
   */
  setTextStyle(itemId, cssClassName) {
    const item = this.getState(itemId);
    if (item !== undefined) {
      item.textStyles.push(cssClassName);
    }
  }

  /**
   * Finds the direction statement in the root document.
   * @private
   * @returns {{ value: string } | undefined} - the direction statement if present
   */
  getDirectionStatement() {
    return this.rootDoc.find((doc) => doc.stmt === STMT_DIRECTION);
  }

  getDirection() {
    return this.getDirectionStatement()?.value ?? DEFAULT_DIAGRAM_DIRECTION;
  }
Repository: mermaid-js/mermaid
File: packages/mermaid/src/docs/.vitepress/components/TopBar.vue (Vue)

Code before:
const index: Ref<number> = ref(0);
const isPaused: Ref<boolean> = ref(false);
const route = useRoute();

const isHomePage = computed(() => {
  return route.path === '/';
});

onMounted(() => {
  index.value = Math.floor(Math.random() * taglines.length);
  setInterval(() => {
    if (isPaused.value) {
      return;
    }
    index.value = (index.value + 1) % taglines.length;
  }, 5_000);
});
</script>

<template>
  <div
    class="mb-4 w-full top-bar flex p-2 bg-[#E0095F]"
    @mouseenter="isPaused = true"
    @mouseleave="isPaused = false"
  >
    <p class="w-full tracking-wide fade-text text-lg" :class="isHomePage ? 'text-lg' : 'text-sm'">
      <transition name="fade" mode="out-in">
        <a
          :key="index"
          :href="taglines[index].url"
          target="_blank"
          class="unstyled flex justify-center items-center gap-4 no-tooltip text-white tracking-wide plausible-event-name=bannerClick"
        >
          <span class="font-semibold">{{ taglines[index].label }}</span>
          <button
            class="bg-[#1E1A2E] shrink-0 rounded-lg p-1.5 px-4 font-semibold tracking-wide"
            :class="isHomePage ? 'text-lg' : 'text-sm'"
          >
            Try now
          </button>
        </a>
      </transition>
    </p>
  </div>
</template>

<style>
.fade-enter-active,
.fade-leave-active {
  transition: opacity 1s;
}

Review comment (suggestion):
```suggestion
    <p class="w-full tracking-wide fade-text" :class="isHomePage ? 'text-lg' : 'text-sm'">
```

Code after:
const index: Ref<number> = ref(0);
const isPaused: Ref<boolean> = ref(false);
const route = useRoute();

const isHomePage = computed(() => {
  return route.path === '/';
});

onMounted(() => {
  index.value = Math.floor(Math.random() * taglines.length);
  setInterval(() => {
    if (isPaused.value) {
      return;
    }
    index.value = (index.value + 1) % taglines.length;
  }, 5_000);
});
</script>

<template>
  <div
    class="mb-4 w-full top-bar flex p-2 bg-[#E0095F]"
    @mouseenter="isPaused = true"
    @mouseleave="isPaused = false"
  >
    <p class="w-full tracking-wide fade-text" :class="isHomePage ? 'text-lg' : 'text-sm'">
      <transition name="fade" mode="out-in">
        <a
          :key="index"
          :href="taglines[index].url"
          target="_blank"
          class="unstyled flex justify-center items-center gap-4 no-tooltip text-white tracking-wide plausible-event-name=bannerClick"
        >
          <span class="font-semibold">{{ taglines[index].label }}</span>
          <button
            class="bg-[#1E1A2E] shrink-0 rounded-lg p-1.5 px-4 font-semibold tracking-wide"
            :class="isHomePage ? 'text-lg' : 'text-sm'"
          >
            Try now
          </button>
        </a>
      </transition>
    </p>
  </div>
</template>

<style>
.fade-enter-active,
.fade-leave-active {
  transition: opacity 1s;
}
Repository: mermaid-js/mermaid
File: packages/mermaid/src/diagrams/pie/pieDb.ts (TypeScript)

Code before:
import DEFAULT_CONFIG from '../../defaultConfig.js';

export const DEFAULT_PIE_CONFIG: Required<PieDiagramConfig> = DEFAULT_CONFIG.pie;

export const DEFAULT_PIE_DB: RequiredDeep<PieFields> = {
  sections: new Map(),
  showData: false,
  config: DEFAULT_PIE_CONFIG,
} as const;

let sections: Sections = DEFAULT_PIE_DB.sections;
let showData: boolean = DEFAULT_PIE_DB.showData;
const config: Required<PieDiagramConfig> = structuredClone(DEFAULT_PIE_CONFIG);

const getConfig = (): Required<PieDiagramConfig> => structuredClone(config);

const clear = (): void => {
  sections = new Map();
  showData = DEFAULT_PIE_DB.showData;
  commonClear();
};

const addSection = ({ label, value }: D3Section): void => {
  if (value < 0) {
    throw new Error(
      `"${label}" has invalid value: ${value}.Negative values are not allowed in pie charts. All slice values must be > 0.`
    );
  }
  if (!sections.has(label)) {
    sections.set(label, value);
    log.debug(`added new section: ${label}, with value: ${value}`);
  }
};

const getSections = (): Sections => sections;

const setShowData = (toggle: boolean): void => {
  showData = toggle;
};

const getShowData = (): boolean => showData;

export const db: PieDB = {
  getConfig,

  clear,
  setDiagramTitle,
  getDiagramTitle,
  setAccTitle,
  getAccTitle,
  setAccDescription,

Review comment (suggestion):
```suggestion
      `"${label}" has invalid value: ${value}. Negative values are not allowed in pie charts. All slice values must be >= 0.`
```

Code after:
import DEFAULT_CONFIG from '../../defaultConfig.js';

export const DEFAULT_PIE_CONFIG: Required<PieDiagramConfig> = DEFAULT_CONFIG.pie;

export const DEFAULT_PIE_DB: RequiredDeep<PieFields> = {
  sections: new Map(),
  showData: false,
  config: DEFAULT_PIE_CONFIG,
} as const;

let sections: Sections = DEFAULT_PIE_DB.sections;
let showData: boolean = DEFAULT_PIE_DB.showData;
const config: Required<PieDiagramConfig> = structuredClone(DEFAULT_PIE_CONFIG);

const getConfig = (): Required<PieDiagramConfig> => structuredClone(config);

const clear = (): void => {
  sections = new Map();
  showData = DEFAULT_PIE_DB.showData;
  commonClear();
};

const addSection = ({ label, value }: D3Section): void => {
  if (value < 0) {
    throw new Error(
      `"${label}" has invalid value: ${value}. Negative values are not allowed in pie charts. All slice values must be >= 0.`
    );
  }
  if (!sections.has(label)) {
    sections.set(label, value);
    log.debug(`added new section: ${label}, with value: ${value}`);
  }
};

const getSections = (): Sections => sections;

const setShowData = (toggle: boolean): void => {
  showData = toggle;
};

const getShowData = (): boolean => showData;

export const db: PieDB = {
  getConfig,

  clear,
  setDiagramTitle,
  getDiagramTitle,
  setAccTitle,
  getAccTitle,
  setAccDescription,
Repository: mermaid-js/mermaid
File: packages/mermaid/src/diagrams/git/gitGraph.spec.ts (TypeScript)

Code before:
     commit id:"working on MDR"
     checkout v1-rc
     commit id:"minor design changes for MDR" tag:"v1.0.2"
     checkout FORK-v1.0-MDR
     merge v1-rc
     checkout main
     commit id:"new feature for v1.1…"
     checkout FORK-v1.0-MDR
     commit id:"working on MDR"
     commit id:"finishing MDR"
     branch v1.0-MDR
     checkout v1.0-MDR
     commit id:"brush up release" tag:"v1.0.2-MDR"
     checkout v1-rc
     commit id:"bugfix without MDR"
     checkout main
     commit id:"work on v1.1"
  `;

    const spyOn = vi.spyOn(log, 'warn').mockImplementation(() => undefined);

    await parser.parse(str);

    expect(spyOn).toHaveBeenCalledWith('Commit ID working on MDR already exists');

    spyOn.mockRestore();
  });
});


Review comment (suggestion):
**nitpick**: This might be a better name for a variable than `spyOn`.

```suggestion
    const logWarnSpy = vi.spyOn(log, 'warn').mockImplementation(() => undefined);

    await parser.parse(str);

    expect(logWarnSpy).toHaveBeenCalledWith('Commit ID working on MDR already exists');

    logWarnSpy.mockRestore();
```

Code after:
     commit id:"working on MDR"
     checkout v1-rc
     commit id:"minor design changes for MDR" tag:"v1.0.2"
     checkout FORK-v1.0-MDR
     merge v1-rc
     checkout main
     commit id:"new feature for v1.1…"
     checkout FORK-v1.0-MDR
     commit id:"working on MDR"
     commit id:"finishing MDR"
     branch v1.0-MDR
     checkout v1.0-MDR
     commit id:"brush up release" tag:"v1.0.2-MDR"
     checkout v1-rc
     commit id:"bugfix without MDR"
     checkout main
     commit id:"work on v1.1"
  `;

    const logWarnSpy = vi.spyOn(log, 'warn').mockImplementation(() => undefined);

    await parser.parse(str);

    expect(logWarnSpy).toHaveBeenCalledWith('Commit ID working on MDR already exists');

    logWarnSpy.mockRestore();
  });
});
Repository: mermaid-js/mermaid
File: packages/mermaid/src/diagrams/treemap/parser.ts (TypeScript)

Code before:
      }
    }
  };

  addNodesRecursively(hierarchyNodes, 0);
};

/**
 * Gets the name of a treemap item
 * @param item - The treemap item
 * @returns The name of the item
 */
const getItemName = (item: { name?: string | number }): string => {
  return item.name ? String(item.name) : '';
};

export const parser: ParserDefinition = {
  parser: { yy: {} as TreemapDB },
  parse: async (text: string): Promise<void> => {
    try {
      // Use a generic parse that accepts any diagram type

      const parseFunc = parse as (diagramType: string, text: string) => Promise<TreemapAst>;
      const ast = await parseFunc('treemap', text);
      log.debug('Treemap AST:', ast);
      populate(ast, parser.parser?.yy as TreemapDB);
    } catch (error) {
      log.error('Error parsing treemap:', error);
      throw error;
    }
  },
};


Review comment (bug):
**suggestion(non-blocking)**: Is it worth having something like this instead?

The `throw new Error('');` should never happen.

```suggestion
      const db = parser.parser?.yy;
      if (!(db instanceof TreemapDB)) {
        throw new Error('parser.parser?.yy was not a TreemapDB. This is due to a bug within Mermaid, please report this issue at https://github.com/mermaid-js/mermaid/issues.');
      }
      populate(ast, db);
```

**non-blocking** since the current code should be safe too, I just really don't like using `as XXXXXX` if I can avoid it.

Code after:
        addNodesRecursively(node.children, level + 1);
      }
    }
  };

  addNodesRecursively(hierarchyNodes, 0);
};

/**
 * Gets the name of a treemap item
 * @param item - The treemap item
 * @returns The name of the item
 */
const getItemName = (item: { name?: string | number }): string => {
  return item.name ? String(item.name) : '';
};

export const parser: ParserDefinition = {
  // @ts-expect-error - TreeMapDB is not assignable to DiagramDB
  parser: { yy: undefined },
  parse: async (text: string): Promise<void> => {
    try {
      // Use a generic parse that accepts any diagram type

      const parseFunc = parse as (diagramType: string, text: string) => Promise<TreemapAst>;
      const ast = await parseFunc('treemap', text);
      log.debug('Treemap AST:', ast);
      const db = parser.parser?.yy;
      if (!(db instanceof TreeMapDB)) {
        throw new Error(
          'parser.parser?.yy was not a TreemapDB. This is due to a bug within Mermaid, please report this issue at https://github.com/mermaid-js/mermaid/issues.'
        );
      }
      populate(ast, db);
    } catch (error) {
      log.error('Error parsing treemap:', error);
      throw error;
    }
  },
};
Repository: mermaid-js/mermaid
File: packages/mermaid/src/rendering-util/rendering-elements/edges.js (JavaScript)

Code before:
  }
  switch (edge.pattern) {
    case 'solid':
      strokeClasses += ' edge-pattern-solid';
      break;
    case 'dotted':
      strokeClasses += ' edge-pattern-dotted';
      break;
    case 'dashed':
      strokeClasses += ' edge-pattern-dashed';
      break;
    default:
      strokeClasses += ' edge-pattern-solid';
  }
  let svgPath;
  let linePath = lineFunction(lineData);
  const edgeStyles = Array.isArray(edge.style) ? edge.style : edge.style ? [edge.style] : [];
  let strokeColor = edgeStyles.find((style) => style?.startsWith('stroke:'));

  let animationClass = '';
  if (edge.animate) {
    animationClass = ' edge-animation-fast';
  }
  if (edge.animation) {
    animationClass = ' edge-animation-' + edge.animation;
  }

  if (edge.look === 'handDrawn') {
    const rc = rough.svg(elem);
    Object.assign([], lineData);

    const svgPathNode = rc.path(linePath, {
      roughness: 0.3,
      seed: handDrawnSeed,
    });

    strokeClasses += ' transition';

    svgPath = select(svgPathNode)
      .select('path')
      .attr('id', edge.id)
      .attr(
        'class',
        ' ' + strokeClasses + (edge.classes ? ' ' + edge.classes : '') + (animationClass ?? '')
      )
      .attr('style', edgeStyles ? edgeStyles.reduce((acc, style) => acc + ';' + style, '') : '');
    let d = svgPath.attr('d');
    svgPath.attr('d', d);
    elem.node().appendChild(svgPath.node());
  } else {
    const stylesFromClasses = edgeClassStyles.join(';');

Review comment (suggestion):
Handling the space here creates a scenario where some variables have space prefixes and some don't, which leads to inconsistent handling of `' ' +`. 

Code after:
      break;
    case 'natural':
      curve = curveNatural;
      break;
    case 'step':
      curve = curveStep;
      break;
    case 'stepAfter':
      curve = curveStepAfter;
      break;
    case 'stepBefore':
      curve = curveStepBefore;
      break;
    default:
      curve = curveBasis;
  }

  // if (edge.curve) {
  //   curve = edge.curve;
  // }

  const { x, y } = getLineFunctionsWithOffset(edge);
  const lineFunction = line().x(x).y(y).curve(curve);

  let strokeClasses;
  switch (edge.thickness) {
    case 'normal':
      strokeClasses = 'edge-thickness-normal';
      break;
    case 'thick':
      strokeClasses = 'edge-thickness-thick';
      break;
    case 'invisible':
      strokeClasses = 'edge-thickness-invisible';
      break;
    default:
      strokeClasses = 'edge-thickness-normal';
  }
  switch (edge.pattern) {
    case 'solid':
      strokeClasses += ' edge-pattern-solid';
      break;
    case 'dotted':
      strokeClasses += ' edge-pattern-dotted';
      break;
    case 'dashed':
      strokeClasses += ' edge-pattern-dashed';
      break;
    default:
      strokeClasses += ' edge-pattern-solid';
  }