show empty folders in sidebar too#243
Conversation
|
![inkeep-internal-ci[bot]](https://avatars.githubusercontent.com/u/123133689?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
🟠 1) api-extension.ts:590-616 Synchronous disk walk blocks event loop on every request
Issue: The new collectFolderPathsFromDisk() function performs a synchronous recursive directory walk using readdirSync() on every GET /api/pages request. This blocks the Node.js event loop for the duration of the walk.
Why: /api/pages is called frequently: on initial load, on tab focus, on visibility change, and on CC1 files broadcasts. For large content directories, each call could block for 10-100ms, pausing all collaborative editing operations. The existing getFileIndex() uses an incremental pattern (populated at startup, updated on events), but this reintroduces sync disk I/O into the hot path.
Fix: Maintain the folder index incrementally in file-watcher.ts alongside fileIndex. Extend seedLastKnownHashes() to track folder paths at startup, then update incrementally on create/delete/rename DiskEvents. Expose via getFolderIndex(). Alternatively, cache with TTL invalidation if incremental is too complex for v1.
Refs: file-watcher.ts:395-560
🟠 2) api-extension.ts:603 Symlinked directories silently excluded
Issue: entry.isDirectory() returns false for symbolic links pointing to directories, causing symlinked folders to be invisible in the sidebar.
Why: CLAUDE.md states "Symlinks inside the content directory are fully supported." The existing file-watcher.ts explicitly handles symlinked directories (lines 419-460) by checking isSymbolicLink() first. This inconsistency means symlinked empty folders won't appear while symlinked files work correctly.
Fix: Check entry.isSymbolicLink(), resolve with realpathSync, validate target is within contentDir, and handle ENOENT/ELOOP. Follow the pattern in file-watcher.ts:seedLastKnownHashes().
Refs: file-watcher.ts:419-460
Inline Comments:
- 🟠 Major:
api-extension.ts:590-616Synchronous disk walk blocks event loop - 🟠 Major:
api-extension.ts:603Symlinked directories excluded - 🟡 Minor:
api-extension.ts:606Incomplete fallback exclusions
🟡 Minor (1) 🟡
🟡 1) api-extension.ts:606 Fallback exclusions incomplete
Issue: excludedByFallback only checks .git and node_modules, but BUILTIN_SKIP_DIRS in content-filter.ts includes 16 more directories (dist, build, .next, etc.).
Why: When contentFilter is absent, empty build/cache directories would appear in the sidebar. Low impact since standalone.ts always provides contentFilter.
Fix: Import BUILTIN_SKIP_DIRS from content-filter.ts, or make contentFilter required.
Refs: content-filter.ts:39-62
💭 Consider (1) 💭
💭 1) api-extension.ts:596-600 Add structured error context to directory read failures
Issue: The error log doesn't include the error code (EACCES, ENOENT, ELOOP) in a structured way.
Why: During incident response when users report missing folders, operators need to quickly identify if it's a permission issue, broken symlink, or race condition.
Fix: Extract error code: console.warn('[api] Failed to read directory', { path: absDirPath, code: (err as NodeJS.ErrnoException).code })
🚫 REQUEST CHANGES
Summary: The implementation achieves the goal of showing empty folders, with good test coverage for the new buildTree() behavior. The primary concern is the synchronous disk walk on every API call — this will cause noticeable UI stuttering on large content directories and block CRDT operations. Consider either caching the result or maintaining the folder index incrementally (recommended, since the pattern already exists in file-watcher.ts). The symlink handling inconsistency should also be addressed to maintain the documented "full symlink support" contract.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
PageListContext.tsx:86-88 |
Defensive typeof filter on folders array |
Positive observation — follows existing defensive pattern, no action needed |
file-tree-utils.ts:82 |
Function signature change | Positive observation — backward compatible, good use of Iterable<string> |
api-extension.ts:2955 |
New folders field in response |
Positive observation — follows existing envelope pattern, purely additive |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | Inline Comments | Discarded |
|---|---|---|---|---|---|
pr-review-standards |
1 | 1 | 0 | 1 | 0 |
pr-review-consistency |
5 | 1 | 0 | 1 | 3 |
pr-review-tests |
4 | 0 | 0 | 0 | 4 |
pr-review-sre |
3 | 1 | 1 | 1 | 0 |
| Total | 13 | 3 | 1 | 3 | 7 |
| function collectFolderPathsFromDisk(): string[] { | ||
| const folders = new Set<string>(); | ||
|
|
||
| function walk(absDirPath: string, relDirPath: string): void { | ||
| let entries: Dirent[]; | ||
| try { | ||
| entries = readdirSync(absDirPath, { withFileTypes: true }); | ||
| } catch (err) { | ||
| console.warn(`[api] Failed to read directory ${absDirPath}:`, err); | ||
| return; | ||
| } | ||
|
|
||
| for (const entry of entries) { | ||
| if (!entry.isDirectory()) continue; | ||
| const childRelPath = relDirPath ? `${relDirPath}/${entry.name}` : entry.name; | ||
| const excludedByFilter = contentFilter?.isDirExcluded(childRelPath) ?? false; | ||
| const excludedByFallback = entry.name === '.git' || entry.name === 'node_modules'; | ||
| if (excludedByFilter || excludedByFallback) continue; | ||
|
|
||
| folders.add(childRelPath); | ||
| walk(resolve(absDirPath, entry.name), childRelPath); | ||
| } | ||
| } | ||
|
|
||
| walk(contentDir, ''); | ||
| return [...folders].sort((a, b) => a.localeCompare(b)); | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: Synchronous disk walk on every /api/pages request
Issue: This function performs a synchronous recursive directory walk using readdirSync() on every GET /api/pages request, blocking the Node.js event loop for the duration of the walk.
Why: The /api/pages endpoint is called frequently: on initial page load, on tab/window focus, on visibility change, and on every CC1 files channel broadcast. For a content directory with hundreds of folders, each call could block for 10-100ms. During this time, all collaborative editing operations (CRDT syncs, WebSocket messages) are paused. The existing getFileIndex() is already async-safe (populated at startup, updated incrementally), but this reintroduces synchronous disk I/O into the hot path.
Fix: Consider maintaining the folder index incrementally alongside the existing fileIndex in file-watcher.ts. The seedLastKnownHashes() function already walks the directory tree at startup — extend it to also track folder paths, then update incrementally on create/delete/rename DiskEvents. Expose via getFolderIndex() similar to getFileIndex().
Alternatively, if this is acceptable for the initial implementation, consider caching the result with a short TTL or invalidating on DiskEvents to reduce the frequency of walks.
Refs:
| } | ||
|
|
||
| for (const entry of entries) { | ||
| if (!entry.isDirectory()) continue; |
There was a problem hiding this comment.
🟠 MAJOR: Symlinked directories silently excluded
Issue: entry.isDirectory() returns false for symbolic links pointing to directories, causing symlinked folders to be silently excluded from the returned folder list.
Why: CLAUDE.md states "Symlinks inside the content directory are fully supported." The existing file-watcher.ts explicitly handles symlinked directories by first checking isSymbolicLink() then resolving to determine target type (lines 419-460). This new code does not follow that pattern, causing symlinked empty folders to be invisible in the sidebar.
Fix: Check entry.isSymbolicLink() in addition to entry.isDirectory(). For symlinks, resolve with realpathSync and check if the target is a directory, following the pattern in file-watcher.ts:seedLastKnownHashes(). Also handle ENOENT (broken symlink), ELOOP (cyclic symlink), and symlink-escape (target outside contentDir) cases.
Refs:
| if (!entry.isDirectory()) continue; | ||
| const childRelPath = relDirPath ? `${relDirPath}/${entry.name}` : entry.name; | ||
| const excludedByFilter = contentFilter?.isDirExcluded(childRelPath) ?? false; | ||
| const excludedByFallback = entry.name === '.git' || entry.name === 'node_modules'; |
There was a problem hiding this comment.
🟡 Minor: Fallback exclusions incomplete compared to BUILTIN_SKIP_DIRS
Issue: The excludedByFallback check hardcodes only .git and node_modules, but BUILTIN_SKIP_DIRS in content-filter.ts (lines 39-62) includes 16 additional directories: .venv, venv, env, __pycache__, vendor, dist, build, out, output, .next, .nuxt, .svelte-kit, .astro, .turbo, .cache, .parcel-cache, and coverage.
Why: When contentFilter is absent (unlikely in production but possible in tests), empty folders like dist/, build/, .next/ would appear in the sidebar. This creates inconsistent behavior between the fallback path and the normal path.
Fix: Either:
- Import and use
BUILTIN_SKIP_DIRSfromcontent-filter.tsfor the fallback - Make
contentFilterrequired sincestandalone.tsalways provides it - Accept the incomplete fallback since it's defensive code for an unlikely case
Refs:
1 participant
https://inkeep.slack.com/archives/C0AQVFXB139/p1776782956214889