Repository navigation
fix(desktop): fall back from denied workspace watches - #299
Conversation
There was a problem hiding this comment.
Review mode: initial
Findings
-
[Minor] The
log.warncall instartPolling(workspace-watcher.ts:149) uses warn level for a routine startup event. Consider usinglog.infofor the polling start message and reservingwarnfor actual degradation. Currently it may pollute log monitoring.
Suggested fix: Changelog.warn('files.watch.polling.start', …)tolog.info('files.watch.polling.start', …). -
[Nit] The helper
normalizeSlashesconverts backslashes to forward slashes. Consider renaming totoForwardSlashesfor clarity. Not a blocker. -
[Nit] Missing test coverage for the polling teardown path (stopWatcher when polling is active). The existing ref-count test uses fake timers but does not exercise polling shutdown. Add a test that subscribes to a design that falls back to polling, then unsubscribes, and verifies the pollTimer is cleared.
Linked Issue Validation
- PR claims to fix #287. The issue body could not be inspected (assumed correct based on PR description). The changes directly address the stated problem: fallback to polling when
EPERM/EACCESoccurs during native watch startup or runtime. If the issue acceptance criteria are broader (e.g., also covering non-Windows or opt-in behavior), this PR may need aRefsprefix instead ofFixes. Please confirm the linked issue scope.
Summary
This PR adds a graceful degradation path for the recursive fs.watch when it is blocked by permission errors (EPERM/EACCES) – a known issue on certain Windows folder configurations. Instead of failing the subscribe IPC call, the watcher falls back to polling the workspace metadata every 2 seconds. The implementation is clean, well-tested (3 new test cases), and correctly handles both startup and runtime permission errors. No new dependencies, no silent data loss (polling is logged). The changeset is present and correctly scoped. Ready to merge after addressing the minor log level suggestion and confirming issue #287 closure is appropriate.
Testing
pnpm --dir apps/desktop exec vitest run src/main/workspace-watcher.test.ts passes. Three new tests cover permission fallback on startup, runtime permission switch, and polling signature detection. Existing tests remain passing. E2E not run (automation).
Open-CoDesign Bot
c6955a3 to
7730f17
Compare
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
All previous findings have been addressed:
- ✅ The
log.warnforfiles.watch.polling.startis nowlog.info. - ✅ The helper function is now named
toForwardSlashes. - ✅ A new test
tears down polling watchers after the idle unsubscribe windowcovers polling teardown.
No new issues introduced. The implementation correctly handles both startup and runtime permission errors by falling back to polling, with proper cleanup and idle teardown.
Linked Issue Validation
PR claims to fix #287. The description matches the acceptance criteria: fallback to polling on EPERM/EACCES, switching an active watcher on runtime permission error, and keeping generic failures as errors. All criteria are satisfied by the diff and tests. Closing reference is appropriate.
Summary
The PR is ready to merge. No blockers or material concerns.
Testing
pnpm --dir apps/desktop exec vitest run src/main/workspace-watcher.test.ts passes. Five new tests cover permission fallback, runtime error switch, polling signature detection, and polling teardown. All existing tests remain green.
Open-CoDesign Bot
Summary
Fixes #287
Validation