Add dynamic RTL CSS loading support for webpack bundles#14479
Add dynamic RTL CSS loading support for webpack bundles#14479rtibbles wants to merge 2 commits intolearningequality:release-v0.19.xfrom
Conversation
github-actions
bot
added
DEV: renderers
rtibbles
force-pushed
the
claude/rtl-dynamic-loading-011CUuNvu6QLYGJdquD6gQmq
branch
from
34f0b26 to
5280e8b
Compare
Build Artifacts
Smoke test screenshot |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Solid approach to enabling RTL-compatible CSS code splitting via webpack runtime integration. The architecture — core RTLManager singleton, per-bundle registration, miniCssF intercept, STAGE_ATTACH ordering — is well thought out.
CI passing. No UI verification needed (the only .vue change adds contentDirection to Perseus's setup() destructuring — a data flow fix, not visual).
- blocking: Race condition in
reloadBundleCSS—onerrorand double-rAF fire independently, so a load failure still removes the old CSS (see inline) - suggestion:
_findRtlModuleIdreturning null produces__webpack_require__(null)at runtime (see inline) - suggestion: Duplicate runtime-ordering tests (see inline)
- suggestion:
loadDirectionalCSSno longer swaps initial CSS loaded viascriptLoaderfor content viewers (see inline) - praise: Excellent explanatory comments throughout, especially the STAGE_ATTACH ordering rationale and
createCssInsertserialization workaround
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
| link.remove(); | ||
| resolve(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
blocking: Race condition between onerror and the double requestAnimationFrame.
If the new CSS fails to load:
onerrorfires → removesnewLink, callsreject✓- The already-queued rAF callbacks still fire →
link.remove()removes the old CSS,resolve()is ignored (promise settled)
Result: both old and new CSS are removed, leaving no styles. The comment on line 104 says "keep old CSS if new one fails" but the implementation doesn't achieve that.
Fix: gate the rAF callback behind a flag that onerror clears, or cancel the rAF. For example:
let errored = false;
newLink.onerror = () => {
errored = true;
newLink.remove();
reject(new Error(`Failed to load ${newHref}`));
};
requestAnimationFrame(() => {
requestAnimationFrame(() => {
if (!errored) {
link.remove();
resolve();
}
});
});There was a problem hiding this comment.
Fixed. Added an errored flag that gates the rAF callback — if onerror fires first, the old CSS link is preserved.
| return this.compilation.chunkGraph.getModuleId(module); | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
suggestion: _findRtlModuleId returns null if no module has rawRequest === 'kolibri/rtlcss'. The generated code becomes __webpack_require__(null), which would fail at runtime. This shouldn't happen in practice (the module is in apiSpec.js), but a build-time error or warning here would surface misconfiguration early rather than producing a cryptic runtime failure.
There was a problem hiding this comment.
_findRtlModuleId now pushes a WebpackError compilation warning when the module is not found. This surfaces the misconfiguration at build time. A hard throw would break test compilations that legitimately lack the module, and the runtime __webpack_require__(null) still fails loudly if it ever reaches production.
| const bundleName = contentViewerModule.name; | ||
|
|
||
| if (direction === languageDirections.RTL) { | ||
| return rtlManager.enableRTL(bundleName); |
There was a problem hiding this comment.
suggestion: The new loadDirectionalCSS delegates entirely to rtlManager, which only operates on <link> elements tagged with data-webpack-bundle. But content viewer initial CSS is loaded via scriptLoader (lines 398 above), which doesn't add that attribute.
So if a content viewer needs to switch direction after initial load (e.g., RTL content in an LTR UI), the initial bundle CSS won't be swapped — only dynamically loaded chunks will be. The old implementation handled this by finding and replacing the initial CSS URL.
The PR description acknowledges this scope limitation. Can you confirm this is acceptable for the current use cases, or is there a plan to tag the initial CSS links as well?
There was a problem hiding this comment.
No, this was a big oversight and a good catch, this has been reworked to properly make sure that the initial dynamically loaded CSS is properly handled.
| }); | ||
|
|
||
| describe('runtime module ordering', () => { | ||
| it('should order RTLCSSRuntimeModule to run after GetChunkFilenameRuntimeModule', async () => { |
There was a problem hiding this comment.
suggestion: This test (lines 96–148) and the one at lines 265–348 in the integration with MiniCssExtractPlugin describe block both verify the same thing: RTL runtime module orders after the mini-css module. The integration block also creates its own tempDir that shadows the outer one. Consider consolidating into one test to reduce duplication and webpack compilation time.
There was a problem hiding this comment.
Consolidated — removed the duplicate ordering test from the integration block and eliminated the shadowed tempDir.
| // which defines miniCssF at STAGE_NORMAL (0). Runtime modules are sorted by | ||
| // stage first, then by identifier. Without this, "RTL CSS Loading" sorts before | ||
| // "get mini-css chunk filename" alphabetically and the miniCssF intercept fails. | ||
| super('RTL CSS Loading', STAGE_ATTACH); |
There was a problem hiding this comment.
praise: The STAGE_ATTACH comment (lines 52–55) is excellent — clearly explains the non-obvious ordering dependency between runtime modules and why alphabetical sorting would break the miniCssF intercept. This is the kind of comment that prevents future regressions.
| function createCssInsert(bundleId) { | ||
| // MiniCssExtractPlugin serializes functions with .toString(), losing closures. | ||
| // Use new Function to bake the bundleId into the function's source code. | ||
| return new Function( |
There was a problem hiding this comment.
praise: Smart use of new Function to bake the bundleId as a string literal into the function body, working around MiniCssExtractPlugin's .toString() serialization. Well-documented constraint, and the test at line 460 in the spec file verifies the serialization roundtrip.
rtibbles
force-pushed
the
claude/rtl-dynamic-loading-011CUuNvu6QLYGJdquD6gQmq
branch
from
5280e8b to
75c536c
Compare
Implements per-bundle RTL (Right-to-Left) CSS loading capability that allows individual webpack bundles to dynamically switch between LTR and RTL CSS variants without page reloads. This is essential for content viewers that need to display RTL content within an LTR UI or vice versa. Core RTL Manager (packages/kolibri/rtlcss.js): - Singleton RTLManager class that tracks per-bundle RTL state - Initializes bundle RTL state based on current global language direction - CSS link swapping with double RAF for FOUC prevention - Error handling for missing CSS files (keeps old CSS on failure) WebpackRTLPlugin enhancements: - Preserves existing RTL CSS generation via rtlcss - Injects runtime code into each bundle via webpack RuntimeModule - Differentiates between core and plugin bundles (isCoreBundle flag) - Core bundles: access rtlManager via __webpack_require__ - Plugin bundles: access via window[kolibriName] externals - Intercepts miniCssF to transform CSS chunk URLs based on RTL state - Adds link tag label generation to MiniCssExtractPlugin to allow easy swapping of CSS files Package updates: - Added kolibri/rtlcss to package.json exports - Added kolibri/rtlcss to apiSpec.js for external bundle access - Pass isCoreBundle flag from webpack config to RTL plugin
Replaces the manual CSS link tracking and swapping logic with the new RTL manager for content viewers. This removes several limitations: Previous implementation: - Only supported one CSS file per module (noted in code comments) - Required manual DOM manipulation and tracking via urlTags - Complex logic for finding and swapping CSS files New implementation: - Uses rtlManager.enableRTL/disableRTL with bundle name - Handles multiple CSS files per bundle automatically - More robust error handling via Promise-based API - Removes ~40 lines of complex link tracking code Changes in pluginMediator.js: - Import rtlManager from kolibri/rtlcss - Simplify loadDirectionalCSS to delegate to rtlManager - Remove urlTags tracking in retrieveContentViewer - Content viewers now benefit from automatic CSS direction switching The kolibri-viewer package requires no changes as it delegates to the mediator's loadDirectionalCSS method.
rtibbles
force-pushed
the
claude/rtl-dynamic-loading-011CUuNvu6QLYGJdquD6gQmq
branch
from
75c536c to
adf4f6c
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
All four prior findings addressed cleanly. The errored flag, WebpackError warning, test consolidation, and content viewer CSS rework all look correct.
Prior findings
Resolved:
- Race condition in
reloadBundleCSS(blocking) —erroredflag added atrtlcss.js:156 _findRtlModuleIdreturning null (suggestion) —WebpackErrorwarning pushed atwebpackRtlPlugin.js:86-89- Duplicate runtime-ordering tests (suggestion) — consolidated, shadowed
tempDirremoved loadDirectionalCSSnot handling initial CSS (suggestion) — reworked withregisterBundleCSS/loadRegisteredBundleCSS
4/4 prior findings resolved. 0 re-raised.
CI passing. No UI changes to verify (Perseus .vue change is a data-flow fix adding contentDirection to setup() destructuring).
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| link.href = href; | ||
| link.setAttribute('data-webpack-bundle', bundleId); | ||
| document.body.appendChild(link); | ||
| } |
There was a problem hiding this comment.
suggestion: Newly created links here are appended to document.body, while createCssInsert (used by MiniCssExtractPlugin for async chunks) appends to document.head. Both work — CSS applies regardless of DOM location — but the inconsistency could confuse someone debugging link element placement. Consider using document.head here for consistency, or adding a comment explaining why document.body is the right choice for content viewer CSS.
| // Now run the deferred rAF callbacks — the errored flag should prevent old link removal | ||
| rAFCallbacks.forEach(cb => cb()); | ||
|
|
||
| await expect(reloadPromise).rejects.toThrow('Failed to load'); |
There was a problem hiding this comment.
praise: This race condition test is well-constructed — collecting rAF callbacks without executing them, firing onerror first, then replaying the callbacks to verify the errored flag gates correctly. Good verification of the fix.
3 participants
Summary
Enable webpack CSS code splitting to work with RTL by replacing the manual
scriptLoader-based CSS swapping with a webpack runtime integration. AnRTLManagermodule exposed on the core app tracks per-bundle RTL state, and the RTL webpack plugin injects aminiCssFintercept into each bundle's runtime so dynamically loaded CSS chunks automatically resolve to.rtl.cssvariants when needed.References
Closes #13536
Reviewer guidance
pnpm devserver(notdevserver-hot) since HMR usesstyle-loaderwhich bypasses MiniCssExtractPlugin. To verify CSS code splitting works, convert a route component to an async import. For example, inkolibri/plugins/learn/frontend/routes/index.js:miniCssFintercept inwebpackRtlPlugin.jsresolves thekolibri/rtlcssmodule ID differently for core vs plugin bundles — core looks up the real webpack module ID at code generation time, plugins use the window global. Worth verifying both paths in the generated output.reloadBundleCSSinrtlcss.jsuses doublerequestAnimationFrameinstead ofonloadfor CSS swap timing — this only covers link elements tagged withdata-webpack-bundle, which currently only applies to async CSS chunks loaded by MiniCssExtractPlugin's runtime.loadDirectionalCSSmust destructurecontentDirectionfromuseContentViewerin theirsetup(). Currently only Perseus and EPUB do this — Perseus was missing it and is fixed in this PR.AI usage
Implemented with Claude Code. Debugged the content viewer loading stall by driving a browser to reproduce, then tracing the promise chain to find the root cause.