From 2b65a0d3b9700cd85d3e4e07fb54cef90c8fc99b Mon Sep 17 00:00:00 2001
From: Italo Valcy <italovalcy@gmail.com>
Date: Sat, 11 Jul 2026 05:52:28 -0300
Subject: [PATCH] enhance: detect duplicate input ids in lab guide questions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

parseLabGuide() already rejects duplicate question `name`s, but radio and
checkbox options legitimately repeat a name (they form one group) while
each option must still carry a unique `id` — the "Add checkbox question"
button emits `id`/`<label for="...">` pairs, and duplicate ids silently
break those associations so only the first matching option reacts to a
label click.

Add an independent id-uniqueness pass in the same preview loop: any input
with a repeated id is collected (deduplicated), highlighted red, and the
form is aborted with a "Duplicated input id found: ..." message shown
alongside the existing duplicate-name message.

Extend the jsdom regression suite (tests/js/parseLabGuide.test.js): the
per-case expectation is now { name, id } so a case can assert which
message fires. Adds duplicate-id cases (within a group, ids reused across
groups, checkbox groups, distinct-name text inputs, combined name+id) and
legitimate groups with unique ids. Verified the new cases fail against the
pre-change template and 25/25 pass after.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 apps/templates/pages/labs_edit.html | 29 ++++++++++-
 tests/js/parseLabGuide.test.js      | 79 +++++++++++++++++++----------
 2 files changed, 80 insertions(+), 28 deletions(-)

diff --git a/apps/templates/pages/labs_edit.html b/apps/templates/pages/labs_edit.html
index ea8be96..15299ba 100644
--- a/apps/templates/pages/labs_edit.html
+++ b/apps/templates/pages/labs_edit.html
@@ -880,8 +880,26 @@ <h6 id="field-history-detail-title"></h6>
         const htmlLabGuide = marked.parse(editorLabGuide.getValue());
         document.getElementById('custom-tabs-labguide-preview').innerHTML = htmlLabGuide;
         var seenNames = {};
+        var seenIds = {};
         var duplicates = [];
+        var duplicateIds = [];
         $('#custom-tabs-labguide-preview').find(':input').each(function () {
+          // Every input id must be unique. radio/checkbox options repeat their
+          // name (they form a single group) but each option still needs its own
+          // id, otherwise the <label for="..."> associations collide and only
+          // the first matching option reacts to the label.
+          var id = $(this).attr("id");
+          if (id) {
+            if (seenIds.hasOwnProperty(id)) {
+              if (duplicateIds.indexOf(id) === -1) {
+                duplicateIds.push(id);
+              }
+              $(this).css('border', '2px solid red');
+            } else {
+              seenIds[id] = true;
+            }
+          }
+
           var name = $(this).attr("name");
           if (!name) {
             duplicates.push("this-input-is-missing-its-name!");
@@ -905,9 +923,16 @@ <h6 id="field-history-detail-title"></h6>
           }
         });
         questionCounter = Object.keys(seenNames).length;
-        if (duplicates.length > 0) {
+        if (duplicates.length > 0 || duplicateIds.length > 0) {
 	  if (shouldAbort) event.preventDefault();
-          alert('Duplicated question name found: ' + duplicates.join(', '));
+          var messages = [];
+          if (duplicates.length > 0) {
+            messages.push('Duplicated question name found: ' + duplicates.join(', '));
+          }
+          if (duplicateIds.length > 0) {
+            messages.push('Duplicated input id found: ' + duplicateIds.join(', '));
+          }
+          alert(messages.join('\n'));
           return false;
         }
       }
diff --git a/tests/js/parseLabGuide.test.js b/tests/js/parseLabGuide.test.js
index 4a77bc7..db7cd6a 100644
--- a/tests/js/parseLabGuide.test.js
+++ b/tests/js/parseLabGuide.test.js
@@ -5,7 +5,8 @@
  * parseLabGuide() previews the lab guide Markdown and rejects the form when two
  * questions share the same `name` attribute. radio/checkbox inputs are allowed
  * to repeat a name (they form a single option group); every other repeat is a
- * duplicate question.
+ * duplicate question. Separately, every input `id` must be unique (a repeated id
+ * breaks the <label for="..."> associations of radio/checkbox options).
  *
  * These tests extract the real function from the template (rather than copying
  * its body) and exercise it with the project's actual jQuery and marked builds,
@@ -72,50 +73,76 @@ const TEXT = (n) => `<input type='text' name='${n}' placeholder='x'>\n`;
 const TEXTAREA = (n) => `<textarea name='${n}' rows='6' cols='80'> </textarea>\n`;
 const RADIO = (n) => `<input type='radio' name='${n}' value='1'/>\n`;
 const CHECKBOX = (n) => `<input type='checkbox' name='${n}' value='1'/>\n`;
+// radio/checkbox option with an explicit id + matching label, like the
+// "Add checkbox question" button inserts (name repeats, ids must be unique).
+const RADIO_ID = (n, id) => `<input type='radio' name='${n}' id='${id}' value='1'/> <label for='${id}'>x</label><br>\n`;
+const CHECK_ID = (n, id) => `<input type='checkbox' name='${n}' id='${id}' value='1'/> <label for='${id}'>x</label><br>\n`;
+const TEXT_ID = (n, id) => `<input type='text' name='${n}' id='${id}'>\n`;
+// A full 3-option radio group (name `n`, ids `${p}-1..3`) — the button output.
+const RADIO_GROUP = (n, p) => RADIO_ID(n, `${p}-1`) + RADIO_ID(n, `${p}-2`) + RADIO_ID(n, `${p}-3`);
 
-// { name: [markdown, expectDuplicate] }
+// { label: [markdown, expect] } where expect is { name: bool, id: bool }.
+// Omitted keys default to false; the form must abort iff name || id is true.
 const cases = {
   // --- the reported bug: a select name reused by another field ------------
-  'two identical selects':               [SELECT('q1') + SELECT('q1'), true],
-  'multi-selects same name':             [SELECT_MULTI('q1') + SELECT_MULTI('q1'), true],
-  'select then radio (regression)':      [SELECT('q1') + RADIO('q1'), true],
-  'select then checkbox (regression)':   [SELECT('q1') + CHECKBOX('q1'), true],
-  'radio then select':                   [RADIO('q1') + SELECT('q1'), true],
-  'checkbox then select':                [CHECKBOX('q1') + SELECT('q1'), true],
-  'text then select':                    [TEXT('q1') + SELECT('q1'), true],
-  'select then text':                    [SELECT('q1') + TEXT('q1'), true],
-  'textarea then select':                [TEXTAREA('q1') + SELECT('q1'), true],
-  'selects split by markdown':           [`## Q1\ntext\n\n${SELECT('q1')}\n## Q2\n\n${SELECT('q1')}`, true],
+  'two identical selects':               [SELECT('q1') + SELECT('q1'), { name: true }],
+  'multi-selects same name':             [SELECT_MULTI('q1') + SELECT_MULTI('q1'), { name: true }],
+  'select then radio (regression)':      [SELECT('q1') + RADIO('q1'), { name: true }],
+  'select then checkbox (regression)':   [SELECT('q1') + CHECKBOX('q1'), { name: true }],
+  'radio then select':                   [RADIO('q1') + SELECT('q1'), { name: true }],
+  'checkbox then select':                [CHECKBOX('q1') + SELECT('q1'), { name: true }],
+  'text then select':                    [TEXT('q1') + SELECT('q1'), { name: true }],
+  'select then text':                    [SELECT('q1') + TEXT('q1'), { name: true }],
+  'textarea then select':                [TEXTAREA('q1') + SELECT('q1'), { name: true }],
+  'selects split by markdown':           [`## Q1\ntext\n\n${SELECT('q1')}\n## Q2\n\n${SELECT('q1')}`, { name: true }],
 
   // --- non-select duplicates still caught --------------------------------
-  'two text inputs same name':           [TEXT('q1') + TEXT('q1'), true],
-  'two textareas same name':             [TEXTAREA('q1') + TEXTAREA('q1'), true],
+  'two text inputs same name':           [TEXT('q1') + TEXT('q1'), { name: true }],
+  'two textareas same name':             [TEXTAREA('q1') + TEXTAREA('q1'), { name: true }],
+
+  // --- duplicate ids on radio/checkbox groups (the new check) ------------
+  'duplicate id within radio group':     [RADIO_ID('q1', 'id1-1') + RADIO_ID('q1', 'id1-1'), { id: true }],
+  'two radio groups reuse ids':          [RADIO_GROUP('q1', 'id1') + RADIO_GROUP('q2', 'id1'), { id: true }],
+  'duplicate id within checkbox group':  [CHECK_ID('q1', 'c1') + CHECK_ID('q1', 'c1'), { id: true }],
+  'duplicate id on text inputs':         [TEXT_ID('q1', 'dup') + TEXT_ID('q2', 'dup'), { id: true }],
+  'duplicate name and duplicate id':     [RADIO_ID('q1', 'x') + RADIO_ID('q1', 'x') + SELECT('q1'), { name: true, id: true }],
 
   // --- legitimate content that must NOT be flagged -----------------------
-  'single select':                       [SELECT('q1'), false],
-  'distinct selects':                    [SELECT('q1') + SELECT('q2'), false],
-  'radio option group':                  [RADIO('q1') + RADIO('q1') + RADIO('q1'), false],
-  'checkbox option group':               [CHECKBOX('q1') + CHECKBOX('q1'), false],
-  'one of each distinct':                [TEXT('q1') + SELECT('q2') + TEXTAREA('q3') + RADIO('q4') + RADIO('q4'), false],
-  'empty guide':                         ['', false],
+  'single select':                       [SELECT('q1'), {}],
+  'distinct selects':                    [SELECT('q1') + SELECT('q2'), {}],
+  'radio option group (no ids)':         [RADIO('q1') + RADIO('q1') + RADIO('q1'), {}],
+  'checkbox option group (no ids)':      [CHECKBOX('q1') + CHECKBOX('q1'), {}],
+  'radio group with unique ids':         [RADIO_GROUP('q1', 'id1'), {}],
+  'two radio groups distinct ids':       [RADIO_GROUP('q1', 'id1') + RADIO_GROUP('q2', 'id2'), {}],
+  'one of each distinct':                [TEXT('q1') + SELECT('q2') + TEXTAREA('q3') + RADIO('q4') + RADIO('q4'), {}],
+  'empty guide':                         ['', {}],
 };
 
 // --- Run -------------------------------------------------------------------
 const run = makeRunner();
 let failures = 0;
-for (const [label, [md, expectDup]] of Object.entries(cases)) {
+for (const [label, [md, expect]] of Object.entries(cases)) {
+  const expectName = !!expect.name;
+  const expectId = !!expect.id;
+  const expectAbort = expectName || expectId;
+
   const { ret, alert, prevented } = run(md);
-  const gotDup = ret === false;
+  const gotAbort = ret === false;
+  const hasNameMsg = !!alert && /Duplicated question name found/.test(alert);
+  const hasIdMsg = !!alert && /Duplicated input id found/.test(alert);
+
   const ok =
-    gotDup === expectDup &&
-    (expectDup ? alert && /Duplicated question name found/.test(alert) && prevented
-               : alert === null && !prevented);
+    gotAbort === expectAbort &&
+    prevented === expectAbort &&
+    hasNameMsg === expectName &&
+    hasIdMsg === expectId;
+
   if (ok) {
     console.log(`  ok    ${label}`);
   } else {
     failures++;
     console.error(`  FAIL  ${label}`);
-    console.error(`        expected duplicate=${expectDup}, got=${gotDup}, alert=${JSON.stringify(alert)}, prevented=${prevented}`);
+    console.error(`        expected {name:${expectName}, id:${expectId}}, got abort=${gotAbort}, name=${hasNameMsg}, id=${hasIdMsg}, prevented=${prevented}, alert=${JSON.stringify(alert)}`);
   }
 }