refactor(weight-screen): improve API handling, validation, and state …#177
refactor(weight-screen): improve API handling, validation, and state …#177Popie52 wants to merge 2 commits intoAOSSIE-Org:mainfrom
Conversation
…management - Centralized API calls via weightApi service - Added loading indicators and pull-to-refresh - Fixed unsafe date handling with dayjs - Enhanced input validation for week number and weight - Used stable keys for history list rendering - Prevented state mutation on reverse() - Implemented optimistic UI updates for create, update, delete - Safer initialization and handling of editData - Improved UX with empty state message and form reset
📝 WalkthroughWalkthroughRefactors WeightScreen to use a new weightApi service with timeout/retries, adds dayjs for date handling, implements optimistic create/update/delete with rollback, introduces loading state and stronger validation (week 1–40, weight >0), and switches list keys to stable Changes
Sequence DiagramsequenceDiagram
participant User as User/UI
participant Screen as WeightScreen
participant Service as weightApi
participant Server as Backend API
User->>Screen: Submit create/update/delete action
Screen->>Screen: Validate input (week, weight)
alt Valid
Screen->>Screen: Apply optimistic local state change (insert/update/remove)
Note right of Screen: UI updates immediately
Screen->>Service: call create/update/remove API
activate Service
Service->>Server: HTTP request (with timeout + retry)
alt Server success
Server-->>Service: success response (entry with id)
Service-->>Screen: resolve with server data
Screen->>Screen: reconcile optimistic state (replace temp id / confirm)
else Server failure after retries
Server-->>Service: error
Service-->>Screen: reject with error
Screen->>Screen: rollback optimistic change, show error
end
deactivate Service
else Invalid
Screen->>User: show validation error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@Frontend/src/Screens/WeightScreen.jsx`:
- Around line 39-41: There is a typo in the error Alert message inside the
WeightScreen component's fetch error handling: change the Alert.alert call that
currently reads "unable to laod weight history" to "Unable to load weight
history" (or at minimum fix "laod" → "load") in the catch block where
console.error('Failed to fetch weights:', err) and Alert.alert are used.
- Around line 63-75: The weight validation currently treats Number('abc') as NaN
which bypasses the `weightNum <= 0` check; update both handleSubmit and
handleUpdate to explicitly reject non-numeric weights by checking
Number.isFinite(weightNum) (or !Number.isNaN(weightNum)) before the >0 check,
and show the same Alert when the value is not a finite positive number so
non-numeric inputs are not sent to the API.
In `@Frontend/src/services/weightApi.js`:
- Around line 20-26: The retry logic in the request function currently retries
all methods, which can re-submit non‑idempotent requests (e.g., POST) and create
duplicates; modify the catch blocks inside request to check options.method (or
default to 'GET') and only decrement retries/recursively retry for idempotent
methods (GET, HEAD, PUT, DELETE, OPTIONS) while avoiding retries for POST/PATCH
or when options.safeToRetry is false; apply the same guard to the second catch
block referenced around the other retry call so both retry paths use the method
check before calling request(endpoint, options, retries - 1).
- Around line 14-19: The current response handling always calls res.json(),
which throws on 204/empty responses (affecting DELETE in weightApi.js around the
delete call at lines 46–49); update the response handling block that currently
does "if (!res.ok) { ... } return res.json();" to first check for empty bodies
(e.g., if res.status === 204 || res.headers.get('content-length') === '0' ||
await res.clone().text() === '') and return an appropriate empty value (null or
{}) instead of calling res.json(), otherwise call res.json() for non-empty
successful responses so DELETE and other 204 responses don't raise a
SyntaxError.
| console.error('Failed to fetch weights:', err); | ||
| Alert.alert('Error', `unable to laod weight history`); | ||
| } finally { |
There was a problem hiding this comment.
Fix typo in error alert copy
Line 40: “laod” → “load”.
✏️ Suggested fix
- Alert.alert('Error', `unable to laod weight history`);
+ Alert.alert('Error', 'Unable to load weight history');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error('Failed to fetch weights:', err); | |
| Alert.alert('Error', `unable to laod weight history`); | |
| } finally { | |
| console.error('Failed to fetch weights:', err); | |
| Alert.alert('Error', 'Unable to load weight history'); | |
| } finally { |
🤖 Prompt for AI Agents
In `@Frontend/src/Screens/WeightScreen.jsx` around lines 39 - 41, There is a typo
in the error Alert message inside the WeightScreen component's fetch error
handling: change the Alert.alert call that currently reads "unable to laod
weight history" to "Unable to load weight history" (or at minimum fix "laod" →
"load") in the catch block where console.error('Failed to fetch weights:', err)
and Alert.alert are used.
| const handleSubmit = async () => { | ||
| if (!week || !weight) { | ||
| Alert.alert('Error', 'Please fill in all required fields'); | ||
| const weekNum = Number(week); | ||
| const weightNum = Number(weight); | ||
|
|
||
| if (!Number.isInteger(weekNum) || weekNum < 1 || weekNum > 40) { | ||
| Alert.alert('Invalid week', 'Week must be between 1 and 40'); | ||
| return; | ||
| } | ||
|
|
||
| if (weightNum <= 0) { | ||
| Alert.alert('Invalid weight', 'Weight must be greater than 0'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f WeightScreen.jsxRepository: AOSSIE-Org/BabyNest
Length of output: 100
🏁 Script executed:
wc -l Frontend/src/Screens/WeightScreen.jsx 2>/dev/null || echo "File not found or path needs correction"Repository: AOSSIE-Org/BabyNest
Length of output: 104
🌐 Web query:
JavaScript Number.isFinite NaN behavior compared to isNaN and less-than comparison with NaN
💡 Result:
-
Number.isFinite(value)- Returns
trueonly for values whose type isnumberand that are neitherNaNnor±Infinity; it does not coerce non-numbers. - So:
Number.isFinite(NaN) === false,Number.isFinite("0") === false. [1]
- Returns
-
Global
isNaN(value)vsNumber.isNaN(value)- Global
isNaNcoerces its argument to a number first, so many non-numbers becomeNaNand producetrue(e.g.isNaN("blabla") === true). [2] Number.isNaNdoes not coerce and is onlytruewhen the value is actually the numericNaN(e.g.Number.isNaN("NaN") === false,Number.isNaN(NaN) === true). [3]
- Global
-
Relational comparisons with
NaN(<,>,<=,>=)- If either operand becomes
NaN, the comparison returnsfalse. - So:
NaN < 3isfalse,3 < NaNisfalse(and similarly for the other relational operators). [4]
- If either operand becomes
Sources: [1] [2] [3] [4]
🏁 Script executed:
sed -n '60,80p' Frontend/src/Screens/WeightScreen.jsxRepository: AOSSIE-Org/BabyNest
Length of output: 613
🏁 Script executed:
sed -n '135,155p' Frontend/src/Screens/WeightScreen.jsxRepository: AOSSIE-Org/BabyNest
Length of output: 614
Add NaN validation to reject non-numeric weight inputs in create/update operations
When users enter non-numeric input, Number('abc') returns NaN. Since NaN <= 0 evaluates to false, the validation passes and invalid weight values are sent to the API. Both the handleSubmit (create) and handleUpdate methods have this vulnerability.
🔧 Suggested fix
- if (weightNum <= 0) {
+ if (!Number.isFinite(weightNum) || weightNum <= 0) {
Alert.alert('Invalid weight', 'Weight must be greater than 0');
return;
}Apply this fix at both locations: lines 63-75 (handleSubmit) and lines 135-150 (handleUpdate).
🤖 Prompt for AI Agents
In `@Frontend/src/Screens/WeightScreen.jsx` around lines 63 - 75, The weight
validation currently treats Number('abc') as NaN which bypasses the `weightNum
<= 0` check; update both handleSubmit and handleUpdate to explicitly reject
non-numeric weights by checking Number.isFinite(weightNum) (or
!Number.isNaN(weightNum)) before the >0 check, and show the same Alert when the
value is not a finite positive number so non-numeric inputs are not sent to the
API.
| if (!res.ok) { | ||
| const msg = await res.text(); | ||
| throw new Error(msg || 'Request failed'); | ||
| } | ||
|
|
||
| return res.json(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Fetch API Response.json() empty body 204 No Content behavior throws error
💡 Result:
response.json() rejects on a 204 No Content (or any empty body) because it reads the body to completion and parses it as JSON—and an empty string is not valid JSON, so you get a SyntaxError. MDN explicitly notes json() throws SyntaxError when the body can’t be parsed as JSON. [1] Also, by definition 204 responses do not include content. [2] The HTTP semantics spec (RFC 9110) likewise states that for status 204 “there is no content in the response.” [3]
Fix (client side): don’t call .json() when there’s no body
const res = await fetch(url);
if (res.status === 204) return null; // or undefined
// optionally also guard by content-type
const ct = res.headers.get("content-type") ?? "";
if (ct.includes("application/json")) return await res.json();
return await res.text();Alternative: parse conditionally from text
const res = await fetch(url);
const text = await res.text();
const data = text ? JSON.parse(text) : null;Fix (server side): if you want JSON, don’t return 204
If the client should always receive JSON (even “empty”), return 200 with {} / [] instead of 204, because 204 means no response content at all. [2][3]
Sources: [1] [2] [3]
🏁 Script executed:
# Find and examine the weightApi.js file
find . -name "weightApi.js" -type fRepository: AOSSIE-Org/BabyNest
Length of output: 99
🏁 Script executed:
cat -n ./Frontend/src/services/weightApi.jsRepository: AOSSIE-Org/BabyNest
Length of output: 1569
Handle empty/204 responses before calling res.json()
Line 19 will throw SyntaxError when res.json() is called on a 204 No Content response (or any empty body). DELETE requests (line 46–49) typically receive 204 responses, making delete operations fail unexpectedly.
🔧 Suggested fix (safe parsing)
- return res.json();
+ const text = await res.text();
+ if (!text) return null;
+ try {
+ return JSON.parse(text);
+ } catch {
+ return text;
+ }🤖 Prompt for AI Agents
In `@Frontend/src/services/weightApi.js` around lines 14 - 19, The current
response handling always calls res.json(), which throws on 204/empty responses
(affecting DELETE in weightApi.js around the delete call at lines 46–49); update
the response handling block that currently does "if (!res.ok) { ... } return
res.json();" to first check for empty bodies (e.g., if res.status === 204 ||
res.headers.get('content-length') === '0' || await res.clone().text() === '')
and return an appropriate empty value (null or {}) instead of calling
res.json(), otherwise call res.json() for non-empty successful responses so
DELETE and other 204 responses don't raise a SyntaxError.
| } catch (err) { | ||
| if (retries > 0) { | ||
| console.warn( | ||
| `Retrying request to ${endpoint}, attempts left: ${retries}`, | ||
| ); | ||
| return request(endpoint, options, retries - 1); | ||
| } |
There was a problem hiding this comment.
Guard retries for non‑idempotent requests
Line 20: retrying POST can create duplicate entries if the server processed the first request but the client timed out. Consider limiting retries to idempotent methods (or set retries=0 for create).
🔧 Suggested fix (limit retries to idempotent methods)
-const request = async (endpoint, options = {}, retries = 1) => {
+const request = async (endpoint, options = {}, retries = 1) => {
+ const method = (options.method || 'GET').toUpperCase();
+ const canRetry = ['GET', 'PUT', 'DELETE', 'HEAD', 'OPTIONS'].includes(method);
const controller = new AbortController();
const timeout = setTimeout(() => controller.abort(), 10000);
@@
- } catch (err) {
- if (retries > 0) {
+ } catch (err) {
+ if (canRetry && retries > 0) {
console.warn(
`Retrying request to ${endpoint}, attempts left: ${retries}`,
);
return request(endpoint, options, retries - 1);
}
throw err;
} finally {
clearTimeout(timeout);
}
};Also applies to: 35-40
🤖 Prompt for AI Agents
In `@Frontend/src/services/weightApi.js` around lines 20 - 26, The retry logic in
the request function currently retries all methods, which can re-submit
non‑idempotent requests (e.g., POST) and create duplicates; modify the catch
blocks inside request to check options.method (or default to 'GET') and only
decrement retries/recursively retry for idempotent methods (GET, HEAD, PUT,
DELETE, OPTIONS) while avoiding retries for POST/PATCH or when
options.safeToRetry is false; apply the same guard to the second catch block
referenced around the other retry call so both retry paths use the method check
before calling request(endpoint, options, retries - 1).
|
@Popie52 |
3 participants
Description:
This PR refactors the WeightScreen component to enhance reliability, maintainability, and user experience.
Key improvements:
weightApiserviceeditDataCloses Issue:
Closes #172
Changes Made:
entry.idkeysChecklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.