[Bug]: Broken Screenshot Images in Screenshots Tab - Issue(#3739) #3751
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request refactors and enhances the screenshot components. The primary component is renamed from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TeamPage as ScreenshotPerHourTeam
participant Component as ScreenshotPerHour
participant Item as ScreenshotItem
User->>TeamPage: Render team page
TeamPage->>Component: Call with isTeamPage=true
Component->>Component: Execute useEffect logging on timeSlots change
Component->>Item: Render screenshot items with conditional layout
Poem
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. Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🪧 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 (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR addresses broken screenshots by adding fallback image handling and refactoring duplicate code in screenshot components for better maintainability.
- Added fallback image handling in
apps/web/lib/features/activity/components/screenshot-item.tsxwith onError handler to display a placeholder when images fail to load - Refactored
apps/web/lib/features/activity/components/screenshoots-per-hour.tsxto merge team and regular screenshot components using anisTeamPageprop - Standardized naming from "Screenshoot" to "Screenshot" across components for consistency
- Added debug logging in screenshot components to help troubleshoot loading issues
- Improved conditional class assignments for better code organization and readability
3 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/web/lib/features/activity/screenshoots.tsx (1)
9-82: Consider consistent naming throughout the codebaseThe component name has been corrected from "Screenshoot" to "Screenshot", but the file name (
screenshoots.tsx) and some component names (ScreenshootTab,ScreenshootTeamTab,ScreenshootSkeleton) still use the old spelling.For complete consistency, consider updating the remaining occurrences of "Screenshoot" in file names and component names.
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx (3)
32-37: Remove debugging console logs before productionDebug console logs were added to trace time slots and screenshot data. While useful during development, these should be removed before merging to production.
-useEffect(() => { - console.log('Time slots received:', timeSlots); - if (timeSlots && timeSlots.length > 0) { - console.log('First slot screenshots:', timeSlots[0]?.screenshots); - } -}, [timeSlots]);
63-63: Use optional chaining for better code qualityConsider using optional chaining for a more concise syntax when accessing nested properties.
-imageUrl={(el.screenshots && el.screenshots[0]?.thumbUrl) || '/assets/jpeg/placeholder-image.jpeg'} +imageUrl={el.screenshots?.[0]?.thumbUrl || '/assets/jpeg/placeholder-image.jpeg'}Also applies to: 79-79
🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
1-95: Consider renaming the file to match component name spellingThe component name has been corrected from "Screenshoot" to "Screenshot", but the file name still uses the old spelling (
screenshoots-per-hour.tsx).For complete consistency, consider updating the file name to
screenshots-per-hour.tsx.🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 79-79: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/web/public/assets/jpeg/placeholder-image.jpegis excluded by!**/*.jpeg
📒 Files selected for processing (3)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx(2 hunks)apps/web/lib/features/activity/components/screenshot-item.tsx(4 hunks)apps/web/lib/features/activity/screenshoots.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/web/lib/features/activity/screenshoots.tsx (1)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx (2)
ScreenshotPerHour(10-90)ScreenshotPerHourTeam(92-94)
apps/web/lib/features/activity/components/screenshot-item.tsx (1)
apps/web/lib/components/progress-bar.tsx (1)
ProgressBar(3-42)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx (2)
apps/web/app/interfaces/timer/ITimerSlot.ts (1)
ITimerSlot(4-26)apps/server-web/src/renderer/libs/utils/clsxm.ts (1)
clsxm(5-7)
🪛 Biome (1.9.4)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx
[error] 63-63: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 79-79: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: deploy
🔇 Additional comments (12)
apps/web/lib/features/activity/screenshoots.tsx (2)
2-2: Components renamed for better consistencyThe import statement has been updated to use the correctly spelled component names
ScreenshotPerHourandScreenshotPerHourTeam(instead of "Screenshoot").
43-43: Component usage updated to match renamed componentsThe component references have been correctly updated to match the renamed components.
Also applies to: 67-67
apps/web/lib/features/activity/components/screenshot-item.tsx (5)
39-47: Improved image error handling with fallbackGood job adding error handling for image loading failures:
- The src attribute now includes a fallback path for when imageUrl is empty
- The onError handler logs errors and sets the fallback image when loading fails
This enhancement improves the user experience by ensuring something is always displayed.
50-56: Styling and layout improvementsThe layout structure and class organization have been improved for better readability and maintainability.
59-69: Consistent button stylingThe view buttons container has been updated with more consistent class ordering and spacing.
98-98: Removed extra whitespace in ProgressBar classNameExtra whitespace has been removed from the ProgressBar className.
110-111: Improved modal content layoutThe delete confirmation modal layout has been improved with better spacing and alignment.
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx (5)
8-8: Added missing importsThe
useCallbackanduseEffectimports have been added to support the new functionality.
10-20: Component renamed and enhanced with isTeamPage propThe component has been renamed from
ScreenshootPerHourtoScreenshotPerHourfor correct spelling, and an optionalisTeamPageboolean prop has been added to support different rendering scenarios.
39-45: Good refactoring of class names for reusabilityExtracting the class names into variables based on the
isTeamPageprop improves code readability and maintainability. This is a good refactoring approach.
53-85: Refactored rendering logic with conditional displayThe rendering logic has been effectively refactored to conditionally display content based on the
isTeamPageprop. This eliminates duplicate code and improves maintainability.The fallback image path (
'/assets/jpeg/placeholder-image.jpeg') is now consistently used when screenshots are unavailable.🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 79-79: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
92-94: Good implementation of the wrapper componentThe new
ScreenshotPerHourTeamcomponent effectively wrapsScreenshotPerHourwith theisTeamPageprop set to true. This is a clean pattern that promotes code reuse.
AnicetFantomas
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx (1)
56-56: Duplicate key warning from previous review.The key prop is used on both the parent div and the child component. Using it only on the parent div is sufficient.
🧹 Nitpick comments (3)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx (3)
32-37: Remove debugging console logs before production.The useEffect hook includes console log statements that appear to be for debugging purposes. These should be removed before merging to production.
-useEffect(() => { - console.log('Time slots received:', timeSlots); - if (timeSlots && timeSlots.length > 0) { - console.log('First slot screenshots:', timeSlots[0]?.screenshots); - } -}, [timeSlots]);
53-54: Consider using item IDs as keys instead of array indices.Using array indices as keys in React lists can cause issues when items are reordered, added, or removed. Since each
elhas anidproperty, consider using that as the key for better performance and reliability.-{timeSlots.map((el, i) => { +{timeSlots.map((el) => { if (isTeamPage) { return ( - <div key={i} className={clsxm('min-w-[15rem] xl:w-1/6 p-4')}> + <div key={el.id} className={clsxm('min-w-[15rem] xl:w-1/6 p-4')}> <ScreenshotItem /* ... props ... */ /> </div> ); } return ( <ScreenshotItem - key={i} + key={el.id} /* ... props ... */ /> ); })}Also applies to: 72-73, 83-84
62-63: Optimize optional chaining in image URL handling.The static analysis is suggesting to improve the optional chaining in the image URL expressions.
-imageUrl={(el.screenshots && el.screenshots[0]?.thumbUrl) || '/assets/jpeg/placeholder-image.jpeg'} +imageUrl={el.screenshots?.[0]?.thumbUrl || '/assets/jpeg/placeholder-image.jpeg'}Also applies to: 78-78
🧰 Tools
🪛 Biome (1.9.4)
[error] 62-62: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx (2)
apps/web/app/interfaces/timer/ITimerSlot.ts (1)
ITimerSlot(4-26)apps/server-web/src/renderer/libs/utils/clsxm.ts (1)
clsxm(5-7)
🪛 Biome (1.9.4)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx
[error] 62-62: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: deploy
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
apps/web/lib/features/activity/components/screenshoots-per-hour.tsx (5)
8-8: Good addition of useEffect import.You've properly added the required React hooks to support the new functionality.
10-14: Component name correction and props enhancement.The component name correction from "Screenshoot" to "Screenshot" improves consistency. The addition of the optional
isTeamPageprop with a default value follows React best practices for component interfaces.Also applies to: 19-19
39-45: Well-structured conditional styling.Creating separate variables for container, heading, and content classes based on the
isTeamPageprop is a clean approach to conditional styling. This makes the JSX more readable and maintainable.
48-52: Clean implementation of conditional classes.The usage of conditional class variables in the component structure makes the code more maintainable and readable.
91-93: Good component composition with ScreenshotPerHourTeam.Creating a wrapper component that sets
isTeamPage={true}is a clean way to provide a specialized component for team pages while reusing the existing functionality.
Innocent-Akim
dismissed
their stale review
Hello,
I’m withdrawing my review request for now, as some changes are still in progress.
| }) => { | ||
| const { isOpen, closeModal, openModal } = useModal(); | ||
| useEffect(() => { | ||
| console.log('Time slots received:', timeSlots); |
There was a problem hiding this comment.
@AnicetFantomas please review each line of your PR when you created it and make sure you clean up those console.log and other things. I really suggest to literally ALWAYS review EACH line, it helps a LOT, thanks!
| const { isOpen, closeModal, openModal } = useModal(); | ||
| useEffect(() => { | ||
| if (timeSlots && timeSlots.length > 0) { | ||
| console.log('First slot screenshots:', timeSlots[0]?.screenshots); |
There was a problem hiding this comment.
@AnicetFantomas do we need this whole useEffect() (except for temp debugging)? Can we remove it? If yes, do in another PR based on develop branch as I will merge this PR with Squash.
|
@Innocent-Akim / @Cedric921 / @NdekoCode when do review (not only in this PR, but in general), please make sure you review EACH line of code and note those console.log and other problematic things etc. |
Description
Type of Change
Checklist
Previous screenshots
Current screenshots
Please add here videos or images of the current (new) status
Summary by CodeRabbit
New Features
Bug Fixes
Enhancements