ronantakizawa/codereview-bench huggingface dataset:
Repository: mem0ai/mem0
File: mem0/embeddings/gemini.py (Python)

Code before:
import os
from typing import Literal, Optional

import google.genai as genai 

from mem0.configs.embeddings.base import BaseEmbedderConfig
from mem0.embeddings.base import EmbeddingBase


class GoogleGenAIEmbedding(EmbeddingBase):
    def __init__(self, config: Optional[BaseEmbedderConfig] = None):
        super().__init__(config)

        self.config.model = self.config.model or "models/text-embedding-004"
        # Only embedding_dims is supported for output dimensionality
        self.output_dimensionality = getattr(self.config, 'embedding_dims', 768)

        api_key = self.config.api_key or os.getenv("GOOGLE_API_KEY")


        if api_key:
            self.client = genai.Client(api_key="api_key")
        else:

            self.client = genai.Client()


    def embed(self, text, memory_action: Optional[Literal["add", "search", "update"]] = None):
        """
        Get the embedding for the given text using Google Generative AI.
        Args:
            text (str): The text to embed.
            memory_action (optional): The type of embedding to use. (Currently not used by Gemini for task_type)
        Returns:
            list: The embedding vector.
        """
        text = text.replace("\n", " ")


        response = self.client.models.embed_content(
            model=self.config.model,

Review comment (suggestion):
Please let' use `self.config.embedding_dims` because the embedding_dims is also used at other places

Code after:
import os
from typing import Literal, Optional

import google.genai as genai

from mem0.configs.embeddings.base import BaseEmbedderConfig
from mem0.embeddings.base import EmbeddingBase


class GoogleGenAIEmbedding(EmbeddingBase):
    def __init__(self, config: Optional[BaseEmbedderConfig] = None):
        super().__init__(config)

        self.config.model = self.config.model or "models/text-embedding-004"
        self.config.embedding_dims = self.config.embedding_dims or self.config.output_dimensionality or 768

        api_key = self.config.api_key or os.getenv("GOOGLE_API_KEY")

        if api_key:
            self.client = genai.Client(api_key="api_key")
        else:
            self.client = genai.Client()

    def embed(self, text, memory_action: Optional[Literal["add", "search", "update"]] = None):
        """
        Get the embedding for the given text using Google Generative AI.
        Args:
            text (str): The text to embed.
            memory_action (optional): The type of embedding to use. (Currently not used by Gemini for task_type)
        Returns:
            list: The embedding vector.
        """
        text = text.replace("\n", " ")

        response = self.client.models.embed_content(
            model=self.config.model, content=text, output_dimensionality=self.config.embedding_dims
        )

        return response["embedding"]
Repository: mermaid-js/mermaid
File: packages/mermaid/src/dagre-wrapper/edgeMarker.ts (TypeScript)

Code before:
  url: string,
  id: string,
  diagramType: string
) => {
  if (edge.arrowTypeStart) {
    addEdgeMarker(svgPath, 'start', edge.arrowTypeStart, url, id, diagramType);
  }
  if (edge.arrowTypeEnd) {
    addEdgeMarker(svgPath, 'end', edge.arrowTypeEnd, url, id, diagramType);
  }
};

const addEdgeMarker = (
  svgPath: SVG,
  position: 'start' | 'end',
  arrowType: string,
  url: string,
  id: string,
  diagramType: string
) => {
  if (arrowType.startsWith('arrow_')) {
    arrowType = arrowType.replace('arrow_', '');
  }

  const suffix = position === 'start' ? 'Start' : 'End';
  svgPath.attr(`marker-${position}`, `url(${url}#${id}_${diagramType}-${arrowType}${suffix})`);
};


Review comment (bug):
Currently, the default action for an unrecognized arrow type is to ignore it:

https://github.com/mermaid-js/mermaid/blob/2049915c2ad5d1de713548b25d39d9db93d675f5/packages/mermaid/src/dagre-wrapper/edges.js#L564

This PR changes it to instead add an invalid edge marker (could this cause SVG corruption/attacks if we add a string with a `)` in it?)

If we want to the old behavior, maybe something like the following is better?

```ts

const arrowTypesMap = {
  "arrow_cross": "cross",
  "arrow_point": "point",
  "arrow_barb": "barb",
  "arrow_circle": "circle",
  "aggregation": "aggregation",
  "extension": "extension",
  "composition": "composition",
  "dependency": "dependency",
  "lollipop": "lollipop",
} as const;

const addEdgeMarker = (
  svgPath: SVG,
  position: 'start' | 'end',
  arrowType: string,
  url: string,
  id: string,
  diagramType: string
) => {
  if (arrowTypesMap[arrowType] === undefined) {
    // TODO: should we log a debug/warning message?
    return; // unknown arrow type, ignore
  }
  
  const suffix = position === 'start' ? 'Start' : 'End';
  svgPath.attr(`marker-${position}`, `url(${url}#${id}_${diagramType}-${arrowTypesMap[arrowType]}${suffix})`);
};
```

Code after:
  edge: Pick<EdgeData, 'arrowTypeStart' | 'arrowTypeEnd'>,
  url: string,
  id: string,
  diagramType: string
) => {
  if (edge.arrowTypeStart) {
    addEdgeMarker(svgPath, 'start', edge.arrowTypeStart, url, id, diagramType);
  }
  if (edge.arrowTypeEnd) {
    addEdgeMarker(svgPath, 'end', edge.arrowTypeEnd, url, id, diagramType);
  }
};

const arrowTypesMap = {
  arrow_cross: 'cross',
  arrow_point: 'point',
  arrow_barb: 'barb',
  arrow_circle: 'circle',
  aggregation: 'aggregation',
  extension: 'extension',
  composition: 'composition',
  dependency: 'dependency',
  lollipop: 'lollipop',
} as const;

const addEdgeMarker = (
  svgPath: SVG,
  position: 'start' | 'end',
  arrowType: string,
  url: string,
  id: string,
  diagramType: string
) => {
  const endMarkerType = arrowTypesMap[arrowType as keyof typeof arrowTypesMap];

  if (!endMarkerType) {
    log.warn(`Unknown arrow type: ${arrowType}`);
    return; // unknown arrow type, ignore
  }

  const suffix = position === 'start' ? 'Start' : 'End';
  svgPath.attr(`marker-${position}`, `url(${url}#${id}_${diagramType}-${endMarkerType}${suffix})`);
};
Repository: mermaid-js/mermaid
File: packages/mermaid/src/rendering-util/handle-markdown-text.ts (TypeScript)

Code before:
import type { Content } from 'mdast';
import { fromMarkdown } from 'mdast-util-from-markdown';
import { dedent } from 'ts-dedent';
import type { MarkdownLine, MarkdownWordType } from './types.js';
import { getConfig } from '../config.js';

/**
 * @param markdown - markdown to process
 * @returns processed markdown
 */
function preprocessMarkdown(markdown: string): string {
  const markdownAutoWrap = getConfig().markdownAutoWrap;
  // Replace multiple newlines with a single newline
  const withoutMultipleNewlines = markdown.replace(/\n{2,}/g, '\n');
  // Remove extra spaces at the beginning of each line
  const withoutExtraSpaces = dedent(withoutMultipleNewlines);
  if (markdownAutoWrap === false) {
    return withoutExtraSpaces.replace(/ /g, '&nbsp;');
  }
  return withoutExtraSpaces;
}

/**
 * @param markdown - markdown to split into lines
 */
export function markdownToLines(markdown: string): MarkdownLine[] {
  const preprocessedMarkdown = preprocessMarkdown(markdown);
  const { children } = fromMarkdown(preprocessedMarkdown);
  const lines: MarkdownLine[] = [[]];
  let currentLine = 0;

  function processNode(node: Content, parentType: MarkdownWordType = 'normal') {
    if (node.type === 'text') {
      const textLines = node.value.split('\n');
      textLines.forEach((textLine, index) => {
        if (index !== 0) {
          currentLine++;

Review comment (suggestion):
Please pass the config to the function, instead of importing `getConfig`.
This makes the function independent and will make writing tests easier. 

Code after:
import type { Content } from 'mdast';
import { fromMarkdown } from 'mdast-util-from-markdown';
import { dedent } from 'ts-dedent';
import type { MarkdownLine, MarkdownWordType } from './types.js';
import type { MermaidConfig } from '../config.type.js';

/**
 * @param markdown - markdown to process
 * @returns processed markdown
 */
function preprocessMarkdown(markdown: string, { markdownAutoWrap }: MermaidConfig): string {
  // Replace multiple newlines with a single newline
  const withoutMultipleNewlines = markdown.replace(/\n{2,}/g, '\n');
  // Remove extra spaces at the beginning of each line
  const withoutExtraSpaces = dedent(withoutMultipleNewlines);
  if (markdownAutoWrap === false) {
    return withoutExtraSpaces.replace(/ /g, '&nbsp;');
  }
  return withoutExtraSpaces;
}

/**
 * @param markdown - markdown to split into lines
 */
export function markdownToLines(markdown: string, config: MermaidConfig = {}): MarkdownLine[] {
  const preprocessedMarkdown = preprocessMarkdown(markdown, config);
  const { children } = fromMarkdown(preprocessedMarkdown);
  const lines: MarkdownLine[] = [[]];
  let currentLine = 0;

  function processNode(node: Content, parentType: MarkdownWordType = 'normal') {
    if (node.type === 'text') {
      const textLines = node.value.split('\n');
      textLines.forEach((textLine, index) => {
        if (index !== 0) {
          currentLine++;
          lines.push([]);
Repository: mermaid-js/mermaid
File: packages/mermaid/src/rendering-util/handle-markdown-text.ts (TypeScript)

Code before:
      });
    }
  }

  children.forEach((treeNode) => {
    if (treeNode.type === 'paragraph') {
      treeNode.children.forEach((contentNode) => {
        processNode(contentNode);
      });
    }
  });

  return lines;
}

export function markdownToHTML(markdown: string) {
  const { children } = fromMarkdown(markdown);
  const markdownAutoWrap = getConfig().markdownAutoWrap;

  function output(node: Content): string {
    if (node.type === 'text') {
      if (markdownAutoWrap === false) {
        return node.value.replace(/\n/g, '<br/>').replace(/ /g, '&nbsp;');
      } else {
        return node.value.replace(/\n/g, '<br/>');
      }
    } else if (node.type === 'strong') {
      return `<strong>${node.children.map(output).join('')}</strong>`;
    } else if (node.type === 'emphasis') {
      return `<em>${node.children.map(output).join('')}</em>`;
    } else if (node.type === 'paragraph') {
      return `<p>${node.children.map(output).join('')}</p>`;
    }
    return `Unsupported markdown: ${node.type}`;
  }

  return children.map(output).join('');
}


Review comment (suggestion):
nit: You don't need this `else`, as the `if` ends in return. 
```suggestion
      }
      return node.value.replace(/\n/g, '<br/>');
```

Code after:
    }
  }

  children.forEach((treeNode) => {
    if (treeNode.type === 'paragraph') {
      treeNode.children.forEach((contentNode) => {
        processNode(contentNode);
      });
    }
  });

  return lines;
}

export function markdownToHTML(markdown: string, { markdownAutoWrap }: MermaidConfig = {}) {
  const { children } = fromMarkdown(markdown);

  function output(node: Content): string {
    if (node.type === 'text') {
      if (markdownAutoWrap === false) {
        return node.value.replace(/\n/g, '<br/>').replace(/ /g, '&nbsp;');
      }
      return node.value.replace(/\n/g, '<br/>');
    } else if (node.type === 'strong') {
      return `<strong>${node.children.map(output).join('')}</strong>`;
    } else if (node.type === 'emphasis') {
      return `<em>${node.children.map(output).join('')}</em>`;
    } else if (node.type === 'paragraph') {
      return `<p>${node.children.map(output).join('')}</p>`;
    }
    return `Unsupported markdown: ${node.type}`;
  }

  return children.map(output).join('');
}
Repository: mermaid-js/mermaid
File: packages/mermaid/src/diagrams/flowchart/flowDb.js (JavaScript)

Code before:

/**
 * Updates a link's line interpolation algorithm
 *
 * @param positions
 * @param interp
 */
export const updateLinkInterpolate = function (positions, interp) {
  positions.forEach(function (pos) {
    if (pos === 'default') {
      edges.defaultInterpolate = interp;
    } else {
      edges[pos].interpolate = interp;
    }
  });
};

/**
 * Updates a link with a style
 *
 * @param positions
 * @param style
 */
export const updateLink = function (positions, style) {
  positions.forEach(function (pos) {
    if (pos >= edges.length) {
      let error = new Error(
        `Incorrect index ${pos} of linkStyle. (Help: Index must be from 0 to ${edges.length - 1})`
      );
      throw error;
    }
    if (pos === 'default') {
      edges.defaultStyle = style;
    } else {
      if (utils.isSubstringInArray('fill', style) === -1) {
        style.push('fill:none');
      }
      edges[pos].style = style;
    }
  });
};

export const addClass = function (ids, style) {
  ids.split(',').forEach(function (id) {
    if (classes[id] === undefined) {
      classes[id] = { id, styles: [], textStyles: [] };
    }

    if (style !== undefined && style !== null) {
      style.forEach(function (s) {
        if (s.match('color')) {

Review comment (suggestion):
We should have a testcase to make sure a non-TypeError is thrown.

Code after:

/**
 * Updates a link's line interpolation algorithm
 *
 * @param positions
 * @param interp
 */
export const updateLinkInterpolate = function (positions, interp) {
  positions.forEach(function (pos) {
    if (pos === 'default') {
      edges.defaultInterpolate = interp;
    } else {
      edges[pos].interpolate = interp;
    }
  });
};

/**
 * Updates a link with a style
 *
 * @param positions
 * @param style
 */
export const updateLink = function (positions, style) {
  positions.forEach(function (pos) {
    if (pos >= edges.length) {
      throw new Error(
        `The index ${pos} for linkStyle is out of bounds. Valid indices for linkStyle are between 0 and ${
          edges.length - 1
        }. (Help: Ensure that the index is within the range of existing edges.)`
      );
    }
    if (pos === 'default') {
      edges.defaultStyle = style;
    } else {
      if (utils.isSubstringInArray('fill', style) === -1) {
        style.push('fill:none');
      }
      edges[pos].style = style;
    }
  });
};

export const addClass = function (ids, style) {
  ids.split(',').forEach(function (id) {
    if (classes[id] === undefined) {
      classes[id] = { id, styles: [], textStyles: [] };
    }

    if (style !== undefined && style !== null) {
      style.forEach(function (s) {
Repository: mermaid-js/mermaid
File: packages/mermaid/src/diagrams/flowchart/flowDb.js (JavaScript)

Code before:
 * @param positions
 * @param interp
 */
export const updateLinkInterpolate = function (positions, interp) {
  positions.forEach(function (pos) {
    if (pos === 'default') {
      edges.defaultInterpolate = interp;
    } else {
      edges[pos].interpolate = interp;
    }
  });
};

/**
 * Updates a link with a style
 *
 * @param positions
 * @param style
 */
export const updateLink = function (positions, style) {
  positions.forEach(function (pos) {
    if (pos >= edges.length) {
      let error = new Error(
        `The index for linkStyle is out of bounds. (Help: Ensure that the index is within the range of existing edges.)`
      );
      throw error;
    }
    if (pos === 'default') {
      edges.defaultStyle = style;
    } else {
      if (utils.isSubstringInArray('fill', style) === -1) {
        style.push('fill:none');
      }
      edges[pos].style = style;
    }
  });
};

export const addClass = function (ids, style) {
  ids.split(',').forEach(function (id) {
    if (classes[id] === undefined) {
      classes[id] = { id, styles: [], textStyles: [] };
    }

    if (style !== undefined && style !== null) {
      style.forEach(function (s) {
        if (s.match('color')) {
          const newStyle = s.replace('fill', 'bgFill').replace('color', 'fill');
          classes[id].textStyles.push(newStyle);
        }
        classes[id].styles.push(s);

Review comment (suggestion):
```suggestion
      throw new Error(
        `The index for linkStyle is out of bounds. (Help: Ensure that the index is within the range of existing edges.)`
      );
```

Code after:
 * @param positions
 * @param interp
 */
export const updateLinkInterpolate = function (positions, interp) {
  positions.forEach(function (pos) {
    if (pos === 'default') {
      edges.defaultInterpolate = interp;
    } else {
      edges[pos].interpolate = interp;
    }
  });
};

/**
 * Updates a link with a style
 *
 * @param positions
 * @param style
 */
export const updateLink = function (positions, style) {
  positions.forEach(function (pos) {
    if (pos >= edges.length) {
      throw new Error(
        `The index ${pos} for linkStyle is out of bounds. Valid indices for linkStyle are between 0 and ${
          edges.length - 1
        }. (Help: Ensure that the index is within the range of existing edges.)`
      );
    }
    if (pos === 'default') {
      edges.defaultStyle = style;
    } else {
      if (utils.isSubstringInArray('fill', style) === -1) {
        style.push('fill:none');
      }
      edges[pos].style = style;
    }
  });
};

export const addClass = function (ids, style) {
  ids.split(',').forEach(function (id) {
    if (classes[id] === undefined) {
      classes[id] = { id, styles: [], textStyles: [] };
    }

    if (style !== undefined && style !== null) {
      style.forEach(function (s) {
        if (s.match('color')) {
          const newStyle = s.replace('fill', 'bgFill').replace('color', 'fill');
          classes[id].textStyles.push(newStyle);
        }
Repository: mermaid-js/mermaid
File: packages/mermaid/src/diagrams/flowchart/parser/flow-style.spec.js (JavaScript)

Code before:
        'A-->B7\n' +
        'A-->B8\n' +
        'A-->B9\n' +
        'A-->B10\n' +
        'A-->B11\n' +
        'linkStyle 10 stroke-width:1px;'
    );

    const vert = flow.parser.yy.getVertices();
    const edges = flow.parser.yy.getEdges();

    expect(edges[0].type).toBe('arrow_point');
  });

  it('should handle style definitions within number of edges', function () {
    try {
      flow.parser.parse(`graph TD
      A-->B
      linkStyle 1 stroke-width:1px;`);
      // Fail test if above expression doesn't throw anything.
      expect(true).toBe(false);
    } catch (e) {
      expect(e.message).toBe(
        `The index for linkStyle is out of bounds. (Help: Ensure that the index is within the range of existing edges.)`
      );
    }
  });

  it('should handle style definitions within number of edges', function () {
    const res = flow.parser.parse(`graph TD
    A-->B
    linkStyle 0 stroke-width:1px;`);

    const edges = flow.parser.yy.getEdges();

    expect(edges[0].style[0]).toBe('stroke-width:1px');
  });

  it('should handle multi-numbered style definitions with more then 1 digit in a row', function () {
    const res = flow.parser.parse(
      'graph TD\n' +
        'A-->B1\n' +
        'A-->B2\n' +
        'A-->B3\n' +
        'A-->B4\n' +
        'A-->B5\n' +
        'A-->B6\n' +
        'A-->B7\n' +
        'A-->B8\n' +
        'A-->B9\n' +
        'A-->B10\n' +

Review comment (suggestion):
```suggestion
      expect(() => flow.parser.parse(`graph TD
      A-->B
      linkStyle 1 stroke-width:1px;`).toThrow('The index for linkStyle is out of bounds. (Help: Ensure that the index is within the range of existing edges.)');
```

Code after:
        'A-->B7\n' +
        'A-->B8\n' +
        'A-->B9\n' +
        'A-->B10\n' +
        'A-->B11\n' +
        'linkStyle 10 stroke-width:1px;'
    );

    const vert = flow.parser.yy.getVertices();
    const edges = flow.parser.yy.getEdges();

    expect(edges[0].type).toBe('arrow_point');
  });

  it('should handle style definitions within number of edges', function () {
    expect(() =>
      flow.parser
        .parse(
          `graph TD
    A-->B
    linkStyle 1 stroke-width:1px;`
        )
        .toThrow(
          'The index 1 for linkStyle is out of bounds. Valid indices for linkStyle are between 0 and 0. (Help: Ensure that the index is within the range of existing edges.)'
        )
    );
  });

  it('should handle style definitions within number of edges', function () {
    const res = flow.parser.parse(`graph TD
    A-->B
    linkStyle 0 stroke-width:1px;`);

    const edges = flow.parser.yy.getEdges();

    expect(edges[0].style[0]).toBe('stroke-width:1px');
  });

  it('should handle multi-numbered style definitions with more then 1 digit in a row', function () {
    const res = flow.parser.parse(
      'graph TD\n' +
        'A-->B1\n' +
        'A-->B2\n' +
        'A-->B3\n' +
        'A-->B4\n' +
        'A-->B5\n' +
        'A-->B6\n' +
        'A-->B7\n' +
        'A-->B8\n' +
        'A-->B9\n' +
        'A-->B10\n' +
Repository: mermaid-js/mermaid
File: packages/mermaid/src/utils/imperativeState.ts (TypeScript)

Code before:
export const createImperativeState = <S extends Record<string, unknown>>(init: () => S) => {
  const state = init();

  return {
    get records() {
      return state;
    },
    reset: () => {
      Object.keys(state).forEach((key) => {
        delete state[key];
      });
      Object.entries(init()).forEach(([key, value]: [keyof S, any]) => {
        state[key] = value;
      });
    },
  };
};

export const domain = {
  optional: <V>(value?: V) => value,
  identity: <V>(value: V) => value,
} as const;

/*
const state = createImperativeState(() => ({
  foo: domain.optional<string>(),

Review comment (suggestion):
I feel like this will probably be nicer as an ES6 class, e.g. something like:

```typescript
/**
 * Resettable state storage.
 *
 * @example
 * ```
 * // you can use the `domain` functions for easier typing
 * const state = new ImperativeState(() => {
 *   foo: domain.optional<string>("default"),
 *   bar: domain.identity<number[]>([]),
 *   baz: domain.optional(1),
 * });
 *
 * state.records.foo = "hi";
 * console.log(state.records.foo); // prints "hi";
 * state.reset();
 * console.log(state.records.foo); // prints "default";
 *
 * // typeof state.records:
 * // {
 * //   foo: string | undefined, // actual: undefined
 * //   bar: number[],           // actual: []
 * //   baz: number | undefined, // actual: 1
 * // }
 * ```
 */
export class ImperativeState<S extends Record<string, unknown>> {
  init: () => S;
  records: S;

  /**
   * @param init - Function that creates the default state.
   */
  constructor(init: () => S) {
    this.init = init;
    this.records = init();
  }

  reset() {
    Object.keys(this.records).forEach((key) => {
      delete this.records[key];
    });
    Object.entries(this.init()).forEach(
      ([key, value]: [
        keyof S,
        any // eslint-disable-line @typescript-eslint/no-explicit-any
      ]) => {
        this.records[key] = value;
      }
    );
  }
}
```

The behavior will then be exactly the same, but it will match the normal JavaScript/TypeScript code-style that most projects use.

I've also got an example commit at https://github.com/aloisklink/mermaid/commit/35191a611947ff152eaaf849c8d006a387b6dccb that we could push to this branch.

Code after:
/**
 * Resettable state storage.
 * @example
 * ```
 * const state = new ImperativeState(() => {
 *   foo: undefined as string | undefined,
 *   bar: [] as number[],
 *   baz: 1 as number | undefined,
 * });
 *
 * state.records.foo = "hi";
 * console.log(state.records.foo); // prints "hi";
 * state.reset();
 * console.log(state.records.foo); // prints "default";
 *
 * // typeof state.records:
 * // {
 * //   foo: string | undefined, // actual: undefined
 * //   bar: number[],           // actual: []
 * //   baz: number | undefined, // actual: 1
 * // }
 * ```
 */
export class ImperativeState<S extends Record<string, unknown>> {
  public records: S;