Skip to content

feat: Allow flow control with pressure limit#320

Merged
jniebuhr merged 12 commits intomasterfrom
feature/flow-control
Jul 26, 2025
Merged

feat: Allow flow control with pressure limit#320
jniebuhr merged 12 commits intomasterfrom
feature/flow-control

Conversation

@jniebuhr
Copy link
Owner

@jniebuhr jniebuhr commented Jul 23, 2025

Summary by CodeRabbit

  • New Features

    • Introduced selectable control modes (Power, Pressure, Flow) for enhanced pump control flexibility.
    • Added support for setting flow and pressure limits during operation.
  • Refactor

    • Simplified and streamlined pump control logic for improved reliability and maintainability.
    • Enhanced internal consistency and accuracy of flow and pressure regulation.
    • Updated UI and control signals to reflect real-time pump pressure and flow values dynamically.
    • Unified target variables for pump control and delegated power calculations to the pressure controller.
    • Refined pressure controller to handle separate pressure and flow setpoints with mode-dependent control logic.
    • Improved UI components with clearer pump and valve controls and updated styling for mode selection.
    • Applied smoothing to flow measurements for more stable readings.
    • Replaced dynamic size classes with predefined mappings in spinner component for consistency.
    • Enabled debugging features during development for improved diagnostics.
  • Bug Fixes

    • Improved consistency in pump behavior when switching between control modes.
    • Corrected tooltip attribute references for proper display.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jul 23, 2025

Warning

Rate limit exceeded

@jniebuhr has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 18 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 61f5b12 and 1535638.

📒 Files selected for processing (1)
  • web/src/pages/ProfileEdit/StandardProfileForm.jsx (6 hunks)

Walkthrough

The control logic for the DimmedPump and PressureController classes was refactored to delegate power and limit handling to the controller, introduce explicit control modes (POWER, PRESSURE, FLOW), and improve internal state management. Several helper functions were removed, interfaces updated, and const-correctness applied to relevant methods. Additionally, pump target handling was enhanced in the UI and process layers to support separate flow and pressure retrieval and conditional pressure control signaling.

Changes

File(s) Change Summary
lib/GaggiMateController/src/peripherals/DimmedPump.cpp, .h Refactored to delegate power/limit handling to PressureController, replaced separate flow/pressure targets with unified control variables, removed helper methods.
lib/NayrodPID/src/PressureController/PressureController.cpp, .h Added ControlMode enum, refactored update/filter logic for mode awareness, added flow and pressure limit setters, improved const-correctness, and restructured pump duty cycle computations.
src/display/core/Controller.cpp, src/display/core/Process.h, src/display/ui/default/DefaultUI.cpp Enhanced pump target handling with new PumpTarget return type and separate flow/pressure getters; updated control signaling to conditionally enable pressure control and reflect actual pump state; updated UI to use current pump pressure.
web/src/pages/ProfileEdit/StandardProfileForm.jsx, web/src/pages/ProfileEdit/index.jsx, web/src/pages/ProfileEdit/style.css Refactored profile form to support multiple pump modes ("Off," "Power," "Pressure," "Flow") with segmented control UI; added pressureAvailable prop; updated valve control UI; added new CSS styles for mode selector.
web/src/components/Spinner.jsx Changed size styling to use a predefined class mapping for specific sizes instead of dynamic template literals.
web/src/index.jsx Added import of preact/debug for development debugging features.
web/src/style.css Fixed tooltip attribute reference from tooltip to data-tooltip; removed unused CSS directive.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant DimmedPump
    participant PressureController

    User->>DimmedPump: setFlowTarget() / setPressureTarget()
    DimmedPump->>PressureController: setFlowLimit() / setPressureLimit()
    User->>DimmedPump: updatePower()
    DimmedPump->>PressureController: update(mode)
    PressureController-->>DimmedPump: controllerPower
    DimmedPump-->>User: power updated
Loading
sequenceDiagram
    participant BrewProcess
    participant Controller
    participant ClientController

    Controller->>BrewProcess: getPumpTarget()
    BrewProcess-->>Controller: PumpTarget (pressure, flow)
    Controller->>ClientController: sendAdvancedOutputControl(relayActive, targetTemp, pressureControlEnabled, pressure, flow)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Poem

A bunny hopped to see the code,
Where pumps and pressures now unload.
With modes for flow, and limits set,
The logic’s clean—no need to fret!
Targets split, controls refined,
In circuits neat, the signals bind.
🥕✨🐇

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/flow-control

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🔭 Outside diff range comments (1)
lib/GaggiMateController/src/peripherals/DimmedPump.cpp (1)

27-27: Remove assignment to unused variable _targetPressure.

This line assigns to _targetPressure which should be removed along with the variable declaration as suggested in the header file review.

-    _targetPressure = setpoint == 100 ? 20.0f : 0.0f;
+    _ctrlTarget = setpoint == 100 ? 20.0f : 0.0f;
🧹 Nitpick comments (8)
lib/NayrodPID/src/PressureController/PressureController.h (1)

19-20: Improve formatting consistency for inline setter methods.

The inline implementations have inconsistent spacing. Consider using consistent formatting:

-    void setFlowLimit(float lim){_flowLimit = lim;};
-    void setPressureLimit(float lim){_pressureLimit = lim;};
+    void setFlowLimit(float lim) { _flowLimit = lim; }
+    void setPressureLimit(float lim) { _pressureLimit = lim; }
lib/GaggiMateController/src/peripherals/DimmedPump.cpp (1)

54-54: Remove unnecessary static_cast between identical enum types.

The cast from DimmedPump::ControlMode to PressureController::ControlMode appears to be between identical enum types and should be removed.

-    _pressureController.update(static_cast<PressureController::ControlMode>(_mode));
+    _pressureController.update(_mode);

However, if the enums are actually different types, ensure they have the same values in the same order to avoid incorrect conversions.

lib/NayrodPID/src/PressureController/PressureController.cpp (6)

51-51: Remove extra space before closing parenthesis.

-void PressureController::update(ControlMode mode ) {
+void PressureController::update(ControlMode mode) {

55-55: Fix typos in comment.

-            if(isRconverged){// With R estimated we can gestimate the appropriate pressure setpoint to not go above flow rate limite
+            if(isRconverged){// With R estimated we can estimate the appropriate pressure setpoint to not go above flow rate limit

60-60: Add spaces around operators for better readability.

-                if(fabs(_r-_filteredPressureSensor) <0.2 ){ // We consider the pressure to be established so pump flow = coffee flow
+                if(fabs(_r - _filteredPressureSensor) < 0.2 ){ // We consider the pressure to be established so pump flow = coffee flow

167-180: Fix typos in comments.

-    // COMMAND IS ACTUALLY ZERO: The profil is asking for no pressure (ex: blooming phase)
+    // COMMAND IS ACTUALLY ZERO: The profile is asking for no pressure (ex: blooming phase)
-    // CONTROL: The boiler is pressurised, the profil is something specific, let's try to
+    // CONTROL: The boiler is pressurized, the profile is something specific, let's try to

80-80: Simplify expression formatting for better readability.

The calculation has inconsistent spacing that makes it harder to read.

-                *_ctrlOutput = 100.0f  * _r /( _Q0 * (1 - _filteredPressureSensor / _Pmax));
+                *_ctrlOutput = 100.0f * _r / (_Q0 * (1 - _filteredPressureSensor / _Pmax));

72-73: Fix spacing in comment.

-            // Coffee flow  = Pressure / R, with the estimated R we can find the appropiate pressure.
+            // Coffee flow = Pressure / R, with the estimated R we can find the appropriate pressure.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 82eb56e and c8505a9.

📒 Files selected for processing (4)
  • lib/GaggiMateController/src/peripherals/DimmedPump.cpp (2 hunks)
  • lib/GaggiMateController/src/peripherals/DimmedPump.h (1 hunks)
  • lib/NayrodPID/src/PressureController/PressureController.cpp (7 hunks)
  • lib/NayrodPID/src/PressureController/PressureController.h (3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
lib/NayrodPID/src/PressureController/PressureController.cpp (1)
lib/NayrodPID/src/HydraulicParameterEstimator/HydraulicParameterEstimator.cpp (2)
  • update (56-122)
  • update (56-56)
lib/NayrodPID/src/PressureController/PressureController.h (1)
lib/NayrodPID/src/PressureController/PressureController.cpp (21)
  • PressureController (9-20)
  • filterSetpoint (22-29)
  • filterSetpoint (22-22)
  • initSetpointFilter (31-37)
  • initSetpointFilter (31-31)
  • setupSetpointFilter (39-45)
  • setupSetpointFilter (39-39)
  • update (51-92)
  • update (51-51)
  • getPumpDutyCycleForFlowRate (115-119)
  • getPumpDutyCycleForFlowRate (115-115)
  • computePumpDutyCycle (158-222)
  • computePumpDutyCycle (158-158)
  • virtualScale (127-156)
  • virtualScale (127-127)
  • filterSensor (47-47)
  • filterSensor (47-47)
  • computeAdustedCoffeeFlowRate (94-100)
  • computeAdustedCoffeeFlowRate (94-94)
  • pumpFlowModel (102-113)
  • pumpFlowModel (102-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: test
  • GitHub Check: deploy
  • GitHub Check: test
🔇 Additional comments (2)
lib/NayrodPID/src/PressureController/PressureController.h (1)

42-43: Remove default parameter values from method declarations.

Having default parameter values in these const method declarations is problematic as they're not used consistently in the implementation. This could lead to compilation errors or unexpected behavior.

-    float computeAdustedCoffeeFlowRate(float pressure) const;
-    float pumpFlowModel(float alpha) const;
+    float computeAdustedCoffeeFlowRate(float pressure = 0.0f) const;
+    float pumpFlowModel(float alpha = 100.0f) const;

Likely an incorrect or invalid review comment.

lib/NayrodPID/src/PressureController/PressureController.cpp (1)

56-58: Verify resistance value usage in pressure calculation.

The code modifies *_rawSetpoint directly when flow exceeds the limit. This side effect might be unexpected to callers who pass in a setpoint value.

Consider using a local variable instead of modifying the input parameter:

float adjustedSetpoint = *_rawSetpoint;
if(flowPerSecond > _flowLimit){
    adjustedSetpoint = _flowLimit * this->R_estimator->getResistance();
}
filterSetpoint(adjustedSetpoint);

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
lib/NayrodPID/src/PressureController/PressureController.cpp (1)

76-76: Move default parameter values to header file

Default parameter values should be in the header declaration, not in the implementation file.

Also applies to: 84-84

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 14acc24 and 0591fbf.

📒 Files selected for processing (4)
  • lib/GaggiMateController/src/peripherals/DimmedPump.cpp (3 hunks)
  • lib/GaggiMateController/src/peripherals/DimmedPump.h (1 hunks)
  • lib/NayrodPID/src/PressureController/PressureController.cpp (8 hunks)
  • lib/NayrodPID/src/PressureController/PressureController.h (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • lib/GaggiMateController/src/peripherals/DimmedPump.h
  • lib/NayrodPID/src/PressureController/PressureController.h
  • lib/GaggiMateController/src/peripherals/DimmedPump.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: test
  • GitHub Check: deploy
  • GitHub Check: test
🔇 Additional comments (4)
lib/NayrodPID/src/PressureController/PressureController.cpp (4)

9-12: Constructor properly handles dual setpoints

The separation of pressure and flow setpoints in the constructor aligns well with the PR's objective of allowing flow control with pressure limits.


23-28: Filter implementation is flexible and correct

The parameterized approach allows filtering different setpoints, and the second-order filter dynamics are properly implemented.


119-131: Improved convergence checking and setpoint validation

Good use of the hasConverged() method for better readability and proper validation of the pressure setpoint.


140-204: Excellent refactoring to return-based approach

The change from void to float return type improves the method's testability and follows the single responsibility principle. The control logic properly handles all edge cases.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
web/src/components/Spinner.jsx (1)

1-12: Safer class handling with predefined mappings

Good refactoring to use predefined class mappings instead of dynamic template literals. This prevents potential issues with PurgeCSS and ensures only valid Tailwind classes are used.

Consider documenting supported sizes or making the component more extensible for future size additions.

web/src/pages/ProfileEdit/StandardProfileForm.jsx (1)

143-143: Use strict equality for zero check

Consider using strict equality to avoid type coercion issues.

-    if (value === 0) {
+    if (value === 0 || value === '0') {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0591fbf and 13dcf68.

📒 Files selected for processing (8)
  • lib/GaggiMateController/src/peripherals/DimmedPump.cpp (3 hunks)
  • lib/GaggiMateController/src/peripherals/DimmedPump.h (1 hunks)
  • web/src/components/Spinner.jsx (1 hunks)
  • web/src/index.jsx (1 hunks)
  • web/src/pages/ProfileEdit/StandardProfileForm.jsx (6 hunks)
  • web/src/pages/ProfileEdit/index.jsx (2 hunks)
  • web/src/pages/ProfileEdit/style.css (1 hunks)
  • web/src/style.css (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • web/src/index.jsx
  • web/src/pages/ProfileEdit/style.css
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/GaggiMateController/src/peripherals/DimmedPump.h
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/src/pages/ProfileEdit/StandardProfileForm.jsx (3)
web/src/pages/ProfileEdit/index.jsx (1)
  • pressureAvailable (10-10)
web/src/pages/Settings/index.jsx (1)
  • onChange (42-87)
web/src/pages/OTA/index.jsx (1)
  • phase (10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: deploy
  • GitHub Check: test
  • GitHub Check: test
🔇 Additional comments (9)
web/src/style.css (1)

111-111: Correct tooltip attribute reference

Good catch! This fixes the tooltip content to correctly reference the data-tooltip attribute, matching the CSS selectors used throughout the file.

lib/GaggiMateController/src/peripherals/DimmedPump.cpp (4)

5-9: Constructor properly updated for new control architecture

The constructor correctly initializes the PressureController with the new unified control variables, aligning with the refactored control model.


24-30: Verify hardcoded pressure value in power mode

Setting _ctrlPressure to a fixed 20.0f when power > 0 appears arbitrary. Please verify if this is intended as a safety limit and document why this specific value was chosen.


55-61: Clean delegation to PressureController

Good refactoring - the power update logic properly delegates control calculations to the PressureController based on the current mode.


63-75: Well-structured target setters with proper limit handling

Both setFlowTarget and setPressureTarget correctly configure the control mode and set appropriate limits on the PressureController, following a consistent pattern.

web/src/pages/ProfileEdit/index.jsx (2)

10-10: Proper integration of machine capabilities

Good use of computed signals to derive pressure availability from machine capabilities.


91-97: Correctly propagates pressure capability to form

The pressureAvailable prop is properly passed to StandardProfileForm, enabling conditional UI features based on machine capabilities.

web/src/pages/ProfileEdit/StandardProfileForm.jsx (2)

222-265: Excellent UI improvements for pump and valve controls

The refactored segmented controls provide a much cleaner interface than checkboxes. The conditional rendering of pressure/flow modes based on machine capabilities is well implemented.


283-316: Well-designed parameter inputs with clear labeling

Excellent implementation of dynamic labels that clearly indicate whether pressure/flow values are targets or limits based on the selected mode. The input constraints and step values are appropriate.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3e753ab and 6d8c934.

📒 Files selected for processing (1)
  • web/src/pages/ProfileEdit/StandardProfileForm.jsx (6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: deploy
  • GitHub Check: test
  • GitHub Check: test
🔇 Additional comments (8)
web/src/pages/ProfileEdit/StandardProfileForm.jsx (8)

3-4: Good import organization and CSS addition.

The addition of isNumber utility from chart.js/helpers and the CSS import for styling are appropriate for the new mode selector functionality.


6-7: Props destructuring correctly includes new pressureAvailable prop.

The addition of pressureAvailable = false with a default value ensures backward compatibility while enabling the new pressure/flow control modes.


112-112: Correct propagation of pressureAvailable prop to Phase components.

The pressureAvailable prop is properly passed down to each Phase component to enable conditional rendering of pressure/flow modes.


134-134: Phase function signature updated correctly.

The Phase component signature now includes the pressureAvailable prop with proper destructuring.


189-189: Good standardization of tooltip attributes.

Changing from tooltip to data-tooltip follows HTML5 data attribute conventions and is more semantic.


221-235: Valve control refactored to use mode selector pattern.

The change from checkbox to a two-option selector ("Closed"/"Open") provides better UX consistency with the new pump mode selector. The implementation correctly handles the boolean valve state.


236-264: Well-structured pump mode selector with conditional rendering.

The segmented control for pump modes is well implemented:

  • Clear mode states (off, power, pressure, flow)
  • Conditional rendering based on pressureAvailable
  • Proper click handlers that prevent unnecessary state changes
  • Good visual indicators with "PRO" superscript for advanced modes

265-281: Power mode input handling is correct.

The numeric input for pump power properly handles the range (0-100) with appropriate step and constraints. The parseFloat conversion is appropriate for numeric inputs.

Comment on lines +163 to +166
const pumpPower = isNumber(phase.pump) ? phase.pump : 100;
const pressure = !isNumber(phase.pump) ? phase.pump.pressure : 0;
const flow = !isNumber(phase.pump) ? phase.pump.flow : 0;
const mode = isNumber(phase.pump) ? (phase.pump === 0 ? 'off' : 'power') : phase.pump.target;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical issue: Potential runtime errors in pump mode detection logic.

The logic for determining pump modes has several potential issues:

  1. Line 163: isNumber(phase.pump) ? phase.pump : 100 - if phase.pump is not a number, it defaults to 100, but then uses this value as power
  2. Lines 164-165: Access phase.pump.pressure and phase.pump.flow without null checks
  3. Line 166: Complex nested ternary for mode detection could fail if phase.pump is null/undefined

Apply this defensive programming approach:

-  const pumpPower = isNumber(phase.pump) ? phase.pump : 100;
-  const pressure = !isNumber(phase.pump) ? phase.pump.pressure : 0;
-  const flow = !isNumber(phase.pump) ? phase.pump.flow : 0;
-  const mode = isNumber(phase.pump) ? (phase.pump === 0 ? 'off' : 'power') : phase.pump.target;
+  const pumpPower = isNumber(phase.pump) ? phase.pump : 100;
+  const pressure = !isNumber(phase.pump) && phase.pump ? phase.pump.pressure || 0 : 0;
+  const flow = !isNumber(phase.pump) && phase.pump ? phase.pump.flow || 0 : 0;
+  const mode = isNumber(phase.pump) 
+    ? (phase.pump === 0 ? 'off' : 'power') 
+    : (phase.pump && phase.pump.target ? phase.pump.target : 'off');
📝 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.

Suggested change
const pumpPower = isNumber(phase.pump) ? phase.pump : 100;
const pressure = !isNumber(phase.pump) ? phase.pump.pressure : 0;
const flow = !isNumber(phase.pump) ? phase.pump.flow : 0;
const mode = isNumber(phase.pump) ? (phase.pump === 0 ? 'off' : 'power') : phase.pump.target;
const pumpPower = isNumber(phase.pump) ? phase.pump : 100;
const pressure = !isNumber(phase.pump) && phase.pump ? phase.pump.pressure || 0 : 0;
const flow = !isNumber(phase.pump) && phase.pump ? phase.pump.flow || 0 : 0;
const mode = isNumber(phase.pump)
? (phase.pump === 0 ? 'off' : 'power')
: (phase.pump && phase.pump.target ? phase.pump.target : 'off');
🤖 Prompt for AI Agents
In web/src/pages/ProfileEdit/StandardProfileForm.jsx around lines 163 to 166,
the current logic assumes phase.pump is either a number or an object without
null checks, leading to potential runtime errors. Refactor by first verifying
phase.pump is defined and not null before accessing its properties. Use explicit
checks to distinguish between number and object types, and provide safe default
values. Simplify the mode detection logic by handling null/undefined cases
upfront to avoid nested ternaries that can fail.

</span>
<span
className={`mode-selector ${mode === 'power' && 'selected'}`}
onClick={() => mode !== 'power' && onFieldChange('pump', 100)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Potential issue: Mode change logic may cause state inconsistency.

The condition mode !== 'power' prevents setting pump to 100 when already in power mode, but clicking "Power" when pump is already at a different power value (e.g., 50) won't trigger the onChange. This could confuse users.

Consider always allowing mode changes to ensure consistent behavior:

-            onClick={() => mode !== 'power' && onFieldChange('pump', 100)}
+            onClick={() => onFieldChange('pump', mode === 'power' ? pumpPower : 100)}

-                onClick={() => mode !== 'pressure' && onFieldChange('pump', { target: 'pressure', pressure: 0, flow: 0 })}
+                onClick={() => onFieldChange('pump', { target: 'pressure', pressure: pressure, flow: flow })}

-                onClick={() => mode !== 'flow' && onFieldChange('pump', { target: 'flow', pressure: 0, flow: 0 })}
+                onClick={() => onFieldChange('pump', { target: 'flow', pressure: pressure, flow: flow })}

Also applies to: 252-252, 258-258

🤖 Prompt for AI Agents
In web/src/pages/ProfileEdit/StandardProfileForm.jsx at lines 244, 252, and 258,
the onClick handlers conditionally prevent mode changes when mode is 'power',
which can cause state inconsistencies if the pump value differs. Remove the
condition `mode !== 'power'` to always call onFieldChange and update the pump
value on click, ensuring consistent state updates and user experience.

Comment on lines +282 to +316
{(mode === 'pressure' || mode === 'flow') && (
<>
<div className="col-span-12 md:col-span-6 flex flex-col">
<label className="block mb-2 text-sm font-medium text-gray-900 dark:text-gray-300">
Pressure {mode === 'pressure' ? 'Target' : 'Limit'}
</label>
<div className="flex">
<input
className="input-field"
type="number"
step="0.01"
value={pressure}
onChange={(e) => onFieldChange('pump', { ...phase.pump, pressure: parseFloat(e.target.value) })}
/>
<span className="input-addition">bar</span>
</div>
</div>
<div className="col-span-12 md:col-span-6 flex flex-col">
<label className="block mb-2 text-sm font-medium text-gray-900 dark:text-gray-300">
Flow {mode === 'flow' ? 'Target' : 'Limit'}
</label>
<div className="flex">
<input
className="input-field"
type="number"
step="0.01"
value={flow}
onChange={(e) => onFieldChange('pump', { ...phase.pump, flow: parseFloat(e.target.value) })}
/>
<span className="input-addition">g/s</span>
</div>
</div>
</>
)}
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Pressure and flow inputs implement good UX patterns.

The conditional rendering and dynamic labels ("Target" vs "Limit") based on mode provide clear user guidance. The input handling correctly preserves the existing pump object structure while updating specific properties.

However, there's a potential issue with object mutation:

The spread operator usage { ...phase.pump, ... } assumes phase.pump is always an object when in pressure/flow modes. Add safety check:

-                  onChange={(e) => onFieldChange('pump', { ...phase.pump, pressure: parseFloat(e.target.value) })}
+                  onChange={(e) => onFieldChange('pump', { ...(phase.pump || {}), pressure: parseFloat(e.target.value) })}

-                  onChange={(e) => onFieldChange('pump', { ...phase.pump, flow: parseFloat(e.target.value) })}
+                  onChange={(e) => onFieldChange('pump', { ...(phase.pump || {}), flow: parseFloat(e.target.value) })}
📝 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.

Suggested change
{(mode === 'pressure' || mode === 'flow') && (
<>
<div className="col-span-12 md:col-span-6 flex flex-col">
<label className="block mb-2 text-sm font-medium text-gray-900 dark:text-gray-300">
Pressure {mode === 'pressure' ? 'Target' : 'Limit'}
</label>
<div className="flex">
<input
className="input-field"
type="number"
step="0.01"
value={pressure}
onChange={(e) => onFieldChange('pump', { ...phase.pump, pressure: parseFloat(e.target.value) })}
/>
<span className="input-addition">bar</span>
</div>
</div>
<div className="col-span-12 md:col-span-6 flex flex-col">
<label className="block mb-2 text-sm font-medium text-gray-900 dark:text-gray-300">
Flow {mode === 'flow' ? 'Target' : 'Limit'}
</label>
<div className="flex">
<input
className="input-field"
type="number"
step="0.01"
value={flow}
onChange={(e) => onFieldChange('pump', { ...phase.pump, flow: parseFloat(e.target.value) })}
/>
<span className="input-addition">g/s</span>
</div>
</div>
</>
)}
</div>
{(mode === 'pressure' || mode === 'flow') && (
<>
<div className="col-span-12 md:col-span-6 flex flex-col">
<label className="block mb-2 text-sm font-medium text-gray-900 dark:text-gray-300">
Pressure {mode === 'pressure' ? 'Target' : 'Limit'}
</label>
<div className="flex">
<input
className="input-field"
type="number"
step="0.01"
value={pressure}
onChange={(e) => onFieldChange('pump', { ...(phase.pump || {}), pressure: parseFloat(e.target.value) })}
/>
<span className="input-addition">bar</span>
</div>
</div>
<div className="col-span-12 md:col-span-6 flex flex-col">
<label className="block mb-2 text-sm font-medium text-gray-900 dark:text-gray-300">
Flow {mode === 'flow' ? 'Target' : 'Limit'}
</label>
<div className="flex">
<input
className="input-field"
type="number"
step="0.01"
value={flow}
onChange={(e) => onFieldChange('pump', { ...(phase.pump || {}), flow: parseFloat(e.target.value) })}
/>
<span className="input-addition">g/s</span>
</div>
</div>
</>
)}
</div>
🤖 Prompt for AI Agents
In web/src/pages/ProfileEdit/StandardProfileForm.jsx around lines 282 to 316,
the code uses the spread operator on phase.pump assuming it is always an object,
which may cause errors if phase.pump is undefined or null. To fix this, add a
safety check by defaulting phase.pump to an empty object when spreading, for
example using {...(phase.pump || {}), pressure: ...} and similarly for flow,
ensuring no runtime errors occur due to undefined values.

@jniebuhr jniebuhr merged commit 3167131 into master Jul 26, 2025
5 checks passed
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@jniebuhr jniebuhr deleted the feature/flow-control branch July 26, 2025 15:21
@jniebuhr jniebuhr added this to the v1.6.0 milestone Aug 17, 2025
mikld86 pushed a commit to mikld86/GMTiredandForked that referenced this pull request Sep 3, 2025
* feat: Allow flow control with pressure limit

* fix: Cleanup unused code

* feat: Allow controlling flow from profile

* fix: Add codereview feedback

* fix: Fix unused variable

* fix: Fix flow control to use pump not puck flow

* feat: Add UI for flow

* fix: Small fixes to pressure controller

* fix: Remove console.log

* fix: Fix wrong attribute

* fix: Fix volumetric target
KT-0001 pushed a commit to KT-0001/gaggimate that referenced this pull request Nov 30, 2025
* feat: Allow flow control with pressure limit

* fix: Cleanup unused code

* feat: Allow controlling flow from profile

* fix: Add codereview feedback

* fix: Fix unused variable

* fix: Fix flow control to use pump not puck flow

* feat: Add UI for flow

* fix: Small fixes to pressure controller

* fix: Remove console.log

* fix: Fix wrong attribute

* fix: Fix volumetric target
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant