-
Notifications
You must be signed in to change notification settings - Fork 57
Add documentation to authenticate Ably client #1983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Frontend shouldn't expose API key. Alternatively, it should use token that is generated by the backend.
WalkthroughThe update introduces a new method for establishing a connection to Ably using an Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Ably.Realtime Client
participant Backend as Token Service
Client->>+Backend: Invoke authCallback (fetch token)
Backend-->>-Client: Return token or error
alt Successful token retrieval
Client->>Client: Establish connection with token
else Error fetching token
Client->>Client: Handle error
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/react.md (2)
54-54
: Improve Explanatory Text Phrasing
Consider adding a comma after “Alternatively” for improved clarity. For example, “Alternatively, you can use the auth callback…” reads more naturally.🧰 Tools
🪛 LanguageTool
[typographical] ~54-~54: Consider adding a comma after ‘Alternatively’ for more clarity.
Context: ...the client app isn't the best practice. Alternatively you can use the auth callback to provid...(RB_LY_COMMA)
86-86
: Enhance Documentation Grammar
On line 86, consider adding a comma after "token generation" to improve readability. For example: “More about token generation, please refer to this documentation.”🧰 Tools
🪛 LanguageTool
[typographical] ~86-~86: Consider adding a comma here.
Context: ...er>, ); ``` More about token generation please refer to this [documentation](https://a...(PLEASE_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/react.md
(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/react.md
[typographical] ~54-~54: Consider adding a comma after ‘Alternatively’ for more clarity.
Context: ...the client app isn't the best practice. Alternatively you can use the auth callback to provid...
(RB_LY_COMMA)
[typographical] ~86-~86: Consider adding a comma here.
Context: ...er>, ); ``` More about token generation please refer to this [documentation](https://a...
(PLEASE_COMMA)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/react.md (3)
54-54
: Enhance Documentation ClarityConsider adding a comma after "Alternatively" for improved readability. For example, changing the line to:
"However, exposing API key in the client app isn't the best practice. Alternatively, you can use the auth callback to provide the token needed to establish the connection."🧰 Tools
🪛 LanguageTool
[typographical] ~54-~54: Consider adding a comma after ‘Alternatively’ for more clarity.
Context: ...the client app isn't the best practice. Alternatively you can use the auth callback to provid...(RB_LY_COMMA)
55-84
: Improve Auth Token Fetching in the Ably Client ConfigurationThe new auth callback implementation successfully removes the hardcoded API key and correctly uses asynchronous fetching with
await response.json()
. A couple of suggestions:
- HTTP Response Validation: Before parsing the JSON, consider checking whether the HTTP response was successful. This could prevent unexpected issues if the backend returns an error status. For example:
- const response = await fetch(BACKEND_URL, { - method: "POST", - }); - token = await response.json(); // adjust it based on your backend data structure + const response = await fetch(BACKEND_URL, { + method: "POST", + }); + if (!response.ok) { + throw new Error(`Token fetch failed: ${response.statusText}`); + } + token = await response.json(); // adjust according to your backend data structure
- Error Handling: The callback in the catch block correctly passes the error message to the auth callback. Ensuring robust error handling here helps prevent the client from attempting to connect with an invalid or missing token.
Overall, the implementation clearly improves security by preventing direct exposure of the API key.
86-86
: Refine Punctuation in the Documentation LinkConsider adding a comma to improve clarity in the sentence. For example, change:
"More about token generation please refer to this documentation"
to
"More about token generation, please refer to this documentation".🧰 Tools
🪛 LanguageTool
[typographical] ~86-~86: Consider adding a comma here.
Context: ...er>, ); ``` More about token generation please refer to this [documentation](https://a...(PLEASE_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
docs/react.md
(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/react.md
[typographical] ~54-~54: Consider adding a comma after ‘Alternatively’ for more clarity.
Context: ...the client app isn't the best practice. Alternatively you can use the auth callback to provid...
(RB_LY_COMMA)
[typographical] ~86-~86: Consider adding a comma here.
Context: ...er>, ); ``` More about token generation please refer to this [documentation](https://a...
(PLEASE_COMMA)
Frontend shouldn't expose API key. Alternatively, it should use token that is generated by the backend.
Summary by CodeRabbit
New Features
Documentation