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
22 changes: 22 additions & 0 deletions .github/workflows/code_reviewer-complete-diff.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
name: AI PR Reviewer Updated

on:
pull_request:
types:
- opened
- synchronize
permissions:
pull-requests: write
jobs:
tc-ai-pr-review:
runs-on: ubuntu-latest
steps:
- name: Checkout Repo
uses: actions/checkout@v3

- name: TC AI PR Reviewer
uses: topcoder-platform/tc-ai-pr-reviewer@prompt-update
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)

Choose a reason for hiding this comment

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

The comment about the GITHUB_TOKEN is informative but could be misleading if it suggests that no action is needed. Consider rephrasing to clarify that the token is automatically provided by GitHub Actions and does not need to be manually added as a secret.

Choose a reason for hiding this comment

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

[💡 readability]
The comment about the GITHUB_TOKEN is informative but could be misleading. It suggests that adding the token as a secret will throw an error, which might not be accurate. Consider clarifying that the token is automatically provided by GitHub Actions and does not need to be manually added as a secret.

LAB45_API_KEY: ${{ secrets.LAB45_API_KEY }}
exclude: '**/*.json, **/*.md, **/*.jpg, **/*.png, **/*.jpeg, **/*.bmp, **/*.webp' # Optional: exclude patterns separated by commas

Choose a reason for hiding this comment

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

The exclude pattern is optional, but it might be beneficial to ensure that it aligns with the project's requirements. Double-check that the patterns listed here are indeed the ones you want to exclude from the AI PR review process.

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()

Choose a reason for hiding this comment

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

[💡 maintainability]
The destructuring of useNotification() into showBannerNotification and removeNotification is correct, but ensure that these functions are used within the component. If they are not used, consider removing the destructuring to avoid unnecessary code.

Choose a reason for hiding this comment

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

The destructuring of NotificationContextType seems unnecessary here. You can directly use const { showBannerNotification, removeNotification } = useNotification(); without specifying the type, as TypeScript can infer it.

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
const shouldShowChallengeMetaRow = Boolean(statusLabel) || trackTypePills.length > 0

useEffect(() => {
const notification = showBannerNotification({
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.

[❗❗ correctness]
The message string contains a grammatical error: 'AI Review Scores are not influence winner selection.' should be corrected to 'AI Review Scores do not influence winner selection.'

Choose a reason for hiding this comment

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

Typographical error 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)

Choose a reason for hiding this comment

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

[⚠️ maintainability]
Consider adding error handling for removeNotification(notification.id) to ensure that any issues during the removal process do not cause unintended side effects.

Choose a reason for hiding this comment

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

Consider checking if showBannerNotification is defined before calling it to prevent potential runtime errors.

}, [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)

Choose a reason for hiding this comment

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

[⚠️ correctness]
Consider adding error handling for cases where useProfileContext is called outside of a ProfileContext.Provider. This can help prevent runtime errors and improve developer experience by providing a clear error message.


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}>
{notifications.map(n => (
<Notification key={n.id} notification={n} onClose={removeNotification} />

Choose a reason for hiding this comment

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

[💡 readability]
Consider using a more descriptive name for the variable n in the map function to improve readability. For example, notification would make the code clearer.

Choose a reason for hiding this comment

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

Consider renaming the variable n to something more descriptive, such as notification, to improve code readability.

))}
</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'

Choose a reason for hiding this comment

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

[⚠️ correctness]
Using a default user ID of 'annon' might lead to potential issues if multiple anonymous users trigger notifications simultaneously. Consider using a more unique identifier for anonymous users to avoid ID collisions.

const [notifications, setNotifications] = useState<Notification[]>([])

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

const notify = useCallback(
(message: NotifyPayload, type: NotificationType = 'info', duration = 3000) => {
const id = `${uuid}[${typeof message === 'string' ? message : message.id}]`

Choose a reason for hiding this comment

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

The id generation logic here uses the message as part of the ID, which could lead to potential collisions if the same message is used multiple times. Consider using a more unique identifier, such as a timestamp or a UUID, to ensure uniqueness.

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)

Choose a reason for hiding this comment

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

[⚠️ performance]
The setTimeout function does not clear the timeout when the component unmounts, which could lead to memory leaks if the component is unmounted before the timeout completes. Consider storing the timeout ID and clearing it in a useEffect cleanup function.

Choose a reason for hiding this comment

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

The setTimeout function here does not clear the timeout if the component unmounts before the duration ends. This could lead to memory leaks. Consider storing the timeout ID and clearing it in a useEffect cleanup function.

}

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'

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.

[❗❗ correctness]
Consider handling potential exceptions from localStorage.getItem. If localStorage is unavailable or disabled, this could throw an error, impacting the application's stability.

)

export const dismiss = (id: string): void => {
localStorage.setItem(`${lsKeyPrefix}[${id}]`, JSON.stringify(true))

Choose a reason for hiding this comment

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

[❗❗ correctness]
Consider handling potential exceptions from localStorage.setItem. If localStorage is full or unavailable, this could throw an error, impacting the application's stability.

}
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;
message: string;
type: 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 improve type safety and prevent errors from unexpected values.

Choose a reason for hiding this comment

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

Consider using a union type for type to restrict it to known values, such as 'banner' | 'alert' | 'info', to improve type safety.

}
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}
/>
)
}

return <></>

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 (<></>) might not be the best approach if the notification type is not 'banner'. Consider handling other types or providing a fallback UI to improve clarity and maintainability.

Choose a reason for hiding this comment

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

Returning an empty fragment <> </> might not be the best approach for handling unsupported notification types. Consider logging a warning or handling other types explicitly.

}

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 instead of hardcoding #60267D. This improves maintainability by allowing easier updates and consistency across the application.

Choose a reason for hiding this comment

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

Consider using a variable for the background color instead of a hardcoded hex value to maintain consistency and ease of updates 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%;
border: 2px solid white;

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The border color white is hardcoded. Consider using a SCSS variable for consistency and easier updates.

Choose a reason for hiding this comment

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

Consider using a variable for the border color instead of hardcoding 'white'. This will help maintain consistency and make future updates easier.

@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.

Consider using control to define the type of control for the persistent prop in argTypes to enhance the Storybook UI experience.

Choose a reason for hiding this comment

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

[⚠️ correctness]
Consider adding a type annotation for defaultValue to ensure type safety and clarity. This can help prevent unintended type-related bugs.

description: 'Set to true to hide the close icon button',
},
},
component: NotificationBanner,
excludeStories: /.*Decorator$/,

Choose a reason for hiding this comment

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

[💡 maintainability]
The regex /.*Decorator$/ used in excludeStories might unintentionally exclude stories that do not follow the intended naming convention. Consider refining the regex to match specific patterns if necessary.

tags: ['autodocs'],
title: 'Components/NotificationBanner',
}

export default meta

type Story = StoryObj<typeof NotificationBanner>;

export const Primary: Story = {
args: {
content: 'Help tooltip',

Choose a reason for hiding this comment

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

The content argument is set to a generic string 'Help tooltip'. Consider providing a more descriptive or varied example to better demonstrate the component's capabilities.

},
}
Loading