-
Notifications
You must be signed in to change notification settings - Fork 0
[PROD] - UTM & updates #396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| $ctx.auth = {...$ctx.auth, ready: true, user: authUser ?? undefined}; | ||
| }); | ||
| function onSignIn(signup?: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
The signup parameter is typed as any, which can lead to potential type-related issues. Consider using a more specific type, such as boolean | undefined, to improve type safety and maintainability.
| ].filter(Boolean).join('&') | ||
| // Append UTM parameters from cookie if they exist | ||
| signupUrl = appendUtmParamsToUrl(signupUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
Ensure that the appendUtmParamsToUrl function correctly handles URLs that already contain query parameters, to avoid malformed URLs. This is crucial for maintaining the correctness of the URL redirection.
| const params: UtmParams = {}; | ||
|
|
||
| try { | ||
| const searchParams = new URLSearchParams(window.location.search); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[design]
Using window.location directly ties this function to a browser environment, which may limit testing or reuse in non-browser contexts. Consider passing the URL as a parameter to increase flexibility.
| domain = getCookieDomain(), | ||
| path = COOKIE_PATH, | ||
| sameSite = COOKIE_SAMESITE, | ||
| secure = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[security]
The secure option is hardcoded to true, which means this code will not work over HTTP. Consider making this configurable or documenting that this function requires HTTPS.
| */ | ||
| export function getUtmCookie(): UtmParams | null { | ||
| try { | ||
| const cookies = document.cookie.split(';'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 correctness]
Splitting document.cookie by ; assumes that cookies do not contain semicolons, which is generally safe but not guaranteed. Consider using a more robust cookie parsing library if this becomes an issue.
| } | ||
|
|
||
| try { | ||
| const urlObj = new URL(url, window.location.origin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[design]
Using window.location.origin assumes the code is running in a browser environment. Consider passing the base URL as a parameter to make this function more versatile.
| * @param input - The string to sanitize | ||
| * @returns Sanitized string | ||
| */ | ||
| export declare function sanitize(input: string): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Consider specifying the behavior of the sanitize function when the input contains characters that are not A-Z, a-z, 0-9, hyphen (-), or underscore (_). This will improve the clarity of the function's contract.
| * Extracts UTM parameters from URL, sanitizes them, and persists to cookie | ||
| * Only sets the cookie if it doesn't already exist | ||
| */ | ||
| export declare function initializeUtmCookieHandler(): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The initializeUtmCookieHandler function should specify the expected behavior if the URL does not contain any UTM parameters. This will help ensure consistent handling of edge cases.
| * @param url - The base URL to append parameters to | ||
| * @returns URL with UTM parameters appended, or original URL if no cookie exists | ||
| */ | ||
| export declare function appendUtmParamsToUrl(url: string): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Clarify the behavior of appendUtmParamsToUrl when the url parameter already contains UTM parameters. This will help avoid potential conflicts or duplication of parameters.
PM-3267 - nasa referral
…a-referral Revert "PM-3267 - nasa referral"
…PM-3267_nasa-referral Revert "Revert "PM-3267 - nasa referral""
| url: getMarketingUrl('/talent'), | ||
| }, | ||
| referralProgram: { | ||
| label: 'Referral Programme', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 readability]
Consider using consistent spelling for 'Programme' as 'Program' to match the rest of the codebase and ensure uniformity across the application.
https://topcoder.atlassian.net/browse/PM-3201
https://topcoder.atlassian.net/browse/PM-3267