summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-08-13 11:33:09 +0200
committerDanilo M. <danix@danix.xyz>2026-08-13 11:33:09 +0200
commit4cb43886ceb18cc6078ab2e8ec28fa15a1c701f2 (patch)
tree7ebcc32f0192b8a901e00bee5d2aab7edc00fe1c
parentd10a5860b290ee35f657e95e509c8dac7608f809 (diff)
downloadqtmaildir-4cb43886ceb18cc6078ab2e8ec28fa15a1c701f2.tar.gz
qtmaildir-4cb43886ceb18cc6078ab2e8ec28fa15a1c701f2.zip
docs: record the rules dialog data-loss defect as item 79
Opening the tagging rules dialog and pressing Save destroyed the first rule in the list, without any editing. The rule lost its query and its tags, then vanished entirely on the next load, since a rule with an empty query is dropped as malformed. Reproduced against the released tag rather than the branch, in a throwaway worktree at 9585674 with a two-rule fixture: constructing the dialog and running its save path left one rule of two. onSelectionChanged blocked signals for the note widget only, while m_enabled::toggled two lines later reached applyEditsToCurrentRule, which writes every field from widgets the loader has not filled yet. The existing comment there shows the hazard was known for one widget and not extended to the other. The fix landed with the builder work: the reloading flag now covers the whole load, and switchingRulesDoesNotLeakRowsBetweenThem is the regression test, verified by mutation to fail without the guard. The live rules file had one casualty, the account rule sitting first in the list, with both its query and its tags empty while every sibling was intact. Restored from the shell backup that the earlier migration kept and verified through mailctl's own reader. The rule had stopped tagging, but only one message had arrived meanwhile; that message is now tagged and the account is complete again at 14969 of 14969.
-rw-r--r--docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md52
-rw-r--r--src/tagrulesdialog.cpp5
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)