Skip to content

Skip file watchers on remote volumes#424

Merged
schuyler merged 1 commit into
schuyler:mainfrom
yusufm:Codex/skip-watchers-on-remote-volumes-019def52-b3da-7962-96cd-afa05d8d1592
Jun 30, 2026
Merged

Skip file watchers on remote volumes#424
schuyler merged 1 commit into
schuyler:mainfrom
yusufm:Codex/skip-watchers-on-remote-volumes-019def52-b3da-7962-96cd-afa05d8d1592

Conversation

@yusufm

@yusufm yusufm commented May 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • skip vnode file watching when a path is not on a local volume
  • avoid creating document/resource watchers for remote/network files
  • cover local and nil watchability checks

Tests

  • xcodebuild test -workspace "MacDown 3000.xcworkspace" -scheme MacDown -only-testing:MacDownTests/MPFileWatcherTests -only-testing:MacDownTests/MPResourceWatcherSetTests -destination 'platform=macOS'

Related to #371

@schuyler schuyler left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the PR, @yusufm!

Issues

  • Resource watchers unguarded: MPResourceWatcherSet -addWatcherForPath: is a second MPFileWatcher instantiation site with no canWatchPath: check, so resource files (images/CSS/scripts) on remote volumes still hit the same failure this PR fixes for documents.
  • Non-existent path regression: NSURLVolumeIsLocalKey requires the file to exist, so canWatchPath: returns NO for a path that transiently doesn't exist (e.g. after an external editor's atomic rename-replace save), which can leave auto-reload disabled — falling back to the parent directory for the locality check would avoid this.
  • Stale watcher on early-return: the new early-return in MPDocument -startFileWatching runs before stopFileWatching, so a previously-armed watcher can leak if canWatchPath: returns NO for an already-watched session.

Suggestions

  • Branch coverage: the empty-string and non-existent-path branches of canWatchPath: are untested and quick to cover in CI.
  • Diagnostics: passing an NSError * instead of NULL to getResourceValue:forKey:error: and logging it would make the "Device not configured" case from #371 easier to diagnose.
  • Doc comment: the header comment describes only the local-volume case, though the method also returns NO for nil, empty, and non-existent paths.

Generated by Claude Code

@schuyler schuyler merged commit e9ff2e6 into schuyler:main Jun 30, 2026
5 checks passed
schuyler pushed a commit that referenced this pull request Jun 30, 2026
## Summary
- skip vnode file watching when a path is not on a local volume
- avoid creating document/resource watchers for remote/network files
- cover local and nil watchability checks

## Tests
- xcodebuild test -workspace "MacDown 3000.xcworkspace" -scheme MacDown
-only-testing:MacDownTests/MPFileWatcherTests
-only-testing:MacDownTests/MPResourceWatcherSetTests -destination
'platform=macOS'

Related to #371
@schuyler schuyler added the rc-pending Included in a release candidate awaiting validation label Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rc-pending Included in a release candidate awaiting validation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants