fix(tasks): don't stamp carry guard key when nothing to carry#232
fix(tasks): don't stamp carry guard key when nothing to carry#232lupinesse wants to merge 1 commit into
Conversation
autoCarryTasks() previously called localStorage.setItem(carryKey, '1') even when all past tasks were done/upcoming and nothing was carried. This blocked re-carry for the rest of the day: if the user later reopened a task (changed it back to inprogress/pending), or if the midnight tick ran while all tasks happened to be done, the guard key prevented the next startup from carrying those tasks forward. Fix: remove the early setItem call. The guard is now only stamped when at least one task is actually carried, so a session where nothing was carried can still attempt the carry again on the next page load. Regression test covers the four cases: all-done past tasks, all-upcoming, empty list (no key set), and the normal carry path (key IS set). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Impact AnalysisChanged modules
Downstream dependants
Test coverage for changed modules
Automated analysis · commit |
JSDoc CoverageSummary
✅ All clearAll exported symbols in scope have complete JSDoc. Verdict: PASS Automated check · commit |
Verdict: REQUEST CHANGES
🟡 Non-blocking issues[test/unit.mjs line 27-30] No CHANGELOG entry accompanies this fix. The carry behaviour is user-facing — a task reopened after midnight on the same day would silently fail to carry the next startup, which is observable. CLAUDE.md requires "Update the CHANGELOG (Keep a Changelog format) for any user-facing change." 🔵 Nitpicks[test/unit.mjs lines 29-30] The block comment [test/unit.mjs line 33] Checklist
Automated review by Claude Code |
lupinesse
left a comment
lupinesse
left a comment
There was a problem hiding this comment.
/pr-review + /jsdoc-check + /a11y-audit + /scss-audit
Verdict: ⛔ DO NOT MERGE YET — 1 🔴 blocking · 2 🟡 non-blocking
GitHub prevents
REQUEST_CHANGESon your own PR, so this is posted as a comment. Treat the single blocking item below as a merge blocker.
🔴 Blocking — CHANGELOG entry missing
CHANGELOG.md has an empty ## Unreleased section. This PR fixes a user-visible bug (tasks invisible after midnight carry found nothing to do). CLAUDE.md definition-of-done item 9:
README / CHANGELOG updated if behaviour changed.
A ### Fixed entry is required before merge. Suggested wording:
### Fixed
- **Tasks invisible after all-done midnight carry** — `autoCarryTasks()` no longer stamps the `wl_carried_` guard key when there are no unfinished tasks to carry. Previously, a midnight clock-tick that found everything `done` would set the guard and block any subsequent carry for the rest of the day, making tasks the user later re-opened invisible on the next startup. (`src/js/11b-timeblock-carry.js`)🟡 Non-blocking — autoCarryTasks() @returns JSDoc is now imprecise
src/js/11b-timeblock-carry.js:81–83:
* @returns {number|undefined} Number of tasks newly carried, or undefined if
* carry has already run today.After this fix undefined is returned in two semantically distinct cases:
- Guard key already set — carry ran today via some earlier session.
- No unfinished tasks found — guard key is not set; the function will run again on next startup.
A caller using the return value to detect "already ran" can no longer distinguish these. Suggested update:
* @returns {number|undefined} Number of tasks newly carried; `undefined` if the
* carry guard is already set for today, or if there were no unfinished past
* tasks (in which case the guard is intentionally left unset so the next
* session can try again).🟡 Non-blocking — test label references wrong PR number
test/unit.mjs:4094,4098 — the block comment says // PR #227 fix and the describe label says regression #227 but this PR is #232. If #227 is an issue number, change the label to issue #227 to avoid confusion with PR numbers.
Audit scope
| Audit | Finding |
|---|---|
/pr-review |
1 blocking (CHANGELOG), 1 non-blocking (JSDoc @returns) |
/jsdoc-check |
No new exported functions; existing autoCarryTasks JSDoc present but @returns description imprecise (noted above) |
/a11y-audit |
No HTML/DOM changes in this diff — no findings |
/scss-audit |
No SCSS changes in this diff — no findings |
What's good
- Fix is minimal and surgical — three lines removed, one bare early-return kept. Hard to mis-read.
- The new WHY comment explains precisely why the guard is intentionally omitted — exactly what CLAUDE.md asks for.
- Five regression tests cover all relevant branches (all-done, all-upcoming, empty list, normal carry, guard-already-set). All 466 tests pass.
- No secrets, build artefacts, or lint issues introduced.
Generated by Claude Code
lupinesse
left a comment
lupinesse
left a comment
There was a problem hiding this comment.
Inline annotation for the non-blocking test-label finding (see top-level review comment for full context).
Generated by Claude Code
| }); | ||
|
|
||
| // ── Regression: autoCarryTasks must not set guard key when nothing to carry ── | ||
| // PR #227 fix: the guard key was stamped even when all past tasks were done/upcoming, |
There was a problem hiding this comment.
🟡 Non-blocking — PR/issue number ambiguity
// PR #227 fix (and describe('regression #227: ...') four lines below) — this PR is #232. If #227 is the issue number that this fix addresses, say issue #227 to distinguish it from the PR number. If it was a draft/rebase artefact, update to #232. Stale cross-references in test labels undermine traceability when someone later bisects history.
Generated by Claude Code
1 participant
Closes the bug where tasks from yesterday were invisible today.
Root cause
autoCarryTasks()stamped thewl_carried_<date>guard key even when there were no unfinished tasks to carry (alldoneorupcoming). This happened most reliably via the midnight clock-tick path (09-clock-weather.js): if all tasks happened to bedoneat midnight, the carry ran, found nothing, set the guard key, and returned. Any tasks the user later reopened (changed back toinprogress/pending) were then invisible on the next startup because the guard blocked re-carry.Fix
Remove the
localStorage.setItemcall from the early-return branch. The guard is now only stamped when at least one task is actually carried, so a session that found nothing to carry does not permanently block the next one.Immediate workaround for today
Run in DevTools console, then reload:
Tests
5 regression cases added to
test/unit.mjscovering: all-done past tasks, all-upcoming, empty list (no key set in any of these), the normal carry path (key IS set), and the guard-already-set early-return.