Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughJSDoc comments across TypeScript type definition files were reflowed and reformatted to improve readability and text wrapping consistency. Additionally, the Vite configuration was updated to enforce a JSDoc formatting preference for code blocks. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #757 +/- ##
=======================================
Coverage 94.65% 94.65%
=======================================
Files 10 10
Lines 730 730
Branches 228 228
=======================================
Hits 691 691
Misses 36 36
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Enables oxfmt’s JSDoc formatting feature in the repo’s vp formatter configuration and applies the resulting JSDoc reflow/normalization across several TypeScript source files.
Changes:
- Add
fmt.jsdoc.preferCodeFencesto the formatter configuration. - Reflow/normalize multiple JSDoc blocks in
src/(line wrapping, tag formatting, block style). - Minor doc-comment consolidation for timing/docs references.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.ts | Enables oxfmt JSDoc formatting behavior via fmt.jsdoc config. |
| src/index.ts | Reflows JSDoc for rejectUnauthorized option. |
| src/Response.ts | Normalizes JSDoc blocks and tags (timing reference, deprecation docs). |
| src/Request.ts | Reflows many option JSDocs; introduces a few readability/grammar/link-wrapping issues to fix. |
| src/HttpClient.ts | Reflows option JSDocs; introduces minor wording/casing issues to fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Auto retry times on 5xx response, default is `0`. Don't work on streaming request It's not supported by using retry | ||
| * and writeStream, because the retry request can't stop the stream which is consuming. |
There was a problem hiding this comment.
This JSDoc became a run-on sentence after reflow: "Don't work on streaming request It's not supported...". Add punctuation (e.g., a period after "request") so the two sentences are properly separated.
| * Auto retry times on socket error, default is `1`. Don't work on streaming request It's not supported by using retry | ||
| * and writeStream, because the retry request can't stop the stream which is consuming. |
There was a problem hiding this comment.
This JSDoc also reads as a run-on sentence after reflow: "Don't work on streaming request It's not supported...". Add punctuation between these sentences for readability.
| * Check request address to protect from SSRF and similar attacks. It receive two arguments(ip and family) and should | ||
| * return true or false to identified the address is legal or not. It rely on lookup and have the same version | ||
| * requirement. |
There was a problem hiding this comment.
In this public API doc comment, "It receive" / "to identified" are grammatically incorrect. Since this block was reformatted, please update the wording (e.g., "It receives..." / "to identify...") to keep generated docs professional and clear.
| * Check request address to protect from SSRF and similar attacks. It receive two arguments(ip and family) and should | |
| * return true or false to identified the address is legal or not. It rely on lookup and have the same version | |
| * requirement. | |
| * Check request addresses to protect from SSRF and similar attacks. It receives two arguments (ip and family) and | |
| * should return true or false to identify whether the address is legal. It relies on lookup and has the same | |
| * version requirement. |
| rejectUnauthorized?: boolean; | ||
| /** | ||
| * socketPath string | null (optional) - Default: null - An IPC endpoint, either Unix domain socket or Windows named pipe | ||
| * SocketPath string | null (optional) - Default: null - An IPC endpoint, either Unix domain socket or Windows named |
There was a problem hiding this comment.
The JSDoc refers to "SocketPath" (capital S), but the actual option is socketPath. Using the exact property name in the comment will avoid confusion when users search docs for this option.
| * SocketPath string | null (optional) - Default: null - An IPC endpoint, either Unix domain socket or Windows named | |
| * socketPath string | null (optional) - Default: null - An IPC endpoint, either Unix domain socket or Windows named |
| * A writable stream to be piped by the response stream. Responding data will be write to this stream and callback | ||
| * will be called with data set null after finished writing. |
There was a problem hiding this comment.
The JSDoc sentence reads ungrammatically: "Responding data will be write". Since this block was reformatted, consider fixing it to "will be written" (and adjust wording as needed) to keep the public API docs clear.
| * A writable stream to be piped by the response stream. Responding data will be write to this stream and callback | |
| * will be called with data set null after finished writing. | |
| * A writable stream to which the response stream will be piped. Response data will be written to this stream, and the | |
| * callback will be called with `data` set to `null` after writing finishes. |
| * Type of response data. Could be text or json. If it's text or html, the callbacked data would be a String. If it's | ||
| * json, the data of callback would be a parsed JSON Object and will auto set Accept: 'application/json' header. | ||
| * Default is 'buffer'. |
There was a problem hiding this comment.
In this JSDoc, "callbacked" is not correct English (should be something like "callback" / "returned" / "passed to the callback"). Since this comment was reformatted, please correct the wording for clarity.
| * Type of response data. Could be text or json. If it's text or html, the callbacked data would be a String. If it's | |
| * json, the data of callback would be a parsed JSON Object and will auto set Accept: 'application/json' header. | |
| * Default is 'buffer'. | |
| * Type of response data. Could be text or json. If it's text or html, the data passed to the callback will be a | |
| * string. If it's json, the data passed to the callback will be a parsed JSON object and will auto set Accept: | |
| * 'application/json' header. Default is 'buffer'. |
| * See [MDN: HTTP - Headers - Keep-Alive | ||
| * directives](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Keep-Alive#directives) for more details. |
There was a problem hiding this comment.
The markdown link text is split across lines inside the [...] label. Some JSDoc/Markdown renderers don’t handle newlines in link labels well, which can break the rendered link text. Consider rewrapping so the line break happens outside the link label (or shorten the label) while keeping the URL intact.
| * See [MDN: HTTP - Headers - Keep-Alive | |
| * directives](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Keep-Alive#directives) for more details. | |
| * See [MDN: HTTP - Headers - Keep-Alive directives](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Keep-Alive#directives) | |
| * for more details. |
![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 reformats JSDoc comments across several files and updates the Vite configuration to prefer code fences in JSDoc. The review feedback identifies several grammatical errors, run-on sentences, and incorrect property name capitalizations that occurred during the reformatting process, providing specific suggestions to improve the clarity and technical accuracy of the documentation.
| * Check request address to protect from SSRF and similar attacks. It receive two arguments(ip and family) and should | ||
| * return true or false to identified the address is legal or not. It rely on lookup and have the same version | ||
| * requirement. |
There was a problem hiding this comment.
The re-wrapped JSDoc contains several grammatical errors that affect clarity: "It receive" should be "It receives", "identified" should be "identify", and "It rely" should be "It relies". Additionally, adding a space between "arguments" and the parenthesis improves readability.
| * Check request address to protect from SSRF and similar attacks. It receive two arguments(ip and family) and should | |
| * return true or false to identified the address is legal or not. It rely on lookup and have the same version | |
| * requirement. | |
| * Check request address to protect from SSRF and similar attacks. It receives two arguments (ip and family) and | |
| * should return true or false to identify whether the address is legal or not. It relies on lookup and has the same | |
| * version requirement. |
| * SocketPath string | null (optional) - Default: null - An IPC endpoint, either Unix domain socket or Windows named | ||
| * pipe |
There was a problem hiding this comment.
The formatter capitalized socketPath to SocketPath. Since this refers to a specific property name, it should remain lowercase. Wrapping it in backticks will prevent the formatter from capitalizing it and clearly identify it as a technical term.
| * SocketPath string | null (optional) - Default: null - An IPC endpoint, either Unix domain socket or Windows named | |
| * pipe | |
| * `socketPath` string | null (optional) - Default: null - An IPC endpoint, either Unix domain socket or Windows | |
| * named pipe |
| * Urllib default use URLSearchParams to stringify form data which don't support nested object, will use qs instead of | ||
| * URLSearchParams to support nested object by set this option to true. |
There was a problem hiding this comment.
This sentence is a run-on and contains grammatical errors ("default use", "don't support nested object"). Improving the structure makes the documentation much clearer.
| * Urllib default use URLSearchParams to stringify form data which don't support nested object, will use qs instead of | |
| * URLSearchParams to support nested object by set this option to true. | |
| * Urllib by default uses URLSearchParams to stringify form data, which doesn't support nested objects. Set this | |
| * option to true to use `qs` instead of URLSearchParams to support nested objects. |
| /** | ||
| * Request timeout in milliseconds for connecting phase and response receiving phase. | ||
| * Defaults is `5000`, both are 5 seconds. You can use timeout: 5000 to tell urllib use same timeout on two phase or set them separately such as | ||
| * Request timeout in milliseconds for connecting phase and response receiving phase. Defaults is `5000`, both are 5 |
There was a problem hiding this comment.
Minor grammar fix: "Defaults is" should be "Default is".
| * Request timeout in milliseconds for connecting phase and response receiving phase. Defaults is `5000`, both are 5 | |
| * Request timeout in milliseconds for connecting phase and response receiving phase. Default is `5000`, both are 5 |
| /** Username:password used in HTTP Digest Authorization. */ | ||
| digestAuth?: string; | ||
| /** follow HTTP 3xx responses as redirects. defaults to true. */ | ||
| /** Follow HTTP 3xx responses as redirects. defaults to true. */ |
There was a problem hiding this comment.
Capitalize "Defaults" for consistency with the start of the sentence.
| /** Follow HTTP 3xx responses as redirects. defaults to true. */ | |
| /** Follow HTTP 3xx responses as redirects. Defaults to true. */ |
| * Auto retry times on 5xx response, default is `0`. Don't work on streaming request It's not supported by using retry | ||
| * and writeStream, because the retry request can't stop the stream which is consuming. |
There was a problem hiding this comment.
The formatter merged two sentences without punctuation, creating a run-on sentence. Adding a period and fixing the grammar ("Don't" to "Doesn't", "request" to "requests") improves readability.
| * Auto retry times on 5xx response, default is `0`. Don't work on streaming request It's not supported by using retry | |
| * and writeStream, because the retry request can't stop the stream which is consuming. | |
| * Auto retry times on 5xx response, default is `0`. Doesn't work on streaming requests. It's not supported when | |
| * using retry and writeStream, because the retry request can't stop the stream which is consuming. |
| * Auto retry times on socket error, default is `1`. Don't work on streaming request It's not supported by using retry | ||
| * and writeStream, because the retry request can't stop the stream which is consuming. |
There was a problem hiding this comment.
The formatter merged two sentences without punctuation, creating a run-on sentence. Adding a period and fixing the grammar ("Don't" to "Doesn't", "request" to "requests") improves readability.
| * Auto retry times on socket error, default is `1`. Don't work on streaming request It's not supported by using retry | |
| * and writeStream, because the retry request can't stop the stream which is consuming. | |
| * Auto retry times on socket error, default is `1`. Doesn't work on streaming requests. It's not supported when | |
| * using retry and writeStream, because the retry request can't stop the stream which is consuming. |
https://oxc.rs/docs/guide/usage/formatter/config-file-reference.html#jsdoc
Summary by CodeRabbit
Documentation
Chores