ADFA- 3701 Protect against project name collisions during project rename action#1272
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:
📝 WalkthroughRelease Notes - ADFA-3701: Project Name Collision PreventionFeatures & Improvements
Known Risks & Best Practice Violations
WalkthroughAdded a project rename flow: adapter accepts a name-existence checker and validates against the old name; ChangesProject Rename Flow
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as "User"
participant UI as "RecentProjectsFragment\n(Dialog via Adapter)"
participant Adapter as "RecentProjectsAdapter"
participant VM as "RecentProjectsViewModel"
participant DB as "Database"
participant FS as "Filesystem"
User->>UI: Trigger rename (open dialog)
UI->>Adapter: Show dialog, provide nameExists callback
Adapter->>Adapter: Validate input (calls nameExists via VM bound callback)
User->>Adapter: Submit new name
Adapter->>VM: updateProject(RenamedFile(oldName,newName,oldPath,newPath))
VM->>DB: update project record (oldName -> newName, oldLocation -> newLocation)
DB-->>VM: success
VM->>FS: rename filesystem path (oldPath -> newPath)
FS-->>VM: success
VM-->>UI: emit renameStatus(true)
Note over VM,UI: On DB failure, VM attempts FS rollback and emits renameStatus(false)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.kt`:
- Around line 289-293: The duplicate-name check in RecentProjectsAdapter (the
conditional using projects.any { it.name == newName } that sets
inputLayout.error and disables positiveButton) only inspects the filtered
adapter list so a hidden project can be missed; update the validation to check
the unfiltered/full project set instead (e.g., use the ViewModel's unfiltered
collection such as viewModel.allProjects.any { it.name == newName } or call a
DAO/repository existence method like projectDao.existsByName(newName)) and keep
the rest of the UI logic (setting inputLayout.error and
positiveButton.isEnabled) the same.
🪄 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: 78efbec4-d668-4eb1-afa7-4a4e59a5edb2
📒 Files selected for processing (3)
app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.ktapp/src/main/java/com/itsaky/androidide/fragments/RecentProjectsFragment.ktapp/src/main/java/com/itsaky/androidide/viewmodel/RecentProjectsViewModel.kt
…n-rename-collision' into ADFA-3701-crash-when-rename-collision
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.kt (1)
230-235:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim
newNamebefore validating and renaming.The text from the input is consumed verbatim:
- Line 230 / Line 271:
binding.textinputEdittext.text.toString()- Line 285
newName.isEmpty()–" "passes this check- Line 290
nameExists(newName)–"MyProj "(trailing space) does not collide with existing"MyProj"- Line 231 builds
newPathdirectly fromnewName, so the on-disk directory ends up with leading/trailing whitespace, which is likely to cause surprises later (lookups, comparisons, shell tooling).Trim once at the source and reuse the trimmed value for both validation and the rename.
🛡️ Suggested change
binding.textinputEdittext.addTextChangedListener(object : TextWatcher { override fun beforeTextChanged(s: CharSequence?, start: Int, count: Int, after: Int) {} override fun onTextChanged(s: CharSequence?, start: Int, before: Int, count: Int) {} override fun afterTextChanged(s: Editable?) { - validateProjectName(binding.textinputLayout, s.toString(), oldName, dialog) + validateProjectName(binding.textinputLayout, s.toString().trim(), oldName, dialog) } }) validateProjectName( binding.textinputLayout, - binding.textinputEdittext.text.toString(), + binding.textinputEdittext.text.toString().trim(), oldName, dialog )And at line 230:
- val newName = binding.textinputEdittext.text.toString() + val newName = binding.textinputEdittext.text.toString().trim()Also applies to: 265-274, 285-294
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.kt` around lines 230 - 235, Trim the user input once when reading it (replace binding.textinputEdittext.text.toString() with a trimmed value) and use that trimmed value for all subsequent checks and operations: validations like newName.isEmpty(), nameExists(newName), path assembly (used in project.rename(newPath)), and onFileRenamed(RenamedFile(...)). This ensures whitespace is removed for comparisons and the filesystem rename; update all occurrences (including the blocks around the current newName creation, the validation paths, and the nameExists checks) to reference the single trimmed variable.
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/viewmodel/RecentProjectsViewModel.kt (1)
58-59: ⚡ Quick winConsider matching
_deletionStatus' replay buffer.
_deletionStatusis declared withreplay = 1(Line 55) so a result emitted while the fragment is being recreated (e.g. configuration change) is still delivered to the new collector._renameStatususes the defaultreplay = 0, so a rename failure that races with a recreation would be lost and the user would not see the error toast. Aligning the two flows would make rename feedback as resilient as deletion feedback.♻️ Suggested change
- private val _renameStatus = MutableSharedFlow<Boolean>() + private val _renameStatus = MutableSharedFlow<Boolean>(replay = 1) val renameStatus = _renameStatus.asSharedFlow()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/viewmodel/RecentProjectsViewModel.kt` around lines 58 - 59, The _renameStatus flow uses a MutableSharedFlow with the default replay=0, so rename results can be lost across fragment recreation; change the declaration of _renameStatus to match _deletionStatus by constructing it with replay = 1 (keeping renameStatus = _renameStatus.asSharedFlow()) so a rename result emitted during configuration changes is replayed to new collectors; update any related comments if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/java/com/itsaky/androidide/viewmodel/RecentProjectsViewModel.kt`:
- Around line 201-219: The DB update can fail after the disk rename, leaving DB
and filesystem out of sync; modify updateProject to accept the original path
(oldLocation) in addition to newName/newLocation (or change the caller to pass
old path), and in the catch block attempt to revert the filesystem rename by
calling File(newLocation).renameTo(File(oldLocation)) on Dispatchers.IO, log
whether the rollback succeeded or failed, then emit false; alternatively, move
the disk rename into updateProject so the VM performs the filesystem rename and
can roll it back if DAO calls throw. Ensure you reference updateProject,
recentProjectDao.updateNameAndLocation, recentProjectDao.updateLastModified and
the catch block for the rollback logic.
---
Outside diff comments:
In `@app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.kt`:
- Around line 230-235: Trim the user input once when reading it (replace
binding.textinputEdittext.text.toString() with a trimmed value) and use that
trimmed value for all subsequent checks and operations: validations like
newName.isEmpty(), nameExists(newName), path assembly (used in
project.rename(newPath)), and onFileRenamed(RenamedFile(...)). This ensures
whitespace is removed for comparisons and the filesystem rename; update all
occurrences (including the blocks around the current newName creation, the
validation paths, and the nameExists checks) to reference the single trimmed
variable.
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/viewmodel/RecentProjectsViewModel.kt`:
- Around line 58-59: The _renameStatus flow uses a MutableSharedFlow with the
default replay=0, so rename results can be lost across fragment recreation;
change the declaration of _renameStatus to match _deletionStatus by constructing
it with replay = 1 (keeping renameStatus = _renameStatus.asSharedFlow()) so a
rename result emitted during configuration changes is replayed to new
collectors; update any related comments if present.
🪄 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: 2806c22e-15f0-48b4-9f07-7011e3edd403
📒 Files selected for processing (3)
app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.ktapp/src/main/java/com/itsaky/androidide/fragments/RecentProjectsFragment.ktapp/src/main/java/com/itsaky/androidide/viewmodel/RecentProjectsViewModel.kt
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.kt (1)
278-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject path separators in rename validation.
newNameis still treated as a filesystem path segment later in Line 231-Line 236, but this validator only checks emptiness and collisions. Names like../OtherProjectornested/dirwill pass here and change the rename target instead of just renaming the current project directory.Suggested fix
when { - newName.isEmpty() -> { + newName.isBlank() -> { inputLayout.error = dialog.context.getString(R.string.msg_cannnot_empty) positiveButton.isEnabled = false } + newName.contains('/') || newName == "." || newName == ".." -> { + inputLayout.error = dialog.context.getString(R.string.msg_invalid_project_name) + positiveButton.isEnabled = false + } + newName != oldName && nameExists(newName) -> { inputLayout.error = dialog.context.getString(R.string.msg_current_name_unavailable) positiveButton.isEnabled = false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.kt` around lines 278 - 299, validateProjectName currently allows path separators and traversal like "../" or "nested/dir"; update it (in validateProjectName) to reject names that contain file-separator characters or traversal segments before checking nameExists. Specifically, check newName for characters/segments such as '/' or '\' (use File.separatorChar or both chars) and for ".." path traversal, set inputLayout.error to an appropriate "invalid project name" message from resources and disable positiveButton, otherwise continue with the existing empty/collision checks (references: validateProjectName, newName, oldName, nameExists, inputLayout, dialog, positiveButton).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/java/com/itsaky/androidide/viewmodel/RecentProjectsViewModel.kt`:
- Around line 58-59: The _renameStatus flow currently only emits failure (false)
so callers can't await a successful DB update; update the rename-handling code
(the MutableSharedFlow _renameStatus and the rename coroutine logic used in
functions like onFileRenamed / any rename handler between lines ~206-235) so
that after both DAO writes complete successfully you emit true (e.g.,
_renameStatus.emit(true)) and on any rollback or exception emit false; ensure
the success emission happens only after both database updates return/complete,
and keep the existing failure emission path for errors so callers can react to
the final outcome.
---
Outside diff comments:
In `@app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.kt`:
- Around line 278-299: validateProjectName currently allows path separators and
traversal like "../" or "nested/dir"; update it (in validateProjectName) to
reject names that contain file-separator characters or traversal segments before
checking nameExists. Specifically, check newName for characters/segments such as
'/' or '\' (use File.separatorChar or both chars) and for ".." path traversal,
set inputLayout.error to an appropriate "invalid project name" message from
resources and disable positiveButton, otherwise continue with the existing
empty/collision checks (references: validateProjectName, newName, oldName,
nameExists, inputLayout, dialog, positiveButton).
🪄 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: ba2d270d-b8be-429e-bbc2-fd2fe5d48806
📒 Files selected for processing (2)
app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.ktapp/src/main/java/com/itsaky/androidide/viewmodel/RecentProjectsViewModel.kt
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/com/itsaky/androidide/viewmodel/RecentProjectsViewModel.kt (1)
224-225: ⚡ Quick winAwait
loadProjects()before signalling success.
loadProjects()returns aJobthat runs on its ownviewModelScope.launch(Dispatchers.IO)(see Line 66-74), so it is not awaited here —_renameStatus.emit(true)can fire beforeallProjectsand_projectsreflect the renamed entry. Any observer that reacts torenameStatus == trueby reading the current project list (or by re-running validation viaprojectNameExists) may briefly see stale data.♻️ Proposed fix
- loadProjects() - _renameStatus.emit(true) + loadProjects().join() + _renameStatus.emit(true)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/viewmodel/RecentProjectsViewModel.kt` around lines 224 - 225, loadProjects() is launched asynchronously and returns a Job, so _renameStatus.emit(true) can fire before projects are updated; capture the Job returned by loadProjects() and wait for it to complete (e.g., call job.join() or make loadProjects() a suspend function and await it) before calling _renameStatus.emit(true); ensure this waiting happens inside a coroutine (e.g., viewModelScope) so the UI observers read the updated allProjects/_projects when renameStatus becomes true.app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.kt (1)
230-232: 💤 Low valueOptional: derive
newPathvia theFileAPI.
oldPath.substringBeforeLast("/") + "/" + newNameworks on Android paths but is fragile ifoldPathever contains a trailing separator or no separator at all (in which casesubstringBeforeLastreturns the whole string). UsingFileis more idiomatic and self-documenting.♻️ Proposed refactor
- val newName = binding.textinputEdittext.text.toString().trim() - val oldPath = project.path - val newPath = oldPath.substringBeforeLast("/") + "/" + newName + val newName = binding.textinputEdittext.text.toString().trim() + val oldFile = File(project.path) + val oldPath = oldFile.absolutePath + val newPath = File(oldFile.parentFile, newName).absolutePath🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.kt` around lines 230 - 232, newPath is built by string manipulation from project.path which breaks on trailing or missing separators; replace that logic by using the File API: construct a File from project.path (reference: project.path), get its parent directory (File(project.path).parentFile) and create a new File(parent, newName) (reference: newName from binding.textinputEdittext), falling back to newName or File(newName).path if parent is null, then use that File's path as newPath so path joining is robust and platform-correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.kt`:
- Around line 230-232: newPath is built by string manipulation from project.path
which breaks on trailing or missing separators; replace that logic by using the
File API: construct a File from project.path (reference: project.path), get its
parent directory (File(project.path).parentFile) and create a new File(parent,
newName) (reference: newName from binding.textinputEdittext), falling back to
newName or File(newName).path if parent is null, then use that File's path as
newPath so path joining is robust and platform-correct.
In
`@app/src/main/java/com/itsaky/androidide/viewmodel/RecentProjectsViewModel.kt`:
- Around line 224-225: loadProjects() is launched asynchronously and returns a
Job, so _renameStatus.emit(true) can fire before projects are updated; capture
the Job returned by loadProjects() and wait for it to complete (e.g., call
job.join() or make loadProjects() a suspend function and await it) before
calling _renameStatus.emit(true); ensure this waiting happens inside a coroutine
(e.g., viewModelScope) so the UI observers read the updated
allProjects/_projects when renameStatus becomes true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a70163d7-abb9-48e4-99c6-328007f00dd6
📒 Files selected for processing (2)
app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.ktapp/src/main/java/com/itsaky/androidide/viewmodel/RecentProjectsViewModel.kt
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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/adapters/RecentProjectsAdapter.kt (1)
229-240:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelay success feedback until the rename is fully committed.
The
project.rename(newPath)call (ProjectFile.kt line 47) silently fails—it doesn't check the return value ofFile.renameTo()or throw an exception—soflashSuccess(R.string.renamed)fires even if the filesystem rename failed. Additionally,flashSuccess()runs beforeonFileRenamed(...)and the ViewModel's database update, which can emit false and show an error later, creating confusing UX where success is followed by failure. The broadcatch (e: Exception)also masks callback/UI errors occurring after a successful filesystem rename.Limit the
try/catchto the actualproject.rename(...)call, catch the concrete exception type (or add verification after the rename), and surface success only after the ViewModel'srenameStatusflow confirms persistence succeeded (see RecentProjectsViewModel.kt:225–234 for the proper pattern with rollback).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.kt` around lines 229 - 240, The rename flow currently shows success before the filesystem and DB persistence are confirmed; narrow the try/catch to only the actual filesystem rename (project.rename(newPath)) and either handle its boolean result or throw on failure so you don't emit success prematurely, then subscribe to the ViewModel's renameStatus (RecentProjectsViewModel.renameStatus) and only call flashSuccess(R.string.renamed), onFileRenamed(RenamedFile(...)) and notifyItemChanged(position) after the ViewModel reports persistence succeeded; catch/handle concrete errors from project.rename (or verify file existence/rename result) and perform rollback/flashError when persistence fails rather than using a broad catch around the whole UI callback sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.kt`:
- Around line 229-240: The rename flow currently shows success before the
filesystem and DB persistence are confirmed; narrow the try/catch to only the
actual filesystem rename (project.rename(newPath)) and either handle its boolean
result or throw on failure so you don't emit success prematurely, then subscribe
to the ViewModel's renameStatus (RecentProjectsViewModel.renameStatus) and only
call flashSuccess(R.string.renamed), onFileRenamed(RenamedFile(...)) and
notifyItemChanged(position) after the ViewModel reports persistence succeeded;
catch/handle concrete errors from project.rename (or verify file
existence/rename result) and perform rollback/flashError when persistence fails
rather than using a broad catch around the whole UI callback sequence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c7d317e-5dae-406c-b9ec-d34cdbd8ecae
📒 Files selected for processing (1)
app/src/main/java/com/itsaky/androidide/adapters/RecentProjectsAdapter.kt
2 participants
validateProjectName now also rejects names matching another project
Disables the Rename button and shows an error message (which is in strings.xml)