Bug #445 - Pump Power empty field #447
Conversation
… if Pump Power is empty** - Replaced `isNumber` with a new `isNumeric` utility for better type checks (NaN). - Added `handleSave` to normalize `pump` values in both forms and ensure `NaN` values are converted to `0` to avoid potential issues.
WalkthroughAdds pump numeric-check helper and replaces isNumber with isNumeric in pump-related logic. Introduces handleSave in profile forms to normalize phases by converting NaN pump values to 0 before invoking onSave. Updates submission flow to route through normalization. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Form as Profile Form (Extended/Standard)
participant Normalizer as handleSave (normalize phases)
participant Store as onSave (consumer)
User->>Form: Submit
Form->>Normalizer: handleSave(data)
Note right of Normalizer: For each phase: if pump is NaN -> set to 0
Normalizer-->>Form: normalizedData
Form->>Store: onSave(normalizedData)
Store-->>Form: ack
sequenceDiagram
autonumber
participant UI as Phase UI
participant Logic as Pump Logic
UI->>Logic: compute pumpPower/mode
alt pump is numeric (isNumeric)
Logic-->>UI: derive from numeric pump
else pump not numeric
Logic-->>UI: derive from phase.pump.target / defaults
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 2
🧹 Nitpick comments (2)
web/src/pages/ProfileEdit/ExtendedPhase.jsx (1)
48-48: Mixing isNumber and isNumeric helpers is inconsistent.Line 48 uses
isNumberfrom chart.js while lines 47 and 50 use the newisNumerichelper. This creates inconsistency in how numeric checks are performed.For consistency, consider using
isNumericthroughout:- const pressure = !isNumber(phase.pump) ? phase.pump.pressure : 0; + const pressure = !isNumeric(phase.pump) ? phase.pump.pressure : 0;And remove the unused import:
-import { isNumber } from 'chart.js/helpers';web/src/pages/ProfileEdit/StandardProfileForm.jsx (1)
219-219: Mixing isNumber and isNumeric helpers is inconsistent.Line 219 uses
isNumberfrom chart.js while lines 218 and 221 use the newisNumerichelper. This creates inconsistency in how numeric checks are performed.For consistency, consider using
isNumericthroughout:- const pressure = !isNumber(phase.pump) ? phase.pump.pressure : 0; + const pressure = !isNumeric(phase.pump) ? phase.pump.pressure : 0;And if
isNumberis no longer used elsewhere in the file, remove the unused import:-import { isNumber } from 'chart.js/helpers';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/src/pages/ProfileEdit/ExtendedPhase.jsx(2 hunks)web/src/pages/ProfileEdit/ExtendedProfileForm.jsx(1 hunks)web/src/pages/ProfileEdit/StandardProfileForm.jsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
web/src/pages/ProfileEdit/ExtendedProfileForm.jsx (2)
web/src/pages/ProfileEdit/StandardProfileForm.jsx (1)
handleSave(57-69)web/src/pages/ProfileEdit/index.jsx (2)
data(21-21)onSave(85-94)
web/src/pages/ProfileEdit/StandardProfileForm.jsx (3)
web/src/pages/ProfileEdit/ExtendedProfileForm.jsx (1)
handleSave(68-80)web/src/pages/ProfileEdit/index.jsx (2)
data(21-21)onSave(85-94)web/src/pages/ProfileEdit/ExtendedPhase.jsx (5)
isNumeric(8-8)pumpPower(47-47)pressure(48-48)flow(49-49)mode(50-50)
web/src/pages/ProfileEdit/ExtendedPhase.jsx (2)
web/src/pages/ProfileEdit/StandardProfileForm.jsx (5)
isNumeric(188-188)pumpPower(218-218)pressure(219-219)flow(220-220)mode(221-221)web/src/pages/ProfileList/utils.js (1)
phase(36-48)
🔇 Additional comments (4)
web/src/pages/ProfileEdit/ExtendedProfileForm.jsx (2)
68-80: LGTM! NaN normalization prevents invalid pump states.The normalization logic correctly identifies NaN values and converts them to 0 before saving, which should resolve the disappearing field issue mentioned in the PR objectives.
89-89: LGTM! Submission now routes through normalization.Form submission correctly invokes
handleSaveto ensure pump values are normalized before persisting.web/src/pages/ProfileEdit/StandardProfileForm.jsx (2)
57-69: LGTM! NaN normalization prevents invalid pump states.The normalization logic correctly identifies NaN values and converts them to 0 before saving, consistent with the implementation in ExtendedProfileForm.
75-75: LGTM! Submission now routes through normalization.Form submission correctly invokes
handleSaveto ensure pump values are normalized before persisting.
| import { faPlus } from '@fortawesome/free-solid-svg-icons/faPlus'; | ||
|
|
||
| export function ExtendedPhase({ phase, index, onChange, onRemove, pressureAvailable }) { | ||
| const isNumeric = v => typeof v === 'number'; |
There was a problem hiding this comment.
isNumeric doesn't filter out NaN as described in PR objectives.
The helper returns true for NaN since typeof NaN === 'number' in JavaScript. This contradicts the PR description stating it "handles NaN" to improve type checks.
Apply this diff to properly exclude NaN:
- const isNumeric = v => typeof v === 'number';
+ const isNumeric = v => typeof v === 'number' && !Number.isNaN(v);Without this fix, lines 47 and 50 will incorrectly treat NaN pump values as valid numbers, causing:
- Line 47:
pumpPowerwill be NaN instead of defaulting to 100 - Line 50:
modewill be 'power' when pump is NaN (undefined behavior)
While handleSave in the form normalizes NaN to 0 before saving, the UI will still display incorrectly when pump becomes NaN during editing.
📝 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.
| const isNumeric = v => typeof v === 'number'; | |
| const isNumeric = v => typeof v === 'number' && !Number.isNaN(v); |
🤖 Prompt for AI Agents
In web/src/pages/ProfileEdit/ExtendedPhase.jsx around line 8, the isNumeric
helper currently returns true for NaN because it only checks typeof v ===
'number'; change it to exclude NaN by using Number.isFinite(v) (or equivalently
typeof v === 'number' && !Number.isNaN(v)) so NaN is treated as non-numeric;
update the helper accordingly so downstream logic at lines ~47 and ~50 will
default correctly (pumpPower -> 100 and mode selection) when the value is NaN.
| } | ||
|
|
||
| function Phase({ phase, index, onChange, onRemove, pressureAvailable }) { | ||
| const isNumeric = v => typeof v === 'number'; |
There was a problem hiding this comment.
isNumeric doesn't filter out NaN as described in PR objectives.
The helper returns true for NaN since typeof NaN === 'number' in JavaScript. This contradicts the PR description stating it "handles NaN" to improve type checks.
Apply this diff to properly exclude NaN:
- const isNumeric = v => typeof v === 'number';
+ const isNumeric = v => typeof v === 'number' && !Number.isNaN(v);Without this fix, lines 218 and 221 will incorrectly treat NaN pump values as valid numbers, causing:
- Line 218:
pumpPowerwill be NaN instead of defaulting to 100 - Line 221:
modewill be 'power' when pump is NaN (undefined behavior)
While handleSave normalizes NaN to 0 before saving, the UI will still display incorrectly when pump becomes NaN during editing.
📝 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.
| const isNumeric = v => typeof v === 'number'; | |
| const isNumeric = v => typeof v === 'number' && !Number.isNaN(v); |
🤖 Prompt for AI Agents
In web/src/pages/ProfileEdit/StandardProfileForm.jsx around line 188, the
isNumeric helper currently returns true for NaN because it only checks typeof
=== 'number'; update it to exclude NaN (use Number.isFinite(v) or (typeof v ===
'number' && !Number.isNaN(v))) so NaN is treated as non-numeric; this ensures
downstream logic on lines ~218 and ~221 will default pumpPower to 100 and not
switch mode to 'power' when the value is NaN.
1 participant
Avoid disappearing field and undefined Pump mode if Pump Power is empty
isNumberwith a newisNumericutility for better type checks including NaN.handleSaveto normalizepumpvalues in both forms and ensureNaNvalues are converted to0to avoid potential issues.#445
Summary by CodeRabbit
Bug Fixes
Improvements