Pluggable sandbox handler architecture for content viewer plugins#15036
Pluggable sandbox handler architecture for content viewer plugins#15036rtibbles wants to merge 7 commits into
Conversation
github-actions
Bot
added
DEV: dev-ops
npm Package VersionsWarning The following packages have changed files but no version bump:
If these changes affect published code, consider bumping the version. |
Build Artifacts
Smoke test screenshot |
5eff691 to
ff9c8e0
Compare
🔵 Review postedLast updated: 2026-07-24 23:59 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — pluggable sandbox handler refactor. The core layering is clean: SandboxHandler/SandboxShim carry no content-type-specific code, handlers register via SandboxedContentViewerHook, and the three viewers each ship their own handler. CI passing; manual QA confirmed HTML5/H5P/Bloom all render through the alt-origin server and the Fullscreen rename leaves the four non-sandbox viewers intact, with no new a11y violations.
Nothing hard-blocking, but several things worth addressing before merge:
- Test coverage — the new backend command + hook methods and the new
SandboxedContentViewer.vue/useSandbox.jsland with no tests, and the branch deletes the old html5 renderer smoke test — a net loss of coverage on logic-bearing code. See inline. collectsandboxstaticequivalence — last-wins copy order + dropped.file_sizesidecars diverge from whatalt_wsgiserves, contradicting the command's own docstring. Inline.- Sandbox init handshake — QA observed a recurring (8–10×/load)
TypeErrorfrom an unguardedremote.postMessageduring iframe navigation; currently caught-and-retried but the retry path is the untested case on slow hardware. Inline onmainClient.js. - Smaller items: silent-vs-loud stats handling and a dead
except KeyErrorinhooks.py; dropped xAPI validation leaving dead!s.errorfilters; unwiredMediator.destroy()(listener leak) and a permanently-nullexportedsandbox. All inline.
@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
| dest_file = os.path.join(dest_dir, filename) | ||
|
|
||
| os.makedirs(dest_dir, exist_ok=True) | ||
| shutil.copy2(src_file, dest_file) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: This diverges from what alt_wsgi.py serves, contradicting the "exact equivalence" docstring. alt_wsgi mounts every get_sandbox_static_paths() dir at one root and FileFinder resolves first-wins (core static, then plugins in registration order). This flat copy2 into one destination is last-wins — on a colliding relative path the plugin overwrites core, so the collected bundle serves a different file than the live origin. Either preserve first-wins (skip when dest_file exists) or document that collisions resolve differently. Relatedly, skipping .file_size sidecars drops data TruncatableFileEntry consults when serving — another equivalence gap if any served file relies on one.
| help="Clear the destination directory before collecting", | ||
| ) | ||
|
|
||
| def handle(self, *args, **options): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: No Python tests accompany this command or the new hook methods (get_sandbox_static_paths, sandbox_handler_url, _get_sandbox_handler_stats). The branching here (--clear rmtree, missing-source warning, zero-file CommandError) is cheap to cover against a temp source/destination and currently unverified.
| this.on(this.events.IFRAMEREADY, () => { | ||
| this.__setData(this.data, this.userData); | ||
| // Update remote reference - contentWindow may have changed after iframe navigation | ||
| this.mediator.remote = this.iframe.contentWindow; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: contentWindow is transiently null mid-navigation, so remote becomes null and the sendMessage at line 74 (and the REGISTRATION_ACK send at 121) throws TypeError: Cannot read properties of null (reading 'postMessage'). QA saw this fire 8–10× per sandboxed load on every content type — swallowed by handleMessage's try/catch and retried until contentWindow is valid, so content renders, but the handshake's correctness depends on an unguarded null-deref being caught-and-retried. On the slow hardware Kolibri targets the null window is wider, making the retry path the untested case. Suggest not assigning remote when contentWindow is null (defer the send), and/or guarding Mediator.sendMessage against a null remote.
| resolve(); | ||
| }); | ||
| } | ||
| self.storeStatements(statement); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The old sendStatement validated each statement (Statement.clean) against xAPISchema and set statement.error on failure before storing; this PR deletes xAPISchema.js and stores here without validating, so nothing ever sets s.error. That leaves the two !s.error filters in getProgress and getStatements as dead code, and for a migration scoped as behavior-preserving it silently drops the old behavior where malformed statements were flagged and excluded from progress/statement queries. Please confirm the drop is intentional; if so, remove the dead s.error reads so the code doesn't imply validation that no longer happens.
| if (isSandboxed.value) { | ||
| emit('stopTracking'); | ||
| } | ||
| sandbox = null; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: MainClient's Mediator adds a window message listener that keeps the mediator (and accumulated _shimData) alive; unmount only does sandbox = null, so navigating between sandboxed items leaks one dead listener per view. This PR adds Mediator.destroy() but nothing calls it. This composable is the natural single wire point — expose a destroy() on MainClient and call it here before nulling sandbox.
|
|
||
| // Sandbox state and methods | ||
| iframeRef, | ||
| sandbox, |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: sandbox is a let initialized to null and only reassigned inside initializeSandbox; the returned object captures the value at return time, so this exported property (re-exposed in setup.js) is permanently null and non-reactive. Everything that needs the instance uses closures, so it's harmless today but misleading for a future consumer. Drop it, or back it with a ref.
| .joinpath("{}_stats.json".format(self.sandbox_handler_unique_id)) | ||
| .read_text() | ||
| ) | ||
| except OSError: |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: _get_sandbox_handler_stats catches OSError and returns {}, so a misbuilt/missing handler yields sandbox_handler_url() == None and the viewer registers with no handler URL, failing opaquely at runtime. The parent WebpackBundleHook.get_stats instead raises WebpackError on a missing stats file. Consider matching that fail-loud behavior (at least under DEVELOPER_MODE) so a missing build is caught at render time.
| url = chunk.get("publicPath") | ||
| if url and not url.startswith("auto"): | ||
| return url | ||
| except KeyError: |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: dict.get() never raises KeyError, so this try/except KeyError is inert — likely adapted from the parent bundle property, which uses subscript access. The guard can be dropped, leaving just the if url and not url.startswith("auto") check.
| Fullscreen, | ||
| }, | ||
|
|
||
| setup(props, context) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: This component and useSandbox.js carry non-trivial logic (fullscreen wiring, duration-fallback progress emission, interval polling, user-data watching, unmount teardown) but have no *.spec.js, and the branch deletes the old Html5AppRendererIndex.spec.js smoke test — a net loss of frontend coverage. Per the unit-testing convention, add at least a render-without-throwing smoke test (asserting the fullscreen toggle/label and iframe render) plus a useSandbox test for the emitProgress duration-fallback and progress >= 1 finished branch.
ff9c8e0 to
8652c43
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — delta re-review. All 9 prior findings resolved, but the fix for the hooks.py fail-silent finding over-corrected and is the direct cause of the currently-red CI. 1 new blocking finding (see inline).
CI: failing (Python tests / postgres / 3.10) — see below. Manual QA was required but did not run; UI not verified.
Prior-finding status
RESOLVED — collectsandboxstatic.py:87 — first-wins order matches FileFinder + .file_size sidecars
RESOLVED — collectsandboxstatic.py:41 — Python tests added for command + hook methods
RESOLVED — mainClient.js:72 — null-remote postMessage guarded
RESOLVED — xAPIShim.js:303 — xAPI validation restored
RESOLVED — useSandbox.js:240 — Mediator.destroy() listener wired on unmount
RESOLVED — useSandbox.js:247 — exported sandbox no longer permanently null
UNADDRESSED — hooks.py:172 — fail-silent handler stats over-corrected to unconditional raise (new blocking below)
RESOLVED — hooks.py:192 — dead except KeyError removed
RESOLVED — SandboxedContentViewer.vue:64 — component + composable tests added
@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
| .read_text() | ||
| ) | ||
| except OSError as e: | ||
| raise WebpackError( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: This raises WebpackError unconditionally on a missing handler stats file, and it is the direct cause of the red CI. sandbox_handler_url → template_html runs during every page render that includes the content-viewer tag. The handler bundle (..._sandbox_handler_stats.json) is a separate build artifact from the plugin's main bundle and is not produced by the frontend build the Python test job runs against, so the file is legitimately absent and the raise bubbles to a 500. That's the failure in test/test_key_urls.py (test_{admin,anonymous,coach,learner,superuser}_responses, test_setup_wizard_not_redirected): WebpackError: ... No such file or directory: ...h5p_viewer/build/...sandbox_handler_stats.json → Internal Server Error: /en/setup/.
Beyond CI, any packaged build that ships a registered SandboxedContentViewerHook without its handler bundle would 500 on every page.
Scope the fail-loud to development, matching the original suggestion: raise under DEVELOPER_MODE, otherwise return {} so the tag is omitted and the page still renders (this was the graceful degradation that kept the prior round green). Then have test_missing_stats_file_raises assert under self.settings(DEVELOPER_MODE=True). Alternatively, if handler bundles are meant to always be present, build them in the test/packaging pipelines — but graceful degradation outside dev is the safer contract.
8652c43 to
d13085e
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — all 9 prior findings resolved; no new findings. CI green. Manual QA was required but did not run this pass, so this is a COMMENT rather than an approval.
Prior-finding status
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py:87 — first-wins order matches FileFinder
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py:41 — Python tests added for command + hook methods
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:72 — null-remote postMessage guarded
RESOLVED — kolibri/plugins/h5p_viewer/frontend/sandbox_handler/xAPIShim.js:303 — xAPI validation restored
RESOLVED — packages/kolibri/composables/internal/useSandbox.js:240 — Mediator.destroy() wired on unmount
RESOLVED — packages/kolibri/composables/internal/useSandbox.js:247 — exported sandbox no longer permanently null
RESOLVED — kolibri/core/content/hooks.py:172 — fail-loud raise retained (matches parent hook); page-render tests now mock sandbox_handler_url, CI green
RESOLVED — kolibri/core/content/hooks.py:192 — dead except KeyError removed
RESOLVED — packages/kolibri/components/SandboxedContentViewer/internal/SandboxedContentViewer.vue:64 — component + composable tests added
@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
|
Getting 404 errors for all 3 resource types, with 
|
d13085e to
55d3f46
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — delta re-review after the rebase onto develop (8cf0940) and recompose into 6 logical commits. All 10 prior findings remain resolved in the recomposed tree; Python CI is green. One blocking regression was introduced by the recompose: the lockfile is internally inconsistent, which is the single root cause of all five red JS-dependent jobs.
- blocking —
pnpm-lock.yaml:terser@5.48.0is referenced but never defined (see inline). Failspnpm install --frozen-lockfile. - suggestion —
handlerLoader.js: handler<script>and stale resolver aren't cleaned up (see inline).
Manual QA was required but did not run this round — no UI verification was performed; do not treat the UI as visually verified.
Prior-finding status
RESOLVED — collectsandboxstatic.py:87 — first-wins copy order mirrors FileFinder / alt_wsgi equivalence
RESOLVED — collectsandboxstatic.py:41 — command + hook tests added
RESOLVED — mainClient.js — null contentWindow/remote guarded on IFRAMEREADY
RESOLVED — xAPIShim.js:310 — xAPI statement validation restored
RESOLVED — useSandbox.js:240 — Mediator.destroy() wired on unmount
RESOLVED — useSandbox.js — sandbox assigned in initializeSandbox, no longer permanently null
RESOLVED — hooks.py — _get_sandbox_handler_stats OSError / fail-loud handling addressed
RESOLVED — hooks.py — dead except KeyError removed
RESOLVED — SandboxedContentViewer.vue:64 — component + composable tests added
RESOLVED — hooks.py:173 — fail-loud WebpackError; page-render tests patch sandbox_handler_url=None
@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
| param-case: 3.0.4 | ||
| relateurl: 0.2.7 | ||
| terser: 5.46.0 | ||
| terser: 5.48.0 |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: terser: 5.48.0 is referenced here (the html-minifier-terser@6.1.0 snapshot), but the packages/snapshots sections only define terser@5.46.0 and terser@5.49.0 — there is no terser@5.48.0 entry. pnpm install --frozen-lockfile aborts with ERR_PNPM_LOCKFILE_MISSING_DEPENDENCY ("probably caused by a badly resolved merge conflict"), which is the single root cause of all five red JS jobs (Frontend tests, Build WHL, Licenses check, WHL smoke tests, All file linting) — each fails at the install step before doing any work. Regenerate the lockfile (pnpm install --no-frozen-lockfile), commit it, and confirm a clean --frozen-lockfile install locally before pushing.
| sandbox._handlerRegistrationResolver = null; | ||
| reject(new Error(`Failed to load handler script: ${url}`)); | ||
| }; | ||
| document.head.appendChild(script); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The appended <script> and sandbox._handlerRegistrationResolver are never torn down on the success or timeout paths (only onerror nulls the resolver). On content navigation within a single SandboxEnvironment, createIframe calls loadHandler again, leaving the prior script tag in the DOM. Per the project's "whoever allocates a resource releases it" convention, remove the script in an onload/finally and clear the resolver on timeout. Low impact (handlers are per-content-type), not blocking.
55d3f46 to
d5d72c9
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — delta re-review. All 11 prior findings resolved (incl. the blocking terser@5.48.0 lockfile inconsistency, now internally consistent). One prior suggestion remains open; no new findings. CI is still pending across the Python/build matrix, and required manual QA did not run — so this stays a COMMENT rather than an approval.
- suggestion (unchanged):
handlerLoader.jsnever tears down the appended<script>or the resolver on the success/timeout paths — see inline.
Prior-finding status
RESOLVED — pnpm-lock.yaml — terser@5.48.0 referenced-but-undefined lockfile inconsistency
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py:87 — first-wins copy order mirrors FileFinder
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py:41 — command + hook tests added
RESOLVED — packages/kolibri-sandbox/src/mainClient.js — null contentWindow/remote guarded on IFRAMEREADY
RESOLVED — kolibri/plugins/h5p_viewer/frontend/sandbox_handler/xAPIShim.js:310 — xAPI statement validation restored (Statement.clean)
RESOLVED — packages/kolibri/composables/internal/useSandbox.js:240 — sandbox?.destroy() wired on unmount
RESOLVED — packages/kolibri/composables/internal/useSandbox.js — sandbox assigned in initializeSandbox
RESOLVED — kolibri/core/content/hooks.py:173 — fail-loud WebpackError on OSError
RESOLVED — kolibri/core/content/hooks.py — dead except KeyError removed
RESOLVED — packages/kolibri/components/SandboxedContentViewer/internal/SandboxedContentViewer.vue:64 — component + composable tests added
UNADDRESSED — packages/kolibri-sandbox/src/handlerLoader.js:35 — handler script/resolver not cleaned up on success/timeout
@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
| sandbox._handlerRegistrationResolver = null; | ||
| reject(new Error(`Failed to load handler script: ${url}`)); | ||
| }; | ||
| document.head.appendChild(script); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The appended <script> and sandbox._handlerRegistrationResolver are only cleaned up on the onerror path (L30-34). On the success and timeout paths the script stays in document.head and the resolver stays assigned, so navigating between content within one SandboxEnvironment re-invokes loadHandler and leaks a script tag each time. Per the project's "whoever allocates a resource releases it" convention, remove the script (e.g. in onload/a finally) and clear the resolver in the timeout branch. Low impact since handlers are per-content-type and typically loaded once — not blocking.
|
Checked all the HTML5 (H5P included) and Bloom resources in the QA channel, and all now load correctly after the fix in the latest commits. 🎉 |
🚀
d5d72c9 to
67c93b6
Compare
|
Fixed the one last review comment and then retested to be sure. |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — delta re-review. All 12 prior findings resolved (handler cleanup, mediator null-guard + destroy(), xAPI validation restored, added tests, CI lockfile + test_key_urls fixes). Five new findings on newly changed code: one important clock-sync regression plus four suggestions — see inline. No blocking issues.
Manual QA was required but did not run, and CI is still pending, so this is a COMMENT, not an approval.
Minor (folded, not inline): setup.js documents/forwards customExtractors in useSandbox but setup.js/index.js factory drop it silently; mainClient.js emits REGISTRATION_ACK that nothing consumes; hooks.py computes the core static dir two different idioms (files(...) vs import_module+dirname).
Prior-finding status
RESOLVED — collectsandboxstatic.py:87 — first-wins collision handling
RESOLVED — collectsandboxstatic.py:41 — missing Python tests
RESOLVED — mainClient.js — null contentWindow deref during navigation
RESOLVED — xAPIShim.js — dropped statement validation / dead s.error
RESOLVED — useSandbox.js:240 — Mediator.destroy() never called
RESOLVED — useSandbox.js — non-reactive sandbox return property
RESOLVED — hooks.py — _get_sandbox_handler_stats fail-loud gating
RESOLVED — hooks.py — inert try/except KeyError
RESOLVED — SandboxedContentViewer.vue:64 — missing spec
RESOLVED — hooks.py:173 — unconditional WebpackError red CI
RESOLVED — pnpm-lock.yaml — dangling terser@5.48.0
RESOLVED — handlerLoader.js:44 — script-tag/resolver cleanup on all paths
@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
| xAPI: new xAPI(this.mediator), | ||
| }; | ||
| this.kolibri = new Kolibri(this.mediator); | ||
| this.now = now; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: this.now is stored but never read — the server-clock sync is dropped. On develop, mainClient.js called this.storage[key].setNow(this.now()), syncing every storage shim's clock; that path is gone (MAINREADY at line 80 carries no now, and nothing calls SandboxShim.setNow, leaving the NOW/setNow/__setNowDiff machinery in SandboxShim.js:104-106 unreachable). Compounding it, useSandbox.js:141 now passes now: Date.now (raw device time) instead of the skew-corrected now from kolibri/utils/serverClock. Net: shim-written timestamps (xAPI statements, H5P save data) use uncorrected device time on clock-skewed devices — a data-correctness regression from the documented behavior-preserving refactor. Re-wire now into MAINREADY → setNow on each registered shim and source it from serverClock.
| event, | ||
| data: message, | ||
| // Handle registration events from iframe | ||
| this.on(events.HANDLER_REGISTRATION, registration => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The HANDLER_REGISTRATION callback registers a fresh per-shim STATEUPDATE handler (lines 105-123), and registerMessageHandler only pushes (no dedup). The iframe emits IFRAMEREADY twice by design (constructor + READYCHECK response) to cover the handshake race; both reaching MainClient fire MAINREADY twice → HANDLER_REGISTRATION twice → each shim's STATEUPDATE handler registered twice, double-emitting onStateUpdate/progress (and possibly finished). Pre-refactor this was registered once in initialize(). Guard re-registration (bail if this.registration is already set, or remove the shim namespace handler before re-adding).
| // and returning a promise that resolves when content is loaded | ||
| await this.handler.init(this.iframe, startUrl, { contentNamespace }); | ||
|
|
||
| if (this.iframe.contentWindow) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: LOADING:false (and the error meta message) is sent only inside if (this.iframe.contentWindow). If handler.init resolves while contentWindow is transiently null, neither LOADING:false nor ERROR is emitted and the consumer stays in the loading state indefinitely. Consider sending LOADING:false unconditionally and guarding only the contentDocument meta lookup.
| def handle(self, *args, **options): | ||
| destination = options["destination"] | ||
|
|
||
| if options["clear"] and os.path.exists(destination): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: shutil.rmtree(destination) deletes the operator-supplied path unconditionally; if it's a symlink or overlaps a source dir, real data is lost. Django's own collectstatic --clear clears storage contents rather than the directory node. A symlink/overlap guard would be safer (low severity — path is operator-controlled).
|
|
||
| logger.info("Collecting from: %s", source_dir) | ||
|
|
||
| for root, dirs, files in os.walk(source_dir): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: os.walk + shutil.copy2 copies Kolibri's production truncation artifacts (0-byte primary files plus .file_size/.gz sidecars) verbatim. DynamicWhiteNoise reconstructs these when serving, but the stated target — upload to a CDN or object storage — will not, serving empty files. If source static dirs are always untruncated at collection time this is a non-issue; worth confirming or documenting the precondition.
Rename ContentRendererHook to ContentViewerHook and add new SandboxedContentViewerHook for plugins that use the sandbox with pluggable handlers. - Rename ContentRendererHook -> ContentViewerHook (with backwards compat alias) - Add SandboxedContentViewerHook for sandboxed content types - Rename content_renderer_assets template tag -> content_viewer_assets - Update all existing viewer plugins to use new hook names Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move the CoreFullscreen component from kolibri-common to the kolibri package as a public component named Fullscreen. This allows external plugins to use fullscreen functionality without depending on kolibri-common internals. Update all existing usages in epub_viewer, media_player, pdf_viewer, and slideshow_viewer plugins. Add backwards-compatible alias in moduleMapping.js for CoreFullscreen. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
67c93b6 to
fb916ac
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — delta re-review of 67c93b6..fb916ac (11 files: clock-sync rewire, duplicate-STATEUPDATE guard, LOADING:false fix, collectsandboxstatic symlink/overlap guards + gzip reconstruction).
All prior findings resolved; 5 new, all non-blocking (2 suggestions, 3 nitpicks) — see inline.
CI is still pending on fb916ac, and the manual QA pass for this PR did not run (dev server failed to start), so nothing here was visually verified. Withholding approval on those two grounds rather than on the findings.
One note that doesn't map to a changed line: kolibri manage is a KolibriDjangoCommand, so the new release step runs the full initialize() (KOLIBRI_HOME creation, migrations, version upgrades) before copying static files — machinery that is only ever exercised during an actual release. A smoke invocation in the whl-build job would catch a breakage before it costs a release.
Prior-finding status
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:25 — server-clock sync dropped; shims used raw device time
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:105 — repeated HANDLER_REGISTRATION stacks per-shim STATEUPDATE handlers
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js — LOADING:false not emitted when contentWindow is transiently null
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py:46 — unconditional shutil.rmtree(destination)
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py:61 — build-truncated files copied verbatim
RESOLVED — packages/kolibri/composables/internal/useSandbox.js — customExtractors documented but silently dropped
RESOLVED — packages/kolibri-sandbox/src/mainClient.js — REGISTRATION_ACK emitted but never consumed
RESOLVED — kolibri/core/content/hooks.py:138 — two idioms for computing a package static dir
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — diverges from what alt_wsgi.py serves
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py:42 — no Python tests for the command or new hook methods
RESOLVED — packages/kolibri-sandbox/src/mainClient.js — transiently-null contentWindow makes remote null
RESOLVED — kolibri/plugins/h5p_viewer/frontend/sandbox_handler/xAPIShim.js:310 — dropped Statement.clean validation against xAPISchema
RESOLVED — packages/kolibri/composables/internal/useSandbox.js:239 — Mediator message listener never torn down
RESOLVED — packages/kolibri/composables/internal/useSandbox.js — sandbox let not reactive
RESOLVED — kolibri/core/content/hooks.py — _get_sandbox_handler_stats swallowed OSError
RESOLVED — kolibri/core/content/hooks.py — inert try/except KeyError around dict.get()
RESOLVED — packages/kolibri/components/SandboxedContentViewer/internal/SandboxedContentViewer.vue:64 — missing specs for non-trivial logic
RESOLVED — kolibri/core/content/hooks.py:170 — unconditional WebpackError on missing handler stats file
RESOLVED — pnpm-lock.yaml — terser: 5.48.0 referenced but absent from the packages/snapshots section
RESOLVED — packages/kolibri-sandbox/src/handlerLoader.js:44 — script/resolver not torn down on the success path
RESOLVED — packages/kolibri-sandbox/src/handlerLoader.js:44 — script/resolver cleaned up only on onerror
@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
| * server-corrected clock (kolibri/utils/serverClock) so shim-written timestamps | ||
| * stay consistent across clock-skewed devices. | ||
| */ | ||
| constructor({ iframe, now } = {}) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: now is documented as Required but unenforced, and omitting it fails silently.
this.now() is now called unconditionally in the IFRAMEREADY handler. On develop it was guarded (if (this.now) { ... }), so omitting it degraded to uncorrected timestamps. Now it throws a TypeError — inside a Mediator callback, which swallows it (mediator.js:37-46 wraps each callback(data) in try/catch → console.debug).
Net effect: MAINREADY is never sent, the iframe never creates its content frame, and the only trace is a console.debug. Both in-repo consumers (useSandbox.js:140, CustomContentRenderer.vue:94) pass it, so nothing is broken today — but a constructor guard (if (typeof now !== 'function') throw ...) turns a silent hang into an immediate, attributable error.
| for filename in files: | ||
| # Truncation metadata; reconstructed content is written whole, | ||
| # so the sidecar serves no purpose to a plain static host. | ||
| if filename.endswith(".file_size"): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: .gz sidecars are still collected, so the output isn't the serve-equivalent tree the docstring promises.
Skipping .file_size and inflating truncated primaries is right, but app.js.gz is still copied verbatim alongside the reconstructed app.js. DynamicWhiteNoise uses the .gz for content-encoding negotiation on the same URL — it never serves it as a distinct path. A collected tree containing both diverges from the "exact equivalence with what alt_wsgi.py serves" claim in the module docstring, and the release workflow deletes them anyway (release_kolibri.yml:210 gunzips each .gz, removing it).
Skipping X.gz when X also exists in the same directory would make the command self-sufficient and let the workflow drop its gunzip loop. Guarding on the primary's existence avoids dropping a .gz that is genuinely a content file.
| rm static/**/*.file_size | ||
| pip install dist/${{ needs.whl.outputs.whl-file-name }} | ||
| kolibri manage collectsandboxstatic static --clear | ||
| rm -f static/**/*.file_size |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: rm -f static/**/*.file_size is now dead — the command skips .file_size at collection time (collectsandboxstatic.py:67), so nothing matches. Harmless with -f, but it reads as if sidecars still land in static/.
| * Propagate the server-corrected clock to every shim. | ||
| * @param {number|Date} now - Current server time | ||
| */ | ||
| setNow(now) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: the NOW message round-trip is vestigial in this architecture.
setNow fans out to shim.setNow(now), which sets __nowDiff locally and does this.sendMessage(this.events.NOW, this.__nowDiff) (SandboxShim.js:106). On develop that send mattered: MainClient owned main-side shim instances and the message carried the diff to their iframe-side counterparts. Here shims exist only inside the iframe, so the emitted NOW reaches a parent with no handler, and each shim's constructor subscription (this.on(this.events.NOW, this.__setNowDiff), SandboxShim.js:42) can never fire. Both halves could go, leaving setNow as the single path.
| await new Promise(resolve => setTimeout(resolve, 0)); | ||
| } | ||
|
|
||
| expect(sandbox.mediator.__messageHandlers['myShim'][events.STATEUPDATE]).toHaveLength(1); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: this asserts on a private mediator field to prove a behavioural property. Dispatching one STATEUPDATE for myShim and asserting the local STATEUPDATE fires exactly once tests the same thing against the observable contract, and survives a mediator refactor.
| os.path.exists(os.path.join(self.destination, "app.js.file_size")) | ||
| ) | ||
|
|
||
| def test_clear_refuses_symlinked_destination(self): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: both guard tests assert the at-risk data still exists after the CommandError, not just that it raised — that's the assertion that catches a regression where the guard fires after rmtree.
fb916ac to
a4497fb
Compare
Introduce a new pluggable architecture for sandbox handlers that allows content viewer plugins to register custom handlers for different content types. This replaces the monolithic approach with hardcoded H5P, Bloom, and SCORM handling. Key changes: - Add SandboxHandler and SandboxShim base classes for creating handlers - Add IndexedDB shim for sandboxed storage isolation - Add dynamic handler loading via handlerLoader.js - Update existing shims (localStorage, sessionStorage, cookie, kolibri) to extend SandboxShim - Update kolibri-build to support sandbox_handler bundles - Update ESLint ecmaVersion to 2022 for static class fields The legacy H5P, Bloom, SCORM, and xAPI code is migrated out of kolibri-sandbox into the plugin sandbox handlers in the following commit. This enables external plugins to create custom sandbox handlers by extending SandboxHandler and registering shims specific to their content type requirements. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Convert html5_viewer, bloompub_viewer, and h5p_viewer plugins to use the new sandboxed content viewer architecture. Key changes: - Add SandboxedContentViewer component to kolibri package - Add useSandbox composable for sandbox lifecycle management - Create sandbox_handler bundles for each plugin: - html5_viewer: Html5ZipHandler with SCORM shim support - bloompub_viewer: BloomHandler with BloomShim - h5p_viewer: H5PHandler with xAPI shim (new plugin) - Migrate the legacy H5P, Bloom, SCORM, and xAPI code out of kolibri-sandbox into these plugin sandbox handlers (moved as renames in this commit so history and blame are preserved) - Move Bloom static assets from core to bloompub_viewer plugin - Move H5P static assets from core to h5p_viewer plugin - Update CustomContentRenderer to use new SandboxedContentViewer - Update useContentViewer with sandbox handler URL resolution - Update pluginMediator with sandbox handler registration - Enable h5p_viewer as a default plugin Each plugin now defines a sandbox_handler entry point that extends SandboxHandler and provides content-type-specific initialization and shim configuration. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Cloud deployments serve the sandbox origin's static files from object storage or a CDN rather than through alt_wsgi.py, which needs them collected into one directory first. - Add `kolibri-build collect-sandbox-static`. It resolves the source directories from the sandbox handler bundles, mirrors the sandbox server's first-wins path resolution, and inflates the files that compress.js truncated — a CDN won't rebuild those the way DynamicWhiteNoise does - Collect in the whl build, where the built tree is already on disk, and upload it as an artifact for the release job to publish. Running on every build means a breakage surfaces in PR CI, not mid-release - Mount every sandbox handler's static directory on the alt origin, replacing the single hardcoded core content path - Update update_h5p workflow for new h5p_viewer plugin location - Update .gitignore for sandbox static output directory Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016sXJTZVKU7Xz2ubqTHCwKF
Update the single page apps documentation to reflect the renamed ContentViewerHook and new SandboxedContentViewerHook for sandboxed content types. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The global prek exclude's `build/` pattern is unanchored, so it also swallowed two directories of hand-written source: the kolibri-build package and h5p_viewer's h5p_build scripts. 22 tracked files were silently unlinted — eslint.config.mjs has exempted `packages/kolibri-build/**` from the CommonJS rules all along, for files the linter never reached. Anchor the pattern to a whole path segment and widen lint-frontend's glob to cover h5p_build, then fix what that surfaces: - Exempt h5p_build from the CommonJS rules, alongside the other Node-only build scripts - Correct h5pBundle.js's `eslint-disable`, which named the pre-import-x rule (so it suppressed nothing and errored as an unknown rule) and sat below the unresolvable import it was meant to cover - Drop `url.parse`, deprecated since Node 11; `https.get` takes the URL string directly Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016sXJTZVKU7Xz2ubqTHCwKF
a4497fb to
c87a743
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — 25 of 25 prior findings resolved; no blocking issues remain. Three new items below, all non-blocking.
The substantive delta is replacing the collectsandboxstatic Django management command with a build-time kolibri-build collect-sandbox-static, moving collection into build_whl and reducing the release job to an artifact download — this resolves the prior note about kolibri manage running a full initialize() mid-release, and the JS port keeps every behaviour the Python tests covered while adding four cases they didn't.
Not approving, for three reasons unrelated to the findings: the head advanced to c87a7432 after these passes ran (Lint the hand-written build scripts is unreviewed — I confirmed it does not touch the files below); manual QA was required for this PR but did not run, so nothing here is a claim about the rendered UI; and CI last exercised the sandbox delta on an earlier SHA.
- suggestion:
collect_sandbox_static.js:89— the containment guard only covers one direction, so a destination nested inside a source self-collects. - nitpick:
.github/workflows/build_whl.yml:75— the artifact uploads on every caller, but only the release consumes it. - nitpick:
.gitignore:110— the output directory still isn't ignored, and this hunk doesn't match its commit message.
collect_sandbox_static.spec.js:143 is worth calling out: reconstructs a file compressed by the real compressor drives compressFile directly rather than the local writeTruncated mirror, so the truncate/inflate contract can't drift from compress.js unnoticed.
Prior-finding status
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:21 — now documented Required but unenforced; omission fails silently
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — .gz sidecars collected, diverging from the serve-equivalent tree
RESOLVED — .github/workflows/release_kolibri.yml — dead rm -f static/**/*.file_size
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js:184 — vestigial NOW message round-trip
RESOLVED — packages/kolibri-sandbox/test/mainClient.spec.js — asserted on a private mediator field instead of observable behaviour
ACKNOWLEDGED — kolibri/core/content/test/test_collectsandboxstatic.py — praise for the --clear guard assertions (file removed with the Python command; the equivalent JS assertions carry the property forward)
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:25 — server-clock sync dropped; shims used raw device time
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:105 — repeated HANDLER_REGISTRATION stacked per-shim STATEUPDATE handlers
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js — LOADING:false not emitted when contentWindow transiently null
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py:46 — unconditional shutil.rmtree(destination)
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py:61 — build-truncated files copied verbatim
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py:42 — no tests for the command or new hook methods
RESOLVED — packages/kolibri/composables/internal/useSandbox.js — customExtractors documented but silently dropped
RESOLVED — packages/kolibri-sandbox/src/mainClient.js — REGISTRATION_ACK emitted but never consumed
RESOLVED — kolibri/core/content/hooks.py:138 — two idioms for computing a package static dir
RESOLVED — kolibri/plugins/h5p_viewer/frontend/sandbox_handler/xAPIShim.js:310 — dropped Statement.clean validation
RESOLVED — packages/kolibri/composables/internal/useSandbox.js:239 — Mediator message listener never torn down
RESOLVED — packages/kolibri/composables/internal/useSandbox.js — sandbox not reactive / permanently null
RESOLVED — kolibri/core/content/hooks.py — _get_sandbox_handler_stats swallowed OSError
RESOLVED — kolibri/core/content/hooks.py — inert try/except KeyError
RESOLVED — packages/kolibri/components/SandboxedContentViewer/internal/SandboxedContentViewer.vue:64 — missing specs
RESOLVED — kolibri/core/content/hooks.py:170 — unconditional WebpackError on missing handler stats
RESOLVED — pnpm-lock.yaml — dangling terser@5.48.0
RESOLVED — packages/kolibri-sandbox/src/handlerLoader.js:44 — script/resolver not torn down on all paths
RESOLVED — .github/workflows/release_kolibri.yml — release step ran a full KolibriDjangoCommand initialize()
@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
| continue; | ||
| } | ||
| const sourceReal = fs.realpathSync(sourceDir); | ||
| if (sourceReal === destReal || sourceReal.startsWith(destReal + path.sep)) { |
There was a problem hiding this comment.
suggestion: the containment guard only covers one direction, so a destination nested inside a source self-collects.
This rejects a destination that contains a source, but not a source that contains the destination. In the second case walkFiles snapshots each directory with readdirSync as it descends, so by the time the walk reaches the destination subdirectory the files already written there are visible and get collected again under a doubled relative path.
Reproduced against this file as written, with 8 files plus one nested file in a single source and destination set to <source>/out:
collected 17 # expected 9
core/content/static/out/a.js … h.js, out/sub/n.js
core/content/static/out/out/a.js … h.js # <- collected twice
The CI invocation (static at the repo root) never hits this, but collect-sandbox-static kolibri/core/content/static/collected is the natural local mistake and it fails silently — a wrong tree, not an error. The symmetric check is one line next to the existing one:
if (destReal === sourceReal || destReal.startsWith(sourceReal + path.sep)) {
throw new Error(`Refusing to collect into ${destination}: it is inside source ${sourceDir}`);
}Worth hoisting out of clearDestination so it applies without --clear too, since the self-collection happens either way.
| # surfaces in PR CI rather than mid-release. | ||
| - name: Collect sandbox static files | ||
| run: pnpm run collect-sandbox-static static --clear | ||
| - uses: actions/upload-artifact@v7 |
There was a problem hiding this comment.
nitpick: the artifact is uploaded on every caller, but only the release consumes it.
build_whl.yml is called from release_kolibri.yml, pr_build_kolibri.yml and warm_build_cache.yml. The rationale in the step comment above — surfacing a breakage in PR CI rather than mid-release — is served by the collect step; the upload only matters for the release path. Gating it (if: github.event_name == 'release', or an input from the caller) keeps the early-warning property without storing the tree on every PR push and every develop build.
| # Check in h5p & bloom specific files | ||
| !kolibri/core/content/static/h5p/ | ||
| !kolibri/core/content/static/bloom/ | ||
| # Check in h5p & bloom specific files in their respective plugins |
There was a problem hiding this comment.
nitpick: the output directory still isn't ignored, and this hunk doesn't match its commit message.
83c414adc7 lists "Update .gitignore for sandbox static output directory", but the only .gitignore change is relocating the h5p/bloom check-in exceptions to their new plugin paths — which belongs with the plugin migration in 114a7ec8f7, not with the cloud-deployment commit. git check-ignore static/foo.js reports not-ignored: kolibri/**/static/* doesn't reach the repo root, so running the documented command locally leaves a few thousand untracked files in git status.
4 participants
Summary
Generalizes our sandbox handling across all the plugins that use it:
kolibri-sandboxcode to handle the logic.References
Fixes #14055.
Reviewer guidance
For manual QA - regression testing of Kolibri QA channel resources for:
Nothing else should be affected by this work.
packages/kolibri/composables/internal/useSandbox.js:35—useSandboxcallsuseContentViewerand re-exposes its API by spreading the result.packages/kolibri/internal/pluginMediator.js:232—getSandboxHandlerUrlreturns a registered handler URL for a preset or null; check a non-sandboxed preset (e.g.epub) returns null so those viewers never take the sandbox path.Rendering verified live for each sandboxed content type:
AI usage
Used Claude Code to rebuild the sandbox integration onto the content-viewer composable. Verified with the Jest suite,
prekrun per commit, and manual QA in a browser.