Skip to content

refactor(onerror): inline error page template as string constant#5868

Merged
killagu merged 5 commits intoeggjs:nextfrom
killagu:split/02-onerror-inline-template
Apr 26, 2026
Merged

refactor(onerror): inline error page template as string constant#5868
killagu merged 5 commits intoeggjs:nextfrom
killagu:split/02-onerror-inline-template

Conversation

@killagu
Copy link
Copy Markdown
Contributor

@killagu killagu commented Apr 14, 2026

Summary

Inlines the 1336-line error page HTML template into plugins/onerror/src/lib/onerror_page.ts as a string constant. Removes the runtime readFileSync + import.meta.dirname lookup for the default template, sets the templatePath config default to an empty string, and updates app.ts to lazy-load the built-in template only when no custom template path is configured.

The fallback app-info serialization path now redacts sensitive config values when app.dumpConfigToObject() is unavailable. This is an observable security improvement for rendered error output; user-supplied templatePath behavior remains unchanged.

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.

Turbopack (and any static bundler) cannot follow import.meta.dirname + readFileSync to a template file, so the file would be missing in a bundled deployment. Inlining the HTML as a string constant makes the plugin statically bundleable.

Test plan

  • pnpm exec vitest run plugins/onerror/test/onerror.test.ts - 39/39 passed
  • pnpm --filter=@eggjs/onerror typecheck
  • pnpm run build
  • pnpm exec oxlint --type-aware --type-check plugins/onerror/src/app.ts plugins/onerror/src/lib/error_view.ts plugins/onerror/src/lib/onerror_page.ts plugins/onerror/test/onerror.test.ts plugins/onerror/tsdown.config.ts - 0 errors, 1 existing warning on app.close()

Stack context

Other batch-1 PRs (independent, can land in any order):

  • feat(utils): add setBundleModuleLoader runtime hook
  • refactor(development): inline loader trace template as string constant
  • refactor(watcher): use direct class imports for event sources

Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Built-in error page template now exposed for consumers; includes stack-frame visualization, filtering, frame selection, inline code preview, and syntax highlighting.
  • Bug Fixes & Improvements

    • Built-in template used by default when no custom template path is set.
    • Safer frame selection and more robust client-side error-page behavior.
    • Config serialization now redacts sensitive values and handles circular references.
  • Tests

    • Added tests verifying config redaction in serialized error output.
  • Chores

    • Added a public package subpath export to allow importing the error-page asset.

Copilot AI review requested due to automatic review settings April 14, 2026 03:59
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 14, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The onerror plugin now embeds and exports a built-in HTML error-page template, exposes it as a public subpath, defaults onerror.templatePath to empty to select the builtin template, loads either a file template or the builtin constant at startup, adds config-serialization/redaction with circular detection to ErrorView, and includes a test for redaction.

Changes

Cohort / File(s) Summary
Package Configuration
plugins/onerror/package.json
Added public subpath export ./lib/onerror_page mapping to source (./src/lib/onerror_page.ts) and published artifact (./dist/lib/onerror_page.js).
Configuration
plugins/onerror/src/config/config.default.ts
Changed default onerror.templatePath to '' (empty selects built-in template); updated JSDoc and removed node:path usage.
App Initialization
plugins/onerror/src/app.ts
Guarded sync file read of template; if config.templatePath is falsy, dynamically import/use ONERROR_PAGE_TEMPLATE instead of reading disk.
Built-in Template
plugins/onerror/src/lib/onerror_page.ts
Now exports ONERROR_PAGE_TEMPLATE (full HTML). Re-escaped regexes for template embedding, hardened client-side frame selection guards, and adjusted a UI label.
Error View / Serialization
plugins/onerror/src/lib/error_view.ts
Added serializeConfig(), getConfigIgnoreList(), redactConfig(...), and shouldRedactConfigKey(...) to produce redacted config output and handle circular refs.
Tests
plugins/onerror/test/onerror.test.ts
Added test verifying ErrorView.serializeAppInfo() redacts sensitive config values while preserving non-sensitive ones and imports ErrorView directly.
Build Config
plugins/onerror/tsdown.config.ts
Removed copy step; tsdown config now empty (defineConfig({})) so asset-copy behavior eliminated.

Sequence Diagram(s)

sequenceDiagram
  participant App as App Init
  participant Config as Config
  participant FS as FileSystem
  participant Template as Built-in Template
  participant Client as HTTP Client

  App->>Config: read onerror.templatePath
  alt templatePath truthy
    App->>FS: read template file
    FS-->>App: template content
    App->>Client: serve rendered error page (file template)
  else templatePath empty
    App->>Template: import ONERROR_PAGE_TEMPLATE
    Template-->>App: template content
    App->>Client: serve rendered error page (built-in)
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • fengmk2

Poem

🐰 I hid a page inside my paw,

When paths go empty, I show the draw.
I hush the secrets, mask the keys,
Hop through frames on gentle breeze.
A tiny rabbit guards your seas.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main objective: inlining the error page template as a string constant instead of loading from a file.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request inlines the error page template into a new TypeScript file, onerror_page.ts, and updates the onerror plugin to use this template by default when templatePath is not configured. This change simplifies the plugin by removing the need to read an external HTML file from the file system in the default case. Feedback was provided to correct minor string formatting issues within the inlined JavaScript template, specifically regarding line number alignment and the display of method locations.

Comment thread plugins/onerror/src/lib/onerror_page.ts Outdated
Comment thread plugins/onerror/src/lib/onerror_page.ts Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
plugins/onerror/src/lib/onerror_page.ts (1)

710-722: Consider limiting sensitive information exposure in error pages.

The error page displays appInfo.baseDir and appInfo.config in non-production environments. While this is useful for debugging, be mindful that this could expose sensitive configuration values if not properly filtered upstream.

Based on learnings: "Never expose sensitive configuration values in logs or error messages" - the filtering should be handled in ErrorView to ensure sensitive config values are redacted before rendering.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/onerror/src/lib/onerror_page.ts` around lines 710 - 722, The error
page currently renders raw appInfo.baseDir and appInfo.config which can leak
secrets; update ErrorView to sanitize appInfo before rendering by implementing a
redaction step (e.g., sanitizeAppInfo or redactConfig) that removes or masks
sensitive keys/values from appInfo.config and optionally obfuscates baseDir,
then ensure the template rendering in onerror_page.ts uses the sanitizedAppInfo
fields (sanitizedAppInfo.config and sanitizedAppInfo.baseDir) instead of the raw
appInfo; keep the redaction centralized in ErrorView so all error rendering
paths use the sanitized object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@plugins/onerror/src/lib/onerror_page.ts`:
- Around line 710-722: The error page currently renders raw appInfo.baseDir and
appInfo.config which can leak secrets; update ErrorView to sanitize appInfo
before rendering by implementing a redaction step (e.g., sanitizeAppInfo or
redactConfig) that removes or masks sensitive keys/values from appInfo.config
and optionally obfuscates baseDir, then ensure the template rendering in
onerror_page.ts uses the sanitizedAppInfo fields (sanitizedAppInfo.config and
sanitizedAppInfo.baseDir) instead of the raw appInfo; keep the redaction
centralized in ErrorView so all error rendering paths use the sanitized object.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e249aee1-450d-499f-86e3-1f74048ca748

📥 Commits

Reviewing files that changed from the base of the PR and between 490f849 and f99e0e7.

📒 Files selected for processing (4)
  • plugins/onerror/package.json
  • plugins/onerror/src/app.ts
  • plugins/onerror/src/config/config.default.ts
  • plugins/onerror/src/lib/onerror_page.ts

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Refactors the onerror plugin to inline the error page HTML template as a TypeScript string constant so it can be statically bundled (e.g., by Turbopack), removing the runtime import.meta.dirname + readFileSync dependency.

Changes:

  • Adds an inlined ONERROR_PAGE_TEMPLATE constant containing the full error page template.
  • Changes the default templatePath config to '' (use built-in template when empty).
  • Updates the boot logic to use the inlined template unless a custom templatePath is provided, and exports the new module entry.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
plugins/onerror/src/lib/onerror_page.ts Adds the inlined HTML template constant used as the default error page.
plugins/onerror/src/config/config.default.ts Removes default filesystem template path; documents and defaults templatePath to empty string.
plugins/onerror/src/app.ts Switches to inlined template when templatePath is not set; retains file override behavior.
plugins/onerror/package.json Exposes the new lib/onerror_page entry for source and dist builds.

Comment thread plugins/onerror/src/lib/onerror_page.ts Outdated
Comment thread plugins/onerror/src/lib/onerror_page.ts
Comment thread plugins/onerror/src/lib/onerror_page.ts
Comment thread plugins/onerror/src/app.ts Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.52%. Comparing base (4a345cc) to head (1e98ce1).
⚠️ Report is 2 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #5868      +/-   ##
==========================================
+ Coverage   85.48%   85.52%   +0.03%     
==========================================
  Files         660      661       +1     
  Lines       18828    18861      +33     
  Branches     3646     3658      +12     
==========================================
+ Hits        16096    16131      +35     
+ Misses       2361     2359       -2     
  Partials      371      371              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread plugins/onerror/src/config/config.default.ts
@killagu killagu force-pushed the split/02-onerror-inline-template branch from f99e0e7 to 4254124 Compare April 21, 2026 15:17
@killagu
Copy link
Copy Markdown
Contributor Author

killagu commented Apr 25, 2026

已按 review comments 把这轮 onerror 修改收完:默认模板改成按需加载,删除了遗留 mustache 文件,补了 frame 相关空值 guard,同时修了 join("\\n") 和 method/line-column 展示分隔。相关讨论线程我这边一并 resolve 了。

Copilot AI review requested due to automatic review settings April 25, 2026 12:12
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
plugins/onerror/test/onerror.test.ts (1)

82-112: Consider also covering the defaultConfigIgnoreList fallback.

The new test always supplies dump.ignore, so the catch branch in getConfigIgnoreList() (which falls back to defaultConfigIgnoreList) is never exercised. A second tiny case constructing an ErrorView whose app.config lacks a dump property would lock in the default-list behavior (e.g. password, keys, /secret/i).

it('should fall back to default ignore list when dump.ignore is unavailable', () => {
  const view = new ErrorView(
    { request: {}, app: { config: { baseDir: '/tmp/app', password: 'p', keys: 'k', token_secret: 's', normal: 'v' } } } as any,
    new Error('test') as any,
    '',
  );
  const { config } = view.serializeAppInfo();
  assert.match(config, /<Redacted>/);
  assert(!config.includes("'p'"));
  assert(!config.includes("'k'"));
  assert(!config.includes("'s'"));
  assert.match(config, /normal: 'v'/);
});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/onerror/test/onerror.test.ts` around lines 82 - 112, The test suite
currently only covers the path where app.config.dump.ignore is provided, so add
a second test that constructs an ErrorView without a dump property to exercise
getConfigIgnoreList()'s fallback to defaultConfigIgnoreList; instantiate
ErrorView (same pattern as the existing test) with app.config containing
sensitive keys like password/keys/token_secret and a visible key, call
serializeAppInfo() and assert the serialized config redacts sensitive values
(contains "<Redacted>" and does not include the raw sensitive strings) while
still including the visible key—this ensures defaultConfigIgnoreList behavior is
validated.
plugins/onerror/src/lib/error_view.ts (1)

340-361: Special object types degrade to {} in the rendered error page.

The typeof === 'object' branch then Object.keys(...) path iterates enumerable own keys only, so Date, RegExp, URL, Map, Set, Buffer, and class instances all get serialized as plain objects (typically empty {}), unlike egg core's convertValue in packages/egg/src/lib/core/utils.ts which preserves them via toString() / primitive passthrough. Since this is the fallback used when dumpConfigToObject is unavailable, consider mirroring that handling for consistent inspect output:

   redactConfig(value: unknown, ignoreList: (string | RegExp)[], ...): unknown {
     if (!value || typeof value !== 'object') {
       return value;
     }
+    if (value instanceof RegExp || value instanceof URL || typeof value === 'symbol') {
+      return (value as { toString(): string }).toString();
+    }
+    if (Object.getPrototypeOf(value) !== Object.prototype && !Array.isArray(value)) {
+      // Date, Map, Set, Buffer, class instances → defer to util.inspect later
+      return value;
+    }
     ...
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/onerror/src/lib/error_view.ts` around lines 340 - 361, The
redactConfig function currently treats any non-null object by iterating
Object.keys and thus serializes special types (Date, RegExp, URL, Map, Set,
Buffer, class instances) to empty objects; update redactConfig (and where it
calls shouldRedactConfigKey) to detect these special types before the
Object.keys path and return sensible primitive/string/array representations
instead (e.g., Date/RegExp/URL -> toString(), Buffer -> Buffer.toString('utf8')
or base64, Map -> Array.from(map.entries()), Set -> Array.from(set), and for
other class instances fall back to instance.toString() or a shallow
property-redaction that preserves their string form). Keep the
circular-detection via seen, still recurse for arrays, and only use the
Object.keys branch for plain objects so rendered error pages match egg core's
convertValue behavior.
plugins/onerror/src/lib/onerror_page.ts (2)

1320-1322: Minor: $method/$file can be null and will render as the string "null".

getAttribute returns null when the attribute is absent, so if a frame is ever rendered without data-method/data-file (e.g., a future change to the Mustache template, or a frame produced by a stack-trace parser variant), line 1321 produces innerHTML = "null" and line 1322 produces "null lineColumn". Today Mustache always emits these, so it's not currently broken — flagging as a small defensive nit while you're touching this code.

Suggested defensive coercion
-            $('#frame-file').innerHTML = $file;
-            $('#frame-method').innerHTML = $method + ' ' + $lineColumn;
+            $('#frame-file').innerHTML = $file || '';
+            $('#frame-method').innerHTML = ($method || '') + ' ' + ($lineColumn || '');
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/onerror/src/lib/onerror_page.ts` around lines 1320 - 1322, Guard
against null attribute values when assigning to innerHTML for element IDs
'#code-drop', '#frame-file', and '#frame-method' by coercing $file and $method
to safe strings (e.g., use an empty string or a default like '(unknown)') before
assignment; ensure $lineColumn is appended only if present so that
$('#frame-method').innerHTML doesn't become "null lineColumn" — locate where
$file, $method, and $lineColumn are read/used and replace direct assignments
with a small safe-coercion step for each variable.

2-1345: Maintainability note on the inlined ~1.3k-line template.

Inlining the template as a TS string constant achieves the stated goal (statically bundleable, no import.meta.dirname + readFileSync), but it does fold a sizable chunk of vendored Prism.js source into the TS module — every future Prism upgrade will require re-doubling backslashes by hand. Two low-effort options to keep the ergonomics without giving up bundleability:

  1. Keep the template in onerror_page.html and use a build-time transform (tsup/tsc plugin, unplugin-raw, or a tiny prebuild script that emits onerror_page.ts with JSON.stringify(html)). This gives you HTML syntax highlighting, no manual escaping, and the same runtime behavior.
  2. If you want to stay in TS, consider String.raw for the regex-heavy script blocks so the doubled backslashes aren't required — though this conflicts with the surrounding template literal, so it would need to be a concatenated piece.

Not blocking — purely a future-maintenance suggestion. Consider whether option (1) fits the wider 19-PR refactor.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/onerror/src/lib/onerror_page.ts` around lines 2 - 1345, The
ONERROR_PAGE_TEMPLATE constant embeds a ~1.3k-line HTML/Prism script which is
painful to maintain (escaped backslashes etc.); fix by moving the literal into a
raw HTML asset and generating the TS export at build-time: create an
onerror_page.html with the full template and add a build step (unplugin-raw,
tsup/tsc plugin, or a small prebuild script) that reads that file and emits a TS
module exporting ONERROR_PAGE_TEMPLATE (e.g., via JSON.stringify or by writing
the literal into the generated file), or alternatively split the large
regex-heavy script into a separate String.raw segment and concatenate it into
ONERROR_PAGE_TEMPLATE to avoid double-escaping; update references that
import/use ONERROR_PAGE_TEMPLATE to consume the generated module.
🤖 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/onerror/src/lib/error_view.ts`:
- Around line 340-361: redactConfig currently uses a WeakSet named seen as a
global visited set so shared non-circular objects are labeled "[Circular]";
change the logic to track only the current ancestor chain by adding the object
to seen before recursing into its children and removing it (delete from the
WeakSet) after finishing that subtree so siblings can visit the same object
without being treated as circular; ensure this add/remove pattern is applied for
both object and array branches inside redactConfig and keep using
shouldRedactConfigKey and redactedValue as the redaction checks.

---

Nitpick comments:
In `@plugins/onerror/src/lib/error_view.ts`:
- Around line 340-361: The redactConfig function currently treats any non-null
object by iterating Object.keys and thus serializes special types (Date, RegExp,
URL, Map, Set, Buffer, class instances) to empty objects; update redactConfig
(and where it calls shouldRedactConfigKey) to detect these special types before
the Object.keys path and return sensible primitive/string/array representations
instead (e.g., Date/RegExp/URL -> toString(), Buffer -> Buffer.toString('utf8')
or base64, Map -> Array.from(map.entries()), Set -> Array.from(set), and for
other class instances fall back to instance.toString() or a shallow
property-redaction that preserves their string form). Keep the
circular-detection via seen, still recurse for arrays, and only use the
Object.keys branch for plain objects so rendered error pages match egg core's
convertValue behavior.

In `@plugins/onerror/src/lib/onerror_page.ts`:
- Around line 1320-1322: Guard against null attribute values when assigning to
innerHTML for element IDs '#code-drop', '#frame-file', and '#frame-method' by
coercing $file and $method to safe strings (e.g., use an empty string or a
default like '(unknown)') before assignment; ensure $lineColumn is appended only
if present so that $('#frame-method').innerHTML doesn't become "null lineColumn"
— locate where $file, $method, and $lineColumn are read/used and replace direct
assignments with a small safe-coercion step for each variable.
- Around line 2-1345: The ONERROR_PAGE_TEMPLATE constant embeds a ~1.3k-line
HTML/Prism script which is painful to maintain (escaped backslashes etc.); fix
by moving the literal into a raw HTML asset and generating the TS export at
build-time: create an onerror_page.html with the full template and add a build
step (unplugin-raw, tsup/tsc plugin, or a small prebuild script) that reads that
file and emits a TS module exporting ONERROR_PAGE_TEMPLATE (e.g., via
JSON.stringify or by writing the literal into the generated file), or
alternatively split the large regex-heavy script into a separate String.raw
segment and concatenate it into ONERROR_PAGE_TEMPLATE to avoid double-escaping;
update references that import/use ONERROR_PAGE_TEMPLATE to consume the generated
module.

In `@plugins/onerror/test/onerror.test.ts`:
- Around line 82-112: The test suite currently only covers the path where
app.config.dump.ignore is provided, so add a second test that constructs an
ErrorView without a dump property to exercise getConfigIgnoreList()'s fallback
to defaultConfigIgnoreList; instantiate ErrorView (same pattern as the existing
test) with app.config containing sensitive keys like password/keys/token_secret
and a visible key, call serializeAppInfo() and assert the serialized config
redacts sensitive values (contains "<Redacted>" and does not include the raw
sensitive strings) while still including the visible key—this ensures
defaultConfigIgnoreList behavior is validated.
🪄 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: 39b00bad-75ec-4cf5-b606-6bf89f1f5fb1

📥 Commits

Reviewing files that changed from the base of the PR and between 4254124 and 7bbda13.

📒 Files selected for processing (4)
  • plugins/onerror/src/app.ts
  • plugins/onerror/src/lib/error_view.ts
  • plugins/onerror/src/lib/onerror_page.ts
  • plugins/onerror/test/onerror.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • plugins/onerror/src/app.ts

Comment thread plugins/onerror/src/lib/error_view.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

plugins/onerror/src/lib/onerror_page.ts:2

  • src/lib/onerror_page.mustache.html no longer exists (template is now inlined), but plugins/onerror/tsdown.config.ts still tries to copy that file into dist/. This will likely break the package build/publish step; update the build config to remove that copy step (or point it at the new source) now that the template lives in onerror_page.ts.

Comment thread plugins/onerror/src/lib/error_view.ts
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
plugins/onerror/src/lib/onerror_page.ts (1)

1321-1321: Minor: leading space when method is empty.

getAttribute('data-method') returns the rendered Mustache value, which is the empty string when frame.method is undefined. In that case the label becomes " 12:34" with a stray leading space. Non-blocking, but trivial to tighten:

✏️ Optional polish
-            $('#frame-method').innerHTML = $method + ' ' + $lineColumn;
+            $('#frame-method').innerHTML = $method ? $method + ' ' + $lineColumn : $lineColumn;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/onerror/src/lib/onerror_page.ts` at line 1321, The label can get a
stray leading space when getAttribute('data-method') returns an empty string;
update the assignment to $('#frame-method').innerHTML to only include the
separating space when $method is non-empty (e.g. use [$method,
$lineColumn].filter(Boolean).join(' ') or conditional expression like $method ?
$method + ' ' + $lineColumn : $lineColumn) so the element shows "12:34" instead
of " 12:34" when frame.method is undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@plugins/onerror/src/lib/onerror_page.ts`:
- Line 1321: The label can get a stray leading space when
getAttribute('data-method') returns an empty string; update the assignment to
$('#frame-method').innerHTML to only include the separating space when $method
is non-empty (e.g. use [$method, $lineColumn].filter(Boolean).join(' ') or
conditional expression like $method ? $method + ' ' + $lineColumn : $lineColumn)
so the element shows "12:34" instead of " 12:34" when frame.method is undefined.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 62887ebf-2e74-4112-a842-cb64474193d2

📥 Commits

Reviewing files that changed from the base of the PR and between 7bbda13 and 751032d.

📒 Files selected for processing (1)
  • plugins/onerror/src/lib/onerror_page.ts

Copilot AI review requested due to automatic review settings April 25, 2026 12:25
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
plugins/onerror/src/lib/error_view.ts (1)

340-365: ⚠️ Potential issue | 🟡 Minor

Shared (non-circular) references are still marked as [Circular].

The seen WeakSet is added to but never removed after a subtree finishes processing, so any object referenced from two distinct keys (e.g. a shared logger / env / options sub-object on app.config) will be replaced with '[Circular]' on its second occurrence even though there is no real cycle. Track only the current ancestor chain (add before recursing, delete in a finally after) so siblings can revisit the same object safely.

🐛 Proposed fix
   redactConfig(
     value: unknown,
     ignoreList: (string | RegExp)[],
-    seen: WeakSet<object> = new WeakSet<object>(),
+    ancestors: WeakSet<object> = new WeakSet<object>(),
   ): unknown {
     if (!value || typeof value !== 'object') {
       return value;
     }

-    if (seen.has(value)) {
+    if (ancestors.has(value)) {
       return '[Circular]';
     }
-    seen.add(value);
-
-    if (Array.isArray(value)) {
-      return value.map((item) => this.redactConfig(item, ignoreList, seen));
-    }
-
-    const result: Record<string, unknown> = {};
-    for (const key of Object.keys(value)) {
-      result[key] = this.shouldRedactConfigKey(key, ignoreList)
-        ? redactedValue
-        : this.redactConfig((value as Record<string, unknown>)[key], ignoreList, seen);
-    }
-    return result;
+    ancestors.add(value);
+    try {
+      if (Array.isArray(value)) {
+        return value.map((item) => this.redactConfig(item, ignoreList, ancestors));
+      }
+      const result: Record<string, unknown> = {};
+      for (const key of Object.keys(value)) {
+        result[key] = this.shouldRedactConfigKey(key, ignoreList)
+          ? redactedValue
+          : this.redactConfig((value as Record<string, unknown>)[key], ignoreList, ancestors);
+      }
+      return result;
+    } finally {
+      ancestors.delete(value);
+    }
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/onerror/src/lib/error_view.ts` around lines 340 - 365, redactConfig
currently marks shared (non-circular) objects as "[Circular]" because it adds
entries to the seen WeakSet and never removes them; change redactConfig so seen
only tracks the current ancestor chain: when you detect a non-primitive
object/array, check seen.has(value) as now, then add value to seen before
recursing into its children and remove it from seen in a finally block after
processing (for both Array.isArray branch and object property loop) so shared
sibling references are not misclassified as circular; keep references to
redactConfig, shouldRedactConfigKey, and redactedValue when implementing this
change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@plugins/onerror/src/lib/error_view.ts`:
- Around line 340-365: redactConfig currently marks shared (non-circular)
objects as "[Circular]" because it adds entries to the seen WeakSet and never
removes them; change redactConfig so seen only tracks the current ancestor
chain: when you detect a non-primitive object/array, check seen.has(value) as
now, then add value to seen before recursing into its children and remove it
from seen in a finally block after processing (for both Array.isArray branch and
object property loop) so shared sibling references are not misclassified as
circular; keep references to redactConfig, shouldRedactConfigKey, and
redactedValue when implementing this change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8572f32d-1116-4650-bbd0-944eb32dcf83

📥 Commits

Reviewing files that changed from the base of the PR and between 751032d and 72a6557.

📒 Files selected for processing (2)
  • plugins/onerror/src/lib/error_view.ts
  • plugins/onerror/tsdown.config.ts

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

killagu and others added 5 commits April 26, 2026 09:16
Inline the mustache error page template as a string constant in
src/lib/onerror_page.ts so plugins can be statically bundled by
turbopack. Previously the template was read from disk via
`import.meta.dirname + readFileSync`, which breaks when modules are
embedded in a single bundled chunk.

`templatePath` default is now `''`; if set, the custom template file
is still loaded via fs, otherwise the inlined constant is used.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 26, 2026 01:21
@killagu killagu force-pushed the split/02-onerror-inline-template branch from 58e915b to 1e98ce1 Compare April 26, 2026 01:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comment on lines +349 to +353
if (value instanceof Date || value instanceof RegExp || value instanceof URL) {
return value.toString();
}

if (Buffer.isBuffer(value)) {
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the fallback redaction path, Set/Map values (e.g. Egg’s default config.dump.ignore is a Set) will currently fall through to the generic object handling and end up serialized as {} because Object.keys(new Set()) is empty. Consider adding explicit handling for Set/Map (e.g. convert to arrays/entries and recursively redact) before the generic object branch so AppInfo stays informative.

Copilot uses AI. Check for mistakes.
@killagu killagu merged commit 0b6c9fb into eggjs:next Apr 26, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants