obsidian-releases
obsidian-releases copied to clipboard
Add Simple Mention plugin
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]
- [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 mymanifest.json
matches theid
in thecommunity-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
.
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.
👋 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.
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)?
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.
👋 So, I finally resolved the review comments and hope that I could remove all the mentioned issues.
- getActiveViewOfType This can be null
- getChildNodes You could turn this function into a generator so that you don't need to create a list just to iterate through it
- completitionComparer typo maybe?
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 🙏
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 🙏