Initial setup for HTML Article Renderer plugin #13274
Conversation
Build Artifacts
|
rtibbles
force-pushed
the
html_article_renderer
branch
from
0f72a24 to
9da0943
Compare
github-actions
bot
added
DEV: renderers
rtibbles
force-pushed
the
html_article_renderer
branch
3 times, most recently
from
f2c2546 to
dd41f25
Compare
rtibbles
force-pushed
the
html_article_renderer
branch
from
dd41f25 to
bda0c45
Compare
rtibbles
force-pushed
the
html_article_renderer
branch
from
bda0c45 to
563befa
Compare
rtibbles
force-pushed
the
html_article_renderer
branch
from
563befa to
970b811
Compare
WalkthroughThis update introduces a new internal plugin, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SafeHtml5RendererIndex (Vue)
participant kolibri-zip
participant SafeHTML (Vue)
User ->> SafeHtml5RendererIndex (Vue): Mount component with storageUrl
SafeHtml5RendererIndex (Vue) ->> kolibri-zip: Open zip archive from storageUrl
kolibri-zip -->> SafeHtml5RendererIndex (Vue): Return zip contents
SafeHtml5RendererIndex (Vue) ->> SafeHtml5RendererIndex (Vue): Extract and read entry HTML file
SafeHtml5RendererIndex (Vue) ->> SafeHTML (Vue): Pass HTML content
SafeHTML (Vue) ->> SafeHTML (Vue): Sanitize HTML with DOMPurify
SafeHTML (Vue) -->> SafeHtml5RendererIndex (Vue): Render sanitized HTML
SafeHtml5RendererIndex (Vue) -->> User: Display HTML content
SafeHtml5RendererIndex (Vue) ->> SafeHtml5RendererIndex (Vue): Poll for progress
SafeHtml5RendererIndex (Vue) -->> User: Emit progress events (startTracking, updateProgress, finished, stopTracking)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
packages/kolibri-common/components/SafeHTML/index.js (2)
32-44: Consider preserving existing class attributes instead of overwriting them.Currently, line 38 sets the class attribute to 'safe-html', which will overwrite any existing classes on elements in the sanitized HTML. Consider preserving existing classes:
- attributes.class = 'safe-html'; + attributes.class = (attributes.class ? attributes.class + ' safe-html' : 'safe-html');
26-31: Add error handling for HTML parsing.The component doesn't include error handling if
parseFromStringfails or returns invalid content. Consider adding try/catch blocks or validation of the parsed document.- const doc = parser.parseFromString(sanitizedHTML, 'text/html'); + let doc; + try { + doc = parser.parseFromString(sanitizedHTML, 'text/html'); + // Verify parsing was successful + if (doc.body === null) { + return []; + } + } catch (e) { + console.error('Error parsing HTML:', e); + return []; + }kolibri/plugins/safe_html5_viewer/assets/src/views/SafeHtml5RendererIndex.vue (3)
80-84: Improve polling mechanism by adding a property for polling interval.The polling interval is hardcoded to 5000ms. It would be better to define this as a property to make it configurable.
+ computed: { + // Other computed properties... + pollingInterval() { + return (this.options && this.options.pollingInterval) || 5000; + }, + }, methods: { // Other methods... pollProgress() { this.timeout = setTimeout(() => { this.recordProgress(); - }, 5000); + }, this.pollingInterval); }, },
49-56: Initialize startTime in the created lifecycle hook.If you implement the durationBasedProgress as suggested, you should initialize the startTime in the created hook rather than in data.
async created() { + this.startTime = Date.now(); try { const storageUrl = this.defaultFile.storage_url; const zipFile = new ZipFile(storageUrl); const entryHtmlFile = await zipFile.file(this.entry); this.html = entryHtmlFile.toString(); this.loading = false; this.$emit('startTracking'); this.pollProgress(); } catch (error) { console.error('Error loading HTML content:', error); this.loading = false; } },
4-12: Consider adding accessibility attributes to the loader.For better accessibility, you might want to add an aria-label to the loader.
<KCircularLoader v-if="loading || !html" :delay="false" class="loader" + aria-label="Loading content" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
kolibri/__init__.py(1 hunks)kolibri/core/content/migrations/0040_html_article_constants.py(1 hunks)kolibri/plugins/safe_html5_viewer/assets/src/module.js(1 hunks)kolibri/plugins/safe_html5_viewer/assets/src/views/SafeHtml5RendererIndex.vue(1 hunks)kolibri/plugins/safe_html5_viewer/buildConfig.js(1 hunks)kolibri/plugins/safe_html5_viewer/kolibri_plugin.py(1 hunks)packages/kolibri-common/components/SafeHTML/index.js(1 hunks)packages/kolibri-common/components/SafeHTML/style.scss(1 hunks)packages/kolibri-common/package.json(1 hunks)requirements/base.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: Morango Integration Tests for currently supported Python versions (3.13)
- GitHub Check: Morango Integration Tests for currently supported Python versions (3.12)
- GitHub Check: Morango Integration Tests for EOL Python versions (3.8)
- GitHub Check: Morango Integration Tests for currently supported Python versions (3.11)
- GitHub Check: Morango Integration Tests for EOL Python versions (3.7)
- GitHub Check: Licenses check
- GitHub Check: Morango Integration Tests for currently supported Python versions (3.10)
- GitHub Check: Morango Integration Tests for EOL Python versions (3.6)
- GitHub Check: Morango Integration Tests for currently supported Python versions (3.9)
- GitHub Check: Python unit tests for currently supported Python versions (3.13)
- GitHub Check: Python unit tests for currently supported Python versions (3.12)
- GitHub Check: No zombies
- GitHub Check: Morango Integration Tests with PostgreSQL
- GitHub Check: Python unit tests for EOL Python versions (3.8)
- GitHub Check: Python unit tests for currently supported Python versions (3.11)
- GitHub Check: Python unit tests on Windows Server (3.8)
- GitHub Check: Python unit tests for EOL Python versions (3.7)
- GitHub Check: Python unit tests for currently supported Python versions (3.10)
- GitHub Check: Python unit tests for EOL Python versions (3.6)
- GitHub Check: Python unit tests for currently supported Python versions (3.9)
- GitHub Check: Python postgres unit tests
- GitHub Check: Frontend tests
- GitHub Check: Python unit tests on Mac OS (3.10)
- GitHub Check: All file linting
- GitHub Check: Build WHL file / Build WHL
🔇 Additional comments (10)
packages/kolibri-common/package.json (1)
13-13: Verify DOMPurify dependency and lockfile updates.Ensure that adding
"dompurify": "^3.2.4"is intentional and that your lockfile (yarn.lock/package-lock.json) has been updated accordingly. Also confirm that the installed version of DOMPurify is compatible with your Vue version and the build pipeline.requirements/base.txt (1)
13-13:✅ Verification successful
Confirm
le-utilsversion availability.Verify that
le-utils==0.2.10has been published on PyPI and matches the new content format presets required by the plugin. You can run:and check that
"0.2.10"appears in the list.
🏁 Script executed:
#!/bin/bash curl -s https://pypi.org/pypi/le-utils/json | jq '.releases | keys'Length of output: 1195
le-utils==0.2.10 is available on PyPI
Verified viacurl -s https://pypi.org/pypi/le-utils/json | jq '.releases | keys'—“0.2.10” is listed.
No changes needed.kolibri/__init__.py (1)
43-45:✅ Verification successful
Approve plugin registration; verify file presence.
The entry
"kolibri.plugins.safe_html5_viewer"is correctly appended toINTERNAL_PLUGINS. Ensure thatkolibri/plugins/safe_html5_viewer/kolibri_plugin.pyexists and exports the expected plugin class.Run:
to confirm the plugin file is present.
🏁 Script executed:
#!/bin/bash fd kolibri_plugin.py kolibri/plugins/safe_html5_viewerLength of output: 108
🏁 Script executed:
#!/bin/bash grep -R "class " -n kolibri/plugins/safe_html5_viewer/kolibri_plugin.pyLength of output: 183
Plugin registration verified
kolibri/plugins/safe_html5_viewer/kolibri_plugin.pyexists.- Defines
class SafeHTML5AppPlugin(KolibriPluginBase), confirming the expected plugin export.No further changes required.
kolibri/plugins/safe_html5_viewer/buildConfig.js (1)
1-6:❓ Verification inconclusive
Validate build configuration.
The
bundle_idandentrylook correct for bundling your plugin’s assets. Please verify that your build system picks up this config and that any required loaders (e.g., for Vue SFCs) are registered elsewhere in the project.You can confirm the bundle reference with:
🏁 Script executed:
#!/bin/bash rg "bundle_id: 'main'" -n kolibri/plugins/safe_html5_viewerLength of output: 139
Verify webpack integration and loader configuration
- Confirm that
kolibri/plugins/safe_html5_viewer/buildConfig.jsis picked up by your project’s main webpack setup (e.g. inwebpack.config.jsor the plugin loader).- Ensure the
entry: './assets/src/module.js'path resolves correctly at build time.- If you’re bundling Vue Single-File Components, verify that a rule for
.vuefiles (usingvue-loader) is registered in your webpack rules.kolibri/plugins/safe_html5_viewer/assets/src/module.js (1)
1-12: Implementation looks good and follows the Kolibri renderer module pattern.The module structure correctly extends ContentRendererModule, provides the appropriate renderer component, and exports an instance following Kolibri's conventions.
kolibri/plugins/safe_html5_viewer/kolibri_plugin.py (1)
1-15: Plugin structure and hook registration look correct.The plugin class and content renderer hook are properly defined following Kolibri's plugin architecture. The hook correctly associates the renderer with the KPUB_ZIP preset content type.
packages/kolibri-common/components/SafeHTML/index.js (2)
4-15: Good security configuration for DOMPurify.The configuration correctly restricts potentially dangerous elements like style tags and attributes, which helps prevent XSS attacks.
4-4:❓ Verification inconclusive
Verify if the URI restriction is intentionally strict.
The current regex only allows blob URLs. Is this intentional? If the component needs to support other URL schemes (http, https, data), the regex should be expanded.
🏁 Script executed:
#!/bin/bash # Check where this component is used to understand URL requirements echo "Searching for imports or uses of SafeHTML component:" rg -A 3 "SafeHTML" --type js --type vue echo "\nSearching for potential URI usage patterns in HTML content:" rg -A 2 "src=|href=" --type htmlLength of output: 4336
I need to inspect the SafeHTML implementation to confirm which schemes are allowed versus how it’s used elsewhere. Let me pull in the relevant portion of packages/kolibri-common/components/SafeHTML/index.js.
Let’s pull in the SafeHTML implementation so we can see how the
ALLOWED_URI_REGEXPis used and whether other schemes are intentionally excluded:
🏁 Script executed:
#!/bin/bash # Show the regex definition and its surrounding logic: rg -C5 "const ALLOWED_URI_REGEXP" -n packages/kolibri-common/components/SafeHTML/index.js # Show all references to ALLOWED_URI_REGEXP in that file: rg -C5 "ALLOWED_URI_REGEXP" -n packages/kolibri-common/components/SafeHTML/index.js # Find all imports/usages of SafeHTML across JS/TS files: rg -n "SafeHTML" --glob "*.{js,ts}" .Length of output: 1320
Confirm blob-only URI restriction
The
ALLOWED_URI_REGEXPis currently set to/^blob:https?:/i, which only permitsblob:http:andblob:https:URLs. If you intend for SafeHTML to support other schemes (e.g.http://,https://,data:), please extend this regex accordingly.• packages/kolibri-common/components/SafeHTML/index.js:4
kolibri/core/content/migrations/0040_html_article_constants.py (1)
1-85: Migration correctly expands content type presets and file extensions.The migration properly adds support for the KPUB format while maintaining existing content types. The structure follows Django migration best practices.
kolibri/plugins/safe_html5_viewer/assets/src/views/SafeHtml5RendererIndex.vue (1)
93-97: LGTM! The loader positioning is appropriately centered.The CSS for positioning the loader is correctly implemented using absolute positioning with calculated offsets.
| } else { | ||
| // TODO: Handle progress tracking based on how | ||
| // much of the article has been scrolled through | ||
| progress = this.scrollBasedProgress; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement scroll-based progress tracking.
There's a TODO comment about scroll-based progress tracking, but it's currently hardcoded to 0.5.
Consider implementing a proper scroll-based progress calculation:
- // TODO: Handle progress tracking based on how
- // much of the article has been scrolled through
- progress = this.scrollBasedProgress;
+ progress = this.calculateScrollProgress();And add a method to calculate the scroll progress:
calculateScrollProgress() {
if (!this.$el) return 0.5;
const scrollHeight = document.documentElement.scrollHeight - window.innerHeight;
if (scrollHeight <= 0) return 1;
const scrollPosition = window.scrollY;
return Math.min(scrollPosition / scrollHeight, 1);
}Don't forget to add a scroll event listener in the mounted hook:
mounted() {
window.addEventListener('scroll', this.handleScroll);
},
beforeDestroy() {
if (this.timeout) {
clearTimeout(this.timeout);
}
window.removeEventListener('scroll', this.handleScroll);
this.$emit('stopTracking');
},
methods: {
handleScroll() {
// Optionally debounce this for performance
this.currentScrollProgress = this.calculateScrollProgress();
},
// other methods...
}| async created() { | ||
| const storageUrl = this.defaultFile.storage_url; | ||
| const zipFile = new ZipFile(storageUrl); | ||
| const entryHtmlFile = await zipFile.file(this.entry); | ||
| this.html = entryHtmlFile.toString(); | ||
| this.loading = false; | ||
| this.$emit('startTracking'); | ||
| this.pollProgress(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for zip file operations.
The code currently doesn't handle potential errors when loading the zip file or extracting the HTML file. If the zip file can't be found or doesn't contain the specified entry, this will result in unhandled promise rejections.
Consider adding try/catch blocks for error handling:
async created() {
+ try {
const storageUrl = this.defaultFile.storage_url;
const zipFile = new ZipFile(storageUrl);
const entryHtmlFile = await zipFile.file(this.entry);
this.html = entryHtmlFile.toString();
this.loading = false;
this.$emit('startTracking');
this.pollProgress();
+ } catch (error) {
+ console.error('Error loading HTML content:', error);
+ this.loading = false;
+ // Consider adding user-facing error state
+ }
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async created() { | |
| const storageUrl = this.defaultFile.storage_url; | |
| const zipFile = new ZipFile(storageUrl); | |
| const entryHtmlFile = await zipFile.file(this.entry); | |
| this.html = entryHtmlFile.toString(); | |
| this.loading = false; | |
| this.$emit('startTracking'); | |
| this.pollProgress(); | |
| async created() { | |
| try { | |
| const storageUrl = this.defaultFile.storage_url; | |
| const zipFile = new ZipFile(storageUrl); | |
| const entryHtmlFile = await zipFile.file(this.entry); | |
| this.html = entryHtmlFile.toString(); | |
| this.loading = false; | |
| this.$emit('startTracking'); | |
| this.pollProgress(); | |
| } catch (error) { | |
| console.error('Error loading HTML content:', error); | |
| this.loading = false; | |
| // Consider adding user-facing error state | |
| } | |
| }, |
| recordProgress() { | ||
| let progress; | ||
| if (this.forceDurationBasedProgress) { | ||
| progress = this.durationBasedProgress; | ||
| } else { | ||
| // TODO: Handle progress tracking based on how | ||
| // much of the article has been scrolled through | ||
| progress = this.scrollBasedProgress; | ||
| } |
There was a problem hiding this comment.
Implement the missing progress tracking variables.
The recordProgress method uses this.forceDurationBasedProgress and this.durationBasedProgress which are not defined in the component.
You need to define these variables either as props or computed properties:
computed: {
/**
* @public
* Note: the default duration historically for HTML5 Apps has been 5 min
*/
defaultDuration() {
return 300;
},
entry() {
return (this.options && this.options.entry) || 'index.html';
},
scrollBasedProgress() {
return 0.5;
},
+ forceDurationBasedProgress() {
+ return this.options && this.options.forceDurationBasedProgress;
+ },
+ durationBasedProgress() {
+ // Calculate progress based on time elapsed
+ const elapsedTime = (Date.now() - this.startTime) / 1000;
+ return Math.min(elapsedTime / this.defaultDuration, 1);
+ },
},
+ data() {
+ return {
+ loading: true,
+ html: null,
+ startTime: Date.now(),
+ };
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| recordProgress() { | |
| let progress; | |
| if (this.forceDurationBasedProgress) { | |
| progress = this.durationBasedProgress; | |
| } else { | |
| // TODO: Handle progress tracking based on how | |
| // much of the article has been scrolled through | |
| progress = this.scrollBasedProgress; | |
| } | |
| <script> | |
| export default { | |
| data() { | |
| return { | |
| loading: true, | |
| html: null, | |
| startTime: Date.now(), | |
| }; | |
| }, | |
| computed: { | |
| /** | |
| * @public | |
| * Note: the default duration historically for HTML5 Apps has been 5 min | |
| */ | |
| defaultDuration() { | |
| return 300; | |
| }, | |
| entry() { | |
| return (this.options && this.options.entry) || 'index.html'; | |
| }, | |
| scrollBasedProgress() { | |
| return 0.5; | |
| }, | |
| forceDurationBasedProgress() { | |
| return this.options && this.options.forceDurationBasedProgress; | |
| }, | |
| durationBasedProgress() { | |
| // Calculate progress based on time elapsed | |
| const elapsedTime = (Date.now() - this.startTime) / 1000; | |
| return Math.min(elapsedTime / this.defaultDuration, 1); | |
| }, | |
| }, | |
| methods: { | |
| recordProgress() { | |
| let progress; | |
| if (this.forceDurationBasedProgress) { | |
| progress = this.durationBasedProgress; | |
| } else { | |
| // TODO: Handle progress tracking based on how | |
| // much of the article has been scrolled through | |
| progress = this.scrollBasedProgress; | |
| } | |
| // …rest of recordProgress… | |
| }, | |
| // …other methods… | |
| }, | |
| }; | |
| </script> |
| computed: { | ||
| /** | ||
| * @public | ||
| * Note: the default duration historically for HTML5 Apps has been 5 min | ||
| */ | ||
| defaultDuration() { | ||
| return 300; | ||
| }, | ||
| entry() { | ||
| return (this.options && this.options.entry) || 'index.html'; | ||
| }, |
There was a problem hiding this comment.
Define missing props used in component.
The component uses this.defaultFile and this.options which are not defined as props in the component.
Add the missing props to ensure the component works correctly:
export default {
name: 'SafeHtml5RendererIndex',
components: {
SafeHTML,
},
+ props: {
+ defaultFile: {
+ type: Object,
+ required: true,
+ },
+ options: {
+ type: Object,
+ default: () => ({}),
+ },
+ },
data() {
return {
loading: true,
html: null,
};
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| computed: { | |
| /** | |
| * @public | |
| * Note: the default duration historically for HTML5 Apps has been 5 min | |
| */ | |
| defaultDuration() { | |
| return 300; | |
| }, | |
| entry() { | |
| return (this.options && this.options.entry) || 'index.html'; | |
| }, | |
| export default { | |
| name: 'SafeHtml5RendererIndex', | |
| components: { | |
| SafeHTML, | |
| }, | |
| props: { | |
| defaultFile: { | |
| type: Object, | |
| required: true, | |
| }, | |
| options: { | |
| type: Object, | |
| default: () => ({}), | |
| }, | |
| }, | |
| data() { | |
| return { | |
| loading: true, | |
| html: null, | |
| }; | |
| }, | |
| computed: { | |
| /** | |
| * @public | |
| * Note: the default duration historically for HTML5 Apps has been 5 min | |
| */ | |
| defaultDuration() { | |
| return 300; | |
| }, | |
| entry() { | |
| return (this.options && this.options.entry) || 'index.html'; | |
| }, | |
| // …other computed props | |
| }, | |
| // …rest of component | |
| }; |
Summary
References
This will require learningequality/le-utils#163 to be merged and a new version released, then for the version update to be propagated here, in order for this to be fully functional.
Reviewer guidance
To test this, the new plugin needs to be explicitly enabled using the command line:
Then you can import the Kolibri QA Channel, specifically the All About Hummingbirds HTML5 resource, which uses this new renderer.
Summary by CodeRabbit
New Features
Enhancements
Chores