feat(pino): Logging Plugin #1191
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
bot
added
the
size:XL
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Handler as StandardHandler
participant Plugin as LoggingHandlerPlugin
participant Logger as PinoLogger
participant Ctx as Context
Client->>Handler: send RPC request
Handler->>Plugin: rootInterceptor(create per-request logger)
Plugin->>Logger: create child logger (id, req meta)
Plugin->>Ctx: attach logger via CONTEXT_LOGGER_SYMBOL
Plugin-->>Handler: continue with enriched context
Handler->>Plugin: interceptor — pre-exec
Plugin->>Logger: info "request received" (if enabled)
alt success
Handler->>Handler: execute procedure
Plugin->>Logger: info "request handled" (if enabled)
else error
Handler->>Plugin: interceptor catches error
alt aborted signal
Plugin->>Logger: info "request aborted" (if enabled)
else business/internal error
Plugin->>Logger: error(...)
end
end
Note over Handler,Client: Streaming outputs are wrapped to log stream errors and preserve abort semantics
Handler-->>Client: response / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Summary of ChangesHello @unnoq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature: an experimental Pino logging plugin for oRPC. This plugin streamlines the process of adding structured logging, request tracking, and robust error monitoring to oRPC applications. It includes a new package, comprehensive documentation, and necessary updates to the build configuration and dependencies. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a new experimental plugin for Pino integration, providing structured logging capabilities to oRPC applications. The implementation is well-structured, with comprehensive test coverage for various scenarios including request lifecycle, error handling, and streaming responses. The documentation is clear, though one of the code examples could be improved by adding missing imports to make it fully self-contained. Overall, this is a great addition to the oRPC ecosystem.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new Pino logging integration package and updates Vue dependencies from version 3.5.22 to 3.5.24.
- Introduces
@orpc/experimental-pinopackage for structured logging with Pino - Updates Vue from 3.5.22 to 3.5.24 across the monorepo
- Enhances type safety of
getfunction in shared package to acceptPropertyKey[]instead ofstring[]
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updates dependency versions for Vue (3.5.22 → 3.5.24) and adds pino@10.1.0 |
| packages/shared/src/object.ts | Improves get function type signature to accept PropertyKey[] (includes symbols) |
| packages/pino/* | New package providing Pino integration with LoggingHandlerPlugin, context utilities, and comprehensive tests |
| apps/content/docs/integrations/pino.md | Documentation for the new Pino integration |
| apps/content/.vitepress/config.ts | Adds navigation entries for Pino documentation |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/pino/src/index.test.ts (1)
1-3: Consider testing all public exports.The test verifies that
LoggingHandlerPluginis exported, but other public exports likeCONTEXT_LOGGER_SYMBOL,getLogger, andLoggerContextfrom the context module are not verified. Consider expanding this test to ensure all intended public API members are properly exported.Apply this diff to test additional exports:
it('exports LoggingHandlerPlugin', async () => { - expect(Object.keys(await import('./index'))).toContain('LoggingHandlerPlugin') + const exports = Object.keys(await import('./index')) + expect(exports).toContain('LoggingHandlerPlugin') + expect(exports).toContain('getLogger') + expect(exports).toContain('CONTEXT_LOGGER_SYMBOL') })packages/pino/README.md (2)
48-48: Consider using more descriptive link text.The link text "here" is generic. More descriptive text like "in the oRPC documentation" would improve accessibility and SEO.
Apply this diff:
-You can find the full documentation [here](https://orpc.unnoq.com). +You can find the full documentation in [the oRPC documentation](https://orpc.unnoq.com).
74-76: Add alt text to the sponsor image.The sponsor image is missing alt text, which is important for accessibility.
Apply this diff:
- <img src='https://cdn.jsdelivr.net/gh/unnoq/unnoq/sponsors.svg'/> + <img src='https://cdn.jsdelivr.net/gh/unnoq/unnoq/sponsors.svg' alt='Sponsors'/>packages/pino/src/handler-plugin.ts (1)
36-46: Reconsider the memory usage remark.The remark states "If a signal is used for multiple requests, this may lead to un-efficient memory usage (listeners never removed)." However, line 111 uses
{ once: true }, which automatically removes the listener after it fires. The listener is also removed if the request completes without aborting (when the request ends, the signal's listeners are eligible for GC). This remark may be outdated or overly cautious.Consider clarifying or removing this remark:
- * @remarks If a signal is used for multiple requests, this may lead to un-efficient memory usage (listeners never removed). + * @remarks The listener is automatically removed after firing due to the { once: true } option.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
apps/content/.vitepress/config.ts(2 hunks)apps/content/docs/integrations/pino.md(1 hunks)packages/pino/.gitignore(1 hunks)packages/pino/README.md(1 hunks)packages/pino/package.json(1 hunks)packages/pino/src/context.test.ts(1 hunks)packages/pino/src/context.ts(1 hunks)packages/pino/src/handler-plugin.test.ts(1 hunks)packages/pino/src/handler-plugin.ts(1 hunks)packages/pino/src/index.test.ts(1 hunks)packages/pino/src/index.ts(1 hunks)packages/pino/tsconfig.json(1 hunks)packages/shared/src/object.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/pino/src/context.ts (1)
packages/shared/src/object.ts (1)
get(79-91)
packages/pino/src/context.test.ts (1)
packages/pino/src/context.ts (2)
getLogger(10-12)CONTEXT_LOGGER_SYMBOL(4-4)
packages/pino/src/handler-plugin.ts (7)
packages/server/src/context.ts (1)
Context(1-1)packages/server/src/adapters/standard/handler.ts (2)
StandardHandlerInterceptorOptions(23-25)StandardHandlerOptions(27-48)packages/server/src/adapters/standard/plugin.ts (1)
StandardHandlerPlugin(5-8)packages/pino/src/context.ts (3)
LoggerContext(6-8)CONTEXT_LOGGER_SYMBOL(4-4)getLogger(10-12)packages/shared/src/iterator.ts (2)
next(60-62)isAsyncIteratorObject(6-12)packages/shared/src/proxy.ts (1)
overlayProxy(62-83)packages/client/src/event-iterator.ts (1)
mapEventIterator(4-52)
packages/shared/src/object.ts (1)
packages/openapi/src/openapi-custom.ts (1)
get(17-23)
packages/pino/src/handler-plugin.test.ts (7)
packages/standard-server/src/types.ts (1)
StandardLazyRequest(26-32)packages/server/src/adapters/standard/rpc-codec.ts (1)
StandardRPCCodec(8-37)packages/server/src/builder.ts (2)
handler(273-280)os(336-352)packages/server/src/adapters/standard/handler.ts (1)
StandardHandler(50-183)packages/server/src/adapters/standard/rpc-matcher.ts (1)
StandardRPCMatcher(24-118)packages/pino/src/handler-plugin.ts (1)
LoggingHandlerPlugin(49-172)packages/pino/src/context.ts (2)
getLogger(10-12)CONTEXT_LOGGER_SYMBOL(4-4)
🪛 Biome (2.1.2)
packages/pino/src/handler-plugin.test.ts
[error] 260-262: This generator function doesn't contain yield.
(lint/correctness/useYield)
🪛 LanguageTool
packages/pino/README.md
[style] ~54-~54: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... your API or implement API contract. - [@orpc/client](https://www.npmjs.com/package/@...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... API on the client with type-safety. - [@orpc/openapi](https://www.npmjs.com/package/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~56-~56: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...I specs and handle OpenAPI requests. - [@orpc/otel](https://www.npmjs.com/package/@or...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~57-~57: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....io/) integration for observability. - [@orpc/nest](https://www.npmjs.com/package/@or...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~58-~58: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... with NestJS. - [@orpc/react](https://www.npmjs.com/package/@o...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~59-~59: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...with React and React Server Actions. - [@orpc/tanstack-query](https://www.npmjs.com/p...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~60-~60: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...stack.com/query/latest) integration. - [@orpc/experimental-react-swr](https://www.npm...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~61-~61: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ttps://swr.vercel.app/) integration. - [@orpc/vue-colada](https://www.npmjs.com/packa...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~62-~62: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ada](https://pinia-colada.esm.dev/). - [@orpc/hey-api](https://www.npmjs.com/package/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~63-~63: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...I](https://heyapi.dev/) integration. - [@orpc/zod](https://www.npmjs.com/package/@orp...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~64-~64: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tps://zod.dev/) doesn't support yet. - [@orpc/valibot](https://www.npmjs.com/package/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~65-~65: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rom Valibot. - [@orpc/arktype](https://www.npmjs.com/package/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
packages/pino/README.md
48-48: Link text should be descriptive
(MD059, descriptive-link-text)
75-75: Images should have alternate text (alt text)
(MD045, no-alt-text)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Agent
- GitHub Check: test
- GitHub Check: publish-commit
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (20)
packages/shared/src/object.ts (1)
79-79: Breaking API change enables symbol key support.Expanding the
pathparameter fromreadonly string[]toreadonly PropertyKey[]is a useful enhancement that enables thegetfunction to work with symbol keys (as required by the new pino package'sCONTEXT_LOGGER_SYMBOL). The implementation is compatible with this broader type.This is a breaking change to the public API that should be noted in release documentation.
apps/content/.vitepress/config.ts (1)
156-156: LGTM! Dual navigation entry provides multiple discovery paths.The Pino integration is appropriately listed in both the Plugins section (as "Logging") and the Integrations section (as "Pino"). This dual listing helps users discover the feature from either a capability perspective or a library perspective.
Also applies to: 194-194
packages/pino/src/index.ts (1)
1-2: LGTM! Clean barrel export pattern.The index module properly re-exports all members from the context and handler-plugin modules, providing a clean public API surface.
packages/pino/src/context.test.ts (1)
4-10: LGTM! Comprehensive test coverage for getLogger.The test properly covers both the negative cases (when no logger is present in the context) and the positive case (when a logger is attached via
CONTEXT_LOGGER_SYMBOL). The test implementation is correct and validates the expected behavior.packages/pino/tsconfig.json (1)
1-16: LGTM! Standard TypeScript library configuration.The TypeScript configuration properly extends the shared library config, references the necessary dependencies (server and shared packages), and correctly includes/excludes files for a library build.
packages/pino/src/context.ts (2)
4-4: LGTM! Well-namespaced symbol identifier.The
CONTEXT_LOGGER_SYMBOLuses a descriptive, namespaced name (ORPC_PINO_CONTEXT_LOGGER_SYMBOL) that prevents collisions and clearly indicates its purpose.
10-12: LGTM! Clean implementation leveraging updated shared utility.The
getLoggerfunction correctly uses the updatedget()utility from@orpc/shared(which now acceptsPropertyKey[]including symbols) to retrieve the logger from the context. The type assertion is safe since the symbol key is controlled by this module.apps/content/docs/integrations/pino.md (3)
44-60: LGTM! Clear setup instructions with comprehensive options.The setup example clearly demonstrates how to configure the
LoggingHandlerPluginwith all available options including custom logger, ID generator, request lifecycle logging, and abort logging. The documentation appropriately notes that lifecycle and abort logging are disabled by default.
74-85: LGTM! Clear usage example with proper null safety.The example correctly demonstrates accessing the logger via
getLogger(context)and uses optional chaining (?.) for null safety, which is important since the logger might not be present in all contexts.
92-124: LGTM! Excellent advanced integration example.The example demonstrates a practical pattern for integrating with
pino-http, showing how to:
- Extend the context interface with
LoggerContext- Pass the per-request logger through the context using
CONTEXT_LOGGER_SYMBOL- Properly type the context in the router
This is a valuable real-world use case that users will benefit from.
packages/pino/package.json (1)
1-46: LGTM! Well-structured package configuration.The package.json is properly configured with appropriate metadata, exports, dependencies, and build scripts. The experimental prefix and version 0.0.0 appropriately signal the package's maturity level.
packages/pino/src/handler-plugin.test.ts (4)
9-46: LGTM! Well-designed test utilities.The FakeLogger mock implementation effectively tracks method calls using shared spies while maintaining internal state. This provides a clean way to verify logger behavior without depending on actual pino instances.
65-90: LGTM! Comprehensive lifecycle logging verification.The test properly validates both matched and unmatched request scenarios, ensuring the plugin logs appropriate messages for each case.
220-284: LGTM! Thorough streaming error handling tests.The test suite properly validates that streaming errors are logged as errors while aborted stream errors are logged as info, maintaining the distinction between business logic errors and abort signals.
Note: The static analysis warning about the generator on line 260 not containing yield is a false positive—this test intentionally validates a generator that throws before yielding.
286-342: LGTM! Valuable edge case coverage.These tests ensure proper handling of logger bindings, ID generation, and default logger initialization—critical behaviors that could easily be overlooked.
packages/pino/src/handler-plugin.ts (5)
10-47: LGTM! Well-documented options interface.The interface provides clear documentation for each option with examples and sensible defaults. The type constraints ensure type safety across the plugin.
49-62: LGTM! Clean constructor with sensible defaults.The constructor properly initializes all fields with appropriate defaults: a pino instance for the logger and
crypto.randomUUID()for ID generation.
64-103: LGTM! Robust root interceptor setup.The root interceptor properly:
- Creates a child logger with per-request bindings
- Generates unique request IDs
- Preserves existing request bindings from pino-http
- Logs internal errors to the base logger while propagating them
This provides a solid foundation for per-request logging context.
105-139: LGTM! Comprehensive request lifecycle interceptor.The interceptor correctly handles:
- Optional abort event logging
- Request lifecycle logging when enabled
- Distinguishing abort signals from business logic errors
The error handling logic appropriately logs aborted requests at info level while treating other errors as genuine errors.
141-171: LGTM! Well-designed client interceptor for streaming support.The client interceptor properly:
- Adds the procedure path to logger bindings
- Handles both regular and streaming outputs
- Applies the same abort-vs-error distinction for streaming errors
The use of
overlayProxywithmapEventIteratorensures streaming errors are logged without breaking the iteration protocol.
More templates
@orpc/ai-sdk
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/experimental-pino
@orpc/experimental-publisher
@orpc/experimental-ratelimit
@orpc/react
@orpc/react-query
@orpc/experimental-react-swr
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fastify
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/pino/src/handler-plugin.ts (2)
42-42: Clarify the memory usage remark.The comment states "listeners never removed," but the implementation at line 110 uses
{ once: true }, which auto-removes the listener after it fires. The actual concern is that if a signal is reused across multiple requests and never aborts between them, listeners accumulate until the signal finally aborts.Apply this diff to clarify:
- * @remarks If a signal is used for multiple requests, this may lead to un-efficient memory usage (listeners never removed). + * @remarks If a signal is reused across multiple requests without aborting, listeners accumulate (removed only when aborted or signal is garbage-collected).
94-101: Consider using per-request logger for consistency.The catch block logs framework errors using
this.logger(root logger) instead of the per-requestloggercreated at line 71. Using the per-request logger would include the request ID and metadata in error logs, aiding troubleshooting.If root logger usage is intentional (e.g., to separate framework errors), add a comment explaining the rationale:
catch (error) { /** - * Any error here is internal (interceptor/framework), not business logic. + * Any error here is internal (interceptor/framework), not business logic. + * Logged to root logger to distinguish from per-request business logic errors. * Indicates unexpected handler failure. */ this.logger.error(error)Alternatively, use the per-request logger:
- this.logger.error(error) + logger.error(error)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/content/docs/integrations/pino.md(1 hunks)packages/pino/src/handler-plugin.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/content/docs/integrations/pino.md
🧰 Additional context used
🧬 Code graph analysis (1)
packages/pino/src/handler-plugin.ts (7)
packages/server/src/context.ts (1)
Context(1-1)packages/server/src/adapters/standard/handler.ts (2)
StandardHandlerInterceptorOptions(23-25)StandardHandlerOptions(27-48)packages/server/src/adapters/standard/plugin.ts (1)
StandardHandlerPlugin(5-8)packages/pino/src/context.ts (3)
LoggerContext(6-8)CONTEXT_LOGGER_SYMBOL(4-4)getLogger(10-12)packages/shared/src/iterator.ts (2)
next(60-62)isAsyncIteratorObject(6-12)packages/shared/src/proxy.ts (1)
overlayProxy(62-83)packages/client/src/event-iterator.ts (1)
mapEventIterator(4-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: test
- GitHub Check: publish-commit
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/pino/src/handler-plugin.ts (4)
1-8: LGTM!Clean and appropriate imports for the logging plugin implementation.
55-62: LGTM!Constructor properly initializes with sensible defaults. The
crypto.randomUUID()usage is safe given the Node.js version requirements.
104-138: LGTM!The interceptor correctly distinguishes aborted signals from genuine errors (lines 130-132) and appropriately logs lifecycle events when enabled. The abort listener with
{ once: true }ensures cleanup after firing.
140-169: LGTM!The client interceptor correctly adds path metadata to logger bindings and wraps streaming responses for error logging. The abort signal detection pattern (lines 156-158) is consistent with the regular interceptor, and returning the error unchanged (line 162) is correct for logging without altering the error flow.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/pino/src/handler-plugin.ts (1)
94-105: Consider cleanup for abort listeners to prevent accumulation.While the
{ once: true }option removes the listener after it fires, if the signal never aborts (request completes normally), the listener remains attached. With long-lived or reused signals, this can accumulate listeners over time, as noted in line 43.Since
logRequestAbortdefaults tofalseand the comment warns users, this is acceptable. However, for robustness, consider explicitly removing the listener after the request completes:if (this.logRequestAbort) { const signal = interceptorOptions.request.signal if (signal?.aborted) { logger?.info(`request was aborted before handling (${String(signal.reason)})`) } else { - signal?.addEventListener('abort', () => { - logger?.info(`request is aborted (${String(signal.reason)})`) - }, { once: true }) + const abortListener = () => { + logger?.info(`request is aborted (${String(signal.reason)})`) + } + signal?.addEventListener('abort', abortListener, { once: true }) + + // Ensure cleanup if request completes without abort + try { + const result = await interceptorOptions.next(...) + return result + } finally { + signal?.removeEventListener('abort', abortListener) + } } }Note: This would require restructuring the try-catch block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/pino/src/handler-plugin.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/pino/src/handler-plugin.ts (7)
packages/server/src/context.ts (1)
Context(1-1)packages/server/src/adapters/standard/handler.ts (2)
StandardHandlerInterceptorOptions(23-25)StandardHandlerOptions(27-48)packages/server/src/adapters/standard/plugin.ts (1)
StandardHandlerPlugin(5-8)packages/pino/src/context.ts (3)
LoggerContext(6-8)CONTEXT_LOGGER_SYMBOL(4-4)getLogger(10-12)packages/shared/src/iterator.ts (2)
next(60-62)isAsyncIteratorObject(6-12)packages/shared/src/proxy.ts (1)
overlayProxy(62-83)packages/client/src/event-iterator.ts (1)
mapEventIterator(4-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: publish-commit
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (4)
packages/pino/src/handler-plugin.ts (4)
10-48: LGTM!The interface is well-documented with clear defaults and helpful examples. The memory leak remark on line 43 appropriately warns users about the tradeoff when enabling
logRequestAbortwith reused signals.
56-63: LGTM!The constructor provides sensible defaults, including
crypto.randomUUID()for ID generation and a fresh pino instance for logging.
146-164: LGTM!The interceptor correctly distinguishes between aborted signals (logged as info) and genuine errors (logged as error), while preserving error propagation. The pattern
request.signal?.aborted && request.signal.reason === errorensures we only treat the exact abort reason as a non-error case.
172-195: LGTM!The async iterator wrapping correctly applies error logging to streaming responses while preserving abort signal semantics. The identity mapper at line 176 is necessary for the
mapEventIteratorAPI, and the error propagation at line 188 ensures downstream handlers receive the original error.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/pino/src/handler-plugin.test.ts (1)
261-265: Rename unused loop variable to avoid lint failureTypeScript/tsconfig forbids unused locals;
visn’t read and will trip the build. Swap it for_so the loop keeps consuming the stream without failing lint.- for await (const v of result1.response?.body as AsyncIterable<any>) { + for await (const _ of result1.response?.body as AsyncIterable<any>) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/pino/src/handler-plugin.test.ts(1 hunks)packages/pino/src/handler-plugin.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/pino/src/handler-plugin.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/pino/src/handler-plugin.test.ts (5)
packages/standard-server/src/types.ts (1)
StandardLazyRequest(26-32)packages/server/src/adapters/standard/rpc-codec.ts (1)
StandardRPCCodec(8-37)packages/server/src/adapters/standard/handler.ts (1)
StandardHandler(50-183)packages/pino/src/handler-plugin.ts (1)
LoggingHandlerPlugin(50-197)packages/pino/src/context.ts (2)
getLogger(10-12)CONTEXT_LOGGER_SYMBOL(4-4)
🪛 Biome (2.1.2)
packages/pino/src/handler-plugin.test.ts
[error] 279-281: This generator function doesn't contain yield.
(lint/correctness/useYield)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: publish-commit
- GitHub Check: lint
1 participant
Fixes #908
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
Other