Pluggable sandbox handler architecture for content viewer plugins - #15036
Pluggable sandbox handler architecture for content viewer plugins#15036rtibbles wants to merge 8 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
🟡 Waiting for changesLast updated: 2026-08-16 18:04 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.
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 329f8376..7dc7702f. All 40 prior findings are resolved or acknowledged; 4 new non-blocking findings, all about behaviour quietly dropped during the migration rather than anything wrong in the new logic.
I traced the restored data flow (MAINREADY → createIframe → handler.setData/setUserData → per-shim __setData) and confirmed the two blocking regressions from the last round are genuinely closed. Collapsing Bloom onto stateUpdated() leaves exactly one route from a shim to _iframeProgress — much easier to reason about than the two bespoke paths it replaces.
CI is pending on 7dc7702f (3 in progress, 7 complete, no failures) and manual QA did not run in this pass, so nothing here is a claim about the rendered UI.
New findings inline: 2 suggestions (options.width regression, template_html drops css_selectors), 2 nitpicks.
Prior-finding status
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js:146 — saved contentState/userData never reached the shims
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js:129 — CustomContentRenderer supplied no handlerUrl
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:146 — dropped DATARETURNED relay
RESOLVED — packages/kolibri/composables/internal/useSandbox.js:144 — Bloom reported no progress
RESOLVED — kolibri/plugins/html5_viewer/frontend/sandbox_handler/SCORMShim.js:618 — SCORM.spec.js deleted without replacement
RESOLVED — packages/kolibri/components/Fullscreen.vue:22 — duplicated rather than moved CoreFullscreen.vue
RESOLVED — packages/kolibri-sandbox/src/patchIndexedDB.js — unreferenced after IndexedDBShim
ACKNOWLEDGED — .github/workflows/build_whl.yml:75 — sandbox-static artifact uploaded on every caller (withdrawn)
RESOLVED — kolibri/core/content/hooks.py:170 — unconditional WebpackError on missing handler stats
RESOLVED — pnpm-lock.yaml — terser version skew vs the packages snapshot
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — diverged from what alt_wsgi.py serves
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — no Python tests for the command or the new hook methods
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — shutil.rmtree on an operator-supplied path
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — copied production truncation artifacts
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — .gz sidecars still collected
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-sandbox/src/mainClient.js — contentWindow transiently null mid-navigation
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 leaked
RESOLVED — packages/kolibri/composables/internal/useSandbox.js — sandbox let never needed reassignment exposure
RESOLVED — packages/kolibri/components/SandboxedContentViewer/internal/SandboxedContentViewer.vue:64 — untested non-trivial logic
RESOLVED — packages/kolibri-sandbox/src/handlerLoader.js:44 — <script> / resolver not torn down on success
RESOLVED — packages/kolibri-sandbox/src/handlerLoader.js:44 — cleanup only on the onerror path
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:30 — this.now stored but never read
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:110 — per-shim STATEUPDATE handler re-registered
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js — LOADING:false gated on contentWindow
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:21 — now documented Required but unenforced
RESOLVED — .github/workflows/release_kolibri.yml — dead rm -f static/**/*.file_size
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js:167 — vestigial NOW round-trip
RESOLVED — packages/kolibri-sandbox/test/mainClient.spec.js — asserted on a private mediator field
RESOLVED — kolibri/core/content/test/test_collectsandboxstatic.py — praise, guard tests assert surviving data
RESOLVED — packages/kolibri-build/src/collect_sandbox_static.js:102 — one-directional containment guard
RESOLVED — .gitignore:110 — output directory not ignored / hunk mismatched its commit message
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — polyfill-injection block never executed
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — hardcoded corejs: '3.46'
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — positional splice(1, 0, ...)
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — asymmetric treatment of adjacent failure modes
RESOLVED — packages/kolibri-sandbox/package.json — dead lodash / toposort-class
RESOLVED — packages/kolibri-sandbox/package.json — mutationobserver-shim in dependencies
RESOLVED — packages/kolibri-build/src/collect_sandbox_static.js:45 — praise, isServeArtifact encodes the serve contract
@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
|
|
||
| <div | ||
| class="iframe-container" | ||
| :style="containerStyle" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: options.width is no longer applied. Both deleted renderers bound the content node's width onto the wrapper — Html5AppRendererIndex.vue:6 and BloomPubRendererIndex.vue:6 each had :style="{ width: iframeWidth }" with iframeWidth() { return (this.options && this.options.width) || 'auto'; }. containerStyle here only ever returns fullscreen positioning (setup.js:40-49), and while useSandbox destructures options as contentOptions (useSandbox.js:37) it reads it solely for urlBuilder (useSandbox.js:79). HTML5, Bloom and H5P nodes that set options.width now render at the container's full width.
Default behaviour is unaffected ('auto' on a block element is already full width), so this only bites content that sets the option explicitly — but it reads as a silent drop rather than a deliberate removal. contentOptions is already in scope in setup.js, so folding a width into containerStyle would restore it.
For reference, iframeHeight was dead on develop too (assigned from RESIZE, never bound in either template), so not carrying that one over is correct.
| """ | ||
| urls = [chunk["url"] for chunk in self.bundle] | ||
|
|
||
| viewer_data = { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: this override rebuilds the whole payload instead of extending the parent's, and the rebuilt dict omits the css_selectors key the parent emits (hooks.py:94). registerAllContentViewers reads that key and forwards it as the viewer's DOM tags (pluginMediator.js:252).
None of the three sandboxed viewers sets css_selectors or allow_object_tag today, so all_css_selectors() returns () and the omission is inert — data.css_selectors is undefined, which falls through to the domTags = [] default. But a sandboxed viewer that later declares either attribute would have it silently ignored, surfacing as "the viewer never matches its DOM tag" with nothing pointing back here. Factoring the shared payload out (e.g. a _viewer_data() on ContentViewerHook that this subclass extends with sandboxHandlerUrl) keeps the two in sync and drops the duplicated frontend_message_tag() + plugin_data_tag() + <template> construction.
| data: this._shimData[shimName], | ||
| }); | ||
| } | ||
| if (userData) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: USERDATAUPDATE goes to every registered shim, but only SCORMShim, xAPIShim and BloomShim subscribe — localStorage, sessionStorage, cookie, indexedDB and kolibri never register a handler, so those posts are discarded on arrival. The STATEUPDATE branch immediately above is already selective (shimName in contentState); gating on declared interest would make the two symmetric. Harmless as-is, just chatter on every timeSpent tick.
| setData(data) { | ||
| if (!data) return; | ||
| for (const [name, shimData] of Object.entries(data)) { | ||
| if (this.shims[name] && typeof this.shims[name].setData === 'function') { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: the typeof this.shims[name].setData === 'function' guard is now vestigial — setData is a concrete method on SandboxShim (SandboxShim.js:78), so every shim has it. The real opt-in moved into SandboxShim.setData's if (this.__setData). setUserData (line 157) carries the same dead check.
7dc7702 to
69f71ef
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — delta re-review, 7dc7702 → 69f71ef plus the rebase onto 721b391.
43 of 44 prior findings settled; 1 still open (inline). The new code — viewer_data extraction, selective USERDATAUPDATE routing, the toolbar move — reads clean; one nitpick inline.
Not approving: CI still queued on 69f71ef, manual QA did not run, no node_modules/Python env here. Read-only pass.
Prior-finding status
RESOLVED — collectsandboxstatic.py — diverges from alt_wsgi serving
RESOLVED — collectsandboxstatic.py — no Python tests
RESOLVED — mainClient.js — null contentWindow nulls remote
RESOLVED — xAPIShim.js:310 — dropped xAPISchema validation
RESOLVED — useSandbox.js:239 — mediator listener leak
RESOLVED — useSandbox.js — sandbox let/null
RESOLVED — hooks.py — _get_sandbox_handler_stats swallows OSError
RESOLVED — hooks.py — inert except KeyError
RESOLVED — SandboxedContentViewer.vue:54 — untested logic
RESOLVED — hooks.py:178 — unconditional WebpackError
RESOLVED — pnpm-lock.yaml — terser 5.48.0 mismatch
RESOLVED — handlerLoader.js:44 — no success-path teardown
RESOLVED — handlerLoader.js:44 — cleanup only on onerror
RESOLVED — mainClient.js:30 — this.now never read
RESOLVED — mainClient.js:110 — per-shim STATEUPDATE re-registered
RESOLVED — iframeClient.js — LOADING:false gated on contentWindow
RESOLVED — collectsandboxstatic.py — unconditional rmtree
RESOLVED — collectsandboxstatic.py — copies truncation artifacts
RESOLVED — mainClient.js:21 — now Required but unenforced
RESOLVED — collectsandboxstatic.py — .gz sidecars collected
RESOLVED — release_kolibri.yml — dead rm -f *.file_size
RESOLVED — SandboxHandler.js:165 — vestigial NOW round-trip
RESOLVED — mainClient.spec.js — asserts a private mediator field
RESOLVED — test_collectsandboxstatic.py — praise: guard tests
RESOLVED — collect_sandbox_static.js:102 — one-way containment guard
ACKNOWLEDGED — build_whl.yml:75 — artifact on every caller (withdrawn)
RESOLVED — .gitignore:110 — output dir not ignored
RESOLVED — webpack.config.plugin.js — polyfill block never ran
RESOLVED — webpack.config.plugin.js — hardcoded corejs: '3.46'
RESOLVED — webpack.config.plugin.js — positional splice ordering
RESOLVED — kolibri-sandbox/package.json — dead lodash/toposort-class
RESOLVED — kolibri-sandbox/package.json — mutationobserver-shim
RESOLVED — webpack.config.plugin.js — inconsistent failure modes
RESOLVED — iframeClient.js:146 — MAINREADY payload dropped
RESOLVED — iframeClient.js:129 — handlerUrl unsupplied by CustomContentRenderer
RESOLVED — mainClient.js:146 — DATARETURNED relay dropped
RESOLVED — useSandbox.js:144 — onUserDataUpdate never bound
RESOLVED — SCORMShim.js:618 — SCORM.spec.js deleted
RESOLVED — Fullscreen.vue:22 — duplicates CoreFullscreen.vue
RESOLVED — collect_sandbox_static.js:45 — praise: serve contract
RESOLVED — hooks.py:208 — template_html omitted css_selectors
RESOLVED — mainClient.js:179 — USERDATAUPDATE to every shim
RESOLVED — SandboxHandler.js:145 — vestigial typeof guards
UNADDRESSED — SandboxedContentViewer.vue:15 — options.width not applied
@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
|
|
||
| <div | ||
| class="iframe-container" | ||
| :style="containerStyle" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: still open — options.width is no longer applied. containerStyle (internal/setup.js:33-42) returns only fullscreen positioning, and contentOptions is read solely for urlBuilder (useSandbox.js:79), so a node setting options.width renders at container width.
There was a problem hiding this comment.
This is intentional, this has been a no-op for over 5 years.
There was a problem hiding this comment.
You're right — I missed that options.width was already inert on develop, so this isn't a regression. Withdrawing this finding.
| this.sendMessage(this.events.STATEUPDATE, data); | ||
| } | ||
|
|
||
| userDataUpdated() { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: nothing on the main side listens for USERDATAUPDATE — MainClient registers only IFRAMEREADY, HANDLER_REGISTRATION and a per-shim STATEUPDATE, and the plugin module.js files pass only navigateTo. Every SandboxHandler.setUserData seed fires one discarded postMessage per user-data shim. Drop the call from setUserData, or wire a consumer. The shims' __setUserData subscription is invoked by the mediator directly, so there's no echo loop either way.
69f71ef to
bd4d3e2
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — delta review of bd4d3e2. 46 prior findings resolved; two new, inline. CI green; no QA.
Prior-finding status
RESOLVED — SandboxShim.js — USERDATAUPDATE
RESOLVED — SandboxedContentViewer.vue:15 — options.width
ACKNOWLEDGED — build_whl.yml:75 — artifact-upload
RESOLVED — collectsandboxstatic.py — alt_wsgi-divergence
RESOLVED — collectsandboxstatic.py — missing-tests
RESOLVED — mainClient.js — null-contentWindow
RESOLVED — xAPIShim.js:310 — Statement.clean
RESOLVED — useSandbox.js:239 — listener-leak
RESOLVED — useSandbox.js — let-null
RESOLVED — hooks.py — OSError
RESOLVED — hooks.py — except-KeyError
RESOLVED — SandboxedContentViewer.vue:54 — missing-tests
RESOLVED — hooks.py:178 — WebpackError
RESOLVED — pnpm-lock.yaml — terser
RESOLVED — handlerLoader.js:44 — success-teardown
RESOLVED — handlerLoader.js:44 — error-teardown
RESOLVED — mainClient.js:30 — clock-sync
RESOLVED — mainClient.js:110 — re-registration
RESOLVED — iframeClient.js — LOADING:false
RESOLVED — collectsandboxstatic.py — rmtree
RESOLVED — collectsandboxstatic.py — truncation-artifacts
RESOLVED — mainClient.js:21 — now
RESOLVED — collectsandboxstatic.py — .gz-sidecars
RESOLVED — release_kolibri.yml — dead-rm
RESOLVED — SandboxHandler.js:165 — NOW
RESOLVED — mainClient.spec.js — private-field
RESOLVED — test_collectsandboxstatic.py — guard-tests
RESOLVED — collect_sandbox_static.js:102 — containment-guard
RESOLVED — build_whl.yml:75 — artifact-upload
RESOLVED — .gitignore:110 — output-dir
RESOLVED — webpack.config.plugin.js — dead-block
RESOLVED — webpack.config.plugin.js — corejs
RESOLVED — webpack.config.plugin.js — splice
RESOLVED — kolibri-sandbox/package.json — dead-deps
RESOLVED — kolibri-sandbox/package.json — mutationobserver-shim
RESOLVED — webpack.config.plugin.js — failure-modes
RESOLVED — iframeClient.js:146 — MAINREADY
RESOLVED — iframeClient.js:129 — handlerUrl
RESOLVED — mainClient.js:146 — DATARETURNED
RESOLVED — useSandbox.js:144 — onUserDataUpdate
RESOLVED — SCORMShim.js:618 — SCORM.spec.js
RESOLVED — Fullscreen.vue:22 — CoreFullscreen
RESOLVED — collect_sandbox_static.js:45 — serve-contract
RESOLVED — hooks.py — viewer_data
RESOLVED — mainClient.js — USERDATAUPDATE-broadcast
RESOLVED — SandboxHandler.js — setData-guard
@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
| */ | ||
| init(iframe, startUrl) { | ||
| return new Promise((resolve, reject) => { | ||
| this.runner = new H5PRunner(this.shims.xAPI); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: H5P user data changed namespace, losing saved resume state.
H5PRunner is handed the xAPI shim (H5PHandler.js:28), so H5P.setUserData persists under _shimData.xAPI, while contentState.H5P restore no-ops in H5PShim (no __setData).
Fix: hand the runner an H5PShim defining data/__setData, as develop did.
| * Collect state data from all shims for persistence. | ||
| * @returns {object} Combined state data keyed by shim name | ||
| */ | ||
| getData() { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: getData() has no callers; shims push STATEUPDATE. Drop it.
bd4d3e2 to
8feea89
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — delta re-review. All 39 prior findings settled; two nitpicks inline. CI passing, 537 sandbox tests pass locally. Manual QA did not run, so nothing is visually verified.
Prior-finding status
RESOLVED — H5PHandler.js:28 — user-data namespace
RESOLVED — SandboxHandler.js:128 — dead getData()
ACKNOWLEDGED — build_whl.yml:75 — artifact upload
ACKNOWLEDGED — SandboxedContentViewer.vue:15 — options.width
RESOLVED — iframeClient.js:146 — setData uncalled
RESOLVED — iframeClient.js:129 — mandatory handlerUrl
RESOLVED — iframeClient.js:142 — gated LOADING:false
RESOLVED — mainClient.js:146 — DATARETURNED relay dropped
RESOLVED — mainClient.js:110 — duplicate STATEUPDATE registration
RESOLVED — mainClient.js:30 — server-clock now dropped
RESOLVED — mainClient.js:21 — now unenforced
RESOLVED — mainClient.js:72 — null contentWindow deref
RESOLVED — mainClient.js:179 — USERDATAUPDATE over-broadcast
RESOLVED — SandboxHandler.js:151 — vestigial NOW round-trip
RESOLVED — SandboxHandler.js:145 — vestigial setData guard
RESOLVED — SandboxShim.js:146 — discarded USERDATAUPDATE
RESOLVED — handlerLoader.js:44 — script tag and resolver leaked
RESOLVED — package.json:36 — dead lodash/toposort-class
RESOLVED — package.json:37 — mutationobserver-shim misplaced
RESOLVED — mainClient.spec.js:258 — asserted a private field
RESOLVED — xAPIShim.js:294 — statement validation dropped
RESOLVED — SCORMShim.js:602 — SCORM.spec.js deleted
RESOLVED — useSandbox.js:239 — Mediator.destroy() never called
RESOLVED — useSandbox.js:247 — exported sandbox always null
RESOLVED — useSandbox.js:144 — Bloom progress unreported
RESOLVED — SandboxedContentViewer.vue:54 — no specs
RESOLVED — Fullscreen.vue:22 — duplicated CoreFullscreen.vue
RESOLVED — hooks.py:178 — unconditional WebpackError
RESOLVED — hooks.py:170 — swallowed OSError
RESOLVED — hooks.py:192 — inert except KeyError
RESOLVED — hooks.py:208 — omitted css_selectors
RESOLVED — webpack.config.plugin.js:239 — dead polyfill block
RESOLVED — webpack.config.plugin.js:250 — hardcoded corejs
RESOLVED — webpack.config.plugin.js:193 — positional splice
RESOLVED — collect_sandbox_static.js:102 — one-way containment guard
RESOLVED — collectsandboxstatic.py:41 — 5 findings; command superseded
RESOLVED — release_kolibri.yml:208 — dead rm -f
RESOLVED — .gitignore:110 — output dir not ignored
RESOLVED — pnpm-lock.yaml:15632 — missing terser@5.48.0
@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
| @@ -97,7 +97,6 @@ const filePathMappers = { | |||
| export default class BloomRunner { | |||
| constructor(shim) { | |||
| this.shim = shim; | |||
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: shim is unused now that this.data = shim.data is gone; H5PRunner gained get data() proxies instead. Drop the parameter and the BloomHandler.js:26 thread.
| // they are handed override __setData or __setUserData. | ||
| this.__setData = this.__setData.bind(this); | ||
| this.__setUserData = this.__setUserData.bind(this); | ||
| this.on(this.events.STATEUPDATE, this.__setData); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: this subscribes every shim, opening a restore path for sessionStorage, which sessionStorage.js:5 documents as never persisted. Inert today (stateUpdated() is a no-op), but the outbound half is now the only thing enforcing that. Override the restore on SessionStorage, or declare the subscription the way consumesUserData is.
|
Ignore the "tiny probe body" review above — posted in error by tooling; the real delta review is the one after it. |
8feea89 to
178f7b8
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — delta at 178f7b8938. 50 prior findings settled (48 resolved, 2 withdrawn); 2 new below, neither blocking. CI is red only on the Pi image build, untouched here. Manual QA did not run; the PR screenshots are author-attached, not verification.
Prior-finding status
RESOLVED — collectsandboxstatic.py — diverges from alt_wsgi's served set
RESOLVED — collectsandboxstatic.py — no Python tests for command or hooks
RESOLVED — mainClient.js — transiently null contentWindow
RESOLVED — xAPIShim.js:294 — dropped statement validation
RESOLVED — useSandbox.js:239 — mediator message listener leak
RESOLVED — useSandbox.js — sandbox let/null init
RESOLVED — hooks.py — _get_sandbox_handler_stats swallows OSError
RESOLVED — hooks.py — inert try/except KeyError
RESOLVED — SandboxedContentViewer.vue:54 — untested non-trivial logic
RESOLVED — hooks.py:178 — unconditional WebpackError
RESOLVED — pnpm-lock.yaml — terser version mismatch
RESOLVED — handlerLoader.js:44 — no teardown on success
RESOLVED — handlerLoader.js:44 — cleanup only on onerror
RESOLVED — mainClient.js:30 — this.now never read
RESOLVED — mainClient.js:110 — per-shim STATEUPDATE re-registration
RESOLVED — iframeClient.js — LOADING:false behind contentWindow guard
RESOLVED — collectsandboxstatic.py — unconditional rmtree
RESOLVED — collectsandboxstatic.py — copies truncation artifacts
RESOLVED — mainClient.js:21 — now unenforced
RESOLVED — collectsandboxstatic.py — .gz sidecars collected
RESOLVED — release_kolibri.yml — dead rm -f *.file_size
RESOLVED — SandboxHandler.js:151 — vestigial NOW round-trip
RESOLVED — test/mainClient.spec.js — asserts private mediator field
RESOLVED — test_collectsandboxstatic.py — praise, guard tests
RESOLVED — collect_sandbox_static.js:102 — one-directional containment guard
ACKNOWLEDGED — build_whl.yml:75 — artifact uploaded on every caller (withdrawn)
RESOLVED — .gitignore:110 — output directory not ignored
RESOLVED — webpack.config.plugin.js — dead block, no polyfill injection
RESOLVED — webpack.config.plugin.js — hardcoded corejs '3.46'
RESOLVED — webpack.config.plugin.js — positional splice(1, 0, ...)
RESOLVED — kolibri-sandbox/package.json — dead lodash/toposort-class
RESOLVED — kolibri-sandbox/package.json — mutationobserver-shim placement
RESOLVED — webpack.config.plugin.js — inconsistent failure modes
RESOLVED — iframeClient.js:146 — dropped MAINREADY contentState/userData
RESOLVED — iframeClient.js:129 — mandatory handlerUrl vs CustomContentRenderer
RESOLVED — mainClient.js:146 — DATARETURNED relay dropped
RESOLVED — useSandbox.js:144 — onUserDataUpdate never bound
RESOLVED — SCORMShim.js:602 — SCORM.spec.js deleted, no replacement
RESOLVED — Fullscreen.vue:22 — duplicates CoreFullscreen.vue
RESOLVED — collect_sandbox_static.js:45 — praise, serve-time contract
ACKNOWLEDGED — SandboxedContentViewer.vue:15 — options.width unapplied (withdrawn)
RESOLVED — hooks.py — override omits parent payload fields
RESOLVED — mainClient.js — USERDATAUPDATE broadcast to every shim
RESOLVED — SandboxHandler.js — vestigial typeof setData guard
RESOLVED — SandboxShim.js — no main-side USERDATAUPDATE listener
RESOLVED — H5PHandler.js — namespace change loses H5P resume state
RESOLVED — SandboxHandler.js — getData() has no callers
RESOLVED — BloomRunner.js — unused shim param
RESOLVED — SandboxShim.js:68 — blanket STATEUPDATE subscription
@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
| } | ||
|
|
||
| createIframe({ contentNamespace, startUrl = '' } = {}) { | ||
| async createIframe({ |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: no re-entry guard, though mainClient.js:119-124 de-dups the same repeat. clearIframe() never off()s the outgoing handler's shims (SandboxShim.js:68-69), and this async method can interleave, so run #1 resumes and inits run #2's iframe. Guard on the in-flight handlerUrl; off() old shims.
| for (const shim of Object.values(this.shims)) { | ||
| try { | ||
| // IndexedDB shim needs the content namespace for database prefixing | ||
| if (typeof shim.setContentNamespace === 'function' && contentNamespace) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: duck-typing setContentNamespace puts IndexedDB knowledge in the base class, against AC 1. Pass via shim.iframeInitialize(contentWindow, { contentNamespace }) instead, dropping this branch and the setter.
178f7b8 to
f0ade07
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — delta re-review.
48 of 51 prior findings resolved; 2 acknowledged, 1 open: the useSandbox.js:53 let sandbox = null nitpick. CI pending; manual QA did not run.
Prior-finding status
RESOLVED — collectsandboxstatic.py — diverges from what alt_wsgi.py serves
RESOLVED — collectsandboxstatic.py — no Python tests for command or hook methods
RESOLVED — mainClient.js — transiently null contentWindow clobbers remote
RESOLVED — xAPIShim.js:294 — statements not validated against xAPISchema
RESOLVED — useSandbox.js:239 — mediator listener never torn down
UNADDRESSED — useSandbox.js:53 — sandbox is a let initialized to null
RESOLVED — hooks.py — _get_sandbox_handler_stats swallowed OSError
RESOLVED — hooks.py — inert try/except KeyError
RESOLVED — SandboxedContentViewer.vue:54 — non-trivial logic untested
RESOLVED — hooks.py:178 — WebpackError raised unconditionally on missing stats
RESOLVED — pnpm-lock.yaml — terser referenced without packages/snapshots entry
RESOLVED — handlerLoader.js:44 — script tag and resolver leaked on success path
RESOLVED — handlerLoader.js:44 — cleanup only on onerror
RESOLVED — mainClient.js:30 — this.now stored but never read
RESOLVED — mainClient.js:110 — STATEUPDATE registered twice on repeated registration
RESOLVED — iframeClient.js — LOADING:false gated on contentWindow
RESOLVED — collectsandboxstatic.py — rmtree on an operator-supplied symlink
RESOLVED — collectsandboxstatic.py — copies production truncation artifacts
RESOLVED — mainClient.js:21 — now documented Required but unenforced
RESOLVED — collectsandboxstatic.py — .gz sidecars still collected
RESOLVED — release_kolibri.yml — rm -f static/**/*.file_size is dead
RESOLVED — SandboxHandler.js:158 — vestigial NOW round-trip
RESOLVED — test/mainClient.spec.js — asserts on a private mediator field
RESOLVED — test_collectsandboxstatic.py — guard-test praise, carried to the JS collector
RESOLVED — collect_sandbox_static.js:102 — containment guard covered one direction
ACKNOWLEDGED — build_whl.yml:75 — artifact uploaded on every caller
RESOLVED — .gitignore:110 — output dir not ignored / hunk mismatched commit message
RESOLVED — webpack.config.plugin.js — polyfill-injection block never executed
RESOLVED — webpack.config.plugin.js — corejs: '3.46' hardcoded
RESOLVED — webpack.config.plugin.js — positional splice(1, 0, ...)
RESOLVED — kolibri-sandbox/package.json — dead lodash and toposort-class
RESOLVED — kolibri-sandbox/package.json — mutationobserver-shim in dependencies
RESOLVED — webpack.config.plugin.js — adjacent failure modes treated differently
RESOLVED — iframeClient.js:180 — createIframe dropped contentState/userData
RESOLVED — iframeClient.js:163 — handlerUrl mandatory, CustomContentRenderer supplied none
RESOLVED — mainClient.js:146 — DATARETURNED relay dropped
RESOLVED — useSandbox.js:144 — onUserDataUpdate never bound
RESOLVED — SCORMShim.js:602 — SCORM.spec.js deleted without replacement
RESOLVED — Fullscreen.vue:22 — duplicated CoreFullscreen
RESOLVED — collect_sandbox_static.js:45 — isServeArtifact/collectFile praise
ACKNOWLEDGED — SandboxedContentViewer.vue:15 — options.width no longer applied
RESOLVED — hooks.py — viewer_data rebuilt the whole payload
RESOLVED — mainClient.js — USERDATAUPDATE broadcast to every shim
RESOLVED — SandboxHandler.js — vestigial typeof setData guard
RESOLVED — SandboxShim.js — nothing on main listened for USERDATAUPDATE
RESOLVED — H5PHandler.js — H5P user data changed namespace
RESOLVED — SandboxHandler.js — getData() had no callers
RESOLVED — BloomRunner.js — unused shim argument
RESOLVED — SandboxShim.js:71 — base subscription opened a sessionStorage restore path
RESOLVED — iframeClient.js — no re-entry guard on createIframe
RESOLVED — SandboxHandler.js — duck-typed setContentNamespace
@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
| "uuid": "^14.0.1", | ||
| "web-streams-polyfill": "catalog:" | ||
| }, | ||
| "devDependencies": { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: build-h5p moved here with only jszip, purgecss and html-webpack-plugin. Undeclared on this package's resolution path:
jquery(h5p_build/h5pBundle.js:7) —jquery@3.5.1is gone from the lockfilemini-css-extract-plugin,css-minimizer-webpack-plugin,terser-webpack-plugin,babel-loader,css-loader(webpack.config.h5p.js:3-6,37,50)webpack,webpack-cli
The root declares only webpack; the rest sit in kolibri-sandbox/kolibri-build, off this path. So pnpm --filter kolibri-h5p-viewer-plugin run build-h5p (README.md:12, dependency_updates.yml:71) fails, uncovered by PR CI. Declare them here. babel.config.js is also no longer adjacent for babel-loader.
| super(mediator); | ||
| this.data = {}; | ||
| this.nameSpace = 'kolibri'; | ||
| this.mediator = new Mediator(window.parent); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: redundant Mediator that escapes teardown. Kolibri is now only a SandboxHandler shim, and sandbox.mediator is already new Mediator(window.parent) (iframeClient.js:25). SandboxShim.destroy() unsubscribes this.__mediator only, so this one survives a handler swap. Use this.__mediator here (test/kolibri.spec.js:38 spies on kolibri.mediator).
| } | ||
|
|
||
| /** | ||
| * Build the iframe for a piece of content, one build at a time. |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: the guard records the failure mode, not the mechanism.
f0ade07 to
12631b0
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — all 52 prior findings resolved or acknowledged; one new suggestion inline.
CI passing. Manual QA did not run — nothing here is visually verified.
Prior-finding status
RESOLVED — kolibri/plugins/h5p_viewer/package.json:24 — undeclared build deps
RESOLVED — packages/kolibri-sandbox/src/kolibri.js:97 — redundant Mediator
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js:113 — praise, guard records failure mode
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — diverges from alt_wsgi
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — no Python tests
RESOLVED — packages/kolibri-sandbox/src/mainClient.js — null contentWindow clobbers remote
RESOLVED — kolibri/plugins/h5p_viewer/frontend/sandbox_handler/xAPIShim.js:294 — dropped Statement.clean
RESOLVED — packages/kolibri/composables/internal/useSandbox.js — mediator listener leak
RESOLVED — packages/kolibri/composables/internal/useSandbox.js — sandbox permanently null
RESOLVED — kolibri/core/content/hooks.py — swallowed OSError
RESOLVED — kolibri/core/content/hooks.py — inert except KeyError
RESOLVED — packages/kolibri/components/SandboxedContentViewer/internal/SandboxedContentViewer.vue:54 — no specs
RESOLVED — kolibri/core/content/hooks.py:178 — unconditional WebpackError
RESOLVED — pnpm-lock.yaml — missing terser entry
RESOLVED — packages/kolibri-sandbox/src/handlerLoader.js:44 — no teardown
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:30 — this.now never read
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:110 — duplicate STATEUPDATE registration
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js — LOADING:false skipped
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — unconditional rmtree
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — copies truncation artifacts
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:21 — now unenforced
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — .gz sidecars collected
RESOLVED — .github/workflows/release_kolibri.yml — dead rm -f
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js:158 — vestigial NOW round-trip
RESOLVED — packages/kolibri-sandbox/test/mainClient.spec.js — asserts private field
RESOLVED — kolibri/core/content/test/test_collectsandboxstatic.py — praise, guard tests
RESOLVED — packages/kolibri-build/src/collect_sandbox_static.js:102 — one-directional containment guard
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — polyfill block never ran
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — hardcoded corejs
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — positional splice
RESOLVED — packages/kolibri-sandbox/package.json — dead lodash, toposort-class
RESOLVED — packages/kolibri-sandbox/package.json — mutationobserver-shim placement
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — uneven failure handling
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js:180 — MAINREADY payload dropped
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js:163 — missing handlerUrl
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:146 — DATARETURNED relay dropped
RESOLVED — packages/kolibri/composables/internal/useSandbox.js — Bloom reported no progress
RESOLVED — kolibri/plugins/html5_viewer/frontend/sandbox_handler/SCORMShim.js:602 — SCORM.spec deleted
RESOLVED — packages/kolibri/components/Fullscreen.vue:22 — duplicated CoreFullscreen
RESOLVED — packages/kolibri-build/src/collect_sandbox_static.js:45 — praise, serve-time contract
RESOLVED — kolibri/core/content/hooks.py — override omitted css_selectors
RESOLVED — packages/kolibri-sandbox/src/mainClient.js — USERDATAUPDATE broadcast
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js — vestigial setData guard
RESOLVED — packages/kolibri-sandbox/src/SandboxShim.js — unlistened USERDATAUPDATE
RESOLVED — kolibri/plugins/h5p_viewer/frontend/sandbox_handler/H5PHandler.js — user data namespace
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js — getData unused
RESOLVED — kolibri/plugins/bloompub_viewer/frontend/sandbox_handler/BloomRunner.js — unused shim param
RESOLVED — packages/kolibri-sandbox/src/SandboxShim.js:71 — every shim subscribed
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js — no re-entry guard
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js — duck-typed setContentNamespace
ACKNOWLEDGED — .github/workflows/build_whl.yml:75 — artifact on every caller (withdrawn)
ACKNOWLEDGED — packages/kolibri/components/SandboxedContentViewer/internal/SandboxedContentViewer.vue:15 — options.width (withdrawn)
@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 (forceDurationBasedProgress.value) { | ||
| currentProgress = durationBasedProgress.value; | ||
| } else { | ||
| currentProgress = sandboxProgress !== null ? sandboxProgress : durationBasedProgress.value; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: the duration fallback now reaches H5P and Bloom, which never used it on develop.
onStateUpdate calls emitProgress, and a null falls back to durationBasedProgress. Html5AppRendererIndex.vue:137 and BloomPubRendererIndex.vue:128 emitted only when progress !== null, and the fallback ran off pollProgress, skipped for H5P. A node with duration whose shim returns null now emits timeSpent / duration and fires finished at 1 — completion on elapsed time alone. (Bloom reaches this via the wiring I asked for last round; that request stands.)
Gate the state-update path on sandboxProgress !== null, or tie the fallback to progressPollingInterval.
12631b0 to
3fe8c9f
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — all 55 prior findings resolved or acknowledged; 3 new suggestions inline. CI pending; manual QA did not run.
Prior-finding status
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — diverges from what alt_wsgi serves
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — no Python tests for the command or new hook methods
RESOLVED — packages/kolibri-sandbox/src/mainClient.js — `contentWindow` transiently null clobbers `remote`
RESOLVED — kolibri/plugins/h5p_viewer/frontend/sandbox_handler/xAPIShim.js:294 — dropped `Statement.clean` validation
RESOLVED — packages/kolibri/composables/internal/useSandbox.js — `Mediator` window listener escapes teardown
RESOLVED — packages/kolibri/composables/internal/useSandbox.js:245 — `sandbox` is a plain `let`
RESOLVED — kolibri/core/content/hooks.py — `_get_sandbox_handler_stats` swallows `OSError`
RESOLVED — kolibri/core/content/hooks.py — inert `try`/`except KeyError`
RESOLVED — packages/kolibri/components/SandboxedContentViewer/internal/SandboxedContentViewer.vue:54 — non-trivial logic untested
RESOLVED — kolibri/core/content/hooks.py:178 — unconditional `WebpackError` breaks `test_key_urls`
RESOLVED — pnpm-lock.yaml — `terser: 5.48.0` snapshot mismatch
RESOLVED — packages/kolibri-sandbox/src/handlerLoader.js:44 — script tag and resolver leak on success
RESOLVED — packages/kolibri-sandbox/src/handlerLoader.js:44 — cleanup only on the `onerror` path
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:30 — `this.now` stored but never read
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:110 — `STATEUPDATE` handler registered twice
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js — `LOADING:false` gated on `contentWindow`
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — `rmtree` on a symlinked destination
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — copies truncation artifacts
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:21 — `now` documented Required but unenforced
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — `.gz` sidecars collected
RESOLVED — .github/workflows/release_kolibri.yml — dead `rm -f static/**/*.file_size`
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js:158 — vestigial `NOW` round-trip
RESOLVED — packages/kolibri-sandbox/test/mainClient.spec.js — asserts on a private mediator field
RESOLVED — kolibri/core/content/test/test_collectsandboxstatic.py — guard tests assert the data survives (praise; file superseded)
RESOLVED — packages/kolibri-build/src/collect_sandbox_static.js:102 — containment guard covered one direction
RESOLVED — .github/workflows/build_whl.yml:75 — artifact uploaded on every caller
RESOLVED — .gitignore:110 — output directory not ignored
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — polyfill-injection block never executes
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — hardcoded `corejs: '3.46'`
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — positional `splice(1, 0, ...)`
RESOLVED — packages/kolibri-sandbox/package.json — dead `lodash` and `toposort-class`
RESOLVED — packages/kolibri-sandbox/package.json — `mutationobserver-shim` in `dependencies`
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — inconsistent treatment of adjacent failure modes
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js:180 — `createIframe` dropped `contentState`/`userData`
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js:163 — `CustomContentRenderer` supplied no `handlerUrl`
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:146 — `DATARETURNED` relay dropped
RESOLVED — packages/kolibri/composables/internal/useSandbox.js — `onUserDataUpdate` never bound, Bloom reported no progress
RESOLVED — kolibri/plugins/html5_viewer/frontend/sandbox_handler/SCORMShim.js:602 — `SCORM.spec.js` deleted without replacement
RESOLVED — packages/kolibri/components/Fullscreen.vue:22 — duplicated `CoreFullscreen` instead of moving it
RESOLVED — packages/kolibri-build/src/collect_sandbox_static.js:45 — `isServeArtifact`/`collectFile` encode the serve-time contract (praise)
RESOLVED — packages/kolibri/components/SandboxedContentViewer/internal/SandboxedContentViewer.vue:15 — `options.width` no longer applied
RESOLVED — kolibri/core/content/hooks.py — `viewer_data` override rebuilt the whole payload
RESOLVED — packages/kolibri-sandbox/src/mainClient.js — `USERDATAUPDATE` broadcast to every shim
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js — vestigial `typeof setData === 'function'` guard
RESOLVED — packages/kolibri-sandbox/src/SandboxShim.js — nothing on main listened for `USERDATAUPDATE`
RESOLVED — kolibri/plugins/h5p_viewer/frontend/sandbox_handler/H5PHandler.js — H5P user data changed namespace
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js — `getData()` had no callers
RESOLVED — kolibri/plugins/bloompub_viewer/frontend/sandbox_handler/BloomRunner.js — unused `shim` constructor arg
RESOLVED — packages/kolibri-sandbox/src/SandboxShim.js:71 — blanket `STATEUPDATE` subscription reopened `sessionStorage` restore
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js — no re-entry guard on repeated `MAINREADY`
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js — duck-typed `setContentNamespace` in the base class
RESOLVED — kolibri/plugins/h5p_viewer/package.json:24 — `build-h5p` dependencies undeclared
RESOLVED — packages/kolibri-sandbox/src/kolibri.js — redundant `Mediator` escaping teardown
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js:113 — guard records the failure mode (praise)
ACKNOWLEDGED — packages/kolibri/composables/internal/useSandbox.js:95 — duration fallback now reaches H5P and Bloom
@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
| * They are responsible for initializing and managing content rendering. | ||
| * | ||
| * Usage: | ||
| * import { SandboxHandler } from 'kolibri-sandbox'; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: exports["."] is mainClient.js, which has no such export. In-tree handlers default-import kolibri-sandbox/SandboxHandler; document that. Same at SandboxShim.js:9.
| RESIZE: 'resize', | ||
| LOADING: 'loading', | ||
| ERROR: 'error', | ||
| BLOOMPAGESREAD: 'Pages Read', |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Bloom-specific event stranded in core, against AC 1. SandboxShim.on() rejects events outside this map, so BloomShim.js:18 cannot declare its own — nor can out-of-tree handlers. this.events is a per-instance copy: merge a subclass static events in the constructor, and document it.
| onMounted(() => { | ||
| viewer.iframeRef.value = iframeElement.value; | ||
|
|
||
| if (viewer.isSandboxed.value) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: an unregistered handler renders a permanent spinner. isSandboxed is false when getSandboxHandlerUrl returns null (mismatched sandbox_handler_id, or no JS chunk); with no else, loading stays true and stopTracking never fires. Call reportLoadingError, as at useSandbox.js:135.
3fe8c9f to
9b80169
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — all 55 prior findings resolved; 4 new suggestions, all on edges the restructure exposed rather than the architecture.
CI pending. Manual QA was required but did not run — nothing here is visually verified.
- Handler stats re-read from disk on every page render (
hooks.py) - SCORM
APIno longer installed onwindow.parent(Html5ZipHandler.js) - Custom channels fail silently when the preset lookup misses (
CustomContentRenderer.vue) h5p_build/vendor/is unignored inside the packaged plugin tree (.gitignore)
Prior-finding status
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — diverges from what alt_wsgi serves
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — no Python tests for the command or new hook methods
RESOLVED — packages/kolibri-sandbox/src/mainClient.js — contentWindow transiently null clobbers remote
RESOLVED — kolibri/plugins/h5p_viewer/frontend/sandbox_handler/xAPIShim.js:294 — Statement.clean validation dropped
RESOLVED — packages/kolibri/composables/internal/useSandbox.js — mediator message listener escapes teardown
RESOLVED — packages/kolibri/composables/internal/useSandbox.js:255 — sandbox should be a ref
RESOLVED — kolibri/core/content/hooks.py — _get_sandbox_handler_stats swallows OSError
RESOLVED — kolibri/core/content/hooks.py — inert try/except KeyError
RESOLVED — packages/kolibri/components/SandboxedContentViewer/internal/SandboxedContentViewer.vue:54 — untested non-trivial logic
RESOLVED — kolibri/core/content/hooks.py:178 — unconditional WebpackError on missing stats
RESOLVED — pnpm-lock.yaml — dangling terser 5.48.0 reference
RESOLVED — packages/kolibri-sandbox/src/handlerLoader.js:44 — script/resolver not torn down on success
RESOLVED — packages/kolibri-sandbox/src/handlerLoader.js:44 — cleanup only on onerror
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:30 — this.now stored but never read
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:110 — duplicate per-shim STATEUPDATE registration
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js — LOADING:false gated on contentWindow
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — rmtree on a symlinked destination
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — copies truncation artifacts
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:21 — now documented Required but unenforced
RESOLVED — kolibri/core/content/management/commands/collectsandboxstatic.py — .gz sidecars collected
RESOLVED — .github/workflows/release_kolibri.yml — dead rm -f static/**/*.file_size
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js:158 — vestigial NOW round-trip
RESOLVED — packages/kolibri-sandbox/test/mainClient.spec.js — asserts a private mediator field
ACKNOWLEDGED — kolibri/core/content/test/test_collectsandboxstatic.py — guard tests assert the at-risk data survives
RESOLVED — packages/kolibri-build/src/collect_sandbox_static.js:102 — containment guard covered one direction
RESOLVED — .github/workflows/build_whl.yml:75 — artifact uploaded on every caller
RESOLVED — .gitignore:110 — output directory not ignored
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — polyfill block never executes
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — hardcoded corejs 3.46
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — positional splice(1, 0, ...)
RESOLVED — packages/kolibri-sandbox/package.json — dead lodash and toposort-class
RESOLVED — packages/kolibri-sandbox/package.json — mutationobserver-shim in dependencies
RESOLVED — packages/kolibri-build/src/webpack.config.plugin.js — inconsistent treatment of adjacent failure modes
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js:180 — createIframe dropped contentState/userData
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js:163 — handlerUrl mandatory, CustomContentRenderer supplied none
RESOLVED — packages/kolibri-sandbox/src/mainClient.js:146 — DATARETURNED relay dropped
RESOLVED — packages/kolibri/composables/internal/useSandbox.js — onUserDataUpdate never bound, Bloom reported no progress
RESOLVED — kolibri/plugins/html5_viewer/frontend/sandbox_handler/SCORMShim.js:602 — SCORM.spec.js deleted without replacement
RESOLVED — packages/kolibri/components/Fullscreen.vue:22 — duplicates CoreFullscreen
ACKNOWLEDGED — packages/kolibri-build/src/collect_sandbox_static.js:45 — isServeArtifact/collectFile encode the serve-time contract
RESOLVED — packages/kolibri/components/SandboxedContentViewer/internal/SandboxedContentViewer.vue:15 — options.width no longer applied
RESOLVED — kolibri/core/content/hooks.py — viewer_data rebuilt instead of extending the parent's
RESOLVED — packages/kolibri-sandbox/src/mainClient.js — USERDATAUPDATE sent to every shim
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js — vestigial typeof setData guard
RESOLVED — packages/kolibri-sandbox/src/SandboxShim.js — nothing on main listens for USERDATAUPDATE
RESOLVED — kolibri/plugins/h5p_viewer/frontend/sandbox_handler/H5PHandler.js — H5P user data changed namespace
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js — getData() has no callers
RESOLVED — kolibri/plugins/bloompub_viewer/frontend/sandbox_handler/BloomRunner.js — unused shim constructor param
RESOLVED — packages/kolibri-sandbox/src/SandboxShim.js:81 — restore path opened for sessionStorage
RESOLVED — packages/kolibri-sandbox/src/iframeClient.js — no re-entry guard, clearIframe() never unsubscribed
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js — duck-typed setContentNamespace in the base class
RESOLVED — kolibri/plugins/h5p_viewer/package.json:24 — build-h5p dependencies undeclared
RESOLVED — packages/kolibri-sandbox/src/kolibri.js — redundant Mediator escaping teardown
ACKNOWLEDGED — packages/kolibri-sandbox/src/iframeClient.js:113 — guard records the failure mode
RESOLVED — packages/kolibri/composables/internal/useSandbox.js:105 — duration fallback reaching H5P and Bloom
RESOLVED — packages/kolibri-sandbox/src/SandboxHandler.js — exports["."] has no SandboxHandler export
RESOLVED — packages/kolibri-sandbox/src/base.js — Bloom-specific event stranded in core
RESOLVED — packages/kolibri/components/SandboxedContentViewer/internal/setup.js:51 — unregistered handler renders a permanent spinner
@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
| """Full unique ID for the sandbox handler bundle.""" | ||
| return "{}.{}".format(self._module_path, self.sandbox_handler_id) | ||
|
|
||
| def _get_sandbox_handler_stats(self): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: stats are re-read and re-parsed from disk on every page render. The parent caches the equivalent read (kolibri/core/webpack/hooks.py:87-90, _cached_stats_file_content gated on DEVELOPER_MODE); this one has no cache. viewer_data reaches sandbox_handler_url on every call, and {% content_viewer_assets %} (kolibri/core/templates/kolibri/base.html:47) renders viewer_data for every registered viewer on every HTML page load — three extra reads plus JSON parses per render with html5, h5p and bloompub registered. Mirroring the parent's _cached_*/DEVELOPER_MODE pattern keeps dev-mode rebuild behaviour and removes the steady-state cost.
(The unconditional WebpackError here is what I asked for last round and I still think it is right — this is a separate concern in the same method.)
| * - SCORM: For SCORM-based learning content | ||
| * (Kolibri data API is provided by SandboxHandler.baseShims) | ||
| */ | ||
| static shims = [SCORMShim]; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: SCORM's API moved off the window SCORM content looks at. On develop, SCORM was the one shim installed on the sandbox environment's own window, pointedly not in the per-content-window list — iframeClient.js:46-48 there: "We initialize SCORM here, as the usual place for SCORM to look for its API is window.parent." initializeIframe (develop L76-93) installed localStorage/sessionStorage/cookie/kolibri/H5P/xAPI/indexedDB on the content window, never SCORM.
As an ordinary entry in static shims, SandboxHandler._initializeShims now puts API on the content window and nothing on the sandbox window. Content using the spec's ScanForAPI(window) walk is fine; content that reads window.parent.API directly — a common shortcut, since in a conventional LMS the SCO is framed by the LMS page — now gets undefined where it worked before. Was that deliberate? If not, installing on both (an _initializeShims override here, or one iframeInitialize(window) from the constructor) restores the old surface without giving up the new one.
| // A custom channel is an HTML5 zip, so it renders through the handler registered | ||
| // for its preset; the base shims that handler brings are what supply window.kolibri. | ||
| this.sandbox.initialize({}, {}, urls.zipContentUrl(zipFile, 'index.html'), zipFile.checksum, { | ||
| handlerUrl: coreApp.getSandboxHandlerUrl(zipFile.preset), |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: custom channels fail silently when this preset lookup misses. (The handlerUrl itself exists because I asked for it last round; this is about its null path.)
getSandboxHandlerUrl returns null for an unregistered preset (pluginMediator.js:230). iframeClient.js:162-164 then throws handlerUrl is required and the catch emits ERROR on the sandbox namespace — but this mounted block registers no events.ERROR or events.LOADING handler, so the frame stays blank with nothing surfaced to the user. That happens whenever kolibri.plugins.html5_viewer is disabled (it is a disableable default plugin), or the topic's first extension === 'zip' file carries a preset other than html5_zip. On develop this path needed no handler and rendered regardless. At minimum bind events.ERROR here; better, check the URL before initialize and report a specific "no viewer available for this channel" state.
| !kolibri/core/content/static/bloom/ | ||
| # Check in h5p & bloom specific files in their respective plugins | ||
| !kolibri/plugins/h5p_viewer/static/h5p/ | ||
| !kolibri/plugins/bloompub_viewer/static/bloom/ |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: kolibri/plugins/h5p_viewer/h5p_build/vendor/ is now inside the packaged tree and unignored. downloadH5PVendor.js:13 unpacks the H5P PHP library into h5p_build/vendor/h5p and never removes it (the only fs.rm calls are the pre-download wipes). Two consequences that did not exist at packages/kolibri-sandbox/vendor:
MANIFEST.inhasgraft kolibri/pluginswith prunes only forfrontendandnode_modules, so a developer who ran the documentedbuild-h5pbeforemake distships the vendored library in the sdist/wheel.dependency_updates.yml:89-99hands the tree tocreate-pull-requestwith noadd-paths, so the directory rides along in the bot's PR.
One line next to the existing h5p/bloom entries closes both.
| * @param {object} options - Options as sent with MAINREADY | ||
| * @returns {Promise<void>} Resolves when the content has loaded, or failed to | ||
| */ | ||
| async createIframe(options = {}) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: the build-serialisation comment names what goes wrong without the guard — "the earlier run resumes to find this.iframe and this.handler replaced, and initializes its successor's iframe" — which is the one thing a future reader cannot recover from the code. Same instinct in SandboxShim.destroy and iframeInitialize, and the latter is backed by a test that says so.
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>
CoreFullscreen becomes the public Fullscreen; ViewerToolbar and the BaseToolbar it builds on move across unchanged. Viewer chrome has to be reachable from packages/kolibri, which cannot import kolibri-common. Usages updated in epub_viewer, learn, media_player, pdf_viewer and slideshow_viewer. 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_01GsUiw3pEyjdB2cCYu9PdBd
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 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
The pinned 3.46 lagged the installed 3.47, so any 3.47-only polyfill was never injected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012cWnDeSF5JegVZi66s9rTo
9b80169 to
30fd3f5
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15036 — 64 of 64 prior findings resolved; no new findings. CI pending and manual QA did not run, so: comment, not approval.
Prior-finding status
RESOLVED — collectsandboxstatic.py — diverged from alt_wsgi
RESOLVED — collectsandboxstatic.py — untested command/hooks
RESOLVED — mainClient.js — null contentWindow
RESOLVED — xAPIShim.js:294 — dropped Statement.clean
RESOLVED — useSandbox.js — listener escapes teardown
RESOLVED — useSandbox.js:255 — non-reactive sandbox
RESOLVED — hooks.py — swallowed OSError
RESOLVED — hooks.py — inert except KeyError
RESOLVED — SandboxedContentViewer.vue:54 — untested logic
RESOLVED — hooks.py:182 — unconditional WebpackError
RESOLVED — pnpm-lock.yaml — terser snapshot mismatch
RESOLVED — handlerLoader.js:44 — script leaked on success
RESOLVED — handlerLoader.js:44 — cleanup only onerror
RESOLVED — mainClient.js:30 — this.now unread
RESOLVED — mainClient.js:110 — duplicate STATEUPDATE
RESOLVED — iframeClient.js — LOADING:false gated
RESOLVED — collectsandboxstatic.py — unguarded rmtree
RESOLVED — collectsandboxstatic.py — truncation artifacts
RESOLVED — mainClient.js:21 — now unenforced
RESOLVED — collectsandboxstatic.py — .gz collected
RESOLVED — release_kolibri.yml — dead rm
RESOLVED — SandboxHandler.js:158 — vestigial NOW
RESOLVED — mainClient.spec.js — private field asserted
RESOLVED — test_collectsandboxstatic.py — guard tests (praise)
RESOLVED — collect_sandbox_static.js:102 — one-way containment guard
RESOLVED — build_whl.yml:75 — artifact on every caller
RESOLVED — .gitignore:110 — output dir unignored
RESOLVED — webpack.config.plugin.js — dead polyfill block
RESOLVED — webpack.config.plugin.js — hardcoded corejs
RESOLVED — webpack.config.plugin.js — positional splice
RESOLVED — kolibri-sandbox/package.json — dead deps
RESOLVED — kolibri-sandbox/package.json — mutationobserver-shim
RESOLVED — webpack.config.plugin.js — inconsistent failure modes
RESOLVED — iframeClient.js:180 — dropped contentState/userData
RESOLVED — iframeClient.js:163 — missing handlerUrl
RESOLVED — mainClient.js:146 — DATARETURNED relay dropped
RESOLVED — useSandbox.js — onUserDataUpdate unbound
RESOLVED — SCORMShim.js:602 — spec deleted
RESOLVED — Fullscreen.vue:22 — duplicated CoreFullscreen
RESOLVED — collect_sandbox_static.js:45 — serve contract (praise)
RESOLVED — SandboxedContentViewer.vue:15 — options.width
RESOLVED — hooks.py — viewer_data rebuilt payload
RESOLVED — mainClient.js — USERDATAUPDATE broadcast
RESOLVED — SandboxHandler.js — vestigial setData guard
RESOLVED — SandboxedContentViewer.vue:15 — options.width (2nd)
RESOLVED — SandboxShim.js — USERDATAUPDATE unheard
RESOLVED — H5PHandler.js — namespace change
RESOLVED — SandboxHandler.js — getData unused
RESOLVED — BloomRunner.js — unused shim arg
RESOLVED — SandboxShim.js:81 — sessionStorage restore
RESOLVED — iframeClient.js — no MAINREADY re-entry guard
RESOLVED — SandboxHandler.js — duck-typed setContentNamespace
RESOLVED — h5p_viewer/package.json:24 — undeclared deps
RESOLVED — kolibri.js — redundant Mediator
RESOLVED — iframeClient.js:113 — guard comment (praise)
RESOLVED — useSandbox.js:105 — duration fallback
RESOLVED — SandboxHandler.js — bad exports["."]
RESOLVED — base.js — Bloom event in core
RESOLVED — setup.js:51 — permanent spinner
RESOLVED — hooks.py:168 — stats re-read per render
RESOLVED — Html5ZipHandler.js:17 — SCORM API window
RESOLVED — CustomContentRenderer.vue — silent preset miss
RESOLVED — .gitignore:112 — vendor unignored
RESOLVED — iframeClient.js:123 — guard comment (praise)
@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
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.