Skip to content

Conversation

@vpetersson
Copy link
Contributor

@vpetersson vpetersson commented Dec 6, 2025

User description

Still requires testing, but the foundation is in here.


PR Type

Tests, Enhancement


Description

  • Add comprehensive CAP v1.2 parser tests

  • Implement robust CAP feed fetcher

  • Add fetcher unit tests with caching

  • Configure project and tooling


Diagram Walkthrough

flowchart LR
  ParserTests["CAP parser tests"] -- validate XML parsing --> MainParser["src/main.ts"]
  Fetcher["CAPFetcher (fetcher.ts)"] -- periodic fetch, cache, retry --> Network["HTTP fetch"]
  FetcherTests["Fetcher unit tests"] -- mock fetch/localStorage --> Fetcher
  AppShell["index.html + assets"] -- loads --> Bundle["static/js/main.js"]
Loading

File Walkthrough

Relevant files
Tests
3 files
index.test.ts
Comprehensive CAP v1.2 parser test suite                                 
+1858/-0
fetcher.test.ts
Unit tests for CAPFetcher caching and retries                       
+612/-0 
test.cap
Test CAP sample file                                                                         
+29/-0   
Enhancement
5 files
fetcher.ts
Implement CAP feed fetcher with cache and backoff               
+348/-0 
index.html
App HTML shell and script includes                                             
+16/-0   
index.ts
Edge app bootstrap and integration                                             
+546/-0 
main.ts
CAP XML parsing and app logic                                                       
+490/-0 
main.js
Compiled frontend logic bundle                                                     
+6118/-0
Configuration changes
7 files
eslint.config.ts
Add TypeScript ESLint configuration                                           
+18/-0   
tailwind.config.js
Tailwind configuration with extended breakpoints                 
+26/-0   
tsconfig.json
TypeScript compiler configuration for app                               
+17/-0   
.prettierrc
Prettier formatting configuration                                               
+6/-0     
package.json
Project package metadata and scripts                                         
+39/-0   
screenly.yml
Screenly app manifest                                                                       
+58/-0   
screenly_qc.yml
Screenly QC configuration                                                               
+58/-0   
Documentation
7 files
demo-6-shooter.cap
Add demo CAP alert: active shooter scenario                           
+47/-0   
demo-5-hazmat.cap
Add demo CAP alert: hazmat spill                                                 
+48/-0   
demo-3-flood.cap
Add demo CAP alert: flood warning                                               
+43/-0   
demo-4-earthquake.cap
Add demo CAP alert: earthquake advisory                                   
+47/-0   
demo-1-tornado.cap
Add demo CAP alert: tornado warning                                           
+38/-0   
demo-2-fire.cap
Add demo CAP alert: fire emergency                                             
+36/-0   
README.md
Documentation for CAP Alerting app                                             
+35/-0   
Formatting
2 files
style.css
Compiled stylesheet for app UI                                                     
+2/-0     
input.css
Tailwind input styles                                                                       
+135/-0 

@github-actions
Copy link

github-actions bot commented Dec 6, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Test Robustness

Several expectations assume numeric parsing (e.g., geocode value to number, resource size to number). Confirm the parser intentionally coerces these fields and handles leading zeros without losing semantic meaning (e.g., FIPS codes).

  const area = alerts[0].infos[0].areas[0]
  expect(area.geocode).toBeDefined()
  expect(area.geocode.valueName).toBe('FIPS6')
  expect(area.geocode.value).toBe(6017)
})
Global Mocks

Tests overwrite globals like window, fetch, and localStorage; ensure isolation/cleanup across runs and environments (Bun/node) to prevent leakage and flaky behavior.

  // Setup mocks
  global.localStorage = localStorageMock as any
  global.fetch = mockFetch as any
  global.window = { setInterval, clearInterval, setTimeout, clearTimeout } as any

  // Clear localStorage before each test
  localStorageMock.clear()

  // Reset fetch mock
  mockFetch.mockReset()
})

afterEach(() => {
  mockFetch.mockRestore()
})
Backoff Timing Flakiness

Exponential backoff assertion relies on elapsed wall time with jitter; this can be flaky on slower CI. Consider loosening thresholds or mocking timers.

it('should use exponential backoff', async () => {
  mockFetch.mockRejectedValue(new Error('Network error'))

  const fetcher = new CAPFetcher({
    feedUrl: 'https://example.com/feed.xml',
    corsProxyUrl: 'https://proxy.com',
    maxRetries: 3,
    initialRetryDelay: 100,
  })

  const updateCallback = mock()
  const startTime = Date.now()

  fetcher.start(updateCallback)

  await waitFor(() => mockFetch.mock.calls.length >= 3, 3000)

  const elapsed = Date.now() - startTime

  // Should take at least 100ms (1st retry) + 200ms (2nd retry) = 300ms
  // With jitter, it could be slightly less, so check for at least 200ms
  expect(elapsed).toBeGreaterThan(200)

  fetcher.stop()
})

@github-actions
Copy link

github-actions bot commented Dec 6, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Normalize proxy URL joining

Concatenating the proxy and full URL with a slash can produce double slashes or
malformed URLs if corsProxyUrl already ends with a slash. Normalize the join to
avoid fetch failures due to bad URLs.

edge-apps/cap-alerting/src/fetcher.ts [274-276]

 if (this.config.feedUrl.match(/^https?:/)) {
-  url = `${this.config.corsProxyUrl}/${this.config.feedUrl}`
+  const proxy = (this.config.corsProxyUrl || '').replace(/\/+$/, '')
+  const target = this.config.feedUrl.replace(/^\/+/, '')
+  url = `${proxy}/${target}`
 }
Suggestion importance[1-10]: 8

__

Why: Normalizing the proxy and target URL prevents malformed URLs (double slashes) and potential fetch failures; it's a precise, high-impact fix in the new fetcher code.

Medium
Stabilize timing-based test assertions

Make the time-based assertion resilient to scheduler variability by spying on
setTimeout calls or summing the scheduled delays rather than wall-clock.
Timing-based tests are flaky under load or CI and can intermittently fail.

edge-apps/cap-alerting/src/fetcher.test.ts [278-301]

 it('should use exponential backoff', async () => {
   mockFetch.mockRejectedValue(new Error('Network error'))
 
   const fetcher = new CAPFetcher({
     feedUrl: 'https://example.com/feed.xml',
     corsProxyUrl: 'https://proxy.com',
     maxRetries: 3,
     initialRetryDelay: 100,
   })
 
   const updateCallback = mock()
-  const startTime = Date.now()
-  
+  const timeouts: number[] = []
+  const originalSetTimeout = global.setTimeout
+  ;(global as any).setTimeout = (fn: (...args: any[]) => any, delay?: number, ...args: any[]) => {
+    if (typeof delay === 'number') timeouts.push(delay)
+    return originalSetTimeout(fn, delay as number, ...args)
+  }
+
   fetcher.start(updateCallback)
-
   await waitFor(() => mockFetch.mock.calls.length >= 3, 3000)
 
-  const elapsed = Date.now() - startTime
-  
-  // Should take at least 100ms (1st retry) + 200ms (2nd retry) = 300ms
-  // With jitter, it could be slightly less, so check for at least 200ms
-  expect(elapsed).toBeGreaterThan(200)
+  expect(timeouts.some(d => d >= 100)).toBe(true)
+  expect(timeouts.some(d => d >= 200)).toBe(true)
 
   fetcher.stop()
+  ;(global as any).setTimeout = originalSetTimeout
 })
Suggestion importance[1-10]: 7

__

Why: Replacing wall-clock timing with captured setTimeout delays makes the exponential backoff test less flaky. This is relevant to the provided test and improves robustness, though it's an enhancement rather than a critical fix.

Medium
Possible issue
Restore mutated globals after tests

Restore mutated globals like window and localStorage in afterEach to prevent
cross-test contamination. Without restoring, later tests may rely on stale mocks
leading to flaky or misleading results.

edge-apps/cap-alerting/src/fetcher.test.ts [54-56]

+const originalWindow = (global as any).window
+const originalFetch = global.fetch
+const originalLocalStorage = (global as any).localStorage
+
 afterEach(() => {
   mockFetch.mockRestore()
+  ;(global as any).window = originalWindow
+  ;(global as any).fetch = originalFetch
+  ;(global as any).localStorage = originalLocalStorage
+  localStorageMock.clear()
 })
Suggestion importance[1-10]: 7

__

Why: Restoring window, fetch, and localStorage after each test prevents contamination and flakiness. It directly addresses the teardown block and improves test reliability without altering functionality.

Medium
Remove lookbehind from splitter

The regex relies on lookbehind, which is unsupported on some embedded browsers,
causing failures in sentence splitting. Replace with a more compatible splitter that
doesn’t use lookbehind to avoid runtime errors on older WebViews.

edge-apps/cap-alerting/index.ts [217-222]

 function splitIntoSentences(text: string): string[] {
-  return text
-    .split(/(?<=[.!?])\s+/)
-    .map((s) => s.trim())
-    .filter((s) => s.length > 0)
+  // Split on punctuation followed by whitespace without using lookbehind
+  const parts = text.split(/([.!?])\s+/)
+  const sentences: string[] = []
+  for (let i = 0; i < parts.length; i += 2) {
+    const chunk = parts[i] ?? ''
+    const punct = parts[i + 1] ?? ''
+    const s = (chunk + punct).trim()
+    if (s) sentences.push(s)
+  }
+  return sentences
 }
Suggestion importance[1-10]: 7

__

Why: This avoids regex lookbehind that may not be supported in some WebViews, improving robustness with a compatible splitting approach; impact is moderate and change is accurate.

Medium
Avoid clobbering global window

Provide a minimal window object only if it does not already exist to avoid
overwriting the real global in environments where window is present. This prevents
unintended side effects across tests and potential timer API mismatches. Guard the
assignment and restore it in teardown.

edge-apps/cap-alerting/src/fetcher.test.ts [45]

-global.window = { setInterval, clearInterval, setTimeout, clearTimeout } as any
+const originalWindow = (global as any).window
+if (!originalWindow) {
+  ;(global as any).window = { setInterval, clearInterval, setTimeout, clearTimeout }
+}
Suggestion importance[1-10]: 6

__

Why: Guarding against overwriting global.window reduces cross-test side effects and is contextually accurate since the test assigns window. Impact is moderate and correctness is sound, though not critical.

Low
Fix robust keyword highlighting

The current regex uses the word boundary token with phrases containing spaces and
apostrophes, which can fail and cause partial matches (e.g., "NOW" in "KNOWN").
Also, keywords are duplicated and not escaped, risking unintended regex behavior.
Normalize and deduplicate keywords, escape regex specials, and match on non-word
boundaries using lookarounds.

edge-apps/cap-alerting/index.ts [188-215]

 function highlightKeywords(text: string): string {
-  const keywords = [
+  const rawKeywords = [
     'DO NOT',
-    'DON\'T',
-    'DO NOT',
+    "DON'T",
     'IMMEDIATELY',
     'IMMEDIATE',
     'NOW',
     'MOVE TO',
     'EVACUATE',
     'CALL',
     'WARNING',
     'DANGER',
     'SHELTER',
     'TAKE COVER',
     'AVOID',
     'STAY',
     'SEEK',
   ]
-
+  // Deduplicate and escape for regex
+  const esc = (s: string) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
+  const keywords = Array.from(new Set(rawKeywords)).map(esc)
   let result = text
-  keywords.forEach((keyword) => {
-    const regex = new RegExp(`\\b(${keyword})\\b`, 'gi')
+  keywords.forEach((kw) => {
+    // Use lookarounds to avoid matching inside larger words and handle spaces/apostrophes
+    const regex = new RegExp(`(?<![A-Za-z0-9])(${kw})(?![A-Za-z0-9])`, 'gi')
     result = result.replace(regex, '<strong>$1</strong>')
   })
-
   return result
 }
Suggestion importance[1-10]: 6

__

Why: The proposal correctly deduplicates and escapes keywords and avoids partial word matches; however, the original use of \b is already reasonable for most cases and the added lookbehind/lookahead may reduce compatibility on older environments.

Low

@nicomiguelino
Copy link
Contributor

As discussed, I'll continue on this PR from this point forward (i.e., refine implementation, write tests, increase coverage).

@nicomiguelino nicomiguelino self-assigned this Dec 8, 2025
title: Offline Mode
optional: true
default_value: 'false'
help_text: When enabled, avoid network fetches and use cached data (true/false).
Copy link
Contributor

@renatgalimov renatgalimov Dec 9, 2025

Choose a reason for hiding this comment

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

🤔 Let's ensure that our system can work offline by default. Otherwise, human factors could affect its reliability.

Copy link
Contributor

Choose a reason for hiding this comment

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

libedgeapp

The change was made by Bun when `bun install` was executed.
- Include `cap-alerting/` in `.gitignore`
- Have the unit tests be run via `bun run test:unit` instead
- Extract common build flags into shared scripts to reduce duplication
- Add build:css:common and build:js:common scripts
- Add watch mode for CSS and JS builds
- Add npm-run-all2 dependency for parallel task execution
- Update isAnywhereScreen to check for empty string or undefined
- Use isAnywhereScreen in cap-alerting demo mode logic
- Extract CAP type definitions into separate types/cap.ts file
- Use getTags() helper instead of direct metadata access
… setting

- Replace separate demo_mode and test_mode boolean settings with single mode enumeration
- Mode supports three values: production, demo, test
- Add CAPMode type for type-safe mode handling
- Update screenly.yml and screenly_qc.yml with mode setting as select type
- Update src/main.ts and index.ts to use CAPMode type
- Delete legacy index.ts entry point
- Delete orphaned CAPFetcher implementation (fetcher.ts)
- Delete comprehensive CAPFetcher test suite (fetcher.test.ts)
- Update index.test.ts to use modern @screenly/edge-apps imports
- Update package.json lint script to reflect active files only
- Create src/fetcher.ts with CAPFetcher class
- Extract test, demo, and live data fetching logic
- Create comprehensive test suite in src/fetcher.test.ts
- Update main.ts to use CAPFetcher instead of inline logic
- Remove DEMO_BASE_URL, fetchCapData, and related inline fetch code
- Update package.json lint script to include fetcher files
…odule

- Create src/parser.ts with parseCap function
- Rename index.test.ts to src/parser.test.ts with CAP parsing tests
- Update src/main.ts to import parseCap from parser module
- Update package.json lint script to reference new parser files
- Use getSettingWithDefault for all settings parsing in main.ts
- Extract getNearestExit, highlightKeywords, splitIntoSentences, and proxyUrl to separate files
- Create render.ts for rendering utilities
- Create utils.ts for shared utility functions
- Add abbreviation support to splitIntoSentences for better sentence parsing
- Move getNearestExit tests to utils.test.ts
- Add comprehensive unit tests for splitIntoSentences and proxyUrl
- Update package.json lint script to use wildcard pattern for TypeScript files
- Fix type-check errors by replacing replaceAll with replace
- Rename audio_alert to mute_sound in both screenly.yml and screenly_qc.yml
- Update help_text to use boolean type structure for toggle switch display
- Invert logic in main.ts: playAudio = !getSettingWithDefault(mute_sound, false)
- Update all settings to use structured help_text format
- Add type information (string, number, boolean, select) to all settings
- Remove (true/false) notation from boolean settings
- Ensures consistent UI presentation for all setting types
@renatgalimov renatgalimov requested a review from Copilot December 30, 2025 15:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a comprehensive Common Alerting Protocol (CAP) v1.2 edge app for displaying emergency alerts on Screenly digital signage screens. The implementation includes robust XML parsing, caching mechanisms, and multiple operating modes (test, demo, production).

Key changes:

  • Complete CAP v1.2 parser with support for all standard fields and multi-language alerts
  • Fetcher with caching, offline mode, and demo capabilities
  • Responsive UI optimized for digital signage displays using viewport-based typography

Reviewed changes

Copilot reviewed 31 out of 33 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
edge-apps/edge-apps-library/src/utils/metadata.ts Adds isAnywhereScreen() helper to detect Anywhere screens
edge-apps/cap-alerting/src/parser.ts CAP XML parser supporting all v1.2 fields
edge-apps/cap-alerting/src/fetcher.ts Feed fetcher with caching and multiple modes
edge-apps/cap-alerting/src/main.ts Main app logic and alert rendering
edge-apps/cap-alerting/src/utils.ts Utility functions for exit tags and text processing
edge-apps/cap-alerting/src/render.ts Keyword highlighting for emergency instructions
edge-apps/cap-alerting/src/types/cap.ts TypeScript interfaces for CAP data structures
edge-apps/cap-alerting/src/input.css Viewport-optimized styles for digital signage
static/*.cap Demo CAP alert files for various emergency scenarios

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +110 to +121
test('should return true when hardware is empty string', () => {
setupScreenlyMock({
coordinates: [37.3861, -122.0839],
hostname: 'test-host',
location: 'Test Location',
hardware: '',
screenly_version: '1.2.3',
screen_name: 'Main Screen',
tags: [],
})
expect(isAnywhereScreen()).toBe(true)
})
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

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

The isAnywhereScreen() function also checks for hardware === undefined, but there's no test case covering when hardware is undefined. Add a test case that validates the function returns true when hardware is undefined.

Copilot uses AI. Check for mistakes.
Comment on lines +216 to +221
} catch (_) {
const cached = localStorage.getItem('screenly_settings')
settings = cached
? (JSON.parse(cached) as Partial<ReturnType<typeof getSettings>>)
: {}
}
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

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

The error is caught but ignored without logging. Consider logging the error to help diagnose issues when getSettings() fails, especially in production environments where debugging is more difficult.

Copilot uses AI. Check for mistakes.
Comment on lines +226 to +231
} catch (_) {
const cachedMeta = localStorage.getItem('screenly_metadata')
metadata = cachedMeta
? (JSON.parse(cachedMeta) as Partial<ReturnType<typeof getMetadata>>)
: {}
}
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

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

The error is caught but ignored without logging. Consider logging the error to help diagnose issues when getMetadata() fails, especially in production environments where debugging is more difficult.

Copilot uses AI. Check for mistakes.
Comment on lines +30 to +33
"@photostructure/tz-lookup": "^11.3.0",
"country-locale-map": "^1.9.11",
"fast-xml-parser": "^5.3.2",
"offline-geocode-city": "^1.0.2"
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

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

The dependencies @photostructure/tz-lookup, country-locale-map, and offline-geocode-city are declared but not imported or used anywhere in the codebase. Consider removing these unused dependencies to reduce bundle size and maintenance overhead.

Suggested change
"@photostructure/tz-lookup": "^11.3.0",
"country-locale-map": "^1.9.11",
"fast-xml-parser": "^5.3.2",
"offline-geocode-city": "^1.0.2"
"fast-xml-parser": "^5.3.2"

Copilot uses AI. Check for mistakes.
Comment on lines +70 to +75
export function isAnywhereScreen(): boolean {
return (
screenly.metadata.hardware === '' ||
screenly.metadata.hardware === undefined
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@rusko124, we should make the screen type part of the metadata

Comment on lines +23 to +24
const container = document.getElementById('alerts')
if (!container) return
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this display an error when the debug mode enabled, or will it silently die?

It looks like there could be more similar issues.

Fail fast, at least in debug mode.

import { getNearestExit, splitIntoSentences, proxyUrl } from './utils'
import { highlightKeywords } from './render'

function renderAlerts(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, don't make functions too big. Refactor if it gets over 70 lines.

import { getNearestExit, splitIntoSentences, proxyUrl } from './utils'
import { highlightKeywords } from './render'

function renderAlerts(
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like the render should be done using a template language, not by using JS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would content templates be a good fit for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants