From 6dbf060eb9fa0471e025d4d1563caf469cdaf6e5 Mon Sep 17 00:00:00 2001
From: Its-My-Work <itsmywork@proton.me>
Date: Tue, 22 Sep 2026 11:44:11 +0000
Subject: [PATCH] fix(plan): a restored plan_review card drew checkboxes as
 checked but counted 0
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Found live: a reloaded chat showed every task checked, yet the footer said "0
of 26 selected" and "Approve to Backlog (0)" stayed disabled — clicking a
checkbox did nothing.

Cause: the default selection ("everything without a lint error of its own")
was only ever computed in _prRender (the live WS path). planReviewCardHtml's
per-task checkbox and its footer/button read that same default two DIFFERENT
ways when _planSelections[messageId] didn't exist yet: a checkbox fell back
to `true`, the count/button fell back to an empty Set — checked boxes, zero
count. The DB-restore render path (a page reload) never went through
_prRender at all, so it always hit this inconsistent fallback; worse, it
also never set _planReviews[messageId], so a click's _prTogglePlanTask found
no review to look up dependents in and silently no-op'd.

- planReviewCardHtml now computes and STORES the default selection itself,
  once, and every place in the function reads that one value — no more two
  fallbacks to drift apart. Self-healing: any caller gets a consistent card,
  restored or live.
- The DB-restore call site now sets _planReviews[id] BEFORE rendering, same
  as _prRender always did, so a click on a restored card works.
- Fixed alongside: _prRerenderTasks (runs after every click) updated the
  checkboxes and the button but never the "N of M selected" line next to it
  — it went stale after the very first click. Now updates all three.

Tests: render/plan-review.test.mjs — a card rendered with NO pre-seeded
selection (the exact restore scenario) now has checkboxes, count and button
agreeing, and _planSelections gets populated as a side effect; a toggle
right after a restore-only render actually mutates the Set. Checked live in
real headless Chrome against a real seeded DB row and a real page load:
before the fix reproduced verbatim (2 checked boxes, "0 of 2 selected",
button disabled(0)); after the fix, consistent from the first render and
correct after a click ("1 of 2 selected", button (1), unchecked box stays
unchecked) — screenshots in the 2026-09-22 session scratchpad.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y2ZSHUdJHqVsvSNQ5ZeQ3f
---
 public/index.html                | 27 +++++++++++++++++-------
 test/render/plan-review.test.mjs | 35 ++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 7 deletions(-)

diff --git a/public/index.html b/public/index.html
index 98279fc..e686c04 100644
--- a/public/index.html
+++ b/public/index.html
@@ -9491,17 +9491,27 @@ <h2 id="dlgModalTitle" data-i18n="dlg.title">Delegate to Agent</h2>
   if (!card) return;
   const review = _planReviews[messageId], sel = _planSelections[messageId];
   card.querySelectorAll('.prc-task input[type=checkbox]').forEach(cb => { cb.checked = sel.has(cb.dataset.taskId); });
+  const n = sel.size;
+  const count = card.querySelector('.prc-count');
+  if (count) count.textContent = t('plan.selected.count').replace('{n}', n).replace('{total}', review ? review.tasks.length : n);
   const btn = card.querySelector('.prc-approve-btn');
-  if (btn) { const n = sel.size; btn.disabled = n === 0; btn.textContent = n ? t('plan.approve.btn').replace('{n}', n) : t('plan.approve.none'); }
+  if (btn) { btn.disabled = n === 0; btn.textContent = n ? t('plan.approve.btn').replace('{n}', n) : t('plan.approve.none'); }
 }
 function planReviewCardHtml(review, messageId, imported) {
+  // The one place a selection is defaulted: every task without a lint error of its own, starts
+  // picked. Self-healing on purpose — a card restored from the DB never went through _prRender,
+  // so nothing else guarantees _planSelections[messageId] exists yet; computing (and STORING) the
+  // default here, once, is what makes a checkbox drawn as checked and the "N selected" count
+  // agree, and what makes clicking that checkbox work on the very first render.
+  if (!_planSelections[messageId]) _planSelections[messageId] = new Set(review.tasks.filter(t => !t.errors.length).map(t => t.id));
+  const sel = _planSelections[messageId];
   const errBlock = review.errors.length
     ? `<div class="prc-errors">⚠️ ${escH(t('plan.errors.title'))}<ul>${review.errors.map(e => `<li>${escH(e)}</li>`).join('')}</ul></div>` : '';
   const warnBlock = (!imported && review.warnings.length)
     ? `<div class="prc-warnings">${escH(t('plan.warnings.title'))}<ul>${review.warnings.slice(0, 8).map(w => `<li>${escH(w)}</li>`).join('')}</ul></div>` : '';
   const depsTitle = (ids) => ids.length ? `${t('plan.deps')} ${ids.join(', ')}` : '';
   const rows = review.tasks.map(tk => {
-    const checked = imported ? true : (_planSelections[messageId] ? _planSelections[messageId].has(tk.id) : true);
+    const checked = imported ? true : sel.has(tk.id);
     const issues = [...tk.errors.map(m => `<span class="prc-e">⚠ ${escH(m)}</span>`), ...tk.warnings.map(m => `<span class="prc-w">${escH(m)}</span>`)];
     return `<label class="prc-task${tk.errors.length ? ' prc-task-err' : ''}" title="${escH(depsTitle(tk.depends_on))}">
       ${imported ? '' : `<input type="checkbox" data-task-id="${escH(tk.id)}" ${checked ? 'checked' : ''} ${tk.errors.length ? 'disabled' : ''} onchange="_prTogglePlanTask('${messageId}','${escH(tk.id)}',this.checked)">`}
@@ -9520,8 +9530,8 @@ <h2 id="dlgModalTitle" data-i18n="dlg.title">Delegate to Agent</h2>
   const footer = imported
     ? `<span class="prc-done">✅ ${escH(t('plan.imported.done'))}</span>
        <span class="prc-done-detail">${escH(t('plan.imported.detail').replace('{c}', imported.created).replace('{u}', imported.updated).replace('{s}', imported.skipped.length))}</span>`
-    : `<span class="prc-count">${escH(t('plan.selected.count').replace('{n}', (_planSelections[messageId] || new Set()).size).replace('{total}', review.tasks.length))}</span>
-       <button class="prc-approve-btn" ${!review.canApprove || !review.tasks.length ? 'disabled' : ''} onclick="_prApprove('${messageId}')">${escH(t('plan.approve.btn').replace('{n}', (_planSelections[messageId] || new Set()).size))}</button>`;
+    : `<span class="prc-count">${escH(t('plan.selected.count').replace('{n}', sel.size).replace('{total}', review.tasks.length))}</span>
+       <button class="prc-approve-btn" ${!review.canApprove || !review.tasks.length ? 'disabled' : ''} onclick="_prApprove('${messageId}')">${escH(t('plan.approve.btn').replace('{n}', sel.size))}</button>`;
   return `<div class="plan-review-card" data-message-id="${messageId}">
     <div class="prc-header">
       <div class="prc-icon">📋</div>
@@ -9536,8 +9546,7 @@ <h2 id="dlgModalTitle" data-i18n="dlg.title">Delegate to Agent</h2>
   </div>`;
 }
 function _prRender(review, messageId) {
-  _planReviews[messageId] = review;
-  if (!_planSelections[messageId]) _planSelections[messageId] = new Set(review.tasks.filter(t => !t.errors.length).map(t => t.id));
+  _planReviews[messageId] = review; // the default selection itself is now planReviewCardHtml's own job — one place, not two
   const old = document.querySelector(`.plan-review-card[data-message-id="${messageId}"]`);
   if (old) old.remove();
   const wrap = document.createElement('div');
@@ -13153,10 +13162,14 @@ <h2 id="dlgModalTitle" data-i18n="dlg.title">Delegate to Agent</h2>
   if (m.role === 'assistant' && m.type === 'plan_review') {
     try {
       const review = JSON.parse(m.content);
+      // Set BEFORE rendering: planReviewCardHtml reads _planReviews[messageId] nowhere itself, but
+      // _prTogglePlanTask/_prApprove do once the user clicks something on this restored card, and
+      // without this they silently no-op (the bug this fixes: checkboxes drawn checked, the "N
+      // selected" count and the button both stuck at 0, because nothing had computed the default yet).
+      _planReviews[String(m.id)] = review;
       const wrap = document.createElement('div');
       wrap.innerHTML = planReviewCardHtml(review, String(m.id), review.imported || null);
       w.querySelector('.msg').before(wrap.firstElementChild);
-      if (!review.imported && !_planSelections[String(m.id)]) _planReviews[String(m.id)] = review;
     } catch {}
   }
   if (m.role === 'assistant' && m.type !== 'agent_plan' && m.type !== 'plan_review') {
diff --git a/test/render/plan-review.test.mjs b/test/render/plan-review.test.mjs
index 2078684..c6272c1 100644
--- a/test/render/plan-review.test.mjs
+++ b/test/render/plan-review.test.mjs
@@ -38,6 +38,37 @@ const planReviewCardHtml = loadFn('planReviewCardHtml');
   assert.ok(out.includes('Зависит от:') && out.includes('T-001'), "T-002's dependency on T-001 is shown");
 }
 
+// ── restored from the DB: no pre-seeded selection (this is the real bug — a page reload renders
+// the card with _planSelections[messageId] never set, and checkboxes drew as checked while the
+// count and the button both read 0, because they defaulted an unset Set two different ways)
+{
+  delete globalThis._planSelections['m1b'];
+  const out = planReviewCardHtml(REVIEW(), 'm1b', null);
+  const checkedBoxes = (out.match(/data-task-id="[^"]*"\s+checked/g) || []).length;
+  assert.strictEqual(checkedBoxes, 2, 'both checkboxes are drawn checked, same as the "N selected" count below');
+  assert.ok(out.includes('2 из 2 выбрано') || out.includes('Утвердить (2)'), 'the count and the button agree with the checkboxes, not stuck at 0');
+  assert.ok(!/prc-approve-btn"[^>]*disabled/.test(out), 'approve is enabled, not disabled by an empty default selection');
+  assert.deepStrictEqual(globalThis._planSelections['m1b'], new Set(['T-001', 'T-002']), 'rendering it also populated the selection Set (self-healing), so a click right after works');
+}
+{
+  // and the click itself: on a card that was only ever rendered via the restore path (no _prRender
+  // call), _prTogglePlanTask must not silently no-op for lack of a Set to mutate. This is exactly
+  // the DB-restore call site's own job (set _planReviews[id] before rendering) — done here by hand
+  // to isolate the check to _prTogglePlanTask/_prDepMap themselves.
+  delete globalThis._planSelections['m1c'];
+  // independent tasks here — the cascade-to-dependents behaviour is a separate concern, not what
+  // this check is isolating (REVIEW()'s default T-002 depends on T-001, which would also unselect)
+  const review = REVIEW({ tasks: [T('T-001'), T('T-002')] });
+  globalThis._planReviews['m1c'] = review;
+  planReviewCardHtml(review, 'm1c', null);
+  globalThis._prDepMap = loadFn('_prDepMap');
+  globalThis._prTogglePlanTask = loadFn('_prTogglePlanTask');
+  globalThis._prRerenderTasks = () => {}; // the DOM half is exercised in the live-Chrome check, not here
+  globalThis._prTogglePlanTask('m1c', 'T-001', false);
+  assert.deepStrictEqual(globalThis._planSelections['m1c'], new Set(['T-002']), 'unchecking one task actually changes the Set after a DB-restored render');
+  delete globalThis._prDepMap; delete globalThis._prTogglePlanTask; delete globalThis._prRerenderTasks;
+}
+
 // ── errors block the whole approval; a task with its own error is disabled and unchecked
 {
   const withErr = REVIEW({ errors: ['duplicate id "T-001"'], canApprove: false, tasks: [T('T-001', { errors: ['duplicate id "T-001"'] }), T('T-002')] });
@@ -88,6 +119,10 @@ const planReviewCardHtml = loadFn('planReviewCardHtml');
 
 // ── wiring: WS cases, DB-restore path, ingestion-loop branch, exclusions, saveBot-style hidden state
 assert.ok(/case 'plan_review':[\s\S]{0,120}_prRender\(d\.review, d\.messageId\)/.test(html), "the WS 'plan_review' frame renders the live card");
+{
+  const m = /_planReviews\[String\(m\.id\)\] = review;[\s\S]{0,200}?planReviewCardHtml\(review, String\(m\.id\)/.exec(html);
+  assert.ok(m, '_planReviews is set BEFORE the DB-restore render, not after (the bug: a click on a restored card found nothing to toggle)');
+}
 assert.ok(/case 'plan_import_result':[\s\S]{0,60}_prShowResult\(d\)/.test(html), "the WS 'plan_import_result' frame updates it");
 assert.ok(/m\.role === 'assistant' && m\.type === 'plan_review'/.test(html), 'a reloaded session restores the card from its DB row');
 assert.ok(/m\.type === 'plan_review'\) \{ _allMsgs\.push\(m\); continue; \}/.test(html), 'the ingestion loop keeps plan_review rows for the restore pass');