feat: improve dev terminal error output#370
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Thank you for following the naming conventions! 🙏 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR adds comprehensive "pretty error" formatting for development terminal output. It introduces a dev-terminal configuration system ( ChangesPretty error flow across runtime, framework, and docs
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/evlog/test/nitro/plugin-enrichment.test.ts (1)
18-57: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftThese tests are exercising a clone of the production helper.
Lines 18-57 reimplement
callEnrichAndDraininstead of invoking the real Nitro helper, so the newdeferDrainassertions can stay green even ifpackages/evlog/src/nitro/plugin.tschanges, regresses, or wires hooks differently. Please extract the runtime helper into an importable module or drive the registered Nitro hooks from the test instead. As per coding guidelines, "Always import real source helpers in tests, never re-implement them in tests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/evlog/test/nitro/plugin-enrichment.test.ts` around lines 18 - 57, Tests reimplement the production helper callEnrichAndDrain instead of using the real runtime helper, causing brittle duplicates; replace the inline implementation by importing and using the shared helper from the production code (e.g., the runtime helper that invokes nitro hooks and handles deferDrain in packages/evlog/src/nitro/plugin.ts) or, alternatively, instantiate the real Nitro plugin and drive the registered hooks ('evlog:enrich' and 'evlog:drain') from the test so assertions exercise production behavior; update the test to call the imported helper (or trigger nitroApp.hooks.callHook via the actual plugin) and remove the local callEnrichAndDrain function.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/evlog/src/nitro-v3/plugin.ts`:
- Around line 278-280: The code currently sets ctx._evlogEmitted true before
running enrichErrorStackForDev, the evlog:emit:keep hook, runner.runKeep, and
log.emit(), so any exception there prevents the response fallback; change this
so the flag is only set after the keep/emit pipeline completes successfully: use
a temporary local "emitting" boolean or an emittedAfterSuccess variable inside
the same scope, run enrichErrorStackForDev(), trigger the evlog:emit:keep hook
and runner.runKeep(), call log.emit(), and only if all of those succeed set
ctx._evlogEmitted = true (apply the same change to the similar block around the
evlog emission at lines ~305-310). Ensure exceptions propagate normally if emit
fails so the response fallback can run.
- Around line 109-115: The deferDrain branch currently fire-and-forgets
drainPromise (options?.deferDrain) which prevents Nitro v3 from registering the
promise with the runtime; instead, when deferDrain is true and a waitUntil API
is available (e.g. event.req?.waitUntil or event?.req?.waitUntil), call that API
with drainPromise so Nitro/platform can keep the promise alive, and still attach
a .catch to log failures; likewise, ensure callEnrichAndDrain forwards options
into callDrainHook so callDrainHook can call event.req.waitUntil(drainPromise)
when available, and update the v3 error-hook call site (where callEnrichAndDrain
is currently invoked with void ... .catch(...)) to register the returned promise
with event.req.waitUntil when possible rather than discarding it.
In `@packages/evlog/src/nitro.ts`:
- Around line 164-166: The current branch in nitro.ts that handles
Array.isArray(errorHandler) only de-duplicates and returns the array unchanged
when it already contains handlerPath, which leaves handlerPath in its original
position; change this so that if errorHandler is an array and includes
handlerPath you remove any existing occurrences and return a new array with
handlerPath forced to the front (e.g., [handlerPath, ...otherHandlers])
otherwise if it does not include handlerPath return [handlerPath,
...errorHandler]; operate on errorHandler and handlerPath identifiers so Evlog
is always positioned first in the chain.
In `@packages/evlog/src/nitro/plugin.ts`:
- Around line 228-229: The flag e.context._evlogEmitted is being set before the
emit pipeline completes, which can mark requests as emitted even if later steps
throw; change the logic in the emit flow (the block that calls
enrichErrorStackForDev, emits the evlog:emit:keep hook, runner.runKeep, and
requestLog.emit()) so that you either use a separate in-progress flag (e.g.,
e.context._evlogEmitting) at the start and only set e.context._evlogEmitted =
true after requestLog.emit() resolves successfully, or move the assignment to
immediately after the successful requestLog.emit() call; apply the same fix to
the other similar blocks referenced around the existing occurrences (the other
emit branches where _evlogEmitted is set currently).
- Around line 112-121: The deferDrain branch currently fire-and-forgets
drainPromise (drainPromise.catch(...)) which can drop background drains when
invoked from Nitro's non-awaited error hook; update the logic in the deferDrain
handling in packages/evlog/src/nitro/plugin.ts so that when callEnrichAndDrain
is used from the error hook it attaches the drain promise to the request/event
lifetime (use event.waitUntil or ctx.waitUntil if available) instead of
unconditionally void-ing it; if the runtime cannot guarantee background work
outliving the response then fall back to using waitUntil, and only use the
current void+catch approach when you detect a runtime that guarantees background
tasks will outlive the response (or when no event/ctx is present). Ensure you
reference and update the call site callEnrichAndDrain(..., { deferDrain: true })
to pass the event/ctx into the drain handler so it can call
event.waitUntil(ctx.waitUntil) with drainPromise, while retaining the existing
drainPromise.catch(...) fallback.
In `@packages/evlog/src/nuxt/module.ts`:
- Around line 383-384: resolver.resolve(...) can return platform-native path
separators which break Nitro's errorHandler string on Windows; update the code
around resolver.resolve('../nitro/errorHandler') so evlogHandler is normalized
to POSIX-style separators before passing into prependNitroErrorHandler and
assigning nitroConfig.errorHandler (i.e., normalize the result of
resolver.resolve into a forward-slash path and then call
prependNitroErrorHandler(evlogHandlerNormalized, ...) / assign
nitroConfig.errorHandler using the normalized value).
In `@packages/evlog/src/shared/pretty-error.ts`:
- Line 75: The SKIP_FRAME_PATH_RE currently treats any path containing
"/error.ts" or "/error.mjs" as internal which incorrectly filters user files;
update SKIP_FRAME_PATH_RE so the error\.(ts|mjs) fragment is only matched when
preceded by evlog-specific path segments (for example require the same
(?:packages[/\\]evlog|evlog[/\\](?:dist|src)) prefix before error\.(?:ts|mjs)),
or split into two checks: one regex for evlog package paths and a separate
stricter check for an evlog error file, ensuring SKIP_FRAME_PATH_RE only
excludes frames that are definitely inside evlog (reference: SKIP_FRAME_PATH_RE
constant in pretty-error.ts).
---
Outside diff comments:
In `@packages/evlog/test/nitro/plugin-enrichment.test.ts`:
- Around line 18-57: Tests reimplement the production helper callEnrichAndDrain
instead of using the real runtime helper, causing brittle duplicates; replace
the inline implementation by importing and using the shared helper from the
production code (e.g., the runtime helper that invokes nitro hooks and handles
deferDrain in packages/evlog/src/nitro/plugin.ts) or, alternatively, instantiate
the real Nitro plugin and drive the registered hooks ('evlog:enrich' and
'evlog:drain') from the test so assertions exercise production behavior; update
the test to call the imported helper (or trigger nitroApp.hooks.callHook via the
actual plugin) and remove the local callEnrichAndDrain function.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d609913f-0d62-4c9f-9231-80ea1c3c4375
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.changeset/pretty-terminal-errors.mdapps/docs/content/2.learn/2.wide-events.mdapps/docs/content/2.learn/3.structured-errors.mdapps/docs/content/3.integrate/frameworks/01.nuxt.mdapps/docs/content/3.integrate/frameworks/02.nextjs.mdapps/docs/content/6.reference/1.configuration.mdapps/playground/app/components/playground/TestCard.vueapps/playground/app/composables/useTestRunner.tsapps/playground/app/composables/useTestState.tsapps/playground/app/config/tests.config.tspackages/evlog/src/logger.tspackages/evlog/src/nitro-v3/errorHandler.tspackages/evlog/src/nitro-v3/module.tspackages/evlog/src/nitro-v3/plugin.tspackages/evlog/src/nitro.tspackages/evlog/src/nitro/errorHandler.tspackages/evlog/src/nitro/module.tspackages/evlog/src/nitro/plugin.tspackages/evlog/src/nuxt/module.tspackages/evlog/src/shared/define.tspackages/evlog/src/shared/enrich-error-stack.node.tspackages/evlog/src/shared/pretty-error-snippet.node.tspackages/evlog/src/shared/pretty-error.tspackages/evlog/src/types.tspackages/evlog/test/core/pretty-error.test.tspackages/evlog/test/nitro/errorHandler.test.tspackages/evlog/test/nitro/plugin-enrichment.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/evlog/src/nitro/errorHandler.ts (1)
23-25: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExtract shared
NitroErrorHandlerContexttype.The
NitroErrorHandlerContexttype is duplicated in bothpackages/evlog/src/nitro/errorHandler.tsandpackages/evlog/src/nitro-v3/errorHandler.ts. Consider extracting it to a shared location (e.g.,packages/evlog/src/nitro.tsor a newpackages/evlog/src/shared/nitro-types.ts) to follow DRY principles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/evlog/src/nitro/errorHandler.ts` around lines 23 - 25, Extract the duplicated NitroErrorHandlerContext type into a shared module (e.g., create packages/evlog/src/shared/nitro-types.ts or packages/evlog/src/nitro.ts), export the type from that module, and update both errorHandler.ts files (packages/evlog/src/nitro/errorHandler.ts and packages/evlog/src/nitro-v3/errorHandler.ts) to import NitroErrorHandlerContext from the new shared location; ensure the exported type signature matches the original (including defaultHandler signature) and run type checks to confirm no other references need updating.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/docs/content/2.learn/3.structured-errors.md`:
- Line 229: The table cell containing the text "`dev: 'evlog'` (default when
`pretty: true` in dev)" is awkwardly phrased; update that parenthetical to a
clearer form such as "default in pretty dev" or "(default in pretty development
mode)" so it reads e.g. "`dev: 'evlog'` (default in pretty dev)"; locate and
edit the exact string "`dev: 'evlog'` (default when `pretty: true` in dev)" in
the file (apps/docs/content/2.learn/3.structured-errors.md) and replace it with
the chosen clearer wording.
In `@packages/evlog/src/nitro/enrich-drain.ts`:
- Around line 101-117: Wrap runner.runEnrich in a try/catch so an enrich
exception doesn't prevent draining (use runner.hasEnrich before calling
runner.runEnrich and log or handle the error inside the catch), and ensure
runner.runDrain is added to drainTasks with a rejection handler like the hook
call (i.e., push runner.runDrain(drainCtx).catch(...)) or use Promise.allSettled
on drainTasks later so any rejection from runner.runDrain (or the hook) doesn't
propagate to the caller; update references: runner.hasEnrich, runner.runEnrich,
runner.hasDrain, runner.runDrain, and nitroApp.hooks.callHook('evlog:drain',
drainCtx) accordingly.
In `@packages/evlog/src/nitro/errorHandler.ts`:
- Line 27: The handler signature passed to defineNitroErrorHandler must reflect
Nitro's contract: allow async/MaybePromise returns and accept a context object
where defaultHandler is required; update the function signature/export so the
third parameter uses the correct NitroErrorHandlerContext type (not optional)
and ensure the handler return type permits Promise (i.e., keep async or use
MaybePromise), and remove any optional/undefined typing for ctx.defaultHandler
so callers and implementations treat defaultHandler as present.
In `@packages/evlog/test/core/dev-terminal.test.ts`:
- Around line 12-65: Add a new unit test for the object-form dev config to cover
the code path in resolveDevTerminal that accepts an object: call
resolveDevTerminal with dev set to an object (e.g., { frameworkOverlay: true,
prettyError: { snippet: false, stackDepth: 1, compact: false, detail: 'guidance'
} }) while NODE_ENV is 'development' and assert the returned frameworkOverlay
and prettyError match the provided object (use the same test file and pattern as
existing tests in dev-terminal.test.ts and reference the resolveDevTerminal
function).
In `@packages/evlog/test/nitro-v3/errorHandler.test.ts`:
- Around line 10-39: Add tests to bring nitro-v3 errorHandler coverage in line
with the v2 suite: extend packages/evlog/test/nitro-v3/errorHandler.test.ts to
include cases for EvlogError serialization with data fields, deriving status
from error.cause and default status handling, excluding internal context from
serialized output, sanitizing production messages differently for 5xx vs 4xx,
verifying H3 event _handled flag behavior, and testing overlay suppression via
resetNitroDevOverlayCache; target the errorHandler function in these tests and
reuse helpers from the v2 tests as needed so the same behaviors are asserted for
the v3 implementation.
---
Outside diff comments:
In `@packages/evlog/src/nitro/errorHandler.ts`:
- Around line 23-25: Extract the duplicated NitroErrorHandlerContext type into a
shared module (e.g., create packages/evlog/src/shared/nitro-types.ts or
packages/evlog/src/nitro.ts), export the type from that module, and update both
errorHandler.ts files (packages/evlog/src/nitro/errorHandler.ts and
packages/evlog/src/nitro-v3/errorHandler.ts) to import NitroErrorHandlerContext
from the new shared location; ensure the exported type signature matches the
original (including defaultHandler signature) and run type checks to confirm no
other references need updating.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 82c07dcf-103c-4946-aa62-b78db2f089e5
📒 Files selected for processing (25)
.changeset/pretty-terminal-errors.mdapps/docs/content/2.learn/3.structured-errors.mdapps/docs/content/3.integrate/frameworks/01.nuxt.mdapps/docs/content/6.reference/1.configuration.mdapps/playground/server/api/test/structured-error.get.tspackages/evlog/src/logger.tspackages/evlog/src/nitro-v3/errorHandler.tspackages/evlog/src/nitro-v3/plugin.tspackages/evlog/src/nitro.tspackages/evlog/src/nitro/enrich-drain.tspackages/evlog/src/nitro/errorHandler.tspackages/evlog/src/nitro/plugin.tspackages/evlog/src/nuxt/module.tspackages/evlog/src/shared/define.tspackages/evlog/src/shared/dev-terminal.tspackages/evlog/src/shared/nitroConfigBridge.tspackages/evlog/src/shared/pretty-error.tspackages/evlog/src/types.tspackages/evlog/test/core/dev-terminal.test.tspackages/evlog/test/core/pretty-error.test.tspackages/evlog/test/nitro-v3/errorHandler.test.tspackages/evlog/test/nitro/errorHandler.test.tspackages/evlog/test/nitro/plugin-enrichment.test.tspackages/evlog/test/nitro/plugin.test.tspackages/evlog/test/shared/nitroConfigBridge.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/docs/content/2.learn/3.structured-errors.md (1)
232-232: 🧹 Nitpick | 🔵 Trivial | 💤 Low valuePhrasing could be simplified for consistency.
The parenthetical "(default when
pretty: truein dev)" is verbose. Consider "default in pretty dev" or "(default in pretty development mode)" for consistency with the changeset and configuration docs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/docs/content/2.learn/3.structured-errors.md` at line 232, Simplify the parenthetical in the table row containing "`dev: 'evlog'` (default when `pretty: true` in dev)`" by replacing "(default when `pretty: true` in dev)" with a more concise phrase such as "(default in pretty dev)" or "(default in pretty development mode)"; update the markdown table cell that includes the `dev: 'evlog'` and `pretty: true` text to use the chosen concise wording for consistency with the changeset and configuration docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@apps/docs/content/2.learn/3.structured-errors.md`:
- Line 232: Simplify the parenthetical in the table row containing "`dev:
'evlog'` (default when `pretty: true` in dev)`" by replacing "(default when
`pretty: true` in dev)" with a more concise phrase such as "(default in pretty
dev)" or "(default in pretty development mode)"; update the markdown table cell
that includes the `dev: 'evlog'` and `pretty: true` text to use the chosen
concise wording for consistency with the changeset and configuration docs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b01d86f1-2e31-4da7-8a8f-9bf36350bf27
📒 Files selected for processing (5)
apps/docs/content/2.learn/2.wide-events.mdapps/docs/content/2.learn/3.structured-errors.mdapps/docs/content/3.integrate/frameworks/02.nextjs.mdpackages/evlog/src/nitro/plugin.tspackages/evlog/test/core/pretty-error.test.ts
💤 Files with no reviewable changes (1)
- packages/evlog/src/nitro/plugin.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
dev: 'evlog' | 'nitro' | 'both' | object) to choose overlay/pretty-error presets.Bug Fixes
Documentation