ronantakizawa/codereview-bench huggingface dataset:
Repository: answerdev/answer
File: ui/src/pages/Install/components/SecondStep/index.tsx (TypeScript)

Code before:
        errorMsg: t('db_file.msg'),
      };
    }
    changeCallback({
      ...data,
    });
    return bol;
  };

  const handleSubmit = (event: FormEvent) => {
    event.preventDefault();
    event.stopPropagation();
    if (!checkValidated()) {
      return;
    }
    nextCallback();
  };

  //checkbox for sslmode enabled or not
  const [checked, setChecked] = useState(false);

  const handleChange = () => {
    // Change state to the opposite when checkbox changes
    setChecked(!checked);
  };

  if (!visible) return null;
  return (
    <Form noValidate onSubmit={handleSubmit}>
      <Form.Group controlId="database_engine" className="mb-3">
        <Form.Label>{t('db_type.label')}</Form.Label>
        <Form.Select
          value={data.db_type.value}
          isInvalid={data.db_type.isInvalid}
          onChange={(e) => {
            changeCallback({
              db_type: {
                value: e.target.value,
                isInvalid: false,
                errorMsg: '',
              },
            });
          }}>
          {sqlData.map((item) => {
            return (
              <option key={item.value} value={item.value}>
                {item.label}
              </option>
            );
          })}
        </Form.Select>

Review comment (suggestion):
Your customized handlechange function doesn't pass the required content, it just saves it on the current page, so you need to add the parameters on the `/Install/index.tsxt`.
![image](https://github.com/user-attachments/assets/38356dae-c2ee-4a16-a1ee-f7db9be2aad4)


Code after:
      if (!db_host.value) {
        bol = false;
        data.db_host = {
          value: '',
          isInvalid: true,
          errorMsg: t('db_host.msg'),
        };
      }
      if (!db_name.value) {
        bol = false;
        data.db_name = {
          value: '',
          isInvalid: true,
          errorMsg: t('db_name.msg'),
        };
      }
      if (db_type.value === 'postgres') {
        if (ssl_enabled.value && ssl_mode.value !== 'require') {
          if (!key_file.value) {
            bol = false;
            data.key_file = {
              value: '',
              isInvalid: true,
              errorMsg: t('key_file.msg'),
            };
          }
          if (!pem_file.value) {
            bol = false;
            data.pem_file = {
              value: '',
              isInvalid: true,
              errorMsg: t('pem_file.msg'),
            };
          }
          if (!cert_file.value) {
            bol = false;
            data.cert_file = {
              value: '',
              isInvalid: true,
              errorMsg: t('cert_file.msg'),
            };
          }
        }
      }
    } else if (!db_file.value) {
      bol = false;
      data.db_file = {
        value: '',
        isInvalid: true,
        errorMsg: t('db_file.msg'),
      };
Repository: answerdev/answer
File: ui/src/pages/Install/components/SecondStep/index.tsx (TypeScript)

Code before:
      if (ssl_mode.value === 'verify-ca' || ssl_mode.value === 'verify-full') {
        if (!key_file.value) {
          bol = false;
          data.key_file = {
            value: '',
            isInvalid: true,
            errorMsg: t('key_file.msg'),
          };
        }
        if (!pem_file.value) {
          bol = false;
          data.pem_file = {
            value: '',
            isInvalid: true,
            errorMsg: t('pem_file.msg'),
          };
        }
        if (!cert_file.value) {
          bol = false;
          data.cert_file = {
            value: '',
            isInvalid: true,
            errorMsg: t('cert_file.msg'),
          };
        }
      }
      if (!db_name.value) {
        bol = false;
        data.db_name = {
          value: '',
          isInvalid: true,
          errorMsg: t('db_name.msg'),
        };
      }
    } else if (!db_file.value) {
      bol = false;
      data.db_file = {
        value: '',
        isInvalid: true,
        errorMsg: t('db_file.msg'),
      };
    }
    changeCallback({
      ...data,
    });
    return bol;
  };

  const handleSubmit = (event: FormEvent) => {
    event.preventDefault();
    event.stopPropagation();

Review comment (bug):
You've added the checksum logic here, and there are a couple of problems with it
1.  you should only check for other content if ssl_enabled allows it.
2. the errorMsg is not displayed on the ui when the validation fails.
3.  why did you set the bol value to true when your own implementation didn't pass the checksum? What's the point of adding the checksum?

Consider carefully the checksum logic here.

Example of displaying a message on a page that does not pass validation
![image](https://github.com/user-attachments/assets/db0b2161-7c8f-4997-8935-13f64b1675b6)




Code after:
            };
          }
          if (!pem_file.value) {
            bol = false;
            data.pem_file = {
              value: '',
              isInvalid: true,
              errorMsg: t('pem_file.msg'),
            };
          }
          if (!cert_file.value) {
            bol = false;
            data.cert_file = {
              value: '',
              isInvalid: true,
              errorMsg: t('cert_file.msg'),
            };
          }
        }
      }
    } else if (!db_file.value) {
      bol = false;
      data.db_file = {
        value: '',
        isInvalid: true,
        errorMsg: t('db_file.msg'),
      };
    }
    changeCallback({
      ...data,
    });
    return bol;
  };

  const handleSubmit = (event: FormEvent) => {
    event.preventDefault();
    event.stopPropagation();
    if (!checkValidated()) {
      return;
    }
    nextCallback();
  };

  if (!visible) return null;
  return (
    <Form noValidate onSubmit={handleSubmit}>
      <Form.Group controlId="database_engine" className="mb-3">
        <Form.Label>{t('db_type.label')}</Form.Label>
        <Form.Select
          value={data.db_type.value}
          isInvalid={data.db_type.isInvalid}
Repository: answerdev/answer
File: ui/src/pages/Install/components/SecondStep/index.tsx (TypeScript)

Code before:
          value: '',
          isInvalid: true,
          errorMsg: t('db_host.msg'),
        };
      }
      if (!db_name.value) {
        bol = false;
        data.db_name = {
          value: '',
          isInvalid: true,
          errorMsg: t('db_name.msg'),
        };
      }
      if (db_type.value === 'postgres') {
        if (!ssl_enabled.value) {
          data.ssl_enabled = {
            value: false,
            isInvalid: false,
            errorMsg: t('ssl_enabled.msg'),
          };
        } else if (!ssl_mode.value) {
          data.ssl_mode = {
            value: 'require',
            isInvalid: false,
            errorMsg: '',
          };
        } else if (
          ssl_mode.value === 'verify-ca' ||
          ssl_mode.value === 'verify-full'
        ) {
          if (!key_file.value) {
            bol = false;
            data.key_file = {
              value: '',
              isInvalid: true,
              errorMsg: t('key_file.msg'),
            };
          }
          if (!pem_file.value) {
            bol = false;
            data.pem_file = {
              value: '',
              isInvalid: true,
              errorMsg: t('pem_file.msg'),
            };
          }
          if (!cert_file.value) {
            bol = false;
            data.cert_file = {
              value: '',
              isInvalid: true,

Review comment (suggestion):
The `ssl_enabled` and `ssl_mode ` checksum is not needed and is meaningless, it's just a variable that controls the data underneath.

What needs to be checked is the value of `key_file` and `pen_file` and `cert_file `if ssl_enabled is true and ssl_mode is not required.

In addition, when switching ssl_anabled, please reinitialize these relevant values (ssl_mode、key_file、pen_file、cert_file)


Code after:
          value: '',
          isInvalid: true,
          errorMsg: t('db_host.msg'),
        };
      }
      if (!db_name.value) {
        bol = false;
        data.db_name = {
          value: '',
          isInvalid: true,
          errorMsg: t('db_name.msg'),
        };
      }
      if (db_type.value === 'postgres') {
        if (ssl_enabled.value && ssl_mode.value !== 'require') {
          if (!key_file.value) {
            bol = false;
            data.key_file = {
              value: '',
              isInvalid: true,
              errorMsg: t('key_file.msg'),
            };
          }
          if (!pem_file.value) {
            bol = false;
            data.pem_file = {
              value: '',
              isInvalid: true,
              errorMsg: t('pem_file.msg'),
            };
          }
          if (!cert_file.value) {
            bol = false;
            data.cert_file = {
              value: '',
              isInvalid: true,
              errorMsg: t('cert_file.msg'),
            };
          }
        }
      }
    } else if (!db_file.value) {
      bol = false;
      data.db_file = {
        value: '',
        isInvalid: true,
        errorMsg: t('db_file.msg'),
      };
    }
    changeCallback({
      ...data,
Repository: answerdev/answer
File: plugin/kv_storage.go (Go)

Code before:
			data:           kv.data,
			pluginSlugName: kv.pluginSlugName,
		}
		shouldCommit = true
	}

	if err := fn(ctx, txKv); err != nil {
		return fmt.Errorf("%w: %v", ErrKVTransactionFailed, err)
	}

	if shouldCommit {
		if err := txKv.session.Commit(); err != nil {
			return fmt.Errorf("%w: commit failed: %v", ErrKVTransactionFailed, err)
		}
	}
	return nil
}

// PluginData defines the interface for plugins that need data storage capabilities
type KVStorage interface {
	Info() Info
	SetOperator(operator *KVOperator)
}

var (
	_,
	registerPluginKVStorage = func() (CallFn[KVStorage], RegisterFn[KVStorage]) {
		callFn, registerFn := MakePlugin[KVStorage](false)
		return callFn, func(p KVStorage) {
			registerFn(p)
			kvStoragePluginStack.plugins = append(kvStoragePluginStack.plugins, p)
		}
	}()
	kvStoragePluginStack = &Stack[KVStorage]{}
)

func SetKVStorageDB(data *Data) {
	for _, p := range kvStoragePluginStack.plugins {
		p.SetOperator(&KVOperator{
			data:           data,
			pluginSlugName: p.Info().SlugName,
		})
	}
}


Review comment (suggestion):
This part I think can be consistent with the way other plugins are initialized. Such as

```go
CallKVStorage,
	registerPluginKVStorage = MakePlugin[KVStorage](true)
```


```go
func NewKVOperator(data *Data, pluginSlugName string) *KVOperator {
	return &KVOperator{
		data:           data,
		pluginSlugName: pluginSlugName,
	}
}
```

Code after:
		}
		shouldCommit = true
	}

	if err := fn(ctx, txKv); err != nil {
		return fmt.Errorf("%w: %v", ErrKVTransactionFailed, err)
	}

	if shouldCommit {
		if err := txKv.session.Commit(); err != nil {
			return fmt.Errorf("%w: commit failed: %v", ErrKVTransactionFailed, err)
		}
	}
	return nil
}

// KVStorage defines the interface for plugins that need data storage capabilities
type KVStorage interface {
	Info() Info
	SetOperator(operator *KVOperator)
}

var (
	CallKVStorage,
	registerKVStorage = MakePlugin[KVStorage](true)
)

// NewKVOperator creates a new KV storage operator with the specified database engine, cache and plugin name.
// It returns a KVOperator instance that can be used to interact with the plugin's storage.
func NewKVOperator(db *xorm.Engine, cache cache.Cache, pluginSlugName string) *KVOperator {
	return &KVOperator{
		data:           &Data{DB: db, Cache: cache},
		pluginSlugName: pluginSlugName,
		cacheTTL:       30 * time.Minute,
	}
}
Repository: answerdev/answer
File: internal/service/siteinfo/siteinfo_service.go (Go)

Code before:
	data := &entity.SiteInfo{
		Type:    constant.SiteTypePrivileges,
		Content: string(content),
		Status:  1,
	}
	err = s.siteInfoRepo.SaveByType(ctx, constant.SiteTypePrivileges, data)
	if err != nil {
		return err
	}

	// update privilege in config
	for _, privilege := range choosePrivileges {
		err = s.configService.UpdateConfig(ctx, privilege.Key, fmt.Sprintf("%d", privilege.Value))
		if err != nil {
			return err
		}
	}
	return
}

func (s *SiteInfoService) CleanUpRemovedBrandingFiles(
	ctx context.Context,
	newBranding *schema.SiteBrandingReq,
	currentBranding *schema.SiteBrandingResp,
) error {
	currentFiles := map[string]string{
		"logo":        currentBranding.Logo,
		"mobile_logo": currentBranding.MobileLogo,
		"square_icon": currentBranding.SquareIcon,
		"favicon":     currentBranding.Favicon,
	}

	newFiles := map[string]string{
		"logo":        newBranding.Logo,
		"mobile_logo": newBranding.MobileLogo,
		"square_icon": newBranding.SquareIcon,
		"favicon":     newBranding.Favicon,
	}

	for key, currentFile := range currentFiles {
		newFile := newFiles[key]
		if currentFile != "" && currentFile != newFile {
			fileRecord, err := s.fileRecordService.GetFileRecordByURL(ctx, currentFile)
			if err != nil {
				log.Error(err)
				continue
			}
			if fileRecord == nil {
				log.Error("could not fetch file record by url")
				continue
			}

Review comment (suggestion):
This function always seems to return nil and no error. So maybe you can just make it no return value.

Code after:
	content, _ := json.Marshal(req)
	data := &entity.SiteInfo{
		Type:    constant.SiteTypePrivileges,
		Content: string(content),
		Status:  1,
	}
	err = s.siteInfoRepo.SaveByType(ctx, constant.SiteTypePrivileges, data)
	if err != nil {
		return err
	}

	// update privilege in config
	for _, privilege := range choosePrivileges {
		err = s.configService.UpdateConfig(ctx, privilege.Key, fmt.Sprintf("%d", privilege.Value))
		if err != nil {
			return err
		}
	}
	return
}

func (s *SiteInfoService) CleanUpRemovedBrandingFiles(
	ctx context.Context,
	newBranding *schema.SiteBrandingReq,
	currentBranding *schema.SiteBrandingResp,
) error {
	var allErrors []error
	currentFiles := map[string]string{
		"logo":        currentBranding.Logo,
		"mobile_logo": currentBranding.MobileLogo,
		"square_icon": currentBranding.SquareIcon,
		"favicon":     currentBranding.Favicon,
	}

	newFiles := map[string]string{
		"logo":        newBranding.Logo,
		"mobile_logo": newBranding.MobileLogo,
		"square_icon": newBranding.SquareIcon,
		"favicon":     newBranding.Favicon,
	}

	for key, currentFile := range currentFiles {
		newFile := newFiles[key]
		if currentFile != "" && currentFile != newFile {
			fileRecord, err := s.fileRecordService.GetFileRecordByURL(ctx, currentFile)
			if err != nil {
				allErrors = append(allErrors, err)
				continue
			}
			if fileRecord == nil {
				err := errpkg.New("file record is nil for key " + key)
Repository: answerdev/answer
File: ui/src/components/SchemaForm/components/Input.tsx (TypeScript)

Code before:

import React, { FC } from 'react';
import { Form } from 'react-bootstrap';

import type * as Type from '@/common/interface';

interface Props {
  type: string | undefined;
  placeholder: string | undefined;
  fieldName: string;
  onChange?: (fd: Type.FormDataType) => void;
  formData: Type.FormDataType;
  readOnly: boolean;
  minValue?: number;
  inputMode?:
    | 'text'
    | 'search'
    | 'none'
    | 'tel'
    | 'url'
    | 'email'
    | 'numeric'
    | 'decimal'
    | undefined;
}
const Index: FC<Props> = ({
  type = 'text',
  placeholder = '',
  fieldName,
  onChange,
  formData,
  readOnly = false,
  minValue = 0,
  inputMode = 'text',
}) => {
  const fieldObject = formData[fieldName];
  const handleChange = (evt: React.ChangeEvent<HTMLInputElement>) => {
    const { name, value } = evt.currentTarget;
    const state = {
      ...formData,
      [name]: {
        ...formData[name],
        value,
        isInvalid: false,
      },
    };
    if (typeof onChange === 'function') {
      onChange(state);
    }
  };


Review comment (suggestion):
1. Just added the relevant TypeScript types inside the input component.
2. Missing relevant TypeScript definitions in SchemaForm(`/ui/src/components/SchemaForm/types.ts`), So there will be a TypeScript error when generating forms through configuration. You can conduct relevant tests in `ui/src/pages/Admin/Smtp/index.tsx`
eg: 
<img width="583" height="481" alt="image" src="https://github.com/user-attachments/assets/1fe2efd3-8856-4363-a5e5-dd243bcb0e8e" />

3. According to the requirement description, it is also necessary to support the max attribute.


Code after:

import React, { FC } from 'react';
import { Form } from 'react-bootstrap';

import type * as Type from '@/common/interface';

interface Props {
  type: string | undefined;
  placeholder: string | undefined;
  fieldName: string;
  onChange?: (fd: Type.FormDataType) => void;
  formData: Type.FormDataType;
  readOnly: boolean;
  min?: number;
  max?: number;
  inputMode?:
    | 'text'
    | 'search'
    | 'none'
    | 'tel'
    | 'url'
    | 'email'
    | 'numeric'
    | 'decimal'
    | undefined;
}
const Index: FC<Props> = ({
  type = 'text',
  placeholder = '',
  fieldName,
  onChange,
  formData,
  readOnly = false,
  min = 0,
  max,
  inputMode = 'text',
}) => {
  const fieldObject = formData[fieldName];
  const numberInputProps =
    type === 'number'
      ? { min, ...(max != null && max > 0 ? { max } : {}) }
      : {};
  const handleChange = (evt: React.ChangeEvent<HTMLInputElement>) => {
    const { name, value } = evt.currentTarget;
    const state = {
      ...formData,
      [name]: {
        ...formData[name],
        value,
        isInvalid: false,
      },
Repository: answerdev/answer
File: ui/src/components/SchemaForm/components/Input.tsx (TypeScript)

Code before:
    const { name, value } = evt.currentTarget;
    const state = {
      ...formData,
      [name]: {
        ...formData[name],
        value,
        isInvalid: false,
      },
    };
    if (typeof onChange === 'function') {
      onChange(state);
    }
  };

  return (
    <Form.Control
      name={fieldName}
      placeholder={placeholder}
      type={type}
      value={fieldObject?.value || ''}
      min={min}
      max={max}
      inputMode={inputMode}
      onChange={handleChange}
      disabled={readOnly}
      isInvalid={fieldObject?.isInvalid}
      style={type === 'color' ? { width: '100px', flex: 'none' } : {}}
    />
  );
};

export default Index;


Review comment (suggestion):
min and max should only be added when the type of input is number， And the default value of max should not be a fixed value. I think it would be better to do this

eg:

<img width="889" height="605" alt="image" src="https://github.com/user-attachments/assets/2cd6a3e5-4081-4fd9-a702-af9e786eea92" />






Code after:
    type === 'number'
      ? { min, ...(max != null && max > 0 ? { max } : {}) }
      : {};
  const handleChange = (evt: React.ChangeEvent<HTMLInputElement>) => {
    const { name, value } = evt.currentTarget;
    const state = {
      ...formData,
      [name]: {
        ...formData[name],
        value,
        isInvalid: false,
      },
    };
    if (typeof onChange === 'function') {
      onChange(state);
    }
  };

  return (
    <Form.Control
      name={fieldName}
      placeholder={placeholder}
      type={type}
      value={fieldObject?.value || ''}
      {...numberInputProps}
      inputMode={inputMode}
      onChange={handleChange}
      disabled={readOnly}
      isInvalid={fieldObject?.isInvalid}
      style={type === 'color' ? { width: '100px', flex: 'none' } : {}}
    />
  );
};

export default Index;
Repository: answerdev/answer
File: internal/service/notification_common/notification.go (Go)

Code before:
			pluginNotificationMsg.TriggerUserID = triggerUser.ID
			pluginNotificationMsg.TriggerUserDisplayName = triggerUser.DisplayName
			pluginNotificationMsg.TriggerUserUrl = display.UserURL(siteInfo.SiteUrl, triggerUser.Username)
		}
	}

	if len(pluginNotificationMsg.ReceiverLang) == 0 && len(msg.ReceiverUserID) > 0 {
		userInfo, _, _ := ns.userCommon.GetUserBasicInfoByID(ctx, msg.ReceiverUserID)
		if userInfo != nil {
			pluginNotificationMsg.ReceiverLang = userInfo.Language
		}
		// If receiver not set language, use site default language.
		if len(pluginNotificationMsg.ReceiverLang) == 0 || pluginNotificationMsg.ReceiverLang == translator.DefaultLangOption {
			pluginNotificationMsg.ReceiverLang = interfaceInfo.Language
		}
	}

	externalLogins, err := ns.userExternalLoginRepo.GetUserExternalLoginList(ctx, msg.ReceiverUserID)
	if err != nil {
		log.Errorf("get user external login list failed for user %s: %v", msg.ReceiverUserID, err)
	} else if len(externalLogins) > 0 {
		pluginNotificationMsg.ReceiverExternalID = externalLogins[0].ExternalID
	}

	_ = plugin.CallNotification(func(fn plugin.Notification) error {
		fn.Notify(pluginNotificationMsg)
		return nil
	})
}


Review comment (suggestion):
We can retain this section so that if a search is available, notifications from the same system can be prioritized. If no match is found, the default `externalLogins[0].ExternalID` will be used.

Code after:
			pluginNotificationMsg.TriggerUserID = triggerUser.ID
			pluginNotificationMsg.TriggerUserDisplayName = triggerUser.DisplayName
			pluginNotificationMsg.TriggerUserUrl = display.UserURL(siteInfo.SiteUrl, triggerUser.Username)
		}
	}

	if len(pluginNotificationMsg.ReceiverLang) == 0 && len(msg.ReceiverUserID) > 0 {
		userInfo, _, _ := ns.userCommon.GetUserBasicInfoByID(ctx, msg.ReceiverUserID)
		if userInfo != nil {
			pluginNotificationMsg.ReceiverLang = userInfo.Language
		}
		// If receiver not set language, use site default language.
		if len(pluginNotificationMsg.ReceiverLang) == 0 || pluginNotificationMsg.ReceiverLang == translator.DefaultLangOption {
			pluginNotificationMsg.ReceiverLang = interfaceInfo.Language
		}
	}

	externalLogins, err := ns.userExternalLoginRepo.GetUserExternalLoginList(ctx, msg.ReceiverUserID)
	if err != nil {
		log.Errorf("get user external login list failed for user %s: %v", msg.ReceiverUserID, err)
	} else if len(externalLogins) > 0 {
		pluginNotificationMsg.ReceiverExternalID = externalLogins[0].ExternalID
	}

	_ = plugin.CallNotification(func(fn plugin.Notification) error {
		userInfo, exist, err := ns.userExternalLoginRepo.GetByUserID(ctx, fn.Info().SlugName, msg.ReceiverUserID)
		if err != nil {
			log.Errorf("get user external login info failed: %v", err)
			return nil
		}
		if exist {
			pluginNotificationMsg.ReceiverExternalID = userInfo.ExternalID
		}
		fn.Notify(pluginNotificationMsg)
		return nil
	})
}