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
WalkthroughThis pull request introduces per-block button occurrence tracking to support multiple SmartBlock buttons with identical labels within the same block. It adds a per-block counter (buttonOccurrences) that computes an occurrenceIndex for each button and passes this index through the trigger registration path to parseSmartBlockButton. The parsing function switches from single-match regex handling to multi-match logic using matchAll to select the appropriate occurrence. Increment and decrement logic is embedded to maintain accurate occurrence counts during dynamic DOM changes. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
| const buttonRegex = trimmedLabel | ||
| ? new RegExp( | ||
| `{{(${trimmedLabel.replace(/\\+/g, "\\+")}):(?:42)?SmartBlock:(.*?)}}` | ||
| `{{(${trimmedLabel.replace(/\\+/g, "\\+")}):(?:42)?SmartBlock:(.*?)}}`, | ||
| "g" | ||
| ) | ||
| : /{{\s*:(?:42)?SmartBlock:(.*?)}}/; | ||
| const match = buttonRegex.exec(text); | ||
| : /{{\s*:(?:42)?SmartBlock:(.*?)}/g; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the file to see the exact code
cat -n src/utils/parseSmartBlockButton.ts | head -30Repository: RoamJS/smartblocks
Length of output: 1081
🏁 Script executed:
# Look for test files related to this utility
find . -type f -name "*parseSmartBlock*" -o -name "*test*" -o -name "*.spec.ts" | grep -i smart | head -20Repository: RoamJS/smartblocks
Length of output: 98
🏁 Script executed:
# Search for usage or tests of this function
rg -n "parseSmartBlockButton|SmartBlock" --type ts --type tsx -A 3 | head -80Repository: RoamJS/smartblocks
Length of output: 89
🏁 Script executed:
# Search for test files
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "*test*" \) | grep -v node_modules | head -20Repository: RoamJS/smartblocks
Length of output: 169
🏁 Script executed:
# Search for usage of parseSmartBlockButton in tests
rg -n "parseSmartBlockButton" --type ts -B 2 -A 5 | head -100Repository: RoamJS/smartblocks
Length of output: 4902
🏁 Script executed:
# Look for examples with + characters in labels or SmartBlock patterns
rg -n "SmartBlock.*\+" --type ts | head -20Repository: RoamJS/smartblocks
Length of output: 44
🏁 Script executed:
# Read the full test file
cat -n tests/buttonParsing.test.tsRepository: RoamJS/smartblocks
Length of output: 2970
🏁 Script executed:
# Look for any tests with special characters in labels
rg -n "parseSmartBlockButton\(" tests/ --type ts -A 2 | grep -E "\+|\\|\\\\|\(|\)|\\[|\\]"Repository: RoamJS/smartblocks
Length of output: 1172
🏁 Script executed:
# Let's understand what the replace is doing: create a simple test
node << 'EOF'
// Test what /\\+/g actually does
const testString = "Add+More";
const result1 = testString.replace(/\\+/g, "\\+");
console.log(`Input: "${testString}"`);
console.log(`Using /\\\\+/g: "${result1}"`);
// What if we want to escape + for regex?
const result2 = testString.replace(/\+/g, "\\+");
console.log(`Using /\\+/g: "${result2}"`);
// Let's see what happens in the actual regex construction
const trimmedLabel = "Add+More";
const builtRegex1 = new RegExp(
`{{(${trimmedLabel.replace(/\\+/g, "\\+")}):(?:42)?SmartBlock:(.*?)}}`,
"g"
);
console.log("\nWith /\\\\+/g:");
console.log(`Regex pattern: ${builtRegex1.source}`);
console.log(`Test string: "{{Add+More:SmartBlock:test}}" matches: ${builtRegex1.test("{{Add+More:SmartBlock:test}}")}`);
// Reset regex for next test
const builtRegex2 = new RegExp(
`{{(${trimmedLabel.replace(/\+/g, "\\+")}):(?:42)?SmartBlock:(.*?)}}`,
"g"
);
console.log("\nWith /\\+/g:");
console.log(`Regex pattern: ${builtRegex2.source}`);
console.log(`Test string: "{{Add+More:SmartBlock:test}}" matches: ${builtRegex2.test("{{Add+More:SmartBlock:test}}")}`);
EOFRepository: RoamJS/smartblocks
Length of output: 382
Fix the regex escape pattern to properly escape + characters in labels.
The current pattern /\\+/g matches one or more backslashes, not plus signs. Labels containing + characters (e.g., Add+More) fail to parse. Change /\\+/g to /\+/g on line 18 to properly escape literal + characters for regex safety.
🧰 Tools
🪛 ast-grep (0.40.3)
[warning] 16-19: 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(
{{(${trimmedLabel.replace(/\\+/g, "\\+")}):(?: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)
🤖 Prompt for AI Agents
In src/utils/parseSmartBlockButton.ts around lines 16 to 21, the replace call
meant to escape literal plus signs is using the wrong regex (/\\+/g) which
matches backslashes; change that regex to /\+/g so trimmedLabel.replace(/\+/g,
"\\+") properly escapes plus characters before building the RegExp pattern.
1 participant
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
✏️ Tip: You can customize this high-level summary in your review settings.