Repository navigation
fix(desktop): stop a folder rename from throwing out of the Files tab fs handler - #6789
Conversation
… fs handler The rename branch of the Files tab's fs-event handler looked up the renamed folder's handle with an unguarded `model.getItem`. Pierre's `getItem` is typed to return null for a path it doesn't hold, but it throws when the lookup walks through a node with no child index, so the throw escaped the workspace event socket callback and aborted the rest of that watcher batch. Contain the lookup: a path the model can't resolve now yields no handle, which is the same answer as "not expanded". Refs DESKTOP-11E
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe change adds ChangesDirectory lookup handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized fix prevents a folder rename lookup from aborting filesystem event processing and leaving the Files tab stale; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
… a file (#7638) The tree library's segment walk resolves `a/b/c` by reading the child index of whatever node it reached so far, without checking that the node is a directory. When the tree holds `a/b` as a file and the leaf name exists anywhere else, the walk reaches the file node and throws "Unknown directory child index", out of getItem, isExpanded and the selection remap inside resetPaths. The Files tab feeds it that shape whenever a loaded directory comes back from a listing as a non-directory while a row beneath it is selected, and the throw leaves the controller half-reset until the next refresh. Patch the walk to answer null for a path beneath a file, which is the documented contract of the lookup, and drop the try/catch wrapper #6789 added at one call site now that every call site gets the same answer. Refs DESKTOP-19G
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 Nis 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:
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 thefs:eventssubscriber 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.getItemis typedgetItem(path: string): FileTreeItemHandle | nulland documented to accept "both canonical directory paths (src/) and bare directory lookup paths (src)". It does not always honour that. Its path-store walk returnsnullfor an unknown segment, but callsgetDirectoryIndexon each node it walks through:getDirectoryIndexthrows for any node that has no child index — that is, any node the tree holds as a file. SogetItem("build/out/")throws (rather than returningnull) wheneverbuildis a file in the tree and the segmentoutis interned anywhere in the tree. Segment names likeout,distandsrcrecur 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
isDirectoryby stat'ing the path, and falls back tofalsewhen the stat loses the race with the rename that produced the event: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 computesnewKey = "<folder>/<name>/", and the unguarded lookup a few lines below throws.The sibling Pierre calls on this path already tolerate their own failures:
model.movehas a documented try/catch with a remove-then-add fallback, andaddKnownPath/removeKnownPatheach swallow internally. ThegetItemlookup was the only unguarded one.Fix
lookupDirectory(model, treePath)inutils/treePathwraps the lookup and returnsnullwhen 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 viaensureDirectoryChainwhenever 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.movecatch 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), andaddKnownPath/removeKnownPathkeep 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, andresolveDeleteTreePath.Adjacent unguarded lookups, deliberately left alone. Two other call sites do the same
asDirectoryHandle(model.getItem(...))— the lazy-expandmodel.subscribecallback (~line 384) andrekeyDescendantsBound(~line 511). Neither appears inDESKTOP-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 —
getItemis typed and documented to returnnullfor 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 checkon the four changed files — one fixable import-order finding,--write, then clean:cd apps/desktop && bun run typecheck— exits non-zero on this worktree with 15 pre-existing errors, all of themnode-pty/@xterm/addon-webglnot being installed here (TS2307plus 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:Test, failing before the change. The reproduction builds a tree in exactly the state the handler reaches —
buildheld as a file, segmentoutinterned by a real directory — and asserts today's call-site expression yields no handle: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 yieldsnull:bun testover the FilesTab directory (there are no existinguseFilesTabBridgetests):And the wider v2-workspace tree, unchanged:
Repo-wide
bun run lintwas 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/treesgetItemcould 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.model.getItemin the rename branch withlookupDirectory, which returns null on failure and avoids aborting the batch.lookupDirectoryand add tests to cover failure and success cases; no changes to other error paths (move/add/remove) or listeners. Refs DESKTOP-11E.Written for commit b0186e1. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests