Fix TODO state detection and prevent duplicate triggers#18
Fix TODO state detection and prevent duplicate triggers#18salmonumbrella wants to merge 6 commits intoRoamJS:mainfrom
Conversation
When pressing Cmd/Ctrl+Enter on an ARCHIVED block, Roam prepends TODO to get TODO+ARCHIVED. This collapses that into just TODO. Also reads block text from Roam's data layer instead of the textarea DOM value, which can lag behind after API updates. Closes RoamJS#4
- Use .closest('.bp3-menu-item') for reliable context menu detection
instead of fragile parent element traversal
- Wrap keydown DONE/TODO detection in setTimeout to read block text
after Roam processes state changes, preventing stale reads
- Collect multi-select block UIDs before setTimeout to avoid race
conditions with DOM changes
- Add focusin/focusout tracking to detect manual DONE transitions
(blocks edited from non-TODO to DONE now fire onDone callbacks)
- Register cleanup for focus listeners on extension unload
Closes RoamJS#8, closes RoamJS#10
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request enhances the extension's TODO and DONE state management. A new utility function normalizes TODO prefixes by removing archived markers. The main extension file introduces per-block state tracking via focusin/focusout event listeners to detect state transitions, reworks TODO menu item identification through DOM structure inspection, adds debounced batch processing to defer state detection after UI updates, and extends keyboard shortcut handling to read block text from Roam's data layer and convert ARCHIVED blocks to TODO. Cleanup logic is added to remove listeners and clear state on extension unload. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
|
@mdroidian broke these up for you! please take a look ❤️ (again) |
|
As per https://github.com/RoamJS/contributing/blob/main/contributing.md, could you include a video walkthrough of this fix then do a CodeRabbitAI review, please? Thank you. |
|
@coderabbitai full review |
|
This fix is related to the other two PRs and was split up across PRs 17, 18, and 19 for reviewability, so there is not a separate Loom walkthrough for this one. |
✅ Actions performedFull review triggered. |
Could you please link me to the walkthrough video? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
There is no walkthrough video for this split PR set. The work was broken across PRs 17, 18, and 19 for reviewability, so I do not have a single Loom link to share for this branch. |
A video walkthrough is required to help me know that the changes have been tested and are working. Could you please compile the changes locally and use Loom (or other) to show what changes are being proposed in this PR? |
|
@mdroidian These three PRs (#17, #18, and #19) were originally one cohesive change that I split into three separate PRs at your request for reviewability. Since they're all part of the same feature set, the video walkthroughs from the other two PRs demonstrate the functionality covered here as well: PR #17 — ARCHIVED → TODO transition fix: PR #19 — TODONT module with configurable hotkey & bulk archive: PR #18's changes (click handler fix, settled state reads, focusin/focusout tracking) are the glue that makes the state detection reliable across both of those workflows. The videos above show the end-to-end behavior with all three PRs compiled together. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
src/index.ts
Outdated
| if (initialState === "other" && getTodoState(value) === "done") { | ||
| onDone(blockUid, value); | ||
| } |
There was a problem hiding this comment.
🔴 focusout handler can double-trigger onDone after Ctrl+Enter already handled it
When a user presses Ctrl+Enter to toggle a block's state (e.g., from no-checkbox to TODO, then to DONE), the keydownEventListener at line 419-426 fires onDone via a setTimeout. However, the initialEditStateByBlock map (set once during focusin at line 324) is never updated by the keydown handler. When the user eventually leaves the block (causing focusout), the focusoutListener at line 336 sees initialState === "other" (the state when the block was first focused) and getTodoState(value) === "done" (the current state), so it calls onDone a second time. This causes duplicate side-effects: append-text appended twice, replace-tags applied twice, and the block potentially moved to "send-to-block" destination after already being moved.
Prompt for agents
In src/index.ts, the keydownEventListener (around lines 418-426) needs to update the initialEditStateByBlock map after it processes a Ctrl+Enter toggle, so that the focusoutListener does not re-trigger onDone or onTodo. Inside the setTimeout callback (lines 419-426), after calling onDone or onTodo, update the map:
initialEditStateByBlock.set(blockUid, getTodoState(value));
This should be added after line 424 (after the if/else if block inside the setTimeout). Similarly, for the ARCHIVED branch (lines 404-417), after the updateBlock call, set:
initialEditStateByBlock.set(blockUid, "todo");
This ensures the focusout listener sees the state that was already handled, preventing duplicate triggers.
Was this helpful? React with 👍 or 👎 to provide feedback.
The domListeners Registry type expects (this: Document, ev: DocumentEventMap[...]) => void, which is incompatible with (e: MouseEvent) => void due to parameter contravariance. Cast to MouseEvent inside, matching the keydownEventListener pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The 50ms setTimeout + getTextByBlockUid approach caused a regression where Roam's data layer hadn't settled, leading to stale pre-toggle reads — onDone fired repeatedly instead of toggling, duplicating append text. Reverting to textArea.value (which Roam updates synchronously) fixes the toggle cycle. The ARCHIVED handler still uses getTextByBlockUid since textarea lags for that case. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
.closest('.bp3-menu-item')with.querySelectorfor reliable context menu TODO detection, replacing fragile parent element traversal that missed clicks.setTimeout(50ms)so block text is read after Roam processes state changes, preventing stale reads that caused duplicate triggers.focusin/focusouttracking so blocks edited from non-TODO to DONE now fireonDonecallbacks correctly.Test plan
{{[[DONE]]}}→ onDone callback firesCloses #8, closes #10
Summary by CodeRabbit
New Features
Bug Fixes