fix: support duplicate SmartBlock buttons with same label (#136)#148
fix: support duplicate SmartBlock buttons with same label (#136)#148salmonumbrella wants to merge 1 commit intoRoamJS:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-block tracking for SmartBlock button occurrences (counts, texts, generation IDs, cleanup callbacks, and rendered elements). Computes an occurrenceIndex for each rendered button and threads it through registerElAsSmartBlockTrigger to parseSmartBlockButton. parseSmartBlockButton gains an optional occurrenceIndex, uses an escaped global regex with matchAll to select a specific occurrence, and returns null for out-of-range indices. Registers per-button cleanup callbacks, decrements occurrence counts on removal, pre-emptively cleans disconnected elements, and removes per-block state when counts reach zero. Tests added for multiple occurrences and edge cases. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.ts (1)
811-812: Consider potential memory growth withbuttonOccurrences.The
buttonOccurrencesmap tracks counts per block UID. While cleanup occurs in the unload callback (lines 842-854), if unloads fail to trigger (e.g., observer disconnects before cleanup), entries may accumulate.This is low-risk since block UIDs are unique per session and the map size is bounded by the number of blocks with SmartBlock buttons, but worth noting for long-running sessions.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.tssrc/utils/parseSmartBlockButton.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (1)
src/utils/parseSmartBlockButton.ts (1)
parseSmartBlockButton(1-56)
🔇 Additional comments (6)
src/utils/parseSmartBlockButton.ts (2)
1-4: LGTM on the signature change.The addition of
occurrenceIndexwith a default value of0ensures backward compatibility while enabling selection of specific button occurrences.
23-28: LGTM on the multi-match logic.The switch to
matchAll()with bounds checking correctly handles:
- Empty match arrays (returns
null)- Out-of-bounds occurrence indices (returns
null)- Valid indices (selects the correct match)
The redundant
if (!match) return null;on line 28 is defensive but harmless since line 25 already ensures the index is valid.src/index.ts (4)
595-612: LGTM on the parameter addition.Adding
occurrenceIndextoregisterElAsSmartBlockTriggerwith a default of0maintains backward compatibility for other callers like the TODO observer (line 872-877).
823-837: Verify button rendering order matches text order.The occurrence tracking assumes buttons are observed in the same order they appear in the block text. If the DOM observer fires callbacks in a different order (e.g., due to async rendering), buttons could receive incorrect
occurrenceIndexvalues.Consider adding a comment documenting this assumption, or sorting buttons by their DOM position before assigning indices:
🔎 Alternative: Sort by DOM position
If ordering issues arise, an alternative approach would be to batch-process all buttons in a block and assign indices based on DOM order:
// Instead of incrementing per-callback, collect all buttons first // and assign indices based on their position in the DOM
860-883: Note: TODO observer doesn't passoccurrenceIndex.The TODO observer (lines 872-877) doesn't track occurrences like the button observer. If multiple TODO SmartBlocks with the same label exist in a block, they would all resolve to the first match.
This may be intentional if TODO SmartBlocks don't support duplicate labels, but worth confirming.
842-854: Occurrence index can silently fail to register buttons if decrement happens without re-observation.When a button is removed, the
blockOccurrencescounter is decremented (line 852). If the remaining buttons aren't re-observed before the next button registration, theoccurrenceIndexread from the map will be incorrect. This causesparseSmartBlockButtonto returnnull(line 25 of parseSmartBlockButton.ts), and the button won't be registered as a trigger—silently.The bounds check prevents crashes, but creates silent registration failures. This is mitigated in practice because block edits typically trigger a full DOM re-render of button elements, causing fresh observation and index reassignment.
There was a problem hiding this comment.
Thanks for the submission @salmonumbrella !
Unfortunately, this one is a bit trickier. We'd have to make sure it passes all of the parseSmartBlockButton.ts tests, and devise new passing tests that cover the new scenarios (multiple buttons, with labels, without labels, with various attributes, etc).
You can run the tests locally with npm run test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/parseSmartBlockButton.ts (1)
23-28: Approve logic with minor observation on null check.The matchAll approach correctly finds all button occurrences and selects the requested index. The bounds check on line 25 prevents out-of-bounds access for positive indices.
The null check on line 28 appears redundant after the bounds check on line 25, but it does provide additional safety for negative
occurrenceIndexvalues (e.g., -1 would bypass line 25 but fail on line 28). If negative indices are not expected in practice, consider adding an explicit check on line 25:occurrenceIndex < 0 || occurrenceIndex >= matches.length.🔎 Optional: Add explicit negative index check
const matches = Array.from(text.matchAll(buttonRegex)); - if (matches.length === 0 || occurrenceIndex >= matches.length) return null; + if (matches.length === 0 || occurrenceIndex < 0 || occurrenceIndex >= matches.length) return null; const match = matches[occurrenceIndex]; - if (!match) return null;This makes the validation more explicit and eliminates the need for the subsequent null check.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/utils/parseSmartBlockButton.tstests/buttonParsing.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/buttonParsing.test.ts (1)
src/utils/parseSmartBlockButton.ts (1)
parseSmartBlockButton(1-56)
🔇 Additional comments (6)
tests/buttonParsing.test.ts (4)
69-85: LGTM!This test effectively validates the core fix for issue #136. It confirms that multiple buttons with identical labels ("Run It") correctly map to their respective workflows (WorkflowOne and WorkflowTwo) when distinguished by occurrenceIndex.
87-102: LGTM!Good coverage for unlabeled buttons. This test ensures the occurrence tracking mechanism works consistently for both labeled and unlabeled SmartBlock buttons.
104-108: LGTM!Essential boundary check. This test ensures the parser fails gracefully when requesting a non-existent occurrence rather than throwing an error or returning incorrect data.
110-117: LGTM!This test validates proper handling of plus signs (regex metacharacters) in button labels. It confirms that the regex escaping fix (changing
/\+/gin the parser) works correctly. Based on past review comments.src/utils/parseSmartBlockButton.ts (2)
1-4: LGTM!The addition of the optional
occurrenceIndexparameter with a default value of 0 maintains backward compatibility while enabling the new multi-occurrence functionality. Existing callers continue to work without modification.
16-21: Address ReDoS vulnerability from incomplete regex escaping of user input.The
labelparameter originates from user-editable document content (DOM button text in Roam Research), making it a genuine ReDoS risk when used in regex construction. While only the+character is escaped via/\+/g, other regex metacharacters (*,.,?,[,],^,$,{,},|,(,),\) remain unescaped. Malicious input likea+a+a+a+a+aaa...can construct patterns that cause exponential backtracking.Recommendations:
- Escape all regex metacharacters in
labelbefore constructing the regex, or- Use a regex safety library like
recheckto detect unsafe patterns, or- Validate/sanitize labels to allow only safe characters
The non-greedy quantifier
(.*?)provides partial mitigation but does not eliminate the vulnerability.
|
Thanks @mdroidian! I added broader regex escaping for SmartBlock labels and a test covering labels with regex special characters, plus ran the full test suite (npm run test). Let me know if you'd like any other cases covered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/parseSmartBlockButton.ts (1)
25-30: Consider explicit handling of negative indices for clarity.The logic correctly finds all matches and validates bounds. Line 30's null check is technically redundant after the bounds check on line 27, but serves as defensive programming.
One optional improvement: explicitly validate that
occurrenceIndex >= 0on line 27 for clarity, rather than relying on array[-1] returning undefined.🔎 Optional refactor for explicit validation
// Find all matches const matches = Array.from(text.matchAll(buttonRegex)); - if (matches.length === 0 || occurrenceIndex >= matches.length) return null; + if (matches.length === 0 || occurrenceIndex < 0 || occurrenceIndex >= matches.length) return null; const match = matches[occurrenceIndex]; - if (!match) return null;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/utils/parseSmartBlockButton.tstests/buttonParsing.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/buttonParsing.test.ts
🧰 Additional context used
🪛 ast-grep (0.40.3)
src/utils/parseSmartBlockButton.ts
[warning] 18-21: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
{{(${escapeRegex(trimmedLabel)}):(?:42)?SmartBlock:(.*?)}},
"g"
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (4)
src/utils/parseSmartBlockButton.ts (4)
1-4: LGTM! Well-designed parameter addition.The
occurrenceIndexparameter with a default value of 0 maintains backward compatibility while enabling the new multiple-occurrence behavior. The parameter is well-named and positioned appropriately.
15-16: LGTM! Proper regex escaping implementation.The
escapeRegexhelper correctly escapes all regex metacharacters using a comprehensive character class. This properly addresses the previous issue with escaping special characters like+in button labels.
18-23: LGTM! Regex construction is correct and safe.The use of
escapeRegexproperly sanitizes the label before regex construction, and the global flag "g" is correctly applied formatchAllto function. The static analysis warning about ReDoS is noted, but the risk is minimal here because:
- The label is fully escaped, treating it as a literal string
- The surrounding pattern
{{(...):(42)?SmartBlock:(.*?)}}uses non-greedy quantifiers and is relatively simple- User input (label) cannot inject malicious regex patterns due to escaping
31-57: LGTM! Parsing logic is sound.The parsing logic correctly extracts all button properties from the selected match occurrence. The handling of nested structures with the ESCAPE_COMMA mechanism is appropriate, and the final variables object construction including the ButtonContent field is correct.
There was a problem hiding this comment.
This seems like we’re getting closer.
I tested two SmartBlocks using the same label. They render and work correctly on the initial pass, but if a re-render is triggered (for example, when the block is edited), they are no longer recognized as components.
This definitely looks like something that needs further investigation.
Loom walkthrough:
https://www.loom.com/share/5bd86471ecff4d4b96ffc83dd3926679
|
@mdroidian Thanks for the repro and Loom! I pushed a fix that resets per-block button tracking on re-render so buttons stay recognized after edits. Would you mind retesting when you get a chance? |
|
@salmonumbrella — do you happen to have access to Loom or another screen-recording tool? If so, would you be able to run through a few manual test flows and record them so we can have an initial stress-test for this? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Around line 850-862: The occurrenceIndex can be wrong when mutations arrive
out-of-order; instead of relying solely on the incremented counter in
blockOccurrences, compute the index from the live DOM to ensure document order:
for the current element b (passed to registerElAsSmartBlockTrigger), query its
parent (parentUid -> DOM node) for all sibling button elements with the same
label/textContent and count how many precede b to derive occurrenceIndex, then
update blockOccurrences and use that DOM-derived occurrenceIndex (and keep using
buttonGenerationByBlockUid as before) before calling
registerElAsSmartBlockTrigger/parseSmartBlockButton so the selected regex match
aligns with actual DOM position.
🧹 Nitpick comments (2)
src/index.ts (2)
811-816: Consider consolidating per-block tracking into a single structure.Five parallel
Maps keyed byparentUid(or element) must stay in sync manually, which is error-prone during cleanup. A singleMap<string, BlockButtonState>holding all per-block fields would reduce the surface for desynchronization bugs.// Example consolidated structure interface BlockButtonState { text: string; generation: number; occurrences: Map<string, number>; elements: Set<HTMLElement>; } const blockButtonState = new Map<string, BlockButtonState>();
864-900: Cleanup has unreachable/redundant occurrence-decrement code after bulk deletion.When
blockButtons.size === 0(line 873), lines 870–878 delete all per-block tracking — includingbuttonOccurrences,buttonTextByBlockUid, andbuttonGenerationByBlockUid. After that:
- Line 880 reads
buttonGenerationByBlockUid.get(parentUid)→undefined(just deleted).- Lines 884–898 read
buttonOccurrences.get(parentUid)→undefined(just deleted).So the code after line 878 is effectively dead in the
blockButtons.size === 0path. It works today because of the null-guards, but it's fragile and confusing. Consider returning early after the bulk cleanup:Suggested simplification
if (blockButtons) { blockButtons.delete(b); if (blockButtons.size === 0) { buttonElementsByBlockUid.delete(parentUid); buttonOccurrences.delete(parentUid); buttonTextByBlockUid.delete(parentUid); buttonGenerationByBlockUid.delete(parentUid); + return; } }
| // Track occurrence index for buttons with the same label in the same block | ||
| const label = (b.textContent || "").trim(); | ||
| const blockOccurrences = buttonOccurrences.get(parentUid)!; | ||
| const occurrenceIndex = blockOccurrences.get(label) || 0; | ||
| blockOccurrences.set(label, occurrenceIndex + 1); | ||
| const generation = buttonGenerationByBlockUid.get(parentUid) || 0; | ||
|
|
||
| const unload = registerElAsSmartBlockTrigger({ | ||
| textContent: b.textContent || "", | ||
| text, | ||
| el: b, | ||
| parentUid, | ||
| occurrenceIndex, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for createHTMLObserver definition
rg -n "createHTMLObserver" --type=ts -B 2 -A 10 | head -100Repository: RoamJS/smartblocks
Length of output: 3497
🏁 Script executed:
#!/bin/bash
# Look for the implementation of createHTMLObserver
rg -n "function createHTMLObserver|const createHTMLObserver" --type=ts -A 20Repository: RoamJS/smartblocks
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Also search for createBlockObserver and MutationObserver usage
rg -n "MutationObserver" --type=ts -B 3 -A 10Repository: RoamJS/smartblocks
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check the buttonOccurrences and buttonGenerationByBlockUid setup and usage
rg -n "buttonOccurrences|buttonGenerationByBlockUid" --type=ts -B 2 -A 3Repository: RoamJS/smartblocks
Length of output: 3375
🏁 Script executed:
#!/bin/bash
# Search for registerElAsSmartBlockTrigger to see how occurrenceIndex is used
rg -n "registerElAsSmartBlockTrigger" --type=ts -B 2 -A 15 | head -150Repository: RoamJS/smartblocks
Length of output: 2574
🏁 Script executed:
#!/bin/bash
# Look for any DOM position or ordering checks in the codebase
rg -n "compareDocumentPosition|getBoundingClientRect|offsetHeight|offsetWidth|parentNode.*indexOf|Array\.from.*siblings" --type=ts | head -50Repository: RoamJS/smartblocks
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check the full context around lines 850-862 to see the cleanup logic
sed -n '810,910p' src/index.tsRepository: RoamJS/smartblocks
Length of output: 4283
🏁 Script executed:
#!/bin/bash
# Look at the full registerElAsSmartBlockTrigger implementation to see how occurrenceIndex is used
sed -n '595,700p' src/index.tsRepository: RoamJS/smartblocks
Length of output: 3676
🏁 Script executed:
#!/bin/bash
# Search for any usage of occurrenceIndex after it's passed to registerElAsSmartBlockTrigger
rg -n "occurrenceIndex" --type=ts -B 2 -A 2Repository: RoamJS/smartblocks
Length of output: 2293
🏁 Script executed:
#!/bin/bash
# Look at the full parseSmartBlockButton implementation
cat src/utils/parseSmartBlockButton.tsRepository: RoamJS/smartblocks
Length of output: 1794
🌐 Web query:
MutationObserver callback DOM order guarantee
💡 Result:
MutationObserver does not promise “DOM/tree order” sorting of records. What it does guarantee (per spec) is queue/insertion order:
- Each observer has a record queue (a spec “queue”). When a mutation is detected, a
MutationRecordis enqueued onto that queue. [1] - In the Infra standard, to enqueue means append to the end of the queue. So the callback receives records in the same order they were enqueued (i.e., the order the DOM algorithms queued them during execution). [2][1]
- Delivery happens later in a microtask (“notify mutation observers” clones the queue and passes that list to your callback). [1]
So: you can rely on chronological enqueue order, but not on “document order”, and you should expect multiple changes from the same task to arrive together in one callback. [1]
Sources
[1] WHATWG DOM Standard — mutation observers, record queue, queue/enqueue, notify steps https://dom.spec.whatwg.org/
[2] WHATWG Infra Standard — “To enqueue in a queue is to append to it.” https://infra.spec.whatwg.org/
🌐 Web query:
MutationObserver DOM traversal order specification W3C
💡 Result:
In the W3C DOM spec line (DOM 4.1 drafts/notes), MutationObserver delivery/ordering is defined algorithmically, not as a “DOM traversal order” like preorder/postorder.
What the spec actually says
- When a mutation happens, the UA queues a mutation record by first computing
nodesas the inclusive ancestors oftarget, then iterating: for eachnodeinnodes, then for each registered observer in that node’s registered-observer list. This is how it determines which observers are “interested”. [1] - The spec defines “tree order” separately as preorder, depth-first traversal, but the mutation-record queuing algorithm above does not say “iterate ancestors in tree order” (or reverse tree order). [2]
- Delivery happens later in a (compound) microtask: for each
MutationObserverin the notify list, it takes a copy of that observer’s record queue (a FIFO concept) and invokes the callback with that list. [3]
Practical implication
- The spec guarantees: records are delivered to each observer in the order they were enqueued into that observer’s record queue (since delivery uses a copy of that queue). [3]
- The spec does not explicitly guarantee a particular ancestor traversal direction (root→target vs target→root) for “inclusive ancestors” in the queuing step, so there isn’t a separately named “DOM traversal order” requirement for MutationObserver beyond the algorithms. [1], [2]
Sources (W3C/WHATWG specs):
[1] DOM Standard / DOM 4.1: “Queuing a mutation record” (inclusive ancestors; nested loops) https://dom.spec.whatwg.org/#queue-a-mutation-record and W3C DOM 4.1 WD text https://www.w3.org/TR/2017/WD-dom41-20170321/
[2] W3C DOM 4.1 WD: definition of “tree order” (preorder, depth-first) https://www.w3.org/TR/2017/WD-dom41-20170321/
[3] W3C DOM 4.1 NOTE: “notify mutation observers” (copy record queue, then callback) https://www.w3.org/TR/2020/NOTE-dom41-20200317/
Occurrence index could map to wrong button if mutations fire out of DOM order.
The occurrence counter assumes createHTMLObserver processes buttons within the same block in the order they appear in the DOM. However, MutationObserver (which underlies createHTMLObserver) only guarantees chronological record enqueue order, not document order. If mutations are detected in a different sequence than the buttons' DOM positions, they receive misaligned occurrence indices, causing parseSmartBlockButton to select the wrong regex match in the block text and trigger the wrong workflow.
The code mitigates this somewhat with cleanup of disconnected elements and generation tracking, but doesn't address the core reordering risk. Consider validating the computed occurrenceIndex against the button's actual DOM position among its siblings, or computing indices from the live DOM before selecting a match.
🤖 Prompt for AI Agents
In `@src/index.ts` around lines 850 - 862, The occurrenceIndex can be wrong when
mutations arrive out-of-order; instead of relying solely on the incremented
counter in blockOccurrences, compute the index from the live DOM to ensure
document order: for the current element b (passed to
registerElAsSmartBlockTrigger), query its parent (parentUid -> DOM node) for all
sibling button elements with the same label/textContent and count how many
precede b to derive occurrenceIndex, then update blockOccurrences and use that
DOM-derived occurrenceIndex (and keep using buttonGenerationByBlockUid as
before) before calling registerElAsSmartBlockTrigger/parseSmartBlockButton so
the selected regex match aligns with actual DOM position.
salmonumbrella
force-pushed
the
fix/duplicate-button-labels
branch
2 times, most recently
from
eb46c78 to
20eba48
Compare
|
@mdroidian Thanks again for the Loom repro. I pushed a follow-up fix to harden duplicate-label button registration across block re-renders/edits. Could you retest the same scenario (two SmartBlocks with the same label, then edit the block) and confirm they still stay recognized as components? |
do you happen to have access to Loom or another screen-recording tool? If so, would you be able to run through a few manual test flows and record them so we can have an initial stress-test for this? |
|
Hi — thank you for putting this together. I appreciate the effort here. We just added a formal contributing guide, so this likely wasn’t visible when you started the PR. Going forward, we’re trying to standardize reviews around it to keep things manageable: https://github.com/RoamJS/contributing/blob/main/contributing.md A couple adjustments are needed before this can move forward:
Once those are complete, please tag me in a comment or as a reviewer and I’ll be happy to take a look. |
salmonumbrella
force-pushed
the
fix/duplicate-button-labels
branch
from
20eba48 to
84de9f8
Compare
salmonumbrella
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 869-904: The cleanup function's control flow is confusing because
after deleting all per-block state (buttonElementsByBlockUid, buttonOccurrences,
buttonTextByBlockUid, buttonGenerationByBlockUid) the later checks that
reference buttonGenerationByBlockUid and buttonOccurrences are effectively dead;
to fix, inside cleanup (the closure referenced by cleanup/unload and using
parentUid, label, generation) either move the generation-check and
occurrence-decrement logic to run before the block-wide deletions, or simply add
an explicit return immediately after the blockButtons.size === 0 branch (after
deleting the per-block maps) with a short comment explaining we already removed
all tracking for parentUid so no further occurrence/generation work is needed;
update references to buttonGenerationByBlockUid and buttonOccurrences
accordingly to avoid reading deleted entries.
---
Duplicate comments:
In `@src/index.ts`:
- Around line 855-867: The occurrenceIndex calculation relying on callback order
is fragile; instead compute the index from the live DOM when registering the
element: inside the registration path that uses label, parentUid,
occurrenceIndex and calls registerElAsSmartBlockTrigger (and where
buttonGenerationByBlockUid/buttonOccurrences are used), query the block element
for all sibling buttons (or elements with the same selector/textContent) whose
trimmed textContent equals label, build the NodeList/array in DOM order and set
occurrenceIndex to the indexOf the specific element b; then use that DOM-derived
occurrenceIndex when calling registerElAsSmartBlockTrigger and update
buttonOccurrences/buttonGenerationByBlockUid as needed (or remove the counter
increment logic) so registration no longer depends on MutationObserver callback
order.
| const cleanup = () => { | ||
| if (!buttonCleanupByElement.has(b)) return; | ||
| buttonCleanupByElement.delete(b); | ||
| unloads.delete(cleanup); | ||
| b.removeAttribute("data-roamjs-smartblock-button"); | ||
| unload(); | ||
| }); | ||
| const blockButtons = buttonElementsByBlockUid.get(parentUid); | ||
| if (blockButtons) { | ||
| blockButtons.delete(b); | ||
| if (blockButtons.size === 0) { | ||
| buttonElementsByBlockUid.delete(parentUid); | ||
| buttonOccurrences.delete(parentUid); | ||
| buttonTextByBlockUid.delete(parentUid); | ||
| buttonGenerationByBlockUid.delete(parentUid); | ||
| } | ||
| } | ||
| if ((buttonGenerationByBlockUid.get(parentUid) || 0) !== generation) { | ||
| return; | ||
| } | ||
| // Clean up occurrence tracking when button is removed | ||
| const blockOccurrences = buttonOccurrences.get(parentUid); | ||
| if (blockOccurrences) { | ||
| const currentCount = blockOccurrences.get(label) || 0; | ||
| if (currentCount <= 1) { | ||
| blockOccurrences.delete(label); | ||
| if (blockOccurrences.size === 0) { | ||
| buttonOccurrences.delete(parentUid); | ||
| if (!buttonElementsByBlockUid.get(parentUid)?.size) { | ||
| buttonTextByBlockUid.delete(parentUid); | ||
| buttonGenerationByBlockUid.delete(parentUid); | ||
| } | ||
| } | ||
| } else { | ||
| blockOccurrences.set(label, currentCount - 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cleanup logic has a dead-code path after full block state deletion.
When blockButtons.size === 0 (Lines 878–883), all tracking maps for parentUid are deleted — including buttonGenerationByBlockUid and buttonOccurrences. The subsequent code at Lines 885–903 then reads from these now-deleted maps:
- Line 885:
buttonGenerationByBlockUid.get(parentUid)→undefined, so(undefined || 0) !== generationistruewhenevergeneration > 0, causing an early return. - If
generation === 0:buttonOccurrences.get(parentUid)→undefined, so theifat Line 890 is skipped.
This means Lines 885–904 are effectively unreachable with any meaningful side-effect when blockButtons.size === 0. While not a bug (behavior is correct), the control flow is confusing. Consider reordering: perform the generation check and occurrence decrement before the full-state deletion, or add an early return after Line 883 with a clarifying comment.
♻️ Suggested reorder for clarity
const blockButtons = buttonElementsByBlockUid.get(parentUid);
if (blockButtons) {
blockButtons.delete(b);
- if (blockButtons.size === 0) {
+ if (blockButtons.size === 0) {
buttonElementsByBlockUid.delete(parentUid);
buttonOccurrences.delete(parentUid);
buttonTextByBlockUid.delete(parentUid);
buttonGenerationByBlockUid.delete(parentUid);
+ // All buttons removed from block — full cleanup done.
+ return;
}
}
if ((buttonGenerationByBlockUid.get(parentUid) || 0) !== generation) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cleanup = () => { | |
| if (!buttonCleanupByElement.has(b)) return; | |
| buttonCleanupByElement.delete(b); | |
| unloads.delete(cleanup); | |
| b.removeAttribute("data-roamjs-smartblock-button"); | |
| unload(); | |
| }); | |
| const blockButtons = buttonElementsByBlockUid.get(parentUid); | |
| if (blockButtons) { | |
| blockButtons.delete(b); | |
| if (blockButtons.size === 0) { | |
| buttonElementsByBlockUid.delete(parentUid); | |
| buttonOccurrences.delete(parentUid); | |
| buttonTextByBlockUid.delete(parentUid); | |
| buttonGenerationByBlockUid.delete(parentUid); | |
| } | |
| } | |
| if ((buttonGenerationByBlockUid.get(parentUid) || 0) !== generation) { | |
| return; | |
| } | |
| // Clean up occurrence tracking when button is removed | |
| const blockOccurrences = buttonOccurrences.get(parentUid); | |
| if (blockOccurrences) { | |
| const currentCount = blockOccurrences.get(label) || 0; | |
| if (currentCount <= 1) { | |
| blockOccurrences.delete(label); | |
| if (blockOccurrences.size === 0) { | |
| buttonOccurrences.delete(parentUid); | |
| if (!buttonElementsByBlockUid.get(parentUid)?.size) { | |
| buttonTextByBlockUid.delete(parentUid); | |
| buttonGenerationByBlockUid.delete(parentUid); | |
| } | |
| } | |
| } else { | |
| blockOccurrences.set(label, currentCount - 1); | |
| } | |
| } | |
| const cleanup = () => { | |
| if (!buttonCleanupByElement.has(b)) return; | |
| buttonCleanupByElement.delete(b); | |
| unloads.delete(cleanup); | |
| b.removeAttribute("data-roamjs-smartblock-button"); | |
| unload(); | |
| const blockButtons = buttonElementsByBlockUid.get(parentUid); | |
| if (blockButtons) { | |
| blockButtons.delete(b); | |
| if (blockButtons.size === 0) { | |
| buttonElementsByBlockUid.delete(parentUid); | |
| buttonOccurrences.delete(parentUid); | |
| buttonTextByBlockUid.delete(parentUid); | |
| buttonGenerationByBlockUid.delete(parentUid); | |
| // All buttons removed from block — full cleanup done. | |
| return; | |
| } | |
| } | |
| if ((buttonGenerationByBlockUid.get(parentUid) || 0) !== generation) { | |
| return; | |
| } | |
| // Clean up occurrence tracking when button is removed | |
| const blockOccurrences = buttonOccurrences.get(parentUid); | |
| if (blockOccurrences) { | |
| const currentCount = blockOccurrences.get(label) || 0; | |
| if (currentCount <= 1) { | |
| blockOccurrences.delete(label); | |
| if (blockOccurrences.size === 0) { | |
| buttonOccurrences.delete(parentUid); | |
| if (!buttonElementsByBlockUid.get(parentUid)?.size) { | |
| buttonTextByBlockUid.delete(parentUid); | |
| buttonGenerationByBlockUid.delete(parentUid); | |
| } | |
| } | |
| } else { | |
| blockOccurrences.set(label, currentCount - 1); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 869 - 904, The cleanup function's control flow is
confusing because after deleting all per-block state (buttonElementsByBlockUid,
buttonOccurrences, buttonTextByBlockUid, buttonGenerationByBlockUid) the later
checks that reference buttonGenerationByBlockUid and buttonOccurrences are
effectively dead; to fix, inside cleanup (the closure referenced by
cleanup/unload and using parentUid, label, generation) either move the
generation-check and occurrence-decrement logic to run before the block-wide
deletions, or simply add an explicit return immediately after the
blockButtons.size === 0 branch (after deleting the per-block maps) with a short
comment explaining we already removed all tracking for parentUid so no further
occurrence/generation work is needed; update references to
buttonGenerationByBlockUid and buttonOccurrences accordingly to avoid reading
deleted entries.
2 participants
Summary
Fixes #136 - Enables multiple SmartBlock buttons with the same label in a single block to trigger their respective workflows correctly.
Problem
When a block contains multiple buttons with identical labels:
Both buttons would trigger the first workflow (
UserDNPToday) becauseparseSmartBlockButtonalways returned the first regex match.Solution
parseSmartBlockButton.ts: Added anoccurrenceIndexparameter that usesmatchAll()to find all matching buttons and returns the Nth occurrenceindex.ts: Track button occurrence counts per block and pass the correct index when registering each buttonHow it works:
occurrenceIndex=0→ matches first{{b:SmartBlock:...}}occurrenceIndex=1→ matches second{{b:SmartBlock:...}}Backward compatibility:
occurrenceIndexdefaults to 0, so blocks with unique button labels work identically to beforeSummary by CodeRabbit
New Features
Bug Fixes
Tests