Skip to content

Feat/update hub integration#407

Merged
ujiro99 merged 6 commits into
mainfrom
feat/update-hub-integration
May 23, 2026
Merged

Feat/update hub integration#407
ujiro99 merged 6 commits into
mainfrom
feat/update-hub-integration

Conversation

@ujiro99
Copy link
Copy Markdown
Owner

@ujiro99 ujiro99 commented May 23, 2026

No description provided.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 23, 2026

Codecov Report

❌ Patch coverage is 44.44444% with 55 lines in your changes missing coverage. Please review.
✅ Project coverage is 27.84%. Comparing base (2adefd5) to head (a633540).
⚠️ Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
...es/extension/src/components/option/ShareButton.tsx 0.00% 17 Missing ⚠️
...ckages/extension/src/services/settings/settings.ts 14.28% 12 Missing ⚠️
.../extension/src/hooks/option/useSharedCommandIds.ts 0.00% 7 Missing ⚠️
packages/hub/eslint.config.mjs 0.00% 7 Missing ⚠️
packages/extension/src/services/hub/background.ts 87.23% 6 Missing ⚠️
...nsion/src/components/option/editor/CommandList.tsx 0.00% 5 Missing ⚠️
...sion/src/components/option/editor/ShortcutList.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #407      +/-   ##
==========================================
+ Coverage   27.78%   27.84%   +0.05%     
==========================================
  Files         317      317              
  Lines       32385    32448      +63     
  Branches     1808     1815       +7     
==========================================
+ Hits         8998     9034      +36     
- Misses      23387    23414      +27     

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

@claude
Copy link
Copy Markdown

claude Bot commented May 23, 2026

コードレビュー: Feat/update hub integration

PR全体として、Hub連携のコマンドID重複対応・共有後のUI更新・URLパス修正など、複数の改善がまとまっており、方向性は適切です。ただし、いくつか修正を推奨する点があります。


🔴 重大な問題

1. フォームステートが更新されない (CommandList.tsx:302-306)

旧実装react-hook-formcommandArray.updatesetValue を通じてフォームステートと永続化を両方更新していました。新実装Settings.updateCommandId(ストレージ更新のみ)だけになっています。

// 現状 (CommandList.tsx:302-306)
const handleUpdateCommandId = (commandId: string, newId: string) => {
  Settings.updateCommandId(commandId, newId).catch((err) => {
    console.error("[handleUpdateCommandId] Failed to update command ID:", err)
  })
}

この関数は ShareButton.tsxCommandTreeRenderer.tsx:79-80CommandList.tsx:396 と伝播します。ユーザーが非UUIDv7のコマンドを共有した際に onCommandIdChange で新IDがセットされますが、フォームステートは古いIDのまま残ります。その後ユーザーが「保存」を押すと、フォームが古いIDをストレージに書き戻してしまい、Settings.updateCommandId による更新を上書きします。

修正案: ストレージ更新に加えて commandArray.update でフォームステートも更新する(旧実装の動作を維持しつつ、ショートカット更新を Settings.updateCommandId に委譲する)。


2. 非同期の fire-and-forget によるID不整合 (background.ts:119-128)

// background.ts:119-128
Settings.updateCommandId(oldId, newId).catch((err) => {
  console.error("[shareCommandToHub] Failed to update command ID:", err)
})

currentParam = { ...currentParam, id: newId }
port.postMessage({ type: "share-command", command: currentParam })  // すぐ送信

ローカルのID更新完了を待たずにHubへメッセージを送信しています。Settings.updateCommandId が失敗した場合、Hubは新IDを保持しているのにローカルストレージは旧IDのままになり、不整合が生じます。

修正案: await Settings.updateCommandId(...) してからメッセージを送信する。


🟡 改善推奨

3. 共有済みアイコンの視覚的フィードバックが弱い (ShareButton.tsx:87-93)

// ShareButton.tsx:87-93
<CloudCheck
  className={cn(
    "stroke-gray-500 group-hover/share-btn:stroke-green-600",  // デフォルトがグレー
    status === "error" && "stroke-red-500",
  )}
  size={16}
/>

isSharedtrue の場合でもデフォルトカラーが stroke-gray-500(グレー)のため、未共有のアイコンと区別がつきにくいです。旧コードでは bg-green-50stroke-green-600 で共有済み状態が明確でした。

修正案:

<CloudCheck
  className={cn(
    "stroke-green-600",  // 共有済みは常にグリーン
    "group-hover/share-btn:stroke-green-700",
    status === "error" && "stroke-red-500",
  )}
  size={16}
/>

4. ShortcutList.tsx の型安全性後退 (ShortcutList.tsx:28)

// 変更前
control: Control<any>
// 変更後
control: any

Control<any> は少なくとも react-hook-formControl 型であることを明示していましたが、any はその情報すら失います。なぜ変更が必要だったか不明です。Control<any> のまま維持することを推奨します。


5. MAX_ID_REGENERATE 定数のスコープ (background.ts:81)

// background.ts:81 — share() のクロージャ内で定義
const MAX_ID_REGENERATE = 3

RETRY_INTERVAL_MSMAX_RETRIES と同様にモジュールスコープで定義すると、定数の一覧性が上がり保守しやすくなります。


🟢 良い点

  • Settings.updateCommandId の抽象化 (settings.ts:186-201): コマンドIDとショートカット参照を一括更新するロジックをサービス層に集約したのは適切な設計です。

  • share-command-submitted による共有済みIDの再取得 (useSharedCommandIds.ts:39-44): HUB_SHARED_AT のリスナーを追加して共有後にIDセットを自動更新するアプローチは明快で、currentHubUser クロージャでログイン状態を追跡している点も適切です。

  • テストの拡充 (background.test.ts): SH-08として share-command-submitted の新フローをカバーするテストが追加されており、既存テスト (SH-07) の説明文も実態に合わせて更新されています。

  • URLパスの修正 (ShareButton.tsx:42, background.ts:161, background.test.ts:1134): /dashboard/commands/dashboard/mycommands が3箇所一貫して修正されています。


ℹ️ 確認事項

  • lucide-react のメジャーバージョンアップ (package.json:51): ^0.483.0^1.16.0 はメジャーバージョン変更です。CloudCheck 以外のアイコン(Share, Keyboard, SquareArrowOutUpRight 等)が新バージョンで引き続き同名・同APIで提供されているか確認を推奨します。

ローカルIDの更新完了前にHubへ再送信していた非同期の不整合を修正。
Settings.updateCommandId が失敗した場合はリスナーを削除して処理を中止し、
ローカルストレージとHub間のID不整合を防ぐ。

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@ujiro99 ujiro99 merged commit a8b8dc9 into main May 23, 2026
3 of 4 checks passed
@ujiro99 ujiro99 deleted the feat/update-hub-integration branch May 23, 2026 10:08
@claude
Copy link
Copy Markdown

claude Bot commented May 23, 2026

PR #407 コードレビュー: Feat/update hub integration

概要

このPRはHub連携のURL変更(/dashboard/commands/dashboard/mycommands)、重複コマンドIDエラーのハンドリング追加、共有完了後の再フェッチ機能追加を行っています。全体的に機能追加の方向性は適切ですが、いくつか検討すべき点があります。


🔴 要対応(バグ・不整合リスク)

1. Settings.updateCommandId の部分失敗リスク

ファイル: packages/extension/src/services/settings/settings.ts:189-201

updateCommandId: async (oldId: string, newId: string): Promise<void> => {
  const commands = await Storage.getCommands()
  await Storage.setCommands(...)         // ← ここで成功
  const shortcuts = await Storage.get(...)
  await Storage.set(STORAGE_KEY.SHORTCUTS, ...)  // ← ここで失敗するとコマンドIDが不整合に
},

Storage.setCommands() が成功した後に Storage.set(STORAGE_KEY.SHORTCUTS) が失敗すると、コマンドIDは更新されたのにショートカットは古いIDのままになります。ストレージAPIにトランザクションがない場合でも、エラー時にコマンドIDをロールバックするかエラーをより明示的にすべきです。


2. DUPLICATE_COMMAND_ID 後の再送にリトライがない

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

currentParam = { ...currentParam, id: newId }
port.postMessage({ type: "share-command", command: currentParam })
return

DUPLICATE_COMMAND_ID 受信後、新しいIDでコマンドを1回だけ送信していますが、タイマーはクリアされており(112行目: clearInterval(timer))、再起動されません。Hubがこの再送を受け取れなかった場合、処理が止まってしまいます。idRegenerateCount をリセットしてタイマーを再起動する、またはリトライカウントを引き継ぐべきです。


3. CommandList.tsx でフォームの状態が更新されない

ファイル: packages/extension/src/components/option/editor/CommandList.tsx:300-305

変更前はフォームの状態(commandArray.update() / setValue())を即座に更新していましたが、変更後は Settings.updateCommandId でストレージのみ更新しています。

const handleUpdateCommandId = (commandId: string, newId: string) => {
  Settings.updateCommandId(commandId, newId).catch((err) => {
    console.error("[handleUpdateCommandId] Failed to update command ID:", err)
  })
}

ストレージは更新されるがUI(フォーム)は即時更新されず、ページリロードするまで表示が古いIDのままになる可能性があります。ストレージ更新後にフォームを同期するロジックが必要です。


🟡 改善推奨

4. 型安全性の後退

ファイル: packages/extension/src/components/option/editor/ShortcutList.tsx:28

// 変更前
control: Control<any>
// 変更後
control: any

Control<any> は react-hook-form の型構造を保持しますが、any は完全に型チェックを無効化します。Control<CommandsSchemaType> や少なくとも Control<any> に戻すことを推奨します。


5. shareCommandToHub にオリジン検証がない

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

onPortConnect = function portConnect(port: chrome.runtime.Port) {
  if (port.name !== "hub-share") return
  if (port.sender?.tab?.id !== tabId) return
  // ← hubOrigin チェックがない

同じファイルの pushEditToHub(622行目)や handleEditCommand(429行目)は if (port.sender?.origin !== hubOrigin) return を行っていますが、shareCommandToHub にはこのチェックがありません。これは本PR以前からの問題ですが、修正の機会として対応を推奨します。


6. lucide-react のメジャーバージョンアップ

ファイル: packages/extension/package.json:51

-"lucide-react": "^0.483.0",
+"lucide-react": "^1.16.0",

0.x → 1.x はメジャーバージョン変更です。使用中の他のアイコン(ShareKeyboardSquareArrowOutUpRight など)が1.x系で名称変更・削除されていないか確認が必要です。また CloudCheck が1.16.0に存在することも確認済みでしょうか?


🟢 良い点

  • SH-08〜SH-10 のテスト追加 (background.test.ts): share-command-submitted のハンドリング、DUPLICATE_COMMAND_ID の正常系・異常系それぞれにテストが追加されており、意図する動作が明確化されています。
  • HUB_SHARED_AT によるリアクティブ更新 (useSharedCommandIds.ts): 共有完了後にSharedコマンドIDの一覧を自動再フェッチするアプローチは適切です。
  • URL パスの統一: dashboard/commandsdashboard/mycommandsShareButton.tsxbackground.ts の両方で一貫して変更しています。
  • アイコン切り替えのロジック: isShared に応じて Share / CloudCheck を切り替える実装は読みやすいです。

まとめ

主な懸念は updateCommandId の部分失敗リスク(1)と DUPLICATE_COMMAND_ID 後のリトライ欠如(2)の2点です。フォーム状態の非同期と型安全性の後退も合わせてご確認ください。

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