feat: add isSentByFetch/fetchOpaque to urllib message #581
Conversation
|
""" WalkthroughThe changes introduce new optional properties, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FetchFactory
participant DiagnosticsChannel
Client->>FetchFactory: send request
FetchFactory->>DiagnosticsChannel: publish request (isSentByFetch, fetchOpaque)
FetchFactory->>DiagnosticsChannel: publish fetchRequest (fetchOpaque)
FetchFactory->>DiagnosticsChannel: publish fetchResponse (fetchOpaque)
FetchFactory->>DiagnosticsChannel: publish response (isSentByFetch, fetchOpaque)
FetchFactory->>Client: return response
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/FetchOpaqueInterceptor.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @killagu, 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 new diagnostic capabilities to the urllib library, specifically for requests initiated via the fetch API. By adding isSentByFetch and fetchOpaque properties to various diagnostic message types, the system can now provide richer context, enabling better tracing, debugging, and understanding of the origin and specific details of fetch-driven network operations.
Highlights
- Enhanced Diagnostic Messages: I've extended the
RequestDiagnosticsMessageandResponseDiagnosticsMessagetypes insrc/HttpClient.tsto include two new optional properties:isSentByFetch(a boolean) andfetchOpaque(of typeFetchOpaque). These additions provide more context for HTTP requests and responses within theurllibsystem. - Fetch-Specific Context: I've also updated the
FetchDiagnosticsMessageandFetchResponseDiagnosticsMessagetypes insrc/fetch.tsto include afetchOpaqueproperty. This ensures that diagnostic messages specifically related tofetchoperations carry their associated opaque data. - Automatic Population of New Fields: The
FetchFactoryinsrc/fetch.tshas been modified to automatically populate these new diagnostic fields. When a request or response originates from afetchcall, theisSentByFetchflag is set totrue, and theinternalOpaqueobject is attached to the relevant diagnostic messages (channels.request,channels.response,channels.fetchRequest,channels.fetchResponse), covering both successful and error scenarios.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
commit: |
|
undici 的 dispatcher 对 options 进行修改时，无法影响到原始的 fetch init。因此在事件中引入 opaque 来对整个请求做串联。 |
There was a problem hiding this comment.
Code Review
This pull request adds isSentByFetch and fetchOpaque fields to the urllib diagnostic messages to provide more context, especially for requests originating from fetch. The changes are consistent and correctly propagate this new information.
However, I've identified a critical type mismatch related to the new fetchOpaque field. The internalOpaque object assigned to it does not conform to the FetchOpaque interface, which could lead to runtime errors. This should be addressed before merging.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #581 +/- ##
==========================================
+ Coverage 95.64% 95.67% +0.02%
==========================================
Files 11 11
Lines 1355 1364 +9
Branches 313 313
==========================================
+ Hits 1296 1305 +9
Misses 55 55
Partials 4 4 ☔ 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: 0
🧹 Nitpick comments (1)
src/FetchOpaqueInterceptor.ts (1)
8-16: Consider removing @ts-ignore comments and improving type safety.The @ts-ignore comments suggest TypeScript is having issues with the symbol keys. Consider using a more type-safe approach for symbol-indexed properties or adding proper type declarations.
You could improve type safety by:
- Creating a proper type for the symbols
- Using mapped types or index signatures
- Adding type assertions instead of suppressing errors
- // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - [symbols.kRequestId]: number; - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - [symbols.kRequestStartTime]: number; - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - [symbols.kEnableRequestTiming]: boolean; + [symbols.kRequestId as keyof FetchOpaque]: number; + [symbols.kRequestStartTime as keyof FetchOpaque]: number; + [symbols.kEnableRequestTiming as keyof FetchOpaque]: boolean;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/FetchOpaqueInterceptor.ts(1 hunks)
⏰ 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). (15)
- GitHub Check: Node.js / Test (macos-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 16)
- GitHub Check: Node.js / Test (ubuntu-latest, 16)
- GitHub Check: Node.js / Test (macos-latest, 16)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 24)
- GitHub Check: Node.js / Test (macos-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (macos-latest, 24)
- GitHub Check: Node.js / Test (macos-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 24)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
🔇 Additional comments (1)
src/FetchOpaqueInterceptor.ts (1)
16-16: kEnableRequestTiming boolean change verified and approvedAll usages of
[symbols.kEnableRequestTiming]inHttpClient.ts,fetch.tsanddiagnosticsChannel.tsnow assign or check a boolean value. There are no numeric operations or comparisons on that property, so the switch fromnumbertobooleanis safe to merge.As a minor cleanup, once your TypeScript setup fully supports computed symbol keys, you can remove the
@ts-ignoredirectives around those properties insrc/FetchOpaqueInterceptor.tsto restore full type‐checking.
[skip ci] ## [4.8.0](v4.7.1...v4.8.0) (2025-07-08) ### Features * add isSentByFetch/fetchOpaque to urllib message ([#581](#581)) ([d7e9f40](d7e9f40))
Summary by CodeRabbit
New Features
Chores