Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the full "React Phone Catalog" application from scratch — transforming a placeholder app into a complete e-commerce product catalog. It includes product listing with sort/filter/pagination, product detail pages with image gallery and color/capacity selectors, a cart using useReducer, a favorites system, a dark/light theme toggle, and responsive design across all breakpoints.
Changes:
- Full React application implementation: all pages (Home, Phones, Tablets, Accessories, Product Details, Cart, Favorites, Not Found), components (Header, Footer, ProductCard, Sliders, Pagination, etc.) and React Contexts (Cart, Favorites, Theme)
- Switch to
HashRouterfor GitHub Pages compatibility, and update of routing and global styles - README completely rewritten in Portuguese with comprehensive project documentation
Reviewed changes
Copilot reviewed 74 out of 94 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/App.tsx |
Wires up all providers, routes, and layout shell |
src/index.tsx |
Wraps app in HashRouter for GitHub Pages deployment |
src/utils/api.ts |
Fetch helpers for all JSON endpoints with 300ms simulated delay |
src/types/*.ts |
TypeScript interfaces for Product, ProductDetails, and CartItem |
src/context/*.tsx |
Cart (useReducer), Favorites (useState), and Theme contexts with localStorage persistence |
src/modules/** |
All page components (Home, Phones, Tablets, Accessories, ProductDetails, Cart, Favorites, NotFound) |
src/components/** |
All reusable UI components (Header, Footer, ProductCard, Sliders, Pagination, Dropdown, etc.) |
src/styles/*.scss |
Global design tokens, CSS custom properties for theming, and utility classes |
public/api/*.json |
Static JSON data files (formatting only changes to arrays) |
package.json |
Adds i18next/react-i18next (unused) and bumps @mate-academy/scripts |
index.html |
Updates title and adds meta description |
README.md |
Fully rewritten project documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const getSuggestedProducts = async () => { | ||
| const products = await getProducts(); | ||
|
|
||
| return [...products].sort(() => Math.random() - 0.5); |
There was a problem hiding this comment.
The getSuggestedProducts function uses Math.random() - 0.5 as a sort comparator. This approach is not a reliable shuffle algorithm — the Fisher-Yates (Knuth) shuffle is the correct algorithm for unbiased random ordering. The sort comparator based on Math.random() produces biased results because many JS engines use unstable sorts in combination with inconsistent comparator calls, leading to non-uniform distributions. While this is a low-stakes scenario (suggested products), it is still a known anti-pattern.
| <Breadcrumbs items={[{ label: title }]} /> | ||
|
|
||
| <h1 className="page__title">{title}</h1> | ||
| <p className="page__subtitle">{filtered.length} models</p> |
There was a problem hiding this comment.
The ProductsPage component shows the subtitle as {filtered.length} models (line 101), which reflects the count of products matching the current search query. However, this count is shown immediately — even while the data is still loading (isLoading is true). During loading, filtered.length will be 0, so the user will briefly see "0 models" before the actual count appears. This should only be rendered after loading completes, or use the total products count rather than the filtered one for the subtitle.
| Promise.all([ | ||
| getProductDetails(productId), | ||
| getProducts(), | ||
| getSuggestedProducts(), | ||
| ]) | ||
| .then(([det, allProducts, sug]) => { | ||
| setDetails(det); | ||
|
|
||
| const found = allProducts.find(p => p.itemId === productId) || null; | ||
|
|
||
| setProduct(found); | ||
| setSuggested(sug.slice(0, 12)); | ||
| }) | ||
| .finally(() => setIsLoading(false)); |
There was a problem hiding this comment.
The ProductDetailsPage does not handle fetch errors. The Promise.all call has no .catch() handler, so if any API call fails (e.g. network error), the promise will reject and isLoading will remain true forever (since .finally() is present only in the missing path). Actually finally IS present — so isLoading will correctly be set to false. But the error is silently swallowed and the user will see "Product was not found" with no indication of what actually happened. Compare this to ProductsPage and HomePage, which both have explicit isError state with a reload button. The missing error handling means a network failure is indistinguishable from a genuinely missing product.
| useEffect(() => { | ||
| setIsLoading(true); | ||
| setSelectedImage(0); | ||
|
|
||
| Promise.all([ | ||
| getProductDetails(productId), | ||
| getProducts(), | ||
| getSuggestedProducts(), | ||
| ]) | ||
| .then(([det, allProducts, sug]) => { | ||
| setDetails(det); | ||
|
|
||
| const found = allProducts.find(p => p.itemId === productId) || null; | ||
|
|
||
| setProduct(found); | ||
| setSuggested(sug.slice(0, 12)); | ||
| }) | ||
| .finally(() => setIsLoading(false)); | ||
| }, [productId]); |
There was a problem hiding this comment.
In ProductDetailsPage, when the product details page is loaded, three separate API calls are made independently: getProductDetails(productId) internally calls getPhones(), getTablets(), and getAccessories(), and then getSuggestedProducts() calls getProducts(). These all run in Promise.all. However, note that getProductDetails itself already calls Promise.all on getPhones, getTablets, and getAccessories. Each of those calls includes a 300ms artificial delay. This results in at minimum 600ms of simulated delay for a single page load (300ms inside getProductDetails + 300ms for getProducts/getSuggestedProducts). Additionally, getProductDetails fetches all three detail JSON files just to find one product by ID, even though getProducts is also being fetched separately to find the matching Product by itemId. This is a significant performance concern — all three detail files are fetched on every product details page load, regardless of the product's category.
| "i18next": "^25.8.14", | ||
| "react": "^18.3.1", | ||
| "react-dom": "^18.3.1", | ||
| "react-i18next": "^16.5.5", |
There was a problem hiding this comment.
The package.json adds i18next and react-i18next as dependencies, but none of the source files in this PR use them. Looking through all the changed files, there are no imports of i18next or react-i18next. These unused dependencies increase bundle size unnecessarily. If internationalization is planned for the future, they should be added in a dedicated PR when they are actually implemented.
| "i18next": "^25.8.14", | |
| "react": "^18.3.1", | |
| "react-dom": "^18.3.1", | |
| "react-i18next": "^16.5.5", | |
| "react": "^18.3.1", | |
| "react-dom": "^18.3.1", |
| const CARD_WIDTH = 272; | ||
| const GAP = 16; | ||
|
|
||
| export const ProductsSlider: React.FC<Props> = ({ | ||
| title, | ||
| products, | ||
| hasDiscount, | ||
| }) => { | ||
| const [scrollPosition, setScrollPosition] = useState(0); | ||
| const trackRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| const step = CARD_WIDTH + GAP; | ||
| const maxScroll = Math.max(0, products.length * step - step * 4); |
There was a problem hiding this comment.
The ProductsSlider component uses a fixed CARD_WIDTH = 272 constant to compute maxScroll. However, this value is also hardcoded in the SCSS (> * { width: 272px; max-width: 272px; }). The slider's maxScroll calculation assumes exactly 4 visible cards (step * 4) regardless of the actual viewport width. On mobile, where the viewport is narrower, the slider still uses the desktop card width calculation for scroll limits, which will result in the slider allowing scroll positions that go past the content or not far enough. Consider computing the number of visible cards dynamically based on viewport width.
| <div className={styles.dropdown}> | ||
| <label className={styles.dropdown__label}>{label}</label> | ||
| <select | ||
| className={styles.dropdown__select} | ||
| value={value} | ||
| onChange={e => onChange(e.target.value)} | ||
| > |
There was a problem hiding this comment.
The Dropdown component renders a <label> element (line 17) without an associated htmlFor attribute. A <label> without htmlFor (or wrapping the associated control) is not properly linked to the <select> it describes. Screen readers may not correctly associate the label with the select, which is an accessibility issue. Add htmlFor to the label with an id matching the <select> element.
| // This is a helper to simulate a delay for loading states | ||
| const BASE_URL = ''; |
There was a problem hiding this comment.
The api.ts comment on line 1 says "This is a helper to simulate a delay for loading states" but it is placed at the top of the file above the BASE_URL constant, making it look like a file-level description. The comment is misplaced — it more accurately describes the wait function (line 4–8). This is a minor clarity issue but could confuse readers about what the comment applies to.
| data-cy="productQa498" | ||
| > |
There was a problem hiding this comment.
The data-cy attribute value "productQa498" appears to be a leftover from a test template or a typo. Looking at the surrounding code, this attribute is on the <span> that displays the cart item quantity. A meaningful value like "productQuantity" would be expected here, since other data-cy attributes in the same component use descriptive names like "cartDeleteButton". This value looks like an accidental artifact (possibly from a generated test ID) and will likely break any Cypress tests that query for this selector.
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| Github |
There was a problem hiding this comment.
The footer link label says "Github" but the correct capitalization of the brand name is "GitHub" (with a capital 'H'). This is a branding/spelling inconsistency.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 74 out of 94 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const hotPrices = [...products] | ||
| .filter(p => p.fullPrice > p.price) | ||
| .sort((a, b) => b.fullPrice - b.price - (a.fullPrice - a.price)); |
There was a problem hiding this comment.
The hotPrices sorting comparator is incorrect. The sort callback computes b.fullPrice - b.price (discount of b) minus a.fullPrice - a.price (discount of a), but the expression is written as b.fullPrice - b.price - (a.fullPrice - a.price). This is correct in arithmetic, but the actual bug is that the sort should be sorting by largest absolute discount (difference), yet the current code is (a, b) => b.fullPrice - b.price - (a.fullPrice - a.price) which computes (b.fullPrice - b.price) - (a.fullPrice - a.price). At first glance this looks correct, but the subtraction is applied without parentheses in the right positions and is actually equivalent to b.fullPrice - b.price - a.fullPrice + a.price, which is mathematically equivalent. The real issue here is subtle: the sort callback returns b.fullPrice - b.price - (a.fullPrice - a.price) without first verifying the result isn't accidentally b.fullPrice - (b.price - a.fullPrice) - a.price due to misreading. Let me re-examine: the actual written expression is b.fullPrice - b.price - (a.fullPrice - a.price) = b.fullPrice - b.price - a.fullPrice + a.price. This is correct arithmetic for sorting by descending absolute discount. However, a cleaner, clearly intentional way to write this would be (b.fullPrice - b.price) - (a.fullPrice - a.price) using explicit parentheses around both operands to make the intent unambiguous and avoid misreading. Consider rewriting the sort comparator with parentheses around both discount computations for clarity.
| <a | ||
| href="#" | ||
| className={styles.footer__link} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| Rights | ||
| </a> | ||
| </li> |
There was a problem hiding this comment.
The "Rights" footer link uses href="#" which does nothing useful and also has target="_blank" — opening a blank tab to # is meaningless and confusing to users. If there is no rights page yet, either remove this link or replace the href with a real URL.
| export const getSuggestedProducts = async () => { | ||
| const products = await getProducts(); | ||
|
|
||
| return [...products].sort(() => Math.random() - 0.5); | ||
| }; |
There was a problem hiding this comment.
The getSuggestedProducts function uses Math.random() - 0.5 as a sort comparator. This is a well-known anti-pattern: the sort order is not guaranteed to be a valid comparison function and different JavaScript engines may produce biased results. Consider using the Fisher-Yates shuffle algorithm for an unbiased random sort.
| data-cy="productQa498" | ||
| > |
There was a problem hiding this comment.
The data-cy attribute value "productQa498" appears to be a leftover placeholder or incorrectly named test selector. Cypress tests that target data-cy="productQuantity" (or similar meaningful name) would not find this element. This attribute should be renamed to a meaningful, consistent value that matches what Cypress tests actually query (e.g., data-cy="cartItemQuantity").
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| Github |
There was a problem hiding this comment.
The link text reads "Github" but the correct capitalization of the brand name is "GitHub". This should be corrected to "GitHub".
| export const getProductDetails = async ( | ||
| productId: string, | ||
| ): Promise<import('../types/ProductDetails').ProductDetails | null> => { | ||
| const [phones, tablets, accessories] = await Promise.all([ | ||
| getPhones(), | ||
| getTablets(), | ||
| getAccessories(), | ||
| ]); | ||
|
|
||
| const allDetails = [...phones, ...tablets, ...accessories]; | ||
|
|
||
| return allDetails.find(item => item.id === productId) || null; | ||
| }; |
There was a problem hiding this comment.
The getProductDetails function fetches all three categories (phones, tablets, accessories) every time it is called, even though each product belongs to exactly one category. This results in three parallel API requests to find one product. Since the ProductDetails type already has a category field, a more efficient approach would be to first get the product's category from the product list, then only fetch the corresponding category details file.
Checklist