Skip to content

[codex] Bound file explorer watcher#104

Draft
den-sq wants to merge 1 commit into
mainfrom
codex/issue51-watch-guardrails
Draft

[codex] Bound file explorer watcher#104
den-sq wants to merge 1 commit into
mainfrom
codex/issue51-watch-guardrails

Conversation

@den-sq

@den-sq den-sq commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Fixes #101
Part of #51

Summary

  • bound the recursive file explorer watcher with explicit depth and path-count limits
  • ignore hidden paths plus dependency/cache folders before watcher traversal
  • send watcher errors and limit warnings to the renderer as folder-contents-error
  • show watcher warnings through the existing alert system
  • add a React 19 JSX namespace compatibility declaration so npm run typecheck can validate the renderer

Root Cause

The file explorer opened folders by starting an unbounded recursive watcher and mirroring every discovered path into renderer state. Very large folders can therefore consume large amounts of main-process watcher memory and renderer heap.

Validation

  • npm run typecheck
    • passed

@tavateva tavateva left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Draft Promotion Criteria PASS ✅

Fixes #101 (part of #51 stack). All 7 criteria hold.

  1. CI & mergeability — coverage check green (52s); `mergeable: MERGEABLE`, `mergeStateStatus: CLEAN`. ✓

  2. Issue #101 AC — 5 items, all addressed in the diff:

    • Ignore hidden + high-churn folders before traversal — `shouldIgnorePath` at filesystem.ts:56 is passed to the watcher's `ignore` callback, running before the watcher walks the subtree. Segment set: `{node_modules, pycache, venv}` plus dotfiles. ✓
    • Explicit `depth` and `limit` — `FILE_EXPLORER_WATCH_DEPTH = 6`, `FILE_EXPLORER_WATCH_LIMIT = 100_000` constants at top of file, both wired into the watcher options. ✓
    • Watcher errors → renderer via IPC event — new `folder-contents-error` IPC channel; `sendFSError(...)` invoked from three sites: the watcher `error` event, the limit-warning helper when `visibleAddCount` crosses the limit, and a try/catch around the per-event handler. Renderer registers a listener in `DirectoryProvider` that logs to console + surfaces through `addAlert(msg, 'warning')`. ✓
    • Normal small-folder behavior unchanged — no functional path change on small folders (limit=100k means the warning only fires on genuinely large trees; ignore list is a strict superset of the previous inline dotfile check). ✓
    • `npm run typecheck` passes — PR body reports passed. ✓
  3. Test Plan checkboxes — N/A.

  4. Tests run — `npm run typecheck` → passed. Issue AC lists typecheck as the only test requirement; concrete result reported. ✓

  5. Scope — 3 files: `filesystem.ts` (main-process watcher), `DirectoryContext.tsx` (renderer listener), `env.d.ts` (React 19 JSX namespace compat). The `env.d.ts` change is a small side-effect of running typecheck against React 19, explained inline in Summary ("add a React 19 JSX namespace compatibility declaration so `npm run typecheck` can validate the renderer"). Within scope. ✓

  6. Regressions & patterns — try/catch guard around the per-event handler prevents a single bad `stat()` from tearing down the watcher subscription (nice hardening). `getMainWindow().isDestroyed()` check in `sendFSError` avoids a use-after-free crash on window teardown. `clearFolderErrorListener` correctly returned from the effect cleanup. ✓

  7. Judgement calls — depth=6 and limit=100k are reasonable but not explicitly reasoned inline (they're specific numeric decisions worth a one-line justification). Ignore-segment list is hardcoded (`node_modules, pycache, venv`) — reasonable for this codebase's Node + Python worlds, but not extensible. Both are documentation nits, not blockers. ✓

All seven criteria hold. Approving.

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.

Bound recursive UI folder watcher for large folders

2 participants