obsidian-releases icon indicating copy to clipboard operation
obsidian-releases copied to clipboard

Add Simple Mention plugin

Open der-tobi opened this issue 2 years ago • 5 comments

I am submitting a new Community Plugin

Repo URL

Link to my plugin: https://github.com/der-tobi/obsidian-simple-mention

Release Checklist

  • [x] I have tested the plugin on
    • [x] Windows
    • [ ] macOS
    • [ ] Linux
    • [ ] Android (if applicable)
    • [ ] iOS (if applicable)
  • [x] My GitHub release contains all required files
    • [x] main.js
    • [x] manifest.json
    • [ ] styles.css (optional)
  • [x] GitHub release name matches the exact version number specified in my manifest.json (Note: Use the exact version number, don't include a prefix v)
  • [x] The id in my manifest.json matches the id in the community-plugins.json file.
  • [x] My README.md describes the plugin's purpose and provides clear usage instructions.
  • [x] I have read the tips in https://github.com/obsidianmd/obsidian-releases/blob/master/plugin-review.md and have self-reviewed my plugin to avoid these common pitfalls.
  • [x] I have added a license in the LICENSE file.
  • [x] My project respects and is compatible with the original license of any code from other plugins that I'm using. I have given proper attribution to these other projects in my README.md.

der-tobi avatar Mar 24 '22 22:03 der-tobi

https://github.com/der-tobi/obsidian-simple-mention/blob/5ca64587aae6d14f17d6a2e628fc16c8164e1c69/src/main.ts#L55

See tip at: https://github.com/obsidianmd/obsidian-releases/blob/master/plugin-review.md#avoid-managing-references-to-custom-views

https://github.com/der-tobi/obsidian-simple-mention/blob/5ca64587aae6d14f17d6a2e628fc16c8164e1c69/src/main.ts#L56 Memory leak here - your view isn't unsubscribed from the event when the view is disposed (view.onunload).

https://github.com/der-tobi/obsidian-simple-mention/blob/5ca64587aae6d14f17d6a2e628fc16c8164e1c69/src/PreviewStyle.ts#L21 Should cache this regex rather then re-creating a new copy of it for every single DOM text node.

https://github.com/der-tobi/obsidian-simple-mention/blob/5ca64587aae6d14f17d6a2e628fc16c8164e1c69/src/RegExp.ts#L7 I'm also a little bit concerned with the performance of this regex, especially given you're applying it to all text in the renderer. Not sure if there's much we can do about it though.

https://github.com/der-tobi/obsidian-simple-mention/blob/5ca64587aae6d14f17d6a2e628fc16c8164e1c69/src/PreviewStyle.ts#L68 Function implicitly returns undefined past this if check.

https://github.com/der-tobi/obsidian-simple-mention/blob/5ca64587aae6d14f17d6a2e628fc16c8164e1c69/src/MentionView.ts#L107 Prefer to use innerText instead of innerHTML.

https://github.com/der-tobi/obsidian-simple-mention/blob/5ca64587aae6d14f17d6a2e628fc16c8164e1c69/src/MentionView.ts#L186 Possible DOM injection vulnerability.

Please read: https://github.com/obsidianmd/obsidian-releases/blob/master/plugin-review.md#avoid-innerhtml-outerhtml-and-insertadjacenthtml

https://github.com/der-tobi/obsidian-simple-mention/blob/5ca64587aae6d14f17d6a2e628fc16c8164e1c69/src/MentionView.ts#L217 Please use async-await instead of .then here.

https://github.com/der-tobi/obsidian-simple-mention/blob/5ca64587aae6d14f17d6a2e628fc16c8164e1c69/src/MentionView.ts#L238 A lot of duplicate code between the if/else - You should use workspace.getLeaf(condition) where condition is whether to open a new pane.

lishid avatar Apr 05 '22 00:04 lishid

👋 Just wanted to also mention that you might benefit by including a screenshot in your README for users to quickly see what the plugin does.

liamcain avatar May 07 '22 00:05 liamcain

Thank you for the detailed feedback. I'm a little short on time right now, and hope to fix the issues soon.

BRAT users have also reported issues using the plugin, which I need to fix as well (see https://github.com/der-tobi/obsidian-simple-mention/issues/1) . Is there a recommended way for performance optimized caching (e.g. worker + persistence with indexeddb)?

der-tobi avatar May 16 '22 21:05 der-tobi

Is there a recommended way for performance optimized caching (e.g. worker + persistence with indexeddb)?

I would recommend just profiling what you have first before going the route of rewriting things to use a worker or indexdb. Often times, there is some simple low-hanging fruit that you can fix up that will speed everything up considerably. There's a basic guide on the Obsidian community vault with some details on profiling.

liamcain avatar May 23 '22 11:05 liamcain

👋 So, I finally resolved the review comments and hope that I could remove all the mentioned issues.

der-tobi avatar Aug 05 '22 18:08 der-tobi


Anyway, I didn't spot any blockers and this PR has waited long enough. I think it can go in. Thanks for all the fixes and for your patience during the review process 🙏

liamcain avatar Sep 06 '22 11:09 liamcain

Hmmm, not sure why I didn't merge this when I left the last comment. Sorry about that! Merging now, thank you for your patience 🙏

liamcain avatar Sep 15 '22 12:09 liamcain