Open sidebar file drags in the file viewer#334
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 46 minutes and 37 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)
📝 WalkthroughWalkthroughファイルドラッグ動作設定を新規追加し、ユーザーがファイルをドラッグ時に「ファイルビューアで開く」または「パスを貼り付け」のいずれかを選択できる機能を実装しました。データベーススキーマ拡張、TRPC設定エンドポイント、UIコンポーネント、ドラッグアンドドロップハンドラーを追加しました。 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Settings UI
participant TRPC Settings
participant Database
participant Drag Handler
participant File Viewer
User->>Settings UI: ファイルドラッグ動作を選択<br/>(open-file-viewer / paste-path)
Settings UI->>TRPC Settings: setFileDragBehavior(newBehavior)
TRPC Settings->>Database: upsert settings.fileDragBehavior
Database-->>TRPC Settings: success
TRPC Settings-->>Settings UI: { success: true }
Settings UI->>User: 設定を保存
User->>Drag Handler: ファイルをドラッグ&ドロップ
Drag Handler->>TRPC Settings: getFileDragBehavior()
TRPC Settings->>Database: select fileDragBehavior
Database-->>TRPC Settings: behavior value
TRPC Settings-->>Drag Handler: behavior
alt behavior === "open-file-viewer"
Drag Handler->>File Viewer: addFileViewerPane(filePath)
File Viewer->>User: ファイルを表示
else behavior === "paste-path"
Drag Handler->>User: パスをペースト
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/components/EditCustomRingtoneDialog/EditCustomRingtoneDialog.tsx (1)
33-91:⚠️ Potential issue | 🟠 Major
useMutationの戻り値オブジェクトを effect の依存配列から削除してください。Line 78 と Line 91 で
openSource/closeSource全体を依存配列に含めると、mutation の状態更新でオブジェクト identity が変わる場合に effect が再実行されます。特に Line 56-78 の acquisition effect は mutation オブジェクトの identity 変化をきっかけにopenCustomSource.mutateAsync()を複数回呼び出す可能性があり、最後のtempIdだけがopenedTempIdRefに残るため、先に確保した temp audio リソースを解放できないリスクがあります。
mutateAsync/mutateを分割代入して、それらだけを依存配列に入れてください。修正案
- const openSource = electronTrpc.ringtone.openCustomSource.useMutation(); - const closeSource = electronTrpc.ringtone.closeCustomSource.useMutation(); + const { mutateAsync: openCustomSource } = + electronTrpc.ringtone.openCustomSource.useMutation(); + const { mutate: closeCustomSource } = + electronTrpc.ringtone.closeCustomSource.useMutation(); const reEdit = electronTrpc.ringtone.reEditCustom.useMutation(); // ... useEffect(() => { if (!open) return; let cancelled = false; - openSource - .mutateAsync() + openCustomSource() .then((result) => { if (cancelled) return; if (result.tempId) { setTempId(result.tempId); openedTempIdRef.current = result.tempId; } else { setErrorMessage( "No saved source audio for this ringtone. Re-import from YouTube to enable editing.", ); } }) .catch((err: Error) => { if (!cancelled) setErrorMessage(err.message); }); return () => { cancelled = true; }; - }, [open, openSource]); + }, [open, openCustomSource]); // Release the tempId when the dialog closes. useEffect(() => { if (open) return; const id = openedTempIdRef.current; if (id) { - closeSource.mutate({ tempId: id }); + closeCustomSource({ tempId: id }); openedTempIdRef.current = null; } setTempId(null); setErrorMessage(null); setDisplayName(currentDisplayName); - }, [open, closeSource, currentDisplayName]); + }, [open, closeCustomSource, currentDisplayName]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/components/EditCustomRingtoneDialog/EditCustomRingtoneDialog.tsx` around lines 33 - 91, The effects should depend only on the mutation functions, not the whole mutation objects; destructure openSource.mutateAsync and closeSource.mutate into local constants (e.g. const { mutateAsync: openMutateAsync } = openSource and const { mutate: closeMutate } = closeSource) and use those in the respective useEffect dependency arrays instead of openSource/closeSource, so the acquisition effect that calls openSource.mutateAsync() and the cleanup effect that calls closeSource.mutate() won't re-run due to identity changes of the mutation objects and risk leaking temp audio (openedTempIdRef/tempId).
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/components/AudioEditor/AudioEditor.tsx (1)
208-237:hasInitialRange依存追加により波形の再デコードが発生する点に注意。
hasInitialRangeを依存配列に追加したことで、initialStartSeconds/initialEndSecondsがundefinedから値ありへ(またはその逆へ)切り替わった際に、decodeWaveformPeaksによる fetch +AudioContext.decodeAudioDataが丸ごと再実行されます。audioUrl(=tempId) が変わっていないのに波形を再計算するのは無駄で、再編集フローでプロップが遅延して入ってくるケースではローディングスピナーが一瞬再表示される可能性もあります。本来このエフェクトが気にしたいのは「初回デコード完了後に
hasInitialRangeが false なら[0, min(10, duration)]を適用する」という初期レンジの適用ロジックだけのはずなので、デコード用エフェクトと初期レンジ適用用エフェクトを分ける方が意図が明確で再デコードも避けられます。♻️ 提案する分割
- useEffect(() => { - let cancelled = false; - setIsLoadingWaveform(true); - setWaveformError(null); - - decodeWaveformPeaks(audioUrl, 2400) - .then((data) => { - if (!cancelled) { - setWaveform(data); - if (!hasInitialRange) { - setStartSeconds(0); - setEndSeconds(Math.min(10, data.duration)); - } - } - }) - .catch((err) => { - if (!cancelled) { - setWaveformError( - err instanceof Error ? err.message : "Failed to load audio", - ); - } - }) - .finally(() => { - if (!cancelled) setIsLoadingWaveform(false); - }); - - return () => { - cancelled = true; - }; - }, [audioUrl, hasInitialRange]); + const hasInitialRangeRef = useRef(hasInitialRange); + hasInitialRangeRef.current = hasInitialRange; + useEffect(() => { + let cancelled = false; + setIsLoadingWaveform(true); + setWaveformError(null); + + decodeWaveformPeaks(audioUrl, 2400) + .then((data) => { + if (!cancelled) { + setWaveform(data); + if (!hasInitialRangeRef.current) { + setStartSeconds(0); + setEndSeconds(Math.min(10, data.duration)); + } + } + }) + .catch((err) => { + if (!cancelled) { + setWaveformError( + err instanceof Error ? err.message : "Failed to load audio", + ); + } + }) + .finally(() => { + if (!cancelled) setIsLoadingWaveform(false); + }); + + return () => { + cancelled = true; + }; + }, [audioUrl]);なお、本 PR のタイトル/説明(サイドバーのファイルドラッグ挙動)とこのファイルの変更は無関係に見えます。意図的な同梱なのか、別ブランチから混入したものなのかご確認ください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/components/AudioEditor/AudioEditor.tsx` around lines 208 - 237, The current useEffect mixes waveform decoding and initial-range application causing unnecessary re-decodes when hasInitialRange changes; split it into two effects: 1) keep the decoding effect using useEffect(..., [audioUrl]) that sets setIsLoadingWaveform, setWaveformError, decodes via decodeWaveformPeaks(audioUrl, 2400) with the existing cancelled flag and finally sets setIsLoadingWaveform(false) and setWaveform(data) (but do NOT touch start/end seconds here), and 2) add a separate effect using useEffect(..., [waveform, hasInitialRange]) that runs only after waveform is available and when hasInitialRange is false to call setStartSeconds(0) and setEndSeconds(Math.min(10, waveform.duration)); this prevents re-running decodeWaveformPeaks when hasInitialRange toggles while preserving the initial-range logic tied to setStartSeconds/setEndSeconds.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx (1)
292-312: ネイティブファイルドロップとの分岐順序に関する軽微な不整合
Terminal.tsx（v1）のhandleDropではfiles.length === 0 && internalFilePath && ...と外部ファイルドロップを優先する条件になっていますが、こちらの v2TerminalPaneではfiles.lengthのチェックが無く、INTERNAL_FILE_PATH_MIMEが立っていればdataTransfer.filesの有無に関わらずファイルビューアを開きます。現状はuseFileDragだけがこの MIME を設定するため実害は無いものの、将来的にネイティブドロップ元が同 MIME を付与した場合に v1/v2 で挙動が分岐し得ます。防御的に両者を揃えておくと安心です。♻️ 揃えるための差分案
- const internalFilePath = getInternalDraggedFilePath(event.dataTransfer); - if ( - internalFilePath && - (fileDragBehavior ?? DEFAULT_FILE_DRAG_BEHAVIOR) === "open-file-viewer" - ) { + const hasNativeFiles = event.dataTransfer.files.length > 0; + const internalFilePath = getInternalDraggedFilePath(event.dataTransfer); + if ( + !hasNativeFiles && + internalFilePath && + (fileDragBehavior ?? DEFAULT_FILE_DRAG_BEHAVIOR) === "open-file-viewer" + ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx around lines 292 - 312, The drop handler handleDrop currently treats an INTERNAL_FILE_PATH_MIME drop as a file-viewer open regardless of native file presence; change the branching to mirror v1 by first checking event.dataTransfer.files.length === 0 AND internalFilePath (from getInternalDraggedFilePath(event.dataTransfer)) before deciding to call onOpenFile (respecting fileDragBehavior/DEFAULT_FILE_DRAG_BEHAVIOR and fileOpenMode/DEFAULT_FILE_OPEN_MODE). Keep the early returns for connectionState === "closed" and empty resolved text (resolveDroppedText), and still focus/paste via terminalRuntimeRegistry.getTerminal(terminalId) / terminalRuntimeRegistry.paste when not opening the file viewer.
🤖 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/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx:
- Around line 358-373: Update the FORK NOTE above handleTerminalOpenFile to
explicitly state the behavioral difference when openInNewTab is false versus
undefined: explain that handleTerminalOpenFile(filePath, openInNewTab?) checks
openInNewTab !== undefined, so callers passing openInNewTab = false (e.g., drag
& drop or fileOpenMode="split-pane") go through openSidebarFilePane(filePath,
false) which triggers sidebar split-width recalculation, whereas callers that
omit the second arg (undefined) call openFilePane(filePath) and do not adjust
the sidebar width; mention the functions handleTerminalOpenFile,
openSidebarFilePane and openFilePane so future readers understand why false must
be treated as a defined value.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/components/EditCustomRingtoneDialog/EditCustomRingtoneDialog.tsx`:
- Around line 33-91: The effects should depend only on the mutation functions,
not the whole mutation objects; destructure openSource.mutateAsync and
closeSource.mutate into local constants (e.g. const { mutateAsync:
openMutateAsync } = openSource and const { mutate: closeMutate } = closeSource)
and use those in the respective useEffect dependency arrays instead of
openSource/closeSource, so the acquisition effect that calls
openSource.mutateAsync() and the cleanup effect that calls closeSource.mutate()
won't re-run due to identity changes of the mutation objects and risk leaking
temp audio (openedTempIdRef/tempId).
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:
- Around line 292-312: The drop handler handleDrop currently treats an
INTERNAL_FILE_PATH_MIME drop as a file-viewer open regardless of native file
presence; change the branching to mirror v1 by first checking
event.dataTransfer.files.length === 0 AND internalFilePath (from
getInternalDraggedFilePath(event.dataTransfer)) before deciding to call
onOpenFile (respecting fileDragBehavior/DEFAULT_FILE_DRAG_BEHAVIOR and
fileOpenMode/DEFAULT_FILE_OPEN_MODE). Keep the early returns for connectionState
=== "closed" and empty resolved text (resolveDroppedText), and still focus/paste
via terminalRuntimeRegistry.getTerminal(terminalId) /
terminalRuntimeRegistry.paste when not opening the file viewer.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/components/AudioEditor/AudioEditor.tsx`:
- Around line 208-237: The current useEffect mixes waveform decoding and
initial-range application causing unnecessary re-decodes when hasInitialRange
changes; split it into two effects: 1) keep the decoding effect using
useEffect(..., [audioUrl]) that sets setIsLoadingWaveform, setWaveformError,
decodes via decodeWaveformPeaks(audioUrl, 2400) with the existing cancelled flag
and finally sets setIsLoadingWaveform(false) and setWaveform(data) (but do NOT
touch start/end seconds here), and 2) add a separate effect using useEffect(...,
[waveform, hasInitialRange]) that runs only after waveform is available and when
hasInitialRange is false to call setStartSeconds(0) and
setEndSeconds(Math.min(10, waveform.duration)); this prevents re-running
decodeWaveformPeaks when hasInitialRange toggles while preserving the
initial-range logic tied to setStartSeconds/setEndSeconds.
🪄 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: 1cc41b2d-6a82-4832-8ba7-17e669f86b5e
📒 Files selected for processing (18)
apps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/main/todo-agent/schedule-sync.tsapps/desktop/src/renderer/lib/file-drag.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/components/AudioEditor/AudioEditor.tsxapps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/components/EditCustomRingtoneDialog/EditCustomRingtoneDialog.tsxapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/hooks/useFileDrag.tsapps/desktop/src/shared/constants.tsapps/desktop/test-setup.tspackages/local-db/drizzle/0065_add_file_drag_behavior_setting.sqlpackages/local-db/drizzle/meta/0065_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/schema.tspackages/local-db/src/schema/zod.ts
![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: 3fdb55cb68
ℹ️ 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".
|
自動レビューの指摘は分類して確認しました。今回の対応は以下です。
対応しなかったのは 補足として、PR テンプレート / docstring coverage の自動警告は今回の変更の blocking issue ではないため未対応です。 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab025afaa1
ℹ️ 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".
| !hasNativeFiles && | ||
| internalFilePath && | ||
| (fileDragBehavior ?? DEFAULT_FILE_DRAG_BEHAVIOR) === "open-file-viewer" | ||
| ) { |
There was a problem hiding this comment.
This condition treats any internal sidebar drag as a file and immediately routes it to onOpenFile, but folder rows are also draggable in the Files sidebar (FileTreeItem wires useFileDrag for every entry.path). Dragging a directory into a terminal now opens a file pane that resolves to an "is directory" error and never pastes the path, which regresses the prior drag-to-terminal workflow for directories; this branch should verify the dropped path is a file (or fall back to paste behavior for directories).
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Summary by CodeRabbit
リリースノート