Skip to content

feat: add timeslider playback speed setting#7541

Open
JohnMcLear wants to merge 1 commit intoether:developfrom
JohnMcLear:feat-timeslider-playback-speed
Open

feat: add timeslider playback speed setting#7541
JohnMcLear wants to merge 1 commit intoether:developfrom
JohnMcLear:feat-timeslider-playback-speed

Conversation

@JohnMcLear
Copy link
Copy Markdown
Member

@JohnMcLear JohnMcLear commented Apr 18, 2026

This is pretty awesome 👯‍♀️

Summary

  • add a core timeslider playback speed selector with Original speed as the default
  • add a Realtime mode that waits for loaded revision deltas instead of falling back to the old fixed speed
  • add frontend coverage for default behavior, persistence, and realtime scheduling

Testing

  • pnpm --filter ep_etherpad-lite run ts-check
  • NODE_ENV=production npx playwright test tests/frontend-new/specs/timeslider_playback_speed.spec.ts --project=chromium

Refs #5012

Add a core timeslider playback speed setting with an original-speed default, a realtime mode that uses revision timestamps, and frontend coverage for the new behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Add timeslider playback speed setting with realtime mode

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add playback speed selector to timeslider with multiple speed options
• Implement realtime mode using revision timestamps for accurate playback
• Persist selected playback speed in browser cookies across sessions
• Add comprehensive frontend tests for default behavior, persistence, and realtime scheduling
Diagram
flowchart LR
  A["User selects playback speed"] --> B["Speed stored in cookie"]
  B --> C["BroadcastSlider updated"]
  C --> D["Playback uses selected speed"]
  D --> E{Speed mode?}
  E -->|Realtime| F["Use revision timestamps"]
  E -->|Fixed| G["Use millisecond delay"]
  F --> H["Schedule next step"]
  G --> H
Loading

Grey Divider

File Changes

1. src/static/js/broadcast_slider.ts ✨ Enhancement +34/-2

Add playback speed control and realtime scheduling

• Add playbackSpeed state variable initialized to '100'
• Implement getPlaybackDelay() function to calculate delay based on speed mode
• Implement scheduleNextPlaybackStep() for realtime playback with fallback polling
• Update playButtonUpdater() to use dynamic delay instead of fixed 100ms
• Update playpause() to handle realtime scheduling
• Export setPlaybackSpeed() and getPlaybackSpeed() methods

src/static/js/broadcast_slider.ts


2. src/static/js/timeslider.ts ✨ Enhancement +11/-0

Add playback speed persistence and initialization

• Add playbackSpeedCookie constant for cookie key management
• Initialize cp (cookie prefix) from clientVars
• Load saved playback speed from cookie on initialization
• Bind playback speed selector change event to update cookie and BroadcastSlider
• Set initial playback speed value from cookie or default to '100'

src/static/js/timeslider.ts


3. src/tests/frontend-new/specs/timeslider_playback_speed.spec.ts 🧪 Tests +108/-0

Add comprehensive timeslider playback speed tests

• Add test for default playback speed ('100') with no cookies
• Add test for playback speed persistence across page reloads
• Add test for realtime mode using mocked revision timestamps
• Verify select element displays correct options and values
• Verify cookie-based persistence works correctly
• Verify realtime mode schedules delays based on revision data

src/tests/frontend-new/specs/timeslider_playback_speed.spec.ts


View more (2)
4. src/templates/timeslider.html ✨ Enhancement +10/-0

Add playback speed selector UI to timeslider

• Add playback speed selector dropdown to timeslider settings panel
• Include five speed options: Original speed, Realtime, 200ms, 500ms, 1000ms
• Add localization attributes for all options
• Position playback speed control after font type and follow contents options

src/templates/timeslider.html


5. src/locales/en.json 📝 Documentation +6/-0

Add playback speed localization strings

• Add label for playback speed setting
• Add localization strings for all five playback speed options
• Include descriptive text for Original speed, Realtime, and millisecond-based speeds

src/locales/en.json


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects bot commented Apr 18, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (2) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Playback speed lacks feature flag 📘 Rule violation ⛨ Security
Description
The new timeslider playback speed UI and behavior are always available with no feature-flag gating,
which violates the requirement that new features be disabled by default behind a flag. This can
introduce unreviewed behavior changes into deployments that expect the pre-change timeslider
UI/behavior only.
Code

src/templates/timeslider.html[R241-250]

+      <p>
+        <label for="playbackspeed" data-l10n-id="timeslider.settings.playbackSpeed">Playback speed:</label>
+        <select id="playbackspeed">
+          <option value="100" data-l10n-id="timeslider.settings.playbackSpeed.original">Original speed</option>
+          <option value="realtime" data-l10n-id="timeslider.settings.playbackSpeed.realtime">Realtime</option>
+          <option value="200" data-l10n-id="timeslider.settings.playbackSpeed.200ms">200 ms</option>
+          <option value="500" data-l10n-id="timeslider.settings.playbackSpeed.500ms">500 ms</option>
+          <option value="1000" data-l10n-id="timeslider.settings.playbackSpeed.1000ms">1000 ms</option>
+        </select>
+      </p>
Evidence
PR Compliance ID 9 requires new features to be behind a feature flag and disabled by default. The PR
adds a new playback speed selector and wires it to persist/apply behavior, but there is no
conditional check or flag guarding the UI or logic.

src/templates/timeslider.html[241-250]
src/static/js/timeslider.ts[178-185]
src/static/js/broadcast_slider.ts[46-65]
Best Practice: Repository guidelines

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new timeslider playback speed feature (UI + behavior) is always enabled and is not protected by a feature flag, violating the requirement that new features be behind a flag and disabled by default.

## Issue Context
A compliant implementation should provide an explicit enable/disable mechanism (feature flag), default it to disabled, and preserve the pre-change behavior and UI when disabled.

## Fix Focus Areas
- src/templates/timeslider.html[241-250]
- src/static/js/timeslider.ts[178-185]
- src/static/js/broadcast_slider.ts[46-65]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Realtime scheduling lacks comments 📘 Rule violation ⚙ Maintainability
Description
The new realtime playback scheduling logic introduces non-obvious polling and delay derivation
behavior without explanatory comments. This reduces maintainability and makes it harder to safely
modify or debug the realtime mode behavior later.
Code

src/static/js/broadcast_slider.ts[R48-65]

+    const getPlaybackDelay = () => {
+      if (playbackSpeed !== 'realtime') return Number(playbackSpeed);
+      const path = window.revisionInfo.getPath(getSliderPosition(), getSliderPosition() + 1);
+      if (path.status !== 'complete' || path.times.length === 0) return null;
+      const delay = Number(path.times[0]);
+      return Number.isFinite(delay) ? Math.max(0, delay) : null;
+    };
+
+    const scheduleNextPlaybackStep = () => {
+      const delay = getPlaybackDelay();
+      if (delay == null) {
+        setTimeout(() => {
+          if (sliderPlaying) scheduleNextPlaybackStep();
+        }, 100);
+        return;
+      }
+      setTimeout(playButtonUpdater, delay);
+    };
Evidence
PR Compliance ID 5 requires comments for complex or non-obvious logic. The added
getPlaybackDelay() / scheduleNextPlaybackStep() realtime logic includes conditional polling when
revision data is incomplete and derives delays from revisionInfo.getPath().times, but does not
document intent or assumptions.

src/static/js/broadcast_slider.ts[48-65]
Best Practice: Repository guidelines

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Non-obvious realtime playback scheduling logic was added without comments explaining why polling is needed, what `path.times[0]` represents, and how delays are chosen.

## Issue Context
Future maintainers need to understand how realtime mode behaves when revision deltas are not yet loaded (polling loop) and how timestamps are mapped to playback delays.

## Fix Focus Areas
- src/static/js/broadcast_slider.ts[48-65]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Playback delay not validated 🐞 Bug ☼ Reliability
Description
timeslider.ts initializes playback speed from a cookie and passes it to BroadcastSlider without
validating it against the supported values. In broadcast_slider.ts, non-"realtime" speeds are
converted via Number() and used as the setTimeout() delay without a finite/non-negative check, so a
malformed cookie value can produce an invalid delay and result in unexpectedly tight playback
scheduling.
Code

src/static/js/broadcast_slider.ts[R48-54]

+    const getPlaybackDelay = () => {
+      if (playbackSpeed !== 'realtime') return Number(playbackSpeed);
+      const path = window.revisionInfo.getPath(getSliderPosition(), getSliderPosition() + 1);
+      if (path.status !== 'complete' || path.times.length === 0) return null;
+      const delay = Number(path.times[0]);
+      return Number.isFinite(delay) ? Math.max(0, delay) : null;
+    };
Evidence
The playback speed is sourced from a cookie and directly applied to the slider. For numeric mode,
getPlaybackDelay() returns Number(playbackSpeed) with no validation, and that value is used as the
timeout delay, unlike the realtime path which explicitly checks Number.isFinite().

src/static/js/timeslider.ts[178-185]
src/static/js/broadcast_slider.ts[46-65]
src/static/js/broadcast_slider.ts[194-208]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Playback speed is initialized from a cookie and can be an arbitrary string. For non-`realtime` speeds, `getPlaybackDelay()` returns `Number(playbackSpeed)` and that value is passed to `setTimeout()` without validating that it is finite and non-negative.

### Issue Context
Even though the UI only offers known `<option>` values, the initial cookie read can still be malformed/corrupted/hand-edited. The realtime branch already uses `Number.isFinite()` and clamps to `>= 0`, but the numeric branch does not.

### Fix Focus Areas
- src/static/js/timeslider.ts[178-185]
- src/static/js/broadcast_slider.ts[46-65]
- src/static/js/broadcast_slider.ts[194-208]
- src/static/js/broadcast_slider.ts[233-236]

### Suggested fix
- In `BroadcastSlider.setPlaybackSpeed()` (or `getPlaybackDelay()`), normalize the value:
 - allow `'realtime'`
 - otherwise parse to a number and fallback to default (e.g. `100`) if not finite
 - clamp to a safe minimum (e.g. `>= 0` or `>= 1`)
- In `timeslider.ts`, if the cookie value is not one of the supported select values, reset the select to `'100'` and store the corrected value back to the cookie (optional but prevents repeated bad init).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment on lines +241 to +250
<p>
<label for="playbackspeed" data-l10n-id="timeslider.settings.playbackSpeed">Playback speed:</label>
<select id="playbackspeed">
<option value="100" data-l10n-id="timeslider.settings.playbackSpeed.original">Original speed</option>
<option value="realtime" data-l10n-id="timeslider.settings.playbackSpeed.realtime">Realtime</option>
<option value="200" data-l10n-id="timeslider.settings.playbackSpeed.200ms">200 ms</option>
<option value="500" data-l10n-id="timeslider.settings.playbackSpeed.500ms">500 ms</option>
<option value="1000" data-l10n-id="timeslider.settings.playbackSpeed.1000ms">1000 ms</option>
</select>
</p>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Playback speed lacks feature flag 📘 Rule violation ⛨ Security

The new timeslider playback speed UI and behavior are always available with no feature-flag gating,
which violates the requirement that new features be disabled by default behind a flag. This can
introduce unreviewed behavior changes into deployments that expect the pre-change timeslider
UI/behavior only.
Agent Prompt
## Issue description
The new timeslider playback speed feature (UI + behavior) is always enabled and is not protected by a feature flag, violating the requirement that new features be behind a flag and disabled by default.

## Issue Context
A compliant implementation should provide an explicit enable/disable mechanism (feature flag), default it to disabled, and preserve the pre-change behavior and UI when disabled.

## Fix Focus Areas
- src/templates/timeslider.html[241-250]
- src/static/js/timeslider.ts[178-185]
- src/static/js/broadcast_slider.ts[46-65]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@JohnMcLear JohnMcLear requested a review from SamTV12345 April 18, 2026 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants