Skip to content

upstream取り込み: Cmd+O 二重発火修正 (#3511)#302

Merged
MocA-Love merged 3 commits intomainfrom
upstream-merge/pr1-cmd-o-fix
Apr 17, 2026
Merged

upstream取り込み: Cmd+O 二重発火修正 (#3511)#302
MocA-Love merged 3 commits intomainfrom
upstream-merge/pr1-cmd-o-fix

Conversation

@MocA-Love
Copy link
Copy Markdown
Owner

@MocA-Love MocA-Love commented Apr 17, 2026

概要

upstream superset-sh/superset から Cmd+O 二重発火 fix (superset-sh#3511) を取り込む PR です。behind 4 → 3。

取り込み commit

SHA タイトル 分類
07c1ee0eb fix(desktop): Cmd+O firing open-in twice on v1 workspace route (superset-sh#3511) 確実に安全

背景

upstream は v1 workspace page.tsx から useHotkey("OPEN_IN_APP", handleOpenInApp) を削除し、OpenInMenuButton 側の登録に一本化することで二重発火を解消しました。

fork 適応修正

fork では tearoff window で TopBar 自体が描画されないため、page 側 registration を丸ごと削除すると tearoff 内で Cmd+O が死にます(Codex pre-review High 指摘)。

対応: useHotkey("OPEN_IN_APP", handler, { enabled: isActive && isTearoffWindow() }) として tearoff 限定で page 側登録を残しました。

  • main window: OpenInMenuButton のみ発火(upstream の意図通り二重発火解消)
  • tearoff window: page.tsx のみ発火(TopBar 不在を補う)
  • FORK NOTE コメント追加

Codex pre-review

  • 初回: No(tearoff で Cmd+O 死ぬ High 指摘)
  • 再レビュー: Yes(fork adaptation で解消確認)

テストチェックリスト

  • main window で v1 workspace を開き Cmd+O が 1 回だけ発火するか(openInApp mutation ログ)
  • tearoff window で v1 workspace を開き Cmd+O が 1 回発火するか
  • 通常クリック(OpenInMenuButton)動作は従来通り

Summary by CodeRabbit

バグ修正

  • ホットキー動作の改善 - 「アプリで開く」ホットキーがティアオフウィンドウ使用時のみ有効になるよう調整されました。

Kitenite and others added 2 commits April 18, 2026 04:37
…set-sh#3511)

OPEN_IN_APP was registered both in OpenInMenuButton and in the v1
workspace page, so pressing Cmd+O ran two separate openInApp mutations
(one per component's own useMutation instance) while clicking the
button only ran one. Remove the page-level registration so the shortcut
goes through the same handler as the click.
FORK NOTE: upstream superset-sh#3511 removed the page-level OPEN_IN_APP hotkey on the
v1 workspace route to avoid firing Cmd+O twice with OpenInMenuButton. The
fork renders no TopBar in tearoff windows (layout.tsx gates it on
!isTearoff), so removing the page-level registration killed Cmd+O inside
tearoff windows entirely. Guard the registration with isTearoffWindow()
so the main window still gets the upstream double-fire fix while tearoff
windows retain the shortcut.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1e119b78-d7c8-42ff-8a15-09442af0c968

📥 Commits

Reviewing files that changed from the base of the PR and between eefca1e and ab4ba8f.

📒 Files selected for processing (1)
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx

📝 Walkthrough

ウォークスルー

OPEN_IN_APPホットキー登録に条件を追加しました。ワークスペースページがアクティブであるとともに、現在のウィンドウがティアオフウィンドウである場合にのみ有効になるよう変更されました。

変更

コホート / ファイル(s) 概要
ホットキーゲーティング
apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx
isTearoffWindowインポートを追加し、OPEN_IN_APPホットキーの有効化条件をisActive && isTearoffWindow()に更新。ティアオフウィンドウでのホットキー動作を制御するフォーク固有のコメントを追加。

コード レビュー難易度の推定

🎯 2 (Simple) | ⏱️ ~8 分

🐰 ティアオフウィンドウで
ホットキーが輝く
条件ひとつ加えて
動作がスマートに
うさぎも喜ぶね 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed タイトルは upstream からの Cmd+O 二重発火修正という主要な変更を明確に示しており、変更内容と直結している。
Description check ✅ Passed PR 説明はテンプレートの主要セクション(概要、背景、対応、Codex pre-review、テストチェックリスト)をカバーしており、fork 特有の適応内容も詳細に記載されている。

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch upstream-merge/pr1-cmd-o-fix

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@MocA-Love
Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 17, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@MocA-Love MocA-Love merged commit 77ae138 into main Apr 17, 2026
6 checks passed
MocA-Love added a commit that referenced this pull request Apr 17, 2026
Upstream commits processed (cherry-picked, then adapted where needed):

- 07c1ee0 fix(desktop): Cmd+O firing open-in twice on v1 workspace route (superset-sh#3511)
  → PR #302 (with fork tearoff-window adaptation for Cmd+O)
- 4a1f41a chore(deps): upgrade tanstack/db + electric, drop durable-streams patch (superset-sh#3509)
  → PR #303 (fork keeps fstream patch)
- a3df489 feat(desktop): v2 PR checkout via widened checkout procedure (superset-sh#3525)
  → PR #304 (clean)
- c504a50 feat(desktop): v2 file editor — foundation, views, and stability pass (superset-sh#3526)
  → PR #310 (foundation: 58 files path-checkout)
  → PR #311 (adaptation: 20 files manual port with SpreadsheetViewer/memo/fork-hotkeys preserved)
- 78b7dc8 feat(desktop): promote "Create Section Below" to top-level on workspace menu (superset-sh#3537)
  → PR #308

Record merge so upstream/main..main shows 0 behind.
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.

2 participants