fix(editor): render file tree from normalized children#2113
Conversation
Greptile SummaryThis PR fixes the Windows file-tree duplication bug (#1952) by replacing the flat
Confidence Score: 4/5Safe to merge for the targeted Windows path-duplication fix; the lazy-loading refactor is well-tested and the reconciliation logic is sound for normal usage. The path normalisation and hierarchical children model work correctly for all documented scenarios. Two narrower issues exist in _addNode: when a directory is swapped for a same-name file without going through watch-event delete/create, the former children linger in _nodes and _loadedPaths, which could prevent a later same-name directory from being lazily loaded; and sorting runs once per insertion inside _applyEntries rather than once at the end, producing unnecessary quadratic work for large directory listings. Neither affects the primary fix path. src/renderer/features/tasks/editor/stores/files-store.ts — specifically the _addNode method's type-change branch and the per-insertion _sortChildren call inside _applyEntries.
|
| Filename | Overview |
|---|---|
| src/renderer/features/tasks/editor/stores/files-store.ts | Core store refactored from flat childIndex to per-node children arrays with lazy loading; two issues found: orphaned _nodes/_loadedPaths on directory→file type change, and O(N²) sorting inside _applyEntries. |
| src/renderer/features/tasks/editor/stores/files-store-utils.ts | Clean refactor: normalizeFileTreePath, makeNode (now sets children:[]), sortFileNodes, and buildVisibleRows all look correct; logic is simpler and better tested than before. |
| src/shared/fs.ts | Only adds the required children: FileNode[] field to the FileNode interface; no issues. |
| src/main/core/fs/impl/local-fs.ts | Single-line fix: appends .replace(/\\/g, '/') to relPath() to normalise Windows backslashes to POSIX; correct and minimal. |
| src/renderer/features/tasks/editor/editor-file-tree.tsx | Consumer updated to pass files.rootNodes instead of files.nodes + files.childIndex; no other callers of the old signature remain in this file. |
| src/renderer/features/tasks/editor/stores/files-store.test.ts | New integration tests cover root-only initial load, lazy child loading, Windows-path normalisation, and revealFile ancestor expansion; all scenarios are meaningful. |
| src/renderer/features/tasks/editor/stores/files-store-utils.test.ts | Unit tests for path normalisation, visibility derivation, sibling sorting, and leaf-file non-expandability are all well-structured and cover the key logic paths. |
Sequence Diagram
sequenceDiagram
participant UI as EditorFileTree
participant Store as FilesStore
participant Utils as files-store-utils
participant RPC as rpc.fs
UI->>Store: tree.load() → _fetchAll()
Store->>RPC: "listFiles(projectId, workspaceId, '.', {recursive:false})"
RPC-->>Store: entries (root level only)
Store->>Store: _applyEntries('', entries)
Store-->>UI: "tree.data = {nodes, rootNodes}"
UI->>Utils: buildVisibleRows(rootNodes, expandedPaths)
Utils-->>UI: visible rows (collapsed dirs hide children)
UI->>Store: loadDir('src') on expand
Store->>RPC: "listFiles(projectId, workspaceId, 'src', {recursive:false})"
RPC-->>Store: entries (src/ children)
Store->>Store: _applyEntries('src', entries) reconcile
Store-->>UI: tree.data updated → re-render
UI->>Store: revealFile('src/a/b.ts', expandedPaths)
Store->>RPC: listFiles(..., 'src', ...)
Store->>RPC: listFiles(..., 'src/a', ...)
Store->>Store: expandedPaths.add ancestors
Store-->>UI: tree updated with ancestors expanded
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/renderer/features/tasks/editor/stores/files-store.ts:265-282
**Orphaned children in `_nodes` / `_loadedPaths` on directory→file type change**
When `_addNode` is called with a `file` node for a path that previously existed as a loaded `directory`, it clears `existing.children = []` but never removes those child nodes from `this._nodes` or `this._loadedPaths`. After the type-change, stale subtree entries (e.g. `src/components/Button.tsx`) remain in `_nodes`, and `_loadedPaths` still contains the directory path. Two concrete consequences: (1) any subsequent `loadDir('src/components')` call returns early because `_loadedPaths.has('src/components')` is still `true`, and (2) incoming watch-`create` events for the old children are silently dropped because `_nodes.has(path)` is still `true`. The fix is to call `_removeNodeFromMaps` on the existing node before overwriting it when the type changes from `directory` to `file`.
### Issue 2 of 2
src/renderer/features/tasks/editor/stores/files-store.ts:277-281
**Quadratic sorting inside `_applyEntries`**
`_addNode` calls `_sortChildren` on every insertion, so loading N entries for the same parent triggers N sort passes, each O(k log k) on a growing array — O(N² log N) total. The explicit `_sortChildren(parent)` at the end of `_applyEntries` is already sufficient to produce a correctly-ordered result; the intermediate sorts only keep the array sorted during the loop, which provides no observable benefit here. Deferring sorting to `_addNode` callers that actually care about order (i.e. `_applyWatchEventsInternal`) resolves the quadratic behaviour.
```suggestion
this._nodes.set(node.path, node);
const parent = node.parentPath ? this._nodes.get(node.parentPath) : null;
const siblings = parent?.children ?? this._rootNodes;
if (!siblings.some((child) => child.path === node.path)) siblings.push(node);
```
Reviews (1): Last reviewed commit: "fix(editor): render file tree from norma..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private _addNode(node: FileNode): void { | ||
| this._nodes.set(node.path, node); | ||
| const parent = node.parentPath; | ||
| const existing = this._childIndex.get(parent) ?? []; | ||
| if (!existing.includes(node.path)) { | ||
| this._childIndex.set(parent, sortedChildPaths([...existing, node.path], this._nodes)); | ||
| const existing = this._nodes.get(node.path); | ||
| if (existing) { | ||
| existing.type = node.type; | ||
| existing.mtime = node.mtime; | ||
| existing.extension = node.extension; | ||
| existing.isHidden = node.isHidden; | ||
| if (node.type === 'file') existing.children = []; | ||
| this._sortChildren(existing.parentPath ? this._nodes.get(existing.parentPath) : null); | ||
| return; | ||
| } | ||
|
|
||
| this._nodes.set(node.path, node); | ||
| const parent = node.parentPath ? this._nodes.get(node.parentPath) : null; | ||
| const siblings = parent?.children ?? this._rootNodes; | ||
| if (!siblings.some((child) => child.path === node.path)) siblings.push(node); | ||
| this._sortChildren(parent); | ||
| } |
There was a problem hiding this comment.
_nodes / _loadedPaths on directory→file type change
When _addNode is called with a file node for a path that previously existed as a loaded directory, it clears existing.children = [] but never removes those child nodes from this._nodes or this._loadedPaths. After the type-change, stale subtree entries (e.g. src/components/Button.tsx) remain in _nodes, and _loadedPaths still contains the directory path. Two concrete consequences: (1) any subsequent loadDir('src/components') call returns early because _loadedPaths.has('src/components') is still true, and (2) incoming watch-create events for the old children are silently dropped because _nodes.has(path) is still true. The fix is to call _removeNodeFromMaps on the existing node before overwriting it when the type changes from directory to file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/tasks/editor/stores/files-store.ts
Line: 265-282
Comment:
**Orphaned children in `_nodes` / `_loadedPaths` on directory→file type change**
When `_addNode` is called with a `file` node for a path that previously existed as a loaded `directory`, it clears `existing.children = []` but never removes those child nodes from `this._nodes` or `this._loadedPaths`. After the type-change, stale subtree entries (e.g. `src/components/Button.tsx`) remain in `_nodes`, and `_loadedPaths` still contains the directory path. Two concrete consequences: (1) any subsequent `loadDir('src/components')` call returns early because `_loadedPaths.has('src/components')` is still `true`, and (2) incoming watch-`create` events for the old children are silently dropped because `_nodes.has(path)` is still `true`. The fix is to call `_removeNodeFromMaps` on the existing node before overwriting it when the type changes from `directory` to `file`.
How can I resolve this? If you propose a fix, please make it concise.| this._nodes.set(node.path, node); | ||
| const parent = node.parentPath ? this._nodes.get(node.parentPath) : null; | ||
| const siblings = parent?.children ?? this._rootNodes; | ||
| if (!siblings.some((child) => child.path === node.path)) siblings.push(node); | ||
| this._sortChildren(parent); |
There was a problem hiding this comment.
_applyEntries
_addNode calls _sortChildren on every insertion, so loading N entries for the same parent triggers N sort passes, each O(k log k) on a growing array — O(N² log N) total. The explicit _sortChildren(parent) at the end of _applyEntries is already sufficient to produce a correctly-ordered result; the intermediate sorts only keep the array sorted during the loop, which provides no observable benefit here. Deferring sorting to _addNode callers that actually care about order (i.e. _applyWatchEventsInternal) resolves the quadratic behaviour.
| this._nodes.set(node.path, node); | |
| const parent = node.parentPath ? this._nodes.get(node.parentPath) : null; | |
| const siblings = parent?.children ?? this._rootNodes; | |
| if (!siblings.some((child) => child.path === node.path)) siblings.push(node); | |
| this._sortChildren(parent); | |
| this._nodes.set(node.path, node); | |
| const parent = node.parentPath ? this._nodes.get(node.parentPath) : null; | |
| const siblings = parent?.children ?? this._rootNodes; | |
| if (!siblings.some((child) => child.path === node.path)) siblings.push(node); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/tasks/editor/stores/files-store.ts
Line: 277-281
Comment:
**Quadratic sorting inside `_applyEntries`**
`_addNode` calls `_sortChildren` on every insertion, so loading N entries for the same parent triggers N sort passes, each O(k log k) on a growing array — O(N² log N) total. The explicit `_sortChildren(parent)` at the end of `_applyEntries` is already sufficient to produce a correctly-ordered result; the intermediate sorts only keep the array sorted during the loop, which provides no observable benefit here. Deferring sorting to `_addNode` callers that actually care about order (i.e. `_applyWatchEventsInternal`) resolves the quadratic behaviour.
```suggestion
this._nodes.set(node.path, node);
const parent = node.parentPath ? this._nodes.get(node.parentPath) : null;
const siblings = parent?.children ?? this._rootNodes;
if (!siblings.some((child) => child.path === node.path)) siblings.push(node);
```
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
children.Root Cause
The renderer tree was backed by a flat parent index and Windows local file paths could arrive with backslashes. That allowed nested entries such as
src\routes\+page.svelteto be treated as a single root identity instead ofsrc -> routes -> +page.svelte, producing repeated folder/file-looking rows.Implementation
childrentoFileNodeand derive visible rows fromrootNodesby walking each expanded directory's own children.expandedPathssnapshots.LocalFileSystem.relPath()to return POSIX-style relative paths.Validation
pnpm run formatpassedpnpm run lintpassedpnpm run typecheckpassedpnpm exec vitest run --project node src/renderer/features/tasks/editor/stores/files-store.test.ts src/renderer/features/tasks/editor/stores/files-store-utils.test.ts src/main/core/fs/impl/local-fs.test.tspassed: 61 testssrcexpands to direct children,routesexpands to direct children, and no duplicated folder/file wrappers appeared.Notes
pnpm run teststill has unrelated existing Windows/environment-sensitive failures in migration fixture paths, MCP config path separator expectations, dependency probe mocks, user env mocks, PTY env, and worktree path assertions.