fix: open linked documents at the requested history entry (CLUE-613)

[scytacki]

CLUE-613

Why

A link carrying both studentDocument and studentDocumentHistoryId — every answer cell in the Researcher Reports CLUE Student Answers report emits one — is supposed to open a student document at the moment that answer was logged. Three things stopped it from doing that:

1. The seek landed one entry early. findHistoryEntryIndex returns an array index; goToHistoryEntry took a position, which counts applied entries. Passing one straight to the other showed the document as it stood before the entry the link named.
2. The scrubber did not follow. The thumb stayed pinned at the end of the history, so a researcher read one moment under a control reporting another, and two different links looked identical.
3. Failure was silent. moveToHistoryEntryAfterLoad gave up in three places, each only reaching console.warn, leaving the document at the end of its history looking entirely normal.

Fixing 2 meant reaching into PlaybackControlComponent, 439 lines of interleaved state, memos and effects. Rather than add a fourth way to move the thumb, the state moved into a PlaybackControlModel, and several further defects surfaced once the rules were written in one place. Those are listed below; each is small on its own, but together they are most of the diff.

The index-versus-position decision

Doug's review asked for this to be settled here rather than left, and it is: an id resolves to index + 1.

A history position counts applied entries — position p is the document with entries 0 … p-1 applied — and everywhere the codebase records "which entry is this document at", it records the last entry included, never the next one to come. currentHistoryEntry is history[numHistoryEventsApplied - 1]; logCurrentHistoryEvent logs an id alongside a position one past that id's index; revisionId and the envelope's lastHistoryEntryId name the last entry the saved content includes. So the moment an id names is the position that includes it.

Worth knowing for anyone reading a link later: a log event's documentHistoryId is the last completed entry when the event was written, and entries complete asynchronously. For a tile logging from onTileAction — which MST calls before the action mutates state — that names the entry before the change being logged; for one logging after its edits settle (the text tile logs on blur) it names the entry the change created. index + 1 restores the document as it stood when the event was logged in both cases. It cannot recover a change whose entry did not exist yet, which is a logging question, not a seek one.

The convention is now written down in docs/history-framework.md, since its absence is what allowed the two to be conflated.

What changed

The seek and the stops

The seek resolves an id to a position rather than passing an index through, and "first" — the sentinel for a change made before the document had any entry — stays at position 0.

The slider carried the same off-by-one, and it is the older half of the bug. Before sliderEntries was introduced the slider value was the history position and the time readout came from currentHistoryEntry; adding the array re-pointed each stop at the entry it had not applied yet. Stops are document states again: sliderStops[0] is an explicit "initial" stop, a stop's array index is the slider value that selects it, and a history stop is the document once its own entry has been applied. That fixes the time readout, which labelled each stop with the timestamp of a change the reader had not seen, and comment markers, which showed the document one change before the comment was written.

Failure is reported. FirestoreHistoryManager gains an observable historyEntryRequestError; all three give-up paths set it, the seek is awaited and its landing position checked before a request counts as met, and PlaybackComponent renders it as a role="alert". It is cleared when a request starts, not only when one succeeds, so a stale message does not sit through the next request's 30-second load wait.

A model for the playback control

PlaybackControlModel owns where the reader is in a document's history, what the slider offers them, and the auto-play that walks them through it. The component keeps its refs, the marker state and the rendering, and shrinks from 439 lines to 279.

Two React workarounds go with it. sliderStops no longer threads the history length through a memo to notice appends, because a MobX computed tracks the MST array directly. And the effect that synced the thumb from numHistoryEventsApplied is now a computed — currentStopIndex derives from the document's position rather than being pushed at it, which is what makes a seek nobody asked for still move the thumb.

Comments cannot move into the model: they come from Firestore through React Query hooks, so the component pushes them in with setComments.

Where the thumb belongs

currentStopIndex is where the document is. sliderValue is where the thumb belongs, and the two differ while a seek of the model's own is running.

This is a real defect, not tidiness. rc-slider renders a dragged position only while it still equals the controlled value (useDrag.js), and numHistoryEventsApplied is assigned once, at the very end of the seek flow. So during a drag the thumb snapped back to where the seek started and crawled after the cursor. sliderValue reports the destination until the document settles there.

It matters which one each reader uses, so they are separated by rule: labels follow the thumb, actions follow the document. The time readout, the (N) entry readout and the chat panel's filter all read sliderStop; playbackDisabled stays on currentStopIndex, because advance() steps from there and a play button that disagrees with what pressing it does is worse than one that lags.

One seek at a time

rc-slider reports every mouse move of a drag, and each one started its own replay through the trees on top of the ones already running — the hazard goToHistoryEntryPosition has a standing TODO about. goToSliderStop now keeps one seek in flight; a request arriving mid-seek updates the target, and the loop picks it up when the current one finishes. A drag runs one replay to wherever the reader ended up rather than one per intermediate stop.

Following the end

A reader who has not picked a stop is following the end of the slider. That is now explicit: requestedStop is number | "end" | undefined — one specific stop, or wherever the end is now, or no standing request. The states are mutually exclusive by construction, and "end" is what an untouched control starts with.

It fixes an asymmetry. A reader at the end already followed new comments (the end of the slider moved under them) but was left a stop behind by new history entries, because those move the end without moving the document. Both now advance them, through one reaction on lastStopIndex.

A deep link is the case that must not follow: the reader asked for one entry, not for the end. PlaybackControlModel takes that as a constructor argument — requestedHistoryId ? undefined : "end" — rather than trying to detect it later. Detection was tried and does not work: by the time a new stop appears, a reader following along and a reader a link sent into the middle are both at a position that is not the end of the history, and are indistinguishable.

PlaybackControlComponent also gains a key on the document, since the model is built once per mount and a document swap has to give it a new one.

The chat panel follows the thumb (CLUE-665, in part)

The chat panel filters comments to createdAt <= playbackTime, and only goToSliderStop ever set that. So a deep link left the panel listing every comment on the document, a blocked seek left it on a moment that was never reached, and closing playback left it filtered by whatever moment was last shown.

playbackTime is now derived from sliderStopTime — the moment the thumb is on — and cleared when the control unmounts. Verified in the app: a link to entry 0 of a commented document shows the marker on the rail and an empty panel; clicking the marker shows the comment.

The initial stop is deliberately left as it was. It has no timestamp, and undefined already means "not filtering" — the value the panel needs before playback opens and after it closes. Expressing "before everything" needs a signal other than a bare Date, so that half of CLUE-665 stays open rather than being smuggled in behind a sentinel.

Smaller things the model surfaced

• stopIndexForHistoryPosition answered the end of the history with the last stop, which is a trailing comment rather than the entry on screen. The (N) readout only names history stops, so a link landing at the end stopped naming the change it had just applied. Now every position within the history maps to its own history stop.
• Requesting the last stop is recorded as "end", not as the index that happens to be last, so a reader who drags to the end is not silently pinned to a stop that new comments then arrive past.
• A blocked seek clears the request. The stop was never reached, so there is nothing left to prefer over wherever the document actually stopped — and it cannot be resurrected as a destination by a later seek.
• uniqueFailures is annotated false rather than left as a computed: a TreeManager's volatile state is shallow, so historyPlaybackFailures.push is not observable and could never invalidate a cached value.

Names

Since the confusion lived in them: goToHistoryEntry → goToHistoryEntryPosition, sliderEntries → sliderStops, goToSliderValue → goToSliderStop, and the two mapping helpers to match. Within the model, eventCreatedTime → sliderStopTime and currentHistoryIndex → sliderStopHistoryIndex, so the getters that describe the thumb read as one family and are not mistaken for ones that describe the document.

The two alerts no longer overlap

.playback-history-request-error and .playback-failure-warning were both absolutely positioned at the same spot against .playback-component, and both can be visible at once — a deep link whose seek stops short, then a drag past the same unplayable entry. The request error stacks above the warning, and eleven near-identical declarations plus three one-off hex colors moved behind a playback-alert mixin in vars.scss.

Testing

npx jest src/components src/models — 2095 passed, 174 suites. Full suite: 359 suites, 4060 passed. npm run check:types clean, npm run lint:build 0 errors.

playback-control-model.test.ts covers the model directly — 50 tests over stop ordering, the position ↔ stop-index mapping, blocked seeks, coalescing, follow-the-end and auto-play. Every behavioral test was written before its code and watched failing, including the ones added late: the thumb reporting stop 0 while a seek to stop 4 was running; three rapid requests producing two goToHistoryEntryPosition calls instead of three; a reader at the end left behind by a new entry; a reader a link sent into the middle being dragged to the end.

The MobX wiring test earns its place by construction — deleting historyEntryRequestError: observable makes it fail while the stub-based test alongside it still passes, which was the gap Doug identified.

history_playback_spec.js was restructured, and it is worth a reviewer's attention. It asserted that an undo in the primary document leaves the playback view a stop behind, showing the row the undo had just removed — the opposite of the follow-the-end rule above. The section it asserted that in was built on the view staying put, so it now checks both halves of the rule instead: a view at the end follows the entry an undo records, and a view scrubbed back stays where it was left and has to be played forward. Verified to still fail when the follow-end reaction is disabled, so it guards the behavior rather than tolerating it. No it blocks were added, given the per-month budget.

A later review pass found five places where a test could not fail — the request cleared after a blocked seek, the unknown-position branch, the failure-marker dedup, the HISTORY_ERROR give-up path, and a sliderValue assertion that compared the expression with itself. Each is now covered by a test checked the same way: delete the line it covers, watch only that test go red.

Two things I did not test. The alert overlap is CSS-only and jsdom loads no stylesheets, so a test rendering both alerts would assert they exist, not that they are apart. And teacher_student_work_spec.js passes under either index-versus-position mapping — it types an extra space between the checkpoint and additionalText — so its being green is not evidence about this fix either way.

Follow-ups, not in this PR

• CLUE-665 is now half done. The deep-link half is fixed here. What remains is the leftmost stop: it has no timestamp, so the panel shows every comment on a document that has not had any of its history applied. Re-verified in the app against this branch.
• CLUE-280 is not fixed here, and the follow-the-end change above is not it. That moves a reader who is already at the last stop; CLUE-280 asks for the opposite — a reader who is not at the end should be taken there when a comment is posted. Re-verified against this branch: scrubbed to stop 9 of 17, posting a comment grew the slider to 18 and left the thumb at 9, and the panel showed nothing, including the comment just written. As Scott notes on the ticket, the real fix is probably recording each comment's history entry id rather than moving the scrubber.
• A dismiss control for the request alert was considered and judged not worth it, now that the message clears when a request starts.
• A personal document did not open through a studentDocument link — fetchFullDocument logged "Could not find metadata doc with key" for one whose metadata carries unit: null. Noticed while building test links, unrelated to this PR, and not diagnosed. Recorded on CLUE-631, which covers personal documents in the history viewer from the launch-parameter side; this looks like a second blocker on its first acceptance criterion.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

A replay failure can still silently leave a deep link at the wrong history position because seek completion is not awaited or validated.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Synchronizes playback controls with deep-linked history positions and surfaces failed history requests.

Changes:

• Maps slider values to document history positions bidirectionally.
• Displays accessible history-request errors.
• Adds playback and history-manager tests.

File summaries

File	Description
src/models/history/firestore-history-manager.ts	Tracks history-request failures.
src/models/history/firestore-history-manager.test.ts	Tests request outcomes and timeouts.
src/components/playback/playback.tsx	Renders history-request alerts.
src/components/playback/playback.test.tsx	Tests alert rendering.
src/components/playback/playback.scss	Styles request alerts.
src/components/playback/playback-control.tsx	Synchronizes slider and history positions.
src/components/playback/playback-control.test.tsx	Tests programmatic seeking and comment selection.

Review details

Suppressed comments (1)

src/models/history/firestore-history-manager.ts:337

• This final sentence becomes false as soon as the user moves the scrubber after the failed deep link: the request error persists, but the document is no longer showing the end. Keep the persistent alert limited to the failed request rather than asserting the current playback position.

      this.setHistoryEntryRequestError(
        `Could not find the requested point in this document's history (id: ${historyId}). ` +
        "The document is showing the end of its history.");

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[scytacki]

Copilot review addressed

All three findings, including the suppressed one, are fixed in fix: only call a history request successful once the seek lands.

Seek not awaited or validated — valid. goToHistoryEntry is now awaited and the applied position checked before the request counts as successful; a rejection is caught too, which turned out to matter more than it looked (unawaited, it escaped as an unhandled rejection and killed the jest process when I wrote the test for it). Replies on the thread.

"Seeked" in a test name — renamed. Thread reply.

The suppressed comment on the unresolved-id message — this one was right, and I would rank it above the confidence it was filed with. Nothing clears historyEntryRequestError except a successful moveToHistoryEntryAfterLoad, which does not run again in that session, so the alert outlives the failed request. The moment the reader moves the scrubber, "The document is showing the end of its history" is a false statement rendered on screen. That sentence is gone; the message now states only what failed, and where the document is stays the scrubber's job to report.

Three tests had stubbed goToHistoryEntry as () => undefined, which leaves the history position untouched — indistinguishable from a seek that stops short. Now that callers check the applied position, those stubs were asserting against a fiction, so they move the position the way a completed seek does.

npx jest src/components/playback src/models/history — 137 passed, 11 suites. npm run check:types clean. The two remaining ESLint warnings in these files (TestTile, and the unused error in subscribeToFirestoreHistory's catch) are pre-existing and outside the diff.

One limit worth stating: the in-app verification in the description was run before these changes and I have not re-run it. The unresolved-id and stop-short paths are covered by tests; deliberately corrupting a history entry to exercise the replay failure in a browser is not something I set up.

[codecov]

Codecov Report

❌ Patch coverage is 99.54955% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.43%. Comparing base (3bac359) to head (5c6f18e).
⚠️ Report is 56 commits behind head on master.

Files with missing lines	Patch %	Lines
src/components/playback/playback-control.tsx	97.43%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2992      +/-   ##
==========================================
+ Coverage   86.38%   86.43%   +0.05%     
==========================================
  Files         996      997       +1     
  Lines       56856    56949      +93     
  Branches    15060    15065       +5     
==========================================
+ Hits        49113    49222     +109     
+ Misses       7723     7707      -16     
  Partials       20       20              

Flag	Coverage Δ
cypress	?
cypress-regression	71.19% <87.09%> (+0.12%)	⬆️
cypress-smoke	41.08% <2.30%> (-0.08%)	⬇️
jest	58.20% <96.39%> (+0.44%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cypress]

collaborative-learning    Run #20282

Run Properties:   Passed #20282  •  5c6f18e949: CLUE-613 docs: say position where a comment said index

Project	collaborative-learning
Branch Review	CLUE-613-history-entry-seek
Run status	Passed #20282
Run duration	03m 39s
Commit	5c6f18e949: CLUE-613 docs: say position where a comment said index
Committer	Scott Cytacki
View all properties for this run ↗︎

---

Test results
Failures	0
Flaky	0
Pending	0
Skipped	0
Passing	4
View all changes introduced in this branch ↗︎

[dougmartin]

The scrubber sync and the failure reporting are both solid work, but the seek underneath them lands one history entry short of the requested revision, and this PR adds the success signal that certifies that position as correct. Resolving the index-versus-position mapping is the one thing I'd like settled before merge.

Changes requested

• src/models/history/firestore-history-manager.ts (lines 331, 346, 352): findHistoryEntryIndex returns an array index, but goToHistoryEntry takes a position, where position n means entries 0..n-1 have been applied. tree-manager.ts (lines 558-562) replays the half-open range [numHistoryEventsApplied, newHistoryPosition), and the repo's own test at src/models/history/tree-manager-multi-tree.test.ts (lines 288-291) pins it: with three entries, goToHistoryEntry(1) leaves treeA at "A1" (entry 0 applied) and treeB undefined (entry 1 not applied). So seeking to index i shows the document before entry i was applied, and the requested revision needs position i + 1. src/models/document/log-document-event.ts (line 54) records latestDocumentHistoryEntry?.id, i.e. history[length - 1].id. The comment just above it (lines 47-50) is careful to say this may be either the entry that was current before the change or the one created by it, depending on where the log call sits relative to the edit, so I do not want to overstate it. It does not matter which: under both readings the id names an entry that had already been applied when the event was logged, so restoring that moment needs that entry included, which is position index + 1. Either way the researcher is currently shown the document without it. Two further signals that i + 1 is the intended rule: README.md (line 179) documents the parameter as "move to the specified revision", and the "first" sentinel on this same line maps to position 0, which is index + 1 for a conceptual index of -1 and is inconsistent with the raw-index rule used by the other branch of the same expression. Suggested fix: resolve to a position rather than an index, e.g. keep const index = this.treeManager.findHistoryEntryIndex(historyId), guard on index < 0, and seek to index + 1, leaving the "first" branch at 0. Add a test in firestore-history-manager.test.ts asserting the argument for a resolvable id (moveToHistoryEntryAfterLoad("a2") over [{id:"a1"},{id:"a2"}] expects goToHistoryEntry called with 2), since no current test pins that mapping. Worth knowing that the existing E2E coverage cannot settle this either: cypress/e2e/functional/teacher_tests/teacher_student_work_spec.js (lines 54-56, 117-125) deliberately types an extra space so the captured id comes from the following event, then asserts only contain initialText and not.contain additionalText. With a whole extra edit of slack between the checkpoint and additionalText, it passes under either mapping, so its being green is not evidence the current one is right. Two notes in fairness: this mapping is byte-identical on master, so the PR does not introduce it, and the in-app verification in the description could not have caught it, because the new thumb sync derives the slider position from the same numHistoryEventsApplied the seek set, so content and scrubber agree with each other while both sit one entry early. What is new here is line 352 clearing historyEntryRequestError on numHistoryEventsApplied === entry, which now affirmatively reports success for that position. If you conclude the raw-index convention is deliberate, then the two branches of line 331 still contradict each other and renderTimeInfo in playback-control.tsx (lines 264-270), which labels position i with entry i's timestamp, is what needs reconciling instead. Either way I'd like it decided in this PR rather than left as is.

Non-blocking

• src/components/playback/playback.scss (lines 198-212) and src/components/playback/playback-control.scss (lines 574-587): the new .playback-history-request-error and the existing .playback-failure-warning both use position: absolute; bottom: 100%; left: 50%; transform: translateX(-50%); z-index: 10, and .playback-controls declares no position, so both resolve against the same containing block, .playback-component. When both are visible they are drawn on the exact same spot. That is reachable normally: a deep link whose seek stops short sets historyEntryRequestError, and dragging the scrubber past the same bad entry then sets playbackFailureWarning. Consider offsetting the request error (bottom: calc(100% + 28px)) or rendering both through one column container, with a test case that has both present.
• src/components/playback/playback.scss (lines 199-211) duplicating src/components/playback/playback-control.scss (lines 575-586): eleven of thirteen declarations are identical, including three hardcoded hex colors (#f8d7da, #721c24, #f5c6cb) that appear nowhere else in src/. Only max-width/text-align versus white-space: nowrap differ. Worth extracting a shared placeholder or mixin into src/components/vars.scss (both files already import it) so the two alerts cannot drift, and promoting the colors to named variables while you are there.
• src/models/history/firestore-history-manager.ts (lines 304-359) and src/components/playback/playback.tsx (lines 45-50): historyEntryRequestError is cleared only by a later successful seek, and there is no dismiss control. Since moveToHistoryEntryAfterLoad does not reset it on entry, a stale message also survives the up-to-30-second load wait of a new request. Clearing it as the first statement of moveToHistoryEntryAfterLoad costs one line; a dismiss button, or clearing when the user takes over the scrubber, would stop the message occupying the strip above the playback bar for the life of the view.
• src/components/playback/playback.tsx (lines 24-28) with src/components/chat/chat-panel.tsx (lines 76-80): a deep-link seek goes through moveToHistoryEntryAfterLoad to goToHistoryEntry and never calls setPlaybackTime, which only goToSliderValue does. So playbackTime stays undefined and the chat panel shows every comment, including ones written after the linked moment, while the thumb now reports an earlier point. Pre-existing rather than a regression, but this PR is what makes the two visibly disagree, and it is adjacent to the stale-playbackTime bug you already flagged in the description.
• src/components/playback/playback.test.tsx (lines 47-54): makeHistoryManager returns a plain object cast to FirestoreHistoryManager, and the manager tests read historyEntryRequestError directly, so nothing exercises the MobX wiring. Deleting historyEntryRequestError: observable from the makeObservable call at firestore-history-manager.ts (line 95) would break the alert in the real app, since nothing would trigger a re-render after the async seek fails, and the whole suite would stay green. A test that renders with a real manager and sets the error after mount would close that.
• src/components/playback/playback-control.tsx (lines 121-122, 142, 179): the comment says sliderIndexForHistoryPosition "Returns -1 if the position has no entry in the slider", but sliderEntries is built by mapping over every history entry (lines 78-79), so every position below history.length has a match and positions at or above it return early. findIndex can never return -1, making the index >= 0 ? index : current guard at line 142 and the actualSliderIndex >= 0 guard at line 179 dead. Either drop the -1 claim and simplify both call sites, or make the guard real.
• src/components/playback/playback-control.tsx (lines 55-56): allComments is rebuilt on every render, so the sliderEntries memo, both new callbacks, goToSliderValue, and the auto-play effect all get fresh identities every render. You noted the effect churn in the description; worth adding that the auto-play cleanup also tears down and restarts the 500 ms advance timer on every render during playback. A useMemo keyed on [comments, simplePathComments] stabilizes all of it in one line, and would let the redundant re-sort at lines 84-86 go too.
• src/components/playback/playback-control.tsx (lines 379-382): sliderEntries.findIndex(e => e.kind === "history" && e.index === failure.historyIndex) is the body of the helper this PR just extracted, left as a third copy in the same file. Replacing it with sliderIndexForHistoryPosition(failure.historyIndex) finishes the refactor; note the helper returns sliderEntries.length rather than -1 past the end, so the guard becomes sliderIndex < 0 || sliderIndex >= sliderEntries.length.
• src/components/playback/playback.test.tsx (lines 57-67): the comment says "The message appears after the document has loaded, so it has to announce itself", but the test renders with the error already set, so the role="alert" node exists at mount, which is the case screen readers do not announce. The assertion only proves the attribute is present. Setting the error after the initial render would make the test match its comment.
• src/components/playback/playback.test.tsx (lines 69-74): "shows no message when the requested history entry was found" overstates it. moveToHistoryEntryAfterLoad is a bare jest.fn(), so requestedHistoryId="entry-0" does nothing and the test only asserts that an undefined error renders nothing. Either rename to something like "renders nothing when there is no request error", or add expect(historyManager.moveToHistoryEntryAfterLoad).toHaveBeenCalledWith("entry-0") so the mount effect is actually covered.
• Comment audit, four spots where the prose narrates the change rather than the code, which will read oddly once there is no diff to sit beside:
  • src/models/history/firestore-history-manager.ts (lines 334-336): "Without this the document silently shows some other moment..." argues for the diff, and the second sentence records a resolved review discussion about wording.
  • src/models/history/firestore-history-manager.ts (lines 342-344): three lines to state one non-obvious constraint, and the same paragraph is repeated at firestore-history-manager.test.ts (lines 389-390). One line would do: "goToHistoryEntry stops at the last position it could apply, so the applied position must be checked."
  • src/components/playback/playback-control.tsx (lines 132-136): five lines where only the last clause carries information the code cannot, namely that a comment and the entry before it share one history position.
  • src/models/history/firestore-history-manager.test.ts (lines 338-342): "Callers now check that, so a stub that leaves the position alone reads as a seek that stopped short" is a before/after framing about this PR; after merge there is no "now". The first sentence of the block stands on its own.

For the record, I re-ran everything on fbbce3b8c: npx jest src/components/playback src/models/history gives 137 passed across 11 suites, npm run check:types is clean, and the only ESLint warnings in the touched files are the two pre-existing ones you named. The refactor in the first commit is behavior-preserving in every branch, the Copilot findings are genuinely addressed, and playback-control.test.tsx is the kind of test that actually fails when the effect is removed.

[Copilot]

🟡 Changes recommended

Disposing the playback model during an active seek can allow autoplay to continue after unmount.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 18/18 changed files
• Comments generated: 2
• Review effort level: Balanced

[scytacki]

@dougmartin Everything from your review is addressed. Walking it in order.

Blocking: index versus position

You were right, and the fix is in "open a linked document with the requested entry applied".

moveToHistoryEntryAfterLoad now resolves to a position: findHistoryEntryIndex still returns an index, the index < 0 guard still short-circuits, and the seek gets index + 1. "first" stays at 0, so the two branches of that expression agree on units for the first time.

Your reasoning from log-document-event.ts holds up from two more directions: currentHistoryEntry is history[numHistoryEventsApplied - 1], and revisionId names the last entry the saved content includes. Under all three readings an id names an entry that had already been applied, so the moment it names is index + 1.

Two tests pin it, since as you say nothing did: moveToHistoryEntryAfterLoad("a2") over [{id:"a1"},{id:"a2"}] asserts the seek is called with 2, and a second asserts "first" maps to 0.

The slider carried the same off-by-one, so rather than pick one of the two branches of your "either way" I fixed both. sliderEntries was built by mapping over history entries, which pointed each stop at the entry it had not applied yet. Stops are document states again: sliderStops[0] is the initial document, and a history stop is the document once its own entry is applied. That corrects the renderTimeInfo labelling you identified, and comment markers, which were showing the document one change before the comment was written — the thing that code always claimed to do. There is now a test that the readout labels a position with the entry that has been applied.

Renamed where the confusion lived: goToHistoryEntry → goToHistoryEntryPosition, sliderEntries → sliderStops, goToSliderValue → goToSliderStop. "Slider value" now survives only at the rc-slider boundary. docs/history-framework.md gained a section on positions versus indexes.

Your note that teacher_student_work_spec.js could not settle this was worth having — it stopped me reading its green as evidence.

Non-blocking

Alerts on the same spot. Fixed. The request error stacks above the warning by the height of a one-line alert, which is what the warning is. I did not add the test you suggested: jsdom loads no stylesheets, so a test rendering both would assert they exist, not that they are apart.

Duplicated SCSS. Fixed. A playback-alert mixin in vars.scss with the three colors promoted to named variables, so the two alerts cannot drift.

Stale request error. Fixed, with a test. Cleared as the first statement of moveToHistoryEntryAfterLoad, so a message describing a replaced request cannot sit through the 30-second wait. I did not add a dismiss control or clear it when the reader takes over the scrubber: the message records that a link failed, which stays true however the reader moves afterwards, and it no longer collides with the warning. Happy to revisit if you would still rather it be dismissible.

Chat panel disagreeing with the thumb. Fixed. The panel's filter is now derived from the stop the thumb is on, so it follows every way the document can move, including a deep-link seek the slider never started. Test drives treeManager.goToHistoryEntryPosition directly and asserts the panel is told the right moment.

MobX wiring untested. Fixed. playback.test.tsx builds a real FirestoreHistoryManager and sets the error after mount. Removing historyEntryRequestError: observable fails that test while the stubbed one stays green, which was your point exactly.

Dead -1 guards. Gone. The mapping now documents what it actually does at the edges rather than claiming a -1 it cannot return, and both call sites lost their unreachable guards.

allComments rebuilt every render. Fixed — and thank you, because this one paid off twice. Stabilizing it broke history_playback_spec in CI: the sliderStops memo listed history as a dependency, but that is an MST array whose identity never changes when entries are appended, so it could never invalidate. It had only ever stayed fresh by accident, via allComments churning. The entry count is now an input. Your item exposed a real defect, not just wasted work.

Third copy of the findIndex. Fixed — the failure markers call the shared mapping.

role="alert" test. Fixed. The error is set after mount, so the test now covers the case screen readers actually announce.

"shows no message when the requested history entry was found". Renamed to say what it checks, and it now asserts the mount effect passed the id along.

Comment audit. All four. The last one — "Callers now check that" — went in a commit just now; the others went with their sections above.

Since your review

The component you reviewed has been reshaped: its state, memos and callbacks moved into a PlaybackControlModel, leaving playback-control.tsx as rendering plus refs (439 → 279 lines) with 43 unit tests on the model. Two later Copilot findings are also fixed — a historyPositionForStopIndex doc pointer, and autoplay continuing after the control was disposed mid-seek. The history_playback_spec section that asserted the old non-following behaviour was rewritten, since the slider now follows the end of a growing history.

npx jest src/components/playback src/models/history is 188 passed across 12 suites; types and lint clean.

[dougmartin]

Looks good 👍

The blocking item from my last review is settled, and settled the right way. moveToHistoryEntryAfterLoad now resolves an id to a position rather than passing an index through, "first" stays at 0, and firestore-history-manager.test.ts pins the mapping with an assertion that actually discriminates. I verified the convention independently rather than taking the description's word for it: tree-manager.ts (lines 558-562) replays the half-open range [numHistoryEventsApplied, newHistoryPosition), currentHistoryEntry reads numHistoryEventsApplied - 1, and setNumHistoryEntriesAppliedFromFirestore (line 370) sets lastHistoryEntry.index + 1, so index + 1 is the position that includes the named entry. Writing the rule down in docs/history-framework.md was the right call, and the rename to goToHistoryEntryPosition is complete: no reference to any of the old names survives anywhere in the tree, including the comment headers in tree.ts and tree-monitor.ts. Every other item from my last review is addressed too, and several of them (the sliderStops computed, the failure-marker helper, the playbackTime derivation) came back better than what I asked for.

On the head commit: npx jest src/components/playback src/models/history gives 188 passed across 12 suites, npm run check:types is clean, and npx eslint on the touched directories reports 0 errors with 4 warnings, all of which I confirmed pre-exist on master. The one real CI failure, regression (09), is document_tests/tiles_copy_test_spec.js timing out on a cell count (found 22, expected 32) and is unrelated to playback; codecov/project is comparing against a report 56 commits behind master, and cypress/flake is reporting flakes rather than failures.

A few non-blocking notes (take or leave):

• src/components/playback/playback-control-model.ts (lines 334-357): runSeekLoop awaits goToHistoryEntryPosition with no try, so a rejected flow leaves queuedStopIndex set forever and escapes as an unhandled rejection through void model.goToSliderStop(value) at playback-control.tsx:88 (or void this.advance() at line 366). Because sliderValue is queuedStopIndex ?? currentStopIndex, the thumb, the time readout and the (N) readout would all keep reporting a stop the document never reached, with no warning, which is precisely the silent-wrong-moment failure this PR exists to remove; during auto-play it would also leave sliderPlaying true with no timer, so the button shows Pause forever. This is the same hazard Copilot found in moveToHistoryEntryAfterLoad, which you guarded at firestore-history-manager.ts:354-360, and the model is the one funnel left unguarded. I am not blocking on it because the ordinary replay failure is handled inside the flow and surfaces as blocked: only an internal throw (the treePatches missing entry for known tree case, or a rejection from the unguarded Promise.all over startApplyingPatchesFromManager) reaches it. Suggested fix: wrap the await in try/catch, treat a rejection the same as a blocked seek (set playbackFailureWarning, clear requestedStop), and move the if (this.queuedStopIndex === target) this.queuedStopIndex = undefined; into a finally so it clears on both paths, with a test that stubs the seek to reject and asserts sliderValue falls back to currentStopIndex.

• src/components/document/canvas.tsx (lines 108-125, 258): checkForHistoryRequest runs only from componentDidMount and never clears requestedHistoryId, but sortedDocuments.getDocumentHistoryViewRequest (src/models/stores/sorted-documents.ts, lines 103-111) deliberately deletes the request once read, so the store is one-shot while the component state that mirrors it is not. sort-work-document-area.tsx (lines 151-160) renders EditableDocumentContent without a key, so switching the open document reuses the same Canvas and the stale id survives. This PR makes that visible in two new ways: the effect in playback.tsx (lines 24-28) re-fires when historyManager changes and now renders Could not find the requested point in this document's history (id: ...) as an alert on the second document, and the new key={document?.key} rebuilds the model with requestedHistoryId ? undefined : "end", so the second document silently loses follow-the-end. Pre-existing rather than introduced here, but this is the PR that surfaces it. Suggested fix: clear the state when the document has no request (an else branch setting requestedHistoryId: undefined), or move the read so it re-runs per document.

• src/components/playback/playback.tsx (lines 48-53): the role="alert" container and its text enter the DOM in the same commit, and assistive technology announces a live region reliably only when the region already exists and its contents change; a region inserted together with its text is announced inconsistently across screen reader and browser combinations. The new test at playback.test.tsx (lines 98-113) asserts the node is present after the state change, which does not distinguish the two cases. Since the whole point is that a researcher following a report link learns the moment was not reached, a missed announcement returns exactly those users to the silent failure. Suggested fix: render the container unconditionally with hidden={!historyEntryRequestError} and only the message inside, then assert in the test that the alert node exists before the error is set and gains its text afterwards.

• src/components/playback/playback-control-model.ts (line 351) with the test at src/components/playback/playback-control-model.test.ts (lines 354-366): if (blocked) this.requestedStop = undefined; has no test that can fail. "goes straight to a new destination after a blocked seek" asserts sliderValue === 1 right after goToSliderStop(1), and that call sets queuedStopIndex = 1 synchronously, so the assertion is true by construction and stays green with line 351 deleted. The other four blocked-seek tests do not cover it either: in each, historyPositionForStopIndex(requestedStopIndex) already fails to match the landed position, so currentStopIndex falls through to stopIndexForHistoryPosition and answers the same either way. The line's real job is to stop a reader whose requestedStop was "end" being dragged back to the end by the follow-end reaction on the next new entry, retrying the entry that just failed. Suggested fix: in the follow-the-end describe, run a blocked goToSliderStop(4) on setupModel(4, { appliedPosition: 0, failingEntryIndex: 2 }), then addEntry(treeManager, 4) and flush, asserting currentStopIndex stays at 2.

• src/components/playback/playback-control-model.ts (line 186) with the test at src/components/playback/playback-control-model.test.ts (lines 409-421): the historyPosition === undefined branch is never exercised. The test whose comment claims to cover it passes appliedPosition: 0, which is a defined position, and setupTreeManager (line 65) can never produce undefined because it always calls setNumHistoryEntriesApplied. With line 186 deleted, stopIndexForHistoryPosition(undefined) falls through both guards, findIndex returns -1, currentStopIndex becomes -1 and the thumb leaves the rail, and nothing in the suite notices. The comment also promises coverage the test does not deliver. Suggested fix: let appliedPosition be explicitly absent ("appliedPosition" in options rather than ??), then assert currentStopIndex === lastStopIndex and playbackDisabled while the position is unknown, and that it follows the document once the position lands.

• src/components/playback/playback-control-model.ts (lines 275-279) with the test at src/components/playback/playback-control-model.test.ts (lines 607-620): the seenIndices dedup is never exercised. TreeManager already dedupes by (historyIndex, direction) (tree-manager.ts, lines 773-796) and the backward seek in the test never touches entry 2, so historyPlaybackFailures holds exactly one element by the time the assertion runs; deleting the whole seenIndices set leaves the suite green. The model's dedup exists precisely because the manager's key includes direction and the model's does not, so the same entry failing on both redo and undo would draw two markers on one spot, and that is the case the test does not create. Suggested fix: push two failures with the same historyIndex and different direction values directly onto treeManager.historyPlaybackFailures, then assert uniqueFailures.length is 1.

• src/models/history/firestore-history-manager.ts (lines 328-332): the branch that reports historyNotLoadedMessage when when() resolves on HISTORY_ERROR rather than HISTORY_LOADED is new here and uncovered; the "reports a history that never loads" test (firestore-history-manager.test.ts, lines 445-456) exercises the 30-second timeout at lines 322-327 instead. It is one of the three give-up paths this ticket is about, and the one a real Firestore permission failure takes. Suggested fix: drive the manager to HISTORY_ERROR via setHistoryError, call moveToHistoryEntryAfterLoad("first"), and assert the error is set and goToHistoryEntryPosition was never called.

• src/models/history/firestore-history-manager.ts (lines 361-367): the success branch is dead. Line 307 clears the error as the first statement of the method and nothing between 307 and 361 sets it on the success path, so setHistoryEntryRequestError(undefined) at line 362 can never change anything, and the test covering it (firestore-history-manager.test.ts, lines 408-416) passes with the line deleted. This is what the move of the clear to the top of the method orphaned. Suggested fix: drop the if/else and keep only the failure path.

• src/components/playback/playback-control-model.test.ts (lines 320-324): "shows where the document is once nothing is moving it" asserts model.sliderValue === model.currentStopIndex, but sliderValue is queuedStopIndex ?? currentStopIndex and no seek is queued, so the two sides are the same expression and the test cannot fail for any implementation that falls back to currentStopIndex. Suggested fix: assert a concrete number instead, for example setupModel(5, { appliedPosition: 2 }) then expect(model.sliderValue).toBe(2), which fails if sliderValue falls back to requestedStopIndex or lastStopIndex.

• src/models/history/log-history-event.ts (line 12): historyIndex?: number; // Index into history array. Used for start, stop, seek. contradicts the convention this PR documents. The value assigned on line 53 is numHistoryEventsApplied, a position, and docs/history-framework.md (line 99) now says so explicitly. This is the exact conflation the PR set out to end, still sitting one line above the field a reader reaches for, and the new doc and this comment now disagree in writing. Suggested fix: // History position (count of applied entries), not an array index. Used for start, stop, seek. A rename would be nicer but it is a logged wire field, so a follow-up.

• src/components/playback/playback-control-model.ts (line 359): "Percentage along the rail, which spans the stops after the initial one" describes the pre-initial-stop arrangement. The slider renders min={0} max={model.lastStopIndex} (playback-control.tsx, lines 161-163), so the rail's left edge is the initial stop and 100 * (stopIndex / lastStopIndex) puts stop 0 at 0%. It is the one line telling a reader what the percentage is relative to, and it names the wrong origin, which is how the marker off-by-one you just fixed got in. Suggested fix: // Percentage along the rail, which runs from the initial stop at 0 to the last stop.

• src/components/playback/playback-control-model.test.ts (lines 433-434): "rc-slider reports every mouse move of a drag, and each one used to start its own replay through the trees on top of the ones already running" is a before/after framing about this PR. After merge there is no "used to". Suggested fix: state it as the reason the coalescing exists, for example "rc-slider reports every mouse move of a drag, so without coalescing each one starts its own replay through the trees on top of the ones already running."

• src/models/history/firestore-history-manager.ts (lines 343-344): "The message says only what failed. It outlives the request, and the reader can move the scrubber, so where the document sits is the scrubber's to report" explains why a sentence that no longer exists was removed from the message. A reader a year from now sees a one-line setHistoryEntryRequestError call and a paragraph defending an absence, and the PR conversation already records the decision. Suggested fix: delete both lines.

• src/models/history/firestore-history-manager.ts (lines 352-353) with src/models/history/firestore-history-manager.test.ts (lines 337-341 and 418-419): the same constraint, that goToHistoryEntryPosition stops at the last position it could apply so callers must check where it landed, is written out three times. This is the item that came down from three lines to two but kept the duplication. Suggested fix: keep the source comment as one line, cut the mockCompletedSeek block to its first sentence, and delete the comment above "reports a seek that stops short", where the test name already says it.

• src/components/playback/playback-control-model.ts (lines 16-20, 181-185, 325-328): three comments longer than the point they make. Lines 16-20 spend five lines on the initial stop, of which only "it describes no change, so it has no date" is not already in docs/history-framework.md; lines 181-185 spend five lines where the load-ordering fact is the whole content; lines 325-328 spend four lines on dispose(), where only "set directly rather than through togglePlay, which would log a pause the reader never performed" is not readable from the two statements below. Suggested fix: one line each.

• cypress/e2e/functional/teacher_tests/history_playback_spec.js (lines 145-146): "so it follows the entry the undo recorded rather than being left one stop behind it" describes the behavior this PR removed, which will not exist for anyone reading the file after merge, and the cy.log on line 143 already states the rule. Suggested fix: cut the trailing clause, or delete the comment.

• PR description, the Testing section: it says playback-control-model.test.ts covers 43 tests, but on 3cbe04b4b the file runs 44. Small, but it is the kind of number that goes stale after a rebase and is worth re-measuring before merge.

For the record on what I checked rather than assumed: the restructured history_playback_spec.js does assert something real (without the follow-end reaction the playback pane would still show '2' after the undo, so the assertion distinguishes the two behaviors, and the removed cy.wait(2000) is covered by the 30-second defaultCommandTimeout in CI), the new uniqueFailures: false annotation is correct because MST volatile is created shallow so pushes onto the array are not observable, and I confirmed the mutations that the key new tests catch, including that deleting historyEntryRequestError: observable fails the real-manager alert test while the stubbed one stays green. Nice piece of work: the model extraction earns itself several times over in what it made visible.

[scytacki]

@dougmartin Thanks — all fifteen are implemented. Five commits, 18fba04ec..5c6f18e94. Where I did something other than what you suggested, it is because the suggested version did not discriminate; details below.

The unguarded seek

Fixed, and it behaves exactly as you predicted. Writing the test took the jest process down with an unhandled rejection, which is what that escape looks like from outside.

A throw is now treated as a seek that stopped short: failure warning set, requestedStop dropped, queue cleared so the thumb comes back to the document. I used a flag and a plain try/catch rather than a finally — with the rejection swallowed both paths reach the same code, so the finally would not add a guarantee. Two tests: the thumb returns to the document, and auto-play stops rather than showing Pause forever.

The four dead-line findings

All four were real, and in two of them the suggested assertion would not have caught the deletion. Every one is now checked by deleting the line it covers and confirming only the new test goes red.

if (blocked) this.requestedStop = undefined. Your test does not discriminate: without the line the follow-end reaction does fire, but the retry is blocked at the same entry, so currentStopIndex reads 2 either way. What does discriminate is the thumb — the reaction sets queuedStopIndex synchronously as the entry is added, so sliderValue is 5 with the line gone and 2 with it there. The test now asserts that, immediately after addEntry, before flushing.

The unknown-position branch. Confirmed unreachable, including via your suggested fix: "appliedPosition" in options alone still leaves the position at the model's default of 0. The setup can now leave it genuinely unset, which needed setNumHistoryEntriesApplied to admit the undefined its own property type already allows and setNumHistoryEntriesAppliedFromFirestore already assigns. Two tests: the end while unknown, and a link's stop while unknown. Deleting the branch fails both with currentStopIndex of -1, as you said it would.

The failure-marker dedup. Real, but not reachable the way you suggested, and finding that out was worth the detour: a seek can only ever fail in one direction. A blocked undo stops at failingIndex + 1, leaving the entry applied, so the reader can never get back below it to fail it forward; a blocked redo stops at failingIndex, so it is never applied and never undone. What does produce both is the position arriving from Firestore — it counts the entries the document has, not the ones that can be applied, so it can sit past a bad entry. The test plays forward into the failure, sets the position the way the lookup does, and seeks back. Two failures, two directions, one marker. I preferred that to pushing fixtures onto historyPlaybackFailures, since it is a sequence the app actually produces.

The sliderValue tautology. Now setupModel(5, { appliedPosition: 2 }) and expect(model.sliderValue).toBe(2), exactly as you suggested.

The rest

HISTORY_ERROR give-up path. Covered, via mirrorMockHistory with a loadingError. Asserts the status, the message, and that the seek was never attempted.

Dead success branch. Removed. You are right that moving the clear to the top orphaned it.

The live region. Fixed, though not with hidden: an element with hidden is out of the accessibility tree, so unhiding it and filling it in the same commit is the same problem as inserting it. The region is now always rendered and always in the tree, with its padding, border and background dropped while :empty so it paints nothing. It is absolutely positioned either way, so it never takes room in the bar. The test asserts the region is present and empty before the error is set.

Canvas holding a stale request. Fixed. One correction to the diagnosis: checkForHistoryRequest does also run from componentDidUpdate (line 171 on master), so it is not mount-only — but your conclusion holds for a different reason, which is that it only ever sets the id and nothing clears it. That also rules out the else branch: it would fire on the very next update, after the store has already deleted the request, and wipe the id before the seek. It is cleared when the document changes instead. The test reuses one canvas across two documents and fails on the unfixed code with the second document requesting the first one's entry.

Comment audit. All seven: historyIndex now says position, railLocation names the initial stop as the origin, the "used to" framing and the two firestore-history-manager paragraphs are gone, the triplicated constraint is one line in the source and nothing in the tests, the three long model comments are one or two lines each, and the cypress comment is deleted.

PR description. Re-measured: 50 tests, not 43. The Testing section now says so and names the five coverage gaps this pass closed.

State on the head commit

npx jest src/components/playback src/models/history is 195 passed across 12 suites, plus 5 in canvas.test.tsx. npm run check:types clean; npx eslint on the touched directories is 0 errors and 4 warnings, the same pre-existing ones you confirmed.

Agreed on historyIndex as a follow-up rename — it is a logged wire field, so it wants its own change.