Update: Limit functionality since clipboard access doesn’t work prope…

[ujiro99]

…rly in SidePanel mode.

[claude]

PR #340 コードレビュー

概要: SidePanel モードでクリップボードアクセスが正常に動作しないため、関連機能を制限する変更。OPEN_MODE_BG から SIDE_PANEL を削除し、windowExists() の返り値をディスクリミネーテッドユニオン型に改善し、ShortcutList コンポーネントで AI プロンプトコマンドの SidePanel モード利用制限を追加。

---

1. コード品質とベストプラクティス

✅ windowExists() の返り値型変更（良い改善）

packages/extension/src/services/chrome.ts 18〜29行目

ディスクリミネーテッドユニオン型への変更は優れた改善です。呼び出し元が chrome.windows.get() を二重に呼び出す必要がなくなり、型安全性が向上しています。

⚠️ 問題: JSDoc コメント（17行目付近）が古い情報のまま残っています。

// @returns {Promise<boolean>} True if the window exists, false otherwise

実際の返り値型はもはや Promise<boolean> ではないため、このコメントは誤っており更新が必要です。

---

✅ isTextSelectionOnly() の関数シグネチャ変更

packages/extension/src/components/option/editor/ShortcutList.tsx 36〜48行目

変更前: (openMode: string) => boolean
変更後: (command: Command) => boolean

AI プロンプトコマンドの詳細（aiPromptOption）を参照できるようになっており、意図した変更として理解できます。

軽微な指摘: 関数先頭で const { openMode } = command と destructuring していますが、AI プロンプトコマンドのブランチでは option.openMode を使っており openMode は使われません。読みやすさの観点から整理を推奨します。

---

2. バグ・潜在的問題

⚠️ isTextSelectionOnly() の文字列マッチングに誤検知リスク

packages/extension/src/components/option/editor/ShortcutList.tsx 36〜48行目

const willUseClipboard =
  option.prompt.includes(InsertSymbol[INSERT.CLIPBOARD]) ||
  option.prompt.includes(InsertSymbol[INSERT.SELECTED_TEXT])

InsertSymbol[INSERT.CLIPBOARD] 等の値が "Clipboard" や "SelectedText" のような単純な文字列の場合、ユーザーがその単語を通常の文章としてプロンプトに含めると誤検知の可能性があります。{{Clipboard}} のようなデリミタ付きフォーマットでの比較が適切かどうか確認が必要です。

---

⚠️ useEffect 内の潜在的な再実行ループ

packages/extension/src/components/option/editor/ShortcutList.tsx 134〜152行目

useEffect(() => {
  if (!shortcutValues) return
  shortcutValues.forEach((shortcut: ShortcutCommand, index: number) => {
    const cmd = userCommands.find(...)
    if (cmd && isTextSelectionOnly(cmd) && shortcut?.noSelectionBehavior !== ...) {
      setValue(`shortcuts.shortcuts.${index}.noSelectionBehavior`, ...)
    }
  })
}, [shortcutValues, userCommands, setValue])

setValue() を呼び出すと shortcutValues が更新され、再度 useEffect がトリガーされます。条件として shortcut?.noSelectionBehavior !== SHORTCUT_NO_SELECTION_BEHAVIOR.DO_NOTHING で設定済みなら再実行されない設計ですが、setValue が非同期的に shortcutValues を更新する場合、複数回の更新が走る可能性があります。react-hook-form の shouldDirty: false オプションや useRef による変更済みフィールドの追跡も検討してください。

---

⚠️ エラーメッセージと処理の不一致

packages/extension/src/background_script.ts 455〜460行目

} catch (error) {
  console.error("Failed to get active screen ID:", error)
}

現在は updateActiveScreenId と updateActiveTabId 両方のエラーをキャッチしていますが、メッセージが "Failed to get active screen ID:" のままです。両方の操作を説明するメッセージ（例: "Failed to update active screen ID or active tab ID:" ）に更新すべきです。

---

3. パフォーマンス

✅ 冗長な Chrome API 呼び出しの削減

packages/extension/src/background_script.ts 455〜458行目

変更前は chrome.tabs.get() で tab オブジェクトを取得していましたが、activeInfo に既に windowId と tabId が含まれているため、不要な API 呼び出しが削除されています。良い最適化です。

packages/extension/src/services/screen.ts 18〜25行目

windowExists() の返り値改善により、chrome.windows.get() の二重呼び出しが解消されています。

---

4. セキュリティ

✅ SidePanel でのクリップボードアクセス制限は適切

OPEN_MODE_BG から SIDE_PANEL を削除することで、バックグラウンドスクリプト経由での SidePanel 実行を不可能にしています。Chrome 拡張機能の navigator.clipboard.readText() が SidePanel コンテキストでは動作しないというセキュリティ制約に対する適切な対応です。

---

5. テストカバレッジ

❌ 主要な変更に対するテストが追加されていない

以下の変更に対してテストの追加を推奨します：

1. windowExists() の返り値型変更 (packages/extension/src/services/chrome.ts)

   • { exists: true, window } を返す正常系テスト
   • { exists: false } を返す異常系テスト

2. isTextSelectionOnly() のロジック変更 (packages/extension/src/components/option/editor/ShortcutList.tsx)

   • AI プロンプトコマンドで SidePanel + クリップボード使用時に true を返すテスト
   • AI プロンプトコマンドで SidePanel + クリップボード不使用時に false を返すテスト

3. 新しい useEffect の動作

   • isTextSelectionOnly なコマンドが選択された場合に noSelectionBehavior が自動的に DO_NOTHING に変更されることの確認

また、既存の background_script.test.ts で chrome.ts のモックが windowExists の新しい返り値型に合わせて更新されているか確認してください。

---

まとめ

カテゴリ	評価
コード品質	概ね良好。JSDoc コメントの未更新あり
バグリスク	useEffect の再実行リスクと文字列マッチングの誤検知に軽微な懸念
パフォーマンス	冗長な Chrome API 呼び出しの削除により改善 ✅
セキュリティ	SidePanel でのクリップボード制限は適切な対応 ✅
テスト	主要な変更に対するテストが追加されておらず、カバレッジ不足

修正推奨事項（優先度順）:

1. packages/extension/src/services/chrome.ts 17行目: JSDoc の @returns を正確な型に更新
2. packages/extension/src/background_script.ts 459行目: エラーメッセージを "Failed to update active screen ID or active tab ID:" 等に修正
3. packages/extension/src/components/option/editor/ShortcutList.tsx 134〜152行目: useEffect の setValue 呼び出しによる再レンダリングループのリスクを評価・対策
4. isTextSelectionOnly 変更・windowExists 返り値型変更・新 useEffect に対するユニットテストの追加

[codecov]

Codecov Report

❌ Patch coverage is 19.51220% with 66 lines in your changes missing coverage. Please review.
✅ Project coverage is 25.17%. Comparing base (45cae14) to head (ed9b842).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...sion/src/components/option/editor/ShortcutList.tsx	0.00%	38 Missing ⚠️
packages/extension/src/background_script.ts	38.46%	16 Missing ⚠️
packages/extension/src/services/chrome.ts	16.66%	5 Missing ⚠️
packages/extension/src/services/screen.ts	0.00%	5 Missing ⚠️
packages/extension/src/action/aiPrompt.ts	50.00%	1 Missing ⚠️
packages/extension/src/action/pageAction.ts	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #340      +/-   ##
==========================================
- Coverage   25.21%   25.17%   -0.05%     
==========================================
  Files         324      324              
  Lines       31836    31863      +27     
  Branches     1557     1559       +2     
==========================================
- Hits         8028     8020       -8     
- Misses      23808    23843      +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.