Repository navigation
fix(player): keep Next available for the final slide's reveals - #6233
Conversation
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for the clear before/after captures and the runnable repro in the issue.
The fix makes canNext (packages/player/src/slideshow/SlideshowController.ts:75) use the same "more fragments on this slide" condition that next() already acts on, so the button now appears exactly when pressing it would do something. I checked the other positions too: resuming a final slide before its first reveal, the last reveal, earlier slides and branches all behave as expected, and canNext is only read by the nav cluster, so nothing else shifts. The test drives the real element and controller through the Next button, which is the path a presenter uses.
Small optional suggestion: the fragment check now lives in two places (canNext at line 75 and next() at line 181). Pulling it into one private getter would keep the button and the action from drifting apart again.
On CI: the two red Windows checks are from a first run whose change detection was cancelled before any tests ran; the later run on the same commit passed, as the description notes.
Verdict: APPROVE
Reasoning: canNext now matches next() exactly, the test enters through the real click path and passes in CI at fc74ac1, and the two Windows failures are a cancelled detection run superseded by a passing rerun on the same commit.
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for the clear before/after captures and the runnable repro in the issue.
The fix makes canNext (packages/player/src/slideshow/SlideshowController.ts:75) use the same "more fragments on this slide" condition that next() already acts on, so the button now appears exactly when pressing it would do something. I checked the other positions too: resuming a final slide before its first reveal, the last reveal, earlier slides and branches all behave as expected, and canNext is only read by the nav cluster, so nothing else shifts. The test drives the real element and controller through the Next button, which is the path a presenter uses.
Small optional suggestion: the fragment check now lives in two places (canNext at line 75 and next() at line 181). Pulling it into one private getter would keep the button and the action from drifting apart again.
On CI: the two red Windows checks are from a first run whose change detection was cancelled before any tests ran; the later run on the same commit passed, as the description notes.
Verdict: APPROVE
Reasoning: canNext now matches next() exactly, the test enters through the real click path and passes in CI at fc74ac1, and the two Windows failures are a cancelled detection run superseded by a passing rerun on the same commit.
What
Keep Next available on the final slideshow slide while fragment reveals remain. Today the button disappears as soon as the presenter reaches that slide, so they cannot finish its reveals using the visible controls.
Why
A final slide with fragment holds at 1, 2 and 4 seconds starts at 1 second with two reveals left.
next()can reveal them, but the navigation hides its button becausecanNextonly considers another slide or a parent branch.Related work
Fixes #6232
#1585 corrected a different
next()fragment-at-end gate. #1581/#1589 added conditional navigation; #1594/#1596 address initialization and hold behavior. Searches of open and closed issues/PRs and Discussions found no overlapping active work for this condition.How
Include remaining fragments in the existing
canNextdecision. One regression binds the real controller to the slideshow element, clicks Next through the final slide, and checks that the button disappears only after its last reveal. The slideshow guide describes that behavior.Before
Baseline
89081b919, v0.8.193. One-slide deck with fragment holds at 1, 2 and 4 seconds, using the repository's existinggsap-heavycomposition. At the first hold,currentTime=1and Next is absent. ArrowRight reaches 2 seconds, confirming an available reveal.After
Same page, composition, first hold and 1000 x 760 viewport. Next is present at 1 second. Two clicks reach 2 then 4 seconds; Next remains at 2 seconds and disappears at 4 seconds. No browser errors on either build.
Test plan
git diff --checkpass.Local limitations: Bun 1.4.1 was available rather than the requested 1.4.4. The tests ran on Node 26 with
NODE_OPTIONS=--no-experimental-webstorageso Node's global localStorage does not mask happy-dom's implementation. Existing presenter tests log refused localhost:3000 requests but all assertions pass. No Safari/mobile run. Mintlify CLI is absent, so local Mintlify checks were unavailable. T3 screenshot capture timed out twice; captures use the repository's Chromium launch workflow.Windows CI note: change detection exceeded its four-minute job limit, which cancelled that prerequisite and failed the dependent render/test gates before they could run. This validation-note update requests a fresh workflow run on the same commit; no source change was needed for the timeout.