Repository navigation
fix(files): always finish the delete dialog after the server result - #498
veryCrunchy wants to merge 6 commits into
Conversation
A server-confirmed WebDAV delete ran its local cache and virtual-file bookkeeping inline before returning. That bookkeeping blocks on the desktop virtual-file provider monitor, which Windows Cloud Files code holds across long provider operations, so the dialog could stay on "Deleting..." indefinitely although the server had already deleted the file. A monitor wait cannot be cancelled or interrupted. Desktop file mutations now run that bookkeeping on one ordered background worker, wait at most three seconds, and report the local result separately from the server result. Unknown results queue the bookkeeping without blocking failure or cancellation. The Files delete dialog now uses a fenced state holder: a definitive rejection allows a safe retry, an unknown or server-failure result is resolved by reading the parent folder instead of resending the delete, and the dialog can be closed while the delete finishes without a later completion changing a newer dialog.
Obiente previewNC Native · Obiente updates this comment as the preview changes. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8129163ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A 412 from an ETag-guarded delete now requires a folder refresh before another attempt, since resending the same precondition cannot succeed. HTTP 429 is a typed throttled file error. A throttled delete is not treated as an unknown result, starts no verification read or folder reload, and keeps retry blocked. File mutations do not expose Retry-After, so no delay is invented. Unresolved deletes are tracked per account and remote path outside the dialog. Closing and reopening Delete no longer allows a second request for the same item until the first reaches a verified outcome, and a reopened dialog for the same version receives that outcome.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 374104f0dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Files deletes now run in an account-session coordinator instead of the Files screen's coroutine scope. Leaving the dialog or the screen no longer cancels a delete or releases its resource before a verified outcome; a returning Files screen applies the finished outcome, and only account session teardown cancels unfinished work. While a delete is unverified, every remote write to that item, its folders, and its children is disabled in the file menu with a reason and rejected by the Files action handler. A parent listing that no longer contains the path after an unknown result is reported as no longer in this folder, not as deleted. When the same file ID appears under another name in that listing, the result requires a refresh. Desktop local follow-ups are coalesced per account and path behind one drain worker, and at most 256 paths wait behind a stalled refresh. Paths beyond that bound are reported as a local failure instead of growing an unbounded queue.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7a6555f5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (action.changesRemoteItem() && fileDeletes.blocksWrites(file)) { | ||
| mutationError = "Wait until the delete of ${file.name} or its folder finishes." | ||
| return |
There was a problem hiding this comment.
When an unverified delete targets a folder, this guard covers only per-item FileMenuActions. After closing the delete dialog, the user can still open that folder and use the workspace's unguarded Create action; if the creation reaches the server before the recursive DELETE finishes, the newly created content can be deleted without warning. Fresh evidence in the final tree is that NativeFilesWorkspace.onCreate remains outside this coordinator check, so current-path creation and other direct write entry points must also consult the overlapping delete state.
AGENTS.md reference: AGENTS.md:L401-L402
Useful? React with 👍 / 👎.
| private fun reset(file: NextcloudFile?) { | ||
| attachedToken = null | ||
| target = file | ||
| error = null | ||
| retryBlocked = false |
There was a problem hiding this comment.
After a 429 completion sets retryBlocked, dismissing and reopening the same item's dialog calls this reset and clears the block; the coordinator has also released its in-flight claim, so Delete can immediately send the same request again without waiting or refreshing. Fresh evidence in the final tree is this reset path, which defeats the newly typed throttled outcome even though the in-dialog test passes. Persist the throttling state per resource until an explicit refresh or an appropriate delay has occurred.
AGENTS.md reference: AGENTS.md:L294-L296
Useful? React with 👍 / 👎.
| withTimeoutOrNull(completionWaitMillis) { waiters.awaitAll() } | ||
| ?.let { results -> results.firstOrNull { it != FileMutationLocalFollowUp.Completed } } | ||
| ?: if (waiters.all { it.isCompleted }) FileMutationLocalFollowUp.Completed else FileMutationLocalFollowUp.StillRunning |
There was a problem hiding this comment.
When a multi-path mutation is submitted with only one queue slot left, one waiter can be accepted while another is immediately completed as Failed; if the accepted refresh remains blocked until this timeout, the Elvis branch returns StillRunning and discards the known failure. The caller then tells the user that local state is still updating even though one affected path was permanently dropped and will never be refreshed. Track any failed waiter separately and return Failed for this partial result.
AGENTS.md reference: AGENTS.md:L305-L306
Useful? React with 👍 / 👎.
| "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/FileDeleteDialog.kt": "b70e3b4fa9754b6af6bdae92e70f6d5392560e9e70c99794463b126cc33b307b", | ||
| "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/FileDeleteFlow.kt": "cffbc353d49c20342046b598e2eaead8cbf5b3d810739826dd120b2019e608eb", |
There was a problem hiding this comment.
The committed capture inventory is stale: the hashes recorded here for FileDeleteDialog.kt and FileDeleteFlow.kt do not match their final contents, several other modified capture sources also mismatch, and the newly added FileDeleteCoordinator.kt is absent from both captureSources and captureSourceHashes even though discovery includes the entire ui/src/commonMain/kotlin directory. The capture freshness verifier therefore rejects this manifest, and the screenshots are not reproducibly tied to the reviewed source until they are regenerated.
AGENTS.md reference: AGENTS.md:L107-L109
Useful? React with 👍 / 👎.
| NextcloudFileOperationError.AuthenticationRequired, | ||
| NextcloudFileOperationError.PermissionDenied, | ||
| NextcloudFileOperationError.Locked, | ||
| NextcloudFileOperationError.InsufficientStorage, | ||
| -> return FileDeleteOutcome.NotDeleted(failure.message ?: "Could not delete ${target.name}.") |
There was a problem hiding this comment.
When DELETE returns 401 or 403, this branch converts the response to NotDeleted; FileDeleteDialogState.complete consequently leaves retryBlocked false, so the Delete button remains enabled even though the credentials or permission state must be corrected before another write is valid. Pressing it simply repeats the unauthorized request with the same stale authorization context. Return a blocked recovery outcome for these errors and require reauthentication or a permission refresh before enabling another write.
AGENTS.md reference: AGENTS.md:L401-L402
Useful? React with 👍 / 👎.
| private fun drain() { | ||
| while (true) { |
There was a problem hiding this comment.
If DesktopNextcloudServices.close() cancels serviceScope while entry.refresh() is blocked, the coroutine is marked cancelled but this non-suspending loop never observes that state. Once the blocking refresh returns, it continues executing every queued cache/provider refresh, and the invokeOnCompletion cleanup cannot clear the queue until the loop exits; up to the full pending bound can therefore run after teardown, racing provider closure and extending shutdown. Check cancellation between entries and abandon the remaining queue as soon as the current non-cancellable refresh returns.
AGENTS.md reference: AGENTS.md:L353-L355
Useful? React with 👍 / 👎.
This branch was successfully deployed
1 participant
Outcome
Advances #477.
On Windows with virtual files enabled, the delete dialog could stay on "Deleting..." forever even though the server had deleted the file. After the server accepted the DELETE,
executeFileMutationran local cleanup inline (refreshRetainedFoldersAfterMutation), which waits onsynchronized(virtualFileProviderLock). That wait cannot be cancelled, and Windows Cloud Files can hold the lock for a long time: storage summaries, corrupt-root recovery, and provider activation with startup recovery of up to 120 s. The unknown-result path ran the same refresh synchronously.mainstill has this structure.Desktop local follow-up:
DesktopFileMutationFollowUps.kt). After a server-confirmed change the caller waits at most 3 s, then gets the server result. The local outcome is reported separately aslocalFollowUp(Completed, StillRunning, Failed); it defaults to Completed, so Android is unchanged. Slow or failed cleanup records afiles.mutation-local-follow-updiagnostic.refreshRetainedFoldersAfterMutationnow returns the first failure it recovered from instead of swallowing it. Other callers ignore the return value.Shared delete flow (
FileDeleteFlow.kt,FileDeleteCoordinator.kt):Retry-Afteris not honored because file mutations do not expose it; no delay is invented.FileDeleteCoordinatorscoped to the account session, not by the Files screen. Leaving the screen does not cancel the request or release the item; finished outcomes wait until a Files screen picks them up, which shows the notice and reloads the folder. Session teardown cancels unfinished work.Documents2vsDocumentsare not parents). In the file menu the write actions (edit, favorite, version history, rename, move, copy, share, delete) are disabled with the visible reason "Wait until the delete of this item or its folder finishes."; open, preview, details, download, send copy and offline stay available. The action handler rejects the same writes.FileDeleteDialog.kt.NextcloudNativeApp.ktto 11026,DesktopNextcloudServices.ktto 6164.Verification
bash tools/check-repository.shpasses (the Debian package check was skipped locally becausedpkg-debis missing; CI runs it)changes/unreleased/fragment records the changeRun on Windows with JDK 21:
:ui:desktopTest(FileDeleteFlowTest21,DesktopFileMutationFollowUpsTest7,FilesRequestStateTest5,FileOperationsTest17,FileActionPlanningTest18,FilesListInteractionTest1,FilesSelectionSemanticsTest1): pass.:ui:desktopTest: 3799 tests, 0 failures, 69 skipped.:androidApp:testDebugUnitTest: 1167 tests, 0 failures.bash tools/check-kotlin-architecture.shandnode tools/changelog-fragments.mjs validate: pass.FileDeleteFlowTest(21) covers success, local cleanup still running or failed, definitive rejection and safe retry, 412 blocking retry, 429 without verification or immediate retry, dialog dismissal keeping the item blocked, leaving the screen during verification and during desktop local cleanup (the outcome is delivered when a screen returns), session teardown cancelling and releasing without an outcome, late completion after teardown publishing nothing, write blocking for the item, parents and children (not name-prefix siblings) scoped per account, path absence not reported as a delete, a same-ID rename, cancellation, stale results, missing ETag, double submit and folder preconditions.DesktopFileMutationFollowUpsTest(7) covers completion, failures, cleanup stuck on an uninterruptible lock returning StillRunning within the bound, 1,020 mutations behind a stall leaving only 10 coalesced entries that each run once with the newest refresh, the path cap reporting dropped follow-ups, caller cancellation and a closed service scope.FileActionPlanningTestchecks that write actions are disabled with the reason while details stays enabled.No Windows native or JNA code changed, so no Windows packaging build was run.
Compatibility and risk
executeFileMutationagainst a mock server while the provider lock is held; that boundary is covered throughDesktopFileMutationFollowUps.refreshRetainedFoldersAfterMutationandscheduleVirtualFolderHydrationtakevirtualFileProviderLockandresourceActivationMonitorin opposite orders.Visual changes
The dialog layout is unchanged; it now always reaches a result or error state. Write actions in the file menu are disabled with a visible reason while a delete of that item or a parent folder is unverified. Screenshots not yet captured.