Roam: ENG-798 Node Tags breaks Color Highlighter #402
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds a local cache of discourse nodes and their tags, initializes and refreshes it during observer setup and navigation events, and updates node tag popup rendering to use the cached nodes. Adjusts imports to use a named DiscourseNode type and changes the popup function signature to accept pre-fetched nodes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Roam as Roam App
participant Init as initObservers
participant Cache as DiscourseNode Cache
participant Renderer as renderNodeTagPopupButton
User->>Roam: Open/Navigate
Roam->>Init: Initialize observers/listeners
Init->>Cache: refreshDiscourseNodeCache()
Cache-->>Init: discourseNodes + lowercase tag set ready
Note over Roam,Cache: On config page creation or relevant hash/URL changes
Roam->>Cache: refreshDiscourseNodeCache()
User->>Roam: Hover/interaction on SPAN[data-tag]
Roam->>Renderer: renderNodeTagPopupButton(SPAN, discourseNodes, extensionAPI)
Renderer->>Renderer: Find matched node by tag
alt match found
Renderer-->>User: Render node tag popup
else no match
Renderer-->>User: Do nothing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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: 0
🧹 Nitpick comments (7)
apps/roam/src/utils/renderNodeTagPopup.tsx (3)
8-8: Use a type-only import to avoid emitting runtime code.-import { DiscourseNode } from "./getDiscourseNodes"; +import type { DiscourseNode } from "./getDiscourseNodes";
35-41: Edge case: tags with multiple leading # when data-tag is absent.When falling back to textContent (rare), "##foo" becomes "#foo" and won’t match node.tag "foo". Strip all leading # only in the fallback path.
- const tagAttr = parent.getAttribute("data-tag") || textContent; - const tag = tagAttr.replace(/^#/, "").toLowerCase(); + const attrTag = parent.getAttribute("data-tag"); + const tagSource = attrTag ?? textContent; + const tag = (attrTag ? tagSource.replace(/^#/, "") : tagSource.replace(/^#+/, "")).toLowerCase();Note: This keeps the prior behavior for data-tag while making the fallback robust, consistent with our prior guidance to preserve user-intended extra hashes in authoring flows.
47-49: Initial title derivation may remove the wrong occurrence if the tag text repeats.Optional: remove only the occurrence corresponding to this span by matching the leading “#” variant first, or by slicing based on the tag’s index in the raw string.
apps/roam/src/utils/initializeObserversAndListeners.ts (4)
28-28: Use a type-only import for DiscourseNode.-import getDiscourseNodes, { DiscourseNode } from "~/utils/getDiscourseNodes"; +import getDiscourseNodes, { type DiscourseNode } from "~/utils/getDiscourseNodes";
116-121: Gated rendering is correct; pass cached nodes.Minor: consider early-return style to reduce nesting, but current form is fine.
171-172: Avoid redundant getDiscourseNodes call on hashchange.You call getDiscourseNodes() in the condition and then refresh the cache again. Use the cached discourseNodes for the check.
- if ( - evt.oldURL.endsWith(configPageUid) || - getDiscourseNodes().some(({ type }) => evt.oldURL.endsWith(type)) - ) { + if ( + evt.oldURL.endsWith(configPageUid) || + discourseNodes.some(({ type }) => evt.oldURL.endsWith(type)) + ) { refreshConfigTree(); refreshDiscourseNodeCache(); }Also applies to: 182-183
48-56: Consider scoping caches inside initObservers to avoid cross-init state.If initObservers can run multiple times in a session, module-level caches may leak state across runs. Keeping them inside initObservers and exposing a lightweight accessor would isolate lifecycles. Low priority.
📜 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 (2)
apps/roam/src/utils/initializeObserversAndListeners.ts(6 hunks)apps/roam/src/utils/renderNodeTagPopup.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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.tsapps/roam/src/utils/renderNodeTagPopup.tsx
🔇 Additional comments (2)
apps/roam/src/utils/renderNodeTagPopup.tsx (1)
12-14: Signature change verified: all call sites updated
Only one usage found (in initializeObserversAndListeners.ts) and it matches the new three-argument signature.apps/roam/src/utils/initializeObserversAndListeners.ts (1)
48-56: Local cache + initial refresh look good.Caching nodes and a lowercase tag set, plus refreshing at init, should eliminate redundant queries in hot paths.
Also applies to: 80-80
sid597
force-pushed
the
eng-798-node-tags-breaks-color-highlighter
branch
from
26226bf to
c797cb9
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
sid597
force-pushed
the
eng-798-node-tags-breaks-color-highlighter
branch
2 times, most recently
from
99be069 to
41e70bf
Compare
sid597
force-pushed
the
eng-798-node-tags-breaks-color-highlighter
branch
from
41e70bf to
8592c3d
Compare
sid597
force-pushed
the
eng-798-node-tags-breaks-color-highlighter
branch
from
8592c3d to
2f88247
Compare
* modify dom only for node tags * add background color to a nodetag * use it as color not background color * remove unused refresh * Roam: ENG-693 handle node tags with # in front and update placeholder to use # (#420) * use text not tag * Move the new block as first child of the current block (#422)
* use getDiscourseNodes * Eng-737 use node color to style node tags (#424) * modify dom only for node tags * add background color to a nodetag * use it as color not background color * remove unused refresh * Roam: ENG-693 handle node tags with # in front and update placeholder to use # (#420) * use text not tag * Move the new block as first child of the current block (#422)
https://www.loom.com/share/0310aa0d89b843cc8eb66f42ff9a2f39?sid=bc015e12-05d3-44e7-b84a-a983524d95b5
Summary by CodeRabbit
Bug Fixes
Refactor