Fix TODO transition handling (#4 #8 #10)

[salmonumbrella]

Summary

• Fix Handle ARCHIVED => TODO #4: ARCHIVED now replaces with TODO instead of clearing the block
• Fix On Todo does not add text #8: Fixed checkbox logic (checked=DONE, unchecked=TODO was inverted), ensuring onTodo callback fires correctly
• Fix Results in duplicate tasks #10: Added deduplication via time-windowed Map to prevent duplicate trigger firing from multiple event paths
• Improved menu item detection using .closest() instead of fragile parent traversal
• Added setTimeout wrappers to read block text after Roam processes state changes

Code Review Cleanup

• Extracted magic number 50 into ROAM_STATE_SETTLE_MS constant
• Added periodic cleanup to deduplication Map (prevents memory leak in long sessions)
• Registered clickListener in cleanup return (was leaked on unload)
• Added unload callback to clean up todoTriggerBound dataset flags
• Added guard for empty block text in bulk selection handler
• Removed unnecessary async from event listeners

Test Plan

• Toggle a TODO checkbox → verify onTodo text is appended
• Toggle a DONE checkbox → verify appendText is added
• ARCHIVED block → Cmd+Shift+Enter → verify it becomes TODO (not cleared)
• Move tasks between pages → verify no duplicate triggers
• Bulk select + Ctrl+Enter → verify all blocks toggle correctly
• Keep Roam open for extended session → verify no performance degradation

Closes #4, closes #8, closes #10

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• New Features

  • Debounced TODO/DONE transitions to prevent unintended rapid state changes.

• Bug Fixes

  • ARCHIVED blocks now replace with TODO marker instead of clearing on transition.
  • Enhanced click handling for TODO menu item selections.
  • Improved batch processing for multiple block updates.

[coderabbitai]

Walkthrough

This pull request introduces debounced transition handling for TODO/DONE state changes to prevent duplicate processing. The src/index.ts file adds a stateful debouncing mechanism with latestHandledTransition tracking and time-window throttling, replacing direct onTodo/onDone calls with triggerOnTodo/triggerOnDone wrappers that gate execution based on recent handling state. Additionally, the ARCHIVED replacement logic in src/utils/todont.ts is modified to insert a TODO marker instead of an empty string, and the README.md documents this updated archive behavior.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• Add 'Ignore Tags' feature to TODO/DONE triggers #12: Modifies the same TODO/DONE transition flow in src/index.ts by adding an ignore-tags early-exit condition within the onTodo/onDone handlers, complementing this PR's debouncing mechanism.

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Merge Conflict Detection	⚠️ Warning	❌ Merge conflicts detected (4 files): ⚔️ README.md (content) ⚔️ package.json (content) ⚔️ src/index.ts (content) ⚔️ src/utils/todont.ts (content) These conflicts must be resolved before merging into main.	Resolve conflicts locally and push changes to this branch.

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically references the three main issues being addressed (#4 #8 #10) and describes the primary fix as TODO transition handling.
Linked Issues check	✅ Passed	All three linked issues are addressed: #4 changes ARCHIVED to insert TODO instead of clearing; #8 fixes checkbox logic and adds debouncing to ensure onTodo fires correctly; #10 adds deduplication to prevent duplicate triggers from multiple event paths.
Out of Scope Changes check	✅ Passed	All changes are directly tied to the three linked issues; improvements like menu item detection refactoring, state settle constant extraction, and cleanup registration are necessary supporting changes for the core fixes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

⚔️ Resolve merge conflicts (beta)

• Auto-commit resolved conflicts to branch fix/issues-4-8-10
• Post resolved changes as copyable diffs in a comment

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

src/index.ts (3)

326-338: Consider deriving transition direction from value instead of inputTarget.checked.

After the 50ms settle delay, you already have the authoritative value from getTextByBlockUid. Using inputTarget.checked to determine direction relies on the DOM element reference surviving Roam's re-render cycle. This is consistent with how the keydown handler (lines 375-379) uses value.startsWith(...) to determine direction.

♻️ Suggested refactor

          setTimeout(() => {
            const value = getTextByBlockUid(blockUid);
-           if (inputTarget.checked) {
+           if (value.startsWith("{{[[DONE]]}}")) {
              const config = triggerOnDone(blockUid, value);
              if (config.explode) {
                setTimeout(() => {
                  explode(position.x, position.y);
                }, 50);
              }
-           } else {
+           } else if (value.startsWith("{{[[TODO]]}}")) {
              triggerOnTodo(blockUid, value);
            }
          }, ROAM_STATE_SETTLE_MS);

---

389-400: Guard against undefined block elements in bulk selection.

Line 387-388 retrieves the first .roam-block child of each highlighted element. If a highlighted element unexpectedly lacks a .roam-block child, the result is undefined, and getUids(undefined) on line 389 would throw.

The empty-value guard on line 393 is a good addition for the block text, but a filter before getUids would be safer:

🛡️ Suggested defensive filter

        const blockUids = Array.from(
          document.getElementsByClassName("block-highlight-blue")
        )
          .map(
            (d) => d.getElementsByClassName("roam-block")[0] as HTMLDivElement
          )
+         .filter(Boolean)
          .map((d) => getUids(d).blockUid);

---

299-310: Verify that callers handle the dedup return value correctly.

triggerOnDone returns { explode: false } when deduplicated (line 307) vs the result of onDone(...) otherwise. At the checkbox call site (line 329), config.explode is accessed unconditionally — this works because both paths return an object with explode. Good.

However, in the keydown handler (line 376), triggerOnDone is called but its return value is ignored. This means the explode animation is silently skipped on keyboard-triggered DONE transitions. This was likely pre-existing behavior, but worth noting if the explode feature is expected to work with Ctrl+Enter.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}