From 6077c63cfd8aa776f5afed353b8ccb70fb3bc9b6 Mon Sep 17 00:00:00 2001
From: jamesread <contact@jread.com>
Date: Mon, 6 Jul 2026 11:39:43 +0100
Subject: [PATCH] chore: Fix coderabbit warnings

---
 .../vue/components/ChoiceChecklist.vue        | 15 ++++--
 .../vue/utils/choiceChecklistHelpers.test.mjs |  1 +
 frontend/resources/vue/views/ArgumentForm.vue | 17 +++----
 .../tests/checklist/checklist.mjs             | 44 +++++++----------
 service/internal/config/sanitize.go           | 47 +++++++++++++++++++
 service/internal/config/sanitize_test.go      | 23 +++++++++
 6 files changed, 107 insertions(+), 40 deletions(-)

diff --git a/frontend/resources/vue/components/ChoiceChecklist.vue b/frontend/resources/vue/components/ChoiceChecklist.vue
index b38f3fa..04235d6 100644
--- a/frontend/resources/vue/components/ChoiceChecklist.vue
+++ b/frontend/resources/vue/components/ChoiceChecklist.vue
@@ -9,7 +9,7 @@
       </button>
     </div>
     <fieldset class="choice-checklist-fieldset">
-      <legend class="visually-hidden">{{ name }}</legend>
+      <legend class="visually-hidden">{{ label || name }}</legend>
       <label
         v-for="(choice, index) in choices"
         :key="choice.value"
@@ -28,9 +28,12 @@
     <input
       :id="`${id}-value`"
       :name="name"
-      type="hidden"
+      type="text"
+      class="visually-hidden choice-checklist-value"
       :value="modelValue"
-      :required="required && modelValue === ''"
+      :required="required"
+      tabindex="-1"
+      aria-hidden="true"
     />
   </div>
 </template>
@@ -54,6 +57,10 @@ const props = defineProps({
     type: String,
     required: true
   },
+  label: {
+    type: String,
+    default: ''
+  },
   choices: {
     type: Array,
     required: true
@@ -119,7 +126,7 @@ function selectNone() {
   border: none;
   display: grid;
   gap: 0.5em 1em;
-  grid-template-columns: repeat(3, minmax(0, 1fr));
+  grid-template-columns: repeat(auto-fill, minmax(12rem, 1fr));
   margin: 0;
   padding: 0;
 }
diff --git a/frontend/resources/vue/utils/choiceChecklistHelpers.test.mjs b/frontend/resources/vue/utils/choiceChecklistHelpers.test.mjs
index 19b7a19..3ccb7d0 100644
--- a/frontend/resources/vue/utils/choiceChecklistHelpers.test.mjs
+++ b/frontend/resources/vue/utils/choiceChecklistHelpers.test.mjs
@@ -15,6 +15,7 @@ const choices = [
 
 test('parseChecklistValue splits comma-delimited values', () => {
   assert.deepEqual(parseChecklistValue('documents,photos'), ['documents', 'photos'])
+  assert.deepEqual(parseChecklistValue('documents, photos'), ['documents', 'photos'])
   assert.deepEqual(parseChecklistValue(''), [])
 })
 
diff --git a/frontend/resources/vue/views/ArgumentForm.vue b/frontend/resources/vue/views/ArgumentForm.vue
index a968d59..e00f23e 100644
--- a/frontend/resources/vue/views/ArgumentForm.vue
+++ b/frontend/resources/vue/views/ArgumentForm.vue
@@ -26,8 +26,8 @@
                 @update:model-value="handleChoiceUpdate(arg, $event)" />
 
               <ChoiceChecklist v-else-if="arg.type === 'checklist'" :id="arg.name" :name="arg.name"
-                :choices="arg.choices" :model-value="getArgumentValue(arg)" :required="arg.required"
-                @update:model-value="handleChecklistUpdate(arg, $event)" />
+                :label="arg.title" :choices="arg.choices" :model-value="getArgumentValue(arg)" :required="arg.required"
+                @update:model-value="handleChoiceUpdate(arg, $event)" />
 
               <component v-else :is="getInputComponent(arg)" :id="arg.name" :name="arg.name"
                 :value="(arg.type === 'checkbox' || arg.type === 'confirmation') ? undefined : getArgumentValue(arg)"
@@ -145,8 +145,6 @@ async function setup() {
         } else {
           argValues.value[arg.name] = false
         }
-      } else if (arg.type === 'checklist') {
-        argValues.value[arg.name] = paramValue !== null ? paramValue : arg.defaultValue || ''
       } else {
         argValues.value[arg.name] = paramValue !== null ? paramValue : arg.defaultValue || ''
       }
@@ -253,12 +251,6 @@ function getValidationElement(arg) {
   return document.getElementById(arg.name)
 }
 
-function handleChecklistUpdate(arg, value) {
-  argValues.value[arg.name] = value
-  updateUrlWithArg(arg.name, value)
-  validateArgument(arg, value)
-}
-
 function handleChoiceUpdate(arg, value) {
   argValues.value[arg.name] = value
   updateUrlWithArg(arg.name, value)
@@ -503,6 +495,11 @@ async function handleSubmit(event) {
     return
   }
 
+  if (Object.keys(formErrors.value).length > 0) {
+    console.log('argument form has validation errors')
+    return
+  }
+
   const argvs = getArgumentValues()
   console.log('argument form has elements that passed validation')
 
diff --git a/integration-tests/tests/checklist/checklist.mjs b/integration-tests/tests/checklist/checklist.mjs
index 8fff8cd..f888466 100644
--- a/integration-tests/tests/checklist/checklist.mjs
+++ b/integration-tests/tests/checklist/checklist.mjs
@@ -27,9 +27,9 @@ async function submitChecklistForm() {
   await submitButton.click()
 }
 
-async function waitForTerminalOutput(expectedValue) {
+async function pollTerminal(matcher, timeoutMs = DEFAULT_UI_WAIT_MS) {
   await webdriver.wait(
-    new Condition(`wait for checklist value ${expectedValue} in output`, async () => {
+    new Condition('wait for terminal output', async () => {
       try {
         const terminalReady = await webdriver.executeScript(`
           return !!(window.terminal && window.terminal.getBufferAsString);
@@ -43,42 +43,34 @@ async function waitForTerminalOutput(expectedValue) {
           return false
         }
 
-        return output.trim().includes(`Selected segments: ${expectedValue}`)
+        return matcher(output.trim())
       } catch (e) {
         return false
       }
     }),
+    timeoutMs
+  )
+}
+
+async function waitForTerminalOutput(expectedValue) {
+  await pollTerminal(
+    (output) => output.includes(`Selected segments: ${expectedValue}`),
     DEFAULT_UI_WAIT_MS
   )
 }
 
 async function waitForTerminalOutputPattern(pattern) {
-  await webdriver.wait(
-    new Condition(`wait for terminal output matching ${pattern}`, async () => {
-      try {
-        const terminalReady = await webdriver.executeScript(`
-          return !!(window.terminal && window.terminal.getBufferAsString);
-        `)
-        if (!terminalReady) {
-          return false
-        }
-
-        const output = await getTerminalBuffer()
-        if (!output) {
-          return false
-        }
-
-        return pattern.test(output.trim())
-      } catch (e) {
-        return false
-      }
-    }),
+  await pollTerminal(
+    (output) => pattern.test(output),
     10000
   )
 }
 
 async function getCheckboxByValueIndex(index) {
-  return await webdriver.findElement(By.id(`segments-${index}`))
+  const checkboxes = await webdriver.findElements(
+    By.css('.choice-checklist-item input[type="checkbox"]')
+  )
+  return checkboxes[index]
 }
 
 describe('config: checklist', function () {
@@ -118,8 +110,8 @@ describe('config: checklist', function () {
     await selectNone.click()
     await webdriver.sleep(300)
 
-    const hidden = await webdriver.findElement(By.id('segments-value'))
-    expect(await hidden.getAttribute('value')).to.equal('')
+    const valueInput = await webdriver.findElement(By.css('.choice-checklist > input'))
+    expect(await valueInput.getAttribute('value')).to.equal('')
 
     await submitChecklistForm()
     await waitForLogsPage()
diff --git a/service/internal/config/sanitize.go b/service/internal/config/sanitize.go
index 0361083..ecca619 100644
--- a/service/internal/config/sanitize.go
+++ b/service/internal/config/sanitize.go
@@ -34,6 +34,10 @@ func (cfg *Config) Sanitize() {
 	if err := cfg.validateReservedActionArgumentNames(); err != nil {
 		log.Fatalf("%v", err)
 	}
+
+	if err := cfg.validateChecklistChoiceValues(); err != nil {
+		log.Fatalf("%v", err)
+	}
 }
 
 func (cfg *Config) validateReservedActionArgumentNames() error {
@@ -60,6 +64,49 @@ func (action *Action) validateReservedArgumentNames() error {
 	return nil
 }
 
+func (cfg *Config) validateChecklistChoiceValues() error {
+	for _, action := range cfg.Actions {
+		if err := action.validateChecklistChoiceValues(); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func (action *Action) validateChecklistChoiceValues() error {
+	if action == nil {
+		return nil
+	}
+
+	for _, arg := range action.Arguments {
+		if err := validateChecklistChoicesForArgument(action.Title, arg); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func validateChecklistChoicesForArgument(actionTitle string, arg ActionArgument) error {
+	if arg.Type != "checklist" {
+		return nil
+	}
+
+	for _, choice := range arg.Choices {
+		if strings.Contains(choice.Value, ",") {
+			return fmt.Errorf(
+				`action %q argument %q choice value %q must not contain commas`,
+				actionTitle,
+				arg.Name,
+				choice.Value,
+			)
+		}
+	}
+
+	return nil
+}
+
 func (cfg *Config) sanitizeDashboardsForInlineActions() {
 	for _, dashboard := range cfg.Dashboards {
 		cfg.sanitizeDashboardComponentForInlineActions(dashboard)
diff --git a/service/internal/config/sanitize_test.go b/service/internal/config/sanitize_test.go
index 6c3d4ad..0f68122 100644
--- a/service/internal/config/sanitize_test.go
+++ b/service/internal/config/sanitize_test.go
@@ -271,3 +271,26 @@ func TestValidateUniqueLocalUserAPIKeys(t *testing.T) {
 	})
 	require.NoError(t, err)
 }
+
+func TestValidateChecklistChoiceValuesRejectsCommas(t *testing.T) {
+	t.Parallel()
+
+	c := DefaultConfig()
+	c.Actions = append(c.Actions, &Action{
+		Title: "Checklist commas",
+		Shell: "true",
+		Arguments: []ActionArgument{
+			{
+				Name: "segments",
+				Type: "checklist",
+				Choices: []ActionArgumentChoice{
+					{Value: "kitchen,bedroom"},
+				},
+			},
+		},
+	})
+
+	err := c.validateChecklistChoiceValues()
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), `choice value "kitchen,bedroom" must not contain commas`)
+}