Handling (Un)Visited Directories Correctly - #10751
Conversation
|
@juliusvaart Please ensure this pull request is based on |
a730381 to
59cf0ea
Compare
|
Now based on |
claucambra
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The code changes themselves look fine to me, but I have issues with the commenting in this PR. In general too much commenting and too verbose (typical of Claude 🙂)
aafad6c to
deff2ad
Compare
claucambra
left a comment
There was a problem hiding this comment.
One last small comment
deff2ad to
96773ba
Compare
96773ba to
318542c
Compare
|
/backport to stable-34.0 |
|
Artifact containing the AppImage: nextcloud-appimage-pr-10751.zip Digest: To test this change/fix you can download the above artifact file, unzip it, and run it. Please make sure to quit your existing Nextcloud app and backup your data. |
|
Artifact containing the Flatpak bundle: com.nextcloud.desktopclient.nextcloud.flatpak.zip Digest: To test this change/fix you can download the above artifact file, unzip it, and install the bundle with: Please make sure to quit your existing Nextcloud app and backup your data. |
|
@juliusvaart Please rebase your branch, run |
A folder created on the Mac is, from the framework's point of view, already fully enumerated: it knows the empty listing it just created and will not ask for the contents again. The refresh subscription a browsed folder gets must therefore be granted here too — `visitedDirectory` is what puts a directory into the materialised set the working-set scan reads, and `downloaded` is ignored for directories there. Without it a locally created folder is never PROPFINDed by the scan, so items added to it on the server (web UI, public upload link, another user) never surface, and no later `enumerateItems` exists to repair that. Resolves: nextcloud#9688 Signed-off-by: Julius van der Vaart <julius@vanderva.art> Assisted-by: Claude Code:claude-opus-5
Browsing into a folder that had never been enumerated showed an empty view and a spinner that never resolved. Finder's status bar read "0 items" for a folder holding three subfolders on the server, and the framework never scheduled a fetch for its children -- it logged a new enumerator and then asked it for nothing at all, not even a sync anchor, while the extension sat idle. `childItemCount` derived the number from the local database, which holds a directory's children only once something has read that directory. For one nobody had read it therefore returned 0. But `NSFileProviderItem.childItemCount` is optional precisely so a provider can say it does not know, and 0 is not "I do not know", it is "this folder is empty". Finder printed that verbatim, and the framework, told the container had no children, had no reason to ask for any. Report nil whenever the local set is empty. There is no flag available to distinguish a genuinely empty directory from an unread one -- `visitedDirectory` is still false after a paginated enumeration, which is the path every server from Nextcloud 31 takes -- so an empty local set cannot support the stronger claim. The cost is one enumeration of a truly empty folder, which the framework performs on first browse anyway. The cost of the previous answer was the folder never being enumerated at all. `delete` had been reading nil as "empty or a file", which was safe only while nil meant a file. It now asks whether the item is a directory, so a directory whose contents are unknown refuses a non-recursive delete rather than assuming it has nothing to lose. Two existing tests asserted the old 0. Their expectations move to nil, with the reason recorded at the assertion. Signed-off-by: Julius van der Vaart <julius@vanderva.art> Assisted-by: Claude Code:claude-opus-5
`visitedDirectory` marks a directory as actually read. Two things depend on it: membership of the working set the remote-change scan walks, and whether an empty child count means "this folder is empty" or "nobody has looked". The flag is local-only, so it takes no part in `isInSameDatabaseStoreableRemoteState`. The depth-1 ingestion decided whether to persist its read target purely on that comparison, so reading a directory whose remote state had not changed persisted nothing and dropped the visit. That is not an edge case: a directory's etag does not change because somebody opened it, so the ordinary browse recorded nothing. The paginated ingestion path already handled this -- `addItemMetadataPreservingLocalState` compares the flag explicitly -- so only the non-paginated path was affected. Record the visit separately rather than by adding the row to `metadatasToUpdate`. That set is also the change set handed to the framework, and announcing a directory as changed every time it is first browsed would re-queue its update-item job for a flag the framework cannot see. Signed-off-by: Julius van der Vaart <julius@vanderva.art> Assisted-by: Claude Code:claude-opus-5
318542c to
9cb8f13
Compare
Summary
Three fixes under one invariant: a directory whose contents are known must say
so, and one whose contents are unknown must not pretend otherwise.
visitedDirectoryis the flag that carries this. Two things depend on it —whether a directory is in the working set the remote-change scan walks, and
whether an empty local child set means "this folder is empty" or "nobody has
looked yet". Each commit fixes one place where the flag, or the answer derived
from it, was wrong.
Problems
A folder created on the Mac never received server-side additions. Files
added to it through the web UI, a public upload link, or by another user never
appeared. The framework considers such a folder already enumerated — it knows
the empty listing it just made and will not ask again — so if nothing marks it
visited it never joins the materialised set, the working-set scan never
PROPFINDs it, and no later
enumerateItemsexists to repair that.Resolves #9688.
Browsing into a never-enumerated folder showed an empty view and a spinner
that never resolved. Finder's status bar read "0 items" for a folder holding
three subfolders on the server.
childItemCountderived the number from thelocal database, which holds a directory's children only once something has read
that directory — so for an unread one it returned 0.
NSFileProviderItem.childItemCountis optional precisely so a provider can sayit does not know, and 0 is not "I do not know", it is "this folder is empty".
Finder printed that verbatim, and the framework, told the container had no
children, had no reason to schedule a fetch. The extension sat idle because we
had told the system there was nothing to ask for.
An ordinary browse persisted no visit at all.
visitedDirectoryislocal-only, so it takes no part in
isInSameDatabaseStoreableRemoteState. Thenon-paginated depth-1 ingestion decided whether to persist its read target
purely on that comparison, and a directory's etag does not change because
somebody opened it — so reading an unchanged directory dropped the visit. The
paginated path already compared the flag explicitly and was unaffected.
Changes
this Mac the same refresh subscription a browsed folder gets.
nilrather than 0 when the local set is empty and the directory has not been
read.
deletehad been readingnilas "empty or a file", which was safeonly while
nilmeant a file; it now asks whether the item is a directory,so a directory whose contents are unknown refuses a non-recursive delete
instead of assuming it has nothing to lose.
separately rather than by adding the row to
metadatasToUpdate. That set isalso the change set handed to the framework, and announcing a directory as
changed on every first browse would re-queue its update-item job for a flag
the framework cannot see.
Testing
Full suite green at 356 XCTest + 59 Swift Testing. Each commit builds and tests
standalone. New tests:
ChildItemCountTests(107 lines),VisitedDirectoryTests(120 lines); each was run against a deliberately broken build first to confirm it
fails without the fix. Two existing
EnumeratorTestsexpectations asserted theold 0 and move to
nil, with the reason recorded at the assertion.Note for anyone testing this by hand: the framework caches every item's
NSFileProviderItemand only refreshes that cache when the extension'sCFBundleShortVersionStringchanges. Rebuilding at the same version leaves thepre-fix
childItemCountvalues in place and the fix appears to do nothing. Bumpthe version between builds.
Checklist
AI (if applicable)