fix: Screenshot Details Card show twice #3396
Conversation
|
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
apps/web/lib/features/activity/components/screenshoots-per-hour.tsxOops! Something went wrong! :( ESLint: 8.46.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx (1)
Line range hint
49-77: Apply the same fix to ScreenshootPerHourTeam component.The
ScreenshootPerHourTeamcomponent still renders multiple modals (one per iteration). Apply the same fix usingselectedElementstate to prevent duplicate modals.export const ScreenshootPerHourTeam = ({ timeSlots, startedAt, stoppedAt }: { timeSlots: ITimerSlot[]; startedAt: string; stoppedAt: string; }) => { const { isOpen, closeModal, openModal } = useModal(); + const [selectedElement, setSelectedElement] = React.useState<ITimerSlot | null>(null); + + const openScreenModal = useCallback((el: ITimerSlot) => { + setSelectedElement(el); + openModal(); + }, [openModal]); + return ( <div className="p-4 my-4 rounded-md dark:bg-[#1E2025] border-[0.125rem] bg-light--theme dark:border-[#FFFFFF0D]"> <h3 className="px-4"> @@ -65,7 +72,7 @@ startTime={el.startedAt} percent={el.percentage} imageUrl={el.screenshots[0]?.thumbUrl} - onShow={() => openModal()} + onShow={() => openScreenModal(el)} idSlot={el.id} isTeamPage /> - <ScreenshotDetailsModal open={isOpen} closeModal={closeModal} slot={el} /> </div> ))} + <ScreenshotDetailsModal open={isOpen} closeModal={closeModal} slot={selectedElement} /> </div> </div> );apps/web/lib/features/activity/components/screenshot-details.tsx (2)
27-28: Optimize optional chaining usage.When
slotis already checked, using optional chaining on its properties is redundant.-{slot ? new Date(slot?.startedAt).toLocaleTimeString() + '-' + new Date(slot?.stoppedAt).toLocaleTimeString(): null} +{slot ? new Date(slot.startedAt).toLocaleTimeString() + '-' + new Date(slot.stoppedAt).toLocaleTimeString() : null}
Line range hint
71-92: Improve stats and status display handling.The keyboard/mouse stats and status displays need better null handling to prevent displaying incomplete or incorrect information.
-{t('timer.TIMES')} : {slot?.keyboard} {slot?.keyboardPercentage}% +{t('timer.TIMES')} : {slot ? `${slot.keyboard} ${slot.keyboardPercentage}%` : '-'} -{t('timer.TIMES')} : {slot?.mouse} {slot?.mousePercentage}% +{t('timer.TIMES')} : {slot ? `${slot.mouse} ${slot.mousePercentage}%` : '-'} -{slot?.isActive ? ( +{slot ? ( slot.isActive ? ( <span className="bg-green-600 rounded-lg px-2 m-1">{t('timer.ACTIVE')}</span> ) : ( <span className="bg-red-600 rounded-lg px-2 m-1">{t('timer.INACTIVE')}</span> ) +) : null} -{slot?.isArchived ? ( +{slot ? ( slot.isArchived ? ( <span className="bg-gray-600 rounded-lg px-2 m-1">{t('timer.ARCHIVED')}</span> ) : ( <span className="bg-blue-600 rounded-lg px-2 m-1">{t('timer.NOT_ARCHIVED')}</span> ) +) : null}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx(3 hunks)apps/web/lib/features/activity/components/screenshot-details.tsx(4 hunks)
🔇 Additional comments (1)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx (1)
20-25: LGTM! Good fix for the duplicate modal issue.
The introduction of selectedElement state and openScreenModal callback effectively addresses the issue of duplicate Screenshot Details Cards by ensuring only one modal instance is rendered and properly managing its state.
apps/web/lib/features/activity/components/screenshot-details.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/web/lib/features/activity/components/screenshot-details.tsx (3)
27-27: Optimize the time display logicThe current implementation has redundant optional chaining after the null check. Consider extracting the time formatting logic to a reusable utility function.
-{slot ? new Date(slot?.startedAt).toLocaleTimeString() + '-' + new Date(slot?.stoppedAt).toLocaleTimeString(): null} +{slot ? `${new Date(slot.startedAt).toLocaleTimeString()} - ${new Date(slot.stoppedAt).toLocaleTimeString()}` : null}
Line range hint
71-92: Consider extracting status displays into separate componentsThe activity and archive status displays could be extracted into separate components to improve readability and maintainability. Also, consider simplifying the conditional rendering logic.
+const ActivityStatus = ({ isActive }: { isActive?: boolean }) => ( + <span className={`rounded-lg px-2 m-1 ${isActive ? 'bg-green-600' : 'bg-red-600'}`}> + {isActive ? t('timer.ACTIVE') : t('timer.INACTIVE')} + </span> +); +const ArchiveStatus = ({ isArchived }: { isArchived?: boolean }) => ( + <span className={`rounded-lg px-2 m-1 ${isArchived ? 'bg-gray-600' : 'bg-blue-600'}`}> + {isArchived ? t('timer.ARCHIVED') : t('timer.NOT_ARCHIVED')} + </span> +); // Replace the existing status displays with: <p className="rounded-lg px-1 mb-1 text-white"> <ActivityStatus isActive={slot?.isActive} /> </p> <p> <ArchiveStatus isArchived={slot?.isArchived} /> </p>
Potential issue with required props being passed as optional values
The verification reveals a mismatch between the component's interface requirements and its usage:
IScreenShootIteminterface shows thatidSlotandendTimeare required props (no?modifier)- These props are being passed with optional chaining (
slot?.id,slot?.stoppedAt)- The component uses these values directly in critical operations:
endTimeis used innew Date(endTime).toLocaleTimeString()idSlotis used indeleteTimeSlots([idSlot])Either:
- The interface should be updated to mark these props as optional
- Or the parent component should ensure these values are defined before rendering ScreenshotItem
🔗 Analysis chain
Line range hint
34-44: Verify ScreenshotItem handles undefined propsEnsure that the ScreenshotItem component properly handles cases where
idSlotandendTimeare undefined.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check ScreenshotItem component implementation ast-grep --pattern 'const ScreenshotItem = ({ idSlot, endTime $$$'Length of output: 68
Script:
#!/bin/bash # Let's try a different pattern to find the ScreenshotItem component ast-grep --pattern 'ScreenshotItem = $$$' # Also search for any file containing ScreenshotItem definition fd ScreenshotItem # As backup, let's use ripgrep to find the component definition rg "ScreenshotItem.*=.*(\{|props)" -A 10Length of output: 973
Script:
#!/bin/bash # Let's check the interface definition and any default value handling rg "interface IScreenShootItem" -A 10 # Let's also check the actual usage of these props in the component rg "const ScreenshotItem" -A 20 apps/web/lib/features/activity/components/screenshot-item.tsx # And check how these props are used within the component body rg "idSlot|endTime" apps/web/lib/features/activity/components/screenshot-item.tsxLength of output: 1697
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/lib/features/activity/components/screenshot-details.tsx(3 hunks)apps/web/lib/features/activity/components/screenshot-item.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/lib/features/activity/components/screenshot-item.tsx
🔇 Additional comments (3)
apps/web/lib/features/activity/components/screenshot-details.tsx (3)
23-23: LGTM: Responsive and dark mode styling
The modal styling changes appropriately handle both dark mode and responsive breakpoints.
29-31:
Handle undefined progress values
The progress bar and percentage display might show "undefined%" when slot is null. Add proper fallbacks for these values.
-<ProgressBar progress={slot?.percentage + '%'} width={'100%'} />
-<p className="font-semibold py-1">
- {slot?.percentage} {t('timer.PERCENT_OF_MINUTES')}
-</p>
+<ProgressBar progress={slot ? `${slot.percentage}%` : '0%'} width={'100%'} />
+{slot && (
+ <p className="font-semibold py-1">
+ {slot.percentage} {t('timer.PERCENT_OF_MINUTES')}
+ </p>
+)}Line range hint 8-16: Verify the optional slot prop handling across the codebase
The change to make slot optional is good for handling cases where no data is available. However, we should ensure all consumers of this component are aware of this breaking change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
apps/web/lib/features/activity/components/screenshot-details.tsx (4)
Line range hint
9-16: Add loading state UI for when slot is undefined.While making
slotoptional is good for handling loading states, consider adding a loading indicator or skeleton UI whenslotis undefined to improve user experience.const ScreenshotDetailsModal = ({ open, closeModal, slot }: { open: boolean; closeModal: () => void; slot?: ITimerSlot| null; }) => { const t = useTranslations(); + if (!slot) { + return ( + <Modal + isOpen={open} + title="Screenshots detail" + closeModal={closeModal} + className="bg-white dark:border-[#26272C] dark:bg-[#191a20] dark:border p-4 rounded-lg lg:w-[60vw] xl:w-[50vw] 2xl:w-[40vw]" + > + <div className="w-full p-4 animate-pulse"> + <div className="h-6 bg-gray-200 dark:bg-gray-700 rounded w-1/4 mb-4"></div> + <div className="h-4 bg-gray-200 dark:bg-gray-700 rounded w-full mb-4"></div> + <div className="h-48 bg-gray-200 dark:bg-gray-700 rounded w-full"></div> + </div> + </Modal> + ); + }
28-28: Simplify time display logic.The code uses both a null check and optional chaining, which is redundant. Since we're already checking if
slotexists, we can directly access its properties.-{slot ? new Date(slot?.startedAt).toLocaleTimeString() + '-' + new Date(slot?.stoppedAt).toLocaleTimeString(): null} +{slot ? `${new Date(slot.startedAt).toLocaleTimeString()} - ${new Date(slot.stoppedAt).toLocaleTimeString()}` : null}
Line range hint
72-94: Improve metrics display and extract status indicators.Consider these improvements:
- Use nullish coalescing for metrics display
- Extract status indicators into a reusable component
+const StatusIndicator = ({ isActive, label }: { isActive: boolean; label: string }) => ( + <span className={`rounded-lg px-2 m-1 ${isActive ? 'bg-green-600' : 'bg-red-600'}`}> + {label} + </span> +); -{t('timer.TIMES')} : {slot?.keyboard} {slot?.keyboardPercentage}% +{t('timer.TIMES')} : {slot?.keyboard ?? 0} {slot?.keyboardPercentage ?? 0}% -{slot?.isActive ? ( - <span className="bg-green-600 rounded-lg px-2 m-1">{t('timer.ACTIVE')}</span> -) : ( - <span className="bg-red-600 rounded-lg px-2 m-1">{t('timer.INACTIVE')}</span> -)} +<StatusIndicator + isActive={Boolean(slot?.isActive)} + label={t(slot?.isActive ? 'timer.ACTIVE' : 'timer.INACTIVE')} +/>
Potential issue with undefined idSlot in ScreenshotItem
The
IScreenShootIteminterface requiresidSlotas a non-optional string parameter, but it's being passed with optional chaining (slot?.id). This could lead to runtime errors in two scenarios:
- When deleting screenshots, as
deleteTimeSlots([idSlot])is called with potentially undefined value- The interface contract is violated when
undefinedis passed instead of the required stringSuggested fixes:
- Add a null check before rendering the
ScreenshotItemcomponent- Or make
idSlotoptional in the interface if undefined values should be handled🔗 Analysis chain
Line range hint
35-44: Verify ScreenshotItem handles undefined slot ID.The
idSlotprop is passed with optional chaining, but we should verify that theScreenshotItemcomponent can handle undefined values properly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if ScreenshotItem component handles undefined idSlot ast-grep --pattern 'interface Props { $$$ idSlot: $_ $$$ }' apps/web/lib/features/activity/components/screenshot-item.tsxLength of output: 129
Script:
#!/bin/bash # Let's first check if the file exists and see its content fd screenshot-item.tsx # If found, let's examine the component's props and implementation rg -A 5 "interface|type.*Props" --type typescriptLength of output: 173
Script:
#!/bin/bash # Let's examine the ScreenshotItem component implementation cat apps/web/lib/features/activity/components/screenshot-item.tsxLength of output: 3303
Script:
#!/bin/bash # Let's check the interface definition for IScreenShootItem cat apps/web/interfaces/IScreenshoot.ts 2>/dev/null || cat apps/web/lib/interfaces/IScreenshoot.ts 2>/dev/null || rg -l "IScreenShootItem"Length of output: 214
Script:
#!/bin/bash # Let's examine the interface definition cat apps/web/app/interfaces/IScreenshoot.tsLength of output: 360
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx(3 hunks)apps/web/lib/features/activity/components/screenshot-details.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/lib/features/activity/components/screenshoots-per-hour.tsx
🔇 Additional comments (2)
apps/web/lib/features/activity/components/screenshot-details.tsx (2)
30-32:
Safeguard percentage display.
While the progress bar now handles undefined values, the percentage display still needs protection against undefined values.
<ProgressBar progress={slot ? `${slot.percentage}%` : '0%'} width={'100%'} />
<p className="font-semibold py-1">
- {slot?.percentage} {t('timer.PERCENT_OF_MINUTES')}
+ {slot?.percentage ? `${slot.percentage} ${t('timer.PERCENT_OF_MINUTES')}` : '0 ' + t('timer.PERCENT_OF_MINUTES')}
</p>Likely invalid or redundant comment.
102-102: Verify memoization necessity.
While memoization can improve performance, it's only beneficial if the component receives stable props and renders frequently. Please verify that this is the case.
✅ Verification successful
Memoization is justified for this component
The memoization is appropriate here because:
- The component receives simple props (
open,closeModal, andslot) that are likely stable between renders - The component renders complex UI with multiple child components and data transformations
- It's used within a parent component that might re-render frequently due to user interactions
- The modal contains expensive operations like date formatting, percentage calculations, and mapping through screenshots
The implementation shows the component is used in a list rendering context (screenshoots-per-hour.tsx), where memoization can prevent unnecessary re-renders of sibling modal instances.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check component usage and prop changes
rg -A 5 "ScreenshotDetailsModal" --type tsx
Length of output: 71
Script:
#!/bin/bash
# Check component usage and prop changes with ripgrep, without file type restriction
rg -A 5 "ScreenshotDetailsModal"
Length of output: 2326
Script:
#!/bin/bash
# Find the file to analyze its props
fd "screenshot-details.tsx" --type f
Length of output: 101
Script:
#!/bin/bash
# If we find the file, let's examine its content to understand the props
cat apps/web/lib/features/activity/components/screenshot-details.tsx
Length of output: 3278
Description
Please include a summary of the changes and the related issue.
Type of Change
Checklist
Previous screenshots
Screen.Recording.2024-12-05.at.11.17.54.mov
Current screenshots
Screen.Recording.2024-12-05.at.12.37.38.mov
Summary by CodeRabbit
New Features
Bug Fixes
Style
These changes improve user experience by ensuring accurate display of screenshot details, enhancing robustness, and providing better visual consistency in dark mode.