Feat/add page url

[ujiro99]

close #429

[claude]

コードレビュー: #430 Feat/add page url

現在選択中のページURL(%pageUrl)を各種アクションのURLテンプレートに埋め込めるようにする機能追加です。全体的な設計・テストの充実度は良いですが、実装とテストの不整合と思われる箇所、およびコンテキストによって location.href の意味が変わってしまうエッジケースを見つけたので報告します。

🔴 重要な指摘

1. バックグラウンド(Service Worker)経由の実行パスで location.href が意図しない値になる

• packages/extension/src/action/tab.ts:24、popup.ts、window.ts、backgroundTab.ts、pageAction.ts に pageUrl: location.href が追加されています。これらのモジュールは2つの異なるコンテキストから呼ばれます。
  • packages/extension/src/action/index.ts 経由(useCommandExecutor.ts → content script内)では location.href は正しく閲覧中のページURLになります。
  • しかし packages/extension/src/action/background.ts の actionsForBackground(Popup/Window/Tab/BackgroundTab/PageAction を含む)は background_script.ts:582 の execute(...) からService Worker上で直接呼び出されます。これはショートカットキー実行時、enableSendTab が false(対象タブが chrome:// やウェブストアページ、または content script へのIPC送信が失敗した場合)にフォールバックするパスです(background_script.ts:560-589)。
  • Service Worker内での location.href はページのURLではなく、Service Worker自身のスクリプトURL(chrome-extension://<id>/...)を指してしまいます。このフォールバックパスを通ると pageUrl が全く見当違いの値になります。
• 対応案: pageUrl を呼び出し元(content script側)で明示的に取得して ExecuteCommandParams に含めて渡すか、バックグラウンド実行時は chrome.tabs.query で取得した tab.url を使うようにすべきです。

2. defaultSettings.test.ts の DS-24 テストが実装と矛盾している

• packages/extension/src/services/option/defaultSettings.ts:402 では openMode: OPEN_MODE.POPUP(値は "popup")を設定しています。
• しかし追加されたテスト packages/extension/src/services/option/defaultSettings.test.ts:295 では expect((cmd as any).openMode).toBe("sidePanel") を期待しています。
• OPEN_MODE.POPUP = "popup"、OPEN_MODE.SIDE_PANEL = "sidePanel"(packages/shared/src/constants/open-mode.ts:5,9)なので、このテストは実装と一致せず失敗するはずです。「Search Commands on Hub」をポップアップで開く仕様なのか、サイドパネルで開く仕様なのか、意図を確認して実装かテストのどちらかを修正してください。

3. settings.test.ts の ST-34 / ST-34-a がマイグレーション処理を実際には実行していない

• packages/extension/src/services/settings/settings.test.ts:677 と :696 で settingVersion: "1.1.0" を設定していますが、本PRで package.json のバージョンも 1.1.0 に上げているため(package.json:3)、テスト実行時の VERSION 定数も "1.1.0" になります。
• settings.ts:266 の if (versionDiff(currentVersion, "1.1.0") === VersionDiff.Old) は「現在のバージョンが1.1.0より古い」場合のみtrueになりますが、versionDiff("1.1.0", "1.1.0") は VersionDiff.Same を返すため条件がfalseとなり、migrate1_1_0 が実行されません。
• 同ファイルの他のマイグレーションテスト(例: 0.11.5 用テストは settingVersion: "0.11.3" を使用、0.15.1 用テストは "0.14.2" を使用)は対象バージョンより明確に古い値を使っており、本テストだけこの慣習から外れています。
• 結果として ST-34 は cmd が undefined になり expect(cmd).toBeDefined() で失敗する可能性が高く、ST-34-a はマイグレーションを実質検証しないまま通ってしまいます。settingVersion は "1.0.0" など明確に古い値にすべきです。

🟡 確認したい点

4. デフォルトコマンド「Search Commands on Hub」の searchUrl が %pageUrl を使っている

• packages/extension/src/services/option/defaultSettings.ts:404: searchUrl: `https://selection-command.com/${hubLocale}?q=%pageUrl` となっており、選択テキスト(%s)ではなく現在のページURLを q パラメータに渡しています。
• Hub側(packages/hub/src/app/[lang]/page.tsx や CommandList コンポーネント)を確認しましたが、現状 q クエリパラメータを読み取る実装は見当たりません(コマンド一覧は静的JSONと tagName のみでフィルタされています)。
• コマンド名が「Search Commands on Hub」(Hubでコマンドを検索)であることを踏まえると、本来は選択テキストを検索クエリとして渡す %s を意図していたのに %pageUrl に誤って置き換えてしまった可能性があります。意図的であれば問題ありませんが、念のため確認をお願いします。

🟢 軽微な指摘

5. 変数名の衝突による可読性の低下

• packages/extension/src/services/option/defaultSettings.ts:85 でエクスポートされている固定ID定数 COMMAND_SEARCH_ID と、インドネシア語("id"ロケール)用コマンド変数 CMD_COMMAND_SEARCH_ID(同ファイル453行目)の名前が非常に紛らわしいです。将来的な可読性のため、ロケール変数側の命名を見直すことを検討してください。

6. toUrl のプレースホルダー置換が単一置換のまま

• packages/extension/src/lib/utils.ts:87-89 で %s と %pageUrl の置換に .replace()(先頭一致のみ)を使用しています。既存の %s 置換も同様の実装なので一貫はしていますが、テンプレート内に同じプレースホルダーが複数回登場するケースには対応できません。将来的に replaceAll へ統一することを検討してもよいかもしれません。
• また同関数は戻り値の型が string ですが、searchUrl が undefined の場合は undefined を返し得ます(既存の問題ですが、今回この行に手を入れているのでついでに直すのもありです)。

✅ 良い点

• pageUrl は encodeURIComponent でエンコードしてからテンプレートに埋め込んでいるため、URLインジェクションなどのセキュリティ上の問題はありません。
• migrate1_1_0(settings.ts:388-402)は既存コマンドとのID重複チェックを行っており、二重追加を防ぐロジック自体は適切です。
• toUrl の新しい分岐(TU-14〜TU-17, utils.test.ts:202-244)や DS-24、ST-34 など、新機能に対するユニットテストがしっかり追加されている点は良い取り組みです(内容の不整合は上記参照)。

---

このレビューはPRの差分とリポジトリの現在のコードを確認した上でClaudeが作成しました。

[codecov]

Codecov Report

❌ Patch coverage is 63.63636% with 64 lines in your changes missing coverage. Please review.
✅ Project coverage is 30.11%. Comparing base (09eb031) to head (fbf6e0e).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...sion/src/components/option/editor/ShortcutList.tsx	0.00%	47 Missing ⚠️
packages/extension/src/action/pageAction.ts	0.00%	3 Missing ⚠️
packages/extension/src/action/background.ts	0.00%	2 Missing ⚠️
packages/extension/src/action/backgroundTab.ts	0.00%	2 Missing ⚠️
packages/extension/src/action/popup.ts	0.00%	2 Missing ⚠️
packages/extension/src/action/sidePanel.ts	0.00%	2 Missing ⚠️
packages/extension/src/action/tab.ts	0.00%	2 Missing ⚠️
packages/extension/src/action/window.ts	0.00%	2 Missing ⚠️
packages/extension/src/action/index.ts	0.00%	1 Missing ⚠️
...s/extension/src/services/option/defaultSettings.ts	98.83%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #430      +/-   ##
==========================================
+ Coverage   29.93%   30.11%   +0.17%     
==========================================
  Files         322      322              
  Lines       33274    33435     +161     
  Branches     1943     1955      +12     
==========================================
+ Hits         9962    10070     +108     
- Misses      23312    23365      +53     

☔ View full report in Codecov by Harness.
📢 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.

[claude]

レビュー結果

pageUrl を content script/background の各実行経路に伝播させ、%pageUrl プレースホルダーと「Search Commands on Hub」デフォルトコマンドを追加する変更、全体として意図が明確で、toUrl・defaultSettings・settings のマイグレーションにテストも追加されており良い対応だと思います。いくつか気になった点を挙げます。

1. pageUrl がバックグラウンド直接実行パスで欠落する（バグの疑い）

packages/extension/src/background_script.ts:582-589 の chrome.commands.onCommand ハンドラでは、タブへの送信が使えない場合（chrome:// ページ等）に execute() を直接呼び出し、pageUrl: tab?.url ?? "" を渡しています。

しかし呼び出し先の packages/extension/src/action/background.ts:22-41 の execute は、分割代入のパラメータに pageUrl を含めておらず、executeAction() にも転送していません。

export async function execute({
  command,
  position,
  selectionText,
  target,
  useSecondary = false,
  useClipboard = false,
  changeState,
  // pageUrl が欠落している
}: ExecuteCommandParams) {
  return executeAction({
    ...
    // pageUrl が渡されていない
    actions: actionsForBackground,
  })
}

このため、この経路（タブへのメッセージ送信が無効/失敗した場合のフォールバック実行）で実行されたコマンドは pageUrl が常に空文字になり、%pageUrl や srcUrl が本来の値にならないと思われます。この PR がまさに解決しようとしている問題が、このパスだけ再発している状態です。

なお packages/extension/src/background_script.test.ts では @/action/background を vi.mock しているため（14行目）、execute() に pageUrl が渡されていることは確認できても、実装側で握りつぶされていることまではテストで検出できていません。action/background.ts に対する単体テストも見当たりませんでした。

ExecuteCommandParams の分割代入に pageUrl を追加し、executeAction に渡すよう修正が必要そうです。

2. SidePanel.execute だけ pageUrl を使わず location.href を直接参照している

packages/extension/src/action/sidePanel.ts:9-13, 42 は、他の Tab/Window/Popup/BackgroundTab/PageAction/AiPrompt と異なり pageUrl パラメータを受け取らず、location.href を直に使っています。

現状は SIDE_PANEL が action/index.ts（content script 側のマップ）からしか呼ばれないため実害はなさそうですが、他のアクションと実装パターンが揃っていないのは今後のバグの温床になりえます（1点目のような、背景コンテキストから直接呼ばれるケースが将来追加された場合に同様の問題が起きえます）。一貫性のため pageUrl を受け取る形に揃えることをお勧めします。

3. Hub 側が q クエリパラメータを読んでいない可能性

新規デフォルトコマンド「Search Commands on Hub」(packages/extension/src/services/option/defaultSettings.ts:602-620) は https://selection-command.com/{locale}?q=%pageUrl を開きますが、packages/hub/src/app/[lang]/page.tsx は searchParams を受け取っておらず、リポジトリ内を検索した限り hub パッケージ側で q パラメータを読んでいる箇所が見つかりませんでした。

意図的に hub 側の対応を別 PR に分けているのであれば問題ありませんが、そうでない場合はこのコマンドをクリックしても Hub のトップページが開くだけで検索は実行されない状態だと思います。ご確認いただけると安心です。

4. ShortcutList.tsx の willUseClipboard / referencesSelection がほぼ重複

packages/extension/src/components/option/editor/ShortcutList.tsx:41-58（willUseClipboard）と :89-106（referencesSelection）は、プレースホルダー判定用のヘルパー（hasSelectionOrClipboardPlaceholder / hasSelectionPlaceholder）が違うだけで分岐構造が完全に同一です。判定関数を引数化して1つの共通ヘルパーにまとめられそうです。

5. createNameRender のインデントが崩れている（フォーマット）

packages/extension/src/components/option/editor/ShortcutList.tsx:108-124 の三項演算子内の JSX ブロックのインデントが、周辺のコードや元のスタイルと比べて1段浅くなっています（diff で確認できます）。Prettier/ESLint 実行後の差分ではない可能性があるので、yarn lint を通してフォーマットを揃えておくと良さそうです。

6. （軽微）toUrl の %pageUrl 置換順序

packages/extension/src/lib/utils.ts:84-88

let url = searchUrl?.replace("%s", textEncoded)
url = url?.replace("%pageUrl", encodeURIComponent(param.pageUrl ?? ""))

%s の置換を先に行っているため、選択テキスト自体に文字列 "%pageUrl" が含まれていた場合、意図せず pageUrl の値で置換されてしまう可能性があります。かなりレアなケースですが、念のため共有します。

テストについて

toUrl（TU-14〜17）、defaultSettings（DS-24）、settings migration（ST-34, ST-34-a）へのテスト追加は良い対応だと思います。一方で上記1点目のとおり、action/background.ts 単体・および pageUrl がバックグラウンド直接実行パスまで実際に伝播することを検証するテストは無いようなので、追加を検討いただけると良さそうです。

---

以上、よろしくお願いいたします 🙏

[claude]

Test comment from review tool - checking gh pr comment with stdin

[claude]

レビュー概要

このPRは「Hub上でコマンドを検索する」デフォルトコマンドを追加し、そのための %pageUrl プレースホルダーをURLテンプレートに導入するものです。あわせて location.href への直接依存を排除して pageUrl を ExecuteCommandParams 経由で明示的に受け渡す設計に変更し、ShortcutList.tsx の「テキスト選択必須」判定ロジックをより正確なもの（openModeごとの分岐、プレースホルダー参照有無の判定）にリファクタリングしています。設定マイグレーション（migrate1_1_0）・toUrl() の %pageUrl 置換・デフォルトコマンド生成には、それぞれ対応するテスト（ST-34系、TU-14〜17、DS-24）が追加されており、全体として丁寧に作り込まれています。

以下、気になった点を挙げます。

潜在的な不具合

1. Api（OPEN_MODE.API）コマンドで %pageUrl が空文字に置換されてしまう

   • packages/extension/src/action/api.ts:12-16
   • packages/extension/src/lib/utils.ts:84-89

   Popup/Tab/Window/BackgroundTab/SidePanel/PageAction/AiPrompt の各アクションモジュールは今回すべて pageUrl を受け取り toUrl() に渡すよう修正されていますが、Api.execute（api.ts:11）だけは ExecuteCommandParams から pageUrl を分割代入しておらず、searchUrl を組み立てる toUrl() 呼び出し（api.ts:44付近）にも pageUrl を渡していません。

   toUrl()（utils.ts:88）は param.pageUrl が undefined でも無条件に %pageUrl を空文字へ置換するため、API種別のコマンドで searchUrl に %pageUrl を使った場合、静かに空文字へ置き換わってしまいます（エラーにはならず気づきにくい）。Api.execute 内では別途 chrome.tabs.query で取得した pageUrl を IPC の pageUrl フィールド（${pageUrl} テンプレート用、helper.ts:274-287）としては使っていますが、toUrl() には渡していません。一貫性の観点から、Api.execute にも pageUrl を渡す（既存の chrome.tabs.query の結果を使う）ことを検討してください。

テストカバレッジ

2. ShortcutList.tsx の新規ロジックに対する単体テストが見当たらない

   • packages/extension/src/components/option/editor/ShortcutList.tsx:33-72

   referencesPlaceholder / willUseClipboard / isTextSelectionOnly / referencesSelection という、openMode とコマンド種別（search / pageAction / aiPrompt / その他）で分岐する非自明なロジックが追加・拡張されていますが、ShortcutList.test.tsx に相当するテストファイルが存在しないようです。特に isPageActionType 分岐（steps の paramToStr を走査する部分、ShortcutList.tsx:410-413）は分岐条件が複雑なので、代表的な入力パターンに対する単体テストを追加すると安心です。

3. e2e の URL 疎通テストが %pageUrl を未置換のまま送信する

   • packages/extension/e2e/url-status.spec.ts:17

   url-status.spec.ts は searchUrl.replace("%s", "test") のみを行っており、今回追加された %pageUrl は置換されないため、新規の「Search Commands on Hub」系URLは ?q=%pageUrl という未置換の文字列のままリクエストされます（CIではスキップされるためブロッカーではありませんが、ローカル実行時の疎通確認としては本来の用途と少しずれます）。余裕があれば %pageUrl もダミー値に置換するよう更新するとよさそうです。

軽微な指摘（Nits）

• packages/extension/src/services/option/defaultSettings.ts:651-702 : 14言語分の createCommandSearchCommand(...) 呼び出しがほぼ同型の記述で並んでいます。ファイル内の他の CMD_* 定義群と同じ手書きスタイルに合わせているのは理解できますが、locale と title のペアを配列にして map() する形にすればさらに見通しがよくなりそうです（必須の変更ではありません）。
• packages/extension/src/lib/utils.ts:87-89 : String.prototype.replace は最初の一致のみを置換するため、searchUrl 内に %pageUrl が複数回出現するケースは想定されていません（既存の %s 置換も同様の制約を引き継いでいるだけなので、既存仕様との整合は取れています）。将来的に複数箇所への埋め込みが必要になった場合は replaceAll の採用を検討してください。

良い点

• pageUrl の受け渡しが ExecuteCommandParams / UrlParam の型定義（types/index.ts:241, :308）まで含めて一貫して通されており、action/index.ts:52-53 のコメントで「このモジュールはcontent script文脈でのみ実行されるため location.href が正しい」という設計判断が明記されている点は好印象です。
• background_script.ts:588（キーボードショートカット経由の実行パス）では content script を経由しないため tab?.url を使うなど、実行コンテキストごとの pageUrl 取得方法が正しく使い分けられています。
• マイグレーション（migrate1_1_0, settings.ts:389-404）は既存の migrate0_10_0 と同じパターン（IDによる重複チェック→Storage.setCommands）を踏襲しており、コードベースの規約に沿っています。
• toUrl() の %pageUrl 置換、マイグレーションの重複防止、全ロケールでのデフォルトコマンド存在確認について、それぞれ的確なテストケース（TU-14〜17, ST-34, ST-34-a, DS-24）が追加されています。

以上、大きな設計上の問題は見当たりませんでした。上記の Api コマンドの %pageUrl 未対応の点だけ確認いただければと思います。