Aggregate embedded viewer progress in the SafeHtml article viewer#15037
Aggregate embedded viewer progress in the SafeHtml article viewer#15037rtibbles wants to merge 1 commit into
Conversation
github-actions
Bot
added
DEV: renderers
Build Artifacts
Smoke test screenshot |
🔵 Review postedLast updated: 2026-07-20 03:55 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15037 aggregates embedded-viewer progress into the SafeHtml5 article renderer. Logic is clean, well-guarded, and thoroughly unit-tested; the wired <SafeHTML> events are inert on this branch by design (they depend on #15035/#15038), so the aggregation path can't be exercised live yet.
Manual QA could not run: no kpub content is seeded in this dev DB and the sibling PRs that make aggregation observable aren't present. No new strings, styles, or a11y surface. CI passing.
Three behavioral questions worth confirming before merge (all suggestions, none blocking) — see inline comments. Verdict is COMMENT because live QA of the changed path wasn't possible here.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| this.$emit('updateProgress', progress); | ||
|
|
||
| if (progress >= 1) { | ||
| if (progress >= 1 && this.allSourcesComplete && !this.hasEmittedFinished) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The finished emit is now gated by allSourcesComplete on both progress paths, which introduces two behavior changes worth confirming:
-
Duration-based path. Under
forceDurationBasedProgress,progresscomes fromdurationBasedProgressand ignores embedded viewers entirely (line 200), yet completion is now blocked until every embedded viewer has emittedfinished. Previously duration completion fired independently. An article with duration-based progress plus a non-completing embed would reach 100% displayed progress via elapsed time but never emitfinished. Consider gatingallSourcesCompleteonly on the scroll path. -
completerequires an explicitfinishedemit. A viewer reaching progress1viaupdateProgress(1)/addProgress— but not emittingfinished— keepscomplete: false(onlyhandleViewerFinishedsets it, :188). SoaggregatedProgresscan hit1.0whileallSourcesCompletestays false: article shows 100% but never completes (no completion event, no points). Confirm every embeddable viewer reliably emitsfinishedat end-of-content, or derivecompletefromprogress >= 1. The spec covers thefinished-driven path but not this one.
| const avgViewerProgress = totalViewerProgress / this.viewerCount; | ||
|
|
||
| // Dynamic weighting: 50% scroll, 50% viewers | ||
| return (this.scrollBasedProgress + avgViewerProgress) / 2; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Because progress averages over the current viewer set, a viewer registering mid-session lowers the average and pushes the emitted updateProgress value down (e.g. reader ~1 with viewer-1, then viewer-2 lazy-loads at 0 → drops to ~0.5). The backend keeps the max so learner-facing progress won't regress, but the emitted stream is non-monotonic. If embeds can mount lazily after scroll, this will happen in practice — worth seeding late registrants or noting the intent.
f3a1cc7 to
bf3b43a
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15037 — delta re-review of embedded-progress aggregation. Both prior findings are settled (see collapsed block); the rework to derive completion from progress >= 1 is clean and the new spec covers the previously-flagged behaviors. Two non-blocking items on the newly changed code below.
Manual QA was required for this PR but did not run this cycle, so verdict is COMMENT rather than APPROVE — no UI verification was performed. CI passing.
Prior-finding status
RESOLVED — SafeHtml5RendererIndex.vue — duration-based completion no longer gated by embedded viewers (allSourcesComplete design removed; spec :212)
ACKNOWLEDGED — SafeHtml5RendererIndex.vue:95 — non-monotonic emitted progress on mid-session viewer registration (documented at :93-94; backend keeps max)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| if (progress >= 1) { | ||
| // Aggregated progress only reaches 1 when scroll and every embedded viewer are at 1 | ||
| if (progress >= 1 && !this.hasEmittedFinished) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: hasEmittedFinished latches on the first progress >= 1. When viewerCount === 0, aggregatedProgress === scrollBasedProgress, so a non-overflowing article (scrollBasedProgress = 1 immediately) or one scrolled to the bottom before any embed emits startTracking fires finished and locks the guard — embeds that lazy-mount afterward can no longer gate completion, contradicting the intent stated at :194. Backend keeps the max so nothing regresses, but the article is marked complete without the embeds ever counting. Is scroll-only completion acceptable when embeds may lazy-load, or should the emit wait for at least one poll cycle / a stabilized viewerCount?
| it('emits finished when scroll is complete and no embedded viewers exist', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| simulateFullScroll(); | ||
| // scroll=1, no viewers, aggregated=1, allSourcesComplete=true |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: This comment (and :200) annotate the assertion with allSourcesComplete=true, but that symbol no longer exists — completion is now derived from progress >= 1. Update to match the current model so a future reader isn't sent looking for a variable that isn't there.
ContentViewer now generates unique viewer IDs and appends them to all interaction events, allowing parent components to track which embedded viewer emitted each event. SafeHtml5RendererIndex uses this to aggregate progress from multiple embedded viewers (e.g., videos in HTML5 content), combining scroll-based progress with embedded viewer progress using dynamic weighting.
bf3b43a to
c20e16f
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15037 — delta re-review. All 4 prior findings resolved or acknowledged; no new findings on the amended code. CI passing. Not approving because required manual QA did not run this cycle (dev server did not start), so the UI is unverified.
Prior-finding status
RESOLVED — SafeHtml5RendererIndex.vue:196 — finished emit gated by allSourcesComplete (design removed)
RESOLVED — SafeHtml5RendererIndex.vue:206 — duration-based completion gated by embedded viewers
RESOLVED — SafeHtml5RendererIndex.vue:195 — hasEmittedFinished latching / scroll-only completion documented at :194-195
ACKNOWLEDGED — SafeHtml5RendererIndex.vue:93 — non-monotonic emitted progress on mid-session viewer registration (documented; backend keeps max)
RESOLVED — SafeHtml5RendererProgressAggregation.spec.js:188 — stale allSourcesComplete=true annotations removed
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
marcellamaki
left a comment
marcellamaki
left a comment
There was a problem hiding this comment.
no real concerns, just a couple of clarifying questions. the progress approach seems reasonable. i think there will be cases where it doesn't totally "make sense" with the content, but it seems like a very balanced general approach that will work for most scenarios in the ways that it needs to for learners and teachers.
| expect(getLastEmittedProgress(emitted)).toBe(0.25); | ||
| }); | ||
|
|
||
| it('clamps negative progress to 0', async () => { |
There was a problem hiding this comment.
does this mean if i am on a video player and i have watched 50% of it, and I restart, it doesn't subtract it? I don't think so.... I think it just means that if I somehow get negative total progress i can't go less than zero, like setting a lower bound. right?
There was a problem hiding this comment.
Yes, basically, if there's a weird error and for some reason a renderer emits a progress < 0.
| expect(getLastEmittedProgress(emitted)).toBe(0.5); | ||
| }); | ||
|
|
||
| it('reverts to scroll-only progress after viewer unregisters', async () => { |
There was a problem hiding this comment.
this one also feels a bit unclear to me. maybe it's because i can't quite keep track of the where the registering and unregistering is managed and I think i might be conflating it with rendering. I think it's within the child (responsible for unregistering itself) but I am having a bit of trouble putting the pieces together about what this means practically -- a scenario where this happens with a particular content node and what that means for the progress for the user and UX
There was a problem hiding this comment.
Yeah, the only case where this would occur is again, if there was an unrecoverable error in the content viewer, and it implodes - so this makes sure that you aren't stuck not being able to complete the article because of a dodgy resource.
4 participants
Summary
References
Contributes to #13824.
Reviewer guidance
SafeHtml5RendererIndex.vue:93— aggregated progress is(scroll + average embedded-viewer progress) / 2.SafeHtml5RendererIndex.vue:150— embedded viewers register byviewerId; confirm ids are unique per viewer instance.Screenshots
Pulled from the combined implementation PR #14548 (this PR is a split of that work). Aggregated progress is only observable when the article renders live embedded viewers, which needs the media-player and toolbar PRs (#15035, #15038) present — so it is not completely reproducible from this branch until all three splits merge.
AI usage
Used Claude Code to implement the embedded-viewer progress aggregation and isolate it as a standalone commit. Verified with the progress-aggregation Jest spec and prek.