feat(desktop): add real-time filesystem watcher with @parcel/watcher#1351
feat(desktop): add real-time filesystem watcher with @parcel/watcher#1351Kitenite wants to merge 7 commits into
Conversation
Replace polling-based file tree and git status updates with a single event-driven watcher service. Consolidates StaticPortsWatcher into the same pattern and decomposes the 500-line filesystem router. - Add FsWatcher service using @parcel/watcher with 100ms debounce + 2s max batch window - Add tRPC subscription (filesystem.subscribe) for renderer consumption - Wire watcher lifecycle to workspace init/delete/close and app startup - Retire StaticPortsWatcher — ports router now consumes FsWatcher batch events - Split filesystem router into search.ts, operations.ts, subscription.ts - Extract shared useFsSubscription hook with optional debounce - Remove 2.5s polling from ChangesView, use subscription instead - Add real-time file tree updates in FilesView via subscription - Auto-invalidate search index cache on filesystem changes - Delete unused useFileTree.ts and FileTreeNode type
📝 WalkthroughWalkthroughThe PR introduces a single-active FsWatcher (using Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Bootstrap
participant DB as Workspace DB
participant FSW as FsWatcher
participant OS as OS (`@parcel/watcher`)
participant TRPC as TRPC (subscription)
participant UI as Renderer (useFsSubscription)
App->>DB: query lastActiveWorkspaceId
DB-->>App: return workspaceId
App->>FSW: switchTo(workspaceId, rootPath)
FSW->>OS: subscribe(rootPath) via `@parcel/watcher`
OS-->>FSW: file events (create/update/delete)
FSW->>FSW: collect & debounce events into batch
alt debounce elapsed / max window
FSW->>TRPC: emit FileSystemBatchEvent(workspaceId, events, ts)
TRPC->>UI: push event to subscribers
UI->>UI: debounced onData -> invalidate/refetch queries
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (1)
409-471:⚠️ Potential issue | 🔴 CriticalMissing filesystem watcher cleanup when deleting a worktree.
The
deleteWorktreemutation does not unwatch filesystem watchers for workspaces that reference the worktree being deleted. Both thedeleteandclosemutations properly callfsWatcher.unwatch()on their workspace ID before cleanup, butdeleteWorktreeskips this step. If any workspace has aworktreeIdpointing to the worktree being deleted, its filesystem watcher would remain active on a path that no longer exists, likely causing continuous errors from@parcel/watcher.Before calling
deleteWorktreeRecord(input.worktreeId), find all workspaces withworktreeId === input.worktreeIdand callfsWatcher.unwatch()for each workspace ID.
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/filesystem/search.ts`:
- Around line 119-132: The background rebuild in the stale-while-revalidate
branch creates buildPromise from buildSearchIndex and currently .catch() deletes
searchIndexBuilds and re-throws the error, which becomes an unhandled rejection
because the function already returns cached.index; update the .catch() on
buildPromise (the one assigned to searchIndexBuilds for cacheKey) to delete
searchIndexBuilds and log the error via the existing logger (or console.error)
instead of re-throwing; keep the searchIndexCache.set and
searchIndexBuilds.delete behavior on success and ensure buildPromise is still
stored in searchIndexBuilds so callers can observe it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/FilesView.tsx`:
- Around line 121-124: The filesystem subscription only invalidates the root
children so expanded subfolders don't refresh; change the onData passed to
useFsSubscription to reuse the same invalidation logic as handleRefresh: iterate
over the tree's currently expanded item ids (the same collection handleRefresh
uses), call tree.getItemInstance(id)?.invalidateChildrenIds() for each expanded
id and also invalidate the root, and then trigger the same refresh UI actions
handleRefresh does (e.g., setRefreshing/state updates) so newly created/deleted
files in expanded directories update immediately.
🧹 Nitpick comments (7)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/hooks/useFsSubscription/useFsSubscription.ts (1)
17-29: Pending debounce timer not cleared whendebounceMschanges.If
debounceMschanges while a timer is in-flight, the old timer (from the previoushandlerclosure) can still fire because the cleanup effect on lines 31-35 only runs on unmount. In practice this is unlikely to cause issues sincedebounceMsis probably constant per call site, but for correctness the cleanup could be tied todebounceMs:♻️ Suggested improvement
useEffect(() => { return () => { if (timerRef.current) clearTimeout(timerRef.current); }; - }, []); + }, [debounceMs]);apps/desktop/src/main/lib/fs-watcher/fs-watcher.ts (2)
43-44: Consider typing the EventEmitter to enforce the"batch"event contract.The
on("batch", ...)/emit("batch", ...)calls rely on string matching and manual casting at each listener site (e.g.,ports.ts). A typed emitter would catch mismatches at compile time.Example using a typed interface
+interface FsWatcherEvents { + batch: [batch: FileSystemBatchEvent]; +} + -class FsWatcher extends EventEmitter { +class FsWatcher extends EventEmitter<FsWatcherEvents> {Node's
EventEmittersupports generic type parameters since@types/nodev18+.
30-41:mapEventTypenever produces"addDir"or"unlinkDir"types.
FileSystemChangeEvent["type"]includes"addDir" | "unlinkDir", but@parcel/watcherdoesn't distinguish file vs. directory events, so these variants are unreachable. If consumers ever need to filter by directory events, this will silently fail. Not blocking, but worth a note or a follow-up to remove unused variants from the shared type.apps/desktop/src/main/index.ts (1)
215-246: Log message is slightly misleading — watchers haven't actually started yet.At line 239 you log "Started fs watchers" immediately after kicking off the
.watch()promises, but none of them have resolved at that point. Consider changing the wording to "Starting fs watchers" or awaiting withPromise.allSettledand logging actual success/failure counts.♻️ Suggested wording fix (minimal)
if (activeWorkspaces.length > 0) { console.log( - `[main] Started fs watchers for ${activeWorkspaces.length} active workspace(s)`, + `[main] Starting fs watchers for ${activeWorkspaces.length} active workspace(s)`, ); }apps/desktop/src/lib/trpc/routers/filesystem/operations.ts (2)
63-73: Thefs.accessexistence-check pattern silently swallows non-ENOENT errors.If
fs.accessfails for a reason other than the file not existing (e.g.,EACCES), the error is caught and silently discarded. This same fragile pattern is repeated increateDirectory,rename, andmove. A more robust approach is to check the error code directly.Also, per coding guidelines, these should throw
TRPCErrorwithBAD_REQUESTinstead of plainError.♻️ Suggested approach (apply similarly to createDirectory, rename, move)
- try { - await fs.access(filePath); - throw new Error(`File already exists: ${input.fileName}`); - } catch (error) { - if ( - error instanceof Error && - error.message.includes("already exists") - ) { - throw error; - } - } + try { + await fs.access(filePath); + throw new TRPCError({ + code: "BAD_REQUEST", + message: `File already exists: ${input.fileName}`, + }); + } catch (error) { + if (error instanceof TRPCError) throw error; + // ENOENT means file doesn't exist — proceed to create + if (!(error instanceof Error) || (error as NodeJS.ErrnoException).code !== "ENOENT") { + throw error; // Re-throw unexpected errors (e.g., EACCES) + } + }As per coding guidelines: "Use appropriate TRPCError codes consistently" and "Never swallow errors silently."
218-232: Unbounded collision counter in the copy loop.The
while (true)loop incrementscounterwithout an upper bound. While unlikely in practice, a pathological case could spin for a very long time. Consider adding a reasonable cap (e.g., 1000) and throwing an error if exceeded.apps/desktop/src/lib/trpc/routers/filesystem/search.ts (1)
42-53: Cache invalidation looks correct but consider logging for observability.The invalidation listener silently drops events when
getRootPathreturnsundefined(e.g., workspace already stopped). This is fine functionally, but a debug-level log could help troubleshoot cases where search results appear stale after file changes.
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
- Add refetchInterval fallback for ChangesView since .git is ignored by @parcel/watcher - Invalidate all directory items on fs events, not just root (no cascade) - Re-attach watcher on failed workspace deletion to avoid orphaned state - Guard fsWatcher.watch with cancellation check in workspace-init
The stale-while-revalidate path returns stale cache immediately, making the background rebuild fire-and-forget. Re-throwing in .catch() caused unhandled promise rejections.
Init's fire-and-forget fsWatcher.watch() can resolve after the initial unwatch in the delete mutation. Adding a second unwatch after waitForInit ensures no watcher leaks for a workspace being deleted.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (1)
329-356:⚠️ Potential issue | 🟡 MinorConsider re-attaching the watcher if
closefails after unwatch.In the
deleteprocedure, every failure path afterunwatchcarefully re-attaches the watcher. Inclose, ifkillByWorkspaceId(line 340) ordeleteWorkspace(line 344) throws after theunwatchon line 338, the workspace survives without its file watcher.If this is intentional (close is always-destructive and partial failure is unlikely), a brief comment would help future readers. Otherwise, wrapping in try/catch with a re-attach fallback would match the defensive pattern used in
delete.
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/filesystem/search.ts`:
- Around line 164-175: The searchFiles procedure currently accepts an arbitrary
rootPath allowing path traversal; update it (and the similar procedures
listDirectory, createFile, rename, delete in operations.ts) to validate rootPath
against the registered workspace roots before any filesystem ops: either require
a workspaceId and derive the canonical root via
fsWatcher.getRootPath(workspaceId) or check that the provided rootPath equals
one of fsWatcher’s known roots/registry, reject requests that don’t match, and
ensure all file operations use the validated/canonicalized root only.
🧹 Nitpick comments (5)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (1)
188-197: Extract the duplicated re-attach-watcher block into a helper.The same fire-and-forget re-attach pattern appears three times in this procedure. A small helper would reduce duplication and make the recovery intent clearer:
♻️ Suggested refactor
Add a local helper at the top of the
deletemutation (or at module scope):+const reattachWatcher = (workspaceId: string, rootPath: string | undefined) => { + if (!rootPath) return; + fsWatcher + .watch({ workspaceId, rootPath }) + .catch((err) => { + console.error("[workspace/delete] Failed to re-attach watcher:", err); + }); +};Then replace each duplicated block, e.g.:
clearWorkspaceDeletingStatus(input.id); -if (watcherRootPath) { - fsWatcher - .watch({ workspaceId: input.id, rootPath: watcherRootPath }) - .catch((err) => { - console.error( - "[workspace/delete] Failed to re-attach watcher:", - err, - ); - }); -} +reattachWatcher(input.id, watcherRootPath);Also applies to: 249-258, 276-285
apps/desktop/src/lib/trpc/routers/filesystem/search.ts (4)
11-19: Duplicated ignore patterns — consider sharing a single source of truth.
DEFAULT_IGNORE_PATTERNSis duplicated verbatim fromapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/constants.ts(lines 7-15). If the list evolves, the two copies can silently diverge.Consider extracting these into a shared constants file (e.g., under
shared/) so both the renderer and the main-process search router reference the same list.
43-53: Module-level event listener is never removed.The
fsWatcher.on("batch", ...)listener is registered as a module-level side effect at import time and is never cleaned up. Since this module is imported once and lives for the app's lifetime this is functionally fine, but it makes the search module harder to test in isolation and couples cache invalidation to import order.A lighter-touch improvement: move the listener registration into
createSearchRouter()so the wiring is explicit and could be torn down if needed.
72-80:onlyFiles: truemeans directories are never searchable.
fast-globis called withonlyFiles: true, so the index only contains files. TheFileSearchItemtype has anisDirectoryfield, and the procedure response includesisDirectory: false(line 193), but users can never find directories via search. If this is intentional, consider removing theisDirectoryfield from the search item shape to avoid confusion. If directory search is desired, you'd needonlyDirectoriesor a second pass.
82-88:idisrelativePath— clarify or deduplicate the field.
idis set torelativePath(line 83), and both are returned in the response (lines 189-191). Having two fields with identical values adds noise. Ifidis meant to be a stable identifier, consider documenting why it duplicatesrelativePath, or simply drop one.
| searchFiles: publicProcedure | ||
| .input( | ||
| z.object({ | ||
| rootPath: z.string(), | ||
| query: z.string(), | ||
| includeHidden: z.boolean().default(false), | ||
| limit: z.number().default(200), | ||
| }), | ||
| ) | ||
| .query(async ({ input }) => { | ||
| const { rootPath, query, includeHidden, limit } = input; | ||
| const trimmedQuery = query.trim(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for rootPath usage in filesystem router procedures
rg -n 'rootPath' --type=ts -C3 'apps/desktop/src/lib/trpc/routers/filesystem/'Repository: superset-sh/superset
Length of output: 10152
🏁 Script executed:
#!/bin/bash
# Search for workspace registry or validation mechanisms
rg -n 'workspace|fsWatcher' --type=ts 'apps/desktop/src/lib/trpc/routers/filesystem/' -B2 -A2Repository: superset-sh/superset
Length of output: 3099
🏁 Script executed:
#!/bin/bash
# Check the full searchFiles implementation
head -220 apps/desktop/src/lib/trpc/routers/filesystem/search.ts | tail -80Repository: superset-sh/superset
Length of output: 1898
🏁 Script executed:
#!/bin/bash
# Search for any validatePath or path validation utilities
fd -e ts -e tsx 'apps/desktop/src' | xargs rg -l 'validatePath|isValidPath|validateRootPath' --type=tsRepository: superset-sh/superset
Length of output: 435
🏁 Script executed:
#!/bin/bash
# Find renderer/client code that calls searchFiles
rg -n 'searchFiles' --type=ts --type=tsx 'apps/desktop/src'Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Look for workspace registry or list of valid workspace roots
rg -n 'workspace.*root|rootPath.*workspace|validateWorkspace' --type=tsRepository: superset-sh/superset
Length of output: 1642
🏁 Script executed:
#!/bin/bash
# Check fsWatcher implementation to understand getRootPath
fd -name 'fs-watcher*' -o -name '*fs-watcher*' | head -5 | xargs head -100Repository: superset-sh/superset
Length of output: 294
🏁 Script executed:
#!/bin/bash
# Find all calls to searchFiles in the codebase
rg -n 'searchFiles' --type=tsRepository: superset-sh/superset
Length of output: 444
🏁 Script executed:
#!/bin/bash
# Look for fs-watcher implementation
find apps/desktop/src/main/lib -name '*fs-watcher*' -type fRepository: superset-sh/superset
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Check if renderer code has access to workspace registry
rg -n 'workspace.*list|getWorkspaces|listWorkspaces' --type=ts 'apps/desktop/src/renderer' -A2 -B2Repository: superset-sh/superset
Length of output: 8983
🏁 Script executed:
#!/bin/bash
# Examine how rootPath is obtained in useFileSearch
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/hooks/useFileSearch/useFileSearch.tsRepository: superset-sh/superset
Length of output: 1208
🏁 Script executed:
#!/bin/bash
# Check the fs-watcher implementation to understand getRootPath method
cat -n apps/desktop/src/main/lib/fs-watcher/fs-watcher.ts | head -150Repository: superset-sh/superset
Length of output: 4575
🏁 Script executed:
#!/bin/bash
# Check where worktreePath comes from in the component that uses useFileSearch
rg -n 'worktreePath' --type=ts apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView -B5 -A5Repository: superset-sh/superset
Length of output: 50376
🏁 Script executed:
#!/bin/bash
# Check what SelectWorkspace and workspace data look like
rg -n 'worktreePath' --type=ts 'apps/desktop/src/renderer' -B2 -A2 | head -60Repository: superset-sh/superset
Length of output: 5002
🏁 Script executed:
#!/bin/bash
# Verify whether searchFiles validates rootPath against fsWatcher
cat -n apps/desktop/src/lib/trpc/routers/filesystem/search.ts | sed -n '164,210p'Repository: superset-sh/superset
Length of output: 1406
🏁 Script executed:
#!/bin/bash
# Check if there's validation in createFile, rename, delete procedures in operations
rg -n 'createFile|deleteFile|rename' --type=ts 'apps/desktop/src/lib/trpc/routers/filesystem/operations.ts' -A10 | head -80Repository: superset-sh/superset
Length of output: 1270
🏁 Script executed:
#!/bin/bash
# Check if workspace.get validates workspaceId belongs to user
rg -n 'workspaces\.get' --type=ts 'apps/desktop/src/lib/trpc/routers/workspaces' -A15 | head -60Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check the actual workspaces router structure
cat -n apps/desktop/src/lib/trpc/routers/workspaces/index.ts | head -40Repository: superset-sh/superset
Length of output: 186
🏁 Script executed:
#!/bin/bash
# Find the workspaces router implementation
find apps/desktop/src/lib/trpc/routers/workspaces -name '*.ts' -type fRepository: superset-sh/superset
Length of output: 1477
🏁 Script executed:
#!/bin/bash
# Check authentication/authorization - are tRPC procedures protected?
rg -n 'publicProcedure|protectedProcedure|middleware' --type=ts 'apps/desktop/src/lib/trpc' -B2 -A2 | head -80Repository: superset-sh/superset
Length of output: 5582
Validate rootPath against known workspace roots before filesystem operations.
The searchFiles procedure (and similarly listDirectory, createFile, rename, delete in operations.ts) accept rootPath as an arbitrary string with no validation that it corresponds to a known workspace root. If the renderer process is compromised or misused, this allows enumeration or modification of files outside intended workspace boundaries via path traversal.
Validate that rootPath matches a workspace registered with fsWatcher (e.g., by checking fsWatcher.getRootPath(workspaceId) or maintaining a registry of allowed roots), or refactor procedures to accept workspaceId and derive the path internally.
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/filesystem/search.ts` around lines 164 -
175, The searchFiles procedure currently accepts an arbitrary rootPath allowing
path traversal; update it (and the similar procedures listDirectory, createFile,
rename, delete in operations.ts) to validate rootPath against the registered
workspace roots before any filesystem ops: either require a workspaceId and
derive the canonical root via fsWatcher.getRootPath(workspaceId) or check that
the provided rootPath equals one of fsWatcher’s known roots/registry, reject
requests that don’t match, and ensure all file operations use the
validated/canonicalized root only.
…tener-pattern # Conflicts: # apps/desktop/package.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/workspace-init-manager.test.ts`:
- Around line 125-156: The timing assertions in the tests that call
manager.waitForInit (and use manager.startJob/manager.finalizeJob) are flaky;
change the assertions to avoid tight wall-clock checks—either widen tolerances
(e.g., replace expect(elapsed).toBeLessThan(100) with a much larger threshold
like <500 for "instant" checks and relax the timeout lower bound to >=150 for
the timeout test) or refactor to assert promise resolution order (e.g., await
Promise.race or use jest fake timers to advance timers) so tests rely on
deterministic resolution of manager.waitForInit rather than precise elapsed
milliseconds.
🧹 Nitpick comments (4)
apps/desktop/src/main/lib/workspace-init-manager.test.ts (1)
5-8: Constructor extraction viagetPrototypeOfis clever but couples tests to the export shape.If
workspace-init-managerever switches from exporting a singleton instance to exporting the class directly (or wraps it in a factory), this will silently break. Consider exporting the class alongside the singleton (even as a named_WorkspaceInitManagerClassfor test use) to make this contract explicit.This is a minor coupling concern and the pattern is used consistently in this PR, so it's not blocking.
apps/desktop/src/main/lib/fs-watcher/fs-watcher.test.ts (2)
138-142:triggerEventsassumes a single active watcher — fine here, but fragile if copied.The helper always fires events on
subscribeCalls[subscribeCalls.length - 1]. If a future test watches multiple workspaces and uses this helper, events will only reach the last-subscribed watcher. Consider parameterizing by subscribe index or workspace if you extend these tests later.
192-214: Slow test: ~2.2 s wall-clock for max batch window.This is a valid behavioral test of the 2 s max window, but it's the slowest test in the suite. If test-suite runtime becomes a concern, consider making the max batch window configurable (injected in the constructor) so tests can use a shorter window.
apps/desktop/src/main/lib/fs-watcher/fs-watcher.lifecycle.test.ts (1)
179-209: Cancellation guard tests validate calling-code discipline, not FsWatcher enforcement.Lines 186-189 simulate the guard (
if (manager.isCancellationRequested(...)) { ... }) in test code. This verifies the pattern works but doesn't guarantee production callers actually perform the check. If this invariant is critical, consider adding a guard insideFsWatcher.watch()itself (accepting an optional cancellation token or check callback) so it's impossible to forget.Not blocking — the tests are valuable as a pattern reference and for documenting the intended protocol.
| it("returns immediately when no job is in progress", async () => { | ||
| // No startJob called — should return immediately | ||
| const start = Date.now(); | ||
| await manager.waitForInit("ws-1"); | ||
| const elapsed = Date.now() - start; | ||
|
|
||
| // Should be nearly instant (well under 100ms) | ||
| expect(elapsed).toBeLessThan(100); | ||
| }); | ||
|
|
||
| it("times out when finalizeJob is never called", async () => { | ||
| manager.startJob("ws-1", "proj-1"); | ||
|
|
||
| const start = Date.now(); | ||
| await manager.waitForInit("ws-1", 200); // 200ms timeout | ||
| const elapsed = Date.now() - start; | ||
|
|
||
| // Should have waited ~200ms for the timeout | ||
| expect(elapsed).toBeGreaterThanOrEqual(180); | ||
| expect(elapsed).toBeLessThan(500); | ||
| }); | ||
|
|
||
| it("returns immediately after job already finalized", async () => { | ||
| manager.startJob("ws-1", "proj-1"); | ||
| manager.finalizeJob("ws-1"); | ||
|
|
||
| // Done promise was removed by finalizeJob, so this should return immediately | ||
| const start = Date.now(); | ||
| await manager.waitForInit("ws-1"); | ||
| const elapsed = Date.now() - start; | ||
| expect(elapsed).toBeLessThan(100); | ||
| }); |
There was a problem hiding this comment.
Timing-based assertions are CI-flaky.
expect(elapsed).toBeLessThan(100) (Lines 132, 155) and expect(elapsed).toBeGreaterThanOrEqual(180) (Line 143) rely on wall-clock timing that can easily fail under CI load. Consider widening the tolerances (e.g., < 500 for "instant" and >= 150 for the timeout) or restructuring to assert on promise resolution order rather than elapsed milliseconds.
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/workspace-init-manager.test.ts` around lines 125 -
156, The timing assertions in the tests that call manager.waitForInit (and use
manager.startJob/manager.finalizeJob) are flaky; change the assertions to avoid
tight wall-clock checks—either widen tolerances (e.g., replace
expect(elapsed).toBeLessThan(100) with a much larger threshold like <500 for
"instant" checks and relax the timeout lower bound to >=150 for the timeout
test) or refactor to assert promise resolution order (e.g., await Promise.race
or use jest fake timers to advance timers) so tests rely on deterministic
resolution of manager.waitForInit rather than precise elapsed milliseconds.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (1)
330-347:⚠️ Potential issue | 🟡 Minor
closemutation: unwatch failure prevents workspace cleanup.If
fsWatcher.unwatch(input.id)throws (e.g., native unsubscribe failure), theclosemutation aborts before deleting the workspace from the DB or killing terminals. The workspace will remain in a partially-closed state. Consider wrapping the unwatch in a try/catch so that cleanup proceeds even if the watcher teardown fails.🛡️ Proposed fix
- await fsWatcher.unwatch(input.id); - + try { + await fsWatcher.unwatch(input.id); + } catch (err) { + console.error("[workspace/close] Failed to unwatch, continuing cleanup:", err); + }
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/filesystem/search.ts`:
- Around line 56-59: When handling the fsWatcher "switched" event, increment a
generation counter (e.g., cacheGeneration) instead of blindly relying on
searchIndexBuilds.clear() to avoid orphaning in-flight promises; in the
fsWatcher.on("switched", ...) handler increment cacheGeneration and clear
searchIndexCache and searchIndexBuilds as before. In getSearchIndex capture
const gen = cacheGeneration immediately before creating and storing the build
promise in searchIndexBuilds, and in the promise .then/.catch handlers verify
that gen === cacheGeneration before calling searchIndexCache.set(...) or
otherwise mutating shared state so results from a prior workspace are discarded.
Ensure all places that create build promises (the function getSearchIndex and
any helpers) use this gen-check before writing to
searchIndexCache/searchIndexBuilds.
In `@apps/desktop/src/main/lib/fs-watcher/fs-watcher.test.ts`:
- Around line 254-279: The test "resets debounce timer on new events" is
timing-sensitive and can flake under CI; update the waits around triggerEvents
to give a larger safety margin: after the first triggerEvents keep the short
wait at ~30ms, then after the second triggerEvents keep ~30ms and assert
batches.length === 0, and then wait a longer period (e.g., ~120ms) to ensure the
100ms debounce window elapses before asserting batches length and contents;
adjust the setTimeout durations in this test (references: fsWatcher.switchTo,
triggerEvents, debounce 100ms) to these safer values so CI timing variance won't
cause premature debounce firing.
In `@apps/desktop/src/main/lib/fs-watcher/fs-watcher.ts`:
- Around line 63-67: The switchTo method currently returns early when
this.active?.workspaceId === workspaceId, which ignores changes to rootPath;
update the early-return check in switchTo to compare both workspaceId and
rootPath (e.g., this.active?.workspaceId === workspaceId &&
this.active?.rootPath === rootPath) so it only no-ops when both match, and
ensure any path comparison normalizes paths before comparing; if rootPath
differs, allow the method to proceed to stop the old watcher and start a new
watcher for the new rootPath (references: switchTo, this.active, workspaceId,
rootPath).
- Around line 142-162: The stopInternal method must guard against unsubscribe
throwing: ensure you capture/unsubscribe in a try/catch/finally (or set
this.active = null before unsubscribe) so that this.active is always cleared and
timers are cleared even if state.subscription.unsubscribe() throws;
specifically, keep the existing flush() and timer clears, then either set
this.active = null immediately (to prevent handleEvents from processing stale
events) and call state.subscription.unsubscribe() inside a try/catch, or call
unsubscribe in a try block and move this.active = null into a finally block so
the watcher cannot remain in an inconsistent active state.
🧹 Nitpick comments (7)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (1)
189-198: Consider extracting the repeated re-attach pattern into a helper.The
fsWatcher.switchTo(...).catch(...)re-attach block is copy-pasted three times with identical logic. A small helper would reduce duplication and ensure consistent error handling if the pattern changes.♻️ Suggested helper
+/** Re-attach the fs watcher after a failed deletion rollback. */ +function reattachWatcher(workspaceId: string, savedRootPath: string | null): void { + if (!savedRootPath) return; + fsWatcher + .switchTo({ workspaceId, rootPath: savedRootPath }) + .catch((err) => { + console.error("[workspace/delete] Failed to re-attach watcher:", err); + }); +}Then replace each occurrence with
reattachWatcher(input.id, savedRootPath);.Also applies to: 250-259, 277-286
apps/desktop/src/main/index.ts (1)
228-237: Log message is premature —switchTohasn't resolved yet.Line 235 logs "Started fs watcher" immediately after the fire-and-forget
switchTo, but the watcher isn't actually started until the promise resolves. This could confuse debugging if the watcher fails to start. Minor nit.♻️ Suggested fix
fsWatcher.switchTo({ workspaceId, rootPath }).catch((err) => { console.error( `[main] Failed to start fs watcher for active workspace ${workspaceId}:`, err, ); }); - console.log( - `[main] Started fs watcher for active workspace ${workspaceId}`, - ); + console.log( + `[main] Starting fs watcher for active workspace ${workspaceId}`, + );apps/desktop/docs/SINGLE_ACTIVE_FS_WATCHER.md (1)
15-36: Add a language identifier to the fenced code block.The ASCII-art data-flow diagram uses a fenced code block without a language specifier (flagged by markdownlint MD040). Use
textorplaintextto silence the lint warning.-``` +```text User action (create / switch / delete / close / open project)apps/desktop/src/main/lib/fs-watcher/fs-watcher.test.ts (4)
44-46: Fragile constructor extraction via prototype chain.This pattern tightly couples the test to the fact that
fsWatcheris a class instance. If the implementation ever switches to a plain object, factory function, or freezes the prototype, these tests silently break. A cleaner alternative is to export the class directly (even if only for testing) alongside the singleton.That said, this is a pragmatic workaround for obtaining fresh instances when only a singleton is exported, and it works fine today.
281-306: 2.2-second real-time wait slows the suite and amplifies flakiness.This test waits over 2 seconds of wall-clock time to validate the max-batch-window flush. In a large test suite this adds up, and the
setInterval+ realsetTimeoutcombination can behave unpredictably under load. If feasible, consider allowing the max window duration to be configurable (e.g., via constructor options) so this test can use a much shorter window (e.g., 200 ms).
220-225: No test covers the error callback path.
triggerEventsalways passesnullas the error argument. There is no test verifying how FsWatcher behaves when@parcel/watcherinvokes the callback with an error. Consider adding a test that callslastCall.callback(new Error("watch failed"), [])to verify the error is logged/handled gracefully and doesn't crash the batch pipeline.
63-81: ExportIGNORE_DIRSand import it in the test to avoid duplication.The ignore list in the implementation (
fs-watcher.ts, line 12) is defined as a non-exported constantIGNORE_DIRSbut duplicated as a hardcoded list in this test expectation. Exporting the constant and importing it here keeps the single source of truth, preventing silent drift if the ignore list evolves.
| fsWatcher.on("switched", () => { | ||
| searchIndexCache.clear(); | ||
| searchIndexBuilds.clear(); | ||
| }); |
There was a problem hiding this comment.
Clearing searchIndexBuilds on switch can orphan in-flight promises.
When "switched" fires, searchIndexBuilds.clear() removes references to in-flight build promises, but those promises continue executing. When they resolve, their .then handler calls searchIndexCache.set(cacheKey, ...) — populating the cache with an index from the previous workspace. This stale entry persists until the next batch or switch event.
Consider a generation counter or check in the .then callback to discard results from a prior workspace.
♻️ One approach: generation counter
+let cacheGeneration = 0;
+
fsWatcher.on("switched", () => {
+ cacheGeneration++;
searchIndexCache.clear();
searchIndexBuilds.clear();
});Then in getSearchIndex, capture const gen = cacheGeneration before creating the build promise, and in .then:
.then((index) => {
+ if (cacheGeneration !== gen) {
+ // Workspace switched — discard stale index
+ searchIndexBuilds.delete(cacheKey);
+ return index;
+ }
searchIndexCache.set(cacheKey, { index, builtAt: Date.now() });
searchIndexBuilds.delete(cacheKey);
return index;
})🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/filesystem/search.ts` around lines 56 - 59,
When handling the fsWatcher "switched" event, increment a generation counter
(e.g., cacheGeneration) instead of blindly relying on searchIndexBuilds.clear()
to avoid orphaning in-flight promises; in the fsWatcher.on("switched", ...)
handler increment cacheGeneration and clear searchIndexCache and
searchIndexBuilds as before. In getSearchIndex capture const gen =
cacheGeneration immediately before creating and storing the build promise in
searchIndexBuilds, and in the promise .then/.catch handlers verify that gen ===
cacheGeneration before calling searchIndexCache.set(...) or otherwise mutating
shared state so results from a prior workspace are discarded. Ensure all places
that create build promises (the function getSearchIndex and any helpers) use
this gen-check before writing to searchIndexCache/searchIndexBuilds.
| it("resets debounce timer on new events", async () => { | ||
| await fsWatcher.switchTo({ | ||
| workspaceId: "ws-1", | ||
| rootPath: "/tmp/project", | ||
| }); | ||
|
|
||
| const batches: FileSystemBatchEvent[] = []; | ||
| fsWatcher.on("batch", (batch: FileSystemBatchEvent) => { | ||
| batches.push(batch); | ||
| }); | ||
|
|
||
| triggerEvents([{ type: "create", path: "/tmp/project/a.ts" }]); | ||
|
|
||
| // Wait 50ms (less than 100ms debounce), then fire another | ||
| await new Promise((r) => setTimeout(r, 50)); | ||
| triggerEvents([{ type: "update", path: "/tmp/project/b.ts" }]); | ||
|
|
||
| // Wait another 50ms — still haven't hit 100ms since last event | ||
| await new Promise((r) => setTimeout(r, 50)); | ||
| expect(batches).toHaveLength(0); | ||
|
|
||
| // Wait for debounce to complete (100ms since last event) | ||
| await new Promise((r) => setTimeout(r, 80)); | ||
| expect(batches).toHaveLength(1); | ||
| expect(batches[0].events).toHaveLength(2); | ||
| }); |
There was a problem hiding this comment.
Timing-sensitive debounce-reset test may be flaky under CI load.
The assertion at line 273 expects zero batches after ~100 ms total elapsed (50 + 50), relying on the debounce timer (100 ms from last event at ~50 ms) not having fired yet. Under CI load or GC pauses, the actual wall-clock time can overshoot, causing the debounce to fire before the assertion runs.
Consider either:
- Increasing the margin (e.g., first wait only 30 ms instead of 50 ms), or
- Using a fake timer / deterministic scheduler if bun:test supports it.
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/fs-watcher/fs-watcher.test.ts` around lines 254 -
279, The test "resets debounce timer on new events" is timing-sensitive and can
flake under CI; update the waits around triggerEvents to give a larger safety
margin: after the first triggerEvents keep the short wait at ~30ms, then after
the second triggerEvents keep ~30ms and assert batches.length === 0, and then
wait a longer period (e.g., ~120ms) to ensure the 100ms debounce window elapses
before asserting batches length and contents; adjust the setTimeout durations in
this test (references: fsWatcher.switchTo, triggerEvents, debounce 100ms) to
these safer values so CI timing variance won't cause premature debounce firing.
| }): Promise<void> { | ||
| // No-op if already watching this workspace | ||
| if (this.active?.workspaceId === workspaceId) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
switchTo no-ops when the same workspaceId is passed, even if rootPath changed.
If, for example, a worktree is recreated at a different path (e.g., after a failed init + retry), calling switchTo with the same workspaceId but a new rootPath will be silently ignored. Consider comparing rootPath as well.
♻️ Suggested fix
- if (this.active?.workspaceId === workspaceId) {
+ if (
+ this.active?.workspaceId === workspaceId &&
+ this.active.rootPath === rootPath
+ ) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }): Promise<void> { | |
| // No-op if already watching this workspace | |
| if (this.active?.workspaceId === workspaceId) { | |
| return; | |
| } | |
| }): Promise<void> { | |
| // No-op if already watching this workspace | |
| if ( | |
| this.active?.workspaceId === workspaceId && | |
| this.active.rootPath === rootPath | |
| ) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/fs-watcher/fs-watcher.ts` around lines 63 - 67, The
switchTo method currently returns early when this.active?.workspaceId ===
workspaceId, which ignores changes to rootPath; update the early-return check in
switchTo to compare both workspaceId and rootPath (e.g.,
this.active?.workspaceId === workspaceId && this.active?.rootPath === rootPath)
so it only no-ops when both match, and ensure any path comparison normalizes
paths before comparing; if rootPath differs, allow the method to proceed to stop
the old watcher and start a new watcher for the new rootPath (references:
switchTo, this.active, workspaceId, rootPath).
| private async stopInternal(): Promise<void> { | ||
| const state = this.active; | ||
| if (!state) return; | ||
|
|
||
| // Flush any pending events before stopping | ||
| this.flush(); | ||
|
|
||
| if (state.debounceTimer) { | ||
| clearTimeout(state.debounceTimer); | ||
| } | ||
| if (state.maxWindowTimer) { | ||
| clearTimeout(state.maxWindowTimer); | ||
| } | ||
|
|
||
| await state.subscription.unsubscribe(); | ||
| this.active = null; | ||
|
|
||
| console.log( | ||
| `[fs-watcher] Stopped watching workspace ${state.workspaceId}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
stopInternal doesn't guard against unsubscribe failure.
If state.subscription.unsubscribe() throws, this.active is never set to null, leaving the watcher in an inconsistent state where it appears active but isn't subscribed. Consider wrapping the unsubscribe in a try/finally.
🛡️ Proposed fix
private async stopInternal(): Promise<void> {
const state = this.active;
if (!state) return;
+ // Null out active first to prevent re-entrant calls
+ this.active = null;
+
// Flush any pending events before stopping
this.flush();
if (state.debounceTimer) {
clearTimeout(state.debounceTimer);
}
if (state.maxWindowTimer) {
clearTimeout(state.maxWindowTimer);
}
- await state.subscription.unsubscribe();
- this.active = null;
+ try {
+ await state.subscription.unsubscribe();
+ } catch (err) {
+ console.error(
+ `[fs-watcher] Failed to unsubscribe workspace ${state.workspaceId}:`,
+ err,
+ );
+ }
console.log(
`[fs-watcher] Stopped watching workspace ${state.workspaceId}`,
);
}Note: Moving this.active = null before unsubscribe() also prevents handleEvents callbacks (which guard on this.active) from processing stale events during teardown.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async stopInternal(): Promise<void> { | |
| const state = this.active; | |
| if (!state) return; | |
| // Flush any pending events before stopping | |
| this.flush(); | |
| if (state.debounceTimer) { | |
| clearTimeout(state.debounceTimer); | |
| } | |
| if (state.maxWindowTimer) { | |
| clearTimeout(state.maxWindowTimer); | |
| } | |
| await state.subscription.unsubscribe(); | |
| this.active = null; | |
| console.log( | |
| `[fs-watcher] Stopped watching workspace ${state.workspaceId}`, | |
| ); | |
| } | |
| private async stopInternal(): Promise<void> { | |
| const state = this.active; | |
| if (!state) return; | |
| // Null out active first to prevent re-entrant calls | |
| this.active = null; | |
| // Flush any pending events before stopping | |
| this.flush(); | |
| if (state.debounceTimer) { | |
| clearTimeout(state.debounceTimer); | |
| } | |
| if (state.maxWindowTimer) { | |
| clearTimeout(state.maxWindowTimer); | |
| } | |
| try { | |
| await state.subscription.unsubscribe(); | |
| } catch (err) { | |
| console.error( | |
| `[fs-watcher] Failed to unsubscribe workspace ${state.workspaceId}:`, | |
| err, | |
| ); | |
| } | |
| console.log( | |
| `[fs-watcher] Stopped watching workspace ${state.workspaceId}`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/fs-watcher/fs-watcher.ts` around lines 142 - 162,
The stopInternal method must guard against unsubscribe throwing: ensure you
capture/unsubscribe in a try/catch/finally (or set this.active = null before
unsubscribe) so that this.active is always cleared and timers are cleared even
if state.subscription.unsubscribe() throws; specifically, keep the existing
flush() and timer clears, then either set this.active = null immediately (to
prevent handleEvents from processing stale events) and call
state.subscription.unsubscribe() inside a try/catch, or call unsubscribe in a
try block and move this.active = null into a finally block so the watcher cannot
remain in an inconsistent active state.
|
Closing as stale: created in Jan-Mar with no recent activity. If still relevant, re-open or re-create from a fresh branch. Bulk audit 2026-05-06. |
1 participant
Summary
@parcel/watcherserviceStaticPortsWatcher+ newFsWatcher) into one unified patternfilesystem/index.tsmonolith into focused sub-routersChanges
New: FsWatcher service (
src/main/lib/fs-watcher/)@parcel/watcherfor native recursive directory watching per workspacenode_modules,.git,dist,build,.next,.turbo,coverageNew: tRPC subscription (
filesystem.subscribe)trpc-electron)Watcher lifecycle
@parcel/watcheradded to rollup externals + native module copy scriptRetired:
StaticPortsWatcherfsWatcherbatch events, filtering for.superset/ports.jsonchangesstatic-ports/watcher.ts(158 lines)Decomposed filesystem router
search.ts— search index with Fuse.js, caching, cache invalidationoperations.ts— CRUD procedures (readDirectory, createFile, rename, delete, move, copy, etc.)subscription.ts— filesystem subscribe procedureindex.ts— thin composer using..._def.proceduresspread patternShared
useFsSubscriptionhookFilesView.tsxandChangesView.tsxdebounceMsparameterRightSidebar/hooks/useFsSubscription/Renderer updates
Dead code removed
useFileTree.ts(141 lines, never imported)FileTreeNodeinterface from shared typesTest Plan
bun run typecheckpasses across all packagesbun run lint:fixreports no issuesnpm installin terminal → events batch correctly, no UI stutter.superset/ports.json→ ports panel reflects changeSummary by CodeRabbit
New Features
Refactor
Docs
Tests