Skip to content

Feat/refine html attach#427

Merged
ujiro99 merged 4 commits into
mainfrom
feat/refine-html-attach
Jul 8, 2026
Merged

Feat/refine html attach#427
ujiro99 merged 4 commits into
mainfrom
feat/refine-html-attach

Conversation

@ujiro99

@ujiro99 ujiro99 commented Jul 7, 2026

Copy link
Copy Markdown
Owner

No description provided.

ujiro99 and others added 2 commits July 7, 2026 12:28
The extension's own content script root and Command Hub root elements
were being included when capturing document.documentElement.outerHTML
for the {{PageHtml}} placeholder, polluting the HTML sent to AI
services. Add getPageHtml() to strip both containers before use.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.48148% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 29.93%. Comparing base (de2762b) to head (9f7e2c1).

Files with missing lines Patch % Lines
...extension/src/components/pageAction/InputPopup.tsx 79.48% 8 Missing ⚠️
packages/extension/src/command_hub.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #427      +/-   ##
==========================================
+ Coverage   29.91%   29.93%   +0.02%     
==========================================
  Files         322      322              
  Lines       33270    33274       +4     
  Branches     1942     1943       +1     
==========================================
+ Hits         9954     9962       +8     
+ Misses      23316    23312       -4     

☔ 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

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

PRレビュー: Feat/refine html attach (#427)

HTML添付メニューのUI改善と、{{PageHtml}}キャプチャ時に拡張機能自身の注入UI(content scriptルート・Command Hubルート)を除外する修正、ありがとうございます。全体的に意図が明確で良い変更だと思います。いくつか気になった点を報告します。

バグ: command_hub.tsx に旧IDへの参照が残っている(要修正)

packages/extension/src/command_hub.tsx:29 で、Command Hubのルート要素IDが ${APP_ID}-command-hub から ${APP_ID}-hub(= selection-command-hub)に変更されていますが、同ファイル内の renderMyCommands() 関数(packages/extension/src/command_hub.tsx:48-50)では旧IDの文字列 "selection-command-command-hub" がハードコードされたまま残っています。

if (isDebug) {
  const from = document.getElementById(
    "selection-command-command-hub",  // 旧ID、更新漏れ
  )?.shadowRoot
  from && cloneCss(from, shadow, "style")
}

vite.config.ts:102 側は targetId = "selection-command-hub" と正しく更新されているため、このリネームがここだけ漏れているように見えます。この結果、デバッグモード(isDebug)では document.getElementById(...) が常に null を返し、MyCommands コンポーネントへのCSSクローンが実行されなくなります(サイレントに失敗するため気づきにくい問題です)。

APP_ID を使って ${APP_ID}-hub に修正するか、定数化することをお勧めします。

挙動変更の懸念: InputPopup.tsxonFocusOutrelatedTarget が null の場合にメニューが閉じなくなる

packages/extension/src/components/pageAction/InputPopup.tsx:184-190:

const onFocusOut = (e: FocusEvent) => {
  if (!e.relatedTarget) return   // 追加された早期return
  if (!isTargetEditable(e.target)) return
  if (isPopup(e.relatedTarget as Element)) return
  setMenuVisible(false)
  setTargetElm(null)
}

型安全化のために e.relatedTarget のnullチェックを追加したと思われますが、これは挙動を変えてしまっています。isPopup(null)false を返す実装(packages/extension/src/lib/utils.ts:178-183)なので、変更前は relatedTarget === null のケースでも後続処理まで到達し、メニューが閉じていました。

relatedTargetnull になるのは「ブラウザのアドレスバーやDevToolsをクリックした」「別のウィンドウ/タブに切り替えた」「フォーカス可能な要素の外側をクリックした」など、ページ外へフォーカスが移動する一般的なケースです。この変更により、これらのケースで挿入メニュー(InputPopup)が閉じずに残ってしまう可能性があります。

意図的な挙動変更でなければ、以下のように早期returnを外し、型アサーションのみにすることをお勧めします。

const onFocusOut = (e: FocusEvent) => {
  if (!isTargetEditable(e.target)) return
  if (isPopup(e.relatedTarget as Element)) return
  setMenuVisible(false)
  setTargetElm(null)
}

テストカバレッジ: 新設の getPageHtml() に対する単体テストがない

packages/extension/src/services/dom/index.ts:112-117 に追加された getPageHtml() は、まさに今回のPRの主目的(拡張機能自身の注入要素を除外する)を担う関数ですが、packages/extension/src/services/dom/index.test.ts には対応するテストが見当たりませんでした。aiPrompt.test.ts 側も payload.pageHtmldefined/undefined かのみを検証しており、実際に #selection-command#selection-command-hub の要素が除去されているかまでは検証していません(packages/extension/src/action/aiPrompt.test.ts:247,280)。

jsdom環境で document.body#selection-command#selection-command-hub のダミー要素を追加した上で getPageHtml() を呼び、それらが結果に含まれないことを確認するテストを追加すると、今回の修正の意図がリグレッションしないことを保証できると思います。

軽微: vite.config.ts の広範なインデント変更

packages/extension/vite.config.ts の差分の大部分(39行の変更のうち大半)は、ネストした三項演算子やconsole系配列のインデントを4スペースずらすだけの整形変更で、機能的な変更は targetId の文字列変更のみです(packages/extension/vite.config.ts:102)。Prettier設定やエディタ差異によるものと思われますが、本来の変更点(ID文字列の更新)が見えにくくなっているので、可能であれば整形専用のコミットを分けるか、.prettierrc の設定を確認して意図しない再整形が起きないようにすると、今後のレビューがしやすくなります。

良かった点

  • getPageHtml() の実装自体(document.documentElement.cloneNode(true) してから対象要素を remove() する方式)はライブDOMを変更せず安全で、PAGE_HTML_MAX_CHARS によるスライスも既存の挙動を踏襲していて適切です。
  • InputPopup.tsxuseVisibleDelay の依存配列に updater を追加した点、FocusOutline の依存配列に container を追加した点は、useEffect の依存漏れ(exhaustive-deps)を正しく修正しており良い変更です。
  • HTML添付メニューのUI調整(区切り線の追加、パディング調整)はシンプルで、目的(見た目の統一)に合っています。

ujiro99 and others added 2 commits July 8, 2026 12:04
Review feedback noted the new getPageHtml() had no direct test, and
aiPrompt.test.ts only checked pageHtml defined/undefined without
verifying the injected containers were actually stripped. Add tests
that inject #selection-command and #selection-command-hub elements
and assert they're absent from the result.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ujiro99 ujiro99 merged commit 09eb031 into main Jul 8, 2026
5 of 6 checks passed
@ujiro99 ujiro99 deleted the feat/refine-html-attach branch July 8, 2026 03:18
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.

1 participant