ADFA-3573 | Implement file drag-and-drop export and import handling#1146
Open
ADFA-3573 | Implement file drag-and-drop export and import handling#1146
Conversation
jatezzz
changed the title
Contributor
|
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 17 minutes and 56 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 (12)
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive drag-and-drop file import system for the sidebar file tree. It adds interfaces for external drop handling, D&D utilities for validating and copying files, a drop controller to wire drop callbacks, updates the file tree fragment to integrate drag-and-drop, and enhances the TreeView library with drag listener support and touch-based drag initiation. Changes
Sequence DiagramsequenceDiagram
actor User
participant Tree as File Tree UI
participant DragStarter as FileDragStarter
participant DragRouter as DragEventRouter
participant Importer as FileImporter
participant FileOps as I/O Operations
participant TreeUpdate as Tree Refresh
User->>Tree: Drag file from tree
Tree->>DragStarter: startDrag(file)
DragStarter->>DragStarter: Validate file existence
DragStarter->>FileOps: Create content:// URI via FileProvider
DragStarter->>Tree: Start system drag
User->>Tree: Drop on tree target
Tree->>DragRouter: onDrag(DROP event)
DragRouter->>DragRouter: Check importable content
DragRouter->>Importer: copyDroppedFiles(clipData, target)
Importer->>FileOps: Extract URIs from ClipData
Importer->>FileOps: Copy each file, handle collisions
Importer-->>DragRouter: ImportResult (Success/Partial/Failure)
DragRouter->>TreeUpdate: onDropCompleted(count)
TreeUpdate->>Tree: Refresh/reload affected nodes
Tree->>User: Updated file tree
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
app/src/main/java/com/itsaky/androidide/utils/DropHighlighter.kt (1)
29-33: Consider type-safe sentinel handling.The current implementation uses a string sentinel
"NULL_FG"mixed withDrawabletypes. While functional since bothhighlightandclearare in the same class, theas? Drawablecast on line 32 would silently returnnullif the tag somehow contains an unexpected type, potentially masking issues.A sealed class or dedicated wrapper could make this more type-safe, though the current approach works for this controlled usage.
♻️ Optional: Type-safe alternative
+private sealed interface SavedForeground { + data object None : SavedForeground + data class Present(val drawable: Drawable) : SavedForeground +} + object DropHighlighter { fun highlight(view: View, context: Context) { if (view.getTag(R.id.filetree_drop_target_tag) == null) { - view.setTag(R.id.filetree_drop_target_tag, view.foreground ?: "NULL_FG") + val saved = view.foreground?.let { SavedForeground.Present(it) } ?: SavedForeground.None + view.setTag(R.id.filetree_drop_target_tag, saved) } // ... } fun clear(view: View) { - val savedFg = view.getTag(R.id.filetree_drop_target_tag) ?: return - view.foreground = if (savedFg == "NULL_FG") null else savedFg as? Drawable + val savedFg = view.getTag(R.id.filetree_drop_target_tag) as? SavedForeground ?: return + view.foreground = when (savedFg) { + is SavedForeground.None -> null + is SavedForeground.Present -> savedFg.drawable + } view.setTag(R.id.filetree_drop_target_tag, null) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/utils/DropHighlighter.kt` around lines 29 - 33, The clear(view: View) method uses a string sentinel "NULL_FG" mixed with Drawable types on the tag; replace this with a type-safe wrapper (e.g., a sealed class or dedicated data class) stored under R.id.filetree_drop_target_tag so the tag always has a known shape, then update both highlight(...) and clear(view: View) to set and read that wrapper instead of a raw string/Drawable, and change the view.foreground assignment to unwrap the Drawable safely from the wrapper (no unsafe/as? casting or silent nulling).app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt (1)
185-221: Broad exception handling in clone operation.The
catch (e: Exception)block handles multiple error scenarios. Based on learnings, the project prefers narrow exception handling. However, this existing block intentionally catches various JGit exceptions (TransportException,UnknownHostException,EOFException) and maps them to appropriate error states.If this pattern is established for clone operations, it may be acceptable. Otherwise, consider catching specific exception types explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt` around lines 185 - 221, The catch-all "catch (e: Exception)" in CloneRepositoryViewModel's clone coroutine should be narrowed: replace it with specific catches for the expected JGit/network exceptions (e.g., catch TransportException to detect UnknownHostException, catch EOFException for connection drops, and catch other known JGit exceptions you rely on) and add a final catch for Throwable that logs/propagates unexpected errors; keep the existing isCloneCancelled check and the mapping to errorResId/errorMessage in those specific catches so behavior remains the same while avoiding broad Exception swallowing.app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt (1)
93-98: Potential race condition between channel send and screen navigation.The
sendcall is launched in a coroutine whilesetScreenexecutes immediately after launching. If the fragment observingcloneRepositoryEventis already visible and collects beforesendcompletes, it might miss the URL. WithChannel.BUFFERED, this is unlikely but theoretically possible under heavy load.Consider awaiting the send or using a different ordering:
♻️ Alternative: ensure URL is sent before navigation
fun requestCloneRepository(url: String) { viewModelScope.launch { cloneRepositoryEventChannel.send(url) + setScreen(SCREEN_CLONE_REPO) } - setScreen(SCREEN_CLONE_REPO) }This ensures the URL is buffered before the screen transition begins.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt` around lines 93 - 98, The current requestCloneRepository launches a coroutine to send the URL to cloneRepositoryEventChannel and then immediately calls setScreen(SCREEN_CLONE_REPO), which can race and let the target fragment miss the event; change the ordering or await the send so the URL is buffered before navigation — e.g., perform cloneRepositoryEventChannel.send(url) synchronously or call setScreen only after the send completes inside the coroutine started in requestCloneRepository (referencing requestCloneRepository, cloneRepositoryEventChannel, setScreen, and SCREEN_CLONE_REPO).app/src/main/java/com/itsaky/androidide/dnd/DragEventRouter.kt (1)
31-48:onDragExitedmay be called multiple times per drag session.The router calls
onDragExitedonACTION_DRAG_EXITED,ACTION_DROP, andACTION_DRAG_ENDED. For a successful drop, this meansonDragExitedis called twice (once for drop, once for ended). This is fine if implementations are idempotent (likeDropHighlighter.clear), but worth documenting inDropTargetCallbackthat implementations should handle repeated calls gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/dnd/DragEventRouter.kt` around lines 31 - 48, Update the DropTargetCallback documentation to state that onDragExited may be invoked multiple times during a single drag session because DragEventRouter calls onDragExited on ACTION_DRAG_EXITED, ACTION_DROP and ACTION_DRAG_ENDED; mention implementations (e.g. DropHighlighter.clear) should be idempotent or otherwise handle repeated calls gracefully and reference the relevant symbols: DragEventRouter, onDragExited, ACTION_DRAG_EXITED, ACTION_DROP, ACTION_DRAG_ENDED, and DropHighlighter.clear so future implementers know to defend against duplicate invocations.app/src/main/java/com/itsaky/androidide/dnd/GitUrlDropTarget.kt (1)
50-52: Potential NPE:event.clipDescriptioncan be null.
DragEvent.getClipDescription()may returnnull(e.g., for certain drag event types likeACTION_DRAG_ENDED). WhileisSupportedDropPayload()handles anullreceiver, thecanAcceptDropis also called duringACTION_DRAG_STARTEDwhereclipDescriptionshould be present. However, defensive coding would be safer.🛡️ Optional: Add explicit null check for clarity
override fun canAcceptDrop(event: DragEvent): Boolean { - return shouldAcceptDrop() && event.clipDescription.isSupportedDropPayload() + return shouldAcceptDrop() && event.clipDescription?.isSupportedDropPayload() == true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/dnd/GitUrlDropTarget.kt` around lines 50 - 52, The call in canAcceptDrop currently assumes event.clipDescription is non-null; make it defensive by checking clipDescription for null before calling isSupportedDropPayload (e.g., ensure event.clipDescription != null && event.clipDescription.isSupportedDropPayload()), keeping the existing shouldAcceptDrop() check; update the canAcceptDrop method to return false when clipDescription is null to avoid an NPE while preserving behavior during ACTION_DRAG_STARTED.app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.kt (1)
50-53: Consider extracting duplicated URL handling logic.Both
handleGitUrlDropcallback (lines 50-53) andobservePendingCloneUrl(lines 216-217) perform the same actions: setting the repository URL text and callingviewModel.onInputChanged(). This could be consolidated into a helper method for maintainability.♻️ Suggested refactor to consolidate URL handling
+ private fun applyRepositoryUrl(url: String) { + val trimmedUrl = url.trim() + if (trimmedUrl.isNotBlank()) { + binding?.repoUrl?.setText(trimmedUrl) + viewModel.onInputChanged(trimmedUrl, binding?.localPath?.text?.toString().orEmpty()) + } + } + override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) setupUI() observePendingCloneUrl() observeViewModel() - handleGitUrlDrop { url -> - binding?.repoUrl?.setText(url) - viewModel.onInputChanged(url, binding?.localPath?.text?.toString().orEmpty()) - } + handleGitUrlDrop { applyRepositoryUrl(it) } } private fun observePendingCloneUrl() { viewLifecycleOwner.lifecycleScope.launch { viewLifecycleOwner.repeatOnLifecycle(Lifecycle.State.STARTED) { mainViewModel.cloneRepositoryEvent.collect { url -> - val trimmedUrl = url.trim() - if (trimmedUrl.isNotBlank()) { - binding?.repoUrl?.setText(trimmedUrl) - viewModel.onInputChanged(trimmedUrl, binding?.localPath?.text?.toString().orEmpty()) - } + applyRepositoryUrl(url) } } } }Also applies to: 210-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.kt` around lines 50 - 53, Extract the duplicated logic that sets the repo URL text and notifies the ViewModel into a single helper like updateRepoUrl(url: String) and call it from both the handleGitUrlDrop callback and the observer in observePendingCloneUrl; specifically, move the binding?.repoUrl?.setText(url) and viewModel.onInputChanged(url, binding?.localPath?.text?.toString().orEmpty()) into updateRepoUrl and replace the two duplicate sites (the lambda passed to handleGitUrlDrop and the observer block in observePendingCloneUrl) to invoke updateRepoUrl(url) instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/dnd/DragAndDropExtensions.kt`:
- Around line 39-43: The toExternalUri() helper currently treats item.text as a
single URI which breaks text/uri-list payloads containing multiple entries;
update ClipData.Item.toExternalUri to parse item.text as a uri-list by splitting
on newlines, trimming entries, filtering out blank lines and comment lines
(lines starting with '#'), and then attempt to parse each remaining line into a
Uri (or accept ones beginning with "content://" or "file://"), returning the
first valid Uri found; apply the same parsing logic to any other similar helper
used for text->uri conversion so multi-entry text/uri-list drops are handled
correctly (refer to the ClipData.Item.toExternalUri function name to locate the
code).
In `@app/src/main/java/com/itsaky/androidide/dnd/DropTargetCallback.kt`:
- Around line 30-34: Fix the KDoc above the onDrop(DragEvent) function by
removing the duplicate asterisk in the return tag: locate the KDoc block for fun
onDrop(event: DragEvent): Boolean and change "* * `@return` True if the drop was
successfully consumed and handled." to "* `@return` True if the drop was
successfully consumed and handled." (ensure the KDoc lines begin with a single
'*' as in standard KDoc formatting).
In `@app/src/main/java/com/itsaky/androidide/utils/FileImporter.kt`:
- Around line 33-35: The failure branches in FileImporter.kt currently construct
raw exceptions (e.g., returning
ImportResult.Failure(IllegalArgumentException("No importable files found")) and
the other read-failure branch around lines 49-54); replace those hard-coded
messages with localized strings by constructing the exception message from the
app string resources (use msg_file_tree_drop_no_files for the "no files" case
and msg_file_tree_drop_read_failed for read-failure), e.g., obtain the localized
text via the appropriate Resources/Context accessor available to FileImporter
and pass context.getString(R.string.msg_file_tree_drop_no_files) or
context.getString(R.string.msg_file_tree_drop_read_failed) into the
IllegalArgumentException (or whatever Throwable you return) so
ImportResult.Failure carries a localized message instead of hard-coded English.
In `@app/src/main/java/com/itsaky/androidide/utils/UriFileImporter.kt`:
- Around line 35-39: In UriFileImporter.kt, the current copy path opens
destinationFile.outputStream() and copies input but if outputStream() or
input.copyTo() throws you must delete the partially written destination file;
wrap the destinationFile.outputStream()/copy logic in a try/catch (or use
runCatching) around the block that calls destinationFile.outputStream() and
input.copyTo(output) and on any exception call destinationFile.delete() (or
safeDelete) before rethrowing the original exception, ensuring the cleanup runs
only when the file was created/modified and preserving the original exception
for callers.
In
`@app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt`:
- Around line 66-76: The Error state for invalid repository URLs is
inconsistent: when parseGitRepositoryUrl(url) returns null the code sets
CloneRepoUiState.Error using the original url variable, but other error paths
use normalizedUrl; update those other error states to use the original
user-supplied url (the url parameter) for consistency. Locate usages of
normalizedUrl in the CloneRepositoryViewModel (references:
parseGitRepositoryUrl, normalizedUrl, _uiState.update, CloneRepoUiState.Error)
and replace normalizedUrl with the original url in error constructions so all
error states present the same user input back to the UI.
In `@git-core/src/main/java/com/itsaky/androidide/git/core/GitRepositoryUrls.kt`:
- Around line 36-44: The current logic in GitRepositoryUrls.kt uses
webUiIndicators and hasWebUiSegments to reject URLs if any path segment matches
a UI keyword, which can falsely reject valid repos like owner or repo named
"tree"; update the check to only consider segments after the owner/repo (use
pathSegments.drop(2)) when computing hasWebUiSegments or require multiple
indicator matches before rejecting; adjust the condition that returns null (the
if that references isExplicitGitUrl, pathSegments.size, hasWebUiSegments, and
uri.query) so it uses the new hasWebUiSegments definition (or multi-match logic)
to avoid rejecting legitimate owner/repo names while preserving UI URL
rejection.
- Line 6: The sshGitUrlRegex currently accepts scp-style SSH URLs with a leading
slash in the path; update the Regex assigned to sshGitUrlRegex so the pattern
disallows a slash immediately after the colon (e.g., use a negative lookahead
like :(?!/) before the path portion). Locate the sshGitUrlRegex declaration and
replace the pattern string with one that includes the (?!/) constraint so URLs
such as git@github.com:/org/repo.git are rejected while valid scp-style URLs are
still accepted.
In `@treeview/src/main/java/com/unnamed/b/atv/view/AndroidTreeView.java`:
- Around line 446-451: handleNodeLongClick currently auto-toggles nodes on
long-click (when enableAutoToggle is true) even during drag gestures; update the
logic so a long-press produced by NodeTouchHandler/GestureDetector.onLongPress
does not trigger toggle when a drag is active. Either make NodeTouchHandler
suppress performLongClick while dragging, or modify handleNodeLongClick to
detect an active drag (e.g., consult the NodeTouchHandler drag state or the
nodeDragListener) and skip toggleNode(n) when a drag is in progress; keep
existing long-click listener behavior
(n.getLongClickListener()/nodeLongClickListener) unchanged.
In `@treeview/src/main/java/com/unnamed/b/atv/view/NodeTouchHandler.java`:
- Around line 55-60: handleMove() currently starts a drag on the first move when
isAwaitingDrag is true; change it to require actual movement beyond a small
threshold before arming the drag: keep isAwaitingDrag true until the motion
delta from the stored initial touch coordinates (e.g.,
initialTouchX/initialTouchY) exceeds a TOUCH_SLOP constant, only then set
isAwaitingDrag = false and call dispatchDrag(); apply the same threshold check
to the code path that arms isAwaitingDrag (the block around lines 93-98) so tiny
finger jitter doesn't trigger a drag.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/dnd/DragEventRouter.kt`:
- Around line 31-48: Update the DropTargetCallback documentation to state that
onDragExited may be invoked multiple times during a single drag session because
DragEventRouter calls onDragExited on ACTION_DRAG_EXITED, ACTION_DROP and
ACTION_DRAG_ENDED; mention implementations (e.g. DropHighlighter.clear) should
be idempotent or otherwise handle repeated calls gracefully and reference the
relevant symbols: DragEventRouter, onDragExited, ACTION_DRAG_EXITED,
ACTION_DROP, ACTION_DRAG_ENDED, and DropHighlighter.clear so future implementers
know to defend against duplicate invocations.
In `@app/src/main/java/com/itsaky/androidide/dnd/GitUrlDropTarget.kt`:
- Around line 50-52: The call in canAcceptDrop currently assumes
event.clipDescription is non-null; make it defensive by checking clipDescription
for null before calling isSupportedDropPayload (e.g., ensure
event.clipDescription != null &&
event.clipDescription.isSupportedDropPayload()), keeping the existing
shouldAcceptDrop() check; update the canAcceptDrop method to return false when
clipDescription is null to avoid an NPE while preserving behavior during
ACTION_DRAG_STARTED.
In
`@app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.kt`:
- Around line 50-53: Extract the duplicated logic that sets the repo URL text
and notifies the ViewModel into a single helper like updateRepoUrl(url: String)
and call it from both the handleGitUrlDrop callback and the observer in
observePendingCloneUrl; specifically, move the binding?.repoUrl?.setText(url)
and viewModel.onInputChanged(url,
binding?.localPath?.text?.toString().orEmpty()) into updateRepoUrl and replace
the two duplicate sites (the lambda passed to handleGitUrlDrop and the observer
block in observePendingCloneUrl) to invoke updateRepoUrl(url) instead.
In `@app/src/main/java/com/itsaky/androidide/utils/DropHighlighter.kt`:
- Around line 29-33: The clear(view: View) method uses a string sentinel
"NULL_FG" mixed with Drawable types on the tag; replace this with a type-safe
wrapper (e.g., a sealed class or dedicated data class) stored under
R.id.filetree_drop_target_tag so the tag always has a known shape, then update
both highlight(...) and clear(view: View) to set and read that wrapper instead
of a raw string/Drawable, and change the view.foreground assignment to unwrap
the Drawable safely from the wrapper (no unsafe/as? casting or silent nulling).
In
`@app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt`:
- Around line 185-221: The catch-all "catch (e: Exception)" in
CloneRepositoryViewModel's clone coroutine should be narrowed: replace it with
specific catches for the expected JGit/network exceptions (e.g., catch
TransportException to detect UnknownHostException, catch EOFException for
connection drops, and catch other known JGit exceptions you rely on) and add a
final catch for Throwable that logs/propagates unexpected errors; keep the
existing isCloneCancelled check and the mapping to errorResId/errorMessage in
those specific catches so behavior remains the same while avoiding broad
Exception swallowing.
In `@app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt`:
- Around line 93-98: The current requestCloneRepository launches a coroutine to
send the URL to cloneRepositoryEventChannel and then immediately calls
setScreen(SCREEN_CLONE_REPO), which can race and let the target fragment miss
the event; change the ordering or await the send so the URL is buffered before
navigation — e.g., perform cloneRepositoryEventChannel.send(url) synchronously
or call setScreen only after the send completes inside the coroutine started in
requestCloneRepository (referencing requestCloneRepository,
cloneRepositoryEventChannel, setScreen, and SCREEN_CLONE_REPO).
🪄 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: d5b060d4-8455-41cd-890d-412413e448af
📒 Files selected for processing (24)
app/src/main/java/com/itsaky/androidide/adapters/viewholders/FileTreeViewHolder.javaapp/src/main/java/com/itsaky/androidide/dnd/DragAndDropExtensions.ktapp/src/main/java/com/itsaky/androidide/dnd/DragEventRouter.ktapp/src/main/java/com/itsaky/androidide/dnd/DropTargetCallback.ktapp/src/main/java/com/itsaky/androidide/dnd/FileDragStarter.ktapp/src/main/java/com/itsaky/androidide/dnd/GitUrlDropExtensions.ktapp/src/main/java/com/itsaky/androidide/dnd/GitUrlDropTarget.ktapp/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.ktapp/src/main/java/com/itsaky/androidide/fragments/MainFragment.ktapp/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.ktapp/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.ktapp/src/main/java/com/itsaky/androidide/tasks/callables/FileTreeCallable.javaapp/src/main/java/com/itsaky/androidide/utils/DropHighlighter.ktapp/src/main/java/com/itsaky/androidide/utils/FileImporter.ktapp/src/main/java/com/itsaky/androidide/utils/UriFileImporter.ktapp/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.ktapp/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.ktapp/src/main/java/com/itsaky/androidide/viewmodels/PluginManagerViewModel.ktgit-core/src/main/java/com/itsaky/androidide/git/core/GitRepositoryUrls.ktresources/src/main/res/values/ids.xmlresources/src/main/res/values/strings.xmltreeview/src/main/java/com/unnamed/b/atv/model/TreeNode.javatreeview/src/main/java/com/unnamed/b/atv/view/AndroidTreeView.javatreeview/src/main/java/com/unnamed/b/atv/view/NodeTouchHandler.java
app/src/main/java/com/itsaky/androidide/dnd/DragAndDropExtensions.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/utils/UriFileImporter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt
Show resolved
Hide resolved
git-core/src/main/java/com/itsaky/androidide/git/core/GitRepositoryUrls.kt
Outdated
Show resolved
Hide resolved
git-core/src/main/java/com/itsaky/androidide/git/core/GitRepositoryUrls.kt
Outdated
Show resolved
Hide resolved
treeview/src/main/java/com/unnamed/b/atv/view/AndroidTreeView.java
Outdated
Show resolved
Hide resolved
jatezzz
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/utils/FileImporter.kt`:
- Around line 24-28: The copyDroppedFiles function currently uses throwing
helpers (require and error) which can escape the ImportResult-based error flow;
change copyDroppedFiles and any helper calls (e.g., resolveTargetDirectory) to
never throw but instead return ImportResult.Failure on error conditions —
replace the require(...) check for targetDirectory existence/dir with an
explicit if that returns ImportResult.Failure(context.getString(...)), and
replace any calls to error(...) (and other throw sites) to return
ImportResult.Failure with a descriptive message or wrapped exception information
so all failure paths use ImportResult.Failure consistently.
🪄 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: 9902c264-ed25-4d75-970f-9d8da332c7af
📒 Files selected for processing (5)
app/src/main/java/com/itsaky/androidide/dnd/DragAndDropExtensions.ktapp/src/main/java/com/itsaky/androidide/dnd/DropTargetCallback.ktapp/src/main/java/com/itsaky/androidide/utils/FileImporter.ktapp/src/main/java/com/itsaky/androidide/utils/UriFileImporter.ktresources/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (2)
- app/src/main/java/com/itsaky/androidide/dnd/DropTargetCallback.kt
- resources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/itsaky/androidide/dnd/DragAndDropExtensions.kt
- app/src/main/java/com/itsaky/androidide/utils/UriFileImporter.kt
Comment on lines
+24
to
+28
| fun copyDroppedFiles(clipData: ClipData, targetFile: File): ImportResult { | ||
| val targetDirectory = resolveTargetDirectory(targetFile) | ||
| require(targetDirectory.exists() && targetDirectory.isDirectory) { | ||
| context.getString(R.string.msg_file_tree_drop_destination_missing) | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid throwing from an ImportResult-based API.
Line 26 (require) and Line 75 (error) can throw and bypass the ImportResult.Failure pathway. This creates inconsistent handling in the drop flow.
💡 Proposed fix
fun copyDroppedFiles(clipData: ClipData, targetFile: File): ImportResult {
- val targetDirectory = resolveTargetDirectory(targetFile)
- require(targetDirectory.exists() && targetDirectory.isDirectory) {
- context.getString(R.string.msg_file_tree_drop_destination_missing)
- }
+ val targetDirectory = resolveTargetDirectory(targetFile)
+ ?: return ImportResult.Failure(
+ IllegalStateException(
+ context.getString(R.string.msg_file_tree_drop_destination_unresolved)
+ )
+ )
+ if (!targetDirectory.exists() || !targetDirectory.isDirectory) {
+ return ImportResult.Failure(
+ IllegalArgumentException(
+ context.getString(R.string.msg_file_tree_drop_destination_missing)
+ )
+ )
+ }
@@
-private fun resolveTargetDirectory(targetFile: File): File {
+private fun resolveTargetDirectory(targetFile: File): File? {
return if (targetFile.isDirectory) {
targetFile
} else {
- targetFile.parentFile
- ?: error(context.getString(R.string.msg_file_tree_drop_destination_unresolved))
+ targetFile.parentFile
}
}Also applies to: 70-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/itsaky/androidide/utils/FileImporter.kt` around lines
24 - 28, The copyDroppedFiles function currently uses throwing helpers (require
and error) which can escape the ImportResult-based error flow; change
copyDroppedFiles and any helper calls (e.g., resolveTargetDirectory) to never
throw but instead return ImportResult.Failure on error conditions — replace the
require(...) check for targetDirectory existence/dir with an explicit if that
returns ImportResult.Failure(context.getString(...)), and replace any calls to
error(...) (and other throw sites) to return ImportResult.Failure with a
descriptive message or wrapped exception information so all failure paths use
ImportResult.Failure consistently.
itsaky-adfa
requested changes
Apr 2, 2026
Contributor
itsaky-adfa
left a comment
itsaky-adfa
left a comment
There was a problem hiding this comment.
- When doing async or I/O work, prefer Kotlin coroutines over legacy APIs in the IDE, or Java threads.
- Please run Spotless to format the changed files. The files are still using 2-space indentation.
app/src/main/java/com/itsaky/androidide/dnd/DragAndDropExtensions.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt
Outdated
Show resolved
Hide resolved
Add gesture-based drag initiation, cross-app file export, and robust URI import handling.
jatezzz
force-pushed
the
feat/ADFA-3125-filetree-drag-and-drop
branch
from
6647bc2 to
ad657d6
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/main/java/com/itsaky/androidide/utils/FileImporter.kt (2)
70-77:⚠️ Potential issue | 🟠 Major
error()throws and escapes theImportResultcontract.
resolveTargetDirectoryuseserror()which throwsIllegalStateException. Combined with the fix above, this should returnnullto allow the caller to handle it viaImportResult.Failure.💡 Proposed fix
-private fun resolveTargetDirectory(targetFile: File): File { +private fun resolveTargetDirectory(targetFile: File): File? { return if (targetFile.isDirectory) { targetFile } else { - targetFile.parentFile - ?: error(context.getString(R.string.msg_file_tree_drop_destination_unresolved)) + targetFile.parentFile } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/utils/FileImporter.kt` around lines 70 - 77, resolveTargetDirectory currently throws via error() which breaks the ImportResult flow; change resolveTargetDirectory to return a nullable File (File?) and replace the error(...) call with returning null when parentFile is absent, and then update the caller of resolveTargetDirectory (the import routine that maps results into ImportResult) to detect a null return and produce ImportResult.Failure instead of relying on an exception; refer to resolveTargetDirectory and ImportResult.Failure when making these changes.
24-28:⚠️ Potential issue | 🟠 MajorAvoid throwing from an
ImportResult-based API.
require()at line 26 throwsIllegalArgumentException, bypassing theImportResult.Failurepathway. Callers expect all errors to be returned viaImportResult, but this path throws instead, creating inconsistent error handling.💡 Proposed fix
fun copyDroppedFiles(clipData: ClipData, targetFile: File): ImportResult { val targetDirectory = resolveTargetDirectory(targetFile) - require(targetDirectory.exists() && targetDirectory.isDirectory) { - context.getString(R.string.msg_file_tree_drop_destination_missing) - } + ?: return ImportResult.Failure( + IllegalStateException( + context.getString(R.string.msg_file_tree_drop_destination_unresolved) + ) + ) + if (!targetDirectory.exists() || !targetDirectory.isDirectory) { + return ImportResult.Failure( + IllegalArgumentException( + context.getString(R.string.msg_file_tree_drop_destination_missing) + ) + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/utils/FileImporter.kt` around lines 24 - 28, The copyDroppedFiles function uses require(...) which throws an IllegalArgumentException and bypasses the ImportResult error path; replace that throwing check with a conditional that returns an ImportResult.Failure (using the same message from context.getString(R.string.msg_file_tree_drop_destination_missing)) when resolveTargetDirectory(targetFile) yields a non-existent or non-directory targetDirectory so callers always receive failures via the ImportResult API rather than exceptions.
🧹 Nitpick comments (4)
app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt (2)
84-89: Silentelsebranch may mask unexpected states.If
resultisnullanderroris alsonull, this branch silently does nothing. Consider logging or handling this unexpected state defensively.💡 Suggested improvement
when (result) { is FileImporter.ImportResult.Success -> handleSuccess(target, result) is FileImporter.ImportResult.PartialSuccess -> handlePartialSuccess(target, result) is FileImporter.ImportResult.Failure -> onDropFailed(result.error.toReadableMessage()) - else -> {} + null -> { + // Unexpected: result was null but no error was reported + onDropFailed(activity.getString(R.string.msg_file_tree_drop_import_failed)) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt` around lines 84 - 89, The when-block in FileTreeDropController.kt handling the FileImporter.ImportResult leaves an empty else that can silently ignore unexpected or null results; update the match to handle the null/unknown case explicitly (e.g., replace the else with a branch that logs the condition and calls onDropFailed with a generic message) so unexpected states don't go unreported—refer to the existing handlers handleSuccess(target, result), handlePartialSuccess(target, result), and onDropFailed(...) when implementing the defensive fallback.
61-67: Consider replacing legacy async API with coroutines.
executeAsyncProvideErrorwas flagged in a past review as a legacy API. Consider migrating to Kotlin coroutines withviewModelScope.launchor a similar scope for better lifecycle management and consistency with other async operations in the codebase.💡 Coroutine-based alternative
// If the fragment exposes a CoroutineScope, use it: scope.launch(Dispatchers.IO) { val result = try { FileImporter(context).copyDroppedFiles(clipData, target.file) } finally { dragPermissions?.release() } withContext(Dispatchers.Main) { handleImportResult(target, result, null) } }.invokeOnCompletion { error -> if (error != null) { activity.runOnUiThread { handleImportResult(target, null, error) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt` around lines 61 - 67, The code currently uses the legacy executeAsyncProvideError; replace it with a coroutine launched from the fragment scope (e.g., viewLifecycleOwner.lifecycleScope.launch(Dispatchers.IO)) so lifecycle is respected. Inside the coroutine call FileImporter(context).copyDroppedFiles(clipData, target.file) and ensure dragPermissions?.release() in a finally block, then switch to Main (withContext(Dispatchers.Main)) to call handleImportResult(target, result, null); catch exceptions and call handleImportResult(target, null, error) on the Main dispatcher. Keep references: replace executeAsyncProvideError, use FileImporter.copyDroppedFiles, dragPermissions?.release, handleImportResult, and viewLifecycleOwner.lifecycleScope.launch (or viewModelScope if more appropriate).app/src/main/java/com/itsaky/androidide/dnd/FileDragStarter.kt (1)
13-23: Consider renaming for clarity.The distinction between
NotStarted(expected validation failures) andFailed(unexpected exceptions) is functionally sound, but the naming could be more explicit. ConsiderValidationFailedvsErroror document the semantic difference.This was noted in a past review comment and appears to be a design discussion point.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/dnd/FileDragStarter.kt` around lines 13 - 23, Rename the sealed variants to make the semantic distinction explicit: change FileDragResult.NotStarted to ValidationFailed (or ValidationError) to indicate expected validation failures tied to FileDragFailureReason, and change FileDragResult.Failed to Error (or Exception) to represent unexpected exceptions; update all usages of FileDragResult, NotStarted, Failed, and FileDragFailureReason accordingly and adjust any docs/comments to reflect the new names so callers clearly see "validation vs runtime error" semantics.app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.kt (1)
73-84: Consider usinglazydelegate for cleaner initialization.The manual backing field pattern could be simplified with Kotlin's
lazydelegate:💡 Simplified version
- private var _dropController: FileTreeDropController? = null - private val dropController: FileTreeDropController - get() { - if (_dropController == null) { - _dropController = FileTreeDropController( - activity = requireActivity(), - onDropCompleted = ::onExternalDropCompleted, - onDropFailed = ::flashError, - ) - } - return _dropController!! - } + private val dropController by lazy(LazyThreadSafetyMode.NONE) { + FileTreeDropController( + activity = requireActivity(), + onDropCompleted = ::onExternalDropCompleted, + onDropFailed = ::flashError, + ) + }Note: With
lazy, you'd need an alternative approach for cleanup inonDestroyViewsincelazydoesn't support resetting. The current manual approach may be intentional if the controller needs to be recreated across view lifecycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.kt` around lines 73 - 84, Replace the manual nullable backing field pattern (_dropController + getter) with a Kotlin lazy-initialized val: change to "private val dropController by lazy { FileTreeDropController(activity = requireActivity(), onDropCompleted = ::onExternalDropCompleted, onDropFailed = ::flashError) }" and remove _dropController and the custom getter; if the controller must be cleared in onDestroyView, instead keep the manual nullable field (or move creation into onViewCreated) — otherwise remove any reset logic in onDestroyView that cleared _dropController so it no longer references a removed property.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/com/itsaky/androidide/utils/FileImporter.kt`:
- Around line 70-77: resolveTargetDirectory currently throws via error() which
breaks the ImportResult flow; change resolveTargetDirectory to return a nullable
File (File?) and replace the error(...) call with returning null when parentFile
is absent, and then update the caller of resolveTargetDirectory (the import
routine that maps results into ImportResult) to detect a null return and produce
ImportResult.Failure instead of relying on an exception; refer to
resolveTargetDirectory and ImportResult.Failure when making these changes.
- Around line 24-28: The copyDroppedFiles function uses require(...) which
throws an IllegalArgumentException and bypasses the ImportResult error path;
replace that throwing check with a conditional that returns an
ImportResult.Failure (using the same message from
context.getString(R.string.msg_file_tree_drop_destination_missing)) when
resolveTargetDirectory(targetFile) yields a non-existent or non-directory
targetDirectory so callers always receive failures via the ImportResult API
rather than exceptions.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/dnd/FileDragStarter.kt`:
- Around line 13-23: Rename the sealed variants to make the semantic distinction
explicit: change FileDragResult.NotStarted to ValidationFailed (or
ValidationError) to indicate expected validation failures tied to
FileDragFailureReason, and change FileDragResult.Failed to Error (or Exception)
to represent unexpected exceptions; update all usages of FileDragResult,
NotStarted, Failed, and FileDragFailureReason accordingly and adjust any
docs/comments to reflect the new names so callers clearly see "validation vs
runtime error" semantics.
In
`@app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt`:
- Around line 84-89: The when-block in FileTreeDropController.kt handling the
FileImporter.ImportResult leaves an empty else that can silently ignore
unexpected or null results; update the match to handle the null/unknown case
explicitly (e.g., replace the else with a branch that logs the condition and
calls onDropFailed with a generic message) so unexpected states don't go
unreported—refer to the existing handlers handleSuccess(target, result),
handlePartialSuccess(target, result), and onDropFailed(...) when implementing
the defensive fallback.
- Around line 61-67: The code currently uses the legacy
executeAsyncProvideError; replace it with a coroutine launched from the fragment
scope (e.g., viewLifecycleOwner.lifecycleScope.launch(Dispatchers.IO)) so
lifecycle is respected. Inside the coroutine call
FileImporter(context).copyDroppedFiles(clipData, target.file) and ensure
dragPermissions?.release() in a finally block, then switch to Main
(withContext(Dispatchers.Main)) to call handleImportResult(target, result,
null); catch exceptions and call handleImportResult(target, null, error) on the
Main dispatcher. Keep references: replace executeAsyncProvideError, use
FileImporter.copyDroppedFiles, dragPermissions?.release, handleImportResult, and
viewLifecycleOwner.lifecycleScope.launch (or viewModelScope if more
appropriate).
In
`@app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.kt`:
- Around line 73-84: Replace the manual nullable backing field pattern
(_dropController + getter) with a Kotlin lazy-initialized val: change to
"private val dropController by lazy { FileTreeDropController(activity =
requireActivity(), onDropCompleted = ::onExternalDropCompleted, onDropFailed =
::flashError) }" and remove _dropController and the custom getter; if the
controller must be cleared in onDestroyView, instead keep the manual nullable
field (or move creation into onViewCreated) — otherwise remove any reset logic
in onDestroyView that cleared _dropController so it no longer references a
removed property.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5fc37021-f70e-45b9-a4a4-2d621829569b
📒 Files selected for processing (14)
app/src/main/java/com/itsaky/androidide/adapters/viewholders/FileTreeViewHolder.javaapp/src/main/java/com/itsaky/androidide/dnd/DragAndDropExtensions.ktapp/src/main/java/com/itsaky/androidide/dnd/DropTargetCallback.ktapp/src/main/java/com/itsaky/androidide/dnd/FileDragStarter.ktapp/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.ktapp/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.ktapp/src/main/java/com/itsaky/androidide/tasks/callables/FileTreeCallable.javaapp/src/main/java/com/itsaky/androidide/utils/FileImporter.ktapp/src/main/java/com/itsaky/androidide/utils/UriFileImporter.ktapp/src/main/java/com/itsaky/androidide/viewmodels/PluginManagerViewModel.ktresources/src/main/res/values/strings.xmltreeview/src/main/java/com/unnamed/b/atv/model/TreeNode.javatreeview/src/main/java/com/unnamed/b/atv/view/AndroidTreeView.javatreeview/src/main/java/com/unnamed/b/atv/view/NodeTouchHandler.java
✅ Files skipped from review due to trivial changes (3)
- app/src/main/java/com/itsaky/androidide/dnd/DropTargetCallback.kt
- resources/src/main/res/values/strings.xml
- app/src/main/java/com/itsaky/androidide/dnd/DragAndDropExtensions.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/com/itsaky/androidide/tasks/callables/FileTreeCallable.java
- treeview/src/main/java/com/unnamed/b/atv/view/AndroidTreeView.java
- app/src/main/java/com/itsaky/androidide/utils/UriFileImporter.kt
itsaky-adfa
approved these changes
Apr 3, 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
2 participants
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.
Description
Enables full bidirectional drag-and-drop support for the FileTree. Users can now export files by dragging them from the IDE into external apps, and import files by dragging them from external sources directly into the IDE. This includes a robust import/export system that safely manages cross-app URI permissions, file copying, and name conflicts.
Details
ClipDataandFileProvidergeneration into a dedicatedFileDragStarterclass. This safely grants external apps read permissions usingDRAG_FLAG_GLOBALandDRAG_FLAG_GLOBAL_URI_READ.NodeTouchHandlertoAndroidTreeViewto support a double-tap-to-drag gesture, avoiding conflicts with standard vertical scrolling and click listeners.FileImporterto manage file copying, directory resolution, and automatic renaming for duplicate files.UriFileImporterto safely abstractContentResolverI/O operations and refactoredPluginManagerViewModelto utilize it.FileTreeDropControllerto coordinate drop targets, validate MIME types viaDragAndDropExtensions, and manage visual feedback through the core drag router.File import
document_5068910296681154105.mp4
File export
document_5068910296681154104.mp4
Ticket
ADFA-3573
Parent: ADFA-3125
Observation
This PR is the second and final part of the Drag and Drop feature implementation. It directly depends on the core DND infrastructure (such as
DragEventRouterandDropHighlighter) introduced in the previous PR (#1132).