Skip to content

[EPIC-09] Folder browser: lazy file tree, FS watching, CRUD - #38

Open
Joncallim wants to merge 1 commit into
masterfrom
epic/09-folder-browser
Open

[EPIC-09] Folder browser: lazy file tree, FS watching, CRUD#38
Joncallim wants to merge 1 commit into
masterfrom
epic/09-folder-browser

Conversation

@Joncallim

Copy link
Copy Markdown
Owner

EPIC-09 — Folder browser: lazy file tree, FS watching, CRUD

Refs #10 · Milestone M4 — Workspace & formats · Depends on E01, E02, E03 (all merged) + E08 as built

Status: architecture pass only — no implementation yet. This PR carries the plan; the implementation, review, and fixes all land on this same branch. Same shape as #31, #32 and #33.

The plan in one paragraph

The FileTree target, its FileCore dependency, its test target, and its app-target entry all already exist from E00 scaffolding, and ⌘⇧O already opens a folder and stores the root per window — so this epic is filling in a stub with everything the opened folder was supposed to do, with no Package.swift, project.yml or ci.yml diff. The module keeps the FileCore rule the issue asks for ("pure synchronous model core, async edges") literally: there are exactly two IO seams — DirectoryReading and FileSystemMutating — plus the watcher, and filtering, sorting, diffing, flattening, name uniquing and every CRUD precondition are pure functions of [DirectoryEntry]. That is what lets sort/filter/diff/naming be tested with no temp directory and no async, leaves only the reader/mutator/real-watcher touching disk, and makes the acceptance boxes assertions about named states rather than about timing. Watching is one DispatchSource per expanded directory with the fd closed in the cancel handler, instrumented by a watchedDirectoryCount that the tests drive to zero — because "no fd leaks, instrumented" is an acceptance box and lsof is not a unit test. A filesystem event carries no payload, so every event re-lists and diffs; the diff is load-bearing rather than an optimisation, since MacDown's own atomic save fires the watcher several times per save on a directory whose listing never changed.

Three things this epic cannot be built without, none of them visible from the spec

1. The sidebar List has exactly one selection binding, and E08 owns it. SidebarView binds List(selection:) to outlineController.selectedItemID — a Binding<Int?> of heading ordinals. Folder rows live in a sibling Section of that same List, and their identity is a URL. List takes one Hashable type for the whole list; there is no second binding and no nesting a List in a List. This is not patchable mid-implementation: it changes the type at the top of SidebarView, what .tag(…) carries on every row including E08's, and what .onKeyPress(.return) dispatches on. Resolved by a SidebarSelection enum confined to the app target, so neither OutlineUI nor FileTree learns the other exists (D8).

2. FileDocument cannot be renamed, so acceptance box 3 is unreachable as built. id and format are let, both derived from fileURL at init. Setting fileURL alone moves the save target and the window title but leaves format stale — rename notes.md to notes.txt and it stays Markdown-highlighted, Markdown-previewed, and outlined, because WindowController already re-attaches the highlighter on a format change and that change never fires. Adds FileDocument.renamed(to:) and relaxes the two properties to private(set) var; lands as its own commit ahead of any folder code (D12).

3. This epic adds four pieces of persisted view state, and the obvious way to add them ships #34 four more times. #34 is open precisely because each window's WorkspaceModel hydrates sidebar layout from the shared UserDefaults suite once at init and never re-reads, so a second window reorders using stale input. Hidden-files, supported-only, folders-first and the recent-roots list go into one shared @Observable instance owned by AppDelegate instead — the "shared observable store object" that #34 itself lists as a fix. It deliberately does not retrofit sectionOrder/sectionExpanded, because #34 says the app-wide-vs-per-window product question has to be answered first (open decision 9).

Two platform facts checked rather than assumed — earlier drafts had both backwards

The design turns on both, and both are commonly mis-remembered, so they were compiled and run against the deployment target (Swift 6.3.3 / macOS 26, -swift-version 6 -strict-concurrency=complete). The corrections are recorded inline in the plan so a reviewer does not have to re-derive them.

  • A nonisolated deinit on a @MainActor class can read isolated stored properties. deinit { source.cancel() } compiles. What fails is calling an isolated methoddeinit { cancel() } is #ActorIsolatedCall. So the reason DirectoryWatcher stays non-isolated is narrower and more useful than the folklore: the moment cancellation is factored into a helper on a main-actor type, RAII stops compiling, and the usual escape (Task { @MainActor in … } in deinit) resurrects self and never runs (D5).
  • bookmarkData(options: .withSecurityScope) succeeds unsandboxed (736 bytes), resolves with .withSecurityScope, and startAccessingSecurityScopedResource() returns true. So migration-plan D7's "designed around security-scoped URLs so sandboxing is additive later" can be done for real now rather than stubbed as a no-op seam: roots persist as scoped bookmarks and every use is wrapped in a balanced access scope, and turning the sandbox on later becomes an entitlement change with no format migration (D14).

Also recorded

  • The E18 boundary is drawn on the origin of the change, not its effect. The Mid-point check-in #28 amendment gives open-document reload/conflict to E18, but two of E09's acceptance boxes are about open documents. They do not conflict, because E09 also owns in-app CRUD: the sidebar's own Delete/Rename drives the document consequences (where old→new is known with certainty and no heuristic is needed), while a change arriving from Finder updates the tree only. E18 later generalises to all open documents, root or not, and must not have to depend on this module (D11).
  • FileFormat.format(for:in:) returns nil for extensionless files, so README, LICENSE, Makefile and Dockerfile are not "supported" by the registry's own definition — which decides what the "Supported files only" toggle does in exactly the folders a developer opens. Plan: the registry is the definition, and the filter therefore ships off by default (open decision 1).
  • TabRecord gains an optional folderRootBookmark, and WorkspaceSession.currentVersion stays 1. loadSession() discards any session whose version does not match, so a bump would delete every existing user's open tabs; a missing optional Codable key already decodes as nil.
  • The likely reason the 10k perf budget fails is localizedStandardCompare — ~130 000 ICU calls for a 10 000-entry sort — not the listing. The plan says to measure that comparator in isolation before building on it, and names the fallback (precomputed sort keys), because the wrong fix (plain <) is a visible Finder-order regression: "f10" before "f2".
  • Six named availability states, including .emptyAfterFilter as distinct from .empty — otherwise turning on "Supported files only" in a folder of .py files looks like the app broke.

Open decisions for review

  1. "Supported files only" hides extensionless files (README, Makefile, …). Plan keeps the registry as the single definition of "supported" and ships the filter off by default. Wants a call before implementation starts.
  2. ⇧⌘J for Reveal Active File — free in the current command table, chosen for Xcode's "Reveal in Project Navigator" precedent. Verify against system-wide shortcuts.
  3. Double-click opens, single-click selects, by default. The epic says "single/double per pref"; only the default is in question. Under native tabs every open creates a window, so single-click-opens spawns one per click — VS Code gets away with it because it has a reusable preview tab and we do not.
  4. Drops from Finder copy; drags within the tree move. Matching Finder exactly (move within a volume, copy across) means reading volume identifiers on every drag.
  5. Name collision on move is rejected, not Replace/Keep Both. A three-way sheet is real UI work for a rare case in a Markdown editor's sidebar.
  6. No cap on watched directories. Scope is already bounded by "expanded"; the instrumented count is what would reveal a problem. The acceptance box says "no fd leaks", not "bounded" — confirm that reading.
  7. .emptyAfterFilter offers a "Clear filters" button, which is app-wide and therefore affects every window.
  8. The root stays per window while filters and recents are app-wide. That is the Mid-point check-in #28 reading, but it is a mixed model and deserves an explicit yes.
  9. This does not fix Sidebar layout caches go stale across windows (E08 open decision #1 tail) #34 — it keeps new state out of the pattern without retrofitting the old state. Confirm the split, or fold the retrofit in now that a working pattern sits next to it.
  10. FileDocument.saveAs(_:) keeps its stale id — same problem renamed(to:) fixes, but changing it moves the RecoveryBuffer key for untitled documents mid-session, which is a session-restore concern deserving its own change.
  11. Deleting an open clean document closes its window with no alert. Acceptance says "offers close/discard flow"; for a clean document there is nothing to discard and the alert would be OK-only.

Validation

Nothing to build yet — this commit is the plan. master was verified clean before branching (swiftformat --lint: 0/149 files require formatting; swiftlint lint --strict: 0 violations at 61cad7c), so as with E08 there is no pre-flight lint commit.

Full validation gate for the implementation commits is in §9 of the plan. One item there is not boilerplate: this is the largest concurrency surface since E07 — a DispatchSource, a @Sendable escaping callback, an @unchecked Sendable class, and a background→main-actor hop — and CI runs Xcode 26.0.1, which rejected code E07's newer local toolchain accepted. Push and read CI after the watcher step, not just at the end.

🤖 Architecture pass by Claude Opus 5. Implementation and review notes will follow as inline comments on this PR.

…CRUD)

Plan only; no implementation. Refs #10.

Records the design for the folder browser, plus three things the epic
cannot be built without and that are not visible from the spec:

- The sidebar List has exactly one selection binding and E08 owns it;
  folder rows need the same List. Resolved with a SidebarSelection enum
  confined to the app target (D8).
- FileDocument.id and .format are `let` and derived from fileURL at init,
  so "renaming an open file updates tab title + save target" cannot be met
  by setting fileURL alone — a .md renamed to .txt would stay Markdown-
  highlighted and Markdown-previewed. Adds FileDocument.renamed(to:) (D12).
- Adding four more UserDefaults-cached-at-init view preferences would ship
  issue #34's stale-cache bug four more times; the new state goes in one
  shared observable instead (D7).

Two platform facts the design turns on were verified against the local
toolchain rather than assumed, and earlier drafts had both backwards:
a nonisolated deinit on a @mainactor class can read isolated stored
properties but cannot call an isolated method (D5), and security-scoped
bookmarks can be created and resolved unsandboxed (D14).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

10. **`FileDocument.saveAs(_:)` keeps its stale `id`** (D12). It has the same problem `renamed(to:)` fixes, but changing it moves the `RecoveryBuffer` key for untitled documents mid-session, which is a session-restore concern that deserves its own change rather than a ride-along. Flagged, not fixed.
11. **Delete of an open clean document closes its window with no alert** (D11). Acceptance says "offers close/discard flow"; for a clean document there is nothing to discard and the alert would be OK-only. Confirm the reading, or add the confirm for symmetry.

## 11. Hand-off notes / known pitfalls (condensed — mirrored to the PR inline comment)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hand-off — implementation starts here.

Read §2 (what already exists and what collides), §3 (D1–D15), and §4.2–4.6 (API contract) before writing any code. Review rejects on §3 and §4, not on style.

Do these first, in this order, and stop at each for a green suite:

  1. FileDocument.renamed(to:) + relaxing id/format (D12) — a FileCore change, own commit, reviewable without folder code mixed in.
  2. DirectoryEntry normalization (D4) — one construction site, symlinks not resolved.
  3. FileTreeArrangement — and before building anything on top of it, microbenchmark localizedStandardCompare over 10 000 names (§4.7). It is the single most likely reason the 200 ms budget fails, and the wrong fix (plain <) is a visible Finder-order regression.

The five ways this goes wrong, in rough order of likelihood:

  • Mixing file:///a/b and file:///a/b/. URL hashes on its string, so a directory keyed both ways can be expanded and collapsed at the same time. Normalize in DirectoryEntry.init and nowhere else. standardizedFileURL yes, resolvingSymlinksInPath no — resolving relocates a symlinked folder's children under the target and breaks the parent→child relation the whole tree is built from.
  • Skipping the diff and just reassigning children on every watcher event (D6). MacDown's own atomic save writes .notes.md.tmp-<uuid> and replaces the file — several events per save, on a directory whose listing never changed. Without the diff, typing re-renders the tree.
  • close(fd) anywhere but the DispatchSource cancel handler (D5). Closing before cancellation completes is a use-after-close on a descriptor number the process can immediately reuse.
  • Making DirectoryWatcher @MainActor (D5). A nonisolated deinit can read isolated stored properties — that part of the folklore is wrong and was verified — but it cannot call an isolated method, so deinit { cancel() } stops compiling the moment cancellation is a helper, and Task { @MainActor in … } in a deinit resurrects self and never runs.
  • Computing rows in SidebarView.body (D15). E08 does exactly that for the outline and it is correct there; at 10 000 entries it is the quadratic trap.

Things that look like polish and are not:

  • renamed(to:) must recompute format, not just fileURL — otherwise notes.mdnotes.txt stays Markdown-highlighted, Markdown-previewed and outlined (D12).
  • Sibling-name collision checks are case-insensitive (APFS) and exclude the item's own name, or notes.mdNotes.md is rejected as a duplicate of itself (D10).
  • Subtree containment is a path-component check. /a/foobar is not inside /a/foo (D10).
  • Renaming re-keys expanded, children and watchers for the node and every cached descendant, or an expanded folder collapses when renamed and leaks its watcher (D10).
  • FileManager.trashItem, never removeItem (D10).
  • Do not bump WorkspaceSession.currentVersion (D13). loadSession() discards any session whose version does not match — bumping it deletes every existing user's open tabs. The added optional Codable key needs no bump.
  • Never call stopAccessingSecurityScopedResource() when start… returned false — the calls are reference-counted and an unbalanced stop revokes access someone else holds (D14).

Module hygiene: FileTree imports FileCore, Foundation, Observation, Dispatch — nothing else. No SwiftUI, no AppKit, no Workspace. If you write import OutlineUI in FileTree or import FileTree in OutlineUI, stop: SidebarSelection (D8) is the app target's job.

No Package.swift / project.yml / ci.yml diff (§6). The target, product, test target and app dependency all already exist. A diff there means something went wrong.

Two numbers must be reported on this PR, because a reviewer cannot re-derive them from the diff: the measured 10k expand time (§4.7) and the watchedDirectoryCount behaviour from the churn test (§7 box 4).

Push and read CI after step 7 (the watcher), not only at the end. CI is Xcode 26.0.1 and has previously rejected concurrency code the newer local toolchain accepted (E07). This epic adds a DispatchSource, a @Sendable escaping callback, an @unchecked Sendable class, and a background→main-actor hop. "Builds locally" is not evidence here.

Do not silently resolve the open decisions in §10 — especially 1 (extensionless files under the "Supported files only" filter) and 3 (single- vs double-click default). Both change user-visible behaviour and both are cheap to flip before the UI test is written against them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar layout caches go stale across windows (E08 open decision #1 tail)

1 participant