Added Water Level calibrate button, Tank Empty indicator, fixed sched…#22
Open
say-paul wants to merge 1 commit intobuildybee:mainfrom
Open
Added Water Level calibrate button, Tank Empty indicator, fixed sched…#22say-paul wants to merge 1 commit intobuildybee:mainfrom
say-paul wants to merge 1 commit intobuildybee:mainfrom
Conversation
…ule page to render the list of schedules
Reviewer's GuideImplements a reusable tank status indicator component wired to new MQTT 'tank_empty' topics across Device, Scheduler, Control, and Login pages, adds a calibration command button on the device list, normalizes scheduler enable flags, and updates project dependencies for Expo and React Native tooling. Sequence diagram for water level calibration command from DevicePagesequenceDiagram
actor User
participant DevicePage
participant PahoMqttClient
participant IoTDevice
User->>DevicePage: Tap calibrateButton for deviceId
DevicePage->>DevicePage: handleCalibrateDevice(deviceId)
DevicePage->>DevicePage: check mqttClient.isConnected
alt mqtt not connected
DevicePage->>User: Alert Error MQTT connection not available
else mqtt connected
DevicePage->>DevicePage: setCalibratingDeviceId(deviceId)
DevicePage->>PahoMqttClient: create Message payload 1
DevicePage->>PahoMqttClient: send topic deviceId/calibrate
PahoMqttClient-->>IoTDevice: MQTT publish deviceId/calibrate payload 1
DevicePage->>User: Alert Calibration Started
Note over DevicePage: 10s calibration + 2s buffer timer
DevicePage-->>DevicePage: setTimeout 12s
DevicePage->>DevicePage: setCalibratingDeviceId(null)
DevicePage->>User: Alert Calibration Complete
end
Sequence diagram for tank_empty MQTT status to TankIndicatorsequenceDiagram
participant IoTDevice
participant MqttBroker
participant MqttClient
participant PageComponent
participant TankIndicator
IoTDevice->>MqttBroker: Publish deviceId/tank_empty payload 1 or 0
MqttBroker-->>MqttClient: MQTT message deviceId/tank_empty
MqttClient-->>PageComponent: onMessageArrived message
PageComponent->>PageComponent: handleMqttMessage(message)
PageComponent->>PageComponent: parseStringPayload payloadString
PageComponent->>PageComponent: setTankStatus(prev merge deviceId isEmpty)
PageComponent-->>TankIndicator: Render with isEmpty tankStatus[deviceId]
TankIndicator->>TankIndicator: getIndicatorConfig based on isEmpty
TankIndicator-->>User: Visual tank status EMPTY NOT_EMPTY UNKNOWN
Class diagram for TankIndicator and updated page componentsclassDiagram
class TankIndicator {
+boolean isEmpty
+boolean showLabel
+string size
+getIndicatorConfig()
+getDimensions()
+render()
}
class DevicePage {
+object mqttClient
+set calibratingDeviceId
+object tankStatus
+handleMqttMessage(message)
+handleCalibrateDevice(deviceId)
+renderDeviceItem(deviceItem)
+render()
}
class SchedulerPage {
+object schedules
+object deviceStatus
+object tankStatus
+handleMqttMessage(message)
+parseSchedulesResponse(payload)
+render()
}
class ControlPage {
+object currentDevice
+object deviceStatus
+object tankStatus
+handleMqttMessage(message)
+render()
}
class LoginPage {
+render()
}
DevicePage --> TankIndicator : uses
SchedulerPage --> TankIndicator : uses
ControlPage --> TankIndicator : uses
LoginPage --> TankIndicator : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- In
SchedulerPagethe parsing logic for schedules now hard-codesen: 1and ignores the incoming enabled flag (both for string and object payloads); if devices can send disabled schedules this will incorrectly treat them as enabled and may need to preserve the original flag. - The
tank_emptyMQTT handling is duplicated acrossDevicePage,SchedulerPage, andControlPage; consider extracting a small shared helper or hook (e.g.useTankStatusFromMqtt) to centralize the parsing and state update logic. - Adding
npxas an app dependency inpackage.jsonis unusual sincenpxis normally provided by the Node toolchain rather than bundled into the React Native app; you may want to remove it unless there is a very specific runtime need.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SchedulerPage` the parsing logic for schedules now hard-codes `en: 1` and ignores the incoming enabled flag (both for string and object payloads); if devices can send disabled schedules this will incorrectly treat them as enabled and may need to preserve the original flag.
- The `tank_empty` MQTT handling is duplicated across `DevicePage`, `SchedulerPage`, and `ControlPage`; consider extracting a small shared helper or hook (e.g. `useTankStatusFromMqtt`) to centralize the parsing and state update logic.
- Adding `npx` as an app dependency in `package.json` is unusual since `npx` is normally provided by the Node toolchain rather than bundled into the React Native app; you may want to remove it unless there is a very specific runtime need.
## Individual Comments
### Comment 1
<location path="components/TankIndicator.js" line_range="21-30" />
<code_context>
+ // Determine icon, color, and label based on isEmpty state
+ const getIndicatorConfig = () => {
+ if (isEmpty === null || isEmpty === undefined) {
+ return {
+ icon: 'help-outline',
+ color: '#9CA3AF',
+ label: 'UNKNOWN',
+ backgroundColor: 'rgba(156, 163, 175, 0.1)',
+ };
+ }
+
+ if (isEmpty) {
+ return {
+ icon: 'water-off',
+ color: '#F44336',
+ label: 'EMPTY',
</code_context>
<issue_to_address>
**issue (bug_risk):** The Material icon names 'water' and 'water-off' may not exist in MaterialIcons and could fail at runtime.
These names are used with `MaterialIcons` from `@expo/vector-icons`, but `water` / `water-off` appear to belong to `MaterialCommunityIcons` instead. Please verify they exist in `MaterialIcons`; if not, either change to valid `MaterialIcons` names or switch the import to an icon set that includes them.
</issue_to_address>
### Comment 2
<location path="components/DevicePage.js" line_range="639-643" />
<code_context>
+ // Show success message
+ Alert.alert('Calibration Started', `10-second pump calibration started for ${deviceId}`);
+
+ // Clear calibrating state after 12 seconds (10s calibration + 2s buffer)
+ setTimeout(() => {
+ setCalibratingDeviceId(null);
+ Alert.alert('Calibration Complete', 'Device calibration finished. Current threshold has been auto-set.');
+ }, 12000);
+ } catch (error) {
+ console.error('DevicePage: Error sending calibration command:', error);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Calibration timeout is not cleaned up and may conflict with multiple calibrations or component unmount.
The timeout that clears `calibratingDeviceId` and shows the completion alert is created on every `handleCalibrateDevice` call and never cleared. This can:
1. Fire after a newer calibration has started, clearing state and showing "Calibration Complete" at the wrong time.
2. Fire after unmount, causing state updates on an unmounted component and flaky alerts.
Store the timeout ID in a `useRef`, clear any existing timeout before setting a new one, and clean it up in a `useEffect` cleanup on unmount.
Suggested implementation:
```javascript
// Clear calibrating state after 12 seconds (10s calibration + 2s buffer)
if (calibrationTimeoutRef.current) {
clearTimeout(calibrationTimeoutRef.current);
}
calibrationTimeoutRef.current = setTimeout(() => {
setCalibratingDeviceId((currentDeviceId) => {
// Only clear the calibrating state if it's still this device
if (currentDeviceId === deviceId) {
Alert.alert(
'Calibration Complete',
'Device calibration finished. Current threshold has been auto-set.',
);
return null;
}
return currentDeviceId;
});
}, 12000);
```
To fully implement the suggestion, you should also:
1. **Import and declare the ref:**
- At the top of the file, ensure `useRef` is imported:
- `import React, { useRef, ... } from 'react';`
- Inside the `DevicePage` component (or the relevant component where `handleCalibrateDevice` lives), add:
- `const calibrationTimeoutRef = useRef(null);`
2. **Clean up on unmount:**
- Inside the same component, add a `useEffect` with a cleanup function:
```js
useEffect(() => {
return () => {
if (calibrationTimeoutRef.current) {
clearTimeout(calibrationTimeoutRef.current);
}
};
}, []);
```
3. **Optional: clear timeout on explicit calibration cancel/finish paths:**
- If there are other code paths that end calibration early (e.g., error handlers, manual cancel buttons), consider also clearing `calibrationTimeoutRef.current` and setting it to `null` there, to avoid stale timers.
</issue_to_address>
### Comment 3
<location path="package.json" line_range="37-40" />
<code_context>
"expo-secure-store": "~15.0.7",
"expo-status-bar": "~3.0.8",
"expo-updates": "~29.0.12",
+ "npx": "^10.2.2",
"paho-mqtt": "^1.1.0",
"react": "19.1.0",
</code_context>
<issue_to_address>
**suggestion:** Adding `npx` as a runtime dependency is unusual and may not be necessary.
Since `npx` is normally supplied by the Node.js/npm toolchain and not used at runtime in app code, including it in `dependencies` will ship it in production builds (e.g. Expo EAS) and may introduce unnecessary bloat or version conflicts. If it’s not required by the app at runtime, move it to `devDependencies` or remove it.
```suggestion
"expo-status-bar": "~3.0.8",
"expo-updates": "~29.0.12",
"paho-mqtt": "^1.1.0",
```
</issue_to_address>
### Comment 4
<location path="components/DevicePage.js" line_range="85" />
<code_context>
+ const [tankStatus, setTankStatus] = useState({});
+
// Handle MQTT version messages
const handleMqttMessage = useCallback((message) => {
const topic = message.destinationName;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the new tank-empty handling, derived TankIndicator state, and calibration MQTT logic into small helper functions to keep DevicePage simpler and more readable while preserving behavior.
You can keep all new behavior while reducing local complexity with a couple of small extra helpers.
### 1. Extract tank-empty handling into a helper
The `/tank_empty` branch is simple but now duplicated across components. Even if you don’t yet want a shared hook, you can at least pull the parsing into a small, testable function:
```js
// near top of file
const handleTankEmptyMessage = (message, setTankStatus) => {
const topic = message.destinationName;
const deviceId = parseDeviceIdFromTopic(topic);
if (!topic.endsWith('/tank_empty') || !deviceId) return;
const payload = parseStringPayload(message.payloadString);
const isEmpty = payload === '1';
setTankStatus(prev => ({ ...prev, [deviceId]: isEmpty }));
console.log(`DevicePage: Tank status for ${deviceId}: ${isEmpty ? 'EMPTY' : 'NOT EMPTY'}`);
};
```
Then in `handleMqttMessage`:
```js
const handleMqttMessage = useCallback((message) => {
const topic = message.destinationName;
const deviceId = parseDeviceIdFromTopic(topic);
if (topic.endsWith('/version') && deviceId) {
const version = parseStringPayload(message.payloadString);
if (version) {
updateFirmwareVersion(deviceId, version);
console.log(`DevicePage: Updated firmware version for ${deviceId}: ${version}`);
}
}
handleTankEmptyMessage(message, setTankStatus);
}, [updateFirmwareVersion]);
```
If `ControlPage` / `SchedulerPage` do the same, this helper can be moved to a shared module later without changing behavior.
### 2. Extract derived state for `TankIndicator`
The inline logic in JSX is a bit dense. A small helper keeps the render tree simpler:
```js
// helper inside component
const getActiveDeviceTankEmpty = (devices, tankStatus) => {
if (devices.length === 0) return null;
const activeDevice = devices.find(d => d.active);
if (!activeDevice) return null;
return tankStatus[activeDevice.id];
};
```
Use it in JSX:
```jsx
<View style={styles.deviceListTitleRow}>
<Text style={styles.deviceListTitle}>My Devices</Text>
<TankIndicator
isEmpty={getActiveDeviceTankEmpty(devices, tankStatus)}
showLabel={true}
size="small"
/>
</View>
```
This keeps the logic equivalent but easier to read.
### 3. Split calibration into a small MQTT helper
`handleCalibrateDevice` currently mixes UI alerts, MQTT message construction, and timeout in one block. You can move the send + timeout into a focused helper while leaving UI behavior unchanged.
```js
// helper inside the file (or shared later)
const sendCalibrationCommand = (client, deviceId, { onStart, onComplete, onError }) => {
const topic = `${deviceId}/calibrate`;
const payload = '1';
try {
const message = new Paho.Message(payload);
message.destinationName = topic;
message.qos = 1;
message.retained = false;
client.send(message);
console.log(`DevicePage: Sent calibration command to ${topic}`);
onStart?.();
setTimeout(() => {
onComplete?.();
}, 12000);
} catch (error) {
console.error('DevicePage: Error sending calibration command:', error);
onError?.(error);
}
};
```
Then `handleCalibrateDevice` becomes more declarative:
```js
const handleCalibrateDevice = (deviceId) => {
if (!mqttClient || !mqttClient.isConnected()) {
Alert.alert('Error', 'MQTT connection not available. Please try again.');
return;
}
setCalibratingDeviceId(deviceId);
sendCalibrationCommand(mqttClient, deviceId, {
onStart: () => {
Alert.alert('Calibration Started', `10-second pump calibration started for ${deviceId}`);
},
onComplete: () => {
setCalibratingDeviceId(null);
Alert.alert(
'Calibration Complete',
'Device calibration finished. Current threshold has been auto-set.'
);
},
onError: () => {
setCalibratingDeviceId(null);
Alert.alert('Error', 'Failed to send calibration command');
},
});
};
```
This preserves all existing behavior (alerts, timeout, state updates), but `DevicePage` becomes easier to scan and reason about.
</issue_to_address>
### Comment 5
<location path="components/SchedulerPage.js" line_range="44" />
<code_context>
const [notificationPermission, setNotificationPermission] = useState(false);
const [currentDevice, setCurrentDevice] = useState(null); // Now stores device object
const [deviceStatus, setDeviceStatus] = useState({}); // Per-device online/offline status
+ const [tankStatus, setTankStatus] = useState({}); // Per-device tank status
const [mqttConnected, setMqttConnected] = useState(false);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared tank-status handling into a hook and moving the tank/next-run header into its own component to keep SchedulerPage simpler and more focused.
You can reduce the added complexity by centralising the tank status handling and extracting the header layout into a small component.
### 1. Share `tank_empty` handling instead of duplicating it
Right now this page has its own `topic.endsWith('/tank_empty')` branch and state. If `DevicePage` and `ControlPage` do the same, you can move this into a small shared hook so each page just consumes `tankStatus` and doesn’t care about MQTT parsing.
Example:
```js
// useTankStatus.js
import { useState, useCallback } from 'react';
import { parseStringPayload } from './tools';
export function useTankStatus() {
const [tankStatus, setTankStatus] = useState({});
const handleTankEmptyMessage = useCallback((topic, payload, deviceId) => {
if (!topic.endsWith('/tank_empty') || !deviceId) return false;
const payloadStr = parseStringPayload(payload);
const isEmpty = payloadStr === '1';
setTankStatus(prev => ({ ...prev, [deviceId]: isEmpty }));
return true; // indicate that we handled it
}, []);
return { tankStatus, handleTankEmptyMessage };
}
```
Then in `SchedulerPage` (and the other pages):
```js
import { useTankStatus } from './useTankStatus';
// ...
const { tankStatus, handleTankEmptyMessage } = useTankStatus();
const mqttMessageHandler = useCallback((topic, payload) => {
const deviceId = parseDeviceIdFromTopic(topic);
// let shared handler consume tank_empty, early return if handled
if (handleTankEmptyMessage(topic, payload, deviceId)) {
return;
}
// existing logic for other topics
if (topic.endsWith(SCHEDULER_TOPICS.GET_SCHEDULE)) {
// ...
}
}, [handleTankEmptyMessage, updateDeviceStatus]);
```
The other pages only need to import the same hook and use `tankStatus[device.id]`, removing the repeated parsing/updating logic.
### 2. Extract a small header component for tank + next-run
You can wrap the tank indicator + next run touchable into a focused component to keep `SchedulerPage` lean, without changing behaviour:
```js
// DeviceHeaderStatus.js
import React from 'react';
import { View, TouchableOpacity, Text } from 'react-native';
import { MaterialIcons } from '@expo/vector-icons';
import TankIndicator from './TankIndicator';
export function DeviceHeaderStatus({
currentDevice,
deviceAvailable,
tankStatus,
refreshingNextRun,
nextRunTime,
requestNextRunTime,
styles,
}) {
if (!currentDevice) return null;
return (
<>
<View style={styles.tankStatusContainer}>
<TankIndicator
isEmpty={tankStatus[currentDevice.id]}
showLabel={true}
size="medium"
/>
</View>
{deviceAvailable && (
<TouchableOpacity
style={[
styles.nextRunContainer,
refreshingNextRun && styles.nextRunContainerRefreshing,
]}
onPress={requestNextRunTime}
disabled={refreshingNextRun || !deviceAvailable}
>
<View style={styles.nextRunIcon}>
{refreshingNextRun ? (
<MaterialIcons
name="refresh"
size={20}
color="#5E72E4"
style={styles.refreshingIcon}
/>
) : (
<MaterialIcons
name="schedule"
size={20}
color="#5E72E4"
/>
)}
</View>
<View style={styles.nextRunTextContainer}>
<Text style={styles.nextRunLabel}>Next Run</Text>
<Text style={styles.nextRunTime}>
{refreshingNextRun ? 'Refreshing...' : formatNextRunTime(nextRunTime)}
</Text>
</View>
</TouchableOpacity>
)}
</>
);
}
```
Use it in `SchedulerPage`:
```js
import { DeviceHeaderStatus } from './DeviceHeaderStatus';
// ...
<DeviceSelector
devices={storedDevices}
currentDevice={currentDevice}
deviceStatus={deviceStatus}
onSelectDevice={switchDevice}
/>
<DeviceHeaderStatus
currentDevice={currentDevice}
deviceAvailable={deviceAvailable}
tankStatus={tankStatus}
refreshingNextRun={refreshingNextRun}
nextRunTime={nextRunTime}
requestNextRunTime={requestNextRunTime}
styles={styles}
/>
```
This removes layout concerns from `SchedulerPage` and keeps all existing behaviour.
</issue_to_address>
### Comment 6
<location path="components/ControlPage.js" line_range="233" />
<code_context>
}
}
}
+ // Handle tank_empty messages
+ else if (topic.endsWith('/tank_empty')) {
+ if (deviceId) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing the tank status message handling and extracting the selector + tank indicator into a small header component to keep ControlPage simpler and less repetitive.
You can reduce the added complexity by (1) centralizing the `tank_empty` handling and (2) extracting the header (selector + tank indicator) into a small child component.
### 1. Centralize tank status handling
Right now each page re-implements:
```js
const payload = parseStringPayload(message.payloadString);
const isEmpty = payload === '1';
setTankStatus(prev => ({ ...prev, [deviceId]: isEmpty }));
```
Create a small shared helper (e.g. in `services/mqtt` or a new `services/tankStatus.js`) and reuse it across pages:
```js
// services/tankStatus.js
import { parseStringPayload } from '../components/tools';
export const handleTankEmptyMessage = (message, deviceId, setTankStatus, logPrefix = 'Tank') => {
if (!deviceId) return;
const payload = parseStringPayload(message.payloadString);
const isEmpty = payload === '1';
setTankStatus(prev => ({ ...prev, [deviceId]: isEmpty }));
console.log(`${logPrefix}: Tank status for ${deviceId}: ${isEmpty ? 'EMPTY' : 'NOT EMPTY'}`);
};
```
Then in `ControlPage`:
```js
import { handleTankEmptyMessage } from '../services/tankStatus';
// ...
else if (topic.endsWith('/tank_empty')) {
handleTankEmptyMessage(message, deviceId, setTankStatus, 'ControlPage');
}
```
You can apply the same helper in `DevicePage` and `SchedulerPage` so they no longer have to duplicate the parsing/state update logic.
### 2. Extract a small header component
To keep `ControlPage` focused, move the selector + tank status into a tiny child component. This keeps behavior intact but shrinks the main component visually:
```js
// ControlHeader.js
import React from 'react';
import { View, StyleSheet } from 'react-native';
import DeviceSelector from './DeviceSelector';
import TankIndicator from './TankIndicator';
const ControlHeader = ({ devices, currentDevice, deviceStatus, onSelectDevice, tankStatus }) => (
<>
<DeviceSelector
devices={devices}
currentDevice={currentDevice}
deviceStatus={deviceStatus}
onSelectDevice={onSelectDevice}
/>
{currentDevice && (
<View style={styles.tankStatusContainer}>
<TankIndicator
isEmpty={tankStatus[currentDevice.id]}
showLabel={true}
size="medium"
/>
</View>
)}
</>
);
const styles = StyleSheet.create({
tankStatusContainer: {
marginHorizontal: 16,
marginTop: 12,
marginBottom: 12,
alignItems: 'flex-start',
},
});
export default ControlHeader;
```
And in `ControlPage`:
```js
import ControlHeader from './ControlHeader';
// ...
<ControlHeader
devices={storedDevices}
currentDevice={currentDevice}
deviceStatus={deviceStatus}
onSelectDevice={switchDevice}
tankStatus={tankStatus}
/>
```
This keeps all functionality the same while reducing duplication and making `ControlPage` easier to scan.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+21
to
+30
| return { | ||
| icon: 'help-outline', | ||
| color: '#9CA3AF', | ||
| label: 'UNKNOWN', | ||
| backgroundColor: 'rgba(156, 163, 175, 0.1)', | ||
| }; | ||
| } | ||
|
|
||
| if (isEmpty) { | ||
| return { |
There was a problem hiding this comment.
issue (bug_risk): The Material icon names 'water' and 'water-off' may not exist in MaterialIcons and could fail at runtime.
These names are used with MaterialIcons from @expo/vector-icons, but water / water-off appear to belong to MaterialCommunityIcons instead. Please verify they exist in MaterialIcons; if not, either change to valid MaterialIcons names or switch the import to an icon set that includes them.
Comment on lines
+639
to
+643
| // Clear calibrating state after 12 seconds (10s calibration + 2s buffer) | ||
| setTimeout(() => { | ||
| setCalibratingDeviceId(null); | ||
| Alert.alert('Calibration Complete', 'Device calibration finished. Current threshold has been auto-set.'); | ||
| }, 12000); |
There was a problem hiding this comment.
suggestion (bug_risk): Calibration timeout is not cleaned up and may conflict with multiple calibrations or component unmount.
The timeout that clears calibratingDeviceId and shows the completion alert is created on every handleCalibrateDevice call and never cleared. This can:
- Fire after a newer calibration has started, clearing state and showing "Calibration Complete" at the wrong time.
- Fire after unmount, causing state updates on an unmounted component and flaky alerts.
Store the timeout ID in a useRef, clear any existing timeout before setting a new one, and clean it up in a useEffect cleanup on unmount.
Suggested implementation:
// Clear calibrating state after 12 seconds (10s calibration + 2s buffer)
if (calibrationTimeoutRef.current) {
clearTimeout(calibrationTimeoutRef.current);
}
calibrationTimeoutRef.current = setTimeout(() => {
setCalibratingDeviceId((currentDeviceId) => {
// Only clear the calibrating state if it's still this device
if (currentDeviceId === deviceId) {
Alert.alert(
'Calibration Complete',
'Device calibration finished. Current threshold has been auto-set.',
);
return null;
}
return currentDeviceId;
});
}, 12000);To fully implement the suggestion, you should also:
-
Import and declare the ref:
- At the top of the file, ensure
useRefis imported:import React, { useRef, ... } from 'react';
- Inside the
DevicePagecomponent (or the relevant component wherehandleCalibrateDevicelives), add:const calibrationTimeoutRef = useRef(null);
- At the top of the file, ensure
-
Clean up on unmount:
- Inside the same component, add a
useEffectwith a cleanup function:useEffect(() => { return () => { if (calibrationTimeoutRef.current) { clearTimeout(calibrationTimeoutRef.current); } }; }, []);
- Inside the same component, add a
-
Optional: clear timeout on explicit calibration cancel/finish paths:
- If there are other code paths that end calibration early (e.g., error handlers, manual cancel buttons), consider also clearing
calibrationTimeoutRef.currentand setting it tonullthere, to avoid stale timers.
- If there are other code paths that end calibration early (e.g., error handlers, manual cancel buttons), consider also clearing
Comment on lines
37
to
40
| "expo-status-bar": "~3.0.8", | ||
| "expo-updates": "~29.0.12", | ||
| "npx": "^10.2.2", | ||
| "paho-mqtt": "^1.1.0", |
There was a problem hiding this comment.
suggestion: Adding npx as a runtime dependency is unusual and may not be necessary.
Since npx is normally supplied by the Node.js/npm toolchain and not used at runtime in app code, including it in dependencies will ship it in production builds (e.g. Expo EAS) and may introduce unnecessary bloat or version conflicts. If it’s not required by the app at runtime, move it to devDependencies or remove it.
Suggested change
| "expo-status-bar": "~3.0.8", | |
| "expo-updates": "~29.0.12", | |
| "npx": "^10.2.2", | |
| "paho-mqtt": "^1.1.0", | |
| "expo-status-bar": "~3.0.8", | |
| "expo-updates": "~29.0.12", | |
| "paho-mqtt": "^1.1.0", |
| const [tankStatus, setTankStatus] = useState({}); | ||
|
|
||
| // Handle MQTT version messages | ||
| const handleMqttMessage = useCallback((message) => { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the new tank-empty handling, derived TankIndicator state, and calibration MQTT logic into small helper functions to keep DevicePage simpler and more readable while preserving behavior.
You can keep all new behavior while reducing local complexity with a couple of small extra helpers.
1. Extract tank-empty handling into a helper
The /tank_empty branch is simple but now duplicated across components. Even if you don’t yet want a shared hook, you can at least pull the parsing into a small, testable function:
// near top of file
const handleTankEmptyMessage = (message, setTankStatus) => {
const topic = message.destinationName;
const deviceId = parseDeviceIdFromTopic(topic);
if (!topic.endsWith('/tank_empty') || !deviceId) return;
const payload = parseStringPayload(message.payloadString);
const isEmpty = payload === '1';
setTankStatus(prev => ({ ...prev, [deviceId]: isEmpty }));
console.log(`DevicePage: Tank status for ${deviceId}: ${isEmpty ? 'EMPTY' : 'NOT EMPTY'}`);
};Then in handleMqttMessage:
const handleMqttMessage = useCallback((message) => {
const topic = message.destinationName;
const deviceId = parseDeviceIdFromTopic(topic);
if (topic.endsWith('/version') && deviceId) {
const version = parseStringPayload(message.payloadString);
if (version) {
updateFirmwareVersion(deviceId, version);
console.log(`DevicePage: Updated firmware version for ${deviceId}: ${version}`);
}
}
handleTankEmptyMessage(message, setTankStatus);
}, [updateFirmwareVersion]);If ControlPage / SchedulerPage do the same, this helper can be moved to a shared module later without changing behavior.
2. Extract derived state for TankIndicator
The inline logic in JSX is a bit dense. A small helper keeps the render tree simpler:
// helper inside component
const getActiveDeviceTankEmpty = (devices, tankStatus) => {
if (devices.length === 0) return null;
const activeDevice = devices.find(d => d.active);
if (!activeDevice) return null;
return tankStatus[activeDevice.id];
};Use it in JSX:
<View style={styles.deviceListTitleRow}>
<Text style={styles.deviceListTitle}>My Devices</Text>
<TankIndicator
isEmpty={getActiveDeviceTankEmpty(devices, tankStatus)}
showLabel={true}
size="small"
/>
</View>This keeps the logic equivalent but easier to read.
3. Split calibration into a small MQTT helper
handleCalibrateDevice currently mixes UI alerts, MQTT message construction, and timeout in one block. You can move the send + timeout into a focused helper while leaving UI behavior unchanged.
// helper inside the file (or shared later)
const sendCalibrationCommand = (client, deviceId, { onStart, onComplete, onError }) => {
const topic = `${deviceId}/calibrate`;
const payload = '1';
try {
const message = new Paho.Message(payload);
message.destinationName = topic;
message.qos = 1;
message.retained = false;
client.send(message);
console.log(`DevicePage: Sent calibration command to ${topic}`);
onStart?.();
setTimeout(() => {
onComplete?.();
}, 12000);
} catch (error) {
console.error('DevicePage: Error sending calibration command:', error);
onError?.(error);
}
};Then handleCalibrateDevice becomes more declarative:
const handleCalibrateDevice = (deviceId) => {
if (!mqttClient || !mqttClient.isConnected()) {
Alert.alert('Error', 'MQTT connection not available. Please try again.');
return;
}
setCalibratingDeviceId(deviceId);
sendCalibrationCommand(mqttClient, deviceId, {
onStart: () => {
Alert.alert('Calibration Started', `10-second pump calibration started for ${deviceId}`);
},
onComplete: () => {
setCalibratingDeviceId(null);
Alert.alert(
'Calibration Complete',
'Device calibration finished. Current threshold has been auto-set.'
);
},
onError: () => {
setCalibratingDeviceId(null);
Alert.alert('Error', 'Failed to send calibration command');
},
});
};This preserves all existing behavior (alerts, timeout, state updates), but DevicePage becomes easier to scan and reason about.
| // State management | ||
| const [schedules, setSchedules] = useState({}); | ||
| const [deviceStatus, setDeviceStatus] = useState({}); // Per-device online/offline status | ||
| const [tankStatus, setTankStatus] = useState({}); // Per-device tank status |
There was a problem hiding this comment.
issue (complexity): Consider extracting shared tank-status handling into a hook and moving the tank/next-run header into its own component to keep SchedulerPage simpler and more focused.
You can reduce the added complexity by centralising the tank status handling and extracting the header layout into a small component.
1. Share tank_empty handling instead of duplicating it
Right now this page has its own topic.endsWith('/tank_empty') branch and state. If DevicePage and ControlPage do the same, you can move this into a small shared hook so each page just consumes tankStatus and doesn’t care about MQTT parsing.
Example:
// useTankStatus.js
import { useState, useCallback } from 'react';
import { parseStringPayload } from './tools';
export function useTankStatus() {
const [tankStatus, setTankStatus] = useState({});
const handleTankEmptyMessage = useCallback((topic, payload, deviceId) => {
if (!topic.endsWith('/tank_empty') || !deviceId) return false;
const payloadStr = parseStringPayload(payload);
const isEmpty = payloadStr === '1';
setTankStatus(prev => ({ ...prev, [deviceId]: isEmpty }));
return true; // indicate that we handled it
}, []);
return { tankStatus, handleTankEmptyMessage };
}Then in SchedulerPage (and the other pages):
import { useTankStatus } from './useTankStatus';
// ...
const { tankStatus, handleTankEmptyMessage } = useTankStatus();
const mqttMessageHandler = useCallback((topic, payload) => {
const deviceId = parseDeviceIdFromTopic(topic);
// let shared handler consume tank_empty, early return if handled
if (handleTankEmptyMessage(topic, payload, deviceId)) {
return;
}
// existing logic for other topics
if (topic.endsWith(SCHEDULER_TOPICS.GET_SCHEDULE)) {
// ...
}
}, [handleTankEmptyMessage, updateDeviceStatus]);The other pages only need to import the same hook and use tankStatus[device.id], removing the repeated parsing/updating logic.
2. Extract a small header component for tank + next-run
You can wrap the tank indicator + next run touchable into a focused component to keep SchedulerPage lean, without changing behaviour:
// DeviceHeaderStatus.js
import React from 'react';
import { View, TouchableOpacity, Text } from 'react-native';
import { MaterialIcons } from '@expo/vector-icons';
import TankIndicator from './TankIndicator';
export function DeviceHeaderStatus({
currentDevice,
deviceAvailable,
tankStatus,
refreshingNextRun,
nextRunTime,
requestNextRunTime,
styles,
}) {
if (!currentDevice) return null;
return (
<>
<View style={styles.tankStatusContainer}>
<TankIndicator
isEmpty={tankStatus[currentDevice.id]}
showLabel={true}
size="medium"
/>
</View>
{deviceAvailable && (
<TouchableOpacity
style={[
styles.nextRunContainer,
refreshingNextRun && styles.nextRunContainerRefreshing,
]}
onPress={requestNextRunTime}
disabled={refreshingNextRun || !deviceAvailable}
>
<View style={styles.nextRunIcon}>
{refreshingNextRun ? (
<MaterialIcons
name="refresh"
size={20}
color="#5E72E4"
style={styles.refreshingIcon}
/>
) : (
<MaterialIcons
name="schedule"
size={20}
color="#5E72E4"
/>
)}
</View>
<View style={styles.nextRunTextContainer}>
<Text style={styles.nextRunLabel}>Next Run</Text>
<Text style={styles.nextRunTime}>
{refreshingNextRun ? 'Refreshing...' : formatNextRunTime(nextRunTime)}
</Text>
</View>
</TouchableOpacity>
)}
</>
);
}Use it in SchedulerPage:
import { DeviceHeaderStatus } from './DeviceHeaderStatus';
// ...
<DeviceSelector
devices={storedDevices}
currentDevice={currentDevice}
deviceStatus={deviceStatus}
onSelectDevice={switchDevice}
/>
<DeviceHeaderStatus
currentDevice={currentDevice}
deviceAvailable={deviceAvailable}
tankStatus={tankStatus}
refreshingNextRun={refreshingNextRun}
nextRunTime={nextRunTime}
requestNextRunTime={requestNextRunTime}
styles={styles}
/>This removes layout concerns from SchedulerPage and keeps all existing behaviour.
| } | ||
| } | ||
| } | ||
| // Handle tank_empty messages |
There was a problem hiding this comment.
issue (complexity): Consider centralizing the tank status message handling and extracting the selector + tank indicator into a small header component to keep ControlPage simpler and less repetitive.
You can reduce the added complexity by (1) centralizing the tank_empty handling and (2) extracting the header (selector + tank indicator) into a small child component.
1. Centralize tank status handling
Right now each page re-implements:
const payload = parseStringPayload(message.payloadString);
const isEmpty = payload === '1';
setTankStatus(prev => ({ ...prev, [deviceId]: isEmpty }));Create a small shared helper (e.g. in services/mqtt or a new services/tankStatus.js) and reuse it across pages:
// services/tankStatus.js
import { parseStringPayload } from '../components/tools';
export const handleTankEmptyMessage = (message, deviceId, setTankStatus, logPrefix = 'Tank') => {
if (!deviceId) return;
const payload = parseStringPayload(message.payloadString);
const isEmpty = payload === '1';
setTankStatus(prev => ({ ...prev, [deviceId]: isEmpty }));
console.log(`${logPrefix}: Tank status for ${deviceId}: ${isEmpty ? 'EMPTY' : 'NOT EMPTY'}`);
};Then in ControlPage:
import { handleTankEmptyMessage } from '../services/tankStatus';
// ...
else if (topic.endsWith('/tank_empty')) {
handleTankEmptyMessage(message, deviceId, setTankStatus, 'ControlPage');
}You can apply the same helper in DevicePage and SchedulerPage so they no longer have to duplicate the parsing/state update logic.
2. Extract a small header component
To keep ControlPage focused, move the selector + tank status into a tiny child component. This keeps behavior intact but shrinks the main component visually:
// ControlHeader.js
import React from 'react';
import { View, StyleSheet } from 'react-native';
import DeviceSelector from './DeviceSelector';
import TankIndicator from './TankIndicator';
const ControlHeader = ({ devices, currentDevice, deviceStatus, onSelectDevice, tankStatus }) => (
<>
<DeviceSelector
devices={devices}
currentDevice={currentDevice}
deviceStatus={deviceStatus}
onSelectDevice={onSelectDevice}
/>
{currentDevice && (
<View style={styles.tankStatusContainer}>
<TankIndicator
isEmpty={tankStatus[currentDevice.id]}
showLabel={true}
size="medium"
/>
</View>
)}
</>
);
const styles = StyleSheet.create({
tankStatusContainer: {
marginHorizontal: 16,
marginTop: 12,
marginBottom: 12,
alignItems: 'flex-start',
},
});
export default ControlHeader;And in ControlPage:
import ControlHeader from './ControlHeader';
// ...
<ControlHeader
devices={storedDevices}
currentDevice={currentDevice}
deviceStatus={deviceStatus}
onSelectDevice={switchDevice}
tankStatus={tankStatus}
/>This keeps all functionality the same while reducing duplication and making ControlPage easier to scan.
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.
…ule page to render the list of schedules
Summary by Sourcery
Add water tank status visibility across app screens and introduce a calibration control for devices.
New Features:
Enhancements:
Build: