Add v1 split previews for sidebar file drags#336
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 45 minutes and 59 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 Walkthroughウォークスルー内部ファイルのドラッグ・ドロップ機能がペインに追加されました。ファイル操作時の動作を管理する新しいフック、ドロップ位置をビジュアルで表示するオーバーレイコンポーネント、相対的な分割位置をサポートするタブストア の改善が含まれています。ターミナルコンポーネントから既存のドラッグハンドラーは削除されました。 変更内容
シーケンス図sequenceDiagram
actor User
participant BasePaneWindow
participant FileSystem as File System<br/>(DataTransfer)
participant Overlay as InternalFileDropOverlay
participant Store as Tabs Store
participant TRPC as TRPC Query
User->>FileSystem: ファイルをドラッグ開始
FileSystem->>BasePaneWindow: onDragEnter (キャプチャ)
BasePaneWindow->>BasePaneWindow: 内部ファイルパス判定<br/>(hasInternalDraggedFilePath)
BasePaneWindow->>BasePaneWindow: ドラッグ深度+1<br/>ドロップ位置計算
BasePaneWindow->>Overlay: position 更新
Overlay->>Overlay: ドロップ領域を<br/>レンダリング
User->>FileSystem: マウス移動
FileSystem->>BasePaneWindow: onDragOver (キャプチャ)
BasePaneWindow->>BasePaneWindow: カーソル位置から<br/>MosaicDropPosition計算
BasePaneWindow->>Overlay: position 更新 (アニメーション)
User->>FileSystem: ファイルをドロップ
FileSystem->>BasePaneWindow: onDropCapture
BasePaneWindow->>BasePaneWindow: 内部ファイルパス抽出
BasePaneWindow->>Store: addFileViewerPane<br/>(relativeToTabId,<br/>relativeToPaneId,<br/>relativeSplitPosition)
Store->>Store: ペイン位置をレイアウト内<br/>で検索<br/>(findPanePathInLayout)
Store->>Store: 分割比率を計算<br/>(相対位置 + サイドバー幅)
Store->>Store: 新ペインを<br/>指定位置に挿入
BasePaneWindow->>Overlay: position = null
Overlay->>Overlay: オーバーレイ非表示
推定コードレビュー時間🎯 3 (中程度) | ⏱️ ~20 分 関連する可能性のあるPR
詩
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
165-184: 既存のfindPanePathと重複しています。Line 676-698 に同じ探索ロジックがあるため、こちらを新規実装すると将来の修正漏れが起きやすくなります。
store.ts側で既存関数を使うか、意味名が必要なら alias にしてください。修正案
-export const findPanePathInLayout = ( - layout: MosaicNode<string>, - paneId: string, - currentPath: MosaicBranch[] = [], -): MosaicBranch[] | null => { - if (typeof layout === "string") { - return layout === paneId ? currentPath : null; - } - - const firstPath = findPanePathInLayout(layout.first, paneId, [ - ...currentPath, - "first", - ]); - if (firstPath) return firstPath; - - return findPanePathInLayout(layout.second, paneId, [ - ...currentPath, - "second", - ]); -};既存
findPanePathの宣言後に alias が必要なら追加できます。export const findPanePathInLayout = findPanePath;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/tabs/utils.ts` around lines 165 - 184, The implemented recursive function findPanePathInLayout duplicates existing logic from findPanePath; remove this duplicate implementation and instead export an alias or reuse the original function: replace the body/definition of findPanePathInLayout with a direct reference to the existing findPanePath (e.g., export const findPanePathInLayout = findPanePath) or import and re-export the original function where needed so updates stay centralized and avoid drift between findPanePathInLayout and findPanePath.apps/desktop/src/renderer/stores/tabs/store.ts (1)
1128-1134:!options.openInNewTabチェックは冗長です。Line 1071〜1120 の
if (options.openInNewTab)分岐で既にreturnしているため、ここに到達した時点でoptions.openInNewTabは常に falsy です。可読性を重視するなら残しても良いですが、条件から外してもロジックは変わりません。♻️ 任意のクリーンアップ例
- const splitTargetTab = - options.relativeToTabId && - !options.openInNewTab && - options.relativeToPaneId - ? (state.tabs.find((tab) => tab.id === options.relativeToTabId) ?? - null) - : null; + const splitTargetTab = + options.relativeToTabId && options.relativeToPaneId + ? (state.tabs.find((tab) => tab.id === options.relativeToTabId) ?? + null) + : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/tabs/store.ts` around lines 1128 - 1134, The check for !options.openInNewTab inside the splitTargetTab assignment is redundant because earlier code returns when options.openInNewTab is truthy; remove the !options.openInNewTab clause from the ternary so the expression becomes: options.relativeToTabId && options.relativeToPaneId ? (state.tabs.find(tab => tab.id === options.relativeToTabId) ?? null) : null, keeping the rest of the logic intact (identify the assignment to splitTargetTab in store.ts and the earlier if (options.openInNewTab) return branch to confirm behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/hooks/useFileDragBehavior/useFileDragBehavior.ts`:
- Around line 12-15: The hook useFileDragBehavior currently mutates the external
mutable cachedFileDragBehavior during render; change this to update the cache
inside a useEffect so the write occurs after commit. Keep using
electronTrpc.settings.getFileDragBehavior.useQuery() and the local behavior =
data ?? DEFAULT_FILE_DRAG_BEHAVIOR, then add a useEffect that runs when behavior
changes and assigns cachedFileDragBehavior = behavior inside the effect to avoid
writing during render and leaking uncommitted values to getFileDragBehavior().
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/components/BasePaneWindow/BasePaneWindow.tsx`:
- Around line 261-293: handleInternalFileDropCapture currently prefers the
cached internalFileDropPositionRef (from dragover) and bails without consuming
the event when getInternalDraggedFilePath returns empty; update it to always
recompute the drop position from the current event by calling
updateInternalFileDropPosition(event) (falling back to the ref only if that
returns undefined), and always call event.preventDefault() and
event.stopPropagation() when hasInternalDraggedFilePath(event.dataTransfer) is
true so the drop is consumed even if getInternalDraggedFilePath yields no
payload; use the existing symbols handleInternalFileDropCapture,
internalFileDropPositionRef, updateInternalFileDropPosition,
hasInternalDraggedFilePath, getInternalDraggedFilePath,
resetInternalFileDragState, and addFileViewerPane to locate and implement the
changes.
In `@apps/desktop/src/renderer/stores/tabs/store.ts`:
- Around line 1139-1180: When splitTargetPath is null the code currently ignores
splitDirection/insertBeforeTarget and always falls back to appending with
direction: "row"; instead, preserve the intended split behavior by using the
already-computed splitNode as the fallback newLayout when splitTargetPath ===
null (i.e. when options.relativeToPaneId was provided but findPanePathInLayout
returned null). Update the newLayout construction (around the use of
splitTargetPath, splitNode, targetTab.layout, and createFileViewerPane) so that
if splitTargetPath is null you assign newLayout = splitNode (or otherwise
construct using splitNode) rather than always building the row append; keep
using updateTree only when splitTargetPath is a non-null path array.
- Around line 68-82: The fallback in addFileViewerPane currently forces
direction: "row" when splitTargetPath is null, ignoring the user's
relativeSplitPosition; update the fallback so direction is computed from
relativeSplitPosition (use "column" for "top"/"bottom" and "row" for
"left"/"right")—you can reuse getRelativeFileViewerSplitPercentage or mirror its
intent—and ensure splitTargetPath null branch sets the split position/direction
according to relativeSplitPosition (and preserve right-sidebar width behavior
when useRightSidebarOpenViewWidth is relevant); also remove the redundant
!options.openInNewTab check (it’s covered by the earlier early return) and
verify references to findPanePathInLayout, splitTargetPath,
relativeSplitPosition, and addFileViewerPane are updated accordingly.
---
Nitpick comments:
In `@apps/desktop/src/renderer/stores/tabs/store.ts`:
- Around line 1128-1134: The check for !options.openInNewTab inside the
splitTargetTab assignment is redundant because earlier code returns when
options.openInNewTab is truthy; remove the !options.openInNewTab clause from the
ternary so the expression becomes: options.relativeToTabId &&
options.relativeToPaneId ? (state.tabs.find(tab => tab.id ===
options.relativeToTabId) ?? null) : null, keeping the rest of the logic intact
(identify the assignment to splitTargetTab in store.ts and the earlier if
(options.openInNewTab) return branch to confirm behavior).
In `@apps/desktop/src/renderer/stores/tabs/utils.ts`:
- Around line 165-184: The implemented recursive function findPanePathInLayout
duplicates existing logic from findPanePath; remove this duplicate
implementation and instead export an alias or reuse the original function:
replace the body/definition of findPanePathInLayout with a direct reference to
the existing findPanePath (e.g., export const findPanePathInLayout =
findPanePath) or import and re-export the original function where needed so
updates stay centralized and avoid drift between findPanePathInLayout and
findPanePath.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a46a785-6350-49c6-babe-2cffac976357
📒 Files selected for processing (10)
apps/desktop/src/renderer/hooks/useFileDragBehavior/index.tsapps/desktop/src/renderer/hooks/useFileDragBehavior/useFileDragBehavior.tsapps/desktop/src/renderer/lib/file-drag.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/components/BasePaneWindow/BasePaneWindow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/components/BasePaneWindow/components/InternalFileDropOverlay/InternalFileDropOverlay.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/components/BasePaneWindow/components/InternalFileDropOverlay/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/utils.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
Comment on lines
+12
to
+15
| export function useFileDragBehavior(): FileDragBehavior { | ||
| const { data } = electronTrpc.settings.getFileDragBehavior.useQuery(); | ||
| const behavior = data ?? DEFAULT_FILE_DRAG_BEHAVIOR; | ||
| cachedFileDragBehavior = behavior; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
fd -t f "useFileDragBehavior.ts" apps/desktopRepository: MocA-Love/superset
Length of output: 136
🏁 Script executed:
# Read the complete file to see the context
cat -n apps/desktop/src/renderer/hooks/useFileDragBehavior/useFileDragBehavior.tsRepository: MocA-Love/superset
Length of output: 836
🏁 Script executed:
# Search for where cachedFileDragBehavior is defined and used
rg "cachedFileDragBehavior" apps/desktop/src/renderer/hooks/useFileDragBehavior/Repository: MocA-Love/superset
Length of output: 429
🏁 Script executed:
# Check the referenced useFileOpenMode pattern
fd -t f "useFileOpenMode*" apps/desktop/src/renderer/hooks/Repository: MocA-Love/superset
Length of output: 128
🏁 Script executed:
# Look for how getFileDragBehavior is exported and used outside React
rg "getFileDragBehavior" apps/desktop/src/renderer/hooks/useFileDragBehavior/Repository: MocA-Love/superset
Length of output: 424
🏁 Script executed:
# Check the useFileOpenMode pattern to see if it has a similar pattern
cat -n apps/desktop/src/renderer/hooks/useFileOpenMode/useFileOpenMode.tsRepository: MocA-Love/superset
Length of output: 768
🏁 Script executed:
# Look for where getFileDragBehavior is called outside of React components
rg "getFileDragBehavior\(\)" apps/desktop/src --type ts --type tsxRepository: MocA-Love/superset
Length of output: 89
🏁 Script executed:
# Check if there are any side effects or IPC calls that use getFileDragBehavior
rg "getFileDragBehavior" apps/desktop/src -A 2 -B 2Repository: MocA-Love/superset
Length of output: 6093
レンダー中のモジュールキャッシュ更新を避けてください。
Line 15 でレンダー実行中に外部可変状態 cachedFileDragBehavior を書き換えると、React の未コミットレンダー値が getFileDragBehavior() に漏れる可能性があります。useEffect で同期することで、コミット後の状態のみを反映できます。
修正案
+import { useEffect } from "react";
import type { FileDragBehavior } from "@superset/local-db";
import { electronTrpc } from "renderer/lib/electron-trpc";
import { DEFAULT_FILE_DRAG_BEHAVIOR } from "shared/constants";
@@
export function useFileDragBehavior(): FileDragBehavior {
const { data } = electronTrpc.settings.getFileDragBehavior.useQuery();
const behavior = data ?? DEFAULT_FILE_DRAG_BEHAVIOR;
- cachedFileDragBehavior = behavior;
+ useEffect(() => {
+ cachedFileDragBehavior = behavior;
+ }, [behavior]);
return behavior;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/hooks/useFileDragBehavior/useFileDragBehavior.ts`
around lines 12 - 15, The hook useFileDragBehavior currently mutates the
external mutable cachedFileDragBehavior during render; change this to update the
cache inside a useEffect so the write occurs after commit. Keep using
electronTrpc.settings.getFileDragBehavior.useQuery() and the local behavior =
data ?? DEFAULT_FILE_DRAG_BEHAVIOR, then add a useEffect that runs when behavior
changes and assigns cachedFileDragBehavior = behavior inside the effect to avoid
writing during render and leaking uncommitted values to getFileDragBehavior().
Owner
Author
There was a problem hiding this comment.
この件は今回は見送ります。理由は 2 点で、まず getFileDragBehavior() は現状どこからも使われておらず、未コミット値が外へ漏れる実害がありません。加えて useFileOpenMode() と useRightSidebarOpenViewWidth() も同じモジュールキャッシュ更新パターンを使っているため、この hook だけ useEffect 化すると設定 getter 群の振る舞いが不揃いになります。もし直すなら 3 つまとめて揃える follow-up の方が筋だと判断しました。
There was a problem hiding this comment.
@MocA-Love、ご説明ありがとうございます。理解しました。getFileDragBehavior() が未使用である点と、useFileOpenMode() / useRightSidebarOpenViewWidth() と設計を揃えるという判断は筋が通っています。3 つまとめて修正する follow-up の方が一貫性の観点からも適切ですね。
今後の follow-up 用に学習として記録しておきます。follow-up issue を作成しましょうか？
✏️ Learnings added
Learnt from: MocA-Love
Repo: MocA-Love/superset PR: 336
File: apps/desktop/src/renderer/hooks/useFileDragBehavior/useFileDragBehavior.ts:12-15
Timestamp: 2026-04-19T01:06:02.134Z
Learning: In `apps/desktop/src/renderer/hooks/`, the hooks `useFileDragBehavior`, `useFileOpenMode`, and `useRightSidebarOpenViewWidth` all intentionally use the same module-level cache update pattern (assigning to a module-level variable directly during render, not inside a `useEffect`). This is a deliberate design choice for consistency across settings-getter hooks. If this pattern is changed (e.g., to use `useEffect`), all three hooks must be updated together in the same PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: MocA-Love/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2026-03-27T17:12:04.711Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f5c090ced
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This was referenced Apr 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
補足
bun run lintとbun run typecheckはローカルで通過済みSummary by CodeRabbit
リリースノート