Skip to content

Hub外部呼び出しでCommand編集ダイアログ起動と保存時Hub同期を追加#401

Merged
ujiro99 merged 20 commits into
mainfrom
copilot/add-command-edit-dialog-launch
May 12, 2026
Merged

Hub外部呼び出しでCommand編集ダイアログ起動と保存時Hub同期を追加#401
ujiro99 merged 20 commits into
mainfrom
copilot/add-command-edit-dialog-launch

Conversation

Copilot AI commented May 10, 2026

Copy link
Copy Markdown
Contributor
  • 新規コメントの要件を確認(未対応3件: listener cleanup / ack timeout / 連携テスト)
  • 変更前の lint/build/test を実行して現状確認
  • EditCommand の onConnectExternal listener にタイムアウト/失敗時クリーンアップを追加
  • editCommandToHub に ack タイムアウトと重複 listener 防止を追加
  • hub background テストへ ack/cleanup/Storage.updateCommands 形状検証を追加
  • 影響範囲テスト・build 実行
  • parallel_validation 実施
  • 新規コメント3件に返信

Copilot AI changed the title [WIP] Add command edit dialog launch from Selection Command Hub Hub外部呼び出しでCommand編集ダイアログ起動と保存時Hub同期を追加 May 10, 2026
Copilot AI requested a review from ujiro99 May 10, 2026 13:41
@codecov

codecov Bot commented May 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 45.10490% with 157 lines in your changes missing coverage. Please review.
✅ Project coverage is 27.38%. Comparing base (45e36b9) to head (81d0c70).
⚠️ Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
...nsion/src/components/option/editor/CommandList.tsx 0.00% 63 Missing ⚠️
...src/components/option/editor/CommandEditDialog.tsx 0.00% 42 Missing ⚠️
packages/extension/src/services/hub/background.ts 88.32% 16 Missing ⚠️
packages/extension/src/services/hubShare.ts 14.28% 12 Missing ⚠️
...es/extension/src/components/option/ShareButton.tsx 0.00% 9 Missing ⚠️
packages/extension/src/lib/utils.ts 30.00% 7 Missing ⚠️
...c/components/option/editor/CommandTreeRenderer.tsx 0.00% 6 Missing ⚠️
.../src/components/option/editor/FolderEditDialog.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #401      +/-   ##
==========================================
+ Coverage   27.19%   27.38%   +0.18%     
==========================================
  Files         316      316              
  Lines       31899    32140     +241     
  Branches     1732     1764      +32     
==========================================
+ Hits         8676     8802     +126     
- Misses      23223    23338     +115     

☔ 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.

@ujiro99 ujiro99 marked this pull request as ready for review May 11, 2026 08:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

概要

Hub から拡張機能へ「既存コマンドの編集ダイアログ起動」を要求できるようにし、保存時に Hub へ更新を同期する導線を追加するPRです。Options 画面のクエリ解釈による自動ダイアログ起動と、Hub とのポート通信(edit-command / ack)を新設しています。

懸念点

  • Hub 編集同期の実装で、SubmitCommandInputlocale / targetUrl付き)をそのままローカルコマンドとして永続化してしまう不具合があります。
  • hub-editonConnectExternal 受付で接続元検証がなく、想定外の外部タブ/オリジンから _hubEditPort を差し替え可能です(セキュリティ/誤動作リスク)。
  • listener/レスポンスが ack 前提で解放されず、接続不成立・ack未達時にリーク/ハングし得ます(タイムアウトや失敗時クリーンアップ不足)。
  • Share 操作でコマンドIDを UUIDv7 に強制置換するため、ショートカット等の commandId 参照が壊れる可能性が高いです(永続IDの変更)。

改善提案

  • Hub送信用の付加情報(locale/targetUrl)は保存前に除去し、保存対象は純粋な SelectionCommand に限定してください。
  • hub-edit 接続は port.sender.tab.id === _hubTabId などで厳密に検証し、タイムアウト/エラー時に listener を確実に解除してください。
  • ack待ちの response についてタイムアウト導入、または即時レスポンス+非同期でack処理など、未応答時にチャネルが残らない設計にしてください。
  • Shareボタンで command.id を書き換えない(UUIDv4も許容する/Hub用に別IDを持つ)か、どうしても変えるなら shortcuts 等の参照も同時更新する仕組みを追加してください。

Changes:

  • Hub→Extension の外部メッセージに EditCommand を追加し、Options ページをクエリ付きで起動
  • Options 側でクエリを解釈して該当コマンドの編集ダイアログを自動オープンし、保存時に Hub へ同期
  • ID生成を crypto.randomUUID() から uuid ベースの generateId()(UUIDv7)へ置換

Reviewed changes

Copilot reviewed 17 out of 19 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
packages/extension/src/services/hub/background.ts Hub外部メッセージに EditCommand を追加し、Hub同期用のポート通信とローカル更新処理を追加
packages/extension/src/components/option/editor/CommandList.tsx クエリ解析による自動編集起動と、保存時の Hub 同期呼び出しを追加
packages/extension/src/services/hubShare.ts Hub同期用に editCommandToHub を追加
packages/extension/src/components/option/ShareButton.tsx 共有時にUUIDv7でないIDを生成し直すロジックを追加
packages/extension/src/lib/utils.ts generateId(UUIDv7)と isUUIDv7 を追加
packages/extension/src/services/settings/settings.ts / settings.test.ts マイグレーションとテストを generateId に追従
packages/extension/src/services/ipc.ts / src/background_script.ts BgCommand.editCommandToHub を追加して background に配線
packages/extension/package.json / yarn.lock uuid を ^14.0.0 へ更新
packages/extension/src/services/hub/background.test.ts EditCommand の外部メッセージケースを追加
.gitignore / packages/extension/.env.development .env.development をリポジトリ管理から外す変更
.serena/project.yml / .claude/settings.local.json 開発ツール設定の更新

Comment thread packages/extension/src/services/hub/background.ts Outdated
Comment thread packages/extension/src/services/hub/background.ts Outdated
Comment thread packages/extension/src/services/hub/background.ts
Comment thread packages/extension/src/services/hub/background.ts
Comment thread packages/extension/src/services/hub/background.ts Outdated
Comment on lines +27 to 36

let commandToShare = command
if (!isUUIDv7(command.id)) {
const newId = generateId()
commandToShare = { ...command, id: newId }
onCommandIdChange?.(newId)
}

const ok = shareCommandToHub(commandToShare)
setStatus(ok ? "sent" : "error")
Comment on lines +299 to +302
const idx = commandArray.fields.findIndex((f) => f.id === commandId)
if (idx >= 0) {
commandArray.update(idx, { ...commandArray.fields[idx], id: newId })
}
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@claude

claude Bot commented May 11, 2026

Copy link
Copy Markdown

コードレビュー

PR の概要を把握しました。Hub から拡張機能の編集ダイアログを起動し、保存後に Hub へ同期する機能追加です。全体的な設計は明確ですが、いくつかの重要な問題点があります。


🔴 重要な問題

1. グローバルミュータブル状態の競合(並行 EditCommand リクエスト)

packages/extension/src/services/hub/background.ts:125-127

let _hubEditPort: chrome.runtime.Port | undefined
let _hubTabId: number | undefined
let _editTabId: number | undefined

Hub から EditCommand が短時間に連続して届いた場合、2 番目のリクエストが1番目のセッション情報を上書きします。

  • 1番目の _hubEditPort が失われ、editCommandToHub が "No hub-edit port available" エラーになる
  • _editTabId_hubTabId が混在し、誤ったタブが閉じられる/フォーカスされる

提案: セッション ID(またはリクエスト ID)をキーにした Map で管理する、もしくは既存のフローが完了するまで後続リクエストを拒否する(mutex 的な処理)。

2. onHubEditConnect リスナーのリーク(タイムアウトなし)

packages/extension/src/services/hub/background.ts:218-228

const onHubEditConnect = (port: chrome.runtime.Port) => { ... }
chrome.runtime.onConnectExternal.addListener(onHubEditConnect)

Hub が port.name === "hub-edit" で接続しない場合(ネットワークエラー、ユーザーが Hub タブを閉じた等)、onHubEditConnect リスナーが永続的に残ります。また、次の EditCommand で別の onHubEditConnect が積み重なります。

提案: setTimeout でタイムアウトを設け、一定時間内に接続がなければリスナーを解除する。

const timeout = setTimeout(() => {
  chrome.runtime.onConnectExternal.removeListener(onHubEditConnect)
}, 30_000) // 30秒
const onHubEditConnect = (port: chrome.runtime.Port) => {
  if (port.name !== "hub-edit") return
  clearTimeout(timeout)
  chrome.runtime.onConnectExternal.removeListener(onHubEditConnect)
  // ...
}

🟡 中程度の問題

3. editCommandToHub のレスポンス形式の不統一

packages/extension/src/services/hub/background.ts:136-138 / 169

  • エラー時: response(false) (プリミティブ)
  • 成功時: response(true) (プリミティブ)
  • 一方 AddCommand/DeleteCommand: sendResponse({ result: boolean }) (オブジェクト)

Ipc.send の呼び出し側は void で捨てているため実害は少ないですが、一貫性のため response({ result: false }) / response({ result: true }) に統一することを推奨します。

4. onMsg リスナーの重複登録リスク

packages/extension/src/services/hub/background.ts:143-170

editCommandToHubedit-command-ack を受け取る前に再度呼ばれた場合、port.onMessageonMsg リスナーが複数登録されます。その場合、同一 response が複数回呼ばれる可能性があります。

提案: editCommandToHub 関数の先頭で既存リスナーの有無をチェック、または onMsg リスナーのフラグ管理。

5. uuid のメジャーバージョン大幅アップ(v11 → v14)

packages/extension/package.json:62 / yarn.lock

uuid@^11.1.0^14.0.0 の変更により、yarn.lockuuid@11.1.0uuid@14.0.02バージョンが共存します。uuid v11 はすでに v7() をサポートしているため、このメジャーバージョンアップは不要なバンドルサイズ増加をもたらします。

提案: ^11.1.0 のまま v7 をインポートするか、プロジェクト全体で uuid を v14 に統一する(他パッケージへの影響を確認の上)。


🟢 軽微な指摘

6. useEffect の依存配列コメントの脆弱性

packages/extension/src/components/option/editor/CommandList.tsx:251-252

// location.search is fixed at page load (set via chrome.tabs.create), so it is
// safe to read it without adding it to the dependency array.

location.search を依存配列に含めない理由をコメントで説明するよりも、useRef で初期値を保持する方がより明示的です。ESLint の exhaustive-deps ルールも自動修正しようとする可能性があります。

const initialSearchRef = useRef(location.search)
// useEffect 内で initialSearchRef.current を使用

7. editParamHandledRef がコマンド未発見時でもフラグを立てる

packages/extension/src/components/option/editor/CommandList.tsx:270-273

const command = commandArray.fields.find((f) => f.id === editCommandId)
editParamHandledRef.current = true  // ← コマンド未発見でも true に
if (!command) {
  console.warn(...)

commandArray.fields.length === 0 のガードはあるが、フィールドが読み込まれた後にコマンドが見つからない場合は warn を出してフラグを立てます。これは意図的であれば問題ないですが、コマンドが非同期で後から追加される場合には再試行できません。ユーザーへのエラー通知(toast など)があると UX が向上します。

8. .env.development の削除

packages/extension/.env.development.gitignore に追加された上で削除されています。これは意図的な変更と理解しますが、VITE_NEW_HUB_URL のローカル開発設定を別途 .env.development.local(既に .gitignore 済み)へ移行するよう開発者への案内があると良いでしょう。


✅ 良い点

  • crypto.randomUUID()generateId() (UUID v7) に統一したことで、ソート可能な ID が得られ、将来の Hub 統合に有利です
  • テストケース EC-01 / EC-02 が正常系・型不正系の両方をカバーしており、テスト更新は適切です
  • ShareButton での isUUIDv7 チェックにより、既存の非 UUID v7 コマンドを Hub 共有前に正規化する処理は堅実なアプローチです
  • syncHub=1 のフラグによる条件付き同期で、通常の編集フローへの影響を最小限に抑えています

以上、特に グローバル状態の競合リスナーリーク は本番で再現しうる問題のため、対応を検討いただけますと幸いです。

@claude

claude Bot commented May 11, 2026

Copy link
Copy Markdown

コードレビュー

概要

このPRは、Hub外部から拡張機能の「コマンド編集ダイアログ」を起動できるようにし、保存時にHubへ同期する機能を追加します。ID生成を crypto.randomUUID() から UUIDv7 (uuid ライブラリ) に統一した変更も含まれています。


重要なバグ・問題点

1. ストレージ更新成功前に response(true) を送信してしまう

packages/extension/src/services/hub/background.ts (editCommandToHub, ~L186付近)

Storage.updateCommands([commandToStore])
  .catch((err) => {
    console.error("[editCommandToHub] Failed to update local command:", err)
  })
  .finally(() => {
    if (editTabId != null) chrome.tabs.remove(editTabId)
    if (hubTabId != null) chrome.tabs.update(hubTabId, { active: true })
  })

response(true)  // ← ストレージ更新完了前に Hub へ "成功" を通知してしまう

response(true)Storage.updateCommands の完了を待たずに呼ばれています。保存に失敗してもHubには成功が通知され、データ不整合が発生します。

修正案:

Storage.updateCommands([commandToStore])
  .then(() => response(true))
  .catch((err) => {
    console.error("[editCommandToHub] Failed to update local command:", err)
    response(false)
  })
  .finally(() => {
    if (editTabId != null) chrome.tabs.remove(editTabId)
    if (hubTabId != null) chrome.tabs.update(hubTabId, { active: true })
  })

2. タイムアウト後の chrome.tabs.create コールバックによるゾンビ状態

packages/extension/src/services/hub/background.ts (onMessageExternal EditCommandハンドラ, ~L290付近)

connectTimeout が発火してステート (_hubTabId など) がクリアされた後も、chrome.tabs.create のコールバックは実行されます。コールバック内で _editTabId = tab.id がセットされ、無効なセッションのタブIDが残る可能性があります。

修正案: セッションIDや無効化フラグを使ってコールバックを無効化する。例:

let sessionInvalidated = false
connectTimeout = setTimeout(() => {
  sessionInvalidated = true
  cleanupHubEditConnectListener()
  // ... state cleanup
}, EDIT_CONNECT_TIMEOUT_MS)

chrome.tabs.create({ url: ... }, (tab) => {
  if (sessionInvalidated) {
    if (tab?.id) chrome.tabs.remove(tab.id)
    return
  }
  // ...
})

3. ShareButton でのコマンドID変更が副作用として実行される

packages/extension/src/components/option/ShareButton.tsx (L30-35付近)

if (!isUUIDv7(command.id)) {
  const newId = generateId()
  commandToShare = { ...command, id: newId }
  onCommandIdChange?.(newId)  // ← IDを変更(未保存のフォーム状態も変更される)
}

「共有」ボタンのクリックという操作の中で、コマンドIDを UUIDv7 に置き換えています。これはユーザーが意図しないデータ変更であり、以下のリスクがあります:

  • まだフォームが保存されていない状態でも commandArray.update() が呼ばれる
  • ユーザーに変更通知がなく、誤って古いIDが参照されているショートカットの更新漏れリスクがある

提案: 共有前にIDが変わる旨をユーザーへ明示するか、Hub側でUUIDv4も受け入れる設計を検討してください。


コード品質・設計上の懸念

4. モジュールレベルの可変状態の脆弱性

packages/extension/src/services/hub/background.ts (L125-131付近)

let _hubEditPort: chrome.runtime.Port | undefined
let _hubTabId: number | undefined
let _editTabId: number | undefined
let _editAckTimeout: ReturnType<typeof setTimeout> | undefined
let _editAckListener: ((msg: unknown) => void) | undefined
let _editPendingResponse: ((res: unknown) => void) | undefined

モジュールレベルの状態はService Worker/background script のライフサイクルで失われます。Chromeがservice workerを終了させると _hubEditPort などがリセットされ、進行中のセッションが孤立します。クリーンアップの仕組み(disconnect handler)は適切に設計されていますが、SW終了による非同期リセットのリスクは残ります。


5. commandToStore の型安全性

packages/extension/src/services/hub/background.ts (editCommandToHub ~L176付近)

const {
  locale: _locale,
  targetUrl: _targetUrl,
  ...commandToStore
} = param
Storage.updateCommands([commandToStore])

SubmitCommandInput から localetargetUrl を除去していますが、commandToStoreSelectionCommand 型として型チェックされていません。SubmitCommandInputSelectionCommand にない追加フィールドがあれば、意図しないデータが保存される可能性があります。明示的な型アサーションまたは型ガードを追加することを推奨します。


6. useEffect の依存配列に commandArray.fields が含まれている

packages/extension/src/components/option/editor/CommandList.tsx (L257-294付近)

useEffect(() => {
  if (editParamHandledRef.current) return
  // ...
}, [commandArray.fields])  // ← フィールドが変わるたびに実行

commandArray.fields は毎レンダーで参照が変わるため、useEffectが意図より多く実行される可能性があります。editParamHandledRef で一度処理済みにする設計は正しいですが、初期ロード前に早期リターンする if (commandArray.fields.length === 0) return の後に editParamHandledRef.current をセットしていないため、フィールドが0→N件に変わるたびにeffectが走ります(意図通りではありますが、パフォーマンス的に注意)。


テストカバレッジ

Codecovレポートによると:

ファイル パッチカバレッジ
background.ts 28.98% (49行未カバー)
CommandList.tsx 0% (44行未カバー)
hubShare.ts 14.28% (12行未カバー)
ShareButton.tsx 0% (9行未カバー)

特に CommandList.tsxeditCommand URLパラメータ処理と Hub同期ロジックはテストがなく、バグ混入リスクが高い部分です。UIコンポーネントのテストは難しいですが、useEffect ロジックを純粋な関数に抽出してユニットテストできる設計を検討してください。


マイナーな指摘

7. uuid のメジャーバージョン大幅アップグレード

packages/extension/package.json, packages/shared/package.json

v11.1.0 → v14.0.0 と2メジャーバージョンのアップグレードです。v7 API自体は維持されていますが、破壊的変更がないか変更履歴の確認を推奨します。

8. handleUpdateCommandId でショートカット更新漏れの可能性

packages/extension/src/components/option/editor/CommandList.tsx (L320-332付近)

getValues("shortcuts.shortcuts") でショートカットのIDを更新していますが、コマンドIDが他の場所でも参照されている場合(例: ネストしたフォルダの設定など)に漏れが生じる可能性があります。


総評

EditCommand機能の基本設計(外部メッセージ → タブ起動 → ポート接続 → ack確認)は適切です。タイムアウト・重複リクエスト防止・クリーンアップの仕組みも実装されており、既存のCopilotレビューで指摘されていた懸念事項の多くは対処済みです。

ただし、「ストレージ更新成功前にresponse(true)を送信する」バグ(問題点1)は確実に修正が必要です。その他の問題点についても対応を検討いただければ幸いです。


レビュー by Claude (claude-sonnet-4-6)

@claude

claude Bot commented May 11, 2026

Copy link
Copy Markdown

コードレビュー

概要

このPRはHubから拡張機能への EditCommand 外部メッセージを追加し、オプションページでのコマンド編集ダイアログの自動起動と、保存時のHub同期機能を実装しています。また、ID生成を crypto.randomUUID() から UUIDv7 ベースの generateId() に統一しています。

全体的な設計方針は明確で、セキュリティ検証(originチェック・tab IDチェック)やタイムアウト処理の追加など、改善されている点は評価できます。ただし、いくつかの問題点を指摘します。


🐛 バグ

1. sessionInvalidated 時に sendResponse が呼ばれない

ファイル: packages/extension/src/services/hub/background.ts(追加行の chrome.tabs.create コールバック内)

(tab) => {
  if (sessionInvalidated) {
    if (tab?.id != null) chrome.tabs.remove(tab.id)
    return  // ← sendResponse が呼ばれていない!
  }

return true でメッセージチャンネルを非同期で保持しているにも関わらず、タイムアウト発火後にタブ作成コールバックが届いた場合に sendResponse が呼ばれません。Hub 側のリクエストが応答なしでハングします。

修正案:

if (sessionInvalidated) {
  if (tab?.id != null) chrome.tabs.remove(tab.id)
  sendResponse({ result: false, error: "Session invalidated" })  // 追加
  return
}

2. editCommandToHub(hubShare.ts)は常に true を返す

ファイル: packages/extension/src/services/hubShare.ts:83-90

export function editCommandToHub(command: SelectionCommand): boolean {
  const input = toSubmitCommandInput(command)
  if (!input) { ... return false }

  void Ipc.send(BgCommand.editCommandToHub, input).catch((err) => {
    console.error("[HubShare] Failed to edit command:", err)
  })
  return true  // ← IPC失敗でも true を返す
}

fire-and-forget パターンにより、IPC 通信失敗時もエラーは console.error のみで呼び出し元に伝播されません。CommandList.tsxonSave ハンドラは結果を使用していませんが、将来的な誤用リスクがあります。


⚠️ 設計上の懸念

3. モジュールスコープのミュータブル状態(6変数)

ファイル: packages/extension/src/services/hub/background.ts:123-128

let _hubEditPort: chrome.runtime.Port | undefined
let _hubTabId: number | undefined
let _editTabId: number | undefined
let _editAckTimeout: ReturnType<typeof setTimeout> | undefined
let _editAckListener: ((msg: unknown) => void) | undefined
let _editPendingResponse: ((res: unknown) => void) | undefined

モジュールレベルの変数でセッション状態を管理しているため、テストでの隔離が難しく、複数のコードパスが同じ状態を変更します。現在は _hubTabId !== undefined チェックによる同時実行防止があり、既存の shareCommandToHub と同じパターンを踏襲していますが、長期的にはセッション状態をオブジェクトとして管理するリファクタリングを検討してください。


4. clearEditCommandAckWait でポート参照を引数に取る設計

ファイル: packages/extension/src/services/hub/background.ts:133-145

const clearEditCommandAckWait = (port: chrome.runtime.Port) => { ... }
const settleEditCommandAck = (port: chrome.runtime.Port, result: boolean) => { ... }

_hubEditPort はモジュール変数として保持しているのに、これらの関数がポートを引数で受け取るのは設計上の不一致です。_hubEditPort が既に undefined にセットされた後に呼ばれるケースを考慮しているのだと推測しますが、関数内部で _hubEditPort を参照することもできます。


📊 テストカバレッジ

Codecov のレポートによると、新規コードのパッチカバレッジは 17.61% で、主に以下がカバーされていません:

ファイル カバレッジ 未カバー行
background.ts 28.98% 49行
CommandList.tsx 0.00% 44行
hubShare.ts 14.28% 12行
ShareButton.tsx 0.00% 9行
CommandTreeRenderer.tsx 0.00% 6行

CommandList.tsx の URL クエリパラメータ解析ロジック(useEffect 内のダイアログ自動起動フロー)はコア機能であり、テストがゼロなのが気になります。


✅ 良い点

  • セキュリティ検証: onHubEditConnectport.sender?.tab?.id !== _hubTabIdport.sender?.origin !== hubOrigin の両方をチェックしている(EC-03, EC-04 のテストも追加済み)
  • locale/targetUrl の除外: ack 受信時に Hub 用フィールドをストレージ保存から除外している(...commandToStore による分割代入)
  • ショートカット参照の更新: handleUpdateCommandId でコマンドIDが変わった際にショートカットの commandId も更新している
  • 重複セッション防止: _hubTabId !== undefined_editPendingResponse の二重チェックで並行リクエストを拒否
  • テスト改善: crypto.randomUUID のモック方法を vi.spyOn(utils, "generateId") に整理(settings.test.ts

💡 その他の提案

  • packages/extension/src/services/hub/background.ts のタイムアウト定数 EDIT_CONNECT_TIMEOUT_MSEDIT_COMMAND_ACK_TIMEOUT_MS が同じ値(10秒)ですが、2つの異なるフェーズのタイムアウトとして意味が異なります。コメントでそれぞれの意図を明示すると可読性が上がります。
  • uuid^11.1.0^14.0.0 のメジャーバージョン更新が含まれています。v7 API の互換性は保たれていますが、yarn.lock の差分が大きい点は確認してください。

@claude

claude Bot commented May 12, 2026

Copy link
Copy Markdown

コードレビュー

概要

このPRは、Hub(外部サービス)から拡張機能へ「既存コマンドの編集ダイアログ起動」を要求できるようにし、保存時にHubへ更新を同期する機能を追加するものです。

全体的な設計方針は理解できますが、以下にいくつかの問題点を指摘します。


重要な問題

1. initialCommand propの直接ミューテーション

ファイル: packages/extension/src/components/option/editor/CommandEditDialog.tsx (変更後 約408行付近)

if (isEmpty(initialCommand.parentFolderId)) {
  initialCommand.parentFolderId = ROOT_FOLDER  // propを直接変更している
}

Reactの原則として、propsは読み取り専用です。呼び出し元のオブジェクトが変更されてしまい、予期しない副作用が生じる可能性があります。

修正案:

const commandForForm = {
  ...initialCommand,
  parentFolderId: isEmpty(initialCommand.parentFolderId)
    ? ROOT_FOLDER
    : initialCommand.parentFolderId,
}
reset((commandForForm as any) ?? InitialValues)

2. サービスワーカー停止による _editSession の消失リスク

ファイル: packages/extension/src/services/hub/background.ts (変更後 約130〜135行付近)

let _editSession: EditSession | undefined

Manifest V3の拡張機能では、バックグラウンドスクリプト(Service Worker)がアイドル時に停止されることがあります。その場合、_editSession のモジュールレベル変数が消失し、編集セッションが宙ぶらりになります。Options タブが開かれても、バックグラウンドが再起動した際にはHubとの通信ポートが切断されている状態になります。少なくともServiceWorkerの停止・再起動シナリオでの動作を検討・文書化することを推奨します。


3. ShareButton でのID変更が永続化されない可能性

ファイル: packages/extension/src/components/option/ShareButton.tsx (変更後 約24〜30行付近)

if (!isUUIDv7(command.id)) {
  const newId = generateId()
  commandToShare = { ...command, id: newId }
  onCommandIdChange?.(newId)  // react-hook-formのフィールド更新のみ
}
const ok = shareCommandToHub(commandToShare)

onCommandIdChange はフォームの状態(react-hook-form)を更新しますが、ユーザーがその後フォームを保存しなかった場合、新しいIDはストレージに永続化されません。その結果:

  • Hubには新しいIDで共有される
  • ローカルストレージには古いIDのコマンドが残る
  • Hub側と拡張機能側でIDの不一致が発生する

推奨: 共有と同時に Storage.updateCommands でIDを保存するか、IDが変わった場合はフォームのサブミットを自動でトリガーする仕組みを検討してください。


改善提案

4. useEffect の依存関係と早期リターンのロジック

ファイル: packages/extension/src/components/option/editor/CommandList.tsx (変更後 約254〜294行付近)

useEffect(() => {
  if (editParamHandledRef.current) return
  if (commandArray.fields.length === 0) return  // コマンドが空の場合は待機
  editParamHandledRef.current = true
}, [commandArray.fields])

コマンドが何らかのエラーで0件のまま読み込まれない場合(ストレージ障害等)、ダイアログが永遠に開かないシナリオが考えられます。一定時間後にタイムアウトして editParamHandledRef.current = true を設定するフォールバックを検討してください。


5. テスト用エクスポートが本番コードに含まれる

ファイル: packages/extension/src/services/hub/background.ts

export function resetEditSession(): void {
  _editSession = undefined
}

テスト専用の関数が本番コードにエクスポートされています。@internal コメントで明示するか、vitestのモジュールモック機能(vi.mock)を使う方法を検討してください。


6. uuid メジャーバージョンアップの確認

ファイル: packages/extension/package.json, packages/shared/package.json

uuid^11.1.0^14.0.0 へのメジャーバージョンアップです。v7() のエクスポートAPIが互換性を保っているか、またNode.jsバージョン要件の変更がないか確認することを推奨します。


良い点

  • セキュリティ面: onHubEditConnectport.sender?.tab?.idport.sender?.origin を両方検証しており、意図しない外部タブからの接続を適切に拒否しています(background.ts 変更後 約271〜275行)
  • タイムアウト処理: 接続タイムアウト(EDIT_CONNECT_TIMEOUT_MS)とAckタイムアウト(EDIT_COMMAND_ACK_TIMEOUT_MS)の両方が実装されており、リソースリークを防いでいます
  • テストカバレッジ: background.test.tsEditCommand / editCommandToHub のテストケースが豊富に追加されており(EC-01〜EC-07, EH-01〜EH-04)、エラー系のカバレッジも十分です
  • データの正規化: ack時に locale / targetUrl をストレージ保存前に除去しており、Hub送信専用フィールドがローカルに混入しない設計になっています(background.ts 変更後 約187〜192行)
  • URL正規化: 編集ダイアログオープン後に history.replaceState でURLからパラメータを削除しており、リロード時の二重起動を防いでいます
  • リファクタリング品質: isUpdate boolean から mode: "new" | "edit" | "hubEdit" への変更は意図が明確になり、コードの可読性が向上しています

テストカバレッジ

Codecovの報告では、新規追加コードのパッチカバレッジが 17.61% と低い状態です。特にカバーされていない主要ファイル:

  • CommandList.tsx: 44行未カバー(URLパラメータ解析・ダイアログ自動起動ロジック)
  • background.ts: 49行未カバー(エラーパス等)
  • hubShare.ts: 12行未カバー

CommandList.tsx のURLパラメータ解析ロジックはユニットテストが難しいかもしれませんが、少なくとも handleUpdateCommandId のショートカット更新ロジックは追加のテストがあると安心です。

@ujiro99 ujiro99 merged commit 095b863 into main May 12, 2026
2 of 4 checks passed
@ujiro99 ujiro99 deleted the copilot/add-command-edit-dialog-launch branch May 12, 2026 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

外部からのCommand編集ダイアログの呼びだし

3 participants