feat(edge-apps): migrate to Vite dev server with dist-based deployment#690
feat(edge-apps): migrate to Vite dev server with dist-based deployment#690nicomiguelino merged 15 commits intomasterfrom
Conversation
- Add Vite dev server plugin with live mock data reloading - Update build system to output to dist/ directory for deployment - Modify index.html to reference TypeScript source files directly - Add edge-apps-scripts dev command for running Vite dev server - Create plugin to copy screenly.yml, screenly_qc.yml, and instance.yml to dist/ - Watch screenly.yml and mock-data.yml for changes during development - Update deploy script to use --path=dist/ flag Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
PR Reviewer Guide 🔍(Review updated until commit 286608d)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 286608d
Previous suggestionsSuggestions up to commit 8873844
|
nicomiguelino
left a comment
There was a problem hiding this comment.
Overall Assessment
This is a solid architectural improvement that modernizes the build system. The changes are well-structured and set up a good foundation for screenshot testing. I've left several inline comments with suggestions for improvements, primarily around error handling and HMR integration.
Main areas to address:
- File watching reliability and browser reload
- Error handling for YAML parsing
- Cleanup of file watchers
Once these are addressed, this will be ready to merge!
nicomiguelino
left a comment
There was a problem hiding this comment.
File: vite-plugins/dev-server.ts (Line 119-122)
Issue: File watching doesn't trigger browser reload
When YAML files change, the config regenerates but the browser doesn't automatically reload to reflect the changes.
Suggestion:
fs.watch(manifestPath, () => {
console.log('screenly.yml changed, regenerating mock data...')
config = generateMockData(rootDir)
server.ws.send({ type: 'full-reload', path: '*' })
})This will trigger a full page reload when the manifest changes.
nicomiguelino
left a comment
There was a problem hiding this comment.
File: vite-plugins/dev-server.ts (Line 119-130)
Issue: fs.watch() can fire multiple events and lacks error handling
The current implementation has a few issues:
- Multiple events can fire for a single file change (causing duplicate regeneration)
- No error handling if watch fails
- Watch handles are never cleaned up
Suggestion:
const watchers: FSWatcher[] = []
// Debounce to prevent multiple rapid regenerations
const debouncedRegenerate = debounce(() => {
config = generateMockData(rootDir)
server.ws.send({ type: 'full-reload' })
}, 100)
try {
const watcher = fs.watch(manifestPath, (eventType) => {
if (eventType === 'change') {
console.log('screenly.yml changed, regenerating mock data...')
debouncedRegenerate()
}
})
watcher.on('error', (err) => console.error('Watch error:', err))
watchers.push(watcher)
} catch (err) {
console.warn('Could not watch screenly.yml:', err)
}
// Add cleanup in plugin lifecycle
return {
name: 'screenly-dev-server',
configureServer(server) { /* ... */ },
buildEnd() {
watchers.forEach(w => w.close())
}
}
nicomiguelino
left a comment
There was a problem hiding this comment.
File: vite-plugins/dev-server.ts (Line 62-79)
Issue: Missing error handling for YAML parsing
If screenly.yml is malformed, the dev server will crash without a helpful error message.
Suggestion:
function generateMockData(rootDir: string): BaseScreenlyMockData {
const manifestPath = path.resolve(rootDir, 'screenly.yml')
const mockDataPath = path.resolve(rootDir, 'mock-data.yml')
let manifest
try {
manifest = YAML.parse(fs.readFileSync(manifestPath, 'utf8'))
} catch (err) {
console.error(`Failed to parse screenly.yml: ${err}`)
return defaultScreenlyConfig // Fallback to defaults
}
// Add validation
if (\!manifest?.settings || typeof manifest.settings \!== 'object') {
console.warn('Invalid screenly.yml: missing or invalid settings')
return defaultScreenlyConfig
}
const screenlyConfig: BaseScreenlyMockData = structuredClone(
defaultScreenlyConfig,
)
// ... rest of function
}
nicomiguelino
left a comment
There was a problem hiding this comment.
File: vite-plugins/dev-server.ts (Line 82-99)
Issue: mock-data.yml parsing also needs error handling
Same issue as with screenly.yml - malformed YAML will crash the server.
Suggestion:
// Override with mock-data.yml if it exists
if (fs.existsSync(mockDataPath)) {
try {
const mockData = YAML.parse(fs.readFileSync(mockDataPath, 'utf8'))
// Override metadata if present
if (mockData.metadata) {
Object.assign(screenlyConfig.metadata, mockData.metadata)
}
// Override settings if present
if (mockData.settings) {
Object.assign(screenlyConfig.settings, mockData.settings)
}
// Override cors_proxy_url if present
if (mockData.cors_proxy_url) {
screenlyConfig.cors_proxy_url = mockData.cors_proxy_url
}
} catch (err) {
console.error(`Failed to parse mock-data.yml: ${err}`)
// Continue with defaults from screenly.yml
}
}
nicomiguelino
left a comment
There was a problem hiding this comment.
File: vite-plugins/dev-server.ts (Line 134-142)
Nitpick: Add cache control header
Adding a cache control header ensures the browser doesn't cache the generated screenly.js file during development.
Suggestion:
server.middlewares.use((req, res, next) => {
if (req.url === '/screenly.js?version=1') {
const screenlyJsContent = generateScreenlyObject(config)
res.setHeader('Content-Type', 'application/javascript')
res.setHeader('Cache-Control', 'no-cache, no-store, must-revalidate')
res.end(screenlyJsContent)
return
}
next()
})
nicomiguelino
left a comment
There was a problem hiding this comment.
File: scripts/cli.ts (Line 107-111)
Issue: Empty catch block swallows errors
The empty catch block makes debugging failures difficult.
Suggestion:
} catch (err) {
console.error('Dev server failed:', err)
process.exit(1)
}
nicomiguelino
left a comment
There was a problem hiding this comment.
File: vite.config.ts (Line 14-20)
Nitpick: Add error handling for file copy operations
If copying manifest files fails, it should log a warning but not crash the build.
Suggestion:
for (const file of filesToCopy) {
const srcPath = resolve(process.cwd(), file)
if (existsSync(srcPath)) {
try {
const destPath = resolve(process.cwd(), 'dist', file)
copyFileSync(srcPath, destPath)
console.log(`Copied ${file} to dist/`)
} catch (err) {
console.warn(`Failed to copy ${file}:`, err)
}
}
}
nicomiguelino
left a comment
There was a problem hiding this comment.
File: clock/package.json (Line 8)
Question: Is the predev hook still needed?
The new dev server automatically generates mock data from screenly.yml and mock-data.yml. The predev hook runs screenly edge-app run --generate-mock-data, which might be redundant now.
Questions:
- Does
generate-mock-datacreate additional files that the dev server needs? - Can this hook be removed to simplify the workflow?
If it's no longer needed, consider removing it for clarity.
- Add server.fs.allow for app and library directories - Remove verbose console logs from file watchers - Add vite-plugins/ to prettier format script
- Update index.html to reference TypeScript source files - Simplify dev scripts to use edge-apps-scripts dev - Add --path=dist/ to deploy scripts - Preserve CORS proxy for cap-alerting and grafana apps
- Add getNodePath() helper for NODE_PATH setup - Add setupSignalHandlers() for process cleanup - Add spawnWithSignalHandling() to combine spawn logic - Add getVitePaths() for Vite binary and config paths - Simplify command functions by reusing helpers
- Import pizza.png and screenly_food.svg assets in utils.ts - Update manifest default values for logo_url to empty string - Add getDefaultLogoUrl() function with hardware-specific logic - Update tests to check bundled asset paths instead of relative paths - Add testAssetUrl helper to eliminate test duplication Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates multiple Edge Apps to use a shared Vite dev server workflow and dist-based deployment, including a custom dev-server plugin to serve screenly.js mock data and copying manifests into dist/ for deploys.
Changes:
- Update Edge App
dev/deployscripts to run Vite dev server and deploy fromdist/. - Switch app HTML entrypoints to Vite module loading (
/src/main.ts) instead ofdist/js/main.js+dist/css/style.css. - Add shared Vite config/plugin support in
edge-apps-library(mockscreenly.js, copy manifests intodist/).
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| edge-apps/weather/package.json | Use edge-apps-scripts dev and deploy from dist/. |
| edge-apps/weather/index.html | Switch to Vite module entry (/src/main.ts), remove direct dist/ asset links. |
| edge-apps/simple-timer/package.json | Use edge-apps-scripts dev and deploy from dist/. |
| edge-apps/simple-timer/index.html | Switch to Vite module entry, remove direct dist/ asset links. |
| edge-apps/qr-code/package.json | Use edge-apps-scripts dev and deploy from dist/. |
| edge-apps/qr-code/index.html | Switch to Vite module entry, remove direct dist/ asset links. |
| edge-apps/menu-board/src/utils.ts | Use bundled asset imports for default background/logo (and add default logo helper). |
| edge-apps/menu-board/src/main.ts | Use getDefaultLogoUrl() as the app-side default for logo_url. |
| edge-apps/menu-board/src/main.test.ts | Refactor tests with helper coverage for background + logo defaults. |
| edge-apps/menu-board/screenly_qc.yml | Clear logo_url manifest default to defer to app-side default logic. |
| edge-apps/menu-board/screenly.yml | Clear logo_url manifest default to defer to app-side default logic. |
| edge-apps/menu-board/package.json | Use edge-apps-scripts dev and deploy from dist/. |
| edge-apps/menu-board/index.html | Switch to Vite module entry, remove direct dist/ asset links. |
| edge-apps/grafana/package.json | Run Vite dev server alongside the CORS proxy; deploy from dist/. |
| edge-apps/grafana/index.html | Switch to Vite module entry, remove direct dist/ asset links. |
| edge-apps/edge-apps-library/vite.config.ts | Build from index.html, add dev plugin, allow fs access for shared library, copy manifests to dist/. |
| edge-apps/edge-apps-library/vite-plugins/dev-server.ts | New Vite middleware to generate and serve mock screenly.js from YAML. |
| edge-apps/edge-apps-library/scripts/cli.ts | Add edge-apps-scripts dev command to start Vite dev server. |
| edge-apps/edge-apps-library/package.json | Add yaml dev dependency and include vite-plugins/ in formatting. |
| edge-apps/edge-apps-library/bun.lock | Lockfile updates for the new yaml dependency. |
| edge-apps/clock/package.json | Use edge-apps-scripts dev and deploy from dist/. |
| edge-apps/clock/index.html | Switch to Vite module entry, remove direct dist/ asset links. |
| edge-apps/cap-alerting/package.json | Run Vite dev server alongside CORS proxy; deploy from dist/. |
| edge-apps/cap-alerting/index.html | Switch to Vite module entry, remove direct dist/ asset links. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…es to be checked for formatting
- Add error handling for YAML parsing to prevent dev server crashes - Accept previousConfig parameter to fallback on parse errors - Switch from fs.watch to Vite's server.watcher for robust file watching - Watch both screenly.yml and mock-data.yml regardless of existence - Handle add, change, and unlink events for both config files Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Persistent review updated to latest commit 286608d |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Send full-reload message to connected clients via WebSocket - Automatically reload browser when screenly.yml or mock-data.yml changes - Eliminates need for manual refresh during development Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Switch from blueprint to edge-apps-library CORS proxy reference - Standardizes with grafana and other migrated apps - No functional changes to dev mode behavior Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Wrap copyFileSync in try/catch with descriptive error message - Re-throw to preserve build failure behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove hardware-specific branching from getDefaultBackgroundImage and getDefaultLogoUrl - Assets are inlined as data URIs by Vite, working on all hardware types - Remove unused getHardware/Hardware imports from utils.ts - Simplify tests by removing hardware-specific mock setup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add dev and deploy commands to edge-apps-library recommended scripts - Add Development Server section to available commands - Fix entry point reference from src/main.ts to index.html - Update menu-board logo_url default to reflect empty manifest value Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
User description
Summary
This PR migrates the clock Edge App to use Vite's dev server with a dist-based deployment approach. The main change is that
index.htmlis now built and output to thedist/directory, which serves as the deployment source.Key Changes
screenlyDevServer) that serves mock data during developmentscreenly.ymlormock-data.ymlchangesindex.htmland all assets todist/directoryindex.htmlto reference TypeScript source files directly (Vite handles transformation)edge-apps-scripts devcommand to run Vite dev serverscreenly.yml,screenly_qc.yml, andinstance.ymltodist/Motivation
This change establishes the foundation for automated screenshot testing (likely via Playwright) by ensuring Edge Apps build to a predictable
dist/directory structure. Havingindex.htmlindist/makes it easier to serve and test the built application programmatically.What's Next
PR Type
Enhancement, Bug fix, Tests
Description
Add Vite dev server CLI command
Provide
screenly.jsmock middlewareBuild from
index.htmlintodist/Fix menu-board assets bundling/tests
Diagram Walkthrough
File Walkthrough
3 files
Add `dev` command and shared spawn helpersAdd Vite middleware to serve mock `screenly.js`Import assets for bundling and add logo helper17 files
Switch build input to `index.html` and add pluginsLoad Vite module entrypoint instead of `dist` assetsLoad Vite module entrypoint instead of `dist` assetsLoad Vite module entrypoint instead of `dist` assetsLoad Vite module entrypoint instead of `dist` assetsLoad Vite module entrypoint instead of `dist` assetsLoad Vite module entrypoint instead of `dist` assetsLoad Vite module entrypoint instead of `dist` assetsRun Vite dev server and deploy from `dist/`Simplify dev script and deploy from `dist/`Use Vite dev server plus CORS proxy; deploy `dist/`Simplify dev script and deploy from `dist/`Clear default `logo_url` setting valueClear default `logo_url` setting valueSimplify dev script and deploy from `dist/`Simplify dev script and deploy from `dist/`Simplify dev script and deploy from `dist/`1 files
Refactor asset URL tests and add logo coverage1 files
Use computed default logo URL from utils1 files
Add `yaml` dependency and format `vite-plugins/`