fix(deletion): leave the route of a deleted contact - #5709
Conversation
Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
|
@coderabbitai review please |
|
✅ Action performedReview finished.
|
|
/backport to stable8.8 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughContact deletion now waits for store removal and redirects to the appropriate list route. Multi-selection deletion redirects only when it includes the open contact. New tests cover route selection, deletion ordering, and conditional navigation. ChangesContact deletion routing
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: 🟡 Moderate · up to This change clears the contact route after deletion, but multi-contact deletion can navigate away from a read-only contact that remains available and can change routes before batch deletion starts. These correctness issues should be resolved before merge. Sequence Diagram(s)sequenceDiagram
participant ContactDetails
participant ContactsList
participant Store
participant Router
ContactDetails->>Store: Dispatch contact deletion
Store-->>ContactDetails: Deletion resolves
ContactDetails->>Router: Replace with listRoute()
ContactsList->>ContactsList: Check whether selected contact is deleted
ContactsList->>Store: Delete selected contacts
alt Open contact was deleted
ContactsList->>Router: Replace with listRoute()
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
tests/javascript/components/contact-deletion-route.test.jsOops! Something went wrong! :( ESLint: 10.10.0 ReferenceError: module is not defined in ES module scope 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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Team
Run ID: a8e82b49-4182-4246-b049-95d3561da743
📒 Files selected for processing (3)
src/components/ContactDetails.vuesrc/components/ContactsList.vuetests/javascript/components/contact-deletion-route.test.js
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
| const deletesSelectedContact = Array.from(this.multiSelectedContacts.values()) | ||
| .some((contact) => contact.key === this.selectedContact) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Navigate only when the open contact will be deleted.
deletesSelectedContact checks only selection membership. The deletion loop skips read-only contacts. If the open contact is read-only and another selected contact is deleted, the new route replacement removes the still-existing open contact from the URL.
src/components/ContactsList.vue#L503-L504: include the same deletion-eligibility condition used by the deletion loop when computingdeletesSelectedContact.tests/javascript/components/contact-deletion-route.test.js#L113-L116: make the navigation case use a deletable open contact, and add a read-only open-contact case that expects no route replacement.
📍 Affects 2 files
src/components/ContactsList.vue#L503-L504(this comment)tests/javascript/components/contact-deletion-route.test.js#L113-L116
| // the route would otherwise keep pointing at a deleted contact | ||
| if (deletesSelectedContact) { | ||
| this.$router.replace(this.listRoute()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- ContactsList.vue relevant methods and route replacement ---'
rg -n -A45 -B25 'deleteAllMultiSelected|deletesSelectedContact|this\.\$router\.replace' src/components/ContactsList.vue
printf '%s\n' '--- file header ---'
head -n 12 src/components/ContactsList.vueRepository: nextcloud/contacts
Length of output: 5090
Await batch deletions before replacing the route.
Map.forEach(async ...) does not await its callbacks. Each callback waits 500 ms, so this.$router.replace(this.listRoute()) can run before deleteContact starts. Make deleteAllMultiSelected async and await all writable-contact deletion promises before replacing the route.
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
2 participants
Assisted-by: ClaudeCode:claude-opus-5
🤖 AI (if applicable)
Summary by CodeRabbit
Bug Fixes
Tests