refactor(development): inline loader trace template as string constant - #5869
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe loader-trace endpoint now serves a pre-imported HTML template and injects timing data via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5869 +/- ##
=======================================
Coverage 85.48% 85.49%
=======================================
Files 660 661 +1
Lines 18828 18830 +2
Branches 3646 3646
=======================================
+ Hits 16096 16099 +3
+ Misses 2361 2360 -1
Partials 371 371 ☔ 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
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). |
f5d9b74 to
d60ef6e
Compare
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-1: Filename uses underscores instead of hyphens.Per the repo convention, files under
plugins/**should be named in lowercase with hyphens (e.g.loader-trace-template.ts). That said, the siblingegg_loader_trace.tsalready uses snake_case, so feel free to defer this if you prefer consistency with the surrounding middleware files or address it in a broader rename.As per coding guidelines: "Name files in lowercase with hyphens (e.g.
loader-context.ts)".🤖 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` at line 1, The file name uses underscores instead of the repo convention of lowercase hyphens; rename the module from loader_trace_template.ts to loader-trace-template.ts and update all import sites that reference loader_trace_template (and any sibling references like egg_loader_trace.ts) to the new hyphenated filename so module resolution continues to work; ensure export names inside the file remain unchanged and run the test/build to catch any missed imports.
🤖 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/loader_trace_template.ts`:
- Line 11: The template insertion currently uses
LOADER_TRACE_TEMPLATE.replace('{{placeholder}}', JSON.stringify(data)) which
lets `$` sequences in the JSON be interpreted as replacement patterns; change
that call to use a replacer function instead, e.g.
LOADER_TRACE_TEMPLATE.replace('{{placeholder}}', () => JSON.stringify(data)), so
the payload is inserted literally and any `$` in trace entry names (module
ids/paths) won't be misinterpreted by String.prototype.replace.
---
Nitpick comments:
In `@plugins/development/src/app/middleware/loader_trace_template.ts`:
- Line 1: The file name uses underscores instead of the repo convention of
lowercase hyphens; rename the module from loader_trace_template.ts to
loader-trace-template.ts and update all import sites that reference
loader_trace_template (and any sibling references like egg_loader_trace.ts) to
the new hyphenated filename so module resolution continues to work; ensure
export names inside the file remain unchanged and run the test/build to catch
any missed imports.
🪄 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: 8b83133b-b711-4901-a551-6101a072ebf1
📒 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
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/development/src/app/middleware/egg_loader_trace.ts
- plugins/development/package.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/timing.test.ts`:
- Around line 44-68: The test 'should safely serialize trace data into inline
script' writes agent_timing_safe_serialize.json into app.config.rundir but never
removes it; wrap the test body in a try-finally (around the fs.writeFile,
request, and assertions) and in the finally remove the fixture (use fs.unlink or
fs.unlinkSync) targeting the file named "agent_timing_safe_serialize.json" in
app.config.rundir so subsequent tests aren't contaminated; ensure the file
removal runs regardless of test outcome and handle missing-file errors silently.
🪄 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: 11e78b26-f7c8-4b90-98dd-6e9cf25aadc9
📒 Files selected for processing (3)
plugins/development/src/app/middleware/egg_loader_trace.tsplugins/development/src/app/middleware/loader-trace-template.tsplugins/development/test/timing.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/development/src/app/middleware/egg_loader_trace.ts
There was a problem hiding this comment.
Pull request overview
This PR refactors the @eggjs/development loader-trace middleware to inline the HTML template as a TypeScript string constant, removing the runtime filesystem read so the plugin can be bundled by static bundlers (e.g. turbopack).
Changes:
- Inline loader trace HTML into
LOADER_TRACE_TEMPLATEand import it from the middleware. - Remove
readFileof the template at runtime and instead replace a placeholder in the inlined string. - Add a test to ensure trace JSON is safely serialized for embedding into an inline
<script>.
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 | Adds an inlined HTML template constant for loader trace rendering. |
| plugins/development/src/app/middleware/egg_loader_trace.ts | Switches middleware to use the inlined template and adds safe JSON serialization for inline script embedding. |
| plugins/development/test/timing.test.ts | Adds coverage for safe serialization when trace data contains </script>-style payloads. |
Comments suppressed due to low confidence (1)
plugins/development/src/app/middleware/loader-trace-template.ts:2
- This file uses kebab-case (
loader-trace-template.ts), but middleware modules in this repo/plugins are typically named with snake_case (e.g.packages/egg/src/app/middleware/override_method.ts,plugins/mock/src/app/middleware/cluster_app_mock.ts) and this directory already hasegg_loader_trace.ts. Consider renaming this file toloader_trace_template.ts(and updating the import) to match established conventions and avoid inconsistent paths.
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) <noreply@anthropic.com>
a718a43 to
f4ac1b7
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the @eggjs/development loader-trace middleware to be friendlier to static bundlers (e.g. turbopack) by inlining the HTML template into a TypeScript string constant and removing runtime filesystem reads. It also adds safe serialization when embedding trace data into an inline <script> and introduces a regression test for that behavior.
Changes:
- Inline loader trace HTML into
LOADER_TRACE_TEMPLATE(TypeScript constant) instead of reading an external template file at request time. - Add
serializeLoaderTraceData()to safely embed JSON into an inline script and update middleware to use it. - Add a test to validate escaping/round-trip parsing of potentially hostile trace payloads.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| plugins/development/test/timing.test.ts | Adds a test ensuring trace data is safely serialized into the inline script and still parses back correctly. |
| plugins/development/src/app/middleware/loader-trace-template.ts | Introduces the inlined HTML template as a exported string constant. |
| plugins/development/src/app/middleware/egg_loader_trace.ts | Switches from runtime template file reads to the inlined constant and adds safe JSON serialization for script embedding. |
Comments suppressed due to low confidence (1)
plugins/development/src/app/middleware/loader-trace-template.ts:2
- PR description references
plugins/development/src/app/middleware/loader_trace_template.ts, but the actual inlined template module in this change isloader-trace-template.ts. Please align the description (or the filename) so future readers can find the file quickly.
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
Bug Fixes
Performance
Tests