ADFA-3074: Clone error handling#1064
Merged
dara-abijo-adfa merged 11 commits intostagefrom Mar 20, 2026
Merged
Conversation
dara-abijo-adfa
requested review from
Daniel-ADFA,
itsaky-adfa,
jatezzz and
jomen-adfa
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.kt (1)
165-182:⚠️ Potential issue | 🟡 MinorMissing
retryButtonvisibility handling in Success state.The
retryButtonvisibility is set in Idle, Cloning, and Error states but not in Success. While the Success state quickly transitions away, explicitly hiding it ensures consistent UI state.🛡️ Suggested fix
is CloneRepoUiState.Success -> { cloneButton.isEnabled = true + retryButton.visibility = View.GONE statusText.text = getString(R.string.clone_successful)🤖 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 165 - 182, In the CloneRepositoryFragment's handling of is CloneRepoUiState.Success, explicitly hide the retryButton (e.g., set its visibility to GONE or isVisible = false) before or when you reset the UI and call viewModel.resetState so the button state stays consistent with Idle/Cloning/Error branches; update the Success branch where cloneButton, statusText, destDir check, and viewModel.resetState() are handled to include hiding retryButton.
🧹 Nitpick comments (4)
resources/src/main/res/values/strings.xml (1)
1170-1177: Minor casing inconsistency indesc_file_conflicted.The string "Conflicted File" uses title case, while all other
desc_file_*strings use sentence case (e.g., "File added", "File modified"). Consider aligning the format for consistency.💅 Suggested fix
- <string name="desc_file_conflicted">Conflicted File</string> + <string name="desc_file_conflicted">File conflicted</string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/src/main/res/values/strings.xml` around lines 1170 - 1177, The string resource desc_file_conflicted is inconsistent (uses "Conflicted File" title case) — update its value to match the other desc_file_* entries by using the same sentence/phrase form; specifically change the value for string name="desc_file_conflicted" to "File conflicted" so it aligns with "File added", "File modified", etc.app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt (1)
163-165: String-based error detection is fragile but pragmatic.The message matching for
"Unexpected end of stream"and"Software caused connection abort"may differ across JGit versions and platforms. However, this is already preceded by anEOFExceptiontype check. If targeting more robustness, consider traversing the exception cause chain to check forSocketExceptionor otherIOExceptionsubtypes before falling back to message matching. Note that similar string-based matching patterns exist elsewhere in the codebase (e.g., WebServer.kt).🤖 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 163 - 165, Replace the fragile message-based detection in the isConnectionDrop logic by walking the exception cause chain (starting from the caught exception e) and checking for specific IO-related types like EOFException, SocketException or other IOException subclasses before falling back to string matching; update the check that currently uses e.cause and e.message to iterate through causes and return true on any matching exception type (refer to isConnectionDrop and the caught exception variable e) so the code is robust across JGit versions/platforms while retaining the existing message checks as a last resort.app/src/main/res/layout/fragment_clone_repository.xml (2)
112-146: Minor: Inconsistent ID naming convention.The button IDs mix naming styles:
exit_button(snake_case)retryButton(camelCase)cloneButton(camelCase)While View Binding handles
exit_buttoncorrectly (converts toexitButton), consistent naming improves maintainability. Consider standardizing on camelCase (exitButton) to match the others.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/layout/fragment_clone_repository.xml` around lines 112 - 146, Rename the inconsistent id exit_button to camelCase exitButton in the layout and update all usages in code to match (replace R.id.exit_button or any findViewById/reference and any viewBinding usages) so that ids for retryButton, cloneButton, and exitButton use the same naming convention; after changing the id, rebuild/run to fix any lingering references or generated binding names.
61-68: Consider using MaterialCheckBox for theme consistency.The layout uses Material3 styles throughout (
MaterialTextView,MaterialButton, MaterialTextInputLayout), but this CheckBox is the standard Android widget. Usingcom.google.android.material.checkbox.MaterialCheckBoxwould provide consistent theming and ripple behavior.♻️ Suggested change
- <CheckBox + <com.google.android.material.checkbox.MaterialCheckBox android:id="@+id/authCheckbox" android:layout_width="wrap_content" android:layout_height="wrap_content" android:layout_marginTop="16dp" android:text="@string/use_authentication" app:layout_constraintStart_toStartOf="parent" app:layout_constraintTop_toBottomOf="@id/localPathLayout" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/layout/fragment_clone_repository.xml` around lines 61 - 68, Replace the standard CheckBox in fragment_clone_repository.xml (id authCheckbox) with the Material component to match the rest of your Material3 widgets: change the XML tag to com.google.android.material.checkbox.MaterialCheckBox, keep the existing attributes (android:id="@+id/authCheckbox", android:layout_width, android:layout_height, android:layout_marginTop, android:text and constraint attributes) and ensure your app uses the Material components theme and the Material library dependency so the control inherits ripples and theming like MaterialTextView/MaterialButton.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.kt`:
- Around line 165-182: In the CloneRepositoryFragment's handling of is
CloneRepoUiState.Success, explicitly hide the retryButton (e.g., set its
visibility to GONE or isVisible = false) before or when you reset the UI and
call viewModel.resetState so the button state stays consistent with
Idle/Cloning/Error branches; update the Success branch where cloneButton,
statusText, destDir check, and viewModel.resetState() are handled to include
hiding retryButton.
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt`:
- Around line 163-165: Replace the fragile message-based detection in the
isConnectionDrop logic by walking the exception cause chain (starting from the
caught exception e) and checking for specific IO-related types like
EOFException, SocketException or other IOException subclasses before falling
back to string matching; update the check that currently uses e.cause and
e.message to iterate through causes and return true on any matching exception
type (refer to isConnectionDrop and the caught exception variable e) so the code
is robust across JGit versions/platforms while retaining the existing message
checks as a last resort.
In `@app/src/main/res/layout/fragment_clone_repository.xml`:
- Around line 112-146: Rename the inconsistent id exit_button to camelCase
exitButton in the layout and update all usages in code to match (replace
R.id.exit_button or any findViewById/reference and any viewBinding usages) so
that ids for retryButton, cloneButton, and exitButton use the same naming
convention; after changing the id, rebuild/run to fix any lingering references
or generated binding names.
- Around line 61-68: Replace the standard CheckBox in
fragment_clone_repository.xml (id authCheckbox) with the Material component to
match the rest of your Material3 widgets: change the XML tag to
com.google.android.material.checkbox.MaterialCheckBox, keep the existing
attributes (android:id="@+id/authCheckbox", android:layout_width,
android:layout_height, android:layout_marginTop, android:text and constraint
attributes) and ensure your app uses the Material components theme and the
Material library dependency so the control inherits ripples and theming like
MaterialTextView/MaterialButton.
In `@resources/src/main/res/values/strings.xml`:
- Around line 1170-1177: The string resource desc_file_conflicted is
inconsistent (uses "Conflicted File" title case) — update its value to match the
other desc_file_* entries by using the same sentence/phrase form; specifically
change the value for string name="desc_file_conflicted" to "File conflicted" so
it aligns with "File added", "File modified", etc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dcbb7850-fdea-4c82-8c29-611e9d07ba24
📒 Files selected for processing (5)
app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.ktapp/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.ktapp/src/main/res/layout/fragment_clone_repository.xmlapp/src/main/res/values/strings.xmlresources/src/main/res/values/strings.xml
💤 Files with no reviewable changes (1)
- app/src/main/res/values/strings.xml
jomen-adfa
approved these changes
Mar 12, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt (1)
178-191: Inspect the full cause chain before falling back to generic clone errors.Checking only
e.causeand a couple of English message fragments is brittle. JGit/network failures are often nested more deeply, so the same connectivity issue can still land in the genericclone_failedpath. A small helper that walks the cause chain will make the new error mapping much more reliable.🤖 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 178 - 191, The current connectivity checks in CloneRepositoryViewModel (the isNetworkError/isConnectionDrop logic and subsequent errorResId/errorMessage) only inspect e.cause and top-level message fragments, which misses nested causes; implement a helper (e.g., a private function that walks the cause chain for a Throwable) and use it to detect UnknownHostException, EOFException, or any message containing "Unexpected end of stream" or "Software caused connection abort" anywhere in the cause chain, then replace the existing isNetworkError/isConnectionDrop checks to call that helper so errorResId is set reliably before falling back to the generic clone_failed path.app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.kt (1)
150-150:isCancellableisn't driving the UI yet.The new
CloneRepoUiState.Cloning.isCancellablebit is ignored here, so the cancel button is shown for every cloning state regardless of what the ViewModel reports. Either gate visibility/enabled state onstate.isCancellableand populate it, or remove the field to avoid dead state.🤖 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` at line 150, The cancel button visibility currently ignores the new Cloning.isCancellable flag; update the UI in CloneRepositoryFragment (where cancelButton.visibility is set) to only show/enable the button when state is CloneRepoUiState.Cloning AND state.isCancellable is true, and ensure the ViewModel populates CloneRepoUiState.Cloning.isCancellable appropriately during cloning (or remove the unused isCancellable field if you prefer not to use it) so the UI reflects the real cancellability state.
🤖 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/CloneRepositoryFragment.kt`:
- Around line 169-173: The UI currently flips the CTA to "Retry" for every
CloneRepoUiState.Error in CloneRepositoryFragment by calling
cloneButton.refreshStatus(isForRetry = true) inside the is
CloneRepoUiState.Error branch; instead, extend CloneRepoUiState.Error with a
boolean (e.g., retryable) or add a distinct error subtype to indicate whether
the error came from a failed clone attempt versus a local validation issue (like
destination_directory_not_empty), then only call
cloneButton.refreshStatus(isForRetry = true) when that retryable flag is true
(otherwise call refreshStatus with isForRetry = false or another appropriate
state) so the CTA text/icon is only switched for actual retryable clone
failures.
In
`@app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt`:
- Line 30: Replace the plain Boolean isCloneCancelled with a thread-safe flag
(e.g., an AtomicBoolean or a `@Volatile` var) so ProgressMonitor callbacks on the
IO dispatcher observe cancellations reliably, and change updateProgressUI() to
update the state atomically by reading and transforming the current _uiState
(e.g., use _uiState.update { current -> if (current is
CloneRepositoryUiState.Cloning) current.copy(progress = progress) else current }
or a compare-and-set style) so a stale Cloning snapshot cannot overwrite a newer
Error or Idle state; refer to the isCloneCancelled symbol, updateProgressUI(),
and the _uiState / CloneRepositoryUiState.Cloning types to locate where to apply
these changes.
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.kt`:
- Line 150: The cancel button visibility currently ignores the new
Cloning.isCancellable flag; update the UI in CloneRepositoryFragment (where
cancelButton.visibility is set) to only show/enable the button when state is
CloneRepoUiState.Cloning AND state.isCancellable is true, and ensure the
ViewModel populates CloneRepoUiState.Cloning.isCancellable appropriately during
cloning (or remove the unused isCancellable field if you prefer not to use it)
so the UI reflects the real cancellability state.
In
`@app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt`:
- Around line 178-191: The current connectivity checks in
CloneRepositoryViewModel (the isNetworkError/isConnectionDrop logic and
subsequent errorResId/errorMessage) only inspect e.cause and top-level message
fragments, which misses nested causes; implement a helper (e.g., a private
function that walks the cause chain for a Throwable) and use it to detect
UnknownHostException, EOFException, or any message containing "Unexpected end of
stream" or "Software caused connection abort" anywhere in the cause chain, then
replace the existing isNetworkError/isConnectionDrop checks to call that helper
so errorResId is set reliably before falling back to the generic clone_failed
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44dd6ba6-d0ea-40d6-bf43-eaf1a4331258
📒 Files selected for processing (5)
app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.ktapp/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.ktapp/src/main/res/layout/fragment_clone_repository.xmlgit-core/src/main/java/com/itsaky/androidide/git/core/models/CloneRepoUiState.ktresources/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (1)
- resources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/res/layout/fragment_clone_repository.xml
app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.kt
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt (1)
162-169: 🛠️ Refactor suggestion | 🟠 MajorCatch specific JGit exceptions instead of all exceptions.
The catch block at line 169 catches
Exceptiongenerically, masking unexpected runtime bugs and converting them into retryable clone errors. This contradicts the codebase pattern (e.g.,JGitRepository.getHistory()catchesNoHeadExceptionnarrowly). Additionally, the postcondition check at line 162 should not throw a genericException. Identify the specific JGit exceptions fromcloneRepository()(likelyTransportExceptionand IO-related exceptions) and catch only those; usecheck()orrequire()for the postcondition instead.🤖 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 162 - 169, Replace the generic postcondition throw and broad catch with narrow checks and exception handlers: change the postcondition that currently does throw Exception("Destination directory was not created.") to use check(localPath.exists() && localPath.isDirectory) or require(...) so it fails with an IllegalStateException if the directory wasn't created; then replace catch (e: Exception) with catches for the specific JGit and IO exceptions that clone can throw (e.g., catch TransportException, InvalidRemoteException/GitAPIException, and java.io.IOException) and handle each by updating _uiState to the appropriate error state (keeping hasCloned = true and CloneRepoUiState.Success(localPath) logic intact). Ensure you import the specific exception classes from JGit and remove the generic Exception catch so unrelated runtime errors are not swallowed.
♻️ Duplicate comments (1)
app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt (1)
30-30:⚠️ Potential issue | 🔴 CriticalUse a thread-safe cancellation flag and state-based
update.
isCloneCancelledis written fromcloneRepository()/cancelClone()and read fromProgressMonitor.isCancelled()across threads. The plainBooleancan miss cancellation, andupdateProgressUI()closes over a stale_uiState.value, so a late progress tick can overwrite a newerIdle/Errorstate back toCloning.🔧 Minimal fix
- private var isCloneCancelled = false + `@Volatile` + private var isCloneCancelled = false @@ - val currentState = _uiState.value - if (currentState is CloneRepoUiState.Cloning) { - _uiState.update { - currentState.copy( + _uiState.update { currentState -> + if (currentState is CloneRepoUiState.Cloning) { + currentState.copy( cloneProgress = progressMsg, clonePercentage = percentage, isCancellable = true, statusTextResId = R.string.cloning_repo ) + } else { + currentState } - } + }Also applies to: 63-63, 126-128, 145-154, 232-234
🤖 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` at line 30, Replace the plain Boolean cancellation flag with a thread-safe AtomicBoolean (e.g., isCloneCancelled = AtomicBoolean(false)) and read it from ProgressMonitor.isCancelled() via isCloneCancelled.get(); set it via isCloneCancelled.set(true) in cancelClone() and reset it at the start of cloneRepository(). Replace any direct writes that close over _uiState.value in updateProgressUI() with a state-based update using _uiState.update { current -> if (current is Cloning) current.withUpdatedProgress(newProgress) else current } (or equivalent when/branching) so a late progress tick never overwrites Idle/Error states; also only transition to the Cloning state when starting the operation after clearing the cancel flag. Ensure all references (isCloneCancelled, cloneRepository(), cancelClone(), ProgressMonitor.isCancelled(), updateProgressUI(), _uiState.value) are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt`:
- Around line 162-169: Replace the generic postcondition throw and broad catch
with narrow checks and exception handlers: change the postcondition that
currently does throw Exception("Destination directory was not created.") to use
check(localPath.exists() && localPath.isDirectory) or require(...) so it fails
with an IllegalStateException if the directory wasn't created; then replace
catch (e: Exception) with catches for the specific JGit and IO exceptions that
clone can throw (e.g., catch TransportException,
InvalidRemoteException/GitAPIException, and java.io.IOException) and handle each
by updating _uiState to the appropriate error state (keeping hasCloned = true
and CloneRepoUiState.Success(localPath) logic intact). Ensure you import the
specific exception classes from JGit and remove the generic Exception catch so
unrelated runtime errors are not swallowed.
---
Duplicate comments:
In
`@app/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.kt`:
- Line 30: Replace the plain Boolean cancellation flag with a thread-safe
AtomicBoolean (e.g., isCloneCancelled = AtomicBoolean(false)) and read it from
ProgressMonitor.isCancelled() via isCloneCancelled.get(); set it via
isCloneCancelled.set(true) in cancelClone() and reset it at the start of
cloneRepository(). Replace any direct writes that close over _uiState.value in
updateProgressUI() with a state-based update using _uiState.update { current ->
if (current is Cloning) current.withUpdatedProgress(newProgress) else current }
(or equivalent when/branching) so a late progress tick never overwrites
Idle/Error states; also only transition to the Cloning state when starting the
operation after clearing the cancel flag. Ensure all references
(isCloneCancelled, cloneRepository(), cancelClone(),
ProgressMonitor.isCancelled(), updateProgressUI(), _uiState.value) are updated
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73f9f227-833b-454d-8f0c-88b378e79e08
📒 Files selected for processing (4)
app/src/main/java/com/itsaky/androidide/fragments/CloneRepositoryFragment.ktapp/src/main/java/com/itsaky/androidide/viewmodel/CloneRepositoryViewModel.ktgit-core/src/main/java/com/itsaky/androidide/git/core/models/CloneRepoUiState.ktresources/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (1)
- resources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- git-core/src/main/java/com/itsaky/androidide/git/core/models/CloneRepoUiState.kt
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.
Uh oh!
There was an error while loading. Please reload this page.