[[🦉Another Quick Switcher]]の公開前レビューと修正。 ## モバイル対応のため[[Node.js]]モジュールを使わない ### レビューコメント - https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/blob/ba35fe444911c4a2ff0bbac68b7841528cb95b30/src/app-helper.ts#L2 > Don't use any NodeJS packages if you intend for your plugin to work on mobile since NodeJS isn't available there. ### 対応 - `path`を使わないようにする - `extname`, `basename`, `dirname` あたり.. https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/commit/cc3408028d7d7361af0180e619baeb3ec7de5aca ## MarkdownViewとは限らない問題 ### レビューコメント - https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/blob/ba35fe444911c4a2ff0bbac68b7841528cb95b30/src/app-helper.ts#L42 > activeLeaf may be null, and might not be a MarkdownView. ### 対応 - `openFile`はMarkdownをターゲットとしていたので名前変更 - `leaf.openFile`の第2引数は`null`許容になっているためそのまま https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/commit/f1e25b3a8dee404cbb6b2da56fd6ac9acb6a06fb ## Nullチェック漏れ ### レビューコメント - https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/blob/ba35fe444911c4a2ff0bbac68b7841528cb95b30/src/app-helper.ts#L46 > Missing null check on the function return. ### 対応 - `null`チェックする https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/commit/fa359b394c29e5881ad415e305099e7ae866cb82 ## MarkdownView.fileを使った方がいい ### レビューコメント - https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/blob/ba35fe444911c4a2ff0bbac68b7841528cb95b30/src/app-helper.ts#L65 > Here you should just use MarkdownView.file instead of getting the active file (which gets you the active view anyway). ### 対応 ```ts const activeFilePath = this.app.workspace.getActiveFile()?.path; if (!activeFilePath) { return; } const editor = this.app.workspace.getActiveViewOfType(MarkdownView)?.editor; if (!editor) { return; } ``` が冗長で`this.app.workspace.getActiveViewOfType(MarkdownView)`を使う。 https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/commit/b1c288c9992ba92a0ade88e74afb3281593d4e6a ## 不要なcallbackチェック ### レビューコメント - https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/blob/ba35fe444911c4a2ff0bbac68b7841528cb95b30/src/commands.ts#L19 > You don't need a checkCallback here if you don't have any condition to check here. どの状態でも使える機能だから、確かに不要だ..。 ### 対応 https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/commit/3969a9e3b556bf4a17f18398c44ca6f21ba359d8 ==↓ 勘違いしててコマンドパレット起動時に全コマンドが呼ばれるようになってしまってた..== https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/commit/2461f516f2bbf67eb06437bb703250846e1a5aef ## Modifierキーをサポートするより良い方法 ### レビューコメント - https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/blob/ba35fe444911c4a2ff0bbac68b7841528cb95b30/src/ui/AnotherQuickSwitcherModal.ts#L119 > A better (but still undocumented way) is to use this.chooser.useSelectedItem(KeyEvent). ### 対応 これも非公開だが`chooser.useSelectedItem`を使うとよりシンプルに書ける。 https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/commit/c14523495e236846a6042fb260da2dec63a338f1 ## 無駄なActive fileの取得 ### レビューコメント - https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/blob/ba35fe444911c4a2ff0bbac68b7841528cb95b30/src/ui/AnotherQuickSwitcherModal.ts#L141 > You're querying for the active file many times here. Store the value in a variable so you aren't doing that thousands of times. ### 対応 `this.app.workspace.getActiveFile()`が何千回も呼ばれるに1回変数に格納する。 https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/commit/e66315cd29b517a7bfb6e5cc439394e0fa30d21f ## raw HTMLは危険 ### レビューコメント - https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/blob/ba35fe444911c4a2ff0bbac68b7841528cb95b30/src/ui/AnotherQuickSwitcherModal.ts#L238 > Don't use raw HTML because this leads to DOM injection attacks. Always build your DOM from elements. Obsidian provides some shortcuts to create elements quickly, like parentEl.createEl/createDiv({cls: 'css-class', text: 'innerText'}). ### 対応 [[Obsidian]]のDOM構築機構を使う。 https://github.com/tadashi-aikawa/obsidian-another-quick-switcher/commit/a1ee854dfc7ea43a14d45ef02e0e901f7740d601