fix(desktop): stop a folder rename from throwing out of the Files tab fs handler

Merged
SA
3mo ago
+59-3

Problem

DESKTOP-11E: 22 unhandled renderer errors in ~17 hours on 1.24.2, one machine, while a workspace file watcher was live. Unknown directory child index for node N is thrown by the tree library out of the v2 workspace sidebar's Files tab fs-event handler.

The throw escapes the workspace event socket callback, which dispatches a watcher batch like this:

for (const event of payload.events) {
    handler(event);              // useWorkspaceEvent.ts:78
}

So the user-visible harm is not the Sentry entry. When one rename in a batch throws, every remaining event in that batch is dropped, and the bus's own listener loop (eventBus.ts:212, no try/catch) stops dispatching that message to any listener registered after ours — including the fs:events subscriber that keeps open editor buffers in sync. The Files tab then shows a file list that no longer matches the working tree, and it stays wrong until the user leaves and re-enters the workspace. Because the tree state that causes it persists, every subsequent rename under the same folder throws again, which is why one machine produced 22 of these.

Reaching the reported failure: the events come from 1.24.2, the code path is unchanged on main (last touched by #6706), and no in-flight PR restructures this handler. The fix ships in the next desktop release to every desktop user.

Why code and not an archive/filter: the events are a symptom of the Files tab silently dropping filesystem changes, so archiving would leave that behaviour in place, and a Sentry-side filter is barred by repo law (PR #6164) and would hide the symptom rather than the divergence. The code path is live and not being deleted. The smallest change that removes the harm is containing the one lookup, so that is the whole change.

Root cause

FileTree.getItem is typed getItem(path: string): FileTreeItemHandle | null and documented to accept "both canonical directory paths (src/) and bare directory lookup paths (src)". It does not always honour that. Its path-store walk returns null for an unknown segment, but calls getDirectoryIndex on each node it walks through:

for (const segment of segments) {
    const segmentId = state.snapshot.segmentTable.idByValue.get(segment);
    if (segmentId === void 0) return null;
    const currentIndex = getDirectoryIndex(state, currentNodeId);   // throws
    ...
}

getDirectoryIndex throws for any node that has no child index — that is, any node the tree holds as a file. So getItem("build/out/") throws (rather than returning null) whenever build is a file in the tree and the segment out is interned anywhere in the tree. Segment names like out, dist and src recur all over a repo, so the second condition is nearly always met.

A directory ends up in the tree as a file because the watcher decides isDirectory by stat'ing the path, and falls back to false when the stat loses the race with the rename that produced the event:

try {
    const stats = await stat(absolutePath);
    isDirectory = stats.isDirectory();
    ...
} catch {
    isDirectory = state.directoryPaths.has(absolutePath);   // false for a path never seen
}

The handler trusts that (event.isDirectory ?? … ?? false) and adds the folder with a bare key, which creates a file node. Every later rename event inside that folder then computes newKey = "<folder>/<name>/", and the unguarded lookup a few lines below throws.

The sibling Pierre calls on this path already tolerate their own failures: model.move has a documented try/catch with a remove-then-add fallback, and addKnownPath / removeKnownPath each swallow internally. The getItem lookup was the only unguarded one.

Fix

lookupDirectory(model, treePath) in utils/treePath wraps the lookup and returns null when the model can't produce a handle. The rename branch uses it. That's it.

Why containment (i) and not repairing the state (ii): the state that breaks the lookup is not an unindexed parent left behind by the remove-then-add fallback — there is nothing there to repair. The fallback's model.add(newKey) creates the missing parent chain via ensureDirectoryChain whenever it can; it is rejected here precisely because the parent already exists, as a file node, created earlier by an unrelated event the watcher mis-typed. Repairing that would mean re-typing a node from file to directory, which is a change to how we trust watcher metadata rather than a fix for this throw — and it would not close the hole, because the disk itself can transiently present a file where a path expects a directory. Meanwhile the value the lookup produces is used only to decide whether to refetch an already-expanded directory, and "we cannot tell" is behaviourally identical to "not expanded". Containment at the call site is both correct and the smaller change.

What still propagates after this change. The guard covers exactly one expression, and nothing else on this path gained a catch. In particular, the divergence signals stay exactly as they were: the model.move catch keeps its existing scope (a rename we could not apply at all still falls through to remove-then-add, and is not conflated with a lookup failure), and addKnownPath / removeKnownPath keep their own pre-existing catches unchanged — a delete that matched nothing behaves exactly as before this PR. Everything else in the handler that can throw still does: invalidateTreeListings, bookkeeping(), purgeDirectory / rekeyDirectory, and resolveDeleteTreePath.

Adjacent unguarded lookups, deliberately left alone. Two other call sites do the same asDirectoryHandle(model.getItem(...)) — the lazy-expand model.subscribe callback (~line 384) and rekeyDescendantsBound (~line 511). Neither appears in DESKTOP-11E, neither runs in the socket callback, and neither is in scope here.

The tree library's behaviour is, in my reading, the real defect — getItem is typed and documented to return null for a path it doesn't hold, and here it leaks an internal invariant instead. Per the task contract the library is untouched and only our call site is fixed.

Verification

bunx biome check on the four changed files — one fixable import-order finding, --write, then clean:

Checked 4 files in 18ms. Fixed 1 file.
=== re-check ===
Checked 4 files in 11ms. No fixes applied.

cd apps/desktop && bun run typecheck — exits non-zero on this worktree with 15 pre-existing errors, all of them node-pty / @xterm/addon-webgl not being installed here (TS2307 plus the implicit-anys that follow), none in a file this PR touches. Verified pre-existing by reverting the change, re-running, and diffing the error lists:

baseline exit=2   (change reverted)
exit=2            (change applied)
15
diff <(grep "error TS" tc-baseline.txt) <(grep "error TS" tc2.txt)  ->  IDENTICAL

Test, failing before the change. The reproduction builds a tree in exactly the state the handler reaches — build held as a file, segment out interned by a real directory — and asserts today's call-site expression yields no handle:

(fail) BEFORE-CHECK: raw call-site expression > yields no handle for a path the model cannot resolve
error: Unknown directory child index for node 4
  at getDirectoryIndex   (@pierre/trees/dist/path-store/src/canonical.js:188)
  at findNodeIdBySegments (@pierre/trees/dist/path-store/src/canonical.js:177)
  at getPathInfo          (@pierre/trees/dist/path-store/src/store.js:168)
  at getItem              (@pierre/trees/dist/model/FileTreeController.js:344)
  at <anonymous>          (treePath.test.ts:42)

 4 pass
 1 fail

That is the reported stack, frame for frame.

Passing after, including the two negative cases — the guard must not blanket-null, so an expanded directory the model does hold still reports isExpanded() === true (otherwise the guard would silently stop refetching), and a file still yields null:

bun test .../FilesTab/utils/treePath/treePath.test.ts
 7 pass
 0 fail

bun test over the FilesTab directory (there are no existing useFilesTabBridge tests):

bun test .../components/FilesTab/
 37 pass
 0 fail
 57 expect() calls
Ran 37 tests across 4 files. [89.00ms]

And the wider v2-workspace tree, unchanged:

bun test .../v2-workspace/
 157 pass
 0 fail
Ran 157 tests across 25 files. [345.00ms]

Repo-wide bun run lint was not run (it hangs on cross-worktree bun locks).

Refs DESKTOP-11E

https://claude.ai/code/session_bee9b833-b56d-44c9-b1f5-f9697621fce3

Summary by cubic

Stops Files tab folder renames from throwing out of the fs-event handler. Previously @pierre/trees getItem could throw when walking through a file ancestor, aborting the watcher batch and desyncing the file list and open buffers; now we guard the lookup and treat unknown paths as not expanded.

  • Replace unguarded model.getItem in the rename branch with lookupDirectory, which returns null on failure and avoids aborting the batch.
  • Export lookupDirectory and add tests to cover failure and success cases; no changes to other error paths (move/add/remove) or listeners. Refs DESKTOP-11E.

Summary by CodeRabbit

  • Bug Fixes

    • Improved file sidebar behavior when renaming directories, including safer handling of expanded and nested folders.
    • Prevented errors when directory paths are missing, invalid, or incorrectly represented as files.

  • Tests

    • Added coverage for directory lookup, expansion, unresolved paths, and invalid tree states.