test(portal-e2e): implement testrail #356
Conversation
Crashes happened due to hackerdock server side rendering
WalkthroughThis pull request introduces a new deletion command and associated script while updating several test suites and UI components. The changes include modifications to OAuth rendering, Playwright configuration, and E2E test flows with revised login and registration actions. Updates are also made to routing, logout, and dashboard links, and adjustments to form submission and toast notifications in the portal. Additional enhancements improve accessibility, client-only rendering in layout, and introduce a new HackerDock component. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Command Line
participant Script as deleteUser Script
participant API as Payload API
CLI->>Script: Run "delete:user" command
Script->>API: Fetch user by email (from env variable)
alt User Found
API-->>Script: Return user data
Script->>API: Request to delete user by ID
Script-->>CLI: Log "User deleted"
else User Not Found
API-->>Script: No user found
Script-->>CLI: Log "User does not exist"
end
sequenceDiagram
participant Browser as Browser
participant Server as Logout Loader
participant API as Logout API
Browser->>Server: Request /logout with Cookie
Server->>API: POST to /api/users/logout (with Cookie)
alt Response OK
API-->>Server: Success response
else Response Fail
API-->>Server: Error response
Server->>Server: Log error
end
Server->>Browser: Redirect to /login with cleared cookie
Suggested labels
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
Progress: resolved 0, reused 0, downloaded 1, added 0 This error happened while installing a direct dependency of /tmp/eslint ✨ Finishing Touches
🪧 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
Documentation and Community
|
HasithDeAlwis
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (4)
apps/portal/app/routes/api.auth.tsx (2)
12-19: 🛠️ Refactor suggestionRevisit commented WIP implementation
The code contains a commented "WIP" section that likely represents the intended final implementation. Consider implementing the proper solution that sets the X-Client-Origin header as indicated in the comments.
try { - return redirect(`${baseUrl}/api/users/oauth/linkedin`) + const response = redirect(`${baseUrl}/api/users/oauth/linkedin`) + response.headers.set('X-Client-Origin', 'portal') + return response - // WIP SHOULD BE DOING THE BELOW - // cannot make a get request if you are doing a redirect - /* const response = redirect(`${baseUrl}/api/users/oauth/linkedin`) -* response.headers.set('X-Client-Origin', 'portal') -* return response */
20-55: 🧹 Nitpick (assertive)Clean up commented-out code
There's a large block of commented-out code that appears to be an older implementation. If it's no longer needed, consider removing it to improve code maintainability. If it contains logic that should be implemented later, add a more specific TODO comment with details about what needs to be done.
libs/portal/features/profile/hooks/use-profile-schema.ts (1)
81-89: 🧹 Nitpick (assertive)Consider expanding Google Drive link regex
The current regex for validating Google Drive links handles basic patterns but might not account for all valid formats. Google Drive links can have various formats including shortened links, shared folders, or different document types. Consider expanding the validation to handle these cases or provide more specific error messages.
apps/portal/app/routes/registration.tsx (1)
32-35: 🛠️ Refactor suggestionApply consistent credentials handling to forms fetch
You've added
credentials: 'include'to the/api/users/mefetch but not to the form submissions fetch below. For consistency and to ensure proper authentication, consider adding the same credentials option to this fetch call as well.const forms = await fetch(`${API_URL}/api/form-submissions?where[submittedBy][equals]=${user.id}`, { method: 'GET', + credentials: 'include', headers: { Cookie: cookie || '' }, })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (29)
apps/axiom/project.json(1 hunks)apps/axiom/src/components/AfterLogin/index.tsx(1 hunks)apps/axiom/src/delete-user.ts(1 hunks)apps/portal-e2e/playwright.config.ts(1 hunks)apps/portal-e2e/src/index.spec.ts(3 hunks)apps/portal-e2e/src/login/hacker/abilities.ts(2 hunks)apps/portal-e2e/src/profile/hacker/abilities.ts(2 hunks)apps/portal-e2e/src/profile/hacker/index.spec.ts(1 hunks)apps/portal-e2e/src/registration/hacker/abilities.ts(1 hunks)apps/portal-e2e/src/registration/hacker/index.spec.ts(1 hunks)apps/portal-e2e/src/registration/questions.ts(2 hunks)apps/portal/app/routes/api.auth.tsx(1 hunks)apps/portal/app/routes/login.tsx(2 hunks)apps/portal/app/routes/registration.tsx(1 hunks)apps/portal/vite.config.ts(1 hunks)libs/cms/auth/config.ts(1 hunks)libs/portal-e2e/abilities/index.ts(1 hunks)libs/portal-e2e/questions/index.ts(1 hunks)libs/portal/features/legal/ui/terms.tsx(2 hunks)libs/portal/features/login/ui/call-to-action.tsx(1 hunks)libs/portal/features/profile/hooks/use-profile-schema.ts(1 hunks)libs/portal/features/profile/ui/questions.tsx(2 hunks)libs/portal/features/registration/ui/registration.tsx(1 hunks)libs/portal/pages/profile/index.tsx(2 hunks)libs/portal/pages/registration/index.tsx(2 hunks)libs/portal/ui/dashboard/social-links/social-links.tsx(1 hunks)libs/portal/ui/hackerdock/hackerdock.client.tsx(0 hunks)libs/portal/ui/hackerdock/index.tsx(1 hunks)libs/portal/ui/layout.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- libs/portal/ui/hackerdock/hackerdock.client.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
libs/portal/features/legal/ui/terms.tsx
[error] 116-120: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 139-139: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
🔇 Additional comments (39)
libs/portal/ui/hackerdock/index.tsx (1)
1-1: Update to client-side component import looks good.This change aligns with modern React frameworks' client/server component separation pattern. The
.clientsuffix is a good convention that clearly indicates this component should only run on the client side.libs/portal/ui/layout.tsx (3)
5-5: Good addition of ClientOnly import.Adding the ClientOnly utility from remix-utils is appropriate for components that need client-only rendering.
8-8: Function signature improvement with optional parameter.Making
isCompleteProfileoptional with a default value improves the API. This is a good TypeScript pattern.
23-28: Proper implementation of client-only rendering.Wrapping HackerDock in ClientOnly ensures it only renders in the browser. The callback pattern with fallback=null correctly handles server-side rendering.
This change correctly addresses issues that would occur if HackerDock uses browser-only APIs.
libs/portal-e2e/questions/index.ts (2)
1-1: Clean and Accurate Import StatementThe import of
buttonfrom the shared test module is correctly implemented and clearly indicates dependence on a common test utility.
3-3: Clear Definition of the LOGIN ConstantDefining
LOGINusingbutton.withText('LOG IN')is straightforward and consistent with project conventions.libs/portal/ui/dashboard/social-links/social-links.tsx (1)
29-35: Discord URL Update:
You've correctly updated thehreffor the Discord link tohttps://discord.gg/VnbWdAe8kA. Please verify that this URL meets the latest requirements and update any related documentation if necessary.apps/portal/vite.config.ts (1)
23-23:✅ Verification successful
Verify impact of disabling lazy route discovery
Changing
v3_lazyRouteDiscoveryfrom true to false will load all routes at build time rather than lazily. This may increase initial bundle size but improve navigation performance. Ensure this change is intentional and doesn't negatively impact the application's performance or loading times.
🏁 Script executed:
#!/bin/bash # Analyze the routes structure to understand the impact echo "Checking Remix route files count:" find apps/portal/app/routes -type f | grep -v "\.test\." | wc -l echo "\nChecking for potential route structure complexity:" find apps/portal/app/routes -type f | grep -v "\.test\." | sortLength of output: 641
Impact of Eager Route Loading Confirmed
- In
apps/portal/vite.config.ts(line 23), settingv3_lazyRouteDiscoverytofalsewill load all routes at build time.- Analysis of the routes folder shows 9 route files, indicating that the increased bundle size should be minimal.
- This eager loading is expected to improve navigation performance; please verify that this trade-off aligns with current performance benchmarks.
apps/portal/app/routes/registration.tsx (1)
16-20:❓ Verification inconclusive
Ensure consistency in fetch credential handling
You've added
credentials: 'include'to enable cookies for cross-origin requests, which is good. However, check that all other API calls in the application consistently handle credentials, especially those related to authentication flows.
🏁 Script executed:
#!/bin/bash # Find all fetch calls in the portal app to check for credentials handling echo "Fetch calls without credentials option:" grep -r "fetch(" --include="*.tsx" --include="*.ts" apps/portal | grep -v "credentials" | grep -B 2 -A 2 "fetch" echo "\nFetch calls with credentials option:" grep -r "fetch(" --include="*.tsx" --include="*.ts" apps/portal | grep "credentials" | grep -B 2 -A 2 "fetch"Length of output: 1387
Action Required: Verify Consistency in Credential Handling Across All API Calls
You've added
credentials: 'include'inapps/portal/app/routes/registration.tsxfor the/api/users/mecall, which is a step in the right direction for ensuring cookies are sent with cross-origin requests. However, an initial search across the portal codebase indicates that several fetch calls—in files such as_index.tsx,dashboard.tsx,login.tsx, andprofile.tsx—do not visibly include a credentials option. Note that our previous automated search might miss multi-line credential configurations (for example, when thecredentialsproperty is defined on a separate line from thefetch(invocation).Please verify that:
- Intended Behavior: All API calls that rely on authentication or need to send cookies use
credentials: 'include'(or another appropriate setting), and that the omission in some cases isn’t deliberate.- Multi-line Definitions: Review instances where the credential handling might be defined over multiple lines to ensure they’re not falsely flagged as missing this configuration.
- Consistency: Update the fetch calls in files like
apps/portal/app/routes/_index.tsx,dashboard.tsx,login.tsx, andprofile.tsxif they are supposed to mirror the behavior established in registration.Manual review is recommended to confirm the consistency across fetch calls.
libs/portal/features/login/ui/call-to-action.tsx (1)
27-27: Added cursor-pointer class to improve button UXGood enhancement to provide visual feedback that the button is clickable.
libs/portal/pages/registration/index.tsx (1)
24-24:✅ Verification successful
Removed hardcoded submittedBy value of '6'
Good change to remove the hardcoded submittedBy field with value '6'.
However, verify that:
- The backend no longer requires this field
- User attribution is still correctly tracked
🏁 Script executed:
#!/bin/bash # Check if the postForm function or backend API still expects submittedBy echo "Checking postForm implementation for submittedBy parameter..." grep -r "postForm" --include="*.ts" --include="*.tsx" libs/portalLength of output: 623
Verification Complete: Dynamic user attribution confirmed.
The change to remove the hardcoded "submittedBy" value and instead pass
user.idis consistent with the backend's expectations, as confirmed by the definition ofpostForminlibs/portal/shared/features/form/api/index.ts(which expects auserIdparameter). No further modifications are needed.apps/portal-e2e/src/login/hacker/abilities.ts (2)
8-10: Improvement: Environment variables for sensitive data.Good move replacing hardcoded credentials with environment variables. This aligns with security best practices and makes the tests configurable.
35-37: Credentials update looks good.The function now properly uses the environment variables instead of hardcoded values.
libs/portal/features/profile/ui/questions.tsx (2)
222-222: Better label for resume link field.Updating the label to clarify PDF requirement helps users understand the expected format.
460-460: Improved cursor styling based on button state.Good UX improvement to provide visual feedback on the disabled state.
apps/axiom/src/components/AfterLogin/index.tsx (1)
29-46: OAuth buttons commented out - intentional?You've commented out GitHub, Discord, and Google login options, leaving only LinkedIn. Is this intentional or part of a phased rollout?
If this is intended to be permanent, consider removing the code instead of commenting it out to keep the codebase clean. If it's temporary (for testing with just LinkedIn), add a TODO comment explaining when these will be re-enabled.
libs/portal/features/registration/ui/registration.tsx (1)
108-111: Enhanced button state feedbackGood enhancement to provide visual feedback through CSS classes that match the disabled state logic. This improves UX by clearly indicating when the button is not interactive.
libs/cms/auth/config.ts (1)
9-17: Improved redirect logic with explicit pathsThe redirect logic is now more explicit with clear paths for admin users vs regular users. The added
elseblock makes the code more readable and ensures proper flow control.libs/portal-e2e/abilities/index.ts (1)
1-24: Good addition of reusable login function for E2E testsThis utility function provides a consistent way to handle authentication in E2E tests, following good DRY principles. The task sequence is logical and handles the login flow well.
Consider adding error handling for cases where elements might not be found or timeouts occur to make the utility more robust.
apps/portal-e2e/src/profile/hacker/index.spec.ts (1)
1-37:❓ Verification inconclusive
Verify if commented tests are intentionally disabled
Commenting out entire test files reduces test coverage and can lead to regression issues. If this is part of the TestRail implementation, please clarify the plan for these tests:
- Are they being replaced by equivalent tests in another location?
- Is this a temporary measure during migration?
- Is there a tracking issue for re-enabling these tests?
Without tests for profile creation scenarios, you may miss regressions in this functionality.
🏁 Script executed:
#!/bin/bash # Check if other test files have been commented out as well echo "Checking for other commented test files..." rg -l "^// describe" --type ts apps/portal-e2e/src/Length of output: 285
Action: Confirm the Intent and Plan for Disabled Tests
It appears that disabled tests via commented code are present in multiple files:
apps/portal-e2e/src/profile/hacker/index.spec.tsapps/portal-e2e/src/registration/hacker/index.spec.tsapps/portal-e2e/src/index.spec.tsPlease clarify:
- Are these tests being replaced by equivalent tests elsewhere?
- Is disabling them a temporary step during a migration (e.g., as part of your TestRail implementation)?
- Is there a tracking issue or timeline for re-enabling these tests?
Since the profile creation scenarios are currently untested, addressing this is essential to avoid regressions.
apps/portal/app/routes/login.tsx (1)
25-27:✅ Verification successful
Logic simplified for redirection when user exists
The redirection logic has been simplified to just check for user existence and redirect directly. This makes the code cleaner, but there's a potential risk that the session-based auth is no longer being managed properly.
🏁 Script executed:
#!/bin/bash # Checking for session management implementation elsewhere rg -A 3 "getSession|commitSession" --type=tsx --type=tsLength of output: 85
Below is an updated shell script to re-verify that session management (e.g. via
getSessionorcommitSession) is implemented elsewhere. The previous command failed due to the tsx file type flag. This script uses explicit globs to include both *.ts and *.tsx files:Once you run this script and review the output, we can confirm whether the redirection logic (i.e., the simple check for user existence in
apps/portal/app/routes/login.tsxat lines 25–27) inadvertently skips important session management. Please provide the output for further verification.
🏁 Script executed:
#!/bin/bash # Re-verify session management by searching for getSession and commitSession across *.ts and *.tsx files using explicit file globs. rg -g "*.{ts,tsx}" -A 3 "getSession|commitSession"Length of output: 1028
Verified session management is properly implemented across the codebase.
The simplified redirection in
apps/portal/app/routes/login.tsx(lines 25–27) now solely checks for user existence to redirect, which is acceptable given the following:
- Session management utilities (
getSession,commitSession,destroySession) are implemented inapps/portal/app/sessions.ts.- These utilities are actively used in routes such as
apps/portal/app/routes/api.auth.tsxto manage session cookies.No immediate issues were detected with simplifying the login redirection, provided that session handling remains consistently managed elsewhere in the code. Please verify that the login flow correctly initializes sessions where needed.
apps/portal-e2e/playwright.config.ts (1)
15-16: Explicit disabling of parallel test executionSetting
fullyParalleltofalseensures consistent test execution behavior regardless of environment, which can help with test reliability. This is especially useful for TestRail integration where test cases might have ordering dependencies.libs/portal/pages/profile/index.tsx (5)
6-6: Added toast notification dependencyGood addition. Using a toast library for user feedback is more intuitive than relying solely on response objects.
15-17: Improved user feedback with better naming and loading stateUsing past tense for the action and adding a loading toast improves the user experience.
24-28: Success toast notificationThe toast success notification provides clear feedback to users on successful profile operations.
29-29: Consistent response messageUsing the same message in the toast and the Response object is a good practice for consistency.
37-41:❓ Verification inconclusive
Error feedback with toast
Good error handling with toast notification. However, there's an inconsistency in how errors are handled across the application.
🏁 Script executed:
#!/bin/bash # Check for error toast pattern consistency rg -A 5 "toast\.error" --type tsx --type tsLength of output: 73
Error Toast Consistency Verification
The toast error handler implementation within the file looks solid, but we need to double-check consistency across the codebase. The initial
rgcommand failed because the file typetsxwasn’t recognized. Please run the revised script below to manually verify that instances oftoast.errorare being used consistently:#!/bin/bash # Search for error toast usage in TypeScript and TSX files. rg -A 5 "toast\.error" -g "*.tsx" -g "*.ts"Once you've verified that the usage is consistent across the codebase, we can proceed accordingly.
apps/portal-e2e/src/registration/hacker/abilities.ts (2)
3-5: Good addition of Navigate and startLoggedIn imports.
These imports correctly align with the new login and navigation flow.
7-13: Verify usage of the destructured actor param.
InsideregisterForHackathon({ actor }), you callactorCalled('Hacker')but never referenceactor. Confirm this is intentional or switch to using the passed-in actor to avoid confusion.apps/portal-e2e/src/registration/questions.ts (2)
5-19: Aria label changes improve clarity.
The updated labels are more descriptive.
28-28: Renaming textArea to ‘first-time-hacker’ is consistent.
This naming is clearer and aligns well with the question’s intent.apps/portal-e2e/src/profile/hacker/abilities.ts (3)
1-4: New imports appear correct.
ImportingstartLoggedInand updated Serenity/JS modules is valid for the new login flow.
8-11: Well-structured login and navigation steps.
The destructuredactorusage and thestartLoggedIn()invocation streamline the flow into the profile page.
66-66: Confirm final route logic.
Switching from/dashboardto/registrationalters the post-profile workflow. Double-check this aligns with user expectations.apps/portal-e2e/src/index.spec.ts (5)
5-7: Imported necessary abilities for extended test coverageGood addition of required imports to support new test cases for profile creation and hackathon registration.
34-35: Navigation flow updated correctly for the login testThe updated flow now aligns with the application's expected behavior, redirecting to '/terms' after login rather than '/dashboard'.
42-42: Explicit navigation to '/terms' ensures test independenceGood practice to explicitly navigate to '/terms' rather than relying on previous test state, making this test more reliable and independently executable.
75-75: XPath selectors provide more precise element targetingSwitching from CSS to XPath selectors for checkbox text content is more robust, especially for elements with complex text content.
Also applies to: 80-80
1-568:❓ Verification inconclusive
Verify environment variables are properly configured in CI
The test depends on environment variables
LOCAL_DEV_EMAIL_ADDRESSandLOCAL_DEV_PASSWORD. Ensure these are configured in your CI environment for TestRail integration.
🏁 Script executed:
#!/bin/bash # Check for environment variable configuration in GitHub Actions workflow files grep -r "LOCAL_DEV_EMAIL_ADDRESS\|LOCAL_DEV_PASSWORD" .github/workflows/Length of output: 74
Ensure CI pipelines properly expose required environment variables for TestRail integration
Please manually verify that the environment variables
LOCAL_DEV_EMAIL_ADDRESSandLOCAL_DEV_PASSWORDare correctly configured in your CI environment. Our initial grep in the GitHub Actions workflow files didn’t return any configuration details, so confirm that these are set through repository secrets or another appropriate mechanism to ensure they’re available at runtime.🧰 Tools
🪛 Biome (1.9.4)
[error] 12-18: Do not export from a test file.
(lint/suspicious/noExportsInTest)
| export default function AuthRedirect() { | ||
| return null | ||
| return ( | ||
| <> | ||
| HELL0 world ;) | ||
| </> | ||
| ) |
There was a problem hiding this comment.
Remove debug output from AuthRedirect component
The component is returning "HELL0 world ;)" text which appears to be a temporary debug statement. This should be removed before deployment to production.
export default function AuthRedirect() {
- return (
- <>
- HELL0 world ;)
- </>
- )
+ return null
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default function AuthRedirect() { | |
| return null | |
| return ( | |
| <> | |
| HELL0 world ;) | |
| </> | |
| ) | |
| export default function AuthRedirect() { | |
| return null | |
| } |
| @@ -84,7 +84,7 @@ export function useProfileSchema( | |||
| link, | |||
| ), | |||
| { | |||
| message: 'Invalid Google Drive link.', | |||
| message: 'Invalid Google Drive link', | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Maintain consistency in error message punctuation
The error message for the Google Drive link validation has had its period removed. For consistency, check if other error messages in this schema follow the same pattern (with or without ending punctuation) and ensure they all follow the same standard.
#!/bin/bash
# Check for consistency in error message punctuation
echo "Error messages with periods:"
grep -r "message: '[^']*\.'" libs/portal/features/profile/hooks/
echo "\nError messages without periods:"
grep -r "message: '[^']*[^.]'" libs/portal/features/profile/hooks/| @@ -20,11 +20,11 @@ export function LoginCallToAction() { | |||
| </h2> | |||
| </header> | |||
|
|
|||
| <Form method="get" action="/api/auth"> | |||
| <Form method="post" > | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Changed form method from "get" to "post" and removed action attribute
This change indicates a shift in the authentication flow. Ensure the receiving endpoint is correctly set up to handle POST requests. With Remix router's conventions, the POST will be handled by the action function in the nearest route.
| // import { actorCalled, Task } from '@serenity-js/core' | ||
| // import { beforeEach, describe, it } from '@serenity-js/playwright-test' | ||
| // import { BrowseTheWeb, Navigate } from '@serenity-js/web' | ||
| // import { registerForHackathon, registerForHackathonMissingFields } from './abilities' | ||
|
|
||
| export function startLoggedInNoProfile() { | ||
| return Task.where( | ||
| '#actor starts logged out', | ||
| Navigate.to('/registration'), | ||
| ) | ||
| } | ||
| // const EMAIL_ADDRESS = process.env.LOCAL_DEV_EMAIL_ADDRESS | ||
| // const PASSWORD = process.env.LOCAL_DEV_PASSWORD | ||
|
|
||
| describe('when the hacker', () => { | ||
| beforeEach(async ({ actor }) => { | ||
| await actorCalled('Hacker') | ||
| .whoCan(BrowseTheWeb.as(actor)) | ||
| .attemptsTo(startLoggedInNoProfile()) | ||
| }) | ||
| // export function startLoggedInNoProfile() { | ||
| // return Task.where( | ||
| // '#actor starts logged out', | ||
| // Navigate.to('/registration'), | ||
| // ) | ||
| // } | ||
|
|
||
| describe('is logged in', () => { | ||
| describe('has profile', () => { | ||
| it('should be able to regsiter', async () => { | ||
| await registerForHackathon() | ||
| }) | ||
| // describe('when the hacker', () => { | ||
| // beforeEach(async ({ actor }) => { | ||
| // await actorCalled('Hacker') | ||
| // .whoCan(BrowseTheWeb.as(actor)) | ||
| // .attemptsTo(startLoggedInNoProfile()) | ||
| // }) | ||
|
|
||
| it('should not be able to regsiter with missing fields', async () => { | ||
| await registerForHackathonMissingFields() | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
| // describe('is logged in', () => { | ||
| // describe('has profile', () => { | ||
| // it('should be able to regsiter', async () => { | ||
| // await registerForHackathon() | ||
| // }) | ||
|
|
||
| // it('should not be able to regsiter with missing fields', async () => { | ||
| // await registerForHackathonMissingFields() | ||
| // }) | ||
| // }) | ||
| // }) | ||
| // }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Commented out entire test file - potential test coverage gap
The entire test file has been commented out, which removes test coverage for the registration flow. If this is part of migrating tests to TestRail, please document this transition in the code comments.
Consider adding a comment at the top of the file explaining:
- Why these tests are disabled
- Where the equivalent test cases now live in TestRail
- Whether these tests will be removed entirely in a future PR
🏁 Script executed:
#!/bin/bash
# Check if there are any other test files covering registration functionality
echo "Checking for other test files covering registration functionality..."
find apps/portal-e2e/src -type f -name "*.spec.ts" | grep -v "index.spec.ts" | grep -i "registration"Length of output: 246
Action: Document Disabled Registration Tests
The entire test file is currently commented out, which creates a potential coverage gap for the registration flow. I couldn’t locate any alternative test files within the repository to cover these scenarios, so please confirm whether these tests have been migrated to TestRail. If so, add a header comment in apps/portal-e2e/src/registration/hacker/index.spec.ts that details:
- Why these tests are disabled
- Where the equivalent test cases now reside in TestRail
- Whether these tests will be removed entirely in an upcoming PR
| "delete:user": { | ||
| "command": "cross-env NODE_OPTIONS=--no-deprecation payload run src/delete-user.ts", | ||
| "options": { | ||
| "cwd": "apps/axiom" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Added new "delete:user" command target
Good addition for testing purposes. This target will help with cleaning up test users during automated testing.
Consider adding:
- Documentation for this command in your README
- Safeguards to prevent accidental deletion of production users
| export const action: LoaderFunction = async () => { | ||
| const baseUrl | ||
| = process.env.NODE_ENV === 'development' | ||
| ? 'http://localhost:8000' | ||
| : 'https://axiom.cuhacking.ca' | ||
|
|
||
| return redirect(`${baseUrl}/api/users/oauth/linkedin`) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
New action function for LinkedIn OAuth
The new action function provides a clean way to handle LinkedIn OAuth redirection, but there's a code duplication issue with the baseUrl construction. The same logic exists in both loader and action functions.
Consider extracting the baseUrl logic into a shared utility function:
+function getBaseUrl() {
+ return process.env.NODE_ENV === 'development'
+ ? 'http://localhost:8000'
+ : 'https://axiom.cuhacking.ca'
+}
export const loader: LoaderFunction = async ({ request }) => {
const cookie = request.headers.get('Cookie')
- const baseUrl
- = process.env.NODE_ENV === 'development'
- ? 'http://localhost:8000'
- : 'https://axiom.cuhacking.ca'
+ const baseUrl = getBaseUrl()
try {
// ...
}
}
export const action: LoaderFunction = async () => {
- const baseUrl
- = process.env.NODE_ENV === 'development'
- ? 'http://localhost:8000'
- : 'https://axiom.cuhacking.ca'
+ const baseUrl = getBaseUrl()
return redirect(`${baseUrl}/api/users/oauth/linkedin`)
}
apps/portal-e2e/playwright.config.ts
Outdated
| @@ -8,11 +8,12 @@ import { defineConfig, devices } from '@playwright/test' | |||
|
|
|||
| const __filename = fileURLToPath(import.meta.url) | |||
|
|
|||
| const baseURL = process.env.CUHACKING_2025_PORTAL_LOCAL_URL | |||
| const baseURL = process.env.CI ? process.env.CUHACKING_2025_PORTAL_LOCAL_URL : process.env.CUHACKING_2025_PORTAL_PUBLIC_URL | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Environment-dependent baseURL
The baseURL is now conditionally set based on the CI environment, which is good for flexibility. However, there's no fallback value if the environment variables are undefined, which could cause tests to fail silently.
Add fallback values:
-const baseURL = process.env.CI ? process.env.CUHACKING_2025_PORTAL_LOCAL_URL : process.env.CUHACKING_2025_PORTAL_PUBLIC_URL
+const baseURL = process.env.CI
+ ? process.env.CUHACKING_2025_PORTAL_LOCAL_URL || 'http://localhost:5173'
+ : process.env.CUHACKING_2025_PORTAL_PUBLIC_URL || 'http://localhost:5173'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const baseURL = process.env.CI ? process.env.CUHACKING_2025_PORTAL_LOCAL_URL : process.env.CUHACKING_2025_PORTAL_PUBLIC_URL | |
| const baseURL = process.env.CI | |
| ? process.env.CUHACKING_2025_PORTAL_LOCAL_URL || 'http://localhost:5173' | |
| : process.env.CUHACKING_2025_PORTAL_PUBLIC_URL || 'http://localhost:5173' |
| <div onClick={() => handleCheckboxChange(`${value}2`)}> | ||
| <Typography variant="paragraph-base" className="text-left"> | ||
| {buttonContent2} | ||
| <span className="text-red-500">*</span> | ||
| </Typography> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Checkbox label needs keyboard accessibility
Similar to the previous issue, this click handler also lacks keyboard accessibility.
-<div onClick={() => handleCheckboxChange(`${value}2`)}>
+<div
+ onClick={() => handleCheckboxChange(`${value}2`)}
+ onKeyDown={(e) => {
+ if ((e.key === 'Enter' || e.key === ' ') && scrollEndReached[value]) {
+ e.preventDefault();
+ handleCheckboxChange(`${value}2`);
+ }
+ }}
+ tabIndex={scrollEndReached[value] ? 0 : -1}
+ role="button"
+ aria-disabled={!scrollEndReached[value]}
+>
<Typography variant="paragraph-base" className="text-left">
{buttonContent2}
<span className="text-red-500">*</span>
</Typography>
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div onClick={() => handleCheckboxChange(`${value}2`)}> | |
| <Typography variant="paragraph-base" className="text-left"> | |
| {buttonContent2} | |
| <span className="text-red-500">*</span> | |
| </Typography> | |
| </div> | |
| <div | |
| onClick={() => handleCheckboxChange(`${value}2`)} | |
| onKeyDown={(e) => { | |
| if ((e.key === 'Enter' || e.key === ' ') && scrollEndReached[value]) { | |
| e.preventDefault(); | |
| handleCheckboxChange(`${value}2`); | |
| } | |
| }} | |
| tabIndex={scrollEndReached[value] ? 0 : -1} | |
| role="button" | |
| aria-disabled={!scrollEndReached[value]} | |
| > | |
| <Typography variant="paragraph-base" className="text-left"> | |
| {buttonContent2} | |
| <span className="text-red-500">*</span> | |
| </Typography> | |
| </div> |
🧰 Tools
🪛 Biome (1.9.4)
[error] 139-139: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
|
|
||
| <div onClick={() => { | ||
| if (scrollEndReached[value]) { | ||
| handleCheckboxChange(`${value}1`) | ||
| } | ||
| }}> | ||
| <Typography variant="paragraph-base" className="text-left"> | ||
| {buttonContent} | ||
| <span className="text-red-500">*</span> | ||
| </Typography> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Added click handler to terms checkbox label, but missing keyboard accessibility
The added onClick handler improves usability by making the label clickable, but it lacks keyboard accessibility support which is flagged by the static analysis.
Add keyboard event handling:
-<div onClick={() => {
+<div
+ onClick={() => {
if (scrollEndReached[value]) {
handleCheckboxChange(`${value}1`)
}
- }}>
+ }}
+ onKeyDown={(e) => {
+ if ((e.key === 'Enter' || e.key === ' ') && scrollEndReached[value]) {
+ e.preventDefault();
+ handleCheckboxChange(`${value}1`);
+ }
+ }}
+ tabIndex={scrollEndReached[value] ? 0 : -1}
+ role="button"
+ aria-disabled={!scrollEndReached[value]}
+>
<Typography variant="paragraph-base" className="text-left">
{buttonContent}
<span className="text-red-500">*</span>
</Typography>
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div onClick={() => { | |
| if (scrollEndReached[value]) { | |
| handleCheckboxChange(`${value}1`) | |
| } | |
| }}> | |
| <Typography variant="paragraph-base" className="text-left"> | |
| {buttonContent} | |
| <span className="text-red-500">*</span> | |
| </Typography> | |
| </div> | |
| </div> | |
| <div | |
| onClick={() => { | |
| if (scrollEndReached[value]) { | |
| handleCheckboxChange(`${value}1`) | |
| } | |
| }} | |
| onKeyDown={(e) => { | |
| if ((e.key === 'Enter' || e.key === ' ') && scrollEndReached[value]) { | |
| e.preventDefault(); | |
| handleCheckboxChange(`${value}1`); | |
| } | |
| }} | |
| tabIndex={scrollEndReached[value] ? 0 : -1} | |
| role="button" | |
| aria-disabled={!scrollEndReached[value]} | |
| > | |
| <Typography variant="paragraph-base" className="text-left"> | |
| {buttonContent} | |
| <span className="text-red-500">*</span> | |
| </Typography> | |
| </div> | |
| </div> |
🧰 Tools
🪛 Biome (1.9.4)
[error] 116-120: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
apps/portal/app/routes/registration.tsx (1)
7-53: 🧹 Nitpick (assertive)Consider refactoring loader function for better maintainability.
The loader function is growing in complexity with multiple redirects and conditions. Consider extracting the user validation logic into a separate function for improved readability and maintainability.
// Example refactored approach function validateUserRegistrationState(user: any) { if (!user) { return { isValid: false, reason: 'not_authenticated' } } if (!user.agreedToTerms) { return { isValid: false, reason: 'terms_not_agreed' } } if (!user.emergencyContactFullName) { return { isValid: false, reason: 'missing_emergency_contact' } } return { isValid: true } } export const loader: LoaderFunction = async ({ request }) => { // Setup and fetch user as before try { // ... existing code to fetch user const validation = validateUserRegistrationState(user) if (!validation.isValid) { return redirect(`/?error=${validation.reason}`) } // Continue with form submissions check } catch { return redirect('/') } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/portal/app/routes/logout.tsx(1 hunks)apps/portal/app/routes/registration.tsx(2 hunks)libs/portal/ui/hackerdock/hackerdock.client.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
apps/portal/app/routes/registration.tsx (1)
17-18: Adding credentials:include enhances authentication flow.The addition of
credentials: 'include'is important to ensure cookies are sent with cross-origin requests. This complements the existing manual Cookie header assignment, working together to maintain proper authentication state.apps/portal/app/routes/logout.tsx (1)
5-10: Code structure change looks good.The updated loader function now correctly accepts the request parameter, allowing access to the cookie headers. The endpoint URL change from
/admin/logoutto/api/users/logoutaligns with API restructuring.
| if (!user.agreedToTerms) { | ||
| return redirect('/') | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
New user validation enhances registration flow.
This check ensures users complete all required steps in the correct order by redirecting those who haven't agreed to terms. Consider adding a query parameter to the redirect to inform the user why they're being redirected for a better user experience.
- return redirect('/')
+ return redirect('/?missingStep=terms')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!user.agreedToTerms) { | |
| return redirect('/') | |
| } | |
| if (!user.agreedToTerms) { | |
| return redirect('/?missingStep=terms') | |
| } |
| if (!user.emergencyContactFullName) { | ||
| return redirect('/') | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Appropriate validation for emergency contact.
This check properly ensures users provide emergency contact information before proceeding with registration. Similar to the above comment, consider adding context to the redirect for improved UX.
- return redirect('/')
+ return redirect('/?missingStep=emergencyContact')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!user.emergencyContactFullName) { | |
| return redirect('/') | |
| } | |
| if (!user.emergencyContactFullName) { | |
| return redirect('/?missingStep=emergencyContact') | |
| } |
| try { | ||
| const res = await fetch(adminURL, { | ||
| method: 'POST', | ||
| credentials: 'include', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Cookie': cookie || '', | ||
| }, | ||
| }) | ||
|
|
||
| if (!res.ok) { | ||
| console.error('ERROR - Could not log user out') | ||
| throw new Error('Failed to log out user') | ||
| } | ||
| } | ||
| catch (error) { | ||
| console.error('ERROR - An error occurred during logout', error) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Error handling implementation is good but consider improving error management.
The try-catch implementation for the fetch request is appropriate, but the current implementation swallows errors after logging them. Since you're not re-throwing after catching, the function will proceed to redirect regardless of whether the logout was successful.
This might be intentional (to ensure users are always redirected to login), but consider adding comments explaining this design decision.
apps/portal/app/routes/logout.tsx
Outdated
| const response = redirect('/login', { | ||
| headers: { | ||
| "Set-Cookie": | ||
| "payload-token=; path=/" | ||
| } | ||
| }) | ||
| return response |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Cookie clearing implementation could be more robust.
While the current implementation will clear the cookie, consider using a more complete cookie clearing approach:
const response = redirect('/login', {
headers: {
"Set-Cookie":
- "payload-token=; path=/"
+ "payload-token=; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT; HttpOnly; SameSite=Lax"
}
})This ensures better cross-browser compatibility and security by:
- Setting an expired date
- Adding HttpOnly flag to prevent JavaScript access
- Including SameSite attribute for CSRF protection
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = redirect('/login', { | |
| headers: { | |
| "Set-Cookie": | |
| "payload-token=; path=/" | |
| } | |
| }) | |
| return response | |
| const response = redirect('/login', { | |
| headers: { | |
| "Set-Cookie": | |
| "payload-token=; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT; HttpOnly; SameSite=Lax" | |
| } | |
| }) | |
| return response |
| label: 'Home', | ||
| disabled: false, | ||
| href: '/dashboard', | ||
| icon: <Logo link="/dashboard" />, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing redundant link prop in Logo component.
The Logo component already has a link prop pointing to /dashboard, but it's used within a Link component that already routes to /dashboard.
- icon: <Logo link="/dashboard" />,
+ icon: <Logo />,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| icon: <Logo link="/dashboard" />, | |
| icon: <Logo />, |
| className={cn( | ||
| '', | ||
| link.disabled && 'opacity-25 pointer-events-none cursor-not-allowed', | ||
| )} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove empty string className and consider using a more descriptive approach for disabled links.
The empty string in the className concatenation is unnecessary. Additionally, for better accessibility, consider using aria-disabled for disabled links.
className={cn(
- '',
link.disabled && 'opacity-25 pointer-events-none cursor-not-allowed',
)}
+ aria-disabled={link.disabled}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| className={cn( | |
| '', | |
| link.disabled && 'opacity-25 pointer-events-none cursor-not-allowed', | |
| )} | |
| className={cn( | |
| link.disabled && 'opacity-25 pointer-events-none cursor-not-allowed', | |
| )} | |
| aria-disabled={link.disabled} |
| ]; | ||
|
|
||
| return ( | ||
| <div className="fixed w-screen bottom-4 flex flex-col items-center justify-center z-10000"> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using more standard z-index values.
The z-index values z-10000 and z-10001 (line 83) are extremely high and could potentially cause conflicts with other UI elements. Consider using a more standard z-index scale.
- <div className="fixed w-screen bottom-4 flex flex-col items-center justify-center z-10000">
+ <div className="fixed w-screen bottom-4 flex flex-col items-center justify-center z-50">You should also update the z-index on line 83 accordingly:
- <TooltipContent className="z-10001">
+ <TooltipContent className="z-51">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="fixed w-screen bottom-4 flex flex-col items-center justify-center z-10000"> | |
| // Context from libs/portal/ui/hackerdock/hackerdock.client.tsx | |
| <div className="fixed w-screen bottom-4 flex flex-col items-center justify-center z-50"> | |
| {/* other content */} | |
| </div> | |
| {/* ...other parts of the component... */} | |
| <TooltipContent className="z-51"> | |
| {/* tooltip content */} | |
| </TooltipContent> | |
| {/* ...rest of the file... */} |
HasithDeAlwis
force-pushed
the
hasith/test/portal-e2e/333-implement-testrail
branch
from
6d14d2a to
4525463
Compare
libs/db/collections/models/Users.ts
Outdated
| auth: { | ||
| cookies: { | ||
| domain: process.env.NODE_ENV === 'development' ? 'localhost' : '.cuhacking.ca', | ||
| sameSite: 'None', | ||
| secure: true | ||
| }, | ||
| // cookies: { | ||
| // domain: process.env.NODE_ENV === 'development' ? 'localhost' : '.cuhacking.ca', | ||
| // sameSite: 'None', | ||
| // secure: true | ||
| // }, | ||
| disableLocalStrategy: { |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Clarify and Document the Cookie Settings Change
The cookie configuration has been commented out within the auth object. Please confirm that this change is intentional, and if so, add a brief comment explaining why these settings are disabled so future maintainers understand the context.
Clarify Cookie Configuration Reasoning
In libs/db/collections/models/Users.ts (lines 261-267), the cookie settings within the auth object are commented out. Please confirm that this change is intentional and add an inline comment explaining the rationale behind disabling these settings. This will help future maintainers understand the context.
HasithDeAlwis
force-pushed
the
hasith/test/portal-e2e/333-implement-testrail
branch
2 times, most recently
from
d522bb1 to
397db6f
Compare
HasithDeAlwis
force-pushed
the
hasith/test/portal-e2e/333-implement-testrail
branch
from
397db6f to
f89551c
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
libs/portal/features/legal/ui/terms.tsx (2)
115-126:⚠️ Potential issueMissing keyboard accessibility in checkbox label click handler.
The div with the onClick handler lacks keyboard event support, making it inaccessible to keyboard users.
Add keyboard event handling as previously suggested:
-<div onClick={() => { +<div + onClick={() => { if (scrollEndReached[value]) { handleCheckboxChange(`${value}1`) } - }}> + }} + onKeyDown={(e) => { + if ((e.key === 'Enter' || e.key === ' ') && scrollEndReached[value]) { + e.preventDefault(); + handleCheckboxChange(`${value}1`); + } + }} + tabIndex={scrollEndReached[value] ? 0 : -1} + role="button" + aria-disabled={!scrollEndReached[value]} +> <Typography variant="paragraph-base" className="text-left"> {buttonContent} <span className="text-red-500">*</span> </Typography> </div>🧰 Tools
🪛 Biome (1.9.4)
[error] 116-120: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
139-144:⚠️ Potential issueMissing keyboard accessibility in second checkbox label click handler.
This click handler also lacks keyboard accessibility, making it inaccessible to keyboard users.
Add keyboard event handling:
-<div onClick={() => handleCheckboxChange(`${value}2`)}> +<div + onClick={() => handleCheckboxChange(`${value}2`)} + onKeyDown={(e) => { + if ((e.key === 'Enter' || e.key === ' ') && scrollEndReached[value]) { + e.preventDefault(); + handleCheckboxChange(`${value}2`); + } + }} + tabIndex={scrollEndReached[value] ? 0 : -1} + role="button" + aria-disabled={!scrollEndReached[value]} +> <Typography variant="paragraph-base" className="text-left"> {buttonContent2} <span className="text-red-500">*</span> </Typography> </div>🧰 Tools
🪛 Biome (1.9.4)
[error] 139-139: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
apps/portal/app/routes/logout.tsx (2)
12-29: 🧹 Nitpick (assertive)Silent error handling during logout.
The try-catch implementation logs errors but doesn't prevent redirection to the login page, even if logout fails.
Add a comment explaining this design decision if intentional:
try { // ... existing code } catch (error) { console.error('ERROR - An error occurred during logout', error) + // Always redirect to login page even if logout fails + // This ensures users can't get stuck in a logged-in state with invalid credentials }
31-38: 🧹 Nitpick (assertive)Cookie clearing implementation could be more robust.
The current cookie clearing lacks security attributes like HttpOnly and SameSite.
Enhance cookie security:
const response = redirect('/login', { headers: { 'Set-Cookie': - `payload-token=; path=/; domain=${domain}`, + `payload-token=; path=/; domain=${domain}; expires=Thu, 01 Jan 1970 00:00:00 GMT; HttpOnly; SameSite=Lax`, }, })apps/portal-e2e/src/index.spec.ts (1)
87-89: 🧹 Nitpick (assertive)Add final result assertion in test
Suggest verifying a successful profile creation within the test itself for better clarity.
it('should be able to create profile', async ({ actor }) => { await createProfile({ actor }) + // e.g. verify final route, or confirm success message })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
apps/axiom/src/components/AfterLogin/index.tsx(1 hunks)apps/portal-e2e/playwright.config.ts(1 hunks)apps/portal-e2e/src/index.spec.ts(3 hunks)apps/portal-e2e/src/profile/hacker/abilities.ts(2 hunks)apps/portal-e2e/src/profile/hacker/index.spec.ts(1 hunks)apps/portal-e2e/src/registration/hacker/abilities.ts(1 hunks)apps/portal-e2e/src/registration/hacker/index.spec.ts(1 hunks)apps/portal-e2e/src/registration/questions.ts(2 hunks)apps/portal/app/routes/logout.tsx(1 hunks)apps/portal/app/routes/registration.tsx(2 hunks)libs/portal-e2e/abilities/index.ts(1 hunks)libs/portal-e2e/questions/index.ts(1 hunks)libs/portal/features/legal/ui/terms.tsx(2 hunks)libs/portal/features/login/ui/call-to-action.tsx(1 hunks)libs/portal/features/profile/hooks/use-profile-schema.ts(1 hunks)libs/portal/features/profile/ui/questions.tsx(2 hunks)libs/portal/features/registration/ui/registration.tsx(1 hunks)libs/portal/pages/profile/index.tsx(2 hunks)libs/portal/ui/dashboard/social-links/social-links.tsx(1 hunks)libs/portal/ui/hackerdock/hackerdock.client.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
libs/portal/features/legal/ui/terms.tsx
[error] 116-120: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 139-139: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (22.13.1)
- GitHub Check: test
🔇 Additional comments (31)
libs/portal/ui/dashboard/social-links/social-links.tsx (1)
32-32: Discord invite link updatedThe Discord invite link has been updated from the previous value to a new one. Ensure this is the correct new invite link for the community.
libs/portal/features/login/ui/call-to-action.tsx (2)
23-23: Method changed to post for form submissionChanging from "get" to "post" is appropriate for authentication flows. In Remix, this will trigger the action function in the nearest route. Verify your route has the proper action handler set up.
27-27: Good UX enhancement with cursor-pointerAdding the cursor-pointer class improves user experience by providing visual feedback that the button is interactive.
apps/axiom/src/components/AfterLogin/index.tsx (1)
29-46: Confirm if limiting to LinkedIn auth only is intentionalYou've commented out GitHub, Discord, and Google OAuth options, leaving only LinkedIn active. This aligns with the LinkedIn-only login form changes in the other file, but reduces authentication options for users.
Is this an intentional simplification of the auth flow for the TestRail implementation, or will these options be re-enabled in a future PR?
libs/portal/ui/hackerdock/hackerdock.client.tsx (6)
14-16: Add documentation to clarify what the status prop represents.The
statusboolean prop isn't self-explanatory. Add JSDoc comments to clarify what this status represents (e.g., whether it indicates user authentication, registration status, etc.).interface HackerDockProps { + /** Indicates whether the user is authenticated/registered */ status: boolean }
38-40: Improve handling of disabled links.Disabled links still have
hrefproperties that point to/, which is redundant. For disabled links, consider using#or removing the href attribute entirely.{ label: 'Sponsors', disabled: true, - href: '/', + href: '#', icon: <img className="size-6" src={handshake} alt="Team Icon" />, }, { label: 'Challenges', disabled: true, - href: '/', + href: '#', icon: <img className="size-6" src={mountain} alt="Challenges Icon" />, }, { label: 'Calendar', disabled: true, - href: '/', + href: '#', icon: <img className="size-6" src={map} alt="Map Icon" />, },Also applies to: 49-52, 55-58
46-46: Remove redundantlinkprop in Logo component.The Logo component has a
linkprop pointing to/dashboard, but it's already within a Link component that routes to/dashboard.- icon: <Logo link="/dashboard" />, + icon: <Logo />,
65-65: Use more standard z-index values.The z-index values
z-10000andz-10001are extremely high and could potentially cause conflicts with other UI elements.- <div className="fixed w-screen bottom-4 flex flex-col items-center justify-center z-10000"> + <div className="fixed w-screen bottom-4 flex flex-col items-center justify-center z-50"> - <TooltipContent className="z-10001"> + <TooltipContent className="z-51">Also applies to: 83-83
75-78: Improve className usage and accessibility for disabled links.The empty string in the className concatenation is unnecessary. For better accessibility, consider using
aria-disabledfor disabled links.className={cn( - '', link.disabled && 'opacity-25 pointer-events-none cursor-not-allowed', )} + aria-disabled={link.disabled}
65-65: Add data-testid attributes for E2E testing.Since this PR is focused on implementing TestRail for E2E tests, adding data-testid attributes would help with testing this component.
- <div className="fixed w-screen bottom-4 flex flex-col items-center justify-center z-10000"> + <div className="fixed w-screen bottom-4 flex flex-col items-center justify-center z-10000" data-testid="hacker-dock"> <Link to={link.href} aria-label={link.label} + data-testid={`dock-link-${link.label.toLowerCase()}`} className={cn(Also applies to: 72-79
libs/portal-e2e/questions/index.ts (1)
1-3: Looks good - adding login test utilityClean implementation of a test utility for selecting the login button. This will help standardize button selection across E2E tests.
libs/portal/features/profile/hooks/use-profile-schema.ts (1)
87-87: Error message period removed for consistencyThe removal of the period from the error message aligns with the formatting of other error messages in this file (like 'Required', 'Invalid email', 'Invalid URL').
libs/portal/features/profile/ui/questions.tsx (2)
222-222: Improved field label with format requirementsChanging "Resume Link" to "Resume Link (PDF Required)" enhances clarity about the expected file format. This provides better up-front guidance to users.
460-460: Enhanced button accessibility with proper cursor statesAdding cursor style based on disabled state improves the UI feedback. This properly indicates when interaction is not possible.
apps/portal-e2e/src/registration/hacker/index.spec.ts (1)
1-34: Action: Document Disabled Registration TestsThe entire test file is currently commented out, which creates a potential coverage gap for the registration flow. I couldn't locate any alternative test files within the repository to cover these scenarios, so please confirm whether these tests have been migrated to TestRail. If so, add a header comment in
apps/portal-e2e/src/registration/hacker/index.spec.tsthat details:
- Why these tests are disabled
- Where the equivalent test cases now reside in TestRail
- Whether these tests will be removed entirely in an upcoming PR
apps/portal/app/routes/registration.tsx (3)
17-18: Good addition of credentials for cross-origin authenticationThe
credentials: 'include'ensures cookies are properly sent with cross-origin requests, which is necessary for maintaining authentication state.
32-34: New user validation enhances registration flow.This check ensures users complete all required steps in the correct order by redirecting those who haven't agreed to terms. Consider adding a query parameter to the redirect to inform the user why they're being redirected for a better user experience.
- return redirect('/') + return redirect('/?missingStep=terms')
36-38: Appropriate validation for emergency contact.This check properly ensures users provide emergency contact information before proceeding with registration. Similar to the above comment, consider adding context to the redirect for improved UX.
- return redirect('/') + return redirect('/?missingStep=emergencyContact')libs/portal/features/registration/ui/registration.tsx (1)
109-111: Improved visual feedback for button statesThe dynamic className addition enhances UX by visually indicating when the form button is disabled. This aligns with the button's functional state controlled by the
disabledattribute.apps/portal-e2e/playwright.config.ts (2)
11-11: Inverted CI environment condition for baseURL.The condition has been inverted from the previous implementation. Before, it used LOCAL_URL in CI and PUBLIC_URL otherwise. Now it's using LOCAL_URL when NOT in CI.
Verify this is intentional. If accidental, correct the condition:
-const baseURL = !process.env.CI ? process.env.CUHACKING_2025_PORTAL_LOCAL_URL : process.env.CUHACKING_2025_PORTAL_PUBLIC_URL +const baseURL = process.env.CI ? process.env.CUHACKING_2025_PORTAL_PUBLIC_URL : process.env.CUHACKING_2025_PORTAL_LOCAL_URLAlso, add fallback URLs:
-const baseURL = !process.env.CI ? process.env.CUHACKING_2025_PORTAL_LOCAL_URL : process.env.CUHACKING_2025_PORTAL_PUBLIC_URL +const baseURL = !process.env.CI + ? process.env.CUHACKING_2025_PORTAL_LOCAL_URL || 'http://localhost:5173' + : process.env.CUHACKING_2025_PORTAL_PUBLIC_URL || 'https://portal.cuhacking.ca'
15-16: Test parallelization explicitly disabled.You've commented out dynamic parallelization based on CI and explicitly set
fullyParallel: false. This will prevent tests from running in parallel in all environments, which could slow down test execution.Is this intentional due to test interference issues? If so, consider adding a comment explaining why:
// fullyParallel: !process.env.CI, -fullyParallel: false, +// Disabling parallel test execution to prevent test interference issues with TestRail +fullyParallel: false,libs/portal/pages/profile/index.tsx (1)
6-40: Well-implemented toast notifications for form feedback.Good addition of toast feedback for loading, success, and error states. This improves user experience by providing clear status indicators during profile operations.
apps/portal-e2e/src/index.spec.ts (3)
5-7: Imports appear correctNo issues with the import references.
34-35: Confirm post-login redirect handlingAfter signing in, we navigate to
'/'and wait for an URL ending with/terms. Verify that the app indeed redirects from'/'to/termsand that no race conditions occur if the page load takes longer.
42-42: Potential redundancy in navigation
Navigate.to('/terms')might overlap with any existing redirect logic if the user is already directed to/termsafter logging in. Confirm that this additional navigation is intended.apps/portal-e2e/src/registration/hacker/abilities.ts (1)
3-5: No issues in importsThese references look correct.
apps/portal-e2e/src/profile/hacker/abilities.ts (3)
1-4: Imports are alignedNo concerns with the new imports or usage.
8-11: Ensure profile route is correct
startLoggedIn()followed byNavigate.to('/profile')appears valid. Confirm it aligns with your desired user flow.
66-66: Validate redirected pathNow the final route check ends with
/registrationinstead of/dashboard. Confirm that’s intentional.apps/portal-e2e/src/registration/questions.ts (2)
5-5: Improved aria labels for better accessibility.The updated aria labels are more descriptive and follow a consistent kebab-case pattern, which aligns with accessibility best practices. Make sure these match the exact labels used in the actual UI components for reliable test selectors.
Also applies to: 12-12, 19-19
28-28:✅ Verification successful
Verify field name change compatibility.
The text area name has been changed from 'adviceForFirstTimers' to 'first-time-hacker'. While this follows the kebab-case convention used in other changes, ensure this name matches exactly what's used in the form implementation to prevent test failures.
🏁 Script executed:
#!/bin/bash # Search for the corresponding form field in the codebase echo "Checking for first-time-hacker field usage in the codebase:" rg -l "first-time-hacker" --type tsx --type jsxLength of output: 204
Field Name Update: Verify Compatibility
The text area field now uses 'first-time-hacker' instead of 'adviceForFirstTimers' to follow the kebab-case convention. Please manually confirm that the form implementation is updated accordingly and that no stale references to the old field name remain, which could lead to test failures.
- Ensure the form component is using 'first-time-hacker'.
- Check for any lingering mentions of 'adviceForFirstTimers' and update them if necessary.
🏁 Script executed:
#!/bin/bash # Check usage of the "first-time-hacker" field across source files (ts, tsx, js, and jsx) echo "Checking for 'first-time-hacker' field usage across source files:" rg -l "first-time-hacker" --glob "*.{ts,tsx,js,jsx}"Length of output: 488
Field Name Compatibility Verified
The updated text area field now correctly uses 'first-time-hacker'. Our search confirms its usage across the codebase in:
libs/portal/features/registration/ui/registration.tsxlibs/portal/features/registration/hooks/use-registration-schema.tsapps/portal-e2e/src/registration/questions.tsPlease double-check that there are no remaining references to the old field name 'adviceForFirstTimers' elsewhere in the application to avoid potential test failures.
| // import { startLoggedIn } from '@cuhacking/portal-e2e/abilities' | ||
| // import { actorCalled } from '@serenity-js/core' | ||
| // import { beforeEach, describe, it } from '@serenity-js/playwright-test' | ||
| // import { BrowseTheWeb, Navigate } from '@serenity-js/web' | ||
| // import { createInvalidProfile, createProfile, createProfileNoSchool } from './abilities' | ||
|
|
||
| export function startLoggedInNoProfile() { | ||
| return Task.where( | ||
| '#actor starts logged out', | ||
| Navigate.to('/profile'), | ||
| ) | ||
| } | ||
| // describe('when the hacker', () => { | ||
| // beforeEach(async ({ actor }) => { | ||
| // actorCalled('Hacker') | ||
| // .whoCan(BrowseTheWeb.as(actor)) | ||
| // .attemptsTo(startLoggedIn(), Navigate.to('/profile')) | ||
| // }) | ||
|
|
||
| describe('when the hacker', () => { | ||
| beforeEach(async ({ actor }) => { | ||
| actorCalled('Hacker') | ||
| .whoCan(BrowseTheWeb.as(actor)) | ||
| .attemptsTo(startLoggedInNoProfile(), | ||
| ) | ||
| }) | ||
| describe('is logged in', () => { | ||
| describe('has agreed to all terms and conditions', () => { | ||
| // ============================== | ||
| // ========= NO PROFILE ========= | ||
| // ============================== | ||
| describe('has no profile', () => { | ||
| describe('is a student', () => { | ||
| it('should be able to create profile', async () => { | ||
| await createProfile() | ||
| }) | ||
| // describe('is logged in', () => { | ||
| // describe('has agreed to all terms and conditions', () => { | ||
| // // ============================== | ||
| // // ========= NO PROFILE ========= | ||
| // // ============================== | ||
| // describe('has no profile', () => { | ||
| // describe('is a student', () => { | ||
| // it('should be able to create profile', async () => { | ||
| // await createProfile() | ||
| // }) | ||
|
|
||
| it('should not be able to create profile when fields are invalid', async () => { | ||
| await createInvalidProfile() | ||
| }) | ||
| }) | ||
| describe('is not a student', () => { | ||
| it('should be able to create profile', async () => { | ||
| await createProfileNoSchool() | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| // ============================== | ||
| // ========= HAS PROFILE ========= | ||
| // ============================== | ||
| describe('has profile', () => { | ||
| describe('is a student', () => { | ||
| it('should be able to update profile', async () => { | ||
| // await updateProfile() | ||
| }) | ||
|
|
||
| it('should not be able to create profile when fields are invalid', async () => { | ||
| // updateInvalidProfile(), | ||
| }) | ||
| }) | ||
| describe('is not a student', () => { | ||
| it('should be able to create profile', async () => { | ||
| // await updateProfileNoSchool() | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
| // it('should not be able to create profile when fields are invalid', async () => { | ||
| // await createInvalidProfile() | ||
| // }) | ||
| // }) | ||
| // describe('is not a student', () => { | ||
| // it('should be able to create profile', async () => { | ||
| // await createProfileNoSchool() | ||
| // }) | ||
| // }) | ||
| // }) | ||
| // }) | ||
| // }) | ||
| // }) |
There was a problem hiding this comment.
Commented-out tests reduce test coverage
Removing these profile tests without replacement reduces the overall test coverage. Is this temporary or intentional? Consider using test.skip() instead of commenting out if these tests need to be temporarily disabled, or document the reason for removal.
If this is part of migrating to TestRail as mentioned in the PR objectives, ensure equivalent test coverage exists in the new system before removing these tests.
| const EMAIL_ADDRESS = process.env.LOCAL_DEV_EMAIL_ADDRESS || '' | ||
| const PASSWORD = process.env.LOCAL_DEV_PASSWORD || '' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Environment variables should have meaningful defaults
Empty string defaults for credentials could lead to silent test failures if environment variables aren't properly configured. Consider using placeholder values that would trigger obvious failures or throw an error if these values aren't provided.
- const EMAIL_ADDRESS = process.env.LOCAL_DEV_EMAIL_ADDRESS || ''
- const PASSWORD = process.env.LOCAL_DEV_PASSWORD || ''
+ const EMAIL_ADDRESS = process.env.LOCAL_DEV_EMAIL_ADDRESS || (() => { throw new Error('LOCAL_DEV_EMAIL_ADDRESS must be set') })()
+ const PASSWORD = process.env.LOCAL_DEV_PASSWORD || (() => { throw new Error('LOCAL_DEV_PASSWORD must be set') })()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const EMAIL_ADDRESS = process.env.LOCAL_DEV_EMAIL_ADDRESS || '' | |
| const PASSWORD = process.env.LOCAL_DEV_PASSWORD || '' | |
| const EMAIL_ADDRESS = process.env.LOCAL_DEV_EMAIL_ADDRESS || (() => { throw new Error('LOCAL_DEV_EMAIL_ADDRESS must be set') })() | |
| const PASSWORD = process.env.LOCAL_DEV_PASSWORD || (() => { throw new Error('LOCAL_DEV_PASSWORD must be set') })() |
| export function startLoggedIn() { | ||
| return Task.where( | ||
| '#actor starts logged in', | ||
| Navigate.to('/'), | ||
| Click.on(PageElement.located(By.cssContainingText('button', 'LOG IN'))), | ||
| Wait.upTo(Duration.ofSeconds(10)) | ||
| .until(PageElement.located(By.css('#username')), isVisible()), | ||
| Enter.theValue(EMAIL_ADDRESS) | ||
| .into(PageElement.located(By.css('#username'))), | ||
| Enter.theValue(PASSWORD) | ||
| .into(PageElement.located(By.css('#password'))), | ||
| Click.on(PageElement.located(By.cssContainingText('button', 'Sign in'))), | ||
| Wait.until(Page.current().url().href, matches(/\/(dashboard|terms|profile|registration)$/)), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Well-structured login automation for E2E tests
This login implementation is a good abstraction for reuse across test suites. A few suggestions for enhancement:
- Consider extracting selectors to constants at the top of the file for easier maintenance
- Add more descriptive step names for better test reporting
- Add error handling for common login failures
+ // Selectors for login elements
+ const LOGIN_BUTTON = By.cssContainingText('button', 'LOG IN')
+ const USERNAME_FIELD = By.css('#username')
+ const PASSWORD_FIELD = By.css('#password')
+ const SIGN_IN_BUTTON = By.cssContainingText('button', 'Sign in')
export function startLoggedIn() {
return Task.where(
- '#actor starts logged in',
+ '#actor logs in with credentials',
Navigate.to('/'),
- Click.on(PageElement.located(By.cssContainingText('button', 'LOG IN'))),
+ Click.on(PageElement.located(LOGIN_BUTTON)).describedAs('click login button'),
Wait.upTo(Duration.ofSeconds(10))
- .until(PageElement.located(By.css('#username')), isVisible()),
+ .until(PageElement.located(USERNAME_FIELD), isVisible()).describedAs('wait for username field'),
- Enter.theValue(EMAIL_ADDRESS)
- .into(PageElement.located(By.css('#username'))),
+ Enter.theValue(EMAIL_ADDRESS)
+ .into(PageElement.located(USERNAME_FIELD)).describedAs('enter email'),
- Enter.theValue(PASSWORD)
- .into(PageElement.located(By.css('#password'))),
+ Enter.theValue(PASSWORD)
+ .into(PageElement.located(PASSWORD_FIELD)).describedAs('enter password'),
- Click.on(PageElement.located(By.cssContainingText('button', 'Sign in'))),
+ Click.on(PageElement.located(SIGN_IN_BUTTON)).describedAs('click sign in'),
Wait.until(Page.current().url().href, matches(/\/(dashboard|terms|profile|registration)$/)),
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function startLoggedIn() { | |
| return Task.where( | |
| '#actor starts logged in', | |
| Navigate.to('/'), | |
| Click.on(PageElement.located(By.cssContainingText('button', 'LOG IN'))), | |
| Wait.upTo(Duration.ofSeconds(10)) | |
| .until(PageElement.located(By.css('#username')), isVisible()), | |
| Enter.theValue(EMAIL_ADDRESS) | |
| .into(PageElement.located(By.css('#username'))), | |
| Enter.theValue(PASSWORD) | |
| .into(PageElement.located(By.css('#password'))), | |
| Click.on(PageElement.located(By.cssContainingText('button', 'Sign in'))), | |
| Wait.until(Page.current().url().href, matches(/\/(dashboard|terms|profile|registration)$/)), | |
| ) | |
| } | |
| // Selectors for login elements | |
| const LOGIN_BUTTON = By.cssContainingText('button', 'LOG IN') | |
| const USERNAME_FIELD = By.css('#username') | |
| const PASSWORD_FIELD = By.css('#password') | |
| const SIGN_IN_BUTTON = By.cssContainingText('button', 'Sign in') | |
| export function startLoggedIn() { | |
| return Task.where( | |
| '#actor logs in with credentials', | |
| Navigate.to('/'), | |
| Click.on(PageElement.located(LOGIN_BUTTON)).describedAs('click login button'), | |
| Wait.upTo(Duration.ofSeconds(10)) | |
| .until(PageElement.located(USERNAME_FIELD), isVisible()).describedAs('wait for username field'), | |
| Enter.theValue(EMAIL_ADDRESS) | |
| .into(PageElement.located(USERNAME_FIELD)).describedAs('enter email'), | |
| Enter.theValue(PASSWORD) | |
| .into(PageElement.located(PASSWORD_FIELD)).describedAs('enter password'), | |
| Click.on(PageElement.located(SIGN_IN_BUTTON)).describedAs('click sign in'), | |
| Wait.until(Page.current().url().href, matches(/\/(dashboard|terms|profile|registration)$/)), | |
| ) | |
| } |
| By.xpath('//*[text()=\'I am at least 18 years of age.\']'), | ||
| ), | ||
| ), | ||
| Wait.until(PageElement.located(By.css('button[role="checkbox"]')), isVisible()), | ||
| Click.on( | ||
| PageElement.located( | ||
| By.css('button[role="checkbox"][aria-checked="false"]'), | ||
| By.xpath('//*[text()=\'I have read cuHacking Terms & Conditions\']'), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fragile text-based selectors
Using text-based XPath selectors can break if the message changes. Consider using data-test attributes or more stable locators.
| it('should be able to register for the hackathon', async ({ actor }) => { | ||
| await registerForHackathon({ actor }) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add assertion for hackathon registration
After registerForHackathon, confirm the user sees a success message or the expected route.
it('should be able to register for the hackathon', async ({ actor }) => {
await registerForHackathon({ actor })
+ // e.g. Ensure relevant element or page shows success
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should be able to register for the hackathon', async ({ actor }) => { | |
| await registerForHackathon({ actor }) | |
| }) | |
| it('should be able to register for the hackathon', async ({ actor }) => { | |
| await registerForHackathon({ actor }) | |
| // e.g. Ensure relevant element or page shows success | |
| }) |
| export async function registerForHackathon({ actor }) { | ||
| const words = `I would tell a first time hacker that they should be freeeeeee! | ||
| Like that one song about being free. | ||
| I\'m trying to get to 50 words right now haha. So how was your day?` | ||
| await actorCalled('Hacker').attemptsTo( | ||
| startLoggedIn(), | ||
| Navigate.to('/registration'), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the provided actor argument
Instead of actorCalled('Hacker'), reuse the function parameter { actor }. Keeps logic consistent with createProfile.
-export async function registerForHackathon({ actor }) {
- await actorCalled('Hacker').attemptsTo(
+export async function registerForHackathon({ actor }) {
+ await actor.attemptsTo(
startLoggedIn(),
Navigate.to('/registration'),
...
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function registerForHackathon({ actor }) { | |
| const words = `I would tell a first time hacker that they should be freeeeeee! | |
| Like that one song about being free. | |
| I\'m trying to get to 50 words right now haha. So how was your day?` | |
| await actorCalled('Hacker').attemptsTo( | |
| startLoggedIn(), | |
| Navigate.to('/registration'), | |
| export async function registerForHackathon({ actor }) { | |
| const words = `I would tell a first time hacker that they should be freeeeeee! | |
| Like that one song about being free. | |
| I'm trying to get to 50 words right now haha. So how was your day?` | |
| await actor.attemptsTo( | |
| startLoggedIn(), | |
| Navigate.to('/registration'), | |
| ... | |
| ) | |
| } |
Checklist
mainbefore creating this PR.Summary by CodeRabbit
New Features
Style