Conversation
…rs can see the password they entered. Especially important since we don't have them verify their password.
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
📝 WalkthroughWalkthroughThis PR refactors the Settings and PluginCard components by introducing reusable UI scaffolding (PluginWrapper, helper components, form utilities), replacing inline plugin sections with standardized compositions, switching autowakeup schedule management from useState to useReducer, and consolidating form handling with new export/import and validation capabilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (4)
web/src/pages/Settings/index.jsx (4)
318-320: Duplicate schedule serialization logic.The schedule-to-string conversion appears identically in both
onSubmit(line 318–320) andonExport(line 380–382). Extract a helper to keep this DRY.Proposed helper
+const serializeSchedules = schedules => + schedules.map(s => `${s.time}|${s.days.map(d => (d ? '1' : '0')).join('')}`).join(';');Then use
serializeSchedules(autowakeupSchedules)in both places.Also applies to: 380-382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Settings/index.jsx` around lines 318 - 320, Extract the duplicated schedule serialization into a new helper function (e.g., serializeSchedules) that accepts autowakeupSchedules and returns the string currently built in onSubmit/onExport; replace the inline logic in both onSubmit and onExport with calls to serializeSchedules(autowakeupSchedules) so both locations use the single helper and remove the duplicated mapping/join logic.
470-470: Redundant native form attributes.The
method='post'andaction='/api/settings'attributes are unused sinceonSubmitcallse.preventDefault()and handles submission viafetch. They could cause an unintended full-page POST ifpreventDefaultever fails to fire (e.g., ifonSubmitthrows synchronously before callingpreventDefault). Consider removing them or movinge.preventDefault()to the very first line of the handler (before theif (submitting) returnguard on line 307).Proposed fix
Option A — remove native attributes:
- <form key='settings' ref={formRef} method='post' action='/api/settings' onSubmit={onSubmit}> + <form key='settings' ref={formRef} onSubmit={onSubmit}>Option B — ensure
preventDefaultis always called first:async (e, restart = false) => { - if (e) e.preventDefault(); - if (submitting) return; + if (e) e.preventDefault(); + if (submitting) return;(Line 306 already does this, but also remove the native attributes to avoid confusion.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Settings/index.jsx` at line 470, Remove the redundant native form attributes or ensure preventDefault is always invoked first: either delete method='post' and action='/api/settings' from the <form ref={formRef} key='settings'> element to avoid accidental full-page POSTs, or update the onSubmit handler (the onSubmit function used by the form) to call e.preventDefault() as the very first statement before the submitting guard (the submitting state check) and any other logic; reference the form element with ref formRef and the onSubmit function to apply the change.
16-19: Shareddaysarray reference inDEFAULT_WAKEUP_SCHEDULE.
Array(7).fill(true)creates a single array instance. TheADDreducer case (line 190) correctly creates a copy with[...DEFAULT_WAKEUP_SCHEDULE.days], but theINITfallback (line 183) uses{ ...DEFAULT_WAKEUP_SCHEDULE, ... }which shares the samedaysreference. This is safe in practice since the reducer uses immutable updates via.map(), but for consistency and defensive coding, consider also copyingdaysinINIT.Suggested fix in autoWakeupReducer INIT case
case 'INIT': return action.payload?.length ? action.payload - : [{ ...DEFAULT_WAKEUP_SCHEDULE, id: crypto.randomUUID() }]; + : [{ ...DEFAULT_WAKEUP_SCHEDULE, days: [...DEFAULT_WAKEUP_SCHEDULE.days], id: crypto.randomUUID() }];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Settings/index.jsx` around lines 16 - 19, DEFAULT_WAKEUP_SCHEDULE's days array is currently shared because it's created with Array(7).fill(true) and in the autoWakeupReducer INIT case you spread DEFAULT_WAKEUP_SCHEDULE without copying days; modify the INIT branch of autoWakeupReducer to deep-copy the days array (e.g. set days: [...DEFAULT_WAKEUP_SCHEDULE.days] when constructing the initial state) so it doesn't share the same reference (note ADD already uses [...DEFAULT_WAKEUP_SCHEDULE.days]).
438-448: Minor a11y: addrole='button'to the import label.The keyboard handler is a nice touch, but screen readers won't announce this
<label>as an interactive element withoutrole='button'.Proposed fix
<label htmlFor='settingsImport' className='btn btn-ghost btn-sm cursor-pointer' title='Import Settings' tabIndex='0' + role='button' onKeyDown={e => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Settings/index.jsx` around lines 438 - 448, The label element used as a clickable import control (htmlFor='settingsImport', tabIndex='0', onKeyDown handler) is not announced as interactive by screen readers; add role='button' to that label to expose it as a button to assistive technologies while leaving the existing tabIndex and onKeyDown intact so keyboard interaction continues to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/pages/Settings/index.jsx`:
- Around line 378-396: onExport currently writes display-formatted values from
formData (including steamPumpPercentage) so a subsequent normalizeSettings on
import divides it again; update onExport to convert steamPumpPercentage back to
the firmware raw value before merging into finalExport (e.g. if a pressure
sensor is present, set steamPumpPercentage:
Math.round((formData.steamPumpPercentage || 0) * 10) or otherwise preserve
existing raw value), keeping the existing pid reconstruction logic that combines
formData.pid and kf; ensure you reference formData, steamPumpPercentage, pid,
kf, autowakeupSchedules and use downloadJson(finalExport, 'settings.json') as
before.
- Around line 329-333: The submit handler currently multiplies
steamPumpPercentage by 10 unconditionally (in onSubmit where payload.append(key,
Math.round(rawNum * 10))), but normalizeSettings only divides by 10 when
pressureAvailable.value is true; update onSubmit to mirror normalizeSettings by
applying the *10 conversion only when pressureAvailable is true (use the same
pressureAvailable symbol/prop used in normalizeSettings to guard the
multiplication before calling payload.append for steamPumpPercentage) so values
are only scaled when the pressure sensor is available.
- Around line 155-174: The code uses crypto.randomUUID() in parseSchedules and
other schedule creation sites which will throw on non-secure (HTTP) pages; add a
small fallback UUID generator at the top of the file (e.g., function uuid() {
try { return crypto.randomUUID(); } catch { return 'id-' +
Date.now().toString(36) + '-' + Math.random().toString(36).slice(2); } }) and
replace every crypto.randomUUID() call (including inside parseSchedules and the
other four schedule creation locations) with uuid() so schedule IDs are
generated safely on HTTP.
---
Nitpick comments:
In `@web/src/pages/Settings/index.jsx`:
- Around line 318-320: Extract the duplicated schedule serialization into a new
helper function (e.g., serializeSchedules) that accepts autowakeupSchedules and
returns the string currently built in onSubmit/onExport; replace the inline
logic in both onSubmit and onExport with calls to
serializeSchedules(autowakeupSchedules) so both locations use the single helper
and remove the duplicated mapping/join logic.
- Line 470: Remove the redundant native form attributes or ensure preventDefault
is always invoked first: either delete method='post' and action='/api/settings'
from the <form ref={formRef} key='settings'> element to avoid accidental
full-page POSTs, or update the onSubmit handler (the onSubmit function used by
the form) to call e.preventDefault() as the very first statement before the
submitting guard (the submitting state check) and any other logic; reference the
form element with ref formRef and the onSubmit function to apply the change.
- Around line 16-19: DEFAULT_WAKEUP_SCHEDULE's days array is currently shared
because it's created with Array(7).fill(true) and in the autoWakeupReducer INIT
case you spread DEFAULT_WAKEUP_SCHEDULE without copying days; modify the INIT
branch of autoWakeupReducer to deep-copy the days array (e.g. set days:
[...DEFAULT_WAKEUP_SCHEDULE.days] when constructing the initial state) so it
doesn't share the same reference (note ADD already uses
[...DEFAULT_WAKEUP_SCHEDULE.days]).
- Around line 438-448: The label element used as a clickable import control
(htmlFor='settingsImport', tabIndex='0', onKeyDown handler) is not announced as
interactive by screen readers; add role='button' to that label to expose it as a
button to assistive technologies while leaving the existing tabIndex and
onKeyDown intact so keyboard interaction continues to work.
| const parseSchedules = scheduleStr => { | ||
| if (typeof scheduleStr !== 'string' || !scheduleStr.trim()) { | ||
| return [{ ...DEFAULT_WAKEUP_SCHEDULE, id: crypto.randomUUID() }]; | ||
| } | ||
| const parsed = scheduleStr | ||
| .split(';') | ||
| .map(str => { | ||
| const [time, daysStr] = str.split('|'); | ||
| if (time && daysStr?.length === 7) { | ||
| return { | ||
| id: crypto.randomUUID(), // Generate a unique ID for each schedule for stable React rendering | ||
| time, | ||
| days: daysStr.split('').map(d => d === '1'), | ||
| }; | ||
| } | ||
| return null; | ||
| }) | ||
| .filter(Boolean); | ||
| return parsed.length > 0 ? parsed : [{ ...DEFAULT_WAKEUP_SCHEDULE, id: crypto.randomUUID() }]; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
crypto.randomUUID() secure context requirement browsers Firefox Chrome
💡 Result:
crypto.randomUUID() is gated behind secure contexts in both Chrome and Firefox: it’s only exposed when the page is in a secure context (typically HTTPS, and also http://localhost for local development). (developer.mozilla.org)
Practical implications:
- On non-secure pages (e.g.,
http://example.com),crypto.randomUUIDmay be missing/undefined, so calls will fail. (developer.mozilla.org) - Support (when in a secure context) is present in modern Chrome/Firefox (Chrome ≥ 92, Firefox ≥ 95). (caniuse.com)
- You can check the requirement at runtime with
globalThis.isSecureContext. (developer.mozilla.org)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID?utm_source=openai
- 3: https://caniuse.com/mdn-api_crypto_randomuuid?utm_source=openai
- 4: https://developer.mozilla.org/en-US/docs/Glossary/Secure_Context?utm_source=openai
🏁 Script executed:
# Check the actual code around the specified lines
cd web/src/pages/Settings
cat -n index.jsx | sed -n '155,195p'Repository: jniebuhr/gaggimate
Length of output: 1822
🏁 Script executed:
# Search for other UUID generation patterns to see if there are inconsistencies
rg -i "(randomuuid|uuid|nanoid|crypto\.random)" web/src/pages/Settings/index.jsxRepository: jniebuhr/gaggimate
Length of output: 436
🏁 Script executed:
# Also check if there are any conditional checks or helpers already for crypto availability
rg -B2 -A2 "crypto\." web/src/pages/Settings/index.jsx | head -30Repository: jniebuhr/gaggimate
Length of output: 992
crypto.randomUUID() requires a secure context and will fail on HTTP connections.
This embedded device UI is likely accessed via plain HTTP (e.g., http://192.168.x.x). crypto.randomUUID() is gated behind secure contexts in both Chrome and Firefox and will be unavailable on non-secure pages. All five call sites (lines 157, 165, 173, 183, 191) will throw, breaking schedule management entirely.
Implement a fallback:
Proposed UUID fallback utility
Add at the top of the file:
+const uuid = () =>
+ typeof crypto !== 'undefined' && typeof crypto.randomUUID === 'function'
+ ? crypto.randomUUID()
+ : `${Date.now()}-${Math.random().toString(36).slice(2, 11)}`;Replace all crypto.randomUUID() calls with uuid().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/Settings/index.jsx` around lines 155 - 174, The code uses
crypto.randomUUID() in parseSchedules and other schedule creation sites which
will throw on non-secure (HTTP) pages; add a small fallback UUID generator at
the top of the file (e.g., function uuid() { try { return crypto.randomUUID(); }
catch { return 'id-' + Date.now().toString(36) + '-' +
Math.random().toString(36).slice(2); } }) and replace every crypto.randomUUID()
call (including inside parseSchedules and the other four schedule creation
locations) with uuid() so schedule IDs are generated safely on HTTP.
| if (key === 'steamPumpPercentage') { | ||
| const rawNum = parseFloat(value); | ||
| payload.append(key, isNaN(rawNum) ? 0 : Math.round(rawNum * 10)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: steamPumpPercentage is unconditionally multiplied by 10 on submit, but only divided by 10 on load when pressureAvailable is true.
In normalizeSettings (line 130–132), the division by 10 is conditional on pressureAvailable.value. However, in onSubmit, the multiplication by 10 is unconditional. When the pressure sensor is not available, the value is never divided during load, but still gets multiplied by 10 on save — corrupting the value on every save cycle.
Proposed fix: make the submit conversion conditional
if (key === 'steamPumpPercentage') {
const rawNum = parseFloat(value);
- payload.append(key, isNaN(rawNum) ? 0 : Math.round(rawNum * 10));
+ payload.append(
+ key,
+ isNaN(rawNum) ? 0 : pressureAvailable.value ? Math.round(rawNum * 10) : rawNum,
+ );
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/Settings/index.jsx` around lines 329 - 333, The submit handler
currently multiplies steamPumpPercentage by 10 unconditionally (in onSubmit
where payload.append(key, Math.round(rawNum * 10))), but normalizeSettings only
divides by 10 when pressureAvailable.value is true; update onSubmit to mirror
normalizeSettings by applying the *10 conversion only when pressureAvailable is
true (use the same pressureAvailable symbol/prop used in normalizeSettings to
guard the multiplication before calling payload.append for steamPumpPercentage)
so values are only scaled when the pressure sensor is available.
| const onExport = useCallback(() => { | ||
| downloadJson(formData, 'settings.json'); | ||
| }, [formData]); | ||
| // Prepare the autowakeupSchedules in the format expected by the firmware for export | ||
| const scheduleString = autowakeupSchedules | ||
| .map(s => `${s.time}|${s.days.map(d => (d ? '1' : '0')).join('')}`) | ||
| .join(';'); | ||
|
|
||
| // Extract the PID and Kf values from formData to merge them back into the firmware format for export, while keeping the rest of the data intact | ||
| const { kf, ...baseData } = formData; | ||
|
|
||
| // Merge the PID and Kf values back into the firmware format for export, and include the autowakeupSchedules in the correct format as well | ||
| const finalExport = { | ||
| ...baseData, | ||
| pid: `${formData.pid},${kf || '0.000'}`, // Merge them back into the firmware format | ||
| autowakeupSchedules: scheduleString, | ||
| }; | ||
|
|
||
| // Use the utility function to trigger a download of the settings as a JSON file, with the filename 'settings.json' | ||
| downloadJson(finalExport, 'settings.json'); | ||
| }, [formData, autowakeupSchedules]); |
There was a problem hiding this comment.
Export/import round-trip corrupts steamPumpPercentage when pressure sensor is available.
onExport writes the display-format value from formData (already divided by 10 via normalizeSettings). onUpload calls normalizeSettings on the imported data, which divides by 10 again. For example: firmware value 15 → display 1.5 → exported as 1.5 → imported → normalizeSettings → 0.15.
The export should convert steamPumpPercentage back to the firmware-expected raw format before writing, similar to how pid is reconstructed.
Proposed fix in onExport
const { kf, ...baseData } = formData;
+ // Convert steamPumpPercentage back to firmware format for export
+ if (pressureAvailable.value && baseData.steamPumpPercentage != null) {
+ const rawNum = parseFloat(baseData.steamPumpPercentage);
+ baseData.steamPumpPercentage = isNaN(rawNum) ? 0 : Math.round(rawNum * 10);
+ }
+
const finalExport = {
...baseData,
pid: `${formData.pid},${kf || '0.000'}`,
autowakeupSchedules: scheduleString,
};📝 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 onExport = useCallback(() => { | |
| downloadJson(formData, 'settings.json'); | |
| }, [formData]); | |
| // Prepare the autowakeupSchedules in the format expected by the firmware for export | |
| const scheduleString = autowakeupSchedules | |
| .map(s => `${s.time}|${s.days.map(d => (d ? '1' : '0')).join('')}`) | |
| .join(';'); | |
| // Extract the PID and Kf values from formData to merge them back into the firmware format for export, while keeping the rest of the data intact | |
| const { kf, ...baseData } = formData; | |
| // Merge the PID and Kf values back into the firmware format for export, and include the autowakeupSchedules in the correct format as well | |
| const finalExport = { | |
| ...baseData, | |
| pid: `${formData.pid},${kf || '0.000'}`, // Merge them back into the firmware format | |
| autowakeupSchedules: scheduleString, | |
| }; | |
| // Use the utility function to trigger a download of the settings as a JSON file, with the filename 'settings.json' | |
| downloadJson(finalExport, 'settings.json'); | |
| }, [formData, autowakeupSchedules]); | |
| const onExport = useCallback(() => { | |
| // Prepare the autowakeupSchedules in the format expected by the firmware for export | |
| const scheduleString = autowakeupSchedules | |
| .map(s => `${s.time}|${s.days.map(d => (d ? '1' : '0')).join('')}`) | |
| .join(';'); | |
| // Extract the PID and Kf values from formData to merge them back into the firmware format for export, while keeping the rest of the data intact | |
| const { kf, ...baseData } = formData; | |
| // Convert steamPumpPercentage back to firmware format for export | |
| if (pressureAvailable.value && baseData.steamPumpPercentage != null) { | |
| const rawNum = parseFloat(baseData.steamPumpPercentage); | |
| baseData.steamPumpPercentage = isNaN(rawNum) ? 0 : Math.round(rawNum * 10); | |
| } | |
| // Merge the PID and Kf values back into the firmware format for export, and include the autowakeupSchedules in the correct format as well | |
| const finalExport = { | |
| ...baseData, | |
| pid: `${formData.pid},${kf || '0.000'}`, // Merge them back into the firmware format | |
| autowakeupSchedules: scheduleString, | |
| }; | |
| // Use the utility function to trigger a download of the settings as a JSON file, with the filename 'settings.json' | |
| downloadJson(finalExport, 'settings.json'); | |
| }, [formData, autowakeupSchedules]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/Settings/index.jsx` around lines 378 - 396, onExport currently
writes display-formatted values from formData (including steamPumpPercentage) so
a subsequent normalizeSettings on import divides it again; update onExport to
convert steamPumpPercentage back to the firmware raw value before merging into
finalExport (e.g. if a pressure sensor is present, set steamPumpPercentage:
Math.round((formData.steamPumpPercentage || 0) * 10) or otherwise preserve
existing raw value), keeping the existing pid reconstruction logic that combines
formData.pid and kf; ensure you reference formData, steamPumpPercentage, pid,
kf, autowakeupSchedules and use downloadJson(finalExport, 'settings.json') as
before.
1 participant
Please note:
Significant refactoring of settings/index.jsx and PluginCard.jsx. Please verify this code works. I did extensive testing, but my knowledge of backend stuff on this project is still limited.
Index.jsx
PluginCard.jsx
Added comments (generated with the help of Copilot) throughout to help future users navigate and/or understand the files and structure.
Summary by CodeRabbit
New Features
UI/UX Improvements
Bug Fixes