ENG-921 - normalize tag comparison in observer #470
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds a tag normalization helper and updates tag-matching logic in apps/roam/src/utils/initializeObserversAndListeners.ts to compare normalized tags (stripping leading hashes, trimming, and lowercasing) before rendering tag popup buttons and applying colors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User/Observer
participant M as initializeObserversAndListeners
participant N as Discourse Nodes
participant R as Render/Color
U->>M: Tag input/event
Note right of M: normalizeTag(tag)<br/>- trim<br/>- strip leading #
M->>M: normalizedTag = toLowerCase(...)
loop For each node
M->>N: Access node.tag
M->>M: normalizedNodeTag = normalizeTag(node.tag)
alt normalizedTag == normalizedNodeTag
M->>R: renderNodeTagPopupButton(node)
M->>R: apply tag color
else
Note over M,N: No action
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
116-127: Guard against false positive matches when tags normalize to empty strings.If the incoming tag is something like
"#"or"##"(only hashes), it normalizes to an empty string. Similarly, ifnode.tagis undefined or empty,normalizedNodeTagbecomes an empty string. This would result in a match between an invalid tag and a node without a tag.Consider adding a guard to skip empty normalized tags:
const normalizedTag = normalizeTag(tag); + if (!normalizedTag) return; for (const node of getDiscourseNodes()) { const normalizedNodeTag = node.tag ? normalizeTag(node.tag) : ""; - if (normalizedTag === normalizedNodeTag) { + if (normalizedNodeTag && normalizedTag === normalizedNodeTag) { renderNodeTagPopupButton(s, node, onloadArgs.extensionAPI);
🧹 Nitpick comments (1)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
112-114: Consider moving the helper function outside the callback for performance.The
normalizeTaghelper is redefined on every observer callback invocation. While this doesn't affect correctness, moving it to module scope or outside the callback would be more efficient and allow potential reuse.Apply this diff to move the helper outside the callback:
+ const normalizeTag = (tag: string): string => { + return tag.replace(/^#+/, "").trim().toLowerCase(); + }; + const nodeTagPopupButtonObserver = createHTMLObserver({ className: "rm-page-ref--tag", tag: "SPAN", callback: (s: HTMLSpanElement) => { const tag = s.getAttribute("data-tag"); if (tag) { - const normalizeTag = (tag: string): string => { - return tag.replace(/^#+/, "").trim().toLowerCase(); - }; - const normalizedTag = normalizeTag(tag);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/utils/initializeObserversAndListeners.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#372
File: apps/roam/src/components/DiscourseNodeMenu.tsx:116-116
Timestamp: 2025-08-25T15:53:21.799Z
Learning: In apps/roam/src/components/DiscourseNodeMenu.tsx, when handling tag insertion, multiple leading hashtags (like ##foo) should be preserved as they represent user intent, not normalized to a single hashtag. The current regex /^#/ is correct as it only removes one leading # before adding one back, maintaining any additional hashtags the user intended.
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#372
File: apps/roam/src/components/DiscourseNodeMenu.tsx:116-116
Timestamp: 2025-08-25T15:53:21.799Z
Learning: In apps/roam/src/components/DiscourseNodeMenu.tsx, when handling tag insertion, multiple leading hashtags (like ##foo) should be preserved as they represent user intent, not normalized to a single hashtag. The current regex /^#/ is correct as it only removes one leading # before adding one back, maintaining any additional hashtags the user intended.
Applied to files:
apps/roam/src/utils/initializeObserversAndListeners.ts
🔇 Additional comments (1)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
112-127: The normalization approach correctly addresses the PR objective.The implementation successfully normalizes tag comparison by stripping leading hashes, trimming whitespace, and converting to lowercase. This ensures tags like
"#foo","##foo", and"foo"are treated as equivalent during matching, which aligns with the PR's goal.Note: The retrieved learning about preserving multiple hashtags in
DiscourseNodeMenu.tsxrelates to tag insertion/creation context, not tag comparison. In this observer context, normalizing by removing all leading hashes is the appropriate behavior for matching purposes.
* normalize tag comparison in observer * use existing getCleanTag
mdroidian
deleted the
eng-921-when-a-nodetag-is-defined-with-a-it-isnt-recognized-by-the
branch
Summary by CodeRabbit
New Features
Bug Fixes
Performance