Add Note for WebSocket subprotocols #1164
Conversation
shugen002
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe recent updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
websocket/README.md (1)
153-153: Clarify sentence structure for better understanding.The sentence structure is awkward and can be improved for clarity. Consider rephrasing it.
Here's a suggested rephrasing:
- If there is a connection with `Sec-Websocket-Protocol` header in the request with failed negotitation, the connection will be disconnected by browser after received upgrade response immediately. + If a connection request includes a `Sec-Websocket-Protocol` header but the negotiation fails, the browser will immediately disconnect the connection after receiving the upgrade response.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- websocket/README.md (1 hunks)
Additional context used
LanguageTool
websocket/README.md
[grammar] ~149-~149: The pronoun ‘you’ must be used with a non-third-person form of a verb.
Context: ...The configSubprotocolsonly help you negotiates subprotocol and set a `Sec-Websocket-Pr...(NON3PRS_VERB)
[grammar] ~149-~149: It seems that a pronoun is missing.
Context: ...d set aSec-Websocket-Protocolheader if has a suitable subprotocol. For more ab...(IF_VB)
[grammar] ~151-~151: There may an error in the verb form ‘be send’.
Context: ...socket#Upgrader) . All connection will be send to handler function no matter the subpr...(MD_BE_NON_VBP)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
websocket/README.md (3)
149-149: Correct grammatical errors for clarity.The sentence contains grammatical errors that affect readability. Consider the following corrections:
- Change "negotiates process" to "negotiation process".
- Add a comma after "fasthttp.Upgrader" for better readability.
Here's the corrected sentence:
- The config `Subprotocols` only helps you negotiate subprotocols and sets a `Sec-Websocket-Protocol` header if it has a suitable subprotocol. For more about negotiates process, check the comment for `Subprotocols` in [fasthttp.Upgrader](https://pkg.go.dev/github.com/fasthttp/websocket#Upgrader) . + The config `Subprotocols` only helps you negotiate subprotocols and sets a `Sec-Websocket-Protocol` header if it has a suitable subprotocol. For more about the negotiation process, check the comment for `Subprotocols` in [fasthttp.Upgrader](https://pkg.go.dev/github.com/fasthttp/websocket#Upgrader).
151-151: Consider adding a comma for better readability.Adding a comma after "no matter" can improve the readability of the sentence.
- All connections will be sent to the handler function no matter whether the subprotocol negotiation is successful or not. + All connections will be sent to the handler function, no matter whether the subprotocol negotiation is successful or not.Tools
LanguageTool
[typographical] ~151-~151: Consider adding a comma here.
Context: ...ons will be sent to the handler function no matter whether the subprotocol negotiat...(NO_MATTER_COMMA)
153-153: Fix typographical errors for clarity.The sentence contains a typographical error. Consider the following corrections:
- Change "negotitation" to "negotiation".
- Change "after received upgrade response immediately" to "immediately after receiving the upgrade response".
Here's the corrected sentence:
- If there is a connection with `Sec-Websocket-Protocol` header in the request with failed negotitation, the connection will be disconnected by browser after received upgrade response immediately. + If there is a connection with a `Sec-Websocket-Protocol` header in the request with a failed negotiation, the connection will be disconnected by the browser immediately after receiving the upgrade response.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- websocket/README.md (1 hunks)
Additional context used
LanguageTool
websocket/README.md
[typographical] ~151-~151: Consider adding a comma here.
Context: ...ons will be sent to the handler function no matter whether the subprotocol negotiat...(NO_MATTER_COMMA)
|
@coderabbitai full review |
Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
websocket/README.md (3)
149-149: Correct grammatical errors for clarity.The sentence contains grammatical errors that affect readability. Consider the following corrections:
- Change "negotiates process" to "negotiation process".
- Add a comma after "subprotocols" to improve readability.
Here's the corrected sentence:
- The config `Subprotocols` only helps you negotiate subprotocols and sets a `Sec-Websocket-Protocol` header if it has a suitable subprotocol. For more about negotiates process, check the comment for `Subprotocols` in [fasthttp.Upgrader](https://pkg.go.dev/github.com/fasthttp/websocket#Upgrader) . + The config `Subprotocols` only helps you negotiate subprotocols and sets a `Sec-Websocket-Protocol` header if it has a suitable subprotocol. For more about the negotiation process, check the comment for `Subprotocols` in [fasthttp.Upgrader](https://pkg.go.dev/github.com/fasthttp/websocket#Upgrader).
151-151: Add a comma for improved readability.Consider adding a comma after "no matter" to clarify the sentence structure.
Here's the corrected sentence:
- All connections will be sent to the handler function no matter whether the subprotocol negotiation is successful or not. You can get the selected subprotocol from `conn.Subprotocol()`. + All connections will be sent to the handler function, no matter whether the subprotocol negotiation is successful or not. You can get the selected subprotocol from `conn.Subprotocol()`.Tools
LanguageTool
[typographical] ~151-~151: Consider adding a comma here.
Context: ...ons will be sent to the handler function no matter whether the subprotocol negotiat...(NO_MATTER_COMMA)
153-153: Correct typographical error and clarify the sentence.The sentence contains a typographical error and could be clearer. Consider the following corrections:
- Change "negotitation" to "negotiation".
- Clarify the sequence of events regarding the browser's behavior.
Here's the corrected sentence:
- If there is a connection with `Sec-Websocket-Protocol` header in the request with failed negotitation, the connection will be disconnected by browser after received upgrade response immediately. + If a connection request includes a `Sec-Websocket-Protocol` header but fails negotiation, the browser will immediately disconnect the connection after receiving the upgrade response.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- websocket/README.md (1 hunks)
Additional context used
LanguageTool
websocket/README.md
[typographical] ~151-~151: Consider adding a comma here.
Context: ...ons will be sent to the handler function no matter whether the subprotocol negotiat...(NO_MATTER_COMMA)
Co-authored-by: Jason McNeil <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
websocket/README.md (2)
151-151: Consider adding a comma for clarity.Add a comma after "no matter" for improved readability.
- All connections will be sent to the handler function no matter whether the subprotocol negotiation is successful or not. + All connections will be sent to the handler function, no matter whether the subprotocol negotiation is successful or not.Tools
LanguageTool
[typographical] ~151-~151: Consider adding a comma here.
Context: ...ons will be sent to the handler function no matter whether the subprotocol negotiat...(NO_MATTER_COMMA)
153-153: Add a comma before "but" for grammatical correctness.Add a comma before "but" to separate the independent clauses.
- If a connection includes the `Sec-Websocket-Protocol` header in the request but the protocol negotiation fails, the browser will immediately disconnect the connection after receiving the upgrade response. + If a connection includes the `Sec-Websocket-Protocol` header in the request, but the protocol negotiation fails, the browser will immediately disconnect the connection after receiving the upgrade response.Tools
LanguageTool
[uncategorized] ~153-~153: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ebsocket-Protocol` header in the request but the protocol negotiation fails, the bro...(COMMA_COMPOUND_SENTENCE_2)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- websocket/README.md (1 hunks)
Additional context used
LanguageTool
websocket/README.md
[typographical] ~151-~151: Consider adding a comma here.
Context: ...ons will be sent to the handler function no matter whether the subprotocol negotiat...(NO_MATTER_COMMA)
[uncategorized] ~153-~153: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ebsocket-Protocol` header in the request but the protocol negotiation fails, the bro...(COMMA_COMPOUND_SENTENCE_2)
Additional comments not posted (1)
websocket/README.md (1)
147-147: New section added: "Note for WebSocket subprotocols".The addition of this section provides valuable information about handling WebSocket subprotocols.
Sorry for my poor English....
Summary by CodeRabbit
README.md, clarifying configuration, negotiation, and connection behavior.