feat(web): extract ListEditor form component (#6962)

Signed-off-by: Daniel Gerber <394442-gerbsen@users.noreply.gitlab.com>
Co-authored-by: Daniel Gerber <394442-gerbsen@users.noreply.gitlab.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

docs/docs/92-development/03-ui.md

@@ -41,6 +41,26 @@ The following list contains some tools and frameworks used by the Woodpecker UI.
 - [Volar & vue-tsc](https://github.com/johnsoncodehk/volar/) for type-checking in .vue file
   - use the take-over mode of Volar as described by [this guide](https://github.com/johnsoncodehk/volar/discussions/471)
 
+## Form components
+
+Reusable form controls live in `web/src/components/form/`. They are all built to be placed inside an `InputField`, which renders the label, the optional description and a docs link, and passes down the `id` the control has to attach to its input:
+
+```vue
+<InputField :label="$t('some.label')">
+  <template #default="{ id }">
+    <TextField :id="id" v-model="value" />
+  </template>
+  <template #description>
+    {{ $t('some.description') }}
+  </template>
+</InputField>
+```
+
+Besides the single-value controls (`TextField`, `NumberField`, `Checkbox`, `SelectField`, `RadioField`) there are two editors for collections:
+
+- `ListEditor` for a list of strings, such as plugin images or usernames
+- `KeyValueEditor` for a `Record<string, string>`, such as environment variables
+
 ## Messages and Translations
 
 Woodpecker uses [Vue I18n](https://vue-i18n.intlify.dev/) as translation library. New translations have to be added to `web/src/assets/locales/en.json`. The English source file will be automatically imported into [Weblate](https://translate.woodpecker-ci.org/) (the translation system used by Woodpecker) where all other languages will be translated by the community based on the English source.

web/src/assets/locales/en.json

@@ -612,6 +612,7 @@
   "executable_desc": "Path to the addon executable.",
   "save": "Save",
   "add": "Add",
+  "delete": "Delete",
   "skip_verify": "Skip SSL verification",
   "skip_verify_desc": "Skip SSL verification for the API connection. This is not recommended for production use.",
   "url": "URL",

web/src/components/form/ListEditor.test.ts

@@ -0,0 +1,287 @@
+import { mount } from '@vue/test-utils';
+import { describe, expect, it } from 'vitest';
+import { defineComponent, h, nextTick, ref } from 'vue';
+import { createI18n } from 'vue-i18n';
+
+import en from '~/assets/locales/en.json';
+
+import ListEditor from './ListEditor.vue';
+
+// resolve the titles through the real locale file, so renaming a key there
+// fails this test instead of silently rendering the raw key in the app
+const i18n = createI18n({
+  legacy: false,
+  locale: 'en',
+  messages: { en },
+});
+
+const global = { plugins: [i18n] };
+
+async function mountListEditor(initialItems: string[] = [], props: Record<string, unknown> = {}) {
+  const items = ref(initialItems);
+
+  const host = defineComponent({
+    setup() {
+      return () =>
+        h(ListEditor, {
+          id: 'images',
+          modelValue: items.value,
+          'onUpdate:modelValue': (value: string[]) => {
+            items.value = value;
+          },
+          ...props,
+        });
+    },
+  });
+
+  const wrapper = mount(host, { global });
+  await nextTick();
+
+  // the last input is the one used to enter new items, the others are the
+  // disabled inputs rendering the existing entries
+  const inputs = () => wrapper.findAll('input');
+  const newItemInput = () => inputs().at(-1)!;
+  const addButton = () => wrapper.findAll('button').at(-1)!;
+
+  async function addItem(value: string) {
+    await newItemInput().setValue(value);
+    await addButton().trigger('click');
+    await nextTick();
+  }
+
+  return { wrapper, items, inputs, newItemInput, addButton, addItem };
+}
+
+describe('listEditor', () => {
+  it('renders an input per existing item plus one for new entries', async () => {
+    const { inputs } = await mountListEditor(['plugins/git', 'plugins/docker']);
+
+    expect(inputs()).toHaveLength(3);
+    expect(inputs()[0].element.value).toBe('plugins/git');
+    expect(inputs()[1].element.value).toBe('plugins/docker');
+    expect(inputs().at(-1)!.element.value).toBe('');
+  });
+
+  it('renders existing items as disabled so they cannot be edited in place', async () => {
+    const { inputs } = await mountListEditor(['plugins/git']);
+
+    expect(inputs()[0].element.disabled).toBe(true);
+    expect(inputs().at(-1)!.element.disabled).toBe(false);
+  });
+
+  it('emits the new list when an item is added via the button', async () => {
+    const { items, addItem } = await mountListEditor(['plugins/git']);
+
+    await addItem('plugins/docker');
+
+    expect(items.value).toStrictEqual(['plugins/git', 'plugins/docker']);
+  });
+
+  it('adds the item when enter is pressed', async () => {
+    const { items, newItemInput } = await mountListEditor([]);
+
+    await newItemInput().setValue('plugins/git');
+    await newItemInput().trigger('keydown.enter');
+    await nextTick();
+
+    expect(items.value).toStrictEqual(['plugins/git']);
+  });
+
+  it('clears the input after adding an item', async () => {
+    const { newItemInput, addItem } = await mountListEditor([]);
+
+    await addItem('plugins/git');
+
+    expect(newItemInput().element.value).toBe('');
+  });
+
+  it('does not mutate the array it was given', async () => {
+    const initialItems = ['plugins/git'];
+    const { addItem } = await mountListEditor(initialItems);
+
+    await addItem('plugins/docker');
+
+    expect(initialItems).toStrictEqual(['plugins/git']);
+  });
+
+  it('trims surrounding whitespace from added items', async () => {
+    const { items, addItem } = await mountListEditor([]);
+
+    await addItem('  plugins/git  ');
+
+    expect(items.value).toStrictEqual(['plugins/git']);
+  });
+
+  it('ignores an empty or whitespace-only input', async () => {
+    const { items, addItem } = await mountListEditor([]);
+
+    await addItem('');
+    expect(items.value).toStrictEqual([]);
+
+    await addItem('   ');
+    expect(items.value).toStrictEqual([]);
+  });
+
+  it('ignores a duplicate and keeps it in the input so the user can see it was not added', async () => {
+    const { items, newItemInput, addItem } = await mountListEditor(['plugins/git']);
+
+    await addItem('plugins/git');
+
+    expect(items.value).toStrictEqual(['plugins/git']);
+    expect(newItemInput().element.value).toBe('plugins/git');
+  });
+
+  it('treats an entry that only differs by whitespace as a duplicate', async () => {
+    const { items, addItem } = await mountListEditor(['plugins/git']);
+
+    await addItem('  plugins/git  ');
+
+    expect(items.value).toStrictEqual(['plugins/git']);
+  });
+
+  it('removes only the clicked item', async () => {
+    const { wrapper, items } = await mountListEditor(['plugins/git', 'plugins/docker', 'plugins/s3']);
+
+    // the delete button of the second entry
+    await wrapper.findAll('button')[1].trigger('click');
+    await nextTick();
+
+    expect(items.value).toStrictEqual(['plugins/git', 'plugins/s3']);
+  });
+
+  it('drops the row once its item is removed', async () => {
+    const { wrapper, inputs } = await mountListEditor(['plugins/git']);
+
+    await wrapper.findAll('button')[0].trigger('click');
+    await nextTick();
+
+    expect(inputs()).toHaveLength(1);
+  });
+
+  it('gives every input a unique id so the labels stay unambiguous', async () => {
+    const { inputs } = await mountListEditor(['plugins/git', 'plugins/docker']);
+
+    const ids = inputs().map((input) => input.element.id);
+
+    expect(new Set(ids).size).toBe(ids.length);
+    // the label of the surrounding InputField points at the new-item input
+    expect(inputs().at(-1)!.element.id).toBe('images');
+  });
+
+  it('renders all buttons as type=button so they never submit the surrounding form', async () => {
+    const { wrapper } = await mountListEditor(['plugins/git']);
+
+    const types = wrapper.findAll('button').map((button) => button.element.type);
+
+    expect(types).toStrictEqual(['button', 'button']);
+  });
+
+  it('passes the placeholder to the new-item input only', async () => {
+    const { inputs } = await mountListEditor(['plugins/git'], { placeholder: 'Plugin image' });
+
+    expect(inputs()[0].attributes('placeholder')).toBe('');
+    expect(inputs().at(-1)!.attributes('placeholder')).toBe('Plugin image');
+  });
+
+  it('titles the delete and add buttons', async () => {
+    const { wrapper } = await mountListEditor(['plugins/git']);
+
+    const buttons = wrapper.findAll('button');
+
+    expect(buttons[0].attributes('title')).toBe('Delete');
+    expect(buttons[1].attributes('title')).toBe('Add');
+  });
+
+  it('treats a missing modelValue as an empty list', async () => {
+    const items = ref<string[] | undefined>(undefined);
+
+    const host = defineComponent({
+      setup() {
+        return () =>
+          h(ListEditor, {
+            modelValue: items.value,
+            'onUpdate:modelValue': (value: string[]) => {
+              items.value = value;
+            },
+          });
+      },
+    });
+
+    const wrapper = mount(host, { global });
+    await nextTick();
+
+    expect(wrapper.findAll('input')).toHaveLength(1);
+
+    await wrapper.find('input').setValue('plugins/git');
+    await wrapper.find('button').trigger('click');
+    await nextTick();
+
+    expect(items.value).toStrictEqual(['plugins/git']);
+  });
+
+  it('commits unconfirmed input when the parent calls commitPendingItem', async () => {
+    const items = ref<string[]>([]);
+    const editor = ref<{ commitPendingItem: () => void }>();
+
+    const host = defineComponent({
+      setup() {
+        return () =>
+          h(ListEditor, {
+            ref: editor,
+            modelValue: items.value,
+            'onUpdate:modelValue': (value: string[]) => {
+              items.value = value;
+            },
+          });
+      },
+    });
+
+    const wrapper = mount(host, { global });
+    await nextTick();
+
+    // the user types but never presses enter or the add button
+    await wrapper.find('input').setValue('plugins/git');
+    expect(items.value).toStrictEqual([]);
+
+    editor.value!.commitPendingItem();
+    await nextTick();
+
+    expect(items.value).toStrictEqual(['plugins/git']);
+  });
+
+  it('commitPendingItem is a no-op when the input is empty', async () => {
+    const items = ref<string[]>(['plugins/git']);
+    const editor = ref<{ commitPendingItem: () => void }>();
+
+    const host = defineComponent({
+      setup() {
+        return () =>
+          h(ListEditor, {
+            ref: editor,
+            modelValue: items.value,
+            'onUpdate:modelValue': (value: string[]) => {
+              items.value = value;
+            },
+          });
+      },
+    });
+
+    mount(host, { global });
+    await nextTick();
+
+    editor.value!.commitPendingItem();
+    await nextTick();
+
+    expect(items.value).toStrictEqual(['plugins/git']);
+  });
+
+  it('reflects items added from outside the component', async () => {
+    const { items, inputs } = await mountListEditor(['plugins/git']);
+
+    items.value = ['plugins/git', 'plugins/docker'];
+    await nextTick();
+
+    expect(inputs()).toHaveLength(3);
+    expect(inputs()[1].element.value).toBe('plugins/docker');
+  });
+});

web/src/components/form/ListEditor.vue

@@ -0,0 +1,61 @@
+<template>
+  <div class="flex flex-col gap-2">
+    <div v-for="(item, index) in modelValue" :key="item" class="flex gap-2">
+      <TextField :id="`${id}-${index}`" :model-value="item" disabled />
+      <Button type="button" color="gray" start-icon="trash" :title="$t('delete')" @click="deleteItem(item)" />
+    </div>
+    <div class="flex gap-2">
+      <TextField :id="id" v-model="newItem" :placeholder="placeholder" @keydown.enter.prevent="addNewItem" />
+      <Button type="button" color="gray" start-icon="plus" :title="$t('add')" @click="addNewItem" />
+    </div>
+  </div>
+</template>
+
+<script lang="ts" setup>
+import { ref } from 'vue';
+
+import Button from '~/components/atomic/Button.vue';
+import TextField from '~/components/form/TextField.vue';
+
+const props = withDefaults(
+  defineProps<{
+    modelValue?: string[];
+    id?: string;
+    placeholder?: string;
+  }>(),
+  {
+    modelValue: () => [],
+    id: undefined,
+    placeholder: undefined,
+  },
+);
+
+const emit = defineEmits<{
+  (e: 'update:modelValue', value: string[]): void;
+}>();
+
+const newItem = ref('');
+
+function addNewItem() {
+  const item = newItem.value.trim();
+  // an entry that is blank or already listed would be indistinguishable from
+  // the existing ones, so keep it in the input instead of adding a duplicate
+  if (!item || props.modelValue.includes(item)) {
+    return;
+  }
+
+  emit('update:modelValue', [...props.modelValue, item]);
+  newItem.value = '';
+}
+
+function deleteItem(item: string) {
+  emit(
+    'update:modelValue',
+    props.modelValue.filter((i) => i !== item),
+  );
+}
+
+// lets a parent commit text the user typed but never confirmed, so submitting
+// the surrounding form does not silently drop it
+defineExpose({ commitPendingItem: addNewItem });
+</script>

web/src/components/secrets/SecretEdit.vue

@@ -24,21 +24,12 @@
       <InputField v-slot="{ id }" :label="$t('secrets.plugins.images')">
         <span class="text-wp-text-alt-100 mb-2 ml-1">{{ $t('secrets.plugins.desc') }}</span>
 
-        <div class="flex flex-col gap-2">
-          <div v-for="image in innerValue.images" :key="image" class="flex gap-2">
-            <TextField :id="id" :model-value="image" disabled />
-            <Button type="button" color="gray" start-icon="trash" @click="removeImage(image)" />
-          </div>
-          <div class="flex gap-2">
-            <TextField
-              :id="id"
-              v-model="newImage"
-              :placeholder="$t('repo.settings.general.netrc_only_trusted.placeholder')"
-              @keydown.enter.prevent="addNewImage"
-            />
-            <Button type="button" color="gray" start-icon="plus" @click="addNewImage" />
-          </div>
-        </div>
+        <ListEditor
+          :id="id"
+          ref="imagesEditor"
+          v-model="innerValue.images"
+          :placeholder="$t('repo.settings.general.netrc_only_trusted.placeholder')"
+        />
       </InputField>
 
       <InputField :label="$t('secrets.events.events')">
@@ -64,7 +55,7 @@
 </template>
 
 <script lang="ts" setup>
-import { computed, ref, toRef } from 'vue';
+import { computed, toRef, useTemplateRef } from 'vue';
 import { useI18n } from 'vue-i18n';
 
 import Button from '~/components/atomic/Button.vue';
@@ -72,6 +63,7 @@ import Warning from '~/components/atomic/Warning.vue';
 import CheckboxesField from '~/components/form/CheckboxesField.vue';
 import type { CheckboxOption } from '~/components/form/form.types';
 import InputField from '~/components/form/InputField.vue';
+import ListEditor from '~/components/form/ListEditor.vue';
 import TextField from '~/components/form/TextField.vue';
 import { WebhookEvents } from '~/lib/api/types';
 import type { Secret } from '~/lib/api/types';
@@ -98,17 +90,7 @@ const innerValue = computed({
 });
 const isEditingSecret = computed(() => !!innerValue.value?.id);
 
-const newImage = ref('');
-function addNewImage() {
-  if (!newImage.value) {
-    return;
-  }
-  innerValue.value.images?.push(newImage.value);
-  newImage.value = '';
-}
-function removeImage(image: string) {
-  innerValue.value.images = innerValue.value.images?.filter((i) => i !== image);
-}
+const imagesEditor = useTemplateRef<InstanceType<typeof ListEditor>>('imagesEditor');
 
 const secretEventsOptions: CheckboxOption[] = [
   { value: WebhookEvents.Push, text: i18n.t('repo.pipeline.event.push') },
@@ -125,9 +107,8 @@ function save() {
     return;
   }
 
-  if (newImage.value) {
-    innerValue.value.images?.push(newImage.value);
-  }
+  // an image the user typed without confirming it should still be saved
+  imagesEditor.value?.commitPendingItem();
 
   emit('save', innerValue.value);
 }

web/src/views/repo/settings/General.vue

@@ -22,21 +22,12 @@
         docs-url="docs/usage/project-settings#custom-trusted-clone-plugins"
       >
         <template #default="{ id }">
-          <div class="flex flex-col gap-2">
-            <div v-for="image in repoSettings.netrc_trusted" :key="image" class="flex gap-2">
-              <TextField :id="id" :model-value="image" disabled />
-              <Button type="button" color="gray" start-icon="trash" @click="removeImage(image)" />
-            </div>
-            <div class="flex gap-2">
-              <TextField
-                :id="id"
-                v-model="newImage"
-                :placeholder="$t('repo.settings.general.netrc_only_trusted.placeholder')"
-                @keydown.enter.prevent="addNewImage"
-              />
-              <Button type="button" color="gray" start-icon="plus" @click="addNewImage" />
-            </div>
-          </div>
+          <ListEditor
+            :id="id"
+            ref="netrcTrustedEditor"
+            v-model="repoSettings.netrc_trusted"
+            :placeholder="$t('repo.settings.general.netrc_only_trusted.placeholder')"
+          />
         </template>
         <template #description>
           {{ $t('repo.settings.general.netrc_only_trusted.desc') }}
@@ -98,16 +89,12 @@
         :label="$t('require_approval.allowed_users.allowed_users')"
       >
         <template #default="{ id }">
-          <div class="flex flex-col gap-2">
-            <div v-for="allowedUser in repoSettings.approval_allowed_users" :key="allowedUser" class="flex gap-2">
-              <TextField :id="id" :model-value="allowedUser" disabled />
-              <Button type="button" color="gray" start-icon="trash" @click="removeUser(allowedUser)" />
-            </div>
-            <div class="flex gap-2">
-              <TextField :id="id" v-model="newUser" :placeholder="$t('username')" @keydown.enter.prevent="addNewUser" />
-              <Button type="button" color="gray" start-icon="plus" @click="addNewUser" />
-            </div>
-          </div>
+          <ListEditor
+            :id="id"
+            ref="approvalAllowedUsersEditor"
+            v-model="repoSettings.approval_allowed_users"
+            :placeholder="$t('username')"
+          />
         </template>
         <template #description>
           {{ $t('require_approval.allowed_users.desc') }}
@@ -183,7 +170,7 @@
 </template>
 
 <script lang="ts" setup>
-import { computed, onMounted, ref } from 'vue';
+import { computed, onMounted, ref, useTemplateRef } from 'vue';
 import { useI18n } from 'vue-i18n';
 
 import Button from '~/components/atomic/Button.vue';
@@ -191,6 +178,7 @@ import Checkbox from '~/components/form/Checkbox.vue';
 import CheckboxesField from '~/components/form/CheckboxesField.vue';
 import type { CheckboxOption, RadioOption } from '~/components/form/form.types';
 import InputField from '~/components/form/InputField.vue';
+import ListEditor from '~/components/form/ListEditor.vue';
 import NumberField from '~/components/form/NumberField.vue';
 import RadioField from '~/components/form/RadioField.vue';
 import TextField from '~/components/form/TextField.vue';
@@ -216,6 +204,9 @@ const { defaultConfigPaths } = useConfig();
 const repo = requiredInject('repo');
 const repoSettings = ref<RepoSettings>();
 
+const netrcTrustedEditor = useTemplateRef<InstanceType<typeof ListEditor>>('netrcTrustedEditor');
+const approvalAllowedUsersEditor = useTemplateRef<InstanceType<typeof ListEditor>>('approvalAllowedUsersEditor');
+
 function loadRepoSettings() {
   repoSettings.value = {
     config_file: repo.value.config_file,
@@ -241,6 +232,10 @@ const { doSubmit: saveRepoSettings, isLoading: isSaving } = useAsyncAction(async
     throw new Error('Unexpected: Repo-Settings should be set');
   }
 
+  // an entry the user typed without confirming it should still be saved
+  netrcTrustedEditor.value?.commitPendingItem();
+  approvalAllowedUsersEditor.value?.commitPendingItem();
+
   await apiClient.updateRepo(repo.value.id, repoSettings.value);
   await loadRepo();
   notifications.notify({ title: i18n.t('repo.settings.general.success'), type: 'success' });
@@ -278,37 +273,5 @@ const cancelPreviousPipelineEventsOptions: CheckboxOption[] = [
   { value: WebhookEvents.Deploy, text: i18n.t('repo.pipeline.event.deploy') },
 ];
 
-const newImage = ref('');
-function addNewImage() {
-  if (!newImage.value) {
-    return;
-  }
-  repoSettings.value?.netrc_trusted.push(newImage.value);
-  newImage.value = '';
-}
-function removeImage(image: string) {
-  if (!repoSettings.value) {
-    throw new Error('Unexpected: repoSettings should be set');
-  }
-
-  repoSettings.value.netrc_trusted = repoSettings.value.netrc_trusted.filter((i) => i !== image);
-}
-
-const newUser = ref('');
-function addNewUser() {
-  if (!newUser.value) {
-    return;
-  }
-  repoSettings.value?.approval_allowed_users.push(newUser.value);
-  newUser.value = '';
-}
-function removeUser(user: string) {
-  if (!repoSettings.value) {
-    throw new Error('Unexpected: repoSettings should be set');
-  }
-
-  repoSettings.value.approval_allowed_users = repoSettings.value.approval_allowed_users.filter((i) => i !== user);
-}
-
 useWPTitle(computed(() => [i18n.t('repo.settings.general.project'), repo.value.full_name]));
 </script>