Handling (Un)Visited Directories Correctly - #10751
Open
juliusvaart wants to merge 3 commits into
Open
Conversation
juliusvaart
requested review from
camilasan,
claucambra,
i2h3,
mgallien and
nilsding
as code owners
September 6, 2026 19:40
Collaborator
|
@juliusvaart Please ensure this pull request is based on |
juliusvaart
force-pushed
the
macos/vf/directory-visits
branch
from
September 7, 2026 13:39
a730381 to
59cf0ea
Compare
Contributor
Author
|
Now based on |
claucambra
requested changes
Sep 8, 2026
claucambra
left a comment
Collaborator
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 🙂)
juliusvaart
force-pushed
the
macos/vf/directory-visits
branch
from
September 8, 2026 06:29
59cf0ea to
aafad6c
Compare
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
juliusvaart
force-pushed
the
macos/vf/directory-visits
branch
from
September 8, 2026 06:50
aafad6c to
deff2ad
Compare
12 tasks
claucambra
requested changes
Sep 8, 2026
claucambra
left a comment
Collaborator
There was a problem hiding this comment.
One last small comment
`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
juliusvaart
force-pushed
the
macos/vf/directory-visits
branch
from
September 8, 2026 16:49
deff2ad to
96773ba
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)