feat(clock, weather, edge-apps-library): redesign the clock and weather apps #616
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive Edge Apps library with web components, utilities, CLI tooling, and sample applications to streamline Screenly Edge App development.
Changes:
- Added reusable web components (auto-scaler, app-header, dev-tools, brand-logo) with encapsulated TypeScript, HTML, and CSS
- Integrated CLI generator for scaffolding new Edge Apps from a template
- Provided sample applications (clock-new, weather-new) demonstrating library usage with Vite configurations
Reviewed changes
Copilot reviewed 71 out of 138 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| edge-apps/edge-apps-library/src/components/* | Web component implementations for UI elements |
| edge-apps/edge-apps-library/src/utils/* | Utility functions for screen, weather, locale, and time |
| edge-apps/edge-apps-library/scripts/* | CLI tools for app generation |
| edge-apps/edge-apps-library/template/* | Template files for new Edge Apps |
| edge-apps/weather-new/* | Sample weather app implementation |
| edge-apps/clock-new/* | Sample clock app implementation |
| edge-apps/edge-apps-library/src/styles/* | Design system styles and tokens |
| edge-apps/edge-apps-library/configs/* | Shared Tailwind configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Run the CLI | ||
| run().catch((error) => { | ||
| console.error('Error:', error.message) | ||
| process.exit(1) | ||
| }) |
There was a problem hiding this comment.
The CLI execution code (lines 214-219) appears at the end of the file but there's already a main execution flow expected through the module. This could cause the CLI to run twice if the file is imported elsewhere or result in unintended execution. This code should likely be wrapped in a check like if (import.meta.url === \file://${process.argv[1]}`) { ... }` to only run when executed directly.
| // Run the CLI | |
| run().catch((error) => { | |
| console.error('Error:', error.message) | |
| process.exit(1) | |
| }) | |
| // Run the CLI only when executed directly | |
| if (import.meta.url === `file://${process.argv[1]}`) { | |
| run().catch((error) => { | |
| console.error('Error:', error.message) | |
| process.exit(1) | |
| }) | |
| } |
| const screenlyGlobal = (window as any).screenly | ||
| const sentryDsn = screenlyGlobal?.settings?.sentry_dsn as string | undefined | ||
| if (sentryDsn) { | ||
| Sentry.init({ dsn: sentryDsn }) |
There was a problem hiding this comment.
The code references Sentry but there's no import statement for the Sentry SDK. This will cause a ReferenceError at runtime when Sentry DSN is configured.
| // Lazy-load offline-geocode-city so apps that never call getLocale | ||
| // don't need to bundle it (and won't hit its lz-string dependency). | ||
| const { getNearestCity } = await import('offline-geocode-city') |
There was a problem hiding this comment.
The comment mentions avoiding the lz-string dependency, but this optimization only works if the module is tree-shakeable and the bundler is configured correctly. Consider documenting this requirement in the function's JSDoc or README to ensure developers configure their build tools appropriately.
| "scripts": { | ||
| "generate-mock-data": "screenly edge-app run --generate-mock-data", | ||
| "predev": "bun run generate-mock-data", | ||
| "dev": "vite", |
There was a problem hiding this comment.
Please make use of the Screenly Edge App server (i.e., screenly edge-app run) instead of vite. Please refer to the package.json of existing new Edge Apps like the qr-code, menu-board, grafana, and cap-alerting apps.
| "scripts": { | ||
| "generate-mock-data": "screenly edge-app run --generate-mock-data", | ||
| "predev": "bun run generate-mock-data", | ||
| "dev": "vite", |
There was a problem hiding this comment.
Please make use of the Screenly Edge App server (i.e., screenly edge-app run) instead of vite. Please refer to the package.json of existing new Edge Apps like the qr-code, menu-board, grafana, and cap-alerting apps.
There was a problem hiding this comment.
Please refer to the package.json of existing new Edge Apps like the qr-code, menu-board, grafana, and cap-alerting apps. This file should look similar to package.json of the Edge Apps mentioned.
There was a problem hiding this comment.
Please refer to the package.json of existing new Edge Apps like the qr-code, menu-board, grafana, and cap-alerting apps. This file should look similar to package.json of the Edge Apps mentioned.
There was a problem hiding this comment.
Would it be possible to move this inside the Edge Apps library instead?
|
@iamplugged, here are my general comments:
|
There was a problem hiding this comment.
This was created before the changes that centralizes Tailwind into the Edge Apps library were merged. Please remove this file if not needed.
There was a problem hiding this comment.
We already moved such files into the Edge Apps library to prevent duplicate files. We can now remove this.
There was a problem hiding this comment.
We already moved such files into the Edge Apps library to prevent duplicate files. We can now remove this.
There was a problem hiding this comment.
We already moved such files into the Edge Apps library to prevent duplicate files. We can now remove this.
There was a problem hiding this comment.
We already moved such files into the Edge Apps library to prevent duplicate files. We can now remove this.
There was a problem hiding this comment.
We already moved such files into the Edge Apps library to prevent duplicate files. We can now remove this.
There was a problem hiding this comment.
We already moved such files into the Edge Apps library to prevent duplicate files. We can now remove this.
XSS Security ReviewI've identified CRITICAL XSS vulnerabilities in this PR that should be addressed before merging: 🚨 CRITICAL: XSS in weather-new appLocation: Problem: Using
Attack Vector Example: If a malicious actor could control Solution: Replace
|
…o the existing `clock` and `weather` directories (#657)
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove all comments from tsconfig.json files in clock, weather, and template directories to fix JSON parsing errors in super-linter. Also add root-level package.json to .prettierignore.
…are rendered in players properly (#661)
nicomiguelino
changed the title
…ort.meta errors (#662)
…ponents Apply necessary changes to enable the clock app redesign based on #616: edge-apps-library changes: - Add web components: auto-scaler, app-header, brand-logo, dev-tools - Add Screenly Design System styles (base, tokens) - Add core utilities and component registration - Add weather icon mapping utilities - Add screen orientation detection utilities - Update package.json exports to include components, core, and styles - Update vite.config.ts output format to IIFE clock-new changes: - Implement clock app with <auto-scaler> and <app-header> web components - Use library utilities (getMetadata, getTimeZone, getLocale, formatTime, etc.) - Add weather integration with OpenWeatherMap API - Include Kelly Slab font and background image - Add all weather condition icons - Import Screenly Design System styles - Implement responsive design for portrait and landscape Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
2 participants
User description
@screenly/edge-appsis a TypeScript library and tooling set for building Screenly Edge Apps. It provides:Use this package when you want to build modern, Vite-based Edge Apps that follow Screenly’s best practices out of the box.
How is it structured?
At a high level:
src/: Source for the library itself.src/components/: Reusable UI components (app-header,brand-logo,dev-tools,auto-scaler, and registry utilities).src/core/: Core entry points for Edge Apps runtime helpers. This is there for backward compatibility.src/styles/: Base styles and design tokens (fonts, colors, spacing, typography, etc.).src/utils/: Utility functions for locale, time/date formatting, metadata, screen data, settings, theme, UTM, and weather.src/types/: Shared TypeScript types.src/test/: Test helpers and JSDOM setup for unit tests.template/: A minimal Edge App template (Vite + TypeScript + Tailwind CSS) used as the starting point when creating new apps.scripts/: CLI helpers (cli.ts,create.ts) used to generate apps from the template.bin/edge-apps-scripts.ts: The published CLI entry point (wired via theedge-apps-scriptsbinary inpackage.json).Associated PRs
Most of these pull requests are stacked PRs (i.e., PRs that are branched off this PR).
clock-newandweather-newdirectories into the existingclockandweatherdirectories #657PR Type
Enhancement, Tests
Description
Add core web components (auto-scaler, header, dev-tools, logo)
Integrate CLI generator and app scaffolding scripts
Provide sample apps (clock-new, weather-new) with Vite configs
Include screen & weather utilities with unit tests
File Walkthrough
13 files
Implement `` web component for scalingCreate `` component with time/date displayAdd `` overlay for developmentIntroduce `` component with fallbackExport and register all web componentsAdd `initEdgeApp` helper for declarative setupDefine orientation and auto-scaler option typesAdd screen orientation detection utilitiesProvide weather icon mapping utilitiesExtend CLI with `create` commandAdd Weather App main logic and renderingAdd Clock App main logic and UI updatesConsolidate exports for utils, core, components, types4 files
Implement app generator CLI scriptConfigure Vite for library with custom pluginsSetup Vite config for weather-new appSetup Vite config for clock-new app1 files
Add unit tests for screen utilities55 files