refactor(development): inline loader trace template as string constant#5869
refactor(development): inline loader trace template as string constant#5869killagu wants to merge 1 commit intoeggjs:nextfrom
Conversation
Inline the loader trace visualization HTML as a string constant in src/app/middleware/loader_trace_template.ts so the plugin can be statically bundled by turbopack. Previously the template was read from disk via `import.meta.dirname + fs.readFile`, which breaks when modules are embedded in a single bundled chunk. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
📝 WalkthroughWalkthroughThe changes introduce a new template module for loader trace visualization, refactor the loader trace middleware to use a statically imported template instead of dynamic file reading, and configure the new module as a public export in the package.json. Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the egg_loader_trace middleware by inlining the HTML visualization template into a new TypeScript module, which eliminates the need for runtime file system reads. The review identified a potential XSS vulnerability where data is injected into the script block, suggesting character escaping as a fix. Additionally, it is recommended to remove the new internal template module from the package exports to keep the public API clean and to update the template's JavaScript to use const instead of var for better practice.
| const template = await fs.readFile(templatePath, 'utf8'); | ||
| const data = await loadTimingData(app); | ||
| ctx.body = template.replace('{{placeholder}}', JSON.stringify(data)); | ||
| ctx.body = LOADER_TRACE_TEMPLATE.replace('{{placeholder}}', JSON.stringify(data)); |
There was a problem hiding this comment.
Directly injecting JSON.stringify(data) into a <script> block is vulnerable to Cross-Site Scripting (XSS) if any string within data contains the sequence </script>. This would cause the browser to terminate the script block prematurely and potentially execute malicious code. It is recommended to escape the < character to prevent this injection.
| ctx.body = LOADER_TRACE_TEMPLATE.replace('{{placeholder}}', JSON.stringify(data)); | |
| ctx.body = LOADER_TRACE_TEMPLATE.replace('{{placeholder}}', JSON.stringify(data).replace(/</g, '\\u003c')); |
| "./agent": "./src/agent.ts", | ||
| "./app": "./src/app.ts", | ||
| "./app/middleware/egg_loader_trace": "./src/app/middleware/egg_loader_trace.ts", | ||
| "./app/middleware/loader_trace_template": "./src/app/middleware/loader_trace_template.ts", |
There was a problem hiding this comment.
The loader_trace_template module appears to be an internal implementation detail used only by egg_loader_trace.ts. Exporting it in package.json unnecessarily increases the public API surface of the package. Since it is imported relatively within the package, this export entry should be removed.
| "./agent": "./dist/agent.js", | ||
| "./app": "./dist/app.js", | ||
| "./app/middleware/egg_loader_trace": "./dist/app/middleware/egg_loader_trace.js", | ||
| "./app/middleware/loader_trace_template": "./dist/app/middleware/loader_trace_template.js", |
There was a problem hiding this comment.
As mentioned for the exports field, this internal template module does not need to be exposed in publishConfig.exports.
| <div id="mountNode"></div> | ||
| <script src="https://gw.alipayobjects.com/os/antv/assets/g2/3.0.9/g2.min.js"></script> | ||
| <script> | ||
| var data = {{placeholder}}; |
There was a problem hiding this comment.
Use const instead of var for variable declarations to adhere to modern JavaScript best practices and improve maintainability.
| var data = {{placeholder}}; | |
| const data = {{placeholder}}; |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5869 +/- ##
=======================================
Coverage 86.00% 86.01%
=======================================
Files 667 668 +1
Lines 18945 18944 -1
Branches 3652 3652
=======================================
Hits 16294 16294
+ Misses 2297 2296 -1
Partials 354 354 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/development/src/app/middleware/loader_trace_template.ts (1)
1-2: Align file and exported variable naming with repo conventions.Please rename the file to hyphen-case and the exported constant to camelCase to match project standards (and then update import/export paths accordingly).
♻️ Suggested rename pattern
-/** Loader trace visualization template - inlined from loader_trace.html */ -export const LOADER_TRACE_TEMPLATE = `<!doctype html> +/** Loader trace visualization template - inlined from loader_trace.html */ +export const loaderTraceTemplate = `<!doctype html>And update references, e.g.:
-import { LOADER_TRACE_TEMPLATE } from './loader_trace_template.ts'; +import { loaderTraceTemplate } from './loader-trace-template.ts';As per coding guidelines, "
{packages,plugins}/**/*.{ts,tsx,js,mjs}: Name files in lowercase with hyphens" and "**/*.{ts,tsx}: Name functions and variables in camelCase".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/development/src/app/middleware/loader_trace_template.ts` around lines 1 - 2, The file name and exported constant violate naming conventions: rename the file from loader_trace_template.ts to loader-trace-template.ts (hyphen-case) and rename the exported constant LOADER_TRACE_TEMPLATE to loaderTraceTemplate (camelCase); update all import/export sites that reference loader_trace_template.ts and LOADER_TRACE_TEMPLATE to use the new file name and new symbol (loader-trace-template and loaderTraceTemplate) so builds and imports resolve correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/development/src/app/middleware/egg_loader_trace.ts`:
- Line 16: The current insertion uses raw JSON.stringify(data) into
LOADER_TRACE_TEMPLATE which can allow string fields like </script> to break out;
update the assignment that sets ctx.body (the call using
LOADER_TRACE_TEMPLATE.replace(...)) to first harden the serialization by
escaping it via the framework helper (e.g. use
ctx.helper.escape(JSON.stringify(data))) or equivalent escaping that neutralizes
</script> and other HTML-sensitive characters, then call replace with that
escaped string so the inline script cannot be broken out.
---
Nitpick comments:
In `@plugins/development/src/app/middleware/loader_trace_template.ts`:
- Around line 1-2: The file name and exported constant violate naming
conventions: rename the file from loader_trace_template.ts to
loader-trace-template.ts (hyphen-case) and rename the exported constant
LOADER_TRACE_TEMPLATE to loaderTraceTemplate (camelCase); update all
import/export sites that reference loader_trace_template.ts and
LOADER_TRACE_TEMPLATE to use the new file name and new symbol
(loader-trace-template and loaderTraceTemplate) so builds and imports resolve
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79bfc717-0fa3-4976-b5f8-264fd4b68b87
📒 Files selected for processing (3)
plugins/development/package.jsonplugins/development/src/app/middleware/egg_loader_trace.tsplugins/development/src/app/middleware/loader_trace_template.ts
| const template = await fs.readFile(templatePath, 'utf8'); | ||
| const data = await loadTimingData(app); | ||
| ctx.body = template.replace('{{placeholder}}', JSON.stringify(data)); | ||
| ctx.body = LOADER_TRACE_TEMPLATE.replace('{{placeholder}}', JSON.stringify(data)); |
There was a problem hiding this comment.
Escape serialized data before injecting it into inline <script>.
Raw JSON.stringify(data) can include </script> in string fields and break out of the script tag during HTML parsing. Please harden the serialization before replace(...).
🔒 Suggested hardening
- ctx.body = LOADER_TRACE_TEMPLATE.replace('{{placeholder}}', JSON.stringify(data));
+ const serializedData = JSON.stringify(data)
+ .replace(/</g, '\\u003C')
+ .replace(/>/g, '\\u003E')
+ .replace(/&/g, '\\u0026')
+ .replace(/\u2028/g, '\\u2028')
+ .replace(/\u2029/g, '\\u2029');
+ ctx.body = LOADER_TRACE_TEMPLATE.replace('{{placeholder}}', serializedData);As per coding guidelines, "Use 'ctx.helper.escape()' and framework security helpers to prevent XSS attacks in template rendering and output".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ctx.body = LOADER_TRACE_TEMPLATE.replace('{{placeholder}}', JSON.stringify(data)); | |
| const serializedData = JSON.stringify(data) | |
| .replace(/</g, '\\u003C') | |
| .replace(/>/g, '\\u003E') | |
| .replace(/&/g, '\\u0026') | |
| .replace(/\u2028/g, '\\u2028') | |
| .replace(/\u2029/g, '\\u2029'); | |
| ctx.body = LOADER_TRACE_TEMPLATE.replace('{{placeholder}}', serializedData); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/development/src/app/middleware/egg_loader_trace.ts` at line 16, The
current insertion uses raw JSON.stringify(data) into LOADER_TRACE_TEMPLATE which
can allow string fields like </script> to break out; update the assignment that
sets ctx.body (the call using LOADER_TRACE_TEMPLATE.replace(...)) to first
harden the serialization by escaping it via the framework helper (e.g. use
ctx.helper.escape(JSON.stringify(data))) or equivalent escaping that neutralizes
</script> and other HTML-sensitive characters, then call replace with that
escaped string so the inline script cannot be broken out.
There was a problem hiding this comment.
Pull request overview
This PR refactors @eggjs/development’s loader-trace middleware to inline the HTML template as a TypeScript string constant, removing the runtime filesystem read so the plugin can be statically bundled (e.g., by turbopack).
Changes:
- Added
LOADER_TRACE_TEMPLATEconstant containing the inlined loader-trace HTML. - Updated
egg_loader_tracemiddleware to use the inlined template instead of readingloader_trace.htmlat runtime. - Exposed the new template module via
package.jsonsubpath exports.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plugins/development/src/app/middleware/loader_trace_template.ts | Introduces the inlined HTML template as an exported string constant. |
| plugins/development/src/app/middleware/egg_loader_trace.ts | Switches middleware rendering from fs.readFile() to the inlined template constant. |
| plugins/development/package.json | Adds a new exported subpath for the template module (src + dist). |
| @@ -0,0 +1,51 @@ | |||
| /** Loader trace visualization template - inlined from loader_trace.html */ | |||
| "./app/middleware/egg_loader_trace": "./src/app/middleware/egg_loader_trace.ts", | ||
| "./app/middleware/loader_trace_template": "./src/app/middleware/loader_trace_template.ts", | ||
| "./config/config.default": "./src/config/config.default.ts", |
Summary
Inlines the loader trace HTML template into `plugins/development/src/app/middleware/loader_trace_template.ts` as a string constant. Removes the runtime fs read in `egg_loader_trace.ts` middleware.
Why
This is batch 1, part of a 19-PR split of #5863 (the egg-bundler PR). #5863 is kept open as a tracking reference. This PR is independent of the other batch-1 PRs.
Same motivation as the parallel onerror inline change: turbopack and other static bundlers cannot follow `import.meta.dirname + readFileSync` lookups to plugin template files. Inlining the template as a string constant lets the plugin be statically bundled.
Pure refactor — runtime behavior is identical.
Test plan
Stack context
Other batch-1 PRs (independent, can land in any order):
🤖 Generated with Claude Code
Summary by CodeRabbit