fix: support multiple SmartBlock buttons with same label in one block #148
Conversation
Add occurrence tracking to correctly identify which button was clicked when multiple buttons share the same label. Uses matchAll() to find all button patterns and returns the Nth match based on position. Fixes RoamJS#136
WalkthroughAdds per-block tracking to distinguish multiple SmartBlock buttons with the same label by introducing maps for button occurrences, text, generation, and cleanup. Computes an occurrenceIndex per button when registering triggers, propagates it to parseSmartBlockButton, and updates parseSmartBlockButton to use matchAll and select the requested occurrence. Adds per-button cleanup/unload logic to maintain counts when DOM changes. Updates function signatures to accept an optional occurrenceIndex. Adds tests covering multiple occurrences, special-character labels, and out-of-bounds indices. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/index.ts (1)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting 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? |
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
✏️ Tip: You can customize this high-level summary in your review settings.