Repository navigation
fix(files): always finish the delete dialog after the server result - #498
veryCrunchy wants to merge 32 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".
Creating an item or moving one into a folder whose delete is still unverified now waits for that delete, like every other write to the folder and its contents. Throttled, stale-version, and rejected-credential or permission outcomes are recorded per resource in the account-session coordinator. Reopening the dialog no longer lifts them; only a server listing of the item's folder that started after the outcome does. HTTP 401 and 403 now block retry without a folder reload, while 423 and 507 still allow a retry because they can clear without changing the item. A desktop follow-up that drops one path now reports a local failure even while another path is still running. After service teardown the drain worker abandons every queued refresh as soon as the blocked one returns.
…alog-completion # Conflicts: # tools/kotlin-file-size-baseline.txt # website/public/screenshots/capture-manifest.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51efc0321e
ℹ️ 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".
…-ups A delete outcome that needs a folder refresh (stale version, throttling, rejected credentials or permission, unverifiable result) now blocks every other write to that item, its folders, and its contents until a server listing of its folder starts after the outcome, including overlapping deletes. Renames, moves, and copies in Files, and moves, copies, and deletes in the media viewer, now report when local file status is still updating or could not be updated instead of announcing plain success. On desktop, sign-out and account removal drop that account's queued local refreshes and fail the waiters of its running one, so a late completion cannot report success. An interrupted mutation whose local reconciliation cannot be queued now says so instead of being dropped silently.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d41fe6e914
ℹ️ 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 delete started from Favorites or search whose outcome needs a folder refresh now reads the item's own folder through the account-scoped delete coordinator, so the block lifts even when that folder is not the one shown. Only a listing that started after the outcome lifts it, and opening the folder still lifts it when that read fails. Disabled Files actions now show the typed reason: an unverified delete asks the user to wait, while a finished delete that needs a refresh names the folder to open or refresh. On desktop, the refresh queued when the server accepts a mutation and the wait after confirmation now share one entry per path. Confirmation attaches to that refresh while it is queued or running and reads its result once it finished, so a path is refreshed once and a full queue no longer reports a failure for a refresh that is already running.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 428e3e8730
ℹ️ 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".
…dinator The media viewer's delete now starts in the account's shared FileDeleteCoordinator, the same owner as a Files delete. It registers the same in-flight and refresh blocks for every screen, verifies an unknown result, follows the same retry, throttling, credential, and identity rules, reports incomplete local follow-up, and is fenced by account, item, and generation. The viewer hosts the delete for its shown item, so an outcome that arrives after the dialog closed, or after the viewer was recreated, is still applied. The item leaves the viewer only when the server confirmed the delete or the item is no longer at its path; any other outcome keeps it and shows the reason. The separate media delete dialog is removed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d94984a973
ℹ️ 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".
…e test waits The media viewer now watches delete outcomes for every item in its list, not only the shown one. A confirmed or no-longer-at-location outcome hides that item whenever it arrives, even after the dialog was closed and the user moved on; when the shown item goes, the next item, or else the previous one, is selected, and the viewer closes after its last item. Any other outcome keeps the item and shows its reason once that item is shown. Swiping is never blocked. Delete tests that need work to outlive a scene now run the delete owner on a dedicated test thread instead of the shared default dispatcher, and every wait on a delete, completion, or block is bounded and fails with a message.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2eab6909fb
ℹ️ 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".
…le ID After an unknown delete result, the verification listing now offers a retry only for positively the same item: matching stable file IDs, or matching type and ETag only when neither the shown item nor the listed one has an ID. A differing ID, or an ID on only one side, requires a refresh, so a replacement at the same path is never deleted by a retry. The rule is shared by Files and media viewer deletes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e92f8914e
ℹ️ 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".
…er removals An item found gone after an unknown delete result now reports the local reconciliation queued for that result: the desktop follow-up, including still running, failed, or dropped, and the Android cache invalidation. Media viewer removals are applied to the process-level navigation route, so a recreated viewer does not show a deleted item again. Each removal issues a new route key, so the viewer updates even when the selected index stays the same.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4305e6e61d
ℹ️ 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".
…hare uploads The media viewer now watches deletes for every RAW and rendered stack member, not only the navigation covers. Deleting a stack cover rebuilds the stack in place around a surviving member using the existing stacking rule; an item leaves only when no member survives. A queued Android share upload blocked by a destination delete, at start or between files, records a typed wait on the durable request. Its progress asks the user to open or refresh the destination folder instead of describing a network wait, and starting the upload clears it.
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.