ronantakizawa/codereview-bench huggingface dataset:
Repository: hashicorp/vault
File: ui/lib/config-ui/addon/routes/login-settings/rule/details.js (JavaScript)

Code before:
import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRuleDetailsRoute extends Route {
  @service('app-router') router;
  @service store;

  beforeModel() {
    const { name } = this.paramsFor('login-settings.rule');
    if (!name) {
      this.router.transitionTo('vault.cluster.config-ui.login-settings.index');
    }
  }

  async model() {
    try {
      const { name } = this.paramsFor('login-settings.rule');

      const adapter = this.store.adapterFor('application');
      const rule = await adapter.ajax(`/v1/sys/config/ui/login/default-auth/${encodeURI(name)}`, 'GET');

      return { rule: rule.data };
    } catch (error) {
      return { rule: {}, error };
    }
  }

  setupController(controller, resolvedModel) {
    super.setupController(controller, resolvedModel);

    controller.breadcrumbs = [{ label: 'UI login rules', route: 'login-settings' }, { label: '' }];
  }
}


Review comment (bug):
This wasn't quite what I meant 😅 Here are the ember docs for error substates: https://guides.emberjs.com/v5.5.0/routing/loading-and-error-substates/

As an example, when i navigate to "fake" secret (which doesn't exist) this error template renders and returns the error thrown by the `model` hook. 
<img width="1500" alt="Screenshot 2025-05-06 at 3 38 12 PM" src="https://github.com/user-attachments/assets/617e0ca7-6e0c-4451-81aa-9757709d639d" />
 
If you review the docs, this is because when the model hook fails it travels up the template directory looking for an `error.hbs` template. The one rendered above is ([here](https://github.com/hashicorp/vault/blob/main/ui/lib/kv/addon/templates/error.hbs#L17))

it looks like the ember engine `config-ui` doesn't have an error template, so we should create one. Then we get error handling "for free" the model hook can be simplified to  

```js
  async model() {
      const { name } = this.paramsFor('login-settings.rule');
      const adapter = this.store.adapterFor('application');
      const rule = await adapter.ajax(`/v1/sys/config/ui/login/default-auth/${encodeURI(name)}`, 'GET');

      return { rule: rule.data };
  }
```



Code after:
import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRuleDetailsRoute extends Route {
  @service('app-router') router;
  @service store;

  beforeModel() {
    const { name } = this.paramsFor('login-settings.rule');
    if (!name) {
      this.router.transitionTo('vault.cluster.config-ui.login-settings.index');
    }
  }

  async model() {
    const { name } = this.paramsFor('login-settings.rule');

    const adapter = this.store.adapterFor('application');
    const rule = await adapter.ajax(`/v1/sys/config/ui/login/default-auth/${encodeURI(name)}`, 'GET');

    return { rule: rule.data };
  }

  setupController(controller, resolvedModel) {
    super.setupController(controller, resolvedModel);

    controller.breadcrumbs = [{ label: 'UI login rules', route: 'login-settings' }, { label: '' }];
  }
}
Repository: hashicorp/vault
File: ui/lib/config-ui/addon/components/login-settings/page/details.js (JavaScript)

Code before:
 *
 * @example
 * ```js
 * <Page::LoginSettingsRuleDetails @rule={{this.rule}}  />
 * ```
 * @param {object} rule - holds login rule data, { backup_auth_types: string[] eg. ['token'], default_auth_type: string "oidc", disable_inheritance: boolean,
 * name: string "Login rule 1", namespace: string eg "admin/"}
 * */

export default class LoginSettingsRuleDetails extends Component {
  @service capabilities;
  @service flashMessages;
  @service('app-router') router;
  @service store;

  @tracked showConfirmModal = false;

  @action
  async onDelete() {
    const { rule } = this.args;

    try {
      const adapter = this.store.adapterFor('application');

      await adapter.ajax(`/v1/sys/config/ui/login/default-auth/${encodeURI(rule.name)}`, 'DELETE');
      this.flashMessages.success(`Successfully deleted rule ${rule.name}`);

      this.router.transitionTo('vault.cluster.config-ui.login-settings.index');
    } catch (error) {
      const message = errorMessage(error, 'Error deleting rule. Please try again.');
      this.flashMessages.danger(message);
    }
  }
}


Review comment (suggestion):
```suggestion
      this.flashMessages.success(`Successfully deleted rule ${rule.name}.`);
```

Code after:
 *
 * @example
 * ```js
 * <Page::LoginSettingsRuleDetails @rule={{this.rule}}  />
 * ```
 * @param {object} rule - holds login rule data, { backup_auth_types: string[] eg. ['token'], default_auth_type: string "oidc", disable_inheritance: boolean,
 * name: string "Login rule 1", namespace: string eg "admin/"}
 * */

export default class LoginSettingsRuleDetails extends Component {
  @service capabilities;
  @service flashMessages;
  @service('app-router') router;
  @service store;

  @tracked showConfirmModal = false;

  @action
  async onDelete() {
    const { rule } = this.args;

    try {
      const adapter = this.store.adapterFor('application');

      await adapter.ajax(`/v1/sys/config/ui/login/default-auth/${encodeURI(rule.name)}`, 'DELETE');
      this.flashMessages.success(`Successfully deleted rule ${rule.name}.`);

      this.router.transitionTo('vault.cluster.config-ui.login-settings.index');
    } catch (error) {
      const message = errorMessage(error, 'Error deleting rule. Please try again.');
      this.flashMessages.danger(message);
    }
  }
}
Repository: hashicorp/vault
File: ui/lib/config-ui/addon/components/login-settings/page/list.js (JavaScript)

Code before:
import errorMessage from 'vault/utils/error-message';

/**
 * @module Page::LoginSettingsList
 * Page::LoginSettingsList components are used to display list of rules.
 * @example
 * ```js
 * <Page::LoginSettingsList @loginRules={{this.rules}}  />
 * ```
 * @param {array} loginRules - array of rule objects
 */

export default class LoginSettingsList extends Component {
  @service capabilities;
  @service flashMessages;
  @service('app-router') router;
  @service store;
  @tracked ruleToDelete = null; // set to the rule intended to delete

  @action
  async onDelete() {
    try {
      const adapter = this.store.adapterFor('application');

      await adapter.ajax(`/v1/sys/config/ui/login/default-auth/${encodeURI(this.ruleToDelete.id)}`, 'DELETE');
      this.flashMessages.success(`Successfully deleted rule ${this.ruleToDelete.id}`);

      this.router.transitionTo('vault.cluster.config-ui.login-settings');
    } catch (error) {
      const message = errorMessage(error, 'Error deleting rule. Please try again.');
      this.flashMessages.danger(message);
    }
  }
}


Review comment (suggestion):
```suggestion
      this.flashMessages.success(`Successfully deleted rule ${this.ruleToDelete.id}.`);
```

Code after:
import errorMessage from 'vault/utils/error-message';

/**
 * @module Page::LoginSettingsList
 * Page::LoginSettingsList components are used to display list of rules.
 * @example
 * ```js
 * <Page::LoginSettingsList @loginRules={{this.rules}}  />
 * ```
 * @param {array} loginRules - array of rule objects
 */

export default class LoginSettingsList extends Component {
  @service capabilities;
  @service flashMessages;
  @service('app-router') router;
  @service store;
  @tracked ruleToDelete = null; // set to the rule intended to delete

  @action
  async onDelete() {
    try {
      const adapter = this.store.adapterFor('application');

      await adapter.ajax(`/v1/sys/config/ui/login/default-auth/${encodeURI(this.ruleToDelete.id)}`, 'DELETE');
      this.flashMessages.success(`Successfully deleted rule ${this.ruleToDelete.id}.`);

      this.router.transitionTo('vault.cluster.config-ui.login-settings');
    } catch (error) {
      const message = errorMessage(error, 'Error deleting rule. Please try again.');
      this.flashMessages.danger(message);
    }
  }
}
Repository: hashicorp/vault
File: ui/app/components/auth-config-form/options.js (JavaScript)

Code before:
        }
      });
    }

    // token_type should not be tuneable for the token auth method.
    if (this.args.model.methodType === 'token') {
      delete data.token_type;
    }

    try {
      yield this.args.model.tune(data);
    } catch (err) {
      if (err instanceof AdapterError) {
        // because we're not calling model.save the model never updates with
        // the error, so we set it manually in the component instead.
        this.errorMessage = errorMessage(err);
        return;
      }
      throw err;
    }
    this.router.transitionTo('vault.cluster.access.methods').followRedirects();
    this.flashMessages.success('The configuration was saved successfully.');
  }

  get getLoginLink() {
    return `${window.origin}/ui/vault/auth?with=${this.args.model.type}`;
  }
}


Review comment (suggestion):
This should be the mount path, not type ☺️. We should also encode the path so it's url safe. 

Code after:
        }
      });
    }

    // token_type should not be tuneable for the token auth method.
    if (this.args.model.methodType === 'token') {
      delete data.token_type;
    }

    try {
      yield this.args.model.tune(data);
    } catch (err) {
      if (err instanceof AdapterError) {
        // because we're not calling model.save the model never updates with
        // the error, so we set it manually in the component instead.
        this.errorMessage = errorMessage(err);
        return;
      }
      throw err;
    }
    this.router.transitionTo('vault.cluster.access.methods').followRedirects();
    this.flashMessages.success('The configuration was saved successfully.');
  }

  get getLoginLink() {
    return `${window.origin}/ui/vault/auth?with=${encodeURIComponent(this.args.model.path)}`;
  }
}
Repository: hashicorp/vault
File: ui/app/components/auth-config-form/options.js (JavaScript)

Code before:
        }
      });
    }

    // token_type should not be tuneable for the token auth method.
    if (this.args.model.methodType === 'token') {
      delete data.token_type;
    }

    try {
      yield this.args.model.tune(data);
    } catch (err) {
      if (err instanceof AdapterError) {
        // because we're not calling model.save the model never updates with
        // the error, so we set it manually in the component instead.
        this.errorMessage = errorMessage(err);
        return;
      }
      throw err;
    }
    this.router.transitionTo('vault.cluster.access.methods').followRedirects();
    this.flashMessages.success('The configuration was saved successfully.');
  }

  get getLoginLink() {
    return `${window.origin}/ui/vault/auth?with=${encodePath(this.args.model.path)}`;
  }
}


Review comment (suggestion):
good find! But since slashes could be part of the mount name we I think we want those encoded here, so this can just use `encodeURIComponent` directly

Code after:
      });
    }

    // token_type should not be tuneable for the token auth method.
    if (this.args.model.methodType === 'token') {
      delete data.token_type;
    }

    try {
      yield this.args.model.tune(data);
    } catch (err) {
      if (err instanceof AdapterError) {
        // because we're not calling model.save the model never updates with
        // the error, so we set it manually in the component instead.
        this.errorMessage = errorMessage(err);
        return;
      }
      throw err;
    }
    this.router.transitionTo('vault.cluster.access.methods').followRedirects();
    this.flashMessages.success('The configuration was saved successfully.');
  }

  get getLoginLink() {
    return `${window.origin}/ui/vault/auth?with=${encodeURIComponent(this.args.model.path)}`;
  }
}
Repository: hashicorp/vault
File: ui/lib/config-ui/addon/routes/login-settings/rule/details.js (JavaScript)

Code before:
import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRuleDetailsRoute extends Route {
  @service('app-router') router;
  @service api;

  beforeModel() {
    const { name } = this.paramsFor('login-settings.rule');
    if (!name) {
      this.router.transitionTo('vault.cluster.config-ui.login-settings.index');
    }
  }

  async model() {
    const { name } = this.paramsFor('login-settings.rule');

    const rule = await this.api.sys.uiLoginDefaultAuthReadConfiguration(name);

    return { rule: { name, ...rule.data } };
  }

  setupController(controller, resolvedModel) {
    super.setupController(controller, resolvedModel);

    controller.breadcrumbs = [{ label: 'UI login rules', route: 'login-settings' }, { label: '' }];
  }
}


Review comment (suggestion):
The label here should be the rule name (without the route, like you have it here) - sometimes the designs don't show the most up-to-date breadcrumbs 😅 
```suggestion
    controller.breadcrumbs = [{ label: 'UI login rules', route: 'login-settings' }, { label: resolvedModel.rule.name }];
```

Code after:
import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRuleDetailsRoute extends Route {
  @service('app-router') router;
  @service api;

  beforeModel() {
    const { name } = this.paramsFor('login-settings.rule');
    if (!name) {
      this.router.transitionTo('vault.cluster.config-ui.login-settings.index');
    }
  }

  async model() {
    const { name } = this.paramsFor('login-settings.rule');

    const rule = await this.api.sys.uiLoginDefaultAuthReadConfiguration(name);

    return { rule: { name, ...rule.data } };
  }

  setupController(controller, resolvedModel) {
    super.setupController(controller, resolvedModel);

    controller.breadcrumbs = [
      { label: 'UI login rules', route: 'login-settings' },
      { label: resolvedModel.rule.name },
    ];
  }
}
Repository: hashicorp/vault
File: ui/tests/acceptance/config-ui/login-settings-test.js (JavaScript)

Code before:
    await click(GENERAL.menuTrigger);
    await click(GENERAL.menuItem('delete-rule'));

    assert.dom(GENERAL.confirmationModal).exists();
    await click(GENERAL.confirmButton);

    // verify success message from deletion
    assert.dom(GENERAL.latestFlashContent).includesText('Successfully deleted rule testRule.');
  });

  test('navigate to rule details page and renders rule data', async function (assert) {
    // visit individual rule page
    await visit('vault/config-ui/login-settings');

    await click(GENERAL.menuTrigger);
    await click(GENERAL.menuItem('view-rule'));

    // verify that user is redirected to the rule details page
    assert.strictEqual(
      currentRouteName(),
      'vault.cluster.config-ui.login-settings.rule.details',
      'goes to rule details page'
    );

    // verify fetched rule data is rendered
    assert.dom(GENERAL.infoRowLabel('Name')).exists();
    assert.dom(GENERAL.infoRowValue('Name')).hasText('testRule');
    assert.dom(GENERAL.infoRowLabel('Namespace')).exists();
    assert.dom(GENERAL.infoRowValue('Namespace')).hasText('ns1/');
  });
});


Review comment (suggestion):
Definitely up to you, but for future reference I think the assertion below is sufficient and there's not much need to check it exists beforehand

Code after:
    // Visit the login settings list index page
    await visit('vault/config-ui/login-settings');

    await click(GENERAL.menuTrigger);
    await click(GENERAL.menuItem('delete-rule'));

    assert.dom(GENERAL.confirmationModal).exists();
    await click(GENERAL.confirmButton);

    // verify success message from deletion
    assert.dom(GENERAL.latestFlashContent).includesText('Successfully deleted rule testRule.');
    assert.dom('[data-test-rule-name="testRule"]').doesNotExist();
  });

  test('navigate to rule details page and renders rule data', async function (assert) {
    // visit individual rule page
    await visit('vault/config-ui/login-settings');

    await click(GENERAL.menuTrigger);
    await click(GENERAL.menuItem('view-rule'));

    // verify that user is redirected to the rule details page
    assert.strictEqual(
      currentRouteName(),
      'vault.cluster.config-ui.login-settings.rule.details',
      'goes to rule details page'
    );

    // verify fetched rule data is rendered
    assert.dom(GENERAL.infoRowValue('Name')).hasText('testRule');
    assert.dom(GENERAL.infoRowValue('Namespace')).hasText('ns1/');
    assert.dom(GENERAL.infoRowValue('Backup methods')).hasText('userpass');
    assert.dom(GENERAL.infoRowValue('Inheritance')).hasText('true');
  });
});
Repository: hashicorp/vault
File: command/plugin_register.go (Go)

Code before:

	// These cases should come after invalid cases have been checked
	case len(args) == 1:
		pluginTypeRaw = "unknown"
		pluginNameRaw = args[0]
	case len(args) == 2:
		pluginTypeRaw = args[0]
		pluginNameRaw = args[1]
	}

	client, err := c.Client()
	if err != nil {
		c.UI.Error(err.Error())
		return 2
	}

	pluginType, err := api.ParsePluginType(strings.TrimSpace(pluginTypeRaw))
	if err != nil {
		c.UI.Error(err.Error())
		return 2
	}
	pluginName := strings.TrimSpace(pluginNameRaw)

	command := c.flagCommand
	if c.flagSHA256 != "" {
		if command == "" && c.flagOCIImage == "" {
			command = pluginName
		}
	}

	resp, err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
		Name:     pluginName,
		Type:     pluginType,
		Args:     c.flagArgs,
		Command:  command,
		SHA256:   c.flagSHA256,
		Version:  c.flagVersion,
		OCIImage: c.flagOCIImage,
		Runtime:  c.flagRuntime,
		Env:      c.flagEnv,
	})
	if err != nil {
		c.UI.Error(fmt.Sprintf("Error registering plugin %s: %s", pluginName, err))
		return 2
	}

	if resp != nil && len(resp.Warnings) > 0 {
		c.UI.Warn(wrapAtLength(fmt.Sprintf(
			"Warnings while registering plugin %s: %s",
			pluginName,
			strings.Join(resp.Warnings, "\n\n"),

Review comment (refactor):
nit: would it be possible to move this conditional to the outer if statement:
`if c.flagSHA256 != "" && (command == "" && c.flagOCIImage == "") {...}`

Code after:
			"If registering with an artifact, please provide -version only.")
		return 1

	// These cases should come after invalid cases have been checked
	case len(args) == 1:
		pluginTypeRaw = "unknown"
		pluginNameRaw = args[0]
	case len(args) == 2:
		pluginTypeRaw = args[0]
		pluginNameRaw = args[1]
	}

	client, err := c.Client()
	if err != nil {
		c.UI.Error(err.Error())
		return 2
	}

	pluginType, err := api.ParsePluginType(strings.TrimSpace(pluginTypeRaw))
	if err != nil {
		c.UI.Error(err.Error())
		return 2
	}
	pluginName := strings.TrimSpace(pluginNameRaw)

	command := c.flagCommand
	if c.flagSHA256 != "" && (command == "" && c.flagOCIImage == "") {
		command = pluginName
	}

	resp, err := client.Sys().RegisterPluginDetailed(&api.RegisterPluginInput{
		Name:     pluginName,
		Type:     pluginType,
		Args:     c.flagArgs,
		Command:  command,
		SHA256:   c.flagSHA256,
		Version:  c.flagVersion,
		OCIImage: c.flagOCIImage,
		Runtime:  c.flagRuntime,
		Env:      c.flagEnv,
	})
	if err != nil {
		c.UI.Error(fmt.Sprintf("Error registering plugin %s: %s", pluginName, err))
		return 2
	}

	if resp != nil && len(resp.Warnings) > 0 {
		c.UI.Warn(wrapAtLength(fmt.Sprintf(
			"Warnings while registering plugin %s: %s",
			pluginName,
			strings.Join(resp.Warnings, "\n\n"),
Repository: hashicorp/vault
File: ui/app/components/mount-backend/type-form.js (JavaScript)

Code before:
 * SPDX-License-Identifier: BUSL-1.1
 */

import Component from '@glimmer/component';
import { service } from '@ember/service';
import { ALL_ENGINES } from 'vault/utils/all-engines-metadata';

/**
 *
 * @module MountBackendTypeForm
 * MountBackendTypeForm components are used to display type options for
 * mounting either an auth method or secret engine.
 *
 * @example
 * ```js
 * <MountBackend::TypeForm @setMountType={{this.setMountType}} @mountType="secret" />
 * ```
 * @param {CallableFunction} setMountType - function will receive the mount type string. Should update the model type value
 * @param {string} [mountType=auth] - mount type can be `auth` or `secret`
 */

export default class MountBackendTypeForm extends Component {
  @service version;

  get secretEngines() {
    return this.version.isEnterprise
      ? ALL_ENGINES.filter((engine) => engine.mountType !== 'auth')
      : ALL_ENGINES.filter((engine) => engine.mountType !== 'auth' && !engine.requiresEnterprise);
  }

  get authMethods() {
    return this.version.isEnterprise
      ? ALL_ENGINES.filter((engine) => engine.mountType !== 'secret')
      : ALL_ENGINES.filter((engine) => engine.mountType !== 'secret' && !engine.requiresEnterprise);
  }

  get mountTypes() {
    return this.args.mountType === 'secret' ? this.secretEngines : this.authMethods;
  }
}


Review comment (security):
Some comments might help a future dev
```suggestion
// If an enterprise license is present, return all secret engines;
// otherwise, return only the secret engines supported in OSS.
    return this.version.isEnterprise
```

Code after:
 * SPDX-License-Identifier: BUSL-1.1
 */

import Component from '@glimmer/component';
import { service } from '@ember/service';
import { filterEnginesByMountCategory } from 'vault/utils/all-engines-metadata';

/**
 *
 * @module MountBackendTypeForm
 * MountBackendTypeForm components are used to display type options for
 * mounting either an auth method or secret engine.
 *
 * @example
 * ```js
 * <MountBackend::TypeForm @setMountType={{this.setMountType}} @mountCategory="secret" />
 * ```
 * @param {CallableFunction} setMountType - function will receive the mount type string. Should update the model type value
 * @param {string} [mountCategory=auth] - mount category type can be `auth` or `secret`
 */

export default class MountBackendTypeForm extends Component {
  @service version;

  get secretEngines() {
    // If an enterprise license is present, return all secret engines;
    // otherwise, return only the secret engines supported in OSS.
    return filterEnginesByMountCategory({ mountCategory: 'secret', isEnterprise: this.version.isEnterprise });
  }

  get authMethods() {
    // If an enterprise license is present, return all auth methods;
    // otherwise, return only the auth methods supported in OSS.
    return filterEnginesByMountCategory({ mountCategory: 'auth', isEnterprise: this.version.isEnterprise });
  }

  get mountTypes() {
    return this.args.mountCategory === 'secret' ? this.secretEngines : this.authMethods;
  }
}