Skip to content

Conversation

@trangdoan982
Copy link
Collaborator

@trangdoan982 trangdoan982 commented Dec 15, 2025

https://www.loom.com/share/be086c1e8e26487eb1f7cf3ea992eaf6

Summary by CodeRabbit

  • Bug Fixes
    • Improved tag replacement to preserve list item formatting (bullets, numbering) when inserting node links into list items.

✏️ Tip: You can customize this high-level summary in your review settings.

@linear
Copy link

linear bot commented Dec 15, 2025

@supabase
Copy link

supabase bot commented Dec 15, 2025

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@trangdoan982
Copy link
Collaborator Author

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 15, 2025

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 15, 2025

📝 Walkthrough

Walkthrough

The change introduces a centralized regex pattern (LIST_INDICATOR_REGEX) to identify and strip list item indicators from titles, adds an extractListPrefix utility function, and updates the node replacement logic to preserve leading list prefixes when replacing tags with discourse node links.

Changes

Cohort / File(s) Summary
List indicator handling refactor
apps/obsidian/src/utils/tagNodeHandler.ts
Introduces centralized LIST_INDICATOR_REGEX pattern and extractListPrefix function for consistent list prefix extraction. Updates sanitizeTitle to use the new regex. Modifies createNodeAndReplace to preserve original line list prefixes when replacing tags with discourse node links instead of stripping them entirely.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Focused scope: Single file with a clear, localized change
  • Pattern consistency: Refactoring to centralize regex logic improves maintainability
  • Logic verification needed: Ensure extractListPrefix correctly captures prefixes and that the prefix preservation in createNodeAndReplace handles edge cases (nested lists, mixed indicators, etc.)

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly references the main change: preserving list indicators when converting node tags, which aligns with the core purpose of introducing LIST_INDICATOR_REGEX and extraction logic.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
apps/obsidian/src/utils/tagNodeHandler.ts (2)

15-30: Centralized list regex + prefix extraction look good; consider broader Markdown coverage

Using a shared LIST_INDICATOR_REGEX in both sanitizeTitle and extractListPrefix is a solid refactor and correctly preserves the exact indentation + bullet/number prefix when replacing lines.

If you want this to be more robust long‑term, consider extending the pattern to also recognize common Markdown variants such as:

  • Ordered lists with 1) as well as 1.
  • Task list prefixes like - [ ] / - [x] / * [ ]

so that checkboxes and alternate numbering styles can be treated as part of the “list prefix” you preserve. For example:

-const LIST_INDICATOR_REGEX = /^(\s*)(\d+\.\s+|-\s+|\*\s+|\+\s+)/;
+const LIST_INDICATOR_REGEX =
+  /^(\s*)(\d+[.)]\s+|[-*+]\s+(?:\[[ xX]\]\s+)?)/;

This keeps current behavior but handles more Markdown list forms and task items in one place. Based on learnings, this also stays clear of # handling, so multi‑# tags remain unaffected.


357-362: List prefix preservation in replacement logic matches PR goal

Using extractListPrefix(actualLineText) and then replacementText = listPrefix + linkText achieves the stated goal of keeping list indicators when turning a tagged line into a node link (e.g., - #tag foo- [[Node]] instead of [[Node]]).

One nuance: any additional list adornments (like [ ] in task lists) and trailing text on the line are still discarded, as before. If you later want to preserve those too, you’ll need to split the line into prefix, tag segment, and suffix rather than replacing the entire line. For this PR, the current behavior is consistent with prior semantics while fixing the list‑indicator loss.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e8b750e and 360e6f2.

📒 Files selected for processing (1)
  • apps/obsidian/src/utils/tagNodeHandler.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/main.mdc)

**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefer type over interface in TypeScript
Use explicit return types for functions
Avoid any types when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability

Files:

  • apps/obsidian/src/utils/tagNodeHandler.ts
apps/obsidian/**

📄 CodeRabbit inference engine (.cursor/rules/obsidian.mdc)

apps/obsidian/**: Prefer existing dependencies from apps/obsidian/package.json when adding dependencies to the Obsidian plugin
Follow the Obsidian style guide from help.obsidian.md/style-guide and docs.obsidian.md/Developer+policies for UI and code styling
Use Lucide and custom Obsidian icons alongside detailed elements to provide visual representation of features in platform-native UI

Files:

  • apps/obsidian/src/utils/tagNodeHandler.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 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
Repo: DiscourseGraphs/discourse-graph PR: 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants