fix(pad): show saved-revision markers in in-pad history mode (#7946)

[JohnMcLear]

Fixes #7946

What was broken

Saved revisions (the Save Revision star button) stopped appearing as markers in the timeslider in 3.3.x.

Root cause

When #7659 moved the timeslider into the pad as an embedded iframe, the user-facing control became the outer #history-slider-input range input in pad.html. The saved-revision stars are still drawn by broadcast_slider.ts into the iframe's #ui-slider-bar, but that DOM is hidden in embedded-history-frame mode. pad_mode.ts bridges rev / max / value / timer / authors onto the outer slider — but never the saved revisions. So saving worked, yet no markers ever showed.

The underlying saved-revisions code path (server getSavedRevisions, clientVars.savedRevisions, broadcast_slider.addSavedRevision) is byte-for-byte unchanged from 3.2.0 — the regression is purely the missing outer-slider bridge.

The fix

• pad.html: wrap #history-slider-input in a positioned .history-slider-wrap with a #history-slider-stars overlay.
• pad.css: style .history-star markers (reusing the existing timeslider star glyph), inset to align with the native range thumb.
• pad_mode.ts: bridge the embedded timeslider's clientVars.savedRevisions onto the outer slider as percentage-positioned markers, rendered on first sync and re-rendered when the slider max changes. A signature guard keeps it cheap on every scrub; percentage positions reflow on resize for free.

Markers are a purely visual, aria-hidden overlay (keyboard / screen-reader users already reach any revision via the slider + step buttons, matching the legacy timeslider's mouse-only stars) with a click-to-seek convenience.

Before	After
no markers on the slider	⭐ marker at the saved revision

Why CI didn't catch it (and how this PR closes that gap)

• The only test that asserted a star renders (src/tests/frontend/specs/timeslider_revisions.js) lives in the legacy mocha suite, which no workflow runs — frontend-tests.yml only runs Playwright (tests/frontend-new/specs/).
• The modern timeslider specs drive the ?embed=1 iframe directly and never exercise the outer in-pad history UI where the bug lives.
• The backend API tests only assert saved-revision counts, never the rendered markers.

This PR adds tests/frontend-new/specs/timeslider_saved_revisions.spec.ts, which exercises the real user flow: write content → click Save Revision → enter in-pad history mode via the toolbar → assert a visible, correctly-positioned marker on the outer slider. Verified TDD-style: the test fails on develop (.history-star not found) and passes with the fix. All 15 existing timeslider_* specs still pass.

🤖 Generated with Claude Code

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0)

1. Stars miss live updates ✓ Resolved 🐞 Bug ≡ Correctness

Description

renderSavedRevisionStars() renders markers from innerWin.clientVars.savedRevisions, but the
timeslider’s NEW_SAVEDREV path only adds an inner DOM star and does not update clientVars, so the
outer overlay won’t reflect saved revisions created after entering history mode (e.g., by
collaborators).

Code

src/static/js/pad_mode.ts[R415-447]

+    const saved = inner.clientVars?.savedRevisions;
+    const max = Number(sliderInput.max) || 0;
+    if (!Array.isArray(saved) || saved.length === 0 || max <= 0) {
+      if (layer.childElementCount) layer.replaceChildren();
+      layer.dataset.sig = '';
+      return;
+    }
+
+    const inRange = saved
+        .map((r: any) => Number(r && r.revNum))
+        .filter((n: number) => Number.isFinite(n) && n >= 0 && n <= max);
+    const sig = `${max}:${[...inRange].sort((a, b) => a - b).join(',')}`;
+    if (layer.dataset.sig === sig) return;
+    layer.dataset.sig = sig;
+    layer.replaceChildren();
+
+    for (const rev of saved) {
+      const revNum = Number(rev && rev.revNum);
+      if (!Number.isFinite(revNum) || revNum < 0 || revNum > max) continue;
+      const frac = max === 0 ? 0 : revNum / max;
+      // A purely visual marker (the layer is aria-hidden): keyboard/screen
+      // reader users already reach any revision via the slider and step
+      // buttons, so we mirror the legacy timeslider's mouse-only stars rather
+      // than inject extra tab stops. The hover title aids mouse users; the
+      // click is a convenience to jump straight to the saved point.
+      const star = document.createElement('span');
+      star.className = 'history-star';
+      star.style.left = `${(frac * 100).toFixed(4)}%`;
+      star.title = (rev && typeof rev.label === 'string' && rev.label) || `Revision ${revNum}`;
+      star.addEventListener('click', () => {
+        try { inner.BroadcastSlider?.setSliderPosition?.(revNum); } catch (_e) { /* inner gone */ }
+      });
+      layer.appendChild(star);

Evidence

pad_mode.ts reads saved revisions only from iframe clientVars; the timeslider runtime update path
for saved revisions does not update clientVars, so the outer overlay’s data source becomes stale
while history mode is open.

src/static/js/pad_mode.ts[402-448]
src/static/js/broadcast.ts[507-518]
src/static/js/broadcast_slider.ts[347-363]

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 outer in-pad history slider markers are rendered from `innerWin.clientVars.savedRevisions`, but that array is never updated when the iframe receives `NEW_SAVEDREV`. As a result, new saved revisions created while the user is already in history mode (including from other collaborators) will not appear as markers on the outer slider.
## Issue Context
The iframe timeslider receives `NEW_SAVEDREV` and calls `BroadcastSlider.addSavedRevision(...)`, which updates the inner hidden slider DOM but does not mutate `clientVars.savedRevisions`. `pad_mode.ts` relies on `clientVars.savedRevisions` as its source of truth.
## Fix Focus Areas
- src/static/js/pad_mode.ts[402-449]
- src/static/js/broadcast.ts[515-518]
- src/static/js/broadcast_slider.ts[227-242]
### Suggested implementation direction
- Ensure the iframe’s `clientVars.savedRevisions` stays updated when `NEW_SAVEDREV` arrives (e.g., in `broadcast.ts` add the saved revision object to `window.clientVars.savedRevisions` with dedupe/sort).
- Trigger a re-render of the outer stars when a saved revision is added (e.g., in `pad_mode.ts` wrap/monkey-patch `inner.BroadcastSlider.addSavedRevision` once to: (a) update `inner.clientVars.savedRevisions` and (b) call `renderSavedRevisionStars(innerWin)`).

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

---

2. Rev0 marker never renders ✓ Resolved 🐞 Bug ≡ Correctness

Description

renderSavedRevisionStars() clears the overlay when max <= 0, which prevents rendering a valid
saved revision at revNum === 0 when the document has only one revision (max === 0).

Code

src/static/js/pad_mode.ts[R415-420]

+    const saved = inner.clientVars?.savedRevisions;
+    const max = Number(sliderInput.max) || 0;
+    if (!Array.isArray(saved) || saved.length === 0 || max <= 0) {
+      if (layer.childElementCount) layer.replaceChildren();
+      layer.dataset.sig = '';
+      return;

Evidence

The early return uses max <= 0, and the backend allows saving a revision with revNum "0";
therefore a rev0 saved revision can exist but will be filtered out by the new rendering logic.

src/static/js/pad_mode.ts[415-421]
src/node/db/Pad.ts[871-885]

Agent prompt

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

## Issue description
`renderSavedRevisionStars()` returns early when `max <= 0`, which blocks rendering a saved revision marker at revision 0 when the slider range is `[0..0]`.
## Issue Context
Saved revisions can legitimately be at revision 0. When the pad’s current head revision is 0, the slider max is 0.
## Fix Focus Areas
- src/static/js/pad_mode.ts[415-434]
### Suggested implementation direction
- Change the guard to allow `max === 0` (only treat `max < 0` as invalid).
- Keep the existing `frac` handling (`max === 0 ? 0 : revNum / max`) so rev 0 renders at `left: 0%`.

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

---

3. Test allows left=0 ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new Playwright test’s position assertion uses `parseFloat(style.left) ||
getBoundingClientRect().left, so a marker with style.left of 0%` falls back to a positive page X
coordinate and the test passes even if the marker is collapsed to the origin.

Code

src/tests/frontend-new/specs/timeslider_saved_revisions.spec.ts[R52-56]

+    // The marker must be positioned within the slider track, not collapsed to
+    // the origin — a degenerate render at left:0 would still be "visible".
+    const left = await marker.first().evaluate(
+        (el) => parseFloat((el as HTMLElement).style.left) || el.getBoundingClientRect().left);
+    expect(left).toBeGreaterThan(0);

Evidence

Because parseFloat('0%') returns 0 (falsy), the code evaluates the fallback bounding rect X,
defeating the intended ‘not at origin’ check.

src/tests/frontend-new/specs/timeslider_saved_revisions.spec.ts[47-56]

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 test is intended to fail if a marker is rendered at `left: 0%`, but the current code treats `0` as falsy and falls back to `getBoundingClientRect().left`, which is almost always > 0.
## Issue Context
This weakens the regression protection: a degenerate render can still pass.
## Fix Focus Areas
- src/tests/frontend-new/specs/timeslider_saved_revisions.spec.ts[52-56]
### Suggested implementation direction
- Explicitly assert on the parsed percentage value and avoid `||` fallback on `0`.
- Example: `const leftPct = await marker.first().evaluate(el => parseFloat((el as HTMLElement).style.left)); expect(leftPct).toBeGreaterThan(0); expect(leftPct).toBeLessThan(100);`
- Or compare marker X against the slider track’s bounding box (relative check).

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

---

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix saved-revision star markers in in-pad history timeslider
🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Walkthroughs

Description

• Restore saved-revision star markers on the in-pad history slider (regression in 3.3.x).
• Bridge iframe savedRevisions to an outer-slider overlay with click-to-seek markers.
• Add Playwright regression test covering save revision → history mode → marker render.

Diagram

graph TD
  user["User"] --> outer["Pad history controls"] --> bridge["pad_mode.ts bridge"] --> stars["#history-slider-stars overlay"]
  iframe["Embedded timeslider iframe"] --> vars["clientVars.savedRevisions"] --> bridge
  outer --> bs["BroadcastSlider API"] --> iframe
  stars --> bs

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Unhide/render iframe star bar in embed mode

• ➕ Avoids duplicating marker rendering logic in outer UI
• ➕ Keeps all timeslider visuals in one place (iframe)
• ➖ Requires changing embed-mode layout/visibility rules; higher risk of UI regressions
• ➖ Outer slider is still the user-facing control, so alignment/click affordance remains awkward

2. Bridge saved revisions via postMessage instead of direct iframe access

• ➕ Looser coupling between outer pad and iframe implementation details
• ➕ More robust if iframe internals change
• ➖ More plumbing (message protocol, lifecycle handling) for a small UI fix
• ➖ pad_mode.ts already directly bridges other inner state, so inconsistency

Recommendation: Current approach (outer overlay + reading clientVars.savedRevisions + seeking via BroadcastSlider) is the lowest-risk, most consistent fix given existing direct iframe-bridge patterns in pad_mode.ts. The added signature guard limits re-render cost during scrubbing, and the Playwright test closes the previous coverage gap.

File Changes

Bug fix (3)

pad.css Add overlay styling for saved-revision star markers on history slider +42/-2

Add overlay styling for saved-revision star markers on history slider

• Introduces '.history-slider-wrap' and '.history-slider-stars' overlay styling, including pointer-events behavior to keep slider dragging unobstructed. Adds '.history-star' glyph styling aligned to the range thumb and updates responsive rules to target the wrapper instead of the input.

src/static/css/pad.css

---

pad_mode.ts Render and sync saved-revision markers onto the outer in-pad history slider +58/-0

Render and sync saved-revision markers onto the outer in-pad history slider

• Clears any existing star markers when exiting history mode. Adds 'renderSavedRevisionStars()' to read 'clientVars.savedRevisions' from the embedded timeslider iframe and render percentage-positioned, clickable markers onto the outer slider overlay, with a signature guard to avoid redundant work during frequent sync calls.

src/static/js/pad_mode.ts

---

pad.html Wrap history slider and add stars overlay container +9/-2

Wrap history slider and add stars overlay container

• Wraps '#history-slider-input' in a positioned '.history-slider-wrap' and adds an 'aria-hidden' '#history-slider-stars' element used as the marker overlay layer in history mode.

src/templates/pad.html

---

Tests (1)

timeslider_saved_revisions.spec.ts Add Playwright regression test for saved-revision markers in in-pad history mode +58/-0

Add Playwright regression test for saved-revision markers in in-pad history mode

• Adds an end-to-end spec that creates multiple revisions, saves a revision, enters in-pad history mode, and asserts that at least one '.history-star' marker is visible and positioned away from the origin. This covers the previously untested outer in-pad history UI path.

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

---

[JohnMcLear]

Addressed all three findings in 212c8c9:

1. Stars miss live updates — Verified, and the root cause was deeper: handleSaveRevisionMessage never broadcast NEW_SAVEDREV, so the client's NEW_SAVEDREV handler was dead code and no timeslider (inner or outer) ever updated saved-rev markers live. Fixed properly:

• Pad.addSavedRevision now returns the new revision (undefined on duplicate).
• handleSaveRevisionMessage broadcasts NEW_SAVEDREV to the pad room. Live editors fall through their if/else chain and ignore the unknown type.
• pad_mode.ts now sources the outer markers from the embedded slider's live #ui-slider-bar .star DOM (pulling labels from the clientVars snapshot) and adds a MutationObserver on that bar, so a revision saved by a collaborator appears on an already-open history slider without a reload.

2. Rev0 marker never renders — Verified. The max <= 0 guard conflated "nothing to show" with a single-revision pad. Now bails only on an empty set / max < 0; max === 0 renders the rev-0 marker at the left edge (frac already handled it).

3. Test allows left=0 — Verified. The || getBoundingClientRect().left fallback let a collapsed left:0% pass. The assertion now parses style.left directly and checks 0 < pct < 100.

Added a two-client Playwright test for the live path (confirmed it fails when the server broadcast is removed). Backend Pad + pad API specs (88) and all 15 timeslider_* specs pass.