ADFA-3574 | Add drag and drop support for Git repository URLs#1132
ADFA-3574 | Add drag and drop support for Git repository URLs#1132
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds drag-and-drop handling for Git repo URLs and external files, a Git URL parser/normalizer, ViewModel clone-request events, treeview drag initiation and touch handler, file-import utilities/controllers, and related UI strings and an ID. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Fragment
participant DropTarget
participant Parser
participant MainVM
participant CloneFragment
User->>Fragment: Drop text or URI onto view
Fragment->>DropTarget: forward event (attached listener)
DropTarget->>DropTarget: check shouldAcceptDrop()
DropTarget->>Parser: parseGitRepositoryUrl(payload)
alt valid normalized URL
Parser-->>DropTarget: normalized URL
DropTarget->>MainVM: requestCloneRepository(url)
MainVM-->>MainVM: emit cloneRepositoryEvent, set screen
MainVM->>CloneFragment: cloneRepositoryEvent observed
CloneFragment->>CloneFragment: update repoUrl input & validate
else not a repo URL
Parser-->>DropTarget: null
DropTarget-->>Fragment: clear highlight / ignore drop
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.kt (1)
50-53: Factor the dropped/pending URL update into one helper.These two blocks now duplicate the same repo-url propagation path. Keeping that logic in one place makes it less likely the drag/drop flow and the pending-event flow drift apart the next time this screen changes.
♻️ Suggested cleanup
- handleGitUrlDrop { url -> - binding?.repoUrl?.setText(url) - viewModel.onInputChanged(url, binding?.localPath?.text?.toString().orEmpty()) - } + handleGitUrlDrop(::applyRepositoryUrl) @@ - binding?.repoUrl?.setText(trimmedUrl) - viewModel.onInputChanged(trimmedUrl, binding?.localPath?.text?.toString().orEmpty()) + applyRepositoryUrl(trimmedUrl) } } } } } + + private fun applyRepositoryUrl(url: String) { + binding?.repoUrl?.setText(url) + viewModel.onInputChanged(url, binding?.localPath?.text?.toString().orEmpty()) + }Also applies to: 210-218
🤖 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 repo-url propagation into a single helper (e.g., updateRepoUrl(url: String)) and replace both the handleGitUrlDrop callback and the pending-event block with calls to that helper; the helper should set binding?.repoUrl?.setText(url) and call viewModel.onInputChanged(url, binding?.localPath?.text?.toString().orEmpty()) so both flows (handleGitUrlDrop and the other pending/url flow) share the same logic and cannot diverge.app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.kt (1)
220-222: Prefer narrow exception handling.Catching broad
Exceptionmasks potential issues.FileProvider.getUriForFilespecifically throwsIllegalArgumentExceptionwhen the file is outside configured provider paths. Also,e.printStackTrace()is not ideal for production logging.Based on learnings: prefer narrow exception handling that catches only the specific exception type (such as
IllegalArgumentException) instead of a broad catch-all.♻️ Suggested improvement
- } catch (e: Exception) { - e.printStackTrace() + } catch (e: IllegalArgumentException) { + // File is outside FileProvider's configured paths + Log.w(TAG, "Cannot create content URI for file: ${file.path}", e) }🤖 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 220 - 222, Replace the broad catch of Exception around the FileProvider.getUriForFile call with a narrow catch for IllegalArgumentException and remove e.printStackTrace(); instead log the error via the app logger (e.g., Log.e(TAG, "Failed to get Uri for file", e) or the existing logging facility) so you only handle the specific provider-path error and record useful diagnostic information; update the try/catch block in FileTreeFragment around the FileProvider.getUriForFile usage accordingly.treeview/src/main/java/com/unnamed/b/atv/view/AndroidTreeView.java (1)
440-451: Minor: Consider adding braces for consistency and readability.The single-line
ifstatements without braces work but reduce readability and can lead to maintenance issues.♻️ Suggested formatting improvement
private void handleNodeClick(TreeNode n) { - if (n.getClickListener() != null) n.getClickListener().onClick(n, n.getValue()); - else if (nodeClickListener != null) nodeClickListener.onClick(n, n.getValue()); - if (enableAutoToggle) toggleNode(n); + if (n.getClickListener() != null) { + n.getClickListener().onClick(n, n.getValue()); + } else if (nodeClickListener != null) { + nodeClickListener.onClick(n, n.getValue()); + } + if (enableAutoToggle) { + toggleNode(n); + } } private boolean handleNodeLongClick(TreeNode n) { - if (n.getLongClickListener() != null) return n.getLongClickListener().onLongClick(n, n.getValue()); - if (nodeLongClickListener != null) return nodeLongClickListener.onLongClick(n, n.getValue()); - if (enableAutoToggle) toggleNode(n); + if (n.getLongClickListener() != null) { + return n.getLongClickListener().onLongClick(n, n.getValue()); + } + if (nodeLongClickListener != null) { + return nodeLongClickListener.onLongClick(n, n.getValue()); + } + if (enableAutoToggle) { + toggleNode(n); + } return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@treeview/src/main/java/com/unnamed/b/atv/view/AndroidTreeView.java` around lines 440 - 451, Update handleNodeClick and handleNodeLongClick to use braces for all single-line if statements to improve consistency and readability: wrap the bodies of the checks of n.getClickListener(), nodeClickListener, n.getLongClickListener(), nodeLongClickListener, and the enableAutoToggle check that calls toggleNode(n) in braces, preserving current logic and return values (i.e., return the long-click listener result when present, otherwise proceed and ultimately return false).
🤖 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/GitUrlDropTarget.kt`:
- Around line 46-50: The current flow uses extractDroppedText to take the first
non-blank clip item and then parseGitRepositoryUrl, which can fail if an earlier
clip entry is a label or content URI; update the logic in GitUrlDropTarget so
you iterate through all ClipData items (or all values returned by
extractDroppedText) and call parseGitRepositoryUrl on each until one returns a
non-null repositoryUrl, then call onRepositoryDropped(repositoryUrl) and return
null; ensure you preserve the existing early-returns only after exhausting all
items and apply the same change for the similar block referenced at lines
102-109.
In
`@app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.kt`:
- Around line 206-208: Replace the URL-based extension extraction with the file
object's extension property: instead of using
MimeTypeMap.getFileExtensionFromUrl(file.name) in FileTreeFragment (where
extension and mimeType are computed), use file.extension to get the extension
string and then pass that to
MimeTypeMap.getSingleton().getMimeTypeFromExtension(...) (fall back to
"application/octet-stream" as before); this ensures correct handling of
filenames with spaces or special characters.
In `@git-core/src/main/java/com/itsaky/androidide/git/core/GitRepositoryUrls.kt`:
- Around line 6-7: The SCP-style SSH regex special-cases the literal "git@"
(sshGitUrlRegex) so inputs like "alice@host:repo.git" are missed; update
sshGitUrlRegex to accept any non-space user identifier before the @ (e.g.,
replace the fixed "git@" with a generic "[^\\s@]+@") so SCP-form user@host:path
and ssh://user@host/... behave consistently; verify any other SCP checks in the
same file (the other occurrences you noted around lines 15-17) are changed to
use the same generalized pattern and keep supportedGitSchemes unchanged.
- Around line 35-37: The current heuristic (pathSegments.size >= 2 ||
path.endsWith(".git")) is too permissive and accepts URLs with extra path parts
or query strings (e.g., /org/repo/tree/..., ?tab=...), so update the check in
GitRepositoryUrls.kt (variables: path, pathSegments, looksLikeRepositoryPath) to
require no query and a canonical repo path: require uri.query.isNullOrBlank() &&
(pathSegments.size == 2 || path.endsWith(".git")), which ensures only bare
"org/repo" or explicit ".git" repo URLs enable cloning; adjust the same logic
used in the other block referenced (lines ~43-52) for consistency.
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.kt`:
- Around line 50-53: Extract the duplicated repo-url propagation into a single
helper (e.g., updateRepoUrl(url: String)) and replace both the handleGitUrlDrop
callback and the pending-event block with calls to that helper; the helper
should set binding?.repoUrl?.setText(url) and call viewModel.onInputChanged(url,
binding?.localPath?.text?.toString().orEmpty()) so both flows (handleGitUrlDrop
and the other pending/url flow) share the same logic and cannot diverge.
In
`@app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.kt`:
- Around line 220-222: Replace the broad catch of Exception around the
FileProvider.getUriForFile call with a narrow catch for IllegalArgumentException
and remove e.printStackTrace(); instead log the error via the app logger (e.g.,
Log.e(TAG, "Failed to get Uri for file", e) or the existing logging facility) so
you only handle the specific provider-path error and record useful diagnostic
information; update the try/catch block in FileTreeFragment around the
FileProvider.getUriForFile usage accordingly.
In `@treeview/src/main/java/com/unnamed/b/atv/view/AndroidTreeView.java`:
- Around line 440-451: Update handleNodeClick and handleNodeLongClick to use
braces for all single-line if statements to improve consistency and readability:
wrap the bodies of the checks of n.getClickListener(), nodeClickListener,
n.getLongClickListener(), nodeLongClickListener, and the enableAutoToggle check
that calls toggleNode(n) in braces, preserving current logic and return values
(i.e., return the long-click listener result when present, otherwise proceed and
ultimately return false).
🪄 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: 6c35082d-2309-4ced-94b5-fa6768089b59
📒 Files selected for processing (11)
app/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/FileTreeFragment.ktapp/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.ktapp/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.ktgit-core/src/main/java/com/itsaky/androidide/git/core/GitRepositoryUrls.kttreeview/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/GitUrlDropTarget.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.kt
Outdated
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
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/fragments/sidebar/FileTreeDropController.kt`:
- Around line 167-173: showDropHighlight replaces the view background and
clearDropHighlight nulls it, which removes row ripples; fix by saving the
original background in showDropHighlight (e.g. on the View via setTag with a
unique key) before applying the semi-transparent highlight, and in
clearDropHighlight restore that saved background (retrieving the tag) instead of
setting view.background = null; update showDropHighlight and clearDropHighlight
to use the same tag key and only set the saved background back when present.
- Around line 41-45: The drag handler is treating any non-empty drag as
importable via hasDroppedContent(), which incorrectly advertises self-drops
(URIs created by FileTreeFragment.onStartDrag) as valid; update handleDragEvent
(and the other similar checks around the file-tree drop logic) to inspect the
DragEvent/ClipData and exclude URIs that belong to our own FileProvider (or
other app-owned authorities) before returning true—i.e., compute canHandleDrop
by verifying there is at least one valid external/importable item and filtering
out any clip items whose URI authority equals our FileProvider/own-package
authority so self-drops are not highlighted and are not passed into
copyDroppedFiles().
- Around line 130-132: Sanitize the untrusted filename returned by
uri.getFileName(context) before passing it to createAvailableTargetFile/create
target File to prevent path traversal; compute sourceName by applying the
existing blank fallback, then strip or replace path separators ('/' and '\') and
any parent-segment patterns (".." or patterns like "../" or "/..") (e.g.,
replace separators with '_' and remove or neutralize leading/trailing ".."
segments) so that File(targetDirectory, sourceName) cannot escape the intended
directory; apply the same sanitization wherever the filename is used (the code
paths around createAvailableTargetFile and the File(directory, originalName)
usage noted in the diff).
In
`@app/src/main/java/com/itsaky/androidide/tasks/callables/FileTreeCallable.java`:
- Around line 52-58: populateChildren is called with the result of
File.listFiles(), which may return null and cause Arrays.sort to throw a
NullPointerException; add a null guard at the start of populateChildren(File[]
files, TreeNode parent) to return immediately (or skip sorting/processing) when
files is null or empty before calling Arrays.sort with SortFileName and
SortFolder, ensuring the method handles inaccessible/unreadable directories
gracefully.
In `@app/src/main/java/com/itsaky/androidide/utils/UriFileImporter.kt`:
- Around line 43-49: getDisplayName currently returns null when
queryDisplayName(contentResolver, uri) yields null for "content" URIs, which
causes downstream misnaming; update getDisplayName to fall back to
uri.lastPathSegment when queryDisplayName(...) is null. Specifically, in
getDisplayName (and the callsite logic relying on OpenableColumns.DISPLAY_NAME),
return queryDisplayName(contentResolver, uri) ?: uri.lastPathSegment for the
"content" branch so callers like PluginManagerViewModel.installPlugin receive a
sensible filename even if DISPLAY_NAME is absent.
🪄 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: 8befafe8-b178-45da-b69d-439702b07a15
📒 Files selected for processing (9)
app/src/main/java/com/itsaky/androidide/adapters/viewholders/FileTreeViewHolder.javaapp/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/UriFileImporter.ktapp/src/main/java/com/itsaky/androidide/viewmodels/PluginManagerViewModel.ktresources/src/main/res/values/ids.xmlresources/src/main/res/values/strings.xmltreeview/src/main/java/com/unnamed/b/atv/view/NodeTouchHandler.java
✅ Files skipped from review due to trivial changes (2)
- resources/src/main/res/values/ids.xml
- resources/src/main/res/values/strings.xml
app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/tasks/callables/FileTreeCallable.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/utils/UriFileImporter.kt
Outdated
Show resolved
Hide resolved
19b7dfc to
d21fcec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
treeview/src/main/java/com/unnamed/b/atv/view/AndroidTreeView.java (1)
148-150: Resolve the default drag listener at gesture time.
NodeTouchHandlerstores whatevernodeDragListeneris passed here, sosetDefaultNodeDragListener(...)only affects nodes bound afterward. Using a lookup instead of a snapshot would make drag listener updates behave like click and long-click.Also applies to: 426-426
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@treeview/src/main/java/com/unnamed/b/atv/view/AndroidTreeView.java` around lines 148 - 150, setDefaultNodeDragListener currently stores a snapshot into NodeTouchHandler so only nodes bound after the call see updates; change NodeTouchHandler to resolve the listener at gesture time by looking up AndroidTreeView.nodeDragListener (e.g., call a getter on AndroidTreeView or reference the enclosing AndroidTreeView instance) instead of using the captured listener reference created during binding. Update occurrences where NodeTouchHandler captures the listener (including the similar case around the other occurrence noted) so drag behavior reflects subsequent setDefaultNodeDragListener calls.
🤖 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/fragments/sidebar/FileTreeFragment.kt`:
- Around line 242-244: Replace the broad catch in FileTreeFragment.kt with a
narrow one that only handles the expected drag-start error type (e.g., catch (e:
IllegalArgumentException)), remove the e.printStackTrace() call, and route the
error into the existing flashError(...) path (e.g., flashError(e.message ?:
"Failed to start drag")); do not swallow other exceptions—let them propagate by
not catching Exception broadly.
In `@treeview/src/main/java/com/unnamed/b/atv/view/NodeTouchHandler.java`:
- Around line 39-41: The drag is being started immediately on the first
ACTION_MOVE while isAwaitingDrag is true; instead, measure the movement delta
from the initial down coordinates and only call dispatchDrag() once the distance
exceeds the system touch slop. In NodeTouchHandler store the initial touch X/Y
on ACTION_DOWN, get the threshold via
ViewConfiguration.get(context).getScaledTouchSlop(), and in the ACTION_MOVE
branch (the block that checks isAwaitingDrag and calls dispatchDrag()) replace
the immediate dispatch with a check comparing the squared distance to the slop
(or a simple abs dx/dy check) and only clear isAwaitingDrag and call
dispatchDrag() when exceeded; apply the same change to the second similar block
referenced (the other isAwaitingDrag/ACTION_MOVE handling).
---
Nitpick comments:
In `@treeview/src/main/java/com/unnamed/b/atv/view/AndroidTreeView.java`:
- Around line 148-150: setDefaultNodeDragListener currently stores a snapshot
into NodeTouchHandler so only nodes bound after the call see updates; change
NodeTouchHandler to resolve the listener at gesture time by looking up
AndroidTreeView.nodeDragListener (e.g., call a getter on AndroidTreeView or
reference the enclosing AndroidTreeView instance) instead of using the captured
listener reference created during binding. Update occurrences where
NodeTouchHandler captures the listener (including the similar case around the
other occurrence noted) so drag behavior reflects subsequent
setDefaultNodeDragListener calls.
🪄 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: 416e203c-8364-405e-bb69-1df945211c92
📒 Files selected for processing (18)
app/src/main/java/com/itsaky/androidide/adapters/viewholders/FileTreeViewHolder.javaapp/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/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
✅ Files skipped from review due to trivial changes (4)
- resources/src/main/res/values/ids.xml
- app/src/main/java/com/itsaky/androidide/fragments/MainFragment.kt
- app/src/main/java/com/itsaky/androidide/utils/UriFileImporter.kt
- resources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (10)
- app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.kt
- app/src/main/java/com/itsaky/androidide/dnd/GitUrlDropExtensions.kt
- app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt
- app/src/main/java/com/itsaky/androidide/viewmodels/PluginManagerViewModel.kt
- app/src/main/java/com/itsaky/androidide/tasks/callables/FileTreeCallable.java
- app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt
- app/src/main/java/com/itsaky/androidide/adapters/viewholders/FileTreeViewHolder.java
- app/src/main/java/com/itsaky/androidide/dnd/GitUrlDropTarget.kt
- git-core/src/main/java/com/itsaky/androidide/git/core/GitRepositoryUrls.kt
- app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt
app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.kt
Outdated
Show resolved
Hide resolved
treeview/src/main/java/com/unnamed/b/atv/view/NodeTouchHandler.java
Outdated
Show resolved
Hide resolved
d21fcec to
8befaec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt (1)
151-159:⚠️ Potential issue | 🟠 MajorDelete the half-written target before returning an import error.
The partial-success flow is better now, but the item that throws can still leave a truncated file behind. On retry,
createAvailableTargetFile()will pickname (1)and the broken original stays in the tree.💡 Minimal cleanup
} catch (e: Exception) { + destinationFile.delete() return if (importedCount > 0) { ImportResult.PartialSuccess(importedCount, e) } else { ImportResult.Failure(e) } }🤖 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 151 - 159, On import failure ensure any partially-written target is removed: in the try/catch around UriFileImporter.copyUriToFile (the call that uses destinationFile created by createAvailableTargetFile()), delete destinationFile if it exists before rethrowing or propagating the error; this guarantees a retry won't see a truncated file (i.e., in the catch for Exception check destinationFile.exists() and delete it, then rethrow or handle the original exception).
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.kt (1)
50-53: Remove duplicateonInputChangeddispatch aftersetText.Line 59 already updates ViewModel via
repoUrl.doAfterTextChanged. CallingviewModel.onInputChanged(...)again in both handlers causes duplicate parsing/state updates for the same input.♻️ Proposed cleanup
handleGitUrlDrop { url -> binding?.repoUrl?.setText(url) - viewModel.onInputChanged(url, binding?.localPath?.text?.toString().orEmpty()) } @@ mainViewModel.cloneRepositoryEvent.collect { url -> val trimmedUrl = url.trim() if (trimmedUrl.isNotBlank()) { binding?.repoUrl?.setText(trimmedUrl) - viewModel.onInputChanged(trimmedUrl, binding?.localPath?.text?.toString().orEmpty()) } }Also applies to: 216-217
🤖 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, The drop handlers call viewModel.onInputChanged immediately after setting the EditText text, causing duplicate updates because repoUrl.doAfterTextChanged already dispatches onInputChanged; remove the explicit viewModel.onInputChanged(...) calls from the handlers (e.g., in handleGitUrlDrop where binding?.repoUrl?.setText(url) is followed by viewModel.onInputChanged(...), and the analogous second handler mentioned at lines 216-217) so that setText triggers the existing doAfterTextChanged flooring the ViewModel update only once.
🤖 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/fragments/sidebar/FileTreeFragment.kt`:
- Around line 226-241: The call to ViewCompat.startDragAndDrop(...) in
FileTreeFragment currently ignores its boolean return value; update the
drag-start logic to capture the returned boolean (e.g., val started =
ViewCompat.startDragAndDrop(...)) and handle the failure case by showing user
feedback and/or logging (for example, show a Toast or call process logger) when
started is false; keep the existing ClipData, shadow, flags and error handling
around FileProvider.getUriForFile and MimeTypeMap, but ensure you react to a
failed startDrag in the same try block (or immediately after) so users are
informed if the drag could not be initiated.
- Around line 71-85: createFileNode() and code paths that run on a background
thread are reading fragment-scoped properties (Fragment.context /
requireActivity() via the externalDropHandler getter that uses dropController)
which can be null after onDestroyView(), so before dispatching any async listing
capture the needed references on the main thread (e.g., val context =
requireContext()/requireActivity(), val handler = externalDropHandler or
snapshot dropController.nodeBinder) and pass those snapshots into the worker
lambda; update all similar call sites including the createFileNode invocation
and the other spots noted (lines ~175–206) to use the captured values instead of
accessing externalDropHandler, dropController, or fragment context from the
background thread.
---
Duplicate comments:
In
`@app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt`:
- Around line 151-159: On import failure ensure any partially-written target is
removed: in the try/catch around UriFileImporter.copyUriToFile (the call that
uses destinationFile created by createAvailableTargetFile()), delete
destinationFile if it exists before rethrowing or propagating the error; this
guarantees a retry won't see a truncated file (i.e., in the catch for Exception
check destinationFile.exists() and delete it, then rethrow or handle the
original exception).
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.kt`:
- Around line 50-53: The drop handlers call viewModel.onInputChanged immediately
after setting the EditText text, causing duplicate updates because
repoUrl.doAfterTextChanged already dispatches onInputChanged; remove the
explicit viewModel.onInputChanged(...) calls from the handlers (e.g., in
handleGitUrlDrop where binding?.repoUrl?.setText(url) is followed by
viewModel.onInputChanged(...), and the analogous second handler mentioned at
lines 216-217) so that setText triggers the existing doAfterTextChanged flooring
the ViewModel update only once.
🪄 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: 8baf483e-50b8-4a5a-b62e-446693cffdf3
📒 Files selected for processing (18)
app/src/main/java/com/itsaky/androidide/adapters/viewholders/FileTreeViewHolder.javaapp/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/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
✅ Files skipped from review due to trivial changes (2)
- resources/src/main/res/values/ids.xml
- app/src/main/java/com/itsaky/androidide/fragments/MainFragment.kt
🚧 Files skipped from review as they are similar to previous changes (9)
- app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt
- app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt
- treeview/src/main/java/com/unnamed/b/atv/model/TreeNode.java
- resources/src/main/res/values/strings.xml
- app/src/main/java/com/itsaky/androidide/dnd/GitUrlDropExtensions.kt
- app/src/main/java/com/itsaky/androidide/viewmodels/PluginManagerViewModel.kt
- git-core/src/main/java/com/itsaky/androidide/git/core/GitRepositoryUrls.kt
- app/src/main/java/com/itsaky/androidide/dnd/GitUrlDropTarget.kt
- treeview/src/main/java/com/unnamed/b/atv/view/NodeTouchHandler.java
app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (6)
app/src/main/java/com/itsaky/androidide/utils/DropHighlighter.kt (1)
13-19: Consider extracting the alpha value and sentinel string as named constants.The magic number
64for alpha opacity and the sentinel string"NULL_BG"could be extracted to named constants for improved readability and to prevent potential typos if this sentinel value is ever referenced elsewhere.♻️ Suggested refactor
object DropHighlighter { + private const val NULL_BG_SENTINEL = "NULL_BG" + private const val HIGHLIGHT_ALPHA = 64 // ~25% opacity (64/255) + /** * Applies a highlight background to the [view] to indicate an active drop target, saving its original background. */ 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.background ?: "NULL_BG") + view.setTag(R.id.filetree_drop_target_tag, view.background ?: NULL_BG_SENTINEL) } val baseColor = ContextCompat.getColor(context, R.color.teal_200) - view.setBackgroundColor((baseColor and 0x00FFFFFF) or (64 shl 24)) + view.setBackgroundColor((baseColor and 0x00FFFFFF) or (HIGHLIGHT_ALPHA shl 24)) }🤖 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 13 - 19, Extract the magic alpha value and the sentinel string into named constants to improve readability and avoid typos: define a private constant (e.g., DROP_HIGHLIGHT_ALPHA = 64) and another for the sentinel (e.g., NULL_BACKGROUND_SENTINEL = "NULL_BG") and replace the literal 64 and "NULL_BG" usages in the highlight(view: View, context: Context) function where you set the tag for R.id.filetree_drop_target_tag and compute the background color; keep all other behavior unchanged.app/src/main/java/com/itsaky/androidide/dnd/DragEventRouter.kt (2)
33-45:onDragExitedcalled twice per successful drop or exit sequence.In the current implementation:
- Drop flow:
ACTION_DROP(line 34) →ACTION_DRAG_ENDED(line 43) = 2 calls- Exit flow:
ACTION_DRAG_EXITED(line 29) →ACTION_DRAG_ENDED(line 43) = 2 callsThis ensures cleanup always occurs (defensive), but requires callbacks to be idempotent. Looking at
FileTreeDropController(context snippet 2),DropHighlighter.clear(view)appears safe to call multiple times.If you'd prefer to avoid redundant calls, consider tracking whether exit was already signaled. Otherwise, document that
onDragExitedmay be invoked multiple times per drag session.🤖 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 33 - 45, The code calls callback.onDragExited twice for a single drag sequence (once in the ACTION_DROP branch and again in ACTION_DRAG_ENDED), so add simple state to prevent duplicate exit notifications: introduce a boolean flag (e.g., exitSignaled) on DragEventRouter that is cleared at the start of a drag and set when you call callback.onDragExited, then only call onDragExited in the ACTION_DROP, ACTION_DRAG_EXITED and ACTION_DRAG_ENDED handlers if exitSignaled is false; ensure the flag is reset when a new drag begins so subsequent drags behave normally.
19-31: Asymmetric callback invocation betweenonDragEnteredandonDragExited.
onDragEnteredis only called whencanHandleistrue(line 20), butonDragExitedis called unconditionally (line 29). IfcanAcceptDropreturns different values across events (e.g.,trueforACTION_DRAG_STARTEDbutfalseforACTION_DRAG_ENTERED), this could result inonDragExitedbeing called without a correspondingonDragEntered.Consider guarding
onDragExitedsimilarly, or documenting that callbacks must be idempotent and tolerate unpaired exit calls.♻️ Optional: Make exit calls symmetric with enter
DragEvent.ACTION_DRAG_EXITED -> { - callback.onDragExited(view) + if (canHandle) callback.onDragExited(view) 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/DragEventRouter.kt` around lines 19 - 31, The onDragExited callback is invoked unconditionally while onDragEntered is only called when canHandle is true, which can produce unpaired exit events; update the DragEventRouter handler so ACTION_DRAG_EXITED also checks the same condition (canHandle / result of canAcceptDrop) before calling callback.onDragExited(view) to make enter/exit symmetric, or alternatively add a comment in the DragEventRouter documenting that callback implementations must be idempotent and tolerate unpaired exits (referencing callback.onDragEntered, callback.onDragExited, and the canHandle / canAcceptDrop check).app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.kt (2)
297-300: Use consistent context access for both tree nodes.Line 297 uses
requireContext()while line 300 uses nullablecontext. For consistency and safety, userequireContext()on both lines or capture the context once and reuse it.♻️ Suggested fix
+ val ctx = requireContext() val rootNode = TreeNode(File("")) - rootNode.viewHolder = FileTreeViewHolder(requireContext(), externalDropHandler) + rootNode.viewHolder = FileTreeViewHolder(ctx, externalDropHandler) val projectRoot = TreeNode.root(projectDir) - projectRoot.viewHolder = FileTreeViewHolder(context, externalDropHandler) + projectRoot.viewHolder = FileTreeViewHolder(ctx, externalDropHandler)🤖 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 297 - 300, The two TreeNode viewHolder assignments use inconsistent context access (rootNode.viewHolder uses requireContext() while projectRoot.viewHolder uses nullable context); fix by consistently using requireContext() or by capturing a non-null Context once (e.g., val ctx = requireContext()) and then pass ctx to both FileTreeViewHolder calls (affecting rootNode.viewHolder and projectRoot.viewHolder where FileTreeViewHolder is constructed with projectDir/externalDropHandler).
249-252: Narrow the exception catch to specific expected types.The broad
catch (Exception)hides unrelated bugs and conflicts with the project's fail-fast preference.FileProvider.getUriForFile()typically throwsIllegalArgumentException. Catch only the expected exception type(s) and let others propagate.♻️ Suggested fix
- } catch (e: Exception) { - e.printStackTrace() + } catch (e: IllegalArgumentException) { flashError(getString(R.string.msg_file_tree_drag_error)) }Based on learnings: "prefer narrow exception handling that catches only the specific exception type reported in crashes (such as IllegalArgumentException) instead of a broad catch-all."
🤖 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 249 - 252, The broad catch of Exception in FileTreeFragment (around the FileProvider.getUriForFile() call) should be narrowed to catch only the expected exception type(s) (e.g., IllegalArgumentException) so unrelated errors can propagate; replace "catch (e: Exception) { e.printStackTrace(); flashError(getString(R.string.msg_file_tree_drag_error)) }" with a specific "catch (e: IllegalArgumentException) { /* optional logging */ flashError(getString(R.string.msg_file_tree_drag_error)) }" (remove the printStackTrace) and do not swallow other exceptions so they can surface/fail fast.app/src/main/java/com/itsaky/androidide/dnd/DragAndDropExtensions.kt (1)
36-41: Handle potentialIllegalArgumentExceptionfromtoUri()on malformed text.The
toUri()extension can throw if the text starts withcontent://orfile://but contains malformed URI syntax. Consider wrapping the parse inrunCatchingto gracefully returnnullon invalid input.♻️ Suggested fix
private fun ClipData.Item.toExternalUri(): Uri? { return uri ?: text?.toString() ?.takeIf { it.startsWith("content://") || it.startsWith("file://") } - ?.toUri() + ?.let { runCatching { it.toUri() }.getOrNull() } }🤖 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/DragAndDropExtensions.kt` around lines 36 - 41, In ClipData.Item.toExternalUri(), avoid crashing on malformed text URIs by wrapping the text-to-Uri conversion in a safe parser (e.g., use runCatching or try/catch around text.toString().toUri()) and return null on failure; update the logic inside the toExternalUri() function so it still prefers the existing uri, but when falling back to text that startsWith "content://" or "file://", parse it defensively and return null if parsing throws an IllegalArgumentException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/dnd/DragAndDropExtensions.kt`:
- Around line 36-41: In ClipData.Item.toExternalUri(), avoid crashing on
malformed text URIs by wrapping the text-to-Uri conversion in a safe parser
(e.g., use runCatching or try/catch around text.toString().toUri()) and return
null on failure; update the logic inside the toExternalUri() function so it
still prefers the existing uri, but when falling back to text that startsWith
"content://" or "file://", parse it defensively and return null if parsing
throws an IllegalArgumentException.
In `@app/src/main/java/com/itsaky/androidide/dnd/DragEventRouter.kt`:
- Around line 33-45: The code calls callback.onDragExited twice for a single
drag sequence (once in the ACTION_DROP branch and again in ACTION_DRAG_ENDED),
so add simple state to prevent duplicate exit notifications: introduce a boolean
flag (e.g., exitSignaled) on DragEventRouter that is cleared at the start of a
drag and set when you call callback.onDragExited, then only call onDragExited in
the ACTION_DROP, ACTION_DRAG_EXITED and ACTION_DRAG_ENDED handlers if
exitSignaled is false; ensure the flag is reset when a new drag begins so
subsequent drags behave normally.
- Around line 19-31: The onDragExited callback is invoked unconditionally while
onDragEntered is only called when canHandle is true, which can produce unpaired
exit events; update the DragEventRouter handler so ACTION_DRAG_EXITED also
checks the same condition (canHandle / result of canAcceptDrop) before calling
callback.onDragExited(view) to make enter/exit symmetric, or alternatively add a
comment in the DragEventRouter documenting that callback implementations must be
idempotent and tolerate unpaired exits (referencing callback.onDragEntered,
callback.onDragExited, and the canHandle / canAcceptDrop check).
In
`@app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.kt`:
- Around line 297-300: The two TreeNode viewHolder assignments use inconsistent
context access (rootNode.viewHolder uses requireContext() while
projectRoot.viewHolder uses nullable context); fix by consistently using
requireContext() or by capturing a non-null Context once (e.g., val ctx =
requireContext()) and then pass ctx to both FileTreeViewHolder calls (affecting
rootNode.viewHolder and projectRoot.viewHolder where FileTreeViewHolder is
constructed with projectDir/externalDropHandler).
- Around line 249-252: The broad catch of Exception in FileTreeFragment (around
the FileProvider.getUriForFile() call) should be narrowed to catch only the
expected exception type(s) (e.g., IllegalArgumentException) so unrelated errors
can propagate; replace "catch (e: Exception) { e.printStackTrace();
flashError(getString(R.string.msg_file_tree_drag_error)) }" with a specific
"catch (e: IllegalArgumentException) { /* optional logging */
flashError(getString(R.string.msg_file_tree_drag_error)) }" (remove the
printStackTrace) and do not swallow other exceptions so they can surface/fail
fast.
In `@app/src/main/java/com/itsaky/androidide/utils/DropHighlighter.kt`:
- Around line 13-19: Extract the magic alpha value and the sentinel string into
named constants to improve readability and avoid typos: define a private
constant (e.g., DROP_HIGHLIGHT_ALPHA = 64) and another for the sentinel (e.g.,
NULL_BACKGROUND_SENTINEL = "NULL_BG") and replace the literal 64 and "NULL_BG"
usages in the highlight(view: View, context: Context) function where you set the
tag for R.id.filetree_drop_target_tag and compute the background color; keep all
other behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86febc2e-474b-4251-9abd-99297c6fae53
📒 Files selected for processing (8)
app/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/fragments/sidebar/FileTreeDropController.ktapp/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeFragment.ktapp/src/main/java/com/itsaky/androidide/utils/DropHighlighter.ktapp/src/main/java/com/itsaky/androidide/utils/FileImporter.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
- app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/src/main/res/values/strings.xml
5f7da2d to
5fb4867
Compare
git-core/src/main/java/com/itsaky/androidide/git/core/GitRepositoryUrls.kt
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/fragments/sidebar/FileTreeDropController.kt
Outdated
Show resolved
Hide resolved
Add reusable DragEventRouter and enable Git URL dropping for repository cloning.
449d472 to
8cb0114
Compare
Description
Implemented a robust and reusable drag-and-drop infrastructure to allow users to drag Git repository URLs directly into the main screen or the clone fragment. This automatically initiates the cloning process, improving the onboarding experience for new projects.
Details
DragEventRouterandDropTargetCallbackto decouple complex AndroidDragEventstates from business logic.DropHighlighterto provide a consistent visual cue (foreground overlay) when a valid drop target is active, preserving the original view state.GitUrlDropTargetandhandleGitUrlDropextensions to safely handle incoming URI and text drops.GitRepositoryUrlsutility to validate and parse multiple Git schemes (HTTP, HTTPS, SSH, Git) while filtering out non-repository URLs.MainViewModelandCloneRepositoryViewModelto process dropped URLs via aChannelandFlow, ensuring a clean transition to the Clone fragment.Quick Git Clone
document_5060103109758420762.mp4
Ticket
ADFA-3574
Parent: ADFA-3125
Observation
This PR serves as the foundational layer for Drag & Drop within the app. File-specific drag-and-drop logic for the File Tree (including
NodeTouchHandlerandFileImporter) has been extracted into a subsequent PR to facilitate a more focused code review.