Put every optimistic action on one helper and fix the ones that failed silently

[AndyScherzinger]

#6694 merged, so this now targets master and is ready to review on its own. #6717 and #6719 are stacked on top of it.

Since the rebase onto current master this branch also depends on #6744, which is merged. onConversationRemoved calls getRooms(user, forceFullSync = true), and forceFullSync arrived with the modifiedSince delta sync. The dependency is not cosmetic: a delta response cannot express a removal, so a plain refresh after leaving or deleting would leave the row in the database and the entry would reappear the moment it stopped being hidden. Reverting #6744 would break this branch rather than merely conflict with it.

#6694 shipped five optimistic chat actions, each with its own hand-rolled apply/retry/revert block. This branch pulls that shape into one helper, puts the conversation list and conversation info actions on it, and fixes the two places where an action told the user it had worked when it had not.

refactor(chat): extract the optimistic action pattern into one helper — optimisticAction(apply, isConfirmed, request) in utils/OptimisticAction.kt. apply writes the local state and returns its own undo action, or null when nothing changed, in which case nothing is undone later either. A transient failure is retried once, any failure or a cancellation takes the change back, and the undo runs under NonCancellable so a revert cannot itself be cancelled half-way. Migrating OfflineFirstChatRepository and ReactionsRepositoryImpl onto it removes 112 lines with no behaviour change.

perf(conversations): keep list and info actions correct when they fail — mark as read, mark as unread, favorite, archive and the important and sensitive toggles now go through the same helper. The two toggles gain a retry they never had; all six gain cancellation safety, so leaving the screen mid-request no longer leaves the change applied although the server may never have heard of it.

fix(conversation info): tell the user when a setting did not save — DatabaseStorageModule.saveString and saveBoolean caught every failure and only logged it, so the notification level, the message expiration and the call notification switch kept showing a setting the server had refused, with no error anywhere. The failure now reaches the view model, which puts the previous value back and shows the error snackbar.

perf(conversation list): drop a deleted conversation from the list at once — the row stayed in the list until the worker had finished and the list had been fetched again. It now disappears on confirmation and comes back if the delete fails, reusing the guard that hides a conversation being left.

test(utils): cover the optimistic action contract directly — optimisticAction was only exercised through the repositories using it. Seven tests now assert the rules every optimistic action depends on: applied before the answer, taken back on a refusal, taken back on a refusal the server reports in the payload, taken back when the screen is left mid-request, nothing taken back when nothing was applied, and one hiccup retried without losing the change.

🖼️ Screenshots

No UI was added or changed — the same controls, the same snackbars, applied earlier.

🏁 Checklist

• ⛑️ Tests (unit and/or integration) are included or not needed
• 🔖 Capability is checked or not needed
• 🔙 Backport requests are created or not needed — not backported, this goes into 25.1.0
• 📅 Milestone is set
• 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

🤖 AI (if applicable)

• The content of this PR was partly or fully generated using AI

[github-actions]

📱 QA build

Download	app-qa-debug.apk
QR code	Open the QR code for this download
Commit	d440ed1
Version	6715
Available until	7 days after this build

The QA build installs alongside a released Nextcloud app, so you can keep
using your existing install while testing.

Downloading the file requires a GitHub account, so open this link on the
device you want to test on, or transfer the APK to it.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

A shared optimisticAction utility applies local changes, retries transient failures once, and restores prior state when requests fail or responses are unconfirmed. Chat repositories, reaction handling, conversation settings, and conversation-list actions use this utility. Preference storage now propagates selected save failures. Conversation removal tracking covers rooms being left or deleted.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to 24c33

Rapid setting changes can leave a toggle showing the wrong value, and a deleted conversation can reappear if its follow-up sync fails. These bounded issues warrant fixes or explicit acceptance before merging.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 24c33

Failure handling generally improves, but overlapping changes can leave the sensitive-conversation switch enabled after the server accepted disabling it and rejected re-enabling it. This can misrepresent whether message previews are protected. The inspected requests retain their existing account credentials and endpoints; no new privilege escalation was established.

Retained concerns

• Medium · security · inferred: The new confirmation baseline can misrepresent a privacy control after overlapping requests. Start with sensitive enabled, begin disabling it, then begin re-enabling it before the first response arrives. If disabling succeeds and re-enabling ultimately throws, the older success cannot update confirmedSetting because it is no longer newest; the newer rollback restores the original enabled value even though the server accepted disabling. The switch promises suppression of message previews in the conversation list and notifications. The base's exception rollback restored the captured disabled value for this sequence. This is an inferred privacy-control consistency regression, not demonstrated notification disclosure; an error snackbar does not correct the misleading switch state.

Security review details

Security Blast Radius

• inferred — The demonstrated privacy-state discrepancy is reached through overlapping authenticated user actions for a selected room. The inspected path uses the conversation account's credentials, base URL and room token; it does not establish cross-account reachability, increased server privileges or broader tenant compromise.

Security Findings and Attack Paths

• inferred — The concern is a mismatch between the displayed privacy setting and an accepted server mutation, not an authentication bypass. Overlapping disable and enable operations with mixed outcomes can produce a false enabled indication. Actual preview disclosure depends on downstream behavior not verified in this review.

Trust Boundaries and Controls

• observed — Sensitive and insensitive operations retain POST and DELETE API calls carrying the Authorization header. The changed caller rejects explicit OCS refusal statuses, performs guarded rollback, and emits an error event. Missing status, zero and HTTP 200 are accepted; these controls improve isolated refusal handling but do not reconcile mixed concurrent outcomes.

Resilience and Maintainability Implications

• observed — Removal completion is not proof of successful reconciliation: the delete worker ignores API errors and returns success, while an offline or failed sync can finish without deleting the cached row. An empty full response also intentionally preserves existing rows. These worker and repository behaviors predate the PR; the new hiding does not establish durable deletion. Successful reconciliation applies deletion and upserts atomically.

Hardening Proposals

• proposed — Serialize or coalesce mutations by account, room and setting, or reconcile authoritative state after mixed outcomes before displaying a confirmed privacy value. Validate both success/failure orderings for sensitive settings rather than assuming that the newest initiated request is the newest accepted state.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 19.23% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 78 functions across 10 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the shared optimistic-action helper and the fixes for actions that failed silently.
Description check	✅ Passed	The description explains the changes, dependency, and tests. It includes the checklist and AI disclosure. The template’s TODO section and before/after screenshot table are omitted, but the description…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: aa64e2c5-e5ad-4a33-a5c5-9a28ff856db3

📥 Commits

Reviewing files that changed from the base of the PR and between 2930cee and ca9e10d.

📒 Files selected for processing (10)

• app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt
• app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt
• app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt
• app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt
• app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt
• app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt
• app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt
• app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt
• app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt
• app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

[coderabbitai]

Actionable comments posted: 2

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 75f1df26-e584-410c-be35-c93ea9648855

📥 Commits

Reviewing files that changed from the base of the PR and between ca9e10d and fdb65eb.

📒 Files selected for processing (3)

• app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt
• app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt
• app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

[coderabbitai]

Actionable comments posted: 2

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: ed7f6f22-e259-4945-a91f-1584f3533dbf

📥 Commits

Reviewing files that changed from the base of the PR and between fdb65eb and 4db6b67.

📒 Files selected for processing (5)

• app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt
• app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt
• app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt
• app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt
• app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: a0fc3847-19c8-4aa5-a024-568916ae8125

📥 Commits

Reviewing files that changed from the base of the PR and between 4db6b67 and 77b4de6.

📒 Files selected for processing (3)

• app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt
• app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt
• app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: cef3d6ce-a5fe-4d85-968d-013083d8e28a

📥 Commits

Reviewing files that changed from the base of the PR and between 77b4de6 and 24c3354.

📒 Files selected for processing (3)

• app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt
• app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt
• app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

[AndyScherzinger]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[AndyScherzinger]

For anyone reading the summary above: there are no open findings on this PR. All seven review threads are resolved, each with a reply pointing at the commit, the code and the test that covers it.

Two things in that summary are not open issues and should not be read as such:

The "Docstring Coverage" pre-merge check does not apply to this repository. It wants 80% of the 78 functions touched by this diff to carry a docstring. AGENTS.md says the opposite:

• Do not narrate self-explanatory code. If the code is readable without a comment, omit the comment.
• Keep comments brief — short and simple, matching the comment density of the surrounding code.

The functions in this diff that carry a non-obvious invariant are documented — optimisticAction, undoIfNewest, startSettingChange, accepted, reportSettingOutcome, onConversationRemoved, holdsShare. The rest are one-line accessors, test methods and copy() wrappers, where a docstring would be exactly the narration AGENTS.md asks us not to write. Raising coverage to 80% would mean writing worse code to satisfy a threshold this project has not adopted.

The "Merge Risk" and "Security architecture risk" banners are stale. Both are pinned to 24c33, which is two force-pushes behind; the branch is now at 43377cb11, rebased onto current master. The moderate rating cited an overlapping-settings race and a cancelled-deletion gap — both have since been fixed and both have regression tests (an older accepted toggle does not become the value a later failure restores, an acceptance that arrives while a newer change is in flight still becomes the baseline, and a CANCELLED branch in the delete observer).

@coderabbitai please re-review the current head so those two banners reflect 43377cb11 rather than 24c33.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
git rev-parse HEAD
fd --hidden --exclude .git '^AGENTS\.md$' --exec sh -c 'printf "\n--- %s ---\n" "$1"; cat "$1"' sh {}

Length of output: 13959

---

@AndyScherzinger, thanks for the clarification. I verified that the checked-out head is 43377cb11.

AGENTS.md confirms that comments must not narrate self-explanatory code. An 80% docstring target is not an appropriate basis for requesting extra comments in this repository.

I’ll request a full re-review of the current head. The review should reassess the Merge Risk and Security architecture risk banners against 43377cb11, not 24c33, including the overlapping-settings fixes and deletion cancellation handling you identified.

---

✏️ Learnings added

Learnt from: AndyScherzinger
URL: https://github.com/nextcloud/talk-android/pull/6715

Timestamp: 2026-10-10T16:43:13.882Z
Learning: In nextcloud/talk-android, AGENTS.md requires brief comments for behavior and constraints that code cannot express, such as non-obvious invariants. It prohibits comments that narrate self-explanatory code. The repository has not adopted an 80% docstring-coverage requirement; do not request redundant documentation for accessors, test methods, or copy() wrappers merely to meet that threshold.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

🧠 Learnings used

Learnt from: AndyScherzinger
Repo: nextcloud/talk-android PR: 6715
File: app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt:1036-1043
Timestamp: 2026-10-10T16:30:18.681Z
Learning: In Nextcloud Talk Android, app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt must update confirmedSetting in reportSettingOutcome according to the per-setting lastAcceptedChange order, not newestSettingChange. An accepted response must update the baseline when its change order exceeds the last accepted order, even if a newer change remains in flight. This lets a newer failed change restore the accepted baseline without allowing an older acceptance to overwrite a newer accepted value.

---

Learnt from: AndyScherzinger
Repo: nextcloud/talk-android

Timestamp: 2026-10-10T16:30:10.567Z
Learning: In Nextcloud Talk Android, DeleteConversationWorker deletes the conversation only on the server, not from the local database. In app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt, successful deletion must keep the pending-removal token until a full room sync removes the local entry. ConversationsListViewModel.onConversationRemoved(user, token) coordinates this behavior; clearing the token before the sync completes can make the deleted conversation reappear.

---

Learnt from: AndyScherzinger
Repo: nextcloud/talk-android PR: 6715
File: app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt:929-929
Timestamp: 2026-10-05T23:34:23.868Z
Learning: In Nextcloud Talk Android, app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt uses undoIfNewest to prevent older setting changes from rolling back newer changes. For important and sensitive toggles, rollback must restore confirmedSetting, not the displayed value captured before the change, because that displayed value may belong to another failed optimistic change. Seed confirmedSetting from the loaded conversation and update it only after an accepted response.

⚠️ Action not completed

Review rate limited.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 25 minutes.

[mahibi]

taking over

[github-actions]

Codacy

Lint

Type	master	PR
Warnings	138	138
Errors	15	15

SpotBugs

Category	Base	New
Bad practice	7	7
Correctness	8	11
Dodgy code	40	40
Internationalization	3	3
Malicious code vulnerability	3	3
Performance	8	8
Security	11	11
Total	80	83

SpotBugs increased!