Add File Shelf, a drag-and-drop staging area for files - #30
Conversation
|
@copilot pls do a review |
Here's the review: Architecture The layer separation is solid: Things done well
Minor observations
Overall the implementation is well-reasoned, the edge cases are handled explicitly rather than silently, and the test coverage directly exercises the promises the tool makes. Nothing blocking. |
1. DroppedURLCollector had no deadline. A multi-file drop into the popover waits for every NSItemProvider before shelving anything, but a provider is another application's promise to call back — a wedged loader or a source app that dies before its XPC reply lands never answers, `remaining` never reaches zero, and the whole drag is swallowed in silence, including the files that arrived perfectly. Added a 10 s deadline that flushes whatever turned up, plus a `hasShelved` latch so the batch is delivered exactly once. 2. DroppedURLCollector.accept counted a repeated answer for the same slot as a new one. A provider that calls its completion handler twice ran the outstanding count past the last file, shelving the batch short. Answers are now tracked per slot, so a duplicate is ignored rather than miscounted. 3. FileShelfStore.shelve accepted non-file URLs. Only the menu-bar overlay filters its drag (urlReadingFileURLsOnly); the popover asks NSItemProvider for "a URL" and takes whatever representation it prefers. A URL is reduced to its path when it is shelved, and the path of https://example.com/tmp is /tmp — which exists, so the row came out *available* carrying the local /tmp's device/inode, and a genuine drop of that folder would collapse onto a web link. shelve now filters on isFileURL. 4. StatusItemFileDropView.fileURLs(from:) was internal but referenced only inside its own file; made it private. Reviewed and deliberately left alone: security-scoped access is balanced correctly across add, replace, remove, clear and termination; the registry wiring (Package.swift, ToolboxCatalog, AppPreferences, package_app.sh, FileShelfInfo.plist) matches the sibling tools and the repo's own ToolRegistrationConsistencyTests; the corrupt-shelf and unversioned-schema behaviour matches every other persisting tool in this package. Tests: four new cases in FileShelfStoreTests covering the timed-out batch, the deadline not re-delivering a completed batch, the double answer, and the web address that names a real local path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Moving a file between two apps, or between two Spaces, currently means keeping a Finder window parked somewhere just to hold it. File Shelf is that holding area without the window: drop files onto the menu bar icon or into the popover, then drag them straight back out wherever you land. Shelving deliberately does not copy bytes. Each entry is a security-scoped bookmark to the original file, which buys three things a list of paths could not: parking a 40 GB video costs the same as parking a note, the shelf survives a relaunch, and an entry keeps working after the file is renamed or moved — exactly the tidying people do while something is parked on its way elsewhere. Dragging out hands over the real file URL, so the receiving app attaches or opens the original rather than a duplicate. A file that has been deleted, or that lives on a disk that is currently unplugged, stays on the shelf and is drawn as unavailable. The alternative — pruning it — makes the shelf look like it lost the thing the user parked, and an unplugged disk comes back on its own. De-duplication compares device and inode rather than paths, so the same file reached through a symlink, or dropped again after a rename, collapses onto the row it already has. Bookmarks are resolved with `.withoutUI` and `.withoutMounting`: without them, a routine refresh of an entry on an unplugged disk pops a system "please insert the disk" panel out of a background menu-bar app. The menu-bar icon is a drop target in its own right, which is the point of the tool — park a file without opening anything. AppKit creates NSStatusBarButton and its dragging-destination methods are overrides rather than delegate callbacks, so the only way in is a transparent child view that registers for file URLs and hands every mouse event straight back to the button underneath; without that pass-through the overlay would be a dead zone over the icon. The rules that decide what the shelf holds — ordering, de-duplication, the cap, bookmark resolution and the on-disk format — live in FileShelfKit as pure functions over values, and the tests drive real bookmarks against files in a temporary directory. A stubbed resolver would have asserted nothing about the one question that matters: whether a bookmark actually follows a rename.
Three things the drop overlay and the shelf logic were getting wrong. The overlay that turns the status button into a drag target wins hit-testing over the button, and NSView answers false to acceptsFirstMouse where NSStatusBarButton answers true. This tool is LSUIElement and never becomes the active app, so every click on its icon is a first-mouse click: AppKit was free to spend that click on activation instead of delivering it, and the mouseDown pass-through that opens the popover never ran. The only other two status-button subviews in this package already override this for exactly the same reason. De-duplication compared a dropped file against identities cached on `items`, which were computed at the last resolve — startup, or the last popover open. A device/inode pair does not survive a safe save, and a drop straight onto the menu-bar icon never opens the popover, so shelving a file, editing it in any app that writes atomically, and dropping it again found no match and minted a second row. Nothing collapsed the two afterwards. shelve() now re-resolves first, through the same helper refresh() uses, so a drop is judged against the shelf as it is rather than as it was. Finally, .missing was unreachable. A bookmark to a deleted file, or to one on an unmounted volume, does not resolve at all — .withoutMounting sees to the second — so every failure landed on .unresolvable, whose copy tells the user the entry has to be shelved again. Someone who merely unplugged an external disk would have removed a row that was about to start working by itself. The two cases are now told apart by reading the bookmark's own cached resource values, which survive while the file does not; junk data carries none.
1. DroppedURLCollector had no deadline. A multi-file drop into the popover waits for every NSItemProvider before shelving anything, but a provider is another application's promise to call back — a wedged loader or a source app that dies before its XPC reply lands never answers, `remaining` never reaches zero, and the whole drag is swallowed in silence, including the files that arrived perfectly. Added a 10 s deadline that flushes whatever turned up, plus a `hasShelved` latch so the batch is delivered exactly once. 2. DroppedURLCollector.accept counted a repeated answer for the same slot as a new one. A provider that calls its completion handler twice ran the outstanding count past the last file, shelving the batch short. Answers are now tracked per slot, so a duplicate is ignored rather than miscounted. 3. FileShelfStore.shelve accepted non-file URLs. Only the menu-bar overlay filters its drag (urlReadingFileURLsOnly); the popover asks NSItemProvider for "a URL" and takes whatever representation it prefers. A URL is reduced to its path when it is shelved, and the path of https://example.com/tmp is /tmp — which exists, so the row came out *available* carrying the local /tmp's device/inode, and a genuine drop of that folder would collapse onto a web link. shelve now filters on isFileURL. 4. StatusItemFileDropView.fileURLs(from:) was internal but referenced only inside its own file; made it private. Reviewed and deliberately left alone: security-scoped access is balanced correctly across add, replace, remove, clear and termination; the registry wiring (Package.swift, ToolboxCatalog, AppPreferences, package_app.sh, FileShelfInfo.plist) matches the sibling tools and the repo's own ToolRegistrationConsistencyTests; the corrupt-shelf and unversioned-schema behaviour matches every other persisting tool in this package. Tests: four new cases in FileShelfStoreTests covering the timed-out batch, the deadline not re-delivering a completed batch, the double answer, and the web address that names a real local path.
150954b to
a3d9ee2
Compare
# Conflicts: # CHANGELOG.md # Package.swift # Scripts/package_app.sh # Sources/DMonteCore/AppPreferences.swift # Sources/DMonteCore/ToolboxCatalog.swift
1. DroppedURLCollector had no deadline. A multi-file drop into the popover waits for every NSItemProvider before shelving anything, but a provider is another application's promise to call back — a wedged loader or a source app that dies before its XPC reply lands never answers, `remaining` never reaches zero, and the whole drag is swallowed in silence, including the files that arrived perfectly. Added a 10 s deadline that flushes whatever turned up, plus a `hasShelved` latch so the batch is delivered exactly once. 2. DroppedURLCollector.accept counted a repeated answer for the same slot as a new one. A provider that calls its completion handler twice ran the outstanding count past the last file, shelving the batch short. Answers are now tracked per slot, so a duplicate is ignored rather than miscounted. 3. FileShelfStore.shelve accepted non-file URLs. Only the menu-bar overlay filters its drag (urlReadingFileURLsOnly); the popover asks NSItemProvider for "a URL" and takes whatever representation it prefers. A URL is reduced to its path when it is shelved, and the path of https://example.com/tmp is /tmp — which exists, so the row came out *available* carrying the local /tmp's device/inode, and a genuine drop of that folder would collapse onto a web link. shelve now filters on isFileURL. 4. StatusItemFileDropView.fileURLs(from:) was internal but referenced only inside its own file; made it private. Reviewed and deliberately left alone: security-scoped access is balanced correctly across add, replace, remove, clear and termination; the registry wiring (Package.swift, ToolboxCatalog, AppPreferences, package_app.sh, FileShelfInfo.plist) matches the sibling tools and the repo's own ToolRegistrationConsistencyTests; the corrupt-shelf and unversioned-schema behaviour matches every other persisting tool in this package. Tests: four new cases in FileShelfStoreTests covering the timed-out batch, the deadline not re-delivering a completed batch, the double answer, and the web address that names a real local path.
Add File Shelf, a drag-and-drop staging area for files
Wave 2 Phase A.
Implementation: Add File Shelf, a drag-and-drop staging area for files
Review fixes: File Shelf: make the icon clickable and stop re-drops forking a row
Built by an isolated agent, then adversarially reviewed by an independent agent that checked the branch out and ran
swift build+ the full suite itself. Every review finding was fixed, with a regression test added for each high and medium.