Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,14 @@
role="region"
:aria-label="$tr('articleContent')"
>
<SafeHTML :html="html" />
<SafeHTML
:html="html"
@startTracking="handleViewerStartTracking"
@stopTracking="handleViewerStopTracking"
@updateProgress="handleViewerUpdateProgress"
@addProgress="handleViewerAddProgress"
@finished="handleViewerFinished"
/>
</div>
</div>

Expand Down Expand Up @@ -52,12 +59,41 @@
html: null,
scrollBasedProgress: 0,
debouncedHandleScroll: null,
// Track embedded viewers for progress aggregation
// Using object instead of Map for Vue 2.7 reactivity
// Structure: { viewerId: { progress: number } }
embeddedViewers: {},
// Guard to prevent emitting 'finished' multiple times
hasEmittedFinished: false,
};
},
computed: {
entry() {
return (this.options && this.options.entry) || 'index.html';
},
// Count of registered embedded viewers
viewerCount() {
return Object.keys(this.embeddedViewers).length;
},
// Aggregated progress using dynamic weighting
// If no embedded viewers: progress = scrollBasedProgress
// If viewers exist: progress = (scrollBasedProgress + avgViewerProgress) / 2
aggregatedProgress() {
if (this.viewerCount === 0) {
return this.scrollBasedProgress;
}

let totalViewerProgress = 0;
for (const viewer of Object.values(this.embeddedViewers)) {
totalViewerProgress += viewer.progress;
}
const avgViewerProgress = totalViewerProgress / this.viewerCount;

// Dynamic weighting: 50% scroll, 50% viewers
// A live average, so a viewer registering mid-session lowers it; the backend
// keeps the maximum, so learner-facing progress never regresses.
return (this.scrollBasedProgress + avgViewerProgress) / 2;

@rtibblesbot rtibblesbot Jul 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

},
},
async created() {
const storageUrl = this.defaultFile.storage_url;
Expand Down Expand Up @@ -100,18 +136,66 @@
}
});
},

// Handle startTracking from embedded viewers
handleViewerStartTracking(viewerId) {
if (viewerId && !this.embeddedViewers[viewerId]) {
this.$set(this.embeddedViewers, viewerId, { progress: 0 });
}
},

// Handle stopTracking from embedded viewers
handleViewerStopTracking(viewerId) {
if (viewerId) {
this.$delete(this.embeddedViewers, viewerId);
}
},

// Handle updateProgress from embedded viewers
handleViewerUpdateProgress(progress, viewerId) {
if (viewerId && this.embeddedViewers[viewerId]) {
this.$set(this.embeddedViewers, viewerId, {
...this.embeddedViewers[viewerId],
progress: Math.min(1, Math.max(0, progress)),
});
}
},

// Handle addProgress from embedded viewers
handleViewerAddProgress(delta, viewerId) {
if (viewerId) {
const viewer = this.embeddedViewers[viewerId];
if (viewer) {
const newProgress = Math.min(1, Math.max(0, viewer.progress + delta));
this.$set(this.embeddedViewers, viewerId, {
...viewer,
progress: newProgress,
});
}
}
},

// Handle finished from embedded viewers
handleViewerFinished(viewerId) {
if (viewerId && this.embeddedViewers[viewerId]) {
this.$set(this.embeddedViewers, viewerId, { progress: 1 });
}
},

recordProgress() {
let progress;
if (this.forceDurationBasedProgress) {
progress = this.durationBasedProgress;
} else {
// Use scroll events to track progress
progress = this.scrollBasedProgress;
// Use aggregated progress from scroll + embedded viewers
progress = this.aggregatedProgress;
}
this.$emit('updateProgress', progress);

if (progress >= 1) {
// Scroll-to-bottom completes the article; embedded viewers
// registered at that point must also be at 1.
if (progress >= 1 && !this.hasEmittedFinished) {

@rtibblesbot rtibblesbot Jul 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

this.$emit('finished');
this.hasEmittedFinished = true;
}
this.pollProgress();
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,234 @@
import { render, screen } from '@testing-library/vue';
import { createTranslator } from 'kolibri/utils/i18n';
// eslint-disable-next-line import-x/named
import useContentViewer, { useContentViewerMock } from 'kolibri/composables/useContentViewer';
import SafeHtml5RendererIndex from '../SafeHtml5RendererIndex.vue';

const { articleContent$ } = createTranslator(
SafeHtml5RendererIndex.name,
SafeHtml5RendererIndex.$trs,
);

let mockEmitFromSafeHTML;

jest.mock('kolibri', () => ({
canHandleElement: jest.fn(() => false),
}));

jest.mock('kolibri/composables/useContentViewer');

// Mock SafeHTML to render html content and provide controllable event emission
// for testing how the parent aggregates progress from embedded viewers.
jest.mock('kolibri-common/components/SafeHTML', () => ({
createSafeHTML: () => ({
name: 'SafeHTML',
props: { html: String },
created() {
mockEmitFromSafeHTML = (event, ...args) => this.$emit(event, ...args);
},
render(h) {
return h('div', { domProps: { innerHTML: this.html || '' } });
},
}),
}));

jest.mock('kolibri-common/components/SafeHTML/style.scss', () => ({}));
jest.mock('kolibri-zip', () => {
return jest.fn().mockImplementation(() => ({
file: jest.fn().mockResolvedValue({
toString: () => '<h1>Mocked HTML content</h1>',
}),
}));
});

const renderComponent = () => {
return render(SafeHtml5RendererIndex);
};

describe('SafeHtml5RendererIndex progress aggregation', () => {
beforeEach(() => {
useContentViewer.mockImplementation(() =>
useContentViewerMock({ defaultFile: { storage_url: 'mock://test.html' } }),
);
jest.useFakeTimers();
mockEmitFromSafeHTML = null;
});

afterEach(() => {
jest.useRealTimers();
});

async function renderAndLoad() {
const result = renderComponent();
await screen.findByRole('region', { name: articleContent$() });
return result;
}

function getLastEmittedProgress(emitted) {
jest.advanceTimersByTime(5000);
const events = emitted().updateProgress;
return events[events.length - 1][0];
}

// In jsdom, scrollHeight and clientHeight are both 0, so maxScroll = 0
// and handleScroll sets scrollBasedProgress = 1 (content considered fully read).
function simulateFullScroll() {
const wrapper = document.querySelector('[data-testid="safe-html-wrapper"]');
wrapper.dispatchEvent(new Event('scroll'));
jest.advanceTimersByTime(150); // flush debounce
}

it('reports scroll-based progress when no embedded viewers exist', async () => {
const { emitted } = await renderAndLoad();
expect(getLastEmittedProgress(emitted)).toBe(0);
});

it('averages scroll and viewer progress with one viewer', async () => {
const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('startTracking', 'viewer-1');
mockEmitFromSafeHTML('updateProgress', 0.5, 'viewer-1');
// scroll=0, viewer=0.5, aggregated=(0+0.5)/2=0.25
expect(getLastEmittedProgress(emitted)).toBe(0.25);
});

it('averages scroll and average viewer progress with multiple viewers', async () => {
const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('startTracking', 'viewer-1');
mockEmitFromSafeHTML('startTracking', 'viewer-2');
mockEmitFromSafeHTML('updateProgress', 0.25, 'viewer-1');
mockEmitFromSafeHTML('updateProgress', 0.75, 'viewer-2');
// scroll=0, viewer avg=(0.25+0.75)/2=0.5, aggregated=(0+0.5)/2=0.25
expect(getLastEmittedProgress(emitted)).toBe(0.25);
});

it('accumulates delta progress via addProgress', async () => {
const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('startTracking', 'viewer-1');
mockEmitFromSafeHTML('addProgress', 0.25, 'viewer-1');
mockEmitFromSafeHTML('addProgress', 0.25, 'viewer-1');
// viewer=0.5, scroll=0, aggregated=(0+0.5)/2=0.25
expect(getLastEmittedProgress(emitted)).toBe(0.25);
});

it('clamps negative progress to 0', async () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, basically, if there's a weird error and for some reason a renderer emits a progress < 0.

const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('startTracking', 'viewer-1');
mockEmitFromSafeHTML('updateProgress', -0.5, 'viewer-1');
// clamped to 0, aggregated=(0+0)/2=0
expect(getLastEmittedProgress(emitted)).toBe(0);
});

it('clamps progress above 1 to 1', async () => {
const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('startTracking', 'viewer-1');
mockEmitFromSafeHTML('updateProgress', 1.5, 'viewer-1');
// clamped to 1, aggregated=(0+1)/2=0.5
expect(getLastEmittedProgress(emitted)).toBe(0.5);
});

it('reverts to scroll-only progress after viewer unregisters', async () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('startTracking', 'viewer-1');
mockEmitFromSafeHTML('updateProgress', 0.8, 'viewer-1');
mockEmitFromSafeHTML('stopTracking', 'viewer-1');
// no viewers left, progress=scrollBasedProgress=0
expect(getLastEmittedProgress(emitted)).toBe(0);
});

it('does not re-register an already registered viewer', async () => {
const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('startTracking', 'viewer-1');
mockEmitFromSafeHTML('updateProgress', 0.5, 'viewer-1');
// Re-registering should not reset progress
mockEmitFromSafeHTML('startTracking', 'viewer-1');
// Still one viewer at 0.5, aggregated=(0+0.5)/2=0.25
expect(getLastEmittedProgress(emitted)).toBe(0.25);
});

it('ignores events without a viewerId', async () => {
const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('startTracking', null);
mockEmitFromSafeHTML('updateProgress', 0.5, null);
mockEmitFromSafeHTML('addProgress', 0.5, null);
mockEmitFromSafeHTML('finished', null);
mockEmitFromSafeHTML('stopTracking', null);
// No viewers registered, progress=scroll=0
expect(getLastEmittedProgress(emitted)).toBe(0);
});

it('ignores updates to unregistered viewers', async () => {
const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('updateProgress', 0.5, 'unknown');
mockEmitFromSafeHTML('addProgress', 0.5, 'unknown');
mockEmitFromSafeHTML('finished', 'unknown');
// No viewers, progress=scroll=0
expect(getLastEmittedProgress(emitted)).toBe(0);
});

it('does not emit finished when progress is below 1', async () => {
const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('startTracking', 'viewer-1');
mockEmitFromSafeHTML('updateProgress', 0.5, 'viewer-1');
jest.advanceTimersByTime(5000);
expect(emitted().finished).toBeUndefined();
});

it('does not emit finished when viewers are incomplete', async () => {
const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('startTracking', 'viewer-1');
mockEmitFromSafeHTML('updateProgress', 0.5, 'viewer-1');
simulateFullScroll();
// scroll=1, viewer=0.5, aggregated=(1+0.5)/2=0.75 and viewer not complete
expect(emitted().finished).toBeUndefined();
});

it('emits finished when scroll is complete and no embedded viewers exist', async () => {
const { emitted } = await renderAndLoad();
simulateFullScroll();
// scroll=1, no viewers, aggregated=1
expect(emitted().finished).toHaveLength(1);
});

it('emits finished when scroll is complete and all viewers are finished', async () => {
const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('startTracking', 'viewer-1');
mockEmitFromSafeHTML('startTracking', 'viewer-2');
mockEmitFromSafeHTML('finished', 'viewer-1');
mockEmitFromSafeHTML('finished', 'viewer-2');
simulateFullScroll();
// scroll=1, both viewers complete at progress=1
// aggregated=(1+1)/2=1
expect(emitted().finished).toHaveLength(1);
});

it('emits finished when a viewer reaches progress 1 without emitting finished', async () => {
const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('startTracking', 'viewer-1');
mockEmitFromSafeHTML('updateProgress', 1, 'viewer-1');
simulateFullScroll();
expect(emitted().finished).toHaveLength(1);
});

it('emits finished on duration-based progress regardless of embedded viewers', async () => {
useContentViewer.mockImplementation(() =>
useContentViewerMock({
defaultFile: { storage_url: 'mock://test.html' },
forceDurationBasedProgress: true,
durationBasedProgress: 1,
}),
);
const { emitted } = await renderAndLoad();
mockEmitFromSafeHTML('startTracking', 'viewer-1');
mockEmitFromSafeHTML('updateProgress', 0.5, 'viewer-1');
jest.advanceTimersByTime(5000);
expect(emitted().finished).toHaveLength(1);
});

it('emits finished only once across multiple recordProgress calls', async () => {
const { emitted } = await renderAndLoad();
simulateFullScroll(); // triggers recordProgress via handleScroll
jest.advanceTimersByTime(5000); // triggers another recordProgress via poll
jest.advanceTimersByTime(5000); // and another
expect(emitted().finished).toHaveLength(1);
});
});
Loading