fix: address review findings on the slide-execution hooks - #11
Merged
Merged
Conversation
Follow-ups from review of the hook extraction, plus the scheduling doc and the template skill update. Hooks: - Move the empty-entries fallback into useMultipleEntrySlideExecution, declared per template via emptyEntriesDuration. Each template had its own copy of the timer, which is the duplication the extraction set out to remove: a new template that omits it locks the playlist. - Start cycling when entries arrive after run turned truthy. The effect keyed only on run, so a feed resolving late was skipped for that rotation. - Return the clamped entryDuration. poster, instagram-feed and slideshow derived their fade timers from the raw prop, so a duration of 0 left the hook cycling at 15s while the fade fired immediately. - Extract clampDuration into slide-utils/duration.js so both clocks agree. - Drop durationRef from useBaseSlideExecution: it is read once, synchronously, in an effect recreated on every run change, so it prevented no staleness. Templates: - image-text: mirror images/duration into refs in a layout effect instead of during render. - video: read slide/slideDone through refs, since finish() runs from a guard timer or media event; pause the element on teardown so a preview that toggles run does not keep playing audio. Tests: 116 pass, 22 files. New coverage for clampDuration, the empty-feed fallback across all five consumers, late-arriving entries, the exposed clamped duration, fade-timer alignment, image-text cycling, and video pause/slideDone. The fade-timer and video cases were confirmed red against this branch before the fix; the image-text ones pin behaviour rather than catching a bug. vitest.config.js gets the svgr plugin from vite.config.js, without which any template importing an SVG throws in jsdom and cannot be tested at all. Docs: - docs/client-scheduling.md — content selection, rotation and the slideDone contract, including the immediate go-live path when nothing is playing, the invalid-slide filter, and the stale-list bug in checkForEmptyContent. - add-slide-template skill — the duration field must be named `duration` (the Admin writes to a hardcoded id) and the Admin stores ms while showing seconds.
4 tasks
tuj
approved these changes
Sep 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review follow-ups for #5, plus the scheduling doc and the template skill update. Targets
feature/template-cleanups, notrelease/3.0.0.Hooks
useMultipleEntrySlideExecution, declared per template viaemptyEntriesDuration(existing timings kept: 1s, 5s for news-feed, 2s for slideshow). Each template had its own copy of the timer, which is the duplication the extraction removes elsewhere; a new template that omits it locks the playlist.run— the effect keyed only onrun, so a feed resolving late was skipped for that rotation.entryDuration—poster,instagram-feedandslideshowderived their fade timers from the raw prop, soduration: 0left the hook cycling at 15s while the fade fired immediately and the entry sat faded out.clampDurationextracted toslide-utils/duration.jsso both clocks agree.durationReffromuseBaseSlideExecution— read once, synchronously, in an effect recreated on everyrunchange, so it prevented no staleness the comment claimed.Templates
image-text: images/duration mirrored into refs in a layout effect instead of during render.video:slide/slideDoneread through refs, sincefinish()runs from a guard timer or media event; element paused on teardown so a preview togglingrunstops making noise.Tests
116 passing, 22 files. New:
clampDuration, empty-feed fallback across all five consumers, late-arriving entries, the exposed clamped duration, fade-timer alignment, image-text cycling, video pause andslideDonerouting.The fade-timer and video cases were confirmed red against this branch before the fix. The image-text cases pass either way — they pin behaviour, since that change is behaviour-preserving.
vitest.config.jsgains thesvgrplugin fromvite.config.js; without it animport X from "./x.svg"resolves to a data-URI string and any template importing an SVG throws in jsdom, soinstagram-feedcould not be tested at all.Docs
docs/client-scheduling.md— content selection, rotation, and theslideDonecontract. Also documents two things easy to miss: a staged list goes live immediately when nothing is playing (wraparound is the swap point only mid-rotation), and slides markedinvalidare dropped by the region.add-slide-templateskill — thedurationfield must be namedduration(the Admin's widget writes to a hardcodedid, so any other name diverges from what it displays), and the Admin stores ms while showing seconds.Not addressed
useBaseSlideExecution.js/useMultipleEntrySlideExecution.jsare camelCase while the repo uses kebab-case, including these hooks' own test files. Left alone — renaming churns every import.checkForEmptyContentreads a stale list (region.slidevsregion.slides); noted in the doc, fix belongs elsewhere.