Uh oh!
There was an error while loading. Please reload this page.
ADFA-3723: Prompt save before performing Git operations - #1199
Merged
dara-abijo-adfa merged 11 commits intoApr 21, 2026
Conversation
dara-abijo-adfa
requested review from
Daniel-ADFA, itsaky-adfa, jatezzz and jomen-adfaApril 17, 2026 11:33
Contributor
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughRelease Notes - ADFA-3723: Prompt save before performing Git operationsFeatures & Improvements
Technical Changes
Risks & Best-Practice Notes
WalkthroughAdds "Mark as Resolved" UI and flow for conflicted files, stages resolved files in the ViewModel, deduplicates Git status paths favoring conflicts, and prompts to save unsaved editor changes before commit or pull actions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Adapter as GitFileChangeAdapter
participant Fragment as GitBottomSheetFragment
participant ViewModel as GitBottomSheetViewModel
participant Repo as JGitRepository
User->>Adapter: Click "Mark as Resolved"
Adapter->>Fragment: onResolveConflict(FileChange)
Fragment->>ViewModel: resolveConflict(path)
ViewModel->>Repo: stage(file path)
Repo->>Repo: JGit add (stage)
Repo-->>ViewModel: staging complete
ViewModel->>ViewModel: refreshStatus()
ViewModel-->>Fragment: updated status
sequenceDiagram
participant User
participant Fragment as GitBottomSheetFragment
participant Handler as IEditorHandler
participant ViewModel as GitBottomSheetViewModel
User->>Fragment: Click Commit or Pull
Fragment->>Fragment: checkUnsavedChangesAndProceed(action)
Fragment->>Handler: areFilesModified()?
alt Files modified and handler present
Fragment->>User: show save-before-git dialog
User->>Fragment: Choose "Save"
Fragment->>Handler: saveAllAsync(callback)
Handler-->>Fragment: onComplete
Fragment->>ViewModel: perform git action
else User chose "No Save"
Fragment->>ViewModel: perform git action
else User chose "Cancel"
Fragment->>Fragment: abort
else No modified files or no handler
Fragment->>ViewModel: perform git action
end
ViewModel-->>Fragment: action result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/src/main/java/com/itsaky/androidide/viewmodel/GitBottomSheetViewModel.kt (1)
371-382: Optional: resolve path againstrepository.rootDir.Minor consistency nit —
stageFilesinJGitRepositorycomputes relative paths againstrootDir. Usingrepository.rootDirhere (rather than re-resolving viaIProjectManager) couples the path base to the repo the operation targets and mirrorsJGitRepository.getDiff.♻️ Proposed tweak
- val repository = currentRepository ?: return@launch- val projectDir = File(IProjectManager.getInstance().projectDirPath)- repository.stageFiles(listOf(File(projectDir, path)))+ val repository = currentRepository ?: return@launch+ repository.stageFiles(listOf(File(repository.rootDir, path)))🤖 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/GitBottomSheetViewModel.kt` around lines 371 - 382, The resolveConflict function currently builds the target File against IProjectManager.projectDirPath; instead, construct the File using the repository's rootDir to match JGitRepository.stageFiles' expectation and keep path resolution consistent with getDiff. In resolveConflict, replace the projectDir-based File(File(IProjectManager.getInstance().projectDirPath), path) with a File(repository.rootDir, path) (using the local variable repository from the same method) before calling repository.stageFiles and then refreshStatus.app/src/main/res/layout/item_git_file_change.xml (1)
29-40: Removeandroid:padding="0dp",android:minWidth="0dp", andandroid:minHeight="0dp".The
Widget.MaterialComponents.Button.TextButton.Iconstyle is designed to auto-adjust padding and spacing for icon + text layouts. Overriding these with explicit 0 values conflicts with the style's intent and creates accessibility issues:minHeight="0dp"violates Material Design's 48dp minimum touch target requirement. Let the style manage these insets.♻️ Recommended change
<com.google.android.material.button.MaterialButton android:id="@+id/btnMarkResolved" style="@style/Widget.MaterialComponents.Button.TextButton.Icon" android:layout_width="wrap_content" android:layout_height="wrap_content" android:layout_marginStart="8dp" - android:minWidth="0dp"- android:minHeight="0dp"- android:padding="0dp" android:text="@string/mark_as_resolved"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/layout/item_git_file_change.xml` around lines 29 - 40, The MaterialButton with id btnMarkResolved overrides the MaterialComponents text-button icon insets and touch target by setting android:padding="0dp", android:minWidth="0dp", and android:minHeight="0dp"; remove these three attributes from the <com.google.android.material.button.MaterialButton> (btnMarkResolved) so the Widget.MaterialComponents.Button.TextButton.Icon style can apply its default padding and 48dp minimum touch target for accessibility.
🤖 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/git/adapter/GitFileChangeAdapter.kt`:
- Around line 67-70: The bind() silently mutates selectedFiles by removing
change.path when isConflicted, which can leave the commit button enabled; modify
bind() to call the existing onSelectionChanged callback (or explicitly invoke
validateCommitButton via the fragment's selection handler) after removing
change.path so the UI is revalidated. Specifically, inside the bind() branch
that checks isConflicted and calls selectedFiles.remove(change.path), follow
that removal with a call to onSelectionChanged() (or the adapter's
selection-change notifier used elsewhere) so validateCommitButton() runs and the
commit button state is updated.
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/viewmodel/GitBottomSheetViewModel.kt`:
- Around line 371-382: The resolveConflict function currently builds the target
File against IProjectManager.projectDirPath; instead, construct the File using
the repository's rootDir to match JGitRepository.stageFiles' expectation and
keep path resolution consistent with getDiff. In resolveConflict, replace the
projectDir-based File(File(IProjectManager.getInstance().projectDirPath), path)
with a File(repository.rootDir, path) (using the local variable repository from
the same method) before calling repository.stageFiles and then refreshStatus.
In `@app/src/main/res/layout/item_git_file_change.xml`:
- Around line 29-40: The MaterialButton with id btnMarkResolved overrides the
MaterialComponents text-button icon insets and touch target by setting
android:padding="0dp", android:minWidth="0dp", and android:minHeight="0dp";
remove these three attributes from the
<com.google.android.material.button.MaterialButton> (btnMarkResolved) so the
Widget.MaterialComponents.Button.TextButton.Icon style can apply its default
padding and 48dp minimum touch target for accessibility.
🪄 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: 4b32499c-6b54-4801-9287-7f26b5242f7a
📒 Files selected for processing (6)
app/src/main/java/com/itsaky/androidide/fragments/git/GitBottomSheetFragment.ktapp/src/main/java/com/itsaky/androidide/fragments/git/adapter/GitFileChangeAdapter.ktapp/src/main/java/com/itsaky/androidide/viewmodel/GitBottomSheetViewModel.ktapp/src/main/res/layout/item_git_file_change.xmlgit-core/src/main/java/com/itsaky/androidide/git/core/JGitRepository.ktresources/src/main/res/values/strings.xml
Uh oh!
There was an error while loading. Please reload this page.
jatezzz
requested changes
Apr 17, 2026
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/fragments/git/adapter/GitFileChangeAdapter.kt (1)
53-97: LGTM — prior selection-stale concern is addressed.The guarded removal at Lines 68-70 now notifies
onSelectionChangedwhen a conflicted path is evicted fromselectedFiles, so the commit button state stays consistent. Clearing the listener before settingisCheckedand re-attaching a guarded listener that hard-rejectsCONFLICTEDselection is a solid pattern and avoids the recursive-callback pitfalls.One minor nit:
bind()is growing and mixing selection, visibility, alpha, and icon-mapping concerns (echoing earlier feedback about nesting). Consider extracting small helpers likeapplyConflictState(isConflicted)andbindCheckbox(change)to flatten the method — purely optional and non-blocking.🤖 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/git/adapter/GitFileChangeAdapter.kt` around lines 53 - 97, The bind(change: FileChange) method is doing multiple concerns; extract two helpers to flatten it: create applyConflictState(isConflicted: Boolean) to set checkbox visibility/enabled, btnMarkResolved visibility, alpha for filePath/statusIcon/root, and ensure conflicted paths are removed from selectedFiles (calling onSelectionChanged when removed); and create bindCheckbox(change: FileChange) to clear/set the checkbox listener, initialize checkbox.isChecked from selectedFiles, and attach the guarded OnCheckedChangeListener that checks ChangeType.CONFLICTED, updates selectedFiles, and calls onSelectionChanged; then update bind(...) to call these two helpers and only set binding.filePath.text and any remaining small bits so behavior (use of selectedFiles, onSelectionChanged, ChangeType.CONFLICTED, binding.checkbox, binding.btnMarkResolved, binding.filePath, binding.statusIcon, binding.root) remains identical.
🤖 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/fragments/git/adapter/GitFileChangeAdapter.kt`:
- Around line 53-97: The bind(change: FileChange) method is doing multiple
concerns; extract two helpers to flatten it: create
applyConflictState(isConflicted: Boolean) to set checkbox visibility/enabled,
btnMarkResolved visibility, alpha for filePath/statusIcon/root, and ensure
conflicted paths are removed from selectedFiles (calling onSelectionChanged when
removed); and create bindCheckbox(change: FileChange) to clear/set the checkbox
listener, initialize checkbox.isChecked from selectedFiles, and attach the
guarded OnCheckedChangeListener that checks ChangeType.CONFLICTED, updates
selectedFiles, and calls onSelectionChanged; then update bind(...) to call these
two helpers and only set binding.filePath.text and any remaining small bits so
behavior (use of selectedFiles, onSelectionChanged, ChangeType.CONFLICTED,
binding.checkbox, binding.btnMarkResolved, binding.filePath, binding.statusIcon,
binding.root) remains identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45486cd3-cf2c-480a-8203-0c35b5ce8cd1
📒 Files selected for processing (2)
app/src/main/java/com/itsaky/androidide/fragments/git/adapter/GitFileChangeAdapter.ktresources/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (1)
- resources/src/main/res/values/strings.xml
jatezzz
approved these changes
Apr 20, 2026
Uh oh!
There was an error while loading. Please reload this page.
hal-eisen-adfa added a commit
that referenced
this pull request
Jun 11, 2026
Once the LayoutInflaterTest hang stopped wedging analyze.yml at the
3-hour job timeout, the SonarQube run completed fast enough to surface
five pre-existing compile failures that no other CI workflow exercises.
debug.yml, instrumentation-test.yml, and release.yml all only invoke
:app:assemble*, which never compiles unit-test sources, never merges
unit-test manifests, and never enumerates :plugin-api:plugin-builder as
a regular subproject. The rot has been silent for at least a week.
- logsender: disable unit-test variant (it has zero test files; the
manifest merger was failing on a 16 vs 28 minSdk skew with :resources
and a long transitive chain of androidx libraries)
- plugin-api/plugin-builder: pin Kotlin api/language to 2.1 to resolve
the kotlin-dsl 1.8 vs project Kotlin 2.3 mismatch, matching the
existing pattern in composite-builds/build-logic/{desugaring,plugins}
- git-core: pass the hasBeenPushed parameter that was added to GitCommit
in PR #1199 (ADFA-3723)
- testing/lsp + lsp/{java,xml}: replace .getDefault() with .default on
ILanguageServerRegistry — the companion object exposes a Kotlin val,
not a synthetic Java accessor
- app: pass a mock GitCredentialsManager to CloneRepositoryViewModel,
matching the second ctor param added in PR #1178 (ADFA-3671)
Verified locally: the six previously-failing compile/manifest tasks all
build clean; :logsender:processV8DebugUnitTestManifest no longer exists.hal-eisen-adfa added a commit
that referenced
this pull request
Jun 11, 2026
…#1371) * ADFA-3863 | Fix LayoutInflaterTest hang that wedged Jacoco/SonarQube CI The analyze.yml workflow was timing out at the 180-minute job limit because :xml-inflater:testV8DebugUnitTest hung waiting on a Tooling API child JVM that never exited. Add defense in depth so a future hang fails in minutes, not hours, and fix the underlying race. Safety nets: - ToolingApiTestLauncher: bound cancelCurrentBuild()/shutdown() RPCs with 30s timeouts and fall back to proc.destroyForcibly() if the child JVM does not exit. The child can no longer leak regardless of RPC behavior. - LayoutInflaterTest: @rule Timeout caps each test at 2 minutes. - build.gradle.kts: project-wide Test task timeout of 10 minutes as a final backstop. - analyze.yml: per-step timeout of 60 minutes on "Build and analyze" so a wedge cannot burn the full 180-minute job budget. Root cause: - XmlInflaterTest.initIfNeeded used a non-atomic get/set pair on AtomicBoolean. Replace with compareAndSet, and reset the flag on failure so a failed init does not silently poison subsequent tests. * ADFA-3863 | Fix five compile failures uncovered by SonarQube run Once the LayoutInflaterTest hang stopped wedging analyze.yml at the 3-hour job timeout, the SonarQube run completed fast enough to surface five pre-existing compile failures that no other CI workflow exercises. debug.yml, instrumentation-test.yml, and release.yml all only invoke :app:assemble*, which never compiles unit-test sources, never merges unit-test manifests, and never enumerates :plugin-api:plugin-builder as a regular subproject. The rot has been silent for at least a week. - logsender: disable unit-test variant (it has zero test files; the manifest merger was failing on a 16 vs 28 minSdk skew with :resources and a long transitive chain of androidx libraries) - plugin-api/plugin-builder: pin Kotlin api/language to 2.1 to resolve the kotlin-dsl 1.8 vs project Kotlin 2.3 mismatch, matching the existing pattern in composite-builds/build-logic/{desugaring,plugins} - git-core: pass the hasBeenPushed parameter that was added to GitCommit in PR #1199 (ADFA-3723) - testing/lsp + lsp/{java,xml}: replace .getDefault() with .default on ILanguageServerRegistry — the companion object exposes a Kotlin val, not a synthetic Java accessor - app: pass a mock GitCredentialsManager to CloneRepositoryViewModel, matching the second ctor param added in PR #1178 (ADFA-3671) Verified locally: the six previously-failing compile/manifest tasks all build clean; :logsender:processV8DebugUnitTestManifest no longer exists. * ADFA-3863 | Code review cleanups on the test-init plumbing - XmlInflaterTest.initIfNeeded: replace the AtomicBoolean compareAndSet with a UNINITIALIZED/READY/FAILED state machine guarded by a synchronized block. The previous code let a losing thread return immediately while the winner was still inside launchServer { ... setup(...) }, so downstream callers could touch IProjectManager before setup completed. Losers now block on the singleton's monitor until the winner exits, then either proceed (READY) or throw with the original error as cause (FAILED). FAILED is terminal: a poisoned init no longer re-launches the Tooling API JVM for every subsequent test. - ToolingApiTestLauncher: convert the two shutdown try/catch swallows in the finally to runCatching { ... }.onFailure { ... }. Same semantics, the lambda shape makes the catch-and-log intent explicit. - build.gradle.kts: import java.time.Duration and drop the FQN at the Test task timeout.
jatezzz pushed a commit
that referenced
this pull request
Jun 22, 2026
* feat(ADFA-3723): Show save prompt for unsaved changes before performing Git operations * feat(ADFA-3723): Prevent conflicted files from getting staged * feat(ADFA-3723): Mark conflicted files as resolved * fix(ADFA-3723): Resolve adapter issues * fix(ADFA-3723): Fix duplicate file listing * fix(ADFA-3723): Ensure adapter updates * refactor(ADFA-3273): Flatten nested ifs
jatezzz pushed a commit
that referenced
this pull request
Jun 22, 2026
…#1371) * ADFA-3863 | Fix LayoutInflaterTest hang that wedged Jacoco/SonarQube CI The analyze.yml workflow was timing out at the 180-minute job limit because :xml-inflater:testV8DebugUnitTest hung waiting on a Tooling API child JVM that never exited. Add defense in depth so a future hang fails in minutes, not hours, and fix the underlying race. Safety nets: - ToolingApiTestLauncher: bound cancelCurrentBuild()/shutdown() RPCs with 30s timeouts and fall back to proc.destroyForcibly() if the child JVM does not exit. The child can no longer leak regardless of RPC behavior. - LayoutInflaterTest: @rule Timeout caps each test at 2 minutes. - build.gradle.kts: project-wide Test task timeout of 10 minutes as a final backstop. - analyze.yml: per-step timeout of 60 minutes on "Build and analyze" so a wedge cannot burn the full 180-minute job budget. Root cause: - XmlInflaterTest.initIfNeeded used a non-atomic get/set pair on AtomicBoolean. Replace with compareAndSet, and reset the flag on failure so a failed init does not silently poison subsequent tests. * ADFA-3863 | Fix five compile failures uncovered by SonarQube run Once the LayoutInflaterTest hang stopped wedging analyze.yml at the 3-hour job timeout, the SonarQube run completed fast enough to surface five pre-existing compile failures that no other CI workflow exercises. debug.yml, instrumentation-test.yml, and release.yml all only invoke :app:assemble*, which never compiles unit-test sources, never merges unit-test manifests, and never enumerates :plugin-api:plugin-builder as a regular subproject. The rot has been silent for at least a week. - logsender: disable unit-test variant (it has zero test files; the manifest merger was failing on a 16 vs 28 minSdk skew with :resources and a long transitive chain of androidx libraries) - plugin-api/plugin-builder: pin Kotlin api/language to 2.1 to resolve the kotlin-dsl 1.8 vs project Kotlin 2.3 mismatch, matching the existing pattern in composite-builds/build-logic/{desugaring,plugins} - git-core: pass the hasBeenPushed parameter that was added to GitCommit in PR #1199 (ADFA-3723) - testing/lsp + lsp/{java,xml}: replace .getDefault() with .default on ILanguageServerRegistry — the companion object exposes a Kotlin val, not a synthetic Java accessor - app: pass a mock GitCredentialsManager to CloneRepositoryViewModel, matching the second ctor param added in PR #1178 (ADFA-3671) Verified locally: the six previously-failing compile/manifest tasks all build clean; :logsender:processV8DebugUnitTestManifest no longer exists. * ADFA-3863 | Code review cleanups on the test-init plumbing - XmlInflaterTest.initIfNeeded: replace the AtomicBoolean compareAndSet with a UNINITIALIZED/READY/FAILED state machine guarded by a synchronized block. The previous code let a losing thread return immediately while the winner was still inside launchServer { ... setup(...) }, so downstream callers could touch IProjectManager before setup completed. Losers now block on the singleton's monitor until the winner exits, then either proceed (READY) or throw with the original error as cause (FAILED). FAILED is terminal: a poisoned init no longer re-launches the Tooling API JVM for every subsequent test. - ToolingApiTestLauncher: convert the two shutdown try/catch swallows in the finally to runCatching { ... }.onFailure { ... }. Same semantics, the lambda shape makes the catch-and-log intent explicit. - build.gradle.kts: import java.time.Duration and drop the FQN at the Test task timeout.
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 freeto 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.
Prompt user to save unsaved changes before Git pull or commit