Uh oh!
There was an error while loading. Please reload this page.
UI Tests for username change on /settings page. - #1203
Conversation
@petercr is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request enhance the end-to-end test suite for the authenticated settings page by adding a new test case focused on the username input field. The modifications include updating the import statements and implementing a series of assertions that validate the visibility, attributes, and functionality of the username input, including error handling for invalid inputs and confirmation of valid submissions. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
e2e/settings.spec.ts(2 hunks)
🔇 Additional comments (1)
e2e/settings.spec.ts (1)
Line range hint 4-7: Implement missing unauthenticated tests
The PR objectives mention testing unauthenticated user redirection to /get-started, but this test is missing.
Would you like me to provide an implementation for the unauthenticated test case that verifies redirection?
| // Test for changing username | ||
| test('Username input field', async ({ page }) => { | ||
| await page.goto('http://localhost:3000/settings', { timeout: 30000 }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test configuration and navigation
Consider these improvements:
- The 30-second timeout seems excessive for local development
- The hardcoded URL should be replaced with a configurable base URL
- await page.goto('http://localhost:3000/settings', { timeout: 30000 });+ await page.goto('/settings');Add this to your playwright.config.ts:
use: {baseURL: process.env.BASE_URL||'http://localhost:3000',}There was a problem hiding this comment.
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!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| import { test, expect } from "@playwright/test"; | ||
| import { loggedInAsUserOne } from "./utils"; | ||
| test.describe("Unauthenticated setttings Page", () => { |
There was a problem hiding this comment.
Can we change this to authenticated settings page. Because the user is authenticated for these tests
There was a problem hiding this comment.
I'm not sure exactly what you're saying here?
Did you want a separate file for auth vs. un-auth tests, because doesn't a user have to be logged in to edit their profile?
I guess I could use more detail on this 👌🏻
There was a problem hiding this comment.
Your new test is ran inside a test suite called 'Unauthenticated setttings Page'
But this is an authenticated test because users need to login to change their username.
If you just change the name of the test it will be fixed
| // Test that the input field can be filled with a valid value and saves it | ||
| await inputField.fill('codu-rules'); | ||
| await page.locator('button[type="submit"]').click(); | ||
| await expect(inputField).toHaveValue('codu-rules'); |
There was a problem hiding this comment.
Will this wait for the username change to take effect?
If you fill in the new username and then click submit.
Will this test see the new username you have entered as the value of the input and complete straight away?
There was a problem hiding this comment.
I mean the tests pass for it. But it could be just catching the still filled in form from before the reset.
Would you like me to change it to a page refresh instead?
Or would you like some kind of timeOut in-between the state change and checking for the change to persist?
I would be happy to add either one 👍🏻
There was a problem hiding this comment.
Yeah I am not sure is there a loading indicator for these types of change? It would be nice to see
- Click submit
- Loading indicator appears
- Loading indicator hidden
- Check new username has taken effect
JohnAllenTech
left a comment
There was a problem hiding this comment.
Just made some general comments. I am not a maintainer here so everything is just a suggestion
NiallJoeMaher
commented
Nov 4, 2024
@JohnAllenTech let me know when this is good to go since you are the overlord of tests. |
petercr
commented
Nov 7, 2024
@JohnAllenTech I made some changes and added the extra tests you had suggested.
I did have to change a field in I also added new class names to both the toast messages for UI testing. Thanks |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
e2e/settings.spec.ts (2)
Line range hint
4-8: Fix typo and remove empty test suite.
- Fix the typo in "setttings" (three 't's).
- The empty test suite for unauthenticated users should either be implemented or removed to maintain clean code.
-test.describe("Unauthenticated setttings Page", () => {- //- // Replace with tests for unauthenticated users-});+// TODO: Implement tests for unauthenticated users being redirected to /get-started+test.describe("Unauthenticated settings Page", () => {+ test("redirects to get-started page", async ({ page }) => {+ await page.goto("/settings");+ await expect(page).toHaveURL("/get-started");+ });+});
27-32: Improve error message validation.The error message validation could be more robust by:
- Using data-testid for reliable selection
- Centralizing error messages
- Verifying error styling
+ // Define error messages+ const ERROR_MESSAGES = {+ INVALID_CHARS: 'Username can only contain alphanumerics and dashes.'+ } as const;+- const errorMessage = page.locator('p:text-is("Username can only contain alphanumerics and dashes.")')+ const errorMessage = page.locator('[data-testid="username-error"]'); await expect(errorMessage).toBeVisible(); - await expect(errorMessage).toHaveText('Username can only contain alphanumerics and dashes.');+ await expect(errorMessage).toHaveText(ERROR_MESSAGES.INVALID_CHARS);+ await expect(errorMessage).toHaveCSS('color', 'rgb(220, 38, 38)'); // Verify error stylinge2e/setup.ts (1)
123-123: Document the reason for URL format change.While the change to use an absolute URL is sensible, it would be helpful to document why this change was necessary, especially since it seems tangential to the main PR objective of testing username changes.
Add a comment explaining the URL format requirement:
+ // Using absolute URL to comply with website URL validation requirements websiteUrl: "https://codu.co",app/(app)/settings/_client.tsx (2)
Line range hint
261-276: Consider adding a loading indicator during username validation.While the username field implementation is solid, it could benefit from a visual indicator during validation to improve user experience. This would be particularly useful when checking for username availability.
Consider adding a loading state:
<Field> <Input id="username" type="text" autoComplete="username" invalid={!!errors?.username} + className={isValidating ? "opacity-50" : ""} {...register("username")} /> + {isValidating && (+ <div className="mt-2 flex items-center text-sm text-gray-500">+ <Loader2 className="mr-2 h-4 w-4 animate-spin" />+ Checking username availability...+ </div>+ )} {errors?.username && ( <ErrorMessage>{errors.username.message}</ErrorMessage> )} </Field>Line range hint
1-458: Consider breaking down the Settings component for better maintainability.The component is well-structured but handles multiple responsibilities. Consider splitting it into smaller, focused components for better maintainability and testing.
Suggested structure:
Create separate components for each section:
ProfilePhotoSectionPersonalInfoSection(name, username, bio)ContactSection(location, website)EmailSectionNotificationsSectionMove the file upload logic to a custom hook:
// hooks/useProfilePhotoUpload.tsconstuseProfilePhotoUpload=()=>{const[profilePhoto,setProfilePhoto]=useState<ProfilePhoto>({...});// ... file upload logicreturn{ profilePhoto, handleImageChange };};This refactoring would:
- Improve code organization
- Make testing easier
- Reduce cognitive load when maintaining the component
- Allow for better error boundary placement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/(app)/settings/_client.tsx(1 hunks)e2e/settings.spec.ts(2 hunks)e2e/setup.ts(1 hunks)
🔇 Additional comments (3)
e2e/settings.spec.ts (1)
43-45: Verify username persistence more thoroughly.
The current persistence check could be enhanced by:
- Verifying the username in the UI header/profile
- Checking the API response
- Testing persistence across multiple page navigations
✅ Verification successful
Current username persistence testing is sufficient
The current e2e test adequately verifies username persistence because:
- The username is properly integrated throughout the application (Nav, BioBar, Settings)
- The page reload test verifies server-side persistence
- Additional UI checks would be redundant as the username update automatically propagates through the layout and components
The implementation shows the username is:
- Stored in the database and fetched server-side (layout.tsx)
- Consistently displayed across navigation and profile components
- Properly validated and persisted in settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash# Description: Verify username persistence across the application# Note: This script helps identify places where the username should be updated# Find files that might display or use the usernameecho"Files potentially using username:"
rg -l "username" --type=tsx --type=ts
# Find API endpoints handling usernameecho"\nAPI endpoints for username:"
rg -l "username.*api" --type=ts
Length of output: 245
Script:
#!/bin/bash# Let's try a different approach to find username-related code# Search for username-related patterns in all filesecho"Files containing username-related patterns:"
rg -l -i "username|user.*name"# Look for settings or profile-related componentsecho -e "\nSettings and profile-related files:"
fd -e ts -e tsx -e js -e jsx "settings|profile" --type f
# Search for API calls or endpointsecho -e "\nAPI-related patterns:"
rg -l "api.*username|username.*api"# Look specifically in the e2e test file to understand the context betterecho -e "\nContext from the e2e test file:"
rg -C 5 "username""e2e/settings.spec.ts"Length of output: 3690
Script:
#!/bin/bash# Let's examine the settings page implementation and username persistence logic# Check settings page implementationecho"Settings page implementation:"
rg -C 10 "username""app/(app)/settings/_client.tsx"# Check username usage in layout and navigationecho -e "\nUsername usage in layout:"
rg -C 5 "username""app/(app)/layout.tsx"# Check profile/bio components that might display usernameecho -e "\nUsername display in BioBar:"
rg -C 5 "username""components/BioBar/BioBar.tsx"# Check navigation components for username displayecho -e "\nUsername in navigation:"
rg -C 5 "username""components/Nav/Nav.tsx"Length of output: 4555
e2e/setup.ts (1)
123-123: Verify URL format change impact on tests.
The change from relative URL "codu.co" to absolute URL "https://codu.co" looks good, but let's ensure this aligns with the validation requirements.
Let's verify the URL validation requirements:
✅ Verification successful
URL format change aligns with schema requirements
The change from "codu.co" to "https://codu.co" is correct as the websiteUrl field in schema/profile.ts uses Zod's .url() validator, which requires a fully qualified URL with protocol. This validation would reject the previous "codu.co" format.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash# Description: Check URL validation schema and related tests# Look for URL validation schema
rg -A 5 'websiteUrl.*zod'# Look for website URL related tests
rg -A 5 'websiteUrl.*test'Length of output: 56
Script:
#!/bin/bash# Let's try a broader search for URL validation patterns and website-related schemas# Search for any URL validation patterns
rg -i "url.*validation|validate.*url" -A 5
# Search for website URL usage in tests
rg "website.*url|url.*website" -A 5
# Search for zod schema definitions that might include URL
rg "z\.string\(\).*url|url.*z\.string" -A 5
# Look for any URL-related constants or configurations
rg "URL_PATTERN|URL_REGEX" -A 5
Length of output: 2168
app/(app)/settings/_client.tsx (1)
89-94: LGTM: Toast styling changes are clean and consistent.
The addition of className props to toast notifications provides better styling control while maintaining the existing functionality.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
JohnAllenTech
commented
Nov 9, 2024
Hey @petercr apologies I was AFK for a few days. I'll take a look at this in a while and reply properly |
NiallJoeMaher
commented
Nov 16, 2024
Tests failing here. |
petercr
commented
Nov 16, 2024
Well crap 😕 that's no good. I wonder if this has to do with the newsletter sub function? |
petercr
commented
Nov 18, 2024
When I run the test locally, they all pass 🤷🏼♂️ see pic below. I added extra code on line 39 to keep from getting the email API errors for the newsletter. Here's what I added locally for this: if(process.env.ENV==="local"||"development"){returntrue;}But it might be simpler to go with: if(process.env.ENV!=="production"){returntrue;}Here is the image of the tests passing.
|
✨ Codu Pull Request 💻
Fixes#1168
** Only Part 3.**
Pull Request details
Added UI tests for
/settingsto:Any Breaking changes
None
Associated Screenshots
None, but the tests pass 😅 I promise!
[Optional] What gif best describes this PR or how it makes you feel
Like this, only instead of Monday, Wed, Friday it's like start, middle, end 😆
