Roam: ENG-693 handle node tags with # in front and update placeholder to use # #372
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR standardizes tag handling across Roam UI components: it normalizes user-entered tags by stripping a leading “#” and lowercasing for comparisons, ensures a single “#” when displaying/inserting tags, and updates the tag input placeholder format. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant M as DiscourseNodeMenu
participant D as Document
U->>M: Select tag to insert
M->>M: normalize(tag): strip leading "#" once
M->>D: Insert text "#"+tag+" "
sequenceDiagram
autonumber
actor U as User
participant P as renderNodeTagPopup
participant S as Tag Set
participant N as Node Store
U->>P: Type tag (e.g., "#Tag")
P->>P: norm = strip leading "#" + toLowerCase()
P->>S: Check contains(norm)
alt exists
S-->>P: match found
P->>U: Show matched node
else not exists
P->>U: Show "Create #"+norm
U->>P: Click Create
P->>N: Create node with tag norm
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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/components/settings/NodeConfig.tsx (1)
158-166: Fix regex group usage to correctly extract tag names (current logic misses#tagand over-captures[[...]]).
match[1] || match[2]is wrong for the current alternation. For#tag, the captured group ismatch[3]; for[[...]],match[2]is the inner page title whilematch[1]is the whole#?[[...]]fragment. As written, the validation will fail to detect#tagconflicts and will compare the entire bracket expression instead of the inner title for[[...]].Apply this diff to capture only tag titles and handle both forms:
- const roamTagRegex = /#?\[\[(.*?)\]\]|#(\S+)/g; + const roamTagRegex = /#?\[\[([^\]]+)\]\]|#([^\s#]+)/g; const matches = format.matchAll(roamTagRegex); const formatTags: string[] = []; for (const match of matches) { - const tagName = match[1] || match[2]; + const tagName = (match[1] ?? match[2] ?? "").trim(); if (tagName) { formatTags.push(tagName.toUpperCase()); } }
🧹 Nitpick comments (4)
apps/roam/src/components/settings/NodeConfig.tsx (1)
232-232: Placeholder casing: consider not forcing lowercase for display.The placeholder now lowercases the example tag. Elsewhere, tag displays are sometimes shown with original casing (e.g., Node Menu). For a consistent UX and to avoid oddities with proper nouns (“iOS”, “GraphQL”), consider keeping the original case in the placeholder.
- placeholder={`#${node.text.toLowerCase()}`} + placeholder={`#${node.text}`}apps/roam/src/components/DiscourseNodeMenu.tsx (1)
113-127: Optional: avoid unconditional trailing space insertion.When inserting, you always append a trailing space. This is convenient, but can introduce double spaces or unwanted spacing mid-word. Consider inserting a space only if the next character isn’t whitespace or punctuation.
- const newText = `${currentText.substring( - 0, - cursorPos, - )}${textToInsert}${currentText.substring(cursorPos)}`; + const needsSpace = + !currentText[cursorPos] || !/\s|[)\],.;:!?]/.test(currentText[cursorPos]); + const newText = `${currentText.substring(0, cursorPos)}${textToInsert}${needsSpace ? "" : ""}${currentText.substring(cursorPos)}`;Note: Alternatively compute
textToInsertwithout trailing space and decide space contextually.apps/roam/src/utils/renderNodeTagPopup.tsx (2)
74-74: Display consistency: consider preserving original case in the “Create” label.Elsewhere (Node Menu), the rendered tag keeps its original case. Here it’s forced to lowercase. Decide on one convention for display; for comparisons, keep normalization under the hood.
- text={`Create #${matchedNode.tag?.replace(/^#/, "").toLowerCase()}`} + text={`Create #${matchedNode.tag?.replace(/^#+/, "")}`}
36-43: Optional: centralize tag normalization to avoid duplication.Normalization is repeated across files with slight variations. Introduce a small utility to reduce drift.
Example (new file
apps/roam/src/utils/tagUtils.ts):export const normalizeForCompare = (s?: string) => (s ?? "").replace(/^#+/, "").trim().toLowerCase(); export const formatForDisplay = (s?: string) => { const core = (s ?? "").replace(/^#+/, "").trim(); return core ? `#${core}` : ""; };Then use:
// comparisons normalizeForCompare(n.tag) === normalizeForCompare(input); // display formatForDisplay(item.tag)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/roam/src/components/DiscourseNodeMenu.tsx(2 hunks)apps/roam/src/components/settings/NodeConfig.tsx(1 hunks)apps/roam/src/utils/renderNodeTagPopup.tsx(2 hunks)
🔇 Additional comments (1)
apps/roam/src/components/DiscourseNodeMenu.tsx (1)
124-126: Analytics consistency: confirm whethertagshould include#.You’re capturing
tagwithout the hash here, while other parts of the app may include it. Align the event property format for easier analysis downstream.Would you like me to scan the repo for
posthog.captureusages that include atagproperty and summarize the formats used?
|
#418 carried over using graphite |
https://www.loom.com/share/7aaa1d3cb17b4d25ba58451d21c2ed4a?sid=8d65c3d8-861f-44e4-b3c5-92b4a000a5c1
Summary by CodeRabbit