-
Notifications
You must be signed in to change notification settings - Fork 46
Add Cat Lovers App with React, Redux, Tailwind, and BFF Integration #34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…s added functionality to remove and add to favourites
…example files and README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Null Assertion Risks in User and Category Handling
Unsafe non-null assertions (!
) are used on catId
and userId
. catId
is an optional parameter, but catId!
is used when adding a favourite without a null check. userId
can be null from the user context, but userId!
is used when fetching favourites without a null check. This can lead to runtime errors if these variables are undefined
or null
.
src/hooks/useFavourites.ts#L46-L49
platform-react-challenge/src/hooks/useFavourites.ts
Lines 46 to 49 in 4edb84a
} else { | |
await dispatch(addFavourite({ image_id: catId!, sub_id: userId })).unwrap(); | |
} | |
dispatch(fetchFavourites(userId!)); |
Bug: Null Country Code Causes TypeError
Potential TypeError
when currentBreed.country_code.toUpperCase()
is called. This occurs if currentBreed
exists but currentBreed.country_code
is null
or undefined
. The current currentBreed && ...
check is insufficient as it only validates currentBreed
. Use optional chaining: currentBreed?.country_code?.toUpperCase()
.
src/components/BreedDetailsModal.tsx#L8-L9
platform-react-challenge/src/components/BreedDetailsModal.tsx
Lines 8 to 9 in 4edb84a
const { isModalOpen, currentBreed, currentBreedImages, handleClose } = useBreedDetails(); | |
const Code = currentBreed && currentBreed.country_code.toUpperCase(); |
Bug: Null Assertion Errors in User Functions
The handleRemove
function uses an unsafe non-null assertion on userId!
. This can cause a runtime error if userId
is null or undefined, for instance, if the user context is not initialized. A similar issue exists in the toggleFavourite
function.
src/hooks/useFavourites.ts#L58-L59
platform-react-challenge/src/hooks/useFavourites.ts
Lines 58 to 59 in 4edb84a
await dispatch(removeFavourite(favourite_id)).unwrap(); | |
dispatch(fetchFavourites(userId!)); |
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @melnikolaidis86 - Thanks a lot for your effort dude! :)
I've checked your assignment and I have some questions for you:
- Noticed that the same favourites network requests are performed multiple times in some cases (ie: in the cats list page). Any idea on why this is happening and how could you prevent it?
- Seems that you're using
headlessui
package to have a modal component ready for you. If you were to create one yourself, how would you approach it? - Are there any performance improvements that you've may thought on doing?
- If you had some more time, what would you have done differently?
Again thanks for your submission and looking forward to your answers!
"name": "catlover", | ||
"version": "0.1.0", | ||
"private": true, | ||
"dependencies": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed that you don't have any devDependencies
defined. Is this on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some devDependencies on package.json
"devDependencies": {
"@types/react-router-dom": "^5.3.3",
"autoprefixer": "^10.4.21",
"concurrently": "^9.2.0",
"cypress": "^14.5.2",
"postcss": "^8.5.6",
"tailwindcss": "^3.4.17"
}
However some of the them could have been moved to devDepenencies
"@testing-library/dom"
"@testing-library/jest-dom"
"@testing-library/react"
"@testing-library/user-event"
"@types/jest"
"@types/node"
"@types/react"
"@types/react-dom"
"react-scripts"
"typescript"
"web-vitals"
@@ -0,0 +1,2 @@ | |||
CAT_API_KEY= | |||
MAX_FAVOURITES=10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May you provide some more info about this env variable?
Why do we need a MAX_FAVOURITES
variable in the server
layer, in the app
layer?
Also: What's the rational behind this decision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason behind this is that there is a hard limit of 100 favorites when fetching favorites and if there are too many favorites I think I should have added pagination on this page too for better ux. However, on second thought I think it would be have been better if I had completely removed this env variable on server and added a hard limit of 100 in FE while checking if the added favorite exists on unique favorites list. Also I think it would have been better if I had added this list on local storage and invalidated when adding a new favorite.
Hey @gpositive - it was a pleasure doing this assignment.
|
This pull request adds a complete Cat Lovers web application built using modern React tooling and best practices. The project showcases several key frontend technologies and architectural patterns, including:
🔧 Tech Stack:
✨ Key Features:
This PR includes all the initial implementation for the core UI, routing, state handling, and styling. It serves as the foundation for future feature additions and scalability improvements.
Please review and let me know if any changes are required. 😊