ronantakizawa/codereview-bench huggingface dataset:
Repository: mermaid-js/mermaid
File: packages/mermaid/src/config.spec.ts (TypeScript)

Code before:

    const userConfig1 = configApi.getUserDefinedConfig();
    const userConfig2 = configApi.getUserDefinedConfig();

    userConfig1.theme = 'neutral';
    userConfig1.flowchart!.nodeSpacing = 999;

    expect(userConfig2.theme).toBe('dark');
    expect(userConfig2.flowchart!.nodeSpacing).toBe(50);
  });

  it('should handle edge cases with undefined values', () => {
    const initConfig: MermaidConfig = { theme: 'dark', layout: undefined };
    const directive: MermaidConfig = { fontSize: 14, fontFamily: undefined };

    configApi.saveConfigFromInitialize(initConfig);
    configApi.addDirective(directive);

    const userConfig = configApi.getUserDefinedConfig();
    expect(userConfig.theme).toBe('dark');
    expect(userConfig.layout).toBeUndefined();
    expect(userConfig.fontFamily).toBeUndefined();
    expect(userConfig.fontSize).toBe(14);
  });

  it('should work correctly after reset', () => {
    const initConfig: MermaidConfig = { theme: 'dark' };
    const directive: MermaidConfig = { layout: 'elk' };

    configApi.saveConfigFromInitialize(initConfig);
    configApi.addDirective(directive);

    let userConfig = configApi.getUserDefinedConfig();
    expect(userConfig).toEqual({ theme: 'dark', layout: 'elk' });

    configApi.reset();

    userConfig = configApi.getUserDefinedConfig();
    expect(userConfig).toEqual({ theme: 'dark' });
  });
});


Review comment (suggestion):
"work correctly" doesn't define the behaviour, so if someone checks this test later, they don't know what "correct" is. 

```suggestion
  it('should retain config from initialize after reset', () => {
```

Code after:
        "fontSize": 12,
        "mindmap": {
          "padding": 15,
        },
        "theme": "default",
      }
    `);
  });

  it('should return independent copies (not references)', () => {
    const initConfig: MermaidConfig = { theme: 'dark', flowchart: { nodeSpacing: 50 } };
    configApi.saveConfigFromInitialize(initConfig);

    const userConfig1 = configApi.getUserDefinedConfig();
    const userConfig2 = configApi.getUserDefinedConfig();

    userConfig1.theme = 'neutral';
    userConfig1.flowchart!.nodeSpacing = 999;

    expect(userConfig2).toMatchInlineSnapshot(`
      {
        "flowchart": {
          "nodeSpacing": 50,
        },
        "theme": "dark",
      }
    `);
  });

  it('should handle edge cases with undefined values', () => {
    const initConfig: MermaidConfig = { theme: 'dark', layout: undefined };
    const directive: MermaidConfig = { fontSize: 14, fontFamily: undefined };

    configApi.saveConfigFromInitialize(initConfig);
    configApi.addDirective(directive);

    expect(configApi.getUserDefinedConfig()).toMatchInlineSnapshot(`
      {
        "fontSize": 14,
        "layout": undefined,
        "theme": "dark",
      }
    `);
  });

  it('should retain config from initialize after reset', () => {
    const initConfig: MermaidConfig = { theme: 'dark' };
    const directive: MermaidConfig = { layout: 'elk' };

    configApi.saveConfigFromInitialize(initConfig);
    configApi.addDirective(directive);
Repository: mermaid-js/mermaid
File: packages/mermaid/src/config.spec.ts (TypeScript)

Code before:
    expect(userConfig2.flowchart!.nodeSpacing).toBe(50);
  });

  it('should handle edge cases with undefined values', () => {
    const initConfig: MermaidConfig = { theme: 'dark', layout: undefined };
    const directive: MermaidConfig = { fontSize: 14, fontFamily: undefined };

    configApi.saveConfigFromInitialize(initConfig);
    configApi.addDirective(directive);

    const userConfig = configApi.getUserDefinedConfig();
    expect(userConfig.theme).toBe('dark');
    expect(userConfig.layout).toBeUndefined();
    expect(userConfig.fontFamily).toBeUndefined();
    expect(userConfig.fontSize).toBe(14);
  });

  it('should work correctly after reset', () => {
    const initConfig: MermaidConfig = { theme: 'dark' };
    const directive: MermaidConfig = { layout: 'elk' };

    configApi.saveConfigFromInitialize(initConfig);
    configApi.addDirective(directive);

    let userConfig = configApi.getUserDefinedConfig();
    expect(userConfig).toEqual({ theme: 'dark', layout: 'elk' });

    configApi.reset();

    userConfig = configApi.getUserDefinedConfig();
    expect(userConfig).toEqual({ theme: 'dark' });
  });
});


Review comment (suggestion):
```suggestion
    expect(configApi.getUserDefinedConfig()).toMatchInlineSnapshot();
```

You can use this pattern to make writing the tests easier. 

Code after:

  it('should return independent copies (not references)', () => {
    const initConfig: MermaidConfig = { theme: 'dark', flowchart: { nodeSpacing: 50 } };
    configApi.saveConfigFromInitialize(initConfig);

    const userConfig1 = configApi.getUserDefinedConfig();
    const userConfig2 = configApi.getUserDefinedConfig();

    userConfig1.theme = 'neutral';
    userConfig1.flowchart!.nodeSpacing = 999;

    expect(userConfig2).toMatchInlineSnapshot(`
      {
        "flowchart": {
          "nodeSpacing": 50,
        },
        "theme": "dark",
      }
    `);
  });

  it('should handle edge cases with undefined values', () => {
    const initConfig: MermaidConfig = { theme: 'dark', layout: undefined };
    const directive: MermaidConfig = { fontSize: 14, fontFamily: undefined };

    configApi.saveConfigFromInitialize(initConfig);
    configApi.addDirective(directive);

    expect(configApi.getUserDefinedConfig()).toMatchInlineSnapshot(`
      {
        "fontSize": 14,
        "layout": undefined,
        "theme": "dark",
      }
    `);
  });

  it('should retain config from initialize after reset', () => {
    const initConfig: MermaidConfig = { theme: 'dark' };
    const directive: MermaidConfig = { layout: 'elk' };

    configApi.saveConfigFromInitialize(initConfig);
    configApi.addDirective(directive);

    expect(configApi.getUserDefinedConfig()).toMatchInlineSnapshot(`
      {
        "layout": "elk",
        "theme": "dark",
      }
    `);
Repository: mermaid-js/mermaid
File: packages/mermaid/src/dagre-wrapper/shapes/note.js (JavaScript)

Code before:
import { updateNodeBounds, labelHelper } from './util.js';
import { log } from '../../logger.js';
import { getConfig } from '../../diagram-api/diagramAPI.js';
import { getEffectiveHtmlLabels } from '../../config.js';
import intersect from '../intersect/index.js';

const note = async (parent, node) => {
  const useHtmlLabels = getEffectiveHtmlLabels(getConfig()) || node.useHtmlLabels;
  if (!useHtmlLabels) {
    node.centerLabel = true;
  }
  const { shapeSvg, bbox, halfPadding } = await labelHelper(
    parent,
    node,
    'node ' + node.classes,
    true
  );

  log.info('Classes = ', node.classes);
  // add the rect
  const rect = shapeSvg.insert('rect', ':first-child');

  rect
    .attr('rx', node.rx)
    .attr('ry', node.ry)
    .attr('x', -bbox.width / 2 - halfPadding)
    .attr('y', -bbox.height / 2 - halfPadding)
    .attr('width', bbox.width + node.padding)
    .attr('height', bbox.height + node.padding);

  updateNodeBounds(node, rect);

  node.intersect = function (point) {

Review comment (suggestion):
**issue(perf)**: Is there any reason why we've inverted the condition here?

Using `node.useHtmlLabels` first should be a little bit faster if it's `false`, since it will short-circuit and prevent calling `getEffectiveHtmlLabels(getConfig())`.

**suggestion**:


```suggestion
  const useHtmlLabels = node.useHtmlLabels || getEffectiveHtmlLabels(getConfig());
```

Code after:
import { updateNodeBounds, labelHelper } from './util.js';
import { log } from '../../logger.js';
import { getConfig } from '../../diagram-api/diagramAPI.js';
import { getEffectiveHtmlLabels } from '../../config.js';
import intersect from '../intersect/index.js';

const note = async (parent, node) => {
  const useHtmlLabels = node.useHtmlLabels || getEffectiveHtmlLabels(getConfig());
  if (!useHtmlLabels) {
    node.centerLabel = true;
  }
  const { shapeSvg, bbox, halfPadding } = await labelHelper(
    parent,
    node,
    'node ' + node.classes,
    true
  );

  log.info('Classes = ', node.classes);
  // add the rect
  const rect = shapeSvg.insert('rect', ':first-child');

  rect
    .attr('rx', node.rx)
    .attr('ry', node.ry)
    .attr('x', -bbox.width / 2 - halfPadding)
    .attr('y', -bbox.height / 2 - halfPadding)
    .attr('width', bbox.width + node.padding)
    .attr('height', bbox.height + node.padding);

  updateNodeBounds(node, rect);

  node.intersect = function (point) {
Repository: mermaid-js/mermaid
File: packages/mermaid/src/dagre-wrapper/shapes/note.js (JavaScript)

Code before:
import { updateNodeBounds, labelHelper } from './util.js';
import { log } from '../../logger.js';
import { getConfig } from '../../diagram-api/diagramAPI.js';
import { getEffectiveHtmlLabels } from '../../config.js';
import intersect from '../intersect/index.js';

const note = async (parent, node) => {
  const useHtmlLabels = node.useHtmlLabels ?? getEffectiveHtmlLabels(getConfig());
  if (!useHtmlLabels) {
    node.centerLabel = true;
  }
  const { shapeSvg, bbox, halfPadding } = await labelHelper(
    parent,
    node,
    'node ' + node.classes,
    true
  );

  log.info('Classes = ', node.classes);
  // add the rect
  const rect = shapeSvg.insert('rect', ':first-child');

  rect
    .attr('rx', node.rx)
    .attr('ry', node.ry)
    .attr('x', -bbox.width / 2 - halfPadding)
    .attr('y', -bbox.height / 2 - halfPadding)
    .attr('width', bbox.width + node.padding)
    .attr('height', bbox.height + node.padding);

  updateNodeBounds(node, rect);

  node.intersect = function (point) {

Review comment (suggestion):
**issue**: I don't know if this is blocking or not, but using `??` instead of `||` doesn't match the existing code, so it might cause some additional changes in behaviour.

Is there any reason why we should pick `??` over `||`? From looking at the code, I think it's safe (since we never actually set `node.useHtmlLabels` to `false` normally), but it's probably easier to change it in a separate PR where we can do it all at once.



Code after:
import { updateNodeBounds, labelHelper } from './util.js';
import { log } from '../../logger.js';
import { getConfig } from '../../diagram-api/diagramAPI.js';
import { getEffectiveHtmlLabels } from '../../config.js';
import intersect from '../intersect/index.js';

const note = async (parent, node) => {
  const useHtmlLabels = node.useHtmlLabels || getEffectiveHtmlLabels(getConfig());
  if (!useHtmlLabels) {
    node.centerLabel = true;
  }
  const { shapeSvg, bbox, halfPadding } = await labelHelper(
    parent,
    node,
    'node ' + node.classes,
    true
  );

  log.info('Classes = ', node.classes);
  // add the rect
  const rect = shapeSvg.insert('rect', ':first-child');

  rect
    .attr('rx', node.rx)
    .attr('ry', node.ry)
    .attr('x', -bbox.width / 2 - halfPadding)
    .attr('y', -bbox.height / 2 - halfPadding)
    .attr('width', bbox.width + node.padding)
    .attr('height', bbox.height + node.padding);

  updateNodeBounds(node, rect);

  node.intersect = function (point) {
Repository: mermaid-js/mermaid
File: packages/mermaid/src/dagre-wrapper/nodes.js (JavaScript)

Code before:
import { select } from 'd3';
import { getConfig } from '../diagram-api/diagramAPI.js';
import { getEffectiveHtmlLabels } from '../config.js';

import { log } from '../logger.js';
import { getArrowPoints } from './blockArrowHelper.js';
import createLabel from './createLabel.js';
import intersect from './intersect/index.js';
import note from './shapes/note.js';
import { insertPolygonShape, labelHelper, updateNodeBounds } from './shapes/util.js';

const formatClass = (str) => {
  if (str) {
    return ' ' + str;
  }
  return '';
};
const getClassesFromNode = (node, otherClasses) => {
  return `${otherClasses ? otherClasses : 'node default'}${formatClass(node.classes)} ${formatClass(
    node.class
  )}`;
};

const question = async (parent, node) => {
  const { shapeSvg, bbox } = await labelHelper(
    parent,
    node,
    getClassesFromNode(node, undefined),
    true

Review comment (refactor):
**todo**: Please remove this extra newline:

```suggestion
```

Code after:
import { select } from 'd3';
import { getConfig } from '../diagram-api/diagramAPI.js';
import { getEffectiveHtmlLabels } from '../config.js';
import { log } from '../logger.js';
import { getArrowPoints } from './blockArrowHelper.js';
import createLabel from './createLabel.js';
import intersect from './intersect/index.js';
import note from './shapes/note.js';
import { insertPolygonShape, labelHelper, updateNodeBounds } from './shapes/util.js';

const formatClass = (str) => {
  if (str) {
    return ' ' + str;
  }
  return '';
};
const getClassesFromNode = (node, otherClasses) => {
  return `${otherClasses ? otherClasses : 'node default'}${formatClass(node.classes)} ${formatClass(
    node.class
  )}`;
};

const question = async (parent, node) => {
  const { shapeSvg, bbox } = await labelHelper(
    parent,
    node,
    getClassesFromNode(node, undefined),
    true
  );
Repository: mermaid-js/mermaid
File: packages/mermaid/src/rendering-util/rendering-elements/shapes/erBox.ts (TypeScript)

Code before:
    await createText(
      label,
      labelText,
      {
        width: calculateTextWidth(labelText, config) + 100,
        style,
        useHtmlLabels: config.htmlLabels ?? undefined,
      },
      config
    )
  );
  // Undo work around now that text passed through correctly
  // Only do this for HTML labels, as SVG text elements don't have children
  const useHtmlLabels = config.htmlLabels ?? undefined;
  if (useHtmlLabels && (labelText.includes('&lt;') || labelText.includes('&gt;'))) {
    let child = text.children[0];
    child.textContent = child.textContent.replaceAll('&lt;', '<').replaceAll('&gt;', '>');
    while (child.childNodes[0]) {
      child = child.childNodes[0];
      // Replace its text content
      child.textContent = child.textContent.replaceAll('&lt;', '<').replaceAll('&gt;', '>');
    }
  }

  let bbox = text.getBBox();
  if (getEffectiveHtmlLabels(getConfig())) {
    const div = text.children[0];
    if (div) {
      div.style.textAlign = 'start';
      const dv = select(text);
      bbox = div.getBoundingClientRect();
      dv.attr('width', bbox.width);
      dv.attr('height', bbox.height);
    }
  }

  return bbox;
}


Review comment (suggestion):
**issue(blocking)**: This value is different from the previous `useHtmlLabels`.

I think this might be causing the following diagram to break:

````markdown
```mermaid
---
config:
    flowchart:
        htmlLabels: false
---
erDiagram
    CUSTOMER {
        string id
    }
```
````

<img width="1733" height="953" alt="Image" src="https://github.com/user-attachments/assets/fd787040-620b-43c4-813c-de413c4849d0" />

Code after:
      labelText,
      {
        width: calculateTextWidth(labelText, config) + 100,
        style,
        useHtmlLabels: config.htmlLabels,
      },
      config
    )
  );
  // Undo work around now that text passed through correctly
  if (labelText.includes('&lt;') || labelText.includes('&gt;')) {
    let child = text.children[0];
    child.textContent = child.textContent.replaceAll('&lt;', '<').replaceAll('&gt;', '>');
    while (child.childNodes[0]) {
      child = child.childNodes[0];
      // Replace its text content
      child.textContent = child.textContent.replaceAll('&lt;', '<').replaceAll('&gt;', '>');
    }
  }

  let bbox = text.getBBox();
  if (evaluate(config.htmlLabels)) {
    const div = text.children[0];
    div.style.textAlign = 'start';
    const dv = select(text);
    bbox = div.getBoundingClientRect();
    dv.attr('width', bbox.width);
    dv.attr('height', bbox.height);
  }

  return bbox;
}
Repository: mermaid-js/mermaid
File: packages/parser/src/language/architecture/valueConverter.ts (TypeScript)

Code before:
import type { CstNode, GrammarAST, ValueType } from 'langium';

import { AbstractMermaidValueConverter } from '../common/index.js';

export class ArchitectureValueConverter extends AbstractMermaidValueConverter {
  protected runCustomConverter(
    rule: GrammarAST.AbstractRule,
    input: string,
    _cstNode: CstNode
  ): ValueType | undefined {
    if (rule.name === 'ARCH_ICON') {
      return input.replace(/[()]/g, '').trim();
    } else if (rule.name === 'ARCH_TEXT_ICON') {
      return input.replace(/["()]/g, '');
    } else if (rule.name === 'ARCH_TITLE') {
      return input.replace(/["'[\]]/g, '').trim();
    }
    return undefined;
  }
}


Review comment (security):
```suggestion
      let result = input.replace(/^\[|]$/g, '').trim();
      
      // Check if wrapped in quotes and remove only outer quotes
      if ((result.startsWith('"') && result.endsWith('"')) || 
          (result.startsWith("'") && result.endsWith("'"))) {
        result = result.slice(1, -1);
        // Unescape escaped quotes
        result = result.replace(/\\"/g, '"').replace(/\\'/g, "'");
      }
      
      return result.trim();;
```

**Critical Issue:**
This regex removes **ALL** quotes and apostrophes from the string, not just the outer ones. This causes problems with:
- Titles containing apostrophes: `["John's Database"]` → `Johns Database` ❌
- Titles with inner quotes: `["The "Main" API"]` → `The Main API` ❌

Code after:
import type { CstNode, GrammarAST, ValueType } from 'langium';

import { AbstractMermaidValueConverter } from '../common/index.js';

export class ArchitectureValueConverter extends AbstractMermaidValueConverter {
  protected runCustomConverter(
    rule: GrammarAST.AbstractRule,
    input: string,
    _cstNode: CstNode
  ): ValueType | undefined {
    if (rule.name === 'ARCH_ICON') {
      return input.replace(/[()]/g, '').trim();
    } else if (rule.name === 'ARCH_TEXT_ICON') {
      return input.replace(/["()]/g, '');
    } else if (rule.name === 'ARCH_TITLE') {
      let result = input.replace(/^\[|]$/g, '').trim();
      // Check if wrapped in quotes and remove only outer quotes
      if (
        (result.startsWith('"') && result.endsWith('"')) ||
        (result.startsWith("'") && result.endsWith("'"))
      ) {
        result = result.slice(1, -1);
        // Unescape escaped quotes
        result = result.replace(/\\"/g, '"').replace(/\\'/g, "'");
      }
      return result.trim();
    }
    return undefined;
  }
}
Repository: mermaid-js/mermaid
File: packages/parser/tests/architecture.test.ts (TypeScript)

Code before:

    it('should handle service with unquoted title (backward compatibility)', () => {
      const context = `architecture-beta
            service db(database)[Database] in api
            `;
      const result = parse(context);
      expectNoErrorsOrAlternatives(result);
      expect(result.value.$type).toBe(Architecture);

      const service = result.value.services?.[0];
      expect(service).toBeDefined();
      expect(service?.title).toBe('Database');
    });

    it('should handle group with quoted title', () => {
      const context = `architecture-beta
            group api(cloud)["API"]
            `;
      const result = parse(context);
      expectNoErrorsOrAlternatives(result);
      expect(result.value.$type).toBe(Architecture);

      const group = result.value.groups?.[0];
      expect(group).toBeDefined();
      expect(group?.title).toBe('API');
    });
  });
});


Review comment (suggestion):
Add the test cases for this suggestion
https://github.com/mermaid-js/mermaid/pull/7310/changes#r2686025514

Code after:

    it('should handle service with unquoted title (backward compatibility)', () => {
      const context = `architecture-beta
            service db(database)[Database] in api
            `;
      const result = parse(context);
      expectNoErrorsOrAlternatives(result);
      expect(result.value.$type).toBe(Architecture);

      const service = result.value.services?.[0];
      expect(service).toBeDefined();
      expect(service?.title).toBe('Database');
    });

    it('should handle group with quoted title', () => {
      const context = `architecture-beta
            group api(cloud)["API"]
            `;
      const result = parse(context);
      expectNoErrorsOrAlternatives(result);
      expect(result.value.$type).toBe(Architecture);

      const group = result.value.groups?.[0];
      expect(group).toBeDefined();
      expect(group?.title).toBe('API');
    });
    it('should preserve apostrophes in service titles', () => {
      const context = `architecture-beta
            service db(database)["John's Database"] in api
            `;
      const result = parse(context);
      expectNoErrorsOrAlternatives(result);
      expect(result.value.$type).toBe(Architecture);

      const service = result.value.services?.[0];
      expect(service).toBeDefined();
      expect(service?.title).toBe("John's Database");
    });

    it('should preserve inner quotes in service titles when escaped', () => {
      const context = `architecture-beta
            service api(server)["The \\"Main\\" API"] in cloud
            `;
      const result = parse(context);
      expectNoErrorsOrAlternatives(result);
      expect(result.value.$type).toBe(Architecture);

      const service = result.value.services?.[0];
      expect(service).toBeDefined();
      expect(service?.title).toBe('The "Main" API');
    });
Repository: meta-llama/llama-stack
File: src/llama_stack/providers/remote/inference/bedrock/config.py (Python)

Code before:
# Copyright (c) Meta Platforms, Inc. and affiliates.
# All rights reserved.
#
# This source code is licensed under the terms described in the LICENSE file in
# the root directory of this source tree.

import os

from pydantic import BaseModel, Field

from llama_stack.providers.utils.inference.model_registry import RemoteInferenceProviderConfig


class BedrockProviderDataValidator(BaseModel):
    aws_bedrock_api_key: str | None = Field(
        default=None,
        description="API key for Amazon Bedrock",
    )


class BedrockConfig(RemoteInferenceProviderConfig):
    api_key: str | None = Field(
        default_factory=lambda: os.getenv("AWS_BEDROCK_API_KEY"),
        description="Amazon Bedrock API key",
    )
    region_name: str = Field(
        default_factory=lambda: os.getenv("AWS_DEFAULT_REGION", "us-east-2"),
        description="AWS Region for the Bedrock Runtime endpoint",
    )

    @classmethod
    def sample_run_config(cls, **kwargs):
        return {
            "api_key": "${env.AWS_BEDROCK_API_KEY:=}",
            "region_name": "${env.AWS_DEFAULT_REGION:=us-east-2}",
        }


Review comment (security):
use `auth_credential` instead of a new `api_key` field, see https://github.com/llamastack/llama-stack/blob/main/src/llama_stack/providers/utils/inference/model_registry.py#L22

Code after:
# Copyright (c) Meta Platforms, Inc. and affiliates.
# All rights reserved.
#
# This source code is licensed under the terms described in the LICENSE file in
# the root directory of this source tree.

import os

from pydantic import BaseModel, Field

from llama_stack.providers.utils.inference.model_registry import RemoteInferenceProviderConfig


class BedrockProviderDataValidator(BaseModel):
    aws_bedrock_api_key: str | None = Field(
        default=None,
        description="API key for Amazon Bedrock",
    )


class BedrockConfig(RemoteInferenceProviderConfig):
    region_name: str = Field(
        default_factory=lambda: os.getenv("AWS_DEFAULT_REGION", "us-east-2"),
        description="AWS Region for the Bedrock Runtime endpoint",
    )

    @classmethod
    def sample_run_config(cls, **kwargs):
        return {
            "api_key": "${env.AWS_BEDROCK_API_KEY:=}",
            "region_name": "${env.AWS_DEFAULT_REGION:=us-east-2}",
        }