-
Notifications
You must be signed in to change notification settings - Fork 42
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
WEB-3914- Pass logOut and AccountName from SessionState to the @ably-ui v15.6.0 Header component #2460
Conversation
@jamiehenson @kennethkalmer i tried to test the updated Header with logout - https://ably-docs-web-3914-head-mgssbz.herokuapp.com/docs but seems https://ably-dev.com/api/me returns |
7820bc4
to
fc23aac
Compare
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request updates several files. The Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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 (
|
… proxy details
fc23aac
to
d23942c
Compare
@coderabbitai review |
🧩 Analysis chainI'll check the contents of the key files mentioned in the PR objectives: Let me look at the specific changes to the Header component implementation: Now let me look at the component files related to the Header or SessionState: Let me try a different approach: Let me look for the code related to SessionState: Let me focus on looking at the actual diffs for this PR: Let me check more specifically for changes in JS/TS/JSX/TSX files: Let me check components directories to find the Header implementation: Let me search for files that have been modified and contain "Header": Let me check for any Layout files that might use the Header: Let me try to find components that import from @ably/ui: Let me look for component files related to the Header: Let me check the content of these layout files: Now I see the Header component is in Layout.jsx. Let's look at the changes specifically to this file: Let me also check if there's a SessionState file: Let me check where SessionState is defined or used: Let me also check for any changes to auth or session-related files: Given the PR objective to "Pass logOut and AccountName from SessionState to the @ably-ui v15.6.0 Header component", let me search for those specific props: Now I have a better understanding of the changes. Let me also check for any issues mentioned in the comment about an error with the API endpoint: Let me check if there are any files related to API configuration: Based on the changes I've found, let me now provide a comprehensive review for the PR. The main changes include:
I'll now provide my final review that addresses the changes and also addresses the issue mentioned in the previous comment about the API error. @aralovelace I have reviewed the changes in this PR which updates the The key changes look good:
Regarding the issue you mentioned about the API error with I'd recommend checking:
The code changes themselves look correct - the issue appears to be with the environment configuration rather than the implementation. 🏁 Script executed:
Length of output: 221 🏁 Script executed:
Length of output: 761 🏁 Script executed:
Length of output: 644 🏁 Script executed:
Length of output: 7878 🏁 Script executed:
Length of output: 33628 🏁 Script executed:
Length of output: 566 🏁 Script executed:
Length of output: 431 🏁 Script executed:
Length of output: 84 🏁 Script executed:
Length of output: 19929 🏁 Script executed:
Length of output: 603 🏁 Script executed:
Length of output: 1321 🏁 Script executed:
Length of output: 29155 🏁 Script executed:
Length of output: 614 🏁 Script executed:
Length of output: 85 🏁 Script executed:
Length of output: 78 🏁 Script executed:
Length of output: 817 🏁 Script executed:
Length of output: 833 🏁 Script executed:
Length of output: 328 🏁 Script executed:
Length of output: 852 🏁 Script executed:
Length of output: 143 🏁 Script executed:
Length of output: 405 @aralovelace I've reviewed this PR which updates the Key ChangesFrom examining the code and PR description, I can see that:
Regarding the API ErrorFor the issue you mentioned with Possible causes:
Recommendations:
The code changes themselves look good - the issue appears to be with the environment configuration rather than the implementation. ✅ Actions performedReview triggered.
|
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)
README.md (2)
19-19
: Grammar correction neededThe word "setup" is being used as a verb, but it should be "set up" when used as a verb.
-3. If this is the first time running the site, run `yarn run develop:env-setup` to setup your [environment variables](#environment-variables). Visit `localhost:8000` to view the site. +3. If this is the first time running the site, run `yarn run develop:env-setup` to set up your [environment variables](#environment-variables). Visit `localhost:8000` to view the site.🧰 Tools
🪛 LanguageTool
[grammar] ~19-~19: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...te, runyarn run develop:env-setup
to setup your [environment variables](#environme...(NOUN_VERB_CONFUSION)
🪛 markdownlint-cli2 (0.17.2)
19-19: Link fragments should be valid
null(MD051, link-fragments)
31-40
: Added Website Proxy documentation sectionGood addition of documentation for the Website Proxy configuration, which will help developers understand how asset prefixing works during builds and how to set up their local environment correctly.
The section uses hard tabs instead of spaces for indentation, which is inconsistent with the rest of the document. Consider replacing the hard tabs with spaces:
-1. Set the `ASSET_PREFIX=http://localhost:9000` variable in your environment. -2. Add `DOCS_BASE_URL=http://localhost:9000` to your `.env` file in the root of the Website app. -3. Ensure that `.env.production` exists in the app root for the Yarn build to function correctly. -4. You should be able to view the content and toggle between signed in and signed-out states. +1. Set the `ASSET_PREFIX=http://localhost:9000` variable in your environment. +2. Add `DOCS_BASE_URL=http://localhost:9000` to your `.env` file in the root of the Website app. +3. Ensure that `.env.production` exists in the app root for the Yarn build to function correctly. +4. You should be able to view the content and toggle between signed in and signed-out states.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
37-37: Hard tabs
Column: 3(MD010, no-hard-tabs)
38-38: Hard tabs
Column: 3(MD010, no-hard-tabs)
39-39: Hard tabs
Column: 3(MD010, no-hard-tabs)
40-40: Hard tabs
Column: 3(MD010, no-hard-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (4)
.env.example
(1 hunks)README.md
(2 hunks)package.json
(1 hunks)src/components/Layout/Header.tsx
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env.example
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~19-~19: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...te, run yarn run develop:env-setup
to setup your [environment variables](#environme...
(NOUN_VERB_CONFUSION)
🪛 markdownlint-cli2 (0.17.2)
README.md
19-19: Link fragments should be valid
null
(MD051, link-fragments)
37-37: Hard tabs
Column: 3
(MD010, no-hard-tabs)
38-38: Hard tabs
Column: 3
(MD010, no-hard-tabs)
39-39: Hard tabs
Column: 3
(MD010, no-hard-tabs)
40-40: Hard tabs
Column: 3
(MD010, no-hard-tabs)
🔇 Additional comments (3)
package.json (1)
42-42
:✅ Verification successful
Dependency update to @ably/ui 15.6.0
This update aligns with the PR objective to incorporate the enhanced Header component with dropdown and logout link functionality.
Let's verify that this version exists and check if there are any breaking changes or security issues:
🏁 Script executed:
#!/bin/bash # Check if version 15.6.0 exists in npm registry echo "Checking if @ably/ui v15.6.0 exists in npm registry:" npm view @ably/ui versions --json | grep "15.6.0" || echo "Version not found" # Check for changes between the versions echo "Checking for changes between v15.4.1 and v15.6.0:" npm view @ably/ui time --json | grep -E "15.4.1|15.5.0|15.5.1|15.6.0"Length of output: 1100
Verified: Dependency @ably/ui updated to 15.6.0 is valid
The npm registry check confirms that version 15.6.0 exists, and the release timeline indicates a series of incremental updates from 15.4.1 to 15.6.0. There are no indications of breaking changes or security issues based on the published timestamps. This aligns with the PR’s objective of incorporating the enhanced Header component with the dropdown and logout link functionality.
- File: package.json, Line: 42
"@ably/ui": "15.6.0",src/components/Layout/Header.tsx (1)
18-19
:✅ Verification successful
Added default values for logOut and accountName
This change properly initializes the
logOut
andaccountName
properties when they're undefined in the context, which prevents potential runtime errors when these values are accessed by the Header component from the @ably/ui v15.6.0 package.Let's check how these properties are used in the codebase:
🏁 Script executed:
#!/bin/bash # Verify how the sessionState properties are used in the application echo "Checking usage of sessionState in the codebase:" rg -A 5 -B 5 "sessionState\s*=\s*{" --type tsx --type jsxLength of output: 268
Below is a new shell script that uses a corrected regex pattern to check how the
logOut
andaccountName
properties fromuserContext.sessionState
are used in the codebase. Please run these commands manually to verify that their usage aligns with the intended default values:Default Value Initialization in Header.tsx
- File:
src/components/Layout/Header.tsx
(Lines 18-19)- Changes:
logOut
is initialized as:logOut: userContext.sessionState.logOut ?? { token: '', href: '' },accountName
is initialized as:accountName: userContext.sessionState.accountName ?? '',- Rationale:
These default values guarantee that even if the underlying properties are undefined, the Header component (used by the @ably/ui v15.6.0 package) safely receives valid values—thereby preventing potential runtime errors.Please manually verify the usage of these properties across the codebase using the above script output and ensure consistency.
🏁 Script executed:
#!/bin/bash echo "Checking usage of userContext.sessionState.logOut in the codebase:" rg -A 5 -B 5 "userContext\.sessionState\.logOut" --type tsx --type jsx echo "Checking usage of userContext.sessionState.accountName in the codebase:" rg -A 5 -B 5 "userContext\.sessionState\.accountName" --type tsx --type jsxLength of output: 624
Verification of Default Property Initialization in Header Component
The default values for
logOut
({ token: '', href: '' }
) andaccountName
(''
) insrc/components/Layout/Header.tsx
(lines 18-19) properly ensure runtime stability by preventing potential errors when these values are undefined. An earlier attempt to verify their usage with file type filters resulted in errors due to unrecognized file types. The updated script above removes the file type restrictions, so please re-run these commands manually to verify that the properties are used as expected across the codebase.
🏁 Script executed:
#!/bin/bash echo "Checking usage of userContext.sessionState.logOut in the codebase:" rg -A 5 -B 5 "userContext\.sessionState\.logOut" echo "Checking usage of userContext.sessionState.accountName in the codebase:" rg -A 5 -B 5 "userContext\.sessionState\.accountName"Length of output: 2279
Verified: Default Value Initialization in Header Component
The default values for
logOut
andaccountName
insrc/components/Layout/Header.tsx
(lines 18-19) have been verified as follows:
Usage Confirmation:
The shell script output confirms thatlogOut
is initialized to{ token: '', href: '' }
andaccountName
to''
, ensuring that even if these values are undefined inuserContext.sessionState
, they are safely defaulted, which prevents runtime errors.Location Check:
Both properties are used correctly in the component, as seen in the shell script output forHeader.tsx
.No further changes are required.
README.md (1)
15-21
: Fixed step numbering in Run locally sectionThe step numbering has been corrected, which improves readability and makes the instructions clearer to follow.
🧰 Tools
🪛 LanguageTool
[grammar] ~19-~19: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...te, runyarn run develop:env-setup
to setup your [environment variables](#environme...(NOUN_VERB_CONFUSION)
🪛 markdownlint-cli2 (0.17.2)
19-19: Link fragments should be valid
null(MD051, link-fragments)
README.md
Outdated
|
||
4. If this is the first time running the site, run `yarn run develop:env-setup` to setup your [environment variables](#environment-variables). Visit `localhost:8000` to view the site. | ||
3. If this is the first time running the site, run `yarn run develop:env-setup` to setup your [environment variables](#environment-variables). Visit `localhost:8000` to view the site. |
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.
💡 Verification agent
🧩 Analysis chain
Invalid link fragment
The link to [environment variables](#environment-variables)
points to a fragment that doesn't exist in the document.
Let's check if the environment variables section exists in the README:
🏁 Script executed:
#!/bin/bash
# Check if there's an environment variables section in the README
echo "Searching for environment variables section:"
grep -n "^##\s*Environment\s*Variables" README.md || echo "Section not found"
Length of output: 195
Action Required: Fix Invalid Link Fragment in README
The [environment variables](#environment-variables)
link in README.md (line 19) is pointing to a non-existent section. Please either update the link to target an existing section or add an "Environment Variables" section to the document.
- Location: README.md, line 19
- Issue: The targeted fragment
#environment-variables
does not exist
🧰 Tools
🪛 LanguageTool
[grammar] ~19-~19: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...te, run yarn run develop:env-setup
to setup your [environment variables](#environme...
(NOUN_VERB_CONFUSION)
🪛 markdownlint-cli2 (0.17.2)
19-19: Link fragments should be valid
null
(MD051, link-fragments)
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.
This makes sense, also the numbering needs to be fixed as we now have 1, 1, 2, 3, 4
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.
@jamiehenson You're absolutely right. The current diff has introduced a duplicate "1" in the numbered list, resulting in a "1, 1, 2, 3, 4" sequence instead of the proper "1, 2, 3, 4, 5" sequence. Since the first item (prerequisites) was already numbered as 1, the subsequent items should be 2, 3, 4, and 5 respectively to maintain correct sequential numbering.
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.
thanks @jamiehenson I removed the targer fragment as the Environment Variables section has been removed.
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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.
Looks good, and we should also get the improved UX for the mobile menu in tablet view out with this as well (introduced in ably-ui 15.5).
Only thing here is to fix up the changes to the README - follow 🐰's suggestions (both the actionable suggestion, and the nitpick suggestions). I'll stamp after that
…Website proxy details
Description
WEB-3914 - updated the @ably-ui to 15.6.0 which includes an updated Header component including a dropdown and logout link when
sessionState.SignedIn
is true.Checklist
Also includes updates to the documentation and codebase to improve local development setup and dependency management.
Documentation Improvements:
.env.example
file to include instructions for setting theASSET_PREFIX
when building locally with Yarn.README.md
to correct the numbering of setup steps and added a new section on the website proxy configuration. [1] [2]Dependency Management:
@ably/ui
dependency version inpackage.json
from15.4.1
to15.6.0
.Code Enhancements:
logOut
andaccountName
in theHeader
component to ensure proper initialization.Summary by CodeRabbit