Feat: Frame events — PopupText popups and Banner notifications#644
Feat: Frame events — PopupText popups and Banner notifications#644rgregg wants to merge 29 commits into
Conversation
…TimeoutMs settings Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tifications Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Without this, FrameEventMode serializes as 0/1 instead of "PopupText"/"Close", causing the frontend string comparison to fail.
The global converter was breaking asset type serialization (IMAGE as string instead of int). Use [JsonConverter] on FrameEventMode and FrameEventAckStatus instead.
The auto-generated API client type was missing the event properties, so eventHostEnabled was never reaching the frontend config store.
Remove eventHostEnabled gate from startEventPolling — the poll loop already checks it each iteration. This avoids timing issues where the config store may not be populated when the function is first called. Also remove dead $effect block and debug console.warn calls.
- Add seconds-remaining countdown indicator to the popup dismiss area - Convert event components to Svelte 5 $props() syntax - Fix weather temperature showing degree symbol twice by using weather.unit from the API instead of hardcoded degree symbol
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughImplements a complete frame event notification system with backend queue, REST API, and frontend Svelte components supporting PopupText and Banner modes, mode-scoped polling, acknowledgment tracking, configuration settings, and independent lifecycle management for each notification type. ChangesFrame Event Notification System
Sequence Diagram(s)sequenceDiagram
participant Client as Client Device
participant EventsAPI as /api/events
participant Queue as Queue (per-device)
participant Service as Event Service
participant Store as Stores<br/>(popup/banner)
participant HomePage as home-page.svelte
participant Overlays as Overlays
Client->>EventsAPI: POST /events (FrameEventRequestDto)
EventsAPI->>Queue: EnqueueAsync(event)
activate Queue
Queue->>Queue: Deduplicate by ID<br/>Track category<br/>Order by priority
deactivate Queue
loop Every polling interval
Service->>EventsAPI: GET /next?deviceId=X&mode=PopupText
Service->>EventsAPI: GET /next?deviceId=X&mode=Banner (concurrent)
EventsAPI->>Queue: PeekNextAsync(deviceId, mode)
Queue-->>EventsAPI: FrameEvent | null
EventsAPI-->>Service: FrameEventResponseDto | 204
end
Service->>Store: activePopupEvent = event
activate Store
Store-->>HomePage: subscribe
deactivate Store
HomePage->>HomePage: On new popup:<br/>Pause slideshow<br/>Ack Shown once<br/>Schedule auto-dismiss
HomePage->>Overlays: Mount PopupTextOverlay/<br/>BannerOverlay
activate Overlays
Overlays->>Overlays: Show countdown<br/>Allow touch/key dismiss
deactivate Overlays
Overlays->>HomePage: onDismiss(Closed)
HomePage->>EventsAPI: POST /{eventId}/ack?deviceId=X (status)
EventsAPI->>Queue: AckAsync(eventId, Closed)
Queue->>Queue: Remove event<br/>Clear active slot
Service->>Store: activePopupEvent = null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
docs/docs/getting-started/configuration.md (1)
195-207: ⚡ Quick winAdd an auth note to the curl example.
On Line 195, the example is easy to copy as-is, but setups with
AuthenticationSecretenabled will fail withoutAuthorization: Bearer .... Please add a one-line note (or alternate curl snippet) right under this example to prevent false-negative setup checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/docs/getting-started/configuration.md` around lines 195 - 207, The curl example that posts to /api/events fails when AuthenticationSecret is enabled because it lacks an Authorization header; update the documentation right under the shown curl snippet (the POST to /api/events) to either add a one-line note telling users to include Authorization: Bearer <token> when AuthenticationSecret is enabled or provide an alternate curl snippet that includes the header (Authorization: Bearer <your-auth-token>) so authenticated setups won't get false-negative errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-05-18-banner-notifications.md`:
- Around line 1177-1193: The plan doc currently embeds sensitive host/user
specifics in the shell snippets (e.g., the command string "ssh hass-hub-01.lan",
the systemd/user service "immich-frame", and the path "/home/ryan/.Xauthority"
and script "frame.sh"); replace those concrete values with neutral placeholders
(e.g., <HOSTNAME>, <USER>, <USER_HOME> or variables like $HOST, $USER_HOME) and
update the example commands to use those placeholders so the content no longer
contains internal hostnames or personal home paths while preserving the
instruction semantics.
In `@docs/superpowers/specs/2026-05-18-banner-notifications-design.md`:
- Line 8: The summary sentence contains a typo: change "renderered" to
"rendered" in the document describing the new Banner notification mode for
ImmichFrame; update the sentence that references Banner, PopupText,
FrameEventMode, and the new overlay component so it reads "...and rendered by a
new overlay component." ensuring the rest of the text and identifiers
(ImmichFrame, frame-event, FrameEventMode, PopupText) remain unchanged.
In `@ImmichFrame.Core/Events/FrameEventAckStatus.cs`:
- Around line 8-11: The enum FrameEventAckStatus is missing the Dismissed member
required by the event contract; update the enum declaration in
FrameEventAckStatus to include Dismissed (e.g., add Dismissed alongside Shown,
Closed, Timeout, Error) so the ack lifecycle can represent dismiss
acknowledgements correctly and avoid remapping/rejection of dismiss events.
In `@ImmichFrame.Core/Services/InMemoryFrameEventQueue.cs`:
- Around line 59-60: The category-based replacement currently ignores
FrameEventMode, causing an event of one mode (e.g., PopupText) to evict another
(e.g., Banner) when categories match; update InMemoryFrameEventQueue to scope
category lookups and replacements by mode—either by including the mode in the
category key (e.g., combine FrameEventMode with category) or by maintaining a
per-mode category dictionary, and then use that scoped mapping wherever
_byCategory is accessed/modified (references: _byCategory, _activeEventIdByMode,
and the add/remove logic around lines handling category replacement and the
blocks noted at 78-82 and 172-173) so replacements only affect events of the
same FrameEventMode.
In `@immichFrame.Web/src/lib/components/elements/clock.svelte`:
- Line 93: The temperature expression can produce "undefined°" when
weather.temperature is absent; update the template so the temperature and unit
are only rendered when temperature is present. Replace the inline expression in
the div with a conditional/guard that checks weather.temperature (e.g., in the
weather-temperature div or a small helper like a computed/derived value) and
render either the formatted toFixed(1) plus unit (weather.unit ?? '°') when
non-null, or an empty string/placeholder when missing to avoid showing
"undefined".
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 192-207: The early return in handlePopupEvent when
!$configStore.eventHostEnabled skips necessary cleanup (timers/state) so ensure
you run the same cleanup sequence before returning: call clearPopupTimer(), set
currentPopup = null, popupShownAcked = false, lastPopupId = null, and if
popupPausedSlideshow and progressBar then resume via progressBar.play() and set
popupPausedSlideshow = false; you can either move that cleanup block above the
if (!($configStore.eventHostEnabled ?? false)) check or extract it to a helper
(e.g., cleanupPopupState) and invoke it both before the early return and in the
existing null-event branch to avoid duplicated logic.
In `@immichFrame.Web/src/lib/events/event-service.ts`:
- Around line 125-143: The delay() function for polling attaches an 'abort'
listener that only auto-removes if the signal aborts, causing listener
accumulation; fix it in the delay(durationMs: number, signal: AbortSignal)
implementation by extracting the abort handler into a const (e.g., const onAbort
= () => { clearTimeout(timeout); resolve(); }) and registering it with
signal.addEventListener('abort', onAbort, { once: true }), then in the timeout
callback (the normal resolution path) call signal.removeEventListener('abort',
onAbort) after clearTimeout/resolve so the handler is removed when the timer
completes; ensure the same handler reference is used for both addEventListener
and removeEventListener to prevent leaks.
- Around line 93-103: The poll and ack fetch handlers currently only handle
200/204 and silently ignore 4xx/5xx responses; update the fetch result handling
for the /api/events/next call (the fetch with url
`/api/events/next?deviceId=${encodeURIComponent(deviceId)}&mode=${mode}`) and
the corresponding ack fetch so that non-2xx responses are handled explicitly:
detect response.ok === false, call the appropriate logger (or console.error)
with response.status and response.statusText and any returned error body, and
either set the store to an error/state indicating failure or throw an Error to
propagate the failure so callers can react instead of leaving stale UI state.
Ensure the same explicit handling is added to the ack flow so failed
acknowledgements are logged and surfaced.
In `@ImmichFrame.WebApi/Controllers/EventsController.cs`:
- Around line 69-77: AckEvent currently dereferences request.Status without
checking that the request body exists; add a null check for the
FrameEventAckRequestDto request at the start of AckEvent and return BadRequest
(e.g., new { message = "request body is required" }) if request is null before
calling _queue.AckAsync(deviceId, eventId, request.Status, cancellationToken),
ensuring you still validate deviceId and _settings.EventHostEnabled as before.
In `@ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs`:
- Around line 59-61: The V1 settings are forwarded without validation; add
bounds checks when mapping/using ServerSettingsV1.EventPollingIntervalSeconds
and ServerSettingsV1.EventDefaultTimeoutMs (and honor EventHostEnabled) in the
V1 adapter path or mapping function so bad legacy values can't create tight
loops or immediate timeouts; enforce reasonable ranges (e.g.
EventPollingIntervalSeconds >= 1 and <= 3600, EventDefaultTimeoutMs >= 1000 and
<= 300000), clamp out-of-range values to defaults or the nearest bound, and emit
a warning log mentioning ServerSettingsV1.EventPollingIntervalSeconds /
ServerSettingsV1.EventDefaultTimeoutMs when adjustments are made.
In `@ImmichFrame.WebApi/Models/Events/FrameEventAckRequestDto.cs`:
- Around line 8-9: The Status property on FrameEventAckRequestDto is currently a
non-nullable enum which prevents [Required] from detecting missing JSON; change
the type of Status to a nullable enum (FrameEventAckStatus?) so the model binder
can set null for omitted fields and [Required] will validate correctly, then
review any usages of FrameEventAckRequestDto.Status (e.g., code reading Status)
and handle the null case or validate before use.
In `@ImmichFrame.WebApi/Models/ServerSettings.cs`:
- Around line 75-77: ServerSettings currently exposes
EventPollingIntervalSeconds and EventDefaultTimeoutMs without bounds checks,
allowing <=0 values that break event hosting; add validation to enforce sensible
minima (e.g., EventPollingIntervalSeconds >= 1 and EventDefaultTimeoutMs >= 1 or
a practical lower bound like 100), either by applying DataAnnotations (e.g.,
Range) to the properties or by enforcing and normalizing values in the
ServerSettings constructor or property setters, and surface validation errors
(throw ArgumentOutOfRangeException or similar) so invalid configuration cannot
be used when EventHostEnabled is true.
In `@ImmichFrame.WebApi/Services/FrameEventValidator.cs`:
- Around line 19-22: The Validate method in FrameEventValidator currently
dereferences dto without a null check; add an explicit null guard at the start
of FrameEventValidator.Validate(FrameEventRequestDto dto) that throws a
ValidationException (e.g., "dto is required" or "request body is required") when
dto is null, before accessing dto.DeviceId (and any other fields), so callers
receive a ValidationException instead of a NullReferenceException.
---
Nitpick comments:
In `@docs/docs/getting-started/configuration.md`:
- Around line 195-207: The curl example that posts to /api/events fails when
AuthenticationSecret is enabled because it lacks an Authorization header; update
the documentation right under the shown curl snippet (the POST to /api/events)
to either add a one-line note telling users to include Authorization: Bearer
<token> when AuthenticationSecret is enabled or provide an alternate curl
snippet that includes the header (Authorization: Bearer <your-auth-token>) so
authenticated setups won't get false-negative errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4251090a-f90b-429b-b3f7-6b49a74d3a99
📒 Files selected for processing (36)
ImmichFrame.Core.Tests/Events/InMemoryFrameEventQueueTests.csImmichFrame.Core/Events/FrameEvent.csImmichFrame.Core/Events/FrameEventAckStatus.csImmichFrame.Core/Events/FrameEventAction.csImmichFrame.Core/Events/FrameEventInput.csImmichFrame.Core/Events/FrameEventMode.csImmichFrame.Core/Interfaces/IFrameEventQueue.csImmichFrame.Core/Interfaces/IServerSettings.csImmichFrame.Core/Services/InMemoryFrameEventQueue.csImmichFrame.WebApi.Tests/Events/FrameEventValidatorTests.csImmichFrame.WebApi.Tests/Resources/TestV1.jsonImmichFrame.WebApi.Tests/Resources/TestV2.jsonImmichFrame.WebApi.Tests/Resources/TestV2.ymlImmichFrame.WebApi/Controllers/EventsController.csImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.csImmichFrame.WebApi/Models/ClientSettingsDto.csImmichFrame.WebApi/Models/Events/FrameEventAckRequestDto.csImmichFrame.WebApi/Models/Events/FrameEventDiagnosticsDto.csImmichFrame.WebApi/Models/Events/FrameEventRequestDto.csImmichFrame.WebApi/Models/Events/FrameEventResponseDto.csImmichFrame.WebApi/Models/ServerSettings.csImmichFrame.WebApi/Program.csImmichFrame.WebApi/Services/FrameEventValidator.csdocker/Settings.example.jsondocker/Settings.example.ymldocs/docs/getting-started/configuration.mddocs/superpowers/plans/2026-05-18-banner-notifications.mddocs/superpowers/specs/2026-05-18-banner-notifications-design.mdimmichFrame.Web/src/lib/components/elements/clock.svelteimmichFrame.Web/src/lib/components/events/BannerOverlay.svelteimmichFrame.Web/src/lib/components/events/EventOverlayHost.svelteimmichFrame.Web/src/lib/components/events/PopupTextOverlay.svelteimmichFrame.Web/src/lib/components/home-page/home-page.svelteimmichFrame.Web/src/lib/events/event-service.tsimmichFrame.Web/src/lib/immichFrameApi.tsimmichFrame.Web/src/lib/stores/config.store.ts
|
|
||
| <div class="weather-location">{weather.location},</div> | ||
| <div class="weather-temperature">{weather.temperature?.toFixed(1)}°</div> | ||
| <div class="weather-temperature">{weather.temperature?.toFixed(1)}{weather.unit ?? '°'}</div> |
There was a problem hiding this comment.
Avoid rendering undefined when temperature is missing.
weather.temperature?.toFixed(1) can render as undefined, producing undefined° (or undefined{unit}) in the UI. Add a fallback for missing temperature.
Proposed fix
- <div class="weather-temperature">{weather.temperature?.toFixed(1)}{weather.unit ?? '°'}</div>
+ <div class="weather-temperature">
+ {weather.temperature == null ? '—' : `${weather.temperature.toFixed(1)}${weather.unit ?? '°'}`}
+ </div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="weather-temperature">{weather.temperature?.toFixed(1)}{weather.unit ?? '°'}</div> | |
| <div class="weather-temperature"> | |
| {weather.temperature == null ? '—' : `${weather.temperature.toFixed(1)}${weather.unit ?? '°'}`} | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@immichFrame.Web/src/lib/components/elements/clock.svelte` at line 93, The
temperature expression can produce "undefined°" when weather.temperature is
absent; update the template so the temperature and unit are only rendered when
temperature is present. Replace the inline expression in the div with a
conditional/guard that checks weather.temperature (e.g., in the
weather-temperature div or a small helper like a computed/derived value) and
render either the formatted toFixed(1) plus unit (weather.unit ?? '°') when
non-null, or an empty string/placeholder when missing to avoid showing
"undefined".
| public bool EventHostEnabled { get; set; } = false; | ||
| public int EventPollingIntervalSeconds { get; set; } = 2; | ||
| public int EventDefaultTimeoutMs { get; set; } = 15000; |
There was a problem hiding this comment.
Validate event timing values in the V1 adapter path as well.
These new values are forwarded directly through the adapter without bounds checks; invalid values can still trigger tight polling loops or immediate timeout behavior in legacy-config deployments (Line 145 currently no-op).
Proposed fix
class GeneralSettingsV1Adapter(ServerSettingsV1 _delegate) : IGeneralSettings
{
@@
- public void Validate() { }
+ public void Validate()
+ {
+ if (_delegate.EventPollingIntervalSeconds <= 0)
+ {
+ throw new InvalidOperationException("EventPollingIntervalSeconds must be greater than 0.");
+ }
+
+ if (_delegate.EventDefaultTimeoutMs <= 0)
+ {
+ throw new InvalidOperationException("EventDefaultTimeoutMs must be greater than 0.");
+ }
+ }
}Also applies to: 141-143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs` around lines 59 - 61,
The V1 settings are forwarded without validation; add bounds checks when
mapping/using ServerSettingsV1.EventPollingIntervalSeconds and
ServerSettingsV1.EventDefaultTimeoutMs (and honor EventHostEnabled) in the V1
adapter path or mapping function so bad legacy values can't create tight loops
or immediate timeouts; enforce reasonable ranges (e.g.
EventPollingIntervalSeconds >= 1 and <= 3600, EventDefaultTimeoutMs >= 1000 and
<= 300000), clamp out-of-range values to defaults or the nearest bound, and emit
a warning log mentioning ServerSettingsV1.EventPollingIntervalSeconds /
ServerSettingsV1.EventDefaultTimeoutMs when adjustments are made.
| public bool EventHostEnabled { get; set; } = false; | ||
| public int EventPollingIntervalSeconds { get; set; } = 2; | ||
| public int EventDefaultTimeoutMs { get; set; } = 15000; |
There was a problem hiding this comment.
Add bounds validation for event timing settings.
EventPollingIntervalSeconds and EventDefaultTimeoutMs are unvalidated; invalid values (e.g., <= 0) can cause tight poll loops or immediate timeout behavior when event hosting is enabled (Line 79 is currently a no-op).
Proposed fix
public class GeneralSettings : IGeneralSettings, IConfigSettable
{
@@
- public void Validate() { }
+ public void Validate()
+ {
+ if (EventPollingIntervalSeconds <= 0)
+ {
+ throw new InvalidOperationException("General.EventPollingIntervalSeconds must be greater than 0.");
+ }
+
+ if (EventDefaultTimeoutMs <= 0)
+ {
+ throw new InvalidOperationException("General.EventDefaultTimeoutMs must be greater than 0.");
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImmichFrame.WebApi/Models/ServerSettings.cs` around lines 75 - 77,
ServerSettings currently exposes EventPollingIntervalSeconds and
EventDefaultTimeoutMs without bounds checks, allowing <=0 values that break
event hosting; add validation to enforce sensible minima (e.g.,
EventPollingIntervalSeconds >= 1 and EventDefaultTimeoutMs >= 1 or a practical
lower bound like 100), either by applying DataAnnotations (e.g., Range) to the
properties or by enforcing and normalizing values in the ServerSettings
constructor or property setters, and surface validation errors (throw
ArgumentOutOfRangeException or similar) so invalid configuration cannot be used
when EventHostEnabled is true.
- Add Dismissed to FrameEventAckStatus - Scope queue category replacement by mode so Banner/PopupText do not evict each other - Make FrameEventAckRequestDto.Status nullable so [Required] validates missing JSON - Null-guard FrameEventValidator.Validate and EventsController.AckEvent - Strip CR/LF from user-supplied log values
The 'show unit from API' change was bundled into commit fcf9c15 along with unrelated popup countdown work. Split out to its own branch (rgregg-weather-unit-fix) so this PR stays focused on the notification system.
- delay() removes its abort listener on normal timeout path (no leak under long uptime) - pollOne / acknowledgeEvent log non-2xx responses - handlePopupEvent / handleBannerEvent run full cleanup before eventHostEnabled early-return so a mid-flight disable does not leave the slideshow paused
- Replace embedded hostnames / home paths in plan with placeholders - Fix 'renderered' typo in spec - Add Authorization-header note under the curl example
Summary
Adds a client-side event notification system to ImmichFrame, surfaced through a new
POST /api/eventsendpoint. Two display modes:categoryreplace the older one.Both modes can be active simultaneously (popup + banner coexist).
New settings
New API
POST /api/events— enqueue an event (validated;typemust start withframe.ui.)GET /api/events/next?deviceId=…&mode=…— kiosk poll endpoint;modefilter (PopupText|Banner) is optionalPOST /api/events/{eventId}/ack?deviceId=…— kiosk reportsShown/Closed/Timeout/Dismissed/ErrorGET /api/events/pending?deviceId=…— diagnosticsExample banner POST:
Implementation notes
InMemoryFrameEventQueue) supports priority ordering, category-based replacement, expiry, and per-mode active tracking so a popup and a banner don't shadow each other inPeekNext.activePopupEventandactiveBannerEvent; the home page polls both modes every interval and rendersPopupTextOverlay+BannerOverlayfromEventOverlayHost.activePopupEventonly — banners never interrupt the slideshow.EventHostEnabledis false (default), no new behavior surfaces.Tests
Design spec and implementation plan committed under
docs/superpowers/specs/anddocs/superpowers/plans/if useful for review context.Test plan
dotnet test— 107/107 passcd immichFrame.Web && npm run check— zero errorsnpm run build— succeedsEventHostEnabled: true, POST a Banner; verify it appears at top of frame and slideshow keeps advancingEventHostEnabled: false(default), POSTs return 404 — no behavior change for users who don't opt inSummary by CodeRabbit
New Features
/api/eventsto enqueue events, GET/api/events/nextto poll pending events, POST/api/events/{eventId}/ackto acknowledge events, GET/api/events/pendingfor diagnostics.EventHostEnabled,EventPollingIntervalSeconds,EventDefaultTimeoutMs.Tests
Documentation