[ENG-926] Add tag as a field in setting (Obsidian) #475
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds an optional tag field to node types and DiscourseNode; sets default tags for Claim and Evidence; updates UI to show a dynamic tag placeholder and validation; refactors tag handling to normalize and match tags without spaces and with optional leading “#”; adjusts content extraction regex and minor UI error layout. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant N as NodeTypeSettings
participant TF as TextField (Tag)
participant GP as generateTagPlaceholder
participant E as Editor
participant TH as tagNodeHandler
U->>N: Open/Edit Node Type
N->>TF: Render tag field (nodeType, value)
TF->>GP: compute placeholder(nodeType.format, nodeType.name)
GP-->>TF: placeholder string
U->>TF: Enter/modify tag
TF-->>N: tag value (validated: no spaces)
rect rgba(227,242,253,0.5)
note right of E: Hover/creation flow
E->>TH: On hover/scan text
TH->>TH: Normalize tag (strip leading "#")
TH->>TH: Build selector .cm-tag-{name}
TH->>E: Match elements and children
TH->>TH: Extract content (remove /#[^\\s]+/g)
alt tag present
TH-->>E: Show tooltip / enable create
else no tag or mismatch
TH-->>E: No action
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. 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
🧹 Nitpick comments (2)
apps/obsidian/src/constants.ts (1)
16-16: Consider adding a tag for the Question node type for consistency.While the
tagfield is optional, the defaultClaimandEvidencenode types have tags butQuestiondoes not. For consistency and better user experience, consider adding a tag like"#que-candidate"to the Question node type as well.Apply this diff to add consistency:
Question: { id: generateUid("node"), name: "Question", format: "QUE - {content}", color: "#99890e", + tag: "#que-candidate", },apps/obsidian/src/components/NodeTypeSettings.tsx (1)
78-86: Consider adding tag format validation.The tag field configuration looks good, but consider adding a
validatefunction to ensure tags follow a consistent format (e.g., start with#, no spaces, etc.). This would help prevent user errors and maintain consistency.Example validation:
tag: { key: "tag", label: "Node tag", description: "Tags that signal a line is a node candidate", type: "text", required: false, placeholder: "Enter tag (e.g., #clm-candidate)", validate: (value) => { if (!value.trim()) return { isValid: true }; // Empty is okay (optional field) if (!value.startsWith('#')) { return { isValid: false, error: "Tag must start with #" }; } if (/\s/.test(value)) { return { isValid: false, error: "Tag cannot contain spaces" }; } return { isValid: true }; }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/obsidian/src/components/NodeTypeSettings.tsx(2 hunks)apps/obsidian/src/constants.ts(1 hunks)apps/obsidian/src/index.ts(1 hunks)apps/obsidian/src/types.ts(1 hunks)
🔇 Additional comments (3)
apps/obsidian/src/types.ts (1)
11-11: LGTM!The optional
tagfield is correctly added to theDiscourseNodetype, consistent with other optional fields liketemplate,description, andcolor.apps/obsidian/src/components/NodeTypeSettings.tsx (1)
268-268: LGTM!The
tagfield is correctly initialized as an empty string in the new node type object, consistent with other optional fields liketemplate.apps/obsidian/src/index.ts (1)
1-1: Events import removal verified
No remaining references toEventsacross the repository; removal is safe.
There was a problem hiding this comment.
Nice.
I'm guessing this needs to be merged after #455 ?
Also, I'm guessing the observer in #455 need to be updated.
How are you planning on handling this?
If we are going to merge this to main, let's comment out the UI in settings until the functionality is there.
UI/UX nit for later:
https://www.loom.com/share/6e7e611608a14ba2b86d540162c7c104
|
|
||
| [email protected]: {} | ||
|
|
||
| [email protected]: {} |
There was a problem hiding this comment.
For good measure, lets nix this change.
In the future, if you don't want prettier to auto format, you can use the command palette and select "Save without formatting".
trangdoan982
force-pushed
the
eng-926-add-node-tags-setting
branch
from
2859d02 to
b6c65bc
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/obsidian/src/utils/tagNodeHandler.ts (1)
133-145: Popover exclusion condition is ineffective for descendants
element.closest(".discourse-tag-popover") === elementonly filters the popover element itself, not its children. Exclude descendants too:- if ( - element.classList.contains("discourse-tag-popover") || - element.closest(".discourse-tag-popover") === element - ) { + if (element.closest(".discourse-tag-popover") !== null) { return false; }
🧹 Nitpick comments (2)
apps/obsidian/src/components/NodeTypeSettings.tsx (2)
10-26: Minor: include “or #…” in dynamic placeholder for consistencyFor cases where a prefix is inferred, consider reflecting both forms to match the generic hint:
- return `Enter tag (e.g., ${prefix}-candidate)`; + return `Enter tag (e.g., ${prefix}-candidate or #${prefix}-candidate)`;
96-109: Add a default placeholder to tag field configWhen format isn’t available, the tag field renders with empty placeholder. Set a generic one:
tag: { key: "tag", label: "Node tag", description: "Tags that signal a line is a node candidate", type: "text", required: false, + placeholder: "Enter tag (e.g., clm-candidate or #clm-candidate)", validate: (value: string) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/obsidian/src/components/NodeTypeSettings.tsx(6 hunks)apps/obsidian/src/constants.ts(1 hunks)apps/obsidian/src/types.ts(1 hunks)apps/obsidian/src/utils/tagNodeHandler.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/obsidian/src/constants.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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/obsidian/src/utils/tagNodeHandler.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/obsidian/src/types.ts
🔇 Additional comments (7)
apps/obsidian/src/types.ts (1)
11-11: Type extension looks goodOptional tag on DiscourseNode aligns with UI and handler usage.
apps/obsidian/src/utils/tagNodeHandler.ts (1)
266-267: Regex widening for tag removal is correctSwitch to
/#[^\s]+/gproperly handles hyphens and nested tags (e.g.,#foo-bar,#foo/bar).apps/obsidian/src/components/NodeTypeSettings.tsx (5)
119-126: Prop pass-through looks goodPassing nodeType to TextField enables dynamic placeholders without widening the component API surface unnecessarily.
127-144: TextField dynamic placeholder logic LGTMConditional placeholder for tag based on nodeType.format is clear and side-effect free.
210-216: Error layout improvement LGTMWrapping children and error ensures stable height and better spacing.
301-309: Initializingtagas empty string is fineOptional field, and falsy check in handler will skip processing.
455-456: Wiring nodeType into TextField LGTMEnables tag placeholder generation during editing.
2 participants
also fixes ENG-935
Summary by CodeRabbit