01.13 - PROD RELEASE Re-enable nudges #400
Conversation
Signed-off-by: Vasilica Olariu <olariu.vasilica@gmail.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }, | ||
| }; | ||
| if (!DISABLE_NUDGES) { | ||
| const completednessData = await fetchUserProfileCompletedness(user, true); |
There was a problem hiding this comment.
[correctness]
The fetchUserProfileCompletedness function is called with user and true as arguments. Ensure that the second argument being true is intentional and correctly handled within the function, as it might affect the logic or performance if not properly managed.
| }; | ||
| if (!DISABLE_NUDGES) { | ||
| const completednessData = await fetchUserProfileCompletedness(user, true); | ||
| if (!completednessData) { |
There was a problem hiding this comment.
[maintainability]
Consider adding error handling for the fetchUserProfileCompletedness call. If it fails, the function will return early without updating the context, which might lead to inconsistent application state.
| $ctx.auth = { | ||
| ...$ctx.auth, | ||
| profileCompletionData: { | ||
| completed: completednessData.data?.percentComplete === 100, |
There was a problem hiding this comment.
[correctness]
The check completednessData.data?.percentComplete === 100 assumes that percentComplete is always a number. Ensure that percentComplete is validated or defaulted to a number to avoid potential issues with non-numeric values.
| @@ -0,0 +1,12 @@ | |||
| const modulePath = BUILD_IS_PROD ? './nudge.js' : '../nudge-app/Nudge.svelte'; | |||
There was a problem hiding this comment.
[❗❗ correctness]
The use of BUILD_IS_PROD suggests a reliance on a global variable or build-time constant. Ensure that BUILD_IS_PROD is defined and correctly set in all environments where this code will run to avoid unexpected behavior.
| return import(/* @vite-ignore */ modulePath).then(d => d.default) | ||
| } | ||
|
|
||
| export const loadNudgeApp = async (ctx: any, targetEl: Element): Promise<void> => { |
There was a problem hiding this comment.
[maintainability]
The use of any for the ctx parameter can lead to runtime errors if the context object does not have the expected structure. Consider defining a more specific type for ctx to improve type safety.
| const locationHostname = window?.location.hostname ?? '' | ||
| return DISABLE_NUDGES || | ||
| (!!NUDGES_DISABLED_HOSTS.find(host => ( | ||
| host.match(new RegExp(`^https?:\/\/${locationHostname}`, 'i')) |
There was a problem hiding this comment.
[correctness]
Using RegExp to match hostnames can be error-prone and may lead to unexpected matches. Consider using a more robust method to compare hostnames, such as direct string comparison or URL parsing.
| } | ||
|
|
||
|
|
||
| let resolve!: (value: ProfileCompletednessResponse) => void; |
There was a problem hiding this comment.
[correctness]
The resolve function is declared with a non-null assertion, which can lead to runtime errors if not handled correctly. Consider initializing resolve within the Promise constructor to ensure it's always defined.
| const requestUrl: string = `${TC_API_HOST}/members/${userHandle}/profileCompleteness${toastOverrideFlag}`; | ||
| const request = fetch(requestUrl, {headers: {...getRequestAuthHeaders()}}); | ||
|
|
||
| const response = await (await request).json(); |
There was a problem hiding this comment.
[❗❗ correctness]
The fetch call does not handle HTTP errors. Consider checking the response status and handling errors appropriately to avoid unexpected failures.
| ...response, | ||
| data: { | ||
| ...response.data, | ||
| percentComplete: (response?.data?.percentComplete ?? 0) * 100, |
There was a problem hiding this comment.
[correctness]
Multiplying percentComplete by 100 suggests it might be intended to be a percentage. Ensure that this transformation aligns with how percentComplete is used elsewhere in the application.
| height: 0; | ||
| width: 100%; | ||
| @include mobile { | ||
| --top-offset: 2px!important; |
There was a problem hiding this comment.
[maintainability]
Avoid using !important unless absolutely necessary, as it can make the CSS harder to maintain and override in the future. Consider restructuring the CSS to achieve the desired effect without !important.
|
|
||
| const ctx = getAppContext(); | ||
|
|
||
| $: ({ |
There was a problem hiding this comment.
[maintainability]
The use of $: for destructuring $ctx.auth is unconventional and might lead to confusion. Consider using a more explicit method to assign these values to improve readability and maintainability.
| hideToast(); | ||
| } | ||
|
|
||
| $: toast = isReady ? getToast(profileCompletionData as ProfileCompletionData) : undefined; |
There was a problem hiding this comment.
[❗❗ correctness]
Casting profileCompletionData to ProfileCompletionData without checking its actual type could lead to runtime errors if the assumption is incorrect. Consider adding a type guard or validation to ensure profileCompletionData is of the expected type.
| height: 100%; | ||
|
|
||
| svg { | ||
| transform: none!important; |
There was a problem hiding this comment.
[maintainability]
Avoid using !important as it can make the CSS harder to maintain and override. Consider refactoring the CSS to achieve the desired effect without !important.
| let coverRef: HTMLDivElement | undefined = undefined; | ||
| const dispatch = createEventDispatcher(); | ||
|
|
||
| function triggerLoaded(animation: any = false) { |
There was a problem hiding this comment.
[maintainability]
The animation parameter in triggerLoaded is typed as any. Consider using a more specific type to improve type safety and maintainability.
| dispatch('loaded', {animation: animation === true}); | ||
| } | ||
|
|
||
| const loadAnimation = (path) => { |
There was a problem hiding this comment.
[maintainability]
The path parameter in loadAnimation is not typed. Consider specifying a type for better type safety and maintainability.
| } | ||
|
|
||
| const loadAnimation = (path) => { | ||
| var animData = { |
There was a problem hiding this comment.
[💡 style]
Consider using const instead of var for animData to prevent potential issues with variable hoisting and to follow modern JavaScript practices.
| progressiveLoad: true | ||
| }, | ||
| }; | ||
| const bmAnim = window['bodymovin'].loadAnimation(animData); |
There was a problem hiding this comment.
[❗❗ correctness]
Accessing window['bodymovin'] directly can lead to runtime errors if bodymovin is not loaded. Consider adding a check to ensure bodymovin is available before calling loadAnimation.
| } | ||
|
|
||
| onMount(async () => { | ||
| await import(/* @vite-ignore */getPublicPath('/assets/nudge/bodymovin.js')); |
There was a problem hiding this comment.
[security]
The use of /* @vite-ignore */ suggests that the import path might be dynamic. Ensure that this dynamic import is safe and that the path is always valid to prevent runtime errors.
| pointer-events: none; | ||
| // move up and fade in when shown | ||
| transform: translate(0, -30px); | ||
| transition: transform 0.3s ease-in-out, opacity 0.18s ease-in-out 0.1s, visibility 0.01ms 0.3s; |
There was a problem hiding this comment.
[correctness]
The use of visibility 0.01ms in the transition is unconventional and might not work as intended across all browsers. Consider revising this to a more standard approach or removing it if not necessary.
| pointer-events: all; | ||
| // move down and fade out when hidden | ||
| transform: translate(0, 0); | ||
| transition: transform 0.3s ease-in-out, opacity 0.18s ease-in-out, visibility 0.01ms; |
There was a problem hiding this comment.
[correctness]
Similar to line 21, the visibility 0.01ms in the transition might not behave as expected. Consider using a more conventional timing or removing it.
| padding: 5px 15px; | ||
| border-radius: 25px; | ||
|
|
||
| background: tc-color(black, 100); |
There was a problem hiding this comment.
[correctness]
The tc-color(black, 100) function usage is unclear without context. Ensure that this function is defined and behaves as expected, especially in terms of color contrast for accessibility.
| transition: 0.15s ease-in-out; | ||
|
|
||
| &:hover { | ||
| background: tc-color(black, tc); |
There was a problem hiding this comment.
[correctness]
The tc-color(black, tc) function usage should be verified for correctness and accessibility, as the parameter tc is not defined in this context.
|
|
||
| function handleCloseKeydown(e: KeyboardEvent) { | ||
| const key = (e as KeyboardEvent).key; | ||
| if (key === 'Enter' || key === ' ' || key === 'Spacebar') { |
There was a problem hiding this comment.
[💡 readability]
The check for the 'Spacebar' key is redundant as it is deprecated and ' ' (space) should suffice. Consider removing 'Spacebar' for clarity.
| <div class={styles.message}>{toast.message}</div> | ||
|
|
||
| <a | ||
| href={toast.ctaLink(userhandle)} |
There was a problem hiding this comment.
[❗❗ security]
Ensure that toast.ctaLink(userhandle) returns a safe URL to prevent potential security vulnerabilities such as open redirects.
| * @returns | ||
| */ | ||
| export const getToast = (completednessData: ProfileCompletionData) => { | ||
| if (dismissNudgesBasedOnHost() || !completednessData || completednessData.completed || isToastDismissed()) { |
There was a problem hiding this comment.
[❗❗ correctness]
The condition !completednessData should be checked first to avoid potential null or undefined errors when accessing properties like completednessData.completed. Consider reordering the conditions for safety.
| } | ||
|
|
||
| const toastToShow = lastToastSeen || completednessData.showToast; | ||
| return toastsMeta[toastToShow]; |
There was a problem hiding this comment.
[correctness]
Accessing toastsMeta[toastToShow] without checking if toastToShow is a valid key could lead to undefined behavior if toastToShow is not present in toastsMeta. Consider adding a check to ensure toastToShow is a valid key before accessing toastsMeta.
| } | ||
|
|
||
| if (navType === 'tool' || navType === 'marketing') { | ||
| loadNudgeApp(ctx, targetEl.querySelector('.tc-universal-nav-wrap') as Element); |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that targetEl.querySelector('.tc-universal-nav-wrap') returns a valid element before passing it to loadNudgeApp. If it returns null, this will cause a runtime error. Consider adding a check to handle this case.
| @@ -0,0 +1 @@ | |||
| export default import('../lib/nudge-app/Nudge.svelte').then(d => d.default); | |||
There was a problem hiding this comment.
[performance]
Using dynamic import() can impact performance as it introduces a runtime overhead. Consider using static imports if the module is not conditionally loaded or if tree-shaking is a concern.
| @@ -0,0 +1 @@ | |||
| export declare const loadNudgeApp: (ctx: any, targetEl: Element) => Promise<void>; | |||
There was a problem hiding this comment.
[maintainability]
Using any for the ctx parameter can lead to potential runtime errors and makes the function harder to understand and maintain. Consider using a more specific type or an interface to define the expected structure of ctx.
| * Fetches the user profile completedness | ||
| * @returns Promise<ProfileCompletednessResponse> | ||
| */ | ||
| export declare const fetchUserProfileCompletedness: (user: AuthUser, force?: boolean) => Promise<ProfileCompletednessResponse | undefined>; |
There was a problem hiding this comment.
[correctness]
The return type Promise<ProfileCompletednessResponse | undefined> suggests that the function might return undefined. Consider clarifying under what conditions undefined is returned, as this can impact how the function is used and error-handling strategies.
| * @param completednessData | ||
| * @returns | ||
| */ | ||
| export declare const getToast: (completednessData: ProfileCompletionData) => import("lib/config/profile-toasts.config").ToastType | undefined; |
There was a problem hiding this comment.
[correctness]
The function getToast relies on ProfileCompletionData and returns a ToastType from profile-toasts.config. Ensure that the ToastType correctly handles all possible states of ProfileCompletionData, especially edge cases where data might be incomplete or malformed.
| @@ -0,0 +1,2 @@ | |||
| declare const _default: Promise<typeof import("svelte").SvelteComponent>; | |||
There was a problem hiding this comment.
[design]
Exporting a Promise of a component can lead to unexpected behavior if the consumer of this module does not handle the promise correctly. Consider exporting the component directly or providing a factory function that returns the promise to ensure proper usage.
Bring back and re-enable the profile nudges for members.