File Shelf: the three follow-ups from the pre-merge review - #31
Merged
Conversation
1. A store file that exists but no longer decodes is moved aside before anything is written over it. File Shelf, Snippets and Clipboard History each read an undecodable file as an empty one, which is right for what to show but meant the store's first ordinary write landed on the same path and took the user's only copy with it. New `CorruptStoreFile.quarantine` renames it to `<name>.corrupt`, or `.corrupt-2` and up so a second casualty cannot clobber the first, and callers carry on exactly as before. The healthy path never runs it. Scratchpad is deliberately left alone: it salvages a damaged note in place and keeps the file, which is the better answer where the payload is the user's own prose. 2. Shelf resolution runs off the main actor. Every row is a security-scoped bookmark and resolving one asks its volume — twice where the unscoped retry is needed — so a full shelf on a sleeping or unmounted volume held the main actor for the whole pass at launch and again on every popover open. Rows now adopt immediately in a new `.resolving` availability and fill in behind the interface. `Task.detached` rather than a plain `Task`, since this type is `@MainActor` and an inherited-context task would run the work straight back on the actor it is meant to keep free. A generation counter makes an edit made mid-pass win over the answer to the older question, so a removal is never undone by a resolve that was already in flight. `.resolving` is drawn as an ordinary row, not a dim one: an entry that has not had its turn is not known to be gone, and dimming on the way in would flash the whole shelf grey at launch. The actions stay shut until the URL is known, so `looksReachable` now drives the styling where `isAvailable` still gates reveal and drag. 3. Launching with `--open` reveals the panel. On a first launch this process is the primary instance, so nothing posts the show notification that a second launch relies on, and the Toolbox tile appeared to do nothing but plant a menu-bar icon. Matches Network Info. Eleven tests added. The four existing store tests that asserted against synchronous resolution now await a new internal `settle()`; the change in what they wait for is the behaviour change, not a weakening of the assertion.
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/performance concerns in the new async resolve scheduling (task cancellation behavior and .resolving row icon handling) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This pull request addresses three follow-ups after File Shelf’s initial merge: preventing silent loss of corrupt store files, moving bookmark resolution off the main actor to avoid UI stalls, and making --open reveal the File Shelf panel on first launch.
Changes:
- Add package-wide “corrupt store file quarantine” behavior (rename to
<name>.corrupt,.corrupt-2, etc.) for File Shelf, Snippets, and Clipboard History. - Make File Shelf shelf-item resolution asynchronous with a
.resolvingavailability state and a generation check to prevent stale resolves from overwriting newer edits. - Ensure
--openshows the panel on the primary instance, and expand regression tests to cover the new behaviors.
File summaries
| File | Description |
|---|---|
| Tests/DMonteCoreTests/FileShelfStoreTests.swift | Updates tests for async resolution and adds coverage for .resolving + generation behavior. |
| Tests/DMonteCoreTests/CorruptStoreFileTests.swift | New tests covering quarantine rename semantics and “next write can’t clobber rescued bytes”. |
| Sources/DMonteFileShelfApp/FileShelfAppDelegate.swift | Shows the panel on first launch when --open is present. |
| Sources/DMonteCore/SnippetsKit.swift | Quarantines undecodable snippet libraries before returning []. |
| Sources/DMonteCore/FileShelfView.swift | Styles rows based on looksReachable and adds .resolving subtitle behavior. |
| Sources/DMonteCore/FileShelfStore.swift | Schedules resolution off-main, adds settle(), and guards against stale resolve adoption. |
| Sources/DMonteCore/FileShelfKit.swift | Adds .resolving, looksReachable, unresolved, and quarantines corrupt shelf files on load. |
| Sources/DMonteCore/CorruptStoreFile.swift | New helper implementing bounded, non-clobbering quarantine rename. |
| Sources/DMonteCore/ClipboardStore.swift | Quarantines undecodable clipboard history before proceeding. |
| CHANGELOG.md | Documents the fixes/changes for Unreleased. |
Review details
Suppressed comments (1)
Sources/DMonteCore/FileShelfStore.swift:267
resolveTask?.cancel()does not cancel the innerTask.detachedwork, so repeatedrefresh()calls can still queue multiple concurrent detached resolves (only discarding results at the end). Consider keeping a handle to the detached task and cancelling it when the outer task is cancelled so the expensive resolution work is actually stopped.
let resolved = await Task.detached(priority: .userInitiated) {
files.map { FileShelfKit.resolveItem($0) }
}.value
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
1. A `.resolving` row wore the missing-file glyph. `FileIcon` keys its real icon off a non-nil `url`, which an unchecked row does not have yet, so it fell through to `questionmark.folder` — the glyph that means "this is gone" — on every row for the length of a background resolve. That is the same thing the `.resolving` state exists to avoid, arriving by a different route than the dimming `looksReachable` already handled. Those rows now take the icon for their kind of file. Derived from the remembered file name via `UTType(filenameExtension:)`, not from `lastKnownPath`: asking the file system about the old path would borrow the icon of whatever sits there now, which is exactly what an unresolved row must not do, and is why the question-mark fallback was written that way in the first place. The decision is `ShelfItem.pendingIconFileName` so it is testable without a view. 2. `scheduleResolve` returned early on an empty shelf without cancelling the task already in flight. Nothing was corrupted — the generation guard discards a stale answer — but the work kept running for a result nobody would adopt, and `settle()` went on waiting for it. The cancel now happens before the empty check rather than after it. Three tests added: an unchecked row reports a kind-icon name, a resolved one reports none, and clearing the shelf leaves it clear across a resolve.
There was a problem hiding this comment.
🟡 Changes recommended
FileShelfStore.scheduleResolve uses an inner Task.detached whose work won’t be cancelled when resolveTask is cancelled, leaving expensive resolves running after refresh/clear.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
Cancelling a resolve stopped the waiting but not the work. `scheduleResolve` awaited an inner `Task.detached`, and a detached task does not inherit its parent's cancellation — so `resolveTask?.cancel()` on refresh, clear or termination left the expensive part running to the end regardless. That is the opposite of what the cancel was added for last round. The pass is now a `nonisolated static` async function instead. Both shapes get the work off this `@MainActor` type — a plain `Task` would inherit the actor and defeat the exercise entirely — but a `nonisolated async` function runs on the generic executor while staying inside the parent's cancellation scope, so the check is real. Verified rather than assumed: a temporary `dispatchPrecondition(condition: .notOnQueue(.main))` inside the function ran the whole store suite green, which it could not have done had the work still been on the main queue. Cancellation is checked per row rather than once at the top. The cost being abandoned is one bookmark resolution per entry against a volume that may be asleep, and the point is not to finish the other forty-nine. Two tests. The cancellation one is deterministic rather than racy — the pass waits until cancellation is visible before starting, so it asserts the check inside the loop rather than winning a scheduling coin flip — and a second test pins that an uncancelled pass still answers for every row, so the first cannot pass by never resolving anything.
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.
The three items left open when #30 merged — one per commit section, all with regression tests that fail against the code as it stands on
main.1. A corrupt store file is no longer overwritten silently
FileShelfKit.load,SnippetsKit.loadandClipboardStore.loadeach read an undecodable file as an empty one. That is right for what to show — there is nothing usable in it and nothing the user can do at that moment — but the store's first ordinary write then lands on the same path and takes the only copy with it.New
CorruptStoreFile.quarantinerenames it to<name>.corruptfirst, or.corrupt-2and up so a second casualty cannot clobber the first. Callers behave exactly as before; the healthy path never runs it.This was flagged as a package-wide policy question rather than a File Shelf bug, and it is fixed package-wide. Scratchpad is deliberately excluded — it already salvages a damaged note in place and keeps the file, which is the better answer where the payload is the user's own prose.
2. Shelf resolution moved off the main actor
Every row is a security-scoped bookmark, and resolving one means asking whatever volume it lives on — twice for an entry that needs the unscoped retry. Inline, that held the main actor for the whole pass at launch and again on every popover open; on a shelf near its 50-row cap pointing at a sleeping or unmounted network volume, that is a visible stall before the tray icon is even placed.
Rows now adopt immediately in a new
.resolvingavailability and fill in behind the interface.Task.detachedrather than a plainTask, sinceFileShelfStoreis@MainActorand an inherited-context task would run the work straight back on the actor it is meant to keep free.A generation counter makes an edit made mid-pass win over the answer to the older question, so a removal is never undone by a resolve that was already in flight.
.resolvingis drawn as an ordinary row rather than a dim one — an entry that has not had its turn is not known to be gone, and dimming on the way in would flash the whole shelf grey at launch.looksReachablenow drives the styling;isAvailablestill gates reveal and drag, so the actions stay shut until the URL is known.3.
--openreveals the panelOn a first launch this process is the primary instance, so nothing posts the show notification that a second launch relies on, and opening File Shelf from the Toolbox tile did nothing visible but plant a menu-bar icon. Now matches
NetworkInfoAppDelegate.Tests
1166 → 1177, all passing.
.corrupt-2not clobbering.corrupt, a healthy file never renamed, a missing file not mistaken for corruption, and that the store's next write cannot destroy what was rescued.refresh()returning before the disk does, and an edit surviving an in-flight pass.Verified as genuine regressions by reverting each fix and watching them fail.
The four existing store tests that asserted against synchronous resolution now await a new internal
settle(). What they wait for changed; what they assert did not.Not covered by tests: item 3 —
FileShelfAppDelegateis in the app target, which has no test target.