Conversation
ujiro99
changed the base branch from
main
to
copilot/add-instant-command-feature
ujiro99
changed the base branch from
copilot/add-instant-command-feature
to
main
PR レビュー: Fix/clipboard at sidepanel概要バグの根本原因と修正方針は正確です。 ✅ 良い点
|
There was a problem hiding this comment.
Pull request overview
ショートカットキー起動かつ SidePanel モード時に、選択テキストが無いケースでクリップボード内容がURL補間に使われず取得できていなかった問題の修正を目的としたPRです。
Changes:
- SidePanel 実行時に、必要に応じて
navigator.clipboard.readText()でクリップボードを直接取得してtoUrl(..., clipboardText)に渡すよう変更 - ショートカット処理で active tab 取得タイミングを後ろに移動
- 拡張のバージョンを 0.17.0 に更新
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/extension/src/services/chrome.ts | sidePanel の open 処理に try/catch を追加し、tabId 参照を簡略化 |
| packages/extension/src/background_script.ts | ショートカット処理で active tab 取得を後段へ移動 |
| packages/extension/src/action/sidePanel.ts | SidePanel 実行時にクリップボード取得→URL生成へ反映、失敗時トースト通知を追加 |
| packages/extension/package.json | extension パッケージのバージョン更新 |
| packages/extension/manifest.json | manifest のバージョン更新 |
| package.json | monorepo ルートのバージョン更新 |
| const tabId = await Ipc.getTabId() | ||
| await Ipc.sendTab<ShowToastParam>(tabId, TabCommand.showToast, { | ||
| title: t("clipboard_error_title"), | ||
| description: t("clipboard_error_description"), | ||
| action: t("clipboard_error_action"), | ||
| }) |
There was a problem hiding this comment.
- SidePanel コンテキストでは
BgCommand.getTabIdがsender.tab?.idを返す実装のため、Ipc.getTabId()がundefinedになり得ます（実際、background_script 側にも「Side panel pages have no tab.id」とコメントがあります）。 - その状態で
Ipc.sendTab(tabId, TabCommand.showToast, ...)を呼ぶとchrome.tabs.sendMessageにundefinedが渡り、トースト表示に失敗します。 - ここは tabId を前提にしない手段（例:
Ipc.callListener(TabCommand.showToast, ...)でローカルに発火、またはIpc.getActiveTabId()で取得した tabId に送る等）に切り替えてください。
| const tabId = await Ipc.getTabId() | |
| await Ipc.sendTab<ShowToastParam>(tabId, TabCommand.showToast, { | |
| title: t("clipboard_error_title"), | |
| description: t("clipboard_error_description"), | |
| action: t("clipboard_error_action"), | |
| }) | |
| // Side panel pages may not have an associated tab.id, so fall back to the active tab. | |
| const currentTabId = (await Ipc.getTabId()) ?? (await Ipc.getActiveTabId()) | |
| if (currentTabId != null) { | |
| await Ipc.sendTab<ShowToastParam>(currentTabId, TabCommand.showToast, { | |
| title: t("clipboard_error_title"), | |
| description: t("clipboard_error_description"), | |
| action: t("clipboard_error_action"), | |
| }) | |
| } else { | |
| console.warn("Unable to determine tabId for showing clipboard error toast.") | |
| } |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #339 +/- ##
==========================================
- Coverage 25.22% 25.21% -0.02%
==========================================
Files 324 324
Lines 31816 31836 +20
Branches 1557 1557
==========================================
+ Hits 8027 8028 +1
- Misses 23789 23808 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
コードレビューPR の目的（ショートカットキー + SidePanel モードでクリップボード内容が取得されない不具合の修正）は明確で、アプローチの方向性は正しいと思います。ただし、いくつか懸念点があります。 🔴 重大な問題1. クリップボードエラー時のトースト送信に対するエラーハンドリング不足ファイル: } catch (e) {
console.warn("Failed to read clipboard text:", e)
const tabId = await Ipc.getTabId() // undefined を返す可能性がある
await Ipc.sendTab<ShowToastParam>(tabId, TabCommand.showToast, { ... })
}SidePanel コンテキストでは さらに、このキャッチブロック内の 修正案: } catch (e) {
console.warn("Failed to read clipboard text:", e)
try {
const tabId = await Ipc.getTabId() ?? await Ipc.getActiveTabId()
if (tabId) {
await Ipc.sendTab<ShowToastParam>(tabId, TabCommand.showToast, {
title: t("clipboard_error_title"),
description: t("clipboard_error_description"),
action: t("clipboard_error_action"),
})
}
} catch (toastError) {
console.warn("Failed to send toast notification:", toastError)
}
}🟡 中程度の問題2.
|
| 区分 | 内容 |
|---|---|
| 🔴 重大 | Ipc.getTabId() が undefined を返した場合の未処理エラー（sidePanel.ts 27–33行） |
| 🟡 中程度 | throw e 追加後の呼び出し元エラーハンドリングの確認（chrome.ts 694–699行） |
| 🟡 中程度 | クリップボードエラー時の処理継続の是非（sidePanel.ts 35行以降） |
| 🟢 良好 | background_script.ts のリファクタリング、targetTabId 削除 |
| 🔵 要追加 | クリップボード関連のユニットテスト |
特に Ipc.getTabId() の undefined ケースは、修正しようとしている問題（SidePanel 環境での動作）と直接関係するため、早急な対応をお勧めします。
PR #339 コードレビューPRの概要ショートカットキーからコマンドを起動し、 潜在的なバグ・問題点🔴 [重要]
|
ショートカットキーからの起動時でかつSidePanelモードの場合に、クリップボードの内容が取得されていない不具合を修正する