Conversation
added 2 commits
February 24, 2026 12:37
- Move useFood() hook from Landing to NutritionSection - Remove data/loading/error props from component chain - Simplify FoodHome by removing loading state - Enable lazy data fetching on user interaction instead of mount - Improve component independence and testability
- Add ErrorBoundary class component with reset functionality wrapping the app - Handle edge cases in fetchFood: empty results, unexpected response shape, axios vs non-axios errors with HTTP status messages - Guard per-card missing nutriments in NutritionSection - Switch data source from Open Food Facts to wger Ingredient API - Update types (WgerIngredient, WgerResponse) and fetchFood to return Product[] - Refactor useFood hook state from Product|null to Product[] - Refactor NutritionSection into responsive 3-column card grid
Change SummaryIntroduces a new App entry wrapped by ErrorBoundary, improving runtime resilience across the experience. File Changes
Based on 0afa153...5293b5f |
PR ReviewPR Communication NotesDescription Quality
PR Size & Scope
Commit Messages
Issue NotesCode Correctness & Design Quality
Test Quality & Coverage
Code Readability & Maintainability
💬 Minor Issues (Nitpicks)Code Correctness & Design Quality
Code Readability & Maintainability
Based on 0afa153...5293b5f |
![mergemitra[bot]](https://avatars.githubusercontent.com/in/2493799?s=60&v=4){kind=small}
Comment on lines
+31
to
+38
| .map(({ name, energy, protein, carbohydrates, fat }) => ({ | ||
| product_name: name, | ||
| nutriments: { | ||
| "energy-kcal_100g": energy, | ||
| proteins_100g: protein ? parseFloat(protein) : undefined, | ||
| carbohydrates_100g: carbohydrates ? parseFloat(carbohydrates) : undefined, | ||
| fat_100g: fat ? parseFloat(fat) : undefined, | ||
| }, |
There was a problem hiding this comment.
Severity: 🟠 Major
Guard parsed numeric fields so NaN never reaches nutriments (it will render as NaN g); convert non-finite values to undefined.
const toFinite = (v: string | null) => {
const n = v == null ? NaN : Number.parseFloat(v);
return Number.isFinite(n) ? n : undefined;
};
| const { data, loading, error } = useFood(); | ||
|
|
||
| return ( | ||
| <main className="min-h-screen bg-linear-to-br from-emerald-900 via-slate-900 to-black text-white px-6 py-12"> |
There was a problem hiding this comment.
Severity: 🟠 Major
bg-linear-to-br isn’t a standard Tailwind gradient utility—if you intended a gradient, use bg-gradient-to-br (as in FoodHome) so the background renders.
<main className="min-h-screen bg-gradient-to-br ...">
Comment on lines
+4
to
+6
| // wger Ingredient API — free, no API key, dedicated nutrition database | ||
| const BASE_URL = | ||
| import.meta.env.VITE_MOOD_API_URL ?? | ||
| "https://jsonplaceholder.typicode.com/users"; | ||
| "https://wger.de/api/v2/ingredient/?format=json&language=2&page_size=12&ordering=name"; |
There was a problem hiding this comment.
Severity: 🟠 Major
Hardcoding the full wger URL (with magic query params) makes environment config and tests harder—move the base URL to an env var and pass params via axios options.
const BASE_URL = import.meta.env.VITE_WGER_API_URL ?? "https://wger.de/api/v2/ingredient/";
Comment on lines
+41
to
+44
| {data.map((product, index) => ( | ||
| <div | ||
| key={`${product.product_name}-${index}`} | ||
| className="bg-slate-800 p-6 rounded-xl shadow-lg hover:shadow-emerald-900/40 hover:scale-[1.01] transition-all" |
There was a problem hiding this comment.
Severity: 🟠 Major
Avoid index in list keys; use a stable unique id from the API (add id to WgerIngredient/Product during mapping) so React doesn’t mis-associate DOM/state when the list order changes.
<div key={product.id}>
Comment on lines
+15
to
+16
| } catch { | ||
| setError("Failed to fetch food data"); |
There was a problem hiding this comment.
Severity: 🟠 Major
Don’t swallow the thrown error from fetchFood—surface its message (with a safe fallback) so the UI can show status-specific failures.
} catch (err) {
setError(err instanceof Error ? err.message : "Failed to fetch food data");
}
PR OverviewPR Type: Feature + Refactor Focus Areas for Architect Review
|
…ng, and tests - guard numeric parsing using tofinite to prevent nan values in nutriments - move wger base url to vite_wger_api_url and pass query params via axios `params` - add `id` to wgeringredient/product models, map through response, and use `product.id` as react list key - fix tailwind class typo (`bg-gradient-to-br`) in nutritionsection.tsx - surface thrown error messages in usefood with safe fallback handling - rename single-letter variables to descriptive names for improved readability - update tests, mocks, and test setup to align with implementation changes; all tests passing
|
Tip Need another review? Tag me and say rereview for re-analysis after you have fixed all the issues. @mergemitra rereview |
shashank-CW
requested changes
Mar 21, 2026
shashank-CW
left a comment
shashank-CW
left a comment
Collaborator
There was a problem hiding this comment.
Hi Saumya, here's my in-depth review. You can prioritise fixing the error boundary related items first. Hope you see these as fun puzzles to be solved and not an overwhelming list of items to be done. We can discuss and solve any obstacles you're facing. All the best!
| @@ -0,0 +1,12 @@ | |||
| import "../index.css"; | |||
| import { FoodLandingPage } from "../feature/food/pages/Landing"; | |||
Collaborator
There was a problem hiding this comment.
Is it better to name "../feature/food/pages/Landing" to "../feature/food/pages/FoodLandingPage"? Why? Why not?
| <div className="min-h-screen flex items-center justify-center bg-black text-white"> | ||
| <div className="text-center space-y-4"> | ||
| <h1 className="text-3xl font-bold">⚠️ Something went wrong</h1> | ||
| <p className="text-gray-400">Error: {this.state.error?.message}</p> |
Collaborator
There was a problem hiding this comment.
What happens when message is an empty string?
Owner
Author
There was a problem hiding this comment.
current code doesn’t break, but it fails silently in UX when the error message is empty
| export function FoodLandingPage() { | ||
| const { data, loading, error } = useFood(); | ||
| const [showNutrition, setShowNutrition] = useState(false); | ||
|
|
Collaborator
There was a problem hiding this comment.
type View = "home" | "nutrition";
const [view, setView] = useState<View>("home");
Is this better? Why / why not?
Owner
Author
There was a problem hiding this comment.
- Using a union type instead of a boolean makes the UI state explicit,
- avoids conflicting states, and scales better as more views are added.
Example:
now - "home" | "nutrition"
later can be scaled to - "home" | "nutrition" | "recipe" | "error"
but with boolean:
- showNutrition
- showRecipe
- showError
Comment on lines
+11
to
+13
| <FoodHome onStart={() => setShowNutrition(true)} /> | ||
| ) : ( | ||
| <div className="w-full max-w-2xl"> | ||
| <div className="flex justify-center mb-6"> | ||
| <Button variant="secondary" onClick={() => setShowNutrition(false)}> | ||
| Return to Home | ||
| </Button> | ||
| </div> | ||
|
|
||
| {error && ( | ||
| <p className="text-red-400 text-center mb-4">Error: {error}</p> | ||
| )} | ||
|
|
||
| {data ? ( | ||
| <div className="bg-slate-800 p-6 rounded-lg"> | ||
| <h2 className="text-xl font-semibold mb-4 text-center"> | ||
| Nutrition Information | ||
| </h2> | ||
| <h3 className="text-lg font-medium mb-2">{data.product_name}</h3> | ||
| {data.brands && ( | ||
| <p className="text-slate-300 mb-4">Brands: {data.brands}</p> | ||
| )} | ||
| {data.nutriments && ( | ||
| <div className="grid grid-cols-2 gap-4"> | ||
| {[ | ||
| { | ||
| label: "Energy", | ||
| value: data.nutriments["energy-kcal_100g"], | ||
| unit: "kcal", | ||
| }, | ||
| { | ||
| label: "Proteins", | ||
| value: data.nutriments.proteins_100g, | ||
| unit: "g", | ||
| }, | ||
| { | ||
| label: "Fat", | ||
| value: data.nutriments.fat_100g, | ||
| unit: "g", | ||
| }, | ||
| { | ||
| label: "Carbohydrates", | ||
| value: data.nutriments.carbohydrates_100g, | ||
| unit: "g", | ||
| }, | ||
| ].map(({ label, value, unit }) => ( | ||
| <div key={label}> | ||
| <p className="text-sm text-slate-300">{label}</p> | ||
| <p className="text-lg"> | ||
| {value || 0} {unit} | ||
| </p> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| )} | ||
| </div> | ||
| ) : ( | ||
| <p className="text-slate-300 text-center">No data available.</p> | ||
| )} | ||
| </div> | ||
| <NutritionSection onBack={() => setShowNutrition(false)} /> |
Collaborator
There was a problem hiding this comment.
If https://github.com/saumya-cw/Learning/pull/2/changes#r2969377155 is better, then
<FoodHome onStart={() => setView("nutrition")} />
<NutritionSection onBack={() => setView("home")} />
| <div key={label}> | ||
| <p className="text-sm text-slate-300">{label}</p> | ||
| <p className="text-lg"> | ||
| {nutriments?.[key] ?? 0} {unit} |
Collaborator
There was a problem hiding this comment.
const value = nutriments?.[key];
<p className="text-lg">
{value == null ? "-" : `${value} ${unit}`}
</p>
Is this better?
| setData(result); | ||
| } catch (err) { | ||
| setError( | ||
| err instanceof Error ? err.message : "Failed to fetch food data", |
Collaborator
There was a problem hiding this comment.
Either align the implementation with the PR description or update the PR description. Fetch failures currently render inline error state and do not reach the ErrorBoundary.
| import { fetchFood } from "../services/foodApi"; | ||
| import type { Product } from "../services/food.type"; | ||
|
|
||
| export const useFood = () => { |
Collaborator
There was a problem hiding this comment.
Add explicit hook return types
| ]; | ||
|
|
||
| (foodApi.fetchFood as any).mockResolvedValue(mockProduct); | ||
| (foodApi.fetchFood as any).mockResolvedValue(mockProducts); |
| }); | ||
|
|
||
| it("should set error when fetch fails", async () => { | ||
| (foodApi.fetchFood as any).mockRejectedValue(new Error("API Error")); |
| }); | ||
|
|
||
| it("should have loading true initially", () => { | ||
| it("should have loading true initially and empty data", () => { |
Collaborator
There was a problem hiding this comment.
Passes with a React act(...) warning. The initial-state test mounts the hook but does not control the async update path cleanly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What This PR Does
Strengthens the app's resilience by introducing an ErrorBoundary for
runtime crash recovery and adding comprehensive edge-case handling across
the data layer and UI. Also migrates to the wger API and refactors the
nutrition display into a multi-product grid.
Error Boundary
New:
ErrorBoundaryclass componentWraps the entire app. Catches any unhandled runtime errors that bubble up
from the component tree and displays a user-friendly fallback instead of
a blank/broken screen.
Handles:
componentDidCatchfor debuggingEdge Case Handling
fetchFood— API layerresultsismissing or not an array
[]cleanly instead of failing silently(e.g.
wger API request failed with status 404)ErrorBoundaryoruseFoodcan handle them appropriatelyname before mapping, preventing blank cards from rendering
NutritionSection— UI layer[]product.nutrimentsbeforerendering
NutritionGrid— missing data shows a graceful fallback textinstead of crashing or rendering an empty card
Other Changes
(free, no API key, returns a list of 12 ingredients)
useFoodstate updated fromProduct | nulltoProduct[]NutritionSectionrebuilt as a responsive 3-column card gridNutritionGridnutrimentsprop made required (parent guards before render)WgerIngredientandWgerResponsetypes; removedFoodResponseManual Steps for Reviewer
Definition of Done
Testing