Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves observability for theme file watching by surfacing Chokidar watcher errors via logging and runtime analytics, helping diagnose reports of files not syncing.
Changes:
- Add a Chokidar
.on('error')handler to log watcher errors and emit an analytics event. - Add a unit test covering watcher error handling and analytics emission.
- Mock
recordEventin tests to assert telemetry behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/theme/src/cli/utilities/theme-fs.ts | Adds watcher error handler that logs and records an analytics event. |
| packages/theme/src/cli/utilities/theme-fs.test.ts | Adds test coverage for watcher error emission and mocks analytics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test('outputs a warning when the watcher emits an error', async () => { | ||
| // Given | ||
| const {outputWarn} = await import('@shopify/cli-kit/node/output') | ||
| const themeFileSystem = mountThemeFileSystem(root) | ||
| await themeFileSystem.ready() | ||
| await themeFileSystem.startWatcher(themeId, adminSession) | ||
|
|
||
| // When | ||
| const watcher = chokidar.watch('') as EventEmitter | ||
| watcher.emit('error', new Error('EMFILE: too many open files')) | ||
|
|
||
| // Then | ||
| expect(outputWarn).toHaveBeenCalledWith('File watcher error: Error: EMFILE: too many open files') | ||
| expect(recordEvent).toHaveBeenCalledWith('theme-service:file-watcher:error') |
There was a problem hiding this comment.
The test expects outputWarn to be called, but the implementation logs watcher errors with outputDebug. As written, this test will fail—either update the expectation to outputDebug (and import it here) or change the implementation to use outputWarn so code and test match the intended user-facing behavior.
| .on('error', (error) => { | ||
| outputDebug(`File watcher error: ${error}`) | ||
| recordEvent('theme-service:file-watcher:error') | ||
| }) |
There was a problem hiding this comment.
recordEvent('theme-service:file-watcher:error') loses the error context (message/category) that would help diagnose OS-level watcher failures. Consider also (or instead) calling recordError(error) from @shopify/cli-kit/node/analytics, and when logging include error.stack when available so debug output contains actionable details.
| .on('unlink', queueFsEvent.bind(null, 'unlink')) | ||
| .on('error', (error) => { | ||
| outputDebug(`File watcher error: ${error}`) | ||
| recordEvent('theme-service:file-watcher:error') |
There was a problem hiding this comment.
Should we use recordError, too?
WHY are these changes introduced?
We have had some reports of files not syncing, and there's the possibility that the watcher is throwing an error we're silently swallowing.
WHAT is this pull request doing?
add a
.on('error')branch which will output a debug log and record it to our monorail events.How to test your changes?
I don't have a good way of replicating a real OS-level failure for this but feel free to run the test I added.
Post-release steps
Checklist
pnpm changeset add