Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/code_reviewer.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jobs:
uses: actions/checkout@v3

- name: TC AI PR Reviewer
uses: topcoder-platform/tc-ai-pr-reviewer@master
uses: topcoder-platform/tc-ai-pr-reviewer@prompt-v2
with:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # The GITHUB_TOKEN is there by default so you just need to keep it like it is and not necessarily need to add it as secret as it will throw an error. [More Details](https://docs.github.com/en/actions/security-guides/automatic-token-authentication#about-the-github_token-secret)
LAB45_API_KEY: ${{ secrets.LAB45_API_KEY }}
Expand Down
3 changes: 2 additions & 1 deletion src/apps/platform/src/PlatformApp.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { FC } from 'react'
import { toast, ToastContainer } from 'react-toastify'

import { useViewportUnitsFix } from '~/libs/shared'
import { NotificationsContainer, useViewportUnitsFix } from '~/libs/shared'

import { AppFooter } from './components/app-footer'
import { AppHeader } from './components/app-header'
Expand All @@ -14,6 +14,7 @@ const PlatformApp: FC<{}> = () => {
return (
<Providers>
<AppHeader />
<NotificationsContainer />
<div className='root-container'>
<PlatformRouter />
</div>
Expand Down
6 changes: 4 additions & 2 deletions src/apps/platform/src/providers/Providers.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { FC, ReactNode } from 'react'

import { authUrlLogout, ProfileProvider } from '~/libs/core'
import { ConfigContextProvider } from '~/libs/shared'
import { ConfigContextProvider, NotificationProvider } from '~/libs/shared'

import { PlatformRouterProvider } from './platform-router.provider'

Expand All @@ -13,7 +13,9 @@ const Providers: FC<ProvidersProps> = props => (
<ConfigContextProvider logoutUrl={authUrlLogout}>
<ProfileProvider>
<PlatformRouterProvider>
{props.children}
<NotificationProvider>
{props.children}
</NotificationProvider>
</PlatformRouterProvider>
</ProfileProvider>
</ConfigContextProvider>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { TableLoading } from '~/apps/admin/src/lib'
import { handleError } from '~/apps/admin/src/lib/utils'
import { EnvironmentConfig } from '~/config'
import { BaseModal, Button, InputCheckbox, InputText } from '~/libs/ui'
import { NotificationContextType, useNotification } from '~/libs/shared'

import {
useFetchScreeningReview,
Expand Down Expand Up @@ -226,6 +227,7 @@ const computePhaseCompletionFromScreenings = (

// eslint-disable-next-line complexity
export const ChallengeDetailsPage: FC<Props> = (props: Props) => {
const { showBannerNotification, removeNotification }: NotificationContextType = useNotification()
const [searchParams, setSearchParams] = useSearchParams()
const location = useLocation()
const navigate = useNavigate()
Expand Down Expand Up @@ -1323,6 +1325,16 @@ export const ChallengeDetailsPage: FC<Props> = (props: Props) => {
: undefined

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The useEffect hook that displays the banner notification does not include removeNotification in its dependency array. This could lead to stale closures if removeNotification changes. Consider adding removeNotification to the dependency array.

const shouldShowChallengeMetaRow = Boolean(statusLabel) || trackTypePills.length > 0

useEffect(() => {
const notification = showBannerNotification({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ correctness]
The message in showBannerNotification contains a typo: 'AI Review Scores are not influence winner selection.' should be 'AI Review Scores do not influence winner selection.'

id: 'ai-review-scores-warning',
message: `AI Review Scores are advisory only to provide immediate,
educational, and actionable feedback to members.
AI Review Scores are not influence winner selection.`,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message text contains a grammatical error. It should be 'AI Review Scores do not influence winner selection.' instead of 'AI Review Scores are not influence winner selection.'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ correctness]
Typo in the message: 'AI Review Scores are not influence winner selection.' should be 'AI Review Scores do not influence winner selection.'

})
return () => notification && removeNotification(notification.id)
}, [showBannerNotification])

return (
<PageWrapper
pageTitle={challengeInfo?.name ?? ''}
Expand Down
2 changes: 1 addition & 1 deletion src/libs/core/lib/profile/profile-context/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
export * from './profile-context-data.model'
export { default as profileContext, defaultProfileContextData } from './profile.context'
export { default as profileContext, defaultProfileContextData, useProfileContext } from './profile.context'
export * from './profile.context-provider'
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Context, createContext } from 'react'
import { Context, createContext, useContext } from 'react'

import { ProfileContextData } from './profile-context-data.model'

Expand All @@ -12,4 +12,6 @@ export const defaultProfileContextData: ProfileContextData = {

const profileContext: Context<ProfileContextData> = createContext(defaultProfileContextData)

export const useProfileContext = (): ProfileContextData => useContext(profileContext)

export default profileContext
1 change: 1 addition & 0 deletions src/libs/shared/lib/components/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ export * from './modals'
export * from './profile-picture'
export * from './input-skill-selector'
export * from './member-skill-editor'
export * from './notifications'
export * from './skill-pill'
export * from './expandable-list'
export * from './grouped-skills-ui'
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { FC } from 'react'

import { Notification } from '~/libs/ui'

import { NotificationContextType, useNotification } from './Notifications.context'
import styles from './NotificationsContainer.module.scss'

const NotificationsContainer: FC = () => {
const { notifications, removeNotification }: NotificationContextType = useNotification()

return (
<div className={styles.wrap}>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ maintainability]
Consider adding a type for the notifications array elements to ensure type safety and improve maintainability. This will help catch errors if the structure of notification objects changes in the future.

{notifications.map(n => (
<Notification key={n.id} notification={n} onClose={removeNotification} />
))}
</div>
)
}

export default NotificationsContainer
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import React, { createContext, ReactNode, useCallback, useContext, useMemo, useState } from 'react'

import { useProfileContext } from '~/libs/core'

import { dismiss, wasDismissed } from './localstorage.utils'

export type NotificationType = 'success' | 'error' | 'info' | 'warning' | 'banner';

export interface Notification {
id: string;
type: NotificationType;
icon?: ReactNode
message: string;
duration?: number; // in ms
}

type NotifyPayload = string | (Partial<Notification> & { message: string })

export interface NotificationContextType {
notifications: Notification[];
notify: (message: NotifyPayload, type?: NotificationType, duration?: number) => Notification | void;
showBannerNotification: (message: NotifyPayload) => Notification | void;
removeNotification: (id: string) => void;
}

const NotificationContext = createContext<NotificationContextType | undefined>(undefined)

export const useNotification = (): NotificationContextType => {
const context = useContext(NotificationContext)
if (!context) throw new Error('useNotification must be used within a NotificationProvider')
return context
}

export const NotificationProvider: React.FC<{
children: ReactNode,
}> = props => {
const profileCtx = useProfileContext()
const uuid = profileCtx.profile?.userId ?? 'annon'
const [notifications, setNotifications] = useState<Notification[]>([])

const removeNotification = useCallback((id: string, persist?: boolean) => {
setNotifications(prev => prev.filter(n => n.id !== id))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The id generation logic using uuid and message.id assumes that message.id is unique and defined when message is an object. Consider adding a check or fallback for cases where message.id might be undefined to avoid potential id collisions.

if (persist) {
dismiss(id)
}
}, [])

const notify = useCallback(
(message: NotifyPayload, type: NotificationType = 'info', duration = 3000) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ performance]
The setTimeout function used for removing notifications after a certain duration does not clear the timeout if the component unmounts before the timeout completes. This could lead to memory leaks. Consider storing the timeout ID and clearing it in a cleanup function.

const id = `${uuid}[${typeof message === 'string' ? message : message.id}]`
const newNotification: Notification
= typeof message === 'string'
? { duration, id, message, type }
: { duration, type, ...message, id }

if (wasDismissed(id)) {
return undefined
}

setNotifications(prev => [...prev, newNotification])

if (duration > 0) {
setTimeout(() => removeNotification(id), duration)
}

return newNotification
},
[uuid],
)

const showBannerNotification = useCallback((
message: NotifyPayload,
) => notify(message, 'banner', 0), [notify])

const ctxValue = useMemo(() => ({
notifications,
notify,
removeNotification,
showBannerNotification,
}), [
notifications,
notify,
removeNotification,
showBannerNotification,
])

return (
<NotificationContext.Provider value={ctxValue}>
{props.children}
</NotificationContext.Provider>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
@import "@libs/ui/styles/includes";

.wrap {
position: relative;
width: 100%;
z-index: 1000;
}
2 changes: 2 additions & 0 deletions src/libs/shared/lib/components/notifications/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { default as NotificationsContainer } from './Notifications.container'
export * from './Notifications.context'
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const lsKeyPrefix = 'notificationDismissed'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ maintainability]
Consider using a more descriptive prefix for lsKeyPrefix to avoid potential key collisions in localStorage. This will improve maintainability and reduce the risk of overwriting unrelated data.


export const wasDismissed = (id: string): boolean => (
(localStorage.getItem(`${lsKeyPrefix}[${id}]`)) !== null

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ design]
The use of localStorage directly ties this utility to the browser environment. If this utility is intended to be used in environments other than the browser, consider abstracting the storage mechanism to improve portability.

)

export const dismiss = (id: string): void => {
localStorage.setItem(`${lsKeyPrefix}[${id}]`, JSON.stringify(true))
}
1 change: 1 addition & 0 deletions src/libs/ui/lib/components/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ export * from './content-layout'
export * from './default-member-icon'
// NOTE: for some reason, modals needs to be imported prior to form
export * from './modals'
export * from './notification'
export * from './form'
export * from './loading-spinner'
export * from './page-divider'
Expand Down
33 changes: 33 additions & 0 deletions src/libs/ui/lib/components/notification/Notification.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { FC, ReactNode, useCallback } from 'react'

import { NotificationBanner } from './banner'

interface NotificationProps {
notification: {
icon?: ReactNode;
id: string;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
Consider using a union type for notification.type to restrict it to known values like 'banner'. This will help catch errors at compile time if an unsupported type is used.

message: string;
type: string;
}
onClose: (id: string, save?: boolean) => void
}

const Notification: FC<NotificationProps> = props => {
const handleClose = useCallback((save?: boolean) => {
props.onClose(props.notification.id, save)
}, [props.onClose, props.notification.id])

if (props.notification.type === 'banner') {
return (
<NotificationBanner
icon={props.notification.icon}
content={props.notification.message}
onClose={handleClose}
/>
)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ maintainability]
Returning an empty fragment (<></>) when the notification type is not 'banner' might not be the best approach. Consider handling other types explicitly or logging a warning for unsupported types to aid debugging.


return <></>
}

export default Notification
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
@import '../../../styles/includes';

.wrap {
background: #60267D;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ maintainability]
Consider using a CSS variable or a SCSS variable for the background color #60267D to improve maintainability and consistency across the project.

color: $tc-white;

font-family: "Nunito Sans", sans-serif;
font-size: 14px;
line-height: 20px;

.inner {
max-width: $xxl-min;
padding: $sp-2 0;
@include pagePaddings;
margin: 0 auto;
width: 100%;
display: flex;
justify-content: space-between;
align-items: center;
}
}

.close {
cursor: pointer;
color: $tc-white;
flex: 0 0;
margin-left: auto;
border-radius: 50%;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ maintainability]
The use of border: 2px solid white could be replaced with $tc-white to ensure consistency with the color scheme defined in your SCSS variables.

border: 2px solid white;
@include ltemd {
margin-left: $sp-3;
}
}

.icon {
flex: 0 0;
margin-right: $sp-2;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { Meta, StoryObj } from '@storybook/react'

import NotificationBanner from './NotificationBanner'

const meta: Meta<typeof NotificationBanner> = {
argTypes: {
content: {
description: 'Content displayed inside the notification banner',
},
persistent: {
defaultValue: false,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ maintainability]
Consider adding a default value for the content prop to ensure the component behaves predictably even if no content is provided. This can improve maintainability and prevent potential runtime errors.

description: 'Set to true to hide the close icon button',
},
},
component: NotificationBanner,
excludeStories: /.*Decorator$/,
tags: ['autodocs'],
title: 'Components/NotificationBanner',
}

export default meta

type Story = StoryObj<typeof NotificationBanner>;

export const Primary: Story = {
args: {
content: 'Help tooltip',
},
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { FC, ReactNode, useCallback } from 'react'

import { InformationCircleIcon } from '@heroicons/react/outline'

import { IconOutline } from '../../svgs'

import styles from './NotificationBanner.module.scss'

interface NotificationBannerProps {
persistent?: boolean
content: ReactNode
icon?: ReactNode
onClose?: (save?: boolean) => void
}

const NotificationBanner: FC<NotificationBannerProps> = props => {
const handleClose = useCallback(() => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 performance]
Using useCallback here is unnecessary unless handleClose is passed to a child component that relies on referential equality for optimization. Consider removing it to simplify the code.

props.onClose?.(true)
}, [props.onClose])

return (
<div className={styles.wrap}>
<div className={styles.inner}>
{props.icon || (
<div className={styles.icon}>
<InformationCircleIcon className='icon-xl' />
</div>
)}

{props.content}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ design]
Using onClick on a div for the close button may not be accessible for keyboard users. Consider using a button element instead to improve accessibility.

{!props.persistent && (
<div className={styles.close} onClick={handleClose}>
<IconOutline.XIcon className='icon-lg' />
</div>
)}
</div>
</div>
)
}

export default NotificationBanner
1 change: 1 addition & 0 deletions src/libs/ui/lib/components/notification/banner/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default as NotificationBanner } from './NotificationBanner'
2 changes: 2 additions & 0 deletions src/libs/ui/lib/components/notification/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from './banner'
export { default as Notification } from './Notification'