diff options
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 52 | ||||
| -rw-r--r-- | src/tagrulesdialog.cpp | 5 |
2 files changed, 55 insertions, 2 deletions
diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index 1216542..f9f8c53 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -135,6 +135,7 @@ taking that too literally. | 76 | Every field in the rules dialog is free text, so a rule is easy to get wrong | workflow | M | open; design approved 2026-08-13, see `specs/2026-08-13-rule-builder-design.md` | | 77 | No way to see what a rule would collect, in the thread list | workflow | S | open; the dialog counts matches, it cannot show them | | 78 | No way to build a rule from something visible in a message | workflow | M | open; wants 76 first, so the created rule lands in a form that can hold it | +| 79 | Opening the rules dialog and saving destroys the first rule | defect | XS | **fixed on `rule-builder`** 2026-08-13, unreleased. Shipped in 0.16.0; damaged one real rule, repaired by hand | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -4866,6 +4867,57 @@ in CLAUDE.md. **Size: M.** +## 79. Opening the rules dialog and saving destroys the first rule + +**Observed.** Open Tagging rules, press Save, change nothing. The first rule +in the list loses its query and its tags. It then vanishes entirely the next +time anything reads the file, because a rule with an empty query is dropped +as malformed on load. + +Found while building item 76, by a test written to catch a different problem. +Reproduced against the released tag rather than the branch, with a throwaway +worktree at 9585674 and a two-rule fixture: after constructing the dialog and +calling its save path, the store held one rule instead of two. + +**Cause.** `TagRulesDialog::onSelectionChanged` blocks signals for `m_note` +only. Two lines later, `m_enabled->setChecked(rule.enabled)` emits `toggled`, +which is connected to `applyEditsToCurrentRule()`. That handler writes every +field of the current rule from the widgets, and it runs BEFORE +`m_query->setText(rule.query)` has filled the query widget, so it writes the +previous rule's text. On the first open there is no previous rule and the +widgets are empty, so rule 0 gets an empty query and an empty tag list. +`TagRules::load` then drops it (`src/tagrules.cpp:150`). + +The existing comment above the `QSignalBlocker` shows the hazard was known for +`m_note` and simply not extended to `m_enabled`. A blocker per widget is the +wrong shape: the whole load needs one guard. + +**Fix.** Raise `m_reloading` for the duration of `onSelectionChanged` and +restore it afterwards, replacing the single-widget blocker. `m_reloading` +already exists for exactly this class of problem and +`applyEditsToCurrentRule()` already honours it. Also take the rule by value +rather than by const reference: the reference points into `m_working`, which +the handler mutates, so it could be read back half overwritten. + +Fixed on the `rule-builder` branch as part of item 76, with +`switchingRulesDoesNotLeakRowsBetweenThem` in `test_tagrules` as the +regression test. It fails against the unfixed code. + +**Damage in the field, and the repair.** The live rules file had exactly one +casualty: the account rule sitting first in the list, with both `query` and +`add` empty while every sibling account rule was intact. +Restored from `post-new.shell-backup`, which item 44's migration kept, and +verified by loading the file through mailctl's own reader: 17 rules, correct +scoping. The rule had stopped tagging, but only one message had arrived in the +meantime (14968 of 14969 in that account still carried the tag); it was tagged +by hand and the account is now complete. + +**Constraints.** The user chose to leave the fix on the branch rather than cut +a patch release, so 0.16.0 in the field still has it. Do not open that dialog +in a released build. + +**Size: XS** for the fix. The reproduction and the field repair were the work. + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 9f4cf6e..797cd92 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -321,8 +321,9 @@ void TagRulesDialog::onSelectionChanged() m_query->setText(rule.query); m_note->setPlainText(rule.note); - // Parse once, on load, and keep it: Task 10's save path compares against - // this to decide whether the stored string may be left alone. + // Parse once, on load, and keep it: the save path compares against this to + // decide whether the stored string may be left alone, so that opening a + // rule and closing it cannot rewrite the file mailctl also reads. m_loadedQuery = RuleQuery::parse(rule.query); if (m_loadedQuery.parsed) |